Temporarily untarget context when deleting from cell0

When deleting an instance we look it up in the _get_instance
method and if it's in cell0 then the context is permanently
targeted to that cell via the set_target_cell method.

When we delete the instance in _delete we need to temporarily
untarget the context when we decrement the quota usage otherwise
the quota usage gets decremented in the cell0 database rather than
the cell database. Once the instance is deleted then we
re-apply the target cell on the context.

Change-Id: I7de87dce216835729283bca69f0eff59a679b624
Closes-Bug: #1670627

nova/compute/api.py

@@ -1847,20 +1847,45 @@ def _delete(self, context, instance, delete_type, cb, **instance_attrs):
                     # quotas must still be decremented in the main cell DB.
                     project_id, user_id = quotas_obj.ids_from_instance(
                         context, instance)
-                    # This is confusing but actually decrements quota usage.
-                    quotas = self._create_reservations(context,
-                                                       instance,
-                                                       instance.task_state,
-                                                       project_id, user_id)
-                    quotas.commit()
+
+                    # TODO(mriedem): This is a hack until we have quotas in the
+                    # API database. When we looked up the instance in
+                    # _get_instance if the instance has a mapping then the
+                    # context is modified to set the target cell permanently.
+                    # However, if the instance is in cell0 then the context
+                    # is targeting cell0 and the quotas would be decremented
+                    # from cell0 and we actually need them decremented from
+                    # the cell database. So we temporarily untarget the
+                    # context while we do the quota stuff and re-target after
+                    # we're done.
+
+                    # We have to get the flavor from the instance while the
+                    # context is still targeted to where the instance lives.
+                    with nova_context.target_cell(context, cell):
+                        # If the instance has the targeted context in it then
+                        # we don't need the context manager.
+                        quota_flavor = self._get_flavor_for_reservation(
+                            instance)
+
+                    with nova_context.target_cell(context, None):
+                        # This is confusing but actually decrements quota usage
+                        quotas = self._create_reservations(
+                            context, instance, instance.task_state,
+                            project_id, user_id, flavor=quota_flavor)
+                        quotas.commit()
+
                     try:
+                        # Now destroy the instance from the cell it lives in.
                         with nova_context.target_cell(context, cell):
+                            # If the instance has the targeted context in it
+                            # then we don't need the context manager.
                             with compute_utils.notify_about_instance_delete(
                                     self.notifier, context, instance):
                                 instance.destroy()
                             return
                     except exception.InstanceNotFound:
-                        quotas.rollback()
+                        with nova_context.target_cell(context, None):
+                            quotas.rollback()
                 if not instance:
                     # Instance is already deleted.
                     return
@@ -2051,21 +2076,33 @@ def _confirm_resize_on_deleting(self, context, instance):
                 src_host, quotas.reservations,
                 cast=False)
 
+    def _get_flavor_for_reservation(self, instance):
+        """Returns the flavor needed for _create_reservations.
+
+        This is used when the context is targeted to a cell that is
+        different from the one that the instance lives in.
+        """
+        if instance.task_state in (task_states.RESIZE_MIGRATED,
+                                   task_states.RESIZE_FINISH):
+            return instance.old_flavor
+        return instance.flavor
+
     def _create_reservations(self, context, instance, original_task_state,
-                             project_id, user_id):
+                             project_id, user_id, flavor=None):
         # NOTE(wangpan): if the instance is resizing, and the resources
         #                are updated to new instance type, we should use
         #                the old instance type to create reservation.
         # see https://bugs.launchpad.net/nova/+bug/1099729 for more details
         if original_task_state in (task_states.RESIZE_MIGRATED,
                                    task_states.RESIZE_FINISH):
-            old_flavor = instance.old_flavor
+            old_flavor = flavor or instance.old_flavor
             instance_vcpus = old_flavor.vcpus
             vram_mb = old_flavor.extra_specs.get('hw_video:ram_max_mb', 0)
             instance_memory_mb = old_flavor.memory_mb + vram_mb
         else:
