libvirt: Remove dead error handling code
We removed calls to configure the libvirt-based firewall driver in '_create_guest_with_network' as part of change I5a9e5532c46a5f7064441ae644125d21efe5fda1. Since then, there only other way we can successfully create a guest and still end up in an exception handler is if the plugging of VIFs fails. Despite this, all three code paths still contained references to a 'guest' attribute that would never be set. Clarify things by removing the use of the 'guest' attribute from the other code paths. This means the '_cleanup_failed_start' helper only has one caller and some unnecessary logic itself, so we can fold that into its caller. We also take this opportunity to remove an unnecessary error handler - 'nova.exception.VirtualInterfaceCreateException' is derived from 'Exception' and doesn't need its own handler - and add the type hints for the function. The heavy lifting for the latter was already done in change I2489bf16dabc8a83b2044139247f4245ae29adb1 so this is pretty easy to grok. Next up, we really need to stop passing around dicts of block device mapping info, but that's another change for another day. Change-Id: I0b93bdc12cdce591c7e642ab8830e92445467b9a Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
This commit is contained in:
Stephen Finucane
Balazs Gibizer
parent
a7f2b65d17
commit
dc814a6822
|
|
@ -18963,7 +18963,7 @@ class LibvirtConnTestCase(test.NoDBTestCase,
|
|||
|
||||
@mock.patch.object(drvr, 'plug_vifs')
|
||||
@mock.patch.object(drvr, '_create_guest')
|
||||
@mock.patch.object(drvr, '_cleanup_failed_start')
|
||||
@mock.patch.object(drvr, '_cleanup')
|
||||
def the_test(mock_cleanup, mock_create, mock_plug):
|
||||
instance = objects.Instance(**self.test_instance)
|
||||
mock_create.side_effect = test.TestingException
|
||||
|
|
@ -18973,67 +18973,11 @@ class LibvirtConnTestCase(test.NoDBTestCase,
|
|||
cleanup_instance_dir=mock.sentinel.cleanup_instance_dir,
|
||||
cleanup_instance_disks=mock.sentinel.cleanup_instance_disks)
|
||||
mock_cleanup.assert_called_once_with(
|
||||
self.context, instance, [], None, None,
|
||||
self.context, instance, [], None, destroy_vifs=True,
|
||||
cleanup_instance_dir=mock.sentinel.cleanup_instance_dir,
|
||||
cleanup_instance_disks=mock.sentinel.cleanup_instance_disks)
|
||||
the_test()
|
||||
|
||||
def test_cleanup_failed_start_no_guest(self):
|
||||
drvr = libvirt_driver.LibvirtDriver(mock.MagicMock(), False)
|
||||
with mock.patch.object(drvr, '_cleanup') as mock_cleanup:
|
||||
drvr._cleanup_failed_start(
|
||||
None, None, None, None, None, False, False)
|
||||
self.assertTrue(mock_cleanup.called)
|
||||
|
||||
def test_cleanup_failed_start_inactive_guest(self):
|
||||
drvr = libvirt_driver.LibvirtDriver(mock.MagicMock(), False)
|
||||
guest = mock.MagicMock()
|
||||
guest.is_active.return_value = False
|
||||
with mock.patch.object(drvr, '_cleanup') as mock_cleanup:
|
||||
drvr._cleanup_failed_start(
|
||||
None, None, None, None, guest, False, False)
|
||||
self.assertTrue(mock_cleanup.called)
|
||||
self.assertFalse(guest.poweroff.called)
|
||||
|
||||
def test_cleanup_failed_start_active_guest(self):
|
||||
drvr = libvirt_driver.LibvirtDriver(mock.MagicMock(), False)
|
||||
guest = mock.MagicMock()
|
||||
guest.is_active.return_value = True
|
||||
with mock.patch.object(drvr, '_cleanup') as mock_cleanup:
|
||||
drvr._cleanup_failed_start(
|
||||
None, None, None, None, guest, False, False)
|
||||
self.assertTrue(mock_cleanup.called)
|
||||
self.assertTrue(guest.poweroff.called)
|
||||
|
||||
def test_cleanup_failed_start_failed_poweroff(self):
|
||||
drvr = libvirt_driver.LibvirtDriver(mock.MagicMock(), False)
|
||||
guest = mock.MagicMock()
|
||||
guest.is_active.return_value = True
|
||||
guest.poweroff.side_effect = test.TestingException
|
||||
with mock.patch.object(drvr, '_cleanup') as mock_cleanup:
|
||||
self.assertRaises(test.TestingException,
|
||||
drvr._cleanup_failed_start,
|
||||
None, None, None, None, guest, False, False)
|
||||
self.assertTrue(mock_cleanup.called)
|
||||
self.assertTrue(guest.poweroff.called)
|
||||
|
||||
def test_cleanup_failed_start_failed_poweroff_destroy_disks(self):
|
||||
drvr = libvirt_driver.LibvirtDriver(mock.MagicMock(), False)
|
||||
guest = mock.MagicMock()
|
||||
guest.is_active.return_value = True
|
||||
guest.poweroff.side_effect = test.TestingException
|
||||
with mock.patch.object(drvr, '_cleanup') as mock_cleanup:
|
||||
self.assertRaises(
|
||||
test.TestingException, drvr._cleanup_failed_start,
|
||||
None, None, None, None, guest,
|
||||
cleanup_instance_dir=mock.sentinel.cleanup_instance_dir,
|
||||
cleanup_instance_disks=mock.sentinel.cleanup_instance_disks)
|
||||
mock_cleanup.assert_called_once_with(
|
||||
None, None, None, block_device_info=None, destroy_vifs=True,
|
||||
cleanup_instance_dir=mock.sentinel.cleanup_instance_dir,
|
||||
cleanup_instance_disks=mock.sentinel.cleanup_instance_disks)
|
||||
self.assertTrue(guest.poweroff.called)
|
||||
|
||||
@mock.patch('os_brick.encryptors.get_encryption_metadata')
|
||||
@mock.patch('nova.virt.libvirt.blockinfo.get_info_from_bdm')
|
||||
def test_create_guest_with_network__with_bdm(
|
||||
|
|
|
|||
|
|
@ -7224,27 +7224,20 @@ class LibvirtDriver(driver.ComputeDriver):
|
|||
return [('network-vif-plugged', vif['id'])
|
||||
for vif in network_info if vif.get('active', True) is False]
|
||||
|
||||
def _cleanup_failed_start(self, context, instance, network_info,
|
||||
block_device_info, guest,
|
||||
cleanup_instance_dir=False,
|
||||
cleanup_instance_disks=False):
|
||||
try:
|
||||
if guest and guest.is_active():
|
||||
guest.poweroff()
|
||||
finally:
|
||||
self._cleanup(context, instance, network_info,
|
||||
block_device_info=block_device_info,
|
||||
destroy_vifs=True,
|
||||
cleanup_instance_dir=cleanup_instance_dir,
|
||||
cleanup_instance_disks=cleanup_instance_disks)
|
||||
|
||||
def _create_guest_with_network(self, context, xml, instance, network_info,
|
||||
block_device_info, power_on=True,
|
||||
vifs_already_plugged=False,
|
||||
post_xml_callback=None,
|
||||
external_events=None,
|
||||
cleanup_instance_dir=False,
|
||||
cleanup_instance_disks=False):
|
||||
def _create_guest_with_network(
|
||||
self,
|
||||
context: nova_context.RequestContext,
|
||||
xml: str,
|
||||
instance: 'objects.Instance',
|
||||
network_info: network_model.NetworkInfo,
|
||||
block_device_info: ty.Optional[ty.Dict[str, ty.Any]],
|
||||
power_on: bool = True,
|
||||
vifs_already_plugged: bool = False,
|
||||
post_xml_callback: ty.Callable = None,
|
||||
external_events: ty.Optional[ty.List[str]] = None,
|
||||
cleanup_instance_dir: bool = False,
|
||||
cleanup_instance_disks: bool = False,
|
||||
) -> libvirt_guest.Guest:
|
||||
"""Do required network setup and create domain."""
|
||||
|
||||
timeout = CONF.vif_plugging_timeout
|
||||
|
|
@ -7258,39 +7251,39 @@ class LibvirtDriver(driver.ComputeDriver):
|
|||
events = []
|
||||
|
||||
pause = bool(events)
|
||||
guest: ty.Optional[libvirt_guest.Guest] = None
|
||||
try:
|
||||
with self.virtapi.wait_for_instance_event(
|
||||
instance, events, deadline=timeout,
|
||||
error_callback=self._neutron_failed_callback):
|
||||
instance, events, deadline=timeout,
|
||||
error_callback=self._neutron_failed_callback,
|
||||
):
|
||||
self.plug_vifs(instance, network_info)
|
||||
with self._lxc_disk_handler(context, instance,
|
||||
instance.image_meta,
|
||||
block_device_info):
|
||||
with self._lxc_disk_handler(
|
||||
context, instance, instance.image_meta, block_device_info,
|
||||
):
|
||||
guest = self._create_guest(
|
||||
context, xml, instance,
|
||||
pause=pause, power_on=power_on,
|
||||
post_xml_callback=post_xml_callback)
|
||||
except exception.VirtualInterfaceCreateException:
|
||||
# Neutron reported failure and we didn't swallow it, so
|
||||
# bail here
|
||||
with excutils.save_and_reraise_exception():
|
||||
self._cleanup_failed_start(
|
||||
context, instance, network_info, block_device_info, guest,
|
||||
cleanup_instance_dir=cleanup_instance_dir,
|
||||
cleanup_instance_disks=cleanup_instance_disks)
|
||||
except eventlet.timeout.Timeout:
|
||||
# We never heard from Neutron
|
||||
LOG.warning('Timeout waiting for %(events)s for '
|
||||
'instance with vm_state %(vm_state)s and '
|
||||
'task_state %(task_state)s.',
|
||||
{'events': events,
|
||||
'vm_state': instance.vm_state,
|
||||
'task_state': instance.task_state},
|
||||
instance=instance)
|
||||
LOG.warning(
|
||||
'Timeout waiting for %(events)s for instance with '
|
||||
'vm_state %(vm_state)s and task_state %(task_state)s',
|
||||
{
|
||||
'events': events,
|
||||
'vm_state': instance.vm_state,
|
||||
'task_state': instance.task_state,
|
||||
},
|
||||
instance=instance)
|
||||
|
||||
if CONF.vif_plugging_is_fatal:
|
||||
self._cleanup_failed_start(
|
||||
context, instance, network_info, block_device_info, guest,
|
||||
# NOTE(stephenfin): don't worry, guest will be in scope since
|
||||
# we can only hit this branch if the VIF plug timed out
|
||||
if guest.is_active():
|
||||
guest.poweroff()
|
||||
self._cleanup(
|
||||
context, instance, network_info, block_device_info,
|
||||
destroy_vifs=True,
|
||||
cleanup_instance_dir=cleanup_instance_dir,
|
||||
cleanup_instance_disks=cleanup_instance_disks)
|
||||
raise exception.VirtualInterfaceCreateException()
|
||||
|
|
@ -7298,14 +7291,16 @@ class LibvirtDriver(driver.ComputeDriver):
|
|||
# Any other error, be sure to clean up
|
||||
LOG.error('Failed to start libvirt guest', instance=instance)
|
||||
with excutils.save_and_reraise_exception():
|
||||
self._cleanup_failed_start(
|
||||
context, instance, network_info, block_device_info, guest,
|
||||
self._cleanup(
|
||||
context, instance, network_info, block_device_info,
|
||||
destroy_vifs=True,
|
||||
cleanup_instance_dir=cleanup_instance_dir,
|
||||
cleanup_instance_disks=cleanup_instance_disks)
|
||||
|
||||
# Resume only if domain has been paused
|
||||
if pause:
|
||||
assert guest is not None
|
||||
guest.resume()
|
||||
|
||||
return guest
|
||||
|
||||
def _get_pcpu_available(self):
|
||||
|
|
|
|||
Loading…
Reference in New Issue