Merge pull request kubevirt#6839 from ShellyKa13/upgrade-snapshot-test

vm restore fixes + add snapshots and restore to kubevirt upgrade test

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

@@ -95,7 +95,7 @@ func (admitter *VMRestoreAdmitter) Admit(ar *admissionv1.AdmissionReview) *admis
 			case core.GroupName:
 				switch vmRestore.Spec.Target.Kind {
 				case "VirtualMachine":
-					causes, targetUID, err = admitter.validateCreateVM(targetField.Child("name"), ar.Request.Namespace, vmRestore.Spec.Target.Name)
+					causes, targetUID, err = admitter.validateCreateVM(targetField, ar.Request.Namespace, vmRestore.Spec.Target.Name)
 					if err != nil {
 						return webhookutils.ToAdmissionResponseError(err)
 					}
@@ -141,7 +141,7 @@ func (admitter *VMRestoreAdmitter) Admit(ar *admissionv1.AdmissionReview) *admis
 				cause := metav1.StatusCause{
 					Type:    metav1.CauseTypeFieldValueInvalid,
 					Message: fmt.Sprintf("VirtualMachineRestore %q in progress", r.Name),
-					Field:   targetField.Child("name").String(),
+					Field:   targetField.String(),
 				}
 				causes = append(causes, cause)
 			}
