Merge ~raharper/curtin:fix/wipe-zfs-no-utils into curtin:master

[Ryan Harper]

clear-holders: handle missing zpool/zfs tools when wiping

Allow curtin to continue to wipe disks on systems where zfs
kernel module is present but zfsutils-linux is not.

LP: #1782744

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

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

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

review: Approve (continuous-integration)

[Scott Moser (smoser)]

Hm..
So we do have 'zfs_supported' in our zfs module.

It seems like it would make sense to use that.

here is something to add check of the needed utilities to that:
 http://paste.ubuntu.com/p/QMzRrR4D6w/

[Ryan Harper (raharper)]

Updated this with changes from Scott.

Needs a vmtest run.

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

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

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

review: Approve (continuous-integration)

[Scott Moser (smoser)]

We discussed this some in a hangout today.

I think http://paste.ubuntu.com/p/jwRYPtFsrm/ cleans up 'is_zfs_supported' relatively nicely.

what do you think?

review: Needs Information

[Ryan Harper (raharper)]

I think we need some changes to the zfs_supported() which I think you mentioned was intended to be a wrapper around assert_zfs_supported.

IIUC, the design was to allow the:

if zfs.zfs_supported():
   # do zfs stuff here

And that zfs_supported would always return a boolean (it does) but it also
doesn't check anything. I think maybe it could be:

try:
    return zfs.assert_zfs_supported()
except RuntimeError:
    return False

And assert_zfs_supported needs a return True at the bottom.

However. clear-holders wants to attempt to make zfs ready
So it cant call assert_zfs_support without handling the
RuntimeException.

I'd also want to only assert_zfs_support once; there are multiple
calls to zfs_support (or assert) through looping (see wipe superblock)
or any of zfs config with multiple pools or zfs volumes. I'd like to
avoid repeating the checks and attempted module loading since one time
is sufficient to determine if it's going to work.

What about this:

http://paste.ubuntu.com/p/3FyRF49w44/

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

FAILED: Continuous integration, rev:89bdcf9ae463e350c2424e6e5a86f21ec3857033
https://jenkins.ubuntu.com/server/job/curtin-ci/1006/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/1006/console
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/1006/console
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/1006/console
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=torkoal/1006/console

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

review: Needs Fixing (continuous-integration)

[Ryan Harper (raharper)]

vmtest run going here:

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

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

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

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

review: Approve (continuous-integration)

[Scott Moser (smoser)]

I don't love the caching of zfs_supported.
the primary reason is that zfs_supported might get set to False.
and then 'apt-get install -qy zfslinux'
and then it would stay false.

I guess its not specifically a problem here now as curtin
never does an install of zfsutils-linux, but if it did
then we could effecitvely cache the wrong value.

that could happen though with a curtin command
executed through user provided config.

Performance wise, this is what you're saving with the cache:
$ python3 -m timeit --setup="from curtin.block import zfs" "zfs.assert_zfs_supported()"
1000 loops, best of 3: 238 usec per loop

So less than 1/1000 of a second for that call.
I'm fine with caching it, but balance the desire to do so with the potential of a stale cache.

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

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

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

review: Approve (continuous-integration)

[Scott Moser (smoser)]

I have one nit inline... You can take it or leave it.

review: Approve

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

FAILED: Autolanding.
More details in the following jenkins job:
https://jenkins.ubuntu.com/server/job/curtin-autoland-test/20/
Executed test runs:
    None: https://jenkins.ubuntu.com/server/job/admin-lp-git-autoland/58/console

review: Needs Fixing (continuous-integration)

[Scott Moser (smoser)]

this merge-conflict.
can you re-base and then set back to approve?

https://jenkins.ubuntu.com/server/job/curtin-autoland-test/20/ says:

CONFLICT (content): Merge conflict in tests/unittests/test_clear_holders.py

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

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

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

review: Approve (continuous-integration)

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

FAILED: Autolanding.
More details in the following jenkins job:
https://jenkins.ubuntu.com/server/job/curtin-autoland-test/21/
Executed test runs:
    None: https://jenkins.ubuntu.com/server/job/admin-lp-git-autoland/61/console

