fix(e2e): capture Talos logs for tenant worker join failures - #3548
Conversation
Tenant Kubernetes worker join failures currently stop at host-side VMI and CSR state because the worker-only topology does not materialize a tenant talosconfig. Mint a short-lived os:reader client certificate and run talosctl from a hardened same-namespace Pod after the Ready deadline. Preserve bounded dmesg and kubelet captures for every worker without masking the original test failure. Assisted-By: GPT-5 <noreply@openai.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
📝 WalkthroughWalkthroughTenant worker-join failures now collect Talos ChangesTenant Talos diagnostics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NodeJoinTest
participant KubernetesAPI
participant TalosDiagnosticsPod
participant WorkerTalos
NodeJoinTest->>KubernetesAPI: discover worker VMIs and IPs
NodeJoinTest->>KubernetesAPI: create reader Certificate and Secret
NodeJoinTest->>TalosDiagnosticsPod: create hardened Pod with Talosconfig
TalosDiagnosticsPod->>WorkerTalos: collect dmesg and kubelet logs
TalosDiagnosticsPod-->>NodeJoinTest: return output and exit status
NodeJoinTest->>KubernetesAPI: delete diagnostic Certificate, Pod, and Secret
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
Use Bash and awk assertions in the Talos diagnostics regression tests so they run in the standard E2E sandbox without optional search utilities. Assisted-By: GPT-5 <noreply@openai.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM. The reader Certificate is rejected at admission, so the capture never runs.
hack/e2e-chainsaw/_lib/run-kubernetes.sh:277-279 sets subject.organizations: [os:reader] and nothing else. cert-manager wants at least one of commonName / dnsNames / uris / emailAddresses / ipAddresses / otherNames, and it does not look at subject at all. From ValidateCertificateSpec in v1.20.2, which is what we ship (packages/system/cert-manager/charts/cert-manager/Chart.yaml:9):
if len(commonName) == 0 &&
len(crt.DNSNames) == 0 &&
len(crt.URIs) == 0 &&
len(crt.EmailAddresses) == 0 &&
len(crt.IPAddresses) == 0 &&
len(crt.OtherNames) == 0 {
el = append(el, field.Invalid(fldPath, "", "at least one of commonName (from the commonName field or from a literalSubject), dnsNames, emailSANs, ipAddresses, otherNames, or uriSANs must be set"))
}So kubectl apply fails, cozy_prepare_tenant_talosconfig returns 1 at :313, and every node-join failure prints failed to create short-lived tenant Talos reader config with zero dmesg and zero kubelet output. Adding a commonName fixes it. Talos takes the role from the Organization, so the value does not matter.
The unit suite can't catch this. kubectl is a mock that returns 0 for apply (hack/run-kubernetes-talos-diagnostics_test.bats:18-21), so the tests check the rendered YAML and never that an API server accepts it. No CI leg reaches this code either, it needs a real node-join failure.
Second one. timeout: 40m on the script op is unchanged (hack/e2e-chainsaw/kubernetes-latest/chainsaw-test.yaml:22, hack/e2e-chainsaw/kubernetes-previous/chainsaw-test.yaml:16), but the new block adds about 4m45s of bounded budget for two workers: 10s + 10s stale deletes, 30s issuance, 10s pod delete, 60s pod readiness, 60s + 20s streaming, 40s per VMI. That lands after the 18m node-join wait, before (a2) and (c), and before the EXIT-trap tenant crust-gather at :235 which can take another 390s. A kill there is SIGKILL on the process group, so the tenant snapshot goes with it. Raise the 40m or show the measured headroom.
Related, same budget: cozy_capture_tenant_talos:474 mints the Certificate and starts the Pod before checking whether any VMI has an IP. In the mode where no worker booted, which is what the empty-IP branch at :516 is for, that burns ~3m30s to produce only capture-error.log.
Nits:
ubuntu:24.04(:374) is not prepulled and is not used anywhere else in the tree, so the 60s readiness wait has to cover a cold Docker Hub pull on an already degraded cluster.talos_image_cache_diagnoseon the same failure path reusesalpine/k8s, which is on every node already (hack/e2e-talos-image-cache.yaml:81). If the talosctl build is statically linked that image works here too, otherwise prepull the ubuntu digest.stat -c '%a'(hack/run-kubernetes-talos-diagnostics_test.bats:123) is GNU-only and the only one in the tree. Linux CI is fine, on a BSD stat the suite goes red for a reason that has nothing to do with the code under test.- #3444 is open and touches the same file and
.chainsaw.yaml. Worth sequencing the two.
The rest holds up and I checked it instead of assuming. os:reader is exactly enough, Talos maps Dmesg and Logs to role.Reader, and no admin credential gets materialised anywhere. <release>-talos-ca is the machine CA the workers carry as machine.ca.crt. Workers are on pod-bridge (packages/apps/kubernetes/templates/cluster.yaml:174), so the VMI IP is routable and server-side TLS stays verified, which an LB or port-forward address would have broken. The Pod passes restricted PSA. Both crust-gather calls use --exclude-kind Secret and the workdir sits outside the report tree. The () function body gets its own trap table, so _tenant_snapshot_on_fail survives. The original failure, the 18m budget and the success path are untouched, and the test-impact claim holds: hack/select-e2e.sh:86 escalates _lib/* to the full suite.
Ran locally: 8/8 new tests green, drain suite still 7/7, bash -n clean, shellcheck --severity=warning only the pre-existing SC2034 at :531.
Fix admission, capture setup, timeout headroom, and portability. Assisted-By: GPT-5 <noreply@openai.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM. Both blockers are closed, but the new test helper is a bashism and the unit job is already red on it.
hack/run-kubernetes-talos-diagnostics_test.bats:84 reads the file with case "$(<"${file}")". hack/cozytest.sh is #!/bin/sh and Makefile:170 runs it directly, so on Ubuntu the interpreter is dash, where $(<file) expands to an empty string instead of the file contents. Every assert_file_contains fails after that.
dash -c 'x=$(</tmp/f); echo "[$x]"' -> []
bash -c 'x=$(</tmp/f); echo "[$x]"' -> [hello-needle]
The suite under /bin/dash ./hack/cozytest.sh stops at "reader Certificate is short-lived and never embeds credential data" with expected ... to contain: name: kubernetes-test-latest-version-e2e-talos-reader. The unit job on 60e44b2 fails at that same test with that same message and make: *** [Makefile:165: bats-unit-tests] Error 1. $(cat "${file}") is the whole fix, line 84 is the only occurrence in the tree. Worth turning around quickly: a red unit job also skips the e2e gate, so the diagnostics path stays unexercised until this is green.
Everything else is closed, and I checked each one rather than looking for the edit.
commonName is in, subject.organizations: [os:reader] is untouched, and the CN comes out 47 and 49 characters for the two suites, nowhere near the 64 limit. ValidateCertificateSpec passes now.
Budget is 50m in both suites, and the arithmetic in the comment is tighter than what I posted: 5m10s counts the -k 5 kill grace on every bounded call, which my 4m45s missed. The yq test pins 50m, so the number is guarded by a check now instead of a comment. One thing to watch, and it is not an objection to this change: two kubernetes suites at 50m move the worst case from 80m to 100m inside the 180-minute job cap that the .chainsaw.yaml preamble warns about. Raising the containing timeout is exactly what that preamble prescribes, so the fix is right, the cap is just closer than it was.
The image moved to alpine/k8s on the same digest the cache manifest already pins, it went into the prepull list, and a test asserts the pod digest also appears in hack/e2e-install-cozystack.bats, so the two cannot drift apart silently. The static-linking premise holds: upstream Talos builds with CGO_ENABLED ?= 0, and even the race build passes -extldflags '-static'.
stat -c is gone and LC_ALL=C ls -ld with a case on -rw------- reads the same on both stats. Dropping the rg dependency was a good call on its own, it just needs line 84 fixed to land.
Skipping Certificate issuance and the helper Pod when no VMI reports an IP is in, with a test that asserts no apply and no exec happen on that path.
Ran locally: 10/10 green under bash, both through the repo runner and plain bats, and the same suite under dash fails as described above.
assert_file_contains used $(<file), a bash-only construct. cozytest.sh is run under /bin/sh (dash on the Ubuntu CI runner via Makefile), where $(<file) expands to an empty string instead of the file contents, so every assertion after it failed and reddened the unit-tests job, which in turn skips the e2e gate and leaves the diagnostics path unexercised. Read the file with $(cat ...), which is POSIX and behaves identically under bash and dash. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
The in-guest diagnostics chain runs only when the sandbox is already degraded enough to miss the 18m node-join deadline (the documented management-etcd fsync-storm regime), yet its kubectl legs were unbounded while every talosctl leg is wrapped in timeout. A stalled apiserver call in the (b) capture could consume the remaining step budget before (a2), (c) and the EXIT-trap tenant crust-gather run, so a diagnostics feature could reduce total diagnostic yield under the exact conditions it targets. Add --request-timeout=30s to the raw Certificate/Pod apply, the three reader-Secret reads and the worker VMI list, and route the Certificate Ready wait through kubectl_wait_retry so a single transient etcd-leader flap retries instead of aborting the whole capture. Update the bats assertion for the reworded wait call. Also correct a stale comment: the chainsaw step timeout is 50m, not 40m. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
The describe/get-events/get-certificaterequests calls that run when the reader Certificate or the diagnostics Pod misses its readiness wait were unbounded, yet those branches fire precisely when the management apiserver is wedged. A stalled forensic call there can burn minutes of the 50m step budget before the pre-existing (a2)/(c) diagnostics and the EXIT-trap tenant snapshot run. Add --request-timeout=30s to them, matching the happy-path capture calls. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hack/run-kubernetes-talos-diagnostics_test.bats (1)
247-251: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why the test creates
$tmp/talosctland changes directory.
cozy_prepare_tenant_talos_diagnostics_podrunstalosctl_bin=$(command -v talosctl). In this filetalosctlis a shell function, socommand -vprints the bare nametalosctlinstead of an absolute path. The later<"${talosctl_bin}"redirect therefore resolves against the current directory, which is why lines 247-249 create a same-named regular file andcdinto$tmp. This coupling is invisible to a future reader and breaks silently if the mock changes.Add a short comment recording the mechanism.
♻️ Proposed comment
+ # `talosctl` is mocked as a shell function, so `command -v talosctl` inside + # cozy_prepare_tenant_talos_diagnostics_pod yields the bare name. Its + # `<"${talosctl_bin}"` redirect then resolves relative to the cwd, so provide + # a readable stand-in file and run from $tmp. printf '%s\n' '#!/bin/sh' >"$tmp/talosctl" chmod 755 "$tmp/talosctl" cd "$tmp"🤖 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-talos-diagnostics_test.bats` around lines 247 - 251, Add a short explanatory comment immediately before the temporary talosctl creation and directory change, referencing cozy_prepare_tenant_talos_diagnostics_pod, command -v resolving the shell-function mock to the bare name talosctl, and the redirect requiring that same-named file in the current directory.
🤖 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.
Nitpick comments:
In `@hack/run-kubernetes-talos-diagnostics_test.bats`:
- Around line 247-251: Add a short explanatory comment immediately before the
temporary talosctl creation and directory change, referencing
cozy_prepare_tenant_talos_diagnostics_pod, command -v resolving the
shell-function mock to the bare name talosctl, and the redirect requiring that
same-named file in the current directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e576d0e9-8b07-4ca2-a3fb-ad9a509f7737
📒 Files selected for processing (5)
hack/e2e-chainsaw/_lib/run-kubernetes.shhack/e2e-chainsaw/kubernetes-latest/chainsaw-test.yamlhack/e2e-chainsaw/kubernetes-previous/chainsaw-test.yamlhack/e2e-install-cozystack.batshack/run-kubernetes-talos-diagnostics_test.bats
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM.
$(< is gone from the tree, and the suite now passes under dash, which is the shell it failed on before: 10/10 there and under bash. The unit job on cfbfd77 is green.
Bounding the previously open-ended calls with --request-timeout=30s is a good addition on its own. It does leave the worst case in the timeout: 50m comment stale. Counted the same way that comment counts, the bounded setup is now around 9m20s rather than 5m10s, mostly from kubectl_wait_retry (three 30s attempts plus two 5s sleeps) and six calls that can each take 30s. 50m still covers 18m + 9m20s + 6m30s with room left for the bring-up waits, so nothing needs raising, but the number in the comment should be corrected: the yq test pins 50m, not the arithmetic behind it, so a future edit will be sized against a figure that is off by nearly half.
One thing for whoever touches this next. The kubectl_wait_retry call added here sits under if !, which suppresses errexit for the whole function body, so the retry allow-list is reachable and the helper does what it says. The other calls to that helper in this file are plain commands, where _out=$(kubectl wait ...) kills the shell on the assignment line under set -e and the retry never runs. Not this PR's problem, and I am not asking for it here, but the two forms behave differently and only one of them retries.
Tenant worker Talos nodes pull ghcr.io/siderolabs/kubelet directly; the CI runner's public ghcr.io egress is flaky and the pull times out (TLS handshake timeout), so the kubelet service never starts and no tenant node joins within the node-join budget (#3513, in-guest evidence from #3548). Run an in-sandbox registry:2 pull-through cache for ghcr.io and point tenant workers at it via the chart's talos.registryMirrors knob, mirroring the talos-image-cache pattern that already decouples the flaky Talos OS-image fetch. A CiliumClusterwideNetworkPolicy punches a tightly-scoped egress hole for the worker VM (kubevirt.io: virt-launcher) pods to reach the mirror; the run-kubernetes helper emits registryMirrors only when the mirror is up, so workers fall back to direct pulls and the mirror can only help. NOT locally validatable (no live cluster): see the draft PR body for the CI assumptions to confirm (registry image availability, Talos http-mirror handling, worker egress identity) and the kubernetes-previous caveat. Refs #3513 Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
…#3579) ## What this PR does The tenant backend Deployment in the kubernetes suites had one 300s budget to reach `condition=Available`, and two unrelated variable costs shared it. The first is scheduling. What the suite establishes before that point is two tenant nodes Ready, which is weaker than schedulable: a Ready node still carries `node.cilium.io/agent-not-ready` until the tenant cilium agent claims it, and a node the bringup has not finished with is `SchedulingDisabled`. In the run recorded in #3577 scheduling took 2m18s and 1m57s in the two suites, which left the image pull to finish inside what remained. It did not, and both suites reported the same `timed out waiting for the condition` for what were two different shortfalls. So this waits for a node that actually accepts a Pod before creating the workload, on its own budget and its own failure message. The gate encodes the scheduler's rule for a Pod that tolerates nothing (Ready, not unschedulable, no `NoSchedule` or `NoExecute` taint) over a `custom-columns` probe, prints the node table on both outcomes so a timeout names the taint that held it, and treats a failed probe as not-schedulable so an API blip cannot release it. On the happy path it adds no wall time: it spends the seconds the Pod would otherwise spend Pending, plus at most one 5s poll interval. On a failing run the two budgets stack, so a run that exhausts both now gives up at ~600s where it used to give up at 300s, inside the 40m Chainsaw script op the suites document as a ~25m bringup. The readiness wait keeps its 300s, now starting from a schedulable node, and dumps deployment, pod and event state when it runs out. The workload image is also pinned by digest. The tenant workers reach no Docker Hub mirror (`hack/e2e-talos-image-cache.yaml` serves the Talos worker OS disk image over HTTP and is not a registry mirror), so nginx is pulled from Docker Hub on every run either way. The digest does not take the pull off the critical path; it fixes what that pull returns instead of leaving a floating tag free to change size and content under a fixed deadline. Preloading the image would need infrastructure this tree does not have, and is not attempted here. Nothing will bump the pin: no renovate manager reads `hack/`, and the comment says so, because for a throwaway test workload the freeze is the point. What this does not claim: it removes two measured consumers from a fixed budget, both taken from the events of a single run. Whether that budget was the only thing making those suites red is not established from one run, so this is not offered as the fix for a red pipeline. `hack/run-kubernetes-schedulable_test.bats` covers the new logic: every branch of the predicate, the multi-node scan, the poll-again path, the deadline, and a probe that fails. Each test was verified by mutating the helper and checking that the intended test, and no other, went red. Note for whoever reviews alongside #3575: that branch mirrors `ghcr.io` for tenant worker pulls, not `docker.io`, so it does not change the Docker Hub pull described here. `git merge-tree` reports no conflict between the two, nor with #3548. Observed in #3577. ### Screenshots Not a UI change. ### Downstream repositories - [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: The trigger map was walked against the diff. The change is confined to `hack/e2e-chainsaw/_lib/run-kubernetes.sh` and one new `hack/*.bats` unit test. It moves and renames nothing under `hack/`, changes no make target, and does not touch `hack/e2e-prepare-cluster.bats`, any package, any CRD or any namespace name, so none of the listed repositories are reached. ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Kubernetes deployment readiness by waiting for a suitable, schedulable node before timing backend startup. * Added clearer diagnostics when nodes are unavailable or backend readiness fails. * Replaced the floating backend container image tag with a fixed, verified version. * **Tests** * Added coverage for node readiness, cordoning, taints, polling, timeouts, probe failures, and deployment sequencing. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Tenant worker Talos nodes pull ghcr.io/siderolabs/kubelet directly; the CI runner's public ghcr.io egress is flaky and the pull times out (TLS handshake timeout), so the kubelet service never starts and no tenant node joins within the node-join budget (#3513, in-guest evidence from #3548). Run an in-sandbox registry:2 pull-through cache for ghcr.io and point tenant workers at it via the chart's talos.registryMirrors knob, mirroring the talos-image-cache pattern that already decouples the flaky Talos OS-image fetch. A CiliumClusterwideNetworkPolicy punches a tightly-scoped egress hole for the worker VM (kubevirt.io: virt-launcher) pods to reach the mirror; the run-kubernetes helper emits registryMirrors only when the mirror is up, so workers fall back to direct pulls and the mirror can only help. NOT locally validatable (no live cluster): see the draft PR body for the CI assumptions to confirm (registry image availability, Talos http-mirror handling, worker egress identity) and the kubernetes-previous caveat. Refs #3513 Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Tenant worker Talos nodes pull ghcr.io/siderolabs/kubelet directly; the CI runner's public ghcr.io egress is flaky and the pull times out (TLS handshake timeout), so the kubelet service never starts and no tenant node joins within the node-join budget (#3513, in-guest evidence from #3548). Run an in-sandbox registry:2 pull-through cache for ghcr.io and point tenant workers at it via the chart's talos.registryMirrors knob, mirroring the talos-image-cache pattern that already decouples the flaky Talos OS-image fetch. A CiliumClusterwideNetworkPolicy punches a tightly-scoped egress hole for the worker VM (kubevirt.io: virt-launcher) pods to reach the mirror; the run-kubernetes helper emits registryMirrors only when the mirror is up, so workers fall back to direct pulls and the mirror can only help. NOT locally validatable (no live cluster): see the draft PR body for the CI assumptions to confirm (registry image availability, Talos http-mirror handling, worker egress identity) and the kubernetes-previous caveat. Refs #3513 Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
…ostics_test.bats #3548 added hack/run-kubernetes-talos-diagnostics_test.bats carrying 8 EXIT-trap cleanups but did not register it in the frozen set of the cozyreport.bats EXIT-trap guard, so the guard's found set no longer matches frozen and the 'Unit & controller tests' job fails on main and on every branch rebased onto it. #3584 fixed the same class for multus-install-cni-plugins.bats but not this file. Add the count, exactly as that guard's own comment prescribes for a new file that arrives carrying traps. Value 8 confirmed by the guard's own fold_source + grep and by the failing CI run. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
The ban on EXIT-trap cleanup in hack/*.bats was enforced by two lists inside hack/cozyreport.bats: one naming the files known to be clean, and one freezing an exact trap count for each file that was not. Both had to be edited from whatever change moved them, and that is where the guard failed, and it never worked even once. The inventory landed in #3567 at 15:25:20; #3195 had landed multus-install-cni-plugins.bats carrying twelve traps forty-three seconds earlier, so replaying the old guard against the tree at the commit that introduced it already gives found != frozen. Main stayed red for about twenty-two and three quarter hours. The first repair, #3584, landed already red the next morning because #3548 had brought in run-kubernetes-talos-diagnostics_test.bats with eight traps two minutes ahead of it, so that fix bought no green time at all; green came only with the second repair. Neither pair of PRs shared a line, and each was green against its own base. A change adding a trap to its own file had to edit a string in a suite it otherwise never touches, so two changes sharing no line still invalidated each other: each stayed green against its own base, git merged both cleanly, and the guard went red only once the second landed. A file arriving with traps hit the same wall from the other side -- the inventory did notice it, since the string it compared was built by scanning the directory, but absorbing it meant an edit in a file nothing in the author's diff pointed at. Replace both lists with a declaration each file makes about itself: one "# EXIT-TRAP DEBT: N" comment, exact rather than a ceiling. A file carrying none must install no EXIT trap, which covers the converted files, the files that never had one, and every file added later. Growing or shedding a trap now fails in the file the change already edits, so two changes that disagree about a count collide textually instead of silently. That collision is a steady-state property, and this change's own arrival is the exception. A branch forked before the declaration existed has no line to disagree with: it converts traps in its own file, merges clean, and the count goes wrong only once both sides are on main -- verified against a sibling branch that takes select-e2e_test.bats from fifteen traps to zero. What the move buys even then is that the red names a file that branch already edited, the repair is one line inside it, and rebasing before merge catches it on the branch's own CI. None of those three held against the inventory. The declaration is read from the leading comment block, not from anywhere in the file. A .bats file is shell that writes shell, so the same line turns up inside a heredoc, a fixture writer or an expected-output string, where it is data belonging to one test. Reading it there as a statement about the whole file would let an unrelated fixture excuse a real trap, and would do it silently, since nothing in that test's own diff looks like a declaration. A comment block is the region with no interior: stopping instead at the first @test would still read a line out of a helper's heredoc. Not every counted handler is debt. A trap inside an explicit subshell does not replace the bats binary's own, so a test failing inside `( ... )` still prints its `not ok`. hack/e2e-test-openapi.bats kills a backgrounded kubectl proxy that way, and moving the kill to the end of the body would leak a process holding a fixed port. Its declaration records that rather than scheduling a conversion, and because the ratchet is exact, removing the trap fails too -- the count protects the construct. What the count cannot do is tell the two apart: substituting a test-level trap for the subshell one keeps the total at 1 and stays green, which the header states rather than leaves to be found. Counting bounds the keyword and the signal the same way, at any character that cannot be part of an identifier, and matches the signal in either case. Whitespace on the right missed `trap ... EXIT; cd "$tmp"`; whitespace on the left missed `tmp=$(mktemp -d);trap ... EXIT` and `(trap ... EXIT; true)`; upper case missed `trap ... exit`, which bash and dash both install. All are real handlers that scored zero, and the left boundary matters most, since the inventory being replaced had none and caught the semicolon form. A bare word boundary is not enough either way: `bootstrap ` ends in `trap `, and it must keep scoring nothing. A quoted signal counts for the same reason as the rest. Two handlers sharing one line are reported rather than counted, since the count is a count of lines and the second would otherwise arrive without moving the total; splitting the line properly needs a shell parser, a semicolon inside a handler's own action not being a separator, and guessing wrong undercounts -- the one direction a ratchet cannot afford. The scan recurses, so hack/e2e-apps/*.bats is covered rather than sitting one directory below the guard that claims the tree. It reads .bats and nothing else, so a handler arriving through a sourced .sh stays outside it -- hack/e2e-chainsaw/_lib/run-kubernetes.sh installs two, each benign for its own reason rather than by design: one sits in a function declared with `(` and so runs in a subshell, the other in a brace function no @test calls. That boundary is stated in the header rather than papered over. The guard moves to hack/bats-no-exit-trap.bats: its subject is every unit suite under hack/, not the report collector it grew up in. Its fixture helpers assemble the trap keyword and the signal from separate arguments, because the guard scans its own source and a fixture written as a literal would be counted as a real trap in it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
…#3622) <!-- 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 ban on EXIT-trap cleanup in `hack/*.bats` was enforced by two lists inside `hack/cozyreport.bats`: one naming the files known to be clean, one freezing an exact trap count for each file that was not. Both had to be edited from whatever change moved them, and that is where the guard kept failing. It is worth being exact about how badly, because the history is sharper than "it went stale a few times". **The inventory was never correct on main for a single commit.** It landed in #3567 at 15:25:20. #3195 had landed `multus-install-cni-plugins.bats` carrying twelve traps at 15:24:37, forty-three seconds earlier. Replaying the old guard's own logic against the tree at the very commit that introduced it already gives `found != frozen`. Main then stayed red for roughly twenty-two and three quarter hours. The first repair, #3584, landed *already red* the next morning at 10:42:43, because #3548 had brought in `run-kubernetes-talos-diagnostics_test.bats` with eight traps at 10:40:03, under three minutes ahead of it. That fix was correct and bought zero green time. Green arrived only with the second repair, #3602. Neither pair of PRs shared a line, and each was green against its own base. That is the whole mechanism, and it is why a third one-line repair is not the answer. The mechanism is structural rather than careless. A change that adds a trap to its own file had to edit a string in a suite it otherwise never touches, so two changes sharing no line still invalidated each other: each stayed green against its own base, git merged both cleanly, and the guard went red only once the second one landed. A file *arriving* with traps was worse still, because nothing in its author's diff pointed at that string at all. That is exactly how `run-kubernetes-talos-diagnostics_test.bats` got in, twice. That contention is not in the past tense. Two open PRs are editing that one line right now, and they disagree about what it should say: #3575 adds `run-kubernetes-talos-diagnostics_test.bats=8` to it, repeating a repair that has already landed, and #3441 removes `select-e2e_test.bats=15` from it, because it converts that file. Neither PR is about EXIT traps. Both have to touch that string anyway, and whichever lands second is wrong until someone edits it again. So this PR is not fixing a red main. It removes the thing that keeps making main red, which is why it is worth more than the one-line fix that is now the established habit. One honest caveat about its own landing. The textual-conflict property is steady-state: a branch forked *before* the declaration exists has no line to disagree with, so it converts traps in its own file, merges clean, and the count only goes wrong once both sides are on main. I checked this against #3441, which takes `select-e2e_test.bats` from fifteen traps to zero. Merged after this, that file would declare fifteen and hold none. What the move buys even in that case is that the red names a file the branch already edited, the repair is one line inside it, and rebasing before merge catches it on the branch's own CI. None of those three held against the central inventory. Whoever merges this should expect one such adjustment on the conversion branches still in flight. So this replaces both lists with a declaration each file makes about itself: one `# EXIT-TRAP DEBT: N` comment in its leading comment block. A file carrying no declaration must install no EXIT trap. Growing or shedding a trap now fails in the file the change already edits, so two changes that disagree about a count get a real textual conflict instead of silently invalidating each other, and a change that leaves the traps alone edits nothing. **The include list is redundant, not lost.** It named the files proven clean, so that a trap reappearing in one of them would fail. Under the new rule those files carry no declaration, and a file with no declaration must hold zero traps, so a trap reappearing in any of them fails on its own, with no list to be on. Coverage widens rather than narrows: the two lists named twenty files between them, and the rule covers all fifty bats files under `hack/`, subdirectories included, plus the ones added tomorrow. Rebasing this branch onto current main is the property working. Main has since gained `hack/kubernetes-pre-delete-hook.bats` and `hack/tenant-pre-delete-hook.bats`, and `hack/cozyreport.bats` grew by some eight hundred lines. Neither new file installs an EXIT trap, so neither needed a declaration and neither needed an edit here; the rebase took no conflict at all. Under the inventory, each arriving file was a coin toss on whether somebody had remembered the string. To be precise about what the inventory could and could not do, since it is easy to overstate: it did *notice* a new file carrying traps. The string it compared was built by scanning the directory, so an arriving file appended a token and failed the comparison, which is exactly how main went red. What it could not do is let that file arrive without an edit in a foreign suite. Being seen and being absorbable are different properties, and only the second one decides whether two changes can land independently. The declaration is pinned in both directions. Declaring N while holding N+1 fails, obviously; declaring N while holding N−1 fails too. Without that second half the number becomes a ceiling and rots upward: somebody converts half a file, the declaration stays, and the guard quietly licenses traps that were removed long ago. It is read only from the leading comment block, under the shebang and above the first line of code. A `.bats` file is shell that writes shell, so the same line turns up inside a heredoc, a fixture writer or an expected-output string, where it is data belonging to one test; honouring it there would let an unrelated fixture excuse a real trap, silently, with nothing in that test's own diff looking like a declaration. A comment block is the region with no interior; stopping instead at the first `@test` would still read a line out of a helper's heredoc. Not every counted handler is debt. A trap inside an explicit subshell does not replace the one the `bats` binary installs, so a test failing inside `( … )` still prints its `not ok`, checked against a test-level trap in the same file, where the TAP line vanishes. `hack/e2e-test-openapi.bats` kills a backgrounded `kubectl proxy` exactly that way, and "convert it like the others" would leak a process holding a fixed port and wedge the next run. Its declaration now records the carve-out instead of scheduling a conversion, and because the ratchet is exact in both directions, *removing* that trap fails too, so the count protects the construct rather than marking it for deletion. `docs/agents/e2e-testing.md` previously scoped this exception to Chainsaw `script` steps only; it now names the BATS subshell case as well. The counting bounds the keyword and the signal the same way, at any character that cannot be part of an identifier, and matches the signal in either case. Whitespace on the right missed `trap … EXIT; cd "$tmp"`; whitespace on the left missed `tmp=$(mktemp -d);trap … EXIT` and `(trap … EXIT; true)`; upper case missed `trap … exit`, which bash and dash both install. All of those are real handlers that scored zero. The left boundary is the one worth dwelling on, because the inventory being replaced had none at all and *did* catch the semicolon form. Getting it wrong here would have narrowed coverage while the commit claimed to widen it. A plain word boundary is not enough either: `bootstrap ` ends in `trap `, and it has to keep scoring nothing, or the documented answer to a red guard (add a debt line) would buy a file a permanent licence for one real trap to silence a line that has none. Two handlers sharing one line are reported rather than counted, because the count is a count of lines and the second would otherwise arrive free. Splitting such a line properly needs a shell parser, since a semicolon inside a handler's own quoted action is not a separator, and guessing wrong undercounts, the one direction a ratchet cannot afford. **What this does not fix.** The declaration is still a loophole: a new file can write `# EXIT-TRAP DEBT: 8` instead of cleaning up, and nothing here makes that impossible. What changes is that the admission is local and visible. It sits at the top of the file it excuses, in front of whoever reviews that file, instead of being a number in a neighbouring suite nobody in that review is reading. Today's loophole is the same size and invisible. Three more limits, all stated in the guard's own header rather than left to be discovered. The scan is lexical, so a signal computed at runtime and a quoted action spanning physical lines without a backslash are both invisible. An exact count catches addition and removal but never substitution: swap the openapi file's subshell trap for a test-level one and the total stays 1. And the scan reads `.bats` only, so a handler arriving through a sourced `.sh` is outside it. `hack/e2e-chainsaw/_lib/run-kubernetes.sh` installs two right now, and each is benign for its own reason rather than by design: the one in `cozy_capture_tenant_talos` because that function is declared with `(` and so runs in a subshell, the one in `run_kubernetes_test` because no `@test` calls it despite being declared with `{`. Three `hack/*.bats` source that library, and two tests in the converted file call `cozy_capture_tenant_talos`, so flipping a single `(` to `{` reinstates a test-level handler in both of them with the guard green. Widening the scan to `.sh` would mean counting handlers that are correct in a script and wrong only in a test body, so the honest answer is that this is where the instrument stops. Three further boundaries, recorded here so they land as known edges rather than as surprises. The old include list also failed when a file named on it disappeared from the tree; the new rule can only judge a file that is present, so a deleted converted file goes unnoticed. That is a genuinely smaller check, though its absence shows up in the diff that deletes the file. The guard's own failure messages are code lines, so they are scanned by the pattern they belong to: they pass today only because no bare `EXIT` or `0` happens to follow the keyword in any of them, and a rewording that introduced one would make the file demand a debt of itself. It fails loudly rather than quietly, and the fixture writers and test titles already split the keyword from the signal for this reason, but the messages do not. Finally, `docs/agents/e2e-testing.md` bans test-level `EXIT` *and* `RETURN` traps, while every mechanical guard this repo has had, the one being deleted included, matches only `EXIT` and `0`. `hack/` holds no RETURN trap today, so nothing regresses here, but half of that documented rule has never had an executor. The guard moves out of `hack/cozyreport.bats` into `hack/bats-no-exit-trap.bats`, because its subject is every unit suite under `hack/` and not the report collector it grew up in. Living inside `cozyreport.bats` is precisely why unrelated PRs kept converging on one line. `hack/md-no-hardwrap.bats` is the neighbouring precedent. Separately, the eight EXIT traps in `hack/run-kubernetes-talos-diagnostics_test.bats` are converted to end-of-body cleanup rather than given a declaration. That file was written six weeks after the ban was documented, so "it predates the rule" is not available to it, and handing it a debt is literally the move that failed the last two times. The remaining files keep their declarations, since a mass conversion would collide with the branches that own them. One note on where the prose sits. The guard's own header carries only what a reader has to keep true: why the ban exists, the two carve-outs, where the declaration lives and why there, that the count is exact in both directions, why the marker is anchored at column zero, and where the lexical scan stops. Everything above about *how* the inventory failed (the timestamps, the PR numbers, the arithmetic) is deliberately not in the file. It is true today and will rot on its own; a comment nobody is obliged to maintain is how a file grows a paragraph that is quietly false. ### 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 test(tests): each hack/*.bats file now declares its own remaining EXIT-trap debt in a `# EXIT-TRAP DEBT: N` header comment, checked by hack/bats-no-exit-trap.bats, replacing the central inventory in hack/cozyreport.bats ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Testing** - Added automated auditing for `EXIT` traps across Bats end-to-end tests, including validation of tracking declarations and edge cases. - Improved diagnostics tests by replacing trap-based temporary-directory cleanup with explicit cleanup steps. - Added tracking annotations for remaining trap-related cleanup work. - **Documentation** - Clarified when traps are permitted inside self-contained subshells and how remaining cleanup debt is reported. - Updated review guidance for consistent end-to-end test maintenance. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…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 ```
…3768) ## What this PR does The most expensive e2e flake, #3513, is a tenant worker missing the 18m node-Ready budget with zero Nodes registered. The in-guest capture in #3548 diagnosed one variant: the worker's kubelet image pull crawls to ghcr.io at 40-165 KB/s over the runner egress and does not finish inside the budget, Talos holds the kubelet service until its image arrives, so the node never registers and the tenant cilium HelmRelease then fails for want of a schedulable node rather than for any fault of its own. The sandbox already runs an in-cluster pull-through mirror for ghcr.io, but a pull-through cache fetches a blob only when a client first asks for it, so the first tenant worker pays the cold fetch inside its node-Ready budget. Measured with the same fetch script against a local instance of the same registry image: 32m47s cold, 1.1s warm, for the same tags the failing runs stalled on. This PR fills the cache from a Job started by the same install step that deploys the mirror, tens of minutes before the kubernetes suites run. It warms the two kubelet tags the suites actually select, resolved from the chart's version map in the same order the suites resolve it, and one platform per tag (the one the workers run, ~61 MiB each) - warming what nobody pulls would be a true addition to the same throttled egress the install itself is using. The warm-up is best-effort by construction: a Job that fails, times out or never starts does not fail the install, and the suites behave exactly as they did before it existed. The Job itself exits non-zero on any missed tag, so its status stays honest. The node-join failure diagnostics now answer the questions a red run used to leave open: whether a worker pulled through the mirror at all (counting GET and HEAD access lines per request, excluding the warm-up's own requests, which name themselves via user-agent), how long the mirror took to answer (server-side `http.response.duration` is the only after-the-fact signal separating a warm cache from a cold one, since the proxy stores blobs in the background and the Job reports success either way), and whether the warm-up itself got anywhere. The diagnostic section grows from five to seven bounded reads, moving its worst case from roughly 100s to roughly 150s inside the existing phase budget gate; unbudgeted diagnostic residuals stay tracked in #3666. This addresses one variant of #3513 and does not close it: the zero-CSR class remains untouched, and a green suite after this change is not evidence the flake is gone. The 18m node-Ready budget is not raised. Covered by 65 unit tests in `hack/ghcr-mirror_test.bats`: tag selection and ordering against the suites' own expressions, rejection of malformed and injected tag values, Job shape and restricted security context, the readiness wait as a wall clock, every failure path of the new diagnostics including an over-1-MiB log fixture that pins the duration filter against real log sizes, and the exclusion of the warm-up's own traffic from both the request count and the duration report. ### Screenshots No UI changes. ### Downstream repositories The diff is confined to e2e infrastructure under `hack/`: the mirror manifest, the warm-up library and its tests, the install step, and diagnostic labels. Walked the trigger map file by file - no chart values, no APIs, no documentation content, nothing any downstream repository consumes. - [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 ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added best-effort GHCR mirror warm-up during installation. * Preloads selected Kubernetes image manifests and blobs to improve cache readiness. * Warm-up failures safely fall back to direct image pulls. * **Diagnostics** * Improved request classification, timing, repository identification, and reporting of warm-up status and logs. * **Tests** * Expanded coverage for warm-up behavior, validation, retries, security settings, caching, logging, installation, and diagnostics. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
Addresses #3513.
When tenant Kubernetes workers miss the 18-minute Ready deadline, the E2E runner now mints a one-hour
os:readerTalos client certificate from the tenant's existing issuer and runs authenticatedtalosctlfrom a hardened same-namespace diagnostics Pod. It captures boundeddmesgand kubelet output for every worker VMI in the cozyreport snapshot, records missing IPs and command exit codes, continues across unreachable workers, and preserves the original test failure.The diagnostics Pod has no service-account token or Secret volume; credentials are streamed into an
emptyDir, and the Certificate, Secret, and Pod are labeled for cleanup. Workers that have not reached Talosapidcan only yield a bounded connection error, while workers that reachedapidprovide guest logs. This adds the evidence needed to distinguish the remaining guest boot failure but does not claim to resolve its underlying cause. The readiness budget and success path are unchanged.Because this changes a shared E2E helper, test-impact analysis selects the full E2E suite for this PR.
Screenshots
Not applicable; no UI changes.
Testing
hack/cozytest.sh hack/run-kubernetes-talos-diagnostics_test.bats(10 tests)hack/cozytest.sh hack/run-kubernetes-drain_test.batshack/cozytest.sh hack/cozyreport-talos.batsbash -n hack/e2e-chainsaw/_lib/run-kubernetes.shgit diff --check origin/main...HEADThe full
make bats-unit-testsrun reached the unrelatedhack/migration-seaweedfs-db-adopt.batssuite and could not continue because Docker is unavailable in the development environment. A live failure-path run was not performed because the available development environment has no Talos tenant fixture; this PR is intended to exercise that path in Cozystack CI.Downstream repositories
Release note
Summary by CodeRabbit
Bug Fixes
Tests
Chores