curtin-dev team mailing list archive

[Olivier Gayot <mp+481223@xxxxxxxxxxxxxxxxxx>]

Olivier Gayot has proposed merging ~ogayot/curtin:raid+multipath into curtin:master.

Commit message:
do not squash

Requested reviews:
  curtin developers (curtin-dev)
Related bugs:
  Bug #2094979 in curtin: "curtin misidentify /dev/md126p3 as a multipath device, generates unbootable grub config"
  https://bugs.launchpad.net/curtin/+bug/2094979

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

block: exclude members of the same RAID when looking for multipath-ed devs

When an IMSM RAID device (level 1) is present, installing to it and then
running partprobe makes it look like the involved RAID members are "clones" of
the RAID device.

This was making our multipath detection code confused. Indeed, it was
implemented in a way that if two block devices share the same UUID, they are
considered multipath devices.

We now try to exclude different members of the same RAID from being considered
multipath devices.

-- 

Also, I added a note where I think we're missing a return statement.
-- 
Your team curtin developers is requested to review the proposed merge of ~ogayot/curtin:raid+multipath into curtin:master.

diff --git a/curtin/block/__init__.py b/curtin/block/__init__.py
index f0445ae..2c459bc 100644
--- a/curtin/block/__init__.py
+++ b/curtin/block/__init__.py
@@ -1,6 +1,6 @@
 # This file is part of curtin. See LICENSE file for copyright and license info.
 import re
-from contextlib import contextmanager
+from contextlib import contextmanager, suppress
 import errno
 import itertools
 import os
@@ -611,9 +611,44 @@ def _legacy_detect_multipath(target_mountpoint=None):
         # Iterating over available devices to see if any other device
         # has the same UUID as the target device. If such device exists
         # we probably installed the system to the multipath device.
-        for other_devpath, other_data in binfo.items():
-            if ((other_data.get('UUID') == target_uuid) and
-                    (other_devpath != devpath)):
+        other_bdevs = {k: w for k, w in binfo.items() if k != devpath}
+        # We need to be extra careful when dealing with RAID devices though.
+        # In many scenarios (especially with mirroring), RAID members will
+        # share attributes with the RAID device. This doesn't mean they are
+        # multipath-ed.
+        if udevadm_info(devpath).get("MD_CONTAINER") is not None:
+            # This is a RAID device, look out for members
+            md_devs = md_get_all_devices_list(devpath)
+            raid_members = [get_major_minor(dev) for dev in md_devs]
+
+            def is_in_the_raid(devpath) -> bool:
+                """Check if the dev referenced by devpath is a direct RAID
+                   member or a child of a RAID member."""
+                info = udevadm_info(devpath)
+
+                with suppress(KeyError):
+                    major, minor = int(info["MAJOR"]), int(info["MINOR"])
+
+                    if (major, minor) in raid_members:
+                        # This is a direct member of the RAID
+                        return True
+
+                with suppress(KeyError):
+                    major_s, minor_s = info["ID_PART_ENTRY_DISK"].split(":")
+
+                    if (int(major_s), int(minor_s)) in raid_members:
+                        # We are a partition and the disk is a member of the
+                        # RAID
+                        return True
+
+                return False
+
+            other_bdevs = {k: w
+                           for k, w in other_bdevs.items()
+                           if not is_in_the_raid(k)}
+
+        for other_devpath, other_data in other_bdevs.items():
+            if other_data.get('UUID') == target_uuid:
                 return True
     # No other devices have the same UUID as the target devices.
     # We probably installed the system to the non-multipath device.
@@ -639,7 +674,7 @@ def _device_is_multipathed(devpath):
 
     elif devpath.startswith('/dev/md'):
         if any((multipath.is_mpath_member(md) for md in
-                md_get_devices_list(devpath) + md_get_spares_list(devpath))):
+                md_get_all_devices_list(devpath))):
             return True
 
     result = multipath.is_mpath_member(devpath)
@@ -668,6 +703,10 @@ def md_get_devices_list(devpath):
     return _md_get_members_list(devpath, state_is_not_spare)
 
 