review: Needs Fixing (continuous-integration)

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

review: Approve (continuous-integration)

diff --git a/curtin/block/__init__.py b/curtin/block/__init__.py
index b49b9d3..b771629 100644
--- a/curtin/block/__init__.py
+++ b/curtin/block/__init__.py
@@ -1074,4 +1074,18 @@ def detect_required_packages_mapping():
     }
     return mapping
 
+
+def get_supported_filesystems():
+    """ Return a list of filesystems that the kernel currently supports
+        as read from /proc/filesystems.
+
+        Raises RuntimeError if /proc/filesystems does not exist.
+    """
+    proc_fs = "/proc/filesystems"
+    if not os.path.exists(proc_fs):
+        raise RuntimeError("Unable to read 'filesystems' from %s" % proc_fs)
+
+    return [l.split('\t')[1].strip()
+            for l in util.load_file(proc_fs).splitlines()]
+
 # vi: ts=4 expandtab syntax=python
diff --git a/curtin/block/clear_holders.py b/curtin/block/clear_holders.py
index a2042d5..a05c9ca 100644
--- a/curtin/block/clear_holders.py
+++ b/curtin/block/clear_holders.py
@@ -304,11 +304,14 @@ def wipe_superblock(device):
     partitions = block.get_sysfs_partitions(device)
 
     # release zfs member by exporting the pool
-    if block.is_zfs_member(blockdev):
+    if zfs.zfs_supported() and 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)
+            try:
+                zfs.zpool_export(poolname)
+            except util.ProcessExecutionError as e:
+                LOG.warning('Failed to export zpool "%s": %s', poolname, e)
 
     if is_swap_device(blockdev):
         shutdown_swap(blockdev)
@@ -633,13 +636,9 @@ def start_clear_holders_deps():
     # happens then there is no need to halt installation, as the bcache devices
     # will never appear and will never prevent the disk from being reformatted
     util.load_kernel_module('bcache')
-    # the zfs module is needed to find and export devices which may be in-use
-    # and need to be cleared, only on xenial+.
-    try:
-        if zfs.zfs_supported():
-            util.load_kernel_module('zfs')
-    except RuntimeError as e:
-        LOG.warning('Failed to load zfs kernel module: %s', e)
+
+    if not zfs.zfs_supported():
+        LOG.warning('zfs filesystem is not supported in this environment')
 
 
 # anything that is not identified can assumed to be a 'disk' or similar
diff --git a/curtin/block/zfs.py b/curtin/block/zfs.py
index cfb07a9..e279ab6 100644
--- a/curtin/block/zfs.py
+++ b/curtin/block/zfs.py
@@ -8,7 +8,7 @@ import os
 
 from curtin.config import merge_config
 from curtin import util
