From: Lee Yarwood Date: Thu, 20 Apr 2017 18:43:32 +0000 (+0100) Subject: libvirt: Always disconnect_volume after rebase failures X-Git-Url: http://xenbits.xensource.com/gitweb?a=commitdiff_plain;h=fbcf8d673342570a1518dbf8d88f289c2c39cd30;p=osstest%2Fopenstack-nova.git 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 c6ab31cffc..84cbcb6941 100644 --- 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 3be22f6eb6..b9a317e823 100644 --- 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 0e8fe0702c..4fc87b2250 100644 --- 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,