Merge ~raharper/curtin:fix/clear-holders-refactor into curtin:master

[Ryan Harper]

block/clear_holders/mdadm: refactor handling of layered device wiping

The block layer handling of checking efi/dos signatures did not ensure
that it always used a devpath (/dev/<kname>) which resulted in
failures when attempting to identify dos partitions, including
extended partitions due to attempting to open /sys/class/block/<kname>
to look for metadata. In addition there is a window where
wiping/removing lvm devices in dos logical partitions renders the
/dev/<kname> path to the extended partition missing, no /dev/<kname>
We now re-read a block device's partition table after completing a
shutdown of each device. Doing this re-read triggered mdadm devices
which had been stopped to resume preventing wiping of the underlying
devices. A refactor of how raid devices are shutdown now includes
collecting the list of raid members and spares, failing each element
out of the array, stopping the rescan/rebuild of the array and finally
removing the mdadm metadata on the underlying devices.

With this in place, we can now successful run the LVM test-cases with
dirty_disk mode enabled and simultaneously continue to pass the
complex bcache/raid examples.

LP: #1768893
LP: #1769742

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

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

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

review: Approve (continuous-integration)

[Scott Moser (smoser)]

some comments in line.

I think i'm generally ok with this.
I'm happy that you can add dirty_disks and show that this fixed something.

f3eb081... by Ryan Harper on 2018-05-15

Change suppress=False to warn_on_fail=True

a032d77... by Ryan Harper on 2018-05-15

Switch to using block.quick_zero(path, partitions=False)

fcb3775... by Ryan Harper on 2018-05-15

use combine_capture=True to merge sfdisk stderr output into logged msg.

f9b2ef5... by Ryan Harper on 2018-05-15

Drop mdadm_ prefix on newly added mdadm methods.

[Ryan Harper (raharper)]

I've updated with your suggestions applied. Kicking off a vmtest run:

https://jenkins.ubuntu.com/server/view/curtin/job/curtin-vmtest-devel-debug/62/console

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

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

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

review: Approve (continuous-integration)

374a1ff... by Ryan Harper on 2018-05-17

Fix remaining clear-holder issues with rescanning partitions and iscsi devices

Moving the scan to run before issuing the next shutdown command satisfies the
LVM scenario where msdos extended partitiosn disappear and further we need to
always trigger the automatic mode for iscsi devices as the iscsiadm discovery
reconfigures the defaults.

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

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

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

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

This now passes vmtest run on jenkins:

https://jenkins.ubuntu.com/server/view/curtin/job/curtin-vmtest-devel-debug/63/console

[Scott Moser (smoser)]

review: Approve

[Scott Moser (smoser)]

An upstream commit landed for this bug.

To view that commit see the following URL:
https://git.launchpad.net/curtin/commit/?id=4bf7750b

diff --git a/curtin/block/__init__.py b/curtin/block/__init__.py
index 50e953e..a8ee8a6 100644
--- a/curtin/block/__init__.py
+++ b/curtin/block/__init__.py
@@ -378,7 +378,7 @@ def stop_all_unused_multipath_devices():
         LOG.warn("Failed to stop multipath devices: %s", e)
 
 
-def rescan_block_devices():
+def rescan_block_devices(warn_on_fail=True):
     """
     run 'blockdev --rereadpt' for all block devices not currently mounted
     """
@@ -399,13 +399,15 @@ def rescan_block_devices():
     try:
         util.subp(cmd, capture=True)
     except util.ProcessExecutionError as e:
-        # FIXME: its less than ideal to swallow this error, but until
-        # we fix LP: #1489521 we kind of need to.
-        LOG.warn("Error rescanning devices, possibly known issue LP: #1489521")
-        # Reformatting the exception output so as to not trigger
-        # vmtest scanning for Unexepected errors in install logfile
-        LOG.warn("cmd: %s\nstdout:%s\nstderr:%s\nexit_code:%s", e.cmd,
-                 e.stdout, e.stderr, e.exit_code)
+        if warn_on_fail:
+            # FIXME: its less than ideal to swallow this error, but until
+            # we fix LP: #1489521 we kind of need to.
+            LOG.warn(
+                "Error rescanning devices, possibly known issue LP: #1489521")
+            # Reformatting the exception output so as to not trigger
+            # vmtest scanning for Unexepected errors in install logfile
+            LOG.warn("cmd: %s\nstdout:%s\nstderr:%s\nexit_code:%s", e.cmd,
+                     e.stdout, e.stderr, e.exit_code)
 
     udevadm_settle()
 
