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
This commit is contained in:
Matt Riedemann
parent
018068c4ca
commit
edf51119fa
@ -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,
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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.
|
||||
|
||||
@ -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'])
|
||||
|
||||
@ -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')
|
||||
|
||||
@ -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)
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user