diff --git a/nova/tests/unit/virt/libvirt/test_imagebackend.py b/nova/tests/unit/virt/libvirt/test_imagebackend.py index 8cba305fe816..e7a86b7078d9 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 @@ -57,6 +58,7 @@ class FakeConn(object): return FakeSecret() +@ddt.ddt class _ImageTestCase(object): def mock_create_image(self, image): @@ -203,6 +205,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): @@ -1268,6 +1288,7 @@ class EncryptedLvmTestCase(_ImageTestCase, test.NoDBTestCase): model) +@ddt.ddt class RbdTestCase(_ImageTestCase, test.NoDBTestCase): FSID = "FakeFsID" POOL = "FakePool" @@ -1492,6 +1513,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 847557e1ea2c..58531aa9dc5d 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 @@ -140,6 +141,7 @@ class LibvirtISCSIVolumeBaseTestCase(LibvirtVolumeBaseTestCase): return ret +@ddt.ddt class LibvirtVolumeTestCase(LibvirtISCSIVolumeBaseTestCase): def _assertDiskInfoEquals(self, tree, disk_info): @@ -383,3 +385,21 @@ class LibvirtVolumeTestCase(LibvirtISCSIVolumeBaseTestCase): 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) diff --git a/nova/virt/libvirt/imagebackend.py b/nova/virt/libvirt/imagebackend.py index 7a966f77fef7..4ae97dda25cf 100644 --- a/nova/virt/libvirt/imagebackend.py +++ b/nova/virt/libvirt/imagebackend.py @@ -181,11 +181,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 aaffaa074c75..ee0994c48606 100644 --- a/nova/virt/libvirt/volume/volume.py +++ b/nova/virt/libvirt/volume/volume.py @@ -94,16 +94,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'] if connection_info.get('multiattach', False): # Note that driver_cache should be disabled (none) when using