Fix and optimize external_events for multiple cells
server_external_events was previously making an api db query and a cell db query for every instance reference. We improve this by making exactly 1 api db query to fetch all instance mappings, and then 1 cell db query per cell to fetch all relevant instances from that cell. Further, it wasn't properly handling the case where events were delivered in one request for multiple instances across cells, which this also fixes. We also document an obtuse edge condition in ComputeAPI.external_instance_event which will cause the current code to break when we support migration between cells. Note this includes a tweak to the SingleCellSimple fixture to mock out the new InstanceMappingList method we use, as well as a fix to the other InstanceMapping mock, which was returning mappings with bogus instance uuids. This patch relies on the results of those being realistic and thus requires those changes. Closes-Bug: #1702959 Co-Authored-By: Dan Smith <dms@danplanet.com> Change-Id: If59453f1899e99040c554bcb9ad54c8a506adc56
This commit is contained in:
parent
7fa8eeefdf
commit
b8be61eb39
|
@ -21,7 +21,6 @@ from nova.api.openstack import wsgi
|
|||
from nova.api import validation
|
||||
from nova import compute
|
||||
from nova import context as nova_context
|
||||
from nova import exception
|
||||
from nova.i18n import _
|
||||
from nova import objects
|
||||
from nova.policies import server_external_events as see_policies
|
||||
|
@ -42,6 +41,28 @@ class ServerExternalEventsController(wsgi.Controller):
|
|||
return False
|
||||
return True
|
||||
|
||||
def _get_instances_all_cells(self, context, instance_uuids,
|
||||
instance_mappings):
|
||||
cells = {}
|
||||
instance_uuids_by_cell = {}
|
||||
for im in instance_mappings:
|
||||
if im.cell_mapping.uuid not in cells:
|
||||
cells[im.cell_mapping.uuid] = im.cell_mapping
|
||||
instance_uuids_by_cell.setdefault(im.cell_mapping.uuid, list())
|
||||
instance_uuids_by_cell[im.cell_mapping.uuid].append(
|
||||
im.instance_uuid)
|
||||
|
||||
instances = {}
|
||||
for cell_uuid, cell in cells.items():
|
||||
with nova_context.target_cell(context, cell) as cctxt:
|
||||
instances.update(
|
||||
{inst.uuid: inst for inst in
|
||||
objects.InstanceList.get_by_filters(
|
||||
cctxt, {'uuid': instance_uuids_by_cell[cell_uuid]},
|
||||
expected_attrs=['migration_context', 'info_cache'])})
|
||||
|
||||
return instances
|
||||
|
||||
@extensions.expected_errors((400, 403, 404))
|
||||
@wsgi.response(200)
|
||||
@validation.schema(server_external_events.create, '2.1', '2.50')
|
||||
|
@ -54,12 +75,17 @@ class ServerExternalEventsController(wsgi.Controller):
|
|||
response_events = []
|
||||
accepted_events = []
|
||||
accepted_instances = set()
|
||||
instances = {}
|
||||
mappings = {}
|
||||
result = 200
|
||||
|
||||
body_events = body['events']
|
||||
|
||||
# Fetch instance objects for all relevant instances
|
||||
instance_uuids = set([event['server_uuid'] for event in body_events])
|
||||
instance_mappings = objects.InstanceMappingList.get_by_instance_uuids(
|
||||
context, list(instance_uuids))
|
||||
instances = self._get_instances_all_cells(context, instance_uuids,
|
||||
instance_mappings)
|
||||
|
||||
for _event in body_events:
|
||||
client_event = dict(_event)
|
||||
event = objects.InstanceExternalEvent(context)
|
||||
|
@ -69,70 +95,54 @@ class ServerExternalEventsController(wsgi.Controller):
|
|||
event.status = client_event.pop('status', 'completed')
|
||||
event.tag = client_event.pop('tag', None)
|
||||
|
||||
response_events.append(_event)
|
||||
|
||||
instance = instances.get(event.instance_uuid)
|
||||
if not instance:
|
||||
try:
|
||||
mapping = objects.InstanceMapping.get_by_instance_uuid(
|
||||
context, event.instance_uuid)
|
||||
cell_mapping = mapping.cell_mapping
|
||||
mappings[event.instance_uuid] = cell_mapping
|
||||
|
||||
# Load migration_context and info_cache here in a single DB
|
||||
# operation because we need them later on
|
||||
with nova_context.target_cell(context,
|
||||
cell_mapping) as cctxt:
|
||||
instance = objects.Instance.get_by_uuid(
|
||||
cctxt, event.instance_uuid,
|
||||
expected_attrs=['migration_context', 'info_cache'])
|
||||
instances[event.instance_uuid] = instance
|
||||
except (exception.InstanceNotFound,
|
||||
exception.InstanceMappingNotFound):
|
||||
LOG.debug('Dropping event %(name)s:%(tag)s for unknown '
|
||||
'instance %(instance_uuid)s',
|
||||
{'name': event.name, 'tag': event.tag,
|
||||
'instance_uuid': event.instance_uuid})
|
||||
_event['status'] = 'failed'
|
||||
_event['code'] = 404
|
||||
result = 207
|
||||
LOG.debug('Dropping event %(name)s:%(tag)s for unknown '
|
||||
'instance %(instance_uuid)s',
|
||||
{'name': event.name, 'tag': event.tag,
|
||||
'instance_uuid': event.instance_uuid})
|
||||
_event['status'] = 'failed'
|
||||
_event['code'] = 404
|
||||
result = 207
|
||||
continue
|
||||
|
||||
# NOTE: before accepting the event, make sure the instance
|
||||
# for which the event is sent is assigned to a host; otherwise
|
||||
# it will not be possible to dispatch the event
|
||||
if instance:
|
||||
if not self._is_event_tag_present_when_required(event):
|
||||
LOG.debug("Event tag is missing for instance "
|
||||
"%(instance)s. Dropping event %(event)s",
|
||||
{'instance': event.instance_uuid,
|
||||
'event': event.name})
|
||||
_event['status'] = 'failed'
|
||||
_event['code'] = 400
|
||||
result = 207
|
||||
elif instance.host:
|
||||
accepted_events.append(event)
|
||||
accepted_instances.add(instance)
|
||||
LOG.info('Creating event %(name)s:%(tag)s for '
|
||||
'instance %(instance_uuid)s on %(host)s',
|
||||
{'name': event.name, 'tag': event.tag,
|
||||
'instance_uuid': event.instance_uuid,
|
||||
'host': instance.host})
|
||||
# NOTE: as the event is processed asynchronously verify
|
||||
# whether 202 is a more suitable response code than 200
|
||||
_event['status'] = 'completed'
|
||||
_event['code'] = 200
|
||||
else:
|
||||
LOG.debug("Unable to find a host for instance "
|
||||
"%(instance)s. Dropping event %(event)s",
|
||||
{'instance': event.instance_uuid,
|
||||
'event': event.name})
|
||||
_event['status'] = 'failed'
|
||||
_event['code'] = 422
|
||||
result = 207
|
||||
|
||||
response_events.append(_event)
|
||||
if not self._is_event_tag_present_when_required(event):
|
||||
LOG.debug("Event tag is missing for instance "
|
||||
"%(instance)s. Dropping event %(event)s",
|
||||
{'instance': event.instance_uuid,
|
||||
'event': event.name})
|
||||
_event['status'] = 'failed'
|
||||
_event['code'] = 400
|
||||
result = 207
|
||||
elif instance.host:
|
||||
accepted_events.append(event)
|
||||
accepted_instances.add(instance)
|
||||
LOG.info('Creating event %(name)s:%(tag)s for '
|
||||
'instance %(instance_uuid)s on %(host)s',
|
||||
{'name': event.name, 'tag': event.tag,
|
||||
'instance_uuid': event.instance_uuid,
|
||||
'host': instance.host})
|
||||
# NOTE: as the event is processed asynchronously verify
|
||||
# whether 202 is a more suitable response code than 200
|
||||
_event['status'] = 'completed'
|
||||
_event['code'] = 200
|
||||
else:
|
||||
LOG.debug("Unable to find a host for instance "
|
||||
"%(instance)s. Dropping event %(event)s",
|
||||
{'instance': event.instance_uuid,
|
||||
'event': event.name})
|
||||
_event['status'] = 'failed'
|
||||
_event['code'] = 422
|
||||
result = 207
|
||||
|
||||
if accepted_events:
|
||||
self.compute_api.external_instance_event(
|
||||
context, accepted_instances, mappings, accepted_events)
|
||||
context, accepted_instances, accepted_events)
|
||||
else:
|
||||
msg = _('No instances found for any event')
|
||||
raise webob.exc.HTTPNotFound(explanation=msg)
|
||||
|
|
|
@ -4312,7 +4312,7 @@ class API(base.Base):
|
|||
|
||||
do_volume_snapshot_delete(self, context, bdm.instance)
|
||||
|
||||
def external_instance_event(self, context, instances, mappings, events):
|
||||
def external_instance_event(self, api_context, instances, events):
|
||||
# NOTE(danms): The external API consumer just provides events,
|
||||
# but doesn't know where they go. We need to collate lists
|
||||
# by the host the affected instance is on and dispatch them
|
||||
|
@ -4320,8 +4320,23 @@ class API(base.Base):
|
|||
instances_by_host = collections.defaultdict(list)
|
||||
events_by_host = collections.defaultdict(list)
|
||||
hosts_by_instance = collections.defaultdict(list)
|
||||
cell_contexts_by_host = {}
|
||||
for instance in instances:
|
||||
for host in self._get_relevant_hosts(context, instance):
|
||||
# instance._context is used here since it's already targeted to
|
||||
# the cell that the instance lives in, and we need to use that
|
||||
# cell context to lookup any migrations associated to the instance.
|
||||
for host in self._get_relevant_hosts(instance._context, instance):
|
||||
# NOTE(danms): All instances on a host must have the same
|
||||
# mapping, so just use that
|
||||
# NOTE(mdbooth): We don't currently support migrations between
|
||||
# cells, and given that the Migration record is hosted in the
|
||||
# cell _get_relevant_hosts will likely have to change before we
|
||||
# do. Consequently we can currently assume that the context for
|
||||
# both the source and destination hosts of a migration is the
|
||||
# same.
|
||||
if host not in cell_contexts_by_host:
|
||||
cell_contexts_by_host[host] = instance._context
|
||||
|
||||
instances_by_host[host].append(instance)
|
||||
hosts_by_instance[instance.uuid].append(host)
|
||||
|
||||
|
@ -4330,24 +4345,23 @@ class API(base.Base):
|
|||
# Volume extend is a user-initiated operation starting in the
|
||||
# Block Storage service API. We record an instance action so
|
||||
# the user can monitor the operation to completion.
|
||||
host = hosts_by_instance[event.instance_uuid][0]
|
||||
cell_context = cell_contexts_by_host[host]
|
||||
objects.InstanceAction.action_start(
|
||||
context, event.instance_uuid,
|
||||
cell_context, event.instance_uuid,
|
||||
instance_actions.EXTEND_VOLUME, want_result=False)
|
||||
for host in hosts_by_instance[event.instance_uuid]:
|
||||
events_by_host[host].append(event)
|
||||
|
||||
for host in instances_by_host:
|
||||
# NOTE(danms): All instances on a host must have the same
|
||||
# mapping, so just use that
|
||||
cell_mapping = mappings[instances_by_host[host][0].uuid]
|
||||
cell_context = cell_contexts_by_host[host]
|
||||
|
||||
# TODO(salv-orlando): Handle exceptions raised by the rpc api layer
|
||||
# in order to ensure that a failure in processing events on a host
|
||||
# will not prevent processing events on other hosts
|
||||
with nova_context.target_cell(context, cell_mapping) as cctxt:
|
||||
self.compute_rpcapi.external_instance_event(
|
||||
cctxt, instances_by_host[host], events_by_host[host],
|
||||
host=host)
|
||||
self.compute_rpcapi.external_instance_event(
|
||||
cell_context, instances_by_host[host], events_by_host[host],
|
||||
host=host)
|
||||
|
||||
def _get_relevant_hosts(self, context, instance):
|
||||
hosts = set()
|
||||
|
|
|
@ -288,6 +288,9 @@ class SingleCellSimple(fixtures.Fixture):
|
|||
self.useFixture(fixtures.MonkeyPatch(
|
||||
'nova.objects.InstanceMapping._get_by_instance_uuid_from_db',
|
||||
self._fake_instancemapping_get))
|
||||
self.useFixture(fixtures.MonkeyPatch(
|
||||
'nova.objects.InstanceMappingList._get_by_instance_uuids_from_db',
|
||||
self._fake_instancemapping_get_uuids))
|
||||
self.useFixture(fixtures.MonkeyPatch(
|
||||
'nova.objects.InstanceMapping._save_in_db',
|
||||
self._fake_instancemapping_get))
|
||||
|
@ -310,13 +313,17 @@ class SingleCellSimple(fixtures.Fixture):
|
|||
'id': 1,
|
||||
'updated_at': None,
|
||||
'created_at': None,
|
||||
'instance_uuid': uuidsentinel.instance,
|
||||
'instance_uuid': args[-1],
|
||||
'cell_id': (self.instances_created and 1 or None),
|
||||
'project_id': 'project',
|
||||
'cell_mapping': (
|
||||
self.instances_created and self._fake_cell_get() or None),
|
||||
}
|
||||
|
||||
def _fake_instancemapping_get_uuids(self, *args):
|
||||
return [self._fake_instancemapping_get(uuid)
|
||||
for uuid in args[-1]]
|
||||
|
||||
def _fake_cell_get(self, *args):
|
||||
return self._fake_cell_list()[0]
|
||||
|
||||
|
|
|
@ -22,36 +22,55 @@ from nova import objects
|
|||
from nova.objects import instance as instance_obj
|
||||
from nova import test
|
||||
from nova.tests.unit.api.openstack import fakes
|
||||
from nova.tests import uuidsentinel as uuids
|
||||
|
||||
fake_instances = {
|
||||
'00000000-0000-0000-0000-000000000001': objects.Instance(
|
||||
'00000000-0000-0000-0000-000000000001': objects.Instance(id=1,
|
||||
uuid='00000000-0000-0000-0000-000000000001', host='host1'),
|
||||
'00000000-0000-0000-0000-000000000002': objects.Instance(
|
||||
'00000000-0000-0000-0000-000000000002': objects.Instance(id=2,
|
||||
uuid='00000000-0000-0000-0000-000000000002', host='host1'),
|
||||
'00000000-0000-0000-0000-000000000003': objects.Instance(
|
||||
'00000000-0000-0000-0000-000000000003': objects.Instance(id=3,
|
||||
uuid='00000000-0000-0000-0000-000000000003', host='host2'),
|
||||
'00000000-0000-0000-0000-000000000004': objects.Instance(
|
||||
'00000000-0000-0000-0000-000000000004': objects.Instance(id=4,
|
||||
uuid='00000000-0000-0000-0000-000000000004', host=None),
|
||||
}
|
||||
fake_instance_uuids = sorted(fake_instances.keys())
|
||||
MISSING_UUID = '00000000-0000-0000-0000-000000000005'
|
||||
|
||||
fake_cells = [objects.CellMapping(uuid=uuids.cell1, database_connection="db1"),
|
||||
objects.CellMapping(uuid=uuids.cell2, database_connection="db2")]
|
||||
fake_instance_mappings = [
|
||||
objects.InstanceMapping(cell_mapping=fake_cells[instance.id % 2],
|
||||
instance_uuid=instance.uuid)
|
||||
for instance in fake_instances.values()]
|
||||
|
||||
|
||||
@classmethod
|
||||
def fake_get_by_uuid(cls, context, uuid, **kwargs):
|
||||
if 'expected_attrs' in kwargs:
|
||||
expected_attrs_set = set(kwargs['expected_attrs'])
|
||||
def fake_get_by_filters(cls, context, filters, expected_attrs=None):
|
||||
if expected_attrs:
|
||||
# This is a regression check for bug 1645479.
|
||||
expected_attrs_set = set(expected_attrs)
|
||||
full_expected_attrs_set = set(instance_obj.INSTANCE_OPTIONAL_ATTRS)
|
||||
assert expected_attrs_set.issubset(full_expected_attrs_set), \
|
||||
('%s is not a subset of %s' % (expected_attrs_set,
|
||||
full_expected_attrs_set))
|
||||
try:
|
||||
return fake_instances[uuid]
|
||||
except KeyError:
|
||||
raise exception.InstanceNotFound(instance_id=uuid)
|
||||
l = objects.InstanceList(objects=[
|
||||
inst for inst in fake_instances.values()
|
||||
if inst.uuid in filters['uuid']])
|
||||
return l
|
||||
|
||||
|
||||
@mock.patch('nova.objects.instance.Instance.get_by_uuid', fake_get_by_uuid)
|
||||
@classmethod
|
||||
def fake_get_by_instance_uuids(cls, context, uuids):
|
||||
mappings = [im for im in fake_instance_mappings
|
||||
if im.instance_uuid in uuids]
|
||||
return objects.InstanceMappingList(objects=mappings)
|
||||
|
||||
|
||||
@mock.patch('nova.objects.InstanceMappingList.get_by_instance_uuids',
|
||||
fake_get_by_instance_uuids)
|
||||
@mock.patch('nova.objects.InstanceList.get_by_filters',
|
||||
fake_get_by_filters)
|
||||
class ServerExternalEventsTestV21(test.NoDBTestCase):
|
||||
server_external_events = server_external_events_v21
|
||||
invalid_error = exception.ValidationError
|
||||
|
@ -87,12 +106,21 @@ class ServerExternalEventsTestV21(test.NoDBTestCase):
|
|||
code = response._code
|
||||
|
||||
self.assertEqual(1, api_method.call_count)
|
||||
for inst in api_method.call_args_list[0][0][1]:
|
||||
expected_uuids.remove(inst.uuid)
|
||||
self.assertEqual([], expected_uuids)
|
||||
for event in api_method.call_args_list[0][0][3]:
|
||||
expected_events.remove(event.name)
|
||||
self.assertEqual([], expected_events)
|
||||
call = api_method.call_args_list[0]
|
||||
args = call[0]
|
||||
|
||||
call_instances = args[1]
|
||||
call_events = args[2]
|
||||
|
||||
self.assertEqual(set(expected_uuids),
|
||||
set([instance.uuid for instance in call_instances]))
|
||||
self.assertEqual(len(expected_uuids), len(call_instances))
|
||||
|
||||
self.assertEqual(set(expected_events),
|
||||
set([event.name for event in call_events]))
|
||||
self.assertEqual(len(expected_events),
|
||||
len(call_events))
|
||||
|
||||
return result, code
|
||||
|
||||
def test_create(self):
|
||||
|
@ -161,7 +189,10 @@ class ServerExternalEventsTestV21(test.NoDBTestCase):
|
|||
self.api.create, self.req, body=body)
|
||||
|
||||
|
||||
@mock.patch('nova.objects.instance.Instance.get_by_uuid', fake_get_by_uuid)
|
||||
@mock.patch('nova.objects.InstanceMappingList.get_by_instance_uuids',
|
||||
fake_get_by_instance_uuids)
|
||||
@mock.patch('nova.objects.InstanceList.get_by_filters',
|
||||
fake_get_by_filters)
|
||||
class ServerExternalEventsTestV251(ServerExternalEventsTestV21):
|
||||
wsgi_api_version = '2.51'
|
||||
|
||||
|
|
|
@ -3449,9 +3449,14 @@ class _ComputeAPIUnitTestMixIn(object):
|
|||
objects.Instance(uuid=uuids.instance_4, host='host2',
|
||||
migration_context=None),
|
||||
]
|
||||
mappings = {inst.uuid: objects.InstanceMapping.get_by_instance_uuid(
|
||||
self.context, inst.uuid)
|
||||
for inst in instances}
|
||||
# Create a single cell context and associate it with all instances
|
||||
mapping = objects.InstanceMapping.get_by_instance_uuid(
|
||||
self.context, instances[0].uuid)
|
||||
with context.target_cell(self.context, mapping.cell_mapping) as cc:
|
||||
cell_context = cc
|
||||
for instance in instances:
|
||||
instance._context = cell_context
|
||||
|
||||
volume_id = uuidutils.generate_uuid()
|
||||
events = [
|
||||
objects.InstanceExternalEvent(
|
||||
|
@ -3470,11 +3475,11 @@ class _ComputeAPIUnitTestMixIn(object):
|
|||
]
|
||||
self.compute_api.compute_rpcapi = mock.MagicMock()
|
||||
self.compute_api.external_instance_event(self.context,
|
||||
instances, mappings, events)
|
||||
instances, events)
|
||||
method = self.compute_api.compute_rpcapi.external_instance_event
|
||||
method.assert_any_call(self.context, instances[0:2], events[0:2],
|
||||
method.assert_any_call(cell_context, instances[0:2], events[0:2],
|
||||
host='host1')
|
||||
method.assert_any_call(self.context, instances[2:], events[2:],
|
||||
method.assert_any_call(cell_context, instances[2:], events[2:],
|
||||
host='host2')
|
||||
mock_action_start.assert_called_once_with(
|
||||
self.context, uuids.instance_4, instance_actions.EXTEND_VOLUME,
|
||||
|
@ -3510,8 +3515,15 @@ class _ComputeAPIUnitTestMixIn(object):
|
|||
objects.Instance(uuid=uuids.instance_3, host='host2',
|
||||
migration_context=None)
|
||||
]
|
||||
mappings = {inst.uuid: objects.InstanceMapping.get_by_instance_uuid(
|
||||
self.context, inst.uuid) for inst in instances}
|
||||
|
||||
# Create a single cell context and associate it with all instances
|
||||
mapping = objects.InstanceMapping.get_by_instance_uuid(
|
||||
self.context, instances[0].uuid)
|
||||
with context.target_cell(self.context, mapping.cell_mapping) as cc:
|
||||
cell_context = cc
|
||||
for instance in instances:
|
||||
instance._context = cell_context
|
||||
|
||||
events = [
|
||||
objects.InstanceExternalEvent(
|
||||
instance_uuid=uuids.instance_1,
|
||||
|
@ -3527,12 +3539,11 @@ class _ComputeAPIUnitTestMixIn(object):
|
|||
with mock.patch('nova.db.sqlalchemy.api.migration_get', migration_get):
|
||||
self.compute_api.compute_rpcapi = mock.MagicMock()
|
||||
self.compute_api.external_instance_event(self.context,
|
||||
instances, mappings,
|
||||
events)
|
||||
instances, events)
|
||||
method = self.compute_api.compute_rpcapi.external_instance_event
|
||||
method.assert_any_call(self.context, instances[0:2], events[0:2],
|
||||
method.assert_any_call(cell_context, instances[0:2], events[0:2],
|
||||
host='host1')
|
||||
method.assert_any_call(self.context, instances[1:], events[1:],
|
||||
method.assert_any_call(cell_context, instances[1:], events[1:],
|
||||
host='host2')
|
||||
self.assertEqual(2, method.call_count)
|
||||
|
||||
|
|
Loading…
Reference in New Issue