fix(monitoring)!: clean up orphaned storage PVCs and TLS secrets on tenant module delete (#3091) - #3094
Conversation
|
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 a post-delete Helm cleanup hook for monitoring that removes vm-operator PVCs and two TLS secrets, with dedicated RBAC and Helm unittest coverage. A new ChangesMonitoring cleanup hook
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels: 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 |
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 resource leakage in the monitoring module by introducing a post-delete cleanup mechanism. When the module is uninstalled, the system now automatically garbage-collects orphaned storage PVCs and TLS secrets that were previously left behind. This change ensures a cleaner teardown process while maintaining security best practices through a hardened Job configuration. 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 cleanup hook for the monitoring package to remove orphaned VM PVCs and TLS secrets, along with a corresponding test suite. The reviewer feedback highlights that the RBAC Role requires the deletecollection verb to delete PVCs using a label selector, and recommends adding the --wait=false flag to the kubectl delete commands to prevent the Helm hook from blocking indefinitely on active finalizers. The test assertions should also be updated to reflect these adjustments.
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.
| rules: | ||
| - apiGroups: [""] | ||
| resources: ["persistentvolumeclaims"] | ||
| verbs: ["get", "list", "delete"] |
There was a problem hiding this comment.
The kubectl delete pvc -l managed-by=vm-operator command uses a label selector, which performs a bulk deletion on the collection of PVCs. In Kubernetes RBAC, deleting resources via a label selector requires the deletecollection verb. Without it, the command will fail with a 403 Forbidden error.
verbs: ["get", "list", "delete", "deletecollection"]| kubectl delete pvc -n {{ .Release.Namespace }} -l managed-by=vm-operator --ignore-not-found \ | ||
| || echo "WARNING: VM storage PVC cleanup failed for {{ .Release.Name }}; orphaned PVCs may remain" >&2 | ||
| kubectl delete secret -n {{ .Release.Namespace }} alerta-tls grafana-ingress-tls --ignore-not-found \ | ||
| || echo "WARNING: TLS secret cleanup failed for {{ .Release.Name }}; orphaned secrets may remain" >&2 |
There was a problem hiding this comment.
By default, kubectl delete waits for the resources to be fully deleted before returning. If any of the PVCs have active finalizers (such as kubernetes.io/pvc-protection while the monitoring pods are still terminating), the kubectl delete command will block indefinitely. Since this is a Helm post-delete hook, a blocked command will cause the entire Helm uninstall process to hang and eventually time out.
Adding the --wait=false flag ensures that the deletion request is submitted asynchronously, preventing the hook from blocking the teardown process.
kubectl delete pvc -n {{ .Release.Namespace }} -l managed-by=vm-operator --ignore-not-found --wait=false \
|| echo "WARNING: VM storage PVC cleanup failed for {{ .Release.Name }}; orphaned PVCs may remain" >&2
kubectl delete secret -n {{ .Release.Namespace }} alerta-tls grafana-ingress-tls --ignore-not-found --wait=false \
|| echo "WARNING: TLS secret cleanup failed for {{ .Release.Name }}; orphaned secrets may remain" >&2| - contains: | ||
| path: rules[0].verbs | ||
| content: delete |
There was a problem hiding this comment.
Since we added deletecollection to the Role verbs, let's update the test assertion to verify that deletecollection is present.
- contains:
path: rules[0].verbs
content: delete
- contains:
path: rules[0].verbs
content: deletecollection| - matchRegex: | ||
| path: spec.template.spec.containers[0].command[2] | ||
| pattern: kubectl delete pvc -n tenant-root -l managed-by=vm-operator --ignore-not-found |
There was a problem hiding this comment.
Update the test assertion pattern to match the command with the --wait=false flag.
- matchRegex:
path: spec.template.spec.containers[0].command[2]
pattern: kubectl delete pvc -n tenant-root -l managed-by=vm-operator --ignore-not-found --wait=false| - matchRegex: | ||
| path: spec.template.spec.containers[0].command[2] | ||
| pattern: kubectl delete secret -n tenant-root alerta-tls grafana-ingress-tls --ignore-not-found |
There was a problem hiding this comment.
Update the test assertion pattern to match the command with the --wait=false flag.
- matchRegex:
path: spec.template.spec.containers[0].command[2]
pattern: kubectl delete secret -n tenant-root alerta-tls grafana-ingress-tls --ignore-not-found --wait=falseThere 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/extra/monitoring/templates/hooks/cleanup.yaml`:
- Around line 46-53: The cleanup hook command is blocking on `kubectl delete
pvc` because it waits for PVC finalizers by default. Update the cleanup logic in
the hook template to make the PVC deletion non-blocking by adding `--wait=false`
or a short `--timeout` to the `kubectl delete pvc` invocation, while keeping the
existing warning fallback; leave the TLS secret deletion behavior unchanged
unless it has the same issue.
🪄 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: 9e408433-90b8-4862-b32e-c17a2325f8b5
📒 Files selected for processing (3)
packages/extra/monitoring/Makefilepackages/extra/monitoring/templates/hooks/cleanup.yamlpackages/extra/monitoring/tests/cleanup_test.yaml
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM — the approach and the critical label/name correctness are solid, but the cleanup hook can hang the uninstall (defeating its own "never block teardown" goal), and the destructive change is missing the breaking-change marker its sibling used.
Blocker — cleanup hook can hang teardown. templates/hooks/cleanup.yaml:50 runs kubectl delete pvc … --ignore-not-found with the default --wait=true. A post-delete hook can run while the vmstorage/vmselect/vlstorage pods are still terminating; the kubernetes.io/pvc-protection finalizer keeps the PVCs in Terminating and the delete blocks. The Job has no activeDeadlineSeconds, and || echo … catches a non-zero exit but not a hang, so a stuck delete stalls the HelmRelease deletion — contradicting the stated "Failures are logged but never block teardown". Please add --wait=false to both deletes (and/or activeDeadlineSeconds on the Job).
Breaking-change marker. Disabling monitoring now permanently deletes its VictoriaMetrics/VictoriaLogs storage and all metrics/logs history. The sibling storage-GC change used fix(clickhouse)!:; for semver consistency this should be fix(monitoring)!:. The WARNING block and release-note are good — only the ! is missing.
Test hardening. The suite pins the exact safe commands, which is great. Please also add a negative assertion (e.g. notMatchRegex) that the PVC delete stays label-scoped (no --all/unscoped sweep) and that the secret rule stays name-scoped.
Verified and correct: the managed-by=vm-operator selector does match the storage PVCs — the operator sets it on the StatefulSet selector.matchLabels, and the StatefulSet controller copies those onto the volumeClaimTemplate PVCs. Scope is release-safe (CNPG PVCs carry cnpg.io/* labels). Secret names alerta-tls/grafana-ingress-tls are name-scoped via resourceNames. Hook ordering, RBAC scoping, and pod hardening are correct. Note: the deletecollection verb is not needed — kubectl delete -l lists then deletes individually.
…cope Address review on #3094: - Add --wait=false to both kubectl delete calls so PVC finalizers (pvc-protection on pods still terminating) can never block the post-delete hook and stall the HelmRelease teardown. - Add activeDeadlineSeconds: 120 on the cleanup Job as a backstop. - Tests: assert the non-blocking flag, plus negative assertions that the PVC delete stays label-scoped (no --all) and the secret delete stays name-scoped (no -l/--all). deletecollection RBAC verb intentionally not added: kubectl delete -l lists then deletes individually, so the existing delete verb suffices. BREAKING CHANGE: disabling monitoring now permanently removes its VictoriaMetrics/VictoriaLogs storage PVCs and the metrics/logs they hold. Signed-off-by: Алексей Артамонов <aleksei.artamonov@aenix.io> Assisted-By: Claude <noreply@anthropic.com>
|
Thanks for the thorough review, Aleksei Sviridkin (@lexfrei) — all three points addressed in 7b4739b. Blocker (hook can hang teardown). Added Breaking-change marker. PR title is now Test hardening. Added negative assertions ( On the bot suggestion: agreed with your note — |
…cope Address review on #3094: - Add --wait=false to both kubectl delete calls so PVC finalizers (pvc-protection on pods still terminating) can never block the post-delete hook and stall the HelmRelease teardown. - Add activeDeadlineSeconds: 120 on the cleanup Job as a backstop. - Tests: assert the non-blocking flag, plus negative assertions that the PVC delete stays label-scoped (no --all) and the secret delete stays name-scoped (no -l/--all). deletecollection RBAC verb intentionally not added: kubectl delete -l lists then deletes individually, so the existing delete verb suffices. BREAKING CHANGE: disabling monitoring now permanently removes its VictoriaMetrics/VictoriaLogs storage PVCs and the metrics/logs they hold. Signed-off-by: Алексей Артамонов <aleksei.artamonov@aenix.io> Assisted-By: Claude <noreply@anthropic.com>
7b4739b to
515e45a
Compare
…cope Address review on #3094: - Add --wait=false to both kubectl delete calls so PVC finalizers (pvc-protection on pods still terminating) can never block the post-delete hook and stall the HelmRelease teardown. - Add activeDeadlineSeconds: 120 on the cleanup Job as a backstop. - Tests: assert the non-blocking flag, plus negative assertions that the PVC delete stays label-scoped (no --all) and the secret delete stays name-scoped (no -l/--all). deletecollection RBAC verb intentionally not added: kubectl delete -l lists then deletes individually, so the existing delete verb suffices. BREAKING CHANGE: disabling monitoring now permanently removes its VictoriaMetrics/VictoriaLogs storage PVCs and the metrics/logs they hold. Signed-off-by: Алексей Артамонов <aleksei.artamonov@aenix.io> Assisted-By: Claude <noreply@anthropic.com>
515e45a to
32312a7
Compare
…cope Address review on #3094: - Add --wait=false to both kubectl delete calls so PVC finalizers (pvc-protection on pods still terminating) can never block the post-delete hook and stall the HelmRelease teardown. - Add activeDeadlineSeconds: 120 on the cleanup Job as a backstop. - Tests: assert the non-blocking flag, plus negative assertions that the PVC delete stays label-scoped (no --all) and the secret delete stays name-scoped (no -l/--all). deletecollection RBAC verb intentionally not added: kubectl delete -l lists then deletes individually, so the existing delete verb suffices. BREAKING CHANGE: disabling monitoring now permanently removes its VictoriaMetrics/VictoriaLogs storage PVCs and the metrics/logs they hold. Signed-off-by: Алексей Артамонов <aleksei.artamonov@aenix.io> Assisted-By: Claude <noreply@anthropic.com>
32312a7 to
ff0bb1c
Compare
IvanHunters
left a comment
There was a problem hiding this comment.
Review: request changes (one blocking issue)
The hook is otherwise correct and the breaking-change marker is justified and documented, but the PVC label selector can delete storage that belongs to a different release, so I am flagging that as blocking.
Blocking
- Release-agnostic PVC selector.
kubectl delete pvc -l managed-by=vm-operator -n <ns>(templates/hooks/cleanup.yaml) matches VM PVCs of any VMCluster/VLCluster in the namespace, regardless of which release created them (verified against vm-operator v0.68.4ClusterSelectorLabels(), which sets the baremanaged-by: vm-operator, propagated to PVCs via the StatefulSet selector). If a namespace ever hosts more than one monitoring release, tearing one down would permanently delete the other's VictoriaMetrics/VictoriaLogs PVCs. The merged seaweedfs hook avoids this by scoping toapp.kubernetes.io/instance={{ .Release.Name }}-system. Please scope this selector to the release as well.
Verified (otherwise correct)
- CNPG (
cnpg.io/*), etcd, and seaweedfs PVCs use different label schemes and carry no baremanaged-by; CNPG PVCs (alerta-db-*,grafana-db-*) are correctly excluded. - Blast radius is contained to the tenant namespace via a namespaced Role (PVC get/list/delete plus two named secrets get/delete), not a ClusterRole.
kubectl delete -lperforms LIST + per-object DELETE, so the grantedget, list, deleteverbs suffice; nodeletecollectionverb is needed.- Best-effort teardown (
--wait=false, errors to stderr,activeDeadlineSeconds: 120) cannot stall HelmRelease teardown. Hook weights 0/5/10 (SA, RBAC, Job) order correctly. Hardened pod (non-root 65534, read-only rootfs, dropped caps, seccomp RuntimeDefault).helm unittestpasses (6 tests).
Breaking-change marker: justified and documented. The ! type, the PR-body WARNING, the release-note WARNING, and the BREAKING CHANGE: commit footer are all present and accurate.
Non-blocking
Address IvanHunters review on #3094: the PVC delete selector was release-agnostic (-l managed-by=vm-operator), matching the VM storage PVCs of every VMCluster/VLCluster in the namespace. If a namespace ever hosted more than one monitoring release, tearing one down would delete the other's VictoriaMetrics/VictoriaLogs storage. Scope the selector to this release via apps.cozystack.io/application.name= <release>-system, the ownership label the monitoring chart stamps on the VM CRs' managedMetadata and vm-operator propagates onto the PVCs — mirroring the merged seaweedfs hook's release scoping. deletecollection is still not needed: kubectl delete -l lists then deletes individually. Tests: assert the release-scoped selector and add a regression guard that the delete never falls back to the bare managed-by=vm-operator selector. Signed-off-by: Алексей Артамонов <aleksei.artamonov@aenix.io> Assisted-By: Claude <noreply@anthropic.com>
|
Thanks IvanHunters — the MAJOR (pre-existing PVCs never relabeled on upgrade) is now handled in Migration 49 (
MINOR (misleading comment): you're right — corrected across PR body: updated to describe the current Image digest-pin (MINOR): still deferred to the separate image-pinning migration per Aleksei Sviridkin (@lexfrei)'s earlier note; the sibling cleanup hooks share the same debt. Both helm-unittest suites green (extra/monitoring 6/6, system/monitoring 6/6); migration passes |
…cope Address review on #3094: - Add --wait=false to both kubectl delete calls so PVC finalizers (pvc-protection on pods still terminating) can never block the post-delete hook and stall the HelmRelease teardown. - Add activeDeadlineSeconds: 120 on the cleanup Job as a backstop. - Tests: assert the non-blocking flag, plus negative assertions that the PVC delete stays label-scoped (no --all) and the secret delete stays name-scoped (no -l/--all). deletecollection RBAC verb intentionally not added: kubectl delete -l lists then deletes individually, so the existing delete verb suffices. BREAKING CHANGE: disabling monitoring now permanently removes its VictoriaMetrics/VictoriaLogs storage PVCs and the metrics/logs they hold. Signed-off-by: Алексей Артамонов <aleksei.artamonov@aenix.io> Assisted-By: Claude <noreply@anthropic.com>
Address IvanHunters review on #3094: the PVC delete selector was release-agnostic (-l managed-by=vm-operator), matching the VM storage PVCs of every VMCluster/VLCluster in the namespace. If a namespace ever hosted more than one monitoring release, tearing one down would delete the other's VictoriaMetrics/VictoriaLogs storage. Scope the selector to this release via apps.cozystack.io/application.name= <release>-system, the ownership label the monitoring chart stamps on the VM CRs' managedMetadata and vm-operator propagates onto the PVCs — mirroring the merged seaweedfs hook's release scoping. deletecollection is still not needed: kubectl delete -l lists then deletes individually. Tests: assert the release-scoped selector and add a regression guard that the delete never falls back to the bare managed-by=vm-operator selector. Signed-off-by: Алексей Артамонов <aleksei.artamonov@aenix.io> Assisted-By: Claude <noreply@anthropic.com>
… cleanup matches Re-review on #3094 (IvanHunters, lexfrei): the release-scoped selector added in ba5797f was a silent no-op on the shipped vm-operator v0.68.4. The apps.cozystack.io/application.name label lived only under the VMCluster/VLCluster spec.managedMetadata.labels, and v0.68.4 does not propagate managedMetadata onto PVCs (that lands only in v0.71.0). So the delete matched nothing and the #3091 storage leak stayed open. Stamp the label where it actually reaches the PVCs: on spec.{vmselect,vmstorage,vlstorage}.storage.volumeClaimTemplate.metadata.labels of the VMCluster/VLCluster CRs (packages/system/monitoring). vm-operator copies claim-template labels onto the PVCs (IntoSTSVolume) even on v0.68.4, so the existing cleanup selector (managed-by=vm-operator,apps.cozystack.io/application.name= <release>-system) now matches and stays scoped to this release. Verified by rendering the CRs (label resolves to monitoring-system, matching the hook). Also add the explicit subjects[].namespace on the cleanup RoleBinding (lexfrei non-blocking) and a helm-unittest suite asserting the label lands on all three storage claim templates. Corrected the misleading test comment about how the label reaches the PVCs. 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>
e8026b8 to
2ec24d3
Compare
…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>
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
The round-2 backfill gap is properly closed: migration 50 relabels pre-existing VM/VL storage PVCs with the exact per-release value the cleanup hook selects, the PVC-name suffix regexes match vm-operator v0.68.4's real names (including the vmselect-cachedir-vmselect-<cr>-0 cache PVC), the migration is idempotent and non-fatal, uses the shared version-stamp helper, and is correctly the next free slot (50, targetVersion→51). This clears my previous CHANGES_REQUESTED. One pre-existing MINOR (unpinned kubectl image) remains, non-blocking.
Findings
[MINOR] packages/extra/monitoring/templates/hooks/cleanup.yaml:36 — cleanup image not digest-pinned and not routed through cozy-lib.image
The hook uses image: docker.io/clastix/kubectl:v1.32 with no @sha256: digest and without {{ include "cozy-lib.image" (list "..." $) }}. Air-gapped / mirrored-registry installs cannot rewrite the reference, and the tag is mutable. The digest-pinned + registry-routed form is the established convention (see the pinned docker.io/clastix/kubectl:v1.32@sha256:b9ef7d8... in packages/system/postgres-operator/tests/webhook-ready-hook_test.yaml:120). Sibling hooks (keycloak-configure, dashboard) share the same unpinned debt, so this is consistent-but-suboptimal, not a regression introduced here. Best folded into the in-flight image-pinning sweep rather than blocking this PR.
Caveats
- Existing-customer upgrade (Phase 5b-A): verified no breakage. Migration 50 runs in the platform
pre-upgradehook (packages/core/platform/templates/migration-hook.yaml:20) before the monitoring HR reconciles. It reads the release-scoping value fromspec.managedMetadata.labels.apps.cozystack.io/application.name— which already exists on main's CRs (git show origin/main:packages/system/monitoring/templates/vm/vmcluster.yamlconfirms it), so pre-upgrade CRs carry it — and stamps that value onto matched PVCs. The VCT-label change (vmcluster.yaml:38,vlogs.yaml:31) is an immutable STS field; the STS-recreate-with-Orphan adopts existing PVCs by name without re-applying claim-template labels, so the migration's label persists across the recreate and the post-delete hook's-l apps.cozystack.io/application.name=<release>-systemselector then matches on already-deployed clusters. - Fresh install (Phase 5b-B): verified no breakage. On a cold cluster the CRDs are absent → migration early-returns per
relabel_storage(migrations/50:48-51) and only stamps the version; new PVCs are created with the VCT label from the start, so the hook matches without any backfill. - PVC-name matching cross-checked against upstream vm-operator v0.68.4 source: STS names are
vmstorage-<cr>/vmselect-<cr>/vlstorage-<cr>, claim templates default tovmstorage-db/vmselect-cachedir/vlstorage-db, yielding PVCsvmstorage-db-vmstorage-<cr>-0,vmselect-cachedir-vmselect-<cr>-0,vlstorage-db-vlstorage-<cr>-0. The migration greps unanchored-left with right-anchored patterns(vmstorage|vmselect)-NAME-[0-9]+$andvlstorage-NAME-[0-9]+$, so all three real names match as a suffix; CNPGgrafana-db-*/alerta-db-*and user CRs without managedMetadata do not match. This is what commit 6f5300c's test fixture pins. - Idempotency / safety verified:
set -euo pipefailis supported by the alpine:3.24 BusyBox ash the migration image ships;kubectl label --overwrite, a per-PVC current-value skip,|| trueon the CR list and per-PVC reads, andgrep ... || trueon empty matches make re-runs and monitoring-less namespaces safe without aborting the platform upgrade. - Minor robustness note (not blocking):
migrations/50:66splices the CR name into the regex viased "s/NAME/${name}/g"unescaped. Only CRs already carrying the cozystack managedMetadata label reach this line, and monitoring CR names are chart-controlled constants (shortterm/longterm/generic) with no regex metacharacters, so there is no practical mismatch or over-match. Worth aprintf %s | sed 's/[^^]/[&]/g'-style escape only if CR names ever become user-controlled. - Round-2 stale-comment MINOR is resolved: the VCT comments (
vmcluster.yaml:35,vlogs.yaml:28) now correctly state vm-operator does not applyspec.managedMetadatato PVCs.
Recommended follow-ups
- Pin
clastix/kubectlby digest and route throughcozy-lib.imageacross the monitoring,keycloak-configure, anddashboardhooks in one sweep, and add the image to RenovatemanagerFilePatterns— separate housekeeping PR, not blocking this one. - Consider an E2E disable-then-verify-no-PVCs check (cf.
hack/e2e-apps/*.bats) as the real regression net: every current assertion (helm-unittest + the migration bats) is string/logic-level; an end-to-end toggle-off would catch a future selector/label drift that string assertions cannot. Non-blocking — the migration bats already pins the real PVC names, which covers the highest-risk part.
…cope Address review on #3094: - Add --wait=false to both kubectl delete calls so PVC finalizers (pvc-protection on pods still terminating) can never block the post-delete hook and stall the HelmRelease teardown. - Add activeDeadlineSeconds: 120 on the cleanup Job as a backstop. - Tests: assert the non-blocking flag, plus negative assertions that the PVC delete stays label-scoped (no --all) and the secret delete stays name-scoped (no -l/--all). deletecollection RBAC verb intentionally not added: kubectl delete -l lists then deletes individually, so the existing delete verb suffices. BREAKING CHANGE: disabling monitoring now permanently removes its VictoriaMetrics/VictoriaLogs storage PVCs and the metrics/logs they hold. Signed-off-by: Алексей Артамонов <aleksei.artamonov@aenix.io> Assisted-By: Claude <noreply@anthropic.com>
Address IvanHunters review on #3094: the PVC delete selector was release-agnostic (-l managed-by=vm-operator), matching the VM storage PVCs of every VMCluster/VLCluster in the namespace. If a namespace ever hosted more than one monitoring release, tearing one down would delete the other's VictoriaMetrics/VictoriaLogs storage. Scope the selector to this release via apps.cozystack.io/application.name= <release>-system, the ownership label the monitoring chart stamps on the VM CRs' managedMetadata and vm-operator propagates onto the PVCs — mirroring the merged seaweedfs hook's release scoping. deletecollection is still not needed: kubectl delete -l lists then deletes individually. Tests: assert the release-scoped selector and add a regression guard that the delete never falls back to the bare managed-by=vm-operator selector. Signed-off-by: Алексей Артамонов <aleksei.artamonov@aenix.io> Assisted-By: Claude <noreply@anthropic.com>
… cleanup matches Re-review on #3094 (IvanHunters, lexfrei): the release-scoped selector added in ba5797f was a silent no-op on the shipped vm-operator v0.68.4. The apps.cozystack.io/application.name label lived only under the VMCluster/VLCluster spec.managedMetadata.labels, and v0.68.4 does not propagate managedMetadata onto PVCs (that lands only in v0.71.0). So the delete matched nothing and the #3091 storage leak stayed open. Stamp the label where it actually reaches the PVCs: on spec.{vmselect,vmstorage,vlstorage}.storage.volumeClaimTemplate.metadata.labels of the VMCluster/VLCluster CRs (packages/system/monitoring). vm-operator copies claim-template labels onto the PVCs (IntoSTSVolume) even on v0.68.4, so the existing cleanup selector (managed-by=vm-operator,apps.cozystack.io/application.name= <release>-system) now matches and stays scoped to this release. Verified by rendering the CRs (label resolves to monitoring-system, matching the hook). Also add the explicit subjects[].namespace on the cleanup RoleBinding (lexfrei non-blocking) and a helm-unittest suite asserting the label lands on all three storage claim templates. Corrected the misleading test comment about how the label reaches the PVCs. 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>
9d7073e to
0105dac
Compare
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — the previously-blocking dead selector is fixed via the volumeClaimTemplate-label approach, the pre-existing-PVC gap is closed by the backfill migration, and the destructive behavior is marked and documented.
Rebased onto main: the backfill migration was renumbered 50→51 (targetVersion→52) to clear the collision with the etcd-adoption migration that landed on main. The migration body is otherwise unchanged.
Business context: disabling the monitoring tenant module left orphaned VictoriaMetrics/VictoriaLogs storage PVCs and the alerta/grafana ingress-TLS secrets in the tenant namespace (#3091); this adds a post-delete cleanup hook plus a migration that backfills the release-scoping label onto pre-existing storage PVCs.
Verified
- Selector is release-scoped by
apps.cozystack.io/application.name=<release>-systemalone, stamped on the VM/VLstorage.volumeClaimTemplate.metadata.labelsand copied onto PVCs by vm-operator (IntoSTSVolume) on the shipped v0.68.4;managed-by=vm-operatorcorrectly dropped. - The backfill migration matches the real v0.68.4 PVC names (incl.
vmselect-cachedir-vmselect-<cr>-0), is idempotent and non-fatal, and reads the value fromspec.managedMetadataso user-owned VMClusters are left untouched. - No data loss: CNPG
grafana-db/alerta-dbPVCs never carry the label; the claim-template change triggers an STS recreate withDeletePropagationOrphan(pods/PVCs preserved); TLS secrets are deleted by name only. - The cleanup Job cannot stall teardown (
--wait=false,activeDeadlineSeconds: 120,backoffLimit: 0, best-effort errors); RBAC is minimal; the pod is PSS-restricted. - Tests cover the full contract (happy path + negative guards against
--alland re-introducingmanaged-by); helm-unittest passes 6/6 in both charts and the migration bats passes. - Breaking change is marked (
!) and documented (body WARNING + release-note).
Non-blocking follow-ups
docker.io/clastix/kubectl:v1.32is not digest-pinned and not routed throughcozy-lib.image— shared debt with thekeycloak-configureanddashboardhooks; best folded into the image-pinning sweep rather than here.- Consider an E2E disable-then-verify-no-PVCs check as the real regression net; the current assertions are all string/logic-level.
…nant module delete (#3091) When the monitoring tenant module is disabled, the vm-operator storage PVCs (VictoriaMetrics/VictoriaLogs) and the alerta/grafana TLS secrets were left orphaned. Add a post-delete cleanup hook that deletes the PVCs by the managed-by=vm-operator label and the two TLS secrets by name, plus a test target and helm-unittest suite (the package had neither). CNPG PVCs are not targeted (CNPG cleans them). The cleanup Job is hardened (non-root, read-only root fs, dropped caps, resource limits); failures are logged, never blocking teardown. NOTE: disabling monitoring now permanently removes its storage PVCs and the data they hold. Signed-off-by: Алексей Артамонов <aleksei.artamonov@aenix.io>
…cope Address review on #3094: - Add --wait=false to both kubectl delete calls so PVC finalizers (pvc-protection on pods still terminating) can never block the post-delete hook and stall the HelmRelease teardown. - Add activeDeadlineSeconds: 120 on the cleanup Job as a backstop. - Tests: assert the non-blocking flag, plus negative assertions that the PVC delete stays label-scoped (no --all) and the secret delete stays name-scoped (no -l/--all). deletecollection RBAC verb intentionally not added: kubectl delete -l lists then deletes individually, so the existing delete verb suffices. BREAKING CHANGE: disabling monitoring now permanently removes its VictoriaMetrics/VictoriaLogs storage PVCs and the metrics/logs they hold. Signed-off-by: Алексей Артамонов <aleksei.artamonov@aenix.io> Assisted-By: Claude <noreply@anthropic.com>
Address IvanHunters review on #3094: the PVC delete selector was release-agnostic (-l managed-by=vm-operator), matching the VM storage PVCs of every VMCluster/VLCluster in the namespace. If a namespace ever hosted more than one monitoring release, tearing one down would delete the other's VictoriaMetrics/VictoriaLogs storage. Scope the selector to this release via apps.cozystack.io/application.name= <release>-system, the ownership label the monitoring chart stamps on the VM CRs' managedMetadata and vm-operator propagates onto the PVCs — mirroring the merged seaweedfs hook's release scoping. deletecollection is still not needed: kubectl delete -l lists then deletes individually. Tests: assert the release-scoped selector and add a regression guard that the delete never falls back to the bare managed-by=vm-operator selector. Signed-off-by: Алексей Артамонов <aleksei.artamonov@aenix.io> Assisted-By: Claude <noreply@anthropic.com>
… cleanup matches Re-review on #3094 (IvanHunters, lexfrei): the release-scoped selector added in ba5797f was a silent no-op on the shipped vm-operator v0.68.4. The apps.cozystack.io/application.name label lived only under the VMCluster/VLCluster spec.managedMetadata.labels, and v0.68.4 does not propagate managedMetadata onto PVCs (that lands only in v0.71.0). So the delete matched nothing and the #3091 storage leak stayed open. Stamp the label where it actually reaches the PVCs: on spec.{vmselect,vmstorage,vlstorage}.storage.volumeClaimTemplate.metadata.labels of the VMCluster/VLCluster CRs (packages/system/monitoring). vm-operator copies claim-template labels onto the PVCs (IntoSTSVolume) even on v0.68.4, so the existing cleanup selector (managed-by=vm-operator,apps.cozystack.io/application.name= <release>-system) now matches and stays scoped to this release. Verified by rendering the CRs (label resolves to monitoring-system, matching the hook). Also add the explicit subjects[].namespace on the cleanup RoleBinding (lexfrei non-blocking) and a helm-unittest suite asserting the label lands on all three storage claim templates. Corrected the misleading test comment about how the label reaches the PVCs. 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>
helm-unittest only inspects rendered manifests, not the operator's runtime PVC labels, so it cannot prove migration 50 actually relabels the right PVCs — the exact no-op/mis-scope class of bug this PR iterated on. Add a bats test (fake kubectl on PATH, mirroring hack/kubernetes-md0-migration.bats) that pins the runtime behaviour: it relabels the monitoring cluster's vmstorage/vmselect/ vlstorage PVCs with application.name=monitoring-system, never touches the CNPG db PVCs or a user's own VMCluster, and stays a safe version-stamping no-op when the VM/VL CRDs are absent. Signed-off-by: Алексей Артамонов <aleksei.artamonov@aenix.io> Assisted-By: Claude <noreply@anthropic.com>
…ackfill test /branch-review flagged that the migration comment and its bats test used the simplified vmselect cache PVC name cache-vmselect-<cr>-<ordinal>, while the real name is vmselect-cachedir-vmselect-<cr>-<ordinal>. The anchored regex matches both (it suffix-matches <component>-<cr>-<ordinal>), so behaviour was correct, but the test did not exercise the production name. Feed the realistic name in the fake kubectl so the regex is validated against what vm-operator actually provisions, and reword the migration comment to describe the suffix match generically instead of asserting a specific claim-template prefix. Signed-off-by: Алексей Артамонов <aleksei.artamonov@aenix.io> Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
0105dac to
ff037c0
Compare
What this PR does
When the
monitoringtenant module is disabled, the VictoriaMetrics/VictoriaLogs storage PVCs (created by vm-operator) and thealerta-tls/grafana-ingress-tlscert-manager secrets were left orphaned in the tenant namespace.This adds a
post-deletecleanup hook (packages/extra/monitoring/templates/hooks/cleanup.yaml) that:-l apps.cozystack.io/application.name={{ .Release.Name }}-system. That label is stamped on thestorage.volumeClaimTemplate.metadata.labelsof the VMCluster/VLCluster CRs (packages/system/monitoring); vm-operator copies claim-template labels onto the PVCs (IntoSTSVolume), so the selector matches and stays scoped to this monitoring release.managed-by=vm-operatoris intentionally not used — vm-operator does not reliably stamp it onto the storage PVCs;alerta-tlsandgrafana-ingress-tlssecrets by name (cert-manager TLS secrets carry only the cluster-widecontroller.cert-manager.io/fao=truelabel, so a label sweep would be unsafe — they are removed by name).CNPG PostgreSQL PVCs (
alerta-db-*,grafana-db-*) are intentionally not targeted — CNPG cleans those up itself, and they do not carryapps.cozystack.io/application.name, so the selector leaves them untouched.Upgrade path: the claim-template label only lands on PVCs created after the change, so migration 51 (
packages/core/platform/images/migrations/migrations/51) backfillsapps.cozystack.io/application.nameonto the pre-existing VM/VL storage PVCs of every cozystack-managed monitoring cluster, so the cleanup reclaims storage on already-deployed clusters too.A
testMakefile target and helm-unittest suites are added for both the cleanup hook and the claim-template label. The cleanup Job is hardened: non-root, read-only root fs, dropped capabilities,seccompProfile: RuntimeDefault, resource requests/limits,activeDeadlineSeconds: 120,backoffLimit: 0,--wait=falseon both deletes; RBAC scoped to PVC get/list/delete and the two named secrets. Failures are logged but never block teardown.Warning
Disabling the monitoring module now permanently deletes its VictoriaMetrics/VictoriaLogs storage PVCs and the metrics/logs they hold. This is intentional and fixes the storage leak. Back up before disabling if the data matters.
Fixes #3091.
Release note
Summary by CodeRabbit
make testtarget to run Helm unit tests.