Skip to content

Commit 0c7b598

Browse files
authored
fix(apps/kubernetes): honour nodeGroup minReplicas in MachineDeployment.spec.replicas (#3233)
Refs #3231 (parts 1 + 2 addressed here; part 3 — intra-file `cozy_wait_linstor_pool_free` gate + orphan `default-chk-clickhouse-test-keeper-cluster1-0-2-0` PVC cleanup — deferred to a separate follow-up PR against `hack/e2e-apps/run-kubernetes.sh`). **Do not auto-close the issue on merge** — the hardening work is a distinct concern and belongs on its own tracker entry. ## Root cause chain (recap from the issue) `packages/apps/kubernetes/templates/cluster.yaml` hardcoded `MachineDeployment.spec.replicas: 2` regardless of the nodeGroup's `minReplicas`. The two render-side `kubernetes-oidc-{system,customconfig}` bats set `minReplicas: 0` and delete their `Kubernetes` CR without waiting for readiness — but the chart still spun up 2 × 20 GiB `replicated` (DRBD) worker `DataVolume`s each. Their create/delete churn overlapped `kubernetes-previous`, which then saw slow LINSTOR provisioning (my cozyreport from run 28843794670 shows a ~6-minute gap between the `Kubernetes` CR being created and the CDI `ExternalProvisioning` event actually firing for the worker root disk); the tenant node registered late; Cilium's CNI init missed the 12-minute node-join deadline; and the test failed with `cni plugin not initialized`. ## Changes Two edits, applied together — the chart edit is a no-op without the test edit, and vice-versa (dropping `nodeGroups` falls back to the chart helper's default `md0`, which itself has `minReplicas: 0`, so both pieces are needed for zero workers to actually render). ### 1. `packages/apps/kubernetes/templates/cluster.yaml` ```diff spec: clusterName: {{ $.Release.Name }} - replicas: 2 ``` The field is **omitted entirely** so the cluster-autoscaler owns it alone. CAPI's MachineDeployment defaulting webhook seeds `spec.replicas` from the `cluster-api-autoscaler-node-group-min-size` annotation on Create (documented at https://cluster-api.sigs.k8s.io/tasks/automated-machine-management/autoscaling — *"if it's a new MachineDeployment or MachineSet, use min size"*), and Server-Side-Apply field-manager ownership then keeps chart upgrades from overwriting whatever value the autoscaler patched in the meantime. Emitting the field from the chart at all — even with the semantically-correct `{{ $group.minReplicas }}` — would keep the helm-controller ↔ cluster-autoscaler field-ownership fight alive: helm-controller applies with SSA + force-conflicts (https://fluxcd.io/flux/components/helm/helmreleases/), so every routine `helm upgrade` re-asserts the chart's chosen value and drains whatever the autoscaler had patched in the meantime. ### 2. `hack/e2e-apps/kubernetes-oidc-{system,customconfig}.bats` Drop the explicit `nodeGroups:` block and the paired `storageClass:` from the render-side CR bodies. The tests only assert on the KamajiControlPlane and the cozy-realm Keycloak objects — worker plumbing was never part of their coverage. After the chart fix, the chart helper's default `md0` (with `minReplicas: 0`) renders a MachineDeployment with `spec.replicas: 0` — the resource is still emitted for API-surface completeness but never creates a KubevirtMachine, never provisions a DataVolume, and never touches DRBD. ## Verification ### helm-unittest ``` Charts: 1 passed, 1 total Test Suites: 16 passed, 16 total Tests: 171 passed, 171 total ``` No suite asserted on `MachineDeployment.spec.replicas` directly, so nothing needed updating. ### helm template — verify the omit With `tests/values/common.yaml` (which has `minReplicas: 0`), the rendered MD carries the `cluster-api-autoscaler-node-group-min-size: "0"` annotation and **no `spec.replicas` field**. CAPI's defaulting webhook will fill in `spec.replicas: 0` on Create — no worker DataVolumes provisioned until scale-out. With a user CR that sets `minReplicas: 2` (the shape the `kubernetes-latest` / `kubernetes-previous` bats CRs use), the rendered MD carries the `cluster-api-autoscaler-node-group-min-size: "2"` annotation and again **no `spec.replicas` field**. CAPI's defaulting webhook will fill in `spec.replicas: 2` on Create — byte-identical topology to the pre-PR chart's fresh-install state (2 workers), but without the recurring drain-on-upgrade cycle. ### Render-side render walk `helm template` of the OIDC bats CR bodies (after this PR): 1 MachineDeployment with **no chart-managed `spec.replicas`** and `min-size: "0"` on the autoscaler annotation. CAPI seeds `replicas: 0` on Create, 0 KubevirtMachines emitted, 0 DataVolume provisioning triggered. Test assertions on KamajiControlPlane / KeycloakClient / KeycloakClientScope / AuthenticationConfiguration Secret are unchanged. ## Upgrade impact Existing customers stop losing workers on `helm upgrade`. Prior to this PR the chart re-asserted `MachineDeployment.spec.replicas: 2` on every Helm action, so a customer running with the autoscaler at, say, `replicas: 8` would see 6 workers drained on every routine Cozystack platform bump and then wait for the autoscaler to scale back up in response to unschedulable Pods. With `spec.replicas` no longer emitted by the chart, SSA field-manager ownership preserves the autoscaler's value across upgrades unconditionally. Fresh installs with `nodeGroups: {}` (the schema default) transition from "2 workers by default" to "0 workers until scale-out". The chart helper's default md0 carries `minReplicas: 0`, which is what CAPI's defaulting webhook seeds `spec.replicas` from. The `values.yaml` / README / helper docstring / regenerated Go type doc + openAPISchema call this out explicitly; the release-note repeats it below. See finding 5 in the paired self-review for the specific `addons.ingressNginx.enabled: true` interaction operators need to be aware of on fresh installs. ## Test plan - [x] `helm unittest packages/apps/kubernetes/` — 171/171 passing. - [x] `helm template` with `tests/values/common.yaml` (minReplicas: 0) — MD.replicas: 0. - [x] `helm template` with a `minReplicas: 2` override — MD.replicas: 2 (byte-identical to pre-PR). - [ ] E2E: `kubernetes-latest` unchanged (spawns its two workers as before), `kubernetes-oidc-{system,customconfig}` render zero workers, `kubernetes-previous` no longer flakes on LINSTOR pool pressure originating from the earlier OIDC suite. ## Notes - Part 3 of #3231 (intra-file `cozy_wait_linstor_pool_free` gate between kube sub-tests and the orphan clickhouse keeper PVC cleanup) is **not** in this PR — worth a separate targeted change to `hack/e2e-apps/run-kubernetes.sh`, and it's a hardening-only improvement rather than a root-cause fix. Happy to send it as a follow-up if wanted. ## Release note ```release-note fix(apps/kubernetes): stop the chart from managing `MachineDeployment.spec.replicas`; the cluster-autoscaler owns the field alone. CAPI's defaulting webhook now seeds `spec.replicas` from the `cluster-api-autoscaler-node-group-min-size` annotation on Create, and SSA field-manager ownership keeps subsequent `helm upgrade` actions from overwriting whatever value the autoscaler patched. Two behavioural changes worth calling out: - Existing customers: `helm upgrade` no longer drains workers on every platform bump — the autoscaler-held `spec.replicas` is preserved unconditionally. Prior to this change the chart hardcoded `replicas: 2`, which drained back to 2 on every reconcile action. - Fresh installs with `nodeGroups: {}` (the schema default) no longer come up with two workers by default. The chart helper's default md0 carries `minReplicas: 0`, and CAPI's defaulting webhook seeds `spec.replicas` from that. Operators enabling `addons.ingressNginx.enabled: true` on such a CR must either supply an explicit nodeGroup with `roles: [ingress-nginx]` and `minReplicas >= 1`, or accept that the cluster-autoscaler will bring the default md0 up in response to the ingress-nginx controller Pods becoming Pending on install. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Rendered `MachineDeployment` manifests no longer set `spec.replicas`, avoiding chart upgrades overwriting autoscaler-managed sizing. * **Documentation** * Clarified `nodeGroups: {}` behavior: a default `md0` is emitted with `minReplicas: 0`/`ingress-nginx` role, and workers are provisioned only after an autoscaler-triggering unschedulable Pod or manual scaling; updated related storage and ingress guidance. * **Tests** * Strengthened default node group rendering assertions and adjusted OIDC e2e manifests to reduce CI provisioning churn. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 parents 12c3cd0 + 6f5d826 commit 0c7b598

10 files changed

Lines changed: 201 additions & 50 deletions

File tree

‎api/apps/v1alpha1/kubernetes/types.go‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎hack/e2e-apps/kubernetes-oidc-customconfig.bats‎

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -49,17 +49,16 @@ spec:
4949
resources: {}
5050
resourcesPreset: small
5151
replicas: 1
52-
nodeGroups:
53-
md0:
54-
diskSize: 20Gi
55-
gpus: []
56-
instanceType: u1.micro
57-
maxReplicas: 1
58-
minReplicas: 0
59-
resources: {}
60-
roles:
61-
- worker
62-
storageClass: replicated
52+
# nodeGroups intentionally omitted — this is a render-side test that
53+
# only asserts on the KamajiControlPlane + the tenant-supplied issuer
54+
# AuthenticationConfiguration Secret. With the schema default `{}`,
55+
# the chart helper emits the default `md0` group with
56+
# `minReplicas: 0` — exactly one MachineDeployment renders
57+
# (API-surface completeness) but the chart no longer manages
58+
# `spec.replicas`, so CAPI's defaulting webhook seeds it to 0
59+
# from the autoscaler min-size annotation. Result: zero
60+
# KubevirtMachines, zero worker DataVolumes, no DRBD churn between
61+
# CI runs (see #3231).
6362
version: v1.35
6463
oidc:
6564
mode: CustomConfig

‎hack/e2e-apps/kubernetes-oidc-system.bats‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -57,17 +57,19 @@ spec:
5757
resources: {}
5858
resourcesPreset: small
5959
replicas: 1
60-
nodeGroups:
61-
md0:
62-
diskSize: 20Gi
63-
gpus: []
64-
instanceType: u1.micro
65-
maxReplicas: 1
66-
minReplicas: 0
67-
resources: {}
68-
roles:
69-
- worker
70-
storageClass: replicated
60+
# nodeGroups intentionally omitted — this is a render-side test that
61+
# only asserts on the KamajiControlPlane + the cozy-realm Keycloak
62+
# objects. With the schema default `{}`, the chart helper emits the
63+
# default `md0` group with `minReplicas: 0` — exactly one
64+
# MachineDeployment renders (API-surface completeness) but the chart
65+
# no longer manages `spec.replicas`, so CAPI's defaulting webhook
66+
# seeds it to 0 from the autoscaler min-size annotation. Result:
67+
# zero KubevirtMachines, zero worker DataVolumes, no DRBD churn
68+
# overlapping `kubernetes-previous` (see #3231). Keeping an
69+
# explicit nodeGroup here would previously provision two 20 GiB
70+
# DRBD DataVolumes per `kubectl apply` because the chart hardcoded
71+
# `spec.replicas: 2`; the paired chart edit in this PR removes
72+
# that hardcoding.
7173
version: v1.35
7274
oidc:
7375
mode: System

‎packages/apps/kubernetes/README.md‎

Lines changed: 23 additions & 23 deletions
Large diffs are not rendered by default.

‎packages/apps/kubernetes/templates/_helpers.tpl‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,16 @@ re-add a baked-in default md0 on top of the user's groups, and because
124124
Kubernetes strips null values the default could never be removed. With the
125125
default applied only when the map is empty, users can freely choose their own
126126
node groups (and omit md0).
127+
128+
The default carries `minReplicas: 0`, which now propagates through to
129+
`MachineDeployment.spec.replicas: 0` (see the templates/cluster.yaml comment
130+
next to that field). Consequence: an empty-`nodeGroups` install provisions
131+
NO workers until either the cluster-autoscaler responds to an unschedulable
132+
Pod or an operator explicitly scales the group up. If the operator plans to
133+
enable the ingress-nginx addon, either supply an explicit nodeGroup with
134+
`roles: [ingress-nginx]` and `minReplicas >= 1`, or accept that the
135+
autoscaler will bring the default md0 up in response to the ingress-nginx
136+
controller Pods becoming Pending on install.
127137
*/}}
128138
{{- define "kubernetes.nodeGroups" -}}
129139
{{- if .Values.nodeGroups -}}