-from . import blkid
+from . import blkid, get_supported_filesystems
 
 ZPOOL_DEFAULT_PROPERTIES = {
     'ashift': 12,
@@ -73,6 +73,15 @@ def _join_pool_volume(poolname, volume):
 
 
 def zfs_supported():
+    """Return a boolean indicating if zfs is supported."""
+    try:
+        zfs_assert_supported()
+        return True
+    except RuntimeError:
+        return False
+
+
+def zfs_assert_supported():
     """ Determine if the runtime system supports zfs.
     returns: True if system supports zfs
     raises: RuntimeError: if system does not support zfs
@@ -85,13 +94,15 @@ def zfs_supported():
     if release in ZFS_UNSUPPORTED_RELEASES:
         raise RuntimeError("zfs is not supported on release: %s" % release)
 
-    try:
-        util.subp(['modinfo', 'zfs'], capture=True)
-    except util.ProcessExecutionError as err:
-        if err.stderr.startswith("modinfo: ERROR: Module zfs not found."):
-            raise RuntimeError("zfs kernel module is not available: %s" % err)
+    if 'zfs' not in get_supported_filesystems():
+        try:
+            util.load_kernel_module('zfs')
+        except util.ProcessExecutionError as err:
+            raise RuntimeError("Failed to load 'zfs' kernel module: %s" % err)
 
-    return True
+    missing_progs = [p for p in ('zpool', 'zfs') if not util.which(p)]
+    if missing_progs:
+        raise RuntimeError("Missing zfs utils: %s" % ','.join(missing_progs))
 
 
 def zpool_create(poolname, vdevs, mountpoint=None, altroot=None,
diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
index 63193f5..6bd430d 100644
--- a/curtin/commands/block_meta.py
+++ b/curtin/commands/block_meta.py
@@ -1264,7 +1264,7 @@ def zpool_handler(info, storage_config):
     """
     Create a zpool based in storage_configuration
     """
-    zfs.zfs_supported()
+    zfs.zfs_assert_supported()
 
     state = util.load_command_environment()
 
@@ -1299,7 +1299,8 @@ def zfs_handler(info, storage_config):
     """
     Create a zfs filesystem
     """
-    zfs.zfs_supported()
+    zfs.zfs_assert_supported()
+
     state = util.load_command_environment()
     poolname = get_poolname(info, storage_config)
     volume = info.get('volume')
diff --git a/tests/unittests/test_block.py b/tests/unittests/test_block.py
index d9b19a4..9cf8383 100644
--- a/tests/unittests/test_block.py
+++ b/tests/unittests/test_block.py
@@ -647,4 +647,39 @@ class TestSlaveKnames(CiTestCase):
         knames = block.get_device_slave_knames(device)
         self.assertEqual(slaves, knames)
 
+
+class TestGetSupportedFilesystems(CiTestCase):
+
+    supported_filesystems = ['sysfs', 'rootfs', 'ramfs', 'ext4']
+
+    def _proc_filesystems_output(self, supported=None):
+        if not supported:
+            supported = self.supported_filesystems
+
+        def devname(fsname):
+            """ in-use filesystem modules not emit the 'nodev' prefix """
+            return '\t' if fsname.startswith('ext') else 'nodev\t'
+
+        return '\n'.join([devname(fs) + fs for fs in supported]) + '\n'
+
+    @mock.patch('curtin.block.util')
+    @mock.patch('curtin.block.os')
+    def test_get_supported_filesystems(self, mock_os, mock_util):
+        """ test parsing /proc/filesystems contents into a filesystem list"""
+        mock_os.path.exists.return_value = True
+        mock_util.load_file.return_value = self._proc_filesystems_output()
+
+        result = block.get_supported_filesystems()
+        self.assertEqual(sorted(self.supported_filesystems), sorted(result))
+
+    @mock.patch('curtin.block.util')
+    @mock.patch('curtin.block.os')
+    def test_get_supported_filesystems_no_proc_path(self, mock_os, mock_util):
+        """ missing /proc/filesystems raises RuntimeError """
+        mock_os.path.exists.return_value = False
+        with self.assertRaises(RuntimeError):
+            block.get_supported_filesystems()
+        self.assertEqual(0, mock_util.load_file.call_count)
+
+
 # vi: ts=4 expandtab syntax=python
diff --git a/tests/unittests/test_block_zfs.py b/tests/unittests/test_block_zfs.py
index c61a6da..ca8f118 100644
--- a/tests/unittests/test_block_zfs.py
+++ b/tests/unittests/test_block_zfs.py
@@ -378,10 +378,10 @@ class TestBlockZfsDeviceToPoolname(CiTestCase):
         self.mock_blkid.assert_called_with(devs=[devname])
 
 
-class TestBlockZfsZfsSupported(CiTestCase):
+class TestBlockZfsAssertZfsSupported(CiTestCase):
 
     def setUp(self):
-        super(TestBlockZfsZfsSupported, self).setUp()
+        super(TestBlockZfsAssertZfsSupported, self).setUp()
         self.add_patch('curtin.block.zfs.util.subp', 'mock_subp')
         self.add_patch('curtin.block.zfs.util.get_platform_arch', 'mock_arch')
         self.add_patch('curtin.block.zfs.util.lsb_release', 'mock_release')
@@ -394,34 +394,41 @@ class TestBlockZfsZfsSupported(CiTestCase):
     def test_unsupported_arch(self):
         self.mock_arch.return_value = 'i386'
         with self.assertRaises(RuntimeError):
-            zfs.zfs_supported()
+            zfs.zfs_assert_supported()
 
     def test_unsupported_releases(self):
         for rel in ['precise', 'trusty']:
             self.mock_release.return_value = {'codename': rel}
             with self.assertRaises(RuntimeError):
-                zfs.zfs_supported()
+                zfs.zfs_assert_supported()
 
-    def test_missing_module(self):
-        missing = 'modinfo: ERROR: Module zfs not found.\n     '
+    @mock.patch('curtin.block.zfs.util.is_kmod_loaded')
+    @mock.patch('curtin.block.zfs.get_supported_filesystems')
+    def test_missing_module(self, mock_supfs, mock_kmod):
+        missing = 'modprobe: FATAL: Module zfs not found.\n     '
         self.mock_subp.side_effect = ProcessExecutionError(stdout='',
                                                            stderr=missing,
                                                            exit_code='1')
+        mock_supfs.return_value = ['ext4']
+        mock_kmod.return_value = False
         with self.assertRaises(RuntimeError):
-            zfs.zfs_supported()
+            zfs.zfs_assert_supported()
 
 
-class TestZfsSupported(CiTestCase):
+class TestAssertZfsSupported(CiTestCase):
 
     def setUp(self):
-        super(TestZfsSupported, self).setUp()
+        super(TestAssertZfsSupported, self).setUp()
 
+    @mock.patch('curtin.block.zfs.get_supported_filesystems')
     @mock.patch('curtin.block.zfs.util')
-    def test_zfs_supported_returns_true(self, mock_util):
-        """zfs_supported returns True on supported platforms"""
+    def test_zfs_assert_supported_returns_true(self, mock_util, mock_supfs):
+        """zfs_assert_supported returns True on supported platforms"""
         mock_util.get_platform_arch.return_value = 'amd64'
         mock_util.lsb_release.return_value = {'codename': 'bionic'}
         mock_util.subp.return_value = ("", "")
+        mock_supfs.return_value = ['zfs']
+        mock_util.which.side_effect = iter(['/wark/zpool', '/wark/zfs'])
 
         self.assertNotIn(mock_util.get_platform_arch.return_value,
                          zfs.ZFS_UNSUPPORTED_ARCHES)
@@ -430,45 +437,94 @@ class TestZfsSupported(CiTestCase):
         self.assertTrue(zfs.zfs_supported())
 
     @mock.patch('curtin.block.zfs.util')
-    def test_zfs_supported_raises_exception_on_bad_arch(self, mock_util):
-        """zfs_supported raises RuntimeError on unspported arches"""
+    def test_zfs_assert_supported_raises_exception_on_bad_arch(self,
+                                                               mock_util):
+        """zfs_assert_supported raises RuntimeError on unspported arches"""
         mock_util.lsb_release.return_value = {'codename': 'bionic'}
         mock_util.subp.return_value = ("", "")
         for arch in zfs.ZFS_UNSUPPORTED_ARCHES:
             mock_util.get_platform_arch.return_value = arch
             with self.assertRaises(RuntimeError):
-                zfs.zfs_supported()
+                zfs.zfs_assert_supported()
 
     @mock.patch('curtin.block.zfs.util')
-    def test_zfs_supported_raises_execption_on_bad_releases(self, mock_util):
-        """zfs_supported raises RuntimeError on unspported releases"""
+    def test_zfs_assert_supported_raises_exc_on_bad_releases(self, mock_util):
+        """zfs_assert_supported raises RuntimeError on unspported releases"""
         mock_util.get_platform_arch.return_value = 'amd64'
         mock_util.subp.return_value = ("", "")
         for release in zfs.ZFS_UNSUPPORTED_RELEASES:
             mock_util.lsb_release.return_value = {'codename': release}
             with self.assertRaises(RuntimeError):
-                zfs.zfs_supported()
+                zfs.zfs_assert_supported()
 
     @mock.patch('curtin.block.zfs.util.subprocess.Popen')
+    @mock.patch('curtin.block.zfs.util.is_kmod_loaded')
+    @mock.patch('curtin.block.zfs.get_supported_filesystems')
     @mock.patch('curtin.block.zfs.util.lsb_release')
     @mock.patch('curtin.block.zfs.util.get_platform_arch')
-    def test_zfs_supported_raises_exception_on_missing_module(self,
-                                                              m_arch,
-                                                              m_release,
-                                                              m_popen):
-        """zfs_supported raises RuntimeError on missing zfs module"""
+    def test_zfs_assert_supported_raises_exc_on_missing_module(self,
+                                                               m_arch,
+                                                               m_release,
+                                                               m_supfs,
+                                                               m_kmod,
+                                                               m_popen,
+                                                               ):
+        """zfs_assert_supported raises RuntimeError modprobe zfs error"""
 
         m_arch.return_value = 'amd64'
         m_release.return_value = {'codename': 'bionic'}
+        m_supfs.return_value = ['ext4']
+        m_kmod.return_value = False
         process_mock = mock.Mock()
         attrs = {
             'returncode': 1,
             'communicate.return_value':
-                ('output', "modinfo: ERROR: Module zfs not found."),
+                ('output', 'modprobe: FATAL: Module zfs not found ...'),
         }
         process_mock.configure_mock(**attrs)
         m_popen.return_value = process_mock
         with self.assertRaises(RuntimeError):
-            zfs.zfs_supported()
+            zfs.zfs_assert_supported()
+
+    @mock.patch('curtin.block.zfs.get_supported_filesystems')
+    @mock.patch('curtin.block.zfs.util.lsb_release')
+    @mock.patch('curtin.block.zfs.util.get_platform_arch')
+    @mock.patch('curtin.block.zfs.util')
+    def test_zfs_assert_supported_raises_exc_on_missing_binaries(self,
+                                                                 mock_util,
+                                                                 m_arch,
+                                                                 m_release,
+                                                                 m_supfs):
+        """zfs_assert_supported raises RuntimeError if no zpool or zfs tools"""
+        mock_util.get_platform_arch.return_value = 'amd64'
+        mock_util.lsb_release.return_value = {'codename': 'bionic'}
+        mock_util.subp.return_value = ("", "")
+        m_supfs.return_value = ['zfs']
+        mock_util.which.return_value = None
+
+        with self.assertRaises(RuntimeError):
+            zfs.zfs_assert_supported()
+
+
+class TestZfsSupported(CiTestCase):
+
+    @mock.patch('curtin.block.zfs.zfs_assert_supported')
+    def test_zfs_supported(self, m_assert_zfs):
+        zfs_supported = True
+        m_assert_zfs.return_value = zfs_supported
+
+        result = zfs.zfs_supported()
+        self.assertEqual(zfs_supported, result)
+        self.assertEqual(1, m_assert_zfs.call_count)
+
+    @mock.patch('curtin.block.zfs.zfs_assert_supported')
+    def test_zfs_supported_returns_false_on_assert_fail(self, m_assert_zfs):
+        zfs_supported = False
+        m_assert_zfs.side_effect = RuntimeError('No zfs module')
+
+        result = zfs.zfs_supported()
+        self.assertEqual(zfs_supported, result)
+        self.assertEqual(1, m_assert_zfs.call_count)
+
 
 # vi: ts=4 expandtab syntax=python
diff --git a/tests/unittests/test_clear_holders.py b/tests/unittests/test_clear_holders.py
index 21f76be..d3f80a0 100644
--- a/tests/unittests/test_clear_holders.py
+++ b/tests/unittests/test_clear_holders.py
@@ -6,6 +6,7 @@ import os
 import textwrap
 
 from curtin.block import clear_holders
+from curtin.util import ProcessExecutionError
 from .helpers import CiTestCase
 
 
@@ -558,6 +559,7 @@ class TestClearHolders(CiTestCase):
         self.assertFalse(mock_block.wipe_volume.called)
         mock_block.is_extended_partition.return_value = False
         mock_block.is_zfs_member.return_value = True
+        mock_zfs.zfs_supported.return_value = True
         mock_zfs.device_to_poolname.return_value = 'fake_pool'
         mock_zfs.zpool_list.return_value = ['fake_pool']
         clear_holders.wipe_superblock(self.test_syspath)
@@ -567,6 +569,58 @@ class TestClearHolders(CiTestCase):
             self.test_blockdev, exclusive=True, mode='superblock')
 
     @mock.patch('curtin.block.clear_holders.is_swap_device')
+    @mock.patch('curtin.block.clear_holders.zfs')
+    @mock.patch('curtin.block.clear_holders.LOG')
+    @mock.patch('curtin.block.clear_holders.block')
+    def test_clear_holders_wipe_superblock_no_zfs(self, mock_block, mock_log,
+                                                  mock_zfs, mock_swap):
+        """test clear_holders.wipe_superblock checks zfs supported"""
+        mock_swap.return_value = False
+        mock_block.sysfs_to_devpath.return_value = self.test_blockdev
+        mock_block.is_extended_partition.return_value = True
+        clear_holders.wipe_superblock(self.test_syspath)
+        self.assertFalse(mock_block.wipe_volume.called)
+        mock_block.is_extended_partition.return_value = False
+        mock_block.is_zfs_member.return_value = True
+        mock_zfs.zfs_supported.return_value = False
+        clear_holders.wipe_superblock(self.test_syspath)
+        mock_block.sysfs_to_devpath.assert_called_with(self.test_syspath)
+        self.assertEqual(1, mock_zfs.zfs_supported.call_count)
+        self.assertEqual(0, mock_block.is_zfs_member.call_count)
+        self.assertEqual(0, mock_zfs.device_to_poolname.call_count)
+        self.assertEqual(0, mock_zfs.zpool_list.call_count)
+        mock_block.wipe_volume.assert_called_with(
+            self.test_blockdev, exclusive=True, mode='superblock')
+
+    @mock.patch('curtin.block.clear_holders.is_swap_device')
+    @mock.patch('curtin.block.clear_holders.zfs')
+    @mock.patch('curtin.block.clear_holders.LOG')
+    @mock.patch('curtin.block.clear_holders.block')
+    def test_clear_holders_wipe_superblock_zfs_no_utils(self, mock_block,
+                                                        mock_log, mock_zfs,
+                                                        mock_swap):
+        """test clear_holders.wipe_superblock handles missing zpool cmd"""
+        mock_swap.return_value = False
+        mock_block.sysfs_to_devpath.return_value = self.test_blockdev
+        mock_block.is_extended_partition.return_value = True
+        clear_holders.wipe_superblock(self.test_syspath)
+        self.assertFalse(mock_block.wipe_volume.called)
+        mock_block.is_extended_partition.return_value = False
+        mock_block.is_zfs_member.return_value = True
+        mock_zfs.zfs_supported.return_value = True
+        mock_zfs.device_to_poolname.return_value = 'fake_pool'
+        mock_zfs.zpool_list.return_value = ['fake_pool']
+        mock_zfs.zpool_export.side_effect = [
+            ProcessExecutionError(cmd=['zpool', 'export', 'fake_pool'],
+                                  stdout="",
+                                  stderr=("cannot open 'fake_pool': "
+                                          "no such pool"))]
+        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')
+
+    @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')
@@ -790,7 +844,7 @@ class TestClearHolders(CiTestCase):
         mock_mdadm.mdadm_assemble.assert_called_with(
             scan=True, ignore_errors=True)
         mock_util.load_kernel_module.assert_has_calls([
-                mock.call('bcache'), mock.call('zfs')])
+                mock.call('bcache')])
 
     @mock.patch('curtin.block.clear_holders.lvm')
     @mock.patch('curtin.block.clear_holders.zfs')