fix(clickhouse)!: clean up orphaned keeper and data PVCs on delete (#3057) - #3072
Conversation
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 ClickHouse data and keeper PVCs remained orphaned after deleting a ClickHouse application CR. By implementing a post-delete cleanup hook and ensuring all relevant PVCs are properly labeled, the system now guarantees that storage resources are reclaimed upon application deletion. Note that this change is destructive, as it ensures all associated data volumes are permanently removed when the application is deleted. 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a post-delete Helm hook to clean up orphaned PersistentVolumeClaims (PVCs) left behind by ClickHouse and ClickHouse Keeper. It adds the app.kubernetes.io/instance label to the ClickHouse Keeper volume claim templates, creates a cleanup Job along with its associated ServiceAccount, Role, and RoleBinding, and adds corresponding unit tests. Feedback on the changes suggests adding an explicit securityContext to the cleanup Job's Pod and container specifications to adhere to the repository's security guidelines and ensure the workload runs as a non-root user with minimal privileges.
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.
| spec: | ||
| serviceAccountName: {{ .Release.Name }}-cleanup | ||
| restartPolicy: Never | ||
| containers: | ||
| - name: cleanup | ||
| image: docker.io/clastix/kubectl:v1.32 |
There was a problem hiding this comment.
According to the repository style guide, workloads should not run with excessive privileges or as root without an explicit reason. Adding an explicit securityContext to both the Pod and the Container ensures the cleanup Job runs securely as a non-root user with minimal privileges.
spec:
serviceAccountName: {{ .Release.Name }}-cleanup
securityContext:
runAsNonRoot: true
runAsUser: 65534
runAsGroup: 65534
seccompProfile:
type: RuntimeDefault
restartPolicy: Never
containers:
- name: cleanup
image: docker.io/clastix/kubectl:v1.32
securityContext:
allowPrivilegeEscalation: false
capabilities:
drop:
- ALL
readOnlyRootFilesystem: trueReferences
- Flag missing securityContext, containers running as root without an explicit reason, and hostPath/hostNetwork usage without clear rationale. (link)
There was a problem hiding this comment.
Done — the cleanup Job carries an explicit securityContext at both pod and container level: pod runAsNonRoot: true, runAsUser/runAsGroup: 65534, seccompProfile: RuntimeDefault; container allowPrivilegeEscalation: false, readOnlyRootFilesystem: true, capabilities.drop: [ALL], with a writable /tmp emptyDir and HOME=/tmp. See the current template.
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ClickHouse keeper PVC cleanup and migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/apps/clickhouse/templates/hooks/cleanup-pvc.yaml`:
- Around line 28-30: The PVC cleanup hook is swallowing deletion failures by
forcing success after the kubectl delete in the cleanup-pvc template. Update the
cleanup logic so the hook does not use a success-override on the PVC deletion
path, and instead lets failures from kubectl delete surface and fail the Helm
hook when orphaned PVCs cannot be removed. Keep the behavior localized to the
cleanup-pvc hook and the delete command that targets app.kubernetes.io/instance
for the current release.
In `@packages/apps/clickhouse/tests/cleanup_pvc_test.yaml`:
- Around line 38-40: The regex assertion in the cleanup PVC test is too loose
because the label key dots are treated as wildcards. Update the matchRegex
pattern in the cleanup_pvc_test.yaml assertion so the selector for
app.kubernetes.io/instance is escaped properly, using the same command path
under spec.template.spec.containers[0].command[2], to ensure the test only
matches the exact label key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 969b4ca6-e029-44bf-987a-12c4100cc5ab
📒 Files selected for processing (3)
packages/apps/clickhouse/templates/chkeeper.yamlpackages/apps/clickhouse/templates/hooks/cleanup-pvc.yamlpackages/apps/clickhouse/tests/cleanup_pvc_test.yaml
3d1e111 to
9341fd8
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/apps/clickhouse/templates/hooks/cleanup-pvc.yaml (1)
43-44: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not mask cleanup command failures.
At Line 44,
|| echo ...turns realkubectl deletefailures into success, so uninstall can appear healthy while PVCs remain orphaned.Suggested fix
echo "Deleting orphaned PVCs for {{ .Release.Name }}..." kubectl delete pvc -n {{ .Release.Namespace }} -l app.kubernetes.io/instance={{ .Release.Name }} --ignore-not-found \ - || echo "WARNING: PVC cleanup failed for {{ .Release.Name }}; orphaned PVCs may remain" >&2 + echo "PVC cleanup complete."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/apps/clickhouse/templates/hooks/cleanup-pvc.yaml` around lines 43 - 44, The PVC cleanup in the hook currently masks real failures by chaining `kubectl delete` with `|| echo ...`, which can make uninstall look successful even when deletion fails. Update the cleanup logic in the PVC deletion hook to surface the `kubectl delete pvc` failure as a failure instead of swallowing it, while keeping any helpful logging separate from the command’s exit status. Refer to the cleanup command in the `cleanup-pvc` hook and preserve the existing `--ignore-not-found` behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/apps/clickhouse/templates/hooks/cleanup-pvc.yaml`:
- Around line 43-44: The PVC cleanup in the hook currently masks real failures
by chaining `kubectl delete` with `|| echo ...`, which can make uninstall look
successful even when deletion fails. Update the cleanup logic in the PVC
deletion hook to surface the `kubectl delete pvc` failure as a failure instead
of swallowing it, while keeping any helpful logging separate from the command’s
exit status. Refer to the cleanup command in the `cleanup-pvc` hook and preserve
the existing `--ignore-not-found` behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4a8efd7b-838b-44ca-946a-89065c7c85e5
📒 Files selected for processing (4)
packages/apps/clickhouse/Makefilepackages/apps/clickhouse/templates/chkeeper.yamlpackages/apps/clickhouse/templates/hooks/cleanup-pvc.yamlpackages/apps/clickhouse/tests/cleanup_pvc_test.yaml
✅ Files skipped from review due to trivial changes (1)
- packages/apps/clickhouse/Makefile
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/apps/clickhouse/tests/cleanup_pvc_test.yaml
- packages/apps/clickhouse/templates/chkeeper.yaml
9341fd8 to
85bb40a
Compare
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
NOT LGTM.
The label-based cleanup hook is well constructed, but the fix is incomplete for existing customers: keeper PVCs already provisioned before this PR have no app.kubernetes.io/instance label, the altinity clickhouse-keeper-operator does not retroactively re-stamp labels on existing PVCs when volumeClaimTemplates.metadata.labels change, so the new selector will miss them and the reported leak persists on delete. The PR body's WARNING acknowledges the destructive nature of the change but does not call out that pre-existing keeper PVCs remain orphaned — i.e. issue #3057 is closed only for fresh installs.
Findings
-
MAJOR — incomplete fix for existing releases (no label-backfill migration).
packages/apps/clickhouse/templates/chkeeper.yaml:67-69adds the release label to the keepervolumeClaimTemplates.metadata. The altinity clickhouse-keeper-operator (and StatefulSet-style controllers in general) only stampsvolumeClaimTemplates.metadata.labelsonto newly created PVCs; existing PVCs are not reconciled. After a customer upgrades to this chart, their keeper PVCdefault-chk-<release>-keeper-cluster1-0-0-0still has no label, the post-delete hook's selector-l app.kubernetes.io/instance=<release>misses it, and the keeper PVC continues to leak on delete (the exact bug #3057 claims to fix). The main data/log PVCs were already labeled before this PR, so for them the hook works in upgrade scenarios — but for the keeper PVC, which is specifically the one called out in the body, the fix only lands for releases created on this chart version or later. Suggested remediation: ship a numbered migration script underpackages/core/platform/images/migrations/migrations/<N>that, for every namespace containing aClickHouseKeeperInstallationowned by a Helm release,kubectl labels the matching keeper PVCs withapp.kubernetes.io/instance=<release>, then bumppackages/core/platform/values.yamlmigrations.targetVersionaccordingly. Alternatively, broaden the selector (e.g. delete PVCs by owner-reference to theClickHouseKeeperInstallation, or byclickhouse-keeper.altinity.com/chk: <release>-keeper) so labelless legacy PVCs are still picked up. -
MAJOR — hook always reports success even when PVC deletion fails.
packages/apps/clickhouse/templates/hooks/cleanup-pvc.yaml:43-44:kubectl delete pvc ... || echo "WARNING: ..." >&2masks the kubectl exit code; the trailingecho "PVC cleanup complete."then returns 0, so the Job always exits successfully. Combined withhelm.sh/hook-delete-policy: before-hook-creation,hook-succeeded, the Job + SA + Role + RoleBinding are deleted the moment cleanup "succeeds", so a partial-failure run (RBAC denial, transient apiserver error, finalizer stuck) is invisible: no surviving Job to inspect, no non-zero hook to surface in helm / helm-controller status, no event on the release. The intent to surface failures via>&2 echois there but the mechanism does not honour the exit code. Fix: capturekubectl's status andexit "$rc"after the warning, and switchhook-delete-policyto also retainhook-failedresources so failed runs leave artefacts to debug.
Claim mismatches
- "the keeper PVC previously had no release label … so the operator never stamped one and a single selector would have missed it — leaving the reported leak unfixed." — [PARTIAL] Correctly identifies the pre-PR gap but, by the same argument, the fix only works for keeper PVCs created on or after this chart version. Existing customer deployments retain unlabeled keeper PVCs and the leak persists for them. The release-note and WARNING admonition do not mention this. Suggest extending the release note: "Existing keeper PVCs from releases installed before this fix are not labeled retroactively and will still leak on delete; back-fill labels manually with
kubectl label pvc -n <ns> -l clickhouse-keeper.altinity.com/chk=<release>-keeper app.kubernetes.io/instance=<release>before deleting the release."
Operational risks
- The hook does not delete
Secrets,ConfigMaps, or other CRs owned by the release — only PVCs. If the altinity operators leave any other state behind (operator-stamped Secrets, monitoring CRs), those will continue to accumulate. Out of scope of this PR but worth confirming explicitly in the body before merging a breaking-change. helm.sh/hook-weightkeys are quoted inconsistently across the four manifests (Job/Role with quoted key, SA/RoleBinding unquoted attemplates/hooks/cleanup-pvc.yaml:10,68,79,94). Cosmetic, matches mariadb's same inconsistency.
Recommended follow-ups
- Add a numbered
packages/core/platform/images/migrations/migrations/<N>script that back-fillsapp.kubernetes.io/instance=<release>onto pre-existing keeper PVCs, and bumppackages/core/platform/values.yamlmigrations.targetVersion. Without this, #3057 stays open for every existing customer. - Honour
kubectl delete pvcexit code in the cleanup script and drophook-succeededfromhook-delete-policy(or keep it but also addhook-failedretention) so partial-failure runs are observable. - Update the release note and the
[!WARNING]admonition to call out that PVCs from releases installed before this version need manual relabeling for the keeper PVC to be cleaned up.
…eper PVC labels Address review on #3072: - Cleanup hook no longer masks kubectl delete failures: the command now fails (exit 1) when PVC deletion fails instead of swallowing the error with || echo. The hook-delete-policy drops hook-succeeded (keeps only before-hook-creation) so a failed cleanup Job survives for inspection. - Add migration 45 (45 -> 46) that backfills app.kubernetes.io/instance onto keeper PVCs created before this change; the altinity keeper operator never re-stamps volumeClaimTemplate labels onto existing PVCs, so without this the cleanup selector misses pre-upgrade keeper PVCs and #3057 persists for existing releases. Bump migrations.targetVersion to 46. - Unify helm.sh/hook-weight key quoting across the four hook manifests. - Extend the unittest to assert the new hook-delete-policy and exit-on-failure. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Алексей Артамонов <alexeyartamonov1987@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/apps/clickhouse/templates/hooks/cleanup-pvc.yaml`:
- Around line 5-19: The Helm template in cleanup-pvc.yaml is using raw templated
scalars that YAML tooling can’t parse reliably. Quote the templated values in
the manifest fields for the release name, the app.kubernetes.io/instance label
values, and the serviceAccountName so the source stays valid for YAML linting
while preserving the rendered output. Update the corresponding entries in the
cleanup hook template, including the metadata and pod template sections.
In `@packages/core/platform/images/migrations/migrations/45`:
- Around line 23-24: The migration in the image backfill script should no-op
when the ClickHouse keeper CRD is not installed. Update the `kubectl get
clickhousekeeperinstallations.clickhouse-keeper.altinity.com` lookup to first
check whether that API resource exists, and skip the backfill if it is
unavailable so the `set -euo pipefail` upgrade path does not abort. Keep the
change localized to the migration script logic that populates `chks`, preserving
the existing behavior when the CRD is present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f406d18b-42a0-4739-b568-af43b369abc9
📒 Files selected for processing (6)
packages/apps/clickhouse/Makefilepackages/apps/clickhouse/templates/chkeeper.yamlpackages/apps/clickhouse/templates/hooks/cleanup-pvc.yamlpackages/apps/clickhouse/tests/cleanup_pvc_test.yamlpackages/core/platform/images/migrations/migrations/45packages/core/platform/values.yaml
✅ Files skipped from review due to trivial changes (1)
- packages/apps/clickhouse/Makefile
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/apps/clickhouse/templates/chkeeper.yaml
- packages/apps/clickhouse/tests/cleanup_pvc_test.yaml
…k scalars Address CodeRabbit review on #3072: - Migration 45 now skips the backfill when the clickhousekeeperinstallations CRD is not installed. The migration runs platform-wide via targetVersion, so on clusters that never deployed ClickHouse the kubectl get would otherwise fail under set -euo pipefail and abort the whole upgrade. - Quote the templated name/label/serviceAccountName scalars in the cleanup hook so the source parses under YAMLlint; rendered output is unchanged. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Алексей Артамонов <alexeyartamonov1987@gmail.com>
|
Thanks for the thorough review, IvanHunters — both MAJOR findings are addressed in 6be0d86 (+ follow-up b17c43b). 1. Incomplete fix for existing releases (label backfill) — fixedAdded a numbered migration
2. Hook always reported success — fixedThe hook now fails on 3. Claim mismatch / release note — updatedThe PR body now has an "Existing releases (label backfill migration)" section and the release-note calls out that the migration relabels legacy keeper PVCs on upgrade. Cosmetic
Operational noteThe hook intentionally reclaims only PVCs of the release; other release state (Secrets/ConfigMaps) goes through the normal Helm uninstall path. Noted explicitly in the PR body. Verified end-to-end on a live cluster (dev9, ClickHouse keeper enabled)
Real keeper PVC labels confirmed the release mapping: PVC carries |
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
NOT LGTM
The chart-side cleanup hook and the keeper PVC label change are sound, but the migration half has become stale against main and would either fail to merge or land as a silent no-op for upgrading customers. Rebase + renumber required.
Findings
[CRITICAL] packages/core/platform/images/migrations/migrations/45 — migration number already taken on main; targetVersion is already 47 upstream.
main HEAD (4404c5c5) carries migrations/45 (Talos worker bootstrap, pins KubeadmConfigTemplate / KubeadmConfig against Helm prune) and migrations/46 (k8s v1.30 -> v1.31 bump). packages/core/platform/values.yaml is at targetVersion: 47. This PR adds its own 45 and sets targetVersion: 46. Two consequences: (1) packages/core/platform/values.yaml and migrations/45 produce hard merge conflicts (verified with git merge-tree); (2) even after a "take ours" conflict resolution, any cluster already past version 46 — i.e. anyone who installed/upgraded after the Talos rollover landed — will skip this backfill because run-migrations.sh:21 bails when CURRENT_VERSION >= TARGET_VERSION. The migration must be renumbered to the next free slot (currently 47) and targetVersion bumped to 48, or the existing-customer backfill story this PR justifies in the description silently fails for the very customers who already migrated past version 46.
[MAJOR] packages/core/platform/images/migrations/migrations/45:65-75 — does not route the version stamp through the shared lib/cozystack-version.sh helper; will fail the bats convention test once rebased.
Main introduced a shared helper (packages/core/platform/images/migrations/migrations/lib/cozystack-version.sh) and a convention-pinning bats suite (hack/cozystack-version-stamp.bats). The last @test in that suite walks every numeric migration file with N >= 42 and asserts both that it sources cozystack-version.sh AND that it calls render_cozystack_version_manifest / stamp_cozystack_version AND that no kubectl … (create configmap|apply).*cozystack-version line outside comments exists. The PR's inline kubectl apply --filename - <<EOF … name: cozystack-version … heredoc fails all three checks. After rebase the migration body needs to become roughly . "$(dirname "$0")/lib/cozystack-version.sh" at the top and stamp_cozystack_version 48 (or whichever the renumbered target ends up being) at the bottom.
[MINOR] packages/apps/clickhouse/templates/hooks/cleanup-pvc.yaml:7,90,107,127 — helm.sh/hook-delete-policy: before-hook-creation only; hook resources persist after a successful uninstall.
PR body justifies dropping hook-succeeded from the Job's policy ("a failed cleanup Job and its logs survive for debugging"). The reasoning is fine for the Job, but the same policy is applied to the ServiceAccount, Role, and RoleBinding — these also live forever after a successful uninstall, since before-hook-creation only fires on a future re-install of a release with the same name. After helm uninstall ch-foo succeeds, the namespace keeps ch-foo-cleanup SA + Role + RoleBinding + (succeeded) Job. In practice tenant teardowns also delete the namespace so the leak is bounded, and this PR is consistent within the release, but it does diverge from the precedent it cites (packages/apps/mariadb/templates/hooks/cleanup-pvc.yaml uses before-hook-creation,hook-succeeded on every resource). Either keep the Job at the new policy and put the SA/Role/RoleBinding back on before-hook-creation,hook-succeeded, or document explicitly in NOTES.txt / release-note that the hook artifacts are intentionally left for post-mortem.
Claim mismatches
[PARTIAL] "adds migration 45 (45 -> 46)" — migration number and target version are both behind main; description must be updated alongside the renumber.
[UNVERIFIABLE] "main ClickHouse data/log PVCs were already labeled before this PR and need no backfill" — verified for newly-rendered releases (templates/clickhouse.yaml:107-126 does set app.kubernetes.io/instance on both data-volume-template and log-volume-template), so the claim holds for installs that did Helm-side-rendering. Not verifiable from the repo alone for the historic case where these labels may have been added in an earlier chart commit — if any release was installed before those labels landed in the chart, those PVCs would also need backfill and migration 45 doesn't cover them. Worth a quick git log -p packages/apps/clickhouse/templates/clickhouse.yaml confirmation before merge that the data/log labels predate the oldest supported upgrade source version.
Operational risks
- Migration runner under
set -euo pipefail:kubectl get clickhousekeeperinstallations … -A(packages/core/platform/images/migrations/migrations/45:31-32) is the first command after the CRD-present branch. If the CRD is present but the apiserver is briefly unavailable mid-upgrade, kubectl fails, pipefail bubbles up, and the platform upgrade halts at this migration. Wrapping thekubectl getin a retry or|| true(consistent with how the innerkubectl get pvc … | grep -E … || trueis already protected) would make the backfill best-effort instead of fatal. Today this migration can take down a platform upgrade on a transient apiserver hiccup with no ClickHouse keeper PVCs to actually backfill.
Caveats
packages/apps/clickhouse/templates/hooks/cleanup-pvc.yaml:37pinsdocker.io/clastix/kubectl:v1.32by tag, not digest. This matches the existingmariadbprecedent so it is not a regression, but the image is mutable and a tag retag upstream would silently change the cleanup binary. Out of scope for this PR; fix together with the mariadb hook if/when image pinning is enforced repo-wide.chart_lintreportshelm templateerrors forpackages/apps/clickhouse("'default' is not a valid tenant identifier") andpackages/core/platform("OCIRepository not found"). Both are environmental (the linter renders intodefaultand without the platform repository CR materialized), not introduced by this PR.- Phase 5b A (existing customer upgrade): blocked by the targetVersion stale-number finding above; backfill won't run for any cluster past version 46.
- Phase 5b B (fresh install): no migrations executed at bootstrap, cleanup hook + labeled keeper
volumeClaimTemplatesship from day-one, behavior correct.
Recommended follow-ups
- After this PR lands, consider a second migration that also relabels the main ClickHouse data/log PVCs defensively, in case any historic releases predate the
app.kubernetes.io/instancelabels ondata-volume-template/log-volume-template. Cheap belt-and-braces; resolves the [UNVERIFIABLE] claim above. - A parallel chart change to
packages/apps/mariadb/templates/hooks/cleanup-pvc.yamlto add the inline PSS-restrictedsecurityContextblock this PR inlines on the ClickHouse Job (the MariaDB hook today omits it and only works because PSS-restricted admission in tenant namespaces does not retroactively enforce on existing pre-restrictedclusters — flag a separate audit issue).
…eper PVC labels Address review on #3072: - Cleanup hook no longer masks kubectl delete failures: the command now fails (exit 1) when PVC deletion fails instead of swallowing the error with || echo. The hook-delete-policy drops hook-succeeded (keeps only before-hook-creation) so a failed cleanup Job survives for inspection. - Add migration 45 (45 -> 46) that backfills app.kubernetes.io/instance onto keeper PVCs created before this change; the altinity keeper operator never re-stamps volumeClaimTemplate labels onto existing PVCs, so without this the cleanup selector misses pre-upgrade keeper PVCs and #3057 persists for existing releases. Bump migrations.targetVersion to 46. - Unify helm.sh/hook-weight key quoting across the four hook manifests. - Extend the unittest to assert the new hook-delete-policy and exit-on-failure. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Алексей Артамонов <alexeyartamonov1987@gmail.com>
…k scalars Address CodeRabbit review on #3072: - Migration 45 now skips the backfill when the clickhousekeeperinstallations CRD is not installed. The migration runs platform-wide via targetVersion, so on clusters that never deployed ClickHouse the kubectl get would otherwise fail under set -euo pipefail and abort the whole upgrade. - Quote the templated name/label/serviceAccountName scalars in the cleanup hook so the source parses under YAMLlint; rendered output is unchanged. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Алексей Артамонов <alexeyartamonov1987@gmail.com>
Address review [MINOR] on #3072: keep the cleanup Job on before-hook-creation (so a failed Job and its logs survive for post-mortem), but restore before-hook-creation,hook-succeeded on the ServiceAccount/Role/RoleBinding so those artifacts do not linger in the namespace after a successful uninstall. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Алексей Артамонов <alexeyartamonov1987@gmail.com>
b17c43b to
581f7d6
Compare
|
Thanks IvanHunters — rebased on latest [CRITICAL] Stale migration number / targetVersion — fixedRebased onto current [MAJOR] Version stamp must use the shared helper — fixedMigration . "$(dirname "$0")/lib/cozystack-version.sh"
...
stamp_cozystack_version 49The inline [MINOR] Hook artifacts lingering — fixedSplit the policy: the Job stays on [Operational] Transient apiserver error halting the upgrade — fixedThe keeper list now tolerates failure ( Claim mismatches
Re-verified end-to-end on a live cluster (dev9), renumbered migration
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/platform/images/migrations/migrations/48`:
- Around line 62-63: The PVC relabel step is still failing the migration under
set -e, which can stop the upgrade before stamp_cozystack_version 49 runs.
Update the relabel logic in the migration script around the kubectl label call
so it is best-effort like the earlier transient-failure handling: keep the
loop/labeling in place, but swallow or guard individual kubectl label failures
so a single API error does not abort the migration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5973c903-6f96-4187-94c3-16ae39f53edc
📒 Files selected for processing (6)
packages/apps/clickhouse/Makefilepackages/apps/clickhouse/templates/chkeeper.yamlpackages/apps/clickhouse/templates/hooks/cleanup-pvc.yamlpackages/apps/clickhouse/tests/cleanup_pvc_test.yamlpackages/core/platform/images/migrations/migrations/48packages/core/platform/values.yaml
✅ Files skipped from review due to trivial changes (3)
- packages/core/platform/values.yaml
- packages/apps/clickhouse/templates/chkeeper.yaml
- packages/apps/clickhouse/Makefile
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/apps/clickhouse/templates/hooks/cleanup-pvc.yaml
- packages/apps/clickhouse/tests/cleanup_pvc_test.yaml
| echo "Labeling PVC $ns/$pvc with app.kubernetes.io/instance=$release" | ||
| kubectl label pvc -n "$ns" "$pvc" "app.kubernetes.io/instance=$release" --overwrite |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep the relabel step best-effort too.
Lines 16-20 say a transient API failure must not abort the platform upgrade, but Line 63 still runs under set -e. If one kubectl label fails, the migration exits before stamp_cozystack_version 49, which turns a tolerated leak into an upgrade failure.
Suggested fix
echo "Labeling PVC $ns/$pvc with app.kubernetes.io/instance=$release"
- kubectl label pvc -n "$ns" "$pvc" "app.kubernetes.io/instance=$release" --overwrite
+ if ! kubectl label pvc -n "$ns" "$pvc" "app.kubernetes.io/instance=$release" --overwrite; then
+ echo "Failed to label PVC $ns/$pvc; continuing" >&2
+ fi
done
done📝 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.
| echo "Labeling PVC $ns/$pvc with app.kubernetes.io/instance=$release" | |
| kubectl label pvc -n "$ns" "$pvc" "app.kubernetes.io/instance=$release" --overwrite | |
| echo "Labeling PVC $ns/$pvc with app.kubernetes.io/instance=$release" | |
| if ! kubectl label pvc -n "$ns" "$pvc" "app.kubernetes.io/instance=$release" --overwrite; then | |
| echo "Failed to label PVC $ns/$pvc; continuing" >&2 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/platform/images/migrations/migrations/48` around lines 62 - 63,
The PVC relabel step is still failing the migration under set -e, which can stop
the upgrade before stamp_cozystack_version 49 runs. Update the relabel logic in
the migration script around the kubectl label call so it is best-effort like the
earlier transient-failure handling: keep the loop/labeling in place, but swallow
or guard individual kubectl label failures so a single API error does not abort
the migration.
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM
The post-delete cleanup hook and the migration-48 label backfill are correct, scoped, and consistent with the MariaDB precedent; the deliberate divergences (Job kept on failure, kubectl delete errors surfaced, version stamped via the shared helper) are improvements and are well-explained in the PR body. The breaking-change marker ! matches the new destructive-on-delete behaviour, which is called out plainly in the release note.
Caveats
- Migration 48's
kubectl label --overwritewill replace an existing differingapp.kubernetes.io/instancevalue on a candidate PVC (the value-equality check atpackages/core/platform/images/migrations/migrations/48:58skips only when the label already matches the derived release). In practice nothing else labels these PVCs, so the exposure is theoretical, but it is worth keeping in mind if a future tool starts stamping its own label. - The cleanup Job (and its pod) intentionally lingers in the namespace on success since
hook-delete-policyomitshook-succeeded(packages/apps/clickhouse/templates/hooks/cleanup-pvc.yaml:14). For tenants that uninstall/reinstall the same release in the same namespace, this is a non-issue becausebefore-hook-creationreaps the old Job on the next uninstall; for tenants that uninstall once and walk away, the artefact persists until the namespace is deleted. PR body documents this trade-off; flagging only so reviewers aren't surprised. .PHONY: testwas added topackages/apps/clickhouse/Makefilebut the sibling app Makefiles (mariadb,postgres,kafka) do not declare it. Harmless drift; consider aligning across apps in a follow-up if the project cares about consistency.
…3057) After deleting a ClickHouse app CR, the keeper PVC and the main data/log PVCs were left orphaned in the tenant namespace, consuming storage permanently. The clickhouse-keeper-operator does not delete data PVCs on CR removal (data-safety default) and the CRD exposes no persistentVolumeClaimRetentionPolicy equivalent. Add a post-delete cleanup hook (modeled on the MariaDB hook) that deletes every PVC carrying the release label app.kubernetes.io/instance=<release>. Also stamp that label on the keeper volumeClaimTemplates in chkeeper.yaml: the keeper PVC previously had no release label (the chart labelled only the keeper pod template, not the PVC), so a single selector would have missed the very PVC the issue reports. BREAKING CHANGE: deleting a ClickHouse app now permanently removes all of its PVCs (keeper + data + logs) and the data they hold. Nothing is left behind to recover from. This matches the existing MariaDB cleanup-hook behaviour and the general cozystack model where deleting a managed app reclaims its storage. Signed-off-by: Алексей Артамонов <aleksei.artamonov@aenix.io>
…eper PVC labels Address review on #3072: - Cleanup hook no longer masks kubectl delete failures: the command now fails (exit 1) when PVC deletion fails instead of swallowing the error with || echo. The hook-delete-policy drops hook-succeeded (keeps only before-hook-creation) so a failed cleanup Job survives for inspection. - Add migration 45 (45 -> 46) that backfills app.kubernetes.io/instance onto keeper PVCs created before this change; the altinity keeper operator never re-stamps volumeClaimTemplate labels onto existing PVCs, so without this the cleanup selector misses pre-upgrade keeper PVCs and #3057 persists for existing releases. Bump migrations.targetVersion to 46. - Unify helm.sh/hook-weight key quoting across the four hook manifests. - Extend the unittest to assert the new hook-delete-policy and exit-on-failure. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Алексей Артамонов <alexeyartamonov1987@gmail.com>
…k scalars Address CodeRabbit review on #3072: - Migration 45 now skips the backfill when the clickhousekeeperinstallations CRD is not installed. The migration runs platform-wide via targetVersion, so on clusters that never deployed ClickHouse the kubectl get would otherwise fail under set -euo pipefail and abort the whole upgrade. - Quote the templated name/label/serviceAccountName scalars in the cleanup hook so the source parses under YAMLlint; rendered output is unchanged. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Алексей Артамонов <alexeyartamonov1987@gmail.com>
Address review [MINOR] on #3072: keep the cleanup Job on before-hook-creation (so a failed Job and its logs survive for post-mortem), but restore before-hook-creation,hook-succeeded on the ServiceAccount/Role/RoleBinding so those artifacts do not linger in the namespace after a successful uninstall. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Алексей Артамонов <alexeyartamonov1987@gmail.com>
581f7d6 to
7faa325
Compare
…nd route it through cozy-lib.images-registry (#3171) ## What this PR does Fixes #3088. The post-delete cleanup Jobs in the **harbor** and **mariadb** charts hardcoded the cleanup image as `docker.io/clastix/kubectl:v1.32` — a moving Docker Hub tag. This: - **broke reproducibility / was a supply-chain risk** — `v1.32` is mutable and can be re-pushed to a different image; - **could not be satisfied on air-gapped / mirrored clusters** — the hardcoded `docker.io` bypassed the cluster images registry, and it failed at the worst moment: during **uninstall**, when the `post-delete` hook runs and a stuck Job stalls release teardown. ### Changes - **New canonical helper `cozy-lib.image`** (`packages/library/cozy-lib/templates/_cozyconfig.tpl`). Given a registry-relative image path and the global context, it prefixes `cozy-lib.images-registry` when set (`<registry>/<image>`) and returns the image unchanged when empty — **with no leading `/`**, so standard installs still resolve from the default registry. This establishes the previously-undefined "how to reference a cluster image" pattern (`images-registry` was defined but never consumed). - **Digest-pinned image** in both cleanup hooks: `clastix/kubectl:v1.32@sha256:b9ef7d8dbe65bcc81a46c09b8dc7543103055021c4f43287bf59e92a8f4fe05c` (the immutable multi-arch OCI index for the `v1.32` tag; the same digest already pinned for `postgres-operator`'s webhook hook). - **Renovate keeps the pin fresh.** The repo disables the helm-values manager (`enabledManagers`), and the ref is assembled at render time by the helper rather than being a Dockerfile / go.mod dependency, so the built-in managers can't see it. A `custom.regex` manager over the hook templates tracks `clastix/kubectl` by tag+digest — mirroring the existing `extra/etcd` manager — so the supply-chain pin doesn't silently age. - **Hardened `cozy-lib.images-registry`** against a nil `.Values._cluster` so the helper never crashes the render — relevant precisely because this hook renders during teardown. - **Helm unit tests** in both charts assert the rendered image for three cases: empty registry (default), registry set, and the no-leading-slash edge case. Assertions match by **pattern** (routed + digest-pinned + no leading slash) rather than an exact digest, so a Renovate bump keeps them green. ### Verification - `helm unittest` passes for both charts (harbor: 16, mariadb: 10). - Real `helm template` renders confirmed for both charts: - empty registry → `clastix/kubectl:v1.32@sha256:b9ef…` - `images-registry=registry.internal:5000` → `registry.internal:5000/clastix/kubectl:v1.32@sha256:b9ef…` - The Renovate `matchStrings` regex verified to capture `currentValue=v1.32` and the digest from both hook templates. ### Note on the wider footprint The same `docker.io/clastix/kubectl:v1.32` hardcode is already merged on `main` in several other cleanup hooks (bucket, qdrant, gateway, seaweedfs, kubernetes, tenant, dashboard, keycloak-configure) and appears in the open PRs #3170 (etcd), #3094 (monitoring), #3072 (clickhouse). This PR fixes harbor + mariadb per the issue and establishes the `cozy-lib.image` + Renovate pattern; migrating the remaining hooks is a follow-up. ### Release note ```release-note fix(cozy-lib,harbor,mariadb): digest-pin the harbor and mariadb post-delete cleanup-hook kubectl image and route it through the cluster images registry (`cozy-lib.images-registry`) via the new `cozy-lib.image` helper, so mirrored / air-gapped installs can resolve it and uninstall no longer depends on a moving Docker Hub tag; a custom Renovate manager keeps the digest fresh ``` 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Updated post-delete cleanup hook Jobs to use digest-pinned container images for more secure, repeatable execution. * Improved image reference rendering so cleanup hooks work correctly with or without an image registry configured, avoiding invalid leading/trailing slash issues. * **Tests** * Extended cleanup hook rendering tests to validate digest pinning and correct registry prefix behavior, including edge cases. * **Chores** * Enhanced automated dependency detection to treat digest-pinned hook image references as stable. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…ged-by Re-review on #3094 (IvanHunters): the AND selector kept managed-by=vm-operator, which on the shipped vm-operator v0.68.4 is not reliably present on the storage PVCs (it lives on the StatefulSet selector/pod labels, not the claim template), making the delete fragile. Match by apps.cozystack.io/application.name=<release>-system alone: that label is stamped on the VM/VL claim templates (67b087a) and copied onto the PVCs by IntoSTSVolume even on v0.68.4, is already release-scoped, and is absent from the CNPG db PVCs (grafana-db/alerta-db carry only policy.cozystack.io + cnpg.io/*), so those stay untouched. Regression guard now forbids re-adding managed-by to the selector. Also apply the review's discipline items: the cleanup Job keeps only before-hook-creation (a Failed run survives for post-mortem; hook-succeeded stays on SA/Role/RoleBinding, cf. #3072) and sets backoffLimit: 0 (best-effort, no retry). Signed-off-by: Алексей Артамонов <aleksei.artamonov@aenix.io> Assisted-By: Claude <noreply@anthropic.com>
…ge PVCs Re-review on #3094 (IvanHunters, MAJOR): the volumeClaimTemplate label lands on PVCs only at creation — the StatefulSet controller never re-labels existing PVCs and vm-operator's PVC reconcile is resize-only. So on an already-deployed cluster (exactly the #3091 population) the pre-upgrade VM/VL storage PVCs stay unlabeled and the cleanup hook's selector misses them. Add migration 49 (mirrors the ClickHouse-keeper backfill in migration 48 for the sibling #3072). It relabels the pre-existing vmstorage/vmselect/vlstorage PVCs of every cozystack-managed VMCluster/VLCluster with apps.cozystack.io/application.name read from the CR's own spec.managedMetadata, so only monitoring's clusters are touched (a user's own VMCluster carries no such label) and the value always matches what the cleanup hook selects. Best-effort like #48: a skipped relabel is no worse than the pre-existing leak, so transient apiserver errors never abort the platform upgrade. Bump platform targetVersion 49 -> 50. Also correct the misleading comments (IvanHunters MINOR): spec.managedMetadata is never applied to PVCs by vm-operator at any current version, so the claim-template label is the only mechanism — do not imply a v0.71.0 crossover. Signed-off-by: Алексей Артамонов <aleksei.artamonov@aenix.io> Assisted-By: Claude <noreply@anthropic.com>
…ged-by Re-review on #3094 (IvanHunters): the AND selector kept managed-by=vm-operator, which on the shipped vm-operator v0.68.4 is not reliably present on the storage PVCs (it lives on the StatefulSet selector/pod labels, not the claim template), making the delete fragile. Match by apps.cozystack.io/application.name=<release>-system alone: that label is stamped on the VM/VL claim templates (67b087a) and copied onto the PVCs by IntoSTSVolume even on v0.68.4, is already release-scoped, and is absent from the CNPG db PVCs (grafana-db/alerta-db carry only policy.cozystack.io + cnpg.io/*), so those stay untouched. Regression guard now forbids re-adding managed-by to the selector. Also apply the review's discipline items: the cleanup Job keeps only before-hook-creation (a Failed run survives for post-mortem; hook-succeeded stays on SA/Role/RoleBinding, cf. #3072) and sets backoffLimit: 0 (best-effort, no retry). Signed-off-by: Алексей Артамонов <aleksei.artamonov@aenix.io> Assisted-By: Claude <noreply@anthropic.com>
…ge PVCs Re-review on #3094 (IvanHunters, MAJOR): the volumeClaimTemplate label lands on PVCs only at creation — the StatefulSet controller never re-labels existing PVCs and vm-operator's PVC reconcile is resize-only. So on an already-deployed cluster (exactly the #3091 population) the pre-upgrade VM/VL storage PVCs stay unlabeled and the cleanup hook's selector misses them. Add migration 49 (mirrors the ClickHouse-keeper backfill in migration 48 for the sibling #3072). It relabels the pre-existing vmstorage/vmselect/vlstorage PVCs of every cozystack-managed VMCluster/VLCluster with apps.cozystack.io/application.name read from the CR's own spec.managedMetadata, so only monitoring's clusters are touched (a user's own VMCluster carries no such label) and the value always matches what the cleanup hook selects. Best-effort like #48: a skipped relabel is no worse than the pre-existing leak, so transient apiserver errors never abort the platform upgrade. Bump platform targetVersion 49 -> 50. Also correct the misleading comments (IvanHunters MINOR): spec.managedMetadata is never applied to PVCs by vm-operator at any current version, so the claim-template label is the only mechanism — do not imply a v0.71.0 crossover. Signed-off-by: Алексей Артамонов <aleksei.artamonov@aenix.io> Assisted-By: Claude <noreply@anthropic.com>
…ged-by Re-review on #3094 (IvanHunters): the AND selector kept managed-by=vm-operator, which on the shipped vm-operator v0.68.4 is not reliably present on the storage PVCs (it lives on the StatefulSet selector/pod labels, not the claim template), making the delete fragile. Match by apps.cozystack.io/application.name=<release>-system alone: that label is stamped on the VM/VL claim templates (67b087a) and copied onto the PVCs by IntoSTSVolume even on v0.68.4, is already release-scoped, and is absent from the CNPG db PVCs (grafana-db/alerta-db carry only policy.cozystack.io + cnpg.io/*), so those stay untouched. Regression guard now forbids re-adding managed-by to the selector. Also apply the review's discipline items: the cleanup Job keeps only before-hook-creation (a Failed run survives for post-mortem; hook-succeeded stays on SA/Role/RoleBinding, cf. #3072) and sets backoffLimit: 0 (best-effort, no retry). Signed-off-by: Алексей Артамонов <aleksei.artamonov@aenix.io> Assisted-By: Claude <noreply@anthropic.com>
…ge PVCs Re-review on #3094 (IvanHunters, MAJOR): the volumeClaimTemplate label lands on PVCs only at creation — the StatefulSet controller never re-labels existing PVCs and vm-operator's PVC reconcile is resize-only. So on an already-deployed cluster (exactly the #3091 population) the pre-upgrade VM/VL storage PVCs stay unlabeled and the cleanup hook's selector misses them. Add migration 49 (mirrors the ClickHouse-keeper backfill in migration 48 for the sibling #3072). It relabels the pre-existing vmstorage/vmselect/vlstorage PVCs of every cozystack-managed VMCluster/VLCluster with apps.cozystack.io/application.name read from the CR's own spec.managedMetadata, so only monitoring's clusters are touched (a user's own VMCluster carries no such label) and the value always matches what the cleanup hook selects. Best-effort like #48: a skipped relabel is no worse than the pre-existing leak, so transient apiserver errors never abort the platform upgrade. Bump platform targetVersion 49 -> 50. Also correct the misleading comments (IvanHunters MINOR): spec.managedMetadata is never applied to PVCs by vm-operator at any current version, so the claim-template label is the only mechanism — do not imply a v0.71.0 crossover. Signed-off-by: Алексей Артамонов <aleksei.artamonov@aenix.io> Assisted-By: Claude <noreply@anthropic.com>
…ged-by Re-review on #3094 (IvanHunters): the AND selector kept managed-by=vm-operator, which on the shipped vm-operator v0.68.4 is not reliably present on the storage PVCs (it lives on the StatefulSet selector/pod labels, not the claim template), making the delete fragile. Match by apps.cozystack.io/application.name=<release>-system alone: that label is stamped on the VM/VL claim templates (67b087a) and copied onto the PVCs by IntoSTSVolume even on v0.68.4, is already release-scoped, and is absent from the CNPG db PVCs (grafana-db/alerta-db carry only policy.cozystack.io + cnpg.io/*), so those stay untouched. Regression guard now forbids re-adding managed-by to the selector. Also apply the review's discipline items: the cleanup Job keeps only before-hook-creation (a Failed run survives for post-mortem; hook-succeeded stays on SA/Role/RoleBinding, cf. #3072) and sets backoffLimit: 0 (best-effort, no retry). Signed-off-by: Алексей Артамонов <aleksei.artamonov@aenix.io> Assisted-By: Claude <noreply@anthropic.com>
…ge PVCs Re-review on #3094 (IvanHunters, MAJOR): the volumeClaimTemplate label lands on PVCs only at creation — the StatefulSet controller never re-labels existing PVCs and vm-operator's PVC reconcile is resize-only. So on an already-deployed cluster (exactly the #3091 population) the pre-upgrade VM/VL storage PVCs stay unlabeled and the cleanup hook's selector misses them. Add migration 49 (mirrors the ClickHouse-keeper backfill in migration 48 for the sibling #3072). It relabels the pre-existing vmstorage/vmselect/vlstorage PVCs of every cozystack-managed VMCluster/VLCluster with apps.cozystack.io/application.name read from the CR's own spec.managedMetadata, so only monitoring's clusters are touched (a user's own VMCluster carries no such label) and the value always matches what the cleanup hook selects. Best-effort like #48: a skipped relabel is no worse than the pre-existing leak, so transient apiserver errors never abort the platform upgrade. Bump platform targetVersion 49 -> 50. Also correct the misleading comments (IvanHunters MINOR): spec.managedMetadata is never applied to PVCs by vm-operator at any current version, so the claim-template label is the only mechanism — do not imply a v0.71.0 crossover. Signed-off-by: Алексей Артамонов <aleksei.artamonov@aenix.io> Assisted-By: Claude <noreply@anthropic.com>
What this PR does
After deleting a
ClickHouseapp CR, data PVCs were left orphaned in the tenantnamespace and consumed storage permanently:
default-chk-<release>-keeper-cluster1-0-0-0, backing aClickHouseKeeperInstallationreconciled by the clickhouse-keeper-operator;The keeper operator does not delete data PVCs when the CR is removed (data-safety
default) and the CRD exposes no
persistentVolumeClaimRetentionPolicyequivalent,so the volumes leaked.
This adds a
post-deletecleanup hook (modeled on the existing MariaDB hook atpackages/apps/mariadb/templates/hooks/cleanup-pvc.yaml) that deletes every PVCcarrying the release label
app.kubernetes.io/instance=<release>. It also addsthat label to the keeper
volumeClaimTemplatesinchkeeper.yaml: the keeperPVC previously had no release label (the chart set labels only on the keeper
pod template, not the PVC), so the operator never stamped one and a single
selector would have missed it — leaving the reported leak unfixed.
Existing releases (label backfill migration)
The altinity clickhouse-keeper-operator only stamps
volumeClaimTemplates.metadata.labelsonto PVCs it creates after the chart change; it never re-labels PVCs that
already exist. So for releases installed before this fix, the keeper PVC stays
unlabeled and the cleanup selector would still miss it. To close #3057 for
existing customers too, this PR adds migration
48(48 -> 49,packages/core/platform/images/migrations/migrations/48) that back-fillsapp.kubernetes.io/instance=<release>onto pre-existing keeper PVCs, and bumpsmigrations.targetVersionto49. The migration:lib/cozystack-version.shhelper(
stamp_cozystack_version 49), per thecozystack-version-stamp.batsconvention;on the keeper list (
|| true) so it cannot abort a platform-wide upgrade — thebackfill is best-effort (worst case is the pre-existing leak, no worse than today);
The main ClickHouse data/log PVCs have carried
app.kubernetes.io/instancesince2025-06-03 (well before the migration era), so they need no backfill.
Hook failure handling
The cleanup hook does not mask failures:
kubectl deletefailing makes theJob exit non-zero. The Job uses
helm.sh/hook-delete-policy: before-hook-creationonly (no
hook-succeeded) so a failed cleanup Job and its logs survive fordebugging; the ServiceAccount/Role/RoleBinding keep
before-hook-creation,hook-succeededso the RBAC artifacts are reclaimed after asuccessful uninstall instead of lingering in the namespace.
Scope
The hook intentionally reclaims only PVCs of the release. Other release
state (Secrets, ConfigMaps) is owned/garbage-collected through the normal Helm
uninstall path and is out of scope here.
Warning
This is destructive — deleting a ClickHouse app now permanently deletes its data.
The hook removes all PVCs of the release: the keeper PVC and the main
ClickHouse data and log PVCs. After deletion nothing is left behind to recover
data from. This is intentional and matches the existing MariaDB cleanup-hook
behaviour and the general cozystack model (deleting a managed app reclaims its
storage). Back up before deleting.
Fixes #3057.
Release note