Skip to content

Commit 6be0d86

Browse files
scooby87scooby87
committed
fix(clickhouse): surface cleanup hook failures and backfill legacy keeper 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 <[email protected]> Signed-off-by: Алексей Артамонов <[email protected]>
1 parent 85bb40a commit 6be0d86

4 files changed

Lines changed: 85 additions & 10 deletions

File tree

‎packages/apps/clickhouse/templates/hooks/cleanup-pvc.yaml‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ metadata:
88
annotations:
99
"helm.sh/hook": post-delete
1010
"helm.sh/hook-weight": "10"
11-
"helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded
11+
"helm.sh/hook-delete-policy": before-hook-creation
1212
spec:
1313
template:
1414
metadata:
@@ -46,9 +46,12 @@ spec:
4646
- /bin/sh
4747
- -c
4848
- |
49+
set -eu
4950
echo "Deleting orphaned PVCs for {{ .Release.Name }}..."
50-
kubectl delete pvc -n {{ .Release.Namespace }} -l app.kubernetes.io/instance={{ .Release.Name }} --ignore-not-found \
51-
|| echo "WARNING: PVC cleanup failed for {{ .Release.Name }}; orphaned PVCs may remain" >&2
51+
if ! kubectl delete pvc -n {{ .Release.Namespace }} -l app.kubernetes.io/instance={{ .Release.Name }} --ignore-not-found; then
52+
echo "ERROR: PVC cleanup failed for {{ .Release.Name }}; orphaned PVCs may remain" >&2
53+
exit 1
54+
fi
5255
echo "PVC cleanup complete."
5356
volumeMounts:
5457
- name: tmp
@@ -65,8 +68,8 @@ metadata:
6568
app.kubernetes.io/instance: {{ .Release.Name }}
6669
annotations:
6770
"helm.sh/hook": post-delete
68-
helm.sh/hook-weight: "0"
69-
"helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded
71+
"helm.sh/hook-weight": "0"
72+
"helm.sh/hook-delete-policy": before-hook-creation
7073
---
7174
apiVersion: rbac.authorization.k8s.io/v1
7275
kind: Role
@@ -77,7 +80,7 @@ metadata:
7780
annotations:
7881
"helm.sh/hook": post-delete
7982
"helm.sh/hook-weight": "5"
80-
"helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded
83+
"helm.sh/hook-delete-policy": before-hook-creation
8184
rules:
8285
- apiGroups: [""]
8386
resources: ["persistentvolumeclaims"]
@@ -91,8 +94,8 @@ metadata:
9194
app.kubernetes.io/instance: {{ .Release.Name }}
9295
annotations:
9396
"helm.sh/hook": post-delete
94-
helm.sh/hook-weight: "5"
95-
"helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded
97+
"helm.sh/hook-weight": "5"
98+
"helm.sh/hook-delete-policy": before-hook-creation
9699
roleRef:
97100
apiGroup: rbac.authorization.k8s.io
98101
kind: Role

‎packages/apps/clickhouse/tests/cleanup_pvc_test.yaml‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,14 +30,20 @@ tests:
3030
path: metadata.annotations["helm.sh/hook"]
3131
value: post-delete
3232
- equal:
33+
# before-hook-creation only (no hook-succeeded): a failed cleanup Job
34+
# must survive so its logs are inspectable instead of being reaped.
3335
path: metadata.annotations["helm.sh/hook-delete-policy"]
34-
value: before-hook-creation,hook-succeeded
36+
value: before-hook-creation
3537
- equal:
3638
path: spec.template.spec.serviceAccountName
3739
value: ch-test-cleanup
3840
- matchRegex:
3941
path: spec.template.spec.containers[0].command[2]
4042
pattern: 'kubectl delete pvc -n tenant-test -l app\.kubernetes\.io/instance=ch-test'
43+
- matchRegex:
44+
# The hook must surface kubectl failures (exit 1), not mask them.
45+
path: spec.template.spec.containers[0].command[2]
46+
pattern: 'exit 1'
4147

4248
- it: "grants only get/list/delete on persistentvolumeclaims"
4349
documentSelector:

‎packages/core/platform/images/migrations/migrations/45‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
#!/bin/sh
2+
# Migration 45 --> 46
3+
# Backfill the app.kubernetes.io/instance release label onto pre-existing
4+
# ClickHouse keeper PVCs.
5+
#
6+
# PR #3072 adds app.kubernetes.io/instance to the keeper volumeClaimTemplates and
7+
# a post-delete hook that garbage-collects PVCs carrying that label. But the
8+
# altinity clickhouse-keeper-operator only stamps volumeClaimTemplates.metadata
9+
# .labels onto PVCs it creates *after* the change; it never re-labels PVCs that
10+
# already exist. Keeper PVCs provisioned before this upgrade therefore stay
11+
# unlabeled, the cleanup hook's selector misses them, and they keep leaking on
12+
# delete (issue #3057). This migration relabels them once so the hook can reclaim
13+
# them. The main ClickHouse data/log PVCs were already labeled before this PR and
14+
# need no backfill.
15+
16+
set -euo pipefail
17+
18+
# Each ClickHouseKeeperInstallation is named "<release>-keeper"; its PVCs are
19+
# created from the "default" volumeClaimTemplate and named
20+
# "default-chk-<release>-keeper-<cluster>-<shard>-<replica>-<ordinal>".
21+
# No early-close (head) in the pipe: under pipefail a large list would SIGPIPE
22+
# kubectl and abort the migration.
23+
chks=$(kubectl get clickhousekeeperinstallations.clickhouse-keeper.altinity.com -A \
24+
-o jsonpath='{range .items[*]}{.metadata.namespace}{" "}{.metadata.name}{"\n"}{end}')
25+
26+
echo "$chks" | while read -r ns chk; do
27+
[ -n "${ns:-}" ] || continue
28+
release="${chk%-keeper}"
29+
30+
# grep exits 1 when a namespace has no matching PVCs; tolerate it so pipefail
31+
# does not abort the whole migration on an empty match.
32+
pvcs=$(kubectl get pvc -n "$ns" \
33+
-o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' \
34+
| grep -E "^default-chk-${chk}-" || true)
35+
36+
for pvc in $pvcs; do
37+
[ -n "${pvc:-}" ] || continue
38+
current=$(kubectl get pvc -n "$ns" "$pvc" \
39+
-o jsonpath='{.metadata.labels.app\.kubernetes\.io/instance}' 2>/dev/null || true)
40+
if [ "$current" = "$release" ]; then
41+
echo "PVC $ns/$pvc already labeled app.kubernetes.io/instance=$release; skipping"
42+
continue
43+
fi
44+
echo "Labeling PVC $ns/$pvc with app.kubernetes.io/instance=$release"
45+
kubectl label pvc -n "$ns" "$pvc" "app.kubernetes.io/instance=$release" --overwrite
46+
done
47+
done
48+
49+
# Stamp version.
50+
# Mirror migration 44's labeled manifest. A label-less apply by the same field
51+
# manager would strip the platform.cozystack.io/no-delete label, dropping
52+
# cozystack-version out of the cozystack-no-delete-guardrail
53+
# ValidatingAdmissionPolicy. templates/cozystack-version.yaml only re-renders
54+
# the labeled ConfigMap on first install, so the label must be carried inline
55+
# here.
56+
kubectl apply --filename - <<EOF
57+
apiVersion: v1
58+
kind: ConfigMap
59+
metadata:
60+
name: cozystack-version
61+
namespace: cozy-system
62+
labels:
63+
platform.cozystack.io/no-delete: "true"
64+
data:
65+
version: "46"
66+
EOF

‎packages/core/platform/values.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ sourceRef:
66
migrations:
77
enabled: false
88
image: ghcr.io/cozystack/cozystack/platform-migrations:v1.5.0@sha256:8bf61f17e99eb4a3365394e3d97ac52a1d68cc84aa104894ecb09de25721dc1d
9-
targetVersion: 45
9+
targetVersion: 46
1010
# Bundle deployment configuration
1111
bundles:
1212
system:

0 commit comments

Comments
 (0)