From 8b9aa3e00101b1258f9b3dedca66aac81f655778 Mon Sep 17 00:00:00 2001 From: Artom Lifshitz Date: Wed, 17 May 2017 00:22:34 +0000 Subject: [PATCH] Use VIR_DOMAIN_BLOCK_REBASE_COPY_DEV when rebasing Previously, in swap_volume, the VIR_DOMAIN_BLOCK_REBASE_COPY_DEV flag was not passed to virDomainBlockRebase. In the case of iSCSI-backed disks, this caused the XML to change from to . This was a problem because /dev/iscsi/lun is not a regular file. This patch passes the VIR_DOMAIN_BLOCK_REBASE_COPY_DEV flag to virDomainBlockRebase, causing the correct to be generated upon volume-update. Conflicts: nova/tests/unit/virt/libvirt/test_driver.py nova/virt/libvirt/driver.py nova/virt/libvirt/guest.py NOTE(mriedem): The conflicts are due to fbcf8d673342570a1518dbf8d88f289c2c39cd30 needing to translate the exception message in driver.py and for passing instance to disconnect_volume in test_driver, which was added in Pike with b66b7d4f9d63e7f45ebfc033696d06c632a33ff1. NOTE(artom): In stable/newton, the conflict in guest.py is due to a different docstring for the rebase() method. NOTE(artom): This backport squashes 5d5c5a5d92458d530115b3d3b8b381524b1a3a90 to guard againt older libvirt versions that don't have the VIR_DOMAIN_BLOCK_REBASE_COPY_DEV flag. Change-Id: I868a0dae0baf8cded9c7c5807ea63ffc5eec0c5e Closes-bug: 1691195 (cherry picked from commit a8a4a8ea7b8e6c85273ddb02d34d6af1740b183f) (cherry picked from commit ef853e038d9a3e9bfe02287c7c01c80b7a022ed6) --- nova/tests/unit/virt/libvirt/fakelibvirt.py | 1 + nova/tests/unit/virt/libvirt/test_driver.py | 97 +++++++++++++++++++-- nova/tests/unit/virt/libvirt/test_guest.py | 6 ++ nova/virt/libvirt/driver.py | 29 ++++-- nova/virt/libvirt/guest.py | 4 +- 5 files changed, 124 insertions(+), 13 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/fakelibvirt.py b/nova/tests/unit/virt/libvirt/fakelibvirt.py index 75cd4ef9abb7..33f1ba7a2a96 100644 --- a/nova/tests/unit/virt/libvirt/fakelibvirt.py +++ b/nova/tests/unit/virt/libvirt/fakelibvirt.py @@ -52,6 +52,7 @@ VIR_DOMAIN_XML_MIGRATABLE = 8 VIR_DOMAIN_BLOCK_REBASE_SHALLOW = 1 VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT = 2 VIR_DOMAIN_BLOCK_REBASE_COPY = 8 +VIR_DOMAIN_BLOCK_REBASE_COPY_DEV = 32 VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC = 1 VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT = 2 diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index ba15f5a526af..0cc4b56969fe 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -14653,7 +14653,7 @@ class LibvirtConnTestCase(test.NoDBTestCase): save.assert_called_once_with() @mock.patch('nova.virt.libvirt.guest.BlockDevice.is_job_complete') - def test_swap_volume(self, mock_is_job_complete): + def test_swap_volume_file(self, mock_is_job_complete): drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI()) mock_dom = mock.MagicMock() @@ -14669,7 +14669,93 @@ class LibvirtConnTestCase(test.NoDBTestCase): mock_dom.isPersistent.return_value = True mock_is_job_complete.return_value = True - drvr._swap_volume(guest, srcfile, dstfile, 1) + drvr._swap_volume(guest, srcfile, + mock.MagicMock(source_type='file', + source_path=dstfile), + 1) + + mock_dom.XMLDesc.assert_called_once_with( + flags=(fakelibvirt.VIR_DOMAIN_XML_INACTIVE | + fakelibvirt.VIR_DOMAIN_XML_SECURE)) + mock_dom.blockRebase.assert_called_once_with( + srcfile, dstfile, 0, flags=( + fakelibvirt.VIR_DOMAIN_BLOCK_REBASE_COPY | + fakelibvirt.VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT)) + mock_dom.blockResize.assert_called_once_with( + srcfile, 1 * units.Gi / units.Ki) + mock_define.assert_called_once_with(xmldoc) + + @mock.patch('nova.virt.libvirt.guest.BlockDevice.is_job_complete') + def test_swap_volume_block(self, mock_is_job_complete): + """If the swapped volume is type="block", make sure that we give + libvirt the correct VIR_DOMAIN_BLOCK_REBASE_COPY_DEV flag to ensure the + correct type="block" XML is generated (bug 1691195) + """ + drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI()) + + mock_dom = mock.MagicMock() + guest = libvirt_guest.Guest(mock_dom) + + with mock.patch.object(drvr._conn, 'defineXML', + create=True) as mock_define: + xmldoc = "" + srcfile = "/first/path" + dstfile = "/second/path" + + mock_dom.XMLDesc.return_value = xmldoc + mock_dom.isPersistent.return_value = True + mock_is_job_complete.return_value = True + + drvr._swap_volume(guest, srcfile, + mock.MagicMock(source_type='block', + source_path=dstfile), + 1) + + mock_dom.XMLDesc.assert_called_once_with( + flags=(fakelibvirt.VIR_DOMAIN_XML_INACTIVE | + fakelibvirt.VIR_DOMAIN_XML_SECURE)) + mock_dom.blockRebase.assert_called_once_with( + srcfile, dstfile, 0, flags=( + fakelibvirt.VIR_DOMAIN_BLOCK_REBASE_COPY | + fakelibvirt.VIR_DOMAIN_BLOCK_REBASE_COPY_DEV | + fakelibvirt.VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT)) + mock_dom.blockResize.assert_called_once_with( + srcfile, 1 * units.Gi / units.Ki) + mock_define.assert_called_once_with(xmldoc) + + @mock.patch('nova.virt.libvirt.guest.BlockDevice.is_job_complete') + def test_swap_volume_block_old_libvirt(self, mock_is_job_complete): + """If the swapped volume is type="block", make sure that we don't give + libvirt the VIR_DOMAIN_BLOCK_REBASE_COPY_DEV flag if the version + of libvirt is so old that it doesn't have that flag (bug 1724039) + """ + drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI()) + + mock_dom = mock.MagicMock() + guest = libvirt_guest.Guest(mock_dom) + + with mock.patch.object(drvr._conn, 'defineXML', + create=True) as mock_define: + xmldoc = "" + srcfile = "/first/path" + dstfile = "/second/path" + + mock_dom.XMLDesc.return_value = xmldoc + mock_dom.isPersistent.return_value = True + mock_is_job_complete.return_value = True + + # Temporarily remove the global. + copy_dev = libvirt_driver.libvirt.VIR_DOMAIN_BLOCK_REBASE_COPY_DEV + delattr(libvirt_driver.libvirt, 'VIR_DOMAIN_BLOCK_REBASE_COPY_DEV') + try: + drvr._swap_volume(guest, srcfile, + mock.MagicMock(source_type='block', + source_path=dstfile), + 1) + finally: + # Reset the global. + libvirt_driver.libvirt.VIR_DOMAIN_BLOCK_REBASE_COPY_DEV = \ + copy_dev mock_dom.XMLDesc.assert_called_once_with( flags=(fakelibvirt.VIR_DOMAIN_XML_INACTIVE | @@ -14715,8 +14801,8 @@ class LibvirtConnTestCase(test.NoDBTestCase): mock_dom.UUIDString.return_value = 'uuid' get_guest.return_value = guest disk_info = {'bus': 'virtio', 'type': 'disk', 'dev': 'vdb'} - get_volume_config.return_value = mock.MagicMock( - source_path='/fake-new-volume') + conf = mock.MagicMock(source_path='/fake-new-volume') + get_volume_config.return_value = conf conn.swap_volume(old_connection_info, new_connection_info, instance, '/dev/vdb', 1) @@ -14724,8 +14810,7 @@ class LibvirtConnTestCase(test.NoDBTestCase): get_guest.assert_called_once_with(instance) connect_volume.assert_called_once_with(new_connection_info, disk_info) - swap_volume.assert_called_once_with(guest, 'vdb', - '/fake-new-volume', 1) + swap_volume.assert_called_once_with(guest, 'vdb', conf, 1) disconnect_volume.assert_called_once_with(old_connection_info, 'vdb') def test_swap_volume_driver_source_is_volume(self): diff --git a/nova/tests/unit/virt/libvirt/test_guest.py b/nova/tests/unit/virt/libvirt/test_guest.py index 1b9ef0259ba6..a6d05d63cac1 100644 --- a/nova/tests/unit/virt/libvirt/test_guest.py +++ b/nova/tests/unit/virt/libvirt/test_guest.py @@ -734,6 +734,12 @@ class GuestBlockTestCase(test.NoDBTestCase): 'vda', "foo", 0, flags=fakelibvirt.VIR_DOMAIN_BLOCK_REBASE_RELATIVE) + def test_rebase_copy_dev(self): + self.gblock.rebase("foo", copy_dev=True) + self.domain.blockRebase.assert_called_once_with( + 'vda', "foo", 0, + flags=fakelibvirt.VIR_DOMAIN_BLOCK_REBASE_COPY_DEV) + def test_commit(self): self.gblock.commit("foo", "top") self.domain.blockCommit.assert_called_once_with( diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index ba01199858d1..6ed4fd05845c 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -1185,7 +1185,7 @@ class LibvirtDriver(driver.ComputeDriver): with excutils.save_and_reraise_exception(): self._disconnect_volume(connection_info, disk_dev) - def _swap_volume(self, guest, disk_path, new_path, resize_to): + def _swap_volume(self, guest, disk_path, conf, resize_to): """Swap existing disk with a new block device.""" dev = guest.get_block_device(disk_path) @@ -1209,9 +1209,26 @@ class LibvirtDriver(driver.ComputeDriver): guest.delete_configuration(support_uefi) 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) + # Start copy with VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT flag to + # allow writing to existing external volume file. Use + # VIR_DOMAIN_BLOCK_REBASE_COPY_DEV if it's a block device to + # make sure XML is generated correctly (bug 1691195) + # NOTE(mriedem): VIR_DOMAIN_BLOCK_REBASE_COPY_DEV was + # introduced in libvirt 1.2.9 and since we support >= 1.2.1 + # we have to guard against the flag not being available. + if hasattr(libvirt, 'VIR_DOMAIN_BLOCK_REBASE_COPY_DEV'): + copy_dev = conf.source_type == 'block' + else: + copy_dev = False + # Leave a breadcrumb. + if conf.source_type == 'block': + LOG.warning(_LW( + 'Rebasing a block device using libvirt < 1.2.9 ' + 'where the VIR_DOMAIN_BLOCK_REBASE_COPY_DEV ' + 'flag is not available. This may lead to failures ' + 'later if the volume is swapped again.')) + dev.rebase(conf.source_path, copy=True, reuse_ext=True, + copy_dev=copy_dev) while not dev.is_job_complete(): time.sleep(0.5) @@ -1224,7 +1241,7 @@ class LibvirtDriver(driver.ComputeDriver): except Exception as exc: LOG.exception(_LE("Failure rebasing volume %(new_path)s on " - "%(new_path)s."), {'new_path': new_path, + "%(new_path)s."), {'new_path': conf.source_path, 'old_path': disk_path}) raise exception.VolumeRebaseFailed(reason=six.text_type(exc)) @@ -1261,7 +1278,7 @@ class LibvirtDriver(driver.ComputeDriver): raise NotImplementedError(_("Swap only supports host devices")) try: - self._swap_volume(guest, disk_dev, conf.source_path, resize_to) + self._swap_volume(guest, disk_dev, conf, resize_to) except exception.VolumeRebaseFailed: with excutils.save_and_reraise_exception(): self._disconnect_volume(new_connection_info, disk_dev) diff --git a/nova/virt/libvirt/guest.py b/nova/virt/libvirt/guest.py index 9b27d6430e84..3e5d5db98437 100644 --- a/nova/virt/libvirt/guest.py +++ b/nova/virt/libvirt/guest.py @@ -735,17 +735,19 @@ class BlockDevice(object): end=status['end']) def rebase(self, base, shallow=False, reuse_ext=False, - copy=False, relative=False): + copy=False, relative=False, copy_dev=False): """Rebases block to new base :param shallow: Limit copy to top of source backing chain :param reuse_ext: Reuse existing external file of a copy :param copy: Start a copy job :param relative: Keep backing chain referenced using relative names + :param copy_dev: Treat the destination as type="block" """ flags = shallow and libvirt.VIR_DOMAIN_BLOCK_REBASE_SHALLOW or 0 flags |= reuse_ext and libvirt.VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT or 0 flags |= copy and libvirt.VIR_DOMAIN_BLOCK_REBASE_COPY or 0 + flags |= copy_dev and libvirt.VIR_DOMAIN_BLOCK_REBASE_COPY_DEV or 0 flags |= relative and libvirt.VIR_DOMAIN_BLOCK_REBASE_RELATIVE or 0 return self._guest._domain.blockRebase( self._disk, base, self.REBASE_DEFAULT_BANDWIDTH, flags=flags)