Skip to content

Commit 4226e35

Browse files
lexfreilexfrei
committed
fix(vm-instance): let complete resources replace the instance type
KubeVirt refuses a VirtualMachine that carries an instancetype matcher and explicit domain sizing at once. validateCPU in pkg/instancetype/apply conflicts on domain.cpu.cores and domain.cpu.sockets when they are non-zero, and validateMemory conflicts on domain.resources.requests.memory whenever the key is present. The chart rendered both, because the matcher keyed off instanceType while the domain sizing keyed off resources, and neither side looked at the other. values.yaml ships instanceType "u1.medium", so clearing that field in the dashboard only falls back to the default. A user who then fills in cpu, sockets and memory gets a VM the webhook rejects, with an error naming fields the form gives them no way to clear. Resources that size the VM completely now replace the matcher, the same way the kubernetes-nodes chart sizes its worker VMs. Sizing that is only partial stops the render, because there is then no safe reading: dropping the matcher would size the VM from fields the user never finished, and keeping it either loses to the webhook or silently discards what was typed. Completeness is decided from what domainResources emitted, not from which keys the user set, and the two readings come apart. 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 a cpu of 500m beside a whole sockets and a memory would read as complete from the values, drop the matcher, and hand KubeVirt a cores of 0 the user never asked for. And a resources block nulled outright leaves .Values.resources nil, where a raw field read is a render-time nil pointer and the emitted object is merely empty. The existence check and the 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 in the hook would see the leftover instanceType, skip the removal patch, and leave spec.instancetype.revisionName on a VM Helm had just given explicit domain resources. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
1 parent d3559d4 commit 4226e35

11 files changed

Lines changed: 665 additions & 41 deletions

File tree

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

Lines changed: 2 additions & 2 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: 30 additions & 30 deletions
Large diffs are not rendered by default.

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

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,70 @@ Used in vm.yaml for rendering and in the update hook for merge patches.
9393
{{- $result | toJson -}}
9494
{{- end -}}
9595

96+
{{/*
97+
Instance type actually rendered into the VM spec, empty when the VM is sized by
98+
explicit resources instead.
99+
100+
KubeVirt v1.9.0 refuses a VirtualMachine that carries an instancetype matcher
101+
together with explicit domain sizing, and the two halves of that rule are not
102+
the same shape. validateCPU in pkg/instancetype/apply/cpu.go conflicts on
103+
domain.cpu.cores and domain.cpu.sockets only when they are non-zero;
104+
validateMemory in memory.go conflicts on domain.resources.requests.memory
105+
whenever the key is present, zero included. A resources block that supplies
106+
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.
110+
111+
Everything below is decided from what domainResources actually emitted, never
112+
from which keys the user set. The two readings differ, and only the first one
113+
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.
124+
125+
A block that emits some sizing but not all of it has no safe reading next to an
126+
instance type: dropping the matcher would size the VM from fields the user
127+
never finished, and keeping it either loses to the webhook or silently discards
128+
what was typed. Stop the render and name both sides instead.
129+
*/}}
130+
{{- define "virtual-machine.effectiveInstanceType" -}}
131+
{{- $domain := include "virtual-machine.domainResources" . | fromJson -}}
132+
{{- $sized := list -}}
133+
{{- $unsized := list -}}
134+
{{- if dig "cpu" "cores" nil $domain -}}
135+
{{- $sized = append $sized "resources.cpu" -}}
136+
{{- else -}}
137+
{{- $unsized = append $unsized "resources.cpu" -}}
138+
{{- end -}}
139+
{{- if dig "cpu" "sockets" nil $domain -}}
140+
{{- $sized = append $sized "resources.sockets" -}}
141+
{{- else -}}
142+
{{- $unsized = append $unsized "resources.sockets" -}}
143+
{{- end -}}
144+
{{- if dig "resources" "requests" "memory" nil $domain -}}
145+
{{- $sized = append $sized "resources.memory" -}}
146+
{{- else -}}
147+
{{- $unsized = append $unsized "resources.memory" -}}
148+
{{- end -}}
149+
{{- if empty $unsized -}}
150+
{{- /* Sized by resources, so no matcher is rendered and nothing conflicts. */ -}}
151+
{{- else if and $sized .Values.instanceType -}}
152+
{{- fail (printf "instanceType %q cannot be combined with a resources block that sizes the VM only in part. Sizing came from %s, but not from %s. resources.cpu and resources.sockets size the VM only when both are set to whole numbers. KubeVirt rejects a VirtualMachine that carries an instancetype matcher together with explicit domain cpu or memory, so either give resources.cpu, resources.sockets and resources.memory values that all size the VM, or clear %s and let the instance type size it." .Values.instanceType (join ", " $sized) (join ", " $unsized) (join ", " $sized)) -}}
153+
{{- else -}}
154+
{{- /* An explicit instanceType: null survives the merge as a nil, which Go
155+
renders as the truthy literal <no value>: a name no lookup matches. */ -}}
156+
{{- .Values.instanceType | default "" -}}
157+
{{- end -}}
158+
{{- end -}}
159+
96160
{{/*
97161
Node Affinity for Windows VMs
98162
*/}}