@@ -203,10 +203,19 @@ func (admitter *VMRestoreAdmitter) validateCreateVM(field *k8sfield.Path, namesp
 	}
 
 	if rs != v1.RunStrategyHalted {
-		cause := metav1.StatusCause{
-			Type:    metav1.CauseTypeFieldValueInvalid,
-			Message: fmt.Sprintf("VirtualMachine %q is running", name),
-			Field:   field.String(),
+		var cause metav1.StatusCause
+		if vm.Spec.Running != nil && *vm.Spec.Running {
+			cause = metav1.StatusCause{
+				Type:    metav1.CauseTypeFieldValueInvalid,
+				Message: fmt.Sprintf("VirtualMachine %q is not stopped", name),
+				Field:   field.String(),
+			}
+		} else {
+			cause = metav1.StatusCause{
+				Type:    metav1.CauseTypeFieldValueInvalid,
+				Message: fmt.Sprintf("VirtualMachine %q run strategy has to be %s", name, v1.RunStrategyHalted),
+				Field:   field.String(),
+			}
 		}
 		causes = append(causes, cause)
 	}

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

@@ -55,6 +55,7 @@ var _ = Describe("Validating VirtualMachineRestore Admitter", func() {
 
 	t := true
 	f := false
+	runStrategyManual := v1.RunStrategyManual
 
 	snapshot := &snapshotv1.VirtualMachineSnapshot{
 		ObjectMeta: metav1.ObjectMeta{
@@ -159,7 +160,7 @@ var _ = Describe("Validating VirtualMachineRestore Admitter", func() {
 			resp := createTestVMRestoreAdmitter(config, nil, snapshot).Admit(ar)
 			Expect(resp.Allowed).To(BeFalse())
 			Expect(len(resp.Result.Details.Causes)).To(Equal(1))
-			Expect(resp.Result.Details.Causes[0].Field).To(Equal("spec.target.name"))
+			Expect(resp.Result.Details.Causes[0].Field).To(Equal("spec.target"))
 		})
 
 		It("should reject when VM and snapshot do not exist", func() {
@@ -182,7 +183,7 @@ var _ = Describe("Validating VirtualMachineRestore Admitter", func() {
 			resp := createTestVMRestoreAdmitter(config, nil).Admit(ar)
 			Expect(resp.Allowed).To(BeFalse())
 			Expect(len(resp.Result.Details.Causes)).To(Equal(2))
-			Expect(resp.Result.Details.Causes[0].Field).To(Equal("spec.target.name"))
+			Expect(resp.Result.Details.Causes[0].Field).To(Equal("spec.target"))
 			Expect(resp.Result.Details.Causes[1].Field).To(Equal("spec.virtualMachineSnapshotName"))
 		})
 
@@ -293,7 +294,33 @@ var _ = Describe("Validating VirtualMachineRestore Admitter", func() {
 				resp := createTestVMRestoreAdmitter(config, vm, snapshot).Admit(ar)
 				Expect(resp.Allowed).To(BeFalse())
 				Expect(len(resp.Result.Details.Causes)).To(Equal(1))
-				Expect(resp.Result.Details.Causes[0].Field).To(Equal("spec.target.name"))
+				Expect(resp.Result.Details.Causes[0].Field).To(Equal("spec.target"))
+			})
+
+			It("should reject when VM run strategy is not halted", func() {
+				restore := &snapshotv1.VirtualMachineRestore{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "restore",
+						Namespace: "default",
+					},
+					Spec: snapshotv1.VirtualMachineRestoreSpec{
+						Target: corev1.TypedLocalObjectReference{
+							APIGroup: &apiGroup,
+							Kind:     "VirtualMachine",
+							Name:     vmName,
+						},
+						VirtualMachineSnapshotName: vmSnapshotName,
+					},
+				}
+
+				vm.Spec.RunStrategy = &runStrategyManual
+
+				ar := createRestoreAdmissionReview(restore)
+				resp := createTestVMRestoreAdmitter(config, vm, snapshot).Admit(ar)
+				Expect(resp.Allowed).To(BeFalse())
+				Expect(len(resp.Result.Details.Causes)).To(Equal(1))
+				Expect(resp.Result.Details.Causes[0].Field).To(Equal("spec.target"))
+				Expect(resp.Result.Details.Causes[0].Message).To(Equal(fmt.Sprintf("VirtualMachine %q run strategy has to be %s", vmName, v1.RunStrategyHalted)))
 			})
 
 			It("should reject when snapshot does not exist", func() {
@@ -489,7 +516,7 @@ var _ = Describe("Validating VirtualMachineRestore Admitter", func() {
 				resp := createTestVMRestoreAdmitter(config, vm, snapshot, restoreInProcess).Admit(ar)
 				Expect(resp.Allowed).To(BeFalse())
 				Expect(len(resp.Result.Details.Causes)).To(Equal(1))
-				Expect(resp.Result.Details.Causes[0].Field).To(Equal("spec.target.name"))
+				Expect(resp.Result.Details.Causes[0].Field).To(Equal("spec.target"))
 			})
 
 			It("should accept when VM is not running", func() {

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

@@ -522,7 +522,8 @@ func validateRestoreStatus(ar *admissionv1.AdmissionRequest, vm *v1.VirtualMachi
 	}
 
 	if !reflect.DeepEqual(oldVM.Spec, vm.Spec) {
-		if *vm.Spec.Running {
+		strategy, _ := vm.RunStrategy()
+		if strategy != v1.RunStrategyHalted {
 			return []metav1.StatusCause{{
 				Type:    metav1.CauseTypeFieldValueNotSupported,
 				Message: fmt.Sprintf("Cannot start VM until restore %q completes", *vm.Status.RestoreInProgress),

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

@@ -67,6 +67,8 @@ var _ = Describe("Validating VM Admitter", func() {
 	}
 
 	notRunning := false
+	runStrategyManual := v1.RunStrategyManual
+	runStrategyHalted := v1.RunStrategyHalted
 
 	BeforeEach(func() {
 		vmiInformer, _ = testutils.NewFakeInformerFor(&v1.VirtualMachineInstance{})
@@ -1337,11 +1339,10 @@ var _ = Describe("Validating VM Admitter", func() {
 		}),
 	)
 
-	table.DescribeTable("when restore is in progress, should", func(mutateFn func(*v1.VirtualMachine) bool) {
+	table.DescribeTable("when restore is in progress, should", func(mutateFn func(*v1.VirtualMachine) bool, updateRunStrategy bool) {
 		vmi := api.NewMinimalVMI("testvmi")
 		vm := &v1.VirtualMachine{
 			Spec: v1.VirtualMachineSpec{
-				Running: &[]bool{false}[0],
 				Template: &v1.VirtualMachineInstanceTemplateSpec{
 					Spec: vmi.Spec,
 				},
@@ -1350,6 +1351,11 @@ var _ = Describe("Validating VM Admitter", func() {
 				RestoreInProgress: &[]string{"testrestore"}[0],
 			},
 		}
+		if updateRunStrategy {
+			vm.Spec.RunStrategy = &runStrategyHalted
+		} else {
+			vm.Spec.Running = &[]bool{false}[0]
+		}
 		oldObjectBytes, _ := json.Marshal(vm)
 
 		allow := mutateFn(vm)
@@ -1379,19 +1385,23 @@ var _ = Describe("Validating VM Admitter", func() {
 		table.Entry("reject update to running true", func(vm *v1.VirtualMachine) bool {
 			vm.Spec.Running = &[]bool{true}[0]
 			return false
-		}),
+		}, false),
+		table.Entry("reject update of runStrategy", func(vm *v1.VirtualMachine) bool {
+			vm.Spec.RunStrategy = &runStrategyManual
+			return false
+		}, true),
 		table.Entry("accept update to spec except running true", func(vm *v1.VirtualMachine) bool {
 			vm.Spec.Template = &v1.VirtualMachineInstanceTemplateSpec{}
 			return true
-		}),
+		}, false),
 		table.Entry("accept update to metadata", func(vm *v1.VirtualMachine) bool {
 			vm.Annotations = map[string]string{"foo": "bar"}
 			return true
-		}),
+		}, false),
 		table.Entry("accept update to status", func(vm *v1.VirtualMachine) bool {
 			vm.Status.Ready = true
 			return true
-		}),
+		}, false),
 	)
 
 	Context("Flavor", func() {

pkg/virt-controller/watch/snapshot/restore.go

@@ -499,8 +499,13 @@ func (t *vmRestoreTarget) Reconcile() (bool, error) {
 	newVM := t.vm.DeepCopy()
 	newVM.Spec = snapshotVM.Spec
 	// update Running state in case snapshot was on online VM
-	running := false
-	newVM.Spec.Running = &running
+	if newVM.Spec.RunStrategy != nil {
+		runStrategyHalted := kubevirtv1.RunStrategyHalted
+		newVM.Spec.RunStrategy = &runStrategyHalted
+	} else if newVM.Spec.Running != nil {
+		running := false
+		newVM.Spec.Running = &running
+	}
 	newVM.Spec.DataVolumeTemplates = newTemplates
 	newVM.Spec.Template.Spec.Volumes = newVolumes
 	if newVM.Annotations == nil {