instancetype: Apply instancetype to copy of VM during admission

Previously the VirtualMachine validation webhook would attempt to apply
any referenced instancetype and preferences to the provided
VirtualMachineInstanceSpec within the VirtualMachine before validating
the contents. Doing so however would cause calls to
validateSnapshotStatus to fail when a snapshot was in progress as the
now updated VirtualMachineInstanceSpec differed from the original. In
issue kubevirt#8435 this left the Snapshot controller unable to update the
VirtualMachine with a required finalizer, blocking the progress of the
overall snapshot.

This change corrects this by applying any referenced instancetype and
preferences to a copy of the VirtualMachine and using this updated copy
to validate the fully rendered VirtualMachineInstanceSpec.

Signed-off-by: Lee Yarwood <lyarwood@redhat.com>

pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter.go

@@ -99,12 +99,16 @@ func (admitter *VMsAdmitter) Admit(ar *admissionv1.AdmissionReview) *admissionv1
 		return webhookutils.ToAdmissionResponseError(err)
 	}
 
-	causes := admitter.applyInstancetypeToVm(&vm)
+	// We apply any referenced instancetype and preferences early here to the VirtualMachine in order to
+	// validate the resulting VirtualMachineInstanceSpec below. As we don't want to persist these changes
+	// we pass a copy of the original VirtualMachine here and to the validation call below.
+	vmCopy := vm.DeepCopy()
+	causes := admitter.applyInstancetypeToVm(vmCopy)
 	if len(causes) > 0 {
 		return webhookutils.ToAdmissionResponse(causes)
 	}
 
-	causes = ValidateVirtualMachineSpec(k8sfield.NewPath("spec"), &vm.Spec, admitter.ClusterConfig, accountName)
+	causes = ValidateVirtualMachineSpec(k8sfield.NewPath("spec"), &vmCopy.Spec, admitter.ClusterConfig, accountName)
 	if len(causes) > 0 {
 		return webhookutils.ToAdmissionResponse(causes)
 	}

pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter_test.go

@@ -1519,6 +1519,30 @@ var _ = Describe("Validating VM Admitter", func() {
 			Expect(response.Result.Details.Causes[0].Field).
 				To(Equal("spec.template.spec.domain.resources.requests.memory"))
 		})
+
+		It("should not apply instancetype to the VMISpec of the original VM", func() {
+
+			instancetypeMethods.FindInstancetypeSpecFunc = func(_ *v1.VirtualMachine) (*instancetypev1alpha1.VirtualMachineInstancetypeSpec, error) {
+				return &instancetypev1alpha1.VirtualMachineInstancetypeSpec{}, nil
+			}
+
+			// Mock out ApplyToVmiFunc so that it applies some changes to the CPU of the provided VMISpec
+			instancetypeMethods.ApplyToVmiFunc = func(_ *k8sfield.Path, _ *instancetypev1alpha1.VirtualMachineInstancetypeSpec, _ *instancetypev1alpha1.VirtualMachinePreferenceSpec, vmiSpec *v1.VirtualMachineInstanceSpec) instancetype.Conflicts {
+				vmiSpec.Domain.CPU = &v1.CPU{Cores: 1, Threads: 1, Sockets: 1}
+				return nil
+			}
+
+			// Nil out CPU within the DomainSpec of the VMISpec being admitted to assert this remains untouched
+			vm.Spec.Template.Spec.Domain.CPU = nil
+
+			// The VM should be admitted successfully
+			response := admitVm(vmsAdmitter, vm)
+			Expect(response.Allowed).To(BeTrue())
+
+			// Ensure CPU has remained nil within the now admitted VMISpec
+			Expect(vm.Spec.Template.Spec.Domain.CPU).To(BeNil())
+
+		})
 	})
 })
 

tests/storage/snapshot.go

@@ -1285,12 +1285,11 @@ var _ = SIGDescribe("VirtualMachineSnapshot Tests", func() {
 			)
 		})
 
-		Context("With VM using instancetype and preferences", Label("instancetype"), func() {
+		Context("With VM using instancetype and preferences", func() {
 
 			var instancetype *instancetypev1alpha1.VirtualMachineInstancetype
 
 			BeforeEach(func() {
-
 				instancetype = &instancetypev1alpha1.VirtualMachineInstancetype{
 					ObjectMeta: metav1.ObjectMeta{
 						GenerateName: "vm-instancetype-",
@@ -1305,28 +1304,20 @@ var _ = SIGDescribe("VirtualMachineSnapshot Tests", func() {
 						},
 					},
 				}
-
 				instancetype, err := virtClient.VirtualMachineInstancetype(util.NamespaceTestDefault).Create(context.Background(), instancetype, metav1.CreateOptions{})
 				Expect(err).ToNot(HaveOccurred())
 
-				running := false
 				vm = tests.NewRandomVMWithDataVolumeWithRegistryImport(
 					cd.DataVolumeImportUrlForContainerDisk(cd.ContainerDiskAlpine),
 					util.NamespaceTestDefault,
 					snapshotStorageClass,
 					corev1.ReadWriteOnce,
 				)
-				vm.Spec.Running = &running
+				vm.Spec.Template.Spec.Domain.Resources = v1.ResourceRequirements{}
 				vm.Spec.Instancetype = &v1.InstancetypeMatcher{
 					Name: instancetype.Name,
 					Kind: "VirtualMachineInstanceType",
 				}
-
-				vm.Spec.Template.Spec.Networks = []v1.Network{}
-
-				// Clear the domainspec so the instnacetype doesn't conflict
-				vm.Spec.Template.Spec.Domain = v1.DomainSpec{}
-
 				vm, err = virtClient.VirtualMachine(vm.Namespace).Create(vm)
 				Expect(err).ToNot(HaveOccurred())
 
@@ -1335,27 +1326,11 @@ var _ = SIGDescribe("VirtualMachineSnapshot Tests", func() {
 				}
 			})
 
-			It("Bug #8435 - should create a snapshot successfully, currently fails as source remains locked", func() {
+			It("Bug #8435 - should create a snapshot successfully", func() {
 				snapshot = newSnapshot()
 				snapshot, err = virtClient.VirtualMachineSnapshot(snapshot.Namespace).Create(context.Background(), snapshot, metav1.CreateOptions{})
 				Expect(err).ToNot(HaveOccurred())
-
-				Eventually(func() bool {
-					snapshot, err = virtClient.VirtualMachineSnapshot(vm.Namespace).Get(context.Background(), snapshot.Name, metav1.GetOptions{})
-					Expect(err).ToNot(HaveOccurred())
-
-					if snapshot.Status == nil {
-						return false
-					}
-
-					// FIXME - The source remains unlocked here as the snapshot controller is unable to apply a finalizer to the VirtualMachine.
-					c1 := snapshot.Status.Conditions[0]
-					if c1.Type == snapshotv1.ConditionProgressing && c1.Status == corev1.ConditionFalse && strings.Contains(c1.Reason, "Source not locked") {
-						return true
-					}
-
-					return false
-				}, 90*time.Second, 10*time.Second).Should(BeTrue())
+				waitSnapshotReady()
 			})
 		})
 	})