Skip to content

Commit ef86974

Browse files
fix(kubernetes-nodes): plan autoscaler capacity on the cores the VM actually gets (#4974)
On the kubevirt substrate `resources.cpu` is written into `domain.cpu.cores` as spelled and decoded as YAML, so `"010"` gives the VM eight cores, while the cluster-autoscaler capacity annotation parsed the same string as decimal ten (`capacity.cluster-autoscaler.kubernetes.io/cpu: "9000m"` on a node that offers 7000m). This is the inconsistency reported in #4554 and in the comment on #4546. - The YAML-decoded value the range check already computes is kept, and on kubevirt the capacity annotation and the reserved-CPU check start from it. - The kubelet reservations are not touched. The worker machineconfig (`_talosconfigtemplate.tpl`) sizes them from the raw string read as a decimal, and the TalosConfigTemplate name is a hash of that config, so reading the decoded value there would roll every leading-zero pool once. Instead the annotation subtracts the reservations the machineconfig really carries from the cores the VM really has: for `"010"` that is 8000m − 2 × 500m = `"7000m"`, the node's real allocatable, while `"8"` keeps `"7200m"` (2 × 400m). Tests pin both sides: the annotation and the bootstrap `configRef` name in `resources_cpu_test.yaml`, and `systemReserved.cpu` / `kubeReserved.cpu` in the machineconfig in `talos_worker_config_test.yaml`. - No pool rolls. Checked by rendering `nodegroup.yaml` on main and on this branch for nine spellings (`"2"`, `2`, `2.0`, `"2.0"`, `"+2"`, `"1e1"`, `"010"`, `999999`, `4294967295`): the only difference is the capacity annotation of the `"010"` case, so the `KubevirtMachineTemplate` name and the TalosConfigTemplate `configRef` stay as they are. The machineconfig template itself is not changed. - Proxmox parses the quantity as decimal for the machine template and for the reservations alike, so it never had the inconsistency and is unchanged (pinned by a test). - The `resources` description says the value is decoded as YAML, what that means for a leading zero, and that the reservations still read it as a decimal. README, schema, the RD and the Go type are regenerated from it. This closes #4554 without the migration and the render-time refusal it proposes. A refusal at render breaks a stored pool on its next upgrade, a pattern in the OpenAPI schema breaks every update of an existing CR, and a platform-wide migration over every `KubernetesNodes` and parent `nodeGroups` entry is a lot of blast radius for a spelling nobody has reported in a live cluster. With this change the autoscaler plans for the node the VM really is. What is left of a leading zero is the octal reading itself and reservations sized from the decimal value, and both are documented. The unquoted zero of the issue is already harmless: it is falsy, counts as unset, and renders the same bytes as an absent field. If a migration is still wanted, please reopen. Verified with `helm unittest .` in `packages/apps/kubernetes-nodes` (38 suites, 233 tests, 12 snapshots; pointing the annotation back at the decimal string turns the `"010"` case red with `9000m`), `make generate` (no diff), and a trial merge with #4973 (clean, 243 tests green). Closes #4554 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * KubeVirt VM sizing and autoscaler capacity now reflect YAML-decoded CPU values. For example, `"010"` represents eight VM cores, while kubelet reservations interpret it as decimal ten. `"8"` and `"10"` yield `7200m` and `9000m` of autoscaler capacity, respectively. Proxmox continues to interpret `"010"` as ten cores, yielding `9000m` of capacity. * **Documentation** * Clarified explicit worker CPU and memory sizing behavior, including how leading-zero CPU values affect VM sizing, reservations, and capacity calculations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Backported-from: 60d1f53
2 parents 7b692a9 + d68a163 commit ef86974

8 files changed

Lines changed: 158 additions & 11 deletions

File tree

‎api/apps/v1alpha1/kubernetesnodes/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.

‎packages/apps/kubernetes-nodes/README.md‎

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

‎packages/apps/kubernetes-nodes/templates/nodegroup.yaml‎

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,19 @@
104104
or 1e1 pass as the integers they decode to, and a leading-zero
105105
spelling passes as whatever YAML reads it as. A bare numeric zero
106106
is left alone: it is falsy, so the pool renders without domain.cpu
107-
and is sized by its instancetype. */}}
107+
and is sized by its instancetype.
108+
109+
The decoded value is kept: it is the core count the VM actually gets,
110+
and the cluster-autoscaler capacity annotation and the reservation
111+
check below start from it rather than from a decimal parse of the raw
112+
string. The kubelet reservations do not: the worker machineconfig in
113+
_talosconfigtemplate.tpl sizes them from the raw string, and the
114+
TalosConfigTemplate name is a hash of that config, so reading the
115+
decoded value there would roll every leading-zero pool. The two
116+
readings differ for a leading zero: YAML reads "010" as octal, so the
117+
VM has 8 cores while the kubelet reserves 2 x 500m (5% of decimal 10),
118+
and the node offers 7000m. */}}
119+
{{- $kubevirtCores := "" }}
108120
{{- $cpuValue := (.Values.resources | default dict).cpu }}
109121
{{- if and (not $isProxmox) (not (kindIs "invalid" $cpuValue)) (not (and (not (kindIs "string" $cpuValue)) (eq (float64 $cpuValue) 0.0))) }}
110122
{{- $cpu := printf "%v" $cpuValue }}
@@ -113,6 +125,7 @@
113125
{{- if not (and $isNumber (gt (float64 $cores) 0.0) (lt (float64 $cores) 4294967296.0) (eq (floor $cores) (float64 $cores))) }}
114126
{{- fail (printf "nodeGroup %s: resources.cpu (%s) must be a whole number of vCPU cores between 1 and 4294967295, such as 2. On the kubevirt substrate it becomes the worker VM's domain.cpu.cores, which KubeVirt declares as an unsigned 32-bit integer, so a millicore or fractional quantity cannot size the guest. Set a core count, or remove the field." $groupName $cpu) }}
115127
{{- end }}
128+
{{- $kubevirtCores = printf "%.0f" (float64 $cores) }}
116129
{{- end }}
117130
{{- /* Assembled once in _helpers.tpl and read back here and in
118131
talos-reconcile-job.yaml, so the TalosConfigTemplate name this file
@@ -749,6 +762,12 @@ spec:
749762
{{- else if and $instanceType $instanceType.spec $instanceType.spec.cpu $instanceType.spec.cpu.guest }}
750763
{{- $effectiveCpu = $instanceType.spec.cpu.guest | toString }}
751764
{{- end }}
765+
{{- /* $effectiveCpu sizes the reservations exactly as the machineconfig
766+
does. $vmCpu is what the node has: the YAML-decoded count on kubevirt
767+
(see the note at the top of this file); proxmox hands the raw quantity
768+
to the machine template through cpuToMillicores, so there the two are
769+
the same string. */}}
770+
{{- $vmCpu := $kubevirtCores | default $effectiveCpu }}
752771
{{- $autoReservedMi := 256 }}
753772
{{- if $effectiveMemory }}
754773
{{- $effectiveMemMi := divf (include "cozy-lib.resources.toFloat" $effectiveMemory | float64) 1048576.0 | int }}
@@ -865,14 +884,14 @@ spec:
865884
{{- end }}
866885
{{- end }}
867886
{{/* Validate that total reserved CPU does not exceed effective node CPU */}}
868-
{{- if $effectiveCpu }}
887+
{{- if $vmCpu }}
869888
{{- $sysCpuMillis := include "kubernetes.cpuToMillicores" $systemReservedCpu | float64 }}
870889
{{- $kubeCpuMillis := include "kubernetes.cpuToMillicores" $kubeReservedCpu | float64 }}
871890
{{- $totalCpuMillis := addf $sysCpuMillis $kubeCpuMillis }}
872-
{{- $effectiveCpuMillicoresV := include "kubernetes.cpuToMillicores" $effectiveCpu | int }}
891+
{{- $effectiveCpuMillicoresV := include "kubernetes.cpuToMillicores" $vmCpu | int }}
873892
{{- $effectiveCpuMillis := float64 $effectiveCpuMillicoresV }}
874893
{{- if ge $totalCpuMillis $effectiveCpuMillis }}
875-
{{- fail (printf "nodeGroup %s: total reserved CPU (systemReservedCpu=%s + kubeReservedCpu=%s) must be less than effective node CPU (%s cores)" $groupName $systemReservedCpu $kubeReservedCpu $effectiveCpu) }}
894+
{{- fail (printf "nodeGroup %s: total reserved CPU (systemReservedCpu=%s + kubeReservedCpu=%s) must be less than effective node CPU (%s cores)" $groupName $systemReservedCpu $kubeReservedCpu $vmCpu) }}
876895
{{- end }}
877896
{{- end }}
878897
{{/* Guard: explicit kubelet overrides require effective memory for validation */}}
@@ -944,10 +963,10 @@ metadata:
944963
{{- $allocatableMi := divf $allocatableBytes 1048576.0 | floor }}
945964
capacity.cluster-autoscaler.kubernetes.io/memory: "{{ $allocatableMi }}Mi"
946965
{{- end }}
947-
{{- if $effectiveCpu }}
966+
{{- if $vmCpu }}
948967
{{- $sysCpuMillisA := include "kubernetes.cpuToMillicores" $systemReservedCpu | float64 }}
949968
{{- $kubeCpuMillisA := include "kubernetes.cpuToMillicores" $kubeReservedCpu | float64 }}
950-
{{- $effectiveCpuMillicoresA := include "kubernetes.cpuToMillicores" $effectiveCpu | int }}
969+
{{- $effectiveCpuMillicoresA := include "kubernetes.cpuToMillicores" $vmCpu | int }}
951970
{{- $effectiveCpuMillisA := float64 $effectiveCpuMillicoresA }}
952971
{{- $allocatableCpuMillis := subf $effectiveCpuMillisA (addf $sysCpuMillisA $kubeCpuMillisA) }}
953972
{{- $allocatableCpuMillisInt := $allocatableCpuMillis | ceil | int }}

