fix(seaweedfs)!: clean up orphaned data PVCs and cert/db secrets on tenant module delete (#3092) - #3095

Code Review

This pull request introduces a post-delete cleanup hook for the SeaweedFS package to remove orphaned PVCs and secrets after the module is disabled, along with corresponding unit tests and a Makefile update. The reviewer feedback suggests removing the restrictive component=volume label filter from the PVC deletion command to ensure that PVCs belonging to custom storage pools or multi-zone topologies are also properly cleaned up, and updating the unit tests accordingly.

Actionable comments posted: 2

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/seaweedfs/templates/hooks/cleanup.yaml`:
- Around line 112-120: The cleanup hook’s kubectl delete calls can hang because
they wait for resource deletion by default, so update the SeaweedFS post-delete
cleanup in the hook command to use --wait=false on both the PVC and secret
deletions. Keep the existing logging and ignore-not-found behavior, but ensure
the commands in the cleanup template complete immediately after issuing the
delete request, preventing hook stalls when finalizers block removal.
- Around line 24-26: The PVC cleanup Role is missing the RBAC permission needed
by the cleanup Job’s labeled delete path. Update the persistentvolumeclaims
permissions in the cleanup hook template so the Role used by the Job includes
deletecollection alongside the existing get, list, and delete verbs; this will
let the cleanup shell command that deletes PVCs by label selector work
correctly.

NOT LGTM — the cleanup is correct and well-tested for the default topology, but the PVC selector silently misses volume PVCs created by custom storage pools and MultiZone topologies, leaving the same leak for those configurations.

Verified the resource names against the rendered chart and the reported leak: the 9 secret names all match, and <release>-system-db-secret carries helm.sh/resource-policy: keep so it genuinely orphans.

Blocker

• templates/hooks/cleanup.yaml:117 — app.kubernetes.io/component=volume matches only the default volume pool. The volume StatefulSet sets app.kubernetes.io/component to {{ $volumeName }}, which the chart computes as trimSuffix "-" (printf "volume-%s" $vname) — so a named pool/zone produces volume-<name>, which the exact component=volume selector does not match, and those PVCs stay orphaned (the same bug this PR fixes). Dropping the component clause and keeping app.kubernetes.io/name=seaweedfs,app.kubernetes.io/instance={{ .Release.Name }}-system is safe and complete: master/filer use hostPath (no PVCs) and the CNPG db PVCs carry cnpg.io/* labels, so only the volume PVCs match those two labels. The test at tests/cleanup_test.yaml pins the current selector and must be updated in lockstep. Because the hook swallows failures (|| echo, the Job always exits 0), a selector miss is permanently silent — selector correctness is the whole gate.

Non-blocking

• The subject is fix(seaweedfs):, but disabling the module now permanently deletes volume data. The data loss is documented in the body and release note, but please mark it breaking (fix(seaweedfs)!:) for consistency with the sibling cleanup PR.
• Consider --wait=false on both kubectl delete commands; the Job has no activeDeadlineSeconds, so a PVC stuck on a finalizer could hang the post-delete hook.

The hardening, hook ordering, --ignore-not-found idempotency, and the helm-unittest coverage (incl. the resourceNames-scoped secret RBAC) all look good. Note: the deletecollection verb is not needed — kubectl delete -l does a list + per-object delete, so get,list,delete is sufficient.

LGTM — the prior blocker is resolved; dropping the component clause while keeping name+instance correctly covers every volume topology, and I verified the selector against the chart's StatefulSet labels.

The earlier blocker (the component=volume clause left custom-pool and MultiZone PVCs orphaned) is fixed. Verified against packages/system/seaweedfs/charts/seaweedfs/templates/volume/volume-statefulset.yaml: the volume StatefulSet's spec.selector.matchLabels is {app.kubernetes.io/name=seaweedfs, app.kubernetes.io/instance=<inner-release>, app.kubernetes.io/component=<volumeName>}, and the StatefulSet controller stamps exactly those matchLabels onto each volumeClaimTemplate PVC. So -l app.kubernetes.io/name=seaweedfs,app.kubernetes.io/instance=<release>-system (no component clause) matches volume, volume-<pool>, volume-<zone>, and volume-<zone>-<pool> PVCs across Simple/pool/MultiZone — and uses an exact instance match, so there is no cross-release prefix collision. The 9 by-name secrets match the reported orphan set exactly. --wait=false + activeDeadlineSeconds: 120 keep the hook from blocking teardown. helm-unittest passes (7/7).

Non-blocking follow-ups

1. The PR description is stale: it states PVCs are deleted by labels including app.kubernetes.io/component=volume, but the shipped selector deliberately omits the component clause (and the test asserts it is absent). The body describes the exact bug the code fixes — worth correcting so the merged record is accurate.
2. No e2e coverage for the cleanup. helm-unittest only pins the rendered command string, not real deletion; seaweedfs has no e2e test today. A toggle-module-off e2e asserting the PVCs/secrets are gone would turn the statically-verified selector into empirical proof and guard against upstream chart label drift.
3. The secret cleanup is an explicit by-name list tied to the current upstream chart's cert set. If a future chart bump adds or renames a per-component cert (or enables an auto-generated s3/sftp config secret that is not helm-managed), the hook silently will not reap it. Worth a comment noting the list must track the chart's cert templates.