Repository navigation
fix(opensearch): remove the data PVCs when the application is deleted - #4136
Aleksei Sviridkin (lexfrei) merged 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe OpenSearch chart adds a post-delete hook that deletes release-labeled PVCs. It creates a cleanup Job and supporting RBAC resources. Helm tests validate the hook configuration, security settings, image configuration, permissions, and bindings. ChangesOpenSearch PVC cleanup
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant HelmRelease
participant CleanupJob
participant KubernetesAPI
participant PVCs
HelmRelease->>CleanupJob: Run post-delete hook
CleanupJob->>KubernetesAPI: Delete PVCs matching the release label
KubernetesAPI->>PVCs: Remove matching persistentvolumeclaims
PVCs-->>KubernetesAPI: Return deletion result
KubernetesAPI-->>CleanupJob: Return command status
Merge Risk: ⚪ Minimal · up to The reported cleanup failure does not occur under Helm’s hook lifecycle, and no actionable merge-blocking risk remains. 🚥 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 |
github-actions
Bot
added
size/L
663bd08 to
3ba3913
Compare
scooby87
left a comment
scooby87
left a comment
There was a problem hiding this comment.
NOT LGTM — one MAJOR blocker (independent cozy-review pass).
The selector is adapted correctly (not a blind clickhouse copy): opster.io/opensearch-cluster={{ .Release.Name }} is what the opensearch operator copies onto the STS-created PVCs (verified against opensearch-k8s-operator v2.8.0), so the hook is not a no-op. Security context and RBAC are tight, helm unittest 6/6.
[MAJOR] Teardown-hang vector: the post-delete Job has no spec.activeDeadlineSeconds and kubectl delete pvc has no --wait=false. templates/hooks/cleanup-pvc.yaml. kubectl delete waits for the pvc-protection finalizer by default; with drainDataNodes: true the pods shut down slowly, and if anything stalls the delete hangs forever, the Job never finishes, and the HelmRelease is stuck uninstalling. Fix: activeDeadlineSeconds (60-300s) + --wait=false. Context for calibration: the clickhouse cleanup hook this PR is modelled on shares the same gap, so ideally fix both or treat it as a consciously accepted repo pattern.
[MINOR] hook-delete-policy: before-hook-creation,hook-succeeded on the Job destroys the Job and its logs on success and orphans it on failure. The cited clickhouse example deliberately keeps only before-hook-creation on the Job (with a "keep a failed cleanup Job and its logs" comment). Drop hook-succeeded from the Job.
[NIT] watch on the PVC Role is unnecessary once the delete uses --wait=false; narrow to get/list/delete.
|
Done in 62d4a59: |
scooby87
left a comment
scooby87
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
Independent cozy-review pass on head 62d4a59a. This head resolves the sole MAJOR blocker from the prior review round (self-reversal explained below — the blocking code was changed, not re-graded).
What the PR does
Adds a post-delete Helm hook (Job + ServiceAccount + namespace-scoped Role/RoleBinding) to packages/apps/opensearch that deletes the OpenSearch data PVCs when the app is uninstalled, closing the orphaned-PVC / leaked-storage bug. Brings opensearch in line with the established apps/{clickhouse,mariadb,qdrant}/templates/hooks/cleanup-pvc.yaml convention, and is in fact a hardened version of it.
Verification performed (hermetic)
helm unittest— 6/6 on the new suite, 64/64 for the whole chart.helm templaterenders the hook cleanly; image resolves toclastix/kubectl:v1.32@sha256:…and picks up the_cluster.images-registryprefix.- Selector correctness cross-checked against the chart itself (
templates/opensearch.yamlnames theOpenSearchCluster{{ .Release.Name }}and usesopster.io/opensearch-cluster: {{ .Release.Name }}as its pod-level selector) and against the operator (opensearch-k8s-operator v2.8.0 copies that label onto the STS-created PVCs). So the delete selector is not a no-op. allow-to-apiserveregress path confirmed:apps/tenant/templates/networkpolicy.yamldefines aCiliumNetworkPolicygranting egress tokube-apiserverfor pods labelledpolicy.cozystack.io/allow-to-apiserver: "true", which the cleanup pod carries.
Phase 5b — upgrade + fresh install
- Fresh install: the four hook objects are
post-deleteonly, so they add nothing at install/upgrade time; they materialise solely on uninstall. No effect on a running cluster until it is deleted. - Existing-customer upgrade: purely additive (two new files). No default flips, no RBAC contraction on the running workload, no Secret/webhook/immutability traps. The new namespace-scoped
Rolegrants onlyget/list/deleteonpersistentvolumeclaims. - Delete semantics: after this change, deleting an OpenSearch app destroys its data PVCs. That is the intended fix and matches the platform-wide managed-service semantic already shipped for clickhouse/mariadb/qdrant — not a behaviour regression.
- Teardown safety:
activeDeadlineSeconds: 120+backoffLimit: 1+kubectl delete … --wait=falsebound the hook; it cannot wedge a namespace/tenant teardown. Fail-closed (exit 1) surfaces a failed cleanup rather than silently re-orphaning PVCs.
Findings
-
[MINOR] The load-bearing
policy.cozystack.io/allow-to-apiserver: "true"pod-template label is not asserted by any test.packages/apps/opensearch/tests/cleanup_pvc_test.yaml. The test covers securityContext, image pinning, RBAC and the kubectl command, but not this label. It is essential: drop it and the cleanup pod cannot reach the apiserver, sokubectl deletehangs untilactiveDeadlineSeconds(120 s) and the Job fails — a failure that is production-only and green-in-CI, hence hard to catch on the next edit. Adding oneequalassertion onspec.template.spec.template.metadata.labels["policy.cozystack.io/allow-to-apiserver"]closes the gap. Non-blocking. -
[NIT]
set -uhere vsset -euin the sibling clickhouse hook.templates/hooks/cleanup-pvc.yaml. Functionally immaterial (the single meaningful command is already guarded byif ! … ; then exit 1), so no behaviour change — noted only for cross-app consistency.
Notes / considerations (non-blocking)
- Correctness rests on the operator labelling the STS-created PVCs with
opster.io/opensearch-cluster=<cluster>; verified against opensearch-k8s-operator v2.8.0 but worth one live-cluster confirmation on the actual operator version this platform ships, since it is the linchpin of the whole fix. - Inherent to any post-delete PVC cleanup of an operator-managed StatefulSet: a narrow window exists where the StatefulSet controller could re-create a just-deleted PVC if the operator has not yet torn down the STS.
--wait=falseplus thepvc-protectionfinalizer make the worst case degrade to the pre-existing orphan behaviour (no new data loss). This pattern is shared verbatim with the reviewed clickhouse/mariadb/qdrant hooks and is accepted repo convention.
Self-reversal disclosure
The prior review on this PR (same reviewer, CHANGES_REQUESTED, 2026-09-17) raised one MAJOR: the post-delete Job had no activeDeadlineSeconds and the kubectl delete had no --wait=false (teardown-hang vector). Head 62d4a59a fixes exactly that — --wait=false added, activeDeadlineSeconds: 120 added, hook-succeeded dropped from the Job only, Role narrowed to get/list/delete, success message no longer over-claims. The reversal to LGTM is on changed code, not a re-grading of identical code.
The OpenSearch operator creates its data volumes through a StatefulSet, so the PVCs carry no ownerReference, and the chart had no cleanup hook. Removing the application left every data volume Bound and counting against the tenant quota, unlike clickhouse, kafka and harbor, whose post-delete hooks reclaim the operator-owned storage. A post-delete hook in the same shape as the clickhouse one now deletes the PVCs labelled opster.io/opensearch-cluster=<release>, with a Role limited to persistentvolumeclaims and the kubectl image pinned by digest. Signed-off-by: Yan Bondarenko <[email protected]>
kubectl delete waits for the claims to be gone unless told otherwise, and OpenSearch data pods stop slowly, so the post-delete hook could outlive helm-controller's 300s uninstall budget on a claim that would not release and leave the HelmRelease stuck uninstalling. The delete now runs with --wait=false, the shape the monitoring, etcd and seaweedfs hooks use: the deletion is requested and the claims go once their pods have stopped, which the log now says instead of "complete". The Job carries activeDeadlineSeconds 120 as the backstop for a kubectl that never returns and keeps only before-hook-creation, so a failed run and its log survive. The Role drops watch, which nothing needs once the wait is gone. Signed-off-by: Yan Bondarenko <[email protected]> Assisted-by: LLM
62d4a59 to
54ac8ed
Compare
Aleksei Sviridkin (lexfrei)
added
the
kind/backport
|
Successfully created backport PR for |
3 participants
What this PR does
Fixes #4086.
The OpenSearch operator provisions its data volumes through a StatefulSet, so the PVCs carry no ownerReference, and the
opensearchapp chart has no cleanup hook. Removing the application therefore leaves every data volumeBound, counting against the tenant's storage quota with nothing left in the namespace that could reclaim it.clickhouse,kafkaandharborremove their operator-owned PVCs in a post-delete hook.This adds a post-delete Job to
opensearchthat requests the deletion of the claims labelledopster.io/opensearch-cluster=<release>with--ignore-not-found --wait=falseand exits non-zero when the request fails, with its own ServiceAccount, a Role limited topersistentvolumeclaims(get,list,delete), theclastix/kubectlimage pinned by digest throughcozy-lib.image, a non-root restricted securityContext,activeDeadlineSeconds: 120so a kubectl that never returns cannot hold the uninstall, andbefore-hook-creationalone on the Job so a failed run and its log survive; the ServiceAccount, Role and RoleBinding keephook-succeeded.The delete does not wait for the claims to disappear, on purpose: OpenSearch data pods stop slowly, and a claim they still hold would keep a waiting hook running into helm-controller's 300s uninstall budget. The claims go once their pods have stopped, which is what the log says. This is the shape the monitoring, etcd and seaweedfs hooks use. Retention is not offered as a knob, consistent with the other charts; an opt-out can be added if the maintainers prefer one.
helm unittest: 64/64 in the chart, includingtests/cleanup_pvc_test.yaml(hook annotations and deadline, selector and exit path, digest pin and registry prefix, securityContext, Role rules, RoleBinding).helm templaterenders the Job, ServiceAccount, Role and RoleBinding next to the existing objects.Downstream repositories
A new hook template and its tests only: no values, schema, README or defaults change.
Release note
Summary by CodeRabbit
New Features
Tests