libvirt: Always disconnect_volume after rebase failures

Previously failures when rebasing onto a new volume would leave the
volume connected to the compute host. For some volume backends such as
iSCSI the subsequent call to terminate_connection would then result in
leftover devices remaining on the host.

This change simply catches any error associated with the rebase and
ensures that disconnect_volume is called for the new volume prior to
terminate_connection finally being called.

NOTE(lyarwood): Conflict caused by MIN_LIBVIRT_VERSION being 1.2.1 in
stable/ocata making I81c32bbea0f04ca876f2078ef2ae0e1975473584
unsuitable. The is_job_complete polling loop removed by that change in
master is now moved into the try block of this change ensuring we catch
any errors that might be thrown while waiting for the async pivot. The
log.exception message also requires translation in Ocata.

Conflicts:
        nova/virt/libvirt/driver.py

Change-Id: I5997000a0ba6341c4987405cdc0760c3b471bd59
Closes-bug: #1685185
(cherry picked from commit 809065485c19fd535db6740bb21b867c41c008fe)

diff --git a/nova/exception.py b/nova/exception.py
index c6ab31cffcdd9649361a6d1d10e83358f9960dfa..84cbcb6941f1b099008b58a8fe785d5071c5ae7f 100644 (file)
--- a/nova/exception.py
+++ b/nova/exception.py
@@ -1790,6 +1790,10 @@ class VolumesNotRemoved(Invalid):
     msg_fmt = _("Failed to remove volume(s): (%(reason)s)")
 
 
+class VolumeRebaseFailed(NovaException):
+    msg_fmt = _("Volume rebase failed: %(reason)s")
+
+
 class InvalidVideoMode(Invalid):
     msg_fmt = _("Provided video model (%(model)s) is not supported.")
 

diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py
index 3be22f6eb642ab668612923320f27d791f4aa2da..b9a317e823905d90732b88ce3890a8685bd6395b 100644 (file)
--- a/nova/tests/unit/virt/libvirt/test_driver.py
+++ b/nova/tests/unit/virt/libvirt/test_driver.py
@@ -14829,6 +14829,70 @@ class LibvirtConnTestCase(test.NoDBTestCase):
     def test_swap_volume_driver_source_is_snapshot(self):
         self._test_swap_volume_driver(source_type='snapshot')
 
+    @mock.patch('nova.virt.libvirt.guest.BlockDevice.rebase')
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume')
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._connect_volume')
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_config')
+    @mock.patch('nova.virt.libvirt.guest.Guest.get_disk')
+    @mock.patch('nova.virt.libvirt.host.Host.get_guest')
+    @mock.patch('nova.virt.libvirt.host.Host.write_instance_config')
+    def test_swap_volume_disconnect_new_volume_on_rebase_error(self,
+            write_config, get_guest, get_disk, get_volume_config,
+            connect_volume, disconnect_volume, rebase):
+        """Assert that disconnect_volume is called for the new volume if an
+           error is encountered while rebasing
+        """
+        conn = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI())
+        instance = objects.Instance(**self.test_instance)
+        guest = libvirt_guest.Guest(mock.MagicMock())
+        get_guest.return_value = guest
+        exc = fakelibvirt.make_libvirtError(fakelibvirt.libvirtError,
+              'internal error', error_code=fakelibvirt.VIR_ERR_INTERNAL_ERROR)
+        rebase.side_effect = exc
+
+        self.assertRaises(exception.VolumeRebaseFailed, conn.swap_volume,
+                          mock.sentinel.old_connection_info,
+                          mock.sentinel.new_connection_info,
+                          instance, '/dev/vdb', 0)
+        connect_volume.assert_called_once_with(
+                mock.sentinel.new_connection_info,
+                {'dev': 'vdb', 'type': 'disk', 'bus': 'virtio'})
+        disconnect_volume.assert_called_once_with(
+                mock.sentinel.new_connection_info, 'vdb')
+
+    @mock.patch('nova.virt.libvirt.guest.BlockDevice.is_job_complete')
+    @mock.patch('nova.virt.libvirt.guest.BlockDevice.abort_job')
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume')
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._connect_volume')
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_config')
+    @mock.patch('nova.virt.libvirt.guest.Guest.get_disk')
+    @mock.patch('nova.virt.libvirt.host.Host.get_guest')
+    @mock.patch('nova.virt.libvirt.host.Host.write_instance_config')
+    def test_swap_volume_disconnect_new_volume_on_pivot_error(self,
+            write_config, get_guest, get_disk, get_volume_config,
+            connect_volume, disconnect_volume, abort_job, is_job_complete):
+        """Assert that disconnect_volume is called for the new volume if an
+           error is encountered while pivoting to the new volume
+        """
+        conn = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI())
+        instance = objects.Instance(**self.test_instance)
+        guest = libvirt_guest.Guest(mock.MagicMock())
+        get_guest.return_value = guest
+        exc = fakelibvirt.make_libvirtError(fakelibvirt.libvirtError,
+              'internal error', error_code=fakelibvirt.VIR_ERR_INTERNAL_ERROR)
+        is_job_complete.return_value = True
+        abort_job.side_effect = [None, exc]
+
+        self.assertRaises(exception.VolumeRebaseFailed, conn.swap_volume,
+                          mock.sentinel.old_connection_info,
+                          mock.sentinel.new_connection_info,
+                          instance, '/dev/vdb', 0)
+        connect_volume.assert_called_once_with(
+                mock.sentinel.new_connection_info,
+                {'dev': 'vdb', 'type': 'disk', 'bus': 'virtio'})
+        disconnect_volume.assert_called_once_with(
+                mock.sentinel.new_connection_info, 'vdb')
+
     @mock.patch('nova.virt.libvirt.guest.BlockDevice.is_job_complete')
     def _test_live_snapshot(self, mock_is_job_complete,
                             can_quiesce=False, require_quiesce=False):

diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py
index 0e8fe0702c149e7a11c4f9a37beea89407e483e3..4fc87b22501e001d69449b6c674fe845b766e3e4 100644 (file)
--- a/nova/virt/libvirt/driver.py
+++ b/nova/virt/libvirt/driver.py
@@ -1265,20 +1265,27 @@ class LibvirtDriver(driver.ComputeDriver):
                 support_uefi = self._has_uefi_support()
                 guest.delete_configuration(support_uefi)
 
-            # Start copy with VIR_DOMAIN_REBASE_REUSE_EXT flag to
-            # allow writing to existing external volume file
-            dev.rebase(new_path, copy=True, reuse_ext=True)
-
-            while not dev.is_job_complete():
-                time.sleep(0.5)
+            try:
+                # Start copy with VIR_DOMAIN_REBASE_REUSE_EXT flag to
+                # allow writing to existing external volume file
+                dev.rebase(new_path, copy=True, reuse_ext=True)
+                while not dev.is_job_complete():
+                    time.sleep(0.5)
 
-            dev.abort_job(pivot=True)
-            if resize_to:
+                dev.abort_job(pivot=True)
                 # NOTE(alex_xu): domain.blockJobAbort isn't sync call. This
                 # is bug in libvirt. So we need waiting for the pivot is
                 # finished. libvirt bug #1119173
                 while not dev.is_job_complete():
                     time.sleep(0.5)
+
+            except Exception as exc:
+                LOG.exception(_LE("Failure rebasing volume %(new_path)s on "
+                    "%(new_path)s."), {'new_path': new_path,
+                                       'old_path': disk_path})
+                raise exception.VolumeRebaseFailed(reason=six.text_type(exc))
+
+            if resize_to:
                 dev.resize(resize_to * units.Gi / units.Ki)
         finally:
             self._host.write_instance_config(xml)
@@ -1310,7 +1317,12 @@ class LibvirtDriver(driver.ComputeDriver):
             self._disconnect_volume(new_connection_info, disk_dev)
             raise NotImplementedError(_("Swap only supports host devices"))
 
-        self._swap_volume(guest, disk_dev, conf.source_path, resize_to)
+        try:
+            self._swap_volume(guest, disk_dev, conf.source_path, resize_to)
+        except exception.VolumeRebaseFailed:
+            with excutils.save_and_reraise_exception():
+                self._disconnect_volume(new_connection_info, disk_dev)
+
         self._disconnect_volume(old_connection_info, disk_dev)
 
     def _get_existing_domain_xml(self, instance, network_info,