Avoid updating cpu and memory in pvc template#1720
Conversation
WalkthroughPVC template handling now sets only Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/backingstore/reconciler.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-hac-test
- GitHub Check: run-operator-tests
- GitHub Check: golangci-lint
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-cli-tests
- GitHub Check: run-admission-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-kms-key-rotate-test
6edb05e to
31dee3f
Compare
31dee3f to
cfb7f35
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/backingstore/reconciler.go (1)
570-577: Add validation for zero storage quantity.When
VolumeResourcesis provided but the storage request is missing or zero, the code uses a zerovolumeSizeinstead of falling back todefaultVolumeSize. This could cause NooBaa pool creation to fail with aBAD_REQUESTerror (as handled at lines 989-992).Apply this diff to validate the storage quantity:
var volumeSize int64 pvPool := r.BackingStore.Spec.PVPool -if pvPool.VolumeResources != nil { - qty := pvPool.VolumeResources.Requests[corev1.ResourceName(corev1.ResourceStorage)] - volumeSize = qty.Value() -} else { - volumeSize = int64(defaultVolumeSize) -} +if pvPool.VolumeResources != nil { + qty := pvPool.VolumeResources.Requests[corev1.ResourceStorage] + if !qty.IsZero() { + volumeSize = qty.Value() + } else { + volumeSize = int64(defaultVolumeSize) + } +} else { + volumeSize = int64(defaultVolumeSize) +}Note: Also removed redundant type cast
corev1.ResourceName(corev1.ResourceStorage)sincecorev1.ResourceStorageis already of typeResourceName.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/backingstore/backingstore.go(1 hunks)pkg/backingstore/reconciler.go(1 hunks)
🔇 Additional comments (1)
pkg/backingstore/backingstore.go (1)
250-253: LGTM! Default volume size constant added correctly.The
defaultVolumeSizeconstant (20Gi) provides a sensible fallback when PVPool volume size is not specified, aligning with the PR objective to handle storage sizing consistently.
pkg/backingstore/reconciler.go
Outdated
| var volumeSize resource.Quantity | ||
| pvPool := r.BackingStore.Spec.PVPool | ||
| if pvPool.VolumeResources != nil { | ||
| volumeSize = pvPool.VolumeResources.Requests[corev1.ResourceName(corev1.ResourceStorage)] | ||
| } else { | ||
| volumeSize = *resource.NewQuantity(defaultVolumeSize, resource.BinarySI) | ||
| } | ||
| r.PvcAgentTemplate.Spec.Resources.Requests[corev1.ResourceName(corev1.ResourceStorage)] = volumeSize |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add validation for zero storage quantity.
Similar to the issue in ReadSystemInfo, when VolumeResources is provided but the storage request is missing or zero, the code uses a zero Quantity instead of falling back to defaultVolumeSize. This would cause PVC creation to fail during Kubernetes validation.
Apply this diff to validate the storage quantity:
var volumeSize resource.Quantity
pvPool := r.BackingStore.Spec.PVPool
-if pvPool.VolumeResources != nil {
- volumeSize = pvPool.VolumeResources.Requests[corev1.ResourceName(corev1.ResourceStorage)]
-} else {
- volumeSize = *resource.NewQuantity(defaultVolumeSize, resource.BinarySI)
-}
+if pvPool.VolumeResources != nil {
+ volumeSize = pvPool.VolumeResources.Requests[corev1.ResourceStorage]
+ if volumeSize.IsZero() {
+ volumeSize = *resource.NewQuantity(defaultVolumeSize, resource.BinarySI)
+ }
+} else {
+ volumeSize = *resource.NewQuantity(defaultVolumeSize, resource.BinarySI)
+}
r.PvcAgentTemplate.Spec.Resources.Requests[corev1.ResourceStorage] = volumeSizeNote: Also removed redundant type cast corev1.ResourceName(corev1.ResourceStorage).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var volumeSize resource.Quantity | |
| pvPool := r.BackingStore.Spec.PVPool | |
| if pvPool.VolumeResources != nil { | |
| volumeSize = pvPool.VolumeResources.Requests[corev1.ResourceName(corev1.ResourceStorage)] | |
| } else { | |
| volumeSize = *resource.NewQuantity(defaultVolumeSize, resource.BinarySI) | |
| } | |
| r.PvcAgentTemplate.Spec.Resources.Requests[corev1.ResourceName(corev1.ResourceStorage)] = volumeSize | |
| var volumeSize resource.Quantity | |
| pvPool := r.BackingStore.Spec.PVPool | |
| if pvPool.VolumeResources != nil { | |
| volumeSize = pvPool.VolumeResources.Requests[corev1.ResourceStorage] | |
| if volumeSize.IsZero() { | |
| volumeSize = *resource.NewQuantity(defaultVolumeSize, resource.BinarySI) | |
| } | |
| } else { | |
| volumeSize = *resource.NewQuantity(defaultVolumeSize, resource.BinarySI) | |
| } | |
| r.PvcAgentTemplate.Spec.Resources.Requests[corev1.ResourceStorage] = volumeSize |
🤖 Prompt for AI Agents
In pkg/backingstore/reconciler.go around lines 1395 to 1402, when
PVPool.VolumeResources is present the code currently reads the storage Quantity
but does not check for a nil/missing or zero value, causing a zero-size PVC to
be used; update the logic to (1) check that
pvPool.VolumeResources.Requests[corev1.ResourceStorage] exists and is greater
than zero, and if not, set volumeSize to resource.NewQuantity(defaultVolumeSize,
resource.BinarySI) as the fallback, and (2) remove the redundant
corev1.ResourceName(...) cast when indexing Requests so you use
corev1.ResourceStorage directly. Ensure the final volumeSize is assigned into
r.PvcAgentTemplate.Spec.Resources.Requests[corev1.ResourceStorage].
Signed-off-by: jackyalbo <jacky.albo@gmail.com>
cfb7f35 to
9804adc
Compare
Signed-off-by: jackyalbo <jacky.albo@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/run-golangci-lint.sh (1)
29-29: Update comment to reflect version pinning.The comment references "latest" but the installation now pins to a specific version. Update the comment to clarify that a fixed version is being installed.
- echo "Installing the latest golangci-lint with local toolchain" + echo "Installing golangci-lint v2.5.0 with local toolchain"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/run-golangci-lint.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-02T10:23:02.154Z
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1687
File: scripts/run-golangci-lint.sh:16-16
Timestamp: 2025-09-02T10:23:02.154Z
Learning: In the noobaa-operator repository's run-golangci-lint.sh script, file list support was intentionally removed as part of the golangci-lint refactoring. The script now always runs lint on all files rather than supporting file-specific linting, which is the intended behavior.
Applied to files:
scripts/run-golangci-lint.sh
📚 Learning: 2025-06-25T15:42:04.436Z
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1637
File: Makefile:193-203
Timestamp: 2025-06-25T15:42:04.436Z
Learning: golangci-lint v2.x versions output the version with a leading "v" prefix (e.g., "golangci-lint has version v2.1.6..."), while v1.x versions output without the "v" prefix (e.g., "golangci-lint has version 1.64.8..."). Version detection logic should account for the specific version format being targeted.
Applied to files:
scripts/run-golangci-lint.sh
📚 Learning: 2025-06-25T15:39:17.448Z
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1637
File: .golangci.yml:1-14
Timestamp: 2025-06-25T15:39:17.448Z
Learning: In golangci-lint v2 configuration format, the `exclusions` section is correctly placed under the `linters:` key, not at the top-level. The structure `linters.exclusions.paths` and `linters.exclusions.rules` is valid and documented in the official golangci-lint v2 configuration.
Applied to files:
scripts/run-golangci-lint.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-admission-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-azure-vault-test
- GitHub Check: golangci-lint
- GitHub Check: run-kms-dev-test
- GitHub Check: run-operator-tests
- GitHub Check: run-hac-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-cli-tests
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-kms-tls-sa-test
🔇 Additional comments (1)
scripts/run-golangci-lint.sh (1)
30-30: golangci-lint v2.5.0 is a valid, stable release.v2.5.0 was published on 2025-09-21 and includes standard maintenance updates. The changelog shows normal development activity with dependency bumps and feature additions, with no reported security issues. The version pinning improves reproducibility as intended.
Explain the changes
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit