Enable resoure configuration via Kruize CR#67
Open
shreyabiradar07 wants to merge 12 commits intokruize:mvp_demofrom
Open
Enable resoure configuration via Kruize CR#67shreyabiradar07 wants to merge 12 commits intokruize:mvp_demofrom
shreyabiradar07 wants to merge 12 commits intokruize:mvp_demofrom
Conversation
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Reviewer's GuideEnables configuring CPU/memory and PV/PVC resources for Kruize and its database via new fields on the Kruize CRD, wires those values through the controller into the resource generator, centralizes defaults, and adds extensive tests for defaulting, overrides, and persistence config on both OpenShift and Kubernetes. Sequence diagram for applying Kruize CR resource configurationsequenceDiagram
actor Admin
participant KubeAPI as Kubernetes_API_Server
participant KruizeController as KruizeReconciler
participant Generator as KruizeResourceGenerator
participant K8sResources as Kubernetes_Resources
Admin->>KubeAPI: Apply Kruize_CR(spec.resources)
KubeAPI-->>KruizeController: Watch event for Kruize
KruizeController->>KruizeController: deployKruizeComponents(ctx, namespace, kruize)
KruizeController->>Generator: NewKruizeResourceGenerator(namespace, autotuneImage, autotuneUIImage, clusterType, kruize.Spec.Resources, ctx)
KruizeController->>Generator: kruizeDBDeployment()
Generator->>Generator: getDBResources()
alt Database_resources_configured
Generator->>Generator: getResourceValue(overrides, defaults)
Generator->>Generator: parseResourceQuantity(configured, default)
else No_DB_overrides
Generator->>Generator: use DefaultDB* constants
end
Generator-->>KruizeController: Deployment with DB resources
KruizeController->>Generator: kruizeDeployment()
Generator->>Generator: getKruizeResources()
alt Kruize_resources_configured
Generator->>Generator: getResourceValue(overrides, defaults)
Generator->>Generator: parseResourceQuantity(configured, default)
else No_Kruize_overrides
Generator->>Generator: use DefaultKruize* constants
end
Generator-->>KruizeController: Deployment with Kruize resources
KruizeController->>Generator: kruizeDBPersistentVolume()/Claim()
alt PersistentVolume_configured
Generator->>Generator: getPVConfigWithDefaults(overrides)
Generator->>Generator: parseResourceQuantity(pvSize, defaults)
else No_PV_overrides
Generator->>Generator: getPVConfigWithDefaults(defaults)
end
Generator-->>KruizeController: PV and PVC specs
KruizeController->>KubeAPI: Create/Update Deployments, PV, PVC
KubeAPI-->>K8sResources: Persist resources
Class diagram for new Kruize resource configuration typesclassDiagram
class KruizeSpec {
+string namespace
+string autotune_image
+string autotune_ui_image
+ResourceConfig resources
}
class ResourceConfig {
+ContainerResources database
+ContainerResources kruize
+PersistentVolumeConfig persistentVolume
}
class ContainerResources {
+string cpuRequest
+string cpuLimit
+string memoryRequest
+string memoryLimit
}
class PersistentVolumeConfig {
+string pvStorageSize
+string pvcStorageSize
+string storageClassName
+string hostPath
+PersistentVolumeAccessMode[] accessModes
}
class PersistentVolumeAccessMode {
<<enum>>
ReadWriteOnce
ReadOnlyMany
ReadWriteMany
ReadWriteOncePod
}
KruizeSpec --> ResourceConfig : owns
ResourceConfig --> ContainerResources : database
ResourceConfig --> ContainerResources : kruize
ResourceConfig --> PersistentVolumeConfig : persistentVolume
PersistentVolumeConfig --> PersistentVolumeAccessMode : uses
Class diagram for updated KruizeResourceGenerator and defaultsclassDiagram
class KruizeResourceGenerator {
+string Namespace
+string Autotune_image
+string Autotune_ui_image
+string ClusterType
+ResourceConfig ResourceConfig
+context_Ctx Ctx
+NewKruizeResourceGenerator(namespace autotuneImage autotuneUIImage clusterType resourceConfig ctx) KruizeResourceGenerator
+ClusterScopedResources() []client_Object
+ManualStorageClass() *storagev1_StorageClass
+kruizeDBPersistentVolume() *corev1_PersistentVolume
+kruizeDBPersistentVolumeClaim() *corev1_PersistentVolumeClaim
+kruizeDBDeployment() *appsv1_Deployment
+kruizeDeployment() *appsv1_Deployment
+kruizeDBPersistentVolumeKubernetes() *corev1_PersistentVolume
+kruizeDBPersistentVolumeClaimKubernetes() *corev1_PersistentVolumeClaim
+kruizeDBDeploymentKubernetes() *appsv1_Deployment
+kruizeDeploymentKubernetes() *appsv1_Deployment
-getResourceValue(configValue defaultValue string) string
-parseResourceQuantity(value defaultValue string) resource_Quantity
-getDBResources() corev1_ResourceRequirements
-getKruizeResources() corev1_ResourceRequirements
-isValidAccessMode(mode string) bool
-getPVConfigWithDefaults(defaultPVStorageSize defaultStorageClassName defaultHostPath string, defaultAccessModes []corev1_PersistentVolumeAccessMode) (pvStorageSize pvcStorageSize storageClassName hostPath string, accessModes []corev1_PersistentVolumeAccessMode)
-getPVConfig() (pvStorageSize pvcStorageSize storageClassName hostPath string, accessModes []corev1_PersistentVolumeAccessMode)
-getPVConfigKubernetes() (pvStorageSize pvcStorageSize storageClassName hostPath string, accessModes []corev1_PersistentVolumeAccessMode)
}
class ResourceDefaultsConstants {
<<static>>
+string DefaultDBCPURequest
+string DefaultDBCPULimit
+string DefaultDBMemoryRequest
+string DefaultDBMemoryLimit
+string DefaultKruizeCPURequest
+string DefaultKruizeCPULimit
+string DefaultKruizeMemoryRequest
+string DefaultKruizeMemoryLimit
+string DefaultOpenShiftPVStorageSize
+string DefaultOpenShiftStorageClassName
+string DefaultOpenShiftHostPath
+string DefaultKubernetesPVStorageSize
+string DefaultKubernetesStorageClassName
+string DefaultKubernetesHostPath
}
class ResourceConfig {
}
KruizeResourceGenerator --> ResourceConfig : reads
KruizeResourceGenerator ..> ResourceDefaultsConstants : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 7 issues, and left some high level feedback:
- The new helpers rely on resource.MustParse for user-supplied values; consider using resource.ParseQuantity with validation/error handling so an invalid CR value doesn’t cause a controller panic.
- getPVConfig and getPVConfigKubernetes duplicate most of their logic with only different defaults and storage class handling; extracting a shared helper with injected defaults would simplify maintenance.
- When converting PersistentVolumeConfig.AccessModes from []string to []corev1.PersistentVolumeAccessMode, you currently trust the input; adding validation (or constraining the CRD schema to allowed enum values) would help avoid invalid modes slipping through.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new helpers rely on resource.MustParse for user-supplied values; consider using resource.ParseQuantity with validation/error handling so an invalid CR value doesn’t cause a controller panic.
- getPVConfig and getPVConfigKubernetes duplicate most of their logic with only different defaults and storage class handling; extracting a shared helper with injected defaults would simplify maintenance.
- When converting PersistentVolumeConfig.AccessModes from []string to []corev1.PersistentVolumeAccessMode, you currently trust the input; adding validation (or constraining the CRD schema to allowed enum values) would help avoid invalid modes slipping through.
## Individual Comments
### Comment 1
<location> `internal/utils/kruize_generator.go:72-81` </location>
<code_context>
+}
+
+// getDBResources returns database resource requirements with defaults
+func (g *KruizeResourceGenerator) getDBResources() corev1.ResourceRequirements {
+ cpuRequest := constants.DefaultDBCPURequest
+ cpuLimit := constants.DefaultDBCPULimit
+ memoryRequest := constants.DefaultDBMemoryRequest
+ memoryLimit := constants.DefaultDBMemoryLimit
+
+ if g.ResourceConfig != nil && g.ResourceConfig.Database != nil {
+ cpuRequest = g.getResourceValue(g.ResourceConfig.Database.CPURequest, cpuRequest)
+ cpuLimit = g.getResourceValue(g.ResourceConfig.Database.CPULimit, cpuLimit)
+ memoryRequest = g.getResourceValue(g.ResourceConfig.Database.MemoryRequest, memoryRequest)
+ memoryLimit = g.getResourceValue(g.ResourceConfig.Database.MemoryLimit, memoryLimit)
+ }
+
+ return corev1.ResourceRequirements{
+ Requests: corev1.ResourceList{
+ corev1.ResourceMemory: resource.MustParse(memoryRequest),
</code_context>
<issue_to_address>
**issue (bug_risk):** Using resource.MustParse on user-provided values can panic and crash the operator.
These values were previously hardcoded and safe, but now come from the CR and may be invalid. Since `resource.MustParse` panics on bad input, a malformed CPU/Memory value can crash the controller. Please switch to `resource.ParseQuantity` and handle errors (e.g., defaulting or setting a Status condition) to avoid panics from user misconfiguration.
</issue_to_address>
### Comment 2
<location> `internal/utils/kruize_generator.go:98-85` </location>
<code_context>
+}
+
+// getKruizeResources returns Kruize application resource requirements with defaults
+func (g *KruizeResourceGenerator) getKruizeResources() corev1.ResourceRequirements {
+ cpuRequest := constants.DefaultKruizeCPURequest
+ cpuLimit := constants.DefaultKruizeCPULimit
+ memoryRequest := constants.DefaultKruizeMemoryRequest
+ memoryLimit := constants.DefaultKruizeMemoryLimit
+
+ if g.ResourceConfig != nil && g.ResourceConfig.Kruize != nil {
+ cpuRequest = g.getResourceValue(g.ResourceConfig.Kruize.CPURequest, cpuRequest)
+ cpuLimit = g.getResourceValue(g.ResourceConfig.Kruize.CPULimit, cpuLimit)
+ memoryRequest = g.getResourceValue(g.ResourceConfig.Kruize.MemoryRequest, memoryRequest)
+ memoryLimit = g.getResourceValue(g.ResourceConfig.Kruize.MemoryLimit, memoryLimit)
+ }
+
+ return corev1.ResourceRequirements{
+ Requests: corev1.ResourceList{
+ corev1.ResourceMemory: resource.MustParse(memoryRequest),
</code_context>
<issue_to_address>
**issue (bug_risk):** Same panic risk in Kruize container resources due to resource.MustParse on CR input.
Since these values now come directly from the CR, invalid cpu/memory strings will cause `resource.MustParse` to panic and crash the reconciler. Consider parsing with error handling (and defaulting or surfacing a configuration error) instead of using `MustParse` here.
</issue_to_address>
### Comment 3
<location> `internal/utils/kruize_generator.go:155-164` </location>
<code_context>
+func (g *KruizeResourceGenerator) getPVConfigKubernetes() (pvStorageSize, pvcStorageSize, hostPath string, accessModes []corev1.PersistentVolumeAccessMode) {
</code_context>
<issue_to_address>
**issue (bug_risk):** PersistentVolumeConfig.StorageClassName is ignored for the Kubernetes/Kind/Minikube PV/PVC path.
This creates a silent no-op for users on non-OpenShift clusters who set storageClassName in the CR. Please either propagate StorageClassName through getPVConfigKubernetes into the PV/PVC specs, or, if Kubernetes is not meant to honor it, adjust the CRD/interface to make that clear (e.g., omit or scope the field) to avoid confusion.
</issue_to_address>
### Comment 4
<location> `internal/controller/kruize_controller_test.go:300` </location>
<code_context>
Context("Resource generation", func() {
It("should generate cluster-scoped resources for OpenShift", func() {
- generator := utils.NewKruizeResourceGenerator("test-namespace", "", "", constants.ClusterTypeOpenShift)
+ generator := utils.NewKruizeResourceGenerator("test-namespace", "", "", constants.ClusterTypeOpenShift, nil)
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests that validate ResourceConfig-driven overrides (CPU/memory) are applied to the generated deployments
Right now the tests only cover the `nil`/default `ResourceConfig` case. Please also add tests that construct a `Kruize` with non-nil `Spec.Resources.Database` and `Spec.Resources.Kruize`, pass it into `NewKruizeResourceGenerator`, and assert that the resulting DB and Kruize deployments have container `Resources.Requests`/`Limits` matching those configured CPU/memory values. A couple of extra `It` blocks or a small `DescribeTable` contrasting default vs custom resources would be sufficient.
Suggested implementation:
```golang
Context("Resource generation", func() {
It("should generate cluster-scoped resources for OpenShift", func() {
generator := utils.NewKruizeResourceGenerator("test-namespace", "", "", constants.ClusterTypeOpenShift, nil)
clusterResources := generator.ClusterScopedResources()
Expect(clusterResources).NotTo(BeEmpty())
})
It("should generate namespaced resources for OpenShift", func() {
generator := utils.NewKruizeResourceGenerator("test-namespace", "", "", constants.ClusterTypeOpenShift, nil)
namespacedResources := generator.NamespacedResources()
})
It("should apply custom ResourceConfig to Kruize and Database deployments", func() {
// NOTE: this test assumes the Kruize API types and deployment names already exist in the project.
// Adjust the package alias and names if they differ in your codebase.
cpuRequest := resource.MustParse("200m")
cpuLimit := resource.MustParse("500m")
memRequest := resource.MustParse("256Mi")
memLimit := resource.MustParse("512Mi")
kruize := &kruizev1alpha1.Kruize{
ObjectMeta: metav1.ObjectMeta{
Name: "test-kruize",
Namespace: "test-namespace",
},
Spec: kruizev1alpha1.KruizeSpec{
Resources: &kruizev1alpha1.ResourceConfig{
Database: &kruizev1alpha1.ContainerResources{
Requests: kruizev1alpha1.ResourceList{
CPU: cpuRequest.String(),
Memory: memRequest.String(),
},
Limits: kruizev1alpha1.ResourceList{
CPU: cpuLimit.String(),
Memory: memLimit.String(),
},
},
Kruize: &kruizev1alpha1.ContainerResources{
Requests: kruizev1alpha1.ResourceList{
CPU: cpuRequest.String(),
Memory: memRequest.String(),
},
Limits: kruizev1alpha1.ResourceList{
CPU: cpuLimit.String(),
Memory: memLimit.String(),
},
},
},
},
}
generator := utils.NewKruizeResourceGenerator("test-namespace", "", "", constants.ClusterTypeOpenShift, kruize)
namespacedResources := generator.NamespacedResources()
Expect(namespacedResources).NotTo(BeEmpty())
var kruizeDeployment *appsv1.Deployment
var dbDeployment *appsv1.Deployment
for _, obj := range namespacedResources {
deploy, ok := obj.(*appsv1.Deployment)
if !ok {
continue
}
switch deploy.Name {
case "kruize":
kruizeDeployment = deploy
case "kruize-db":
dbDeployment = deploy
}
}
Expect(kruizeDeployment).NotTo(BeNil(), "expected Kruize deployment to be generated")
Expect(dbDeployment).NotTo(BeNil(), "expected Database deployment to be generated")
// Assuming single container per deployment; adjust index/name if needed.
kruizeContainer := kruizeDeployment.Spec.Template.Spec.Containers[0]
dbContainer := dbDeployment.Spec.Template.Spec.Containers[0]
Expect(kruizeContainer.Resources.Requests.Cpu()).To(Equal(&cpuRequest))
Expect(kruizeContainer.Resources.Limits.Cpu()).To(Equal(&cpuLimit))
Expect(kruizeContainer.Resources.Requests.Memory()).To(Equal(&memRequest))
Expect(kruizeContainer.Resources.Limits.Memory()).To(Equal(&memLimit))
Expect(dbContainer.Resources.Requests.Cpu()).To(Equal(&cpuRequest))
Expect(dbContainer.Resources.Limits.Cpu()).To(Equal(&cpuLimit))
Expect(dbContainer.Resources.Requests.Memory()).To(Equal(&memRequest))
Expect(dbContainer.Resources.Limits.Memory()).To(Equal(&memLimit))
```
To make this compile and work correctly, you’ll likely need to:
1. **Imports**: Ensure the test file imports the necessary packages (or adjust aliases to match your project):
- `kruizev1alpha1` – your Kruize API package, e.g.:
```go
kruizev1alpha1 "github.com/<org>/<repo>/api/v1alpha1"
```
- Kubernetes types:
```go
appsv1 "k8s.io/api/apps/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
```
2. **Kruize types**: Adjust the `KruizeSpec`, `ResourceConfig`, `ContainerResources`, and `ResourceList` field names to match the actual CRD definitions in your code (for example, you may use `KruizeResources`, `DatabaseResources`, or plain `corev1.ResourceRequirements` instead of the placeholder types in this snippet).
3. **Deployment names**: Update `"kruize"` and `"kruize-db"` in the `switch deploy.Name` block to the actual deployment names (or use labels/owner references) that your `NewKruizeResourceGenerator` produces.
4. **Containers indexing**: If your deployments have more than one container, select the container by name rather than using index `[0]`, e.g. find by `container.Name == "kruize"` or `"kruize-db"`.
5. **Resource parsing**: If your `Kruize` CRD stores resource quantities differently (e.g. as `corev1.ResourceRequirements` instead of string maps), replace the construction of `kruize.Spec.Resources.*` accordingly and adapt the expectations to compare the appropriate types.
</issue_to_address>
### Comment 5
<location> `internal/controller/kruize_controller_test.go:316` </location>
<code_context>
})
It("should generate Kubernetes cluster-scoped resources", func() {
- generator := utils.NewKruizeResourceGenerator("test-namespace", "", "", constants.ClusterTypeMinikube)
+ generator := utils.NewKruizeResourceGenerator("test-namespace", "", "", constants.ClusterTypeMinikube, nil)
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests that cover PersistentVolume/PVC sizing and hostPath behavior from ResourceConfig for both OpenShift and Kubernetes
The new `getPVConfig`/`getPVConfigKubernetes` logic changes PV/PVC sizes, `storageClassName`, `hostPath`, and `accessModes` based on `ResourceConfig.PersistentVolume`, but current tests don’t assert these behaviors.
For both OpenShift and Kubernetes, please add tests that:
- Use a non-nil `ResourceConfig.PersistentVolume` with custom `PVStorageSize`, `PVCStorageSize`, `StorageClassName`, `HostPath`, and `AccessModes`.
- Verify the generated PV and PVC objects use these values.
- Include a case where `PVCStorageSize` is omitted to confirm it falls back to `PVStorageSize`.
This will exercise the new storage configuration end-to-end.
Suggested implementation:
```golang
})
It("should generate namespaced resources for OpenShift", func() {
generator := utils.NewKruizeResourceGenerator("test-namespace", "", "", constants.ClusterTypeOpenShift, nil)
namespacedResources := generator.NamespacedResources()
Expect(namespacedResources).NotTo(BeEmpty())
})
It("should honor PersistentVolume config for OpenShift", func() {
resourceConfig := &v1alpha1.ResourceConfig{
PersistentVolume: &v1alpha1.PersistentVolumeConfig{
PVStorageSize: "10Gi",
PVCStorageSize: "5Gi",
StorageClassName: "fast",
HostPath: "/data/kruize",
AccessModes: []corev1.PersistentVolumeAccessMode{
corev1.ReadWriteOnce,
corev1.ReadOnlyMany,
},
},
}
generator := utils.NewKruizeResourceGenerator("test-namespace", "", "", constants.ClusterTypeOpenShift, resourceConfig)
namespacedResources := generator.NamespacedResources()
Expect(namespacedResources).NotTo(BeEmpty())
var pv *corev1.PersistentVolume
var pvc *corev1.PersistentVolumeClaim
for _, r := range namespacedResources {
switch o := r.(type) {
case *corev1.PersistentVolume:
pv = o
case *corev1.PersistentVolumeClaim:
pvc = o
}
}
Expect(pv).NotTo(BeNil())
Expect(pvc).NotTo(BeNil())
Expect(pv.Spec.Capacity[corev1.ResourceStorage]).To(Equal(resource.MustParse("10Gi")))
Expect(pv.Spec.PersistentVolumeSource.HostPath).NotTo(BeNil())
Expect(pv.Spec.PersistentVolumeSource.HostPath.Path).To(Equal("/data/kruize"))
Expect(pv.Spec.AccessModes).To(ContainElement(corev1.ReadWriteOnce))
Expect(pv.Spec.AccessModes).To(ContainElement(corev1.ReadOnlyMany))
if pv.Spec.StorageClassName != "" {
Expect(pv.Spec.StorageClassName).To(Equal("fast"))
} else if pv.Spec.StorageClassNamePtr != nil {
Expect(*pv.Spec.StorageClassNamePtr).To(Equal("fast"))
}
Expect(pvc.Spec.Resources.Requests[corev1.ResourceStorage]).To(Equal(resource.MustParse("5Gi")))
if pvc.Spec.StorageClassName != nil {
Expect(*pvc.Spec.StorageClassName).To(Equal("fast"))
}
Expect(pvc.Spec.AccessModes).To(ContainElement(corev1.ReadWriteOnce))
Expect(pvc.Spec.AccessModes).To(ContainElement(corev1.ReadOnlyMany))
})
It("should generate Kubernetes cluster-scoped resources", func() {
generator := utils.NewKruizeResourceGenerator("test-namespace", "", "", constants.ClusterTypeMinikube, nil)
clusterResources := generator.KubernetesClusterScopedResources()
Expect(clusterResources).NotTo(BeEmpty())
})
It("should generate Kubernetes namespaced resources", func() {
generator := utils.NewKruizeResourceGenerator("test-namespace", "", "", constants.ClusterTypeMinikube, nil)
namespacedResources := generator.KubernetesNamespacedResources()
Expect(namespacedResources).NotTo(BeEmpty())
})
It("should honor PersistentVolume config for Kubernetes and default PVC size", func() {
resourceConfig := &v1alpha1.ResourceConfig{
PersistentVolume: &v1alpha1.PersistentVolumeConfig{
PVStorageSize: "15Gi",
StorageClassName: "standard",
HostPath: "/var/lib/kruize",
AccessModes: []corev1.PersistentVolumeAccessMode{
corev1.ReadWriteOnce,
},
},
}
generator := utils.NewKruizeResourceGenerator("test-namespace", "", "", constants.ClusterTypeMinikube, resourceConfig)
clusterResources := generator.KubernetesClusterScopedResources()
namespacedResources := generator.KubernetesNamespacedResources()
var pv *corev1.PersistentVolume
var pvc *corev1.PersistentVolumeClaim
for _, r := range clusterResources {
if o, ok := r.(*corev1.PersistentVolume); ok {
pv = o
}
}
for _, r := range namespacedResources {
if o, ok := r.(*corev1.PersistentVolumeClaim); ok {
pvc = o
}
}
Expect(pv).NotTo(BeNil())
Expect(pvc).NotTo(BeNil())
Expect(pv.Spec.Capacity[corev1.ResourceStorage]).To(Equal(resource.MustParse("15Gi")))
Expect(pv.Spec.PersistentVolumeSource.HostPath).NotTo(BeNil())
Expect(pv.Spec.PersistentVolumeSource.HostPath.Path).To(Equal("/var/lib/kruize"))
Expect(pv.Spec.AccessModes).To(ContainElement(corev1.ReadWriteOnce))
if pv.Spec.StorageClassName != "" {
Expect(pv.Spec.StorageClassName).To(Equal("standard"))
} else if pv.Spec.StorageClassNamePtr != nil {
Expect(*pv.Spec.StorageClassNamePtr).To(Equal("standard"))
}
// PVCStorageSize is omitted, so it should fall back to PVStorageSize (15Gi)
Expect(pvc.Spec.Resources.Requests[corev1.ResourceStorage]).To(Equal(resource.MustParse("15Gi")))
if pvc.Spec.StorageClassName != nil {
Expect(*pvc.Spec.StorageClassName).To(Equal("standard"))
}
Expect(pvc.Spec.AccessModes).To(ContainElement(corev1.ReadWriteOnce))
```
1. Ensure the following imports exist at the top of `internal/controller/kruize_controller_test.go` (adjust package paths and aliases to match your project):
- `v1alpha1` for `ResourceConfig` / `PersistentVolumeConfig` (for example: `optapiv1alpha1 "github.com/kruize/optimizer/api/v1alpha1"` and then use `optapiv1alpha1.ResourceConfig` in the tests, updating the code above accordingly).
- `corev1 "k8s.io/api/core/v1"`
- `resource "k8s.io/apimachinery/pkg/api/resource"`
2. The fields used on the PV (`StorageClassName` vs a pointer field) may differ in your actual types. Replace the conditional handling of `StorageClassName` / `StorageClassNamePtr` with whatever your `PersistentVolume` and `PersistentVolumeClaim` types actually expose (typically `*string StorageClassName` in `corev1`).
3. If `NewKruizeResourceGenerator` returns a different type for the resource slices (e.g. `[]runtime.Object` or `[]client.Object`), the type assertions (`r.(*corev1.PersistentVolume)`) are still valid, but you may need to change the loop variable type accordingly.
4. If your `PersistentVolumeConfig` or `ResourceConfig` type names or field names differ from `PVStorageSize`, `PVCStorageSize`, `StorageClassName`, `HostPath`, and `AccessModes`, rename them in the tests to match the actual struct definitions.
5. If PVs for OpenShift are created as cluster-scoped rather than namespaced in your generator, adjust the OpenShift test to inspect the appropriate resource slice (e.g. `ClusterScopedResources()` instead of `NamespacedResources()`).
</issue_to_address>
### Comment 6
<location> `internal/controller/kruize_controller_test.go:438` </location>
<code_context>
Context("Kruize deployment manifest generation", func() {
DescribeTable("should generate valid Kruize deployment manifest",
func(clusterType string, resourceMethod func(*utils.KruizeResourceGenerator) []client.Object) {
- generator := utils.NewKruizeResourceGenerator("test-namespace", "", "", clusterType)
</code_context>
<issue_to_address>
**suggestion (testing):** Extend deployment manifest tests to assert resource Requests/Limits for both default and custom ResourceConfig
The current table only verifies that a `Deployment` is created, but not that its container `Resources` reflect the new configurability.
Please:
- For each `clusterType`, assert the Kruize deployment container’s `Resources` match expected defaults when `Resources == nil`.
- Add a case (either another `Entry` or `DescribeTable`) that uses a non-nil `ResourceConfig.Kruize` and asserts the deployment uses those custom CPU/memory values.
This will validate that manifest generation actually applies the new resource configuration.
</issue_to_address>
### Comment 7
<location> `internal/controller/kruize_controller_test.go:511` </location>
<code_context>
Expect(kruizeUIPod.Spec.Containers).NotTo(BeEmpty())
})
It("should generate Kruize-db pod specification", func() {
- generator := utils.NewKruizeResourceGenerator("test-namespace", "", "", constants.ClusterTypeOpenShift)
+ generator := utils.NewKruizeResourceGenerator("test-namespace", "", "", constants.ClusterTypeOpenShift, nil)
</code_context>
<issue_to_address>
**suggestion (testing):** Add an edge-case test for partial ResourceConfig (e.g., only CPURequest set) to ensure defaulting logic works as intended
Because `getDBResources` / `getKruizeResources` only override non-empty fields, it’s important to confirm that a partially populated `ResourceConfig.Database` preserves defaults for unset values. Please add a test that uses a config with only `CPURequest` (or a single field) set and verifies that the resulting DB container has that custom value while still using defaults for CPU limit and memory request/limit, ensuring partial override semantics remain intact across refactors.
Suggested implementation:
```golang
It("should generate namespaced resources for OpenShift", func() {
generator := utils.NewKruizeResourceGenerator("test-namespace", "", "", constants.ClusterTypeOpenShift, nil)
namespacedResources := generator.NamespacedResources()
})
It("should allow partial DB resource overrides while preserving defaults", func() {
// First, generate resources with default configuration to capture default DB resources
defaultGenerator := utils.NewKruizeResourceGenerator("test-namespace", "", "", constants.ClusterTypeOpenShift, nil)
defaultNamespacedResources := defaultGenerator.NamespacedResources()
// Helper closure to locate the Kruize DB container from the generated resources
getDBContainer := func(resources []client.Object) *corev1.Container {
for _, res := range resources {
deploy, ok := res.(*appsv1.Deployment)
if !ok {
continue
}
// Adjust this name if the DB deployment is named differently in the manifests
if deploy.Name != "kruize-db" {
continue
}
for i, c := range deploy.Spec.Template.Spec.Containers {
// Adjust this name if the DB container is named differently
if c.Name == "kruize-db" {
return &deploy.Spec.Template.Spec.Containers[i]
}
}
}
return nil
}
defaultDBContainer := getDBContainer(defaultNamespacedResources)
Expect(defaultDBContainer).NotTo(BeNil(), "default DB container should exist")
// Now, create a ResourceConfig that only overrides the CPU request for the DB
customConfig := &config.ResourceConfig{
Database: config.ContainerResourceConfig{
CPURequest: "250m",
},
}
customGenerator := utils.NewKruizeResourceGenerator("test-namespace", "", "", constants.ClusterTypeOpenShift, customConfig)
customNamespacedResources := customGenerator.NamespacedResources()
customDBContainer := getDBContainer(customNamespacedResources)
Expect(customDBContainer).NotTo(BeNil(), "custom DB container should exist")
// Verify that the CPU request was overridden
Expect(customDBContainer.Resources.Requests[corev1.ResourceCPU]).To(Equal(resource.MustParse("250m")))
// Verify that CPU limit and memory request/limit are still using the defaults
Expect(customDBContainer.Resources.Limits[corev1.ResourceCPU]).To(Equal(defaultDBContainer.Resources.Limits[corev1.ResourceCPU]))
Expect(customDBContainer.Resources.Requests[corev1.ResourceMemory]).To(Equal(defaultDBContainer.Resources.Requests[corev1.ResourceMemory]))
Expect(customDBContainer.Resources.Limits[corev1.ResourceMemory]).To(Equal(defaultDBContainer.Resources.Limits[corev1.ResourceMemory]))
```
1. Ensure the following imports exist at the top of `internal/controller/kruize_controller_test.go` (adjust module paths if your project uses different ones):
- `appsv1 "k8s.io/api/apps/v1"`
- `corev1 "k8s.io/api/core/v1"`
- `resource "k8s.io/apimachinery/pkg/api/resource"`
- `config "github.com/kruize/kruize-operator/internal/config"` (or the actual path where `ResourceConfig` and `ContainerResourceConfig` are defined).
- `client "sigs.k8s.io/controller-runtime/pkg/client"` if not already imported.
2. Update `"kruize-db"` (deployment name) and `"kruize-db"` (container name) in the `getDBContainer` closure if the actual DB deployment/container names differ in your manifests.
3. If `ResourceConfig` / `ContainerResourceConfig` live under a different package or have different field names, adjust the construction of `customConfig` accordingly to point to the DB-specific CPU request field.
4. If your tests already define a shared helper for locating the DB deployment/container, you can replace the inline `getDBContainer` closure with that existing helper to avoid duplication.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new resource/PV configuration tests repeat a lot of boilerplate for locating deployments, containers, PVs, and PVCs; consider extracting small helper functions (e.g.,
findDeployment,findPV/PVC,getContainer) to reduce duplication and make the tests easier to maintain. - Both
parseResourceQuantityandisValidAccessModesilently ignore invalid user input and fall back to defaults; it may be worth at least logging or surfacing a warning when a user-specified resource quantity or access mode is rejected so misconfigurations are observable. - The
getPVConfigKuberneteshelper reusesDefaultOpenShiftStorageClassNamefor Kubernetes; if the intent is to have different defaults per platform, consider introducing a dedicated Kubernetes storage class default constant for clarity and future flexibility.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new resource/PV configuration tests repeat a lot of boilerplate for locating deployments, containers, PVs, and PVCs; consider extracting small helper functions (e.g., `findDeployment`, `findPV/PVC`, `getContainer`) to reduce duplication and make the tests easier to maintain.
- Both `parseResourceQuantity` and `isValidAccessMode` silently ignore invalid user input and fall back to defaults; it may be worth at least logging or surfacing a warning when a user-specified resource quantity or access mode is rejected so misconfigurations are observable.
- The `getPVConfigKubernetes` helper reuses `DefaultOpenShiftStorageClassName` for Kubernetes; if the intent is to have different defaults per platform, consider introducing a dedicated Kubernetes storage class default constant for clarity and future flexibility.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In the CRD schema for
resources.persistentVolume.accessModes, consider adding an explicitenumfor the four supported values instead of only documenting them in the description so that invalid values are rejected at admission time rather than only logged and ignored. - There is still a fair amount of repeated code in the tests for locating Deployments/PVs/PVCs and extracting containers; since you’ve already introduced helpers like
findDeploymentandgetContainer, you could refactor the remaining manual loops to use these helpers to keep the tests more concise and less error-prone.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the CRD schema for `resources.persistentVolume.accessModes`, consider adding an explicit `enum` for the four supported values instead of only documenting them in the description so that invalid values are rejected at admission time rather than only logged and ignored.
- There is still a fair amount of repeated code in the tests for locating Deployments/PVs/PVCs and extracting containers; since you’ve already introduced helpers like `findDeployment` and `getContainer`, you could refactor the remaining manual loops to use these helpers to keep the tests more concise and less error-prone.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
isValidAccessModehelper inkruize_generator.gois currently unused; consider either wiring it into the PV/PVC access mode handling or removing it to avoid dead code. - In
ResourceConfig.DeepCopyInto,DatabaseandKruizeare copied with**out = **ininstead of callingContainerResources.DeepCopyInto; while safe today, switching to use the generated deepcopy would be more robust ifContainerResourcesever gains slice/map fields.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `isValidAccessMode` helper in `kruize_generator.go` is currently unused; consider either wiring it into the PV/PVC access mode handling or removing it to avoid dead code.
- In `ResourceConfig.DeepCopyInto`, `Database` and `Kruize` are copied with `**out = **in` instead of calling `ContainerResources.DeepCopyInto`; while safe today, switching to use the generated deepcopy would be more robust if `ContainerResources` ever gains slice/map fields.
## Individual Comments
### Comment 1
<location path="internal/utils/kruize_generator.go" line_range="147-148" />
<code_context>
+ }
+}
+
+// isValidAccessMode validates if the given access mode is a valid Kubernetes PersistentVolume access mode
+func (g *KruizeResourceGenerator) isValidAccessMode(mode string) bool {
+ validModes := map[string]bool{
+ string(corev1.ReadWriteOnce): true,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Validate or remove `isValidAccessMode` to avoid dead code and misleading log message
This helper isn’t used, so invalid values in `ResourceConfig.PersistentVolume.AccessModes` (e.g. programmatically constructed ones that bypass the CRD) still reach the PV/PVC spec. The log message also incorrectly says such modes “will be ignored”. Please either integrate this into `getPVConfigWithDefaults` (filter and drop/log invalid modes) or remove it to avoid dead code and misleading behavior.
Suggested implementation:
```golang
```
1. If there are any remaining references to `isValidAccessMode` elsewhere in `internal/utils/kruize_generator.go` (or the wider codebase), they must be removed or replaced, since this helper is now deleted.
2. If you later decide you want to actually validate and filter access modes (as per the alternative in the review comment), you can reintroduce a similar helper and wire it into `getPVConfigWithDefaults` where `PersistentVolume.AccessModes` are being processed, applying the filter before constructing the PV/PVC spec and logging dropped values there.
</issue_to_address>
### Comment 2
<location path="internal/utils/kruize_generator.go" line_range="171-180" />
<code_context>
+func (g *KruizeResourceGenerator) getPVConfigWithDefaults(
</code_context>
<issue_to_address>
**suggestion:** Consider enforcing or logging PV/PVC size consistency to surface misconfigurations earlier
The docstring states `pvStorageSize` should be >= `pvcStorageSize`, but this function doesn’t enforce that. If a PVC is configured larger than the PV, the error only surfaces at bind time. Consider parsing both quantities here and, when PVC > PV, logging a warning and either clamping to PV or explicitly preserving the user value, so misconfigurations are detected earlier.
Suggested implementation:
```golang
func ensurePVAndPVCStorageConsistency(pvStorageSize, pvcStorageSize string) (string, string, error) {
pvQty, err := resource.ParseQuantity(pvStorageSize)
if err != nil {
return pvStorageSize, pvcStorageSize, fmt.Errorf("failed to parse PV storage size %q: %w", pvStorageSize, err)
}
pvcQty, err := resource.ParseQuantity(pvcStorageSize)
if err != nil {
return pvStorageSize, pvcStorageSize, fmt.Errorf("failed to parse PVC storage size %q: %w", pvcStorageSize, err)
}
// If PVC > PV, surface the potential bind-time failure as early as possible.
if pvcQty.Cmp(pvQty) == 1 {
klog.Warningf("PVC storage request (%s) is greater than PV storage (%s); PVC may fail to bind", pvcStorageSize, pvStorageSize)
}
// We explicitly preserve the user-provided values here and only log a warning.
return pvStorageSize, pvcStorageSize, nil
}
// getPVConfigWithDefaults is a shared helper that returns PV configuration with injected defaults
func (g *KruizeResourceGenerator) getPVConfigWithDefaults(
defaultPVStorageSize, defaultStorageClassName, defaultHostPath string,
defaultAccessModes []corev1.PersistentVolumeAccessMode,
) (pvStorageSize, pvcStorageSize, storageClassName, hostPath string, accessModes []corev1.PersistentVolumeAccessMode) {
pvStorageSize = defaultPVStorageSize
pvcStorageSize = defaultPVStorageSize
storageClassName = defaultStorageClassName
hostPath = defaultHostPath
accessModes = defaultAccessModes
if g.ResourceConfig != nil && g.ResourceConfig.PersistentVolume != nil {
```
`.
Here are the changes:
<file_operations>
<file_operation operation="edit" file_path="internal/utils/kruize_generator.go">
<<<<<<< SEARCH
// getPVConfigWithDefaults is a shared helper that returns PV configuration with injected defaults
func (g *KruizeResourceGenerator) getPVConfigWithDefaults(
defaultPVStorageSize, defaultStorageClassName, defaultHostPath string,
defaultAccessModes []corev1.PersistentVolumeAccessMode,
) (pvStorageSize, pvcStorageSize, storageClassName, hostPath string, accessModes []corev1.PersistentVolumeAccessMode) {
pvStorageSize = defaultPVStorageSize
pvcStorageSize = defaultPVStorageSize
storageClassName = defaultStorageClassName
hostPath = defaultHostPath
accessModes = defaultAccessModes
if g.ResourceConfig != nil && g.ResourceConfig.PersistentVolume != nil {
=======
func ensurePVAndPVCStorageConsistency(pvStorageSize, pvcStorageSize string) (string, string, error) {
pvQty, err := resource.ParseQuantity(pvStorageSize)
if err != nil {
return pvStorageSize, pvcStorageSize, fmt.Errorf("failed to parse PV storage size %q: %w", pvStorageSize, err)
}
pvcQty, err := resource.ParseQuantity(pvcStorageSize)
if err != nil {
return pvStorageSize, pvcStorageSize, fmt.Errorf("failed to parse PVC storage size %q: %w", pvcStorageSize, err)
}
// If PVC > PV, surface the potential bind-time failure as early as possible.
if pvcQty.Cmp(pvQty) == 1 {
klog.Warningf("PVC storage request (%s) is greater than PV storage (%s); PVC may fail to bind", pvcStorageSize, pvStorageSize)
}
// We explicitly preserve the user-provided values here and only log a warning.
return pvStorageSize, pvcStorageSize, nil
}
// getPVConfigWithDefaults is a shared helper that returns PV configuration with injected defaults
func (g *KruizeResourceGenerator) getPVConfigWithDefaults(
defaultPVStorageSize, defaultStorageClassName, defaultHostPath string,
defaultAccessModes []corev1.PersistentVolumeAccessMode,
) (pvStorageSize, pvcStorageSize, storageClassName, hostPath string, accessModes []corev1.PersistentVolumeAccessMode) {
pvStorageSize = defaultPVStorageSize
pvcStorageSize = defaultPVStorageSize
storageClassName = defaultStorageClassName
hostPath = defaultHostPath
accessModes = defaultAccessModes
if g.ResourceConfig != nil && g.ResourceConfig.PersistentVolume != nil {
>>>>>>> REPLACE
</file_operation>
</file_operations>
<additional_changes>
To fully implement the behavior described in your review comment, you should also:
1. **Import the required packages** at the top of `internal/utils/kruize_generator.go` (or reuse existing logging/quantity packages if your codebase already uses them):
- Add (or reuse equivalent):
```go
import (
"fmt"
"k8s.io/apimachinery/pkg/api/resource"
"k8s.io/klog/v2"
)
```
If the project already uses a different logger (e.g. `logrus`, `zap`, or a wrapper), replace `klog.Warningf` with the appropriate warning-level log call and remove the `klog` import.
2. **Call the consistency helper after all overrides to PV/PVC sizes are applied** inside `getPVConfigWithDefaults`. Since we can’t see the full function body, place the call just before the function returns, once `pvStorageSize` and `pvcStorageSize` have their final values. For example:
```go
pvStorageSize, pvcStorageSize, err := ensurePVAndPVCStorageConsistency(pvStorageSize, pvcStorageSize)
if err != nil {
// Decide on error handling policy:
// - If invalid quantities should fail fast, return the error upstream (may require changing the signature).
// - Otherwise, log and continue with the raw string values.
klog.Warningf("Failed to validate PV/PVC storage consistency: %v", err)
}
```
If you want to **fail fast on invalid quantity strings**, you may need to:
- Change the signature of `getPVConfigWithDefaults` to return an `error`.
- Propagate that error to callers.
3. If parts of the function or callers rely on specific logging or error-handling patterns, adapt the `ensurePVAndPVCStorageConsistency` helper and the call site to fit those conventions (e.g. using structured logging, context-aware logging, or returning typed errors).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR makes CPU, memory, and storage resources configurable via the Kruize CR instead of being hardcoded in the operator.
Summary by Sourcery
Make Kruize CPU, memory, and storage resources configurable via the Kruize custom resource instead of being hardcoded, while preserving sensible defaults.
New Features:
Enhancements:
Documentation:
Tests: