fix(operator): apply persistence volume size increases online - #1634
fix(operator): apply persistence volume size increases online#1634sylr wants to merge 3 commits into
Conversation
A change to spec.persistence.<vol>.size was silently dropped: the StatefulSet reconcile only detected volumeClaimTemplate *name* changes, and the normal update path skips the immutable VCT entirely — so the size bump bumped the pod-template spec-hash (rolling the pods) while the disks stayed at their old size. Add a grow-only PVC expansion pass: patch each existing PVC's spec.resources.requests.storage upward (CSI online expansion), then delete-recreate the StatefulSet with orphan propagation so future scale-out ordinals inherit the new template size. Grow-only — a shrink is rejected with a VolumeShrinkRejected warning event; each expansion emits VolumeExpanded. Shrink intent is decided from the template delta, not per-PVC size, so a PVC manually expanded past the new template size is left alone rather than warned about on every reconcile. The expansion/recreate is skipped while a scale-down is in progress so it does not bypass the deleteScaledDownPVCs cleanup and orphan the removed ordinals' PVCs. Constraint: StatefulSet volumeClaimTemplates are immutable; live PVCs can only grow via a direct requests.storage patch, and the template can only be re-emitted by orphan delete-recreate Constraint: StorageClass must have allowVolumeExpansion=true for the CSI patch to take effect Rejected: Operator-owned PVCs instead of volumeClaimTemplates | loses per-ordinal RWO identity or forces reimplementing StatefulSet — unacceptable risk for a Raft cluster Rejected: Spec-only shrink validation in validateClusterConfig | it cannot see the prior size, so it cannot detect a shrink Confidence: high Scope-risk: moderate Directive: shrink detection MUST stay keyed on the template delta, not the per-PVC current size, or over-expanded PVCs emit false shrink warnings every reconcile Not-tested: full delete-recreate + pod re-adoption cycle and the resize+scale-down deferral (raftScaleDown execs into pods; not unit-testable with a fake clientset) — needs e2e
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
✅ Approve — automated reviewBoth major issues identified in the automated review — the rejected-shrink size leaking into the recreated StatefulSet template, and the resize-only pod-template hash causing an unintended pod rollout — were acknowledged by the author and addressed in commit 08227de. The fix floors each desired template size to the largest live PVC (grow-only, sourced from live PVCs that survive the orphan delete), clamps rejected shrinks with a VolumeShrinkRejected event, and applies the clamp unconditionally on every reconcile path including the post-delete create. Regression tests covering the mixed grow/shrink scenario were also added. The sole independent reviewer found no additional correctness issues beyond what was already discussed and resolved. No remaining blocker or major findings are present. No findings. |
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #1634 (comment)
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Reviewed live head f064a124. The existing NumaryBot finding on this same head is a correctness/runtime blocker: if one volume grows while another is requested smaller, the PVC expansion pass rejects the shrink but the grow still triggers StatefulSet recreation from the unmodified desired templates. The recreated template therefore persists the rejected smaller size for future ordinals. Please sanitize rejected shrink templates back to their existing sizes (and add the mixed grow/shrink regression) before recreating. I am not duplicating the inline finding. E2E CI is still running and is not the basis of this request for changes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/v3.0 #1634 +/- ##
================================================
- Coverage 74.75% 74.46% -0.29%
================================================
Files 446 446
Lines 47485 47485
================================================
- Hits 35497 35360 -137
- Misses 8777 8929 +152
+ Partials 3211 3196 -15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Addresses the mixed grow/shrink review finding (NumaryBot + @flemzord): when one volume grew and another was requested smaller in the same Cluster update, the grow forced a StatefulSet delete-recreate that rebuilt every volumeClaimTemplate from spec — leaking the rejected smaller size into the recreated template and every future scale-out ordinal. Floor each desired template size up to the largest live PVC for that volume (grow-only), applied on every reconcile path — including the create path right after the orphan delete, where the old template is gone. The floor comes from the live PVCs, which survive the orphan delete, so it is stable across the recreate. A spec size below the live disk is rejected with a single VolumeShrinkRejected event per volume and the template keeps the current size. Merge the clamp and grow into a single PVC pass and run it unconditionally (previously only inside the STS-found branch), so the create path is also clamped. Constraint: the StatefulSet template is destroyed by the orphan delete-recreate, so the pre-shrink size must be recovered from a source that survives it — the live PVCs Rejected: floor against the existing StatefulSet template | the template is gone on the post-recreate create pass, so it cannot floor the rebuilt template Confidence: high Scope-risk: narrow Directive: the grow-only floor MUST be sourced from live PVCs, not the STS template, or the mixed grow/shrink leak returns on the recreate path Not-tested: end-to-end recreate + pod re-adoption cycle (raftScaleDown execs into pods; not unit-testable with a fake clientset)
|
Fixed in 08227de. Root cause: the grow forced a StatefulSet delete-recreate that rebuilt every Fix: floor each desired template size up to the largest live PVC for that volume (grow-only), applied on every reconcile path — including the create path right after the orphan delete. The floor is sourced from the live PVCs (which survive the orphan delete), not the StatefulSet template, so it's stable across the recreate. A spec size below the live disk is rejected with a single The clamp + grow are now a single PVC pass run unconditionally (previously only inside the STS-found branch), so the create path is clamped too. Regression tests added (
Full Thanks @flemzord for the catch. |
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #1634 (comment)
Addresses review finding: a volume-size change bumped computeSpecHash, which is embedded in the pod-template annotation. On the online-expansion recreate the new StatefulSet then derived a different ControllerRevision from the adopted pods and (Partition 0) rolled them — so the "online, no-restart" expansion actually restarted every pod. Size, storageClass, accessMode and volumeAttributesClassName map to the VolumeClaimTemplates / live PVCs, never to the pod template (buildPodTemplate ignores them), so exclude them from the hash — same rationale as the already-excluded DeletionProtection. HostPath is kept: switching PVC<->hostPath genuinely changes the pod template. Also clarify at the recreate site that the branch only deletes; the replacement template is built on the next reconcile, which re-clamps desired against the live PVCs — so a rejected shrink cannot leak into the recreated template (the second reviewer finding, already handled by the live-PVC floor, was a false positive on the deferred-recreate pattern). Constraint: adopted-pod ControllerRevision is derived from the pod template incl. the spec-hash annotation, so any hash input that is not a real pod-template field forces an unwanted rollout Confidence: high Scope-risk: narrow Directive: keep HostPath in the hash — it is the one VolumeSpec field that changes the pod template Not-tested: observing zero pod restarts on a live cluster resize (needs e2e)
|
Second review pass addressed in 97c4450. Finding B — resize alters pod-template hash → pod rollout (valid, fixed). Finding A — rejected shrink persisted into recreated template (already handled; false positive on this head). All CI green on the prior head; |
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot review complete: no remaining inline findings.
Resolved 2 stale NumaryBot review threads (2 fixed, 0 outdated).
Summary: #1634 (comment)
|
Revalidated live head |
|
Non-expandable StorageClass turns a size bump into a wedged reconcile. The |
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Revalidated live head 97c44509 after the new Azorlogh comment. I confirm that existing comment as a correctness/runtime blocker: a rejected PVC expansion patch returns before the normal StatefulSet update and the remaining reconciliation passes, so an unsupported StorageClass can leave unrelated desired changes permanently blocked until the size request is reverted. I am not duplicating the existing details. Please ensure an unsupported expansion cannot wedge the rest of reconciliation. CI is green and is not the basis of this review.
|
superseded by #1674 |
4 participants
Problem
Editing
spec.persistence.<vol>.sizeon aCluster(e.g.data: 2Ti → 3Ti) did nothing to the disks. The StatefulSet reconcile only detected volumeClaimTemplate name changes (volumeClaimTemplatesChangedcompares the name set), and the normal update path deliberately skips the immutable VCT. The size bump did change the pod-template spec-hash — so pods rolled — while the volumes stayed at the old size. The STSvolumeClaimTemplateskept reporting the old size.Change
reconcilePVCExpansion(internal/controller/reconcile_pvc_expansion.go): patches each existing PVC'sspec.resources.requests.storageupward, triggering CSI online expansion. Grow-only.reconcileStatefulSet: after the expansion pass, delete-recreate the StatefulSet withDeletePropagationOrphan(pods/PVCs retained and re-adopted, no restart) whenever VCT names changed or a size grew, so future scale-out ordinals inherit the new template size.allowVolumeExpansion: trueprerequisite, and grow-only semantics.Behavior notes
VolumeShrinkRejectedwarning event and leaves the PVC untouched (K8s cannot shrink a PVC). Each successful expansion emitsVolumeExpanded.deleteScaledDownPVCsand orphan the removed ordinals' PVCs (also closes the same latent issue in the pre-existing name-change branch). The template change is picked up on the next reconcile once the scale-down settles.Why not operator-owned PVCs?
Keeping
volumeClaimTemplatespreserves per-ordinal RWO identity for the Raft nodes for free. Owning PVCs directly means either shared storage (broken) or reimplementing StatefulSet (unacceptable risk). The gap was only the missing grow path — same pattern as Prometheus Operator / CloudNativePG.Review
Reviewed by Codex (gpt-5.5): confirmed the
resource.Quantity.Cmpapproach, therequests.storagepatch API, and orphan re-adoption. It found two Medium issues — false shrink warnings for over-expanded PVCs, and the resize+scale-down cleanup bypass — both fixed in this branch and covered by tests.Tests
reconcile_pvc_expansion_test.go(grow-all + recreate signal, no-op, shrink rejected, missing ordinal skipped, over-expanded left alone with no event, shrink-warns-once-per-volume).internal/controllerpackage passes;golangci-lint0 issues;go build ./...clean.Not covered (needs e2e follow-up)
The full delete-recreate + pod re-adoption cycle and the resize+scale-down deferral are not unit-testable with a fake clientset (
raftScaleDownexecs into pods).