‎packages/apps/vm-instance/templates/vm-update-hook.yaml‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,13 @@
44
{{- $existingVM := lookup "kubevirt.io/v1" "VirtualMachine" $namespace $vmName -}}
55
{{- $existingService := lookup "v1" "Service" $namespace $vmName -}}
66

7-
{{- $instanceType := .Values.instanceType | default "" -}}
7+
{{- /* The effective type, not the raw value: a VM switched from an instance
8+
type to a complete resources block renders no matcher any more, and the
9+
hook has to strip the one already on the object. Reading .Values
10+
directly here would see the leftover instanceType, skip the patch, and
11+
leave spec.instancetype.revisionName in place for KubeVirt to keep
12+
applying against the domain resources Helm just wrote. */}}
13+
{{- $instanceType := include "virtual-machine.effectiveInstanceType" . -}}
814
{{- $instanceProfile := .Values.instanceProfile | default "" -}}
915
{{- $desiredServiceType := ternary "LoadBalancer" "ClusterIP" .Values.external -}}
1016

‎packages/apps/vm-instance/templates/vm.yaml‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,21 @@
1-
{{- if and .Values.instanceType (not (lookup "instancetype.kubevirt.io/v1beta1" "VirtualMachineClusterInstancetype" "" .Values.instanceType)) }}
2-
{{- fail (printf "Specified instanceType does not exist in the cluster: %s" .Values.instanceType) }}
1+
{{- /* The effective type, not the raw value: once complete resources size the
2+
VM the matcher is gone, and a stale or misspelled instanceType that the
3+
dashboard gives no way to clear must not fail a render that never
4+
references it. */}}
5+
{{- $instanceType := include "virtual-machine.effectiveInstanceType" . }}
6+
{{- if and $instanceType (not (lookup "instancetype.kubevirt.io/v1beta1" "VirtualMachineClusterInstancetype" "" $instanceType)) }}
7+
{{- fail (printf "Specified instanceType does not exist in the cluster: %s" $instanceType) }}
38
{{- end }}
49
{{- if and .Values.instanceProfile (not (lookup "instancetype.kubevirt.io/v1beta1" "VirtualMachineClusterPreference" "" .Values.instanceProfile)) }}
510
{{- fail (printf "Specified instanceProfile does not exist in the cluster: %s" .Values.instanceProfile) }}
611
{{- end }}
12+
{{- /* Raw values here, deliberately, and the only place left in this file that
13+
reads them. This gate asks whether the user supplied a sizing source at
14+
all, which is a question about their input; every other read asks what
15+
gets rendered, which is a question about the output. The two cannot
16+
disagree -- a truthy raw value with an empty effective one means the
17+
resources are complete, so the VM is sized either way -- but swapping
18+
this one to $instanceType would quietly change what it means. */}}
719
{{- if and (not .Values.instanceType) (not (and .Values.resources .Values.resources.cpu .Values.resources.sockets .Values.resources.memory)) }}
820
{{- fail "Either instanceType or resources (cpu, sockets, memory) must be specified" }}
921
{{- end }}
@@ -21,7 +33,7 @@ spec:
2133
{{- else }}
2234
runStrategy: {{ ternary "Always" "Halted" (.Values.running | default true) }}
2335
{{- end }}
24-
{{- with .Values.instanceType }}
36+
{{- with $instanceType }}
2537
instancetype:
2638
kind: VirtualMachineClusterInstancetype
2739
name: {{ . }}

0 commit comments

Comments
 (0)