curtin-dev team mailing list archive

[Olivier Gayot <mp+440232@xxxxxxxxxxxxxxxxxx>]

Olivier Gayot has proposed merging ~ogayot/curtin:wwn-match-with-extension into curtin:master.

Commit message:
    block-meta: fix failed disk lookup when WWN includes extension
    
    curtin's storage config extracts the WWN of a disk from one of the
    following udev variables:
    
     * ID_WWN_WITH_EXTENSION (preferred)
     * ID_WWN
    
    However, when trying to look the disk up later in the block-meta code,
    curtin only tries to match the WWN value against the ID_WWN variable
    (and another variable related to DM multipath).
    
    This means that the disk can not be be found just based on the wwn if
    the ID_WWN and ID_WWN_WITH_EXTENSION variables don't hold the same
    value.
    
    In the past, that was often okay because other fields (such as disk path
    or serial) would still make the disk lookup succeed.
    
    However, the following patch introduced a restriction. In v2, all fields
    specified must now match for the disk lookup to be successful:
    
    8c5f87ed block_meta: all fields on a disk action must match with v2 config
    
    Fixed by matching against the ID_WWN_WITH_EXTENSION (preferred) and then
    ID_WWN.
    
    Signed-off-by: Olivier Gayot <olivier.gayot@xxxxxxxxxxxxx>


Requested reviews:
  Server Team CI bot (server-team-bot): continuous-integration
  curtin developers (curtin-dev)

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

We got multiple reports of failed installs where curtin logs show the following pattern:

found candidate disks [set(), {'/dev/sda'}, {'/dev/sda'}]
        An error occured handling 'disk-sda': ValueError - Failed to find storage volume id='disk-sda' config: {'ptable': 'gpt', 'serial': '121212121212121212121212', 'wwn': '0x123456789abcdef0deadbeef', 'path': '/dev/sda', 'wipe': 'superblock-recursive', 'preserve': False, 'name': '', 'grub_device': False, 'type': 'disk', 'id': 'disk-sda'}

When trying to look up the disk, the value of the wwn field (i.e., 0x123456789abcdef0deadbeef here) is compared to the ID_WWN udev variable of available disks.

However, the wwn variable is not always initialized from the ID_WWN variable. If present, the ID_WWN_WITH_EXTENSION variable takes precedence:           

source_keys = {
                'wwn': ['ID_WWN_WITH_EXTENSION', 'ID_WWN'],
                'serial': ['ID_SERIAL', 'ID_SERIAL_SHORT'],
            }

Example
-------

"ID_WWN": '0x123456789abcdef0'
"ID_WWN_VENDOR_EXTENSION": "0xdeadbeef",
"ID_WWN_WITH_EXTENSION": '0x123456789abcdef0deadbeef'

Since present, the wwn variable gets initialized from ID_WWN_WITH_EXTENSION.

Later on, we compare the wwn variable with ID_WWN and the lookup fails.

Fixed by making sure to compare the wwn value with the ID_WWN_WITH_EXTENSION first (if available), and only then ID_WWN.
-- 
Your team curtin developers is requested to review the proposed merge of ~ogayot/curtin:wwn-match-with-extension into curtin:master.

diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
index c0e595d..7988f3a 100644
--- a/curtin/commands/block_meta.py
+++ b/curtin/commands/block_meta.py
@@ -513,7 +513,8 @@ def v2_get_path_to_disk(vol):
         cands.append(set([dev['DEVNAME'] for dev in new_devs]))
 
     if 'wwn' in vol:
-        add_cands(*disk_by_keys(vol['wwn'], 'DM_WWN', 'ID_WWN'))
+        add_cands(*disk_by_keys(vol['wwn'],
+                                'DM_WWN', 'ID_WWN_WITH_EXTENSION', 'ID_WWN'))
     if 'serial' in vol:
         add_cands(
             *disk_by_keys(
diff --git a/tests/unittests/test_commands_block_meta.py b/tests/unittests/test_commands_block_meta.py
index 0c198e6..82530ff 100644
--- a/tests/unittests/test_commands_block_meta.py
+++ b/tests/unittests/test_commands_block_meta.py
@@ -222,6 +222,23 @@ class TestGetPathToStorageVolume(CiTestCase):
             devname,
             block_meta.get_path_to_storage_volume(disk_id, s_cfg))
 
+    def test_v2_match_wwn_with_extension(self):
+        wwn = self.random_string()
+        extension = self.random_string()
+        devname = self.random_string()
+        self.m_udev_all.return_value = [
+            {'DEVNAME': devname,
+             'ID_WWN': wwn,
+             'ID_WWN_WITH_EXTENSION': wwn + extension},
+            ]
+        disk_id = self.random_string()
+        cfg = {'id': disk_id, 'type': 'disk', 'wwn': wwn + extension}
+        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()