+def md_get_all_devices_list(devpath):
+    return md_get_devices_list(devpath) + md_get_spares_list(devpath)
+
+
 def detect_multipath(target_mountpoint=None):
     if multipath.multipath_supported():
         for device in (os.path.realpath(dev)
@@ -681,6 +720,9 @@ def detect_multipath(target_mountpoint=None):
                 continue
             if _device_is_multipathed(device):
                 return device
+        # NOTE: ogayot: I suspect we want to return None, or return False here
+        # rather than going through the legacy function (which is known to
+        # return false positives).
 
     return _legacy_detect_multipath(target_mountpoint)
 
@@ -905,6 +947,12 @@ def disk_to_bypath_path(kname):
     return mapping.get(dev_path(kname))
 
 
+def get_major_minor(devpath):
+    """Return a tuple(major, minor) for the specified device"""
+    info = udevadm_info(devpath)
+    return int(info["MAJOR"]), int(info["MINOR"])
+
+
 def get_device_mapper_links(devpath, first=False):
     """ Return the best devlink to device at devpath. """
     info = udevadm_info(devpath)
diff --git a/tests/unittests/test_block.py b/tests/unittests/test_block.py
index 2818fe4..a82131f 100644
--- a/tests/unittests/test_block.py
+++ b/tests/unittests/test_block.py
@@ -940,4 +940,96 @@ class TestResize(CiTestCase):
             self.assertEqual({'a', 'b'}, block.get_resize_fstypes())
 
 
+class TestGetMajorMinor(CiTestCase):
+    @mock.patch('curtin.block.udevadm_info')
+    def test_disk(self, m_info):
+        m_info.return_value = {'DEVTYPE': 'disk', 'MAJOR': '8', 'MINOR': '0'}
+        self.assertEqual((8, 0), block.get_major_minor("/dev/sda"))
+
+    @mock.patch('curtin.block.udevadm_info')
+    def test_partition(self, m_info):
+        m_info.return_value = {
+            'DEVTYPE': 'partition',
+            'MAJOR': '8',
+            'MINOR': '3'}
+        self.assertEqual((8, 3), block.get_major_minor("/dev/sda"))
+
+
+@mock.patch('curtin.block.blkid')
+@mock.patch('curtin.block.get_devices_for_mp')
+@mock.patch('curtin.block.udevadm_info')
+@mock.patch('curtin.block.rescan_block_devices', new=mock.Mock())
+class TestLegacyDetectMultipath(CiTestCase):
+    def test_simple_no_multipath(self, m_udevadm_info, m_devices_for_mp,
+                                 m_blkid):
+        m_blkid.return_value = {
+            '/dev/nvme0n1p3': {
+                'UUID': '7fbb4ad1-c9ed-4575-bdfd-54ac9622acc2',
+            }, '/dev/nvme0n1p1': {
+                'UUID': '50BA-7B2F',
+            }, '/dev/nvme0n1p2': {
+                'UUID': 'F1EA-BF0A',
+            },
+        }
+        m_devices_for_mp.return_value = "/dev/nvme0n1p1"
+        m_udevadm_info.return_value = {}
+
+        self.assertFalse(block._legacy_detect_multipath())
+
+    def test_isms_raid(self, m_udevadm_info, m_devices_for_mp, m_blkid):
+        '''In LP: #2094979, we have an IMSM RAID device (mirrored). After
+           installing to the RAID device and running partprobe, /dev/nvme0n1
+           and /dev/nvme1n1 appear to be clones of /dev/md126. This made the
+           legacy multipath detection code think that there are multipath
+           devices, since partiions of /dev/md126 share the same UUID as
+           partitions in /dev/nvme0n1 and /dev/nvme1n1. Ensure we don't detect
+           those as multipath anymore.'''
+        m_blkid.return_value = {
+            '/dev/nvme0n1p1': {
+                'UUID': '7fbb4ad1-c9ed-4575-bdfd-54ac9622acc2',
+            }, '/dev/nvme1n1p1': {
+                'UUID': '7fbb4ad1-c9ed-4575-bdfd-54ac9622acc2',
+            }, '/dev/md126p1': {
+                'UUID': '7fbb4ad1-c9ed-4575-bdfd-54ac9622acc2',
+            },
+        }
+        m_devices_for_mp.return_value = '/dev/md126p1'
+
+        def udevadm_info(devpath):
+            if devpath == '/dev/md126p1':
+                return {
+                    'DEVTYPE': 'partition',
+                    'MD_CONTAINER': '/dev/md/imsm0',
+                    'ID_PART_ENTRY_DISK': '8:3',
+                }
+            elif devpath == '/dev/nvme0n1p1':
+                return {
+                    'DEVTYPE': 'partition',
+                    'ID_PART_ENTRY_DISK': '8:1',
+                }
+            elif devpath == '/dev/nvme1n1p1':
+                return {
+                    'DEVTYPE': 'partition',
+                    'ID_PART_ENTRY_DISK': '8:2',
+                }
+            raise ValueError
+
+        def get_major_minor(devpath):
+            if devpath == '/dev/nvme0n1':
+                return 8, 1
+            elif devpath == '/dev/nvme1n1':
+                return 8, 2
+            raise ValueError
+
+        m_udevadm_info.side_effect = udevadm_info
+
+        p_get_major_minor = mock.patch('curtin.block.get_major_minor',
+                                       side_effect=get_major_minor)
+        p_md_get_all_devices = mock.patch(
+            'curtin.block.md_get_all_devices_list',
+            return_value=['/dev/nvme0n1', '/dev/nvme1n1'])
+
+        with p_get_major_minor, p_md_get_all_devices:
+            self.assertFalse(block._legacy_detect_multipath())
+
 # vi: ts=4 expandtab syntax=python