curtin-dev team mailing list archive

[Michael Hudson-Doyle <mwhudsonlp@xxxxxxxxxxx>]

Michael Hudson-Doyle has proposed merging ~mwhudson/curtin:fix-multipath-partition-verification-2 into curtin:master.

Commit message:
block: fixes for verifying existing multipath partitions

Verifying a partition on a multipath disk currently fails for two
reasons:

 1. get_blockdev_for_partition does not know how to go from a multipath
    partition to a multipath disk, so sfdisk_info ends up calling sfdisk
    on the partition, which fails.
 2. sfdisk --json /dev/dm-X prints /dev/mapper paths for the partition
    paths but compares them against the fully deferenced /dev/dm-Y path
    for the partition.

2 is easily fixed by resolving symlinks before comparing nodes. 1 can be
fixed with a little sysfs yoga but then sys_block_path returns paths
like /sys/class/block/dm-1/dm-2 for a partition of a multipath disk,
which doesn't exist. Luckily, nodes for partitions exist directly in
/sys/class/block and have done since /sys/class/block was added in 2008:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=edfaa7c36574f1bf09c65ad602412db9da5f96bf
so we can just remove the call to get_blockdev_for_partition from
sys_block_read.

Requested reviews:
  curtin developers (curtin-dev)

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

This is like https://code.launchpad.net/~mwhudson/curtin/+git/curtin/+merge/396046 but much simpler.
-- 
Your team curtin developers is requested to review the proposed merge of ~mwhudson/curtin:fix-multipath-partition-verification-2 into curtin:master.

diff --git a/curtin/block/__init__.py b/curtin/block/__init__.py
index 0cf0866..eca69d8 100644
--- a/curtin/block/__init__.py
+++ b/curtin/block/__init__.py
@@ -158,9 +158,6 @@ def sys_block_path(devname, add=None, strict=True):
     devname = os.path.normpath(devname)
     if devname.startswith('/dev/') and not os.path.exists(devname):
         LOG.warning('block.sys_block_path: devname %s does not exist', devname)
-    (parent, partnum) = get_blockdev_for_partition(devname, strict=strict)
-    if partnum:
-        toks.append(path_to_kname(parent))
 
     toks.append(path_to_kname(devname))
 
@@ -309,7 +306,7 @@ def get_partition_sfdisk_info(devpath, sfdisk_info=None):
         sfdisk_info = sfdisk_info(devpath)
 
     entry = [part for part in sfdisk_info['partitions']
-             if part['node'] == devpath]
+             if os.path.realpath(part['node']) == os.path.realpath(devpath)]
     if len(entry) != 1:
         raise RuntimeError('Device %s not present in sfdisk dump:\n%s' %
                            devpath, util.json_dumps(sfdisk_info))
@@ -421,6 +418,14 @@ def get_blockdev_for_partition(devpath, strict=True):
     if strict and not os.path.exists(syspath):
         raise OSError("%s had no syspath (%s)" % (devpath, syspath))
 
+    dm_name_path = os.path.join(syspath, 'dm/name')
+    if os.path.exists(dm_name_path):
+        dm_name = util.load_file(dm_name_path).rstrip()
+        if '-part' in dm_name:
+            parent_name, ptnum = dm_name.rsplit('-part', 1)
+            parent_path = os.path.realpath('/dev/mapper/' + parent_name)
+            return (parent_path, ptnum)
+
     ptpath = os.path.join(syspath, "partition")
     if not os.path.exists(ptpath):
         return (rpath, None)
diff --git a/tests/unittests/test_block.py b/tests/unittests/test_block.py
index 78e331d..d96a4a8 100644
--- a/tests/unittests/test_block.py
+++ b/tests/unittests/test_block.py
@@ -193,47 +193,37 @@ class TestBlock(CiTestCase):
 
 
 class TestSysBlockPath(CiTestCase):
-    @mock.patch("curtin.block.get_blockdev_for_partition")
     @mock.patch("os.path.exists")
-    def test_existing_valid_devname(self, m_os_path_exists, m_get_blk):
+    def test_existing_valid_devname(self, m_os_path_exists):
         m_os_path_exists.return_value = True
