Skip to content

Commit c7dca5f

Browse files
myasnikovdaniilmyasnikovdaniil
committed
test(e2e): record the upgrade lane's known failure instead of staying red
The lane is advisory, and an advisory check that is red for a reason everyone already knows trains reviewers to ignore it — then it stops working on the day it finds something new. It currently has exactly one such reason: tenant-test/vm-instance-test does not return to Ready across an upgrade (#3984), while the same workload passes on a fresh install in the very same run. Give the lane a ledger rather than a longer timeout or a suite exclusion. hack/e2e-wait-hr-ready.sh learns an optional EXPECTED_NOT_READY_FILE, and only hack/e2e-upgrade-apply.bats sets it; unset, the script behaves as it always did, down to making no second listing call on the happy path, so the install gate is untouched. The entries have to expire on their own or the file becomes the thing it was meant to prevent, a stale explanation outliving its defect. Three rules do that, all failures rather than warnings: - a not-Ready release that is NOT listed still fails the gate; - a listed release that is Ready again fails until its line is deleted; - a line naming no release in the cluster fails too. So the fastest way to remove a line is to fix the defect, and the file cannot be wrong about the present without saying so. A named file that cannot be read is also a failure: treating it as "nothing expected" would turn a typo into a gate that quietly accepts more than it claims. hack/wait-hr-ready_test.bats drives the real script through a kubectl stub over all of it. It is deliberately not named hack/e2e-*.bats: the unit target is $(filter-out hack/e2e-%.bats,...), so a file named after the script it tests would be excluded from the suite and never run. Verified non-vacuous by mutation — disabling each of the three rules in turn turns exactly one case red, and the missing-file refusal likewise. Assisted-By: Claude <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
1 parent f6c7448 commit c7dca5f

4 files changed

Lines changed: 319 additions & 14 deletions

File tree

‎hack/e2e-chainsaw-upgrade/expected-not-ready‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
# Releases the upgrade lane accepts as not-Ready after the upgrade, one
2+
# `namespace/name` per line; everything after the reference is free text and
3+
# should say what it is and where it is tracked.
4+
#
5+
# This file is read ONLY by the upgrade lane (hack/e2e-upgrade-apply.bats passes
6+
# it to hack/e2e-wait-hr-ready.sh as EXPECTED_NOT_READY_FILE). The install gate
7+
# never sees it, so nothing here can weaken the per-PR e2e suite.
8+
#
9+
# It exists because that lane is advisory: red for a known reason, every run,
10+
# teaches reviewers to ignore it, and then it stops working on the day it finds
11+
# something new. An entry buys exactly one thing — a failure that is already
12+
# tracked stops hiding the next one.
13+
#
14+
# Entries expire on their own, and both rules are failures rather than warnings.
15+
# A release listed here that is Ready again fails the lane until its line is
16+
# deleted, and a line naming no release in the cluster fails it too. So the
17+
# fastest way to remove a line is to fix the defect; the second fastest is to
18+
# find out it was fixed already.
19+
#
20+
# Adding a line is a claim that the failure is understood well enough to be
21+
# tracked elsewhere. If it is not, the lane going red is the correct outcome.
22+
23+
tenant-test/vm-instance-test https://github.com/cozystack/cozystack/issues/3984 VM does not return to Ready across an upgrade; passes on a fresh install

‎hack/e2e-upgrade-apply.bats‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,15 @@
3535
# platform against the new source. Generous budget: the upgrade re-pulls image
3636
# deltas across three nodes and rolls stateful workloads, and a parent HR that
3737
# briefly flips InProgress re-reconciles every 1m until it converges.
38-
hack/e2e-wait-hr-ready.sh 20m
38+
#
39+
# EXPECTED_NOT_READY_FILE names the releases this lane already knows about, so
40+
# a NEW upgrade regression is still a red rather than one more line in a
41+
# permanently red check. The gate fails on anything not in that file, and also
42+
# when a listed release is Ready again or is not in the cluster at all, so the
43+
# file cannot outlive what it describes. The install gate is not given the
44+
# variable and behaves exactly as before.
45+
EXPECTED_NOT_READY_FILE=hack/e2e-chainsaw-upgrade/expected-not-ready \
46+
hack/e2e-wait-hr-ready.sh 20m
3947
}
4048

