fix(qdrant)!: clean up orphaned data PVC on delete (#3059)#3075
fix(qdrant)!: clean up orphaned data PVC on delete (#3059)#3075scooby87 wants to merge 3 commits into
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 Qdrant data PVCs were left orphaned in the namespace after an application deletion. By introducing a post-delete Helm hook and applying unique instance labels to the PVCs, the system now ensures that storage is reclaimed correctly when a Qdrant release is removed. This change prevents long-term storage leaks while maintaining strict security practices for the cleanup process. 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
|
github-actions
Bot
added
area/uncategorized
📝 WalkthroughWalkthroughAdds a Helm post-delete hook to the Qdrant chart that removes orphaned StatefulSet data PVCs by name-prefix matching. The hook includes a Qdrant PVC cleanup hook
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
dosubot
Bot
added
the
area/database
scooby87
removed
the
area/uncategorized
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
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.
| additionalLabels: | ||
| app.kubernetes.io/instance: {{ .Release.Name }} |
There was a problem hiding this comment.
The upstream Qdrant Helm chart uses persistence.labels to apply labels to the PVC volume claim templates, not persistence.additionalLabels. Using additionalLabels will result in the label being ignored, meaning the PVC will not be labeled and the post-delete cleanup hook will fail to find and delete the orphaned PVC.
Please change additionalLabels to labels.
labels:
app.kubernetes.io/instance: {{ .Release.Name }}There was a problem hiding this comment.
This finding is incorrect — no change needed.
The vendored upstream Qdrant chart applies volumeClaimTemplates labels from persistence.additionalLabels, not persistence.labels:
volumeClaimTemplates:
- metadata:
name: {{ .Values.persistence.storageVolumeName | default "qdrant-storage" }}
labels:
app: {{ template "qdrant.name" . }}
{{- with .Values.persistence.additionalLabels }}
{{- toYaml . | nindent 10 }}
{{- end }}There is no persistence.labels key in this chart. Switching to labels would drop the app.kubernetes.io/instance label from the PVC and break the post-delete cleanup hook. The current additionalLabels passthrough is correct, and the helm unittest suite (cleanup_pvc_test.yaml) passes against the rendered output.
| value: HelmRelease | ||
| asserts: | ||
| - equal: | ||
| path: spec.values.qdrant.persistence.additionalLabels["app.kubernetes.io/instance"] |
There was a problem hiding this comment.
Since the field in templates/qdrant.yaml should be changed from additionalLabels to labels, this test assertion needs to be updated accordingly to verify the correct path.
path: spec.values.qdrant.persistence.labels["app.kubernetes.io/instance"]There was a problem hiding this comment.
Same root cause as the additionalLabels thread above — the upstream chart uses persistence.additionalLabels (statefulset.yaml#L264-L271), so this test asserts the correct path and needs no change.
github-actions
Bot
added
the
area/uncategorized
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/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.
🪄 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: a426d42b-fa22-407f-82ad-ba6e3a2bce8d
📒 Files selected for processing (4)
packages/apps/qdrant/Makefilepackages/apps/qdrant/templates/hooks/cleanup-pvc.yamlpackages/apps/qdrant/templates/qdrant.yamlpackages/apps/qdrant/tests/cleanup_pvc_test.yaml
| name: {{ .Release.Name }}-qdrant-cleanup | ||
| labels: | ||
| app.kubernetes.io/instance: {{ .Release.Name }} | ||
| annotations: | ||
| "helm.sh/hook": post-delete | ||
| "helm.sh/hook-weight": "10" | ||
| "helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded | ||
| spec: | ||
| template: | ||
| metadata: | ||
| labels: | ||
| app.kubernetes.io/instance: {{ .Release.Name }} | ||
| policy.cozystack.io/allow-to-apiserver: "true" | ||
| spec: | ||
| serviceAccountName: {{ .Release.Name }}-qdrant-cleanup | ||
| restartPolicy: Never | ||
| securityContext: | ||
| runAsNonRoot: true | ||
| runAsUser: 65534 | ||
| runAsGroup: 65534 | ||
| seccompProfile: | ||
| type: RuntimeDefault | ||
| containers: | ||
| - name: cleanup | ||
| image: docker.io/clastix/kubectl:v1.32 | ||
| securityContext: | ||
| allowPrivilegeEscalation: false | ||
| readOnlyRootFilesystem: true | ||
| capabilities: | ||
| drop: ["ALL"] | ||
| env: | ||
| - name: HOME | ||
| value: /tmp | ||
| command: | ||
| - /bin/sh | ||
| - -c | ||
| - | | ||
| 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." | ||
| volumeMounts: | ||
| - name: tmp | ||
| mountPath: /tmp | ||
| volumes: | ||
| - name: tmp | ||
| emptyDir: {} | ||
| --- | ||
| apiVersion: v1 | ||
| kind: ServiceAccount | ||
| metadata: | ||
| name: {{ .Release.Name }}-qdrant-cleanup | ||
| labels: | ||
| app.kubernetes.io/instance: {{ .Release.Name }} | ||
| annotations: | ||
| "helm.sh/hook": post-delete | ||
| helm.sh/hook-weight: "0" | ||
| "helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded | ||
| --- | ||
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: Role | ||
| metadata: | ||
| name: {{ .Release.Name }}-qdrant-cleanup | ||
| labels: | ||
| app.kubernetes.io/instance: {{ .Release.Name }} | ||
| annotations: | ||
| "helm.sh/hook": post-delete | ||
| "helm.sh/hook-weight": "5" | ||
| "helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded | ||
| rules: | ||
| - apiGroups: [""] | ||
| resources: ["persistentvolumeclaims"] | ||
| verbs: ["get", "list", "delete"] | ||
| --- | ||
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: RoleBinding | ||
| metadata: | ||
| name: {{ .Release.Name }}-qdrant-cleanup | ||
| labels: | ||
| app.kubernetes.io/instance: {{ .Release.Name }} | ||
| annotations: | ||
| "helm.sh/hook": post-delete | ||
| helm.sh/hook-weight: "5" | ||
| "helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded | ||
| roleRef: | ||
| apiGroup: rbac.authorization.k8s.io | ||
| kind: Role | ||
| name: {{ .Release.Name }}-qdrant-cleanup | ||
| subjects: | ||
| - kind: ServiceAccount | ||
| name: {{ .Release.Name }}-qdrant-cleanup |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
Quote templated scalar values to resolve YAML syntax errors.
yamllint fails on this file with syntax error: expected <block end> at line 5. Unquoted Helm template scalars starting with {{ are misinterpreted as YAML flow collections by strict parsers.
Quote all line items containing {{ .Release.Name }} (e.g., name, serviceAccountName, roleRef.name, subjects.name). For example:
name: {{ .Release.Name }}-qdrant-cleanup → name: "{{ .Release.Name }}-qdrant-cleanup".
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 5-5: syntax error: expected , but found ''
(syntax)
🤖 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/qdrant/templates/hooks/cleanup-pvc.yaml` around lines 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.
The StatefulSet data PVC (qdrant-storage-<release>-0) was left orphaned after a Qdrant app CR was deleted. The vendored upstream chart does not support persistentVolumeClaimRetentionPolicy, so add a post-delete PVC cleanup hook (modeled on the MariaDB hook) and pass persistence.additionalLabels through the HelmRelease so the data PVC carries a release-unique app.kubernetes.io/instance label the hook selects on. The cleanup Job runs non-root with a read-only root fs and dropped capabilities; failures are logged but never block namespace teardown. Signed-off-by: Алексей Артамонов <aleksei.artamonov@aenix.io>
scooby87
force-pushed
the
fix/qdrant-data-pvc-orphaned
branch
from
065581a to
178b836
Compare
The .PHONY: test tweak is out of scope for the PVC cleanup fix; remove it to keep this PR focused (flagged by review). Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Алексей Артамонов <alexeyartamonov1987@gmail.com>
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
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.
…e label The previous approach routed a release-unique label through qdrant.persistence.additionalLabels, which the upstream chart renders into the StatefulSet's volumeClaimTemplates[].metadata.labels. That field is immutable on an existing StatefulSet, so for any already-deployed Qdrant the API server would reject the upgrade and the <release>-system HelmRelease (retries: -1) would retry-fail forever — permanently wedging reconciliation. Label changes on a volumeClaimTemplate also never apply to PVCs that already exist, so the cleanup hook could not reclaim those instances anyway. Switch the post-delete hook to select PVCs by the deterministic StatefulSet naming prefix (qdrant-storage-<release>-) instead. PVC names derive purely from the release name, so this needs no StatefulSet change (upgrade-safe) and also reclaims PVCs from instances created before the hook existed. - qdrant.yaml: drop the additionalLabels passthrough entirely. - hooks/cleanup-pvc.yaml: delete by name prefix via kubectl get -o name | grep. - tests: assert the name-prefix command instead of the label passthrough. - e2e: assert qdrant-storage-<name>-0 is gone after delete (async-waited). BREAKING CHANGE: deleting a Qdrant app now permanently removes its data PVC and the vector data it holds. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Алексей Артамонов <alexeyartamonov1987@gmail.com>
scooby87
changed the title
github-actions
Bot
added
the
kind/breaking-change
|
Aleksei Sviridkin (@lexfrei) thanks for the thorough review — you're right on the blocker, fixed in 2587458. Root cause accepted. What changed (your suggested fix):
PTAL when you have a moment. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@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.
🪄 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: 4356b894-8433-4423-a75a-6d449b99c242
📒 Files selected for processing (3)
hack/e2e-apps/qdrant.batspackages/apps/qdrant/templates/hooks/cleanup-pvc.yamlpackages/apps/qdrant/tests/cleanup_pvc_test.yaml
| timeout 120 sh -ec "while kubectl -n tenant-test get pvc qdrant-storage-qdrant-$name-0 >/dev/null 2>&1; do sleep 2; done" \ | ||
| || { echo "❌ data PVC qdrant-storage-qdrant-$name-0 still present after Qdrant delete (issue #3059 regression)"; kubectl -n tenant-test get pvc -o wide; kubectl -n tenant-test get jobs,pods -l app.kubernetes.io/instance=qdrant-$name -o wide; false; } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Only treat NotFound as success here.
This loop passes on any kubectl get pvc failure, so a transient API/auth error can hide a real cleanup regression. Please distinguish NotFound from other non-zero exits and fail on the latter.
Suggested fix
- timeout 120 sh -ec "while kubectl -n tenant-test get pvc qdrant-storage-qdrant-$name-0 >/dev/null 2>&1; do sleep 2; done" \
+ timeout 120 sh -ec '
+ while true; do
+ out=$(kubectl -n tenant-test get pvc qdrant-storage-qdrant-'"$name"'-0 2>&1) && {
+ sleep 2
+ continue
+ }
+ case "$out" in
+ *"(NotFound)"*) exit 0 ;;
+ *) echo "$out" >&2; exit 1 ;;
+ esac
+ done
+ ' \
|| { echo "❌ data PVC qdrant-storage-qdrant-$name-0 still present after Qdrant delete (issue `#3059` regression)"; kubectl -n tenant-test get pvc -o wide; kubectl -n tenant-test get jobs,pods -l app.kubernetes.io/instance=qdrant-$name -o wide; false; }📝 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.
| timeout 120 sh -ec "while kubectl -n tenant-test get pvc qdrant-storage-qdrant-$name-0 >/dev/null 2>&1; do sleep 2; done" \ | |
| || { echo "❌ data PVC qdrant-storage-qdrant-$name-0 still present after Qdrant delete (issue #3059 regression)"; kubectl -n tenant-test get pvc -o wide; kubectl -n tenant-test get jobs,pods -l app.kubernetes.io/instance=qdrant-$name -o wide; false; } | |
| timeout 120 sh -ec ' | |
| while true; do | |
| out=$(kubectl -n tenant-test get pvc qdrant-storage-qdrant-'"$name"'-0 2>&1) && { | |
| sleep 2 | |
| continue | |
| } | |
| case "$out" in | |
| *"(NotFound)"*) exit 0 ;; | |
| *) echo "$out" >&2; exit 1 ;; | |
| esac | |
| done | |
| ' \ | |
| || { echo "❌ data PVC qdrant-storage-qdrant-$name-0 still present after Qdrant delete (issue `#3059` regression)"; kubectl -n tenant-test get pvc -o wide; kubectl -n tenant-test get jobs,pods -l app.kubernetes.io/instance=qdrant-$name -o wide; false; } |
🤖 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 `@hack/e2e-apps/qdrant.bats` around lines 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.
| pvcs=$(kubectl get pvc -n {{ .Release.Namespace }} -o name \ | ||
| | grep "^persistentvolumeclaim/qdrant-storage-{{ .Release.Name }}-" || true) | ||
| if [ -z "$pvcs" ]; then | ||
| echo "No data PVCs found for release {{ .Release.Name }}; nothing to delete." | ||
| else | ||
| echo "$pvcs" | xargs kubectl delete -n {{ .Release.Namespace }} --ignore-not-found \ | ||
| || echo "WARNING: PVC cleanup failed for {{ .Release.Name }}; orphaned PVCs may remain" >&2 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Bound PVC matching to StatefulSet ordinals and log list failures.
The current prefix match can delete another release’s PVC when release names overlap, e.g. cleanup for foo also matches qdrant-storage-foo-bar-0. Also, || true masks kubectl get pvc failures as “no PVCs found,” so cleanup can silently fail.
Proposed fix
- pvcs=$(kubectl get pvc -n {{ .Release.Namespace }} -o name \
- | grep "^persistentvolumeclaim/qdrant-storage-{{ .Release.Name }}-" || true)
+ if ! pvc_names=$(kubectl get pvc -n {{ .Release.Namespace }} -o name); then
+ echo "WARNING: PVC cleanup failed for {{ .Release.Name }}; unable to list PVCs" >&2
+ exit 0
+ fi
+ pvcs=$(echo "$pvc_names" \
+ | grep -E "^persistentvolumeclaim/qdrant-storage-{{ .Release.Name }}-[0-9]+$" || true)📝 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.
| pvcs=$(kubectl get pvc -n {{ .Release.Namespace }} -o name \ | |
| | grep "^persistentvolumeclaim/qdrant-storage-{{ .Release.Name }}-" || true) | |
| if [ -z "$pvcs" ]; then | |
| echo "No data PVCs found for release {{ .Release.Name }}; nothing to delete." | |
| else | |
| echo "$pvcs" | xargs kubectl delete -n {{ .Release.Namespace }} --ignore-not-found \ | |
| || echo "WARNING: PVC cleanup failed for {{ .Release.Name }}; orphaned PVCs may remain" >&2 | |
| if ! pvc_names=$(kubectl get pvc -n {{ .Release.Namespace }} -o name); then | |
| echo "WARNING: PVC cleanup failed for {{ .Release.Name }}; unable to list PVCs" >&2 | |
| exit 0 | |
| fi | |
| pvcs=$(echo "$pvc_names" \ | |
| | grep -E "^persistentvolumeclaim/qdrant-storage-{{ .Release.Name }}-[0-9]+$" || true) | |
| if [ -z "$pvcs" ]; then | |
| echo "No data PVCs found for release {{ .Release.Name }}; nothing to delete." | |
| else | |
| echo "$pvcs" | xargs kubectl delete -n {{ .Release.Namespace }} --ignore-not-found \ | |
| || echo "WARNING: PVC cleanup failed for {{ .Release.Name }}; orphaned PVCs may remain" >&2 |
🤖 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/qdrant/templates/hooks/cleanup-pvc.yaml` around lines 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.
| - matchRegex: | ||
| path: spec.template.spec.containers[0].command[2] | ||
| pattern: "grep \"\\^persistentvolumeclaim/qdrant-storage-qdrant-test-\"" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the ordinal-bounded PVC selector.
This test currently preserves the broad prefix match, so it would still pass if cleanup for qdrant-test matched qdrant-test-other PVCs. Update it alongside the hook selector fix.
Proposed fix
- matchRegex:
path: spec.template.spec.containers[0].command[2]
- pattern: "grep \"\\^persistentvolumeclaim/qdrant-storage-qdrant-test-\""
+ pattern: "grep -E \"\\^persistentvolumeclaim/qdrant-storage-qdrant-test-\\[0-9\\]\\+\\$\""📝 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.
| - matchRegex: | |
| path: spec.template.spec.containers[0].command[2] | |
| pattern: "grep \"\\^persistentvolumeclaim/qdrant-storage-qdrant-test-\"" | |
| - matchRegex: | |
| path: spec.template.spec.containers[0].command[2] | |
| pattern: "grep -E \"\\^persistentvolumeclaim/qdrant-storage-qdrant-test-\\[0-9\\]\\+\\$\"" |
🤖 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/qdrant/tests/cleanup_pvc_test.yaml` around lines 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.
2 participants
What this PR does
After deleting a
Qdrantapp CR, the StatefulSet data PVC(
qdrant-storage-<release>-0) was left orphaned in the tenant namespace,consuming storage permanently. StatefulSet-templated PVCs are never
garbage-collected on StatefulSet deletion by default.
The vendored upstream Qdrant chart does not support
persistentVolumeClaimRetentionPolicy(verified: itsstatefulset.yamlneverrenders that field), so the OpenBAO-style value passthrough is not possible
here. Instead this adds a
post-deletePVC cleanup hook (modeled on the MariaDBhook) that deletes the release's PVCs by label.
The chart's volumeClaimTemplate only stamps
app: qdrant(shared by everyQdrant in a namespace), so to select PVCs safely per release this also passes
persistence.additionalLabels.app.kubernetes.io/instance: <release>through theHelmRelease — giving the data PVC a release-unique label the hook selects on.
The cleanup Job is hardened: runs as non-root (uid 65534), read-only root fs,
all capabilities dropped,
seccompProfile: RuntimeDefault; RBAC scoped topersistentvolumeclaims. A real cleanup failure is logged (not silentlyswallowed) but never blocks namespace teardown.
Warning
Deleting a Qdrant app now permanently removes its data PVC and the vector data it holds.
This is intentional and fixes the storage leak; the hook only fires on full
release deletion (not on scale-down). Back up before deleting.
Fixes #3059.
Release note
Summary by CodeRabbit
New Features
Tests