Merge "libvirt: set device address tag only if setting disk unit" int…

…o stable/rocky

nova/tests/unit/virt/libvirt/test_imagebackend.py

@@ -20,6 +20,7 @@
 import tempfile
 
 from castellan import key_manager
+import ddt
 import fixtures
 import mock
 from oslo_concurrency import lockutils
@@ -57,6 +58,7 @@ def secretLookupByUUIDString(self, uuid):
         return FakeSecret()
 
 
+@ddt.ddt
 class _ImageTestCase(object):
 
     def mock_create_image(self, image):
@@ -203,6 +205,24 @@ def test_get_disk_size(self, get_disk_size):
         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):
 
@@ -1268,6 +1288,7 @@ def test_get_model(self):
                          model)
 
 
+@ddt.ddt
 class RbdTestCase(_ImageTestCase, test.NoDBTestCase):
     FSID = "FakeFsID"
     POOL = "FakePool"
@@ -1492,6 +1513,17 @@ def get_mon_addrs():
 
         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"

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
@@ -140,6 +141,7 @@ def iscsi_connection(self, volume, location, iqn, auth=False,
         return ret
 
 
+@ddt.ddt
 class LibvirtVolumeTestCase(LibvirtISCSIVolumeBaseTestCase):
 
     def _assertDiskInfoEquals(self, tree, disk_info):
@@ -383,3 +385,21 @@ def test_libvirt_volume_driver_encryption_missing_secret(self):
         conf = libvirt_driver.get_config(connection_info, self.disk_info)
         tree = conf.format_dom()
         self.assertIsNone(tree.find("encryption"))
+
+    @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)

nova/virt/libvirt/imagebackend.py

@@ -181,11 +181,17 @@ def libvirt_info(self, disk_bus, disk_dev, device_type, cache_mode,
         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.

nova/virt/libvirt/volume/volume.py

@@ -94,16 +94,21 @@ def get_config(self, connection_info, disk_info):
         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']
 
         if connection_info.get('multiattach', False):
             # Note that driver_cache should be disabled (none) when using