Skip to content

Commit 048088b

Browse files
authored
fix(e2e): bound the tenant failure diagnostics per read and as a phase (#3676)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does The node-join failure diagnostics in `hack/e2e-chainsaw/_lib/run-kubernetes.sh` issued about a dozen `kubectl` reads with neither an outer wall-clock bound nor `--request-timeout`, starting with a `describe nodes` through the tenant kubeconfig. That read targets the component least likely to answer in a node-join failure. An unbounded read there does not lose only itself: it holds the Chainsaw op until the op is killed, so everything scheduled after it is lost rather than truncated, the tenant crust-gather snapshot above all. Every read now carries the per-call bound the newer collectors on this path already use. A read that fails or is cut off says so instead of passing in silence, and says only what was observed, because a read that did not finish is silence about the cluster rather than a finding about it. The CDI importer walk gains a cap, and its listing is read on its own so a listing that never answered cannot read as a namespace with no importer in it. Per-read bounds alone do not end the phase, and the collectors here can together spend more than what is left of the op by the time they run. So the phase is bounded as a phase: once its budget is spent, the collectors that have not started are declined out loud. That is not a worst case and is not written as one, but it stops the snapshot being queued behind work begun past the deadline, which is how the snapshot was being lost. The op ceiling is unchanged, since a larger ceiling multiplied by a sum that grows with every collector added is the same problem one collector later. Because what runs last is what gets declined, the block is reordered so the cheap reads come first. The CSR list and the signer log are the discriminator for the failure mode a follow-up fix has to target, and behind two minute-scale collectors they were exactly what a tight run would drop. Adjacent work on the same failure path, all of it in service of the same requirement: - the sibling scheduling-gate branch ends in the same exit and depends on the same snapshot, so its two reads are bounded too; - `talos_image_cache_diagnose` gets its reads bounded and its gate gets `--ignore-not-found`, because `kubectl get` exits 1 for a refused connection or Unauthorized as readily as for NotFound, so a call that never reached the apiserver was being announced as a mirror that was never deployed; - the four numeric knobs are validated rather than only documented, since a suffix or a leading zero on the budget reached the arithmetic in the block's first statement and unwound the whole function before its own headline, leaving a failing run with no diagnostics at all. Comments carry invariants rather than arithmetic. A comment that states a sum has to be kept true forever in every copy of it, and any edit to the code or the order falsifies one of them. The numbers live in the code as values and in the guard that pins them. Relates to #3656. ### Known gaps, deliberately not closed here Some collectors on this path carry no wall-clock bound at all. The image-cache reachability re-probe makes seven unbounded management-cluster calls, and the guest-Talos capture walks its VMI rows uncapped with applies and waits bounded only by `--request-timeout`, which bounds one HTTP request rather than a client retrying against a wedged apiserver. The phase budget stops them being started late; it does not stop one running long once admitted. They are pre-existing and tracked in #3666. Bounding the re-probe in particular would change when the same function gives up on the happy path, and so which image factory tenant workers use, which is not a diagnostic's decision to make. Named improvements left for later, none of them regressions introduced here: - the phase deadline is stamped at block entry rather than against what is left of the op, so a long bringup still narrows the room the derivation assumes; - `bringup` and `largest` are literals in the guard, so a collector heavier than today's will not move them, and the margin is small by design; - the integer-knob validator now exists in four places in this tree, and a shared `_lib` file sourced ahead of both libraries would collapse two of them; - the per-request bound equals the wall-clock bound, so both deadlines fire together and which of the two notes appears is a race; - the ordered list in the chainsaw comments omits the ungated wedge check, which runs first; - section letters no longer sort after the reorder, and the comment-to-code ratio in the touched block is high. ### Testing `hack/run-kubernetes-node-join_test.bats` is new: 22 cases driving the block with a `kubectl` stub that hangs, refuses and answers in part, and a `timeout` stub that fails any read not wrapped in the bounding idiom. It pins that no read escapes a bound, that a cut-off read is named rather than reported as an empty cluster, that the walk cap reports both counts, that a failed listing is not reported as no importers, that the phase declines the rest out loud, that a malformed knob falls back and says so, that the block returns zero so the caller keeps its `exit 1`, and the source order that keeps the discriminating reads ahead of the expensive collectors. Every one was checked by mutation against the unfixed shape. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(e2e): bound every read in the tenant Kubernetes node-join failure diagnostics and give the collection phase its own wall-clock budget, so a wedged apiserver can no longer hold the Chainsaw op until it is killed and take the tenant crust-gather snapshot with it ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved Kubernetes node-join and scheduling failure diagnostics with bounded collection times, clearer timeout and failure reporting, and more reliable preservation of critical snapshots. - Added safer handling for unavailable diagnostic data, missing tools, failed commands, and undeployed image caches. - Added configurable and validated timeout controls for image-cache and failure diagnostics. - **Documentation** - Clarified diagnostic budgets, collection ordering, and behavior when available time is exhausted. - **Tests** - Added comprehensive coverage for diagnostic timeouts, ordering, validation, failure handling, and bounded reads. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 parents 86fb92a + 0607835 commit 048088b

7 files changed

Lines changed: 1715 additions & 121 deletions

File tree

‎docs/agents/e2e-testing.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,7 @@ Be precise about TIA's scope — it is narrower than "skip E2E for unrelated PRs
9999
- **Capture the PREVIOUS container instance, not just the current one.** For a crash-looping pod `kubectl logs` (and the Chainsaw `podLogs` collector, which in v0.2.15 has no `previous` option — it does accept `name`/`namespace`/`selector`/`container`/`tail`/`timeout`/`cluster`/`clusters`) shows the Nth restart — a fresh replay of the same startup path — while the evidence that explains the failure sits in the immediately preceding instance, reachable only via `--previous` and lost for good once the kubelet garbage-collects that container. `hack/e2e-capture-previous-logs.sh` closes this once for every suite from the global `error.catch` (and from `hack/cozytest.sh` for the e2e BATS suites) rather than per-suite: it discovers containers with `restartCount > 0` at runtime — init containers included, since an interrupted bootstrap lives there — orders the app suites' `tenant-test` namespace first (a best-effort ordering only — it never restricts the capture, and for a suite whose pods live in a child tenant namespace it simply degrades to input order), and dumps each to the job log and to `snapshots/<test>/previous-logs/`. Containers that never restarted are skipped, so a healthy run emits one line, not a page of "previous terminated container not found". Every way the capture can come up short is named rather than inferred: the `COZY_PREVLOG_MAX` overflow, a malformed cap, a `COZY_PREVLOG_TAIL` that was rejected and fell back to 200 (a zero or leading-zero tail asks kubectl for nothing, which would otherwise be reported as the container having logged nothing), the tail bound each kept log was written under — named in `capture-notes.txt` beside the logs rather than inside them, since `--tail` drops the oldest lines and nothing in the output says so while a log read in full is a complete artifact that a trailing prose line would stop any parser from reading, and suppressed on `-1` because no bound was applied then — a pod list that never returned (which is NOT the same as "nothing restarted" and must not be reported as such), a read that timed out versus one kubectl itself refused (kubelet GC, RBAC, an unreachable node — different problems that a blanket "kubelet GC" line would hide), a previous instance that produced no output, and a capture the outer backstop cut short. A partial log is kept and marked truncated, never deleted — it is the only copy of something already gone from the cluster. Each of those lines the script itself emits is written to `snapshots/<test>/previous-logs/capture-notes.txt` as well as to the job log, and the directory is created before the first of them, so a reader who has only the uploaded artifact can still tell a capture that found nothing from one that never ran. The outer backstop firing is the exception: that line comes from the callers, after the script has already been killed, and reaches the job log only. `hack/cozytest.sh` arms this capture, and the other two legs, only for suites whose own filename or whose parent directory carries the `e2e-` prefix, because all three read cluster state and a unit runner has `kubectl` on PATH with no cluster behind it, so a red unit test used to walk into the kubectl-gated legs on its way out, and into the crust-gather one as well on any machine where that binary happened to be installed.
100100
- **The data-plane collector is the third capture leg, and the only one that reads forwarding state off the node.** `hack/e2e-capture-dataplane.sh` runs after the crust-gather snapshot, from `hack/cozytest.sh` for the e2e BATS suites and from the global `error.catch` in `hack/e2e-chainsaw/.chainsaw.yaml` for the Chainsaw ones, and it collects what an object-state snapshot structurally cannot: the cilium endpoint and BPF conntrack view from an affected pod's node, the kernel conntrack table that is the authoritative one for the host-to-local-pod path under `enable-host-legacy-routing`, and the LoadBalancer datapath. Its two callers bound it differently, 600s from `cozytest.sh` and 300s from the Chainsaw catch, so anything added to what it collects has to fit the smaller of the two rather than the one you happened to test under. It holds the same notes contract as the previous-logs collector, with one stated exception: every read carries its own ceiling, and why a read produced nothing goes to `capture-notes.txt` beside the capture rather than only to the job log, because an empty result and a read that never answered are the same bytes on disk and only the note tells them apart; the two EndpointSlice reads in the LoadBalancer path are the exception and report nothing either way, so a `<none>` backend line there does not distinguish an endpointless Service from a read that never answered.
101101
- **A catch script needs an explicit `timeout:` that exceeds the inner budgets it contains.** A Chainsaw `script` op is bounded by `timeouts.exec` (5m in `.chainsaw.yaml`) unless it overrides it, so a catch whose own `timeout -k …` wrappers sum past that is *structurally guaranteed* to be killed — it reports `SCRIPT | ERROR | signal: killed` and yields nothing. That kill is strictly worse than an inner `timeout` firing: SIGKILL takes out the process group, so the collector dies mid-write **and** every later step in the script is skipped, whereas an inner `timeout` exits 124, leaves a partial capture on disk, and lets the rest of the catch run. Size the op timeout above the sum of the inner budgets and let the inner ones be what fires.
102+
- **When the sum cannot fit the op, bound the collection phase as a phase rather than raising the op.** The rule above assumes the inner budgets add up to something an op timeout can sit above. On a failure path that has accumulated collectors it stops being true — the tenant-Kubernetes node-join path in `hack/e2e-chainsaw/_lib/run-kubernetes.sh` sums past what is left of its 50m op once the bringup ahead of it and the snapshot behind it are counted, and several of its reads carry only `--request-timeout`, which bounds one HTTP request rather than a client retrying against a wedged apiserver, so parts of it have no ceiling to add up in the first place. Raising the op ceiling does not fix that shape, it postpones it: a larger ceiling multiplied by a sum that grows with every collector added is the same problem one collector later. What works instead is a wall-clock budget for the whole phase (`COZY_DIAG_PHASE_BUDGET`, with `cozy_diag_phase_start` / `cozy_diag_phase_has_time`): once it is spent, the collectors that have not started are declined out loud instead of attempted, so whatever must run after the phase — the tenant crust-gather snapshot, in that case — is never queued behind work begun past the deadline. Two consequences worth carrying to the next collector. The budget bounds when a collector may *start*, not when the phase ends, so it is not a worst case and the comment next to it must not be written as one. And because what runs last is what gets declined, source order becomes load-bearing: put the cheap reads that discriminate between failure modes ahead of the collectors that cost minutes, or the budget will spend itself on the expensive ones and drop the evidence the path exists to produce.
102103
- **Set `crust-gather --duration` explicitly.** It is crust-gather's own budget for the collection phase and it defaults to **60s**; on elapse it abandons the collection and skips its finish step, leaving a partial tree with nothing to say the rest is missing. The outer wall-clock must exceed it, because the API discovery crust-gather runs *first* is not covered by that budget at all — with an unhealthy aggregated APIService (the state a failed e2e is usually in) aggregated discovery times out and the fallback re-walks every group, which can consume the entire outer timeout before a single object is written. Do not diagnose a killed snapshot by bumping the outer number alone; the two budgets bound different phases. Send the collector's output to a log inside the snapshot rather than `/dev/null` so "complete or truncated?" is answerable from the artifact.
103104
- **Per-failed-test crust-gather snapshots** are the richest diagnostic — a `crust-gather serve`-able archive of the cluster *at the moment of failure*, before cleanup. The global `error.catch` in `hack/e2e-chainsaw/.chainsaw.yaml` captures the **host** cluster into `_out/cozyreport/snapshots/<test>/host` on any failure (the Chainsaw analog of the `hack/cozytest.sh` EXIT-trap snapshot used by the e2e BATS suites); the kubernetes suites additionally snapshot their **tenant** cluster from `hack/e2e-chainsaw/_lib/run-kubernetes.sh` while the tenant API LB is still routable. `hack/cozyreport.sh` folds `snapshots/` into the artifact. Do not remove the global catch — without it the Chainsaw suite uploads a report with no per-test cluster state.
104105

0 commit comments

Comments
 (0)