curtin-dev team mailing list archive

[Michael Hudson-Doyle <mwhudsonlp@xxxxxxxxxxx>]

Michael Hudson-Doyle has proposed merging ~mwhudson/curtin:lp-1893818 into curtin:master.

Commit message:
storage_config: return one type: disk action per multipathed disk

Currently extract_storage_config returns one type: disk action
for every member of a multipathed disk and type: partition
actions for each partition of each disk. This works by generating
a type: disk action for each disk and ignoring the block device
data for /dev/dm-X device for the multipathed disk.

But in groovy+, the udev rule from multipath-tools that has
always attempted to remove the devices nodes for the partitions
of a disk that is a multipath member actually succeeds, and
trying to generate a type: partition action for a partition with
no underlying device node makes things blow up.

Instead, this branch generates type: disk and type: partitions
actions from the /dev/dm-X nodes for the mutipathed disk and its
partitions, and ignores and disks and partitions that are members
of a multipathed disk.

LP: #1893818

Requested reviews:
  curtin developers (curtin-dev)

For more details, see:
https://code.launchpad.net/~mwhudson/curtin/+git/curtin/+merge/392353
-- 
Your team curtin developers is requested to review the proposed merge of ~mwhudson/curtin:lp-1893818 into curtin:master.

diff --git a/curtin/storage_config.py b/curtin/storage_config.py
index 494b142..658ebb3 100644
--- a/curtin/storage_config.py
+++ b/curtin/storage_config.py
@@ -453,72 +453,17 @@ class ProbertParser(object):
 
         return None
 
-    def is_mpath(self, blockdev):
-        if blockdev.get('DM_MULTIPATH_DEVICE_PATH') == "1":
-            return True
-
-        return bool('mpath-' in blockdev.get('DM_UUID', ''))
-
-    def get_mpath_name(self, blockdev):
-        mpath_data = self.probe_data.get('multipath')
-        if not mpath_data:
-            return None
-
-        bd_name = blockdev['DEVNAME']
-        if blockdev['DEVTYPE'] == 'partition':
-            bd_name = self.partition_parent_devname(blockdev)
-        bd_name = os.path.basename(bd_name)
-        for path in mpath_data.get('paths', []):
-            if bd_name == path.get('device'):
-                rv = path.get('multipath')
-                return rv
-
-    def find_mpath_member(self, blockdev):
-        if blockdev.get('DM_MULTIPATH_DEVICE_PATH') == "1":
-            # find all other DM_MULTIPATH_DEVICE_PATH devs with same serial
-            serial = blockdev.get('ID_SERIAL')
-            members = sorted([os.path.basename(dev['DEVNAME'])
-                              for dev in self.blockdev_data.values()
-                              if dev.get("ID_SERIAL", "") == serial and
-                              dev['DEVTYPE'] == blockdev['DEVTYPE']])
-            # [/dev/sda, /dev/sdb]
-            # [/dev/sda1, /dev/sda2, /dev/sdb1, /dev/sdb2]
-
-        else:
-            dm_mpath = blockdev.get('DM_MPATH')
-            dm_uuid = blockdev.get('DM_UUID')
-            dm_part = blockdev.get('DM_PART')
-            dm_name = blockdev.get('DM_NAME')
-
-            if dm_mpath:
-                multipath = dm_mpath
-            elif dm_name:
-                multipath = dm_name
-            else:
-                # part1-mpath-30000000000000064
-                # mpath-30000000000000064
-                # mpath-36005076306ffd6b60000000000002406
-                match = re.search(r'mpath\-([a-zA-Z]*|\d*)+$', dm_uuid)
-                if not match:
-                    LOG.debug('Failed to extract multipath ID pattern from '
-                              'DM_UUID value: "%s"', dm_uuid)
-                    return None
-                # remove leading 'mpath-'
-                multipath = match.group(0)[6:]
-            mpath_data = self.probe_data.get('multipath')
-            if not mpath_data:
-                return None
-            members = sorted([path['device'] for path in mpath_data['paths']
-                              if path['multipath'] == multipath])
-
-            # append partition number if present
-            if dm_part:
-                members = [member + dm_part for member in members]
-
-        if len(members):
-            return members[0]
-
-        return None
+    def is_mpath_disk(self, blockdev):
+        return blockdev.get('DM_UUID', '').startswith('mpath-')
+
+    def is_mpath_partition(self, blockdev):
+        dm_uuid = blockdev.get('DM_UUID')
+        if dm_uuid is None:
+            return False
+        parts = dm_uuid.split('-', 2)
+        if len(parts) < 2:
+            return False
+        return parts[0].startswith('part') and parts[1] == 'mpath'
 
     def blockdev_to_id(self, blockdev):
         """ Examine a blockdev dictionary and return a tuple of curtin
@@ -539,15 +484,13 @@ class ProbertParser(object):
             if 'DM_LV_NAME' in blockdev:
                 devtype = 'lvm-partition'
                 name = blockdev['DM_LV_NAME']
-            elif self.is_mpath(blockdev):
-                # extract a multipath member device
-                member = self.find_mpath_member(blockdev)
-                if member:
-                    name = member
-                else:
-                    name = blockdev['DM_UUID']
-                if 'DM_PART' in blockdev:
-                    devtype = 'partition'
+            elif self.is_mpath_disk(blockdev):
+                devtype = 'mpath-disk'
+                name = blockdev['DM_NAME']
+            elif self.is_mpath_partition(blockdev):
+                devtype = 'mpath-partition'
+                name = '{}-part{}'.format(
+                    blockdev['DM_MPATH'], blockdev['DM_PART'])
             elif is_dmcrypt(blockdev):
                 devtype = 'dmcrypt'
                 name = blockdev['DM_NAME']
@@ -681,10 +624,14 @@ class BlockdevParser(ProbertParser):
         errors = []
 
         for devname, data in self.blockdev_data.items():
-            # skip composed devices here, except partitions
+            # skip composed devices here, except partitions and multipath
             if data.get('DEVPATH', '').startswith('/devices/virtual/block'):
-                if data.get('DEVTYPE', '') != "partition":
-                    continue
+                if not self.is_mpath_disk(data):
+                    if not self.is_mpath_partition(data):
+                        if data.get('DEVTYPE', '') != "partition":
+                            continue
+            if data.get('DM_MULTIPATH_DEVICE_PATH') == '1':
+                continue
             entry = self.asdict(data)
             if entry:
                 try:
@@ -710,10 +657,16 @@ class BlockdevParser(ProbertParser):
             blockdev attribute.
         """
         uniq = {}
