Merge ~mwhudson/curtin:fix-multipath-partition-verification-2 into curtin:master

[Michael Hudson-Doyle]

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 udev poking 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 (see kernel commit
edfaa7c36574f1bf09c65ad602412db9da5f96bf) so we can just remove
the call to get_blockdev_for_partition from sys_block_read.

This is like https://code.launchpad.net/~mwhudson/curtin/+git/curtin/+merge/396046 but much simpler.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:8c6388a8970c5154cd244ffb5d8afaad2bdfa9ca
https://jenkins.ubuntu.com/server/job/curtin-ci/65/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/65/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/65/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/65/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/65/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/65//rebuild

review: Approve (continuous-integration)

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:9cbfc9841af7edda8cb508c568cc41f03840a1a4
https://jenkins.ubuntu.com/server/job/curtin-ci/66/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/66/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/66/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/66/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/66/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/66//rebuild

review: Needs Fixing (continuous-integration)

[Michael Hudson-Doyle (mwhudson)]

Thanks for the review.

[Ryan Harper (raharper)]

> Michael Hudson-Doyle (mwhudson) wrote 6 minutes ago:
> I'm not sure what "really disks" means on a philosophical level <wink> but yes

Maybe I'm missing the <wink> here, but just for the folks following along I'll
be explicit.

I'm referring to multipath partitions which a linear mapping to offsets on the
multipath path disk in the device-mapper table. Device-mapper will create
additional device-mapper devices (/dev/dm-1, /dev/dm-2) which according to
sysfs, udev and any other linux tool, are of type disk. For example, in sysfs
/sys/class/block/dm-1 can be a "partition" and /sys/class/block/dm-1/partition
will not exist.

The mutipath software stack has conventions to determine which dm devices are
representing partitions by exporting values like DM_PART and prefixing dm
names with part- ; etc.

This code makes use of these multipath convensions.

> this is for multipath. I found that there was a currently-unused function in
> multipath.py that does most of what we want so I changed it to return the
> partition number as well and changed this code. (it used udev rather than
> sysfs pokery but well. no real difference).

OK, sounds good! I'll review.

> On the subject of mocks, get_blockdev_for_partition is currently untested.
> Tests could be written but you'd have to mock so many filesystem accesses that
> it would be incredibly fragile and I'm not sure what value it would bring.
>
> You must have thought about a more generic way to mock the filesystem by now,
> surely, like something where you supply a dictionary mapping paths to
> content and mocks for the various os.path.* functions get set up for you?
> Did you ever write anything for this I can steal? :)

I think the closes thing we have is using tests/unittests/helpers.py:dir2dict
and populate_dir and FilesystemMocking unittest class'[1] reRoot() method which
effectively applies a chroot prefix to the operations.

1. https://github.com/canonical/cloud-init/blob/master/cloudinit/tests/helpers.py#L242

[Michael Hudson-Doyle (mwhudson)]

> > Michael Hudson-Doyle (mwhudson) wrote 6 minutes ago:
> > I'm not sure what "really disks" means on a philosophical level <wink> but
> yes
>
> Maybe I'm missing the <wink> here, but just for the folks following along I'll
> be explicit.

Well mostly I'm just being a bit silly.

> I'm referring to multipath partitions which a linear mapping to offsets on the
> multipath path disk in the device-mapper table. Device-mapper will create
> additional device-mapper devices (/dev/dm-1, /dev/dm-2) which according to
> sysfs, udev and any other linux tool, are of type disk. For example, in sysfs
> /sys/class/block/dm-1 can be a "partition" and /sys/class/block/dm-1/partition
> will not exist.

Yeah, although I suspect their classification as "disk" is more by default vs them having any better classification.

> The mutipath software stack has conventions to determine which dm devices are
> representing partitions by exporting values like DM_PART and prefixing dm
> names with part- ; etc.

Yeah, I think it's worth saying that these conventions are pretty baked-in: none of it is enforced (afaict) by the kernel or anything, but if you try to set up a multipath device with a device mapper "uuid" that does not start with "mpath-", you're going to have a bad time.

It's a bit frustrating in a way that this information gets spread around so much -- most of this stuff is encoded by multipathd into the dm "uuid" and then read out of the uuid again by 11-dm-mpath.rules / kpartx_id. It's a bit hard to know which information source to treat as canonical. Probably reading things from udev is the right level of abstraction.

> This code makes use of these multipath convensions.
>
>
> > this is for multipath. I found that there was a currently-unused function in
> > multipath.py that does most of what we want so I changed it to return the
> > partition number as well and changed this code. (it used udev rather than
> > sysfs pokery but well. no real difference).
>
> OK, sounds good! I'll review.
>
> > On the subject of mocks, get_blockdev_for_partition is currently untested.
> > Tests could be written but you'd have to mock so many filesystem accesses
> that
> > it would be incredibly fragile and I'm not sure what value it would bring.
> >
> > You must have thought about a more generic way to mock the filesystem by
> now,
> > surely, like something where you supply a dictionary mapping paths to
> > content and mocks for the various os.path.* functions get set up for you?
> > Did you ever write anything for this I can steal? :)
>
>
> I think the closes thing we have is using tests/unittests/helpers.py:dir2dict
> and populate_dir and FilesystemMocking unittest class'[1] reRoot() method
> which
> effectively applies a chroot prefix to the operations.

