curtin-dev team mailing list archive

[Olivier Gayot <mp+473583@xxxxxxxxxxxxxxxxxx>]

Olivier Gayot has proposed merging ~ogayot/curtin:by-path-by-uuid into curtin:master.

Commit message:
block_meta: ensure we don't use /dev/disk/by-path/XXX/by-uuid as volspec

Systems 256 introduced new symlinks in /dev/disk/by-path to avoid
potential UUID duplicates when the same image is written to multiple
storage units.

Such symlinks include:

 * /dev/disk/by-path/${path}/by-uuid/
 * /dev/disk/by-path/${path}/by-label/
 * /dev/disk/by-path/${path}/by-partnum/
 * /dev/disk/by-path/${path}/by-partuuid/
 * /dev/disk/by-path/${path}/by-partlabel/

When writing the fstab, we want curtin to specify the UUID of the
devices; rather than rely on a devlink which path can change when a
device gets removed or added.

To do so, we look for devlinks that match "by-uuid" and keep the first
match. Unfortunately, we now have two matches:

 * /dev/disk/by-uuid/${uuid}
 * /dev/disk/by-path/${path}/by-uuid/${uuid}

Ensure we use a stricter matching rule to avoid matching
/dev/disk/by-path/${path}/by-uuid/${uuid}.

LP: #2081256
Signed-off-by: Olivier Gayot <olivier.gayot@xxxxxxxxxxxxx>

Requested reviews:
  Server Team CI bot (server-team-bot): continuous-integration
  curtin developers (curtin-dev)
Related bugs:
  Bug #2081256 in curtin: "RISC-V Installation with encrypted LVM boots into emergency mode"
  https://bugs.launchpad.net/curtin/+bug/2081256

For more details, see:
https://code.launchpad.net/~ogayot/curtin/+git/curtin/+merge/473583
-- 
Your team curtin developers is requested to review the proposed merge of ~ogayot/curtin:by-path-by-uuid into curtin:master.

diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
index dc26a8c..9a11540 100644
--- a/curtin/commands/block_meta.py
+++ b/curtin/commands/block_meta.py
@@ -26,13 +26,13 @@ from curtin.udev import (
 import glob
 import json
 import os
+import pathlib
 import platform
 import string
 import sys
 import tempfile
 import time
 
-
 FstabData = namedtuple(
     "FstabData", ('spec', 'path', 'fstype', 'options', 'freq', 'passno',
                   'device'))
@@ -1294,6 +1294,11 @@ def _get_volume_fstype(device_path):
     return lsblock[kname]['FSTYPE']
 
 
+def devlink_is_child_of(devlink: str, path: str) -> bool:
+    """ Tells whether a given devlink is a direct child of path. """
+    return pathlib.Path(devlink).parent == pathlib.Path(path)
+
+
 def get_volume_spec(device_path):
     """
        Return the most reliable spec for a device per Ubuntu FSTAB wiki
@@ -1318,11 +1323,11 @@ def get_volume_spec(device_path):
     elif block_type in ['disk', 'part']:
         if device_path.startswith('/dev/bcache'):
             devlinks = [link for link in info['DEVLINKS']
-                        if link.startswith('/dev/bcache/by-uuid')]
+                        if devlink_is_child_of(link, '/dev/bcache/by-uuid')]
         # on s390x prefer by-path links which are stable and unique.
         if platform.machine() == 's390x':
             devlinks = [link for link in info['DEVLINKS']
-                        if link.startswith('/dev/disk/by-path')]
+                        if devlink_is_child_of(link, '/dev/disk/by-path')]
         # use device-mapper uuid if present
         if 'DM_UUID' in info:
             devlinks = [link for link in info['DEVLINKS']
@@ -1330,10 +1335,10 @@ def get_volume_spec(device_path):
         if len(devlinks) == 0:
             # use FS UUID if present
             devlinks = [link for link in info['DEVLINKS']
-                        if '/by-uuid' in link]
-            if len(devlinks) == 0 and block_type == 'part':
-                devlinks = [link for link in info['DEVLINKS']
-                            if '/by-partuuid' in link]
+                        if devlink_is_child_of(link, '/dev/disk/by-uuid')]
+        if len(devlinks) == 0 and block_type == 'part':
+            devlinks = [link for link in info['DEVLINKS']
+                        if devlink_is_child_of(link, '/dev/disk/by-partuuid')]
 
     return devlinks[0] if len(devlinks) else device_path
 
diff --git a/tests/unittests/test_commands_block_meta.py b/tests/unittests/test_commands_block_meta.py
index 7320fc1..a5ccac1 100644
--- a/tests/unittests/test_commands_block_meta.py
+++ b/tests/unittests/test_commands_block_meta.py
@@ -1463,6 +1463,17 @@ class TestFstabVolumeSpec(CiTestCase):
             "/dev/disk/by-partlabel/zfs-fa8d6afc7a67405c",
             "/dev/disk/by-partuuid/0b3eae85-960f-fb4f-b5ae-0b3551e763f8",
             "/dev/disk/by-path/pci-0000:00:17.0-ata-1-part1",
+            # New symlinks in systemd 256
+            ("/dev/disk/by-path/pci-0000:00:17.0-ata-1-part/" +
+             "by-uuid/14011020183977000633"),
+            "/dev/disk/by-path/pci-0000:00:17.0-ata-1-part/by-partnum/1",
+            ("/dev/disk/by-path/pci-0000:00:17.0-ata-1-part/" +
+             "by-partlabel/zfs-fa8d6afc7a67405c"),
+            ("/dev/disk/by-path/pci-0000:00:17.0-ata-1-part/by-partuuid/" +
+             "0b3eae85-960f-fb4f-b5ae-0b3551e763f8"),
+            "/dev/disk/by-path/pci-0000:00:17.0-ata-1-part/by-label/tank",
+            # Place this one last and make sure it gets used rather than
+            # /dev/disk/by-path/.../by-uuid
             "/dev/disk/by-uuid/14011020183977000633"],
         'raid': [
             "/dev/md/ubuntu-server:0",