Skip to content

feat(kubernetes): route tenant worker image pulls through a registry mirror - #3575

Merged
Aleksei Sviridkin (lexfrei) merged 5 commits into
mainfrom
fix/e2e-tenant-worker-registry-mirror
Aug 9, 2026
Merged

feat(kubernetes): route tenant worker image pulls through a registry mirror#3575
Aleksei Sviridkin (lexfrei) merged 5 commits into
mainfrom
fix/e2e-tenant-worker-registry-mirror

Conversation

@IvanHunters

@IvanHunters IvanHunters commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

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.

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

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The PR adds Talos.RegistryMirrors to both Kubernetes APIs and Helm schemas. It renders mirror settings into worker Talos configurations. It also provisions an optional CI GHCR pull-through mirror with direct-pull fallback.

Changes

Registry mirror support

Layer / File(s) Summary
Talos API fields and deep-copy handling
api/apps/v1alpha1/kubernetes/..., api/apps/v1alpha1/kubernetesnodes/...
Both APIs add Talos.RegistryMirrors as a RawExtension. Generated deepcopy methods copy the nested configuration.
Chart values and Talos rendering
packages/apps/kubernetes/..., packages/apps/kubernetes-nodes/..., packages/system/..., hack/talos-reconcile-heredoc_test.bats
Schemas, values, and documentation expose talos.registryMirrors with an empty default. Reconcile jobs render the configuration and escape shell-sensitive values. Helm and shell-level tests validate the output.
CI mirror deployment and provisioning
hack/e2e-ghcr-mirror.yaml, hack/e2e-chainsaw/..., hack/e2e-install-cozystack.bats, hack/ghcr-mirror_test.bats
CI can deploy a GHCR pull-through cache, apply its Cilium policy, resolve its endpoint, and add it to tenant Talos specifications. Failed or unavailable mirror deployment uses direct GHCR pulls.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CI install test
  participant ghcr-mirror Deployment
  participant mirror resolver
  participant Talos spec builder
  participant tenant manifest

  CI install test->>ghcr-mirror Deployment: Apply Service and Deployment
  ghcr-mirror Deployment-->>mirror resolver: Report readiness
  mirror resolver->>Talos spec builder: Provide mirror endpoint or empty fallback
  Talos spec builder->>tenant manifest: Render spec.talos.registryMirrors
Loading

Possibly related PRs

  • cozystack/cozystack#3315: Both PRs modify shared Kubernetes/Talos worker-pool APIs, charts, and reconciliation templates.
  • cozystack/cozystack#3521: Both PRs modify Kubernetes Talos API and deepcopy structures, but add different configuration fields.

Suggested labels: kind/api-change, area/networking, area/testing

