diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 556f8d56ca1c..d2e58f00a07b 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -4284,14 +4284,17 @@ class ComputeManager(manager.Manager): 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) @@ -4330,13 +4333,6 @@ class ComputeManager(manager.Manager): 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) @@ -4360,8 +4356,9 @@ class ComputeManager(manager.Manager): # 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 @@ -4412,6 +4409,13 @@ class ComputeManager(manager.Manager): '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 @@ -4664,6 +4668,13 @@ class ComputeManager(manager.Manager): 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') @@ -4692,7 +4703,11 @@ class ComputeManager(manager.Manager): 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. @@ -4714,16 +4729,14 @@ class ComputeManager(manager.Manager): 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 @@ -4913,27 +4926,6 @@ class ComputeManager(manager.Manager): 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') @@ -4947,6 +4939,16 @@ class ComputeManager(manager.Manager): 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) @@ -4956,14 +4958,14 @@ class ComputeManager(manager.Manager): 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( diff --git a/nova/tests/functional/libvirt/test_numa_servers.py b/nova/tests/functional/libvirt/test_numa_servers.py index 86a9dcdfbd17..e5aa401ba030 100644 --- a/nova/tests/functional/libvirt/test_numa_servers.py +++ b/nova/tests/functional/libvirt/test_numa_servers.py @@ -696,8 +696,7 @@ class NUMAServersTest(NUMAServersTestBase): 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 @@ class NUMAServersTest(NUMAServersTestBase): # 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): diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index e15736019847..2d4145b59866 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -8793,9 +8793,14 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase, 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)