Skip to content

Commit 8d5ab4c

Browse files
lexfreilexfrei
committed
fix(hack): arm cozytest cluster captures for e2e suites only
The runner's on-failure EXIT trap gates each cluster capture on a binary being installed rather than on there being a cluster to read. A unit runner has kubectl and no cluster, so a red unit test walked into those legs on its way out. What that cost depends on how kubectl fails. Against an endpoint that refuses immediately it is a second; against one that hangs, measured with a stub that never returns, it is ~28s for the previous-logs leg, which self-bounds its pod list and so never reaches the 300s ceiling above it, plus the full 600s for the data-plane leg, whose first pod list carries no bound of its own. Ten minutes of dead waiting for a failure that should cost seconds. Where crust-gather is installed too the bill stops being empty and starts being wrong: that leg needs no reachable cluster to be pointless, only an ambient KUBECONFIG, and it will spend up to 360s snapshotting whatever cluster the current context names. Gate the three legs on the e2e- prefix instead, read from the suite's own name or from the directory holding it. At the top level that prefix is what the project already sorts on: the Makefile builds BATS_UNIT_FILES as $(filter-out hack/e2e-%.bats,$(wildcard hack/*.bats)), so a top-level e2e-* suite is exactly the one `make unit-tests` refuses to run and packages/core/testing/Makefile runs against a live cluster. That wildcard is not recursive, and hack/e2e-apps/ holds live-cluster suites whose own filenames carry no prefix, so the directory is checked too: matching the basename alone would take the captures away from suites that need them, which is the opposite of this change. The e2e path is otherwise unchanged, every leg, bound and message as it was. Not a reachability probe, though that would also tell a cluster from a bare binary. A failing e2e run is frequently one whose apiserver is degraded, and a probe answering "no" there would skip the snapshot in precisely the case the snapshot exists for. The test drives the runner as a subprocess over generated fixtures that differ only in where they sit, with stub kubectl and crust-gather on PATH. Stubbing both matters: the runner calls crust-gather directly rather than through a script that needs kubectl output first, so a kubectl-only stub would leave that leg reaching for the real binary against the ambient KUBECONFIG. Each leg is pinned by the artefact it leaves in the report tree, not only by the one leg that echoes, so no single leg can be ungated without a test going red; each was checked by ungating it alone. All three directions are asserted, and the two armed cases are the positive control for the disarmed one, whose assertion is negative and would otherwise pass just as well if the harness ran nothing; that case also pins that the fixture did fail. Two shell traps are handled rather than left to be rediscovered. The awk transform rewrites any line beginning `@test "` and any line that is exactly `}`, and knows nothing of heredocs, so a fixture written as a heredoc has its own `@test` line harvested into the outer suite as a phantom test that fails first and ends the run before the real tests; the fixtures are built with printf instead. And `set -e` ignores a command whose status is inverted with `!`, so the negative assertion is spelled as an `if`. Both were checked by building the broken variant, not reasoned about. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
1 parent 605030b commit 8d5ab4c

2 files changed

Lines changed: 194 additions & 3 deletions

File tree

‎hack/cozytest-capture-gate.bats‎

Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,145 @@
1+
#!/usr/bin/env bats
2+
# -----------------------------------------------------------------------------
3+
# Behavioural tests for the cluster-capture gate in hack/cozytest.sh.
4+
#
5+
# The runner arms its on-failure cluster captures from an EXIT trap. Those
6+
# captures belong to the e2e suites, which run against a live cluster; a unit
7+
# suite has no cluster to snapshot, so arming them there buys an empty capture
8+
# and pays the capture's timeouts for it. The gate therefore keys on which kind
9+
# of suite is running rather than on whether a kubectl binary happens to be
10+
# installed, because a unit runner has the binary and no cluster.
11+
#
12+
# Both directions are asserted, and they share one fixture generator that
13+
# differs only in the fixture's filename. That pairing is the point: the
14+
# unit-side assertion is a negative one, and a negative assertion passes just as
15+
# well when the harness is broken and nothing ran at all. The e2e-side test is
16+
# the positive control for exactly that failure, and the unit-side test
17+
# additionally pins that the run did fail, so "no captures" cannot be satisfied
18+
# by "no test".
19+
#
20+
# The stub kubectl is what makes this a real test of the gate: with no kubectl
21+
# on PATH every capture leg is skipped for a second reason and both directions
22+
# would agree for the wrong cause.
23+
#
24+
# awk-transform constraint (hack/cozytest.sh rewrites this file before sourcing
25+
# it): it turns any line beginning `@test "` into a function header, and any
26+
# line that is exactly `}` into `return 0` plus the brace. Neither rule knows
27+
# what a heredoc is. So a fixture written as a heredoc here has its own `@test`
28+
# line harvested into THIS suite: the runner registers a phantom extra test,
29+
# runs it first, and the fixture's `false` ends the run before any real test
30+
# executes. Checked by building that variant rather than reasoned about -- it
31+
# fails loudly rather than silently, but it fails the wrong thing, and the gate
32+
# is then covered by nothing. The fixtures are built with printf, which puts the
33+
# `@test` text inside a format string where no rule matches it.
34+
#
35+
# Title syntax constraints (also inherited from that parser):
36+
# - Titles delimited by ASCII double quotes; embedded quotes truncate.
37+
# - Only [A-Za-z0-9] from the title survives into the function name, so keep
38+
# titles distinctive in their alphanumeric run.
39+
#
40+
# Run with: hack/cozytest.sh hack/cozytest-capture-gate.bats
41+
# -----------------------------------------------------------------------------
42+
43+
HACK_DIR="$(cd "$(dirname "${BATS_TEST_FILENAME:-$0}")" && pwd)"
44+
RUNNER="$HACK_DIR/cozytest.sh"
45+
46+
# Marker printed by the runner's data-plane capture leg. Chosen as one probe
47+
# because the runner echoes it before invoking the capture, so the assertion
48+
# reads the gate's decision rather than the capture's outcome.
49+
DATAPLANE_MARKER='capturing host->pod data-plane'
50+
51+
# The gate covers three legs, and each leaves a distinct artefact under the
52+
# snapshot directory. Pinning all three from the report tree is what keeps any
53+
# one of them from being ungated without a test noticing: the echo above covers
54+
# only the data-plane leg, and the crust-gather leg is the one whose cost is
55+
# worst, since it needs no reachable cluster to fire, only an ambient
56+
# KUBECONFIG.
57+
assert_legs_armed() {
58+
_rdir=$1 _name=$2
59+
[ -f "$_rdir/snapshots/$_name/crust-gather.log" ]
60+
[ -d "$_rdir/snapshots/$_name/dataplane" ]
61+
[ -f "$_rdir/snapshots/$_name/previous-logs/capture-notes.txt" ]
62+
}
63+
64+
assert_legs_disarmed() {
65+
_rdir=$1 _name=$2
66+
for _leg in crust-gather.log dataplane previous-logs; do
67+
if [ -e "$_rdir/snapshots/$_name/$_leg" ]; then
68+
echo "cluster capture leg $_leg fired for a unit suite"
69+
exit 1
70+
fi
71+
done
72+
}
73+
74+
# Build a scratch dir holding stub cluster tools and a single-test fixture whose
75+
# test fails. $1 is the fixture's path relative to the scratch dir, which is the
76+
# only thing that differs between the directions under test.
77+
#
78+
# Both kubectl and crust-gather are stubbed, and crust-gather is not optional:
79+
# the runner invokes it directly rather than through a script that needs
80+
# kubectl output first, so a kubectl-only stub leaves that leg reaching for the
81+
# real binary against the ambient KUBECONFIG. On a machine that has both -- a
82+
# maintainer's laptop, the e2e sandbox image -- `make unit-tests` would then
83+
# snapshot whatever cluster happens to be current, for up to the leg's 390s.
84+
# That is the same bill this change exists to stop paying.
85+
make_fixture_dir() {
86+
_dir=$(mktemp -d)
87+
mkdir -p "$_dir/bin" "$_dir/$(dirname "$1")"
88+
for _tool in kubectl crust-gather; do
89+
printf '#!/bin/sh\nexit 0\n' >"$_dir/bin/$_tool"
90+
chmod +x "$_dir/bin/$_tool"
91+
done
92+
printf '@test "fixture fails on purpose" {\n false\n}\n' >"$_dir/$1"
93+
printf '%s' "$_dir"
94+
}
95+
96+
# Run the runner against a fixture with the stub kubectl first on PATH and the
97+
# report directory redirected into the scratch dir, so a capture that does fire
98+
# writes there instead of into the worktree. The output goes to a file rather
99+
# than a variable on purpose: these suites run under `set -x`, and a variable
100+
# holding the fixture's output gets expanded into the trace of whatever command
101+
# reads it, printing the fixture's deliberate "Test failed" line into the job
102+
# log where it reads as a real failure.
103+
run_fixture() {
104+
_fdir=$1 _fname=$2
105+
PATH="$_fdir/bin:$PATH" COZY_REPORT_DIR="$_fdir/report" \
106+
"$RUNNER" "$_fdir/$_fname" >"$_fdir/out" 2>&1 || true
107+
}
108+
109+
@test "an e2e suite failing still arms the cluster captures" {
110+
dir=$(make_fixture_dir e2e-gate-fixture.bats)
111+
run_fixture "$dir" e2e-gate-fixture.bats
112+
# Positive control for the negative assertion in the last test: same fixture,
113+
# same stubs, e2e filename.
114+
grep -qF "$DATAPLANE_MARKER" "$dir/out"
115+
assert_legs_armed "$dir/report" e2e-gate-fixture
116+
rm -rf "$dir"
117+
}
118+
119+
@test "a live cluster suite under an e2e directory keeps its captures" {
120+
# hack/e2e-apps/ holds suites whose own filenames carry no prefix. Matching
121+
# the basename alone disarms them, which takes the captures away from a suite
122+
# that runs against a real cluster and therefore has state worth capturing.
123+
dir=$(make_fixture_dir e2e-apps/plain-named-suite.bats)
124+
run_fixture "$dir" e2e-apps/plain-named-suite.bats
125+
grep -qF "$DATAPLANE_MARKER" "$dir/out"
126+
assert_legs_armed "$dir/report" plain-named-suite
127+
rm -rf "$dir"
128+
}
129+
130+
@test "a unit suite failing does not arm the cluster captures" {
131+
dir=$(make_fixture_dir plain-gate-fixture.bats)
132+
run_fixture "$dir" plain-gate-fixture.bats
133+
# The run must have failed, or "no captures" would be satisfied by "no test".
134+
grep -qF 'Test failed' "$dir/out"
135+
# Spelled as an if rather than `! grep ...`: `set -e` is specified to ignore a
136+
# command whose status is inverted with `!`, so the negated form would report
137+
# success even when the marker is present, which is the whole thing this test
138+
# exists to catch.
139+
if grep -qF "$DATAPLANE_MARKER" "$dir/out"; then
140+
echo "cluster captures were armed for a unit suite"
141+
exit 1
142+
fi
143+
assert_legs_disarmed "$dir/report" plain-gate-fixture
144+
rm -rf "$dir"
145+
}

‎hack/cozytest.sh‎

Lines changed: 49 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,52 @@ TMP_SH=$(mktemp) || { echo "Failed to create temp file" >&2; exit 1; }
9393
# forward — so a test that creates tenant clusters (run-kubernetes.sh) captures
9494
# them from its OWN in-subshell EXIT trap, while the forward is still alive.
9595
COZY_REPORT_DIR="${COZY_REPORT_DIR:-_out/cozyreport}"
96+
# Which suites get the cluster captures below. They read cluster state, so they
97+
# belong to the e2e suites and to nothing else. `command -v kubectl` does not
98+
# tell the two apart: a unit runner has the binary and no cluster, so a red unit
99+
# test used to walk into the legs below on its way out.
100+
#
101+
# What that costs depends on how kubectl fails. Against an endpoint that refuses
102+
# immediately it is a second; against one that hangs -- an unreachable address, a
103+
# context left pointing at a torn-down cluster -- it was measured at ~28s for the
104+
# previous-logs leg, which self-bounds its pod list and so never reaches the 300s
105+
# ceiling set below it, plus the full 600s for the data-plane leg, whose first
106+
# pod list carries no bound of its own. Ten minutes of dead waiting inside a job
107+
# budget, for a failure that should cost seconds.
108+
#
109+
# Where crust-gather is installed as well the bill stops being empty and starts
110+
# being wrong: that leg does not need a reachable cluster to be pointless, it
111+
# needs an ambient KUBECONFIG, and it will spend up to 360s snapshotting whatever
112+
# cluster the current context happens to name.
113+
#
114+
# The discriminator is the e2e- prefix, on the suite's own name or on the
115+
# directory holding it. At the top level the prefix is what the project already
116+
# sorts on: the Makefile builds BATS_UNIT_FILES as
117+
# $(filter-out hack/e2e-%.bats,$(wildcard hack/*.bats)), so a top-level e2e-*
118+
# suite is precisely one `make unit-tests` refuses to run and
119+
# packages/core/testing/Makefile runs against a live cluster instead.
120+
#
121+
# That wildcard is not recursive, so it says nothing about subdirectories, and
122+
# hack/e2e-apps/ holds live-cluster suites whose own filenames carry no prefix.
123+
# Matching the directory too keeps them armed; matching only the basename would
124+
# take the captures away from suites that need them, which is the opposite of
125+
# the fix. Neither test is a claim that every future live-cluster suite will be
126+
# named this way -- it is the only signal the runner has, and a suite placed
127+
# outside both shapes gets no captures.
128+
#
129+
# Deliberately not a reachability probe. Gating on "can I talk to an apiserver"
130+
# would disarm the captures in the case they exist for: a failing e2e run is
131+
# often one whose apiserver is degraded, and a probe answering "no" there would
132+
# skip the snapshot precisely when it is the evidence.
133+
case "$(basename "$TEST_FILE")" in
134+
e2e-*) _cozy_cluster_captures=1 ;;
135+
*)
136+
case "$(basename "$(dirname "$TEST_FILE")")" in
137+
e2e-*) _cozy_cluster_captures=1 ;;
138+
*) _cozy_cluster_captures=0 ;;
139+
esac
140+
;;
141+
esac
96142
_cozy_on_exit() {
97143
_rc=$?
98144
if [ "$_rc" -ne 0 ]; then
@@ -107,7 +153,7 @@ _cozy_on_exit() {
107153
# per pod by default, so each further restart of a still-looping pod drops
108154
# the instance we are here to read — and the snapshot leg alone can hold the
109155
# trap for 390s. The other two captures read state that keeps.
110-
if command -v kubectl >/dev/null 2>&1 && [ -x "$(dirname "$0")/e2e-capture-previous-logs.sh" ]; then
156+
if [ "$_cozy_cluster_captures" -eq 1 ] && command -v kubectl >/dev/null 2>&1 && [ -x "$(dirname "$0")/e2e-capture-previous-logs.sh" ]; then
111157
_prev_rc=0
112158
timeout -k 30 300 "$(dirname "$0")/e2e-capture-previous-logs.sh" \
113159
"$_snap/previous-logs" "${COZY_TEST_NAMESPACE:-tenant-test}" 2>&1 || _prev_rc=$?
@@ -124,7 +170,7 @@ _cozy_on_exit() {
124170
echo "» previous-instance capture INCOMPLETE (exit $_prev_rc); kept what landed in $_snap/previous-logs"
125171
fi
126172
fi
127-
if command -v crust-gather >/dev/null 2>&1; then
173+
if [ "$_cozy_cluster_captures" -eq 1 ] && command -v crust-gather >/dev/null 2>&1; then
128174
echo "» capturing crust-gather snapshot of failed $(basename "$TEST_FILE") -> $_snap"
129175
# Bound with a timeout: crust-gather collect has hung indefinitely on a
130176
# contended/degraded cluster (e.g. streaming logs from a crashlooping pod),
@@ -163,7 +209,7 @@ _cozy_on_exit() {
163209
# capture is time-boxed and `|| true`, and the whole run is wrapped in a
164210
# wall-clock backstop so it cannot stall the job. It no-ops when there are no
165211
# affected pods or when kubectl/the tooling is absent.
166-
if command -v kubectl >/dev/null 2>&1; then
212+
if [ "$_cozy_cluster_captures" -eq 1 ] && command -v kubectl >/dev/null 2>&1; then
167213
echo "» capturing host->pod data-plane for NotReady pods -> $_snap/dataplane"
168214
timeout -k 30 600 "$(dirname "$0")/e2e-capture-dataplane.sh" "$_snap/dataplane" 2>&1 || true
169215
fi

0 commit comments

Comments
 (0)