Suggested reviewers: kvaps, myasnikovdaniil

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes routing tenant worker image pulls through a registry mirror, which is the main change.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/e2e-tenant-worker-registry-mirror

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added size/L This PR changes 100-499 lines, ignoring generated files area/kubernetes Issues or PRs related to the tenant Kubernetes app kind/feature Categorizes issue or PR as related to a new feature size/XL This PR changes 500-999 lines, ignoring generated files and removed size/L This PR changes 100-499 lines, ignoring generated files labels Aug 6, 2026
@IvanHunters
IvanHunters marked this pull request as ready for review August 6, 2026 09:47

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@hack/e2e-chainsaw/_lib/ghcr-mirror.sh`:
- Around line 53-55: Update the successful ghcr-mirror rollout branch to set
endpoint="$GHCR_MIRROR_SVC_URL" only when _apply_ghcr_mirror_egress_policy
succeeds; when that function fails, leave the mirror endpoint unset so callers
fall back to direct GHCR pulls instead of using an unreachable Service.

In `@hack/e2e-chainsaw/_lib/run-kubernetes.sh`:
- Around line 288-294: Update run_kubernetes_test() to prove worker kubelet
images are served by the configured ghcr.io mirror: either block direct public
GHCR egress through the worker Cilium policy during this path, or assert after
worker boot that the mirror successfully handled a ghcr.io/siderolabs/kubelet
request. Ensure the test cannot pass solely because Talos pulled the image
directly from public GHCR.

In `@hack/e2e-ghcr-mirror.yaml`:
- Around line 64-99: Harden the registry Pod around the registry container and
Pod spec: run it as a non-root user, disable privilege escalation, drop all
capabilities, enforce RuntimeDefault seccomp, and set
automountedServiceAccountToken to false. Keep the root filesystem read-only
while preserving writable registry state through the cache volume at
/var/lib/registry, ensuring the selected image or initialization flow permits
the unprivileged process to write there.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8a4b7faa-36d5-4168-9d79-380a20caf915

📥 Commits

Reviewing files that changed from the base of the PR and between df157da and 020c545.

📒 Files selected for processing (22)
  • api/apps/v1alpha1/kubernetes/types.go
  • api/apps/v1alpha1/kubernetes/zz_generated.deepcopy.go
  • api/apps/v1alpha1/kubernetesnodes/types.go
  • api/apps/v1alpha1/kubernetesnodes/zz_generated.deepcopy.go
  • hack/e2e-chainsaw/_lib/ghcr-mirror.sh
  • hack/e2e-chainsaw/_lib/run-kubernetes.sh
  • hack/e2e-ghcr-mirror.yaml
  • hack/e2e-install-cozystack.bats
  • hack/ghcr-mirror_test.bats
  • hack/talos-reconcile-heredoc_test.bats
  • packages/apps/kubernetes-nodes/README.md
  • packages/apps/kubernetes-nodes/templates/talos-reconcile-job.yaml
  • packages/apps/kubernetes-nodes/tests/registry_mirrors_test.yaml
  • packages/apps/kubernetes-nodes/values.schema.json
  • packages/apps/kubernetes-nodes/values.yaml
  • packages/apps/kubernetes/README.md
  • packages/apps/kubernetes/templates/talos/talos-reconcile-job.yaml
  • packages/apps/kubernetes/tests/talos_templates_test.yaml
  • packages/apps/kubernetes/values.schema.json
  • packages/apps/kubernetes/values.yaml
  • packages/system/kubernetes-nodes-rd/cozyrds/kubernetes-nodes.yaml
  • packages/system/kubernetes-rd/cozyrds/kubernetes.yaml

Comment thread hack/e2e-chainsaw/_lib/ghcr-mirror.sh Outdated
Comment thread hack/e2e-chainsaw/_lib/run-kubernetes.sh
Comment thread hack/e2e-ghcr-mirror.yaml Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@hack/e2e-ghcr-mirror.yaml`:
- Around line 121-124: Add sizeLimit values to the cache and tmp emptyDir
volumes in the mirror Pod, and add matching ephemeral-storage requests and
limits to the registry container’s resources. Size both consistently for the
largest expected CI image set so the Pod’s node-local storage usage is bounded.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 77d372a5-a3cd-46bc-9c60-1ed0e1aff283

📥 Commits

Reviewing files that changed from the base of the PR and between 4990143 and 2692c49.

📒 Files selected for processing (2)
  • hack/e2e-ghcr-mirror.yaml
  • hack/ghcr-mirror_test.bats
🚧 Files skipped from review as they are similar to previous changes (1)
  • hack/ghcr-mirror_test.bats

Comment thread hack/e2e-ghcr-mirror.yaml Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOT LGTM

The chart half holds up. With talos.registryMirrors unset the rendered worker reconcile Job is byte-identical to main (I diffed the render, content-hash Job name included), make generate and both charts' cozyvalues-gen runs produce zero drift, the cozyrd openAPISchema matches values.schema.json, and the new tests have teeth: dropping the replace chain from the template turns both the helm-unittest case and the new heredoc bats red. The heredoc test is the good kind, running the rendered script through a real shell instead of regexing the string. Two blockers, both in the e2e half.

