curtin-dev team mailing list archive

[Michael Hudson-Doyle <mp+415700@xxxxxxxxxxxxxxxxxx>]

Michael Hudson-Doyle has proposed merging ~mwhudson/curtin:v2-disk-identification into curtin:master.

Commit message:
block_meta: all fields on a disk action must match with v2 config

This is for https://bugs.launchpad.net/subiquity/+bug/1929213 and
similar reports.

Requested reviews:
  curtin developers (curtin-dev)

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

diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
index f3f19dc..42325c1 100644
--- a/curtin/commands/block_meta.py
+++ b/curtin/commands/block_meta.py
@@ -13,8 +13,13 @@ from curtin.storage_config import (extract_storage_ordered_dict,
 
 
 from . import populate_one_subcmd
-from curtin.udev import (compose_udev_equality, udevadm_settle,
-                         udevadm_trigger, udevadm_info)
+from curtin.udev import (
+    compose_udev_equality,
+    udev_all_block_device_properties,
+    udevadm_info,
+    udevadm_settle,
+    udevadm_trigger,
+    )
 
 import glob
 import os
@@ -24,6 +29,7 @@ import sys
 import tempfile
 import time
 
+
 FstabData = namedtuple(
     "FstabData", ('spec', 'path', 'fstype', 'options', 'freq', 'passno',
                   'device'))
@@ -428,6 +434,112 @@ def get_poolname(info, storage_config):
     return poolname
 
 
+def v1_get_path_to_disk(vol):
+    volume_path = None
+    for disk_key in ['wwn', 'serial', 'device_id', 'path']:
+        vol_value = vol.get(disk_key)
+        try:
+            if not vol_value:
+                continue
+            if disk_key in ['wwn', 'serial']:
+                volume_path = block.lookup_disk(vol_value)
+            elif disk_key == 'path':
+                if vol_value.startswith('iscsi:'):
+                    i = iscsi.ensure_disk_connected(vol_value)
+                    volume_path = os.path.realpath(i.devdisk_path)
+                else:
+                    # resolve any symlinks to the dev_kname so
+                    # sys/class/block access is valid.  ie, there are no
+                    # udev generated values in sysfs
+                    volume_path = os.path.realpath(vol_value)
+                # convert /dev/sdX to /dev/mapper/mpathX value
+                if multipath.is_mpath_member(volume_path):
+                    volume_path = '/dev/mapper/' + (
+                        multipath.get_mpath_id_from_device(volume_path))
+            elif disk_key == 'device_id':
+                dasd_device = dasd.DasdDevice(vol_value)
+                volume_path = dasd_device.devname
+        except ValueError:
+            continue
+        # verify path exists otherwise try the next key
+        if os.path.exists(volume_path):
+            break
+        else:
+            volume_path = None
+
+    if volume_path is None:
+        raise ValueError("Failed to find storage volume id='%s' config: %s"
+                         % (vol['id'], vol))
+
+    return volume_path
+
+
+def v2_get_path_to_disk(vol):
+    all_disks = [
+        dev for dev in udev_all_block_device_properties()
+        if 'DM_PART' not in dev and 'PARTN' not in dev
+    ]
+
+    def disks_matching(key, value):
+        return [
+            dev['DEVNAME']
+            for dev in all_disks
+            if key in dev and dev[key] == value
+            ]
+
+    def disk_by_path(path):
+        for dev in all_disks:
+            if dev['DEVNAME'] == path or path in dev['DEVLINKS'].split():
+                return dev['DEVNAME']
+
+    cands = []
+    if 'wwn' in vol:
+        for key in 'DM_WWN', 'ID_WWN':
+            devs = disks_matching(key, vol['wwn'])
+            if devs:
+                break
+        cands.append(set(devs))
+    if 'serial' in vol:
+        for key in 'DM_SERIAL', 'ID_SERIAL':
+            devs = disks_matching(key, vol['serial'])
+            if devs:
+                break
+        cands.append(set(devs))
+    if 'device_id' in vol:
+        dasd_device = dasd.DasdDevice(vol['device_id'])
+        cands.append(set([dasd_device.devname]))
+        volume_path = dasd_device.devname
+        # verify path exists otherwise try the next key
+        if os.path.exists(volume_path):
+            cands.append(set([volume_path]))
+    if 'path' in vol:
+        path = vol['path']
+        if path.startswith('iscsi:'):
+            i = iscsi.ensure_disk_connected(path)
+            path = i.devdisk_path
+        if multipath.is_mpath_member(path):
+            # if path points to a multipath member, convert that to point
+            # at the multipath device
+            path = '/dev/mapper/' + multipath.get_mpath_id_from_device(path)
+        path = disk_by_path(path)
+        if path is not None:
+            cands.append(set([path]))
+        else:
+            cands.append(set())
+
+    cands = set.intersection(*cands)
+
+    if len(cands) == 0:
+        raise ValueError("Failed to find storage volume id='%s' config: %s"
+                         % (vol['id'], vol))
+    if len(cands) > 1:
+        raise ValueError(
+            "Storage volume id='%s' config: %s identified multiple devices"
+            % (vol['id'], vol))
+
+    return next(iter(cands))
+
+
 def get_path_to_storage_volume(volume, storage_config):
     # Get path to block device for volume. Volume param should refer to id of
     # volume in storage config
@@ -454,41 +566,10 @@ def get_path_to_storage_volume(volume, storage_config):
     elif vol.get('type') == "disk":
         # Get path to block device for disk. Device_id param should refer
         # to id of device in storage config
-        volume_path = None
-        for disk_key in ['wwn', 'serial', 'device_id', 'path']:
-            vol_value = vol.get(disk_key)
-            try:
-                if not vol_value:
-                    continue
-                if disk_key in ['wwn', 'serial']:
-                    volume_path = block.lookup_disk(vol_value)
-                elif disk_key == 'path':
-                    if vol_value.startswith('iscsi:'):
-                        i = iscsi.ensure_disk_connected(vol_value)
-                        volume_path = os.path.realpath(i.devdisk_path)
-                    else:
-                        # resolve any symlinks to the dev_kname so
-                        # sys/class/block access is valid.  ie, there are no
-                        # udev generated values in sysfs
-                        volume_path = os.path.realpath(vol_value)
-                    # convert /dev/sdX to /dev/mapper/mpathX value
-                    if multipath.is_mpath_member(volume_path):
-                        volume_path = '/dev/mapper/' + (
-                            multipath.get_mpath_id_from_device(volume_path))
-                elif disk_key == 'device_id':
-                    dasd_device = dasd.DasdDevice(vol_value)
-                    volume_path = dasd_device.devname
-            except ValueError:
-                continue
-            # verify path exists otherwise try the next key
-            if os.path.exists(volume_path):
-                break
-            else:
-                volume_path = None
-
-        if volume_path is None:
-            raise ValueError("Failed to find storage volume id='%s' config: %s"
-                             % (vol['id'], vol))
+        if getattr(storage_config, 'version', 1) > 1:
+            volume_path = v2_get_path_to_disk(vol)
+        else:
+            volume_path = v1_get_path_to_disk(vol)
 
     elif vol.get('type') == "lvm_partition":
         # For lvm partitions, a directory in /dev/ should be present with the
@@ -2017,8 +2098,8 @@ def meta_custom(args):
 
     storage_config_dict = extract_storage_ordered_dict(cfg)
 
-    version = cfg['storage']['version']
-    if version > 1:
+    storage_config_dict.version = cfg['storage']['version']
+    if storage_config_dict.version > 1:
         from curtin.commands.block_meta_v2 import (
             disk_handler_v2,
             partition_handler_v2,
diff --git a/curtin/udev.py b/curtin/udev.py
index 2b7a46e..11db908 100644
--- a/curtin/udev.py
+++ b/curtin/udev.py
@@ -129,4 +129,13 @@ def udevadm_info(path=None):
     return info
 
 
+def udev_all_block_device_properties():
+    import pyudev
+    props = []
+    c = pyudev.context()
+    for device in c.list_devices(subsystem='block'):
+        props.append(dict(device.properties))
+    return props
+
+
 # vi: ts=4 expandtab syntax=python
diff --git a/tests/unittests/test_commands_block_meta.py b/tests/unittests/test_commands_block_meta.py
index 3e22792..48653e6 100644
--- a/tests/unittests/test_commands_block_meta.py
+++ b/tests/unittests/test_commands_block_meta.py
@@ -23,6 +23,10 @@ class TestGetPathToStorageVolume(CiTestCase):
         self.add_patch(basepath + 'devsync', 'm_devsync')
         self.add_patch(basepath + 'util.subp', 'm_subp')
         self.add_patch(basepath + 'multipath.is_mpath_member', 'm_mp')
+        self.add_patch(
+            basepath + 'multipath.get_mpath_id_from_device', 'm_get_mpath_id')
+        self.add_patch(
+            basepath + 'udev_all_block_device_properties', 'm_udev_all')
 
     def test_block_lookup_called_with_disk_wwn(self):
         volume = 'mydisk'
@@ -57,6 +61,7 @@ class TestGetPathToStorageVolume(CiTestCase):
         self.assertEqual(expected_calls, self.m_lookup.call_args_list)
 
     def test_fallback_to_path_when_lookup_wwn_serial_fail(self):
+        self.m_mp.return_value = False
         volume = 'mydisk'
         wwn = self.random_string()
         serial = self.random_string()
@@ -75,6 +80,7 @@ class TestGetPathToStorageVolume(CiTestCase):
         self.assertEqual(path, result)
 
     def test_block_lookup_not_called_with_wwn_or_serial_keys(self):
+        self.m_mp.return_value = False
         volume = 'mydisk'
         path = "/%s/%s" % (self.random_string(), self.random_string())
         cfg = {'id': volume, 'type': 'disk', 'path': path}
@@ -106,6 +112,202 @@ class TestGetPathToStorageVolume(CiTestCase):
         self.assertEqual(expected_calls, self.m_lookup.call_args_list)
         self.m_exists.assert_has_calls([call(path)])
 
+    def test_v2_match_serial(self):
+        serial = self.random_string()
+        devname = self.random_string()
+        self.m_udev_all.return_value = [
+            {'DEVNAME': devname, 'ID_SERIAL': serial},
+            ]
+        disk_id = self.random_string()
+        cfg = {'id': disk_id, 'type': 'disk', 'serial': serial}
+        s_cfg = OrderedDict({disk_id: cfg})
+        s_cfg.version = 2
+        self.assertEqual(
+            devname,
+            block_meta.get_path_to_storage_volume(disk_id, s_cfg))
+
+    def test_v2_match_serial_skips_partition(self):
+        serial = self.random_string()
+        devname = self.random_string()
+        self.m_udev_all.return_value = [
+            {'DEVNAME': devname, 'ID_SERIAL': serial},
+            {'DEVNAME': devname+'p1', 'ID_SERIAL': serial, 'PARTN': "1"},
+            ]
+        disk_id = self.random_string()
+        cfg = {'id': disk_id, 'type': 'disk', 'serial': serial}
+        s_cfg = OrderedDict({disk_id: cfg})
+        s_cfg.version = 2
+        self.assertEqual(
+            devname,
+            block_meta.get_path_to_storage_volume(disk_id, s_cfg))
+
+    def test_v2_match_serial_prefer_mpath(self):
+        serial = self.random_string()
+        devname1 = self.random_string()
+        devname2 = self.random_string()
+        devname_dm = self.random_string()
+        self.m_udev_all.return_value = [
+            {'DEVNAME': devname1, 'ID_SERIAL': serial},
+            {'DEVNAME': devname2, 'ID_SERIAL': serial},
+            {'DEVNAME': devname_dm, 'DM_SERIAL': serial},
+            ]
+        disk_id = self.random_string()
+        cfg = {'id': disk_id, 'type': 'disk', 'serial': serial}
+        s_cfg = OrderedDict({disk_id: cfg})
+        s_cfg.version = 2
+        self.assertEqual(
+            devname_dm,
+            block_meta.get_path_to_storage_volume(disk_id, s_cfg))
+
+    def test_v2_match_serial_mpath_skip_partition(self):
+        serial = self.random_string()
+        devname1 = self.random_string()
+        devname2 = self.random_string()
+        devname_dm = self.random_string()
+        self.m_udev_all.return_value = [
+            {'DEVNAME': devname1, 'ID_SERIAL': serial},
+            {'DEVNAME': devname2, 'ID_SERIAL': serial},
+            {'DEVNAME': devname_dm, 'DM_SERIAL': serial},
+            {'DEVNAME': devname_dm+'p1', 'DM_SERIAL': serial, 'DM_PART': '1'},
+            ]
+        disk_id = self.random_string()
+        cfg = {'id': disk_id, 'type': 'disk', 'serial': serial}
+        s_cfg = OrderedDict({disk_id: cfg})
+        s_cfg.version = 2
+        self.assertEqual(
+            devname_dm,
+            block_meta.get_path_to_storage_volume(disk_id, s_cfg))
+
+    def test_v2_match_wwn(self):
+        wwn = self.random_string()
+        devname = self.random_string()
+        self.m_udev_all.return_value = [
+            {'DEVNAME': devname, 'ID_WWN': wwn},
+            ]
+        disk_id = self.random_string()
+        cfg = {'id': disk_id, 'type': 'disk', 'wwn': wwn}
+        s_cfg = OrderedDict({disk_id: cfg})
+        s_cfg.version = 2
+        self.assertEqual(
+            devname,
+            block_meta.get_path_to_storage_volume(disk_id, s_cfg))
+
+    def test_v2_match_wwn_prefer_mpath(self):
+        wwn = self.random_string()
+        devname1 = self.random_string()
+        devname2 = self.random_string()
+        devname_dm = self.random_string()
+        self.m_udev_all.return_value = [
+            {'DEVNAME': devname1, 'ID_WWN': wwn},
+            {'DEVNAME': devname2, 'ID_WWN': wwn},
+            {'DEVNAME': devname_dm, 'DM_WWN': wwn},
+            ]
+        disk_id = self.random_string()
+        cfg = {'id': disk_id, 'type': 'disk', 'wwn': wwn}
+        s_cfg = OrderedDict({disk_id: cfg})
+        s_cfg.version = 2
+        self.assertEqual(
+            devname_dm,
+            block_meta.get_path_to_storage_volume(disk_id, s_cfg))
+
+    def test_v2_match_serial_and_wwn(self):
+        serial = self.random_string()
+        wwn = self.random_string()
+        devname = self.random_string()
+        self.m_udev_all.return_value = [
+            {'DEVNAME': devname, 'ID_SERIAL': serial, 'ID_WWN': wwn},
+            ]
+        disk_id = self.random_string()
+        cfg = {'id': disk_id, 'type': 'disk', 'serial': serial, 'wwn': wwn}
+        s_cfg = OrderedDict({disk_id: cfg})
+        s_cfg.version = 2
+        self.assertEqual(
+            devname,
+            block_meta.get_path_to_storage_volume(disk_id, s_cfg))
+
+    def test_v2_different_serial_same_wwn(self):
+        serial1 = self.random_string()
+        serial2 = self.random_string()
+        wwn = self.random_string()
+        devname1 = self.random_string()
+        devname2 = self.random_string()
+        self.m_udev_all.return_value = [
+            {'DEVNAME': devname1, 'ID_SERIAL': serial1, 'ID_WWN': wwn},
+            {'DEVNAME': devname2, 'ID_SERIAL': serial2, 'ID_WWN': wwn},
+            ]
+        disk_id = self.random_string()
+        cfg = {'id': disk_id, 'type': 'disk', 'serial': serial2, 'wwn': wwn}
+        s_cfg = OrderedDict({disk_id: cfg})
+        s_cfg.version = 2
+        self.assertEqual(
+            devname2,
+            block_meta.get_path_to_storage_volume(disk_id, s_cfg))
+
+    def test_v2_fails_duplicate_wwn(self):
+        serial1 = self.random_string()
+        serial2 = self.random_string()
+        wwn = self.random_string()
+        devname1 = self.random_string()
+        devname2 = self.random_string()
+        self.m_udev_all.return_value = [
+            {'DEVNAME': devname1, 'ID_SERIAL': serial1, 'ID_WWN': wwn},
+            {'DEVNAME': devname2, 'ID_SERIAL': serial2, 'ID_WWN': wwn},
+            ]
+        disk_id = self.random_string()
+        cfg = {'id': disk_id, 'type': 'disk', 'wwn': wwn}
+        s_cfg = OrderedDict({disk_id: cfg})
+        s_cfg.version = 2
+        with self.assertRaises(ValueError):
+            block_meta.get_path_to_storage_volume(disk_id, s_cfg)
+
+    def test_v2_match_path(self):
+        self.m_mp.return_value = False
+        devname = self.random_string()
+        self.m_udev_all.return_value = [
+            {'DEVNAME': devname},
+            ]
+        disk_id = self.random_string()
+        cfg = {'id': disk_id, 'type': 'disk', 'path': devname}
+        s_cfg = OrderedDict({disk_id: cfg})
+        s_cfg.version = 2
+        self.assertEqual(
+            devname,
+            block_meta.get_path_to_storage_volume(disk_id, s_cfg))
+
+    def test_v2_match_path_link(self):
+        self.m_mp.return_value = False
+        devname = self.random_string()
+        link1 = self.random_string()
+        link2 = self.random_string()
+        links = link1 + ' ' + link2
+        self.m_udev_all.return_value = [
+            {'DEVNAME': devname, 'DEVLINKS': links},
+            ]
+        disk_id = self.random_string()
+        cfg = {'id': disk_id, 'type': 'disk', 'path': link1}
+        s_cfg = OrderedDict({disk_id: cfg})
+        s_cfg.version = 2
+        self.assertEqual(
+            devname,
+            block_meta.get_path_to_storage_volume(disk_id, s_cfg))
+
+    def test_v2_match_path_prefers_multipath(self):
+        self.m_mp.return_value = True
+        self.m_get_mpath_id.return_value = 'mpatha'
+        devname1 = self.random_string()
+        devname2 = self.random_string()
+        self.m_udev_all.return_value = [
+            {'DEVNAME': devname1, 'DEVLINKS': ''},
+            {'DEVNAME': devname2, 'DEVLINKS': '/dev/mapper/mpatha'},
+            ]
+        disk_id = self.random_string()
+        cfg = {'id': disk_id, 'type': 'disk', 'path': devname1}
+        s_cfg = OrderedDict({disk_id: cfg})
+        s_cfg.version = 2
+        self.assertEqual(
+            devname2,
+            block_meta.get_path_to_storage_volume(disk_id, s_cfg))
+
 
 class TestBlockMetaSimple(CiTestCase):
     def setUp(self):