Merge ~dbungert/curtin:zpool-disable-features into curtin:master

[Dan Bungert]

zpool: add default_features flag support

And a new test type for validating storage configs.

[Dan Bungert (dbungert)]

actually I want to invert this so the field name is default_features, will follow up tomorrow

[Michael Hudson-Doyle (mwhudson)]

Flipping this around to avoid the double negatives sounds like a good thing, this looks ok otherwise though.

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

PASSED: Continuous integration, rev:ca55f021befc7bd580ba43797c7b6aef2fec92ea
https://jenkins.canonical.com/server-team/job/curtin-ci/166/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/166/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/166/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/166/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/166/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/166//rebuild

review: Approve (continuous-integration)

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

PASSED: Continuous integration, rev:04f90a97e9af713db56ef4dd1f3488fbe9752495
https://jenkins.canonical.com/server-team/job/curtin-ci/167/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/167/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/167/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/167/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/167/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/167//rebuild

review: Approve (continuous-integration)

[Dan Bungert (dbungert)]

I'm glad we inverted the name because I was definitely confused while writing it, but I think it's correct now.

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

PASSED: Continuous integration, rev:1cc0ec1eb3c5cc0617d6df7320291f706f1804bd
https://jenkins.canonical.com/server-team/job/curtin-ci/168/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/168/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/168/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/168/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/168/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/168//rebuild

review: Approve (continuous-integration)

[Michael Hudson-Doyle (mwhudson)]

Yeah this is much cleaner.

[Michael Hudson-Doyle (mwhudson)]

review: Approve

