Merge "libvirt: set device address tag only if setting disk unit" into stable/pikepike-em16.1.8

4 files changed, 73 insertions, 10 deletions

diff --git a/nova/tests/unit/virt/libvirt/test_imagebackend.py b/nova/tests/unit/virt/libvirt/test_imagebackend.py
index a083a2cf30..6a92a999ee 100644
--- a/nova/tests/unit/virt/libvirt/test_imagebackend.py
+++ b/nova/tests/unit/virt/libvirt/test_imagebackend.py
@@ -20,6 +20,7 @@ import shutil
 import tempfile
 
 from castellan import key_manager
+import ddt
 import fixtures
 import mock
 from oslo_concurrency import lockutils
@@ -56,6 +57,7 @@ class FakeConn(object):
         return FakeSecret()
 
 
+@ddt.ddt
 class _ImageTestCase(object):
 
     def mock_create_image(self, image):
@@ -202,6 +204,24 @@ class _ImageTestCase(object):
         self.assertEqual(2361393152, image.get_disk_size(image.path))
         get_disk_size.assert_called_once_with(image.path)
 
+    def _test_libvirt_info_scsi_with_unit(self, disk_unit):
+        # The address should be set if bus is scsi and unit is set.
+        # Otherwise, it should not be set at all.
+        image = self.image_class(self.INSTANCE, self.NAME)
+        disk = image.libvirt_info(disk_bus='scsi', disk_dev='/dev/sda',
+                                  device_type='disk', cache_mode='none',
+                                  extra_specs={}, hypervisor_version=4004001,
+                                  disk_unit=disk_unit)
+        if disk_unit:
+            self.assertEqual(0, disk.device_addr.controller)
+            self.assertEqual(disk_unit, disk.device_addr.unit)
+        else:
+            self.assertIsNone(disk.device_addr)
+
+    @ddt.data(5, None)
+    def test_libvirt_info_scsi_with_unit(self, disk_unit):
+        self._test_libvirt_info_scsi_with_unit(disk_unit)
+
 
 class FlatTestCase(_ImageTestCase, test.NoDBTestCase):
 
@@ -1256,6 +1276,7 @@ class EncryptedLvmTestCase(_ImageTestCase, test.NoDBTestCase):
                          model)
 
 
+@ddt.ddt
 class RbdTestCase(_ImageTestCase, test.NoDBTestCase):
     FSID = "FakeFsID"
     POOL = "FakePool"
@@ -1480,6 +1501,17 @@ class RbdTestCase(_ImageTestCase, test.NoDBTestCase):
 
         super(RbdTestCase, self).test_libvirt_info()
 
+    @ddt.data(5, None)
+    @mock.patch.object(rbd_utils.RBDDriver, "get_mon_addrs")
+    def test_libvirt_info_scsi_with_unit(self, disk_unit, mock_mon_addrs):
+        def get_mon_addrs():
+            hosts = ["server1", "server2"]
+            ports = ["1899", "1920"]
+            return hosts, ports
+        mock_mon_addrs.side_effect = get_mon_addrs
+
+        super(RbdTestCase, self)._test_libvirt_info_scsi_with_unit(disk_unit)
+
     @mock.patch.object(rbd_utils.RBDDriver, "get_mon_addrs")
     def test_get_model(self, mock_mon_addrs):
         pool = "FakePool"
diff --git a/nova/tests/unit/virt/libvirt/volume/test_volume.py b/nova/tests/unit/virt/libvirt/volume/test_volume.py
index 4cd4115738..e2174a5b7b 100644
--- a/nova/tests/unit/virt/libvirt/volume/test_volume.py
+++ b/nova/tests/unit/virt/libvirt/volume/test_volume.py
@@ -13,6 +13,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import ddt
 import mock
 
 from nova import exception
@@ -129,6 +130,7 @@ class LibvirtISCSIVolumeBaseTestCase(LibvirtVolumeBaseTestCase):
         return ret
 
 
+@ddt.ddt
 class LibvirtVolumeTestCase(LibvirtISCSIVolumeBaseTestCase):
 
     def _assertDiskInfoEquals(self, tree, disk_info):
