Browse Source

libvirt: set device address tag only if setting disk unit

In Pike, we began setting disk unit values manually for the
'virtio-scsi' controller model in order to allow up to 256 devices [1].
We do this by setting the disk unit of the address tag manually for the
guest config. If we do not set the address tag manually, libvirt would
autogenerate it for us.

A problem occurs when a user has a SCSI disk that is a volume or isn't
using the 'virtio-scsi' controller model because we're not guarding our
manual setting of the address tag in the guest config by the disk unit,
in addition to the SCSI bus. This means that for a SCSI volume, we
generate an address tag like '<address type="drive" controller="0"/>'
for any SCSI volume, so a user with more than one device will get the
following error when they try to boot an instance:

  Failed to start libvirt guest: libvirtError: unsupported
    configuration: Found duplicate drive address for disk with target name
    'sda' controller='0' bus='0' target='0' unit='0'

This updates the conditionals to only manually set the address tag if
the bus is SCSI _and_ the disk unit has been specified. Otherwise, let
libvirt autogenerate the address tag and take care of avoiding
collisions.

[1] https://bugs.launchpad.net/nova/+bug/1686116

Closes-Bug: #1792077

Change-Id: Iefab05e84ccc0bf8f15bdbbf515a290d282dbc5d
(cherry picked from commit 48fd81648a)
tags/19.0.1
melanie witt 11 months ago
parent
commit
7500c1910c

+ 36
- 0
nova/tests/unit/virt/libvirt/test_imagebackend.py View File

@@ -20,6 +20,7 @@ import shutil
20 20
 import tempfile
21 21
 
22 22
 from castellan import key_manager
23
+import ddt
23 24
 import fixtures
24 25
 import mock
25 26
 from oslo_concurrency import lockutils
@@ -58,6 +59,7 @@ class FakeConn(object):
58 59
         return FakeSecret()
59 60
 
60 61
 
62
+@ddt.ddt
61 63
 class _ImageTestCase(object):
62 64
 
63 65
     def mock_create_image(self, image):
@@ -207,6 +209,28 @@ class _ImageTestCase(object):
207 209
         self.assertEqual(2361393152, image.get_disk_size(image.path))
208 210
         get_disk_size.assert_called_once_with(image.path)
209 211
 
212
+    def _test_libvirt_info_scsi_with_unit(self, disk_unit):
213
+        # The address should be set if bus is scsi and unit is set.
214
+        # Otherwise, it should not be set at all.
215
+        image = self.image_class(self.INSTANCE, self.NAME)
216
+        disk_info = {
217
+            'bus': 'scsi',
218
+            'dev': '/dev/sda',
219
+            'type': 'disk',
220
+        }
221
+        disk = image.libvirt_info(disk_info, cache_mode='none', extra_specs={},
222
+                                  hypervisor_version=4004001,
223
+                                  disk_unit=disk_unit)
224
+        if disk_unit:
225
+            self.assertEqual(0, disk.device_addr.controller)
226
+            self.assertEqual(disk_unit, disk.device_addr.unit)
227
+        else:
228
+            self.assertIsNone(disk.device_addr)
229
+
230
+    @ddt.data(5, None)
231
+    def test_libvirt_info_scsi_with_unit(self, disk_unit):
232
+        self._test_libvirt_info_scsi_with_unit(disk_unit)
233
+
210 234
 
211 235
 class FlatTestCase(_ImageTestCase, test.NoDBTestCase):
212 236
 
@@ -1279,6 +1303,7 @@ class EncryptedLvmTestCase(_ImageTestCase, test.NoDBTestCase):
1279 1303
                          model)
1280 1304
 
1281 1305
 
1306
+@ddt.ddt
1282 1307
 class RbdTestCase(_ImageTestCase, test.NoDBTestCase):
1283 1308
     FSID = "FakeFsID"
1284 1309
     POOL = "FakePool"
@@ -1503,6 +1528,17 @@ class RbdTestCase(_ImageTestCase, test.NoDBTestCase):
1503 1528
 
1504 1529
         super(RbdTestCase, self).test_libvirt_info()
1505 1530
 
1531
+    @ddt.data(5, None)
1532
+    @mock.patch.object(rbd_utils.RBDDriver, "get_mon_addrs")
1533
+    def test_libvirt_info_scsi_with_unit(self, disk_unit, mock_mon_addrs):
1534
+        def get_mon_addrs():
1535
+            hosts = ["server1", "server2"]
1536
+            ports = ["1899", "1920"]
1537
+            return hosts, ports
1538
+        mock_mon_addrs.side_effect = get_mon_addrs
1539
+
1540
+        super(RbdTestCase, self)._test_libvirt_info_scsi_with_unit(disk_unit)
1541
+
1506 1542
     @mock.patch.object(rbd_utils.RBDDriver, "get_mon_addrs")
1507 1543
     def test_get_model(self, mock_mon_addrs):
1508 1544
         pool = "FakePool"

+ 20
- 0
nova/tests/unit/virt/libvirt/volume/test_volume.py View File

