curtin-dev team mailing list archive

[Dan Bungert <mp+460876@xxxxxxxxxxxxxxxxxx>]

Dan Bungert has proposed merging ~dbungert/curtin:fix-volume-id into curtin:master.

Commit message:
block_meta: improve get_volume_uuid

blkid can return 3 different types of UUIDs, and they aren't
interchangable.  Clarify which one we're using.

Also, leverage the existing blkid() wrapper to do so.



Requested reviews:
  curtin developers (curtin-dev)

For more details, see:
https://code.launchpad.net/~dbungert/curtin/+git/curtin/+merge/460876
-- 
Your team curtin developers is requested to review the proposed merge of ~dbungert/curtin:fix-volume-id into curtin:master.

diff --git a/curtin/block/__init__.py b/curtin/block/__init__.py
index 87990d6..2e19467 100644
--- a/curtin/block/__init__.py
+++ b/curtin/block/__init__.py
@@ -811,16 +811,16 @@ def read_sys_block_size_bytes(device):
     return size
 
 
-def get_volume_uuid(path):
+def get_volume_id(path):
     """
-    Get uuid of disk with given path. This address uniquely identifies
-    the device and remains consistant across reboots
+    Get identifier of device with given path. This address uniquely identifies
+    the device and remains consistant across reboots.
     """
-    (out, _err) = util.subp(["blkid", "-o", "export", path], capture=True)
-    for line in out.splitlines():
-        if "UUID" in line:
-            return line.split('=')[-1]
-    return ''
+    ids = blkid([path])[path]
+    for key in ("UUID", "PARTUUID", "PTUUID"):
+        if key in ids:
+            return (key, ids[key])
+    return (None, '')
 
 
 def get_mountpoints():
diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
index 9fde9c6..c7bd0dd 100644
--- a/curtin/commands/block_meta.py
+++ b/curtin/commands/block_meta.py
@@ -1733,10 +1733,15 @@ def dm_crypt_handler(info, storage_config, context):
     if state['fstab']:
         state_dir = os.path.dirname(state['fstab'])
         crypt_tab_location = os.path.join(state_dir, "crypttab")
