From 26da4418a972aca24ff69c1e3f4517cc14724a02 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Tue, 26 Feb 2019 16:21:57 -0500 Subject: [PATCH] Deal with cross-cell resize in _remove_deleted_instances_allocations When reverting a cross-cell resize, conductor will: 1. clean up the destination host 2. set instance.hidden=True and destroy the instance in the target cell database 3. finish the revert on the source host which will revert the allocations on the source host held by the migration record so the instance will hold those again and drop the allocations against the dest host which were held by the instance. If the ResourceTracker.update_available_resource periodic task runs between steps 2 and 3 it could see that the instance is deleted from the target cell but there are still allocations held by it and delete them. Step 3 is what handles deleting those allocations for the destination node, so we want to leave it that way and take the ResourceTracker out of the flow. This change simply checks the instance.hidden value on the deleted instance and if hidden=True, assumes the allocations will be cleaned up elsehwere (finish_revert_snapshot_based_resize_at_source). Ultimately this is probably not something we *have* to have since finish_revert_snapshot_based_resize_at_source is going to drop the destination node allocations anyway, but it is good to keep clear which actor is doing what in this process. Part of blueprint cross-cell-resize Change-Id: Idb82b056c39fd167864cadd205d624cb87cbe9cb --- nova/compute/resource_tracker.py | 12 +++++-- .../unit/compute/test_resource_tracker.py | 33 +++++++++++++++++-- 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/nova/compute/resource_tracker.py b/nova/compute/resource_tracker.py index f7f74ce8643d..9993202f948d 100644 --- a/nova/compute/resource_tracker.py +++ b/nova/compute/resource_tracker.py @@ -1506,11 +1506,17 @@ class ResourceTracker(object): exc_info=False) continue - if instance.deleted: + # NOTE(mriedem): A cross-cell migration will work with instance + # records across two cells once the migration is confirmed/reverted + # one of them will be deleted but the instance still exists in the + # other cell. Before the instance is destroyed from the old cell + # though it is marked hidden=True so if we find a deleted hidden + # instance with allocations against this compute node we just + # ignore it since the migration operation will handle cleaning up + # those allocations. + if instance.deleted and not instance.hidden: # The instance is gone, so we definitely want to remove # allocations associated with it. - # NOTE(jaypipes): This will not be true if/when we support - # cross-cell migrations... LOG.debug("Instance %s has been deleted (perhaps locally). " "Deleting allocations that remained for this " "instance against this compute host: %s.", diff --git a/nova/tests/unit/compute/test_resource_tracker.py b/nova/tests/unit/compute/test_resource_tracker.py index 7de59728e8fe..78d5772fae6e 100644 --- a/nova/tests/unit/compute/test_resource_tracker.py +++ b/nova/tests/unit/compute/test_resource_tracker.py @@ -3355,7 +3355,7 @@ class TestUpdateUsageFromInstance(BaseTestCase): return_value=allocs) rc.delete_allocation_for_instance = mock.MagicMock() mock_inst_get.return_value = objects.Instance( - uuid=uuids.deleted, deleted=True) + uuid=uuids.deleted, deleted=True, hidden=False) cn = self.rt.compute_nodes[_NODENAME] ctx = mock.MagicMock() # Call the method. @@ -3370,6 +3370,35 @@ class TestUpdateUsageFromInstance(BaseTestCase): expected_attrs=[]) ctx.elevated.assert_called_once_with(read_deleted='yes') + @mock.patch('nova.objects.Instance.get_by_uuid') + def test_remove_deleted_instances_allocations_deleted_hidden_instance(self, + mock_inst_get): + """Tests the scenario where there are allocations against the local + compute node held by a deleted instance but it is hidden=True so the + ResourceTracker does not delete the allocations because it assumes + the cross-cell resize flow will handle the allocations. + """ + rc = self.rt.reportclient + allocs = report.ProviderAllocInfo( + allocations={uuids.deleted: "fake_deleted_instance"}) + rc.get_allocations_for_resource_provider = mock.MagicMock( + return_value=allocs) + rc.delete_allocation_for_instance = mock.MagicMock() + cn = self.rt.compute_nodes[_NODENAME] + mock_inst_get.return_value = objects.Instance( + uuid=uuids.deleted, deleted=True, hidden=True, + host=cn.host, node=cn.hypervisor_hostname, + task_state=task_states.RESIZE_MIGRATING) + ctx = mock.MagicMock() + # Call the method. + self.rt._remove_deleted_instances_allocations(ctx, cn, [], {}) + # Only one call should be made to delete allocations, and that should + # be for the first instance created above + rc.delete_allocation_for_instance.assert_not_called() + mock_inst_get.assert_called_once_with( + ctx.elevated.return_value, uuids.deleted, expected_attrs=[]) + ctx.elevated.assert_called_once_with(read_deleted='yes') + @mock.patch('nova.objects.Instance.get_by_uuid') def test_remove_deleted_instances_allocations_building_instance(self, mock_inst_get): @@ -3401,7 +3430,7 @@ class TestUpdateUsageFromInstance(BaseTestCase): return_value=allocs) rc.delete_allocation_for_instance = mock.MagicMock() mock_inst_get.return_value = objects.Instance( - uuid=uuids.deleted, deleted=True) + uuid=uuids.deleted, deleted=True, hidden=False) cn = self.rt.compute_nodes[_NODENAME] ctx = mock.MagicMock() # Call the method.