mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-08 21:14:56 +09:00
Aleksei Sviridkin
0bc3f9f25c
## What this PR does Fixes #3515. `TalosConfigTemplate.spec` is immutable — the `vtalosconfigtemplate.cluster.x-k8s.io` webhook denies any mutation with `TalosConfigTemplate.Spec is immutable` — but the `talos-reconcile` Job applied it under a fixed `<release>-<group>` name on every reconcile. Once the template existed, **no change to the rendered worker machine config could ever reach the cluster**: the apply was rejected server-side while the HelmRelease stayed `Ready`. Reported with `machine.network.searchDomains` after a `clusterDomain` correction; the same held for `certSANs`, `machine.registries` and nameservers. - Name the `TalosConfigTemplate` `<release>-<group>-<6-char hash of its spec>`, mirroring the `KubevirtMachineTemplate` content-hash idiom the chart already uses, and point `MachineDeployment.spec.template.spec.bootstrap.configRef` at that name. A content change now creates a **new** template instead of attempting a rejected mutation, and CAPI rolls the workers onto it. - Extract the machine config into a single named template (`_talosconfigtemplate.tpl`) shared by the two consumers that have to agree on the name byte-for-byte: the Job, which embeds it in the heredoc it `kubectl apply`s, and the `MachineDeployment`, which hashes it into `configRef`. - Assemble the node group once, in `kubernetes-nodes.group`, and read it back at both call sites. Sharing the rendering was not enough: each file also built its own `$group` dict, so the two names agreed only by coincidence of the keys the spec helper happens to read. - The runtime values the Job substitutes (`${TALOS_CA_B64}`, `${SVC_IP}`, `${COREDNS_IP}`, tokens) stay shell placeholders and are deliberately **not** part of the hash: they are facts of an already-provisioned cluster, not chart inputs, so a certificate rotation or a Service re-address does not roll workers. ### What the shared spec carries Worker node pools live in the `kubernetes-nodes` chart, which owns `_talosconfigtemplate.tpl` outright: `make update` copies nothing from the parent chart. The helper renders everything `main` puts into the worker machine config, so the hashed spec is the one the cluster gets: - The heredoc escaping from #3513: backslash, dollar and backtick escaping on `talosVersion`, `installerRepository` and `schematicID`, the escaped `machine.registries.mirrors` block, and the `INVARIANT` comment above `data:`, which belongs in that file since it is where the next field will be added. The spec is still emitted from an unquoted heredoc in a host-cluster Pod, so an unescaped copy would be command injection. - The per-pool install image. `_talosconfigtemplate.tpl` calls `kubernetes-nodes.resolveOsImage` itself instead of reading `talos.schematicID` and `talos.version`, so a pool on `osImage.builtin` or `osImage.factory` reinstalls the schematic and version it actually booted. The install image is part of the hashed spec, so the hash has to be taken over the resolved image, or an `osImage` change would render a different config under an unchanged name. - The Proxmox branches: `nameservers` from `proxmox.dnsServers` where a KubeVirt pool points at the cluster's CoreDNS, and the kubelet's `cloud-provider: external`, without which the Proxmox cloud-controller-manager never initialises the Node. - `machine.kernel.modules`, emitted only when the pool resolves to a non-empty list. `kubernetes-nodes.group` carries every key those read, `podCpuLimit`, `podCpuRequest`, `osImage`, `proxmox` and `kernelModules` included. A key missing there is the failure this PR exists to prevent: with `kernelModules` read at one call site only, an explicit module list hashes to two different names and the `MachineDeployment` points at a template nothing creates. The `kubectl apply` stays `--server-side --force-conflicts` (#4163), so the Job is not OOMKilled on the merge path; with a content-hash name the apply is normally a create or a no-op, and server-side apply keeps the no-op cheap too. ### The replace-on-immutable path and the Role The runtime values stay outside the hash, so one failure is left that the hash cannot absorb: a new apiserver address renders a different spec under an unchanged name, and the webhook rejects it. The replace path from #4089 handles exactly that case and is kept, with its `kubectl delete` pointed at `${TCT_NAME}`. The talos-reconcile Role pins its TalosConfigTemplate `get`, `patch`, `update` and `delete` rights to the pool's own template, so it names the hashed template too. A server-side apply of a template that does not exist yet is still a PATCH on its name: a rule left on `<release>-<group>` would leave the Job Forbidden and the pool with no template to scale onto, with nothing turning red. The name-agreement guard below therefore covers the Role as well. ## Upgrade impact — please read **Existing clusters roll their Talos workers once.** The `configRef` moves from `<release>-<group>` to the hashed name; CAPI creates a new `MachineSet` for it and rolls machines onto it under the existing `RollingUpdate` strategy (`maxSurge: maxReplicas`, `maxUnavailable: 1`) plus the group's `MachineHealthCheck`. The rendered machine config itself is byte-identical to `main` for the same values, so this particular roll delivers no config change — it is the one-time cost of adopting the content-hash name, the same cost `KubevirtMachineTemplate` already paid. Proxmox pools roll the same way: the `ProxmoxMachineTemplate` name does not move, but the `configRef` does. **Until the new template exists, the pool cannot scale.** The Helm upgrade points the `MachineDeployment`'s `configRef` at the hashed name before the `talos-reconcile` Job has created that template, and there is no render-time ordering fix, since the template needs values Helm does not have. Existing workers are untouched in that window, but the pool is frozen: in CAPI v1.10.1, which this repository ships, `getTemplatesAndSetOwner` records the missing bootstrap template, and `rolloutRolling` then fails in `getAllMachineSetsAndSyncRevision` with `cannot create a new MachineSet when templates do not exist` and returns before it scales either the new or the old `MachineSet`. A replica change from the cluster-autoscaler therefore reaches no `MachineSet`: scale-up and scale-down both wait. On an upgraded cluster the Job's inputs already exist, so the window is normally a few seconds. **A failed `talos-reconcile` Job now blocks autoscaling, and nothing turns red.** After `backoffLimit: 30` the Job is not retried until the next Helm upgrade of the pool release, and the HelmRelease stays `Ready` meanwhile, as it did in #3515. Before this PR the same failure left the pool on its old template and still scaling; with it, the pool cannot scale at all until the template exists. Where to look: - the `MachineDeployment` `kubernetes-<cluster>-<group>`: its `ScalingUp` condition under `status.v1beta2.conditions` reads `Scaling up from N to M replicas is blocked because TalosConfigTemplate does not exist` (or `Scaling up would be blocked because …` while no scale-up is pending), and the capi-controller-manager logs carry `cannot create a new MachineSet when templates do not exist` for it; - whether a `TalosConfigTemplate` named after its `spec.template.spec.bootstrap.configRef.name` exists in the tenant namespace; - the `talos-reconcile` Job's status and Pod logs — but the Job has `ttlSecondsAfterFinished: 600`, so a failed Job and its logs are deleted ten minutes after it gives up. Past that, the `MachineDeployment` condition and the missing template are the only evidence left. The pre-existing fixed-name template is **not** deleted — the outgoing `MachineSet` still references it — and is garbage-collected with the `KamajiControlPlane` when the cluster is deleted. Hashed templates therefore accumulate, one per distinct machine config. Pruning them safely is filed as #4355 rather than widened into this PR. **The one roll is not the only one.** Hashing the whole rendered spec makes anything inside it a roll trigger, including two things that are not edits to this cluster's resource. For a pool sized by `instanceType`, the kubelet reservations come from a live `lookup` of the `VirtualMachineClusterInstancetype`, so a later platform-side edit to that instancetype rotates the name and rolls the pool again — and the `KubevirtMachineTemplate` hash this mirrors does not have that property, since it embeds the instancetype by name and never its resolved size. And `talos.registryMirrors` is inside the hashed spec, so setting or changing a mirror now rolls the pool rather than applying only to nodes created later; the field's documentation said nothing about this and now says it. The same holds on Proxmox for `proxmox.dnsServers`, and on every substrate for `kernelModules`: both are inside the hashed spec, so changing either rolls the pool, and the `kernelModules` documentation now says so instead of describing the fixed name as the current state. **The roll is not free on a node group holding a scarce resource.** From exercising this same rollout path on a production fleet of 13 tenant clusters, by rotating the `KubevirtMachineTemplate` hash on a GPU node group. `maxSurge: maxReplicas` is dangerous when the resource is constrained: with 6 GPUs and 1 free, CAPI created all 5 new machines at once and 3 sat `Pending` on `Insufficient nvidia.com/...`, each having already imported a 200 GiB disk. It converges, but it wastes a lot of import. The natural reflex, `maxSurge: 0` with `maxUnavailable: 1`, deadlocks instead: CAPI computes `maxScaledDown = available - (desired - maxUnavailable)`, which was `4 - 4 = 0`, so it removed no old machine while the new ones waited for exactly those machines to release their GPUs; nothing moved until `maxUnavailable` was loosened to 2. And the `MachineHealthCheck` amplifies both — at `maxUnhealthy: 100%` and `nodeStartupTimeout: 10m`, machines queuing for a GPU were declared unhealthy and remediated in a loop, three deleted and recreated while perfectly healthy, each iteration re-importing the disk. The per-node-group `nodeStartupTimeout` override fixes that, but the default is shorter than a plausible queue wait. Anyone upgrading a fleet with a GPU pool hits all three, and this PR is what starts the roll. ## Validation - `helm unittest packages/apps/kubernetes-nodes` passes, snapshots included. Pointing `configRef` back at the fixed name fails `nodegroup_test.yaml`, the snapshot and the name-agreement bats; dropping `osImage` from `kubernetes-nodes.group` fails the `worker_image_*` suites; removing the Proxmox nameserver branch fails `substrate_branches_test.yaml` and the Proxmox heredoc case. - `hack/check-talosconfigtemplate-name-agreement.bats` is new. The unittest suites pin each name against its own literal in its own file, so a divergence shows up as one literal having moved, which reads as a stale fixture and invites re-pasting the value. The guard renders the chart once and compares the Job's `TCT_NAME`, the Role's TalosConfigTemplate `resourceNames` and the `MachineDeployment`'s `configRef.name` against each other, so the failure states the invariant rather than a number. Asserted on a GPU-shaped pool as well as the default one, because the default renders `gpus []`, `kubelet {}` and `instanceType ""` as falsy and a consumer reading a subset of the group is indistinguishable there from one reading all of it; on an explicit `kernelModules` list, which neither of those shapes sets; and a last case pins that the shapes hash differently, so the others cannot hold vacuously. Verified by injecting each divergence: dropping `gpus` at one call site, dropping `kernelModules` at one call site, and pointing the Role back at `<release>-<group>` each fail it, the last one also failing the Role cases in `talos_tct_immutable_test.yaml`, as does pointing the `delete` back at the fixed name. - `hack/cozytest.sh hack/talos-reconcile-heredoc_test.bats` passes, the Proxmox cases included. This is the execution-level evidence that the escaping survived the move into `_talosconfigtemplate.tpl`: it renders with a hostile `registryMirrors` endpoint and a hostile `installerRepository`, runs the extracted heredoc through a real shell, and asserts the values come out literal rather than command-substituted; hostile `schematicID` and `version` values, and `proxmox.dnsServers` entries that are not addresses, are asserted refused at render; and the Proxmox heredoc is asserted to emit the nameservers it was given. A rendered-string regex cannot catch a heredoc the shell refuses to emit; this does. - `make generate` in `packages/apps/kubernetes-nodes` — no diff. - The "byte-identical to `main`" claim was measured against `main` on six pool shapes: default, GPU, an explicit `kernelModules` list, `kernelModules: []` on a GPU pool, `osImage.factory` and Proxmox. For each, the `TalosConfigTemplate` manifest the Job applies differs only in its name (`${RELEASE}-${GROUP_NAME}` → `${TCT_NAME}`); the spec is unchanged, the Job's `TCT_NAME` equals the `MachineDeployment`'s `configRef.name`, and the `KubevirtMachineTemplate` and `ProxmoxMachineTemplate` names do not move. - **Not verified:** no live-cluster or e2e run (needs bare-metal nodes). The upgrade sequence above, the scaling freeze included, comes from reading the CAPI v1.10.1 `MachineDeployment` controller source and the chart's existing `KubevirtMachineTemplate` precedent, not from watching it end to end. ### Downstream repositories - [ ] 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: Every box is left empty on purpose, including the first, because I am not confident about one line of the map and would rather a maintainer decided than have me tick it. The change adds no field and changes no default: the only schema movement is the description text of `talos.registryMirrors` and `kernelModules`, regenerated from their `values.yaml` doc comments. Nothing else downstream restates this chart's templates, the hash naming is internal to the chart, and no image reference, CI behaviour or CRD shape moves. The open question is whether `terraform-provider-cozystack` restates field descriptions as well as field shapes; if it does, those description changes reach it and want a follow-up. ## Release note ```release-note fix(kubernetes): name the worker TalosConfigTemplate after a hash of its content so machine-config changes (searchDomains, certSANs, registries, nameservers) actually propagate instead of being silently rejected by the immutability webhook. Upgrading rolls existing Talos workers once, on KubeVirt and Proxmox pools alike. Until the talos-reconcile Job has created the new template, the pool cannot scale, so a failed talos-reconcile Job now blocks cluster-autoscaler scale-up and scale-down while the HelmRelease stays Ready; check the MachineDeployment's ScalingUp condition ("blocked because TalosConfigTemplate does not exist") and the Job status, which is deleted ten minutes after it fails. On a node group holding a scarce resource such as GPUs, review the MachineDeployment surge settings and the MachineHealthCheck nodeStartupTimeout first: `maxSurge: 0` can deadlock the roll and the default startup timeout can trigger spurious remediation while machines queue for the resource. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Worker Talos configuration templates now use content-hashed names for consistent reconciliation and bootstrap references. * Changes to worker machine configuration, including GPU and registry mirror settings, automatically roll existing workers. * **Documentation** * Updated registry mirror documentation to explain that configuration changes roll all workers in the pool. * **Tests** * Added coverage for hashed template names, configuration-triggered worker rotations, and matching references across worker resources. <!-- end of auto-generated comment: release notes by coderabbit.ai -->