From ef853e038d9a3e9bfe02287c7c01c80b7a022ed6 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 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. Change-Id: I868a0dae0baf8cded9c7c5807ea63ffc5eec0c5e Closes-bug: 1691195 (cherry picked from commit a8a4a8ea7b8e6c85273ddb02d34d6af1740b183f) --- nova/tests/unit/virt/libvirt/fakelibvirt.py | 1 + nova/tests/unit/virt/libvirt/test_driver.py | 52 ++++++++++++++++++--- nova/tests/unit/virt/libvirt/test_guest.py | 6 +++ nova/virt/libvirt/driver.py | 16 ++++--- nova/virt/libvirt/guest.py | 4 +- 5 files changed, 66 insertions(+), 13 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/fakelibvirt.py b/nova/tests/unit/virt/libvirt/fakelibvirt.py index 99a554416ca7..f6555fd52bb0 100644 --- a/nova/tests/unit/virt/libvirt/fakelibvirt.py +++ b/nova/tests/unit/virt/libvirt/fakelibvirt.py @@ -53,6 +53,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 c4b4328ab176..b67f9aafda8c 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -14745,7 +14745,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() @@ -14761,7 +14761,10 @@ 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 | @@ -14774,6 +14777,44 @@ class LibvirtConnTestCase(test.NoDBTestCase): 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.driver.LibvirtDriver._disconnect_volume') @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._swap_volume') @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_config') @@ -14807,8 +14848,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) @@ -14816,8 +14857,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 780ad076dd32..d2d341d063b4 100644 --- a/nova/tests/unit/virt/libvirt/test_guest.py +++ b/nova/tests/unit/virt/libvirt/test_guest.py @@ -659,6 +659,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 bf447133e4c2..88fea1635e3f 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -1242,7 +1242,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) @@ -1266,9 +1266,13 @@ 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) + copy_dev = conf.source_type == 'block' + dev.rebase(conf.source_path, copy=True, reuse_ext=True, + copy_dev=copy_dev) while not dev.is_job_complete(): time.sleep(0.5) @@ -1281,7 +1285,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)) @@ -1318,7 +1322,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 87a7d3573f63..4d2e47e05a59 100644 --- a/nova/virt/libvirt/guest.py +++ b/nova/virt/libvirt/guest.py @@ -726,7 +726,7 @@ 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): """Copy data from backing chain into a new disk This copies data from backing file(s) into overlay(s), giving @@ -739,10 +739,12 @@ class BlockDevice(object): pre-created :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)