@@ -303,3 +305,21 @@ class LibvirtVolumeTestCase(LibvirtISCSIVolumeBaseTestCase):
         conf = libvirt_driver.get_config(connection_info, self.disk_info)
         tree = conf.format_dom()
         self.assertIsNone(tree.find("driver[@discard]"))
+
+    @ddt.data(5, None)
+    def test_libvirt_volume_driver_address_tag_scsi_unit(self, disk_unit):
+        # The address tag should be set if bus is scsi and unit is set.
+        # Otherwise, it should not be set at all.
+        libvirt_driver = volume.LibvirtVolumeDriver(self.fake_host)
+        connection_info = {'data': {'device_path': '/foo'}}
+        disk_info = {'bus': 'scsi', 'dev': 'sda', 'type': 'disk'}
+        if disk_unit:
+            disk_info['unit'] = disk_unit
+        conf = libvirt_driver.get_config(connection_info, disk_info)
+        tree = conf.format_dom()
+        address = tree.find('address')
+        if disk_unit:
+            self.assertEqual('0', address.attrib['controller'])
+            self.assertEqual(str(disk_unit), address.attrib['unit'])
+        else:
+            self.assertIsNone(address)
diff --git a/nova/virt/libvirt/imagebackend.py b/nova/virt/libvirt/imagebackend.py
index 4b75b00238..81063f241c 100644
--- a/nova/virt/libvirt/imagebackend.py
+++ b/nova/virt/libvirt/imagebackend.py
@@ -152,11 +152,17 @@ class Image(object):
         return info
 
     def disk_scsi(self, info, disk_unit):
-        # The driver is responsible to create the SCSI controller
-        # at index 0.
-        info.device_addr = vconfig.LibvirtConfigGuestDeviceAddressDrive()
-        info.device_addr.controller = 0
+        # NOTE(melwitt): We set the device address unit number manually in the
+        # case of the virtio-scsi controller, in order to allow attachment of
+        # up to 256 devices. So, we should only be setting the address tag
+        # if we intend to set the unit number. Otherwise, we will let libvirt
+        # handle autogeneration of the address tag.
+        # See https://bugs.launchpad.net/nova/+bug/1792077 for details.
         if disk_unit is not None:
+            # The driver is responsible to create the SCSI controller
+            # at index 0.
+            info.device_addr = vconfig.LibvirtConfigGuestDeviceAddressDrive()
+            info.device_addr.controller = 0
             # In order to allow up to 256 disks handled by one
             # virtio-scsi controller, the device addr should be
             # specified.
diff --git a/nova/virt/libvirt/volume/volume.py b/nova/virt/libvirt/volume/volume.py
index 327aeca050..004f2fec8b 100644
--- a/nova/virt/libvirt/volume/volume.py
+++ b/nova/virt/libvirt/volume/volume.py
@@ -93,16 +93,21 @@ class LibvirtBaseVolumeDriver(object):
         if data.get('discard', False) is True:
             conf.driver_discard = 'unmap'
 
-        if disk_info['bus'] == 'scsi':
+        # NOTE(melwitt): We set the device address unit number manually in the
+        # case of the virtio-scsi controller, in order to allow attachment of
+        # up to 256 devices. So, we should only be setting the address tag
+        # if we intend to set the unit number. Otherwise, we will let libvirt
+        # handle autogeneration of the address tag.
+        # See https://bugs.launchpad.net/nova/+bug/1792077 for details.
+        if disk_info['bus'] == 'scsi' and 'unit' in disk_info:
             # The driver is responsible to create the SCSI controller
             # at index 0.
             conf.device_addr = vconfig.LibvirtConfigGuestDeviceAddressDrive()
             conf.device_addr.controller = 0
-            if 'unit' in disk_info:
-                # In order to allow up to 256 disks handled by one
-                # virtio-scsi controller, the device addr should be
-                # specified.
-                conf.device_addr.unit = disk_info['unit']
+            # In order to allow up to 256 disks handled by one
+            # virtio-scsi controller, the device addr should be
+            # specified.
+            conf.device_addr.unit = disk_info['unit']
 
         return conf