@@ -753,8 +755,9 @@ def check_dos_signature(device):
     # the underlying disk uses a larger logical block size, so the start of
     # this signature must be at 0x1fe
     # https://en.wikipedia.org/wiki/Master_boot_record#Sector_layout
-    return (is_block_device(device) and util.file_size(device) >= 0x200 and
-            (util.load_file(device, decode=False, read_len=2, offset=0x1fe) ==
+    devname = dev_path(path_to_kname(device))
+    return (is_block_device(devname) and util.file_size(devname) >= 0x200 and
+            (util.load_file(devname, decode=False, read_len=2, offset=0x1fe) ==
              b'\x55\xAA'))
 
 
@@ -769,10 +772,11 @@ def check_efi_signature(device):
     # the start of the gpt partition table header shoult have the signaure
     # 'EFI PART'.
     # https://en.wikipedia.org/wiki/GUID_Partition_Table
-    sector_size = get_blockdev_sector_size(device)[0]
-    return (is_block_device(device) and
-            util.file_size(device) >= 2 * sector_size and
-            (util.load_file(device, decode=False, read_len=8,
+    devname = dev_path(path_to_kname(device))
+    sector_size = get_blockdev_sector_size(devname)[0]
+    return (is_block_device(devname) and
+            util.file_size(devname) >= 2 * sector_size and
+            (util.load_file(devname, decode=False, read_len=8,
                             offset=sector_size) == b'EFI PART'))
 
 
diff --git a/curtin/block/clear_holders.py b/curtin/block/clear_holders.py
index e0ee044..cbb7074 100644
--- a/curtin/block/clear_holders.py
+++ b/curtin/block/clear_holders.py
@@ -110,6 +110,9 @@ def shutdown_bcache(device):
                          'Device path must start with /sys/class/block/',
                          device)
 
+    LOG.info('Wiping superblock on bcache device: %s', device)
+    _wipe_superblock(block.sysfs_to_devpath(device), exclusive=False)
+
     # bcache device removal should be fast but in an extreme
     # case, might require the cache device to flush large
     # amounts of data to a backing device.  The strategy here
@@ -189,14 +192,27 @@ def shutdown_lvm(device):
     name_file = os.path.join(device, 'dm', 'name')
     lvm_name = util.load_file(name_file).strip()
     (vg_name, lv_name) = lvm.split_lvm_name(lvm_name)
+    vg_lv_name = "%s/%s" % (vg_name, lv_name)
+    devname = "/dev/" + vg_lv_name
+
+    # wipe contents of the logical volume first
+    LOG.info('Wiping lvm logical volume: %s', devname)
+    block.quick_zero(devname, partitions=False)
 
-    # use dmsetup as lvm commands require valid /etc/lvm/* metadata
-    LOG.debug('using "dmsetup remove" on %s', lvm_name)
-    util.subp(['dmsetup', 'remove', lvm_name])
+    # remove the logical volume
+    LOG.debug('using "lvremove" on %s', vg_lv_name)
+    util.subp(['lvremove', '--force', '--force', vg_lv_name])
 
     # if that was the last lvol in the volgroup, get rid of volgroup
     if len(lvm.get_lvols_in_volgroup(vg_name)) == 0:
+        pvols = lvm.get_pvols_in_volgroup(vg_name)
         util.subp(['vgremove', '--force', '--force', vg_name], rcs=[0, 5])
+
+        # wipe the underlying physical volumes
+        for pv in pvols:
+            LOG.info('Wiping lvm physical volume: %s', pv)
+            block.quick_zero(pv, partitions=False)
+
     # refresh lvmetad
     lvm.lvm_scan()
 
@@ -213,10 +229,31 @@ def shutdown_mdadm(device):
     """
     Shutdown specified mdadm device.
     """
+
     blockdev = block.sysfs_to_devpath(device)
+
+    LOG.info('Wiping superblock on raid device: %s', device)
+    _wipe_superblock(blockdev, exclusive=False)
+
+    md_devs = (
+        mdadm.md_get_devices_list(blockdev) +
+        mdadm.md_get_spares_list(blockdev))
+    mdadm.set_sync_action(blockdev, action="idle")
+    mdadm.set_sync_action(blockdev, action="frozen")
+    for mddev in md_devs:
+        try:
+            mdadm.fail_device(blockdev, mddev)
+            mdadm.remove_device(blockdev, mddev)
+        except util.ProcessExecutionError as e:
+            LOG.debug('Non-fatal error clearing raid array: %s', e.stderr)
+            pass
+
     LOG.debug('using mdadm.mdadm_stop on dev: %s', blockdev)
     mdadm.mdadm_stop(blockdev)
 
+    for mddev in md_devs:
+        mdadm.zero_device(mddev)
+
     # mdadm stop operation is asynchronous so we must wait for the kernel to
     # release resources. For more details see  LP: #1682456
     try:
@@ -244,34 +281,49 @@ def wipe_superblock(device):
     blockdev = block.sysfs_to_devpath(device)
     # when operating on a disk that used to have a dos part table with an
     # extended partition, attempting to wipe the extended partition will fail
-    if block.is_extended_partition(blockdev):
-        LOG.info("extended partitions do not need wiping, so skipping: '%s'",
-                 blockdev)
-    else:
-        # release zfs member by exporting the pool
-        if block.is_zfs_member(blockdev):
-            poolname = zfs.device_to_poolname(blockdev)
-            # only export pools that have been imported
-            if poolname in zfs.zpool_list():
-                zfs.zpool_export(poolname)
-
-        if is_swap_device(blockdev):
-            shutdown_swap(blockdev)
-
-        # some volumes will be claimed by the bcache layer but do not surface
-        # an actual /dev/bcacheN device which owns the parts (backing, cache)
-        # The result is that some volumes cannot be wiped while bcache claims
-        # the device.  Resolve this by stopping bcache layer on those volumes
-        # if present.
-        for bcache_path in ['bcache', 'bcache/set']:
-            stop_path = os.path.join(device, bcache_path)
-            if os.path.exists(stop_path):
-                LOG.debug('Attempting to release bcache layer from device: %s',
-                          device)
-                maybe_stop_bcache_device(stop_path)
-                continue
-
-        _wipe_superblock(blockdev)
+    try:
+        if block.is_extended_partition(blockdev):
+            LOG.info("extended partitions do not need wiping, so skipping:"
+                     " '%s'", blockdev)
+            return
+    except OSError as e:
+        if util.is_file_not_found_exc(e):
+            LOG.debug('Device to wipe disappeared: %s', e)
+            LOG.debug('/proc/partitions says: %s',
+                      util.load_file('/proc/partitions'))
+
+            (parent, partnum) = block.get_blockdev_for_partition(blockdev)
+            out, _e = util.subp(['sfdisk', '-d', parent],
+                                capture=True, combine_capture=True)
+            LOG.debug('Disk partition info:\n%s', out)
+            return
+        else:
+            raise e
+
+    # release zfs member by exporting the pool
+    if block.is_zfs_member(blockdev):
+        poolname = zfs.device_to_poolname(blockdev)
+        # only export pools that have been imported
+        if poolname in zfs.zpool_list():
+            zfs.zpool_export(poolname)
+
+    if is_swap_device(blockdev):
+        shutdown_swap(blockdev)
+
+    # some volumes will be claimed by the bcache layer but do not surface
+    # an actual /dev/bcacheN device which owns the parts (backing, cache)
+    # The result is that some volumes cannot be wiped while bcache claims
+    # the device.  Resolve this by stopping bcache layer on those volumes
+    # if present.
+    for bcache_path in ['bcache', 'bcache/set']:
+        stop_path = os.path.join(device, bcache_path)
+        if os.path.exists(stop_path):
+            LOG.debug('Attempting to release bcache layer from device: %s',
+                      device)
+            maybe_stop_bcache_device(stop_path)
+            continue
+
+    _wipe_superblock(blockdev)
 
 
 def _wipe_superblock(blockdev, exclusive=True):
@@ -512,28 +564,7 @@ def clear_holders(base_paths, try_preserve=False):
     LOG.info('Current device storage tree:\n%s',
              '\n'.join(format_holders_tree(tree) for tree in holder_trees))
     ordered_devs = plan_shutdown_holder_trees(holder_trees)
-
-    # run wipe-superblock on layered devices
-    for dev_info in ordered_devs:
-        dev_type = DEV_TYPES.get(dev_info['dev_type'])
-        shutdown_function = dev_type.get('shutdown')
-        if not shutdown_function:
-            continue
-
-        if try_preserve and shutdown_function in DATA_DESTROYING_HANDLERS:
-            LOG.info('shutdown function for holder type: %s is destructive. '
-                     'attempting to preserve data, so skipping' %
-                     dev_info['dev_type'])
-            continue
-
-        # for layered block devices, wipe first, then shutdown
-        if dev_info['dev_type'] in ['bcache', 'raid']:
-            LOG.info("Wiping superblock on layered device type: "
-                     "'%s' syspath: '%s'", dev_info['dev_type'],
-                     dev_info['device'])
-            # we just want to wipe data, we don't care about exclusive
-            _wipe_superblock(block.sysfs_to_devpath(dev_info['device']),
-                             exclusive=False)
+    LOG.info('Shutdown Plan:\n%s', "\n".join(map(str, ordered_devs)))
 
     # run shutdown functions
     for dev_info in ordered_devs:
@@ -548,11 +579,12 @@ def clear_holders(base_paths, try_preserve=False):
                      dev_info['dev_type'])
             continue
 
+        # scan before we check
+        block.rescan_block_devices(warn_on_fail=False)
         if os.path.exists(dev_info['device']):
             LOG.info("shutdown running on holder type: '%s' syspath: '%s'",
                      dev_info['dev_type'], dev_info['device'])
             shutdown_function(dev_info['device'])
-            udev.udevadm_settle()
 
 
 def start_clear_holders_deps():
diff --git a/curtin/block/iscsi.py b/curtin/block/iscsi.py
index 461f615..0c666b6 100644
--- a/curtin/block/iscsi.py
+++ b/curtin/block/iscsi.py
@@ -416,18 +416,17 @@ class IscsiDisk(object):
             self.portal, self.target, self.lun)
 
     def connect(self):
-        if self.target in iscsiadm_sessions():
-            return
-
-        iscsiadm_discovery(self.portal)
+        if self.target not in iscsiadm_sessions():
+            iscsiadm_discovery(self.portal)
 
-        iscsiadm_authenticate(self.target, self.portal, self.user,
-                              self.password, self.iuser, self.ipassword)
+            iscsiadm_authenticate(self.target, self.portal, self.user,
+                                  self.password, self.iuser, self.ipassword)
 
-        iscsiadm_login(self.target, self.portal)
+            iscsiadm_login(self.target, self.portal)
 
-        udev.udevadm_settle(self.devdisk_path)
+            udev.udevadm_settle(self.devdisk_path)
 
+        # always set automatic mode
         iscsiadm_set_automatic(self.target, self.portal)
 
     def disconnect(self):
diff --git a/curtin/block/mdadm.py b/curtin/block/mdadm.py
index 2e73e71..e0fe0d3 100644
--- a/curtin/block/mdadm.py
+++ b/curtin/block/mdadm.py
@@ -237,6 +237,44 @@ def mdadm_examine(devpath, export=MDADM_USE_EXPORT):
     return data
 
 
+def set_sync_action(devpath, action=None, retries=None):
+    assert_valid_devpath(devpath)
+    if not action:
+        return
+
+    if not retries:
+        retries = [0.2] * 60
+
+    sync_action = md_sysfs_attr_path(devpath, 'sync_action')
+    if not os.path.exists(sync_action):
+        # arrays without sync_action can't set values
+        return
+
+    LOG.info("mdadm set sync_action=%s on array %s", action, devpath)
+    for (attempt, wait) in enumerate(retries):
+        try:
+            LOG.debug('mdadm: set sync_action %s attempt %s',
+                      devpath, attempt)
+            val = md_sysfs_attr(devpath, 'sync_action').strip()
+            LOG.debug('sync_action = "%s" ? "%s"', val, action)
+            if val != action:
+                LOG.debug("mdadm: setting array sync_action=%s", action)
+                try:
+                    util.write_file(sync_action, content=action)
+                except (IOError, OSError) as e:
+                    LOG.debug("mdadm: (non-fatal) write to %s failed %s",
+                              sync_action, e)
+            else:
+                LOG.debug("mdadm: set array sync_action=%s SUCCESS", action)
+                return
+
+        except util.ProcessExecutionError:
+            LOG.debug(
+                "mdadm: set sync_action failed, retrying in %s seconds", wait)
+            time.sleep(wait)
+            pass
+
+
 def mdadm_stop(devpath, retries=None):
     assert_valid_devpath(devpath)
     if not retries:
@@ -305,6 +343,33 @@ def mdadm_remove(devpath):
     LOG.debug("mdadm remove:\n%s\n%s", out, err)
 
 
+def fail_device(mddev, arraydev):
+    assert_valid_devpath(mddev)
+
+    LOG.info("mdadm mark faulty: %s in array %s", arraydev, mddev)
+    out, err = util.subp(["mdadm", "--fail", mddev, arraydev],
+                         rcs=[0], capture=True)
+    LOG.debug("mdadm mark faulty:\n%s\n%s", out, err)
+
+
+def remove_device(mddev, arraydev):
+    assert_valid_devpath(mddev)
+
+    LOG.info("mdadm remove %s from array %s", arraydev, mddev)
+    out, err = util.subp(["mdadm", "--remove", mddev, arraydev],
+                         rcs=[0], capture=True)
+    LOG.debug("mdadm remove:\n%s\n%s", out, err)
+
+
+def zero_device(devpath):
+    assert_valid_devpath(devpath)
+
+    LOG.info("mdadm zero superblock on %s", devpath)
+    out, err = util.subp(["mdadm", "--zero-superblock", devpath],
+                         rcs=[0], capture=True)
+    LOG.debug("mdadm zero superblock:\n%s\n%s", out, err)
+
+
 def mdadm_query_detail(md_devname, export=MDADM_USE_EXPORT):
     valid_mdname(md_devname)
 
diff --git a/tests/unittests/test_clear_holders.py b/tests/unittests/test_clear_holders.py
index ddb149f..9c93b5a 100644
--- a/tests/unittests/test_clear_holders.py
+++ b/tests/unittests/test_clear_holders.py
@@ -132,6 +132,7 @@ class TestClearHolders(CiTestCase):
             mock_block.path_to_kname.assert_called_with(self.test_syspath)
             mock_get_dmsetup_uuid.assert_called_with(self.test_syspath)
 
+    @mock.patch('curtin.block.clear_holders.block')
     @mock.patch('curtin.block.clear_holders.udev.udevadm_settle')
     @mock.patch('curtin.block.clear_holders.get_bcache_sys_path')
     @mock.patch('curtin.block.clear_holders.util')
@@ -140,7 +141,7 @@ class TestClearHolders(CiTestCase):
     @mock.patch('curtin.block.clear_holders.get_bcache_using_dev')
     def test_shutdown_bcache(self, mock_get_bcache, mock_log, mock_os,
                              mock_util, mock_get_bcache_block,
-                             mock_udevadm_settle):
+                             mock_udevadm_settle, mock_block):
         """test clear_holders.shutdown_bcache"""
         #
         # pass in a sysfs path to a bcache block device,
@@ -152,6 +153,7 @@ class TestClearHolders(CiTestCase):
         #
 
         device = self.test_syspath
+        mock_block.sys_block_path.return_value = '/dev/null'
         bcache_cset_uuid = 'c08ae789-a964-46fb-a66e-650f0ae78f94'
 
         mock_os.path.exists.return_value = True
@@ -197,6 +199,7 @@ class TestClearHolders(CiTestCase):
         self.assertEqual(0, len(mock_util.call_args_list))
         self.assertEqual(0, len(mock_get_bcache_block.call_args_list))
 
+    @mock.patch('curtin.block.clear_holders.block')
     @mock.patch('curtin.block.clear_holders.get_bcache_sys_path')
     @mock.patch('curtin.block.clear_holders.util')
     @mock.patch('curtin.block.clear_holders.os')
@@ -204,18 +207,20 @@ class TestClearHolders(CiTestCase):
     @mock.patch('curtin.block.clear_holders.get_bcache_using_dev')
     def test_shutdown_bcache_no_device(self, mock_get_bcache, mock_log,
                                        mock_os, mock_util,
-                                       mock_get_bcache_block):
+                                       mock_get_bcache_block, mock_block):
         device = "/sys/class/block/null"
