Repository navigation
fix(vm-instance)!: refuse resources that cannot size the VM - #4405
Conversation
resources.cpu and resources.sockets are typed as quantities, but they land in domain.cpu.cores and domain.cpu.sockets, which KubeVirt declares as uint32. The chart cast them with int64, which turns 500m, 1.5 or 1e1 into 0 without an error, and KubeVirt boots a zero core count as one core. The user asked for half a core and got a one-core VM with nothing reporting it. The raw spelling is now decoded as YAML, the way an integer field reads it, and the render stops unless the result is a positive whole number that fits in uint32. A leading zero is refused, because YAML reads 010 as octal 8. The check keys on presence rather than truthiness, so an unquoted 0 is refused instead of being read as unset. Memory gets the same presence check. A quoted "0" was truthy and rendered requests.memory: "0", while an unquoted 0 was falsy and dropped, so one quantity took two paths. A zero or negative amount now stops the render in both spellings. BREAKING CHANGE: values that used to render now either fail or render differently. A cpu or sockets that does not decode to a plain whole number within uint32 (a millicore, suffixed such as 1k, fractional, zero, negative or leading-zero value), and a zero or negative memory, fail the next upgrade of the release until the value is fixed. This includes cpu or sockets set on its own beside an instanceType, which used to be ignored whatever it held. A spelling that the int64 cast truncated to 0 but that YAML decodes to a whole number, such as the exponent 1e1 or the trailing dot 2., used to boot one core and now sizes the VM with the count it spells, so such a VM changes size on upgrade. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
github-actions
Bot
added
area/virtualization
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe VM-instance Helm template now validates CPU, socket, and memory resource values before rendering VM sizing. Tests cover accepted and rejected values, including resources used with instance types. API, chart, and VM-instance documentation describe the validation requirements. ChangesVM resource validation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The resource validation and its documented constraints are ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation [
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
This goes after the root cause rather than the symptom, and refusing a value beats rounding it into something that looks set. The breaking half is declared instead of smuggled in. What's left is smaller: one missing comparison, and two sentences that undersell what the guard already does.
Findings
- [MINOR]
packages/apps/vm-instance/templates/_helpers.tpl:106, the memory guard has no floor, and KubeVirt puts one at 1M
- [MINOR]
packages/apps/vm-instance/values.yaml:23, the cpu and sockets descriptions name two of the five refused shapes
- [MINOR]
packages/apps/vm-instance/templates/_helpers.tpl:93, the error quotes a number the operator never wrote
Notes
- "It is the same rule #4386 uses for kubernetes-nodes" is not quite it yet: #4386 is still open and its
$wholehas no upper bound, while this one caps at4294967295. Both counts land in auint32, so until #4386 picks the cap up the two charts accept different sets. - Nothing under
hack/e2e-chainsaw/setsresourcesat all (vminstance.yamlsizes byinstanceType), so the coverage here is helm-unittest only. - That a failed upgrade leaves the VM running is read off
pkg/registry/apps/application/rest.go:1608and the nil-remediation assertions inrest_helmrelease_spec_test.go:107,126, not replayed on a cluster. Worth one manual pass on a disposable cluster with an existing VMInstance carryingcpu: 500mbefore this ships, to confirm the HelmRelease message actually reaches the tenant. - Corners with a real
instanceTypecannot be rendered outside the unittest harness, so the "a whole cpu on its own beside an instance type is ignored" half rests on the suite.
| {{- $memory := index $resources "memory" -}} | ||
| {{- if not (kindIs "invalid" $memory) -}} | ||
| {{- $rawMemory := printf "%v" $memory -}} | ||
| {{- if not (gt (regexFind "^[+-]?([0-9]+(\\.[0-9]*)?|\\.[0-9]+)" $rawMemory | float64) 0.0) -}} |
There was a problem hiding this comment.
[MINOR] the memory guard has no floor, and KubeVirt puts one at 1M
regexFind "^[+-]?(...)" takes the leading numeric prefix, so the suffix never enters the comparison and the test is only "greater than zero". Rendered at this head, memory of 500m, 1n, 1m, 0.0001Gi, 1e-3 and 0.5 all pass through unchanged; only 0, "0" and a negative are refused.
KubeVirt refuses the same values a step later. At v1.9.0, the tag pinned by packages/system/kubevirt-operator/templates/kubevirt-operator.yaml:8685, pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go:1135 reads:
} else if spec.Domain.Resources.Requests.Memory().Value() > 0 && spec.Domain.Resources.Requests.Memory().Cmp(resource.MustParse("1M")) < 0 {
causes = append(causes, metav1.StatusCause{
Type: metav1.CauseTypeFieldValueInvalid,
Message: fmt.Sprintf("%s '%s': must be greater than or equal to 1M.", field.Child("domain", "resources", "requests", "memory").String(),
spec.Domain.Resources.Requests.Memory()),Value() rounds a sub-unit quantity up to 1, so memory: 500m lands squarely in that window. The result is the shape this PR exists to close, one field over: the render succeeds, admission rejects the object, and the message names spec.template.spec.domain.resources.requests.memory rather than the field the user set. The PR puts this out of scope and it is unchanged from main, so it does not block, but the fix is the comparison you already have: require at least 1M instead of more than zero.
| ## @field {quantity} [cpu] - Number of CPU cores allocated. | ||
| ## @field {quantity} [memory] - Amount of memory allocated. | ||
| ## @field {quantity} [sockets] - Number of CPU sockets (vCPU topology). | ||
| ## @field {quantity} [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`. |
There was a problem hiding this comment.
[MINOR] the cpu and sockets descriptions name two of the five refused shapes
"Millicore and fractional quantities are rejected at render time" leaves out a whole count carried by a suffix, a value past uint32, zero and negative, and a leading zero, each of which the new suite has a case for. Rendered at this head, resources.cpu: "1k" fails with resources.cpu (1k) must be a positive whole number, such as 2.
cozyvalues-gen copies this prose verbatim into values.schema.json, README.md and the -rd openAPISchema, which is what the dashboard shows, so an operator reading the field is told 1k is fine. Naming the rule beats naming two ways to break it: "a plain whole number from 1 to 4294967295, no suffix, no leading zero".
| {{- range $field := list "cpu" "sockets" -}} | ||
| {{- $value := index $resources $field -}} | ||
| {{- if not (kindIs "invalid" $value) -}} | ||
| {{- $raw := printf "%v" $value -}} |
There was a problem hiding this comment.
[MINOR] the error quotes a number the operator never wrote
$raw is printf "%v" of the value, and a numeric literal reaches the template as a Go float64, so %v prints it in %g form. A VMInstance carrying resources.cpu: 4294967296 fails with resources.cpu (4.294967296e+09) must be a positive whole number, and grepping the CR for that string finds nothing. printf "%.0f" for an integral count, %v for the rest.
…ount (#4386) ## What this PR does A worker pool's CPU value could land in the VM as something KubeVirt cannot accept. `resources.cpu` is typed as a quantity, but on the kubevirt substrate the template wrote it unchanged into `domain.cpu.cores`, which is `uint32` in KubeVirt and an integer in the KubevirtMachineTemplate CRD. With `resources.cpu: 500m` the chart rendered `cores: 500m`. That string was truthy, so it also counted as explicit sizing and dropped the pool's `instanceType` from the VM, and then the API server refused the template anyway. The render now decodes the value as YAML, the same way `cores: <raw>` is decoded, and stops unless the result is a whole number from 1 to 4294967295. `500m`, `"1.5"` and `"1e10"` now fail at render time with an error naming the field, before the value reaches the VM template or the instancetype decision. `"0"` and `"-2"` were already refused on `main` and still are. Only inputs that could not produce a working pool are refused. Every value that rendered a pool before renders the same bytes now, so the content-hashed template keeps its name and nothing rolls. That includes `2`, `"2.0"`, `"+2"` and `"1e1"`, and also two odd cases I kept on purpose because pools may already carry them. An unquoted `0` is falsy, so the pool renders without `domain.cpu` and is sized by its instancetype, as on `main`. A leading-zero value such as `"010"` is still emitted as written and keeps the reading it gets on `main` today, where YAML treats it as octal. I checked both against a render of `main`, and the tests pin the template names that `main` produces for them. This is where kubernetes-nodes and vm-instance now differ. Since #4405, vm-instance refuses a leading zero and an unquoted `0`, and this chart does not. Stored worker pools can carry these values, because migration 56 copies `resources` verbatim, so refusing them here without a migration would fail those pools' releases on their next upgrade. #4554 tracks the migration and the refusal after it. The proxmox substrate is not checked. It converts the quantity to millicores and rounds up to whole cores, so `500m` is a valid size there and becomes one core. The field description now says what the value means on each substrate. Memory is left alone. It goes to `domain.memory.guest`, which is a `resource.Quantity`, so a quantity string is valid there. The issue suggested coercing to an integer, as vm-instance did before #4405. I didn't, because `int64` turns `500m` into `0` without an error. The new helm-unittest suite covers each input form, both preserved cases, the upper bound, the instancetype case and the proxmox path. Two neighbouring descriptions still say only "CPU available.": `nodeGroups[].resources.cpu` in `packages/extra/computeplane` and in the parent `kubernetes` chart. computeplane passes `resources` straight into this chart, so the new check applies to its pools as well. I left both alone to keep this change to one chart. Closes #4150 ### 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: ### Release note ```release-note fix(kubernetes-nodes): on the kubevirt substrate, a worker pool whose resources.cpu cannot be a whole vCPU core count (for example 500m, 1.5 or a value above 4294967295) now fails at render time with a clear error, instead of rendering a VM template that is refused later or dropping the pool's instanceType. Values that rendered a working pool before render unchanged. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * KubeVirt CPU settings now accept whole-core values from 1 to 4,294,967,295. Fractional, millicore, negative, and larger values are rejected; an unquoted `0` is treated as unset. * Proxmox rounds fractional and millicore CPU values up to whole cores. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
What this PR does
A vm-instance could ask for half a core and get one, with no error anywhere.
resources.cpuandresources.socketsare typed as quantities, but they end up indomain.cpu.coresanddomain.cpu.sockets, which KubeVirt declares as unsigned integers. The chart cast them withint64. That turns500mor1.5into0without 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 reads010as octal 8. The check looks at presence, not truthiness, so an unquoted0is 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 renderedrequests.memory: "0", while an unquoted0was 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 as500mor1e-3, which renders as it does onmaintoday; 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 pinnedcores: 0as 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 theint64cast, 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
500mbecame 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 what500mmeant, 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 generatecarried them into the README, the schema, the Go types and the ApplicationDefinition.The new
tests/resources_values_test.yamlcovers the accepted and refused spellings for cpu, sockets and memory.tests/instancetype_resources_test.yamlgets a case for a lone cpu next tocpuModel. The whole vm-instance suite passes. Every refusal case in the new file, and the"1e1"case, fails against the currentmaintemplate.Closes #4104
Screenshots
No UI change.
Downstream repositories
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
Summary by CodeRabbit
Bug Fixes
Documentation