feat(kubernetes): per-pool kernel modules for Talos worker nodes - #3571

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

sequenceDiagram
  participant PoolValues
  participant talosReconcileJob
  participant kernelModulesHelper
  participant WorkerMachineConfig
  PoolValues->>talosReconcileJob: Pass kernelModules in the group context
  talosReconcileJob->>kernelModulesHelper: Resolve modules from pool values and GPU names
  kernelModulesHelper->>WorkerMachineConfig: Provide non-empty kernel.modules configuration

Merge Risk: ⚪ Minimal · up to f7c04

This change adds per-pool kernel-module configuration with automatic NVIDIA defaults and opt-out. No merge-blocking issue was identified in the supplied changes.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to f7c04

Input checks limit the new module settings before they reach the reconciliation job, and the change does not establish a new authorization path. However, existing pools may not receive the requested machine configuration, and malformed input may be treated as an opt-out rather than rejected. These risks are pool-scoped but can leave actual worker configuration different from the requested configuration.

Retained concerns

• Medium · reliability · inferred: For an existing pool, a newly selected module list is applied to a stable-name TalosConfigTemplate whose spec is documented as immutable. A rejected update leaves workers and newly provisioned replacements using the old configuration; per-pool image changes can therefore advance independently of the requested module configuration.
• Medium · architecture · inferred: The application defaulting path can convert a supplied non-array kernelModules value to an empty array. Because an empty array now means explicit opt-out, malformed input may silently suppress the automatic NVIDIA module selection rather than fail as invalid configuration.

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

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

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.

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.

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.

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.

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.

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.

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.

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:

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.

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.

Aleksei Sviridkin (@lexfrei) Agreed on both points. The branch is rebased onto current main as a single commit, and the schematic half is gone: no root-level schematicID, no kubernetes.schematicID helper, no schematic_per_pool_test.yaml, and hack/talos-reconcile-heredoc_test.bats keeps main's refusal of a hostile talos.schematicID untouched. The kernelModules description and the release note point a GPU pool at osImage.factory.schematicID.

What is left is kernelModules in the $group dict next to osImage, the kernel: block between nodeLabels and kubelet, and the helper, renamed kubernetes-nodes.kernelModules since there is no parent-chart copy to keep in step with any more. Stale references to tests/render-parity.sh and to the parent chart are gone from the helper and the test header. The heredoc bats file now lists kernelModules in its validated arm, with a refusal case for a hostile name and parameter and an execution case showing a semicolon parameter surviving the real shell.

On the open point: the description no longer states the fixed name as a property of the field. It says how a change reaches an existing pool depends on the TalosConfigTemplate naming. Today the apply is refused (#3515), so running workers keep their config and new workers boot from the old template. Once the template is content-named (#3523), the change rolls the pool. It also says the automatic set is itself such a change for every existing nvidia.com/* pool that leaves the field unset. The release note and the PR body spell out the consequence you describe: with both in, those GPU pools are rolled on upgrade.

helm unittest packages/apps/kubernetes-nodes passes (31 suites, 172 tests, 12 snapshots, render_snapshot_test.yaml unmodified), as does hack/cozytest.sh hack/talos-reconcile-heredoc_test.bats (8 cases). The PR body is updated to match.

Aleksei Sviridkin (@lexfrei) both points are handled in 91c87b8.

Parameter guard: it is now the two checks you proposed, ^[[:graph:]]+$ and ^[^$\x60\\'\"]+$, and the error message says "printable ASCII with no whitespace" instead of listing whitespace and control characters. kernel_modules_test.yaml gains one case for U+2028 and one for U+2029, and both assert the kernelModules message, not just any failure. Against the old regex they failed with yaml: line 287: could not find expected ':', so they pin the guard itself rather than the manifest failing to parse. helm unittest packages/apps/kubernetes-nodes: 174 passed. make bats-unit-talos-reconcile-heredoc_test: 8 passed, the semicolon parameter included.

PR body: the "Rebased onto main and reduced" block is gone, and so is the "narrow with this reduction" wording in the downstream section. The body now describes only what the PR ships, and the parameter guard paragraph and the test counts match the new commit.