fix(vm-instance): let complete resources replace the instance type - #4109

What this PR does

Closes #2909.

The matcher keyed off instanceType, the domain sizing keyed off resources, and neither side looked at the other, so the chart could render both. KubeVirt refuses that, and the two halves of its rule are different shapes: validateCPU conflicts on domain.cpu.cores and domain.cpu.sockets only when they are non-zero, while validateMemory conflicts on domain.resources.requests.memory whenever the key is present. values.yaml ships instanceType: "u1.medium", so clearing the field in the dashboard just falls back to that default. Fill in cpu, sockets and memory after clearing it and you get a VM the webhook rejects, naming fields the form gives you no way to clear.

Resources that size the VM completely now suppress the matcher and size it directly. packages/apps/kubernetes-nodes already does this for its worker VMs. Beside an instance type, sizing that is only partial stops the render, naming what sized the VM and what did not; with no instance type in play there is nothing to conflict with, so that half renders as it does today. Letting resources win unconditionally would silently drop the instance type for a VM that sets memory alone, which is a different bug in the same place.

Completeness is decided from what domainResources emitted, never from which keys the user set, and all three fields go through that one reading. The two readings come apart in more than one place. resources.cpu on its own emits nothing, so it conflicts with nothing and goes on rendering a plain instancetype-sized VM. cpu and sockets pass through int64, which yields 0 for a quantity the schema accepts, so cpu: 500m beside a whole sockets and a memory would read as complete from the values, drop the matcher, and hand KubeVirt a cores: 0 the user never asked for. And a resources block nulled outright leaves .Values.resources nil rather than the chart's {}, so a raw field read is a render-time nil pointer where the emitted object is merely empty.

The existence check and the pre-upgrade update hook read the same effective value. Keying the lookup on the raw one would fail a resource-sized render over an instance type it never references, which is the same confusing error this fixes. Reading .Values.instanceType in the hook would see the leftover value, skip the removal patch, and leave spec.instancetype.revisionName on a VM Helm had just given explicit domain resources.

What changes in the rendered output

The rows that stay unchanged are deliberate. virtual-machine.domainResources writes domain.cpu only when cpu and sockets arrive together, so cpu alone emits no override at all. A cores: 0 beside a matcher is not a conflict either, because validateCPU guards on non-zero, so KubeVirt admits it and the instance type sizes the VM. Both shapes render today and go on rendering. cpuModel is left alone for a related reason: it writes domain.cpu.model, which KubeVirt flags only when the instance type declares a model of its own, and no instance type shipped in packages/system/kubevirt-instancetypes declares one.

Why no working release breaks

Every shape that now fails at render was already being rejected by the KubeVirt validating webhook at admission, so it could not have produced a running VM in the first place. The failure moves from an opaque admission error to a render error that names the fields. Nothing that worked stops working.

The unchanged rows above are measured. Each non-failing shape in the table, plus firmware on and off, an explicitly nulled resources block and the three update-hook branches, was rendered against a mocked cluster on both sides and compared document by document. Across 14 value shapes and 78 rendered documents, 77 come out byte-identical. The one that differs is the row the table marks as changed, which is also what the harness was checked against first, so it is known to be able to fail.

Live-VM impact

This changes a rendered VM spec, so an upgrade can resize or restart a running VM. Adding a complete resources block to a VM currently sized by an instance type drops its matcher, and the pre-upgrade hook patches spec.instancetype off the live object and writes the domain resources in its place. Cozystack runs KubeVirt with vmRolloutStrategy: LiveUpdate (packages/system/kubevirt/templates/kubevirt-cr.yaml:125), so KubeVirt applies what it can to the running VMI and marks the rest RestartRequired; I have not measured which half a given resize lands in, so plan the upgrade as if it restarts.

Dropping the matcher drops the whole instance type, sizing and everything else it carries. Of the 69 shipped cluster instancetypes, 36 declare dedicatedCPUPlacement, 36 declare hugepages and 26 declare both. 20 declare NUMA passthrough, 8 declare realtime. A VM moved from rt1.medium to explicit resources loses those characteristics along with the type. Before this change that combination was rejected outright, so the VM never existed; now it is admitted, and the field help says so.