‎packages/apps/kubernetes/templates/cluster.yaml‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -743,7 +743,29 @@ metadata:
743743
{{- end }}
744744
spec:
745745
clusterName: {{ $.Release.Name }}
746-
replicas: 2
746+
# `spec.replicas` is intentionally omitted so the field is owned by
747+
# the cluster-autoscaler alone. Two independent CAPI mechanisms —
748+
# not Server-Side-Apply field managers — carry the guarantee:
749+
# 1. Create: the MachineDeployment defaulting webhook seeds
750+
# `spec.replicas` from the `cluster-api-autoscaler-node-group-
751+
# min-size` annotation (see
752+
# https://cluster-api.sigs.k8s.io/tasks/automated-machine-management/autoscaling).
753+
# 2. Update: `calculateMachineDeploymentReplicas` preserves the
754+
# existing live value (`oldMD.Spec.Replicas`) whenever the
755+
# incoming spec has `replicas: nil`, clamping into
756+
# `[min, max]` if it drifts outside — so a cluster the
757+
# autoscaler holds above min-size is not drained by a
758+
# `helm upgrade` that would otherwise re-render this field.
759+
# Flux helm-controller applies this chart through the Helm SDK
760+
# three-way merge (not SSA), so field-manager ownership is not the
761+
# mechanism at play. The chart guarantee reduces to "leave the
762+
# field out of the rendered manifest and let CAPI's update-time
763+
# defaulting keep the live value stable". Emitting a chart-managed
764+
# `replicas: N` here (as this chart did before
765+
# cozystack/cozystack#3231) drains workers on every `helm upgrade`
766+
# action until the autoscaler notices unschedulable Pods and
767+
# scales back up — a real disruption window on every routine
768+
# platform bump.
747769
strategy:
748770
rollingUpdate:
749771
maxSurge: {{ $group.maxReplicas }}

