Repository navigation
fix(apps/kubernetes): make pre-delete hook steps 3/5/6 fire-and-forget (#3271) - #3274
myasnikovdaniil wants to merge 1 commit into
Conversation
#3271) The Step-2 fix already merged with #2826 bounded the child-HelmRelease wait, but the pre-delete hook's remaining teardown steps still inherited kubectl's default --wait=true with no timeout: Step 3 delete KamajiControlPlane Step 5 delete DataVolumes Step 6 delete LoadBalancer Services Unlike the child HelmReleases in Step 2 — whose Flux finalizer only clears once helm-controller can uninstall INSIDE the tenant cluster, which is impossible on a nodeless tenant — these three target resources are finalized by controllers in THIS management cluster (Kamaji/CAPI, CDI + the CSI driver, the LB controller). Their teardown does not depend on tenant node health, so there is nothing to wait for on the Kubernetes-CR finalization path. Waiting only hurt: - Step 5's unbounded wait blocked the pre-cleanup Job until every worker-disk PVC was physically reclaimed; under storage pressure (the exact #3271 failure mode) that exceeds the parent Helm uninstall budget and wedges teardown. - Step 3's unbounded wait ran ahead of Step 4 on the same TCP teardown, so it could defeat Step 4's bounded wait + Step 4b datastore-secret backstop. Make all three --wait=false (fire-and-forget) and non-fatal (|| echo WARNING), so the Job completes fast, the parent Helm uninstall succeeds and the CR finalizes. The actual VM/PVC drain is waited on separately and best-effort by the caller (the e2e cozy_wait_tenant_drained barrier), not on the CR-finalization path — this restores the layering the kubernetes e2e harness was built around. Deliberately do NOT force-clear a stuck PVC/DataVolume finalizer: that would orphan the LINSTOR backing volume; a genuinely wedged PVC surfaces honestly as a Terminating namespace instead of being masked. Step 2 (bounded wait + child-HR finalizer force-clear) and Step 4/4b are left untouched — those are the paths that legitimately need a wait. Pins the new behavior in tests/delete_hook_test.yaml with step-local regexes (anchored to each command's own continuation lines, not a loose gap that would match a later step's flag and pass vacuously — mutation-verified). Co-Authored-By: Claude Opus 4.8 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]> Assisted-By: Claude <[email protected]>
github-actions
Bot
added
area/kubernetes
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where the tenant Kubernetes pre-delete hook would hang indefinitely during teardown if the tenant cluster lacked functional nodes. By switching specific cleanup steps to a fire-and-forget model, the process now completes within a bounded time, preventing the namespace from becoming stuck in a Terminating state while still allowing the system to handle storage and resource reclamation asynchronously. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pre-delete Kubernetes hook now bounds HelmRelease cleanup and performs non-blocking deletion of control-plane, DataVolume, and LoadBalancer resources, with tests verifying wait flags and finalizer behavior. ChangesTenant teardown cleanup
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses issue #3271 by updating the Kubernetes delete hook to prevent teardown from hanging on a nodeless tenant cluster. Specifically, it configures Step 3 (KamajiControlPlane), Step 5 (DataVolumes), and Step 6 (LoadBalancer Services) to use fire-and-forget deletion (--wait=false) with added warning logs on failure. Additionally, comprehensive unit tests have been added to delete_hook_test.yaml to verify these behaviors and ensure proper handling of finalizers. There are no review comments, and I have no further feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM — the --wait=false flags themselves are safe and the new assertions are solid, but three of the four rationale blocks this PR adds describe mechanisms that do not exist in this codebase, and the new || echo WARNING guards turn a genuine delete failure into a silent success.
Business context: the hang #3271 actually reported (Step 2's unbounded child-HelmRelease wait) is already fixed on main; this follow-up hardens Steps 3/5/6 of the tenant Kubernetes pre-delete hook against the same class of stall.
Blockers
B1: Step 5's rationale names resources it does not select
File: packages/apps/kubernetes/templates/delete.yaml:153-167
Issue: the comment says the default --wait=true "would block this hook until every worker-disk PVC is physically reclaimed". Neither half of that holds.
Evidence: (a) Worker boot disks come from dataVolumeTemplates in templates/cluster.yaml:91-94, whose metadata is only name: disk-system plus annotations — no labels. KubeVirt builds the DataVolume's ObjectMeta from the template alone and adds exactly one label, kubevirt.io/created-by; cluster-api-provider-kubevirt sets cluster.x-k8s.io/cluster-name on the VirtualMachine and on spec.template.objectMeta, but never on dataVolumeTemplates. So those DataVolumes never carry the label Step 5 selects on. The repo already records this in its own words at hack/e2e-chainsaw/_lib/run-kubernetes.sh:85 — "the worker-disk PVCs carry no cluster-scoping label to select on" — which is exactly why cozy_wait_tenant_drained falls back to a namespace-wide probe. What Step 5 does match are the tenant-workload DataVolumes the kubevirt-csi driver creates in the infra namespace, because the driver runs with INFRACLUSTER_LABELS=cluster.x-k8s.io/cluster-name=<release> (templates/csi/deploy.yaml:56). (b) kubectl delete --wait=true waits for the selected objects and their finalizers; under the default --cascade=background the dependent PVC is reaped asynchronously by the garbage collector and is never waited on. kubectl delete --help states both.
Impact: fifteen lines of load-bearing rationale in the most delicate teardown path point the next maintainer at a mechanism that cannot fire, and the release note credits the fix to worker-disk cleanup this step does not perform.
Fix: rewrite the comment to describe what Step 5 actually deletes (tenant CSI DataVolumes) and what the wait actually covers, and correct the PR body and release note to match.
B2: Step 3's rationale describes a hang that cannot occur
File: packages/apps/kubernetes/templates/delete.yaml:93-103
Issue: the comment claims a default --wait=true here "would run first and defeat" Step 4's bounded wait and the Step 4b backstop. In this chart's configuration the KamajiControlPlane carries no finalizer, so that wait was already instantaneous.
Evidence: cluster-api-control-plane-provider-kamaji adds its ExternalClusterReferenceFinalizer only inside if kcp.Spec.Deployment.ExternalClusterReference != nil (controllers/kamajicontrolplane_controller.go), and handleDeletion returns early without touching finalizers when that field is nil. The chart never sets externalClusterReference — grep over packages/apps/kubernetes/templates/ returns nothing. An object with no finalizer is removed the moment the DELETE lands, so --wait=true returned immediately; Step 4's --timeout=120s was already the only bound on TenantControlPlane teardown, both before and after this PR.
Impact: the Step 3 change is a no-op, justified by eleven lines describing a stall the controller cannot produce.
Fix: either drop the Step 3 change, or replace the rationale with the real reason (consistency / defence-in-depth) and say so plainly.
B3: || echo WARNING turns a real delete failure into a silent success, and then deletes the evidence
File: packages/apps/kubernetes/templates/delete.yaml:106, 171, 184
Issue: the script runs under set -e. Before this PR a failing delete (missing RBAC, an API 5xx, an absent CRD) aborted the Job. The Job's hook-delete-policy is hook-succeeded,before-hook-creation (line 8 — note the absence of hook-failed), so a failed Job and its logs were retained and Helm surfaced the error. Now the same failure is swallowed, the script still prints Cleanup completed successfully and exits 0, Helm deletes the Job as succeeded, and the WARNING lines go with it.
Evidence: sh -c 'set -e; false || echo WARN >&2; echo "Cleanup completed successfully"' exits 0. A leaked LoadBalancer Service keeps holding an address from the pool and a leaked CSI DataVolume keeps holding LINSTOR capacity, with no operator-visible signal anywhere.
Impact: silent resource leaks on the one code path whose entire job is cleanup.
Fix: the || guards are not needed for the stated fix — --wait=false alone already gives fire-and-forget. They change behaviour only on genuine errors, which is precisely where this hook should stay loud. Drop them, or track a failure flag and stop reporting success when it is set. Either way the behaviour needs a test.
Non-blocking follow-ups
delete.yaml:175-176names "MetalLB / kube-vip" as the LB controller. kube-vip is not used anywhere in this platform — the only occurrence of the string underpackages/is this new comment. The actual implementations are MetalLB / Cilium LB-IPAM.tests/delete_hook_test.yaml:118-120— the Step 5notMatchRegexis positional (delete datavolumes[\s\S]*?patch[\s\S]*?finalizers), so a force-clear added before Step 5 would slip through it. Anchor on the resource rather than on ordering.- Step 4's
--timeout=120sis the only bound on TenantControlPlane teardown and is not pinned by any assertion, while the new suite does pin Step 2's. Worth pinning while this file is open. - The PR body says Step 2's wait "was already bounded in #2826" — it was bounded by the commit that landed for this same issue, not by #2826.
- The PR body carries no
Closes #3271.
| # here would only block this hook on the same TCP teardown that | ||
| # Step 4 already handles with a bounded wait + Step 4b backstop — an | ||
| # unbounded wait here would run first and defeat that backstop. So | ||
| # trigger deletion and move on; Step 4 owns the bounded TCP wait. |
There was a problem hiding this comment.
B2 — this rationale describes a hang that cannot occur here.
The KamajiControlPlane carries no finalizer in this chart's configuration: cluster-api-control-plane-provider-kamaji adds ExternalClusterReferenceFinalizer only inside if kcp.Spec.Deployment.ExternalClusterReference != nil, and handleDeletion returns early without touching finalizers when that field is nil. The chart never sets externalClusterReference (grep over packages/apps/kubernetes/templates/ is empty).
An object with no finalizer is removed the moment the DELETE lands, so the previous --wait=true here returned immediately. It never "ran first" and never defeated anything — Step 4's --timeout=120s was already the only bound on TenantControlPlane teardown, before and after this change.
Either drop the Step 3 change or state the real reason (consistency / defence-in-depth) instead of a stall the controller cannot produce.
| # path. Do NOT force-clear a stuck PVC/DataVolume finalizer here: | ||
| # that would orphan the LINSTOR backing volume. A genuinely wedged | ||
| # PVC is a storage-layer problem and surfaces honestly as a | ||
| # Terminating namespace rather than being masked. |
There was a problem hiding this comment.
B1 — this comment names resources Step 5 does not select, and a kubectl behaviour that does not exist.
Worker disks are not matched. They come from dataVolumeTemplates in templates/cluster.yaml:91-94, whose metadata is only name: disk-system plus annotations — no labels. KubeVirt builds a DataVolume's ObjectMeta from the template alone and adds exactly one label, kubevirt.io/created-by; cluster-api-provider-kubevirt sets cluster.x-k8s.io/cluster-name on the VirtualMachine and on spec.template.objectMeta, never on dataVolumeTemplates. The repo already says this at hack/e2e-chainsaw/_lib/run-kubernetes.sh:85: "the worker-disk PVCs carry no cluster-scoping label to select on" — which is why cozy_wait_tenant_drained uses a namespace-wide probe instead.
What Step 5 does match are the tenant-workload DataVolumes the kubevirt-csi driver creates in the infra namespace, since it runs with INFRACLUSTER_LABELS=cluster.x-k8s.io/cluster-name=<release> (templates/csi/deploy.yaml:56). The selector is right for what it actually does; the comment explaining it is not.
The wait never covered PVC reclaim either. kubectl delete --wait=true waits for the selected objects and their finalizers; under the default --cascade=background the dependent PVC is reaped asynchronously by the garbage collector and is never waited on.
Please rewrite this block to describe what the step really deletes, and correct the PR body / release note, which carry the same claim.
| --field-selector spec.type=LoadBalancer \ | ||
| --ignore-not-found=true | ||
| --ignore-not-found=true --wait=false \ | ||
| || echo "WARNING: failed to trigger deletion of LoadBalancer Services for {{ .Release.Name }}" >&2 |
There was a problem hiding this comment.
B3 — this guard (and its twins on lines 106 and 171) converts a real failure into a silent success, then destroys the evidence.
The script runs under set -e. Before this change, a failing delete — missing RBAC, an API 5xx, an absent CRD — aborted the Job; because hook-delete-policy is hook-succeeded,before-hook-creation (line 8, no hook-failed), the failed Job and its logs were retained and Helm surfaced the error. Now the same failure is swallowed, the script still prints Cleanup completed successfully and exits 0, Helm deletes the Job as succeeded — and these WARNING lines go with it. A leaked LoadBalancer Service keeps holding an address from the pool; a leaked CSI DataVolume keeps holding LINSTOR capacity. No signal reaches the operator.
The || guards are not needed for the stated fix: --wait=false alone already gives fire-and-forget. They change behaviour only on genuine errors, which is exactly where this hook should stay loud. Drop them, or track a failure flag and stop reporting success when it is set — with a test either way.
| echo "Step 6: Cleaning up LoadBalancer Services" | ||
| # Fire-and-forget (--wait=false): these LoadBalancer Services are | ||
| # reconciled by the management cluster's LB controller (MetalLB / | ||
| # kube-vip), so their teardown does not depend on tenant node |
There was a problem hiding this comment.
kube-vip is not used anywhere in this platform — the only occurrence of the string under packages/ is this new comment. The actual load-balancer implementations are MetalLB and Cilium LB-IPAM. Please drop the invented reference.
| # Kamaji datastore Secret (Step 4b) are ever finalizer-stripped. | ||
| - notMatchRegex: | ||
| path: spec.template.spec.containers[0].command[2] | ||
| pattern: 'delete datavolumes[\s\S]*?patch[\s\S]*?finalizers' |
There was a problem hiding this comment.
This guard is positional rather than resource-anchored: it only catches a finalizer force-clear that appears after delete datavolumes. One added before Step 5 would pass. Worth anchoring on the resource (patch targeting a datavolume/pvc) instead of on ordering.
The rest of the new suite is genuinely non-vacuous — anchoring the flags to each command's own continuation lines does fail when the step's --wait=false is removed, and that is a real improvement over the usual [\s\S]* gap.
|
Aleksei Sviridkin (@lexfrei) Thanks — this is correct on every point, and it goes deeper than the individual blockers: the premise of the PR doesn't hold. The hang #3271 actually demonstrated is Step 2's unbounded child-HelmRelease wait — the tenant's child addon HelmReleases carry a Flux finalizer that can only clear once helm-controller uninstalls inside the tenant, impossible on a nodeless tenant. That's already fixed on
Rather than rewrite three rationale blocks around a stall that doesn't occur, I'm closing this PR — it doesn't fix a demonstrated problem, and the real fix is already on |
2 participants
What this PR does
Completes the fix for #3271. The tenant Kubernetes pre-delete hook (
packages/apps/kubernetes/templates/delete.yaml, Job<release>-pre-cleanup) could hang teardown when the tenant cluster has no working nodes (LINSTOR/ZFS exhaustion, worker VMs that never joined, or a nodeGroup atminReplicas: 0).The Step 2 child-HelmRelease wait was already bounded in #2826, but the remaining teardown steps still inherited
kubectl delete's default--wait=truewith no timeout:KamajiControlPlaneDataVolumesServicesWhy these three are different from Step 2
Step 2's child HelmReleases carry a Flux finalizer that only clears once helm-controller can uninstall inside the tenant cluster — impossible with no Ready nodes — so Step 2 genuinely needs its bounded wait + finalizer force-clear (unchanged here).
Steps 3/5/6 target resources finalized by controllers in this management cluster (Kamaji/CAPI, CDI + the CSI driver, the LB controller). Their teardown does not depend on tenant node health, so there is nothing to wait for on the
Kubernetes-CR finalization path — the default wait only hurt:The change
Make Steps 3/5/6
--wait=false(fire-and-forget) and non-fatal (|| echo WARNING), so the Job completes promptly, the parent Helm uninstall succeeds, and the CR finalizes. The actual VM/PVC drain is waited on separately and best-effort by the caller (the e2ecozy_wait_tenant_drainedbarrier inhack/e2e-chainsaw/_lib/run-kubernetes.sh), not on the CR-finalization path — this restores the layering the kubernetes e2e harness was built around.Deliberately not force-clearing a stuck PVC/DataVolume finalizer: that would orphan the LINSTOR backing volume. A genuinely wedged PVC is a storage-layer problem and surfaces honestly as a
Terminatingnamespace rather than being masked.Tests
Adds four behavioural cases to
tests/delete_hook_test.yamlpinning the fire-and-forget behaviour of Steps 3/5/6 and the retained bounded-wait + force-clear of Step 2. The regex assertions are anchored to each command's own continuation lines (a loose[\s\S]*?gap matches a later step's flag and passes vacuously) — each assertion was mutation-verified to fail when its step's--wait=falseis removed. Fullkuberneteschart unittest suite: 180/180. Rendered hook passesshellcheck -s sh.Release note
Summary by CodeRabbit