fix(e2e): bound the tenant failure diagnostics per read and as a phase - #3676
Merged
Aleksei Sviridkin (lexfrei) merged 3 commits intoAug 9, 2026
Merged
Conversation
The node-join diagnostics 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, and an unbounded read there does not lose only itself: it holds the Chainsaw op until the op is killed, so everything after it is lost rather than truncated, the tenant crust-gather snapshot the failing exit triggers above all. Each read now carries the per-call bound the newer collectors on this path already use, sized lower because this block issues a dozen back to back where those bound one. A read that fails or is cut off says so instead of passing in silence, and says only what was observed: a read that did not finish is silence about the cluster, not a finding about it. The importer walk gains a cap for the same reason its listing is now read on its own, since a listing that never answered must not 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. Much of that is not ceiling-bounded at all: on the guest-Talos path the applies, waits and deletes carry --request-timeout or --timeout, which bound one HTTP request rather than a client retrying against a wedged apiserver. 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 does stop the snapshot being queued behind work begun past the deadline, which is how the snapshot was being lost. Raising the op ceiling was the alternative and it is not one: a larger ceiling times a sum that grows with every collector added is the same problem 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; behind a serial-console walk and a guest-Talos capture that can spend the whole budget between them, they were exactly what a tight run would drop. The 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; the prose keeps why the budget comes from an inequality, why admission gates the start rather than the finish, why the cheap reads run first, and which residuals the guard's literals do not cover. The sibling scheduling-gate branch ends in the same exit and depends on the same snapshot, so its two reads are bounded too. So are the reads in the talos-image-cache diagnosis, whose gate also gains --ignore-not-found: exit 1 from kubectl get means a refused connection or Unauthorized as readily as NotFound, and reading it as absent announced the mirror as never deployed on the strength of a call that never reached the apiserver. Bounding those dumps needed a note of its own first, since timeout prints nothing when it fires and a cut-off dump would otherwise leave a bare section header that reads as a cache with no Pod, no Service and no EndpointSlice. The knobs those bounds come from are validated rather than only documented. Each is pasted somewhere that takes digits and nothing else, and each failed differently and quietly: a suffixed or leading-zero budget reached the arithmetic in the block's first statement and unwound the whole function before its own headline, so a failing run produced no diagnostics at all; a suffixed read bound became a millisecond request timeout and made every note blame the cluster for a value; zero disabled the bound outright; a suffixed importer cap made the comparison exit non-zero, so the cap stopped existing; and a non-numeric grace makes timeout exit before running the command at all. A rejected value falls back and says so, which is what the rest of this tree does with its own knobs. All four are re-checked where they are used, since a value set after sourcing never passed the assignment, and the read bound is checked again before the per-request string is composed from it so the two halves cannot disagree. Each default is declared once as a constant, and the re-checks read the bare variable so an unset knob is named rather than silently defaulted. When timeout itself is absent the reads run unbounded rather than being bounded with a binary that is not there: the latter turns every read into an exit 127 and every note into the cluster refusing, and collects nothing, where unbounded is what this path did before any of this. The phase says so once, and says what it costs rather than reassuring. The reachability re-probe in the image-cache helper stays unbounded and its comment names all seven of the calls it makes, the fire-and-forget delete after the probe Pod included: the same function decides the mirror on the happy path, so changing when it gives up is not a diagnostic's decision to make. Its bounds are spelled at each call site rather than precomputed, since a prefix built at source time cannot follow a knob a test lowers afterwards while the --request-timeout beside it does. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
The 50m ceiling on the kubernetes-* script ops is justified by a comment enumerating what the failure path can spend, and the enumeration was both incomplete and reassuring in the wrong direction. It listed some collectors, said nothing about the node-join diagnostics or the image-cache diagnosis, carried a figure for the guest Talos capture that was half its real cost, and closed by asking the reader to keep the op above the sum, which no ceiling can do because the sum runs past what is left of the op on its own. It now names the phase budget as the thing that keeps the snapshot reachable and lists the collectors this ceiling covers, in the order the phase spends it. It states no sums and no per-collector figures. Why the budget is that number is derived once, beside the budget, and pointed at from here: a figure repeated across three files is a figure that drifts, and the drift is what the reader trusts. It also says what the ceiling does not promise. The budget bounds when a collector may start rather than when the phase ends; several collectors on this path carry no wall-clock bound at all; and the bringup's own waits already exceed this op on ceilings before a single diagnostic runs. So no worst case is claimed, because none can be stated while that holds. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
The e2e guide tells a collector author to size the op timeout above the sum of the inner budgets and let the inner ones fire. That works while the sum is something an op timeout can sit above, and on the tenant-Kubernetes node-join path it no longer is: the terms run past the 50m op on their own, and several of the reads there carry only --request-timeout, which bounds one HTTP request rather than a client retrying against a wedged apiserver, so parts of that path have no ceiling to add up at all. Left as it stood, the next collector author follows the documented rule and adds a bounded read to a sum that already does not fit. So the phase-budget pattern goes next to it, with the two properties that are easy to get wrong: the budget bounds when a collector may start rather than when the phase ends, so it is not a worst case and the comment beside it must not claim to be one; and because what runs last is what gets declined, source order decides which evidence survives a tight run. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Aleksei Sviridkin (lexfrei)
requested a review
from myasnikovdaniil
as a code owner
August 9, 2026 00:28
Contributor
|
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 (7)
📝 WalkthroughWalkthroughDiagnostic collection now uses validated timeouts, bounded reads, phase-wide budgets, ordered evidence gathering, and explicit failure reporting. Node-join, scheduling-gate, and Talos image-cache paths are covered by expanded shell tests and updated documentation. ChangesBounded diagnostic collection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Chainsaw
participant run-kubernetes.sh
participant cozy_report_node_join_failure
participant cozy_diag_read
participant talos_image_cache_diagnose
Chainsaw->>run-kubernetes.sh: detect node-join or scheduling-gate failure
run-kubernetes.sh->>cozy_report_node_join_failure: start diagnostic phase
cozy_report_node_join_failure->>cozy_diag_read: run bounded evidence reads
cozy_report_node_join_failure->>talos_image_cache_diagnose: collect cache evidence if phase time remains
talos_image_cache_diagnose->>cozy_diag_read: run bounded Kubernetes reads
run-kubernetes.sh->>Chainsaw: trigger snapshot and exit with failure
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
1 task
11 tasks
Aleksei Sviridkin (lexfrei)
deleted the
fix/node-join-diagnostics-bounds
branch
August 9, 2026 09:33
Aleksei Sviridkin (lexfrei)
added a commit
that referenced
this pull request
Aug 9, 2026
…actory (#3677) ## What this PR does The tenant Kubernetes e2e suites decide once whether worker image pulls go to the in-cluster Talos mirror or to the public Image Factory, and that decision came from a plain `kubectl get deploy talos-image-cache`. A plain `get` exits non-zero for a connection refused, an Unauthorized and a real absence alike, so any blip during that one call read as "no mirror", and the answer went straight into the decision file every later tenant test reads. One unlucky moment sent every worker in the run to the public factory over live egress, which is the dependency the mirror exists to remove, and nothing in the log said so. The run went ahead against the flaky thing and the failures read as environment noise. The lookup now asks with `--ignore-not-found -o name`. A real absence answers exit 0 with no output, which leaves a non-zero exit meaning only that the question failed, and the helper re-asks a couple of times before giving up. Only the empty answer turns the suite toward the public factory. An unanswered lookup warns, falls back for its own caller alone, and is deliberately not cached, so the next tenant test asks again instead of inheriting a guess. The warning has to be backed by something, and kubectl's stderr alone is not enough. When the wall-clock bound kills an attempt, neither the bound nor the SIGTERMed kubectl writes a word, and that stall is precisely the failure the re-ask was written against, so a warning that sent the reader to kubectl's message would be pointing at an empty stretch of log exactly when the lookup mattered. The lookup therefore names the last attempt's exit status before giving up, in the vocabulary the diagnostic reads already use for 124 and 137, and kubectl's own text stays on stderr for the cases where there is any. Each attempt is bounded twice, and the two bounds are deliberately not equal: `timeout -k 5 40` around `--request-timeout=30s`. kubectl defaults the request timeout to 0, so a connection that establishes and then stalls has no deadline at all, and the flag on its own is not a wall-clock bound either, because kubectl retries discovery several times before it gives up. Re-asking an unbounded call three times would have multiplied a hang instead of riding out a blip. The ten seconds between the two are load-bearing, and I only found out why by measuring rather than reasoning. Driven against a stub apiserver the three failure modes are not alike: a refused port answers in under a second and names the refusal, one that completes TCP but stalls mid-TLS hits kubectl's own handshake deadline and still produces text despite being killed, and a blackholed address (SYN dropped, no RST, which is what a wedged node or a netpol looks like) produces nothing at all until some deadline expires. That last mode is the one the headroom buys back. With both bounds at the same value the expiry that arrives first is always the wall-clock kill, so the blackhole case ends as a bare exit 124; with the wrapper ten seconds above, the request deadline ends the attempt and kubectl says what happened, and the wrapper stays as the backstop for a client that overruns its own deadline. Controlled A/B, same target and same inner bound, only the outer differs: 30 over 30 gives exit 124 and zero bytes, 40 over 30 gives exit 1 at the thirty second mark and a message naming the cause. The cost is ten more seconds per attempt on a path where the apiserver is already unreachable, inside a 50m op. Worth stating separately, because it is a different question with a different answer: before this change the inner flag was not doing anything at all. `--request-timeout=30s` sat under a `timeout` of the same 30 seconds, and across all three measured modes it never once fired. The refused case never reached it, the TLS stall was ended by kubectl's own shorter handshake deadline, and the blackhole was ended by the wall-clock kill landing at the same instant. So the flag was decoration, and the comment above it claimed otherwise. That is a separate defect from the lost diagnostics, and it is fixed by the same separation rather than by adding anything. Where `timeout` is not installed the read runs unwrapped instead of through it. Its exit 127 would otherwise count as an attempt that did not answer, three times over, so a missing local binary would pin the whole suite to the public factory while every message blamed the apiserver. `talos_image_cache_diagnose` asks the same question on the node-join failure path, and #3676 landed its own three-way gate there while this branch was open. The two stay separate rather than sharing this helper. That one runs under a phase budget where re-asking spends what the tenant snapshot behind it needs, so it answers on a single attempt; this one runs on the happy path, where a re-ask costs a suite nothing and buys back the mirror. This branch leaves that gate as it was merged. Unit tests put a throwaway `kubectl` and `timeout` on PATH and drive all three outcomes, the re-ask, the caching rule, the `timeout`-absent path and the killed-attempt path where nothing but the lookup itself can explain the failure: `hack/cozytest.sh hack/talos-image-cache_test.bats`. The stubs are files rather than shell functions because an external `timeout` binary execs a real file and would never see a shell override. The re-ask is a retry loop, which `docs/agents/e2e-testing.md` restricts. It falls under the exemption there rather than the prohibition: what is retried is a network read with no product or test logic under it, and the deterministic steps the rule protects are untouched. This relates to #3668 and closes the first door only. The decision is cached once at the end of the function regardless of which inner check produced the negative answer, and inside the "mirror is present" branch the `rollout status` wait and the three reads in `_talos_image_cache_reachable_from_tenant` still conflate "the mirror does not work" with "the probe could not be run", with the same suite-wide effect one layer down. The issue's own status comment says it stays open after this change, so there is no closing keyword here. Three weaknesses this PR does not remove, named because each is a real thing a later reader will hit. The `absent` case in `resolve_talos_image_factory_url` is reached by `else` fall-through rather than by matching the token, so a fourth state added to the helper later would land in the branch that turns the suite toward the public factory rather than in the safe one; a test pins the `present` token against exactly that drift, but the shape stays fall-through. The lookup's own `30` and `5` are literals while the diagnostic reads next door run off a validated, overridable pair, and that is deliberate rather than an oversight: those knobs are named and documented for the dump budget, so wiring the lookup to them would let a test that turns the dumps down silently change which image factory a whole suite pulls from. And `_TALOS_IMAGE_CACHE_QUERY_TRIES` is the one knob in the file without that validation. A non-numeric value spins the loop forever rather than failing: the comparison sits in an `if` condition, so `set -e` is exempt, the branch is simply never taken, and the job burns to its ceiling with no output. Nothing outside the tests sets it, which is why it is here and not in the diff. Worth recording for whoever does fix it: the obvious remedy, reusing this file's own `_talos_image_cache_seconds`, does not work where the knob is assigned, because the validator is defined a hundred lines further down and the assignment runs at source time. The call resolves to nothing, the substitution yields an empty string, and an empty bound spins the same loop. A real fix has to move the definition up or validate inside the function at call time. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff. It touches `hack/e2e-chainsaw/_lib/talos-image-cache.sh` and `hack/talos-image-cache_test.bats` and nothing else: no file under `hack/` is moved or renamed, no make target changes behaviour, and no package, chart, CRD or values key is involved. The `cozystack/ccp` entry is the closest one and it does not fire. - [x] No downstream repository is affected by this change ### Release note ```release-note fix(e2e): a single failed apiserver lookup no longer sends a whole e2e run to the public Talos Image Factory ```
Aleksei Sviridkin (lexfrei)
added a commit
that referenced
this pull request
Aug 9, 2026
…mirror (#3575) ## What this PR does Refs #3513. It addresses one variant of that flake and does not close it; see "Scope against #3513" below. ### Root cause of the variant this addresses The chainsaw `kubernetes-latest` / `kubernetes-previous` suites fail with `node-join failed: fewer than 2 tenant nodes Ready within 18m` and zero registered tenant nodes. The in-guest Talos capture on #3548 shows where the time goes on one class of those runs: the worker loops trying to pull `ghcr.io/siderolabs/kubelet` from public ghcr.io and fails with `Head "https://ghcr.io/v2/siderolabs/kubelet/manifests/...": net/http: TLS handshake timeout host=ghcr.io`. The Talos `kubelet` service never starts (`kubelet was not registered`), so the node never registers, cilium-operator stays `Pending` for lack of a node to schedule on, and the budget expires. On those runs the tenant apiserver is healthy and the worker VMs boot with kube-ovn IPs, so the failing link is the worker's public image-pull egress, and the worker machine config has no `machine.registries.mirrors`, so there is no way to route around it. Same class as the Talos OS image, which #3231 solved with the `talos.imageFactoryURL` cache. The thread also carries a later analysis putting worker-VM CPU starvation from nested-virt oversubscription at the root, with serial-console RCU stalls showing the guest never reaching Talos userland, and reading the TLS-handshake variant as a symptom of the same contention. Nothing here contradicts that. Removing a live dependency on public egress from worker bring-up is worth doing on its own terms, and if the contention account is the dominant one, this narrows what is left to explain rather than competing with it. ### The change **Chart (`packages/apps/kubernetes`, `packages/apps/kubernetes-nodes`)**, a `talos.registryMirrors` passthrough, a map of upstream registry host to `{ endpoints: [ ... ] }`, rendered into `machine.registries.mirrors` of the worker `TalosConfigTemplate` that the MachineDeployment clones from, so it applies at first boot rather than after it. Empty by default: with the default values the rendered chart is byte-identical to main, both content-hash object names included, so no existing worker rolls. This is the durable half; an air-gapped, rate-limited or flaky-egress environment can point worker image pulls at its own mirror, which is the `machine.registries.mirrors` knob the Talos rollover left unfilled. Talos still falls back to the upstream registry unless a host also sets `skipFallback`, so a mirror on its own is not air-gap enforcement, and the field doc says so. The value lands inside the reconcile Job's unquoted heredoc, so it is escaped for backslash, dollar and backtick and renders as a literal. `talos.installerRepository`, `talos.schematicID` and the Talos version render into that same heredoc one block above it and now get the same treatment, so the file is consistent about it rather than escaping only the newest field. Escaping the rendered value covers every byte by construction, which enumerating known-bad shapes does not. The chain is a no-op for values carrying none of those characters, so the default render, content-hash Job name included, is unchanged and no existing worker is replaced. **e2e harness**, an in-sandbox `registry:2` pull-through cache for ghcr.io (`hack/e2e-ghcr-mirror.yaml`), applied at install time from its own step, with `hack/e2e-chainsaw/_lib/ghcr-mirror.sh` setting `spec.talos.registryMirrors` on the tenant CR when the mirror is up and its egress allow is in place, and emitting nothing otherwise so workers pull directly. A `CiliumClusterwideNetworkPolicy` opens one tightly-scoped hole for worker VM (`kubevirt.io: virt-launcher`) Pods in `tenant-test` to reach the mirror, mirroring the existing `talos-image-cache` policy. Two properties of the helper worth calling out, because both are easy to get wrong and both are now pinned by tests. A failed API call is never cached: the decision is reused by every later suite in the shared sandbox, so caching one blip would disable the mirror for the whole run. Readiness is decided once and acted on once. `rollout status` is the cheap way to learn it, but it also exits non-zero on a broken watch, so when it fails the Deployment's own `Available` condition is read rather than the watch's exit code taken as the answer. Exactly one failure is cached, a definite `Available=False`, and that is a trade rather than an invariant: it avoids re-paying the wait in every later suite, and costs a mirror that recovers mid-run. On a node-join failure the suite dumps the mirror's state alongside the Talos image cache it already dumped. Workers being pointed at the mirror and then joining does not establish that the mirror served anything, because Talos falls back to public ghcr.io on its own; the registry's access log is what separates the two, so the dump counts kubelet-image requests across the whole log rather than tailing it. The readiness probe writes an access line every five seconds, so by the time a run has spent its 18m budget a fixed tail window is hundreds of lines short of the request worth finding. ### Testing - `helm unittest packages/apps/kubernetes`, 205 tests, 22 suites; `helm unittest packages/apps/kubernetes-nodes`, 14 tests, 4 suites. New cases: the default omits `machine.registries.mirrors`, and a set value renders it with the endpoint. - `hack/ghcr-mirror_test.bats`, 23 tests. Beyond the manifest's two-phase split, egress identities, pull-through config and the pure YAML builder, this covers every outcome of `resolve_ghcr_mirror_endpoint` against a stubbed `kubectl`: an API NotFound caches, a transient query failure does not, a shell reporting a missing binary is not mistaken for either, a failed egress allow does not cache and its reason reaches the log, a rollout watch that failed for a reason the API does not confirm does not cache while a definite `Available=False` does, and a committed endpoint is cached and short-circuits the next call. The stub honours `--tail`, so the diagnostic's tests tell a filtered read from a truncated one instead of trusting the flag, and it reports errors on stderr as the real binary does. The suite also pins that the install step excludes the Cilium policy from its pre-Cilium apply, not only what that exclusion produces. - `hack/talos-reconcile-heredoc_test.bats`, 5 tests, which render both charts and run the heredoc through a real shell with hostile values rather than regexing the rendered string. A `matchRegex` cannot catch a heredoc the shell refuses to emit; this runs it. - `hack/run-kubernetes-talos-spec_test.bats`, 5 tests over `talos_spec_block`, the point where the two optional fragments are merged. The assertions splice the result under `spec:` and read it back with yq, so a block indented into the wrong parent fails instead of passing. - `packages/apps/kubernetes-nodes/tests/render-parity.sh` byte-identical; per-chart `make generate` in both charts with no drift; both `cozyrds` `openAPISchema` blocks equal to their `values.schema.json`. - The pinned `mirror.gcr.io/library/registry:2.8.3@sha256:a3d8aaa6...` is the digest Docker Hub serves for that tag, and `mirror.gcr.io` serves the same OCI index with a linux/amd64 manifest. Run under exactly the security context the Deployment sets, non-root 65532, read-only root filesystem, all capabilities dropped, writable state only on the mounted volumes, it starts, answers `/v2/` with 200, and serves `ghcr.io/siderolabs/kubelet` manifests and blobs as an anonymous pull-through. - `shellcheck` and `git diff --check` clean on the new and edited shell. ### What a CI run still has to answer Two runtime facts the harness half rests on, both recorded in the manifest: 1. Talos honours a plain-http mirror endpoint via `machine.registries.mirrors[ghcr.io].endpoints=[http://...]`. If a scheme or TLS knob is additionally needed, `machine.registries.config` has to be added too. 2. The containerd pull from inside the worker VM leaves with the virt-launcher Pod's own Cilium identity, which is what the policy selects. The label half is settled: `run-kubernetes.sh` already reads tenant worker Pods by `kubevirt.io=virt-launcher`, but whether the in-guest pull is subject to that Pod's egress policy at all is not. The second one bounds how strong a claim the harness can make. The gate is a `rollout status` plus an accepted egress-allow object, and neither observes the tenant-side datapath, so the fallback paths cannot make CI worse while the committed path is only as safe as that assumption. The in-repo comments say this rather than claiming the mirror can only ever help. ### Named improvements left out A tenant-scoped reachability probe, like the byte-level 206 check `talos-image-cache.sh` runs from a Pod carrying the consumer's own label, is what turns assumption 2 from an assertion into a measurement, and would also let the suite assert the mirror served the kubelet pull instead of the worker quietly falling back. Left out because a probe that does not exercise the tenant network path would pass while real workers stayed blocked, and the one that does is a change of its own size. The same gap has a cheaper edge: both the install step and the resolver key on the Deployment alone, so an apply that made the Deployment but not the Service still reads as success. That one costs little in practice, since the guest gets a fast NXDOMAIN instead of a dropped SYN, but the gate does check less than it commits to. `registryMirrors` could be a typed `map[string]struct{ endpoints []string }`, the way `nodeGroups` already is in this chart, so malformed input is rejected at admission instead of failing late inside Talos. The helm-unittest escape cases assert the `$` path; the backtick path is covered by the execution-level heredoc bats and the pure-backslash path by the escape chain's construction, so the security contract is fully exercised across the two layers. A backtick assertion at the helm-unittest layer would express it at the fast layer too. The escape chain is spelled out at five interpolation sites across the two reconcile-Job templates. A named helper (an `include` template) would make the escape a single reusable primitive and shrink the surface where the heredoc invariant can be forgotten; the current per-site form is correct and covered, so this is a readability refactor rather than a fix. Only `machine.registries.mirrors` is passed through. `machine.registries.config` is not, so a mirror behind a private CA or one needing credentials still has no knob, and the platform-wide `registries` value does not reach tenant workers. The chart's breaking-change note says so, and also says which knob covers which artifact: the mirror routes registry pulls, while the worker OS disk image arrives as a raw HTTP artifact under `talos.imageFactoryURL` and no registry mirror can touch it. The helper's failure model is worth revisiting as a whole rather than per call site. Every call it makes to the cluster has three outcomes (the world says no, the world says yes, the call could not be made) and a shell exit code carries two, so the third is re-derived at each site from a different signal: stderr text for a NotFound, a field value for readiness, the returned message for a failed apply. Since the resulting decision is cached for the whole sandbox, a misclassification is permanent for the run rather than a blip, which makes each of those sites load-bearing. A single prober answering `yes`/`no`/`unknown`, with the caching rule stated once over that answer, would move the question out of the branches; the current shape reaches the same behaviour but re-establishes it five times. Left out here because it is a visible rework of code this PR only extends, and it would need its own verification of every site; tracked in #3682. One nicety left in the same helper: the diagnostic reads the mirror log twice, once with `--tail=-1` to count kubelet-image requests across the whole log and once with `--tail=50 --prefix` for a bounded context tail. The second read exists for `--prefix`, which attributes each line to its pod; collapsing to one read drops that. The attribution is worthless while the mirror is a single replica and load-bearing the moment it is not, in the one artifact read after a failure, and the second read is itself wall-clock bounded, so it is left in place. ### Merge-order dependency (resolved) #3676 landed first and this branch is rebased onto it. `ghcr_mirror_diagnose` now runs in the node-join failure block under the same phase gate as its neighbours, and each of its reads is wall-clock bounded by the same `COZY_DIAG_READ_TIMEOUT`/`COZY_DIAG_READ_GRACE` the block validates, so no single read can hold the op open. It is placed after the guest captures and before the image-cache re-probe: the console evidence is irreplaceable and must not be starved of budget, and the dump is still cheaper than the Pod-creating re-probe, so the spend order puts it between the two. When the diagnostics phase runs out of budget the dump is declined out loud instead of attempted. An ungated dump would spend that time anyway and take it from the captures behind it, while the mirror's state is partly recoverable from the reads that already ran, so gating costs less than it saves. The known cost is #3686: cheap reads ahead of the gate can drain the whole budget on a viscous apiserver, and the dump is then declined in exactly the runs where the mirror is a suspect. That is a defect of the budget, not of this dump, and it is tracked there. ### Scope against #3513 #3513 asks for three things: capture the guest side, act on the evidence, and do not raise the 18m budget again. The capture landed in #3548 and this consumes its output; the budget is untouched. What this acts on is the TLS-handshake variant only. The same thread documents runs that issue no apid CSR at all inside the whole budget, time lost before Talos reaches the kubelet image, which no registry mirror can affect. So #3513 stays open after this merges, and a green suite afterwards is not evidence the flake is closed. The manifest header says so too, for whoever reads it next. ### Both tenant suites are covered `kubernetes-previous` selects the previous Kubernetes *minor* out of `packages/apps/kubernetes/files/versions.yaml`, `run_kubernetes_test 'keys | sort_by(.) | .[-2]'`, not a previously-released chart. Both suites build their tenant CR from the same in-tree chart through the same helper, so both get the mirror. ### Screenshots Not applicable; no UI changes. ### Downstream repositories Walking the trigger map in `docs/agents/contributing.md` against the diff: **cozystack/terraform-provider-cozystack is affected**. The provider is hand-written, and the map lists "add, remove or rename a field in an app's `values.schema.json`" and "change a default in an app's `values.yaml`" as triggers for its schema, model and expand/flatten pair. This adds `talos.registryMirrors` with a `{}` default to both the `kubernetes` and `kubernetes-nodes` schemas. No other repository matches a trigger. The website's managed-app reference pages regenerate from each package's `README.md` on a stable tag, and this only changes an existing package's README. I have not opened the provider follow-up and have not ticked the box, because a ticked box with no link claims work that does not exist. Leaving it to a maintainer to decide whether the provider models this field and who files it. - [ ] 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 ```release-note feat(kubernetes): add `talos.registryMirrors` to route tenant worker node image pulls through a registry mirror for air-gapped or flaky-egress environments ```
This was referenced Aug 10, 2026
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
The node-join failure diagnostics in
hack/e2e-chainsaw/_lib/run-kubernetes.shissued about a dozenkubectlreads with neither an outer wall-clock bound nor--request-timeout, starting with adescribe nodesthrough 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:
talos_image_cache_diagnosegets its reads bounded and its gate gets--ignore-not-found, becausekubectl getexits 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;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:
bringupandlargestare literals in the guard, so a collector heavier than today's will not move them, and the margin is small by design;_libfile sourced ahead of both libraries would collapse two of them;Testing
hack/run-kubernetes-node-join_test.batsis new: 22 cases driving the block with akubectlstub that hangs, refuses and answers in part, and atimeoutstub 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 itsexit 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
Downstream repositories
Release note
Summary by CodeRabbit
Bug Fixes
Documentation
Tests