-        source_keys = {
-            'wwn': ['ID_WWN_WITH_EXTENSION', 'ID_WWN'],
-            'serial': ['ID_SERIAL', 'ID_SERIAL_SHORT'],
-        }
+        if self.is_mpath_disk(blockdev):
+            source_keys = {
+                'wwn': ['DM_WWN'],
+                'serial': ['DM_SERIAL'],  # only present with focal+
+            }
+        else:
+            source_keys = {
+                'wwn': ['ID_WWN_WITH_EXTENSION', 'ID_WWN'],
+                'serial': ['ID_SERIAL', 'ID_SERIAL_SHORT'],
+            }
         for skey, id_keys in source_keys.items():
             for id_key in id_keys:
                 if id_key in blockdev and skey not in uniq:
@@ -740,6 +693,10 @@ class BlockdevParser(ProbertParser):
             storage config dictionary.  This method
             will return curtin storage types: disk, partition.
         """
+        dev_type = blockdev_data['DEVTYPE']
+        if self.is_mpath_partition(blockdev_data):
+            dev_type = 'partition'
+
         # just disks and partitions
         if blockdev_data['DEVTYPE'] not in ["disk", "partition"]:
             return None
@@ -751,20 +708,24 @@ class BlockdevParser(ProbertParser):
             return None
 
         devname = blockdev_data.get('DEVNAME')
+        path = devname
         entry = {
-            'type': blockdev_data['DEVTYPE'],
+            'type': dev_type,
             'id': self.blockdev_to_id(blockdev_data),
         }
-        if blockdev_data.get('DM_MULTIPATH_DEVICE_PATH') == "1":
-            mpath_name = self.get_mpath_name(blockdev_data)
-            if mpath_name:
-                entry['multipath'] = mpath_name
+        if self.is_mpath_disk(blockdev_data):
+            entry['multipath'] = blockdev_data['DM_NAME']
+            for link in blockdev_data['DEVLINKS'].split():
+                if link.startswith('/dev/mapper'):
+                    path = link
+        elif self.is_mpath_partition(blockdev_data):
+            entry['multipath'] = blockdev_data['DM_MPATH']
 
         # default disks to gpt
         if entry['type'] == 'disk':
             uniq_ids = self.get_unique_ids(blockdev_data)
             # always include path, block_meta will prefer wwn/serial over path
-            uniq_ids.update({'path': devname})
+            uniq_ids.update({'path': path})
             # set wwn, serial, and path
             entry.update(uniq_ids)
 
@@ -791,16 +752,28 @@ class BlockdevParser(ProbertParser):
 
         if entry['type'] == 'partition':
             attrs = blockdev_data['attrs']
-            entry['number'] = int(attrs['partition'])
-            parent_devname = self.partition_parent_devname(blockdev_data)
-            parent_blockdev = self.blockdev_data[parent_devname]
+            if self.is_mpath_partition(blockdev_data):
+                entry['number'] = int(blockdev_data['DM_PART'])
+                parent_mpath = blockdev_data['DM_MPATH']
+                for data in self.blockdev_data.values():
+                    if data.get('DM_NAME') == parent_mpath:
+                        parent_blockdev = data
+                        break
+                else:
+                    raise ValueError("cannot find parent")
+                self_name = '/dev/mapper/{}-part{}'.format(
+                    parent_mpath, entry['number'])
+            else:
+                entry['number'] = int(attrs['partition'])
+                self_name = blockdev_data['DEVNAME']
+                parent_devname = self.partition_parent_devname(blockdev_data)
+                parent_blockdev = self.blockdev_data[parent_devname]
             ptable = parent_blockdev.get('partitiontable')
             if ptable:
                 part = None
                 for pentry in ptable['partitions']:
                     node = pentry['node']
-                    node_p = node.replace(parent_devname, '')
-                    if node_p.replace('p', '') == attrs['partition']:
+                    if node == self_name:
                         part = pentry
                         break
 
@@ -868,6 +841,9 @@ class FilesystemParser(ProbertParser):
                 errors.append(err)
                 continue
 
+            if blockdev_data.get('DM_MULTIPATH_DEVICE_PATH') == "1":
+                continue
+
             # no floppy, no cdrom
             if blockdev_data['MAJOR'] in ["11", "2"]:
                 continue
diff --git a/tests/unittests/test_storage_config.py b/tests/unittests/test_storage_config.py
index a38f9cd..45809eb 100644
--- a/tests/unittests/test_storage_config.py
+++ b/tests/unittests/test_storage_config.py
@@ -467,149 +467,50 @@ class TestBlockdevParser(CiTestCase):
             self.assertDictEqual(expected_dict,
                                  self.bdevp.asdict(blockdev))
 
-    def test_blockdev_detects_multipath(self):
+    def test_blockdev_multipath_disk(self):
         self.probe_data = _get_data('probert_storage_multipath.json')
         self.bdevp = BlockdevParser(self.probe_data)
-        blockdev = self.bdevp.blockdev_data['/dev/sda2']
+        blockdev = self.bdevp.blockdev_data['/dev/dm-0']
         expected_dict = {
-            'flag': 'linux',
-            'id': 'partition-sda2',
-            'offset': 2097152,
+            'id': 'mpath-disk-mpatha',
             'multipath': 'mpatha',
-            'size': 10734272512,
-            'type': 'partition',
-            'device': 'disk-sda',
-            'number': 2}
-        self.assertDictEqual(expected_dict, self.bdevp.asdict(blockdev))
-
-    def test_blockdev_skips_multipath_entry_if_no_multipath_data(self):
-        self.probe_data = _get_data('probert_storage_multipath.json')
-        del self.probe_data['multipath']
-        self.bdevp = BlockdevParser(self.probe_data)
-        blockdev = self.bdevp.blockdev_data['/dev/sda2']
-        expected_dict = {
-            'flag': 'linux',
-            'id': 'partition-sda2',
-            'offset': 2097152,
-            'size': 10734272512,
-            'type': 'partition',
-            'device': 'disk-sda',
-            'number': 2}
+            'path': '/dev/mapper/mpatha',
+            'ptable': 'gpt',
+            'type': 'disk',
+            'wwn': '0x0000000000000064',
+            }
         self.assertDictEqual(expected_dict, self.bdevp.asdict(blockdev))
 
-    def test_blockdev_skips_multipath_entry_if_bad_multipath_data(self):
+    def test_blockdev_multipath_partition(self):
         self.probe_data = _get_data('probert_storage_multipath.json')
-        for path in self.probe_data['multipath']['paths']:
-            path['multipath'] = ''
         self.bdevp = BlockdevParser(self.probe_data)
-        blockdev = self.bdevp.blockdev_data['/dev/sda2']
+        blockdev = self.bdevp.blockdev_data['/dev/dm-2']
         expected_dict = {
+            'device': 'mpath-disk-mpatha',
             'flag': 'linux',
-            'id': 'partition-sda2',
+            'id': 'mpath-partition-mpatha-part2',
+            'multipath': 'mpatha',
+            'number': 2,
             'offset': 2097152,
             'size': 10734272512,
             'type': 'partition',
-            'device': 'disk-sda',
-            'number': 2}
+            }
         self.assertDictEqual(expected_dict, self.bdevp.asdict(blockdev))
 
-    def test_blockdev_skips_multipath_entry_if_no_mp_paths(self):
+    def test_blockdev_skips_underlying_disks_and_partitions(self):
         self.probe_data = _get_data('probert_storage_multipath.json')
-        del self.probe_data['multipath']['paths']
         self.bdevp = BlockdevParser(self.probe_data)
-        blockdev = self.bdevp.blockdev_data['/dev/sda2']
-        expected_dict = {
-            'flag': 'linux',
-            'id': 'partition-sda2',
-            'offset': 2097152,
-            'size': 10734272512,
-            'type': 'partition',
-            'device': 'disk-sda',
-            'number': 2}
-        self.assertDictEqual(expected_dict, self.bdevp.asdict(blockdev))
+        configs = self.bdevp.parse()[0]
+        config_paths = {c.get('path') for c in configs}
+        self.assertNotIn('/dev/sda', config_paths)
 
     def test_blockdev_finds_multipath_id_from_dm_uuid(self):
         self.probe_data = _get_data('probert_storage_zlp6.json')
         self.bdevp = BlockdevParser(self.probe_data)
         blockdev = self.bdevp.blockdev_data['/dev/dm-2']
         result = self.bdevp.blockdev_to_id(blockdev)
-        self.assertEqual('disk-sda', result)
-
-    def test_blockdev_find_mpath_members_checks_dm_name(self):
-        """ BlockdevParser find_mpath_members uses dm_name if present."""
-        dm14 = {
-            "DEVTYPE": "disk",
-            "DEVLINKS": "/dev/disk/by-id/dm-name-mpathb",
-            "DEVNAME": "/dev/dm-14",
-            "DEVTYPE": "disk",
-            "DM_NAME": "mpathb",
-            "DM_UUID": "mpath-360050768028211d8b000000000000062",
-            "DM_WWN": "0x60050768028211d8b000000000000062",
-            "MPATH_DEVICE_READY": "1",
-            "MPATH_SBIN_PATH": "/sbin",
-        }
-        multipath = {
-            "maps": [
-                {
-                    "multipath": "360050768028211d8b000000000000061",
-                    "sysfs": "dm-11",
-                    "paths": "4"
-                },
-                {
-                    "multipath": "360050768028211d8b000000000000062",
-                    "sysfs": "dm-14",
-                    "paths": "4"
-                },
-                {
-                    "multipath": "360050768028211d8b000000000000063",
-                    "sysfs": "dm-15",
-                    "paths": "4"
-                }],
-            "paths": [
-                {
-                    "device": "sdej",
-                    "serial": "0200a084762cXX00",
-                    "multipath": "mpatha",
-                    "host_wwnn": "0x20000024ff9127de",
-                    "target_wwnn": "0x5005076802065e38",
-                    "host_wwpn": "0x21000024ff9127de",
-                    "target_wwpn": "0x5005076802165e38",
-                    "host_adapter": "[undef]"
-                },
-                {
-                    "device": "sdel",
-                    "serial": "0200a084762cXX00",
-                    "multipath": "mpathb",
-                    "host_wwnn": "0x20000024ff9127de",
-                    "target_wwnn": "0x5005076802065e38",
-                    "host_wwpn": "0x21000024ff9127de",
-                    "target_wwpn": "0x5005076802165e38",
-                    "host_adapter": "[undef]"
-                },
-                {
-                    "device": "sdet",
-                    "serial": "0200a084762cXX00",
-                    "multipath": "mpatha",
-                    "host_wwnn": "0x20000024ff9127de",
-                    "target_wwnn": "0x5005076802065e37",
-                    "host_wwpn": "0x21000024ff9127de",
-                    "target_wwpn": "0x5005076802165e37",
-                    "host_adapter": "[undef]"
-                },
-                {
-                    "device": "sdev",
-                    "serial": "0200a084762cXX00",
-                    "multipath": "mpathb",
-                    "host_wwnn": "0x20000024ff9127de",
-                    "target_wwnn": "0x5005076802065e37",
-                    "host_wwpn": "0x21000024ff9127de",
-                    "target_wwpn": "0x5005076802165e37",
-                    "host_adapter": "[undef]"
-                }],
-        }
-        self.bdevp.blockdev_data['/dev/dm-14'] = dm14
-        self.probe_data['multipath'] = multipath
-        self.assertEqual('disk-sdel', self.bdevp.blockdev_to_id(dm14))
+        self.assertEqual(
+            'mpath-disk-36005076306ffd6b60000000000002406', result)
 
     def test_blockdev_detects_dasd_device_id_and_vtoc_ptable(self):
         self.probe_data = _get_data('probert_storage_dasd.json')
@@ -983,22 +884,6 @@ class TestExtractStorageConfig(CiTestCase):
                                  extracted)
 
     @skipUnlessJsonSchema()
-    def test_find_all_multipath(self):
-        """ verify probed multipath paths are included in config. """
-        self.probe_data = _get_data('probert_storage_multipath.json')
-        extracted = storage_config.extract_storage_config(self.probe_data)
-        config = extracted['storage']['config']
-        blockdev = self.probe_data['blockdev']
-
-        for mpmap in self.probe_data['multipath']['maps']:
-            nr_disks = int(mpmap['paths'])
-            mp_name = blockdev['/dev/%s' % mpmap['sysfs']]['DM_NAME']
-            matched_disks = [cfg for cfg in config
-                             if cfg['type'] == 'disk' and
-                             cfg.get('multipath', '') == mp_name]
-            self.assertEqual(nr_disks, len(matched_disks))
-
-    @skipUnlessJsonSchema()
     def test_find_raid_partition(self):
         """ verify probed raid partitions are found. """
         self.probe_data = _get_data('probert_storage_raid1_partitions.json')
@@ -1047,6 +932,19 @@ class TestExtractStorageConfig(CiTestCase):
         self.assertEqual(expected_dict, disks[0])
 
     @skipUnlessJsonSchema()
+    def test_blockdev_multipath(self):
+        self.probe_data = _get_data('probert_storage_zlp6.json')
+        extracted = storage_config.extract_storage_config(self.probe_data)
+        config = extracted['storage']['config']
+        disks = [cfg for cfg in config if cfg['type'] == 'disk']
+        expected_count = len([
+            1 for bd_name, bd_data in self.probe_data['blockdev'].items()
+            if bd_data.get('DM_UUID', '').startswith('mpath-')
+            or bd_name.startswith('/dev/dasd') and bd_data['DEVTYPE'] == 'disk'
+            ])
+        self.assertEqual(expected_count, len(disks))
+
+    @skipUnlessJsonSchema()
     def test_blockdev_skips_invalid_wwn(self):
         self.probe_data = _get_data('probert_storage_bogus_wwn.json')
         extracted = storage_config.extract_storage_config(self.probe_data)