curtin-dev team mailing list archive

[Olivier Gayot <mp+490400@xxxxxxxxxxxxxxxxxx>]

Olivier Gayot has proposed merging ~ogayot/curtin:nvmeXcYnZ into curtin:master.

Commit message:
block_meta: skip blockdevs with no DEVNAME when searching for a specific dev

In the presence of nvme*c*n1 block devices, the
udev_all_block_device_properties function used to raise a KeyError "DEVNAME".

Apparently those block devices have no associated DEVNAME, so we should not
lean on the presence of such key.

LP: #2095211


Requested reviews:
  curtin developers (curtin-dev)
Related bugs:
  Bug #2095211 in curtin: "partitioning crashed with CurtinInstallError: KeyError DEVNAME"
  https://bugs.launchpad.net/curtin/+bug/2095211

For more details, see:
https://code.launchpad.net/~ogayot/curtin/+git/curtin/+merge/490400

In the linked bug reports, the installation fails because of a KeyError 'DEVNAME'.

This seems to be caused by the presence of a nvme0c0n1 (or nvme1c0n1) device in the udev info. Such device (which appears to exist when a NVMe controller has multiple paths to the NVM) comes with no DEVNAME:

P: /devices/pci0000:00/0000:00:05.1/0000:09:00.0/nvme/nvme0/nvme0c0n1
M: nvme0c0n1
R: 1
U: block
T: disk
Q: 10
E: DEVPATH=/devices/pci0000:00/0000:00:05.1/0000:09:00.0/nvme/nvme0/nvme0c0n1
E: SUBSYSTEM=block
E: DEVTYPE=disk
E: DISKSEQ=10

Unfortunately, I wasn't able to simulate this with QEMU but hopefully the unittests are good enough.
-- 
Your team curtin developers is requested to review the proposed merge of ~ogayot/curtin:nvmeXcYnZ into curtin:master.

diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
index 7c053e4..8dad729 100644
--- a/curtin/commands/block_meta.py
+++ b/curtin/commands/block_meta.py
@@ -496,6 +496,11 @@ def v2_get_path_to_disk(vol):
     for dev in udev_all_block_device_properties():
         if 'DM_PART' in dev or 'PARTN' in dev:
             continue
+        if 'DEVNAME' not in dev:
+            # Some block devices, typically nvme*c*n* devices (e.g., nvme0c0n1)
+            # have no DEVNAME. Skip them to avoid raising a KeyError
+            # (LP: #2095211).
+            continue
         for link in dev.get('DEVLINKS', '').split():
             link2dev[link] = dev
         link2dev[dev['DEVNAME']] = dev
diff --git a/tests/unittests/test_commands_block_meta.py b/tests/unittests/test_commands_block_meta.py
index a5ccac1..bcf16d8 100644
--- a/tests/unittests/test_commands_block_meta.py
+++ b/tests/unittests/test_commands_block_meta.py
@@ -54,6 +54,70 @@ class TestToUTF8HexNotation(CiTestCase):
                 '\\xeb\\xa6\\xac\\xeb\\x88\\x85\\xec\\x8a\\xa4')
 
 
+class TestGetV2GetPathToDisk(CiTestCase):
+    @patch('curtin.commands.block_meta.udev_all_block_device_properties')
+    def test_single_match(self, m_udev_all):
+        properties = [
+            {'DEVTYPE': 'disk',
+             'DEVNAME': '/dev/vda',
+             'DEVPATH': '/devices/pci0000:00/0000:00:04.0/virtio2/block/vda',
+             'DEVLINKS': '/dev/disk/by-path/virtio-pci-0000:00:00.0'
+                         ' /dev/disk/by-diskseq/9'
+                         ' /dev/disk/by-path/pci-0000:00:04.0',
+             'SUBSYSTEM': 'block',
+             'MAJOR': '253',
+             'MINOR': '0'}
+        ]
+        m_udev_all.return_value = properties
+
+        self.assertEqual('/dev/vda',
+                         block_meta.v2_get_path_to_disk({'path': '/dev/vda'}))
+
+    @patch('curtin.commands.block_meta.udev_all_block_device_properties')
+    def test_path_no_match(self, m_udev_all):
+        properties = [
+            {'DEVTYPE': 'disk',
+             'DEVNAME': '/dev/vda',
+             'DEVPATH': '/devices/pci0000:00/0000:00:04.0/virtio2/block/vda',
+             'DEVLINKS': '/dev/disk/by-path/virtio-pci-0000:00:00.0'
+                         ' /dev/disk/by-diskseq/9'
+                         ' /dev/disk/by-path/pci-0000:00:04.0',
+             'SUBSYSTEM': 'block',
+             'MAJOR': '253',
+             'MINOR': '0'}
+        ]
+        m_udev_all.return_value = properties
+
+        with self.assertRaises(KeyError):
+            block_meta.v2_get_path_to_disk({'path': '/dev/vdb'})
+
+    @patch('curtin.commands.block_meta.udev_all_block_device_properties')
+    def test_path_item_with_no_devname(self, m_udev_all):
+        # In LP: #2095211, the presence of a nvme0c0n1 (which has no DEVNAME)
+        # causes an issue.
+        devpath = '''\
+/devices/pci0000:00/0000:00:05.1/0000:09:00.0/nvme/nvme0/nvme0c0n1\
+'''
+        properties = [
+            {'DEVTYPE': 'disk',
+             'DEVPATH': devpath,
+             'SUBSYSTEM': 'block'},
+            {'DEVTYPE': 'disk',
+             'DEVNAME': '/dev/vda',
+             'DEVPATH': '/devices/pci0000:00/0000:00:04.0/virtio2/block/vda',
+             'DEVLINKS': '/dev/disk/by-path/virtio-pci-0000:00:00.0'
+                         ' /dev/disk/by-diskseq/9'
+                         ' /dev/disk/by-path/pci-0000:00:04.0',
+             'SUBSYSTEM': 'block',
+             'MAJOR': '253',
+             'MINOR': '0'},
+        ]
+        m_udev_all.return_value = properties
+
+        self.assertEqual('/dev/vda',
+                         block_meta.v2_get_path_to_disk({'path': '/dev/vda'}))
+
+
 class TestGetPathToStorageVolume(CiTestCase):
 
     def setUp(self):