+        mock_block.sysfs_to_devpath.return_value = '/dev/null'
         mock_os.path.exists.return_value = False
 
         clear_holders.shutdown_bcache(device)
 
-        self.assertEqual(1, len(mock_log.info.call_args_list))
+        self.assertEqual(3, len(mock_log.info.call_args_list))
         self.assertEqual(1, len(mock_os.path.exists.call_args_list))
         self.assertEqual(0, len(mock_get_bcache.call_args_list))
         self.assertEqual(0, len(mock_util.call_args_list))
         self.assertEqual(0, len(mock_get_bcache_block.call_args_list))
 
+    @mock.patch('curtin.block.clear_holders.block')
     @mock.patch('curtin.block.clear_holders.get_bcache_sys_path')
     @mock.patch('curtin.block.clear_holders.util')
     @mock.patch('curtin.block.clear_holders.os')
@@ -223,8 +228,9 @@ class TestClearHolders(CiTestCase):
     @mock.patch('curtin.block.clear_holders.get_bcache_using_dev')
     def test_shutdown_bcache_no_cset(self, mock_get_bcache, mock_log,
                                      mock_os, mock_util,
-                                     mock_get_bcache_block):
+                                     mock_get_bcache_block, mock_block):
         device = "/sys/class/block/null"
+        mock_block.sysfs_to_devpath.return_value = '/dev/null'
         mock_os.path.exists.side_effect = iter([
                 True,   # backing device exists
                 False,  # cset device not present (already removed)
@@ -236,7 +242,7 @@ class TestClearHolders(CiTestCase):
 
         clear_holders.shutdown_bcache(device)
 
-        self.assertEqual(2, len(mock_log.info.call_args_list))
+        self.assertEqual(4, len(mock_log.info.call_args_list))
         self.assertEqual(3, len(mock_os.path.exists.call_args_list))
         self.assertEqual(1, len(mock_get_bcache.call_args_list))
         self.assertEqual(1, len(mock_get_bcache_block.call_args_list))
@@ -252,6 +258,7 @@ class TestClearHolders(CiTestCase):
             mock.call(device, retries=retries),
             mock.call(device + '/bcache', retries=retries)])
 
