fix(apps/kubernetes): honour nodeGroup minReplicas in MachineDeployment.spec.replicas - #3233

Actionable comments posted: 2

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`:
- Line 25: The comment in the worker nodes configuration doc is too strict about
custom ingress-nginx node groups needing minReplicas >= 1. Reword the guidance
around the nodeGroups default behavior so it says that if you want immediate
workers you should set a custom group with roles: [ingress-nginx] and
minReplicas >= 1, but that minReplicas can still be 0 and the autoscaler will
scale it when the ingress-nginx controller Pods are pending. Keep the reference
centered on the Worker nodes configuration map and the default md0 behavior.

In `@packages/apps/kubernetes/values.yaml`:
- Line 92: The nodeGroups documentation in values.yaml is too restrictive by
stating that an ingress-nginx node group must have minReplicas >= 1; update the
wording to stay permissive and accurate. In the nodeGroups description, and any
related generated text driven by it, clarify that a custom group with roles:
[ingress-nginx] may also start at 0 and be scaled by the cluster-autoscaler when
the controller Pods are Pending, while keeping the guidance about the default
md0 and custom groups intact.

---

Nitpick comments:
In `@packages/apps/kubernetes/tests/nodegroups_default_test.yaml`:
- Around line 193-229: The MachineDeployment regression test for the
nonzero-minReplicas case is missing the max-size annotation check that the
zero-minReplicas case already covers. Update the existing test block for
nodeGroups.worker0 so it also asserts
metadata.annotations["cluster.x-k8s.io/cluster-api-autoscaler-node-group-max-size"]
equals "10", keeping parity with the neighboring zero-minReplicas assertion set
and using the existing MachineDeployment documentSelector.

Code Review

This pull request updates the Kubernetes application package to omit spec.replicas from the MachineDeployment template, allowing the cluster-autoscaler to solely own and manage the replica count. This prevents worker nodes from being drained during routine Helm upgrades. Consequently, the default nodeGroups configuration now defaults to minReplicas: 0, meaning no workers are provisioned initially until triggered by the autoscaler or scaled manually. Documentation, schemas, and helper templates have been updated to reflect this behavior, and regression tests have been added to verify that spec.replicas is omitted and autoscaler annotations are correctly set. Additionally, E2E tests have been updated to omit nodeGroups to prevent storage churn and test flakes. No review comments were provided, so there is no feedback to evaluate.

The functional change is correct — removing the hardcoded MachineDeployment.spec.replicas: 2 and letting the field be owned by the cluster-autoscaler is the standard CAPI + autoscaler pattern, and it's safe in both paths:

• Fresh install (Create): CAPI's MachineDeployment defaulting webhook seeds spec.replicas from the cluster-api-autoscaler-node-group-min-size annotation (already emitted by the chart, unchanged here).
• Existing cluster (Update): even under Helm 3's client-side three-way merge (which will generate a patch removing replicas on the first upgrade off the old chart), CAPI's calculateMachineDeploymentReplicas preserves the previous live value (oldMD.Spec.Replicas) when the incoming value is nil on an update, so a cluster the autoscaler holds above min-size is not drained. The real guarantee is the CAPI webhook's update path rather than SSA field ownership, but the outcome is correct either way.

The generated artifacts and the new helm-unittest regression guard are all consistent and well-targeted.

Two comment fixes are needed before merge:

1. Both bats comments claim "zero MachineDeployments render" — this is inaccurate.

When nodeGroups is empty, templates/_helpers.tpl (kubernetes.nodeGroups) emits a default md0 group, so exactly one MachineDeployment renders (default md0 with minReplicas: 0). What is zero is the number of KubevirtMachines / worker DataVolumes, because CAPI seeds replicas to 0 — which matches the PR description. Affects:

• hack/e2e-apps/kubernetes-oidc-customconfig.bats
• hack/e2e-apps/kubernetes-oidc-system.bats

2. kubernetes-oidc-system.bats describes the bug as still present in the PR that fixes it.

The comment states the chart "hardcodes spec.replicas: 2 ... (see #3231)", but this PR removes that hardcoding, so the statement is false post-merge — and it contradicts the sibling customconfig comment, which correctly references the post-fix behavior. Please rewrite it to describe the post-fix rationale (default md0, minReplicas: 0, CAPI seeds replicas: 0, no DataVolumes), matching the customconfig block.

Non-blocking notes:

• Fresh installs with the default nodeGroups: {} now come up with 0 workers instead of 2, relying on the autoscaler to scale from zero. This is well-documented and scale-from-zero is wired via the capacity.cluster-autoscaler.kubernetes.io/{cpu,memory} annotations — flagging only as an intentional behavior change worth confirming.
• The PR body's claim that the chart edit and the bats edit are each "a no-op without the other" isn't strictly accurate — the chart fix alone would already render replicas: 0 for the existing minReplicas: 0 nodeGroup. Not a correctness issue.

LGTM — the core mechanism is verified against CAPI v1.10.1 and the chart's own unittest, and both the real-cluster e2e and the helm upgrade path are preserved. A few non-blocking follow-ups below.

Business context: The chart hardcoded MachineDeployment.spec.replicas: 2, defeating a nodeGroup's minReplicas (especially minReplicas: 0). That forced the render-side kubernetes-oidc-* e2e tests to provision two 20 GiB DRBD worker DataVolumes each, whose create/delete churn overlapped kubernetes-previous and flaked it under LINSTOR pool pressure (#3231, parts 1+2).

Verification

The PR pivots to omitting spec.replicas entirely and relying on CAPI's defaulting webhook. I confirmed this against cluster-api v1.10.1 (the pinned cluster-api-controller), function calculateMachineDeploymentReplicas:

• New MachineDeployment (oldMD == nil) with both autoscaler annotations present → returns minSize. So a fresh install with minReplicas: 2 → replicas=2 (identical to pre-PR), and minReplicas: 0 → replicas=0.
• Update with incoming replicas == nil → keeps the existing value when within [min, max], clamps into range otherwise → no worker drain on helm upgrade.
• Requires both the min-size and max-size annotations, otherwise it defaults to 1. The template emits both unconditionally and the annotation keys match the CAPI constants exactly.

Also confirmed: the per-tenant cluster-autoscaler is deployed by default (gated on _namespace.etcd) with --enforce-node-group-min-size=true and the capacity.cluster-autoscaler.kubernetes.io/{cpu,memory} annotations, so scale-from-zero is wired; run-kubernetes.sh sets minReplicas: 2 and its .status.replicas=2 / readyReplicas=2 waits still hold; helm unittest passes 173/173.

Non-blocking follow-ups

1. The two new OIDC test comments are inaccurate (see inline). Both state "zero MachineDeployments render", but the helper emits the default md0 whenever nodeGroups is empty — the nodegroups_default_test.yaml case "renders the default md0 group when nodeGroups is empty" asserts exactly that. What is actually zero is the worker DataVolumes / KubevirtMachines, because spec.replicas is omitted and CAPI defaults it to 0. kubernetes-oidc-system.bats additionally describes the pre-PR hardcode in the present tense, and kubernetes-oidc-customconfig.bats describes the intermediate replicas: {{ minReplicas }} approach that the final commit superseded by omitting the field. Since aligning docs with actual behavior is the stated purpose of the docs commit, worth correcting.

2. Fresh-install default changes from 2 workers to 0. A default CR (nodeGroups: {}) now comes up with no workers until the autoscaler scales md0 from zero on the first pending Pod. This is intentional and documented across values / README / schema / CRD / Go doc and the release note — flagging only for an explicit maintainer ack, since it is a user-visible default change and scale-from-zero on a brand-new cluster is now a load-bearing path that previously never executed (clusters always started at 2).

3. I concur with the existing bot nit to add the max-size annotation assertion to the nonzero-minReplicas regression case, for parity with the zero case. On the minReplicas >= 1 wording: I read it as already permissive — the same sentence offers the autoscaler-from-zero alternative — so I would treat that one as optional.

LGTM. The comment fixes are in and the change is correct, safe, and well-tested — I traced the full mechanism:

• Both cluster-api-autoscaler-node-group-min-size and -max-size annotations are emitted on every MachineDeployment, which is what lets CAPI default spec.replicas to minSize on Create.
• The cluster-autoscaler is always deployed with --enforce-node-group-min-size=true, so min-size is actively enforced, not just a create-time default.
• Scale-from-zero works: the default md0 has minReplicas: 0 and the chart emits the capacity.cluster-autoscaler.kubernetes.io/{cpu,memory} hints the autoscaler needs to scale an empty group up for a pending Pod.
• No drain on upgrade: the field is simply absent from the rendered manifest and CAPI's defaulting webhook preserves the live spec.replicas on UPDATE.

Worth noting the old replicas: 2 on a minReplicas: 0 group was itself a flap — the autoscaler drove the idle group back to 0, then every helm upgrade reset it to 2. This PR removes that oscillation; 0 was already the steady state, so "0 workers by default" is not a regression.

Non-blocking, but worth correcting: the cluster.yaml comment (and the paired _helpers.tpl and .bats comments) attribute upgrade-safety to "Server-Side-Apply field-manager ownership". That's the wrong mechanism — this chart is delivered as a HelmRelease via Flux helm-controller, which applies through the Helm SDK three-way merge, not SSA field managers. The value actually survives upgrades because (a) the field is absent from the rendered manifest and (b) CAPI's defaulting webhook preserves the existing live value on UPDATE. The conclusion is right, but the stated reason could send a future maintainer down the wrong path when debugging drift/ownership. Suggest rewording to reference CAPI update-time defaulting + the field being unmanaged by the chart, and dropping the SSA claim.

lgtm, thank you!