4149
@test "Migration stamp advanced to the current target" {

‎hack/e2e-wait-hr-ready.sh‎

Lines changed: 107 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -15,25 +15,119 @@
1515
# (e.g. a rejected CRD) is in the test log, not only in the cozyreport;
1616
# - exit 1 on any non-Ready HR.
1717
#
18+
# EXPECTED_NOT_READY_FILE (optional) names a file of releases already known to
19+
# be not-Ready, one `namespace/name` per line, rest of the line free text. Only
20+
# the upgrade lane sets it. Unset — the install path — this script behaves
21+
# exactly as it always did, down to making no extra call.
22+
#
23+
# Why the upgrade lane needs it, and why it is a ledger rather than a bump in
24+
# the timeout or a suite exclusion. That lane is advisory, and an advisory check
25+
# that is red for a reason everyone already knows trains readers to ignore it;
26+
# the day it catches something new, nobody looks. So the known set is written
27+
# down, the gate accepts exactly that set, and anything else still fails.
28+
#
29+
# The entries have to expire on their own or the file becomes the very thing it
30+
# was meant to prevent — a stale explanation that outlives its defect by a
31+
# release or two. Two rules do that, and both are failures rather than warnings:
32+
# an entry whose release is Ready again fails the gate until it is deleted, and
33+
# an entry naming no release in the cluster fails it too. A ledger that cannot
34+
# be wrong about the present is worth reading; one that can is worse than none.
35+
#
1836
# Usage: e2e-wait-hr-ready.sh [timeout] (default 15m)
1937
# Runs under /bin/sh (dash on Ubuntu CI) — no bashisms, no pipefail.
2038
# -----------------------------------------------------------------------------
2139
set -eu
2240

2341
TIMEOUT="${1:-15m}"
42+
EXPECTED_FILE="${EXPECTED_NOT_READY_FILE:-}"
43+
44+
# Print the full Ready-condition message per named release; kubectl's STATUS
45+
# column truncates it, and the real error (a rejected CRD, a failed upgrade)
46+
# lives in the part it cuts.
47+
dump_refs() {
48+
while IFS= read -r _ref; do
49+
[ -n "$_ref" ] || continue
50+
echo "--- Non-ready HelmRelease: $_ref" >&2
51+
kubectl get hr -n "${_ref%%/*}" "${_ref#*/}" \
52+
-o jsonpath='{range .status.conditions[*]}{.type}={.status} reason={.reason}: {.message}{"\n"}{end}' >&2 || true
53+
done < "$1"
54+
}
55+
56+
wait_rc=0
57+
kubectl wait hr --all -A --timeout="$TIMEOUT" --for=condition=ready || wait_rc=1
58+
59+
# ── Install path: unchanged, including the absence of a second listing call on
60+
# the happy path.
61+
if [ -z "$EXPECTED_FILE" ]; then
62+
if [ "$wait_rc" -eq 0 ]; then
63+
exit 0
64+
fi
65+
# The wait timed out on at least one HR. Re-list (covers HRs created after the
66+
# wait began) and surface the real reason per non-Ready HR.
67+
kubectl get hr -A || true
68+
kubectl get hr -A --no-headers | awk '$4 != "True"' | while read -r ns name _; do
69+
echo "--- Non-ready HelmRelease: $ns/$name" >&2
70+
kubectl get hr -n "$ns" "$name" \
71+
-o jsonpath='{range .status.conditions[*]}{.type}={.status} reason={.reason}: {.message}{"\n"}{end}' >&2 || true
72+
done
73+
echo "Some HelmReleases failed to reconcile" >&2
74+
exit 1
75+
fi
76+
77+
# ── Ledger path. A named file that cannot be read is a failure, not an empty
78+
# expectation: the whole point is that the accepted set is explicit, and
79+
# treating an unreadable file as "nothing expected" would turn a typo into a
80+
# gate that quietly accepts more than it says.
81+
if [ ! -f "$EXPECTED_FILE" ]; then
82+
echo "FAIL: EXPECTED_NOT_READY_FILE=$EXPECTED_FILE is not a readable file" >&2
83+
exit 1
84+
fi
85+
86+
tmp=$(mktemp -d)
87+
88+
# The listing goes through a file, and not into a pipeline: a pipeline takes the
89+
# exit status of its LAST command, so a failed `kubectl get` would leave an
90+
# empty list and every check below would pass having compared nothing.
91+
kubectl get hr -A --no-headers > "$tmp/hr"
92+
# Column 4 of `kubectl get hr -A --no-headers` is READY (NAMESPACE NAME AGE
93+
# READY STATUS).
94+
awk '$4 == "True" { print $1"/"$2 }' "$tmp/hr" | sort -u > "$tmp/ready"
95+
awk '$4 != "True" { print $1"/"$2 }' "$tmp/hr" | sort -u > "$tmp/notready"
96+
awk '{ print $1"/"$2 }' "$tmp/hr" | sort -u > "$tmp/all"
97+
98+
# Full-line comments only. A trailing `#` is not a comment marker here because
99+
# the free text after the reference is expected to carry issue URLs, which
100+
# contain one.
101+
grep -v '^[[:space:]]*#' "$EXPECTED_FILE" | awk 'NF { print $1 }' | sort -u > "$tmp/expected" || true
102+
103+
comm -23 "$tmp/notready" "$tmp/expected" > "$tmp/unexpected"
104+
comm -12 "$tmp/expected" "$tmp/ready" > "$tmp/came_back"
105+
comm -23 "$tmp/expected" "$tmp/all" > "$tmp/absent"
106+
107+
rc=0
108+
if [ -s "$tmp/unexpected" ]; then
109+
echo "FAIL: HelmRelease(s) not Ready and not recorded in $EXPECTED_FILE:" >&2
110+
cat "$tmp/unexpected" >&2
111+
dump_refs "$tmp/unexpected"
112+
rc=1
113+
fi
114+
if [ -s "$tmp/came_back" ]; then
115+
echo "FAIL: recorded as expected-not-Ready but Ready now — delete these entries from $EXPECTED_FILE:" >&2
116+
cat "$tmp/came_back" >&2
117+
rc=1
118+
fi
119+
if [ -s "$tmp/absent" ]; then
120+
echo "FAIL: recorded in $EXPECTED_FILE but no such HelmRelease in the cluster — delete or correct these entries:" >&2
121+
cat "$tmp/absent" >&2
122+
rc=1
123+
fi
24124

25-
if kubectl wait hr --all -A --timeout="$TIMEOUT" --for=condition=ready; then
26-
exit 0
125+
if [ "$rc" -eq 0 ] && [ -s "$tmp/notready" ]; then
126+
echo "::warning::upgrade lane accepted known not-Ready HelmRelease(s); see $EXPECTED_FILE" >&2
127+
echo "=== expected not-Ready after upgrade — recorded defects, NOT a healthy platform ===" >&2
128+
grep -v '^[[:space:]]*#' "$EXPECTED_FILE" | awk 'NF' >&2 || true
129+
dump_refs "$tmp/notready"
27130
fi
28131

29-
# The wait timed out on at least one HR. Re-list (covers HRs created after the
30-
# wait began) and surface the real reason per non-Ready HR.
31-
kubectl get hr -A || true
32-
# Column 4 of `kubectl get hr -A --no-headers` is READY (NAMESPACE NAME AGE READY STATUS).
33-
kubectl get hr -A --no-headers | awk '$4 != "True"' | while read -r ns name _; do
34-
echo "--- Non-ready HelmRelease: $ns/$name" >&2
35-
kubectl get hr -n "$ns" "$name" \
36-
-o jsonpath='{range .status.conditions[*]}{.type}={.status} reason={.reason}: {.message}{"\n"}{end}' >&2 || true
37-
done
38-
echo "Some HelmReleases failed to reconcile" >&2
39-
exit 1
132+
rm -rf "$tmp"
133+
exit "$rc"

‎hack/wait-hr-ready_test.bats‎

Lines changed: 180 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,180 @@
1+
#!/usr/bin/env bats
2+
# -----------------------------------------------------------------------------
3+
# Unit tests for the EXPECTED_NOT_READY_FILE half of hack/e2e-wait-hr-ready.sh.
4+
#
5+
# That half can turn a red gate green, so what it accepts and what it refuses is
6+
# pinned here rather than left to the one lane that sets the variable. Each test
7+
# drives the real script through a `kubectl` stub on PATH, so the parsing, the
8+
# set arithmetic and the exit status are exercised end to end; there is no
9+
# cluster and no HelmRelease anywhere.
10+
#
11+
# NOT named hack/e2e-*.bats on purpose: the Makefile's bats-unit-tests target is
12+
# `$(filter-out hack/e2e-%.bats,...)`, so a file named for the script it tests
13+
# would be silently excluded from the unit suite and never run.
14+
#
15+
# Run under cozytest.sh (hack/cozytest.sh hack/wait-hr-ready_test.bats), NOT
16+
# real bats: there is no `run`/`$status`/setup(); each @test is a shell function
17+
# under `set -eu -x`, so assertions are direct shell tests that exit non-zero on
18+
# failure. The script is invoked inside an `if`, because `set -e` exempts a
19+
# condition but would abort the test before a following `rc=$?` could read a
20+
# non-zero — read after the fact, such an assertion could only ever see 0.
21+
#
22+
# Each test removes its scratch directory as the last statement of its body
23+
# rather than from an EXIT handler: a handler installed inside an @test replaces
24+
# the one the bats binary keeps for its own bookkeeping, and a test that then
25+
# fails prints no TAP line at all. hack/bats-no-exit-trap.bats enforces this.
26+
# -----------------------------------------------------------------------------
27+
28+
HACK_DIR="$(cd "$(dirname "${BATS_TEST_FILENAME:-$0}")" && pwd)"
29+
GATE="$HACK_DIR/e2e-wait-hr-ready.sh"
30+
31+
# A kubectl stub that answers the two calls the ledger path makes: `wait` (whose
32+
# exit status the caller only uses to decide whether to look further) and `get hr
33+
# -A --no-headers`, whose rows are the state under test. The rows are passed in
34+
# as the fixture; anything else the script might call answers success with no
35+
# output, so an unexpected call cannot masquerade as a listing.
36+
#
37+
# WAIT_RC is deliberately settable: the gate must reach the same verdict whether
38+
# the wait timed out or everything went Ready while it was running, and the
39+
# came-back-Ready case can only be reached with a wait that succeeded.
40+
_stub() {
41+
_dir=$1
42+
_rows=$2
43+
_wait_rc=${3:-1}
44+
mkdir -p "$_dir"
45+
{
46+
printf '%s\n' '#!/bin/sh'
47+
printf '%s\n' 'case "$1 $2" in'
48+
printf ' "wait hr") exit %s ;;\n' "$_wait_rc"
49+
printf '%s\n' 'esac'
50+
printf '%s\n' 'if [ "$1" = "get" ] && [ "$2" = "hr" ] && [ "$3" = "-A" ]; then'
51+
printf ' cat <<'"'"'ROWS'"'"'\n%s\nROWS\n' "$_rows"
52+
printf '%s\n' ' exit 0'
53+
printf '%s\n' 'fi'
54+
printf '%s\n' 'exit 0'
55+
} > "$_dir/kubectl"
56+
chmod +x "$_dir/kubectl"
57+
return 0
58+
}
59+
60+
# NAMESPACE NAME AGE READY STATUS — column 4 is the one the gate reads.
61+
_rows_one_bad() {
62+
printf '%s\n' \
63+
'cozy-system cozystack 40m True Helm upgrade succeeded' \
64+
'tenant-test vm-instance-test 30m False timeout waiting for VirtualMachine'
65+
}
66+
67+
_rows_all_good() {
68+
printf '%s\n' \
69+
'cozy-system cozystack 40m True Helm upgrade succeeded' \
70+
'tenant-test vm-instance-test 30m True Helm upgrade succeeded'
71+
}
72+
73+
@test "a not-Ready release named in the file is accepted, and the file is echoed" {
74+
tmp=$(mktemp -d)
75+
_stub "$tmp/bin" "$(_rows_one_bad)"
76+
printf '%s\n' '# a comment line is not an entry' \
77+
'tenant-test/vm-instance-test https://example.invalid/issues/1 VM does not return across upgrade' \
78+
> "$tmp/expected"
79+
80+
if out=$(PATH="$tmp/bin:$PATH" EXPECTED_NOT_READY_FILE="$tmp/expected" sh "$GATE" 1s 2>&1); then
81+
:
82+
else
83+
echo "FAIL: a recorded not-Ready release should not fail the gate: $out" >&2
84+
false
85+
fi
86+
# The reason has to reach the log, or the next reader sees an accepted red with
87+
# no way to tell what was accepted.
88+
echo "$out" | grep -q 'VM does not return across upgrade'
89+
echo "$out" | grep -q 'NOT a healthy platform'
90+
rm -rf "$tmp"
91+
}
92+
93+
@test "a not-Ready release absent from the file still fails" {
94+
# The whole point: the ledger accepts a named set, never "whatever is broken".
95+
tmp=$(mktemp -d)
96+
_stub "$tmp/bin" "$(_rows_one_bad)"
97+
printf '%s\n' '# nothing recorded' > "$tmp/expected"
98+
99+
if out=$(PATH="$tmp/bin:$PATH" EXPECTED_NOT_READY_FILE="$tmp/expected" sh "$GATE" 1s 2>&1); then
100+
echo "FAIL: an unrecorded not-Ready release passed the gate: $out" >&2
101+
false
102+
fi
103+
echo "$out" | grep -q 'not Ready and not recorded'
104+
echo "$out" | grep -q 'tenant-test/vm-instance-test'
105+
rm -rf "$tmp"
106+
}
107+
108+
@test "an entry whose release is Ready again fails until the line is deleted" {
109+
# This is what stops the file outliving the defect it describes. Reached with a
110+
# wait that SUCCEEDED, which is the real shape of the fix landing.
111+
tmp=$(mktemp -d)
112+
_stub "$tmp/bin" "$(_rows_all_good)" 0
113+
printf '%s\n' 'tenant-test/vm-instance-test https://example.invalid/issues/1 stale' > "$tmp/expected"
114+
115+
if out=$(PATH="$tmp/bin:$PATH" EXPECTED_NOT_READY_FILE="$tmp/expected" sh "$GATE" 1s 2>&1); then
116+
echo "FAIL: a stale entry was accepted: $out" >&2
117+
false
118+
fi
119+
echo "$out" | grep -q 'Ready now'
120+
echo "$out" | grep -q 'tenant-test/vm-instance-test'
121+
rm -rf "$tmp"
122+
}
123+
124+
@test "an entry naming no release in the cluster fails too" {
125+
# A renamed or removed release leaves a line that can never expire on the
126+
# rule above, so it has its own. Otherwise the file accumulates entries that
127+
# are neither true nor falsifiable.
128+
tmp=$(mktemp -d)
129+
_stub "$tmp/bin" "$(_rows_all_good)" 0
130+
printf '%s\n' 'tenant-test/no-such-release https://example.invalid/issues/2 typo' > "$tmp/expected"
131+
132+
if out=$(PATH="$tmp/bin:$PATH" EXPECTED_NOT_READY_FILE="$tmp/expected" sh "$GATE" 1s 2>&1); then
133+
echo "FAIL: an entry naming nothing was accepted: $out" >&2
134+
false
135+
fi
136+
echo "$out" | grep -q 'no such HelmRelease'
137+
rm -rf "$tmp"
138+
}
139+
140+
@test "a file that cannot be read fails instead of expecting nothing" {
141+
# Treating an unreadable path as an empty expectation would turn a typo in the
142+
# variable into a gate that accepts more than it claims — and it would do it
143+
# silently, since every release being Ready looks identical to that.
144+
tmp=$(mktemp -d)
145+
_stub "$tmp/bin" "$(_rows_one_bad)"
146+
147+
if out=$(PATH="$tmp/bin:$PATH" EXPECTED_NOT_READY_FILE="$tmp/does-not-exist" sh "$GATE" 1s 2>&1); then
148+
echo "FAIL: a missing ledger file was treated as no expectations: $out" >&2
149+
false
150+
fi
151+
echo "$out" | grep -q 'is not a readable file'
152+
rm -rf "$tmp"
153+
}
154+
155+
@test "with no file set the gate is the install gate: every release must be Ready" {
156+
tmp=$(mktemp -d)
157+
_stub "$tmp/bin" "$(_rows_one_bad)"
158+
159+
if out=$(PATH="$tmp/bin:$PATH" sh "$GATE" 1s 2>&1); then
160+
echo "FAIL: the unconfigured gate accepted a not-Ready release: $out" >&2
161+
false
162+
fi
163+
echo "$out" | grep -q 'Some HelmReleases failed to reconcile'
164+
# And nothing about a ledger, which the install path must not even mention.
165+
[ "$(echo "$out" | grep -c 'recorded')" -eq 0 ]
166+
rm -rf "$tmp"
167+
}
168+
169+
@test "with no file set an all-Ready cluster passes" {
170+
tmp=$(mktemp -d)
171+
_stub "$tmp/bin" "$(_rows_all_good)" 0
172+
173+
if out=$(PATH="$tmp/bin:$PATH" sh "$GATE" 1s 2>&1); then
174+
:
175+
else
176+
echo "FAIL: the unconfigured gate failed a healthy cluster: $out" >&2
177+
false
178+
fi
179+
rm -rf "$tmp"
180+
}

0 commit comments

Comments
 (0)