Tests

tests/instancetype_resources_test.yaml covers matcher only, resources only, both complete, each failing partial shape, the shapes that emit nothing, both cpu and sockets truncating to zero, cpuModel beside a matcher and beside complete resources, a stale instance type with and without complete resources, an explicit instanceType: null, an explicitly nulled resources block, and every branch of the update hook's instancetype selector, in both directions plus the two no-op cases. 24 cases in this package, 2127 across make helm-unit-tests, all green. Every case pinning new behaviour was checked against a reverted template to confirm it goes red; the rest pin behaviour this change deliberately leaves alone.

A ten-mutation ladder over the helper, vm.yaml and the update hook leaves no survivor. The memory branch took the most work. Swapping its emitted read for a raw .Values.resources.memory looks like an equivalent mutant, since domainResources emits resources.requests.memory under the same truthiness condition. It is not: a resources block nulled outright leaves .Values.resources nil, and the raw read dereferences it. The case pinning that shape kills the mutant.

The update-hook cases assert the kubectl patch command the Job will run, which is as far as a render-level test reaches. They do not show that KubeVirt accepts the resulting object, that helm-controller converges on a live VM whose spec.instancetype also carries a revisionName written by virt-controller, or which half of a resize lands as a live update rather than RestartRequired. That needs a cluster.

The dig calls survive the shapes that reach them. values.schema.json rejects a non-object resources before render; Helm coalescing turns a nulled one into a nil .Values.resources, which digs to an empty object; and for any other non-map intermediate, current Helm turns the underlying sprig type assertion into a render error rather than a panic.

Interaction with the firmware block

firmware (UEFI/BIOS selection, Secure Boot, persistent EFI NVRAM) landed in this chart in the meantime, and it does not touch the sizing question. domainResources does not read it, so it takes no part in the completeness decision. KubeVirt's instancetype spec carries neither firmware nor domain.features, so nothing an instance type declares can conflict with what the firmware block renders. The boot mode a VM inherits when the block is empty comes from the preference, and the preference is not what gets dropped when complete resources replace the matcher. Both firmware states are covered on each side by the byte-identity comparison above.

README.md, values.schema.json, api/apps/v1alpha1/vminstance/types.go and the ApplicationDefinition all come out of make generate in the package, not out of a hand merge. Re-running it on the result produces no further diff, and the root make generate reports no drift on top of it.

On the ticket's second proposal

#2909 also asks whether the non-empty u1.medium default belongs in the chart at all. That requirement is met, and not by removing the default. The default was a problem because a schema-generated form gave no way to say "no instance type": clearing the field falls back to u1.medium. Filling cpu, sockets and memory now says it directly, so the obstacle the ticket names is gone while the default stays. Dropping the default on top of that would change the rendered spec of every release that never set the field, with nothing left to gain.

Named improvements, not done here

The failure message names the fields that did not size the VM, computed from what was emitted, so for cpu: 4 with no sockets it names resources.cpu even though the user set it. The message states the pair rule that explains this, but naming the actually-missing field first would read better. The two tests that pin domain.cpu.cores: 0 document the pre-existing int64 truncation in prose rather than through a FIXME reference, so a later reader has to read the comment to tell a contract from a bug we chose not to fix. Both are wording; neither changes behaviour.

resources.cpu, sockets and memory are checked for shape and not for value, so memory: "0" and -1Gi pass the schema and fail further down. Both forms fail before and after this change and only the place of the failure moves, so that belongs with #4104 rather than here.

The new template comments run to about 46 lines against about 28 lines of logic. All of it is why rather than what and all of it is accurate, but the helper's third paragraph restates the commit body, and two sentences would carry that weight. The seven-line block in vm.yaml is the one worth keeping: swapping that gate to the effective value survives the entire suite, so what the block records is an equivalence no test can pin, and trimming it would delete the only record of it.

