diff --git a/nova/compute/api.py b/nova/compute/api.py index 76814f7c8c30..23d2c5c38118 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -1847,20 +1847,45 @@ class API(base.Base): # 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 @@ 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/context.py b/nova/context.py index 4dfd0257228a..be49d2945c42 100644 --- a/nova/context.py +++ b/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 diff --git a/nova/tests/fixtures.py b/nova/tests/fixtures.py index 64d0a2d698a2..ceb46a271551 100644 --- a/nova/tests/fixtures.py +++ b/nova/tests/fixtures.py @@ -399,6 +399,16 @@ class CellDatabases(fixtures.Fixture): @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. diff --git a/nova/tests/functional/regressions/test_bug_1670627.py b/nova/tests/functional/regressions/test_bug_1670627.py index 6a503768a1ef..5fc37e45266a 100644 --- 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 8ae7c45d7794..3aec40541876 100644 --- 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() @@ -1487,6 +1489,16 @@ 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') @@ -1506,10 +1518,13 @@ class _ComputeAPIUnitTestMixIn(object): 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()) @@ -1517,11 +1532,27 @@ class _ComputeAPIUnitTestMixIn(object): 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 @@ class _ComputeAPIUnitTestMixIn(object): 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 @@ class _ComputeAPIUnitTestMixIn(object): 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') diff --git a/nova/tests/unit/test_context.py b/nova/tests/unit/test_context.py index f1f254419430..89a1db237292 100644 --- a/nova/tests/unit/test_context.py +++ b/nova/tests/unit/test_context.py @@ -309,6 +309,25 @@ class ContextTestCase(test.NoDBTestCase): 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)