Fix cancel_all_events event name parsing
The cancel_all_events function was incorrectly assuming that event tags didn't have dashes in them, but they all do since they are either port or volume IDs, which are UUIDs. Instead of just storing our event data indexed by the composite event key, we should just store the name and tag distinctly so that there is no question about how to dissect the two from the key later. This is all internal record-keeping inside of the compute manager, so we can change it to whatever we want. This includes a slight change to the internal virtapi interface for wait_for_instance_event(), which removes the ability to pass composite event keys as strings instead of (name, tag) tuples. In reality, none of the virt code was using the key string format, so this just cleans up a few tests that were. It makes that interface less ambiguous, which is good anyway. Change-Id: I15f041e820c2dabe82e8e25bf77f9cc86330a76a Closes-Bug: #1760303
This commit is contained in:
Matt Riedemann
Dan Smith
parent
167023b507
commit
d9369d034c
|
|
@ -298,7 +298,7 @@ class InstanceEvents(object):
|
|||
def _lock_name(instance):
|
||||
return '%s-%s' % (instance.uuid, 'events')
|
||||
|
||||
def prepare_for_instance_event(self, instance, event_name):
|
||||
def prepare_for_instance_event(self, instance, name, tag):
|
||||
"""Prepare to receive an event for an instance.
|
||||
|
||||
This will register an event for the given instance that we will
|
||||
|
|
@ -307,7 +307,8 @@ class InstanceEvents(object):
|
|||
object should be wait()'d on to ensure completion.
|
||||
|
||||
:param instance: the instance for which the event will be generated
|
||||
:param event_name: the name of the event we're expecting
|
||||
:param name: the name of the event we're expecting
|
||||
:param tag: the tag associated with the event we're expecting
|
||||
:returns: an event object that should be wait()'d on
|
||||
"""
|
||||
if self._events is None:
|
||||
|
|
@ -319,10 +320,10 @@ class InstanceEvents(object):
|
|||
@utils.synchronized(self._lock_name(instance))
|
||||
def _create_or_get_event():
|
||||
instance_events = self._events.setdefault(instance.uuid, {})
|
||||
return instance_events.setdefault(event_name,
|
||||
return instance_events.setdefault((name, tag),
|
||||
eventlet.event.Event())
|
||||
LOG.debug('Preparing to wait for external event %(event)s',
|
||||
{'event': event_name}, instance=instance)
|
||||
LOG.debug('Preparing to wait for external event %(name)s-%(tag)s',
|
||||
{'name': name, 'tag': tag}, instance=instance)
|
||||
return _create_or_get_event()
|
||||
|
||||
def pop_instance_event(self, instance, event):
|
||||
|
|
@ -349,7 +350,7 @@ class InstanceEvents(object):
|
|||
events = self._events.get(instance.uuid)
|
||||
if not events:
|
||||
return no_events_sentinel
|
||||
_event = events.pop(event.key, None)
|
||||
_event = events.pop((event.name, event.tag), None)
|
||||
if not events:
|
||||
del self._events[instance.uuid]
|
||||
if _event is None:
|
||||
|
|
@ -386,7 +387,11 @@ class InstanceEvents(object):
|
|||
LOG.debug('Unexpected attempt to clear events during shutdown',
|
||||
instance=instance)
|
||||
return dict()
|
||||
return self._events.pop(instance.uuid, {})
|
||||
# NOTE(danms): We have historically returned the raw internal
|
||||
# format here, which is {event.key: [events, ...])} so just
|
||||
# trivially convert it here.
|
||||
return {'%s-%s' % k: e
|
||||
for k, e in self._events.pop(instance.uuid, {}).items()}
|
||||
return _clear_events()
|
||||
|
||||
def cancel_all_events(self):
|
||||
|
|
@ -398,12 +403,12 @@ class InstanceEvents(object):
|
|||
self._events = None
|
||||
|
||||
for instance_uuid, events in our_events.items():
|
||||
for event_name, eventlet_event in events.items():
|
||||
LOG.debug('Canceling in-flight event %(event)s for '
|
||||
for (name, tag), eventlet_event in events.items():
|
||||
LOG.debug('Canceling in-flight event %(name)s-%(tag)s for '
|
||||
'instance %(instance_uuid)s',
|
||||
{'event': event_name,
|
||||
{'name': name,
|
||||
'tag': tag,
|
||||
'instance_uuid': instance_uuid})
|
||||
name, tag = event_name.rsplit('-', 1)
|
||||
event = objects.InstanceExternalEvent(
|
||||
instance_uuid=instance_uuid,
|
||||
name=name, status='failed',
|
||||
|
|
@ -444,9 +449,9 @@ class ComputeVirtAPI(virtapi.VirtAPI):
|
|||
or raise an exception which will bubble up to the waiter.
|
||||
|
||||
:param instance: The instance for which an event is expected
|
||||
:param event_names: A list of event names. Each element can be a
|
||||
string event name or tuple of strings to
|
||||
indicate (name, tag).
|
||||
:param event_names: A list of event names. Each element is a
|
||||
tuple of strings to indicate (name, tag),
|
||||
where name is required, but tag may be None.
|
||||
:param deadline: Maximum number of seconds we should wait for all
|
||||
of the specified events to arrive.
|
||||
:param error_callback: A function to be called if an event arrives
|
||||
|
|
@ -457,14 +462,12 @@ class ComputeVirtAPI(virtapi.VirtAPI):
|
|||
error_callback = self._default_error_callback
|
||||
events = {}
|
||||
for event_name in event_names:
|
||||
if isinstance(event_name, tuple):
|
||||
name, tag = event_name
|
||||
event_name = objects.InstanceExternalEvent.make_key(
|
||||
name, tag)
|
||||
name, tag = event_name
|
||||
event_name = objects.InstanceExternalEvent.make_key(name, tag)
|
||||
try:
|
||||
events[event_name] = (
|
||||
self._compute.instance_events.prepare_for_instance_event(
|
||||
instance, event_name))
|
||||
instance, name, tag))
|
||||
except exception.NovaException:
|
||||
error_callback(event_name, instance)
|
||||
# NOTE(danms): Don't wait for any of the events. They
|
||||
|
|
|
|||
|
|
@ -2556,13 +2556,14 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
|
|||
def test_prepare_for_instance_event(self, lock_name_mock):
|
||||
inst_obj = objects.Instance(uuid=uuids.instance)
|
||||
result = self.compute.instance_events.prepare_for_instance_event(
|
||||
inst_obj, 'test-event')
|
||||
inst_obj, 'test-event', None)
|
||||
self.assertIn(uuids.instance, self.compute.instance_events._events)
|
||||
self.assertIn('test-event',
|
||||
self.assertIn(('test-event', None),
|
||||
self.compute.instance_events._events[uuids.instance])
|
||||
self.assertEqual(
|
||||
result,
|
||||
self.compute.instance_events._events[uuids.instance]['test-event'])
|
||||
self.compute.instance_events._events[uuids.instance]
|
||||
[('test-event', None)])
|
||||
self.assertTrue(hasattr(result, 'send'))
|
||||
lock_name_mock.assert_called_once_with(inst_obj)
|
||||
|
||||
|
|
@ -2571,7 +2572,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
|
|||
event = eventlet_event.Event()
|
||||
self.compute.instance_events._events = {
|
||||
uuids.instance: {
|
||||
'network-vif-plugged': event,
|
||||
('network-vif-plugged', None): event,
|
||||
}
|
||||
}
|
||||
inst_obj = objects.Instance(uuid=uuids.instance)
|
||||
|
|
@ -2587,13 +2588,13 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
|
|||
event = eventlet_event.Event()
|
||||
self.compute.instance_events._events = {
|
||||
uuids.instance: {
|
||||
'test-event': event,
|
||||
('test-event', None): event,
|
||||
}
|
||||
}
|
||||
inst_obj = objects.Instance(uuid=uuids.instance)
|
||||
result = self.compute.instance_events.clear_events_for_instance(
|
||||
inst_obj)
|
||||
self.assertEqual(result, {'test-event': event})
|
||||
self.assertEqual(result, {'test-event-None': event})
|
||||
lock_name_mock.assert_called_once_with(inst_obj)
|
||||
|
||||
def test_instance_events_lock_name(self):
|
||||
|
|
@ -2604,24 +2605,25 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
|
|||
def test_prepare_for_instance_event_again(self):
|
||||
inst_obj = objects.Instance(uuid=uuids.instance)
|
||||
self.compute.instance_events.prepare_for_instance_event(
|
||||
inst_obj, 'test-event')
|
||||
inst_obj, 'test-event', None)
|
||||
# A second attempt will avoid creating a new list; make sure we
|
||||
# get the current list
|
||||
result = self.compute.instance_events.prepare_for_instance_event(
|
||||
inst_obj, 'test-event')
|
||||
inst_obj, 'test-event', None)
|
||||
self.assertIn(uuids.instance, self.compute.instance_events._events)
|
||||
self.assertIn('test-event',
|
||||
self.assertIn(('test-event', None),
|
||||
self.compute.instance_events._events[uuids.instance])
|
||||
self.assertEqual(
|
||||
result,
|
||||
self.compute.instance_events._events[uuids.instance]['test-event'])
|
||||
self.compute.instance_events._events[uuids.instance]
|
||||
[('test-event', None)])
|
||||
self.assertTrue(hasattr(result, 'send'))
|
||||
|
||||
def test_process_instance_event(self):
|
||||
event = eventlet_event.Event()
|
||||
self.compute.instance_events._events = {
|
||||
uuids.instance: {
|
||||
'network-vif-plugged': event,
|
||||
('network-vif-plugged', None): event,
|
||||
}
|
||||
}
|
||||
inst_obj = objects.Instance(uuid=uuids.instance)
|
||||
|
|
@ -2921,7 +2923,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
|
|||
fake_eventlet_event = mock.MagicMock()
|
||||
self.compute.instance_events._events = {
|
||||
inst.uuid: {
|
||||
'network-vif-plugged-bar': fake_eventlet_event,
|
||||
('network-vif-plugged', uuids.portid): fake_eventlet_event,
|
||||
}
|
||||
}
|
||||
self.compute.instance_events.cancel_all_events()
|
||||
|
|
@ -2930,7 +2932,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
|
|||
self.assertTrue(fake_eventlet_event.send.called)
|
||||
event = fake_eventlet_event.send.call_args_list[0][0][0]
|
||||
self.assertEqual('network-vif-plugged', event.name)
|
||||
self.assertEqual('bar', event.tag)
|
||||
self.assertEqual(uuids.portid, event.tag)
|
||||
self.assertEqual('failed', event.status)
|
||||
|
||||
def test_cleanup_cancels_all_events(self):
|
||||
|
|
@ -2944,7 +2946,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
|
|||
callback = mock.MagicMock()
|
||||
body = mock.MagicMock()
|
||||
with self.compute.virtapi.wait_for_instance_event(
|
||||
instance, ['network-vif-plugged-bar'],
|
||||
instance, [('network-vif-plugged', 'bar')],
|
||||
error_callback=callback):
|
||||
body()
|
||||
self.assertTrue(body.called)
|
||||
|
|
|
|||
|
|
@ -93,10 +93,12 @@ class FakeCompute(object):
|
|||
event.status = 'completed'
|
||||
return event
|
||||
|
||||
def _prepare_for_instance_event(self, instance, event_name):
|
||||
def _prepare_for_instance_event(self, instance, name, tag):
|
||||
m = mock.MagicMock()
|
||||
m.instance = instance
|
||||
m.event_name = event_name
|
||||
m.name = name
|
||||
m.tag = tag
|
||||
m.event_name = '%s-%s' % (name, tag)
|
||||
m.wait.side_effect = self._event_waiter
|
||||
self._events.append(m)
|
||||
return m
|
||||
|
|
@ -125,7 +127,7 @@ class ComputeVirtAPITest(VirtAPIBaseTest):
|
|||
event_2_tag = objects.InstanceExternalEvent.make_key(
|
||||
'event2', 'tag')
|
||||
events = {
|
||||
'event1': event_1_tag,
|
||||
('event1', None): event_1_tag,
|
||||
('event2', 'tag'): event_2_tag,
|
||||
}
|
||||
with self.virtapi.wait_for_instance_event('instance', events.keys()):
|
||||
|
|
@ -135,7 +137,7 @@ class ComputeVirtAPITest(VirtAPIBaseTest):
|
|||
self.assertEqual(2, len(self.compute._events))
|
||||
for event in self.compute._events:
|
||||
self.assertEqual('instance', event.instance)
|
||||
self.assertIn(event.event_name, events.values())
|
||||
self.assertIn((event.name, event.tag), events.keys())
|
||||
event.wait.assert_called_once_with()
|
||||
|
||||
def test_wait_for_instance_event_failed(self):
|
||||
|
|
@ -146,7 +148,8 @@ class ComputeVirtAPITest(VirtAPIBaseTest):
|
|||
|
||||
@mock.patch.object(self.virtapi._compute, '_event_waiter', _failer)
|
||||
def do_test():
|
||||
with self.virtapi.wait_for_instance_event('instance', ['foo']):
|
||||
with self.virtapi.wait_for_instance_event('instance',
|
||||
[('foo', 'bar')]):
|
||||
pass
|
||||
|
||||
self.assertRaises(exception.NovaException, do_test)
|
||||
|
|
@ -160,7 +163,8 @@ class ComputeVirtAPITest(VirtAPIBaseTest):
|
|||
@mock.patch.object(self.virtapi._compute, '_event_waiter', _failer)
|
||||
def do_test():
|
||||
callback = mock.MagicMock()
|
||||
with self.virtapi.wait_for_instance_event('instance', ['foo'],
|
||||
with self.virtapi.wait_for_instance_event('instance',
|
||||
[('foo', None)],
|
||||
error_callback=callback):
|
||||
pass
|
||||
callback.assert_called_with('foo', 'instance')
|
||||
|
|
@ -177,7 +181,8 @@ class ComputeVirtAPITest(VirtAPIBaseTest):
|
|||
@mock.patch.object(self.virtapi._compute, '_event_waiter', _failer)
|
||||
@mock.patch('eventlet.timeout.Timeout')
|
||||
def do_test(timeout):
|
||||
with self.virtapi.wait_for_instance_event('instance', ['foo']):
|
||||
with self.virtapi.wait_for_instance_event('instance',
|
||||
[('foo', 'bar')]):
|
||||
pass
|
||||
|
||||
self.assertRaises(TestException, do_test)
|
||||
|
|
|
|||
|
|
@ -10545,8 +10545,9 @@ class LibvirtConnTestCase(test.NoDBTestCase,
|
|||
def fake_recover():
|
||||
pass
|
||||
|
||||
def fake_prepare(instance, event_name):
|
||||
ev = mock.MagicMock(instance=instance, event_name=event_name)
|
||||
def fake_prepare(instance, name, tag):
|
||||
ev = mock.MagicMock(instance=instance,
|
||||
event_name='%s-%s' % (name, tag))
|
||||
ev.wait.return_value = mock.MagicMock(status='completed')
|
||||
generated_events.append(ev)
|
||||
return ev
|
||||
|
|
@ -10577,7 +10578,7 @@ class LibvirtConnTestCase(test.NoDBTestCase,
|
|||
CONF.libvirt.live_migration_bandwidth)
|
||||
|
||||
prepare.assert_has_calls([
|
||||
mock.call(instance, 'network-vif-plugged-%s' % uuids.vif_1)])
|
||||
mock.call(instance, 'network-vif-plugged', uuids.vif_1)])
|
||||
for event in generated_events:
|
||||
event.wait.assert_called_once_with()
|
||||
|
||||
|
|
@ -15760,10 +15761,10 @@ class LibvirtConnTestCase(test.NoDBTestCase,
|
|||
event.status = 'completed'
|
||||
return event
|
||||
|
||||
def fake_prepare(instance, event_name):
|
||||
def fake_prepare(instance, name, tag):
|
||||
m = mock.MagicMock()
|
||||
m.instance = instance
|
||||
m.event_name = event_name
|
||||
m.event_name = '%s-%s' % (name, tag)
|
||||
m.wait.side_effect = wait_timeout
|
||||
generated_events.append(m)
|
||||
return m
|
||||
|
|
@ -15802,8 +15803,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
|
|||
|
||||
if utils.is_neutron() and CONF.vif_plugging_timeout and power_on:
|
||||
prepare.assert_has_calls([
|
||||
mock.call(instance, 'network-vif-plugged-%s' % uuids.vif_1),
|
||||
mock.call(instance, 'network-vif-plugged-%s' % uuids.vif_2)])
|
||||
mock.call(instance, 'network-vif-plugged', uuids.vif_1),
|
||||
mock.call(instance, 'network-vif-plugged', uuids.vif_2)])
|
||||
for event in generated_events:
|
||||
if neutron_failure and generated_events.index(event) != 0:
|
||||
self.assertEqual(0, event.call_count)
|
||||
|
|
|
|||
Loading…
Reference in New Issue