feat(kubernetes): route tenant worker image pulls through a registry mirror - #3575
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds ChangesRegistry mirror support
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
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
There was a problem hiding this comment.
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
📒 Files selected for processing (22)
api/apps/v1alpha1/kubernetes/types.goapi/apps/v1alpha1/kubernetes/zz_generated.deepcopy.goapi/apps/v1alpha1/kubernetesnodes/types.goapi/apps/v1alpha1/kubernetesnodes/zz_generated.deepcopy.gohack/e2e-chainsaw/_lib/ghcr-mirror.shhack/e2e-chainsaw/_lib/run-kubernetes.shhack/e2e-ghcr-mirror.yamlhack/e2e-install-cozystack.batshack/ghcr-mirror_test.batshack/talos-reconcile-heredoc_test.batspackages/apps/kubernetes-nodes/README.mdpackages/apps/kubernetes-nodes/templates/talos-reconcile-job.yamlpackages/apps/kubernetes-nodes/tests/registry_mirrors_test.yamlpackages/apps/kubernetes-nodes/values.schema.jsonpackages/apps/kubernetes-nodes/values.yamlpackages/apps/kubernetes/README.mdpackages/apps/kubernetes/templates/talos/talos-reconcile-job.yamlpackages/apps/kubernetes/tests/talos_templates_test.yamlpackages/apps/kubernetes/values.schema.jsonpackages/apps/kubernetes/values.yamlpackages/system/kubernetes-nodes-rd/cozyrds/kubernetes-nodes.yamlpackages/system/kubernetes-rd/cozyrds/kubernetes.yaml
There was a problem hiding this comment.
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
📒 Files selected for processing (2)
hack/e2e-ghcr-mirror.yamlhack/ghcr-mirror_test.bats
🚧 Files skipped from review as they are similar to previous changes (1)
- hack/ghcr-mirror_test.bats
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
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.
…#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 -->
31c5728 to
6bfdfba
Compare
|
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. |
6bfdfba to
7119db2
Compare
…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 ```
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
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.
|
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 Blocker 3 (fixed CPU on the e2e tenant): reverted; the suite exercises the 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 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; |
4be75ed to
a1680fa
Compare
…#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 -->
a1680fa to
8f9ad78
Compare
0357d34 to
c1f97d0
Compare
c1f97d0 to
e34e54f
Compare
…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>
990bf92 to
f2a09fb
Compare
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>
2ed086a to
3ae1e10
Compare
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-previoussuites fail withnode-join failed: fewer than 2 tenant nodes Ready within 18mand 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 pullghcr.io/siderolabs/kubeletfrom public ghcr.io and fails withHead "https://ghcr.io/v2/siderolabs/kubelet/manifests/...": net/http: TLS handshake timeout host=ghcr.io. The Taloskubeletservice never starts (kubelet was not registered), so the node never registers, cilium-operator staysPendingfor 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 thetalos.imageFactoryURLcache.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), atalos.registryMirrorspassthrough, a map of upstream registry host to{ endpoints: [ ... ] }, rendered intomachine.registries.mirrorsof the workerTalosConfigTemplatethat 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 themachine.registries.mirrorsknob the Talos rollover left unfilled. Talos still falls back to the upstream registry unless a host also setsskipFallback, 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.schematicIDand 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:2pull-through cache for ghcr.io (hack/e2e-ghcr-mirror.yaml), applied at install time from its own step, withhack/e2e-chainsaw/_lib/ghcr-mirror.shsettingspec.talos.registryMirrorson the tenant CR when the mirror is up and its egress allow is in place, and emitting nothing otherwise so workers pull directly. ACiliumClusterwideNetworkPolicyopens one tightly-scoped hole for worker VM (kubevirt.io: virt-launcher) Pods intenant-testto reach the mirror, mirroring the existingtalos-image-cachepolicy.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 statusis the cheap way to learn it, but it also exits non-zero on a broken watch, so when it fails the Deployment's ownAvailablecondition is read rather than the watch's exit code taken as the answer. Exactly one failure is cached, a definiteAvailable=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 omitsmachine.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 ofresolve_ghcr_mirror_endpointagainst a stubbedkubectl: 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 definiteAvailable=Falsedoes, 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. AmatchRegexcannot catch a heredoc the shell refuses to emit; this runs it.hack/run-kubernetes-talos-spec_test.bats, 5 tests overtalos_spec_block, the point where the two optional fragments are merged. The assertions splice the result underspec: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.shbyte-identical; per-chartmake generatein both charts with no drift; bothcozyrdsopenAPISchemablocks equal to theirvalues.schema.json.mirror.gcr.io/library/registry:2.8.3@sha256:a3d8aaa6...is the digest Docker Hub serves for that tag, andmirror.gcr.ioserves 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 servesghcr.io/siderolabs/kubeletmanifests and blobs as an anonymous pull-through.shellcheckandgit diff --checkclean 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:
machine.registries.mirrors[ghcr.io].endpoints=[http://...]. If a scheme or TLS knob is additionally needed,machine.registries.confighas to be added too.run-kubernetes.shalready reads tenant worker Pods bykubevirt.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 statusplus 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.shruns 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.registryMirrorscould be a typedmap[string]struct{ endpoints []string }, the waynodeGroupsalready 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
includetemplate) 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.mirrorsis passed through.machine.registries.configis not, so a mirror behind a private CA or one needing credentials still has no knob, and the platform-wideregistriesvalue 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 undertalos.imageFactoryURLand 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=-1to count kubelet-image requests across the whole log and once with--tail=50 --prefixfor 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_diagnosenow 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 sameCOZY_DIAG_READ_TIMEOUT/COZY_DIAG_READ_GRACEthe 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-previousselects the previous Kubernetes minor out ofpackages/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.mdagainst 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'svalues.schema.json" and "change a default in an app'svalues.yaml" as triggers for its schema, model and expand/flatten pair. This addstalos.registryMirrorswith a{}default to both thekubernetesandkubernetes-nodesschemas. No other repository matches a trigger. The website's managed-app reference pages regenerate from each package'sREADME.mdon 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