+    @mock.patch('curtin.block.clear_holders.block')
     @mock.patch('curtin.block.clear_holders.udev.udevadm_settle')
     @mock.patch('curtin.block.clear_holders.get_bcache_sys_path')
     @mock.patch('curtin.block.clear_holders.util')
@@ -262,8 +269,10 @@ class TestClearHolders(CiTestCase):
                                                      mock_log, mock_os,
                                                      mock_util,
                                                      mock_get_bcache_block,
-                                                     mock_udevadm_settle):
+                                                     mock_udevadm_settle,
+                                                     mock_block):
         device = "/sys/class/block/null"
+        mock_block.sysfs_to_devpath.return_value = '/dev/null'
         mock_os.path.exists.side_effect = iter([
                 True,  # backing device exists
                 True,  # cset device not present (already removed)
@@ -276,7 +285,7 @@ class TestClearHolders(CiTestCase):
 
         clear_holders.shutdown_bcache(device)
 
-        self.assertEqual(2, len(mock_log.info.call_args_list))
+        self.assertEqual(4, len(mock_log.info.call_args_list))
         self.assertEqual(3, len(mock_os.path.exists.call_args_list))
         self.assertEqual(1, len(mock_get_bcache.call_args_list))
         self.assertEqual(1, len(mock_get_bcache_block.call_args_list))
@@ -293,6 +302,7 @@ class TestClearHolders(CiTestCase):
             mock.call(device, retries=self.remove_retries)
         ])
 
