test(e2e): capture worker CPU throttling counters on node-join failure - #3723
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds tenant worker CPU-throttling capture to Kubernetes failure diagnostics. It records cAdvisor CPU metrics and failure states, limits node traversal, integrates phase-budget handling, and adds extensive BATS coverage. ChangesTenant worker CPU throttling diagnostics
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant NodeJoinFailureReporter
participant CPUCollector as cozy_capture_tenant_worker_cpu_throttle
participant Kubectl
participant Cadvisor as kubelet cAdvisor metrics
NodeJoinFailureReporter->>CPUCollector: invoke failure diagnostic
CPUCollector->>Kubectl: list tenant worker Pods and nodes
CPUCollector->>Cadvisor: read node CPU CFS metrics
Cadvisor-->>CPUCollector: return metrics or read status
CPUCollector-->>NodeJoinFailureReporter: write per-node artifacts
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/agents/e2e-testing.md`:
- Line 47: Update the run-kubernetes sourcing statement in the e2e-testing
documentation to use the accurate count and explicitly define the scope as
top-level hack/*.bats files or all hack/**/*.bats files. Ensure the wording
matches the glob scope being counted and does not imply subdirectory files are
included unless they are.
In `@hack/run-kubernetes-cpu-throttle_test.bats`:
- Around line 1168-1179: Update the staged-binary validation in the
stripped-PATH test to verify every command listed in the staging loop—mkdir,
sort, grep, mv, rm, mktemp, wc, tr, and cat—not just grep. Reuse the existing
missing-binary failure behavior so the collector and assertion at line 1203 run
only when the complete required command set is available.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f6eec2c-0354-4d17-b3bf-b7ef06a168cc
📒 Files selected for processing (6)
docs/agents/e2e-testing.mdhack/e2e-chainsaw/_lib/run-kubernetes.shhack/e2e-chainsaw/kubernetes-latest/chainsaw-test.yamlhack/e2e-chainsaw/kubernetes-previous/chainsaw-test.yamlhack/run-kubernetes-cpu-throttle_test.batshack/run-kubernetes-node-join_test.bats
| - 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. | ||
| - 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. | ||
| - 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. | ||
| - 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. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Count the bats files that source run-kubernetes.sh.
set -euo pipefail
echo "=== top-level hack/*.bats sourcing run-kubernetes.sh ==="
fd -t f -e bats . hack --max-depth 1 --exec sh -c '
grep -l "run-kubernetes\.sh" "$1" 2>/dev/null || true
' _ {} \; | sort -u | tee /tmp/top.txt | wc -l
echo "=== any depth hack/**/*.bats sourcing run-kubernetes.sh ==="
fd -t f -e bats . hack --exec sh -c '
grep -l "run-kubernetes\.sh" "$1" 2>/dev/null || true
' _ {} \; | sort -u | tee /tmp/all.txt | wc -l
echo "=== files ==="
cat /tmp/all.txtRepository: cozystack/cozystack
Length of output: 257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== top-level hack files ==="
find hack -maxdepth 1 -type f -name '*.bats' 2>/dev/null | sort
echo "=== all hack bats files sourcing run-kubernetes.sh ==="
find hack -type f -name '*.bats' 2>/dev/null | sort | while IFS= read -r f; do
if grep -q -- 'run-kubernetes\.sh' "$f"; then
echo "$f"
fi
done
echo "=== exact count top-level ==="
find hack -maxdepth 1 -type f -name '*.bats' 2>/dev/null | sort | while IFS= read -r f; do
grep -q -- 'run-kubernetes\.sh' "$f"
echo $?
done | grep -x '^0$' | wc -l
echo "=== exact count all depths ==="
find hack -type f -name '*.bats' 2_ARRepository: cozystack/cozystack
Length of output: 2641
🏁 Script executed:
#!/bin/sh
set -uu
echo "=== top-level hack/*.bats sourcing run-kubernetes.sh ==="
find hack -maxdepth 1 -type f -name '*.bats' -print | sort | while IFS= read -r f; do
if grep -q -- 'run-kubernetes\.sh' "$f"; then
echo "$f"
fi
done
echo "=== hash total top-level count ==="
find hack -maxdepth 1 -type f -name '*.bats' -print | sort | while IFS= read -r f; do
grep -q -- 'run-kubernetes\.sh' "$f"
done | grep -c '^0$' || true
echo "=== hash total all depths count ==="
find hack -type f -name '*.bats' -print | sort | while IFS= read -r f; do
grep -q -- 'run-kubernetes\.sh' "$f"
done | grep -c '^0$' || true
echo "=== occurrences/context ==="
find hack -type f -name '*.bats' -print | sort | while IFS= read -r f; do
if grep -q -- 'run-kubernetes\.sh' "$f"; then
printf '%s\n' "$f"
grep -n -- 'run-kubernetes\.sh' "$f"
fi
doneRepository: cozystack/cozystack
Length of output: 10053
Update the run-kubernetes source-line count.
docs/agents/e2e-testing.md says “Seven hack/*.bats source that library”, but there are more. The glob hack/*.bats also excludes subdirectories, so make the sentence explicit about whether it means top-level files only or all hack/**/*.bats files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/agents/e2e-testing.md` at line 47, Update the run-kubernetes sourcing
statement in the e2e-testing documentation to use the accurate count and
explicitly define the scope as top-level hack/*.bats files or all hack/**/*.bats
files. Ensure the wording matches the glob scope being counted and does not
imply subdirectory files are included unless they are.
| for c in mkdir sort grep mv rm mktemp wc tr cat; do | ||
| for d in /bin /usr/bin /usr/local/bin /opt/homebrew/bin; do | ||
| if [ -x "$d/$c" ]; then | ||
| ln -sf "$d/$c" "$tmp/bin/$c" | ||
| break | ||
| fi | ||
| done | ||
| done | ||
| if [ ! -x "$tmp/bin/grep" ]; then | ||
| echo "could not stage a PATH without timeout" >&2 | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check every staged binary, not only grep.
The first stripped-PATH test verifies seven binaries at lines 1116-1121 and explains why: a missing one fails the test for the wrong reason. This test stages the same list at line 1168 but verifies only grep. The collector also calls mkdir, sort, mv, rm, mktemp, wc and tr on this path. If one of them is absent from the staged PATH, the capture misbehaves and the assertion at line 1203 can still pass.
🛡️ Proposed fix to verify the full staged set
- if [ ! -x "$tmp/bin/grep" ]; then
- echo "could not stage a PATH without timeout" >&2
- return 1
- fi
+ for c in mkdir sort grep mv rm mktemp wc tr; do
+ if [ ! -x "$tmp/bin/$c" ]; then
+ echo "FAIL: could not stage $c in the stripped PATH; the check below would be vacuous" >&2
+ return 1
+ fi
+ done
+ if [ -e "$tmp/bin/timeout" ]; then
+ echo "FAIL: timeout leaked into the stripped PATH; this test would prove nothing" >&2
+ return 1
+ fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hack/run-kubernetes-cpu-throttle_test.bats` around lines 1168 - 1179, Update
the staged-binary validation in the stripped-PATH test to verify every command
listed in the staging loop—mkdir, sort, grep, mv, rm, mktemp, wc, tr, and
cat—not just grep. Reuse the existing missing-binary failure behavior so the
collector and assertion at line 1203 run only when the complete required command
set is available.
IvanHunters
left a comment
There was a problem hiding this comment.
Overview
This adds cozy_capture_tenant_worker_cpu_throttle() to hack/e2e-chainsaw/_lib/run-kubernetes.sh, plus wiring into cozy_report_node_join_failure(), two one-line chainsaw-test.yaml comment updates, a one-line doc count fix, and a large new BATS suite (hack/run-kubernetes-cpu-throttle_test.bats, 1453 lines) plus updates to hack/run-kubernetes-node-join_test.bats.
On the large diff: verified it is not vendored/generated. run-kubernetes.sh gets +450/-9 of hand-written shell (collector logic + heavy inline rationale comments); the new .bats file is +1453 hand-written test lines with real mocks (kubectl, timeout) and multiple failure-mode fixtures; run-kubernetes-node-join_test.bats gets +135/-9 to add use_temp_report_dir and audit assertions. This is proportionate for a diagnostics collector this defensive plus its regression coverage — appropriate, not padding.
Runs only on failure, cannot fail the job: traced the call chain — cozy_report_node_join_failure is invoked only from the if ! timeout 18m ... node-Ready-deadline failure branch in run_kubernetes_test (never on the happy path), and the new collector is invoked as cozy_capture_tenant_worker_cpu_throttle || true, gated by cozy_diag_phase_has_time '(d) tenant worker CPU throttling'. It cannot slow passing runs and cannot itself fail the suite.
Robustness of the capture: bounded to at most 4 reads (1 Pod listing + up to max_nodes=3 per-node kubectl get --raw .../metrics/cadvisor), each wrapped in the existing COZY_DIAG_READ_TIMEOUT/COZY_DIAG_READ_GRACE machinery via the same _cozy_diag_seconds re-validation pattern used elsewhere in the file (verified signature match against other call sites in run-kubernetes.sh). Every command whose failure matters is captured via || var=$? / || true rather than relying on set -e, so a failed sub-step can't silently abort the function mid-write. The metric regex (^container_(cpu_cfs_(periods_total|throttled_periods_total|throttled_seconds_total)|spec_cpu_(period|quota))\{) is anchored to avoid partial-name matches, and the three-stage grep filter (metric name -> namespace -> pod-name prefix) deliberately keeps exit statuses separable to distinguish "kubelet never answered" from "kubelet answered, no matching series" from "local read-back failed" — verified this distinction is both implemented and covered by dedicated tests (killed read, timeout off PATH, tenant-control-plane exclusion).
Doc accuracy check: independently counted the hack/*.bats files that actually . hack/e2e-chainsaw/_lib/run-kubernetes.sh (not just mention the path in a comment) at the PR's head commit — confirmed exactly 7. The docs/agents/e2e-testing.md edit from "Three" to "Seven" is factually correct.
Ordering trade-off: the collector is placed ahead of the guest serial-console capture in the failure-report sequence. This is explicitly reasoned about in both the PR description and inline comments (this collector's answer has no other source in the tree; console evidence is irreplaceable but only starved if the budget actually runs out, and this collector's own ceiling is ~100s of a 480s budget). Reasonable and disclosed, not hidden.
Existing review thread status: CodeRabbit posted 2 actionable comments on the current head (no follow-up commits since). Neither is addressed yet; see non-blocking notes below — neither rises to a regression.
No confirmed regression, no crash/mask-the-real-failure risk, no teardown/happy-path impact found. Approving with notes — the two open CodeRabbit items are worth a quick follow-up but aren't blocking.
Non-blocking / nits
- Unaddressed CodeRabbit finding — incomplete PATH-staging assertion: in
hack/run-kubernetes-cpu-throttle_test.bats, the test"a killed read is not written up as a grace period that never ran"stages 9 binaries into a stripped$tmp/binbut only asserts thatgrepwas staged successfully before running the subprocess. Its sibling test immediately above checks 8 of the 9 staged binaries. If any of the other 8 failed to resolve on some runner, this test could pass or fail for the wrong reason instead of testing what it claims to. Low real-world risk (these are universal core utils on any Linux/macOS CI box), but worth tightening for consistency with the sibling test. - Unaddressed CodeRabbit finding — doc wording scope: the
docs/agents/e2e-testing.mdline "Sevenhack/*.batssource that library" is numerically correct for the literalhack/*.bats(top-level only) glob, but doesn't explicitly say whether the count would change underhack/**/*.bats. A one-clause clarification would remove the ambiguity CodeRabbit flagged. - Several limitations are already self-disclosed in the PR body (unchecked
mkdir -p, unquotedfor node in ${nodes}word-splitting, conservative whole-file "uncapped" note, stale bats-file count risk) — all pre-existing patterns shared with neighboring collectors in the same file, not new debt introduced here, and correctly called out rather than hidden.
Closing
Solid, clearly-scoped diagnostics addition: failure-path-only, budget-gated, non-fatal by construction, and unusually well tested for its size. Recommend landing after a look at the two CodeRabbit items (optional, non-blocking).
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 <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
641efd3 to
b8a5b9b
Compare
What this PR does
When a tenant worker misses the node-Ready deadline, the failure bundle currently cannot tell two different causes apart: a worker held at its own CFS ceiling, and a worker losing host CPU it was entitled to. Both look identical from outside the Pod, and node-level utilisation answers neither, because it shows what the node used rather than what the container was allowed.
This adds a collector that reads the CFS counters and the ceiling itself from the kubelet's cAdvisor endpoint, for every tenant worker on every node in the sandbox. The counters are the only place that difference is recorded, and nothing else in the tree collects them for these workers.
The motivating measurement is on nightly run 31290999592: the
computecontainer reports 0.968 and 0.920 core against alimits.cpuof 1, which is 92-97% of a one-core ceiling. Whether the container ever actually met that ceiling is exactly what the artifact cannot say, and what these counters answer.Most of the change is the failure taxonomy rather than the read. An empty capture is the dangerous outcome here, because zero bytes reads as a container that never hit its ceiling -- the conclusion the collector exists to stop a reader reaching by default. So every way the capture can come up short gets its own sentence: the kubelet was not read, the read was cut short part way, the metric stream could not be read back on this runner, the kubelet answered and carried no series for this namespace, the namespace was there and the worker Pods were not. The one non-failure outcome is written down too: a container with no CPU limit puts a single line on the wire, which is also what a truncated read leaves behind, so it says which one it is.
Ordering
The collector runs first among the gated collectors, ahead of the guest serial console. Its ceiling is four bounded reads, about 100s of a 480s phase budget, so on a slow run the console capture starts that much later. That is a deliberate trade and not a side effect of putting the cheapest first: cost is not what settles the order, survivability of the answer is. The console evidence cannot be re-taken, and this collector's question has no other answer in the tree, so both go ahead of the captures whose state is partly recoverable from reads above them.
Known limits, named rather than left to be found
see read-error.logwhile the file on disk is<node>.read-error.log. That is the convention the neighbouring serial-console collector already uses, and changing one side alone would make the two inconsistent.mkdir -pfor the report directory is unchecked, as it is in four neighbouring collectors in the same file. Worth fixing across the file rather than in one collector.docs/agents/e2e-testing.md, is corrected here from three to seven. Nothing pins it, so it will drift again.for node in ${nodes}unquoted, so pathname expansion applies alongside word splitting. Node names are RFC1123 so it cannot bite today;while IFS= read -r nodewould make that structural instead of incidental.Screenshots
Downstream repositories
Release note
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation