feat(kubernetes): per-pool kernel modules and Talos schematic for worker nodes by mattia-eleuteri · Pull Request #3571 · cozystack/cozystack

mattia-eleuteri · 2026-08-06T06:11:19Z

What this PR does

Makes a GPU worker node pool expressible on Talos, which today it is not. Two pool-scoped fields on KubernetesNodes, one commit each:

Field	Fixes
`kernelModules`	The extension ships the NVIDIA module and nothing loads it, so the GPU never initialises.
`schematicID`	`talos.schematicID` is cluster-wide, so the NVIDIA schematic reaches non-GPU pools and reboots them every ~70 minutes.

They are two halves of one problem: fixing only the first leaves an operator who follows the documentation — set the NVIDIA schematic, declare the modules — with every non-GPU pool in a reboot cycle. They are not independently usable, which is why they are filed together.

Rebased and reduced, 2026-09-03. This PR originally carried the same two fields on both charts that rendered a worker pool. Upstream has since merged refactor(kubernetes): remove worker node pools from the chart: nodeGroups no longer exists in packages/apps/kubernetes, and its talos-reconcile-job.yaml and the cross-chart tests/render-parity.sh are gone, replaced by tests/render_snapshot_test.yaml in kubernetes-nodes. The parent-chart half of this branch has nowhere to land and was dropped rather than merged — packages/apps/kubernetes, packages/system/kubernetes-rd, api/apps/v1alpha1/kubernetes and hack/ are now byte-identical to main. The feature is unchanged; it just lives on the one chart that still renders a worker pool, where both fields are pool-scoped by construction rather than by a per-node-group key.

Gap 1: nothing loads the kernel modules

Implements gap 1 of #3563: a values surface for the kernel modules a Talos worker pool loads at boot.

A Talos system extension installs a kernel module but does not load it — loading is machine.kernel.modules' job. The chart emitted no such block and no values key could add one, so a GPU pool on Talos workers was unusable as shipped: the extension supplied the NVIDIA module, nothing loaded it, and the failure was silent end to end. The VM held the PCI device, the node advertised no GPU, and no component logged an error. The only workaround was a hand-written TalosConfigTemplate, which is impractical because its spec is immutable. Found in a production 1.6.0 environment, where the worker rollover took the 4 GPUs of a tenant cluster out of service with no error anywhere.

Design: three states, and no default

The shape was arbitrated in #3563 (option 3 of the three proposed there) rather than decided here, because terraform-provider-cozystack transcribes values.schema.json by hand and a later rename breaks that side too. Andrei Kvapil (@kvaps) / Aleksei Sviridkin (@lexfrei): if you would rather have option 1 (explicit field only, no automatic set), say so — the change is small and local, and I will document the NVIDIA value to set instead.

absent — the chart chooses. A pool holding at least one nvidia.com/* GPU gets nvidia, nvidia_uvm, nvidia_drm, nvidia_modeset; any other pool gets no kernel block at all.
non-empty list — taken verbatim, replacing the chart's choice outright.
[] — explicit opt-out: no modules even on a GPU pool.

The order is not cosmetic: Talos loads the list in sequence, nvidia has to come first because the other three depend on it, and nvidia_uvm is what CUDA unified memory needs. This is the order validated against a production GB202 passthrough node.

The three states are distinguishable only because the field carries no default — no entry in values.yaml, no default in values.schema.json. A default of [] would collapse "absent" into "opted out" for every pool and make the automatic set unreachable, and a bare kernelModules: (null) fails schema validation under helm-unittest, which sees the null before Helm's coalescing drops it. Both traps are called out in comments where someone would be tempted to add one.

That undefaulted shape has two consequences worth stating plainly, both raised in review and both living in generated files rather than in this diff. The generated parameter table prints [] in the Default column for any array with no default, which for this field reads as the opt-out rather than as "unset" — the description now says so explicitly, since the column itself is the generator's. And keysOrder in the ApplicationDefinition is derived by hack/update-crd.sh from the keys of values.yaml, so a field with no key is not in it: the dashboard renders kernelModules in RJSF's * bucket, at the bottom of the form, rather than between schematicID and kubelet. Fixing that properly means teaching the generator about schema properties with no values.yaml key, which regenerates every package's keysOrder and wants its own PR.

Automatic emission is keyed on the nvidia.com/ resource prefix, not on the mere presence of gpus, so an AMD pool is not given NVIDIA modules on a guess.

Loading the module is all this does. Which extension supplies it remains the schematic's business, and that part is a docs gap rather than a chart gap — see the website follow-up below.

No involuntary worker roll

A pool that resolves to no modules renders the machine config it rendered before this field existed, byte for byte. The evidence is now upstream's own gate rather than a bespoke one: tests/render_snapshot_test.yaml and its committed golden snapshot pass unmodified on this branch, all 12 snapshots, so every rendered object for a default pool is identical — including the talos-reconcile Job's content-hash name, which means the Job is not recreated and no worker is reconciled for this change alone.

Gap 2: the schematic is cluster-wide, and that reboots every other pool

Found in production, on a 1.6.0 cluster with one GPU pool and one non-GPU pool. All four nodes of the non-GPU pool were rebooting in a loop, each at almost exactly 4206 s of uptime, in staggered phases. Every pod on a node died at once (SandboxChanged, TaintManagerEviction), which is why every pod on a given node carried an identical restart count — about 26 over 30 hours.

The cause is that a Talos schematic is a fixed set of system extensions baked into one image, and Talos refuses to finish the boot sequence when an extension service in it cannot start:

[talos] task startAllServices (1/1): service "ext-nvidia-cdi-gen" to be "up", service "ext-nvidia-persistenced" to be "up"
[talos] task startAllServices (1/1): failed: 2 errors occurred:
[talos] phase startEverything (9/9): failed
[talos] boot sequence: failed        -> reboot

Both services need an NVIDIA card. On a node that has none they never come up, so the node reboots, forever. talos.schematicID is a single value feeding every node's boot image, so a cluster that mixes GPU and non-GPU pools has no correct value to set: the GPU pool needs the extensions and every other pool is broken by them.

What makes this expensive to diagnose is that nothing reports it. kubelet starts before the failing phase, so the node holds Ready for the whole 70 minutes and kubectl get nodes looks healthy. kubectl top nodes shows up to 96% memory on small nodes just before the reboot, which reads as an OOM — it is page cache, and min_over_time(node_memory_MemAvailable_bytes[30h]) never drops below 1.2 GiB. The discriminant is the regularity: four nodes rebooting within a few seconds of the same uptime is a timer, never memory pressure.

So this adds an optional pool-level schematicID that falls back to talos.schematicID, letting the NVIDIA schematic be scoped to the pool that actually has the cards. Both consumers resolve it through one helper — the boot disk image the DataVolume pulls, and the installer image in the TalosConfigTemplate — because they have to agree: the installer is what an in-place Talos upgrade runs, so a mismatch would swap a node's extension set out from under it. A parity case covers both at once, so a chart that overrode one and not the other fails.

Unlike kernelModules this is deliberately not derived from gpus. A schematic ID is an opaque image-factory digest; the chart cannot know which one carries the NVIDIA extensions, and cannot synthesise one. Supplying it stays the operator's job — this PR only makes it possible to supply it per pool.

Unset renders byte-identically to before, so the content-hash-named KubevirtMachineTemplate keeps its name and no worker is rolled. Setting it does roll that pool's workers, which is inherent rather than incidental: changing a node's boot image means replacing the node.

I could not find an existing issue for this; #3563 is the closest and covers the schematic only as "which one to use", not the per-pool scoping. Happy to split it into its own issue if you prefer the paper trail.

Sequencing with #3523

Read together with #3523 (fix(kubernetes): name TalosConfigTemplate by content hash), which is mine and still open. Aleksei Sviridkin (@lexfrei) agreed this surface goes first or together.

Before #3523 merges, kernelModules changes nothing for existing pools. That is #3515: the TalosConfigTemplate apply is rejected server-side while the HelmRelease stays Ready, so a kernelModules added to a pool that already exists is silently not applied. Only pools created afterwards get it, because their template does not exist yet. The release note says so explicitly rather than letting this read as a fix for running clusters. If both PRs go in the same release the caveat disappears; if this one ships alone, the note is the honest description.

schematicID is split across that line, and the split is worth being precise about. Its effect on the boot disk image goes through the KubevirtMachineTemplate, which is content-hash named and referenced by the MachineDeployment, so it lands on an existing pool today and the reboot loop is fixed without waiting for #3523. Its effect on the installer image goes through the TalosConfigTemplate and is therefore subject to #3515, so until #3523 an in-place Talos upgrade on a pool with an overridden schematic would still run the previous schematic's installer. That is a narrower window than it sounds — the two only diverge during an upgrade — but it is the reason the two PRs belong in the same release.

Tests

Both suites are new, and the whole kubernetes-nodes suite passes: 98 tests, 18 suites, 12 snapshots.

kernel_modules_test.yaml, 10 cases: the NVIDIA set and its order for an nvidia.com/* pool, no kernel block for a pool with neither GPUs nor modules, an explicit list taken verbatim including parameters, an explicit list replacing the NVIDIA default, [] opting out while leaving the gpu=on label intact, no NVIDIA assumption for a non-NVIDIA vendor, a semicolon-separated module parameter accepted, a module name and a module parameter each rejected when they could inject shell into the reconcile Job, and an unvalidated extra key on an item dropped rather than copied through. The patterns pin absolute indentation rather than \s+, because modules: one level out is still valid YAML, is still accepted by the apiserver, and silently loads nothing.
schematic_per_pool_test.yaml, 7 cases: the override reaching both the boot image and the installer, fallback to the cluster-wide value, an empty string treated as unset rather than rendering a URL with an empty path segment (which would 404 at the factory and hang the import), two shell-injection rejections, and two acceptance cases for the readable and multi-segment schematic names a mirror or file server serves.
hack/talos-reconcile-heredoc_test.bats gains two execution-level cases for the schematic guard, one through the cluster-wide talos.schematicID and one through the pool-level schematicID, because the guard runs on the effective value and a pool override that skipped it would be a hole. Upstream's first case asserts a hostile schematicID reaches the heredoc and survives literally; this branch refuses that render instead, so that case now carries a benign multi-segment schematic (gpu/nvidia-open) to keep the site covered, and the refusal is what the two new cases pin. The escape chain stays in the template as a second line of defence, unreachable for hostile input while the guard holds. 4 passed.
render_snapshot_test.yaml — upstream's, unmodified — is what pins the no-involuntary-roll property now that there is only one chart rendering a worker pool and nothing left to compare it against.
make generate in the package, and controller-gen object for the api/apps/v1alpha1 submodule, whose deepcopy the package-level target does not cover.

Two pre-existing failures in make unit-tests on this branch are unrelated and also fail on main: hack/migration-54-redis-adopt.bats needs a running Docker daemon, and make rd-presets-check reports postgres-rd missing the t1.large preset.

Not in scope

The rest of #3563, deliberately: the nvidia-operator-validator → device-plugin chain that cannot validate an OS-provided driver, CDI spec generation and the nvidia containerd runtime default, install.image derivation from spec.talos.schematicID (already fixed by #3523), and the maxSurge / nodeStartupTimeout defaults (already raised in comments on #3523).

Also out of scope, and noted here because review surfaced them: the omitempty on every slice field in api/apps/v1alpha1 — which makes an explicitly empty [] invisible to a Go typed client, and can only be changed in cozyvalues-gen — and, newly, the orphaned talos.registryMirrors left in packages/apps/kubernetes/values.yaml by the worker-pool refactor, whose only consumer now lives in kubernetes-nodes. Neither is in this diff.

Screenshots

No UI changes.

Downstream repositories

Both are issues rather than PRs, and deliberately so. Both predate the reduction above and both narrow with it: the fields now land on KubernetesNodes only, not on Kubernetes.nodeGroups, so each follow-up covers one resource instead of two. I have said so on each issue.

terraform-provider-cozystack is hand-written with no codegen from values.schema.json, so both new fields need a schema entry, a model entry and an expand/flatten pair on KubernetesNodes. I filed an issue instead of a PR because the undefaulted three-state behaviour is exactly where that provider's habit of sending its own defaults would break users silently: a provider that materialises kernel_modules = [] when the user did not set it would disable the automatic NVIDIA modules for every GPU pool managed through Terraform, reintroducing the production failure this PR fixes. That needs a maintainer who knows the provider's conventions for list-of-object attributes, not a guess from me. The issue spells the trap out.

website needs the three-state behaviour written down (a generated parameter table cannot convey it), and separately it needs the Blackwell constraint that is documented nowhere today: on GB202 the schematic must carry siderolabs/nvidia-open-gpu-kernel-modules-production, because with the proprietary nonfree-kmod-nvidia-production extension the module loads, /dev/nvidia0 appears, and nvidia-smi -L then reports No devices found with no error anywhere. That cost us about an hour, and Aleksei Sviridkin (@lexfrei) acknowledged the gap. #561 already asks for GPU passthrough documentation but predates the Talos worker rollover and covers different ground, so #643 is filed as new and cross-references it.

No other repository in the map is touched: this change adds no package, no platform component, no bundle or variant, no release asset, and does not alter ApplicationDefinition semantics or packages/core/platform/values.yaml.

Release note

feat(kubernetes): Worker node pools (`KubernetesNodes`) accept two new pool-scoped fields, together making a GPU pool usable on Talos workers. `kernelModules` is emitted as Talos `machine.kernel.modules`: a Talos system extension installs a kernel module without loading it, so a GPU pool previously came up with no working driver and no way to fix it through values. Left unset, a pool holding at least one `nvidia.com/*` GPU now loads `nvidia`, `nvidia_uvm`, `nvidia_drm` and `nvidia_modeset` automatically; an explicit list replaces that set, and an explicit `[]` opts out. `schematicID` overrides the cluster-wide `talos.schematicID` for one pool, applying to both its boot disk image and its Talos installer image. This fixes a reboot loop in clusters that mix GPU and non-GPU pools: a schematic is a fixed set of system extensions, `ext-nvidia-persistenced` and `ext-nvidia-cdi-gen` require an NVIDIA card, and Talos refuses to finish booting when an extension service cannot start, so a non-GPU node booting an NVIDIA schematic rebooted roughly every 70 minutes while still reporting `Ready`. Scope the NVIDIA schematic to the GPU pool instead. Pools that set neither field render exactly what they rendered before, so no existing worker is reconciled by this change; setting `schematicID` on a pool does replace its boot image and therefore rolls that pool's workers. NOTE: until the fix for the `TalosConfigTemplate` naming (#3523) is released, setting `kernelModules` on a pool that already exists has no effect — the apply is rejected server-side while the HelmRelease still reports `Ready` (#3515). Only pools created after this release pick it up. `schematicID` is affected by #3515 in one half only: its change to the boot disk image takes effect on an existing pool, because the `KubevirtMachineTemplate` is content-hash named and the MachineDeployment follows it, so the reboot loop is fixed immediately; its change to the installer image in the `TalosConfigTemplate` is not applied until #3523, so an in-place Talos upgrade on such a pool would run the previous schematic's installer until then. On Blackwell (GB202) the schematic must carry the open kernel modules extension, not the proprietary one.

Summary by CodeRabbit

New Features
- Added per-pool Talos schematic ID overrides, with cluster-wide fallback support.
- Added configurable kernel modules with optional parameters.
- GPU node pools automatically receive NVIDIA kernel modules unless explicitly overridden.
- Kernel module lists can be replaced or explicitly disabled.
Bug Fixes
- Added validation rejecting unsafe schematic IDs, module names, and parameters before rendering.
Documentation
- Updated configuration guidance, supported formats, defaults, validation rules, and rollout behavior.

A Talos system extension installs a kernel module but does not load it; loading is machine.kernel.modules' job. Neither chart emitted that block and no values key could add one, so a GPU node group on the Talos workers introduced in 1.6.0 was unusable as shipped: the extension supplied the NVIDIA module, nothing loaded it, and the failure was silent end to end. The VM held the PCI device, the node advertised no GPU, and no component logged an error. The only workaround was a hand-written TalosConfigTemplate, which is impractical because its spec is immutable. Add kernelModules to both charts that render a worker pool: nodeGroups.<name>.kernelModules in packages/apps/kubernetes, and kernelModules at the root of packages/apps/kubernetes-nodes, which takes pool fields flat as it already does for gpus. The field is three-state, and the states are distinguishable because it carries no default: absent lets the chart choose (a group holding at least one nvidia.com/* GPU gets nvidia, nvidia_uvm, nvidia_drm, nvidia_modeset, in that order, since Talos loads the list in sequence and the last three depend on the first; any other group gets nothing), a non-empty list replaces that choice outright, and an explicit [] opts out even on a GPU group. A default of [] would collapse absent into opted-out for every group and make the automatic set unreachable. A group that resolves to no modules renders the machine config it rendered before, byte for byte, so its content-hash Job name does not rotate and no worker is reconciled for this change alone. Also extend the render-parity check to compare the machine config the two talos-reconcile Jobs apply. It previously stopped at the four pool objects, on the reasoning that the Job's content-hash name makes a divergence visible separately; that holds for one chart drifting over time, not for two charts disagreeing with each other, and the machine config is the one thing they duplicate outright. It is comparable because it refers to the release only through shell variables the Job expands at runtime, so the rendered text is release-name independent. Refs: cozystack#3563 Assisted-By: Claude <[email protected]> Signed-off-by: Mattia Eleuteri <[email protected]>

coderabbitai · 2026-08-06T06:11:27Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: 2f7fa4d7-06e6-4064-989a-8aec00e2c937

📥 Commits

Reviewing files that changed from the base of the PR and between 1cb5705 and c749681.

📒 Files selected for processing (1)

hack/talos-reconcile-heredoc_test.bats

Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review.

📝 Walkthrough

Walkthrough

The change adds per-pool Talos schematic selection and explicit worker kernel-module configuration. Helm templates validate and render both settings, apply GPU defaults, preserve empty overrides, and use the resolved schematic for boot and installer images.

Changes

Talos worker configuration

Layer / File(s)	Summary
API contracts and deep-copy support `api/apps/v1alpha1/kubernetesnodes/types.go`, `api/apps/v1alpha1/kubernetesnodes/zz_generated.deepcopy.go`	Adds `ConfigSpec.SchematicID`, `ConfigSpec.KernelModules`, and the public `KernelModule` type with deep-copy methods.
Kubernetes-nodes rendering `packages/apps/kubernetes-nodes/templates/_helpers.tpl`, `packages/apps/kubernetes-nodes/templates/nodegroup.yaml`, `packages/apps/kubernetes-nodes/templates/talos-reconcile-job.yaml`	Resolves and validates schematic IDs, renders kernel modules, applies NVIDIA defaults, and propagates values to worker images and reconcile jobs.
Kubernetes-nodes validation `packages/apps/kubernetes-nodes/tests/kernel_modules_test.yaml`, `packages/apps/kubernetes-nodes/tests/schematic_per_pool_test.yaml`, `hack/talos-reconcile-heredoc_test.bats`	Tests module defaults, explicit lists, opt-outs, parameter handling, schematic precedence, accepted formats, and unsafe input rejection.
Documentation and system schemas `packages/apps/kubernetes-nodes/README.md`, `packages/apps/kubernetes-nodes/values.schema.json`, `packages/apps/kubernetes-nodes/values.yaml`, `packages/system/kubernetes-nodes-rd/cozyrds/kubernetes-nodes.yaml`	Documents the configuration, validation rules, GPU behavior, rollout semantics, schema fields, and dashboard ordering.

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

Merge Risk: ⚪ Minimal · up to c7496

Pool-scoped Talos schematic and kernel-module configuration is covered by validation and regression tests, with no concrete merge-blocking risk identified.

Sequence Diagram(s)

sequenceDiagram
  participant PoolValues
  participant KubernetesHelpers
  participant NodeGroupTemplate
  participant ReconcileJob
  PoolValues->>KubernetesHelpers: provide schematicID and kernelModules
  KubernetesHelpers->>NodeGroupTemplate: resolve schematicID
  KubernetesHelpers->>ReconcileJob: render validated kernel modules
  NodeGroupTemplate->>ReconcileJob: select boot and installer image references

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the main changes: per-pool kernel modules and Talos schematic configuration for Kubernetes worker nodes.

Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3 files.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

A Talos schematic is a fixed set of system extensions baked into one image, and Talos refuses to finish booting when an extension service in it cannot start. ext-nvidia-persistenced and ext-nvidia-cdi-gen require an NVIDIA card, so a node with no GPU that boots an NVIDIA schematic fails startAllServices and reboots, indefinitely. talos.schematicID was cluster-wide, so a cluster mixing GPU and non-GPU node groups had no correct value: the GPU group needs the extensions and every other group is broken by them. Setting the NVIDIA schematic for the sake of the GPU group put every other group into a reboot cycle of about 70 minutes per node. Nothing reports it — kubelet starts before the failing phase, so the node holds Ready and only the pod restart counters, identical across a node, betray it. Seen in production on 1.6.0. Add an optional per-node-group schematicID falling back to talos.schematicID, so the NVIDIA schematic can be scoped to the group that has the cards. Both consumers resolve it through one helper: the boot disk image the DataVolume pulls, and the installer image in the TalosConfigTemplate. They have to agree, or an in-place Talos upgrade swaps a node's extension set out from under it. Deliberately not derived from gpus, unlike kernelModules in the previous commit: a schematic ID is an opaque image-factory digest, so the chart cannot know which one carries the NVIDIA extensions or synthesise one. Unset renders byte-identically to before, so the content-hash-named KubevirtMachineTemplate keeps its name and no worker is rolled. Setting it does roll that group's workers, which is inherent — changing a node's boot image means replacing the node. Refs: cozystack#3563 Assisted-By: Claude <[email protected]> Signed-off-by: Mattia Eleuteri <[email protected]>

Aleksei Sviridkin (lexfrei)

NOT LGTM — two things: the new fields reach the shell unvalidated, and kernelModules does not apply to workers that already exist, which the field description does not say.

Business context: a GPU node group is not expressible on Talos workers today. Nothing loads the NVIDIA modules, and one cluster-wide schematic cannot serve a cluster that mixes GPU and non-GPU groups.

On the API shape you asked me to arbitrate in #3563: take option 3, the one you implemented. Option 1 leaves every GPU user to discover four module names and their order, and that missing knowledge is the actual gap, not the typing. Keying the automatic set on the nvidia.com/ prefix instead of on gpus being non-empty is the right call too.

Blockers

B1: kernelModules and schematicID are not validated before they reach the shell

File: packages/apps/kubernetes/templates/talos/talos-reconcile-job.yaml:401, mirrored at packages/apps/kubernetes-nodes/templates/talos-reconcile-job.yaml:303

The machine config is written through cat <<EOF | kubectl apply -f - with an unquoted delimiter, which the script needs so ${RELEASE} and friends expand. That also means the shell expands everything else in the block. kernelModules[].name, kernelModules[].parameters[] and schematicID are free-form CR strings typed as plain string, and they land there verbatim.

Evidence: rendering with kernelModules: [{name: "nvidia$(id)"}] emits - name: nvidia$(id) into command[2], and an unquoted heredoc of that shape substitutes instead of passing through (printf 'cat <<EOF\n- name: x$(echo IN)\nEOF\n' | sh prints - name: xIN). The neighbouring tenant-supplied strings in the same block are guarded before they get there: cluster.yaml runs regexMatch over systemReservedMemory, kubeReservedMemory, systemReservedCpu and kubeReservedCpu. That guard is the file's existing convention for this hazard, and the new fields skip it.

Fix: a render-time fail in both new helpers, with a failedTemplate assert next to the cases already in kernel_modules_test.yaml. ^[a-z0-9_-]+$ for module names and ^[0-9a-f]{64}$ for a schematic ID cover the real syntax. cozyvalues-gen has no pattern vocabulary, so the schema cannot carry this.

B2: kernelModules does not reach workers that already exist

File: packages/apps/kubernetes/values.yaml:94

MachineDeployment.spec.template.spec.bootstrap.configRef.name is the fixed <release>-<group> (cluster.yaml:797), so rewriting the TalosConfigTemplate leaves spec.template untouched and CAPI starts no rollout. A running Machine keeps the config it booted with. I rendered a GPU group with neither field set against main: the KubevirtMachineTemplate hash is identical, only the Job hash moves.

Impact: an operator with the dead GPU node group from #3563 upgrades, gets the automatic NVIDIA set, and still has no GPU until the Machines are replaced. The schematicID description states its rollout semantics; kernelModules says nothing, so the asymmetry reads as "this one applies immediately".

Fix: one sentence in the values.yaml annotation. It propagates to README, schema and CRD.

Non-blocking follow-ups

#3294 is open and adds a second surface for the same value: nodeGroups.<name>.image.builtin.schematicID and image.factory.schematicID, both falling back to the cluster-wide talos.schematicID. It edits the same two files, so whichever lands second conflicts textually, and merging both leaves two ways to set one thing with no precedence. This PR is the better base for it. #3294's own description says its factory.schematicID redirects only the boot-disk import while the installer keeps coming from the cluster-wide value, which is exactly the divergence this PR closes by routing both through one helper.
hack/e2e-talos-image-cache.yaml pre-fetches a single schematic. Nothing to do now, since no e2e case sets a per-group override, but one that does would miss the cache and fall back to the public factory that cache exists to avoid.

What I checked rather than took from the description: reordering the module list in one chart's helper makes render-parity.sh exit 1 on the gpu and schematic cases, and it exits 0 unmutated, so the machine-config comparison is a real gate. With no new fields set, the md0 template and Job hashes match main exactly. The rendered kernel.modules block lands at the right level, and the per-group schematic reaches both the boot image URL and the installer. make generate in both packages produces no drift.

Review B1. The worker machine config is written through `cat <<EOF | kubectl apply -f -` with an unquoted delimiter, which the script needs so ${RELEASE} and friends expand and which therefore expands everything else in the block. kernelModules[].name, kernelModules[].parameters[] and schematicID are free-form strings from a tenant-facing CR that land in it verbatim: a module name of `nvidia$(id)` runs `id` inside the talos-reconcile pod, whose ServiceAccount can write TalosConfigTemplates and read Talos secrets. Guard all three at render time with regexMatch and fail, the convention cluster.yaml already uses for the kubelet reservation strings in the same block. cozyvalues-gen has no pattern vocabulary, so values.schema.json cannot carry this. Module names take ^[a-z0-9_-]+$ and parameters ^[A-Za-z0-9_.,:=+/-]+$, validated only when the operator supplied the list — the automatic NVIDIA set is valid by construction. schematicID takes ^[0-9a-f]{64}$, the whole of an image-factory digest's syntax, and is validated on the EFFECTIVE value, which also closes the pre-existing path through the cluster-wide talos.schematicID. Review B2. Document that kernelModules does not reach workers that already exist: MachineDeployment.spec.template.spec.bootstrap.configRef names a fixed TalosConfigTemplate, so rewriting the template leaves spec.template untouched, CAPI starts no rollout, and a running Machine keeps the config it booted with. The asymmetry with schematicID, which rolls the group by changing its boot image, is now stated in both. Refs: cozystack#3563 Assisted-By: Claude <[email protected]> Signed-off-by: Mattia Eleuteri <[email protected]>

mattia-eleuteri · 2026-08-06T09:12:54Z

Both blockers fixed in c6444ab. One correction to follow-up 1, with evidence, because it changes who should own the schematic field.

B1 — validated before the shell

Reproduced first: rendering kernelModules: [{name: "nvidia$(id)"}] emitted - name: nvidia$(id) into command[2], and printf 'cat <<EOF\n- name: x$(echo IN)\nEOF\n' | sh prints - name: xIN, so the unquoted delimiter substitutes as you described.

Guarded all three at render time with regexMatch + fail, following the convention cluster.yaml already uses for the kubelet reservation strings in the same block:

kernelModules[].name → ^[a-z0-9_-]+$
kernelModules[].parameters[] → ^[A-Za-z0-9_.,:=+/-]+$, which admits NVreg_EnableGpuFirmware=1 and excludes $, backticks, quotes and whitespace
schematicID → ^[0-9a-f]{64}$

Two choices worth flagging rather than burying:

Module names and parameters are validated only when the operator supplied the list, mirroring the comment on the kubelet guards — the automatic NVIDIA set is valid by construction. schematicID is validated on the effective value instead, so it also closes the pre-existing path through the cluster-wide talos.schematicID, which reached the same heredoc before this PR. That does tighten an existing field: a cluster carrying a non-64-hex talos.schematicID today renders fine and would now fail. Every image-factory digest is 64 lowercase hex and anything else 404s at the factory, so I judged a loud render failure better than a VM stuck importing a URL that cannot resolve — but say the word and I will scope the guard to the per-group override only.

failedTemplate cases sit next to the existing ones in kernel_modules_test.yaml and the schematic suites, four in total. The schematic ones carry a template: scope with a comment saying why: the guard is in a shared helper reached from both templates in the suite, and Helm aborts on the first to hit it, so the assert names the template that reports the failure rather than the only one able to.

B2 — documented

You are right that this is not just #3515. configRef.name is the fixed <release>-<group> at cluster.yaml:797, so the MachineDeployment's spec.template never changes and CAPI starts no rollout; a running Machine keeps the config it booted with, independently of whether the apply succeeded. Added to the values.yaml annotation in both charts, so it propagates to README, schema and CRD, and stated the asymmetry explicitly — schematicID rolls the group by changing its boot image, kernelModules does not roll anything by itself.

Follow-up 1 — #3294 already does this, more completely

Checking it, I do not think the schematic field should stay in my PR at all, and the reason cuts against what you wrote. The NOTE you are quoting is about imageFactoryURL, not the schematic: it says redirecting a group's factory URL does not redirect the installer, because talos.installerRepository has no per-group override. On the schematic #3294 already routes both consumers per group. Its reconcile job renders image: {{ ... }}/{{ .talosSchematicID }}:{{ .talosVersion }}, with $grpSchematicID resolved from image.builtin.schematicID or image.factory.schematicID falling back to the cluster-wide value, and it ships tests pinning exactly that (install.image pins the group's per-group schematicID/version, and a fallback case). So it is not the divergence my helper closes — it closes it too, and adds per-group version and the golden-clone path on top.

That makes my flat nodeGroups.<name>.schematicID a strict subset of nodeGroups.<name>.image.factory.schematicID, which is exactly the two-ways-to-set-one-thing you want to avoid, and #3294 is the richer surface. Concretely, the two also collide in a way that shows the duplication is not benign: #3294's test uses schematicID: deadbeef, which my new ^[0-9a-f]{64}$ guard rejects.

My proposal, yours to overrule:

feat(kubernetes): boot tenant worker disks from a shared golden Talos image (CDI clone) #3294 owns per-group schematic scoping. I drop my second commit on your word — it is self-contained, so this is a git revert and a release-note edit, not a rewrite.
This PR keeps kernelModules, which feat(kubernetes): boot tenant worker disks from a shared golden Talos image (CDI clone) #3294 does not provide, plus the B1 guards and the render-parity machine-config comparison.
Until feat(kubernetes): boot tenant worker disks from a shared golden Talos image (CDI clone) #3294 lands, my commit is the only thing that stops the reboot loop, which is live in production. So I would rather not drop it before feat(kubernetes): boot tenant worker disks from a shared golden Talos image (CDI clone) #3294 is close to merging. If you would rather I drop it now and let the loop wait on feat(kubernetes): boot tenant worker disks from a shared golden Talos image (CDI clone) #3294, that is fine too — just tell me which.

If instead you want the flat field kept as the canonical one and #3294's union to defer to it, that works mechanically, but I would argue against: #3294 needs per-group version and builtin anyway, so the union has to exist regardless, and a flat field beside it is the redundancy rather than a simplification.

Follow-up 2 — e2e image cache

Noted, nothing changed. No e2e case sets a per-group override, so nothing misses the cache today. Worth saying that #3294 removes hack/e2e-talos-image-cache.yaml outright and has the suites clone a golden instead, so if the schematic field lands there this concern resolves itself rather than needing a guard here.

Aleksei Sviridkin (lexfrei)

NOT LGTM — the schematic guard rejects a configuration that renders on main today.

Both previous blockers are closed. I re-ran the payload against this head: nvidia$(id) now fails the render in both charts, and validating the effective value closes the cluster-wide path as well, which is the better of the two options you offered. Weakening ^[a-z0-9_-]+$ to ^.*$ turns kernel_modules_test.yaml red, so the new asserts are load-bearing rather than decorative. The B2 wording is accurate and landed in both charts.

You are right about #3294 and I was wrong. The NOTE I quoted sits on imageFactoryURL, not on the schematic, and #3294 does route the per-group schematic into install.image through .talosSchematicID, with a test pinning it. My follow-up 1 argued from a misreading of that field. Disregard the reasoning; the ownership question it raised is answered below on different grounds.

Blockers

B3: the 64-hex rule rejects air-gapped and mirrored configurations that work today

File: packages/apps/kubernetes/templates/_helpers.tpl:283, mirrored in packages/apps/kubernetes-nodes/templates/_helpers.tpl

^[0-9a-f]{64}$ on the effective value encodes the public factory's naming as the only legal one. The chart supports more: talos.imageFactoryURL documents "a self-hosted Image Factory, a caching mirror, or an internal HTTP file server", and talos.installerRepository documents mirrored registries. On a file server or a mirrored registry the operator picks the path, and a readable name is the obvious choice.

Evidence: values carrying imageFactoryURL: http://images.internal.example.com/talos, installerRepository: registry.internal.example.com/talos/installer and schematicID: talos-gpu-nvidia-open render on main into .../image/talos-gpu-nvidia-open/v1.13.6/openstack-amd64.raw.xz and .../installer/talos-gpu-nvidia-open:v1.13.6, and fail the render on this head. Same values, same command, only the checkout differs. #3294's schematicID: deadbeef test hits the same wall, which makes it two independent cases rather than one contrived one.

Impact: a cluster running against a mirror stops reconciling after the upgrade, on a field its operator never touched.

Fix: make the guard the injection guard the comment says it is, instead of a format guard. A class such as ^[A-Za-z0-9._:-]+$ on the effective value rejects $, backticks, quotes and whitespace, keeps the cluster-wide path closed, and leaves mirror naming alone. A wrong ID still 404s at the factory, exactly as it did before this PR, and that failure is visible in the DataVolume rather than hidden.

On who owns the schematic field

Keep it here for now, and I will take the redundancy. #3294 is the richer surface and should own this in the end, but it is not close to landing: it brings a new package, a golden catalog and an e2e rework, while the reboot loop is live. When #3294 rebases, have image.factory.schematicID fall back to nodeGroups.<name>.schematicID rather than replace it, so nothing that shipped breaks, the union stays canonical, and the flat field can be deprecated on its own schedule. One documented redundancy for a release is cheaper than leaving a production reboot loop waiting on a large PR.

Follow-up 2 is answered and nothing is owed there: #3294 deletes hack/e2e-talos-image-cache.yaml outright, so the concern resolves itself if the schematic ends up there.

Aleksei Sviridkin (lexfrei)

NOT LGTM — the injection guard is bypassable. This lands on the same head as my previous review, which I posted before finishing this pass.

B4: any extra key on a kernelModules item walks past the guard

File: packages/apps/kubernetes/templates/_helpers.tpl:194, and the same copy in packages/apps/kubernetes-nodes/templates/_helpers.tpl

The validator walks .name and .parameters, but the emit is toYaml over the raw user dict, so every other key on the item is copied into the heredoc verbatim.

Evidence: a group carrying

kernelModules:
  - name: dummy
    evil: "$(touch /tmp/pwned)"

renders - evil: $(touch /tmp/pwned) inside cat <<EOF | kubectl apply -f -, in both charts. Nothing upstream catches it either: the items object in values.schema.json has no additionalProperties: false, and the aggregated apiserver does not enforce that schema on write. specSchema in pkg/registry/apps/application/ is wired into rest_defaulting.go only, with no pruning and no validation on the Create or Update path. So the render is the last gate, exactly as your comment in the helper says.

Fix: build the emitted list from validated fields, dict "name" $name "parameters" $params, instead of passing user input through toYaml. Then anything unvalidated cannot reach the output by construction rather than by enumeration. A third case in kernel_modules_test.yaml with an extra key carrying $(...) fails today and pins it.

B5: review provenance in two committed test files

packages/apps/kubernetes/tests/kernel_modules_test.yaml:260 and packages/apps/kubernetes-nodes/tests/kernel_modules_test.yaml:156 both open with # B1 from review:. Comments here are held to the same standard as commit messages: self-contained, no review-iteration references, because the next reader has no access to this thread. The rest of the sentence is good, just drop the first four words.

Smaller

schematic_per_nodegroup_test.yaml has the $(id) rejection case but not the plain not-64-hex one that schematic_per_pool_test.yaml:117 carries. And since the guard now constrains the effective value, talos.schematicID's own description should say so instead of reading as a free-form override. Both of these are moot if B3 changes the guard's shape.

One sentence is also worth adding to the kernelModules description: the automatic NVIDIA set keys on the nvidia.com/ prefix alone, with no relation to whether the effective schematic actually carries those modules. In Talos v1.13 a module that cannot load leaves the kernel-module controller restart-backing-off rather than bricking the node, so it is a soft failure, but it is the same silent-mismatch class the schematic half of this PR exists to remove.

Not yours to fix here: talos.installerRepository and talos.version reach the same heredoc unvalidated and are unchanged from the merge base. I am tracking those separately.

…uard Review B3. `^[0-9a-f]{64}$` on the effective schematic encoded the public factory's naming as the only legal one, but the chart supports more: talos.imageFactoryURL documents a self-hosted factory, a caching mirror or an internal HTTP file server, and talos.installerRepository documents mirrored registries. On those the operator picks the path, and a readable name is the obvious choice. Verified against the merge base: values with schematicID `talos-gpu-nvidia-open` and an internal factory URL render on main and failed on this branch, so the guard stopped a cluster reconciling on a field its operator never touched. Replace it with `^[A-Za-z0-9._:-]+$`, which is the injection guard the comment claimed to be: it rejects $, backticks, quotes and whitespace, keeps the cluster-wide path closed, and leaves mirror naming alone. A wrong ID still 404s at the factory, visibly, on the DataVolume. Review B4. The validator walked .name and .parameters but the emit was toYaml over the raw user dict, so any other key was copied into the heredoc verbatim: `- {name: dummy, evil: "$(touch /tmp/pwned)"}` reached it in both charts. Nothing upstream prunes it either — `items` in values.schema.json has no additionalProperties: false, and the aggregated apiserver wires that schema into defaulting only, with no validation on Create or Update. Rebuild the emitted list from validated fields instead, so unvalidated input cannot reach the output by construction rather than by enumeration. Rendering for valid input is byte-identical. Review B5. Drop the review-iteration reference from the test comments; the next reader has no access to that thread. Also: pin mirror-style naming as accepted in both charts so B3 cannot regress, replace the now-legitimate not-64-hex case with one the class actually rejects, add the extra-key case in both charts, and document that the automatic NVIDIA set keys on the resource prefix alone with no cross-check against the effective schematic. Refs: cozystack#3563 Assisted-By: Claude <[email protected]> Signed-off-by: Mattia Eleuteri <[email protected]>

mattia-eleuteri · 2026-08-06T14:42:54Z

B3, B4 and B5 fixed in 88ae4de, plus the smaller items. Both blockers reproduced against the merge base first.

B3 — the guard was a format rule, not an injection guard

Confirmed the regression before changing anything: values carrying imageFactoryURL: http://images.internal.example.com/talos, installerRepository: registry.internal.example.com/talos/installer and schematicID: talos-gpu-nvidia-open render on main into .../image/talos-gpu-nvidia-open/v1.13.6/openstack-amd64.raw.xz and fail on the previous head. Same values, same command, only the checkout differs — so it broke a cluster on a field nobody touched, which is not a trade I should have made. I flagged the tightening and still misjudged it: I reasoned from the public factory's naming and treated the documented mirror and file-server paths as though they did not exist.

Now ^[A-Za-z0-9._:-]+$ on the effective value, as you proposed. It rejects $, backticks, quotes and whitespace, keeps the cluster-wide path closed, and leaves mirror naming alone. A wrong ID 404s at the factory exactly as before, on the DataVolume where it is visible.

Both charts now pin the mirror case as accepted — readable name, internal factory URL, mirrored installer repository, asserting both rendered image references — so this cannot regress into a format rule again. The not-64-hex case in schematic_per_pool_test.yaml was itself testing the wrong rule and is replaced by sch`whoami`, which the class does reject, mirrored into the parent chart's suite so the two are symmetric.

B4 — closed by construction

Reproduced: - {name: dummy, evil: "$(touch /tmp/pwned)"} rendered - evil: $(touch /tmp/pwned) inside the heredoc in both charts. I also checked the two upstream claims rather than take them: items in both values.schema.json files has no additionalProperties: false, and specSchema in pkg/registry/apps/application/ appears only in rest_defaulting.go and the constructor in rest.go, with no pruning or validation on the write path. So the render is the last gate, and enumerating fields was the wrong shape for the fix.

The emitted list is now rebuilt as dict "name" $name plus parameters when non-empty, from the validated values, so an unknown key cannot reach the output at all. Rendering for valid input is byte-identical — the explicit br_netfilter + parameters case and the render-parity comparison both still pass unchanged. A case in kernel_modules_test.yaml in each chart pins it: the extra key is absent and - name: dummy is still emitted.

B5 — dropped

Both files now open with "These values reach…". Point taken on the standard: a test comment referencing a review round is dead weight to whoever reads it next.

Smaller items

talos.schematicID's own description now says the effective per-group value is constrained to ^[A-Za-z0-9._:-]+$ and why, and that both a 64-hex digest and an operator-chosen mirror path satisfy it.
kernelModules now says the automatic set keys on the nvidia.com/ prefix alone with nothing cross-checking that the effective schematic carries those modules, and that on Talos v1.13 such a module leaves its controller retrying rather than failing the boot, so the symptom is a missing driver rather than a dead node. I took the v1.13 behaviour from your description and scoped the sentence to that version rather than stating it unqualified, since I have not verified it myself.
talos.installerRepository and talos.version left alone, as you said.

Ownership

Taking your call: the field stays here, and when #3294 rebases image.factory.schematicID falls back to nodeGroups.<name>.schematicID rather than replacing it, so the union stays canonical and nothing that shipped breaks. I have noted that on #3294 so it is not discovered at rebase time. Once it lands I will open the deprecation of the flat field rather than leave the redundancy undated.

State on this head: 207 tests in the parent chart, 23 in kubernetes-nodes, render-parity green including the machine-config comparison and the new schematic case. Rendering for a node group setting neither field is still byte-identical to main, KubevirtMachineTemplate and Job content hashes included, so nothing rolls.

mattia-eleuteri · 2026-08-06T14:59:17Z

Unit & controller tests is red on this head and it is not this PR. Filing the detail so nobody re-derives it.

The failing test is the unconverted bats files hold no more EXIT traps than they already did in hack/cozyreport.bats. hack/multus-install-cni-plugins.bats arrived on main in 7739d1e carrying 12 EXIT traps, one day before that ratchet was written, so the frozen list never learned about it. The guard fails on main itself, and this PR's CI builds a merge with main.

Reproduced against a pristine checkout of main at 879d0f6, with none of this branch: the guard's found set differs from frozen by exactly multus-install-cni-plugins.bats=12. Fix is a one-line count update, which is what the guard's own comment prescribes for a file that predates it — #3584. This PR goes green on its own once that lands; nothing to rebase here.

In the same run, everything else that matters passed: 70 helm suites with none failing, including all four suites this PR adds and GOLDEN PARITY on the render-parity check, plus pre-commit, Verify generated code is up to date, CodeQL and the API owner gate. Exactly one ❌ Test failed in the whole job, and it is the ratchet.

Separately, while probing the same class as B4 I checked the two remaining shapes an unenforced schema would let through, and both are already closed: Helm validates values against values.schema.json at render time, so a bare string in place of a module item (kernelModules: ["nvidia"]) and a scalar where parameters should be a list are both rejected before any template runs. What the schema does not reject is extra keys, because it carries no additionalProperties: false — which is exactly B4, and is now closed by construction rather than by enumeration. So the field is guarded on three independent axes: types by the schema, characters by the regex, keys by rebuilding the emitted list.

…odules The chart already sets NVreg_NvLinkDisable=1 for every tenant GPU cluster: the gpu-operator addon default turns on kernelModuleConfig, whose ConfigMap content is exactly that one line, and the operator's driver container appends it to the nvidia modprobe options. Cozystack passes individual GPUs into worker VMs without the NVSwitches, so without it the driver waits forever for an NVLink fabric that cannot come up: Fabric State stays "In Progress" and every CUDA call fails with "system not yet initialized". On Talos the driver comes from a system extension and that container is disabled, so nothing applies the parameter and machine.kernel.modules is the only place left to carry it. A GPU node group moved to Talos therefore loses it silently — the same class of quiet failure the rest of this branch removes. Add it to the automatic set so the Talos path reproduces what the chart already chose, rather than making every operator rediscover it. A no-op on a PCIe card with no NVLink. An explicit kernelModules list still replaces the set outright, so an operator who does have a fabric can drop it. Refs: cozystack#3563 Assisted-By: Claude <[email protected]> Signed-off-by: Mattia Eleuteri <[email protected]>

mattia-eleuteri · 2026-08-06T15:23:58Z

One more commit, 665c3f2, which changes something you already verified — flagging it rather than letting it slip past.

The automatic NVIDIA set now carries NVreg_NvLinkDisable=1 on the nvidia module. Reason: the chart already applies that parameter to every tenant GPU cluster today. cozystack.defaultGpuOperatorValues turns on kernelModuleConfig, whose ConfigMap content in packages/system/gpu-operator/values.yaml is exactly that one line, and the operator's driver container appends it to the nvidia modprobe options. Cozystack passes individual GPUs into worker VMs without the NVSwitches, so without it the driver waits forever for a fabric that cannot come up — Fabric State stays In Progress and every CUDA call fails with system not yet initialized.

On Talos that driver container is disabled (the driver comes from the system extension), so nothing applies the parameter and machine.kernel.modules is the only place left to carry it. A GPU node group moved to Talos loses it silently, which is the same class of quiet failure this PR exists to remove. So the automatic set reproduces what the chart already chose rather than making every operator rediscover it. It is a no-op on a PCIe card with no NVLink, and an explicit kernelModules list still replaces the set outright, so an operator who genuinely has a fabric can drop it.

Consequences in the diff: the pinned auto-set patterns in both charts now include the parameter, the GPU group's content-hash fixture in talos_templates_test.yaml moves again (the non-GPU one still does not), and the field descriptions name the parameter and say why Talos cannot inherit it. Rendering for a node group setting neither field is still byte-identical to main.

This is the first half of the gap 3 + gap 4 work from #3563. The second half — a gpu-operator addon default for the OS-provided-driver shape, which is what makes driver.enabled: false correct in the first place — is coming as its own PR against main, since it touches a different file and should not enlarge this one further.

Aleksei Sviridkin (lexfrei)

NOT LGTM — the schematic class I suggested last round is one character too narrow, and it rejects a value that renders on main. My recommendation, my miss; details below.

Everything else from the previous rounds is closed and I re-ran the checks rather than reading the diff. The allowlist closes B4 by construction: the extra-key payload now renders - name: dummy and nothing else, a nested extra.nested.deep: "$(...)" is dropped the same way, and swapping the rebuild back to deepCopy . turns kernel_modules_test.yaml red, so the new case is load-bearing. Both injection paths stay closed, including the effective-value one through the cluster-wide field. B5 is gone from both files. The schematic suites gained a positive case pinning a readable mirror name, so that boundary is now tested rather than incidental.

On 665c3f21: for a GPU node group setting neither field the KubevirtMachineTemplate hash is unchanged against main, so the parameter does not roll an existing GPU group by itself, and the non-GPU group stays byte-identical on both objects. Suites are green in both charts (207 and 23), GOLDEN PARITY passes, make generate leaves no drift.

Blocker

B6: the schematic class has no `/`, and a mirror path needs one

File: packages/apps/kubernetes/templates/_helpers.tpl:320, mirrored in packages/apps/kubernetes-nodes/templates/_helpers.tpl

Evidence: same values file at both revisions, imageFactoryURL: http://images.internal.example.com/talos, installerRepository: registry.internal.example.com/talos/installer, schematicID: gpu/nvidia-open. Main renders http://images.internal.example.com/talos/image/gpu/nvidia-open/v1.13.6/openstack-amd64.raw.xz and image: registry.internal.example.com/talos/installer/gpu/nvidia-open:v1.13.6, both well-formed, since an OCI repository path takes multiple segments. This head fails the render. That is the same regression class as B3, one character narrower, and the error text plus all four field descriptions call the value "the path an operator chose", which is exactly what a class without / forbids.

Fix, and the shape of it matters more than the character: / is inert in an unquoted heredoc, as are ;, |, & and parentheses. Only $, a backtick and a backslash are special there. So the guard is better written as a rejection of those three plus quotes and whitespace, rather than as an allowlist of permitted characters. An allowlist is right for kernelModules items because the set of legal keys is closed and the chart owns it, which is why B4's fix is correct. Here the set of legal values is open, it belongs to whatever registry or file server the operator runs, and every round of enumerating it has cost a legitimate configuration. Extend the existing positive case with a path-shaped value so the boundary is pinned.

The parameters class bars ;, and NVIDIA's own multi-value form needs it: NVreg_RegistryDwords="PowerMizerEnable=0x1;PerfLevelSrc=0x2222" is the documented shape, semicolon-separated key=value pairs read by the module at load time. Same reasoning as above, and worth widening while that class is being touched.

Two wordings over-claim slightly. "Byte-for-byte the same helper as the parent kubernetes chart's" is not literally true: the fail messages differ (nodeGroup %s: against pool %s:) and so do the comments. What is identical is the emitted output, which render-parity.sh proves. And "that container is disabled there" describes a configuration the chart does not produce: templates/helmreleases/gpu-operator.yaml sets only kernelModuleConfig.create and driver.kernelModuleConfig.name, so on Talos an operator still has to disable the driver container through addons.gpuOperator.valuesOverride. The conclusion holds, only the passive voice claims the chart already did it. That sentence lives in six generated copies, so it is one edit plus make generate.

Outside this PR

talos.installerRepository and talos.version reach the same unquoted heredoc unvalidated, unchanged at the merge base, and the support-matrix guard does not catch the version one because regexFind extracts the prefix and the membership check that follows tests the Kubernetes version rather than the Talos one. Not yours to fix here, and I am filing them separately, but the new field descriptions now assert the sink is constrained, which reads as broader coverage than exists.

The red Unit & controller tests is not this branch. I reproduced it on a pristine checkout of main at 879d0f6: the EXIT-trap ratchet in hack/cozyreport.bats differs from its frozen set by exactly multus-install-cni-plugins.bats=12, so every PR inherits it through the merge commit CI builds.

One non-blocking note for the terraform provider follow-up rather than for this chart: kernelModules []KernelModule with omitempty in the generated types cannot represent the [] opt-out, because an empty slice serialises away and becomes indistinguishable from unset. The in-cluster path is unaffected, since the CR spec travels as raw JSON and never passes through those structs, so applying YAML behaves as documented. A typed Go client building the object would silently lose the opt-out.

mattia-eleuteri · 2026-08-06T18:01:26Z

The NVreg_NvLinkDisable=1 addition from 665c3f2 is now verified on hardware, not just argued from the ConfigMap.

On a fresh single-node tenant cluster with an L40S and a machine.kernel.modules block byte-identical to what this branch renders for a nvidia.com/* node group:

$ cat /proc/modules | grep ^nvidia
nvidia_uvm 2068480 0 - Live
nvidia_drm 159744 0 - Live
nvidia_modeset 2150400 1 nvidia_drm, Live
nvidia 15994880 7 nvidia_uvm,nvidia_modeset, Live

$ grep -i nvlinkdisable /proc/driver/nvidia/params
NvLinkDisable: 1

So the parameter does reach the driver through the machine config, which is the part I could only infer before. The four modules load in the declared order, nvidia-cuda-validator reports Succeeded, the node advertises nvidia.com/gpu: 1, and a tenant pod gets GPU 0: NVIDIA L40S (UUID: …) from nvidia-smi -L. The node has held one bootID for 93 minutes, so nothing about the block destabilises the boot.

Two honest limits. The run cannot show the parameter is needed: an L40S is PCIe with no NVLink, so this establishes that it is harmless and that CUDA initialises with it — its necessity rests on the SXM passthrough case in production. And the modules were added by hand, since this branch is not released; what was verified is the rendered content, not the chart applying it.

Useful diagnostic side-note, since /proc is where this is visible: /proc/modules and /proc/driver/nvidia/params are not namespaced, so a plain unprivileged pod can read both. /sys/module/nvidia/parameters/* cannot be read without root, which is the wrong place to look.

Also for the record: that cluster has a single GPU node group, and it does not show the ~70 minute reboot loop — consistent with the loop being confined to node groups whose nodes do not match the cluster-wide schematic, which is what the schematicID half of this PR exists to fix.

… class Review B6. `^[A-Za-z0-9._:-]+$` has no `/`, and a mirror path needs one: with an internal factory URL and a mirrored installer repository, a schematicID of `gpu/nvidia-open` renders `http://images.internal.example.com/talos/image/gpu/nvidia-open/...` and `installer/gpu/nvidia-open:v1.13.6` on main, both well-formed, because an OCI repository path takes several segments and so does a URL path. This branch rejected it. Same regression class as B3, one character narrower. The shape was the mistake, not the character. Inside an unquoted heredoc only $, a backtick and a backslash are special — verified: `/ ; | & ( )` and spaces pass through as text. So both guards now deny those three plus quotes, whitespace and control characters, rather than permitting an enumerated set. An allowlist stays correct for kernelModules item keys, where the set is closed and the chart owns it, which is why B4's fix is built that way; the schematic's set is open and belongs to whatever registry or file server the operator runs, and enumerating it cost a legitimate configuration twice. Control characters are excluded because `\s` covers only tab, newline, form feed, carriage return and space. A bare one otherwise slips through and surfaces later as `yaml: control characters are not allowed`, pointing at the template rather than at the value. Also per the review, the `parameters` class barred `;`, which NVIDIA's documented multi-value form needs: NVreg_RegistryDwords=PowerMizerEnable=0x1;PerfLevelSrc=0x2222. It takes the same denylist for the same reason. Two wordings corrected. The nodes chart claimed its helpers were "byte-for-byte the same" as the parent's; the fail messages and comments differ, and what is identical is the emitted output, which render-parity.sh proves. And the NvLinkDisable rationale said the driver container "is disabled" on Talos, which the chart does not do — an operator turns it off through addons.gpuOperator.valuesOverride today. The schematic descriptions also no longer read as though every value reaching that heredoc were guarded; only this field is. New cases pin the boundary in both charts: a multi-segment mirror path is accepted, and a semicolon-bearing module parameter renders. Refs: cozystack#3563 Assisted-By: Claude <[email protected]> Signed-off-by: Mattia Eleuteri <[email protected]>

mattia-eleuteri · 2026-08-07T06:08:54Z

B6 fixed in c1ea547, along with both recommendations and both wording corrections. Verified your evidence before changing anything, and the mistake was the shape, not the character — twice now, which is the actual lesson.

B6 — denylist, not a class

Reproduced: with imageFactoryURL: http://images.internal.example.com/talos, installerRepository: registry.internal.example.com/talos/installer and schematicID: gpu/nvidia-open, main renders .../talos/image/gpu/nvidia-open/v1.13.6/openstack-amd64.raw.xz and installer/gpu/nvidia-open:v1.13.6, and the previous head failed the render.

Checked the premise rather than taking it: printf 'cat <<EOF\nA: a/b;c|d&e(f) g\nB: $(echo SUBST)\nC: echo BACKTICK\nEOF\n' | sh prints the first line verbatim and expands only the last two. So / ; | & ( ) and spaces are inert and $, backtick, backslash are not. Both guards now deny those three plus quotes and whitespace, instead of permitting an enumerated set. The allowlist stays where you said it belongs — kernelModules item keys, a closed set the chart owns.

One addition beyond your fix, from testing it: \s covers only tab, newline, form feed, carriage return and space, so a bare control character passed the class and surfaced later as yaml: control characters are not allowed, pointing at the template rather than at the value. [:cntrl:] is in the class now. Not a shell concern — it corrupts the YAML the heredoc carries, which is the other thing these guards are for.

Worth recording how I nearly reported the wrong result here: my first pass tested the backslash through --set-string and then through a values file, and both times the shell and YAML ate the escape before Helm saw it, so it read as ACCEPTED. It is rejected; a five-line template evaluating regexMatch against a literal sch\back returns false. The harness was wrong, not the guard.

Recommended, both taken

parameters takes the same denylist, so NVreg_RegistryDwords=PowerMizerEnable=0x1;PerfLevelSrc=0x2222 renders — pinned by a new case.

Both over-claims corrected. The nodes chart no longer says "byte-for-byte the same helper": the fail messages differ (pool against nodeGroup) and so do the comments, and what is identical is the emitted output, which render-parity.sh proves. And the NvLinkDisable rationale no longer says the driver container "is disabled" on Talos, since the chart does not do that — an operator turns it off through addons.gpuOperator.valuesOverride today, which is exactly what #3585 changes. That sentence lives in the generated copies, so it went through make generate.

On your point about the descriptions reading as broader coverage than exists: fixed too. They now say the guard covers this field only, not every value reaching that heredoc.

Boundaries now pinned rather than incidental

Both charts accept a multi-segment mirror path and a semicolon-bearing parameter, and still reject $(id), a backtick, a backslash, quotes and whitespace on the effective schematic. Suites green in both charts (208 and 25), GOLDEN PARITY passes, make generate idempotent, and a node group setting neither field still renders byte-identically to main including both content hashes.

The Go `omitempty` note

Good catch, and it is the sharper half of the terraform follow-up rather than a chart issue. Recorded on cozystack/terraform-provider-cozystack#20: []KernelModule with omitempty cannot represent the [] opt-out, so a typed Go client building the object drops it silently, while the YAML path is unaffected because the CR spec travels as raw JSON and never passes through those structs. That makes the provider's flatten side the place where the three states have to be preserved deliberately.

Also confirming your ratchet finding independently reproduced mine — #3584 is the one-line fix for it.

….bats (#3584)  ## What this PR does `hack/multus-install-cni-plugins.bats` arrived on main in 7739d1e carrying 12 EXIT traps, one day before the EXIT-trap ratchet in `hack/cozyreport.bats` was written, so the frozen list never learned about it. The guard therefore fails on main itself, and on every PR whose CI builds a merge with main — `make bats-unit-tests` exits 1 with: ``` FAIL: the set of unconverted EXIT-trap files changed. frozen: ... nightly-mirror_test.bats=5 ... found: ... multus-install-cni-plugins.bats=12 nightly-mirror_test.bats=5 ... ``` This records the count, which is what the guard's own comment prescribes for exactly this case: *"a file that did not exist when this guard was written arrives carrying its own \[traps\] ... Counts are updated rather than the files converted: all of them are owned by other branches, and a conflict there costs more than an uncovered trap."* Converting the file is not in scope here and belongs to whoever owns the multus branch. Verified by reproducing the guard's logic against a pristine checkout of `main` at 879d0f6: the `found` set differs from `frozen` by exactly this one entry before the change, and matches after it. Found while investigating a red `Unit & controller tests` on #3571, which carries none of this. ### Screenshots No UI changes. ### Screenshots  ### 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: ### Release note  ```release-note NONE ```  ## Summary by CodeRabbit * **Tests** * Updated the expected EXIT-trap inventory to include `multus-install-cni-plugins.bats`.

Aleksei Sviridkin (lexfrei)

LGTM.

B6 is closed in both charts, and I checked it by rendering rather than by reading the diff. With imageFactoryURL: http://images.internal.example.com/talos and installerRepository: registry.internal.example.com/talos/installer, gpu/nvidia-open renders http://images.internal.example.com/talos/image/gpu/nvidia-open/v1.13.6/openstack-amd64.raw.xz and registry.internal.example.com/talos/installer/gpu/nvidia-open:v1.13.6 (the same output main produces), and so do talos-gpu-nvidia-open, deadbeef and a 64-hex digest, in packages/apps/kubernetes and packages/apps/kubernetes-nodes alike. The rejection side holds: $(...), a backtick, a backslash, both quote characters, a space, a real tab, a real newline and a bare control character all fail the render, each naming the group or the pool. ; | & ( ) > ~ * pass, which is the point of the reshape.

The denylist is also complete for the sink it guards, not merely wider than the last one. Inside an unquoted here-document the shell performs parameter expansion, command substitution and arithmetic expansion, and \ keeps its meaning only before $, a backtick, \ or a newline, so those three are the whole set, and everything else in the class is there for YAML integrity rather than for the shell. Validating the effective value keeps the cluster-wide path closed; talos.schematicID: nvidia$(id) with no per-group override still fails.

Both guards are pinned in both directions and in both charts. Reverting the schematic class to ^[A-Za-z0-9._:-]+$ (last round's suggestion, the one missing /) turns schematic_per_nodegroup_test.yaml and schematic_per_pool_test.yaml red; widening both guards to ^.*$ turns all four suites red. As they stand the suites are green (208 and 25), GOLDEN PARITY passes, and make generate in both packages leaves no drift.

The parity script's new machine-config comparison is a real gate, which matters more than usual here because it is the only thing holding the two copies of the helper together. Adding one rendered line to the nodes chart's kernel: block fails it on three cases, and cutting the block out entirely fails the same three. Worth knowing for whoever tests it next: a {{- /* ... */}} comment renders nothing and so passes, which is the check being right rather than asleep. I also diffed the two helper bodies by hand. They differ only in nodeGroup against pool in the fail messages.

NVreg_NvLinkDisable=1 lands where it should and nowhere else. It appears only when the group resolves to the automatic set; an explicit list is emitted verbatim with nothing added, [] still emits no kernel block on a GPU group, and a non-NVIDIA vendor still gets nothing. Against the merge base with neither new field set, the only difference in the entire render is the GPU group's talos-reconcile Job: new content hash, new module block. Every KubevirtMachineTemplate name is unchanged and the non-GPU group is byte-identical, so no worker is rolled. The hardware run answers the half I cannot reach by rendering, which is whether the parameter actually arrives at the driver.

Both wording corrections landed, in all twelve files carrying the description, and the schematic descriptions now say the guard covers that field alone.

On the semicolon: you took the bare form and I had quoted the modprobe.conf spelling at you, which is the wrong shape for a YAML list element. NVreg_RegistryDwords=PowerMizerEnable=0x1;PerfLevelSrc=0x2222 renders, the field description and the error text both prescribe it, and the two agree. Nothing owed.

The semicolon regression is pinned in one chart out of two. kernel_modules_test.yaml in kubernetes-nodes gained accepts a semicolon-separated module parameter; the parent chart has no equivalent. I narrowed only the parent's parameters class back to bar ; (the exact defect that case exists to prevent), and the parent suite stayed 208/208, the nodes suite stayed 25/25, and GOLDEN PARITY passed. The parity script does not cover it either, since its kernelmodules case carries nf_conntrack_helper=0. It is the same asymmetry you closed on the schematic side when the parent gained the readable-name case, and the same fifteen lines in the other file.