+    @mock.patch('curtin.block.clear_holders.block')
     @mock.patch('curtin.block.clear_holders.udev.udevadm_settle')
     @mock.patch('curtin.block.clear_holders.get_bcache_sys_path')
     @mock.patch('curtin.block.clear_holders.util')
@@ -303,8 +313,10 @@ class TestClearHolders(CiTestCase):
                                                     mock_log, mock_os,
                                                     mock_util,
                                                     mock_get_bcache_block,
-                                                    mock_udevadm_settle):
+                                                    mock_udevadm_settle,
+                                                    mock_block):
         device = "/sys/class/block/null"
+        mock_block.sysfs_to_devpath.return_value = '/dev/null'
         mock_os.path.exists.side_effect = iter([
                 True,   # backing device exists
                 True,   # cset device not present (already removed)
@@ -317,7 +329,7 @@ class TestClearHolders(CiTestCase):
 
         clear_holders.shutdown_bcache(device)
 
-        self.assertEqual(2, len(mock_log.info.call_args_list))
+        self.assertEqual(4, len(mock_log.info.call_args_list))
         self.assertEqual(3, len(mock_os.path.exists.call_args_list))
         self.assertEqual(1, len(mock_get_bcache.call_args_list))
         self.assertEqual(1, len(mock_get_bcache_block.call_args_list))
@@ -333,6 +345,8 @@ class TestClearHolders(CiTestCase):
         ])
 
     # test bcache shutdown with 'stop' sysfs write failure