1. The documented fallback is unreachable under the shell the suite actually runs. hack/e2e-chainsaw/_lib/ghcr-mirror.sh:56 captures the probe with a bare assignment: out=$(kubectl -n kube-system get deploy ghcr-mirror 2>&1); rc=$?. Chainsaw v0.2.15 runs script.content through sh -c (the kubernetes suites set no shell: override), the step body sets set -eu, and /bin/sh on the CI runner is dash, which does inherit errexit into command substitutions. So a non-zero kubectl get kills the script on that line and neither branch below it runs. I reproduced it with a stub kubectl under dash: NotFound exits 1 without ever printing "ghcr-mirror not deployed", the transient-error branch (the one carrying the "must NOT be cached" comment) exits 1 the same way, and only the rollout-timeout path falls back correctly, because that one sits behind elif. Under bash it survives, since bash does not inherit errexit into $() without inherit_errexit, which is why it looks fine locally. Effect: hack/e2e-install-cozystack.bats deliberately tolerates a failed mirror apply with a warning, and after that every kubernetes-* suite hard-fails at tenant CR creation rather than pulling directly. That inverts the "can only help, never make CI worse" property the helper is built around. talos-image-cache.sh avoids it by putting the same call inside if kubectl ...; then; if out=$(...); then rc=0; else rc=$?; fi is enough here.

2. The mirror's own image is an unpinned anonymous Docker Hub pull on the install path of every run. hack/e2e-ghcr-mirror.yaml:81 is docker.io/library/registry:2.8.3, by tag. Every other helper manifest in hack/ pins by digest (docker.io/alpine/k8s:1.36.2@sha256:... in e2e-talos-image-cache.yaml and e2e-cilium-leak-healer.yaml), and this image is not in the pre-pull set, which covers only the kubeovn, linstor and cert-manager renders. The tree already moved away from anonymous Docker Hub pulls three times for this exact reason: the ouroboros kubectl sidecar and the harbor trivy DB both point at mirror.gcr.io, and PR base images are routed through it in the buildkit config. A change whose purpose is to stop depending on flaky anonymous registry egress should not add a fresh instance of it. mirror.gcr.io/library/registry plus a digest matches what the repo already does. The PR lists this as open assumption 1, so I read this as agreement rather than disagreement.

Non-blocking, roughly in priority order.

The helper commits to the mirror on rollout status plus the exit code of kubectl apply, and a zero from kubectl apply means the policy object was accepted, not that the datapath allows the flow. talos-image-cache.sh deliberately does not stop there: it runs a probe Pod in the tenant namespace, labelled with the identity the real consumer carries, and commits only on a byte-verified 206. The comment calls the probe a hardening follow-up, but the probe is what makes the invariant true instead of asserted, and the two things it would cover (Talos accepting a plain-http endpoint, and kubevirt.io: virt-launcher being the identity Cilium sees) are precisely the assumptions the PR lists as unvalidated. Without it, a mirror that is Available but unreachable costs a containerd dial timeout per pull inside the 18m node-join budget this change exists to protect. The cost is not only in the failure case either: the 5m rollout status is serial, in the same 40m step as the existing 12m image-cache wait, and blocker 2 is what triggers it.

I could not confirm the CI half without a run, but I did rule out the obvious objection to it. The worker's host resolver is the management cluster's CoreDNS with management search domains (machine.network.nameservers is set to the management kube-dns ClusterIP by the reconcile Job), and the worker already dials a management ClusterIP for its own apiserver through extraHostEntries, so ghcr-mirror.kube-system.svc resolving and routing from inside the VM is consistent with what already works. The tenant allow-to-dns policy allows kube-system/kube-dns specifically and not kube-system generally, so the manifest's reasoning for needing the extra allow holds.

spec.talos.registryMirrors is a free-form RawExtension, and it is the first one in this API that lands inside a shell-expanded heredoc rather than in Helm values. The escaping is correct and tested, but a typed map[string]struct{ Endpoints []string }, which this chart already does for nodeGroups, would reject a malformed value at admission instead of failing late inside Talos, and would shrink the surface the escaping has to cover. Related: the field doc sells air-gapped environments, and Talos falls back to the upstream registry by default unless skipFallback is set, so a mirror on its own does not give an operator air-gap behaviour. One sentence in the description would save someone that discovery.

Smaller things. hack/talos-reconcile-heredoc_test.bats:4 cites the review round in a tracked file; the header reads better describing what the test pins than where it came from. The mirrors block is duplicated verbatim across both charts while tests/render-parity.sh covers only the pool objects, so the copies can drift silently, though the new bats does currently exercise both. The PR body still opens with "Draft" while the PR is marked ready, with no downstream box ticked. And the API-owner gate is red because the generator marks the new field required in both cozyrds, which is the usual shape for a defaulted field here (imageFactoryURL, installerRepository, schematicID and version are already required), but it still needs an owner approval before this can merge.

