Repository navigation
fix(kubernetes-nodes): refuse a resources.cpu that cannot be a core count - #4386
Conversation
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe Kubernetes nodes chart now validates supplied CPU values during rendering for KubeVirt node pools. Tests cover accepted and rejected values, unset and zero behavior, instance-type cases, and Proxmox handling. API and chart descriptions document CPU handling for both substrates. ChangesKubernetes node CPU validation
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The CPU validation and substrate-specific behavior appear consistent with the documented contract, with no identified merge-blocking regression. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change rejects invalid worker sizing earlier without expanding access or privileges. Accepted values retain their existing rendering and sizing precedence. Recovery after interrupted upgrades was not demonstrated. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
github-actions
Bot
added
size/L
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
NOT LGTM: the guard refuses two inputs that render a working pool on main today, and nothing in the upgrade path detects a stored pool carrying one.
Findings
- [MAJOR]
packages/apps/kubernetes-nodes/templates/nodegroup.yaml:54, an existing pool whose stored resources.cpu is 0 or leading-zero stops rendering, with no migration and no pre-adoption check
- [MINOR]
packages/apps/kubernetes-nodes/templates/nodegroup.yaml:52, no upper bound, and the fallback the PR body names is not where it is claimed
Claim mismatches
[PARTIAL] "There is still no upper bound ... and the API server refuses it later": the KubevirtMachineTemplate CRD does not refuse it. Run cores: 1e10 and cores: 4294967296 through apiextensions' schema validator against that CRD's own declaration (type: integer, format: int32) and both are accepted. Whatever refuses them sits downstream of admission, in virt-controller's uint32 decode. Same probe, same numbers as the [MINOR] above.
Caveats
resources.cpuset withresources.memoryunset andinstanceType: ""rendersdomain.cpu.coreswith no memory of any kind and exits 0, whilevalues.yaml:66(propagated to README and the cozyrdopenAPISchema) states "setting only one is rejected at render time". The all-or-nothing guard atnodegroup.yaml:247is conditioned oninstanceTypebeing non-empty. Pre-existing, adjacent to the field this PR tightens, covered by no test.- Per-term mutation of the new predicate:
>0, wholeness and the leading-zero regex each redden the suite alone; deleting$isNumberleaves 97/97 green. Its isolating input iscpu: true, whichvalues.schema.jsonrejects, so it is defence in depth, but no test holds it. - Not executed: live SSA, real admission, virt-controller's typed decode.
| {{- $isNumber := or (kindIs "int" $cores) (kindIs "int64" $cores) (kindIs "float64" $cores) }} | ||
| {{- $whole := and $isNumber (gt (float64 $cores) 0.0) (eq (floor $cores) (float64 $cores)) }} | ||
| {{- if or (not $whole) (regexMatch "^\\+?0[0-9]" $cpu) }} | ||
| {{- fail (printf "nodeGroup %s: resources.cpu (%s) must be a positive whole number of vCPU cores, such as 2. It becomes the worker VM's domain.cpu.cores, which KubeVirt declares as an unsigned integer, so a millicore, fractional or zero quantity cannot size the guest. Set a core count, or remove the field." $groupName $cpu) }} |
There was a problem hiding this comment.
[MAJOR] an existing pool whose stored resources.cpu is 0 or leading-zero stops rendering, with no migration and no pre-adoption check
Rendering every schema-valid spelling at the merge base and at c2d4b80, same values and release name:
cpu base ff0b851 head c2d4b80
2 / +2 / 1e1 / 2.0 / 1e10 renders (same bytes) renders (same bytes)
500m cores: 500m REFUSED (fix)
"1.5" cores: 1.5 REFUSED (fix)
0 and 0.0 renders, no domain.cpu REFUSED (regression)
"010" cores: 010 REFUSED (regression)
"0" and "-2" REFUSED REFUSED
The last two rows are not broken objects on main. A Go probe against the kubevirtmachinetemplates CRD schema (type: integer, format: int32, packages/system/capi-providers-infraprovider/files/infrastructure-components.yaml:7252) decodes 010 to 8 and accepts it, so such a pool runs eight-core workers today. After this lands its HelmRelease stops converging and no later change to that release applies.
The population is not hypothetical. Migration 56 copies resources verbatim from the parent Kubernetes CR's nodeGroups into each new KubernetesNodes CR (packages/core/platform/images/migrations/migrations/56:488), and its pre-adoption guard deliberately excludes it: "Container-typed fields (roles, resources, gpus, kubelet) are excluded" (:468, field list at :472). That guard exists, in its own words, so a stored value the child chart cannot render is pinned and skipped rather than "leaving the pool's child release stuck Failed", which is the state this new guard creates, past a migration that has already stamped and will not re-run.
Either ship a numbered migration that drops resources.cpu: 0 and rewrites a leading-zero value to its decimal form on every KubernetesNodes CR and parent nodeGroups, or extend migration 56 to route such a value through the same warn-pin-skip path. For 0 the rewrite is free: cpu: 0 and cpu absent produce the same KubevirtMachineTemplate hash (kubernetes-myk8s-md0-28d812 both), so no pool rolls. For 010 it is not (010 hashes ce2335, 8 hashes 6e2c38), so that case needs the roll called out.
There was a problem hiding this comment.
I changed the approach so that nothing that renders a working pool on main gets refused, and no migration is needed. An unquoted 0 is falsy and still renders with no domain.cpu, sized by the instancetype. A leading zero like "010" is still written out verbatim. I rendered both against main with the same values and the output is byte-identical, and the tests pin the template names main produces (-28d812 and -ce2335). A quoted "0" is still refused, but main refused it too: the kubelet CPU reservation check can't fit 100m of reservations into zero cores. The ! is gone from the title.
| {{- $cpu := printf "%v" $cpuValue }} | ||
| {{- $cores := (printf "v: %s" $cpu | fromYaml).v }} | ||
| {{- $isNumber := or (kindIs "int" $cores) (kindIs "int64" $cores) (kindIs "float64" $cores) }} | ||
| {{- $whole := and $isNumber (gt (float64 $cores) 0.0) (eq (floor $cores) (float64 $cores)) }} |
There was a problem hiding this comment.
[MINOR] no upper bound, and the fallback the PR body names is not where it is claimed
"1e10" passes $whole and renders cores: 1e10. A Go probe validating that against the kubevirtmachinetemplates CRD schema returns ACCEPTED=true, decoded=1e+10: apiextensions does not enforce format: int32, so the object is admitted and the failure moves to virt-controller's uint32 decode, further from the operator than a render-time error. The PR body says "the API server refuses it later", which does not hold at this layer. One more term (lt (float64 $cores) 4294967296.0) closes it inside the same predicate.
There was a problem hiding this comment.
Fixed. The predicate now also requires the value to be below 4294967296, with tests for "4294967296" and "1e10", plus tests that "1" and "4294967295" still render. I removed the "the API server refuses it later" sentence from the description.
## 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 -->
8b99ea3 to
644ba48
Compare
Aleksei Sviridkin (lexfrei)
changed the title
|
IvanHunters on the rest of the review body:
The proxmox substrate landed after your review and is left out of the check. It converts the quantity to millicores and rounds up to whole cores, so |
644ba48 to
ecdbdbd
Compare
|
IvanHunters I rebased onto |
…ount On the kubevirt substrate a worker pool's resources.cpu, although schema-typed as a Kubernetes quantity, is written verbatim into domain.cpu.cores, which KubeVirt declares as uint32 and the KubevirtMachineTemplate CRD as an integer. A value such as 500m or "1.5" therefore rendered a template the API server refuses, and because the same raw string was truthy it also counted as explicit sizing and dropped the pool's instancetype matcher first. A whole count past the uint32 ceiling rendered too, although KubeVirt cannot hold it. The render now decodes the value the way the emitted cores field is decoded and stops unless that is a whole number between 1 and the uint32 maximum, before the value reaches the VM template or the precedence decision. Only inputs that could not produce a working pool are refused. Every value that rendered one still renders the same bytes, so no pool rolls: a bare numeric zero stays falsy and means unset, and a leading-zero spelling is still emitted as written. The proxmox substrate is left out of the check, because it converts the quantity to millicores and rounds up to whole cores, so 500m is a valid size there. The field description now says what the value means on each substrate. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
ecdbdbd to
4583ee5
Compare
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
A fail-closed render guard on resources.cpu; I rendered the merge base and the head across the whole value domain and no input that produces a working pool today is now refused, so there is no upgrade regression. Both findings from my earlier round are fixed.
Findings
- [NIT]
packages/apps/kubernetes-nodes/templates/nodegroup.yaml:113, guard ceiling is uint32-max but the CRD types cores as int32
- [NIT]
packages/apps/kubernetes-nodes/templates/nodegroup.yaml:112, the $isNumber conjunct is untested and redundant
Checked and correct
Both earlier-round findings are closed, verified by render: bare 0/0.0 and "010" render byte-identically to main (no pool rolls); "0"/"-2" were already refused on main by the kubelet-reservation check; the upper bound now refuses 1e10/4294967296 while 1 and 4294967295 still render. helm unittest is green (206/206) on the reviewed head, and current origin/main touches nothing in this chart and adds no test that references it, so merging introduces no cross-main break.
| {{- $cpu := printf "%v" $cpuValue }} | ||
| {{- $cores := (printf "v: %s" $cpu | fromYaml).v }} | ||
| {{- $isNumber := or (kindIs "int" $cores) (kindIs "int64" $cores) (kindIs "float64" $cores) }} | ||
| {{- if not (and $isNumber (gt (float64 $cores) 0.0) (lt (float64 $cores) 4294967296.0) (eq (floor $cores) (float64 $cores))) }} |
There was a problem hiding this comment.
[NIT] guard ceiling is uint32-max but the CRD types cores as int32
The guard admits cores up to 4294967295 and the message says "unsigned 32-bit integer", matching KubeVirt's own uint32; the KubevirtMachineTemplate CRD declares domain.cpu.cores as {type: integer, format: int32} (max 2147483647) at packages/system/capi-providers-infraprovider/files/infrastructure-components.yaml:2690-2695. A value in (2147483647, 4294967295] passes the guard and renders in exponent form (cores: 3e+09) at that magnitude. format: int32 is advisory and apiextensions does not enforce it, so nothing downstream rejects the sliver, and it is byte-identical to the merge base (not a regression) at a core count no pool uses. Flagging only so the ceiling reads as a deliberate uint32 choice; tighten to 2147483647 for render-time parity with the CRD's declared format.
| {{- if and (not $isProxmox) (not (kindIs "invalid" $cpuValue)) (not (and (not (kindIs "string" $cpuValue)) (eq (float64 $cpuValue) 0.0))) }} | ||
| {{- $cpu := printf "%v" $cpuValue }} | ||
| {{- $cores := (printf "v: %s" $cpu | fromYaml).v }} | ||
| {{- $isNumber := or (kindIs "int" $cores) (kindIs "int64" $cores) (kindIs "float64" $cores) }} |
There was a problem hiding this comment.
[NIT] the $isNumber conjunct is untested and redundant
Removing $isNumber leaves the resources_cpu suite 21/21 green: every schema-valid non-number decodes to a string that float64 maps to 0, which the gt 0 term already refuses, and cpu: true/non-numeric strings are rejected by values.schema.json before the template runs. Harmless defence-in-depth; pin it with an assertion or drop the term.
On 1.6, seven of the twelve originals rule 6 holds back build on code the line never got: cozystack#5071 changes files cozystack#3940 added on main, cozystack#4974 reads a variable only cozystack#4386 defines. Each conflicts on every run, and nothing tells a maintainer which missing pull request the conflict comes from, which every human backport so far had to work out by hand. For each original the policy takes and nothing carries, run now reads what it builds on from git alone. A modified or deleted file the line lacks is named by the first-parent commit on main that added it (V1). A removed or changed line is blamed at M^1, mapped to the first-parent commit that brought the blamed commit, and kept only while its text, edges trimmed, is absent from the line's file (V2c), so a hand backport under another SHA clears it. Generated files and the keys update-crd.sh writes into an ApplicationDefinition are not blamed. Paths go to git literally, blame gets a validated full SHA and no --end-of-options, which git refuses there, and every call has a two-minute deadline. Caps, deadlines, binary files, symlinks, submodules, unmapped commits and skipped rename detection become diagnostics shown with the original on every outcome and in the report; malformed git output stops the run. This commit only annotates; nothing is parked yet. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <[email protected]>
What this PR does
A worker pool's CPU value could land in the VM as something KubeVirt cannot accept.
resources.cpuis typed as a quantity, but on the kubevirt substrate the template wrote it unchanged intodomain.cpu.cores, which isuint32in KubeVirt and an integer in the KubevirtMachineTemplate CRD. Withresources.cpu: 500mthe chart renderedcores: 500m. That string was truthy, so it also counted as explicit sizing and dropped the pool'sinstanceTypefrom 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 onmainand 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 unquoted0is falsy, so the pool renders withoutdomain.cpuand is sized by its instancetype, as onmain. A leading-zero value such as"010"is still emitted as written and keeps the reading it gets onmaintoday, where YAML treats it as octal. I checked both against a render ofmain, and the tests pin the template names thatmainproduces 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 copiesresourcesverbatim, 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
500mis 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 aresource.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
int64turns500minto0without 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.cpuinpackages/extra/computeplaneand in the parentkuberneteschart. computeplane passesresourcesstraight 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
Release note
Summary by CodeRabbit
0is treated as unset.