@@ -13,6 +13,7 @@
13 13
 #    License for the specific language governing permissions and limitations
14 14
 #    under the License.
15 15
 
16
+import ddt
16 17
 import mock
17 18
 from oslo_utils.fixture import uuidsentinel as uuids
18 19
 
@@ -140,6 +141,7 @@ class LibvirtISCSIVolumeBaseTestCase(LibvirtVolumeBaseTestCase):
140 141
         return ret
141 142
 
142 143
 
144
+@ddt.ddt
143 145
 class LibvirtVolumeTestCase(LibvirtISCSIVolumeBaseTestCase):
144 146
 
145 147
     def _assertDiskInfoEquals(self, tree, disk_info):
@@ -383,3 +385,21 @@ class LibvirtVolumeTestCase(LibvirtISCSIVolumeBaseTestCase):
383 385
         conf = libvirt_driver.get_config(connection_info, self.disk_info)
384 386
         tree = conf.format_dom()
385 387
         self.assertIsNone(tree.find("encryption"))
388
+
389
+    @ddt.data(5, None)
390
+    def test_libvirt_volume_driver_address_tag_scsi_unit(self, disk_unit):
391
+        # The address tag should be set if bus is scsi and unit is set.
392
+        # Otherwise, it should not be set at all.
393
+        libvirt_driver = volume.LibvirtVolumeDriver(self.fake_host)
394
+        connection_info = {'data': {'device_path': '/foo'}}
395
+        disk_info = {'bus': 'scsi', 'dev': 'sda', 'type': 'disk'}
396
+        if disk_unit:
397
+            disk_info['unit'] = disk_unit
398
+        conf = libvirt_driver.get_config(connection_info, disk_info)
399
+        tree = conf.format_dom()
400
+        address = tree.find('address')
401
+        if disk_unit:
402
+            self.assertEqual('0', address.attrib['controller'])
403
+            self.assertEqual(str(disk_unit), address.attrib['unit'])
404
+        else:
405
+            self.assertIsNone(address)

+ 10
- 4
nova/virt/libvirt/imagebackend.py View File

@@ -180,11 +180,17 @@ class Image(object):
180 180
         return info
181 181
 
182 182
     def disk_scsi(self, info, disk_unit):
183
-        # The driver is responsible to create the SCSI controller
184
-        # at index 0.
185
-        info.device_addr = vconfig.LibvirtConfigGuestDeviceAddressDrive()
186
-        info.device_addr.controller = 0
183
+        # NOTE(melwitt): We set the device address unit number manually in the
184
+        # case of the virtio-scsi controller, in order to allow attachment of
185
+        # up to 256 devices. So, we should only be setting the address tag
186
+        # if we intend to set the unit number. Otherwise, we will let libvirt
187
+        # handle autogeneration of the address tag.
188
+        # See https://bugs.launchpad.net/nova/+bug/1792077 for details.
187 189
         if disk_unit is not None:
190
+            # The driver is responsible to create the SCSI controller
191
+            # at index 0.
192
+            info.device_addr = vconfig.LibvirtConfigGuestDeviceAddressDrive()
193
+            info.device_addr.controller = 0
188 194
             # In order to allow up to 256 disks handled by one
189 195
             # virtio-scsi controller, the device addr should be
190 196
             # specified.

+ 11
- 6
nova/virt/libvirt/volume/volume.py View File

@@ -94,16 +94,21 @@ class LibvirtBaseVolumeDriver(object):
94 94
         if data.get('discard', False) is True:
95 95
             conf.driver_discard = 'unmap'
96 96
 
97
-        if disk_info['bus'] == 'scsi':
97
+        # NOTE(melwitt): We set the device address unit number manually in the
98
+        # case of the virtio-scsi controller, in order to allow attachment of
99
+        # up to 256 devices. So, we should only be setting the address tag
100
+        # if we intend to set the unit number. Otherwise, we will let libvirt
101
+        # handle autogeneration of the address tag.
102
+        # See https://bugs.launchpad.net/nova/+bug/1792077 for details.
103
+        if disk_info['bus'] == 'scsi' and 'unit' in disk_info:
98 104
             # The driver is responsible to create the SCSI controller
99 105
             # at index 0.
100 106
             conf.device_addr = vconfig.LibvirtConfigGuestDeviceAddressDrive()
101 107
             conf.device_addr.controller = 0
102
-            if 'unit' in disk_info:
103
-                # In order to allow up to 256 disks handled by one
104
-                # virtio-scsi controller, the device addr should be
105
-                # specified.
106
-                conf.device_addr.unit = disk_info['unit']
108
+            # In order to allow up to 256 disks handled by one
109
+            # virtio-scsi controller, the device addr should be
110
+            # specified.
111
+            conf.device_addr.unit = disk_info['unit']
107 112
 
108 113
         if connection_info.get('multiattach', False):
109 114
             # Note that driver_cache should be disabled (none) when using

Loading…
Cancel
Save