curtin-dev team mailing list archive

[Olivier Gayot <mp+490256@xxxxxxxxxxxxxxxxxx>]

Olivier Gayot has proposed merging ~ogayot/curtin:partition_kname-dm-fix into curtin:master.

Commit message:
do not squash

Requested reviews:
  curtin developers (curtin-dev)
Related bugs:
  Bug #2119429 in subiquity: "Failure to install 25.10 on multipath  disk"
  https://bugs.launchpad.net/subiquity/+bug/2119429

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

block: fix logic to determine DM partition kname on Ubuntu 25.10+

The logic we had to determine a partition kname for a DM device is based
on adding -part{n} to the first devlink of the device.

Before Ubuntu 25.10, the first devlink was typically
/dev/disk/by-id/dm-name-{name}, and the logic worked because
/dev/disk/by-id/dm-name-{name}-part{n} is a thing when the device is
partitioned.

Unfortunately, since Ubuntu 25.10, the first devlink is
/dev/disk/by-diskseq/{seq} where seq is a number. The logic does not
work anymore since /dev/disk/by-diskseq/{seq}-part{n} is not a thing.

Fixed by looking specifically for the devlink which starts with
/dev/disk/by-id/dm-name-

LP: #2119429
-- 
Your team curtin developers is requested to review the proposed merge of ~ogayot/curtin:partition_kname-dm-fix into curtin:master.

diff --git a/curtin/block/__init__.py b/curtin/block/__init__.py
index 8b0f17c..314780b 100644
--- a/curtin/block/__init__.py
+++ b/curtin/block/__init__.py
@@ -129,10 +129,13 @@ def partition_kname(disk_kname, partition_number):
         # e.g. multipath disk is at dm-2, new partition could be dm-11, but
         # linux will create a -partX symlink against the disk by-id name.
         devpath = '/dev/' + disk_kname
-        disk_link = get_device_mapper_links(devpath, first=True)
-        return path_to_kname(
-                    os.path.realpath('%s-part%s' % (disk_link,
-                                                    partition_number)))
+        for disk_link in get_device_mapper_links(devpath):
+            if disk_link.startswith('/dev/disk/by-id/dm-name-'):
+                return path_to_kname(
+                        os.path.realpath('%s-part%s' % (disk_link,
+                                                        partition_number)))
+        raise OSError('could not find p{} devlink for {}'.format(
+            partition_number, devpath))
 
     # follow the same rules the kernel check_partition() does
     # https://github.com/torvalds/linux/blob/0473719/block/partitions/core.c#L330
@@ -852,7 +855,7 @@ def disk_to_bypath_path(kname):
     return mapping.get(dev_path(kname))
 
 
-def get_device_mapper_links(devpath, first=False):
+def get_device_mapper_links(devpath):
     """ Return the best devlink to device at devpath. """
     info = udevadm_info(devpath)
     if 'DEVLINKS' not in info:
@@ -861,9 +864,6 @@ def get_device_mapper_links(devpath, first=False):
     if not devlinks:
         raise ValueError('Unexpected DEVLINKS list contained empty values')
 
-    if first:
-        return devlinks[0]
-
     return devlinks
 
 
diff --git a/tests/unittests/test_block.py b/tests/unittests/test_block.py
index 9fcdb43..b704213 100644
--- a/tests/unittests/test_block.py
+++ b/tests/unittests/test_block.py
@@ -9,6 +9,8 @@ import textwrap
 
 from collections import OrderedDict
 
+from parameterized import parameterized
+
 from .helpers import CiTestCase, simple_mocked_open
 from curtin import util
 from curtin import block
@@ -135,15 +137,6 @@ class TestBlock(CiTestCase):
         self.assertEqual(device, path)
 
     @mock.patch('curtin.block.udevadm_info')
-    def test_get_device_mapper_links_returns_first_non_none(self, m_info):
-        """ get_device_mapper_links returns first by sort entry in DEVLINKS."""
-        devlinks = [self.random_string(), self.random_string()]
-        m_info.return_value = {'DEVLINKS': devlinks}
-        devpath = self.random_string()
-        self.assertEqual(sorted(devlinks)[0],
-                         block.get_device_mapper_links(devpath, first=True))
-
-    @mock.patch('curtin.block.udevadm_info')
     def test_get_device_mapper_links_raises_valueerror_no_links(self, m_info):
         """ get_device_mapper_links raises ValueError if info has no links."""
         m_info.return_value = {self.random_string(): self.random_string()}
