Skip to content

Commit 7331ecb

Browse files
androndoclaude
andcommitted
test(e2e): correct the round-trip budget and its stale comments
Second review pass on this branch. 1. The op timeout equalled the inner sum instead of exceeding it: the comment enumerated 3900s of inner waits and then set 65m, and 3900s IS 65m — so there was zero headroom for the applies and SQL calls between those waits, and the claim of "room" was false. Sized the inner budgets against what CI actually takes instead (the bucket suite completes in ~37s and a backup-enabled ClickHouse plus Keeper reaches Ready in ~98s, measured on run 30353894332): bucket claim/access 300->180 and the three job waits 600->480, summing to 3300s, under a 60m op timeout. Shrinking the inner budgets rather than raising the ceiling also keeps this suite closer to its postgres and mariadb siblings on a 180-minute job cap. 2. The strategy Pod's securityContext comment still justified root with "Pod runs apk add at startup", which the previous commit made conditional — the default path now runs the platform's pre-baked image and installs nothing. Scoped the note to the alpine fallback and said explicitly that the field set matches cozy-default-altinity, runAsNonRoot deliberately unset in both. 3. clickhouse-backup.yaml still referenced clickhouse-backup-{1-contracts, 2-roundtrip}: the file moved unchanged, so the Test rename never reached its own header and a reader following either name found nothing. 4. wait_sts_ready's timeout diagnostic could not print anything. It selected clickhouse.altinity.com/chi=${name#chi-}, which yields clickhouse-<app>-clickhouse-0-0 while the label carries the CHI name (clickhouse-<app>), and 2>/dev/null then swallowed kubectl's "No resources found". Ask for the StatefulSet's own first replica by name instead and let both streams through; verified it now surfaces the Pod events that explain why the replica never came up. Also dropped the clickhouse bats case added in the previous commit: it asserted what the generic examples/backups/<app> -> <app> mapping already guarantees, so it did not lock the invariant its comment claimed. That invariant is now recorded where it belongs — in the mapping's own comment in select-e2e.sh. The dedicated ch-backup-client image this example now inherits is tracked in #3420; noted there that the command: override is load-bearing in two files. Assisted-By: Claude <[email protected]> Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Signed-off-by: Andrey Kolkov <[email protected]>
1 parent 61eb58d commit 7331ecb

10 files changed

Lines changed: 56 additions & 44 deletions

File tree