-        m_get_blk.return_value = ('foodevice', None)
         self.assertEqual('/sys/class/block/foodevice',
                          block.sys_block_path("foodevice"))
 
-    @mock.patch("curtin.block.get_blockdev_for_partition")
     @mock.patch("os.path.exists")
-    def test_existing_devpath_allowed(self, m_os_path_exists, m_get_blk):
+    def test_existing_devpath_allowed(self, m_os_path_exists):
         m_os_path_exists.return_value = True
-        m_get_blk.return_value = ('foodev', None)
         self.assertEqual('/sys/class/block/foodev',
                          block.sys_block_path("/dev/foodev"))
 
-    @mock.patch("curtin.block.get_blockdev_for_partition")
     @mock.patch("os.path.exists")
-    def test_add_works(self, m_os_path_exists, m_get_blk):
+    def test_add_works(self, m_os_path_exists):
         m_os_path_exists.return_value = True
-        m_get_blk.return_value = ('foodev', None)
         self.assertEqual('/sys/class/block/foodev/md/b',
                          block.sys_block_path("/dev/foodev", "md/b"))
 
-    @mock.patch("curtin.block.get_blockdev_for_partition")
     @mock.patch("os.path.exists")
-    def test_add_works_leading_slash(self, m_os_path_exists, m_get_blk):
+    def test_add_works_leading_slash(self, m_os_path_exists):
         m_os_path_exists.return_value = True
-        m_get_blk.return_value = ('foodev', None)
         self.assertEqual('/sys/class/block/foodev/md/b',
                          block.sys_block_path("/dev/foodev", "/md/b"))
 
-    @mock.patch("curtin.block.get_blockdev_for_partition")
     @mock.patch("os.path.exists")
-    def test_invalid_devname_raises(self, m_os_path_exists, m_get_blk):
+    def test_invalid_devname_raises(self, m_os_path_exists):
         m_os_path_exists.return_value = False
-        with self.assertRaises(ValueError):
+        with self.assertRaises(OSError):
             block.sys_block_path("foodevice")
 
-    @mock.patch("curtin.block.get_blockdev_for_partition")
-    def test_invalid_with_add(self, m_get_blk):
+    def test_invalid_with_add(self):
         # test the device exists, but 'add' does not
         # path_exists returns true unless 'md/device' is in it
         #  so /sys/class/foodev/ exists, but not /sys/class/foodev/md/device
@@ -242,28 +232,20 @@ class TestSysBlockPath(CiTestCase):
         def path_exists(path):
             return add not in path
 
-        m_get_blk.return_value = ("foodev", None)
         with mock.patch('os.path.exists', side_effect=path_exists):
             self.assertRaises(OSError, block.sys_block_path, "foodev", add)
 
-    @mock.patch("curtin.block.get_blockdev_for_partition")
     @mock.patch("os.path.exists")
-    def test_not_strict_does_not_care(self, m_os_path_exists, m_get_blk):
+    def test_not_strict_does_not_care(self, m_os_path_exists):
         m_os_path_exists.return_value = False
-        m_get_blk.return_value = ('foodev', None)
         self.assertEqual('/sys/class/block/foodev/md/b',
                          block.sys_block_path("foodev", "/md/b", strict=False))
 
-    @mock.patch('curtin.block.get_blockdev_for_partition')
     @mock.patch('os.path.exists')
-    def test_cciss_sysfs_path(self, m_os_path_exists, m_get_blk):
+    def test_cciss_sysfs_path(self, m_os_path_exists):
         m_os_path_exists.return_value = True
-        m_get_blk.return_value = ('cciss!c0d0', None)
         self.assertEqual('/sys/class/block/cciss!c0d0',
                          block.sys_block_path('/dev/cciss/c0d0'))
-        m_get_blk.return_value = ('cciss!c0d0', 1)
-        self.assertEqual('/sys/class/block/cciss!c0d0/cciss!c0d0p1',
-                         block.sys_block_path('/dev/cciss/c0d0p1'))
 
 
 class TestWipeFile(CiTestCase):