diff --git a/curtin/block/schemas.py b/curtin/block/schemas.py
index 514f791..6a5c5b4 100644
--- a/curtin/block/schemas.py
+++ b/curtin/block/schemas.py
@@ -391,6 +391,7 @@ ZPOOL = {
         'pool': {'$ref': '#/definitions/name'},
         'pool_properties': {'$ref': '#/definitions/params'},
         'fs_properties': {'$ref': '#/definitions/params'},
+        'default_features': {'type': 'boolean'},
         'mountpoint': {
             'type': 'string',
             'oneOf': [
diff --git a/curtin/block/zfs.py b/curtin/block/zfs.py
index 25751d3..2d13d6b 100644
--- a/curtin/block/zfs.py
+++ b/curtin/block/zfs.py
@@ -107,6 +107,7 @@ def zfs_assert_supported():
 
 
 def zpool_create(poolname, vdevs, mountpoint=None, altroot=None,
+                 default_features=True,
                  pool_properties=None, zfs_properties=None):
     """
     Create a zpool called <poolname> comprised of devices specified in <vdevs>.
@@ -114,6 +115,7 @@ def zpool_create(poolname, vdevs, mountpoint=None, altroot=None,
     :param poolname: String used to name the pool.
     :param vdevs: An iterable of strings of block devices paths which *should*
                   start with '/dev/disk/by-id/' to follow best practices.
+    :param default_features: If true, keep the default features enabled.
     :param pool_properties: A dictionary of key, value pairs to be passed
                             to `zpool create` with the `-o` flag as properties
                             of the zpool.  If value is None, then
@@ -155,6 +157,9 @@ def zpool_create(poolname, vdevs, mountpoint=None, altroot=None,
     if altroot:
         options.extend(['-R', altroot])
 
+    if not default_features:
+        options.extend(['-d'])
+
     cmd = ["zpool", "create"] + options + [poolname] + vdevs
     util.subp(cmd, capture=True)
 
diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
index 11972b9..dd6992d 100644
--- a/curtin/commands/block_meta.py
+++ b/curtin/commands/block_meta.py
@@ -1979,6 +1979,7 @@ def zpool_handler(info, storage_config, context):
     mountpoint = info.get('mountpoint')
     pool_properties = info.get('pool_properties', {})
     fs_properties = info.get('fs_properties', {})
+    default_features = info.get('default_features', True)
     altroot = state['target']
 
     if not vdevs or not poolname:
@@ -1999,6 +2000,7 @@ def zpool_handler(info, storage_config, context):
     LOG.info('Creating zpool %s with vdevs %s', poolname, vdevs_byid)
     zfs.zpool_create(poolname, vdevs_byid,
                      mountpoint=mountpoint, altroot=altroot,
+                     default_features=default_features,
                      pool_properties=pool_properties,
                      zfs_properties=fs_properties)
 
diff --git a/doc/topics/storage.rst b/doc/topics/storage.rst
index 918f3cd..67657c1 100644
--- a/doc/topics/storage.rst
+++ b/doc/topics/storage.rst
@@ -1121,6 +1121,14 @@ are:
 - canmount: off
 - normalization: formD
 
+**default_features**: *true, false*
+
+If *true*, keep the default features enabled.  For fine-grained control of the
+desired features, set to *false* and enable the desired features with
+pool_properties. This controls the presence or absence of the `-d` flag of
+`zpool create`. Default value is *true*, which means the zpool default feature
+set is used.
+
 **Config Example**::
 
  - type: zpool
diff --git a/tests/unittests/schema/storage/bad/zpool-default-features.yaml b/tests/unittests/schema/storage/bad/zpool-default-features.yaml
new file mode 100644
index 0000000..5f8e74f
--- /dev/null
+++ b/tests/unittests/schema/storage/bad/zpool-default-features.yaml
@@ -0,0 +1,8 @@
+storage:
+  version: 2
+  config:
+    - type: zpool
+      id: zpool-1
+      pool: mypool
+      vdevs: [/dev/sda]
+      default_features: 0
diff --git a/tests/unittests/schema/storage/good/zpool-default-features.yaml b/tests/unittests/schema/storage/good/zpool-default-features.yaml
new file mode 100644
index 0000000..1d0a69d
--- /dev/null
+++ b/tests/unittests/schema/storage/good/zpool-default-features.yaml
@@ -0,0 +1,8 @@
+storage:
+  version: 2
+  config:
+    - type: zpool
+      id: zpool-1
+      pool: mypool
+      vdevs: [/dev/sda]
+      default_features: false
diff --git a/tests/unittests/test_block_zfs.py b/tests/unittests/test_block_zfs.py
index 72cfd3f..2f78863 100644
--- a/tests/unittests/test_block_zfs.py
+++ b/tests/unittests/test_block_zfs.py
@@ -210,6 +210,26 @@ class TestBlockZfsZpoolCreate(CiTestCase):
             self.assertEqual(sorted(expected_args[index]), sorted(args[0]))
             self.assertEqual(expected_kwargs, kwargs)
 
+    def test_zpool_default_features_absent(self):
+        """ when default_features is missing, no -d arg in zpool create """
+        zfs.zpool_create('mypool', ['/dev/disk/by-id/virtio-abcfoo1'])
+        _name, args, _ = self.mock_subp.mock_calls[0]
+        self.assertNotIn('-d', args[0])
+
+    def test_zpool_default_features_true(self):
+        """ when default_features is true, no -d arg in zpool create """
+        zfs.zpool_create('mypool', ['/dev/disk/by-id/virtio-abcfoo1'],
+                         default_features=True)
+        _name, args, _ = self.mock_subp.mock_calls[0]
+        self.assertNotIn('-d', args[0])
+
+    def test_zpool_default_features_false(self):
+        """ when default_features is false, -d arg in zpool create """
+        zfs.zpool_create('mypool', ['/dev/disk/by-id/virtio-abcfoo1'],
+                         default_features=False)
+        _name, args, _ = self.mock_subp.mock_calls[0]
+        self.assertIn('-d', args[0])
+
 
 class TestBlockZfsZfsCreate(CiTestCase):
 
diff --git a/tests/unittests/test_commands_block_meta.py b/tests/unittests/test_commands_block_meta.py
index 6388e0b..87e46eb 100644
--- a/tests/unittests/test_commands_block_meta.py
+++ b/tests/unittests/test_commands_block_meta.py
@@ -933,6 +933,7 @@ class TestZpoolHandler(CiTestCase):
             info['pool'], [disk_path],
             mountpoint="/",
             altroot="mytarget",
+            default_features=True,
             pool_properties={'ashift': 42},
             zfs_properties={'compression': 'lz4'})
 
diff --git a/tests/unittests/test_schemas.py b/tests/unittests/test_schemas.py
new file mode 100644
index 0000000..1422ce6
--- /dev/null
+++ b/tests/unittests/test_schemas.py
@@ -0,0 +1,18 @@
+# This file is part of curtin. See LICENSE file for copyright and license info.
+
+import glob
+import unittest
+
+from parameterized import parameterized
+
+from curtin.commands.schema_validate import schema_validate_storage
+
+
+class TestSchemaValidation(unittest.TestCase):
+    @parameterized.expand(glob.glob("tests/unittests/schema/storage/good/*"))
+    def test_storage_good(self, filename):
+        self.assertEqual(0, schema_validate_storage(filename))
+
+    @parameterized.expand(glob.glob("tests/unittests/schema/storage/bad/*"))
+    def test_storage_bad(self, filename):
+        self.assertEqual(1, schema_validate_storage(filename))