-            instance_vcpus = instance.flavor.vcpus
-            instance_memory_mb = instance.flavor.memory_mb
+            flavor = flavor or instance.flavor
+            instance_vcpus = flavor.vcpus
+            instance_memory_mb = flavor.memory_mb
 
         quotas = objects.Quotas(context=context)
         quotas.reserve(project_id=project_id,

nova/context.py

@@ -365,17 +365,23 @@ def set_target_cell(context, cell_mapping):
     This is used for permanently targeting a cell in a context.
     Use this when you want all subsequent code to target a cell.
 
+    Passing None for cell_mapping will untarget the context.
+
     :param context: The RequestContext to add connection information
-    :param cell_mapping: An objects.CellMapping object
+    :param cell_mapping: An objects.CellMapping object or None
     """
-    # avoid circular import
-    from nova import db
-    from nova import rpc
-    db_connection_string = cell_mapping.database_connection
-    context.db_connection = db.create_context_manager(db_connection_string)
-    if not cell_mapping.transport_url.startswith('none'):
-        context.mq_connection = rpc.create_transport(
-            cell_mapping.transport_url)
+    if cell_mapping is not None:
+        # avoid circular import
+        from nova import db
+        from nova import rpc
+        db_connection_string = cell_mapping.database_connection
+        context.db_connection = db.create_context_manager(db_connection_string)
+        if not cell_mapping.transport_url.startswith('none'):
+            context.mq_connection = rpc.create_transport(
+                cell_mapping.transport_url)
+    else:
+        context.db_connection = None
+        context.mq_connection = None
 
 
 @contextmanager
@@ -386,8 +392,10 @@ def target_cell(context, cell_mapping):
     This context manager makes a temporary change to the context
     and restores it when complete.
 
+    Passing None for cell_mapping will untarget the context temporarily.
+
     :param context: The RequestContext to add connection information
-    :param cell_mapping: A objects.CellMapping object
+    :param cell_mapping: An objects.CellMapping object or None
     """
     original_db_connection = context.db_connection
     original_mq_connection = context.mq_connection

nova/tests/fixtures.py

@@ -399,6 +399,16 @@ def _cache_schema(self, connection_str):
     @contextmanager
     def _wrap_target_cell(self, context, cell_mapping):
         with self._cell_lock.write_lock():
+            if cell_mapping is None:
+                # NOTE(danms): The real target_cell untargets with a
+                # cell_mapping of None. Since we're controlling our
+                # own targeting in this fixture, we need to call this out
+                # specifically and avoid switching global database state
+                try:
+                    with self._real_target_cell(context, cell_mapping) as c:
+                        yield c
+                finally:
+                    return
             ctxt_mgr = self._ctxt_mgrs[cell_mapping.database_connection]
             # This assumes the next local DB access is the same cell that
             # was targeted last time.

nova/tests/functional/regressions/test_bug_1670627.py

@@ -135,9 +135,8 @@ def test_delete_error_instance_in_cell0_and_check_quota(self):
         self._delete_server(server['id'])
         self._wait_for_instance_delete(server['id'])
 
-        # Now check the quota again. Because we have not fixed the bug, the
-        # quota is still going to be showing a usage for instances. When the
-        # bug is fixed, ending usage should be current usage - 1.
+        # Now check the quota again. Since the bug is fixed, ending usage
+        # should be current usage - 1.
         ending_usage = self.api.get_limits()
-        self.assertEqual(current_usage['absolute']['totalInstancesUsed'],
+        self.assertEqual(current_usage['absolute']['totalInstancesUsed'] - 1,
                          ending_usage['absolute']['totalInstancesUsed'])

nova/tests/unit/compute/test_compute_api.py

@@ -16,6 +16,7 @@
 import contextlib
 import datetime
 
+import ddt
 import iso8601
 import mock
 from mox3 import mox
@@ -67,6 +68,7 @@
 SHELVED_IMAGE_EXCEPTION = 'fake-shelved-image-exception'
 
 
+@ddt.ddt
 class _ComputeAPIUnitTestMixIn(object):
     def setUp(self):
         super(_ComputeAPIUnitTestMixIn, self).setUp()
@@ -1487,6 +1489,16 @@ def test(mock_create_res, mock_lookup, mock_attempt):
 
         test()
 
+    @ddt.data((task_states.RESIZE_MIGRATED, True),
+              (task_states.RESIZE_FINISH, True),
+              (None, False))
+    @ddt.unpack
+    def test_get_flavor_for_reservation(self, task_state, is_old):
+        instance = self._create_instance_obj({'task_state': task_state})
+        flavor = self.compute_api._get_flavor_for_reservation(instance)
+        expected_flavor = instance.old_flavor if is_old else instance.flavor
+        self.assertEqual(expected_flavor, flavor)
+
     @mock.patch('nova.context.target_cell')
     @mock.patch('nova.compute.utils.notify_about_instance_delete')
     @mock.patch('nova.objects.Instance.destroy')
@@ -1506,22 +1518,41 @@ def test_delete_instance_from_cell0(self, destroy_mock, notify_mock,
                               return_value=False),
             mock.patch.object(self.compute_api, '_lookup_instance',
                               return_value=(cell0, instance)),
+            mock.patch.object(self.compute_api, '_get_flavor_for_reservation',
+                              return_value=instance.flavor),
             mock.patch.object(self.compute_api, '_create_reservations',
                               return_value=quota_mock)
         ) as (
-            _delete_while_booting, _lookup_instance, _create_reservations
+            _delete_while_booting, _lookup_instance,
+            _get_flavor_for_reservation, _create_reservations
         ):
             self.compute_api._delete(
                 self.context, instance, 'delete', mock.NonCallableMock())
             _delete_while_booting.assert_called_once_with(
                 self.context, instance)
             _lookup_instance.assert_called_once_with(
                 self.context, instance.uuid)
+            _get_flavor_for_reservation.assert_called_once_with(instance)
             _create_reservations.assert_called_once_with(
                 self.context, instance, instance.task_state,
-                self.context.project_id, instance.user_id)
+                self.context.project_id, instance.user_id,
+                flavor=instance.flavor)
             quota_mock.commit.assert_called_once_with()
