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

Author: mriedem

nova/compute/resource_tracker.py

@@ -1506,11 +1506,17 @@ def _remove_deleted_instances_allocations(self, context, cn,
                              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.",

nova/tests/unit/compute/test_resource_tracker.py

@@ -3355,7 +3355,7 @@ def test_remove_deleted_instances_allocations_deleted_instance(self,
             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 @@ def test_remove_deleted_instances_allocations_deleted_instance(self,
             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 @@ def test_remove_deleted_instances_allocations_ignores_migrations(self,
             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.