+    @mock.patch('curtin.block.clear_holders.block')
+    @mock.patch('curtin.block.wipe_volume')
     @mock.patch('curtin.block.clear_holders.udev.udevadm_settle')
     @mock.patch('curtin.block.clear_holders.get_bcache_sys_path')
     @mock.patch('curtin.block.clear_holders.util')
@@ -343,9 +357,12 @@ class TestClearHolders(CiTestCase):
                                                     mock_log, mock_os,
                                                     mock_util,
                                                     mock_get_bcache_block,
-                                                    mock_udevadm_settle):
+                                                    mock_udevadm_settle,
+                                                    mock_wipe,
+                                                    mock_block):
         """Test writes sysfs write failures pass if file not present"""
         device = "/sys/class/block/null"
+        mock_block.sysfs_to_devpath.return_value = '/dev/null'
         mock_os.path.exists.side_effect = iter([
                 True,   # backing device exists
                 True,   # cset device not present (already removed)
@@ -363,7 +380,7 @@ class TestClearHolders(CiTestCase):
 
         clear_holders.shutdown_bcache(device)
 
-        self.assertEqual(2, len(mock_log.info.call_args_list))
+        self.assertEqual(4, len(mock_log.info.call_args_list))
         self.assertEqual(3, len(mock_os.path.exists.call_args_list))
         self.assertEqual(1, len(mock_get_bcache.call_args_list))
         self.assertEqual(1, len(mock_get_bcache_block.call_args_list))
@@ -378,15 +395,20 @@ class TestClearHolders(CiTestCase):
             mock.call(cset, retries=self.remove_retries)
         ])
 
