Merge "Don't unset Instance.old_flavor, new_flavor until necessary"

Author: Zuul

nova/compute/manager.py

@@ -4307,14 +4307,17 @@ def do_confirm_resize(context, instance, migration):
                             instance=instance)
                 finally:
                     # Whether an error occurred or not, at this point the
-                    # instance is on the dest host so to avoid leaking
-                    # allocations in placement, delete them here.
+                    # instance is on the dest host. Avoid leaking allocations
+                    # in placement by deleting them here...
                     self._delete_allocation_after_move(
                         context, instance, migration)
-                    # Also as the instance is not any more on this host, update
-                    # the scheduler about the move
+                    # ...inform the scheduler about the move...
                     self._delete_scheduler_instance_info(
                         context, instance.uuid)
+                    # ...and unset the cached flavor information (this is done
+                    # last since the resource tracker relies on it for its
+                    # periodic tasks)
+                    self._delete_stashed_flavor_info(instance)
 
         do_confirm_resize(context, instance, migration)
 
@@ -4353,13 +4356,6 @@ def _confirm_resize(self, context, instance, migration=None):
             self.host, action=fields.NotificationAction.RESIZE_CONFIRM,
             phase=fields.NotificationPhase.START)
 
-        # NOTE(danms): delete stashed migration information
-        old_instance_type = instance.old_flavor
-        instance.old_flavor = None
-        instance.new_flavor = None
-        instance.system_metadata.pop('old_vm_state', None)
-        instance.save()
-
         # NOTE(tr3buchet): tear down networks on source host
         self.network_api.setup_networks_on_host(context, instance,
                            migration.source_compute, teardown=True)
@@ -4383,8 +4379,9 @@ def _confirm_resize(self, context, instance, migration=None):
         # instance.migration_context so make sure to not call
         # instance.drop_migration_context() until after drop_move_claim
         # is called.
-        self.rt.drop_move_claim(context, instance, migration.source_node,
-                                old_instance_type, prefix='old_')
+        self.rt.drop_move_claim(
+            context, instance, migration.source_node, instance.old_flavor,
+            prefix='old_')
         instance.drop_migration_context()
 
         # NOTE(mriedem): The old_vm_state could be STOPPED but the user
@@ -4435,6 +4432,13 @@ def _delete_allocation_after_move(self, context, instance, migration):
                        'migration_uuid': migration.uuid})
             raise
 
+    def _delete_stashed_flavor_info(self, instance):
+        """Remove information about the flavor change after a resize."""
+        instance.old_flavor = None
+        instance.new_flavor = None
+        instance.system_metadata.pop('old_vm_state', None)
+        instance.save()
+
     @wrap_exception()
     @wrap_instance_event(prefix='compute')
     @errors_out_migration
