Repository navigation
Commit 0c7b598
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 -->10 files changed
Lines changed: 201 additions & 50 deletions
File tree
- api/apps/v1alpha1/kubernetes
- hack/e2e-apps
- packages
- apps/kubernetes
- templates
- tests
- system/kubernetes-rd/cozyrds
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
49 | 49 | | |
50 | 50 | | |
51 | 51 | | |
52 | | - | |
53 | | - | |
54 | | - | |
55 | | - | |
56 | | - | |
57 | | - | |
58 | | - | |
59 | | - | |
60 | | - | |
61 | | - | |
62 | | - | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
63 | 62 | | |
64 | 63 | | |
65 | 64 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
57 | 57 | | |
58 | 58 | | |
59 | 59 | | |
60 | | - | |
61 | | - | |
62 | | - | |
63 | | - | |
64 | | - | |
65 | | - | |
66 | | - | |
67 | | - | |
68 | | - | |
69 | | - | |
70 | | - | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
71 | 73 | | |
72 | 74 | | |
73 | 75 | | |
| |||
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
124 | 124 | | |
125 | 125 | | |
126 | 126 | | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
127 | 137 | | |
128 | 138 | | |
129 | 139 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
743 | 743 | | |
744 | 744 | | |
745 | 745 | | |
746 | | - | |
| 746 | + | |
| 747 | + | |
| 748 | + | |
| 749 | + | |
| 750 | + | |
| 751 | + | |
| 752 | + | |
| 753 | + | |
| 754 | + | |
| 755 | + | |
| 756 | + | |
| 757 | + | |
| 758 | + | |
| 759 | + | |
| 760 | + | |
| 761 | + | |
| 762 | + | |
| 763 | + | |
| 764 | + | |
| 765 | + | |
| 766 | + | |
| 767 | + | |
| 768 | + | |
747 | 769 | | |
748 | 770 | | |
749 | 771 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
126 | 126 | | |
127 | 127 | | |
128 | 128 | | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
17 | 17 | | |
18 | 18 | | |
19 | 19 | | |
20 | | - | |
| 20 | + | |
21 | 21 | | |
22 | 22 | | |
23 | 23 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
89 | 89 | | |
90 | 90 | | |
91 | 91 | | |
92 | | - | |
| 92 | + | |
93 | 93 | | |
94 | 94 | | |
95 | 95 | | |
| |||
Large diffs are not rendered by default.
0 commit comments