Merge ~raharper/curtin:fix/mpath-partitions-missed-wiping-devices into curtin:master

[Ryan Harper]

clear-holders: ensure we wipe device even if multipath enabled not not mp

On Ubuntu releases where multipath is enabled but not in-use some devices
would fail to be wiped due to logic bug. This was found in the
BcacheBasic test. Refactor this logic to ensure that we wipe either
the in-use multipath dm device or the block device value originally
supplied to the wipe function.

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

PASSED: Continuous integration, rev:507498e2bfd9c506f27b5b957b0664d43beed62c
https://jenkins.ubuntu.com/server/job/curtin-ci/3821/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/3821/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/3821/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/3821/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=torkoal/3821/

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

review: Approve (continuous-integration)

[Dan Watkins (oddbloke)]

Do we need a test that would catch a similar regression in future?

[Ryan Harper (raharper)]

On Thu, Jan 30, 2020 at 10:43 AM Dan Watkins <email address hidden>
wrote:

> Do we need a test that would catch a similar regression in future?
>

Sure, let me add a unittest; vmtests caught it but no reason not to catch
it in unittest.

> --
> https://code.launchpad.net/~raharper/curtin/+git/curtin/+merge/378340
> You are the owner of
> ~raharper/curtin:fix/mpath-partitions-missed-wiping-devices.
>

410d419... by Ryan Harper on 2020-01-30

clear-holders: add multipath related scenarios

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

PASSED: Continuous integration, rev:410d41920d7c1fdbe08f165bba2b93ddbce1cad6
https://jenkins.ubuntu.com/server/job/curtin-ci/3822/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/3822/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/3822/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/3822/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=torkoal/3822/

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

review: Approve (continuous-integration)

[Dan Watkins (oddbloke)]

Thanks!

review: Approve

diff --git a/curtin/block/clear_holders.py b/curtin/block/clear_holders.py
index 08aa70a..b691841 100644
--- a/curtin/block/clear_holders.py
+++ b/curtin/block/clear_holders.py
@@ -301,9 +301,10 @@ def wipe_superblock(device):
             # if we don't find a mapping then the mp partition has already been
             # wiped/removed
             if mp_dev:
-                _wipe_superblock(mp_dev)
-    else:
-        _wipe_superblock(blockdev)
+                LOG.debug('Found multipath device over %s, wiping holder %s',
+                          blockdev, mp_dev)
+
+    _wipe_superblock(mp_dev if mp_dev else blockdev)
 
     # if we had partitions, make sure they've been removed
     if partitions:
diff --git a/tests/unittests/test_clear_holders.py b/tests/unittests/test_clear_holders.py
index 0a8f2da..fca5dd3 100644
--- a/tests/unittests/test_clear_holders.py
+++ b/tests/unittests/test_clear_holders.py
@@ -481,6 +481,92 @@ class TestClearHolders(CiTestCase):
             [mock.call(devices=[self.test_blockdev])] * 2)
         self.assertEqual(1, m_time.sleep.call_count)
 
