Merge pull request kubevirt#13127 from awels/validate_webhook_snapsho…

…t_volumes_12 [release-1.2] Allow modifications to VM spec during VM snapshot, except disks/volumes
fossedihelm · Oct 28, 2024 · c34534d · c34534d
2 parents eab6fd9 + ff22bbd
commit c34534d
Show file tree

Hide file tree

Showing 2 changed files with 93 additions and 4 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
@@ -713,10 +713,17 @@ func validateSnapshotStatus(ar *admissionv1.AdmissionRequest, vm *v1.VirtualMach
 		}}
 	}
 
-	if !equality.Semantic.DeepEqual(oldVM.Spec, vm.Spec) {
+	if !compareVolumes(oldVM.Spec.Template.Spec.Volumes, vm.Spec.Template.Spec.Volumes) {
+		return []metav1.StatusCause{{
+			Type:    metav1.CauseTypeFieldValueNotSupported,
+			Message: fmt.Sprintf("Cannot update VM disks or volumes until snapshot %q completes", *vm.Status.SnapshotInProgress),
+			Field:   k8sfield.NewPath("spec").String(),
+		}}
+	}
+	if !compareRunningSpec(&oldVM.Spec, &vm.Spec) {
 		return []metav1.StatusCause{{
 			Type:    metav1.CauseTypeFieldValueNotSupported,
-			Message: fmt.Sprintf("Cannot update VM spec until snapshot %q completes", *vm.Status.SnapshotInProgress),
+			Message: fmt.Sprintf("Cannot update VM running state until snapshot %q completes", *vm.Status.SnapshotInProgress),
 			Field:   k8sfield.NewPath("spec").String(),
 		}}
 	}
@@ -736,3 +743,33 @@ func (admitter *VMsAdmitter) isMigrationInProgress(vmi *v1.VirtualMachineInstanc
 	}
 	return nil
 }
+
+func compareVolumes(old, new []v1.Volume) bool {
+	if len(old) != len(new) {
+		return false
+	}
+
+	for i, volume := range old {
+		if !equality.Semantic.DeepEqual(volume, new[i]) {
+			return false
+		}
+	}
+
+	return true
+}
+
+func compareRunningSpec(old, new *v1.VirtualMachineSpec) bool {
+	if old == nil || new == nil {
+		// This should never happen, but just in case return false
+		return false
+	}
+
+	// Its impossible to get here while both running and RunStrategy are nil.
+	if old.Running != nil && new.Running != nil {
+		return *old.Running == *new.Running
+	}
+	if old.RunStrategy != nil && new.RunStrategy != nil {
+		return *old.RunStrategy == *new.RunStrategy
+	}
+	return false
+}
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
@@ -1608,6 +1608,17 @@ var _ = Describe("Validating VM Admitter", func() {
 
 	DescribeTable("when snapshot is in progress, should", func(mutateFn func(*v1.VirtualMachine) bool) {
 		vmi := api.NewMinimalVMI("testvmi")
+		vmi.Spec.Domain.Devices.Disks = []v1.Disk{
+			{
+				Name: "orginalvolume",
+			},
+		}
+		vmi.Spec.Volumes = []v1.Volume{
+			{
+				Name:         "orginalvolume",
+				VolumeSource: v1.VolumeSource{EmptyDisk: &v1.EmptyDiskSource{}},
+			},
+		}
 		vm := &v1.VirtualMachine{
 			Spec: v1.VirtualMachineSpec{
 				Running: &[]bool{false}[0],
@@ -1642,13 +1653,54 @@ var _ = Describe("Validating VM Admitter", func() {
 
 		if !allow {
 			Expect(resp.Result.Details.Causes).To(HaveLen(1))
-			Expect(resp.Result.Details.Causes[0].Field).To(Equal("spec"))
+			Expect(resp.Result.Details.Causes[0].Field).To(Equal("spec"), resp.Result.Details.Causes[0].Message)
 		}
 	},
-		Entry("reject update to spec", func(vm *v1.VirtualMachine) bool {
+		Entry("reject update to disks", func(vm *v1.VirtualMachine) bool {
+			vm.Spec.Template.Spec.Domain.Devices.Disks = []v1.Disk{
+				{
+					Name: "testvolume",
+				},
+			}
+			vm.Spec.Template.Spec.Volumes = []v1.Volume{
+				{
+					Name:         "testvolume",
+					VolumeSource: v1.VolumeSource{EmptyDisk: &v1.EmptyDiskSource{}},
+				},
+			}
+			return false
+		}),
+		Entry("reject adding volumes", func(vm *v1.VirtualMachine) bool {
+			vm.Spec.Template.Spec.Domain.Devices.Disks = append(vm.Spec.Template.Spec.Domain.Devices.Disks, v1.Disk{
+				Name: "testvolume",
+			})
+			vm.Spec.Template.Spec.Volumes = append(vm.Spec.Template.Spec.Volumes, v1.Volume{
+				Name:         "testvolume",
+				VolumeSource: v1.VolumeSource{EmptyDisk: &v1.EmptyDiskSource{}},
+			})
+			return false
+		}),
+		Entry("reject update to volumees", func(vm *v1.VirtualMachine) bool {
+			vm.Spec.Template.Spec.Volumes[0].VolumeSource = v1.VolumeSource{DataVolume: &v1.DataVolumeSource{Name: "fake"}}
+			return false
+		}),
+		Entry("accept update to spec, that is not volumes or running state", func(vm *v1.VirtualMachine) bool {
+			vm.Spec.Template.Spec.Affinity = &k8sv1.Affinity{}
+			return true
+		}),
+		Entry("reject update to running state", func(vm *v1.VirtualMachine) bool {
 			vm.Spec.Running = &[]bool{true}[0]
 			return false
 		}),
+		Entry("accept update to running state, if value doesn't change", func(vm *v1.VirtualMachine) bool {
+			vm.Spec.Running = &[]bool{false}[0]
+			return true
+		}),
+		Entry("reject update to running state, when switch state type", func(vm *v1.VirtualMachine) bool {
+			vm.Spec.Running = nil
+			vm.Spec.RunStrategy = &runStrategyManual
+			return false
+		}),
 		Entry("accept update to metadata", func(vm *v1.VirtualMachine) bool {
 			vm.Annotations = map[string]string{"foo": "bar"}
 			return true