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 <[email protected]>
(cherry picked from commit dc814a6)

Author: stephenfin

nova/tests/unit/virt/libvirt/test_driver.py

@@ -18963,7 +18963,7 @@ def test_create_guest_with_network__with_other_error(self):
 
         @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 @@ def the_test(mock_cleanup, mock_create, mock_plug):
                 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(

nova/virt/libvirt/driver.py

@@ -7224,27 +7224,20 @@ def _get_neutron_events(self, network_info):
         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,54 +7251,56 @@ def _create_guest_with_network(self, context, xml, instance, network_info,
             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()
         except Exception:
             # 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):