-        uuid = block.get_volume_uuid(volume_path)
+        key, value = block.get_volume_id(volume_path)
+        if key is None:
+            raise RuntimeError(f"Failed to find suitable ID for {volume_path}")
+
         util.write_file(
             crypt_tab_location,
-            f"{dm_name} UUID={uuid} {crypttab_keyfile} {options}\n", omode="a")
+            f"{dm_name} {key}={value} {crypttab_keyfile} {options}\n",
+            omode="a",
+        )
     else:
         LOG.info("fstab configuration is not present in environment, so \
             cannot locate an appropriate directory to write crypttab in \
diff --git a/tests/integration/test_block_meta.py b/tests/integration/test_block_meta.py
index ec9b33c..568f466 100644
--- a/tests/integration/test_block_meta.py
+++ b/tests/integration/test_block_meta.py
@@ -1324,7 +1324,6 @@ table-length: 256'''.encode()
             crypttab = fp.read()
         tokens = re.split(r'\s+', crypttab)
         self.assertEqual(cryptoswap, tokens[0])
-        self.assertTrue(tokens[1].startswith("UUID="))
         self.assertEqual("/dev/urandom", tokens[2])
         self.assertEqual("swap,initramfs", tokens[3])
 
diff --git a/tests/unittests/test_block.py b/tests/unittests/test_block.py
index 1ef8f80..2818fe4 100644
--- a/tests/unittests/test_block.py
+++ b/tests/unittests/test_block.py
@@ -16,17 +16,18 @@ from curtin import block
 
 class TestBlock(CiTestCase):
 
-    @mock.patch("curtin.block.util")
-    def test_get_volume_uuid(self, mock_util):
+    @mock.patch("curtin.block.util.subp")
+    def test_get_volume_id(self, mock_subp):
         path = "/dev/sda1"
-        expected_call = ["blkid", "-o", "export", path]
-        mock_util.subp.return_value = ("""
-            UUID=182e8e23-5322-46c9-a1b8-cf2c6a88f9f7
-            """, "")
+        expected_call = ["blkid", "-o", "full", path]
+        mock_subp.return_value = (
+            "/dev/sda1: UUID=182e8e23-5322-46c9-a1b8-cf2c6a88f9f7", "",
+        )
 
-        uuid = block.get_volume_uuid(path)
+        key, uuid = block.get_volume_id(path)
 
-        mock_util.subp.assert_called_with(expected_call, capture=True)
+        mock_subp.assert_called_with(expected_call, capture=True)
+        self.assertEqual(key, "UUID")
         self.assertEqual(uuid, "182e8e23-5322-46c9-a1b8-cf2c6a88f9f7")
 
     @mock.patch("curtin.block.get_proc_mounts")
diff --git a/tests/unittests/test_commands_block_meta.py b/tests/unittests/test_commands_block_meta.py
index 5adb46c..c2d592d 100644
--- a/tests/unittests/test_commands_block_meta.py
+++ b/tests/unittests/test_commands_block_meta.py
@@ -627,8 +627,8 @@ class TestBlockMeta(CiTestCase):
                        'mock_assert_clear')
         self.add_patch('curtin.block.iscsi.volpath_is_iscsi',
                        'mock_volpath_is_iscsi')
-        self.add_patch('curtin.block.get_volume_uuid',
-                       'mock_block_get_volume_uuid')
+        self.add_patch('curtin.block.get_volume_id',
+                       'mock_block_get_volume_id')
         self.add_patch('curtin.commands.block_meta._get_volume_type',
                        'mock_get_volume_type')
         self.add_patch('curtin.commands.block_meta.udevadm_info',
@@ -1313,7 +1313,7 @@ class TestFstabData(CiTestCase):
         self.assertEqual(1, m_uinfo.call_count)
         self.assertEqual(1, m_vol_type.call_count)
 
-    @patch('curtin.block.get_volume_uuid')
+    @patch('curtin.block.get_volume_id')
     def test_fstab_line_for_data__spec_and_dev_prefers_spec(self, m_get_uuid):
         """fstab_line_for_data should prefer spec over device."""
         spec = "/dev/xvda1"
@@ -2048,8 +2048,8 @@ class DmCryptCommon(CiTestCase):
         self.cipher = self.random_string()
         self.keysize = self.random_string()
         self.m_block.zkey_supported.return_value = False
-        self.block_uuids = [random_uuid() for unused in range(2)]
-        self.m_block.get_volume_uuid.side_effect = self.block_uuids
+        self.block_uuids = [("UUID", random_uuid()) for unused in range(2)]
+        self.m_block.get_volume_id.side_effect = self.block_uuids
 
         self.m_which.return_value = False
         self.fstab = self.tmp_path('fstab')
@@ -2469,8 +2469,8 @@ class TestCrypttab(DmCryptCommon):
 
         data = util.load_file(self.crypttab)
         self.assertEqual(
-            f"cryptroot UUID={self.block_uuids[0]} none luks\n"
-            f"cryptfoo UUID={self.block_uuids[1]} none luks\n",
+            f"cryptroot UUID={self.block_uuids[0][1]} none luks\n"
+            f"cryptfoo UUID={self.block_uuids[1][1]} none luks\n",
             data
         )
 
@@ -2509,7 +2509,7 @@ class TestCrypttab(DmCryptCommon):
             self.storage_config['dmcrypt0'], self.storage_config, empty_context
         )
 
-        uuid = self.block_uuids[0]
+        uuid = self.block_uuids[0][1]
         self.assertEqual(
             f"cryptswap UUID={uuid} /dev/urandom swap,initramfs\n",
             util.load_file(self.crypttab),
diff --git a/tests/unittests/test_partitioning.py b/tests/unittests/test_partitioning.py
index 50ff07c..ab09260 100644
--- a/tests/unittests/test_partitioning.py
+++ b/tests/unittests/test_partitioning.py
@@ -195,7 +195,7 @@ class TestBlock(CiTestCase):
                                                            "target":
                                                            "/tmp/mntdir"}
         mock_get_path_to_storage_volume.return_value = "/dev/fake0"
-        mock_block.get_volume_uuid.return_value = "UUID123"
+        mock_block.get_volume_id.return_value = ("UUID", "UUID123")
 
         curtin.commands.block_meta.mount_handler(
             self.storage_config.get("sda2_mount"), self.storage_config)
@@ -208,7 +208,7 @@ class TestBlock(CiTestCase):
         args = mock_get_path_to_storage_volume.call_args_list
         self.assertTrue(len(args) == 1)
         self.assertTrue(args[0] == mock.call("sda2", self.storage_config))
-        mock_block.get_volume_uuid.assert_called_with("/dev/fake0")
+        mock_block.get_volume_id.assert_called_with("/dev/fake0")
 
         curtin.commands.block_meta.mount_handler(
             self.storage_config.get("raid_mount"), self.storage_config)
@@ -260,7 +260,7 @@ class TestBlock(CiTestCase):
                                                            "/tmp/dir/fstab"}
         mock_get_path_to_storage_volume.return_value = "/dev/fake0"
         mock_tempfile.mkstemp.return_value = ["fp", tmp_path]
-        mock_block.get_volume_uuid.return_value = "UUID123"
+        mock_block.get_volume_id.return_value = ("UUID", "UUID123")
 
         curtin.commands.block_meta.dm_crypt_handler(
             self.storage_config.get("crypt0_key"), self.storage_config)
@@ -280,7 +280,7 @@ class TestBlock(CiTestCase):
             calls[1])
         mock_remove.assert_called_with(tmp_path)
         mock_open.assert_called_with("/tmp/dir/crypttab", "a")
-        mock_block.get_volume_uuid.assert_called_with(
+        mock_block.get_volume_id.assert_called_with(
             mock_get_path_to_storage_volume.return_value)
 
     @mock.patch("curtin.commands.block_meta.block")
@@ -296,7 +296,7 @@ class TestBlock(CiTestCase):
         mock_util.load_command_environment.return_value = {"fstab":
                                                            "/tmp/dir/fstab"}
         mock_get_path_to_storage_volume.return_value = "/dev/fake0"
-        mock_block.get_volume_uuid.return_value = "UUID123"
+        mock_block.get_volume_id.return_value = ("UUID", "UUID123")
 
         config = self.storage_config["crypt0_keyfile"]
         curtin.commands.block_meta.dm_crypt_handler(
@@ -318,7 +318,7 @@ class TestBlock(CiTestCase):
             calls[1])
         self.assertFalse(mock_remove.called)
         mock_remove.assert_not_called()
-        mock_block.get_volume_uuid.assert_called_with(
+        mock_block.get_volume_id.assert_called_with(
             mock_get_path_to_storage_volume.return_value)
 
     @mock.patch("curtin.commands.block_meta.block")
@@ -330,7 +330,7 @@ class TestBlock(CiTestCase):
         mock_util.load_command_environment.return_value = {"fstab":
                                                            "/tmp/dir/fstab"}
         mock_get_path_to_storage_volume.return_value = "/dev/fake0"
-        mock_block.get_volume_uuid.return_value = "UUID123"
+        mock_block.get_volume_id.return_value = ("UUID", "UUID123")
 
         self.assertRaises(
             ValueError,