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:
Matthew Booth 2017-03-13 18:28:17 +00:00 committed by Matt Riedemann
parent 7fa8eeefdf
commit b8be61eb39
5 changed files with 173 additions and 100 deletions

View File

@ -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)

View File

@ -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()

View File

@ -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]

View File

@ -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'

View File

@ -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)