-            target_cell_mock.assert_called_once_with(self.context, cell0)
+            expected_target_cell_calls = [
+                # Get the instance.flavor.
+                mock.call(self.context, cell0),
+                mock.call().__enter__(),
+                mock.call().__exit__(None, None, None),
+                # Create the quota reservation.
+                mock.call(self.context, None),
+                mock.call().__enter__(),
+                mock.call().__exit__(None, None, None),
+                # Destroy the instance.
+                mock.call(self.context, cell0),
+                mock.call().__enter__(),
+                mock.call().__exit__(None, None, None),
+            ]
+            target_cell_mock.assert_has_calls(expected_target_cell_calls)
             notify_mock.assert_called_once_with(
                 self.compute_api.notifier, self.context, instance)
             destroy_mock.assert_called_once_with()
@@ -1552,10 +1583,13 @@ def test_delete_instance_from_cell0_rollback_quota(
                               return_value=False),
             mock.patch.object(self.compute_api, '_lookup_instance',
                               return_value=(cell0, instance)),
+            mock.patch.object(self.compute_api, '_get_flavor_for_reservation',
+                              return_value=instance.flavor),
             mock.patch.object(self.compute_api, '_create_reservations',
                               return_value=quota_mock)
         ) as (
-            _delete_while_booting, _lookup_instance, _create_reservations
+            _delete_while_booting, _lookup_instance,
+            _get_flavor_for_reservation, _create_reservations
         ):
             self.assertRaises(
                 test.TestingException, self.compute_api._delete,
@@ -1564,14 +1598,37 @@ def test_delete_instance_from_cell0_rollback_quota(
                 self.context, instance)
             _lookup_instance.assert_called_once_with(
                 self.context, instance.uuid)
+            _get_flavor_for_reservation.assert_called_once_with(instance)
             _create_reservations.assert_called_once_with(
                 self.context, instance, instance.task_state,
-                self.context.project_id, instance.user_id)
+                self.context.project_id, instance.user_id,
+                flavor=instance.flavor)
             quota_mock.commit.assert_called_once_with()
-            target_cell_mock.assert_called_once_with(self.context, cell0)
             notify_mock.assert_called_once_with(
                 self.compute_api.notifier, self.context, instance)
             destroy_mock.assert_called_once_with()
+            expected_target_cell_calls = [
+                # Get the instance.flavor.
+                mock.call(self.context, cell0),
+                mock.call().__enter__(),
+                mock.call().__exit__(None, None, None),
+                # Create the quota reservation.
+                mock.call(self.context, None),
+                mock.call().__enter__(),
+                mock.call().__exit__(None, None, None),
+                # Destroy the instance.
+                mock.call(self.context, cell0),
+                mock.call().__enter__(),
+                mock.call().__exit__(
+                    exception.InstanceNotFound,
+                    destroy_mock.side_effect,
+                    mock.ANY),
+                # Rollback the quota reservation.
+                mock.call(self.context, None),
+                mock.call().__enter__(),
+                mock.call().__exit__(None, None, None),
+            ]
+            target_cell_mock.assert_has_calls(expected_target_cell_calls)
             quota_mock.rollback.assert_called_once_with()
 
     @mock.patch.object(context, 'target_cell')

nova/tests/unit/test_context.py

@@ -309,6 +309,25 @@ def test_target_cell(self, mock_create_ctxt_mgr, mock_rpc):
         self.assertEqual(mock.sentinel.db_conn, ctxt.db_connection)
         self.assertEqual(mock.sentinel.mq_conn, ctxt.mq_connection)
 
+    @mock.patch('nova.rpc.create_transport')
+    @mock.patch('nova.db.create_context_manager')
+    def test_target_cell_unset(self, mock_create_ctxt_mgr, mock_rpc):
+        """Tests that passing None as the mapping will temporarily
+        untarget any previously set cell context.
+        """
+        mock_create_ctxt_mgr.return_value = mock.sentinel.cdb
+        mock_rpc.return_value = mock.sentinel.cmq
+        ctxt = context.RequestContext('111',
+                                      '222',
+                                      roles=['admin', 'weasel'])
+        ctxt.db_connection = mock.sentinel.db_conn
+        ctxt.mq_connection = mock.sentinel.mq_conn
+        with context.target_cell(ctxt, None):
+            self.assertIsNone(ctxt.db_connection)
+            self.assertIsNone(ctxt.mq_connection)
+        self.assertEqual(mock.sentinel.db_conn, ctxt.db_connection)
+        self.assertEqual(mock.sentinel.mq_conn, ctxt.mq_connection)
+
     def test_get_context(self):
         ctxt = context.get_context()
         self.assertIsNone(ctxt.user_id)