The instanceType description runs to about 370 characters and reaches the dashboard as tooltip help, through values.schema.json and the ApplicationDefinition. That is long for a tooltip. It is accurate as it stands, and rewriting accurate text for length is a fresh unmeasured claim in the surface with the most readers, so it is not done here: the place for the detail about what a dropped instance type takes with it is the chart README, leaving the field help carrying the precedence rule alone.

Choosing a cx1.* type and then giving it complete resources drops what the type guaranteed along with the type, and nothing says so at runtime. That is a consequence of the precedence rule rather than a defect in it, and the field help states it. A runtime signal would need the chart to compare what the type declares against what resources sets, which is its own task.

Defaulting sockets to 1 would let a user size a VM from cpu and memory alone and would remove the commonest way to land in the partial case. It is more than the ticket asked for and it changes the rendered spec of stored releases that set cpu and memory without sockets, so it needs its own call on compatibility.

Not changed

Two pre-existing quirks stay as they are, because both reach past the conflict this fixes. resources fields are read for truthiness, so an explicit 0 reads as unset, in domainResources and in the existing "Either instanceType or resources" check alike. And with no instance type in play, a fractional resources.cpu still truncates to cores: 0 and renders a VM sized by nothing, with no error. The new guard covers only the half where a matcher is present. The tests pin both as they stand, which is what keeps working releases rendering.

Screenshots

No dashboard code is touched. The only user-visible surface is two field descriptions the dashboard renders as help text, regenerated into values.schema.json and the ApplicationDefinition. instanceType now reads "Ignored when resources sizes the VM, which needs whole-number cpu and sockets plus memory; the whole type is dropped then, so any dedicated CPU placement, hugepages, NUMA or realtime settings it carries go with it. While this field is set, a resources block that sizes only part of the VM is rejected outright instead of ignored, so complete it or clear it." resources states the same rule from its own side.

Downstream repositories

• No downstream repository is affected by this change
• cozystack/website - follow-up:
• cozystack/terraform-provider-cozystack - follow-up:
• cozystack/ansible-cozystack - follow-up:
• cozystack/ccp - follow-up:
• cozystack/talm - follow-up:
• cozystack/cozyhr - follow-up:
• cozystack/cozy-proxy - follow-up:
• cozystack/cozystack-telemetry-server - follow-up:
• cozystack/external-apps-example - follow-up:
• cozystack/examples - follow-up:
• cozystack/community - follow-up:

I walked the trigger map against the diff and no listed trigger fires: no package added, renamed or removed; no values.schema.json field added, renamed or removed; no default changed; nothing under hack/ moved; templates/service.yaml and its wholeIP and allowICMP annotations untouched. Two repositories do carry prose this makes incomplete. I left every box empty rather than push follow-ups nobody agreed to, so a maintainer decides.

cozystack/website: content/en/docs/next/virtualization/resources.md opens with "Each virtual machine has these two configuration settings" and lists only instanceType and instanceProfile. The page is hand-written, so the release-time bot that regenerates app reference pages from README.md will not reach it.

cozystack/terraform-provider-cozystack: internal/provider/vminstance_schema.go describes instance_type as "Virtual machine instance type." Its kubernetes_schema.go and kubernetes_nodes_schema.go already spell out the equivalent precedence rule for their own charts. No schema field or default changed here, so its new-field test guard has nothing to catch.

Release note

fix(vm-instance): a complete `resources` block (whole-number cpu and sockets, plus memory) now sizes the VM directly and suppresses the `instanceType` matcher, instead of rendering both and having KubeVirt refuse the VM. Beside an `instanceType`, a `resources` block that sizes the VM only in part now stops the render with a message naming both sides, where it previously produced a VM the admission webhook rejected. Note that suppressing the matcher drops the whole instance type, including any dedicated CPU placement, hugepages, NUMA or realtime settings it carries. A release whose values already carry both converts on the next platform upgrade rather than at a moment the operator picks: the pre-upgrade hook strips `spec.instancetype` off the live VM and writes the domain sizing in its place, so plan that upgrade as if the VM restarts.