+    @mock.patch('curtin.block.clear_holders.multipath')
+    @mock.patch('curtin.block.clear_holders.is_swap_device')
+    @mock.patch('curtin.block.clear_holders.time')
+    @mock.patch('curtin.block.clear_holders.LOG')
+    @mock.patch('curtin.block.clear_holders.block')
+    def test_clear_holders_mp_enabled_not_active_wipes_dev(self, mock_block,
+                                                           mock_log, m_time,
+                                                           mock_swap,
+                                                           mock_mpath):
+        """wipe_superblock wipes dev with multipath enabled but inactive."""
+        mock_swap.return_value = False
+        mock_block.sysfs_to_devpath.return_value = self.test_blockdev
+        mock_block.is_extended_partition.return_value = False
+        mock_mpath.multipath_supported.return_value = True
+        mock_mpath.find_mpath_id_by_path.return_value = None
+        mock_block.get_blockdev_for_partition.return_value = (
+            self.test_blockdev, 1)
+        mock_block.is_zfs_member.return_value = False
+        mock_block.get_sysfs_partitions.side_effect = iter([
+            ['p1', 'p2'],  # has partitions before wipe
+            ['p1', 'p2'],  # still has partitions after wipe
+            [],  # partitions are now gone
+        ])
+        clear_holders.wipe_superblock(self.test_syspath)
+        mock_block.sysfs_to_devpath.assert_called_with(self.test_syspath)
+        mock_block.wipe_volume.assert_called_with(
+            self.test_blockdev, exclusive=True, mode='superblock', strict=True)
+
+    @mock.patch('curtin.block.clear_holders.multipath')
+    @mock.patch('curtin.block.clear_holders.is_swap_device')
+    @mock.patch('curtin.block.clear_holders.time')
+    @mock.patch('curtin.block.clear_holders.LOG')
+    @mock.patch('curtin.block.clear_holders.block')
+    def test_clear_holders_mp_disabled_wipes_dev(self, mock_block, mock_log,
+                                                 m_time, mock_swap,
+                                                 mock_mpath):
+        """wipe_superblock wipes blockdev with multipath disabled."""
+        mock_swap.return_value = False
+        mock_block.sysfs_to_devpath.return_value = self.test_blockdev
+        mock_block.is_extended_partition.return_value = False
+        mock_mpath.multipath_supported.return_value = False
+        mock_block.get_blockdev_for_partition.return_value = (
+            self.test_blockdev, 1)
+        mock_block.is_zfs_member.return_value = False
+        mock_block.get_sysfs_partitions.side_effect = iter([
+            ['p1', 'p2'],  # has partitions before wipe
+            ['p1', 'p2'],  # still has partitions after wipe
+            [],  # partitions are now gone
+        ])
+        clear_holders.wipe_superblock(self.test_syspath)
+        mock_block.sysfs_to_devpath.assert_called_with(self.test_syspath)
+        mock_block.wipe_volume.assert_called_with(
+            self.test_blockdev, exclusive=True, mode='superblock', strict=True)
+
+    @mock.patch('curtin.block.clear_holders.multipath')
+    @mock.patch('curtin.block.clear_holders.is_swap_device')
+    @mock.patch('curtin.block.clear_holders.time')
+    @mock.patch('curtin.block.clear_holders.LOG')
+    @mock.patch('curtin.block.clear_holders.block')
+    def test_clear_holders_mp_enabled_and_active_wipes_dm_dev(self, mock_block,
+                                                              mock_log, m_time,
+                                                              mock_swap,
+                                                              mock_mpath):
+        """wipe_superblock wipes parent mp_dev and removes from dev mapper."""
+        mock_swap.return_value = False
+        mock_block.sysfs_to_devpath.return_value = self.test_blockdev
+        mock_block.is_zfs_member.return_value = False
+        mock_block.is_extended_partition.return_value = False
+        mock_block.get_blockdev_for_partition.return_value = (
+            self.test_blockdev, 1)
+
+        mock_mpath.multipath_supported.return_value = True
+        mock_mpath.find_mpath_id_by_path.return_value = 'mpath-wark'
+        mp_dev = '/wark/dm-1'
+        mock_mpath.find_mpath_id_by_parent.return_value = (
+            'mpath-wark', mp_dev)
+        mock_mpath.is_mpath_partition.return_value = False
+        mock_block.get_sysfs_partitions.side_effect = iter([
+            [],  # partitions are now gone
+        ])
+        clear_holders.wipe_superblock(self.test_syspath)
+        mock_block.sysfs_to_devpath.assert_called_with(self.test_syspath)
+        mock_block.wipe_volume.assert_called_with(
+            mp_dev, exclusive=True, mode='superblock', strict=True)
+        mock_mpath.remove_partition.assert_called_with(mp_dev)
+
     @mock.patch('curtin.block.clear_holders.LOG')
     @mock.patch('curtin.block.clear_holders.block')
     @mock.patch('curtin.block.clear_holders.os')