Two claims in the PR body have drifted from the branch. "New kernel_modules_test.yaml in both packages, 6 cases each" is 9 in the parent and 10 in the nodes chart now. And "reordering the module list in one chart's helper only makes the new MachineConfig(talos-reconcile) assertion fail, for the GPU case only" is off too: reordering the nodes chart's automatic set fails on gpu and on schematic, because the schematic case also carries an nvidia.com/* GPU and inherits the same automatic list. The body becomes the merge commit, so both are worth a pass before this leaves draft.

CI

None of the red is this branch. All twenty-seven Build packages/* jobs die at exporting to image with failed to push iad.ocir.io/..., on packages this PR does not touch (mariadb, clickhouse and metallb among them), so it is a registry-side failure and a re-run question. Unit & controller tests is still the EXIT-trap ratchet in hack/cozyreport.bats that every PR inherits through the merge with main. The run dates from the day the head commit was pushed, and nothing since asks for a code change here.

Resolves against 3a1292d (escape tenant values in the worker reconcile heredoc) and 58910c0 (registry mirror passthrough), both of which edit the machine-config block this branch also touches. The guard and the escaping compose rather than compete, so both are kept at every site: - The installer image line takes main's escape chain on all three coordinates and this branch's `kubernetes.schematicID` include, so the per-node-group schematic still resolves and the value is still escaped on the way into the heredoc. - `nodegroup.yaml` keeps both new group keys, `logSerialConsole` and `schematicID`. - `render-parity.sh` keeps main's guest-console-log case alongside the kernel-module, opt-out and schematic cases, and keeps this branch's machine-config comparison, which supersedes the "four pool objects" scope note it replaced. Two cases in hack/talos-reconcile-heredoc_test.bats fed a hostile `talos.schematicID` and asserted the heredoc kept it literal. That value is now refused at render time by this branch's guard, which the INVARIANT above the data block admits as the alternative to escaping. The cases keep their hostile `installerRepository` and `version`, which only the escaping protects, and the schematicID half moves to a new pair of cases asserting the render is refused and no heredoc is produced. Its escape chain stays in the template as a second line of defence. The default render of the reconcile Job is byte-identical to main's, so the merge rolls no worker. Assisted-By: Claude <[email protected]> Signed-off-by: Mattia Eleuteri <[email protected]>

…t too The kubernetes-nodes chart carries `accepts a semicolon-separated module parameter`; the parent chart had no equivalent, so the exact defect that case exists to prevent was pinned in one chart out of two. render-parity does not cover it either: its kernelmodules case uses `nf_conntrack_helper=0`, which carries no semicolon. Load-bearing check: narrowing the parent's `parameters` class to an allowlist that bars `;` turns the new case red on the guard's own error message, and restoring it leaves the suite at 223 green. Assisted-By: Claude <[email protected]> Signed-off-by: Mattia Eleuteri <[email protected]>

mattia-eleuteri · 2026-08-10T11:47:49Z

Merged main and resolved. The guard and the escaping are kept at every site rather than one side being picked.

The installer image line now carries both: main's escape chain on all three coordinates, and this branch's kubernetes.schematicID include so the per-node-group schematic still resolves. nodegroup.yaml keeps both new group keys (logSerialConsole and schematicID). render-parity.sh keeps main's guest-console-log case alongside the kernel-module, opt-out and schematic cases, and keeps this branch's machine-config comparison, which supersedes the "four pool objects" scope note it replaced.

One thing worth flagging because it touches a file from #3513 rather than one this PR owns. Two cases in hack/talos-reconcile-heredoc_test.bats set a hostile talos.schematicID and asserted the heredoc kept it literal. That value is now refused at render time by this branch's guard, so those two cases went red on the merge. The INVARIANT above the data block admits escaping or render-time validation, and schematicID now takes the second route, so the resolution was to keep the hostile installerRepository and version in those cases (they only have the escaping) and move the schematicID half to a new pair of cases asserting the opposite property: the render is refused and no heredoc is produced at all. The escape chain on schematicID stays in the template as a second line of defence, unreachable for hostile input while the guard holds. Happy to reshape that if you would rather the bats file stayed as you wrote it.

Checks after the merge: 223 tests in packages/apps/kubernetes, 31 in packages/apps/kubernetes-nodes, all 7 cases in hack/talos-reconcile-heredoc_test.bats, GOLDEN PARITY clean, make generate leaves no drift in either package and api/apps/v1alpha1 builds with every type covered by a deepcopy. The default render of the reconcile Job is byte-identical to main's, so the merge rolls no worker.

Also took the two recommended items. The parent chart gained accepts a semicolon-separated module parameter, the case that existed only in kubernetes-nodes; narrowing the parent's parameters class to an allowlist barring ; turns it red on the guard's own error message, so it is load-bearing. And both drifted claims in the body are corrected: 10 cases each rather than 6, and the reordering check fails on gpu and schematic rather than the GPU case alone, which I reproduced by swapping nvidia_drm and nvidia_modeset in the nodes chart's automatic set.

coderabbitai

Actionable comments posted: 4

🧹 Nitpick comments (3)

packages/apps/kubernetes-nodes/tests/schematic_per_pool_test.yaml (1)
103-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a case that pins validation of the cluster-wide talos.schematicID.

The comment states that validating the effective value also closes the pre-existing path through talos.schematicID. No test covers that path. A future change that moves the guard to the per-pool override only would keep every test green.
🧪 Proposed extra case
+  - it: rejects an unsafe cluster-wide schematicID inherited by the pool
+    set:
+      talos:
+        schematicID: "$(id)"
+    asserts:
+      - failedTemplate:
+          errorPattern: 'invalid schematicID'
+        template: templates/talos-reconcile-job.yaml
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/apps/kubernetes-nodes/tests/schematic_per_pool_test.yaml` around
lines 103 - 115, Extend the schematic validation tests with a case that sets the
cluster-wide talos.schematicID to a shell-injection value while leaving the
per-pool override unset. Assert Helm rendering fails with the existing “invalid
schematicID” error through templates/talos-reconcile-job.yaml, proving
validation covers the effective cluster-wide value.
packages/apps/kubernetes/templates/_helpers.tpl (1)
240-246: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider guarding against a non-list parameters value.

range .parameters | default list fails the render with a Go template error if a user sets parameters as a string or map. The schema declares an array, but the aggregated apiserver does not validate on write, per the comment at lines 213-221. A kindIs "slice" check would produce the same clear message as the other guards.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/apps/kubernetes/templates/_helpers.tpl` around lines 240 - 246,
Guard the parameters handling in the node-group validation before ranging over
`.parameters` by checking that it is a slice with `kindIs "slice"`. For
non-slice values, fail using the same clear invalid-kernelModules-parameter
message pattern as the existing guards; retain the current validation and append
behavior for valid lists.
api/apps/v1alpha1/kubernetes/zz_generated.deepcopy.go (1)
340-359: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Regenerate the deep-copy files from controller-gen source instead of editing them.

hack/update-codegen.sh is the repo generation entrypoint, and these api/apps/v1alpha1/kubernetes/zz_generated.deepcopy.go and api/apps/v1alpha1/kubernetesnodes/zz_generated.deepcopy.go files are controller-gen output. Avoid leaving manual edits in generated Go files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/apps/v1alpha1/kubernetes/zz_generated.deepcopy.go` around lines 340 -
359, Regenerate the DeepCopyInto and DeepCopy implementations using
hack/update-codegen.sh and the controller-gen source definitions instead of
manually editing generated output. Apply this to
api/apps/v1alpha1/kubernetes/zz_generated.deepcopy.go at lines 340-359 and
432-438, and api/apps/v1alpha1/kubernetesnodes/zz_generated.deepcopy.go at lines
68-74 and 120-139; commit only the resulting generated changes.
Source: Coding guidelines

🤖 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 `@api/apps/v1alpha1/kubernetes/types.go`:
- Around line 281-282: Remove omitempty from NodeGroup.KernelModules in
api/apps/v1alpha1/kubernetes/types.go (lines 281-282) and
ConfigSpec.KernelModules in api/apps/v1alpha1/kubernetesnodes/types.go (lines
51-52) so an explicit [] remains serialized as opt-out; then regenerate the
generated deepcopy outputs.

In `@packages/apps/kubernetes-nodes/README.md`:
- Line 45: Update the generator input for kernelModules so its three-state
behavior is explicit: unset/automatic selects chart defaults, while [] is an
explicit opt-out. Regenerate both generated tables:
packages/apps/kubernetes-nodes/README.md:45-45 and
packages/apps/kubernetes/README.md:126-126, ensuring neither presents [] as the
sole default; do not edit the generated READMEs manually.

In `@packages/apps/kubernetes/README.md`:
- Line 229: Keep the Phase 1 contract for talos.registryMirrors consistent
across packages/apps/kubernetes/README.md:229-229 and
packages/system/kubernetes-rd/cozyrds/kubernetes.yaml:35-35. Since per-tenant
registries.mirrors has no consumer before Phase 2, remove or mark
talos.registryMirrors unsupported in both the documentation and generated
schema, unless implementing the consumer and restoring the Phase 2 behavior is
intended.

In `@packages/system/kubernetes-nodes-rd/cozyrds/kubernetes-nodes.yaml`:
- Line 41: Add ["spec", "kernelModules"] to the keysOrder list in the Kubernetes
node configuration, placing it immediately after ["spec", "schematicID"] and
before ["spec", "kubelet"], while preserving the existing ordering.

---

Nitpick comments:
In `@api/apps/v1alpha1/kubernetes/zz_generated.deepcopy.go`:
- Around line 340-359: Regenerate the DeepCopyInto and DeepCopy implementations
using hack/update-codegen.sh and the controller-gen source definitions instead
of manually editing generated output. Apply this to
api/apps/v1alpha1/kubernetes/zz_generated.deepcopy.go at lines 340-359 and
432-438, and api/apps/v1alpha1/kubernetesnodes/zz_generated.deepcopy.go at lines
68-74 and 120-139; commit only the resulting generated changes.

In `@packages/apps/kubernetes-nodes/tests/schematic_per_pool_test.yaml`:
- Around line 103-115: Extend the schematic validation tests with a case that
sets the cluster-wide talos.schematicID to a shell-injection value while leaving
the per-pool override unset. Assert Helm rendering fails with the existing
“invalid schematicID” error through templates/talos-reconcile-job.yaml, proving
validation covers the effective cluster-wide value.

In `@packages/apps/kubernetes/templates/_helpers.tpl`:
- Around line 240-246: Guard the parameters handling in the node-group
validation before ranging over `.parameters` by checking that it is a slice with
`kindIs "slice"`. For non-slice values, fail using the same clear
invalid-kernelModules-parameter message pattern as the existing guards; retain
the current validation and append behavior for valid lists.

🪄 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: 29c0e880-7cd9-422a-b4db-08b830ff9f7d

📥 Commits

Reviewing files that changed from the base of the PR and between cfaf2e0 and 63f6485.

📒 Files selected for processing (25)

api/apps/v1alpha1/kubernetes/types.go
api/apps/v1alpha1/kubernetes/zz_generated.deepcopy.go
api/apps/v1alpha1/kubernetesnodes/types.go
api/apps/v1alpha1/kubernetesnodes/zz_generated.deepcopy.go
hack/talos-reconcile-heredoc_test.bats
packages/apps/kubernetes-nodes/README.md
packages/apps/kubernetes-nodes/templates/_helpers.tpl
packages/apps/kubernetes-nodes/templates/nodegroup.yaml
packages/apps/kubernetes-nodes/templates/talos-reconcile-job.yaml
packages/apps/kubernetes-nodes/tests/kernel_modules_test.yaml
packages/apps/kubernetes-nodes/tests/render-parity.sh
packages/apps/kubernetes-nodes/tests/schematic_per_pool_test.yaml
packages/apps/kubernetes-nodes/values.schema.json
packages/apps/kubernetes-nodes/values.yaml
packages/apps/kubernetes/README.md
packages/apps/kubernetes/templates/_helpers.tpl
packages/apps/kubernetes/templates/cluster.yaml
packages/apps/kubernetes/templates/talos/talos-reconcile-job.yaml
packages/apps/kubernetes/tests/kernel_modules_test.yaml
packages/apps/kubernetes/tests/schematic_per_nodegroup_test.yaml
packages/apps/kubernetes/tests/talos_templates_test.yaml
packages/apps/kubernetes/values.schema.json
packages/apps/kubernetes/values.yaml
packages/system/kubernetes-nodes-rd/cozyrds/kubernetes-nodes.yaml
packages/system/kubernetes-rd/cozyrds/kubernetes.yaml

Aleksei Sviridkin (lexfrei) · 2026-08-14T08:48:32Z

NOT LGTM. Two things, both new since the last round: the merge of main dropped a paragraph main owns, and the kernelModules field description states a property that stops being true the moment #3523 lands.

Everything I checked from the previous rounds still holds. The helm suites are green (223 in kubernetes, 31 in kubernetes-nodes), render-parity.sh reports GOLDEN PARITY on all seven cases, the heredoc suite passes all seven of its tests, and make generate in both packages plus controller-gen object for the api submodule produce no diff.

B1: the merge of main reverted the air-gapped bullet in `packages/apps/kubernetes/README.md`

The merge commit resolved README.md by keeping the branch side, so the paragraph #3575 rewrote is gone and the pre-#3575 text is back. Relative to its branch parent the merge added 2 lines to that file; relative to main it changed 39 and removed 35.

$ git show origin/main:packages/apps/kubernetes/README.md | grep -c 'talos.registryMirrors` maps an upstream registry host'
1
$ git show HEAD:packages/apps/kubernetes/README.md       | grep -c 'talos.registryMirrors` maps an upstream registry host'
0
$ git show origin/main:packages/apps/kubernetes/README.md | grep -c 'Phase 2 follow-up'
0
$ git show HEAD:packages/apps/kubernetes/README.md       | grep -c 'Phase 2 follow-up'
1

So the shipped README now tells operators that per-tenant registry mirrors "remain a Phase 2 follow-up, file an issue if you depend on this and it is not yet landed", while talos.registryMirrors is shipped, is in the parameter table two screens below, and is in the RD schema. The bot flagged the inconsistency at README:229 and proposed removing talos.registryMirrors from the docs and the schema; that is backwards, the prose is the stale half. Restore main's paragraph.

I checked whether anything else was lost in that merge and nothing was: the set of files where the merge result differs from main is exactly the set the PR declares it touches, and in packages/apps/kubernetes-nodes/README.md only generated table rows changed.

B2: the `kernelModules` description is falsified by #3523, and it ships in the CRD

The description says, in ten files including both cozyrds ResourceDefinitions:

Changing this on a node group that already has running workers does not reach them: the TalosConfigTemplate its MachineDeployment references has a fixed name, so rewriting the template leaves spec.template untouched, CAPI starts no rollout, and a running Machine keeps the machine config it booted with. Replace the group's Machines to apply it. Unlike schematicID, which changes the boot image and therefore rolls the group by itself.

That is true on today's main and false with #3523 in the tree. I merged the two branches, ported the two hunks into the shared _talosconfigtemplate.tpl, and read the name off MachineDeployment.spec.template.spec.bootstrap.configRef:

tree	node group	TalosConfigTemplate
#3523 only	one `nvidia.com/*` GPU	`t-md0-52cee2`
#3523 + this PR, `kernelModules` unset	same	`t-md0-cc5ae0`
#3523 + this PR, `kernelModules: []`	same	`t-md0-52cee2`
#3523 only	no GPU	`t-md0-35293f`
#3523 + this PR	no GPU	`t-md0-35293f`

With both in a release, the automatic NVIDIA set rotates the content-hashed template name of every existing GPU node group with no operator action, and CAPI rolls those workers. kernelModules then rolls the group by itself, exactly like schematicID. Non-GPU groups are untouched, and [] reproduces the pre-change hash byte for byte, which is the opt-out working as designed.

The release note carries the #3515 caveat and says it disappears when the naming fix ships. The field description does not: it states the fixed-name mechanism as a permanent property of the system, and it is the text an operator reads in the dashboard long after the release note is history. Either drop the mechanism and say the change may not reach existing workers until the TalosConfigTemplate naming fix is released, or qualify it in place.

Recommendations

The release note says setting kernelModules on an existing node group has no effect before #3523. It does not say the automatic set fires unasked. On this head a nvidia.com/* group's reconcile Job name moves from t-talos-reconcile-md0-752f85 to t-talos-reconcile-md0-80ebc9, so an upgrade creates a fresh Job for every existing GPU node group and that Job applies a changed TalosConfigTemplate under the fixed name. The script runs under sh -ec and the apply carries no tolerance, so if the immutability webhook rejects it the way #3515 describes, the Job exits non-zero, retries to backoffLimit: 30 and ends Failed. I could not exercise the webhook here, so treat the rejection as read from #3515 rather than measured. One sentence in the release note covers it.

invalid schematicID "" names a rule the value does not break. --set talos.schematicID= renders on the merge base (producing image//v1.13.6/openstack-amd64.raw.xz, which 404s at the factory) and now fails the render with "must not contain $, a backtick, a backslash, quotes or whitespace". Refusing it is right; the message should say the value must also be non-empty.

treats an empty per-node-group schematic as unset asserts only the boot disk image. Its two sibling cases assert both consumers, and the whole point of routing both through one helper is that they cannot disagree. One more assert on the installer image closes it.

^[a-z0-9_-]+$ rejects an uppercase module name. Nothing in the kernel forbids one; the convention is lowercase and the guard is otherwise correct, so this is a note rather than a request.

Sequencing with #3523

The two branches conflict in four files, so whoever merges second resolves by hand, and the interesting part is what a plausible resolution does. I took #3523's side wherever it had rewritten a region, which is the obvious call since it moved the machine config out of the Job template. packages/apps/kubernetes-nodes/templates/nodegroup.yaml auto-merged and kept this PR's "schematicID" line, while the Job's own $group dict came from #3523's side and lost both new keys. The two sites then hash different specs:

MachineDeployment configRef -> kubernetes-myk8s-md0-4522a1
Job TCT_NAME                -> kubernetes-myk8s-md0-ffdbe1

That is the cannot create a new MachineSet when templates do not exist deadlock, produced by a resolution nobody would call careless. The good news is that this PR's own tests catch it: render-parity.sh fails on MachineDeployment and on MachineConfig(talos-reconcile), and both kubernetes-nodes suites go red. Worth knowing before the rebase rather than during it.

Two smaller integration details for that rebase. #3523's kubernetes.talosConfigTemplateSpec takes root and group but no groupName, which both helpers here need for their fail messages. And nodegroup.yaml's $group will need kernelModules added, because after #3523 that dict feeds the template hash rather than only the KubevirtMachineTemplate.

What I verified

No worker roll for anything that resolves to no modules: rendering the parent chart at the merge base and at this head, for the default node group and for an amd.com/gpu group, the only difference is the six per-render random Talos secret lines. The nvidia.com/* group differs by the Job hash and the eight-line kernel: block, which is the intended change.

The tests are not decorative. I broke each thing a test claims to guard and every one went red: reordering the module list in one chart's helper alone fails MachineConfig(talos-reconcile) on the gpu and schematic cases and nowhere else; dropping the schematic guard turns the new heredoc test red; kindIs "slice" replaced by truthiness, the module-name regex removed, and the rebuilt dict replaced by the raw item each fail exactly one case; removing the kernel: block or gating it on true fails five and three; nindent 28 moved to 26 or 30 fails five, so the absolute indentation really is pinned in both directions; routing the installer image or the boot disk URL around the helper fails five and four cases of the schematic suite. The machine-config comparison is also not a no-op: run against main's two unmodified charts it passes.

The three-state contract holds at the schema layer too. A bare kernelModules: is refused by Helm with at '/nodeGroups/md0/kernelModules': got null, want array, and [] is accepted, which is what the undefaulted field buys.

Every consumer of the schematic goes through the helper. There is no remaining .Values.talos.schematicID in a rendered position in either chart; the four call sites are the two boot disk URLs and the two installer images.

Aleksei Sviridkin (lexfrei)

Two blockers, both in the delta since the last approval rather than in the earlier work.

First, the merge commit resolved packages/apps/kubernetes/README.md in favour of the branch and dropped a paragraph that belongs to main. The shipped README now says per-tenant registry mirrors "remain a Phase 2 follow-up", while talos.registryMirrors landed in #3575 and is documented in the parameter table of that same README. Measured both ways: the sentence about mapping an upstream registry host is present on main and absent on this head, and "Phase 2 follow-up" is the reverse. Nothing else was lost in that merge.

Second, the description of kernelModules becomes false the moment #3523 lands, and it ships in the CRD, so it is what an operator reads in the dashboard. It states that the fixed TalosConfigTemplate name means CAPI starts no rollout and that machines must be replaced, unlike schematicID. With both changes in a release, the automatic NVIDIA set turns the content hash of every existing GPU node group, with no operator action, and CAPI rolls those workers. Measured by assembling both branches in one tree: the same group renders t-md0-52cee2 with 3523 alone and t-md0-cc5ae0 with both, returning to 52cee2 when kernelModules is set to an empty list. On today's main the text is true, which is why this is worth fixing now rather than after.

The branches conflict textually in four files, so nothing merges silently, but the obvious resolution in favour of 3523 drops both new keys from the Job dict while nodegroup.yaml keeps them, producing two different hashes. The suites of this PR catch that. Details in the comment above.

Aleksei Sviridkin (lexfrei) · 2026-08-19T11:23:06Z

Checked both PRs for a re-review pass today. The branches haven't moved since Aug 10, so the Aug 14 reviews stand: the hash-input duplication in #3523 and the README/values claims here are still open. This branch has also picked up merge conflicts with main in 6 files since then. Happy to re-review as soon as a new revision lands — flagging in case the review notifications got lost.

Upstream landed refactor(kubernetes): remove worker node pools from the chart, so `nodeGroups` no longer exists in packages/apps/kubernetes and its talos-reconcile Job template is gone. The parent-chart half of this branch therefore has nowhere to land, and is dropped rather than merged: - nodeGroups[name].kernelModules and nodeGroups[name].schematicID, with their _helpers.tpl and talos-reconcile-job.yaml plumbing - packages/apps/kubernetes/tests/kernel_modules_test.yaml and schematic_per_nodegroup_test.yaml - the talos_templates_test.yaml hash assertions they moved - tests/render-parity.sh and its machine-config comparison, replaced upstream by tests/render_snapshot_test.yaml now that there is only one chart rendering a worker pool and nothing left to compare packages/apps/kubernetes, packages/system/kubernetes-rd, api/apps/v1alpha1/kubernetes and hack/ are taken from upstream verbatim and carry no change from this branch. What survives is the whole feature, on the one chart that still renders a worker pool: kernelModules and schematicID at the root of kubernetes-nodes, where they are pool-scoped by construction. The upstream golden render snapshot passes unmodified, which is the property the deleted parity script was there to protect: a pool that sets neither field renders exactly what upstream renders. Assisted-By: Claude <[email protected]> Signed-off-by: Mattia Eleuteri <[email protected]>

…able cannot The generated parameter table prints `[]` in the Default column for `kernelModules`. That is cozyvalues-gen printing what it prints for any array, not a default the schema carries -- `values.schema.json` gives the field no `default`, deliberately, because the three states are only distinguishable while it has none. For every other field that reads as harmless. For this one it inverts the meaning: `[]` is the explicit opt-out, so a table saying the default is `[]` tells the reader that a GPU pool loads no modules unless they ask, which is the opposite of what the chart does. The Default column is not reachable from here -- it is the generator's, and giving the field a default to make the column right would collapse unset into opted-out for every pool and make the automatic NVIDIA set unreachable. The description is reachable, so it now states that the field has no default and that the `[]` in the column is a placeholder, next to the sentence that defines the three states. Regenerated README.md, values.schema.json, the kubernetesnodes Go types and the kubernetes-nodes ApplicationDefinition. Assisted-By: Claude <[email protected]> Signed-off-by: Mattia Eleuteri <[email protected]>

coderabbitai · 2026-09-03T08:45:34Z

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.

…ender The merge took upstream's talos-reconcile-heredoc_test.bats verbatim and lost the adaptation this branch carried, so CI failed on the first case: it sets talos.schematicID to `sch$(id)`q`z` and asserts the value reaches the heredoc and survives literally, while this branch refuses that render outright. The INVARIANT above the data block admits two protections, escaping or render-time validation. talos.version and talos.installerRepository have only the first, so for them the assertion stays what upstream wrote: the heredoc emits and the value survives verbatim. schematicID now has the second as well, so the execution-level assertion inverts -- no heredoc is produced at all -- and asserting a hostile value renders literally would pin behaviour the guard exists to prevent. So the coordinates case keeps schematicID at the same site with a benign multi-segment value (`gpu/nvidia-open`, which the guard admits), and two new cases pin the refusal itself: once through the cluster-wide talos.schematicID and once through the pool-level schematicID this PR adds, since the guard runs on the effective value and a pool override that skipped it would be a hole. The escape chain stays in the template as a second line of defence, unreachable for hostile input while the guard holds. hack/cozytest.sh hack/talos-reconcile-heredoc_test.bats: 4 passed. Assisted-By: Claude <[email protected]> Signed-off-by: Mattia Eleuteri <[email protected]>

github-actions Bot added size/XL This PR changes 500-999 lines, ignoring generated files area/kubernetes Issues or PRs related to the tenant Kubernetes app kind/feature Categorizes issue or PR as related to a new feature labels Aug 6, 2026

github-actions Bot added size/XXL This PR changes 1000+ lines, ignoring generated files and removed size/XL This PR changes 500-999 lines, ignoring generated files labels Aug 6, 2026

mattia-eleuteri changed the title ~~feat(kubernetes): expose kernel modules for Talos worker node groups~~ feat(kubernetes): per-node-group kernel modules and Talos schematic Aug 6, 2026

mattia-eleuteri mentioned this pull request Aug 6, 2026

GPU node groups on Talos workers: kernel modules, NVIDIA schematic, gpu-operator wiring, CDI #3563

Open

Aleksei Sviridkin (lexfrei) requested changes Aug 6, 2026

View reviewed changes

This was referenced Aug 6, 2026

feat(kubernetes): boot tenant worker disks from a shared golden Talos image (CDI clone) #3294

Merged

fix(tests): freeze the EXIT-trap count for multus-install-cni-plugins.bats #3584

Merged

mattia-eleuteri mentioned this pull request Aug 6, 2026

feat(kubernetes): default gpu-operator to an OS-provided driver on Talos #3585

Open

11 tasks

Aleksei Sviridkin (lexfrei) requested changes Aug 6, 2026

View reviewed changes

Aleksei Sviridkin (lexfrei) mentioned this pull request Aug 10, 2026

fix(kubernetes): name TalosConfigTemplate by content hash #3523

Open

11 tasks

Aleksei Sviridkin (lexfrei) previously approved these changes Aug 10, 2026

View reviewed changes

mattia-eleuteri added 2 commits August 10, 2026 13:41

mattia-eleuteri dismissed Aleksei Sviridkin (lexfrei)’s stale review via 63f6485 August 10, 2026 11:45

mattia-eleuteri marked this pull request as ready for review August 10, 2026 11:47

mattia-eleuteri requested review from IvanHunters, Andrei Kvapil (kvaps) and myasnikovdaniil as code owners August 10, 2026 11:47

coderabbitai Bot reviewed Aug 10, 2026

View reviewed changes

Comment thread api/apps/v1alpha1/kubernetes/types.go Outdated

Comment thread packages/apps/kubernetes-nodes/README.md Outdated

Comment thread packages/apps/kubernetes/README.md Outdated

Comment thread packages/system/kubernetes-nodes-rd/cozyrds/kubernetes-nodes.yaml Outdated

Matthieu ROBIN (matthieu-robin) mentioned this pull request Aug 10, 2026

feat(vm-import): add VMware VM import via Forklift #1982

Closed

Aleksei Sviridkin (lexfrei) requested changes Aug 14, 2026

View reviewed changes

mattia-eleuteri added 2 commits September 3, 2026 10:44

mattia-eleuteri requested a review from Timofei Larkin (lllamnyp) as a code owner September 3, 2026 08:45

github-actions Bot added size/XL This PR changes 500-999 lines, ignoring generated files and removed size/XXL This PR changes 1000+ lines, ignoring generated files labels Sep 3, 2026

mattia-eleuteri changed the title ~~feat(kubernetes): per-node-group kernel modules and Talos schematic~~ feat(kubernetes): per-pool kernel modules and Talos schematic for worker nodes Sep 3, 2026

mattia-eleuteri requested a review from Aleksei Sviridkin (lexfrei) September 21, 2026 12:51

Conversation

mattia-eleuteri commented Aug 6, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

Gap 1: nothing loads the kernel modules

Design: three states, and no default

No involuntary worker roll

Gap 2: the schematic is cluster-wide, and that reboots every other pool

Sequencing with #3523

Tests

Not in scope

Screenshots

Downstream repositories

Release note

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

Aleksei Sviridkin (lexfrei) left a comment

Choose a reason for hiding this comment

Blockers

B1: kernelModules and schematicID are not validated before they reach the shell

B2: kernelModules does not reach workers that already exist

Non-blocking follow-ups

Uh oh!

mattia-eleuteri commented Aug 6, 2026

B1 — validated before the shell

B2 — documented

Follow-up 1 — #3294 already does this, more completely

Follow-up 2 — e2e image cache

Uh oh!

Aleksei Sviridkin (lexfrei) left a comment

Choose a reason for hiding this comment

Blockers

B3: the 64-hex rule rejects air-gapped and mirrored configurations that work today

On who owns the schematic field

Uh oh!

Aleksei Sviridkin (lexfrei) left a comment

Choose a reason for hiding this comment

B4: any extra key on a kernelModules item walks past the guard

B5: review provenance in two committed test files

Smaller

Uh oh!

mattia-eleuteri commented Aug 6, 2026

B3 — the guard was a format rule, not an injection guard

B4 — closed by construction

B5 — dropped

Smaller items

Ownership

Uh oh!

mattia-eleuteri commented Aug 6, 2026

Uh oh!

mattia-eleuteri commented Aug 6, 2026

Uh oh!

Aleksei Sviridkin (lexfrei) left a comment

Choose a reason for hiding this comment

Blocker

B6: the schematic class has no /, and a mirror path needs one

Recommended

Outside this PR

Uh oh!

mattia-eleuteri commented Aug 6, 2026

Uh oh!

mattia-eleuteri commented Aug 7, 2026

B6 — denylist, not a class

Recommended, both taken

Boundaries now pinned rather than incidental

The Go omitempty note

Uh oh!

Aleksei Sviridkin (lexfrei) left a comment

Choose a reason for hiding this comment

Recommended

CI

Uh oh!

mattia-eleuteri commented Aug 10, 2026

Uh oh!

mattia-eleuteri commented Aug 6, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 6, 2026 •

edited

Loading

B6: the schematic class has no `/`, and a mirror path needs one

The Go `omitempty` note

B1: the merge of main reverted the air-gapped bullet in `packages/apps/kubernetes/README.md`

B2: the `kernelModules` description is falsified by #3523, and it ships in the CRD