Repository navigation
feat(multus)!: stage reference CNI plugins from the multus image - #3195
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 Multus CNI image now bundles verified reference CNI plugins. Platform bundles configure when staging runs. A DaemonSet init container installs plugins on hosts with atomic replacement and failure reporting. Tests and documentation cover the new behavior. ChangesReference CNI plugin staging and installation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PlatformValues
participant MultusDaemonSet
participant MultusImage
participant InstallCniPlugins
participant HostCniDirectory
PlatformValues->>MultusDaemonSet: Set validated stageCniPlugins
MultusImage->>InstallCniPlugins: Provide /cni-plugins
MultusDaemonSet->>InstallCniPlugins: Run conditional init container
InstallCniPlugins->>HostCniDirectory: Atomically publish staged plugins
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Multus CNI deployment by ensuring essential reference plugins are available on the host system. By bundling these plugins directly into the Multus image and deploying an init container to stage them into the host's CNI binary directory, the change resolves runtime failures where NetworkAttachmentDefinitions would otherwise fail due to missing dependencies in environments like k3s. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request packages and stages upstream reference CNI plugins (such as bridge, macvlan, etc.) into the Multus-CNI image and adds an init container to copy them to the host's /opt/cni/bin directory. This ensures that Multus-backed NetworkAttachmentDefinitions function correctly on distributions like k3s. The review feedback highlights that using cp -f can cause Text file busy errors if the binaries are in use, suggesting the use of install instead. Additionally, it recommends dropping privileged: true on the init container in favor of a more restricted security context, and updating the corresponding test assertions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| command: | ||
| - "sh" | ||
| - "-c" | ||
| - "cp -f /cni-plugins/* /host/opt/cni/bin/" |
There was a problem hiding this comment.
Using cp -f to copy binaries to /host/opt/cni/bin/ can fail with a Text file busy (ETXTBSY) error if any of the CNI plugins (like bridge or loopback) are currently being executed or held open by the container runtime or another process on the host.
To prevent this, use the install utility instead of cp. The install command unlinks the destination file before copying, which avoids the Text file busy error and allows running processes to continue using the old inode safely.
Since the image is based on debian:stable-slim, install is guaranteed to be available.
command:
- "sh"
- "-c"
- "install -t /host/opt/cni/bin/ /cni-plugins/*"| securityContext: | ||
| privileged: true |
There was a problem hiding this comment.
The install-cni-plugins container does not require privileged: true to copy files to the hostPath volume. Running with full privileges is a security risk.
Instead, you can run without privileged: true and apply a more restricted security context. Since it only needs to write to the hostPath volume (which requires root permissions if /opt/cni/bin is owned by root), you can drop all capabilities and prevent privilege escalation.
securityContext:
allowPrivilegeEscalation: false
capabilities:
drop:
- ALL
readOnlyRootFilesystem: trueReferences
- Flag overly broad RBAC, missing securityContext, containers running as root without an explicit reason, and hostPath/hostNetwork usage without clear rationale. (link)
| path: spec.template.spec.initContainers[1].command | ||
| value: | ||
| - "sh" | ||
| - "-c" | ||
| - "cp -f /cni-plugins/* /host/opt/cni/bin/" | ||
| - equal: |
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM with non-blocking notes
Vendored-chart discipline is correct (patches/customize-deployment.patch +
make update, make image rewrites all three image refs), unit test is green,
Phase 5b (upgrade = rolling DaemonSet, fresh install = digest-pinned image) is fine.
Two non-blocking notes:
[MINOR] Unconditional cp -f silently overwrites host CNI plugins
packages/system/multus/templates/multus-daemonset-thick.yml:75-91
cp -f /cni-plugins/* /host/opt/cni/bin/ runs on every (re)start. If k3s or another
actor already placed a differently-versioned bridge/portmap/loopback there, it
is silently overwritten by the containernetworking v1.9.1 build (last-writer-wins).
Plugins are ABI-stable so risk is low, but the overwrite is silent and repeats.
Consider cp -n, or document that last-writer-wins is intentional.
[MINOR] CNI plugins tarball fetched without checksum
packages/system/multus/images/multus-cni/Dockerfile:20-24
The tarball is pulled via curl | tar -xz with no sha256 verification, baking an
unverified third-party binary into a privileged host-writing image. Not a regression
(the existing multus download is the same), but consider pinning the tarball digest.
## What this PR does Adds `docs/vm-external-vlan.md`, a guide for attaching a `vm-instance` VM directly to an external, physically-routed VLAN via a Linux bridge and a `bridge`-type NetworkAttachmentDefinition. The documented VM networking story is overlay-only (pod network + KubeOVN VPC subnets). Attaching a VM to a real VLAN — so it shares a broadcast domain with external hardware and takes an address from that VLAN's subnet — is undocumented, and the obvious `macvlan` approach silently fails with KubeVirt's default bridge binding: macvlan demultiplexes ingress by the child interface's MAC while bridge binding puts the guest's own MAC on the wire, so gateway replies reach the parent interface and are dropped before the guest. The working recipe is a Linux bridge + `bridge` CNI NAD. The guide covers why bridge and not macvlan, the host bridge, the NAD (one per tenant namespace), assigning a static guest address through cloud-init (the chart has no `networkData`), the always-present dual-homed pod NIC, and the post-recreate ARP staleness from KubeVirt regenerating the guest MAC. Docs-only; no chart or manifest changes. The recipe depends on the `bridge` CNI plugin being present in `/opt/cni/bin`; #3195 makes Cozystack's multus package stage the reference plugins there. ### Screenshots N/A — no UI changes. ### Release note ```release-note docs(vm-instance): add a guide for attaching a VM to an external VLAN via a Linux bridge and a bridge-type NetworkAttachmentDefinition ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added a new guide for connecting a `vm-instance` directly to an externally routed VLAN. * Includes setup steps for creating a bridged VLAN interface, defining a network attachment, and configuring the VM with a static IP. * Notes important networking caveats such as dual-homing behavior, MAC changes after recreation, and host-to-VM connectivity requirements. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
myasnikovdaniil
left a comment
There was a problem hiding this comment.
LGTM overall — approve-level with a few non-blocking notes below. The motivation (bridge-type NADs failing on nodes that don't populate /opt/cni/bin, e.g. k3s) is real and well-documented, and the implementation follows the repo's conventions correctly: the manifest change lives in patches/customize-deployment.patch so it survives make update, and there's a focused helm-unittest pinning the container name/command/mount and the image repository.
I verified the parts that looked risky:
- The reused
@sha256:e006fe…digest (the same oneinstall-multus-binarycarries) is the pre-existing, plugin-less image — but that's a build-time placeholder, not what ships.make imagere-stamps everymultus-cniref viased "s|image: .*multus-cni.*|…@$DIGEST|g", so the PR build (fragment →finalize→ installer OCI artifact consumed by e2e) and the release build (make build→Prepare releasecommit) both carry the freshly-built, plugin-carrying digest. The committed digest never reaches a cluster as-is. Only a hypothetical promote-without-rebuild that reused a pre-this-change digest could regress, which isn't the current flow. - e2e gives real coverage: cozystack always installs multus, so the
install-cni-pluginsinit container actually runscp -f /cni-plugins/*against the freshly-built image during e2e — a crashloop (empty/missing/cni-plugins) would fail the install, not just the unittest. - cni-plugins v1.9.1 exists and the asset names match the URL template exactly for both
amd64andarm64; all 15 extracted members are present in the tarball (excludeddhcp/dummy/tapare a deliberate curation). A typo'd member name would failtarloudly at build.
Nothing blocking. Say the word if you'd like me to convert this to a formal Approve.
| ARG TARGETARCH | ||
| ARG CNI_PLUGINS_VERSION=v1.9.1 | ||
| RUN mkdir -p /cni-plugins && \ | ||
| curl --fail --silent --show-error --location "https://github.com/containernetworking/plugins/releases/download/${CNI_PLUGINS_VERSION}/cni-plugins-linux-${TARGETARCH}-${CNI_PLUGINS_VERSION}.tgz" | \ |
There was a problem hiding this comment.
Nit (non-blocking): the tarball is pinned by version tag but not checksum/digest-verified. This is consistent with the multus source fetch above (curl -sSL … | tar at line 9), so it's not a regression — just noting it for supply-chain hardening if you ever want to add a sha256sum -c. --fail correctly turns a 404 / error page into a loud build failure instead of feeding garbage into tar, which is the important part here. 👍
| RUN mkdir -p /cni-plugins && \ | ||
| curl --fail --silent --show-error --location "https://github.com/containernetworking/plugins/releases/download/${CNI_PLUGINS_VERSION}/cni-plugins-linux-${TARGETARCH}-${CNI_PLUGINS_VERSION}.tgz" | \ | ||
| tar -xz -C /cni-plugins \ | ||
| ./bridge ./host-local ./loopback ./static ./macvlan ./ipvlan ./vlan \ |
There was a problem hiding this comment.
Heads-up on loopback: it's bundled here and later cp -f'd unconditionally, and Cilium also installs its own loopback. Identical today (both are the canonical reference build), but the init container re-runs on every multus pod (re)start, so it will silently pin the node's loopback to reference v1.9.1 even if Cilium later ships a different one. Low risk given how stable loopback is — flagging the version coupling, no change required.
| command: | ||
| - "sh" | ||
| - "-c" | ||
| - "cp -f /cni-plugins/* /host/opt/cni/bin/" |
There was a problem hiding this comment.
The glob makes this all-or-nothing: if /cni-plugins is ever absent/empty (a stale image, or a future make update regenerating this container onto the upstream image), the glob stays literal, cp exits non-zero, and the init container crashloops — blocking the whole multus pod, not just skipping plugin install. That's arguably the right (loud) behavior given the build always re-stamps to a plugin-carrying digest and e2e exercises it, and the unittest guards the image repo. Just recording the coupling: correctness here depends on the build re-stamping the digest — worth keeping in mind if the release path ever moves toward promote-without-rebuild. No change requested.
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM. Reference CNI plugins are baked into the digest-pinned image and staged via an init container mirroring the existing install-multus-binary pattern; the manifest change lives in patches/customize-deployment.patch so it survives make update, and the helm-unittest guard catches both patch drift and image reset. Verified upgrade (rolling DaemonSet restart only, primary CNI untouched) and fresh install (no new bundle/RBAC/PackageSource).
Non-blocking follow-ups: (1) Dockerfile fetches the cni-plugins tarball without sha256 verification (build-time/CI only; runtime is digest-pinned); (2) the init container cp -fs all plugins into /opt/cni/bin on every restart, so on managed-k8s this could overwrite a provider's newer portmap/bandwidth (tradeoff acknowledged in-code); (3) the test asserts on positional initContainers[1].
7bbdf8c to
44801b4
Compare
44801b4 to
0de4de0
Compare
IvanHunters
left a comment
There was a problem hiding this comment.
Reviewed hermetically. LGTM with non-blocking notes.
Verified:
- The
cni-plugins v1.9.1SHA-256 values in the Dockerfile match the upstream.sha256sidecars. - Atomic rename staging;
helm unittest6/6, dropping to 5/6 under a mutation of the atomic-rename step (non-vacuous); the bats suite (7 tests) passes; shellcheck clean.
Non-blocking:
- PR body omits the mandatory Downstream Repositories checklist; the trigger map shows no downstream repo affected, but it should be stated.
- The
install-cni-pluginsinit container is a second non-toggleable sequential startup dependency; a persistent failure on a node blockskube-multusfrom starting, the same class as the existinginstall-multus-binary. - The init-container image is digest-pinned but not via
cozy-lib.image; not new debt, the line matches sibling containers in the same vendored file. - CI E2E is red for an unrelated reason: a stale fixture references an already-removed API group; the build and unit/controller jobs are green.
0de4de0 to
6be23f9
Compare
Multus-backed NetworkAttachmentDefinitions delegate to the upstream containernetworking reference plugins (bridge, macvlan, ipvlan, ...), and nothing guarantees a node has them in the directory Multus searches. On k3s they live under /var/lib/rancher/k3s/data/cni while Multus and Cilium install into /opt/cni/bin, so a bridge-type NAD fails there with `failed to find plugin "bridge" in path [/opt/cni/bin]`. Bundle 14 of the release's plugins into the multus image and add an install-cni-plugins init container that installs them into the host /opt/cni/bin, mirroring the existing install-multus-binary container. loopback, dhcp, tap and dummy are left out; the Dockerfile records why. Gate it on a new chart value, stageCniPlugins, resolved per bundle and on for both. Talos bundles only bridge, firewall, flannel, host-local, loopback and portmap, so the other ten are missing there exactly as on generic Linux; the four that overlap are rewritten with the same upstream version, v1.9.1 on both sides, rather than an older one. A marker in the image Dockerfile records the Talos release that pin was compared against, and a test holds it to the release the platform ships. multus therefore moves out of system.common-packages into the two variant branches, the way linstor already does for talos.enabled. networking.stageCniPlugins overrides the variant default in either direction. Where staging is on the install is unconditional: a plugin is replaced on every pod recreation, not only when the destination is missing. A destination that is already a directory is reported and left alone, since replacing it is not something this can do safely. Turning the value off afterwards does not undo it; nothing here removes or restores a plugin. The container is not privileged and takes no mount propagation. It runs in the spc_t SELinux domain, without which an enforcing node refuses the write. Each plugin is installed under a temporary name and swapped in with a rename: /opt/cni/bin is shared with cilium and kube-ovn. The plugin tarball is checksummed before extraction. A plugin that cannot be installed is reported on stderr and to /dev/termination-log, and the container still exits 0: Multus stays the node's primary CNI and nothing removes its config, so an init container that never completes would leave the shim with no daemon behind it. The manifest change lives in customize-deployment.patch so it survives make update. BREAKING CHANGE: enabling stageCniPlugins replaces reference CNI plugins in /opt/cni/bin on existing nodes, Talos included -- four of the names it writes are ones the Talos node image also carries. Opt out before upgrading: declining afterwards does not restore what was replaced. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
The check that multus is pointed at the cilium conflist matched its JSON fragment anywhere in the rendered daemon config. The same text nested inside any other key satisfies it, so a config whose effective multusMasterCNI names a different conflist passes. Multus uses that value as written when it is non-empty -- it delegates to whatever is named, so a wrong-but-existing name silently routes through the other CNI instead of cilium, which is the state this assertion exists to prevent. Auto-discovery happens only when the value is empty, and a name that does not exist fails manager startup rather than falling back. Anchor it to the top level, where the template writes its keys. That pins the position rather than parsing the document; deciding which key is effective needs a JSON parser, which helm-unittest does not have. Signed-off-by: Aleksei Sviridkin <[email protected]>
6be23f9 to
cd57ee6
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. |
…ndbox Signed-off-by: Aleksei Sviridkin <[email protected]>
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM.
No confirmed CRITICAL/MAJOR defects and no regression on any supported path: every schema-valid corner of the toggle matrix renders correctly, the tests are non-vacuous and reach the dangerous branch, the patch still reproduces the committed template, and the feature is inert until the image is rebuilt. The breaking change is declared explicitly (! + BREAKING CHANGE + release note) with an opt-out and a migration path.
Two items to confirm before merge (not blocking review):
- Please confirm that siderolabs/pkgs on Talos v1.13.6 pins containernetworking/plugins v1.9.1. This is the safety premise for defaulting ON on Talos (the change rewrites bridge/firewall/host-local/portmap on every Talos node). The contract test only compares the
talos-cni-plugins-checked-againstmarker, not the actual plugin versions, so a future drift would silently change 4 system CNI binaries per node. - Minor claim mismatch in the PR body: for isp-full (Talos) the default is ON, not off — nothing is staged only because the pinned image carries no /cni-plugins yet, not because the value is off. The body's conclusion (no CI job stages plugins) is correct; the stated reason is not.
Non-blocking: of the flagged comment blocks, only the # block in multus-daemonset-thick.yml actually reaches the rendered manifest (the others are {{/* */}} / .tpl and strip out); it explains a non-obvious securityContext (SELinux spc_t), so it is borderline justified.
Recommended follow-up: run an end-to-end staging test on a generic-Linux node after the release rebuilds the multus image with /cni-plugins — the on-node staging path (SELinux write, atomic rename over a live path, copy-error delivery to kubectl describe) is the one surface covered by neither static analysis nor CI.
The ban on EXIT-trap cleanup in hack/*.bats was enforced by two lists inside hack/cozyreport.bats: one naming the files known to be clean, and one freezing an exact trap count for each file that was not. Both had to be edited from whatever change moved them, and that is where the guard failed, and it never worked even once. The inventory landed in #3567 at 15:25:20; #3195 had landed multus-install-cni-plugins.bats carrying twelve traps forty-three seconds earlier, so replaying the old guard against the tree at the commit that introduced it already gives found != frozen. Main stayed red for about twenty-two and three quarter hours. The first repair, #3584, landed already red the next morning because #3548 had brought in run-kubernetes-talos-diagnostics_test.bats with eight traps two minutes ahead of it, so that fix bought no green time at all; green came only with the second repair. Neither pair of PRs shared a line, and each was green against its own base. A change adding a trap to its own file had to edit a string in a suite it otherwise never touches, so two changes sharing no line still invalidated each other: each stayed green against its own base, git merged both cleanly, and the guard went red only once the second landed. A file arriving with traps hit the same wall from the other side -- the inventory did notice it, since the string it compared was built by scanning the directory, but absorbing it meant an edit in a file nothing in the author's diff pointed at. Replace both lists with a declaration each file makes about itself: one "# EXIT-TRAP DEBT: N" comment, exact rather than a ceiling. A file carrying none must install no EXIT trap, which covers the converted files, the files that never had one, and every file added later. Growing or shedding a trap now fails in the file the change already edits, so two changes that disagree about a count collide textually instead of silently. That collision is a steady-state property, and this change's own arrival is the exception. A branch forked before the declaration existed has no line to disagree with: it converts traps in its own file, merges clean, and the count goes wrong only once both sides are on main -- verified against a sibling branch that takes select-e2e_test.bats from fifteen traps to zero. What the move buys even then is that the red names a file that branch already edited, the repair is one line inside it, and rebasing before merge catches it on the branch's own CI. None of those three held against the inventory. The declaration is read from the leading comment block, not from anywhere in the file. A .bats file is shell that writes shell, so the same line turns up inside a heredoc, a fixture writer or an expected-output string, where it is data belonging to one test. Reading it there as a statement about the whole file would let an unrelated fixture excuse a real trap, and would do it silently, since nothing in that test's own diff looks like a declaration. A comment block is the region with no interior: stopping instead at the first @test would still read a line out of a helper's heredoc. Not every counted handler is debt. A trap inside an explicit subshell does not replace the bats binary's own, so a test failing inside `( ... )` still prints its `not ok`. hack/e2e-test-openapi.bats kills a backgrounded kubectl proxy that way, and moving the kill to the end of the body would leak a process holding a fixed port. Its declaration records that rather than scheduling a conversion, and because the ratchet is exact, removing the trap fails too -- the count protects the construct. What the count cannot do is tell the two apart: substituting a test-level trap for the subshell one keeps the total at 1 and stays green, which the header states rather than leaves to be found. Counting bounds the keyword and the signal the same way, at any character that cannot be part of an identifier, and matches the signal in either case. Whitespace on the right missed `trap ... EXIT; cd "$tmp"`; whitespace on the left missed `tmp=$(mktemp -d);trap ... EXIT` and `(trap ... EXIT; true)`; upper case missed `trap ... exit`, which bash and dash both install. All are real handlers that scored zero, and the left boundary matters most, since the inventory being replaced had none and caught the semicolon form. A bare word boundary is not enough either way: `bootstrap ` ends in `trap `, and it must keep scoring nothing. A quoted signal counts for the same reason as the rest. Two handlers sharing one line are reported rather than counted, since the count is a count of lines and the second would otherwise arrive without moving the total; splitting the line properly needs a shell parser, a semicolon inside a handler's own action not being a separator, and guessing wrong undercounts -- the one direction a ratchet cannot afford. The scan recurses, so hack/e2e-apps/*.bats is covered rather than sitting one directory below the guard that claims the tree. It reads .bats and nothing else, so a handler arriving through a sourced .sh stays outside it -- hack/e2e-chainsaw/_lib/run-kubernetes.sh installs two, each benign for its own reason rather than by design: one sits in a function declared with `(` and so runs in a subshell, the other in a brace function no @test calls. That boundary is stated in the header rather than papered over. The guard moves to hack/bats-no-exit-trap.bats: its subject is every unit suite under hack/, not the report collector it grew up in. Its fixture helpers assemble the trap keyword and the signal from separate arguments, because the guard scans its own source and a fixture written as a literal would be counted as a real trap in it. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
…#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 -->
What this PR does
Multus-backed NetworkAttachmentDefinitions delegate to the upstream containernetworking reference plugins (
bridge,macvlan,ipvlan, ...), and nothing guarantees a node has them where Multus looks. On k3s they live under/var/lib/rancher/k3s/data/cniwhile Multus and Cilium install into/opt/cni/bin, which is left without abridgebinary. Abridge-type NAD then fails withfailed to find plugin "bridge" in path [/opt/cni/bin], and today you work around it by copying the binary in by hand. #3196 documents one such pattern (attaching a VM to an external VLAN) that this unblocks.This bundles 14 of the reference plugins into the
multus-cniimage and adds aninstall-cni-pluginsinit container that installs them into the host/opt/cni/bin, mirroring the existinginstall-multus-binaryinit container.Where it applies, and how to turn it off
The new
networking.stageCniPluginsvalue defaults to on for both bundles. Talos puts six binaries in that directory --bridge,firewall,host-local,loopbackandportmapstripped from the same upstream release, plusflannel, which is a different project -- so the other ten are missing there exactly as on generic Linux and a NAD namingmacvlan,ipvlanorvlanfails on either platform. Four of those six are names this change stages;loopbackis deliberately not staged andflannelwas never in the reference set. The four names that overlap are rewritten on every Multus pod recreation with the same upstream version -- Talos pinscontainernetworking/pluginsv1.9.1insiderolabs/pkgs, and so does this image -- so it is not a downgrade of a system component. A marker in the image Dockerfile records the Talos release that pin was compared against, and a test holds it to the release the platform ships.isp-hostedships no multus at all and is unaffected. Both defaults can be overridden either way.If you run a generic-Linux cluster and manage
/opt/cni/binyourself, this changes behaviour on upgrade. Multus starts installing 14 plugins into that directory every time its pod is recreated, replacing files of the same name, so a hand-placed or locally patched plugin there will not survive. Setnetworking.stageCniPlugins: falseto decline, and set it before the upgrade that first enables staging: nothing here removes or restores a plugin, so declining afterwards protects what is still yours and does not give back what was already replaced. Changing the value edits the Multus DaemonSet's pod template, so Multus rolls node by node, and while a node's daemon is down pod creation and deletion on that node fail for the length of the restart. The four plugins the package never touches areloopback,dhcp,dummyandtap.Where staging runs it is unconditional, not only-when-absent, because nothing here ever revisits a plugin it has written. Installing only when absent would pin each node to whichever version arrived first, and a later
CNI_PLUGINS_VERSIONbump, including one fixing a binary that runs as root on every node, would reach zero existing nodes. The directory is not ours alone and the decision does not assume it is: kube-ovn copiesloopback,portmapandmacvlanthere from its own image, two of which this change also writes, node provisioning may place the set, and/opt/cni/binis also the CNI bin directory on RKE2. Overwriting is what delivering a version requires, andnetworking.stageCniPlugins: falseis how an operator who owns those files declines it.Notes
patches/customize-deployment.patchso it survivesmake update, and the release rewrite in that target runs after the patch rather than before it, so the line the patch adds lands on the same reference as the two it already had. The patch seeds that line with the upstream image reference while the committed template carries the pinned cozystack digest, because the package'simage:target rewrites everymultus-cniimage line to one digest. So it does not reverse-apply against the committed template. Thatmake updatereproduces the tree is verified forward from pristine upstream instead./opt/cni/binstays shared where staging is on. kube-ovn installsportmapandmacvlanthere and has been seen to do so with a plain copy onto the live path, which truncates in place. Each plugin is installed under a temporary name and swapped in with a rename, atomic within the mount, so the kubelet never execs a half-written binary. The Dockerfile records which kube-ovn release the version pin was checked against, andhack/cni-plugins-staging-contract.batsfails when that marker and the vendored release part ways. That check fires on the next kube-ovn bump and compares the marker with the vendored release; it does not compare the twoCNI_PLUGINS_VERSIONpins. Moving the marker is the acknowledgement that someone looked -- so read kube-ovn'sdist/images/Dockerfile.basebefore moving it, because a silent divergence there makesportmapandmacvlanflap between the two DaemonSets on every pod recreation.install-cni-binariescopies plugin binaries into the same directory on the same nodes.Bidirectionalpropagation governs mounts made inside a container, this script makes none, and asking for it is what would forceprivileged. The tests pin the absence.loopbackis excluded because replacing it is what a node cannot survive: the runtime calls it for every sandbox, and Cilium and kube-ovn already install it.dhcpneeds a host daemon this package does not run.tapanddummyhave no consumer here.portmapis included because a NAD conflist may chain it for hostPort.ARG CNI_PLUGINS_VERSION(v1.9.1) and the release tarball is verified against a per-arch SHA-256 before extraction. These are root-executed node binaries, so a tag alone is not enough. An unknown or emptyTARGETARCHfails the build./cni-plugins, rather than crashlooping the DaemonSet node-wide. The manifest and the image digest are re-pinned on different schedules, so a tree taken between a Dockerfile change and the next release bake can carry one without the other. A plugin it cannot copy is reported on stderr and skipped, for the same reason.multusConfigFile: automakes00-multus.confthe primary CNI config and there is nocleanupConfigOnExit, so on a node where Multus has already run, an init container that never completes leaves the shim's socket gone with that config still primary, and the node stops scheduling anything that goes through CNI; host-network pods still start. Failing on a copy error would therefore leave the node worse off than before this change, which only ever left it without one plugin. The cost of continuing is that the pod still reports Ready, so the summary is also written to/dev/termination-log, which the kubelet surfaces inkubectl describe podregardless of the exit code.Testing
packages/core/platform/tests/bundles_multus_staging_test.yamlpins the delivery point: both variant defaults, both explicit overrides, and that multus is still shipped in each bundle at all. That last one because multus is now included by hand in two branches rather than by the shared helper.hack/multus-install-cni-plugins.batsextracts the init container's script from the rendered DaemonSet and runs it. It pins that every plugin lands executable, that a replacement swaps the inode rather than truncating in place, that temp files never leak, that a missing or empty directory skips instead of failing, and that a plugin which cannot be installed is reported on stderr and skipped rather than failing the container. Both halves of that last one are asserted, because exit 0 alone would still pass with the report deleted.hack/cni-plugins-staging-contract.batscovers what the rendered manifest cannot show: that the Dockerfile stages the intended plugins and only those, copies them into the final stage, and that nothing there removes them again, that the tarball is verified before extraction, and that the version marker matches the vendored kube-ovn release. helm-unittest pins both states of the value, including off, which is what most installs run, plus the quoted-string forms a values file can produce. An explicitnullis covered too: it is how a values file clears the key, and the chart folds it onto its own default rather than refusing it as an unreadable spelling.The feature is inert until the image is re-baked. All three
image:lines still point at the pre-change digest, which has no/cni-plugins, so onmainafter merge a generic install renders the init container, logsno /cni-plugins in this image; skipping staging, and installs nothing until release-prep re-pins. That is the intended flow (first-party digests are release-managed, not hand-edited), and the guide documents the log line, but anyone testing this onmainwill meet it first and the pod will look healthy.No CI job stages plugins on a node. CI boots Talos, where the value is off, and
isp-full-genericappears in no e2e workflow. The mechanics are covered by the bats above, which run the real script from the real manifest and also underdash, the shell in the runtime image, but nothing demonstrates it end to end on a node.Defaulting it on for the generic bundle is a deliberate maintainer decision taken with that gap known, not a side effect of the diff. Shipping it off would leave the bug in place on the only platform where the feature does anything, and would not add coverage either, since the code stays unexecuted in CI both ways. What makes the risk acceptable: the init container skips rather than crashlooping when the image carries no plugins, each plugin is swapped in with an atomic rename instead of being written onto the live path, the tarball is checksummed before extraction, and declining is a value rather than disabling the package. And a plugin it cannot install is reported and skipped rather than failing the pod, so the worst case leaves the node exactly as it was before this change rather than unable to schedule.
Screenshots
N/A, no UI changes.
Release note
BREAKING CHANGE: enabling stageCniPlugins replaces reference CNI plugins in /opt/cni/bin on existing nodes, Talos included -- four of the names it writes are ones the Talos node image also carries. Opt out before upgrading: declining afterwards does not restore what was replaced.
Summary by CodeRabbit
New Features
/opt/cni/bin, adding missing plugins and safely replacing matching versions.stageCniPluginssetting, with platform-specific defaults and explicit opt-out support.Documentation