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

 Conflicts:
     nova/tests/unit/virt/libvirt/volume/test_volume.py

NOTE(melwitt): Conflict is due to not having change
Ibfa64f18bbd2fb70db7791330ed1a64fe61c1355 in Pike.

Change-Id: Iefab05e84ccc0bf8f15bdbbf515a290d282dbc5d
(cherry picked from commit 48fd81648a)
(cherry picked from commit 7500c1910c)
(cherry picked from commit 8703282508)
(cherry picked from commit c8ad9f4e19)
tags/16.1.8^2
melanie witt 11 months ago
parent
commit
b50b70a182

+ 32
- 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
@@ -56,6 +57,7 @@ class FakeConn(object):
56 57
         return FakeSecret()
57 58
 
58 59
 
60
+@ddt.ddt
59 61
 class _ImageTestCase(object):
60 62
 
61 63
     def mock_create_image(self, image):
@@ -202,6 +204,24 @@ class _ImageTestCase(object):
202 204
         self.assertEqual(2361393152, image.get_disk_size(image.path))
203 205
         get_disk_size.assert_called_once_with(image.path)
204 206
 
207
+    def _test_libvirt_info_scsi_with_unit(self, disk_unit):
208
+        # The address should be set if bus is scsi and unit is set.
209
+        # Otherwise, it should not be set at all.
210
+        image = self.image_class(self.INSTANCE, self.NAME)
211
+        disk = image.libvirt_info(disk_bus='scsi', disk_dev='/dev/sda',
212
+                                  device_type='disk', cache_mode='none',
213
+                                  extra_specs={}, hypervisor_version=4004001,
214
+                                  disk_unit=disk_unit)
215
+        if disk_unit:
216
+            self.assertEqual(0, disk.device_addr.controller)
217
+            self.assertEqual(disk_unit, disk.device_addr.unit)
218
+        else:
219
+            self.assertIsNone(disk.device_addr)
220
+
221
+    @ddt.data(5, None)
222
+    def test_libvirt_info_scsi_with_unit(self, disk_unit):
223
+        self._test_libvirt_info_scsi_with_unit(disk_unit)
224
+
205 225
 
206 226
 class FlatTestCase(_ImageTestCase, test.NoDBTestCase):
207 227
 
@@ -1256,6 +1276,7 @@ class EncryptedLvmTestCase(_ImageTestCase, test.NoDBTestCase):
1256 1276
                          model)
1257 1277
 
1258 1278
 
1279
+@ddt.ddt
1259 1280
 class RbdTestCase(_ImageTestCase, test.NoDBTestCase):
1260 1281
     FSID = "FakeFsID"
1261 1282
     POOL = "FakePool"
@@ -1480,6 +1501,17 @@ class RbdTestCase(_ImageTestCase, test.NoDBTestCase):
1480 1501
 
1481 1502
         super(RbdTestCase, self).test_libvirt_info()
1482 1503
 
1504
+    @ddt.data(5, None)
1505
+    @mock.patch.object(rbd_utils.RBDDriver, "get_mon_addrs")
1506
+    def test_libvirt_info_scsi_with_unit(self, disk_unit, mock_mon_addrs):
1507
+        def get_mon_addrs():
1508
+            hosts = ["server1", "server2"]
1509
+            ports = ["1899", "1920"]
1510
+            return hosts, ports
1511
+        mock_mon_addrs.side_effect = get_mon_addrs
1512
+
1513
+        super(RbdTestCase, self)._test_libvirt_info_scsi_with_unit(disk_unit)
1514
+
1483 1515
     @mock.patch.object(rbd_utils.RBDDriver, "get_mon_addrs")
1484 1516
     def test_get_model(self, mock_mon_addrs):
1485 1517
         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
 
18 19
 from nova import exception
@@ -129,6 +130,7 @@ class LibvirtISCSIVolumeBaseTestCase(LibvirtVolumeBaseTestCase):
129 130
         return ret
130 131
 
131 132
 
133
+@ddt.ddt
132 134
 class LibvirtVolumeTestCase(LibvirtISCSIVolumeBaseTestCase):
133 135
 
134 136
     def _assertDiskInfoEquals(self, tree, disk_info):
@@ -303,3 +305,21 @@ class LibvirtVolumeTestCase(LibvirtISCSIVolumeBaseTestCase):
303 305
         conf = libvirt_driver.get_config(connection_info, self.disk_info)
304 306
         tree = conf.format_dom()
305 307
         self.assertIsNone(tree.find("driver[@discard]"))
308
+
309
+    @ddt.data(5, None)
310
+    def test_libvirt_volume_driver_address_tag_scsi_unit(self, disk_unit):
311
+        # The address tag should be set if bus is scsi and unit is set.
312
+        # Otherwise, it should not be set at all.
313
+        libvirt_driver = volume.LibvirtVolumeDriver(self.fake_host)
314
+        connection_info = {'data': {'device_path': '/foo'}}
315
+        disk_info = {'bus': 'scsi', 'dev': 'sda', 'type': 'disk'}
316
+        if disk_unit:
317
+            disk_info['unit'] = disk_unit
318
+        conf = libvirt_driver.get_config(connection_info, disk_info)
319
+        tree = conf.format_dom()
320
+        address = tree.find('address')
321
+        if disk_unit:
322
+            self.assertEqual('0', address.attrib['controller'])
323
+            self.assertEqual(str(disk_unit), address.attrib['unit'])
324
+        else:
325
+            self.assertIsNone(address)

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

@@ -152,11 +152,17 @@ class Image(object):
152 152
         return info
153 153
 
154 154
     def disk_scsi(self, info, disk_unit):
155
-        # The driver is responsible to create the SCSI controller
156
-        # at index 0.
157
-        info.device_addr = vconfig.LibvirtConfigGuestDeviceAddressDrive()
158
-        info.device_addr.controller = 0
155
+        # NOTE(melwitt): We set the device address unit number manually in the
156
+        # case of the virtio-scsi controller, in order to allow attachment of
157
+        # up to 256 devices. So, we should only be setting the address tag
158
+        # if we intend to set the unit number. Otherwise, we will let libvirt
159
+        # handle autogeneration of the address tag.
160
+        # See https://bugs.launchpad.net/nova/+bug/1792077 for details.
159 161
         if disk_unit is not None:
162
+            # The driver is responsible to create the SCSI controller
163
+            # at index 0.
164
+            info.device_addr = vconfig.LibvirtConfigGuestDeviceAddressDrive()
165
+            info.device_addr.controller = 0
160 166
             # In order to allow up to 256 disks handled by one
161 167
             # virtio-scsi controller, the device addr should be
162 168
             # specified.

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

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

Loading…
Cancel
Save