Skip to content

Commit 2b5770c

Browse files
lexfreilexfrei
authored
fix(vm-instance)!: refuse resources that cannot size the VM (#4405)
## What this PR does A vm-instance could ask for half a core and get one, with no error anywhere. `resources.cpu` and `resources.sockets` are typed as quantities, but they end up in `domain.cpu.cores` and `domain.cpu.sockets`, which KubeVirt declares as unsigned integers. The chart cast them with `int64`. That turns `500m` or `1.5` into `0` without an error, and KubeVirt boots a zero core count as one core. The chart now reads these two fields the way an integer field reads them. It decodes the raw value as YAML and stops the render unless the result is a positive whole number that fits in uint32. So `2`, `"2.0"`, `"+2"` and `"1e1"` render as the integer they decode to. `500m`, `2000m`, `1k`, `"1.5"`, `0`, `"0"`, `"-2"`, `"010"` and `"4294967296"` stop the render with an error that names the field. A leading zero is refused because YAML reads `010` as octal 8. The check looks at presence, not truthiness, so an unquoted `0` is refused instead of being read as unset. It is the same rule #4386 uses for kubernetes-nodes. Memory gets a presence check too, which covers the case from the issue thread. A quoted `"0"` was truthy and rendered `requests.memory: "0"`, while an unquoted `0` was falsy and got dropped. Both spellings of zero, and any negative amount, now stop the render. Any other value passes through unchanged, because KubeVirt takes memory as a quantity. That includes an amount below one byte, such as `500m` or `1e-3`, which renders as it does on `main` today; this change does not refuse it. The check lives in `virtual-machine.domainResources`. The VM spec, the instance type decision and the update hook's patch all take their values from that helper. The tests that pinned `cores: 0` as the old behaviour now expect the refusal, and the helper comments about the truncation are trimmed. This is a breaking change. A release that carries one of these values fails its next upgrade until the value is fixed: a cpu or sockets that is not a positive whole number within uint32 or has a leading zero, and a zero or negative memory. This includes a cpu or sockets set on its own next to an `instanceType`, which used to be ignored whatever it held. Some values change meaning instead of failing. A value like `"1e1"` or `"2."` was truncated to 0 by the `int64` cast, so the VM booted with one core. Now it gets the count the value spells, so that VM changes size on upgrade. I refuse these values instead of rounding them, because silent coercion is how `500m` became one core in the first place. The issue asks for a deprecation path, and I did not add a migration that rewrites stored values. It would have to guess what `500m` meant, and both options (one core, or dropping the field) hide the same mistake again. The HelmRelease upgrade retries without uninstalling, so the running VM stays up until someone fixes the value. #4386 makes the same trade-off. The check runs at render time, not at admission. The schema still accepts `500m`, so the API takes the object and the error shows up in the HelmRelease status. Narrowing the schema to integers, as the issue also proposes, would move the error to admission, but it needs a migration of stored values. I left it for a separate change. The field descriptions now say which values are accepted, and `make generate` carried them into the README, the schema, the Go types and the ApplicationDefinition. The new `tests/resources_values_test.yaml` covers the accepted and refused spellings for cpu, sockets and memory. `tests/instancetype_resources_test.yaml` gets a case for a lone cpu next to `cpuModel`. The whole vm-instance suite passes. Every refusal case in the new file, and the `"1e1"` case, fails against the current `main` template. Closes #4104 ### Screenshots No UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: I walked the trigger map against the diff. No field is added, removed or renamed, and no default or enum changes. Only three field descriptions change, and the website reference page is regenerated from the README at release time. ### Release note ```release-note fix(vm-instance)!: resources.cpu and resources.sockets must now be positive whole numbers, and resources.memory a positive quantity. A cpu or sockets that is not a plain whole number within uint32 (for example 500m, 1k or 1.5), or a zero or negative memory, now fails at render time with an error naming the field, instead of silently becoming a one-core VM or a zero memory request. A release carrying such a value fails its next upgrade until the value is fixed. A value such as 1e1 or 2., which used to be truncated to one core, now sizes the VM with the count it spells. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * VM sizing now accepts only positive whole-number CPU and socket values within the supported range. Fractional, millicore, zero, negative, and out-of-range values are rejected during rendering. * Memory values must be positive quantities. A CPU or socket value specified alone does not size the VM and is ignored; incomplete sizing is rejected where required. * **Documentation** * Updated VM instance resource guidance to clarify CPU and socket requirements, memory validation, and how socket values define vCPU topology. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 parents 99de79f + c290dee commit 2b5770c

9 files changed

Lines changed: 456 additions & 84 deletions

File tree

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

Lines changed: 4 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎packages/apps/vm-instance/README.md‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -59,10 +59,10 @@ virtctl ssh <user>@<vm>
5959
| `firmware.bootloader` | Bootloader to boot the VM with: "uefi" (OVMF) or "bios" (SeaBIOS). Empty inherits the instanceProfile default. | `string` | `""` |
6060
| `firmware.secureBoot` | Enable UEFI Secure Boot. Only applies when bootloader is "uefi". | `bool` | `false` |
6161
| `firmware.efiPersistent` | Persist EFI NVRAM (e.g. enrolled Secure Boot keys, such as an updated Microsoft UEFI CA) across reboots. Only applies when bootloader is "uefi". On default RWO storage the VM is node-pinned (no live-migration). | `bool` | `false` |
62-
| `resources` | Resource configuration for the virtual machine. Set whole-number cpu and sockets together with memory to size the VM directly, which overrides `instanceType`. Alongside an `instanceType`, a block that sizes only part of the VM is rejected; cpu or sockets on its own sizes nothing and is ignored. | `object` | `{}` |
63-
| `resources.cpu` | Number of CPU cores allocated. | `quantity` | `""` |
64-
| `resources.memory` | Amount of memory allocated. | `quantity` | `""` |
65-
| `resources.sockets` | Number of CPU sockets (vCPU topology). | `quantity` | `""` |
62+
| `resources` | Resource configuration for the virtual machine. Set whole-number cpu and sockets together with memory to size the VM directly, which overrides `instanceType`. Alongside an `instanceType`, a block that sizes only part of the VM is rejected; a whole-number cpu or sockets on its own sizes nothing and is ignored. | `object` | `{}` |
63+
| `resources.cpu` | Number of CPU cores allocated, as a positive whole number such as `2`. Millicore and fractional quantities are rejected at render time, because the value becomes the VM's integer `domain.cpu.cores`. | `quantity` | `""` |
64+
| `resources.memory` | Amount of memory allocated, as a positive quantity such as `8Gi`. | `quantity` | `""` |
65+
| `resources.sockets` | Number of CPU sockets (vCPU topology), as a positive whole number such as `1`. Millicore and fractional quantities are rejected at render time, because the value becomes the VM's integer `domain.cpu.sockets`. | `quantity` | `""` |
6666
| `sshKeys` | List of SSH public keys for authentication. | `[]string` | `[]` |
6767
| `cloudInit` | Cloud-init user data. | `string` | `""` |
6868
| `cloudInitSeed` | Seed string to generate SMBIOS UUID for the VM. | `string` | `""` |

‎packages/apps/vm-instance/templates/_helpers.tpl‎

Lines changed: 46 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -73,22 +73,54 @@ Generate a stable UUID for cloud-init re-initialization upon upgrade.
7373
{{/*
7474
Domain resources (cpu, memory) as a JSON object.
7575
Used in vm.yaml for rendering and in the update hook for merge patches.
76+
77+
cpu and sockets are schema-typed as quantities but land in domain.cpu.cores
78+
and domain.cpu.sockets, which KubeVirt declares as uint32. Casting 500m to an
79+
integer yields 0, and KubeVirt boots a zero core count as one core, so only a
80+
whole count within uint32 gets past this point. The raw spelling is decoded
81+
as YAML, so +2, 2.0 or 1e1 pass as the integers they decode to; a leading
82+
zero is refused outright, because YAML decodes 010 as octal 8. Presence
83+
rather than truth, so that a 0 is refused instead of read as unset. Memory
84+
stays a quantity, which KubeVirt accepts as such, but a zero or negative
85+
amount sizes nothing.
7686
*/}}
7787
{{- define "virtual-machine.domainResources" -}}
88+
{{- $resources := .Values.resources | default dict -}}
89+
{{- $counts := dict -}}
90+
{{- range $field := list "cpu" "sockets" -}}
91+
{{- $value := index $resources $field -}}
92+
{{- if not (kindIs "invalid" $value) -}}
93+
{{- $raw := printf "%v" $value -}}
94+
{{- $count := (printf "v: %s" $raw | fromYaml).v -}}
95+
{{- $isNumber := or (kindIs "int" $count) (kindIs "int64" $count) (kindIs "float64" $count) -}}
96+
{{- $whole := and $isNumber (gt (float64 $count) 0.0) (le (float64 $count) 4294967295.0) (eq (floor $count) (float64 $count)) -}}
97+
{{- if or (not $whole) (regexMatch "^\\+?0[0-9]" $raw) -}}
98+
{{- fail (printf "resources.%s (%s) must be a positive whole number, such as 2. It becomes the VM's domain.cpu.%s, which KubeVirt declares as an unsigned integer, so a millicore, suffixed, fractional, zero, out-of-range or leading-zero quantity cannot size the guest. Set a whole number, or remove the field." $field $raw (ternary "cores" "sockets" (eq $field "cpu"))) -}}
99+
{{- end -}}
100+
{{- $_ := set $counts $field ($count | int64) -}}
101+
{{- end -}}
102+
{{- end -}}
103+
{{- $memory := index $resources "memory" -}}
104+
{{- if not (kindIs "invalid" $memory) -}}
105+
{{- $rawMemory := printf "%v" $memory -}}
106+
{{- if not (gt (regexFind "^[+-]?([0-9]+(\\.[0-9]*)?|\\.[0-9]+)" $rawMemory | float64) 0.0) -}}
107+
{{- fail (printf "resources.memory (%s) must be a positive quantity, such as 8Gi. A zero or negative amount cannot size the guest. Set an amount, or remove the field." $rawMemory) -}}
108+
{{- end -}}
109+
{{- end -}}
78110
{{- $result := dict -}}
79-
{{- if or .Values.cpuModel (and .Values.resources .Values.resources.cpu .Values.resources.sockets) -}}
111+
{{- if or .Values.cpuModel (and $counts.cpu $counts.sockets) -}}
80112
{{- $cpu := dict -}}
81-
{{- if and .Values.resources .Values.resources.cpu .Values.resources.sockets -}}
82-
{{- $_ := set $cpu "cores" (.Values.resources.cpu | int64) -}}
83-
{{- $_ := set $cpu "sockets" (.Values.resources.sockets | int64) -}}
113+
{{- if and $counts.cpu $counts.sockets -}}
114+
{{- $_ := set $cpu "cores" $counts.cpu -}}
115+
{{- $_ := set $cpu "sockets" $counts.sockets -}}
84116
{{- end -}}
85117
{{- if .Values.cpuModel -}}
86118
{{- $_ := set $cpu "model" .Values.cpuModel -}}
87119
{{- end -}}
88120
{{- $_ := set $result "cpu" $cpu -}}
89121
{{- end -}}
90-
{{- if and .Values.resources .Values.resources.memory -}}
91-
{{- $_ := set $result "resources" (dict "requests" (dict "memory" .Values.resources.memory)) -}}
122+
{{- with $memory -}}
123+
{{- $_ := set $result "resources" (dict "requests" (dict "memory" .)) -}}
92124
{{- end -}}
93125
{{- $result | toJson -}}
94126
{{- end -}}
@@ -104,23 +136,18 @@ domain.cpu.cores and domain.cpu.sockets only when they are non-zero;
104136
validateMemory in memory.go conflicts on domain.resources.requests.memory
105137
whenever the key is present, zero included. A resources block that supplies
106138
all three therefore replaces the matcher, the same way the kubernetes-nodes
107-
chart already sizes its worker VMs. The zero guard on the cpu half is why a
108-
cores of 0 next to a matcher renders instead of failing: KubeVirt does not
109-
call it a conflict, so the instance type simply sizes the VM.
139+
chart already sizes its worker VMs.
110140
111141
Everything below is decided from what domainResources actually emitted, never
112142
from which keys the user set. The two readings differ, and only the first one
113143
is the question KubeVirt asks. domainResources needs cpu and sockets together
114-
before it writes domain.cpu, so resources.cpu on its own emits nothing and
115-
conflicts with nothing; those releases render today and must keep rendering.
116-
It also puts cpu and sockets through int64, which yields 0 for a quantity the
117-
schema accepts, so resources.cpu of 500m emits a domain.cpu.cores of 0 that
118-
sizes nothing while looking set. Reading the emitted values catches that one
119-
alongside a plain unset field, and it survives a resources block the user nulls
120-
outright, which Helm coalescing turns into a nil .Values.resources that a raw
121-
field read would dereference. cpuModel writes domain.cpu.model, which
122-
conflicts only when the instance type declares a model of its own, so it is
123-
deliberately not among the three fields read back here.
144+
before it writes domain.cpu, so a whole resources.cpu on its own emits nothing
145+
and conflicts with nothing; those releases render today and must keep
146+
rendering. Reading the emitted values also survives a resources block the
147+
user nulls outright, which Helm coalescing turns into a nil .Values.resources
148+
that a raw field read would dereference. cpuModel writes domain.cpu.model,
149+
which conflicts only when the instance type declares a model of its own, so
150+
it is deliberately not among the three fields read back here.
124151
125152
A block that emits some sizing but not all of it has no safe reading next to an
126153
instance type: dropping the matcher would size the VM from fields the user

0 commit comments

Comments
 (0)