Merge "Track error migrations in resource tracker"

Author: Zuul

nova/compute/manager.py

@@ -8847,7 +8847,7 @@ def _rollback_volume_bdms(self, context, bdms, original_bdms, instance):
     @wrap_instance_fault
     def _rollback_live_migration(self, context, instance,
                                  dest, migrate_data=None,
-                                 migration_status='error',
+                                 migration_status='failed',
                                  source_bdms=None):
         """Recovers Instance/volume state from migrating -> running.
 
@@ -8988,9 +8988,8 @@ def _rollback_live_migration(self, context, instance,
                 phase=fields.NotificationPhase.END,
                 bdms=bdms)
 
-        # TODO(luyao): set migration status to 'failed' but not 'error'
-        # which means rollback_live_migration is done, we have successfully
-        # cleaned up and returned instance back to normal status.
+        # NOTE(luyao): we have cleanup everything and get instance
+        # back to normal status, now set migration status to 'failed'
         self._set_migration_status(migration, migration_status)
 
     @wrap_exception()
@@ -10508,11 +10507,14 @@ def _run_pending_deletes(self, context):
 
     @periodic_task.periodic_task(spacing=CONF.instance_delete_interval)
     def _cleanup_incomplete_migrations(self, context):
-        """Delete instance files on failed resize/revert-resize operation
-
-        During resize/revert-resize operation, if that instance gets deleted
-        in-between then instance files might remain either on source or
-        destination compute node because of race condition.
+        """Cleanup on failed resize/revert-resize operation and
+        failed rollback live migration operation.
+
+        During resize/revert-resize operation, or after a failed rollback
+        live migration operation, if that instance gets deleted then instance
+        files might remain either on source or destination compute node and
+        other specific resources might not be cleaned up because of the race
+        condition.
         """
         LOG.debug('Cleaning up deleted instances with incomplete migration ')
         migration_filters = {'host': CONF.host,
@@ -10534,21 +10536,29 @@ def _cleanup_incomplete_migrations(self, context):
                 context, inst_filters, expected_attrs=attrs, use_slave=True)
 
         for instance in instances:
-            if instance.host != CONF.host:
-                for migration in migrations:
-                    if instance.uuid == migration.instance_uuid:
-                        # Delete instance files if not cleanup properly either
-                        # from the source or destination compute nodes when
-                        # the instance is deleted during resizing.
-                        self.driver.delete_instance_files(instance)
-                        try:
-                            migration.status = 'failed'
-                            migration.save()
-                        except exception.MigrationNotFound:
-                            LOG.warning("Migration %s is not found.",
-                                        migration.id,
-                                        instance=instance)
-                        break
+            if instance.host == CONF.host:
+                continue
+            for migration in migrations:
+                if instance.uuid != migration.instance_uuid:
+                    continue
+                self.driver.delete_instance_files(instance)
+                # we are not sure whether the migration_context is applied
+                # during incompleted migrating, we need to apply/revert
+                # migration_context to get instance object content matching
+                # current host.
+                revert = (True if migration.source_compute == CONF.host
+                          else False)
+                with instance.mutated_migration_context(revert=revert):
+                    self.driver.cleanup_lingering_instance_resources(instance)
+
+                try:
+                    migration.status = 'failed'
+                    migration.save()
+                except exception.MigrationNotFound:
+                    LOG.warning("Migration %s is not found.",
+                                migration.id,
+                                instance=instance)
+                break
 
     @messaging.expected_exceptions(exception.InstanceQuiesceNotSupported,
                                    exception.QemuGuestAgentNotEnabled,

nova/compute/resource_tracker.py

@@ -919,9 +919,9 @@ def _update_available_resource(self, context, resources, startup=False):
         instance_by_uuid = self._update_usage_from_instances(
             context, instances, nodename)
 
-        # Grab all in-progress migrations:
-        migrations = objects.MigrationList.get_in_progress_by_host_and_node(
-                context, self.host, nodename)
+        # Grab all in-progress migrations and error migrations:
+        migrations = objects.MigrationList.get_in_progress_and_error(
+            context, self.host, nodename)
 
         self._pair_instances_to_migrations(migrations, instance_by_uuid)
         self._update_usage_from_migrations(context, migrations, nodename)
@@ -1360,6 +1360,11 @@ def _update_usage_from_migrations(self, context, migrations, nodename):
 
             try:
                 if uuid not in instances:
+                    # Track migrating instance even if it is deleted but still
+                    # has database record. This kind of instance might be
+                    # deleted during unfinished migrating but exist in the
+                    # hypervisor.
+                    migration._context = context.elevated(read_deleted='yes')
                     instances[uuid] = migration.instance
             except exception.InstanceNotFound as e:
                 # migration referencing deleted instance

nova/db/api.py

@@ -470,6 +470,14 @@ def migration_get_by_sort_filters(context, sort_keys, sort_dirs, values):
                                               values)
 
 