+    @mock.patch('curtin.block.quick_zero')
     @mock.patch('curtin.block.clear_holders.LOG')
     @mock.patch('curtin.block.clear_holders.block.sys_block_path')
     @mock.patch('curtin.block.clear_holders.lvm')
     @mock.patch('curtin.block.clear_holders.util')
-    def test_shutdown_lvm(self, mock_util, mock_lvm, mock_syspath, mock_log):
+    def test_shutdown_lvm(self, mock_util, mock_lvm, mock_syspath, mock_log,
+                          mock_zero):
         """test clear_holders.shutdown_lvm"""
         lvm_name = b'ubuntu--vg-swap\n'
         vg_name = 'ubuntu-vg'
         lv_name = 'swap'
+        vg_lv_name = "%s/%s" % (vg_name, lv_name)
+        devname = "/dev/" + vg_lv_name
+        pvols = ['/dev/wda1', '/dev/wda2']
         mock_syspath.return_value = self.test_blockdev
         mock_util.load_file.return_value = lvm_name
         mock_lvm.split_lvm_name.return_value = (vg_name, lv_name)
@@ -394,18 +416,22 @@ class TestClearHolders(CiTestCase):
         clear_holders.shutdown_lvm(self.test_blockdev)
         mock_syspath.assert_called_with(self.test_blockdev)
         mock_util.load_file.assert_called_with(self.test_blockdev + '/dm/name')
+        mock_zero.assert_called_with(devname, partitions=False)
         mock_lvm.split_lvm_name.assert_called_with(lvm_name.strip())
         self.assertTrue(mock_log.debug.called)
         mock_util.subp.assert_called_with(
-            ['dmsetup', 'remove', lvm_name.strip()])
-
+            ['lvremove', '--force', '--force', vg_lv_name])
         mock_lvm.get_lvols_in_volgroup.assert_called_with(vg_name)
         self.assertEqual(len(mock_util.subp.call_args_list), 1)
-        self.assertTrue(mock_lvm.lvm_scan.called)
         mock_lvm.get_lvols_in_volgroup.return_value = []
+        self.assertTrue(mock_lvm.lvm_scan.called)
+        mock_lvm.get_pvols_in_volgroup.return_value = pvols
         clear_holders.shutdown_lvm(self.test_blockdev)
         mock_util.subp.assert_called_with(
             ['vgremove', '--force', '--force', vg_name], rcs=[0, 5])
+        for pv in pvols:
+            mock_zero.assert_any_call(pv, partitions=False)
+        self.assertTrue(mock_lvm.lvm_scan.called)
 
     @mock.patch('curtin.block.clear_holders.block')
     @mock.patch('curtin.block.clear_holders.util')
@@ -417,18 +443,38 @@ class TestClearHolders(CiTestCase):
         mock_util.subp.assert_called_with(
             ['cryptsetup', 'remove', self.test_blockdev], capture=True)
 
