Set error state after failed evacuation

When evacuation fails with NoValidHost, the migration status remains
'accepted' instead of 'error'. This causes problem in case the compute
service starts up again and looks for evacuations with status 'accepted',
as it then removes the local instances for those evacuations even though
the instance was never actually evacuated to another host.

Conflicts:
      nova/conductor/manager.py

NOTE(mriedem): The conflict is due to not having change
I6590f0eda4ec4996543ad40d8c2640b83fc3dd9d in Ocata.

Change-Id: I06d78c744fa75ae5f34c5cfa76bc3c9460767b84
Closes-Bug: #1713783
(cherry picked from commit a8ebf5f1aa)
(cherry picked from commit a3f286f43d)

nova/conductor/manager.py

@@ -706,6 +706,15 @@ class ComputeTaskManager(base.Base):
         with compute_utils.EventReporter(context, 'rebuild_server',
                                           instance.uuid):
             node = limits = None
+
+            try:
+                migration = objects.Migration.get_by_instance_and_status(
+                    context, instance.uuid, 'accepted')
+            except exception.MigrationNotFoundByStatus:
+                LOG.debug("No migration record for the rebuild/evacuate "
+                          "request.", instance=instance)
+                migration = None
+
             if not host:
                 if not request_spec:
                     # NOTE(sbauza): We were unable to find an original
@@ -744,6 +753,9 @@ class ComputeTaskManager(base.Base):
                                           host_dict['nodename'],
                                           host_dict['limits'])
                 except exception.NoValidHost as ex:
+                    if migration:
+                        migration.status = 'error'
+                        migration.save()
                     # Rollback the image_ref if a new one was provided (this
                     # only happens in the rebuild case, not evacuate).
                     if orig_image_ref and orig_image_ref != image_ref:
@@ -759,6 +771,9 @@ class ComputeTaskManager(base.Base):
                         compute_utils.add_instance_fault_from_exc(context,
                             instance, ex, sys.exc_info())
                 except exception.UnsupportedPolicyException as ex:
+                    if migration:
+                        migration.status = 'error'
+                        migration.save()
                     # Rollback the image_ref if a new one was provided (this
                     # only happens in the rebuild case, not evacuate).
                     if orig_image_ref and orig_image_ref != image_ref:
@@ -775,14 +790,6 @@ class ComputeTaskManager(base.Base):
                         compute_utils.add_instance_fault_from_exc(context,
                             instance, ex, sys.exc_info())
 
-            try:
-                migration = objects.Migration.get_by_instance_and_status(
-                    context, instance.uuid, 'accepted')
-            except exception.MigrationNotFoundByStatus:
-                LOG.debug("No migration record for the rebuild/evacuate "
-                          "request.", instance=instance)
-                migration = None
-
             compute_utils.notify_about_instance_usage(
                 self.notifier, context, instance, "rebuild.scheduled")
 

nova/tests/functional/regressions/test_bug_1713783.py

@@ -124,7 +124,4 @@ class FailedEvacuateStateTests(test.TestCase,
         self.assertEqual('evacuation', migrations[0]['migration_type'])
         self.assertEqual(server['id'], migrations[0]['instance_uuid'])
         self.assertEqual(self.hostname, migrations[0]['source_compute'])
-        self.assertEqual('accepted', migrations[0]['status'])
-        # NOTE(elod.illes): Migration status should be 'error' and not
-        # 'accepted'. Needs to be replaced when bug #1713783 is fixed.
-        # self.assertEqual('error', migrations[0]['status'])
+        self.assertEqual('error', migrations[0]['status'])

nova/tests/unit/conductor/test_conductor.py

@@ -1313,6 +1313,15 @@ class _BaseTaskTestCase(object):
         # build_instances() is a cast, we need to wait for it to complete
         self.useFixture(cast_as_call.CastAsCall(self.stubs))
 
+        # Create the migration record (normally created by the compute API).
+        migration = objects.Migration(self.context,
+                                      source_compute=inst_obj.host,
+                                      source_node=inst_obj.node,
+                                      instance_uuid=inst_obj.uuid,
+                                      status='accepted',
+                                      migration_type='evacuation')
+        migration.create()
+
         self.assertRaises(exc.UnsupportedPolicyException,
                           self.conductor.rebuild_instance,
                           self.context,
@@ -1327,6 +1336,10 @@ class _BaseTaskTestCase(object):
         self.assertIn('ServerGroup policy is not supported',
                       inst_obj.fault.message)
 
+        # Assert the migration status was updated.
+        migration = objects.Migration.get_by_id(self.context, migration.id)
+        self.assertEqual('error', migration.status)
+
     def test_rebuild_instance_evacuate_migration_record(self):
         inst_obj = self._create_fake_instance_obj()
         migration = objects.Migration(context=self.context,