Decrement quota usage when deleting an instance in cell0

When we fail to schedule an instance, e.g. there are no hosts
available, conductor creates the instance in the cell0 database
and deletes the build request. At this point quota usage
has been incremented in the main 'nova' database.

When the instance is deleted, the build request is already gone
so _delete_while_booting returns False and we lookup the instance
in cell0 and delete it from there, but that flow wasn't decrementing
quota usage like _delete_while_booting was.

This change adds the same quota usage decrement handling that
_delete_while_booting performs.

NOTE(mriedem): This change also pulls in some things from
I7de87dce216835729283bca69f0eff59a679b624 which is not being
backported to Ocata since in Pike it solves a slightly different
part of this quota usage issue. In Pike the cell mapping db_connection
is actually stored on the context object when we get the instance
from nova.compute.api.API.get(). So the fix in Pike is slightly
different from Ocata. However, what we need to pull from that Pike
change is:

1. We need to target the cell that the instance lives in to get the
   flavor information when creating the quota reservation.

2. We need to change the functional regression test to assert that
   the bug is fixed.

The code and tests are adjusted to be a sort of mix between both
changes in Pike without requiring a full backport of the 2nd
part of the fix in Pike.

Change-Id: I4cb0169ce0de537804ab9129bc671d75ce5f7953
Partial-Bug: #1670627
(cherry picked from commit 018068c4caac324643c7c6a4360fad855dd096eb)

diff --git a/nova/compute/api.py b/nova/compute/api.py
index fee89d42500c023fb9f5b413977f4af9f562f4d6..439e5707718960e92e6d3fd6d04ba5737aeb02b8 100644 (file)
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -1788,11 +1788,32 @@ class API(base.Base):
                 # acceptable to skip the rest of the delete processing.
                 cell, instance = self._lookup_instance(context, instance.uuid)
                 if cell and instance:
+                    # Conductor may have buried the instance in cell0 but
+                    # quotas must still be decremented in the main cell DB.
+                    project_id, user_id = quotas_obj.ids_from_instance(
+                        context, instance)
+
+                    # 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):
-                        with compute_utils.notify_about_instance_delete(
-                                self.notifier, context, instance):
-                            instance.destroy()
-                        return
+                        quota_flavor = self._get_flavor_for_reservation(
+                            instance)
+
+                    # 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:
+                        with nova_context.target_cell(context, cell):
+                            with compute_utils.notify_about_instance_delete(
+                                    self.notifier, context, instance):
+                                instance.destroy()
+                            return
+                    except exception.InstanceNotFound:
+                        quotas.rollback()
                 if not instance:
                     # Instance is already deleted.
                     return
@@ -1977,21 +1998,33 @@ class API(base.Base):
                 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,

diff --git a/nova/tests/functional/regressions/test_bug_1670627.py b/nova/tests/functional/regressions/test_bug_1670627.py
index 6a503768a1ef7677757b9b4fa8b0a869c7bb0c91..5fc37e45266a23b250a0f6e35ae77facd57d36fe 100644 (file)
--- a/nova/tests/functional/regressions/test_bug_1670627.py
+++ b/nova/tests/functional/regressions/test_bug_1670627.py
@@ -135,9 +135,8 @@ class TestDeleteFromCell0CheckQuota(test.TestCase):
         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'])

diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py
index c2d2ee0437b6c4cb84c22208c70cd1b75debacd0..fa1d8635d367622459e42f135d8c8417952e6eae 100644 (file)
--- a/nova/tests/unit/compute/test_compute_api.py
+++ b/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_NOT_AUTHORIZED = 'fake-shelved-image-not-authorized'
 SHELVED_IMAGE_EXCEPTION = 'fake-shelved-image-exception'
 
 
+@ddt.ddt
 class _ComputeAPIUnitTestMixIn(object):
     def setUp(self):
         super(_ComputeAPIUnitTestMixIn, self).setUp()
@@ -1482,6 +1484,138 @@ class _ComputeAPIUnitTestMixIn(object):
 
         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')
+    def test_delete_instance_from_cell0(self, destroy_mock, notify_mock,
+                                        target_cell_mock):
+        """Tests the case that the instance does not have a host and was not
+        deleted while building, so conductor put it into cell0 so the API has
+        to delete the instance from cell0 and decrement the quota from the
+        main cell.
+        """
+        instance = self._create_instance_obj({'host': None})
+        cell0 = objects.CellMapping(uuid=objects.CellMapping.CELL0_UUID)
+        quota_mock = mock.MagicMock()
+
+        with test.nested(
+            mock.patch.object(self.compute_api, '_delete_while_booting',
+                              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,
+                _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,
+                flavor=instance.flavor)
+            quota_mock.commit.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),
+                # 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()
+
+    @mock.patch('nova.context.target_cell')
+    @mock.patch('nova.compute.utils.notify_about_instance_delete')
+    @mock.patch('nova.objects.Instance.destroy')
+    @mock.patch('nova.objects.BlockDeviceMappingList.get_by_instance_uuid',
+                # This just lets us exit the test early.
+                side_effect=test.TestingException)
+    def test_delete_instance_from_cell0_rollback_quota(
+            self, bdms_get_by_instance_uuid, destroy_mock, notify_mock,
+            target_cell_mock):
+        """Tests the case that the instance does not have a host and was not
+        deleted while building, so conductor put it into cell0 so the API has
+        to delete the instance from cell0 and decrement the quota from the
+        main cell. When we go to delete the instance, it's already gone so we
+        rollback the quota change.
+        """
+        instance = self._create_instance_obj({'host': None})
+        cell0 = objects.CellMapping(uuid=objects.CellMapping.CELL0_UUID)
+        quota_mock = mock.MagicMock()
+        destroy_mock.side_effect = exception.InstanceNotFound(
+            instance_id=instance.uuid)
+
+        with test.nested(
+            mock.patch.object(self.compute_api, '_delete_while_booting',
+                              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,
+                _get_flavor_for_reservation, _create_reservations
+        ):
+            self.assertRaises(
+                test.TestingException, 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,
+                flavor=instance.flavor)
+            quota_mock.commit.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),
+                # Destroy the instance.
+                mock.call(self.context, cell0),
+                mock.call().__enter__(),
+                mock.call().__exit__(
+                    exception.InstanceNotFound,
+                    destroy_mock.side_effect,
+                    mock.ANY),
+            ]
+            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()
+            quota_mock.rollback.assert_called_once_with()
+
     @mock.patch.object(context, 'target_cell')
     @mock.patch.object(objects.InstanceMapping, 'get_by_instance_uuid',
                        side_effect=exception.InstanceMappingNotFound(