instancetypes: Clear VM guest memory when ignoring inference failures

With this change the guest memory of VMs is cleared when inference of an instancetype was successful and the IgnoreInferFromVolumeFailure policy was set in the instancetype matcher. This allows to enable inference of instancetypes in virtctl by default without breaking the previous behavior by allowing virtctl to provide a fallback amount of memory to be used when inference fails. Signed-off-by: Felix Matouschek <[email protected]>
nuwang · Oct 10, 2023 · fc1731e · fc1731e
1 parent 89b4e1d
commit fc1731e
Show file tree

Hide file tree

Showing 3 changed files with 88 additions and 3 deletions.
diff --git a/pkg/instancetype/instancetype.go b/pkg/instancetype/instancetype.go
@@ -735,12 +735,12 @@ func (m *InstancetypeMethods) InferDefaultInstancetype(vm *virtv1.VirtualMachine
 		return nil
 	}
 
+	ignoreFailure := vm.Spec.Instancetype.InferFromVolumeFailurePolicy != nil &&
+		*vm.Spec.Instancetype.InferFromVolumeFailurePolicy == virtv1.IgnoreInferFromVolumeFailure
+
 	defaultName, defaultKind, err := m.inferDefaultsFromVolumes(vm, vm.Spec.Instancetype.InferFromVolume, apiinstancetype.DefaultInstancetypeLabel, apiinstancetype.DefaultInstancetypeKindLabel)
 	if err != nil {
 		var ignoreableInferenceErr *IgnoreableInferenceError
-		ignoreFailure := vm.Spec.Instancetype.InferFromVolumeFailurePolicy != nil &&
-			*vm.Spec.Instancetype.InferFromVolumeFailurePolicy == virtv1.IgnoreInferFromVolumeFailure
-
 		if errors.As(err, &ignoreableInferenceErr) && ignoreFailure {
 			//nolint:gomnd
 			log.Log.Object(vm).V(3).Info("Ignored error during inference of instancetype, clearing matcher.")
@@ -751,6 +751,10 @@ func (m *InstancetypeMethods) InferDefaultInstancetype(vm *virtv1.VirtualMachine
 		return err
 	}
 
+	if ignoreFailure {
+		vm.Spec.Template.Spec.Domain.Memory = nil
+	}
+
 	vm.Spec.Instancetype = &virtv1.InstancetypeMatcher{
 		Name: defaultName,
 		Kind: defaultKind,

diff --git a/pkg/virt-api/webhooks/mutating-webhook/mutators/vm-mutator_test.go b/pkg/virt-api/webhooks/mutating-webhook/mutators/vm-mutator_test.go
@@ -30,6 +30,7 @@ import (
 	. "github.com/onsi/gomega"
 	admissionv1 "k8s.io/api/admission/v1"
 	k8sv1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	k8smetav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	k8sfield "k8s.io/apimachinery/pkg/util/validation/field"
@@ -1814,6 +1815,48 @@ var _ = Describe("VirtualMachine Mutator", func() {
 				Kind: defaultInferedKindFromPVC,
 			}))
 		})
+
+		DescribeTable("When inference was successful", func(failurePolicy v1.InferFromVolumeFailurePolicy, expectMemoryCleared bool) {
+			By("Setting guest memory")
+			guestMemory := resource.MustParse("512Mi")
+			vm.Spec.Template.Spec.Domain.Memory = &v1.Memory{
+				Guest: &guestMemory,
+			}
+
+			By("Creating a VM using a PVC as boot and inference Volume")
+			vm.Spec.Instancetype = &v1.InstancetypeMatcher{
+				InferFromVolume:              inferVolumeName,
+				InferFromVolumeFailurePolicy: &failurePolicy,
+			}
+			vm.Spec.Template.Spec.Volumes = []v1.Volume{{
+				Name: inferVolumeName,
+				VolumeSource: v1.VolumeSource{
+					PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{
+						PersistentVolumeClaimVolumeSource: k8sv1.PersistentVolumeClaimVolumeSource{
+							ClaimName: pvc.Name,
+						},
+					},
+				},
+			}}
+			vmSpec, _ := getVMSpecMetaFromResponse(rt.GOARCH)
+
+			expectedInstancetypeMatcher := &v1.InstancetypeMatcher{
+				Name: defaultInferedNameFromPVC,
+				Kind: defaultInferedKindFromPVC,
+			}
+			Expect(vmSpec.Instancetype).To(Equal(expectedInstancetypeMatcher))
+
+			if expectMemoryCleared {
+				Expect(vmSpec.Template.Spec.Domain.Memory).To(BeNil())
+			} else {
+				Expect(vmSpec.Template.Spec.Domain.Memory).ToNot(BeNil())
+				Expect(vmSpec.Template.Spec.Domain.Memory.Guest).ToNot(BeNil())
+				Expect(*vmSpec.Template.Spec.Domain.Memory.Guest).To(Equal(guestMemory))
+			}
+		},
+			Entry("it should clear guest memory when ignoring inference failures", v1.IgnoreInferFromVolumeFailure, true),
+			Entry("it should not clear guest memory when rejecting inference failures", v1.RejectInferFromVolumeFailure, false),
+		)
 	})
 
 	It("should default architecture to compiled architecture when not provided", func() {

diff --git a/tests/instancetype_test.go b/tests/instancetype_test.go
@@ -1347,6 +1347,11 @@ var _ = Describe("[crit:medium][vendor:[email protected]][level:component][sig-c
 		)
 
 		It("should ignore failure when trying to infer defaults from DataVolumeSpec with unsupported DataVolumeSource when policy is set", func() {
+			guestMemory := resource.MustParse("512Mi")
+			vm.Spec.Template.Spec.Domain.Memory = &virtv1.Memory{
+				Guest: &guestMemory,
+			}
+
 			// Inference from blank image source is not supported
 			dv := libdv.NewDataVolume(
 				libdv.WithNamespace(namespace),
@@ -1368,7 +1373,40 @@ var _ = Describe("[crit:medium][vendor:[email protected]][level:component][sig-c
 			By("Validating the VirtualMachine")
 			Expect(vm.Spec.Instancetype).To(BeNil())
 			Expect(vm.Spec.Preference).To(BeNil())
+			Expect(vm.Spec.Template.Spec.Domain.Memory).ToNot(BeNil())
+			Expect(vm.Spec.Template.Spec.Domain.Memory.Guest).ToNot(BeNil())
+			Expect(*vm.Spec.Template.Spec.Domain.Memory.Guest).To(Equal(guestMemory))
 		})
+
+		DescribeTable("should reject VM creation when inference was successful but memory and RejectInferFromVolumeFailure were set", func(explicit bool) {
+			guestMemory := resource.MustParse("512Mi")
+			vm.Spec.Template.Spec.Domain.Memory = &virtv1.Memory{
+				Guest: &guestMemory,
+			}
+
+			vm.Spec.Template.Spec.Volumes = []v1.Volume{{
+				Name: inferFromVolumeName,
+				VolumeSource: v1.VolumeSource{
+					PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{
+						PersistentVolumeClaimVolumeSource: k8sv1.PersistentVolumeClaimVolumeSource{
+							ClaimName: sourceDV.Name,
+						},
+					},
+				},
+			}}
+
+			if explicit {
+				failurePolicy := v1.RejectInferFromVolumeFailure
+				vm.Spec.Instancetype.InferFromVolumeFailurePolicy = &failurePolicy
+			}
+
+			By("Creating the VirtualMachine")
+			vm, err = virtClient.VirtualMachine(namespace).Create(context.Background(), vm)
+			Expect(err).To(MatchError("admission webhook \"virtualmachine-validator.kubevirt.io\" denied the request: VM field spec.template.spec.domain.memory conflicts with selected instance type"))
+		},
+			Entry("with explicitly setting RejectInferFromVolumeFailure", true),
+			Entry("with implicitly setting RejectInferFromVolumeFailure (default)", false),
+		)
 	})
 
 	Context("instance type with dedicatedCPUPlacement enabled", func() {