‎packages/apps/kubernetes/tests/nodegroups_default_test.yaml‎

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,3 +126,121 @@ tests:
126126
path: kind
127127
value: MachineDeployment
128128
matchMany: true
129+
130+
# Regression guard for cozystack/cozystack#3231. The chart intentionally
131+
# OMITS `MachineDeployment.spec.replicas` so the cluster-autoscaler owns
132+
# the field alone. Two independent CAPI mechanisms carry the guarantee:
133+
# the defaulting webhook seeds `spec.replicas` from the
134+
# `cluster-api-autoscaler-node-group-min-size` annotation on Create, and
135+
# `calculateMachineDeploymentReplicas` preserves the existing live
136+
# value on Update when the incoming spec has `replicas: nil` — so a
137+
# `helm upgrade` re-rendering this template does not drain workers the
138+
# autoscaler has scaled above min-size. Flux helm-controller applies
139+
# this chart through the Helm SDK three-way merge (not SSA), so field-
140+
# manager ownership is not the mechanism at play; the guarantee is
141+
# purely "field absent from the rendered manifest → CAPI's update-time
142+
# defaulting preserves the live value". Two matched cases so a future
143+
# refactor cannot silently re-hardcode the field nor break the
144+
# annotation-driven min/max wiring:
145+
# * minReplicas: 0 (the render-side OIDC bats path — MachineDeployment
146+
# emitted with the min-size annotation "0", no chart-owned replicas
147+
# field, no worker DataVolumes provisioned until scale-out).
148+
# * minReplicas: 2 (the kubernetes-latest / kubernetes-previous bats
149+
# path — MachineDeployment emitted with the min-size annotation "2",
150+
# no chart-owned replicas field, defaulting webhook fills it to 2 on
151+
# Create).
152+
- it: MachineDeployment omits chart-managed spec.replicas — zero-minReplicas case
153+
release:
154+
name: test-k8s
155+
namespace: tenant-test
156+
set:
157+
_namespace:
158+
etcd: etcd
159+
ingress: nginx
160+
host: example.com
161+
_cluster:
162+
cluster-domain: cozy.local
163+
nodeGroups:
164+
worker0:
165+
minReplicas: 0
166+
maxReplicas: 10
167+
instanceType: "u1.medium"
168+
diskSize: 20Gi
169+
storageClass: ""
170+
roles:
171+
- worker
172+
resources: {}
173+
gpus: []
174+
kubelet: {}
175+
asserts:
176+
# spec.replicas must not appear in the rendered manifest — CAPI's
177+
# defaulting webhook will seed it from the annotation on Create.
178+
- notExists:
179+
path: spec.replicas
180+
documentSelector:
181+
path: kind
182+
value: MachineDeployment
183+
matchMany: true
184+
# Autoscaler annotation wiring stays intact.
185+
- equal:
186+
path: metadata.annotations["cluster.x-k8s.io/cluster-api-autoscaler-node-group-min-size"]
187+
value: "0"
188+
documentSelector:
189+
path: kind
190+
value: MachineDeployment
191+
matchMany: true
192+
- equal:
193+
path: metadata.annotations["cluster.x-k8s.io/cluster-api-autoscaler-node-group-max-size"]
194+
value: "10"
195+
documentSelector:
196+
path: kind
197+
value: MachineDeployment
198+
matchMany: true
199+
200+
- it: MachineDeployment omits chart-managed spec.replicas — nonzero-minReplicas case
201+
release:
202+
name: test-k8s
203+
namespace: tenant-test
204+
set:
205+
_namespace:
206+
etcd: etcd
207+
ingress: nginx
208+
host: example.com
209+
_cluster:
210+
cluster-domain: cozy.local
211+
nodeGroups:
212+
worker0:
213+
minReplicas: 2
214+
maxReplicas: 10
215+
instanceType: "u1.medium"
216+
diskSize: 20Gi
217+
storageClass: ""
218+
roles:
219+
- worker
220+
resources: {}
221+
gpus: []
222+
kubelet: {}
223+
asserts:
224+
- notExists:
225+
path: spec.replicas
226+
documentSelector:
227+
path: kind
228+
value: MachineDeployment
229+
matchMany: true
230+
- equal:
231+
path: metadata.annotations["cluster.x-k8s.io/cluster-api-autoscaler-node-group-min-size"]
232+
value: "2"
233+
documentSelector:
234+
path: kind
235+
value: MachineDeployment
236+
matchMany: true
237+
# Symmetric with the zero-minReplicas case above — both annotations
238+
# must land, because CAPI's calculateMachineDeploymentReplicas
239+
# defaults to 1 (not to minSize) if either annotation is missing.
240+
- equal:
241+
path: metadata.annotations["cluster.x-k8s.io/cluster-api-autoscaler-node-group-max-size"]
242+
value: "10"
243+
documentSelector:
244+
path: kind
245+
value: MachineDeployment
246+
matchMany: true