‎packages/apps/kubernetes-nodes/tests/resources_cpu_test.yaml‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,70 @@ tests:
188188
- failedTemplate:
189189
errorPattern: 'nodeGroup md0: resources.cpu \(0\) must be a whole number of vCPU cores between 1 and 4294967295'
190190

191+
# A leading zero is read as octal by YAML, so "010" gives the VM eight cores.
192+
# The worker machineconfig still sizes the kubelet reservations from the
193+
# decimal string (2 x 500m, 5% of 10 clamped to 500m), and both template
194+
# names are the ones main renders for this value, so nothing rolls. The
195+
# autoscaler annotation takes those reservations off the cores the VM really
196+
# has: 8000m - 1000m, the node's real allocatable.
197+
- it: plans a leading-zero pool for the cores the VM gets, less the reservations it carries
198+
set:
199+
resources:
200+
cpu: "010"
201+
memory: 4Gi
202+
asserts:
203+
- equal:
204+
path: spec.template.spec.virtualMachineTemplate.spec.template.spec.domain.cpu.cores
205+
value: 8
206+
documentSelector:
207+
path: kind
208+
value: KubevirtMachineTemplate
209+
- equal:
210+
path: metadata.name
211+
value: kubernetes-myk8s-md0-ce2335
212+
documentSelector:
213+
path: kind
214+
value: KubevirtMachineTemplate
215+
- equal:
216+
path: spec.template.spec.bootstrap.configRef.name
217+
value: kubernetes-myk8s-md0-c570c9
218+
documentSelector:
219+
path: kind
220+
value: MachineDeployment
221+
- equal:
222+
path: metadata.annotations["capacity.cluster-autoscaler.kubernetes.io/cpu"]
223+
value: "7000m"
224+
documentSelector:
225+
path: kind
226+
value: MachineDeployment
227+
228+
# Same VM as "010", but the machineconfig reserves 2 x 400m here.
229+
- it: plans a plain eight with the reservations eight cores get
230+
set:
231+
resources:
232+
cpu: "8"
233+
memory: 4Gi
234+
documentSelector:
235+
path: kind
236+
value: MachineDeployment
237+
asserts:
238+
- equal:
239+
path: metadata.annotations["capacity.cluster-autoscaler.kubernetes.io/cpu"]
240+
value: "7200m"
241+
242+
- it: keeps the decimal reading for a value without a leading zero
243+
set:
244+
resources:
245+
cpu: "10"
246+
memory: 4Gi
247+
documentSelector:
248+
path: kind
249+
value: MachineDeployment
250+
asserts:
251+
- equal:
252+
path: metadata.annotations["capacity.cluster-autoscaler.kubernetes.io/cpu"]
253+
value: "9000m"
254+
191255
# The schema's quantity pattern admits a leading minus.
192256
- it: refuses a negative core count
193257
set:
@@ -294,3 +358,32 @@ tests:
294358
- equal:
295359
path: spec.template.spec.numCores
296360
value: 1
361+
362+
# No YAML round trip on this substrate: the quantity is parsed as decimal
363+
# for the machine template and for the reservations alike, so a leading
364+
# zero is ten cores on both sides and nothing here changes for it.
365+
- it: reads a leading zero as decimal on proxmox, on both sides
366+
set:
367+
substrate: proxmox
368+
proxmox:
369+
templateTags: [capmox-template, talos]
370+
full: true
371+
network:
372+
bridge: vmbr0
373+
dnsServers: ["8.8.8.8"]
374+
resources:
375+
cpu: "010"
376+
memory: 4Gi
377+
asserts:
378+
- equal:
379+
path: spec.template.spec.numCores
380+
value: 10
381+
documentSelector:
382+
path: kind
383+
value: ProxmoxMachineTemplate
384+
- equal:
385+
path: metadata.annotations["capacity.cluster-autoscaler.kubernetes.io/cpu"]
386+
value: "9000m"
387+
documentSelector:
388+
path: kind
389+
value: MachineDeployment

