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 <source dev=/dev/iscsi/lun>
to <source file=/dev/iscsi/lun>. 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 <source dev=/dev/iscsi/lun> 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)

diff --git a/nova/tests/unit/virt/libvirt/fakelibvirt.py b/nova/tests/unit/virt/libvirt/fakelibvirt.py
index 99a554416ca7ede45bee113d4fe5e6f98a5c2208..f6555fd52bb032fb4676550cb954416985ff5680 100644 (file)
--- 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 c4b4328ab1762ae17eddd0fee46e6519c6309056..b67f9aafda8c4ac3fdb0389114b1ef2bb7c56708 100644 (file)
--- 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 = "<domain/>"
+            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 780ad076dd32c335082d4b64e1efd05ea317327f..d2d341d063b438c0a113c42f9b5e3da1d57d3357 100644 (file)
--- 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 bf447133e4c271ef159166b5a06e86232f12e980..88fea1635e3f991ba410d0dde2ea728e03c97c1a 100644 (file)
--- 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 87a7d3573f630f31f5dc1652ce5ad4cfe733604b..4d2e47e05a592a8bf8b51e8382e0f88c2027aa56 100644 (file)
--- 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)