+    @mock.patch('curtin.block.wipe_volume')
+    @mock.patch('curtin.block.path_to_kname')
+    @mock.patch('curtin.block.sysfs_to_devpath')
     @mock.patch('curtin.block.clear_holders.time')
     @mock.patch('curtin.block.clear_holders.util')
     @mock.patch('curtin.block.clear_holders.LOG')
     @mock.patch('curtin.block.clear_holders.mdadm')
-    @mock.patch('curtin.block.clear_holders.block')
-    def test_shutdown_mdadm(self, mock_block, mock_mdadm, mock_log, mock_util,
-                            mock_time):
+    def test_shutdown_mdadm(self, mock_mdadm, mock_log, mock_util,
+                            mock_time, mock_sysdev, mock_path, mock_wipe):
         """test clear_holders.shutdown_mdadm"""
-        mock_block.sysfs_to_devpath.return_value = self.test_blockdev
-        mock_block.path_to_kname.return_value = self.test_blockdev
+        devices = ['/dev/wda1', '/dev/wda2']
+        spares = ['/dev/wdb1']
+        md_devs = (devices + spares)
+        mock_sysdev.return_value = self.test_blockdev
+        mock_path.return_value = self.test_blockdev
         mock_mdadm.md_present.return_value = False
+        mock_mdadm.md_get_devices_list.return_value = devices
+        mock_mdadm.md_get_spares_list.return_value = spares
+
         clear_holders.shutdown_mdadm(self.test_syspath)
+
+        mock_wipe.assert_called_with(
+            self.test_blockdev, exclusive=False, mode='superblock')
+        mock_mdadm.set_sync_action.assert_has_calls([
+                mock.call(self.test_blockdev, action="idle"),
+                mock.call(self.test_blockdev, action="frozen")])
+        mock_mdadm.fail_device.assert_has_calls(
+            [mock.call(self.test_blockdev, dev) for dev in md_devs])
+        mock_mdadm.remove_device.assert_has_calls(
+            [mock.call(self.test_blockdev, dev) for dev in md_devs])
+        mock_mdadm.zero_device.assert_has_calls(
+            [mock.call(dev) for dev in md_devs])
         mock_mdadm.mdadm_stop.assert_called_with(self.test_blockdev)
         mock_mdadm.md_present.assert_called_with(self.test_blockdev)
         self.assertTrue(mock_log.debug.called)
diff --git a/tests/vmtests/test_lvm.py b/tests/vmtests/test_lvm.py
index 40f6cb7..ed708fd 100644
--- a/tests/vmtests/test_lvm.py
+++ b/tests/vmtests/test_lvm.py
@@ -10,10 +10,15 @@ class TestLvmAbs(VMBaseClass):
     conf_file = "examples/tests/lvm.yaml"
     interactive = False
     extra_disks = ['10G']
+    dirty_disks = True
     collect_scripts = VMBaseClass.collect_scripts + [textwrap.dedent("""
         cd OUTPUT_COLLECT_D
         cat /etc/fstab > fstab
         ls /dev/disk/by-dname > ls_dname
+        ls -al /dev/disk/by-dname > lsal_dname
+        ls -al /dev/disk/by-id/ > ls_byid
+        ls -al /dev/disk/by-uuid/ > ls_byuuid
+        cat /proc/partitions > proc_partitions
         find /etc/network/interfaces.d > find_interfacesd
         pvdisplay -C --separator = -o vg_name,pv_name --noheadings > pvs
         lvdisplay -C --separator = -o lv_name,vg_name --noheadings > lvs
diff --git a/tests/vmtests/test_lvm_iscsi.py b/tests/vmtests/test_lvm_iscsi.py
index e94fae3..2a11d6e 100644
--- a/tests/vmtests/test_lvm_iscsi.py
+++ b/tests/vmtests/test_lvm_iscsi.py
@@ -9,6 +9,7 @@ import textwrap
 
 class TestLvmIscsiAbs(TestLvmAbs, TestBasicIscsiAbs):
     interactive = False
+    dirty_disks = True
     iscsi_disks = [
         {'size': '6G'},
         {'size': '5G', 'auth': 'user:passw0rd', 'iauth': 'iuser:ipassw0rd'}]
@@ -20,6 +21,8 @@ class TestLvmIscsiAbs(TestLvmAbs, TestBasicIscsiAbs):
         """
         cd OUTPUT_COLLECT_D
         ls -al /sys/class/block/dm*/slaves/  > dm_slaves
+        cp -a /etc/udev/rules.d udev_rules_d
+        cp -a /etc/iscsi etc_iscsi
         """)]
 
     fstab_expected = {