+def migration_get_in_progress_and_error_by_host_and_node(context, host, node):
+    """Finds all in progress migrations and error migrations for the given
+    host + node.
+    """
+    return IMPL.migration_get_in_progress_and_error_by_host_and_node(
+        context, host, node)
+
+
 ####################
 
 

nova/db/sqlalchemy/api.py

@@ -3204,6 +3204,10 @@ def migration_get_all_by_filters(context, filters,
     elif "source_compute" in filters:
         host = filters['source_compute']
         query = query.filter(models.Migration.source_compute == host)
+    if "node" in filters:
+        node = filters['node']
+        query = query.filter(or_(models.Migration.source_node == node,
+                                 models.Migration.dest_node == node))
     if "migration_type" in filters:
         migtype = filters["migration_type"]
         query = query.filter(models.Migration.migration_type == migtype)
@@ -3272,6 +3276,20 @@ def migration_migrate_to_uuid(context, count):
     return done, done
 
 
+@pick_context_manager_reader
+def migration_get_in_progress_and_error_by_host_and_node(context, host, node):
+    return model_query(context, models.Migration).\
+            filter(or_(and_(models.Migration.source_compute == host,
+                            models.Migration.source_node == node),
+                       and_(models.Migration.dest_compute == host,
+                            models.Migration.dest_node == node))).\
+            filter(~models.Migration.status.in_(['confirmed', 'reverted',
+                                                 'failed', 'completed',
+                                                 'cancelled', 'done'])).\
+            options(_joinedload_all('instance.system_metadata')).\
+            all()
+
+
 ########################
 # User-provided metadata
 

nova/objects/instance.py

@@ -1047,8 +1047,8 @@ def _set_migration_context_to_instance(self, prefix):
                 setattr(self, inst_attr_name, attr_value)
 
     @contextlib.contextmanager
-    def mutated_migration_context(self):
-        """Context manager to temporarily apply the migration context.
+    def mutated_migration_context(self, revert=False):
+        """Context manager to temporarily apply/revert the migration context.
 
         Calling .save() from within the context manager means that the mutated
         context will be saved which can cause incorrect resource tracking, and
@@ -1063,7 +1063,10 @@ def mutated_migration_context(self):
         current_values = {}
         for attr_name in _MIGRATION_CONTEXT_ATTRS:
             current_values[attr_name] = getattr(self, attr_name)
-        self.apply_migration_context()
+        if revert:
+            self.revert_migration_context()
+        else:
+            self.apply_migration_context()
         try:
             yield
         finally:

nova/objects/migration.py

@@ -222,7 +222,9 @@ class MigrationList(base.ObjectListBase, base.NovaObject):
     #              for an instance.
     # Version 1.4: Added sort_keys, sort_dirs, limit, marker kwargs to
     #              get_by_filters for migrations pagination support.
-    VERSION = '1.4'
+    # Version 1.5: Added a new function to get in progress migrations
+    #              and error migrations for a given host + node.
+    VERSION = '1.5'
 
     fields = {
         'objects': fields.ListOfObjectsField('Migration'),
@@ -266,3 +268,11 @@ def get_in_progress_by_instance(cls, context, instance_uuid,
             context, instance_uuid, migration_type)
         return base.obj_make_list(context, cls(context), objects.Migration,
                                   db_migrations)
+
+    @base.remotable_classmethod
+    def get_in_progress_and_error(cls, context, host, node):
+        db_migrations = \
+            db.migration_get_in_progress_and_error_by_host_and_node(
+                context, host, node)
+        return base.obj_make_list(context, cls(context), objects.Migration,
+                                  db_migrations)

nova/tests/functional/libvirt/test_numa_live_migration.py

@@ -354,7 +354,7 @@ def _test(self, pin_dest=False):
         # rollback test, so server_a is expected to remain on host_a.
         if pin_dest:
             self._rpc_pin_host('host_b')
-        self._live_migrate(self.server_a, 'error')
+        self._live_migrate(self.server_a, 'failed')
         self.assertEqual('host_a', self.get_host(self.server_a['id']))
         self.assertIsNone(self._get_migration_context(self.server_a['id']))
 
@@ -448,7 +448,7 @@ def _test(self, pin_source, pin_cond, expect_success=True):
                                             ['completed'])
         else:
             final_host = 'source'
-            self._wait_for_migration_status(self.migrating_server, ['error'])
+            self._wait_for_migration_status(self.migrating_server, ['failed'])
         self.assertEqual(final_host,
                          self.get_host(self.migrating_server['id']))
         self.assertTrue(self.migrate_stub_ran)

nova/tests/functional/regressions/test_bug_1889108.py

@@ -82,7 +82,7 @@ def test_vol_attachments_during_driver_pre_live_mig_failure(
         # Migrate the instance and wait until the migration errors out thanks
         # to our mocked version of pre_live_migration raising
         # test.TestingException
-        self._live_migrate(server, 'error')
+        self._live_migrate(server, 'failed')
 
         # Assert that we called the fake pre_live_migration method
         mock_plm.assert_called_once()

nova/tests/functional/test_servers.py

@@ -2870,8 +2870,10 @@ def stub_pre_live_migration(context, instance, block_device_info,
         }
         self.api.post_server_action(server['id'], post)
         # The compute manager will put the migration record into error status
-        # when pre_live_migration fails, so wait for that to happen.
-        migration = self._wait_for_migration_status(server, ['error'])
+        # when start _cleanup_pre_live_migration, then set it to failed status
+        # at the end of _rollback_live_migration
+        migration = self._wait_for_migration_status(
+            server, ['error', 'failed'])
         # The _rollback_live_migration method in the compute manager will reset
         # the task_state on the instance, so wait for that to happen.
         server = self._wait_for_server_parameter(

nova/tests/unit/compute/test_compute.py

@@ -6258,7 +6258,7 @@ def do_it(mock_client, mock_setup):
         self.assertEqual('src_host', instance.host)
         self.assertEqual(vm_states.ACTIVE, instance.vm_state)
         self.assertIsNone(instance.task_state)
-        self.assertEqual('error', migration.status)
+        self.assertEqual('failed', migration.status)
         mock_get_disk.assert_called()
         mock_pre.assert_called_once_with(c,
                 instance, True, mock_get_disk.return_value, dest_host,
@@ -6658,7 +6658,7 @@ def _test(mock_drop_claim, mock_nw_api, mock_lmcf, mock_ra,
 
         _test()
 
-        self.assertEqual(migration_status or 'error', migration.status)
+        self.assertEqual(migration_status or 'failed', migration.status)
         self.assertEqual(0, instance.progress)
 
     def test_rollback_live_migration_set_migration_status(self):