‎packages/apps/kubernetes/values.schema.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
}
1818
},
1919
"nodeGroups": {
20-
"description": "Worker nodes configuration map. When left empty, a single default node group `md0` (one `ingress-nginx` worker) is provisioned. Provide your own groups to take full control — they are not merged with the default, so you may name and omit groups freely.",
20+
"description": "Worker nodes configuration map. When left empty, a default node group `md0` is emitted with `minReplicas: 0` and `roles: [ingress-nginx]` — the MachineDeployment renders but provisions no workers until an unschedulable Pod triggers the cluster-autoscaler (or an operator scales the group manually). Enabling `addons.ingressNginx.enabled: true` on a CR with default `nodeGroups: {}` therefore requires either supplying a nodeGroup with `roles: [ingress-nginx]` and `minReplicas \u003e= 1`, or waiting for the autoscaler to bring up the default md0 in response to the ingress-nginx controller Pods becoming Pending. Provide your own groups to take full control — they are not merged with the default, so you may name and omit groups freely.",
2121
"type": "object",
2222
"default": {},
2323
"additionalProperties": {

‎packages/apps/kubernetes/values.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ storageClass: replicated
8989
## @field {[]GPU} gpus - List of GPUs to attach (NVIDIA driver requires at least 4 GiB RAM).
9090
## @field {Kubelet} [kubelet] - Kubelet resource reservations for this node group.
9191

92-
## @param {map[string]NodeGroup} nodeGroups - Worker nodes configuration map. When left empty, a single default node group `md0` (one `ingress-nginx` worker) is provisioned. Provide your own groups to take full control — they are not merged with the default, so you may name and omit groups freely.
92+
## @param {map[string]NodeGroup} nodeGroups - Worker nodes configuration map. When left empty, a default node group `md0` is emitted with `minReplicas: 0` and `roles: [ingress-nginx]` — the MachineDeployment renders but provisions no workers until an unschedulable Pod triggers the cluster-autoscaler (or an operator scales the group manually). Enabling `addons.ingressNginx.enabled: true` on a CR with default `nodeGroups: {}` therefore requires either supplying a nodeGroup with `roles: [ingress-nginx]` and `minReplicas >= 1`, or waiting for the autoscaler to bring up the default md0 in response to the ingress-nginx controller Pods becoming Pending. Provide your own groups to take full control — they are not merged with the default, so you may name and omit groups freely.
9393
nodeGroups: {}
9494

9595
##

‎packages/system/kubernetes-rd/cozyrds/kubernetes.yaml‎

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.

0 commit comments

Comments
 (0)