What I ran on the branch: make generate plus per-chart make generate (no drift), helm unittest on both charts (193 and 11 passing), tests/render-parity.sh (byte-identical), the three bats files under hack/cozytest.sh (green), shellcheck and bash -n on the changed shell (the new helper is clean; the rest predates the branch), a default-values render diff of the reconcile Job against main, a mutation of the escaping to confirm the tests fail on it, and the dash reproduction above. On the overlap with the other open change to run-kubernetes.sh: git merge-tree is clean, so whichever lands second only needs to re-read the top of the file.

Aleksei Sviridkin (lexfrei) added a commit that referenced this pull request Aug 6, 2026
…#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 -->
@IvanHunters
IvanHunters force-pushed the fix/e2e-tenant-worker-registry-mirror branch from 31c5728 to 6bfdfba Compare August 7, 2026 07:42
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@IvanHunters
IvanHunters force-pushed the fix/e2e-tenant-worker-registry-mirror branch from 6bfdfba to 7119db2 Compare August 7, 2026 09:56
Aleksei Sviridkin (lexfrei) added a commit that referenced this pull request Aug 7, 2026
…ostics_test.bats (#3602)

## What this PR does

`hack/run-kubernetes-talos-diagnostics_test.bats` landed on main
carrying eight EXIT traps and was not added to the frozen set in
`hack/cozyreport.bats`, so `found` and `frozen` differ by exactly that
one entry and the guard fails on every branch cut from main since then.

This adds the entry. That is what the comment above the guard prescribes
for a file that did not exist when the freeze was written: the counts
are expected to move, and the file is updated rather than converted,
because it is owned by another branch. The guard behaved correctly here,
it noticed.

## Why it is worth fixing on its own rather than waiting

The red check is the smallest part of the cost.

`hack/cozytest.sh` stops at the first failing test in a file, and this
guard is test 83 of 166 in `cozyreport.bats`, so the remaining 83 never
run. The Makefile loop is `for f in $(BATS_UNIT_FILES); do
hack/cozytest.sh "$f" || exit 1; done`, which aborts the whole loop
rather than the single file, and `cozyreport.bats` sorts fourteenth of
forty-two, so twenty-eight further files never run either. `finalize`
needs `checks`, and the e2e job needs `finalize`, so `E2E Tests` ends up
skipped rather than executed, and a skipped required context satisfies
branch protection.

The practical effect is that a PR can be green enough to merge while
neither its new unit tests nor e2e have run.

## Verification

`bats` walks past a failure where `cozytest.sh` stops, so running the
file under `bats` shows what is behind the cut. With this entry present,
`cozyreport.bats` is 166 of 166.

I also ran the twenty-eight files that sort after it: 368 tests pass and
15 fail, all 15 confined to `migration-seaweedfs-db-adopt.bats` and
`seaweedfs-naming-audit.bats`, both for local reasons on macOS (a docker
bind mount that does not resolve, and BSD versus GNU column formatting).
Both are pre-existing on main and unrelated to this change, so nothing
has rotted behind the cut while it was unreachable.

## Note on overlap

`#3575` carries the same one-line change as part of a larger feature.
This PR exists so that main can be unblocked without waiting for that
one, and the two will resolve to the same line.

### Downstream repositories

Walked the trigger map against the diff. This changes one string inside
a bats guard that exists only in this repository, so nothing downstream
sees it.

- [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
```

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOT LGTM — both blockers from the previous round are genuinely closed, but the four commits added on 2026-08-07 put diagnostic scaffolding into two shipped chart templates, and one of them renames the content-hash KubevirtMachineTemplate, which rolls every live tenant worker VM on upgrade.

Previous round, verified against the code rather than the replies. The dash-errexit blocker is fixed in 9eabafb9 with exactly the if out=$(...); then rc=0; else rc=$?; fi shape, and the comment above it now records why a bare assignment survives locally but not under the suite's shell. The unpinned Docker Hub pull is fixed in 02e93fec: mirror.gcr.io/library/registry:2.8.3@sha256:a3d8aaa6..., matching what the other helper manifests in hack/ already do, plus ephemeral-storage requests/limits and sizeLimit on both emptyDirs. Both are closed.

Blockers

1. logSerialConsole: true is emitted unconditionally, which renames the content-hash KubevirtMachineTemplate and rolls every live worker VM.

File: packages/apps/kubernetes/templates/cluster.yaml:157, packages/apps/kubernetes-nodes/templates/nodegroup.yaml:135.

Issue: the key is rendered for every node group, not behind a value, so it changes the rendered body of a template whose name is a hash of that body.

Evidence: rendering packages/apps/kubernetes with tests/values/common.yaml gives test-k8s-md0-291d4b on this branch and test-k8s-md0-fdbcb9 at the merge base. The PR writes the same fact down itself: tests/cluster_test.yaml updates its two pins from fdbcb9 to 291d4b and from 4a2527 to 1b2627.

Impact: on upgrade every existing tenant worker MachineTemplate is renamed, so CAPI replaces every running worker VM across every tenant cluster. The change is carried under a comment reading "Revert before merge", so the rollout is not something the PR intends to ship.

Fix: drop both commits from the branch. If the diagnostic is wanted as a product feature, it needs to be opt-in and rendered only when true, so a group that did not ask for it renders the same nothing as an absent field.

2. Four commits marked TEMP, three of them saying "Revert before merge", on a PR that is marked ready.

File: commits 9fc79034, 85b88343, 6e740d41, 95da7ac6.

Issue: the branch is being used as a CI experiment vehicle while presenting as a merge candidate.

Evidence: the four commit subjects all carry TEMP; the in-tree comments say "Revert before merge" and "Revert together with the chart logSerialConsole change". The PR body describes none of them, and there is no comment on the thread explaining the state.

Impact: a maintainer reading the description sees a scoped chart passthrough plus an e2e mirror, and gets the diagnostics as well. The body's claim that "the render (and the content-hash reconcile-Job name) is byte-identical unless a mirror is set" is no longer true of the branch it describes.

Fix: drop the four commits, or mark the PR draft and say on the thread that the head is an experiment.

3. The e2e tenant worker is given fixed CPU/memory to test a different hypothesis.

File: hack/e2e-chainsaw/_lib/run-kubernetes.sh:788-790.

Issue: resources: {} becomes cpu: "4", memory: 4Gi, which overrides the u1.medium instancetype for the suite's own tenant.

Evidence: the commit message states the intent, testing whether relieving the CPU cap fixes node-join, and notes the worker boots fine on an uncontended cluster.

Impact: if merged, the suite permanently stops exercising the instancetype path it is supposed to cover, raises the sandbox's CPU footprint, and masks the starvation condition being investigated. It also concedes that the mirror alone has not greened node-join, which the PR body does not reflect.

Fix: keep the experiment on a scratch branch and land the mirror change on its own.

Non-blocking

7119db2d duplicates a freeze bump that already merged on 2026-08-07, so origin/main already carries run-kubernetes-talos-diagnostics_test.bats=8; a rebase drops the commit. Worth being explicit that this is not new debt from this branch: the file already held 8 EXIT traps at the merge base and main's frozen list simply did not know about it, so the guard was red before this PR existed. The bats files this PR adds hold zero traps, and the one new case in the diagnostics file says so on purpose; that is the right call.

Merge order against the per-file EXIT-trap ratchet change: git merge-tree reports a content conflict in hack/cozyreport.bats, since that change replaces the single frozen string this PR edits with per-file declarations. Whichever lands second re-resolves one line. If this PR rebases past the already-merged freeze, the conflict disappears with it.

Merge order against the serial-console capture branch: git merge-tree reports content conflicts in both cozyrds files, and the two changes implement the same capability. That branch adds it as a per-group opt-in with the content-hash hazard called out in the template comment; this PR hardcodes it. Only one of the two should carry it, and the opt-in shape is the one that does not roll live VMs.

The API-owner gate is still red, unchanged from the previous round, and still needs an owner approval before this can merge.

On the underlying problem: the mirror does take the worker's kubelet pull off live public egress, which is a real dependency to remove regardless of whether it turns out to be the dominant failure mode, and the in-guest TLS-handshake-timeout evidence in the description supports it. That case stands on its own once the diagnostics are off the branch.

What I ran on the branch head: bats and the repository's bats runner on hack/ghcr-mirror_test.bats (9 passing), hack/talos-reconcile-heredoc_test.bats (3 passing) and hack/run-kubernetes-talos-diagnostics_test.bats (11 passing), all green under both; the EXIT-trap guard at the branch head (green) and again with the merge-base frozen list against the same tree (red, which is what established the debt predates this branch); a helm template render of packages/apps/kubernetes at the branch head and at the merge base to compare KubevirtMachineTemplate names; and git merge-tree against origin/main (clean) and against both in-flight branches.

@IvanHunters

Copy link
Copy Markdown
Collaborator Author

Thanks, this was an accurate read: those four commits were a CI diagnostic experiment that should not have been sitting on a ready PR. All three blockers are addressed.

Blockers 1 and 2 (logSerialConsole in shipped templates, TEMP commits): dropped. The logSerialConsole additions to cluster.yaml and nodegroup.yaml, the serial-console capture in run-kubernetes.sh, and the 4-vCPU experiment are all reverted (no force-push, so they appear as revert commits; the net tree is back to the registry-mirror change only, and a squash-merge collapses the pair). The content-hash pins in cluster_test.yaml are back to fdbcb9 / 4a2527, helm template of packages/apps/kubernetes matches the merge base again, and render-parity.sh is byte-identical across both charts. So the PR-body claim holds once more: with talos.registryMirrors unset the rendered worker reconcile Job is byte-identical to main.

Blocker 3 (fixed CPU on the e2e tenant): reverted; the suite exercises the u1.medium instancetype path again.

The diagnostics did their job before coming off the branch: the serial console they captured pinned the root cause of the node-join failures, and it is not this chart. The worker VM starves for host CPU under the sandbox's nested-virtualization oversubscription and RCU-stalls in early kernel boot, never reaching Talos userland. Details and console evidence are in #3513. Giving the guest more vCPU made it worse, and dedicatedCpuPlacement is not usable here because the sandbox nodes report cpumanager: false. The fix belongs to the CI runner sizing, not the chart. The opt-in serial-console shape you mentioned on the other branch is the right home for that capability; I am not duplicating it here.

So this PR is now just the registry-mirror passthrough plus the e2e ghcr-mirror, which as you noted stands on its own: it takes the worker kubelet pull off live public egress. Scoped to that, Refs #3513 for the egress variant rather than claiming to close it.

Non-blocking: agreed on the rebase; 7119db2d drops once the branch rebases past the already-merged freeze, which also clears the cozyreport.bats merge-order conflict. The API-owner gate still needs an owner approval.

@IvanHunters
IvanHunters force-pushed the fix/e2e-tenant-worker-registry-mirror branch from 4be75ed to a1680fa Compare August 8, 2026 16:46
Aleksei Sviridkin (lexfrei) added a commit that referenced this pull request Aug 8, 2026
…#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 -->
@lexfrei
Aleksei Sviridkin (lexfrei) force-pushed the fix/e2e-tenant-worker-registry-mirror branch from a1680fa to 8f9ad78 Compare August 8, 2026 22:55
@github-actions github-actions Bot added size/XXL This PR changes 1000+ lines, ignoring generated files and removed size/XL This PR changes 500-999 lines, ignoring generated files labels Aug 8, 2026
@lexfrei
Aleksei Sviridkin (lexfrei) force-pushed the fix/e2e-tenant-worker-registry-mirror branch 2 times, most recently from 0357d34 to c1f97d0 Compare August 9, 2026 10:09
@lexfrei
Aleksei Sviridkin (lexfrei) force-pushed the fix/e2e-tenant-worker-registry-mirror branch from c1f97d0 to e34e54f Compare August 9, 2026 10:17
…mirror

Add a talos.registryMirrors passthrough to the kubernetes and kubernetes-nodes
charts that renders into the worker TalosConfigTemplate machine.registries.mirrors,
so an operator can point a registry such as ghcr.io at an in-cluster pull-through
mirror and a worker boot no longer depends on live public egress. Default {}
keeps the rendered worker reconcile Job byte-identical to before, content-hash
Job name included. Free-form mirror values are shell-escaped for the unquoted
apply heredoc, covered by an execution-level bats test.

Also add the e2e ghcr-mirror (registry:2 pull-through plus a Cilium egress
policy) and wire it into the kubernetes chainsaw suites, so CI exercises the
mirror path and takes the flaky public ghcr.io kubelet-image pull off live
egress.

Refs #3513 (addresses the egress variant of the tenant-worker node-join
failures; the dominant variant is CI-runner CPU starvation, tracked in the
issue).

Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
@lexfrei
Aleksei Sviridkin (lexfrei) force-pushed the fix/e2e-tenant-worker-registry-mirror branch from 990bf92 to f2a09fb Compare August 9, 2026 11:16
The worker TalosConfigTemplate is applied from an unquoted
`cat <<EOF | kubectl apply` heredoc, so every free-form value
interpolated into it is subject to shell parameter expansion and
command substitution at Job runtime. Escape backslash, dollar and
backtick on talosVersion, installerRepository, schematicID and the
registryMirrors block so each renders as a literal, and record the
invariant above the data block in both charts: a new free-form field
here must be escaped or render-time pattern-validated, or it reopens
command injection into a host-cluster Pod. Escaping covers every byte
by construction; the chain is a no-op for ordinary values, so the
default render and its content-hash Job name are unchanged.

Tests render both charts and run the heredoc through a real shell with
hostile values, which a rendered-string regex cannot do, and the
default-omits-mirrors assertion selects the Job by kind rather than by
document index so it cannot pass against the wrong document.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
resolve_ghcr_mirror_endpoint decides once, for the whole shared
sandbox, whether tenant workers are pointed at the in-sandbox mirror,
so a misclassified transient failure would disable it for the rest of
the run. Cache only stable answers: a genuine API NotFound and a
definite Available=False; never a transient query error, a missing
kubectl, or a failed egress-allow apply. Read readiness from the
Deployment's own Available condition when the rollout watch exits
non-zero, and commit the endpoint only once the egress allow is in
place, surfacing the apply failure reason rather than a generic blip.

The node-join diagnostic counts kubelet-image requests across the whole
access log rather than a fixed tail, and every read it makes is
wall-clock bounded by the caller's COZY_DIAG_READ_TIMEOUT so no single
read can hold the failure path open. The bats suite exercises every
resolver branch and the diagnostic's filtering, with a kubectl stub
that reports errors on stderr as the real binary does and a timeout
mock so the bounded reads stay reachable under test.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
…in flow

Deploy the pull-through mirror from its own install step rather than
folding it into the Talos image-cache step, whose early return on a
missing values key would otherwise skip it silently. Build the tenant
CR spec.talos block from both the OS image cache and the registry
mirror, each included only when its in-sandbox backend is up. On a
node-join failure dump the mirror's state alongside the Talos image
cache, under the same phase gate as its neighbours so it cannot starve
the guest captures of budget. Record the mirror's legs in the step's
budget inventory.

talos_spec_block is covered by a bats suite over all four mirror-up and
mirror-down combinations, splicing the result under spec: and reading
it back with yq so a block indented into the wrong parent fails, and
the install step is pinned to exclude the Cilium policy from its
pre-Cilium apply.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
State in the README that in-guest registry pulls are routable again via
talos.registryMirrors, which knob covers which artifact (the mirror
routes registry pulls; the worker OS disk image arrives as a raw HTTP
artifact under talos.imageFactoryURL and no registry mirror touches
it), and the limits: skipFallback is needed for air-gap,
machine.registries.config is not passed through, and existing nodes
keep their boot-time config until replaced. Trim the field doc comment.
The regenerated schema, README tables and cozyrds openAPISchema follow
from those sources.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
@lexfrei
Aleksei Sviridkin (lexfrei) force-pushed the fix/e2e-tenant-worker-registry-mirror branch from 2ed086a to 3ae1e10 Compare August 9, 2026 13:18
@lexfrei
Aleksei Sviridkin (lexfrei) merged commit 677d80a into main Aug 9, 2026
39 of 42 checks passed
@lexfrei
Aleksei Sviridkin (lexfrei) deleted the fix/e2e-tenant-worker-registry-mirror branch August 9, 2026 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/kubernetes Issues or PRs related to the tenant Kubernetes app kind/feature Categorizes issue or PR as related to a new feature size/XXL This PR changes 1000+ lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants