fix(qdrant)!: clean up orphaned data PVC on delete (#3059) - #3075

Code Review

This pull request introduces a post-delete cleanup hook to delete orphaned PersistentVolumeClaims (PVCs) when a Qdrant release is deleted, along with corresponding unit tests. Feedback on the changes points out that the upstream Qdrant Helm chart uses persistence.labels rather than persistence.additionalLabels to apply labels to the PVC volume claim templates. Consequently, the HelmRelease values and the associated unit tests need to be updated to use labels to ensure the PVCs are correctly labeled and subsequently cleaned up.

Actionable comments posted: 1

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/qdrant/templates/hooks/cleanup-pvc.yaml`:
- Around line 5-95: The cleanup PVC Helm template has unquoted Helm scalars that
strict YAML parsers treat as syntax errors. Update the qdrant cleanup manifest
so every field using template expressions like {{ .Release.Name }} is quoted,
including the resource names, serviceAccountName, roleRef.name, and
subjects.name. Use the cleanup job, ServiceAccount, Role, RoleBinding, and
related identifiers to locate all affected scalar values and keep the rendered
YAML valid.

NOT LGTM — the PVC-label mechanism breaks reconciliation for already-deployed Qdrant instances.

Blocker — StatefulSet volumeClaimTemplates is immutable. This PR routes the release-unique label through qdrant.persistence.additionalLabels, which the upstream chart renders into volumeClaimTemplates[].metadata.labels (packages/system/qdrant/charts/qdrant/templates/statefulset.yaml ~L264, L269). For any Qdrant that already exists, this is a change to an immutable StatefulSet field, which the API server rejects (only replicas, ordinals, template, updateStrategy, persistentVolumeClaimRetentionPolicy, minReadySeconds are mutable). The <release>-system HelmRelease (retries: -1) then retry-fails indefinitely on upgrade — no data loss, but reconciliation is permanently broken, and the cleanup still can't reclaim those instances' PVCs (volumeClaimTemplate label changes don't apply retroactively to existing PVCs). This is the opposite of the openbao case, where the change worked precisely because persistentVolumeClaimRetentionPolicy is in the mutable list.

Suggested fix: drop the additionalLabels passthrough and have the hook delete by deterministic PVC name-prefix (qdrant-storage-<release>-). PVC names derive from the release, so this needs no StatefulSet change (upgrade-safe) and also reclaims PVCs from instances created before this change.

Also: hack/e2e-apps/qdrant.bats deletes the Qdrant but never asserts the data PVC is gone afterward, so the behavior this PR exists to fix isn't covered end-to-end — please add a post-delete (async-waited) not-found check on qdrant-storage-<name>-0.

Notes: the unit test can only assert the value reaches the HelmRelease, not that the subchart consumes it (I confirmed consumption separately at statefulset.yaml L269 — it is not a no-op). Consider the fix(...)! marker for the data-loss-on-delete behavior change, to match the sibling ClickHouse PR. The automated "use persistence.labels" suggestions are incorrect — the chart uses persistence.additionalLabels (no persistence.labels key exists). The security hardening on the cleanup Job is a clear improvement over the hook it is modeled on.

Actionable comments posted: 3

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 `@hack/e2e-apps/qdrant.bats`:
- Around line 34-35: The PVC wait check in qdrant.bats currently treats any
kubectl get pvc failure as success, so update the delete-verification logic to
only exit cleanly when kubectl reports NotFound for
qdrant-storage-qdrant-$name-0 and fail the script on all other errors. Adjust
the timeout loop and its surrounding shell condition so it inspects kubectl’s
stderr/exit status, keeping the existing regression message but ensuring
transient API/auth issues are surfaced instead of masked.

In `@packages/apps/qdrant/templates/hooks/cleanup-pvc.yaml`:
- Around line 59-65: The PVC cleanup in the cleanup logic should only target
StatefulSet-owned PVCs for the current release and must not treat list-command
errors as “no PVCs found.” Update the matching in the pvc collection block so it
binds to the exact qdrant StatefulSet ordinal naming used by the release (not a
broad prefix match), and handle the kubectl get pvc failure path explicitly in
this cleanup script instead of suppressing it with the current fallback. Use the
existing cleanup flow around pvcs, kubectl get pvc, and kubectl delete to locate
and adjust the behavior.

In `@packages/apps/qdrant/tests/cleanup_pvc_test.yaml`:
- Around line 41-43: The cleanup PVC test is still asserting only a broad prefix
match, so it won’t catch accidental matches like qdrant-test-other. Tighten the
expectation in cleanup_pvc_test.yaml for the hook command built by the cleanup
selector so it verifies the ordinal-bounded PVC pattern used by qdrant-test, and
update the matchRegex on spec.template.spec.containers[0].command[2] to reflect
the selector fix.

NOT LGTM — the post-delete cleanup selects PVCs by an unanchored name prefix, so deleting one Qdrant release can delete a sibling release's data PVC.

Note: the earlier blocker (routing a release-unique label through the subchart, breaking reconciliation for already-deployed StatefulSets) is resolved — the final revision switches to name-prefix selection and touches no chart values. That fix is correct. A new blocker was introduced by the prefix approach.

Blockers

B1: Unanchored PVC name prefix deletes other releases' data

File: packages/apps/qdrant/templates/hooks/cleanup-pvc.yaml (the cleanup command)
Issue: grep "^persistentvolumeclaim/qdrant-storage-<release>-" is a prefix with no ordinal anchor. Release names are qdrant-<app>, and the StatefulSet-templated PVCs are qdrant-storage-qdrant-<app>-<ordinal>.
Evidence: For two apps test and test-2 in the same namespace, the PVCs are qdrant-storage-qdrant-test-0 and qdrant-storage-qdrant-test-2-0. Deleting test runs grep prefix qdrant-storage-qdrant-test-, which matches BOTH — the hook deletes test-2's data PVC. If test-2's pod is running, pvc-protection holds the PVC in Terminating (a landmine that detonates on the next reschedule); if not running, immediate deletion.
Impact: Permanent, irreversible data loss for a sibling release whenever one release name is a prefix of another. Severe for a change whose explicit purpose is permanent PVC deletion.
Fix: Anchor the ordinal, e.g. grep -E "^persistentvolumeclaim/qdrant-storage-<release>-[0-9]+$" (matches single/multi-digit ordinals, excludes ...-test-2-0). Update the helm-unittest matchRegex in tests/cleanup_pvc_test.yaml — it currently pins the broad prefix and would pass against the buggy selector.

Non-blocking follow-ups

1. e2e (hack/e2e-apps/qdrant.bats): the while kubectl get pvc ... loop treats ANY non-zero exit as "gone", so a transient API/auth error reads as success. Gate on NotFound specifically.

LGTM — both prior blockers (immutable volumeClaimTemplates label; unanchored PVC prefix) are resolved; the anchored selector is correct and the tests pin it.

The earlier blocker is fully addressed: the cleanup now selects PVCs by grep -E "^persistentvolumeclaim/qdrant-storage-<release>-[0-9]+$". Anchoring the ordinal closes the cross-release collision in both directions — deleting qdrant-test no longer matches qdrant-storage-qdrant-test-2-0, and vice versa. The helm-unittest pins the anchored form (matchRegex) and guards against regressing to the bare prefix (notMatchRegex); the e2e test now gates on NotFound specifically rather than any non-zero exit. Tests pass locally (60/60).

Non-blocking follow-ups

1. cleanup-pvc.yaml — pvcs=$(kubectl get pvc ... | grep -E ... || true) masks a kubectl get failure: if the list call errors, the pipeline yields empty and the hook reports "No data PVCs found; nothing to delete" and exits success, silently leaving the PVC orphaned. The PR body states cleanup failures are logged, but this path isn't. Non-destructive (degrades to the pre-fix behavior) and consistent with the existing MariaDB hook, so not blocking — but checking the get exit status separately before grep would honor the stated intent.
2. The PR description is stale: it still describes the abandoned persistence.additionalLabels label-passthrough mechanism. The shipped code uses name-prefix selection and touches no chart values. Worth updating so the merged record matches the implementation.
3. (Optional, hook-family-wide) The Job has no activeDeadlineSeconds and kubectl delete runs with the default --wait=true, so an abnormally stuck pod termination could block the post-delete hook. Same as the existing MariaDB hook — optional hardening for the whole family, not a qdrant regression.