@@ -444,24 +437,12 @@ class TestWipeVolume(CiTestCase):
 class TestBlockKnames(CiTestCase):
     """Tests for some of the kname functions in block"""
 
-    @mock.patch('curtin.block.os.path.realpath')
-    @mock.patch('curtin.block.get_device_mapper_links')
-    def test_determine_partition_kname(self, m_mlink, m_realp):
-        dm0_link = '/dev/disk/by-id/dm-name-XXXX2406'
-        m_mlink.return_value = dm0_link
-
-        # we need to convert the -part path to the real dm value
-        def _my_realp(pp):
-            if pp.startswith(dm0_link):
-                return 'dm-1'
-            return pp
-        m_realp.side_effect = _my_realp
+    def test_determine_partition_kname(self):
         part_knames = [(('sda', 1), 'sda1'),
                        (('vda', 1), 'vda1'),
                        (('nvme0n1', 1), 'nvme0n1p1'),
                        (('mmcblk0', 1), 'mmcblk0p1'),
                        (('cciss!c0d0', 1), 'cciss!c0d0p1'),
-                       (('dm-0', 1),  'dm-1'),
                        (('md0', 1), 'md0p1'),
                        (('mpath1', 2), 'mpath1p2'),
                        (('pmem0', 1), 'pmem0p1'),
@@ -472,6 +453,50 @@ class TestBlockKnames(CiTestCase):
             self.assertEqual(part_kname,
                              block.partition_kname(disk_kname, part_number))
 
+    @parameterized.expand([
+        ([
+            # Before 25.10, we didn't have the by-diskseq
+            '/dev/disk/by-id/dm-name-mpatha'
+            '/dev/disk/by-id/dm-uuid-mpath-0QEMU_QEMU_HARDDISK_MPIO0',
+            '/dev/disk/by-id/scsi-0QEMU_QEMU_HARDDISK_MPIO0',
+            '/dev/disk/by-id/wwn-0xQEMU_QEMU_HARDDISK_MPIO0',
+            '/dev/mapper/mpatha',
+        ], True),
+        ([
+            # But we do have it since 25.10, and it used caused trouble
+            # See LP: #2119429.
+            '/dev/disk/by-diskseq/27',
+            '/dev/disk/by-id/dm-name-mpatha'
+            '/dev/disk/by-id/dm-uuid-mpath-0QEMU_QEMU_HARDDISK_MPIO0',
+            '/dev/disk/by-id/scsi-0QEMU_QEMU_HARDDISK_MPIO0',
+            '/dev/disk/by-id/wwn-0xQEMU_QEMU_HARDDISK_MPIO0',
+            '/dev/mapper/mpatha',
+        ], True),
+        ([
+            '/dev/disk/by-diskseq/27',
+            '/dev/disk/by-id/dm-uuid-mpath-0QEMU_QEMU_HARDDISK_MPIO0',
+        ], False),
+    ])
+    def test_determine_partition_kname_dm(self, devlinks, expected_found):
+        # we need to convert the -part path to the real dm value
+        def _my_realp(pp):
+            if pp in ['%s-part1' % link for link in devlinks]:
+                return 'dm-1'
+            return pp
+
+        p_realp = mock.patch('curtin.block.os.path.realpath',
+                             side_effect=_my_realp)
+        p_get_links = mock.patch('curtin.block.get_device_mapper_links',
+                                 return_value=devlinks)
+
+        with p_realp, p_get_links:
+            if expected_found:
+                self.assertEqual('dm-1', block.partition_kname('dm-0', 1))
+            else:
+                with self.assertRaises(
+                        OSError, msg='could not find p1 devlink for dm-0'):
+                    block.partition_kname('dm-0', 1)
+
     @mock.patch('curtin.block.os.path.realpath')
     def test_path_to_kname(self, mock_os_realpath):
         mock_os_realpath.side_effect = lambda x: os.path.normpath(x)