‎examples/backups/clickhouse/00-helpers.sh‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,14 @@ wait_sts_ready() {
170170
fi
171171
if [[ $elapsed -ge $timeout ]]; then
172172
log_error "Timeout waiting for StatefulSet/$name to become ready"
173-
kubectl -n "$NAMESPACE" get pods -l "clickhouse.altinity.com/chi=${name#chi-}" >&2 2>/dev/null || true
173+
# A StatefulSet names its Pods "<sts>-N", so ask for the first
174+
# replica by name instead of guessing an operator label — and let
175+
# both streams through, since the point of this branch is to say
176+
# WHY the Pod never came up. Events carry the reason a Pod is
177+
# Pending (unschedulable, missing PVC, image pull) that neither the
178+
# Pod nor the StatefulSet status spells out.
179+
kubectl -n "$NAMESPACE" get statefulset.apps "$name" -o wide >&2 || true
180+
kubectl -n "$NAMESPACE" describe pod "${name}-0" >&2 || true
174181
return 1
175182
fi
176183
sleep 5

‎examples/backups/clickhouse/01-create-strategy.sh‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -160,11 +160,19 @@ spec:
160160
cpu: 200m
161161
memory: 128Mi
162162
securityContext:
163-
# Pod runs apk add at startup; that needs root. Other fields stay
164-
# locked down (no privilege escalation, no extra caps, default
165-
# seccomp profile). Tenants that enforce PSA "restricted" should
166-
# swap this image for one with curl+jq pre-baked and re-enable
167-
# runAsNonRoot on the strategy CR.
163+
# Same field set as the platform's cozy-default-altinity strategy
164+
# (packages/system/backupstrategy-controller/templates/strategy-altinity-default.yaml):
165+
# no privilege escalation, all caps dropped, default seccomp
166+
# profile, and runAsNonRoot deliberately left unset rather than
167+
# false — the Pod only runs curl and jq, and neither the platform
168+
# strategy nor this one pins a UID.
169+
#
170+
# On the default path there is no `apk add` to need root: the image
171+
# above is the platform's, with curl+jq already baked in. Only the
172+
# alpine:3.19 fallback (taken when cozy-default-altinity is absent)
173+
# installs packages at startup, and that is the one case where a
174+
# cluster enforcing PSA "restricted" has to supply its own image
175+
# with the tools pre-baked.
168176
allowPrivilegeEscalation: false
169177
capabilities:
170178
drop: ["ALL"]

‎examples/backups/clickhouse/03-create-bucket.sh‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,11 @@ EOF
2828

2929
log_substep "Waiting for bucket HelmRelease to be Ready..."
3030
wait_hr_ready "bucket-${BUCKET_NAME}" 300
31-
kubectl -n "$NAMESPACE" wait bucketclaims.objectstorage.k8s.io "bucket-${BUCKET_NAME}" --for=jsonpath='{.status.bucketReady}'=true --timeout=300s
31+
kubectl -n "$NAMESPACE" wait bucketclaims.objectstorage.k8s.io "bucket-${BUCKET_NAME}" --for=jsonpath='{.status.bucketReady}'=true --timeout=180s
3232
# Cozystack's bucket app provisions a BucketAccess named "<bucket-name>-backup"
3333
# (the "-backup" suffix is the BucketAccessClass name); the BucketInfo Secret
3434
# carries the same name.
35-
kubectl -n "$NAMESPACE" wait bucketaccesses.objectstorage.k8s.io "bucket-${BUCKET_NAME}-backup" --for=jsonpath='{.status.accessGranted}'=true --timeout=300s
35+
kubectl -n "$NAMESPACE" wait bucketaccesses.objectstorage.k8s.io "bucket-${BUCKET_NAME}-backup" --for=jsonpath='{.status.accessGranted}'=true --timeout=180s
3636

3737
log_substep "Reading bucket coordinates from BucketInfo Secret..."
3838
TMP=$(mktemp)

‎examples/backups/clickhouse/05-create-backupjob.sh‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ log_substep "Waiting for BackupJob to Succeed (this triggers clickhouse-backup c
2727
# `kubectl wait --for=jsonpath='{.status.phase}'=Running` would time out even
2828
# though the BackupJob actually finished. wait_for_field already polls until
2929
# the terminal phase, which covers both the slow and the fast path.
30-
wait_for_field backupjob "$BACKUPJOB_NAME" '{.status.phase}' Succeeded "$NAMESPACE" 600 Failed
30+
wait_for_field backupjob "$BACKUPJOB_NAME" '{.status.phase}' Succeeded "$NAMESPACE" 480 Failed
3131

3232
backup_ref=$(kubectl -n "$NAMESPACE" get backupjob "$BACKUPJOB_NAME" -o jsonpath='{.status.backupRef.name}')
3333
[[ -n "$backup_ref" ]] || { log_error "BackupJob succeeded but BackupRef is empty"; exit 1; }

‎examples/backups/clickhouse/06-restore-in-place.sh‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ spec:
2424
EOF
2525

2626
log_substep "Waiting for in-place RestoreJob to Succeed..."
27-
wait_for_field restorejob "$RESTOREJOB_INPLACE_NAME" '{.status.phase}' Succeeded "$NAMESPACE" 600 Failed
27+
wait_for_field restorejob "$RESTOREJOB_INPLACE_NAME" '{.status.phase}' Succeeded "$NAMESPACE" 480 Failed
2828

2929
log_substep "Verifying sentinel data is restored..."
3030
count=$(clickhouse_query "$CLICKHOUSE_NAME" "SELECT count() FROM default.sentinel" | tr -d '[:space:]')

‎examples/backups/clickhouse/07-restore-to-copy.sh‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ spec:
8686
EOF
8787

8888
log_substep "Waiting for to-copy RestoreJob to Succeed..."
89-
wait_for_field restorejob "$RESTOREJOB_TOCOPY_NAME" '{.status.phase}' Succeeded "$NAMESPACE" 600 Failed
89+
wait_for_field restorejob "$RESTOREJOB_TOCOPY_NAME" '{.status.phase}' Succeeded "$NAMESPACE" 480 Failed
9090

9191
log_substep "Verifying sentinel data exists on the copy..."
9292
count=$(clickhouse_query "$CLICKHOUSE_RESTORE_NAME" "SELECT count() FROM default.sentinel" | tr -d '[:space:]')

‎hack/e2e-chainsaw/clickhouse/chainsaw-test.yaml‎

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -300,22 +300,30 @@ spec:
300300
NAMESPACE="$NAMESPACE" examples/backups/clickhouse/cleanup.sh
301301
check:
302302
($error == null): true
303-
# Budget: the op timeout must exceed the sum of the inner waits it contains,
304-
# per the rule .chainsaw.yaml states for its own catch scripts — otherwise
305-
# Chainsaw SIGKILLs the script part-way and the failure arrives with no
306-
# diagnostics at all, which is strictly worse than an inner wait timing out
307-
# and reporting which resource stalled. The harness's inner budgets sum to
308-
# 3900s (03: 300 HR + 300 claim + 300 access; 04: 300 HR + 300 STS; 05: 600
309-
# BackupJob; 06: 600 RestoreJob; 07: 300 HR + 300 STS + 600 RestoreJob), so
310-
# 65m clears it with room for the SQL and apply steps between them
311-
# (convention 6: justified in-line).
303+
# Budget: the op timeout must be strictly ABOVE the sum of the inner waits,
304+
# per docs/agents/e2e-testing.md — let an inner wait be what fires, because
305+
# it names the resource that stalled, whereas hitting the op timeout means
306+
# Chainsaw SIGKILLs the script and the failure arrives with no diagnostics
307+
# at all (.chainsaw.yaml calls that "strictly worse").
312308
#
313-
# Reaching that ceiling takes a pathological hang: every wait here fails
309+
# Inner budgets, sized against what CI actually takes rather than guessed
310+
# generously — the whole bucket suite completes in ~37s and a backup-enabled
311+
# ClickHouse plus Keeper reaches Ready in ~98s (measured on run 30353894332):
312+
# 03 660s = 300 bucket HR + 180 claim + 180 access
313+
# 04 600s = 300 HR + 300 StatefulSet
314+
# 05 480s = BackupJob to Succeeded
315+
# 06 480s = in-place RestoreJob
316+
# 07 1080s = 300 HR + 300 StatefulSet + 480 to-copy RestoreJob
317+
# sum = 3300s (55m)
318+
# 60m therefore leaves ~5m of headroom for the kubectl apply / exec SQL
319+
# calls interleaved between those waits (convention 6: justified in-line).
320+
#
321+
# Reaching the ceiling at all takes a pathological hang: every wait fails
314322
# fast on a terminal state — wait_for_field bails on phase=Failed and
315323
# wait_hr_ready on Stalled=True — so a genuinely broken backup reports in
316324
# seconds. The happy path is minutes.
317325
- script:
318-
timeout: 65m
326+
timeout: 60m
319327
content: |
320328
set -eu
321329
cd ../../..

‎hack/e2e-chainsaw/clickhouse/clickhouse-backup.yaml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
# Backup-enabled ClickHouse for the clickhouse-backup-1-contracts Test.
1+
# Backup-enabled ClickHouse for the clickhouse-1-backup-contracts Test.
22
#
33
# A single shard / single replica keeps CI cost close to the plain
44
# `clickhouse` suite while still rendering the `clickhouse-backup` sidecar
@@ -11,7 +11,7 @@
1111
# platform useSystemBucket flow marks the S3 secret keys optional so the Pod
1212
# starts before any bucket is projected). So the Pod becomes Ready and the
1313
# chart<->strategy contract is assertable with no reachable object store. The
14-
# real S3 backup/restore round-trip lives in clickhouse-backup-2-roundtrip,
14+
# real S3 backup/restore round-trip lives in clickhouse-2-backup-roundtrip,
1515
# which runs in tenant-root against the in-cluster SeaweedFS.
1616
#
1717
# backup.endpointCA is deliberately left unset: this Test never reaches S3, and

‎hack/select-e2e.sh‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -92,11 +92,15 @@ while IFS= read -r file; do
9292
trigger_any=1
9393
continue ;;
9494
examples/backups/*/*)
95-
# The etcd and postgres backup round-trip tests execute the example
96-
# scripts under examples/backups/<app>/ as their harness, so an edit
97-
# there must run that app's suite. A dir with no matching suite is a
98-
# docs-only demo and stays ignored (adding it to selected_apps would
99-
# empty the final intersection and trip the full-suite safety net).
95+
# The etcd, postgres, mariadb and clickhouse backup round-trip tests
96+
# execute the example scripts under examples/backups/<app>/ as their
97+
# harness, so an edit there must run that app's suite. This mapping is
98+
# also why a round-trip Test belongs in its app's own suite dir: put it
99+
# in a dir whose name does not match examples/backups/<app>/ and an edit
100+
# to the harness selects a suite that cannot exercise it. A dir with no
101+
# matching suite is a docs-only demo and stays ignored (adding it to
102+
# selected_apps would empty the final intersection and trip the
103+
# full-suite safety net).
100104
app=$(echo "$file" | sed -nE 's,^examples/backups/([^/]+)/.*,\1,p')
101105
if echo "$all_apps" | grep -Fxq "$app"; then
102106
selected_apps="$selected_apps $app"

‎hack/select-e2e_test.bats‎

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -99,21 +99,6 @@
9999
echo "$output" | grep -q "kubernetes-oidc-customconfig"
100100
}
101101

102-
@test "clickhouse backup harness edit selects the suite that runs it" {
103-
# The clickhouse suite dir holds the backup round-trip Test that executes
104-
# examples/backups/clickhouse/run-all.sh, so an edit to that harness has to
105-
# select `clickhouse` — the suite is what runs the scripts. Keeping the
106-
# round-trip in the app's own suite dir (rather than a clickhouse-backup one)
107-
# is what makes the examples/backups/<app> -> <app> mapping line up; this
108-
# locks that alignment for clickhouse specifically.
109-
tmp=$(mktemp -d)
110-
trap 'rm -rf "$tmp"' EXIT
111-
cp -r packages/core/platform/sources "$tmp/sources"
112-
echo "examples/backups/clickhouse/run-all.sh" > "$tmp/diff"
113-
output=$(hack/select-e2e.sh "$tmp/diff" "$tmp/sources")
114-
[ "$output" = "clickhouse" ]
115-
}
116-
117102
@test "dashboards-only diff selects nothing (path is plural)" {
118103
tmp=$(mktemp -d)
119104
trap 'rm -rf "$tmp"' EXIT

0 commit comments

Comments
 (0)