From 809065485c19fd535db6740bb21b867c41c008fe Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Thu, 20 Apr 2017 19:43:32 +0100 Subject: [PATCH] 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. Change-Id: I5997000a0ba6341c4987405cdc0760c3b471bd59 Closes-bug: #1685185 --- nova/exception.py | 4 ++ nova/tests/unit/virt/libvirt/test_driver.py | 64 +++++++++++++++++++++ nova/virt/libvirt/driver.py | 26 ++++++--- 3 files changed, 87 insertions(+), 7 deletions(-) diff --git a/nova/exception.py b/nova/exception.py index 297053072f78..4109a35d1a46 100644 --- a/nova/exception.py +++ b/nova/exception.py @@ -1738,6 +1738,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 bbd0ebf50b74..e094d28a049f 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -14955,6 +14955,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 7237fd60ff49..f17ef62f0ae8 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -1279,14 +1279,21 @@ 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) + 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) - while not dev.is_job_complete(): - time.sleep(0.5) + dev.abort_job(pivot=True) + + except Exception as exc: + LOG.exception("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)) - dev.abort_job(pivot=True) if resize_to: dev.resize(resize_to * units.Gi / units.Ki) finally: @@ -1319,7 +1326,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,