Skip to content

Commit 641efd3

Browse files
lexfreilexfrei
committed
test(e2e): capture worker CPU throttling counters on node-join failure
When a tenant worker misses the node-Ready deadline, the sandbox node it runs on is routinely at half its capacity with a clean kernel log. In that state "the host starved the VM" and "the VM was held at its own CFS ceiling" produce identical evidence: node-level utilisation reports what the node used, never what the container was allowed. The CFS counters are the only place the difference is recorded, and nothing in the tree collected them for the tenant workers, so every diagnosis of this failure so far had to be inferred from outside. Read them from the kubelet's cAdvisor endpoint on the node hosting each worker. Five series carry the whole answer and cAdvisor publishes all of them per container, so nothing is computed here: the scheduled and throttled period counts, the time spent throttled, and the quota and period that form the ceiling. The throttled counters alone say a container hit some ceiling, not which one, and a VM capped at one core and a VM capped at eight are the same number without the quota. Absence of the quota series is itself a reading rather than a gap. cAdvisor emits it only when the quota is non-zero, so a container reporting the period and no quota is one running uncapped - which is one of the two answers this collector exists to distinguish, arriving without being derived. Reading the kubelet rather than the container matters beyond convenience. A reader running inside the container shares the cgroup it measures, so it slows down in proportion to how much the answer matters and would need a budget sized for the pathological case; the kubelet is outside that cgroup. It also drops any dependency on what the container image ships and on how the host lays cgroups out. The namespace is not a worker filter and is not used as one: it also holds the tenant control plane, whose apiserver and etcd carry CPU limits of their own and can be genuinely throttled. The Pod name carries the second half of the filter, so a throttled apiserver cannot answer under a heading that says worker. The walk is over nodes rather than Pods, because the endpoint is per node and one read covers every worker it hosts. It is gated on the diagnostics phase budget, and placed first among the five gated collectors: the budget declines whatever has not started when it runs out, its own ceiling is four bounded reads rather than the minutes the four after it can spend, and it is the only one whose question has no other answer in the tree. That order is a trade, stated here because it is a decision rather than a detail - it puts the guest serial console one place further from the budget, and that capture is the only one describing a worker which never reached apid. Both reads take the block's own read-budget knob rather than a literal equal to it, so lowering that knob lowers these too. Each keeps its own exit status rather than routing through the shared helper, which always returns zero by design: this collector has to tell "the kubelet was not read" from "the kubelet answered and carried no worker", and only the status separates them. The stream is captured whole and filtered after, since a pipeline reports its last command's status and would otherwise hand that decision to grep. Every way a read can come up short is named rather than left as zero bytes: a kubelet that was not read, one that answered with nothing for the namespace, a stream cut short, and exit 124 - which cannot distinguish this collector timing out from the read exiting 124 on its own - each get their own line, and each says whether an error log exists to explain it. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
1 parent 677d80a commit 641efd3

6 files changed

Lines changed: 2041 additions & 19 deletions

File tree

‎docs/agents/e2e-testing.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ Trap-based cleanup was the single biggest source of false failures in the BATS s
4444

