Merge pull request kubevirt#2229 from vladikr/fix_mig_cancel

fix migration cancellation that was posted before migration has been started

pkg/virt-controller/watch/migration.go

@@ -293,15 +293,14 @@ func (c *MigrationController) updateStatus(migration *virtv1.VirtualMachineInsta
 		migrationCopy.Status.Phase = virtv1.MigrationFailed
 		c.recorder.Eventf(migration, k8sv1.EventTypeWarning, FailedMigrationReason, "Source node reported migration failed")
 		log.Log.Object(migration).Error("VMI reported migration failed.")
-	} else if migration.DeletionTimestamp != nil && !migration.IsFinal() {
-		if !conditionManager.HasCondition(migration, virtv1.VirtualMachineInstanceMigrationAbortRequested) {
-			condition := virtv1.VirtualMachineInstanceMigrationCondition{
-				Type:          virtv1.VirtualMachineInstanceMigrationAbortRequested,
-				Status:        k8sv1.ConditionTrue,
-				LastProbeTime: v1.Now(),
-			}
-			migrationCopy.Status.Conditions = append(migrationCopy.Status.Conditions, condition)
+	} else if migration.DeletionTimestamp != nil && !migration.IsFinal() &&
+		!conditionManager.HasCondition(migration, virtv1.VirtualMachineInstanceMigrationAbortRequested) {
+		condition := virtv1.VirtualMachineInstanceMigrationCondition{
+			Type:          virtv1.VirtualMachineInstanceMigrationAbortRequested,
+			Status:        k8sv1.ConditionTrue,
+			LastProbeTime: v1.Now(),
 		}
+		migrationCopy.Status.Conditions = append(migrationCopy.Status.Conditions, condition)
 	} else {
 
 		switch migration.Status.Phase {
@@ -415,7 +414,8 @@ func (c *MigrationController) sync(key string, migration *virtv1.VirtualMachineI
 		pod = pods[0]
 	}
 
-	if vmi != nil && migration.DeletionTimestamp != nil && !migration.IsFinal() {
+	if vmi != nil && migration.DeletionTimestamp != nil &&
+		migration.Status.Phase == virtv1.MigrationRunning {
 		vmiCopy := vmi.DeepCopy()
 		if vmiCopy.Status.MigrationState != nil {
 			vmiCopy.Status.MigrationState.AbortRequested = true

pkg/virt-controller/watch/migration_test.go

@@ -671,7 +671,7 @@ var _ = Describe("Migration watcher", func() {
 		It("should abort the migration", func() {
 			vmi := newVirtualMachine("testvmi", v1.Running)
 			vmi.Status.NodeName = "node02"
-			migration := newMigration("testmigration", vmi.Name, v1.MigrationTargetReady)
+			migration := newMigration("testmigration", vmi.Name, v1.MigrationRunning)
 			condition := v1.VirtualMachineInstanceMigrationCondition{
 				Type:          v1.VirtualMachineInstanceMigrationAbortRequested,
 				Status:        k8sv1.ConditionTrue,
@@ -693,7 +693,6 @@ var _ = Describe("Migration watcher", func() {
 			podFeeder.Add(pod)
 
 			vmiInterface.EXPECT().Patch(vmi.Name, types.JSONPatchType, gomock.Any()).Return(vmi, nil)
-
 			controller.Execute()
 			testutils.ExpectEvent(recorder, SuccessfulAbortMigrationReason)
 		})

tests/migration_test.go

@@ -123,9 +123,7 @@ var _ = Describe("Migrations", func() {
 			vmi, err = virtClient.VirtualMachineInstance(vmi.Namespace).Get(vmi.Name, &metav1.GetOptions{})
 			Expect(err).To(BeNil())
 
-			Expect(vmi.Status.MigrationState).ToNot(BeNil())
-
-			if vmi.Status.MigrationState.Completed &&
+			if vmi.Status.MigrationState != nil && vmi.Status.MigrationState.Completed &&
 				vmi.Status.MigrationState.AbortStatus == v1.MigrationAbortSucceeded {
 				return true
 			}
@@ -208,6 +206,32 @@ var _ = Describe("Migrations", func() {
 
 		return uid
 	}
+	runAndImmediatelyCancelMigration := func(migration *v1.VirtualMachineInstanceMigration, vmi *v1.VirtualMachineInstance, timeout int) string {
+		By("Starting a Migration")
+		Eventually(func() error {
+			_, err := virtClient.VirtualMachineInstanceMigration(migration.Namespace).Create(migration)
+			return err
+		}, timeout, 1*time.Second).ShouldNot(HaveOccurred())
+
+		By("Waiting until the Migration is Running")
+
+		uid := ""
+		Eventually(func() bool {
+			migration, err := virtClient.VirtualMachineInstanceMigration(migration.Namespace).Get(migration.Name, &metav1.GetOptions{})
+			Expect(err).To(BeNil())
+			uid = string(migration.UID)
+			if uid != "" {
+				return true
+			}
+			return false
+
+		}, timeout, 1*time.Second).Should(Equal(true))
+
+		By("Cancelling a Migration")
+		Expect(virtClient.VirtualMachineInstanceMigration(migration.Namespace).Delete(migration.Name, &metav1.DeleteOptions{})).To(Succeed(), "Migration should be deleted successfully")
+
+		return uid
+	}
 
 	runMigrationAndExpectFailure := func(migration *v1.VirtualMachineInstanceMigration, timeout int) string {
 		By("Starting a Migration")
@@ -700,6 +724,39 @@ var _ = Describe("Migrations", func() {
 				tests.WaitForVirtualMachineToDisappearWithTimeout(vmi, 240)
 
 			})
+			It("should be able successfully cancel a migration right after posting it", func() {
+				vmi := tests.NewRandomFedoraVMIWitGuestAgent()
+				vmi.Spec.Domain.Resources.Requests[k8sv1.ResourceMemory] = resource.MustParse("1Gi")
+
+				By("Starting the VirtualMachineInstance")
+				vmi = runVMIAndExpectLaunch(vmi, 240)
+
+				By("Checking that the VirtualMachineInstance console has expected output")
+				expecter, expecterErr := tests.LoggedInFedoraExpecter(vmi)
+				Expect(expecterErr).To(BeNil())
+				defer expecter.Close()
+
+				// execute a migration, wait for finalized state
+				By("Starting the Migration")
+				migration := tests.NewRandomMigration(vmi.Name, vmi.Namespace)
+
+				migrationUID := runAndImmediatelyCancelMigration(migration, vmi, 180)
+
+				// check VMI, confirm migration state
+				confirmVMIPostMigrationAborted(vmi, migrationUID, 180)
+
+				By("Waiting for the migration object to disappear")
+				tests.WaitForMigrationToDisappearWithTimeout(migration, 240)
+
+				// delete VMI
+				By("Deleting the VMI")
+				err = virtClient.VirtualMachineInstance(vmi.Namespace).Delete(vmi.Name, &metav1.DeleteOptions{})
+				Expect(err).To(BeNil())
+
+				By("Waiting for VMI to disappear")
+				tests.WaitForVirtualMachineToDisappearWithTimeout(vmi, 240)
+
+			})
 		})
 	})