@@ -4687,6 +4691,13 @@ def _revert_snapshot_based_resize_at_dest(
         self.rt.drop_move_claim(ctxt, instance, instance.node,
                                 instance_type=instance.new_flavor)
 
+    def _revert_instance_flavor_host_node(self, instance, migration):
+        """Revert host, node and flavor fields after a resize-revert."""
+        self._set_instance_info(instance, instance.old_flavor)
+        instance.host = migration.source_compute
+        instance.node = migration.source_node
+        instance.save(expected_task_state=[task_states.RESIZE_REVERTING])
+
     @wrap_exception()
     @reverts_task_state
     @wrap_instance_event(prefix='compute')
@@ -4715,7 +4726,11 @@ def do_revert():
             with self._error_out_instance_on_exception(ctxt, instance):
                 self._finish_revert_snapshot_based_resize_at_source(
                     ctxt, instance, migration)
-        do_revert()
+
+        try:
+            do_revert()
+        finally:
+            self._delete_stashed_flavor_info(instance)
 
         # Broadcast to all schedulers that the instance is on this host.
         # This is best effort so if anything fails just log it.
@@ -4737,16 +4752,14 @@ def _finish_revert_snapshot_based_resize_at_source(
             task_state is "resize_reverting".
         :param migration: Migration object whose status is "reverting".
         """
-        # Delete stashed old_vm_state information. We will use this to
-        # determine if the guest should be powered on when we spawn it.
-        old_vm_state = instance.system_metadata.pop(
+        # Get stashed old_vm_state information to determine if guest should
+        # be powered on after spawn; we default to ACTIVE for backwards
+        # compatibility if old_vm_state is not set
+        old_vm_state = instance.system_metadata.get(
             'old_vm_state', vm_states.ACTIVE)
 
-        # Update instance host/node and flavor-related fields. After this
-        # if anything fails the instance will get rebuilt/rebooted on this
-        # host.
-        self._finish_revert_resize_update_instance_flavor_host_node(
-            instance, migration)
+        # Revert the flavor and host/node fields to their previous values
+        self._revert_instance_flavor_host_node(instance, migration)
 
         # Move the allocations against the source compute node resource
         # provider, held by the migration, to the instance which will drop
@@ -4936,27 +4949,6 @@ def _finish_revert_resize_network_migrate_finish(
                 LOG.error('Timeout waiting for Neutron events: %s', events,
                           instance=instance)
 
-    def _finish_revert_resize_update_instance_flavor_host_node(self, instance,
-                                                               migration):
-        """Updates host/node and flavor-related fields on the instance.
-
-        This is used when finish the revert resize operation on the source
-        host and updates the instance flavor-related fields back to the old
-        flavor and then nulls out the old/new_flavor fields.
-
-        The instance host/node fields are also set back to the source compute
-        host/node.
-
-        :param instance: Instance object
-        :param migration: Migration object
-        """
-        self._set_instance_info(instance, instance.old_flavor)
-        instance.old_flavor = None
-        instance.new_flavor = None
-        instance.host = migration.source_compute
-        instance.node = migration.source_node
-        instance.save(expected_task_state=[task_states.RESIZE_REVERTING])
-
     @wrap_exception()
     @reverts_task_state
     @wrap_instance_event(prefix='compute')
@@ -4970,6 +4962,16 @@ def finish_revert_resize(
         revert the resized attributes in the database.
 
         """
+        try:
+            self._finish_revert_resize(
+                context, instance, migration, request_spec)
+        finally:
+            self._delete_stashed_flavor_info(instance)
+
+    def _finish_revert_resize(
+        self, context, instance, migration, request_spec=None,
+    ):
+        """Inner version of finish_revert_resize."""
         with self._error_out_instance_on_exception(context, instance):
             bdms = objects.BlockDeviceMappingList.get_by_instance_uuid(
                 context, instance.uuid)
@@ -4979,14 +4981,14 @@ def finish_revert_resize(
                 self.host, action=fields.NotificationAction.RESIZE_REVERT,
                     phase=fields.NotificationPhase.START, bdms=bdms)
 
-            # NOTE(mriedem): delete stashed old_vm_state information; we
-            # default to ACTIVE for backwards compatibility if old_vm_state
-            # is not set
-            old_vm_state = instance.system_metadata.pop('old_vm_state',
-                                                        vm_states.ACTIVE)
+            # Get stashed old_vm_state information to determine if guest should
+            # be powered on after spawn; we default to ACTIVE for backwards
+            # compatibility if old_vm_state is not set
+            old_vm_state = instance.system_metadata.get(
+                'old_vm_state', vm_states.ACTIVE)
 
-            self._finish_revert_resize_update_instance_flavor_host_node(
-                instance, migration)
+            # Revert the flavor and host/node fields to their previous values
+            self._revert_instance_flavor_host_node(instance, migration)
 
             try:
                 source_allocations = self._revert_allocation(

nova/tests/functional/libvirt/test_numa_servers.py

@@ -696,8 +696,7 @@ def fake_confirm_migration(*args, **kwargs):
                     self.ctxt, dst_host,
                 ).numa_topology,
             )
-            # FIXME(stephenfin): There should still be two pinned cores here
-            self.assertEqual(0, len(src_numa_topology.cells[0].pinned_cpus))
+            self.assertEqual(2, len(src_numa_topology.cells[0].pinned_cpus))
             self.assertEqual(2, len(dst_numa_topology.cells[0].pinned_cpus))
 
             # before continuing with the actualy confirm process
@@ -738,14 +737,10 @@ def fake_confirm_migration(*args, **kwargs):
 
         # Now confirm the resize
 
-        # FIXME(stephenfin): This should be successful, but it's failing with a
-        # HTTP 500 due to bug #1879878
         post = {'confirmResize': None}
-        exc = self.assertRaises(
-            client.OpenStackApiException,
-            self.api.post_server_action, server['id'], post)
-        self.assertEqual(500, exc.response.status_code)
-        self.assertIn('CPUUnpinningInvalid', str(exc))
+        self.api.post_server_action(server['id'], post)
+
+        server = self._wait_for_state_change(server, 'ACTIVE')
 
 
 class NUMAServerTestWithCountingQuotaFromPlacement(NUMAServersTest):

nova/tests/unit/compute/test_compute_mgr.py

@@ -8797,9 +8797,14 @@ def fake_drop_move_claim(*args, **kwargs):
                                         self.migration)
             mock_delete.assert_called_once_with(self.context, self.instance,
                                                 self.migration)
-            mock_save.assert_called_with(expected_task_state=
-                                         [None, task_states.DELETING,
-                                         task_states.SOFT_DELETING])
+            mock_save.assert_has_calls([
+                mock.call(
+                    expected_task_state=[
+                        None, task_states.DELETING, task_states.SOFT_DELETING,
+                    ],
+                ),
+                mock.call(),
+            ])
             mock_delete_scheduler_info.assert_called_once_with(
                 self.context, self.instance.uuid)