From 44376d2e212e0f9405a58dc7fc4d5b38d70ac42e Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 5 Aug 2020 14:27:06 +0100 Subject: [PATCH] Don't unset Instance.old_flavor, new_flavor until necessary Since change Ia6d8a7909081b0b856bd7e290e234af7e42a2b38, the resource tracker's 'drop_move_claim' method has been capable of freeing up resource usage. However, this relies on accurate resource reporting. It transpires that there's a race whereby the resource tracker's 'update_available_resource' periodic task can end up not accounting for usage from migrations that are in the process of being completed. The root cause is the resource tracker's reliance on the stashed flavor in a given migration record [1]. Previously, this information was deleted by the compute manager at the start of the confirm migration operation [2]. The compute manager would then call the virt driver [3], which could take a not insignificant amount of time to return, before finally dropping the move claim. If the periodic task ran between the clearing of the stashed flavor and the return of the virt driver, it would find a migration record with no stashed flavor and would therefore ignore this record for accounting purposes [4], resulting in an incorrect record for the compute node, and an exception when the 'drop_move_claim' attempts to free up the resources that aren't being tracked. The solution to this issue is pretty simple. Instead of unsetting the old flavor record from the migration at the start of the various move operations, do it afterwards. [1] https://github.com/openstack/nova/blob/6557d67/nova/compute/resource_tracker.py#L1288 [2] https://github.com/openstack/nova/blob/6557d67/nova/compute/manager.py#L4310-L4315 [3] https://github.com/openstack/nova/blob/6557d67/nova/compute/manager.py#L4330-L4331 [4] https://github.com/openstack/nova/blob/6557d67/nova/compute/resource_tracker.py#L1300 Change-Id: I4760b01b695c94fa371b72216d398388cf981d28 Signed-off-by: Stephen Finucane Partial-Bug: #1879878 Related-Bug: #1834349 Related-Bug: #1818914 --- nova/compute/manager.py | 102 +++++++++--------- .../functional/libvirt/test_numa_servers.py | 13 +-- nova/tests/unit/compute/test_compute_mgr.py | 11 +- 3 files changed, 64 insertions(+), 62 deletions(-) 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)