From c9e474a6e9be9daa8f9a35e191d00508880f0b9a Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Tue, 13 Sep 2022 16:36:08 +0100 Subject: [PATCH] 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 #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 --- .../admitters/vms-admitter.go | 8 +++-- .../admitters/vms-admitter_test.go | 24 ++++++++++++++ tests/storage/snapshot.go | 33 +++---------------- 3 files changed, 34 insertions(+), 31 deletions(-) diff --git a/pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter.go b/pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter.go index f20913177333..8612342f5cca 100644 --- a/pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter.go +++ b/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) } diff --git a/pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter_test.go b/pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter_test.go index 58f7f43b1620..fbd51e50b783 100644 --- a/pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter_test.go +++ b/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()) + + }) }) }) diff --git a/tests/storage/snapshot.go b/tests/storage/snapshot.go index f9f047b1aedf..e9e97b919edd 100644 --- a/tests/storage/snapshot.go +++ b/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() }) }) })