4545
- Chainsaw deletes the resources it `apply`-ed during its cleanup phase (bounded by the `delete`/`cleanup` timeouts in `hack/e2e-chainsaw/.chainsaw.yaml`). Do not hand-roll teardown for resources Chainsaw created.
4646
- A self-contained `trap '… ' EXIT` **inside a single `script` step** — to kill a port-forward or remove a temp dir — is fine, because it runs in a contained subprocess with its variables in scope. See `hack/e2e-chainsaw/bucket/chainsaw-test.yaml`. What is banned is test-level trap-based cleanup of the BATS kind. The same carve-out holds inside a BATS `@test` when the trap sits in an explicit subshell — `( … trap "kill $pid" EXIT … )` — because a subshell trap does not replace the one the `bats` binary installs, so a failure inside it still prints its `not ok`. `hack/e2e-test-openapi.bats` relies on this to kill a backgrounded `kubectl proxy`; moving that cleanup to the end of the body would leak a process holding a fixed port rather than fix anything.
47-
- The ban extends to every BATS file under `hack/`, subdirectories and the `e2e-` prefixed ones included, for a second reason worth knowing before you debug one: an `EXIT` trap inside an `@test` body replaces the one the `bats` binary installs for its own bookkeeping, and a test that then **fails** prints no TAP line at all. It does not appear as `not ok`; it disappears, and the run ends with `# bats warning: Executed N instead of expected M tests` and a non-zero exit. Verified with Bats 1.14.0. Anyone reading the tail of the output, or grepping it for `not ok`, sees a green suite — and the CI runner `hack/cozytest.sh`, which is not the `bats` binary, reports the same failure correctly, so the two disagree exactly when it matters. Clean up at the end of the test body instead: both runners set `-e`, so the cleanup is unreachable on failure and the scratch directory is left behind for inspection, which is what you want from a failed test anyway. When a suite reports zero failures, confirm it also reports how many tests it ran: an exit code answers "did anything fail", never "did anything run". `hack/bats-no-exit-trap.bats` enforces this across every `hack/**/*.bats`, subdirectories included: a file that carries no `# EXIT-TRAP DEBT: N` comment must contain no EXIT-trap line at all, and a file that carries one must install exactly N — so a trap appearing or disappearing fails until the file's own number is corrected. Note what that does and does not buy: it is a ratchet on the number's *accuracy*, not on the debt itself, because adding a trap and raising `N` in the same change is green. Nothing mechanical stops the count growing — review does, which is the point of the number living in the file being reviewed. A trap inside an explicit subshell is exempt from the ban but still counted, so a declaration is not by itself an admission of debt; `hack/e2e-test-openapi.bats` is the current example and says so in its own header. The scan reads `.bats` files only, so a handler reaching a test body from a sourced `.sh` is outside it — `hack/e2e-chainsaw/_lib/run-kubernetes.sh` installs two, each benign for its own reason rather than by design: the one in `cozy_capture_tenant_talos` because that function is declared with `(` and runs in a subshell, and the one in `run_kubernetes_test` because no `@test` calls it despite being declared with `{`. Three `hack/*.bats` source that library. Treat the guard as a ratchet over a common spelling, not as proof that a file installs no handler. A handful of files still declare a debt while their conversion waits on the branches that own them; when one of those reports zero failures, reconcile its `1..N` plan against its `ok` count before believing the run.
47+
- The ban extends to every BATS file under `hack/`, subdirectories and the `e2e-` prefixed ones included, for a second reason worth knowing before you debug one: an `EXIT` trap inside an `@test` body replaces the one the `bats` binary installs for its own bookkeeping, and a test that then **fails** prints no TAP line at all. It does not appear as `not ok`; it disappears, and the run ends with `# bats warning: Executed N instead of expected M tests` and a non-zero exit. Verified with Bats 1.14.0. Anyone reading the tail of the output, or grepping it for `not ok`, sees a green suite — and the CI runner `hack/cozytest.sh`, which is not the `bats` binary, reports the same failure correctly, so the two disagree exactly when it matters. Clean up at the end of the test body instead: both runners set `-e`, so the cleanup is unreachable on failure and the scratch directory is left behind for inspection, which is what you want from a failed test anyway. When a suite reports zero failures, confirm it also reports how many tests it ran: an exit code answers "did anything fail", never "did anything run". `hack/bats-no-exit-trap.bats` enforces this across every `hack/**/*.bats`, subdirectories included: a file that carries no `# EXIT-TRAP DEBT: N` comment must contain no EXIT-trap line at all, and a file that carries one must install exactly N — so a trap appearing or disappearing fails until the file's own number is corrected. Note what that does and does not buy: it is a ratchet on the number's *accuracy*, not on the debt itself, because adding a trap and raising `N` in the same change is green. Nothing mechanical stops the count growing — review does, which is the point of the number living in the file being reviewed. A trap inside an explicit subshell is exempt from the ban but still counted, so a declaration is not by itself an admission of debt; `hack/e2e-test-openapi.bats` is the current example and says so in its own header. The scan reads `.bats` files only, so a handler reaching a test body from a sourced `.sh` is outside it — `hack/e2e-chainsaw/_lib/run-kubernetes.sh` installs two, each benign for its own reason rather than by design: the one in `cozy_capture_tenant_talos` because that function is declared with `(` and runs in a subshell, and the one in `run_kubernetes_test` because no `@test` calls it despite being declared with `{`. Seven `hack/*.bats` source that library. Treat the guard as a ratchet over a common spelling, not as proof that a file installs no handler. A handful of files still declare a debt while their conversion waits on the branches that own them; when one of those reports zero failures, reconcile its `1..N` plan against its `ok` count before believing the run.
4848

4949
### 4. Do not mask cleanup or teardown failures
5050

0 commit comments

Comments
 (0)