‎packages/apps/kubernetes-nodes/tests/talos_worker_config_test.yaml‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,41 @@ tests:
7474
path: spec.template.spec.containers[0].command[2]
7575
pattern: 'kubeReserved:\s*\n\s+cpu: "100m"\s*\n\s+memory: "512Mi"'
7676

77+
# The machineconfig sizes the reservations from resources.cpu as a decimal
78+
# string, so a leading-zero "010" (an eight-core VM, see
79+
# resources_cpu_test.yaml) reserves 5% of ten, clamped to 500m, and keeps
80+
# the config, and so the TalosConfigTemplate name, it has always had.
81+
- it: sizes the reservations of a leading-zero pool from the decimal string
82+
set:
83+
gpus: []
84+
resources:
85+
cpu: "010"
86+
memory: 16Gi
87+
documentSelector:
88+
path: kind
89+
value: Job
90+
asserts:
91+
- matchRegex:
92+
path: spec.template.spec.containers[0].command[2]
93+
pattern: 'systemReserved:\s*\n\s+cpu: "500m"'
94+
- matchRegex:
95+
path: spec.template.spec.containers[0].command[2]
96+
pattern: 'kubeReserved:\s*\n\s+cpu: "500m"'
97+
98+
- it: sizes the reservations of a plain eight from eight cores
99+
set:
100+
gpus: []
101+
documentSelector:
102+
path: kind
103+
value: Job
104+
asserts:
105+
- matchRegex:
106+
path: spec.template.spec.containers[0].command[2]
107+
pattern: 'systemReserved:\s*\n\s+cpu: "400m"'
108+
- matchRegex:
109+
path: spec.template.spec.containers[0].command[2]
110+
pattern: 'kubeReserved:\s*\n\s+cpu: "400m"'
111+
77112
# On proxmox the worker kubelet runs with an external cloud provider: the
78113
# node is tainted uninitialized until the cloud-controller-manager clears it
79114
# and applies the topology labels the CSI node plugin needs. On kubevirt no

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@
6868
}
6969
},
7070
"resources": {
71-
"description": "Explicit CPU and memory for each worker node, as an alternative to `instanceType` sizing. Optional: when omitted, the node is sized by `instanceType`. When both `cpu` and `memory` are set, they take precedence and `instanceType` is ignored (the instancetype is omitted from the VM, since KubeVirt cannot override an instancetype's CPU/memory). Set both `cpu` and `memory` together or neither; setting only one is rejected at render time.",
71+
"description": "Explicit CPU and memory for each worker node, as an alternative to `instanceType` sizing. Optional: when omitted, the node is sized by `instanceType`. When both `cpu` and `memory` are set, they take precedence and `instanceType` is ignored (the instancetype is omitted from the VM, since KubeVirt cannot override an instancetype's CPU/memory). Set both `cpu` and `memory` together or neither; setting only one is rejected at render time. On the kubevirt substrate `cpu` is written into the VM as spelled and decoded as YAML, so a leading zero is read as octal, as in any Kubernetes manifest: `\"010\"` is eight cores, and the autoscaler capacity counts eight. The kubelet reservations still read the value as a decimal, so `\"010\"` reserves what `\"10\"` does.",
7272
"type": "object",
7373
"default": {},
7474
"properties": {

‎packages/apps/kubernetes-nodes/values.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ diskSize: 20Gi
6767
## @param {[]string} roles - List of node roles. Each role `r` labels nodes with `node-role.kubernetes.io/<r>`. Use `[ingress-nginx]` for a pool that hosts the tenant ingress-nginx controller.
6868
roles: []
6969

70-
## @param {Resources} [resources] - Explicit CPU and memory for each worker node, as an alternative to `instanceType` sizing. Optional: when omitted, the node is sized by `instanceType`. When both `cpu` and `memory` are set, they take precedence and `instanceType` is ignored (the instancetype is omitted from the VM, since KubeVirt cannot override an instancetype's CPU/memory). Set both `cpu` and `memory` together or neither; setting only one is rejected at render time.
70+
## @param {Resources} [resources] - Explicit CPU and memory for each worker node, as an alternative to `instanceType` sizing. Optional: when omitted, the node is sized by `instanceType`. When both `cpu` and `memory` are set, they take precedence and `instanceType` is ignored (the instancetype is omitted from the VM, since KubeVirt cannot override an instancetype's CPU/memory). Set both `cpu` and `memory` together or neither; setting only one is rejected at render time. On the kubevirt substrate `cpu` is written into the VM as spelled and decoded as YAML, so a leading zero is read as octal, as in any Kubernetes manifest: `"010"` is eight cores, and the autoscaler capacity counts eight. The kubelet reservations still read the value as a decimal, so `"010"` reserves what `"10"` does.
7171
resources: {}
7272

7373
## @param {quantity} [podCpuLimit] - CPU limit for the worker VM's virt-launcher `compute` container, which is the ceiling the guest vCPU threads, the QEMU emulator thread and the IO threads share. Distinct from `resources.cpu`, which sizes the guest itself. Cannot be combined with `instanceType` sizing: KubeVirt refuses a VM that references an instancetype and also sets `domain.resources.limits.cpu`, so the chart rejects that combination at render time. Sizing the pool by `resources` (both `cpu` and `memory`) is what drops the instancetype from the VM, which is where the limit is usable. When unset, KubeVirt derives the limit from the guest vCPU count in any namespace carrying a ResourceQuota that limits CPU, so a worker sized at N vCPUs runs under a ceiling of N with the emulation overhead taken out of the same N. Setting a value declares the limit on the VM directly, which is both what lands it on the container and what stops KubeVirt deriving one. It moves the CPU request with it: KubeVirt copies a declared CPU limit into the CPU request when the VM declares no request of its own, so a pool that sets only this field asks the scheduler for its whole ceiling instead of the fraction of the vCPU count it asked for before, and a ceiling the nodes cannot spare leaves the workers Pending on `Insufficient cpu`. Pair it with `podCpuRequest` to move the ceiling without moving the request. Zero means no ceiling at all rather than the tightest one, since the kubelet turns a zero CPU limit into no CFS quota while the declared key still stops KubeVirt deriving one — but it is only accepted together with `podCpuRequest: 0`, because KubeVirt keeps deriving a request from the vCPU count when the VM declares none and a request above a declared limit of zero is rejected when the worker's Pod is created. A negative value is refused at render time. Changing it rolls the pool, because the worker VM template is named by a hash of its content. Keep in sync with the parent kubernetes chart.

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

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

0 commit comments

Comments
 (0)