Yeah, that's the sort of thing I was thinking of, although I guess I was more imagining something in memory (which would be more effort to implement but presumably faster in execution).

> 1. https://github.com/canonical/cloud-
> init/blob/master/cloudinit/tests/helpers.py#L242

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:3cb7c3e575634af78b5257a4e9723eb2d6fec7ae
https://jenkins.ubuntu.com/server/job/curtin-ci/67/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/67/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/67/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/67/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/67/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/67//rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:283184ef83e46c5643da4b58c349343aabef1737
https://jenkins.ubuntu.com/server/job/curtin-ci/68/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/68/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/68/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/68/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/68/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/68//rebuild

review: Needs Fixing (continuous-integration)

[Michael Hudson-Doyle (mwhudson)]

FWIW I tested this and it allowed me to reuse multipath partitions.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:283184ef83e46c5643da4b58c349343aabef1737
https://jenkins.ubuntu.com/server/job/curtin-ci/72/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/72/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/72/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/72/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/72/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/72//rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

Looks good.

review: Approve

[Server Team CI bot (server-team-bot)]

Commit message lints:
- Line #17 has 44 too many characters. Line starts with: "https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=edfaa7c36574f1bf09c65ad602412db9da5f96b"...

review: Needs Fixing

[Server Team CI bot (server-team-bot)]

review: Approve (continuous-integration)

diff --git a/curtin/block/__init__.py b/curtin/block/__init__.py
index 0cf0866..1b33002 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,13 @@ 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))
 
+    if rpath.startswith('/dev/dm-'):
+        parent_info = multipath.mpath_partition_to_mpath_id_and_partnumber(
+            rpath)
+        if parent_info is not None:
+            mpath_id, ptnum = parent_info
+            return os.path.realpath('/dev/mapper/' + mpath_id), ptnum
+
     ptpath = os.path.join(syspath, "partition")
     if not os.path.exists(ptpath):
         return (rpath, None)
diff --git a/curtin/block/multipath.py b/curtin/block/multipath.py
index 7ad1791..a28e930 100644
--- a/curtin/block/multipath.py
+++ b/curtin/block/multipath.py
@@ -88,11 +88,11 @@ def is_mpath_partition(devpath, info=None):
     return result
 
 
-def mpath_partition_to_mpath_id(devpath):
-    """ Return the mpath id of a multipath partition. """
+def mpath_partition_to_mpath_id_and_partnumber(devpath):
+    """ Return the mpath id and partition number of a multipath partition. """
     info = udev.udevadm_info(devpath)
-    if 'DM_MPATH' in info:
-        return info['DM_MPATH']
+    if 'DM_MPATH' in info and 'DM_PART' in info:
+        return info['DM_MPATH'], info['DM_PART']
 
     return 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):
diff --git a/tests/unittests/test_block_multipath.py b/tests/unittests/test_block_multipath.py
index 96cbcba..c56ec1e 100644
--- a/tests/unittests/test_block_multipath.py
+++ b/tests/unittests/test_block_multipath.py
@@ -82,19 +82,25 @@ class TestMultipath(CiTestCase):
         """is_mpath_member returns false if DM_PART is not present for dev."""
         self.assertFalse(multipath.is_mpath_partition(self.random_string()))
 
-    def test_mpath_partition_to_mpath_id(self):
+    def test_mpath_partition_to_mpath_id_and_partnumber(self):
         """mpath_part_to_mpath_id extracts MD_MPATH value from mp partition."""
         dev = self.random_string()
         mpath_id = self.random_string()
-        self.m_udev.udevadm_info.return_value = {'DM_MPATH': mpath_id}
-        self.assertEqual(mpath_id,
-                         multipath.mpath_partition_to_mpath_id(dev))
+        mpath_ptnum = self.random_string()
+        self.m_udev.udevadm_info.return_value = {
+            'DM_MPATH': mpath_id,
+            'DM_PART': mpath_ptnum,
+            }
+        self.assertEqual(
+            (mpath_id, mpath_ptnum),
+            multipath.mpath_partition_to_mpath_id_and_partnumber(dev))
 
-    def test_mpath_partition_to_mpath_id_none(self):
+    def test_mpath_partition_to_mpath_id_and_partnumber_none(self):
         """mpath_part_to_mpath_id returns none if DM_MPATH missing."""
         dev = self.random_string()
         self.m_udev.udevadm_info.return_value = {}
-        self.assertIsNone(multipath.mpath_partition_to_mpath_id(dev))
+        self.assertIsNone(
+            multipath.mpath_partition_to_mpath_id_and_partnumber(dev))
 
     @mock.patch('curtin.block.multipath.os.path.exists')
     @mock.patch('curtin.block.multipath.util.wait_for_removal')