Changes how zfs properties are handled

This moves the validation of properties to the zfs command itself. The
properties and their choices were not really correct anyway due to
differences between OpenZFS and Solaris/ZFS.

lib/ansible/modules/extras/system/zfs.py

@@ -24,7 +24,7 @@ DOCUMENTATION = '''
 module: zfs
 short_description: Manage zfs
 description:
-  - Manages ZFS file systems on Solaris and FreeBSD. Can manage file systems, volumes and snapshots. See zfs(1M) for more information about the properties.
+  - Manages ZFS file systems, volumes, clones and snapshots.
 version_added: "1.1"
 options:
   name:
@@ -34,183 +34,21 @@ options:
   state:
     description:
       - Whether to create (C(present)), or remove (C(absent)) a file system, snapshot or volume.
+    choices: ['present', 'absent']
     required: true
-    choices: [present, absent]
-  aclinherit:
-    description:
-      - The aclinherit property.
-    required: False
-    choices: [discard,noallow,restricted,passthrough,passthrough-x]
-  aclmode:
-    description:
-      - The aclmode property.
-    required: False
-    choices: [discard,groupmask,passthrough]
-  atime:
-    description:
-      - The atime property.
-    required: False
-    choices: ['on','off']
-  canmount:
-    description:
-      - The canmount property.
-    required: False
-    choices: ['on','off','noauto']
-  casesensitivity:
-    description:
-      - The casesensitivity property.
-    required: False
-    choices: [sensitive,insensitive,mixed]
-  checksum:
-    description:
-      - The checksum property.
-    required: False
-    choices: ['on','off',fletcher2,fletcher4,sha256]
-  compression:
-    description:
-      - The compression property.
-    required: False
-    choices: ['on','off',lzjb,gzip,gzip-1,gzip-2,gzip-3,gzip-4,gzip-5,gzip-6,gzip-7,gzip-8,gzip-9,lz4,zle]
-  copies:
-    description:
-      - The copies property.
-    required: False
-    choices: [1,2,3]
-  dedup:
-    description:
-      - The dedup property.
-    required: False
-    choices: ['on','off']
-  devices:
-    description:
-      - The devices property.
-    required: False
-    choices: ['on','off']
-  exec:
-    description:
-      - The exec property.
-    required: False
-    choices: ['on','off']
-  jailed:
-    description:
-      - The jailed property.
-    required: False
-    choices: ['on','off']
-  logbias:
-    description:
-      - The logbias property.
-    required: False
-    choices: [latency,throughput]
-  mountpoint:
-    description:
-      - The mountpoint property.
-    required: False
-  nbmand:
-    description:
-      - The nbmand property.
-    required: False
-    choices: ['on','off']
-  normalization:
-    description:
-      - The normalization property.
-    required: False
-    choices: [none,formC,formD,formKC,formKD]
   origin:
     description:
-      - Name of the snapshot to clone
+      - Snapshot from which to create a clone
-    required: False
+    required: false
-    version_added: "2.0"
+  createparent:
-  primarycache:
     description:
-      - The primarycache property.
+      - Creates all non-existing parent file systems.
-    required: False
+    required: false
-    choices: [all,none,metadata]
+    default: "on"
-  quota:
+  key_value:
     description:
-      - The quota property.
+      - The C(zfs) module takes key=value pairs for zfs properties to be set. See the zfs(8) man page for more information.
-    required: False
+
-  readonly:
-    description:
-      - The readonly property.
-    required: False
-    choices: ['on','off']
-  recordsize:
-    description:
-      - The recordsize property.
-    required: False
-  refquota:
-    description:
-      - The refquota property.
-    required: False
-  refreservation:
-    description:
-      - The refreservation property.
-    required: False
-  reservation:
-    description:
-      - The reservation property.
-    required: False
-  secondarycache:
-    description:
-      - The secondarycache property.
-    required: False
-    choices: [all,none,metadata]
-  setuid:
-    description:
-      - The setuid property.
-    required: False
-    choices: ['on','off']
-  shareiscsi:
-    description:
-      - The shareiscsi property.
-    required: False
-    choices: ['on','off']
-  sharenfs:
-    description:
-      - The sharenfs property.
-    required: False
-  sharesmb:
-    description:
-      - The sharesmb property.
-    required: False
-  snapdir:
-    description:
-      - The snapdir property.
-    required: False
-    choices: [hidden,visible]
-  sync:
-    description:
-      - The sync property.
-    required: False
-    choices: ['standard','always','disabled']
-  utf8only:
-    description:
-      - The utf8only property.
-    required: False
-    choices: ['on','off']
-  volsize:
-    description:
-      - The volsize property.
-    required: False
-  volblocksize:
-    description:
-      - The volblocksize property.
-    required: False
-  vscan:
-    description:
-      - The vscan property.
-    required: False
-    choices: ['on','off']
-  xattr:
-    description:
-      - The xattr property.
-    required: False
-    choices: ['on','off']
-  zoned:
-    description:
-      - The zoned property.
-    required: False
-    choices: ['on','off']
 author: "Johan Wiren (@johanwiren)"
 '''
 
@@ -237,20 +75,33 @@ EXAMPLES = '''
 
 import os
 
+
 class Zfs(object):
+
     def __init__(self, module, name, properties):
         self.module = module
         self.name = name
         self.properties = properties
         self.changed = False
+        self.is_solaris = os.uname()[0] == 'SunOS'
+        self.pool = name.split('/')[0]
+        self.zfs_cmd = module.get_bin_path('zfs', True)
+        self.zpool_cmd = module.get_bin_path('zpool', True)
+        self.enhanced_sharing = self.check_enhanced_sharing()
 
-        self.immutable_properties = [ 'casesensitivity', 'normalization', 'utf8only' ]
+    def check_enhanced_sharing(self):
+        if os.uname()[0] == 'SunOS':
+            cmd = [self.zpool_cmd]
+            cmd.extend(['get', 'version'])
+            cmd.append(self.pool)
+            (rc, out, err) = self.module.run_command(cmd, check_rc=True)
+            version = out.splitlines()[-1].split()[2]
+            if int(version) >= 34:
+                return True
+        return False
 
     def exists(self):
-        cmd = [self.module.get_bin_path('zfs', True)]
+        cmd = [self.zfs_cmd, 'list', '-t', 'all', self.name]
-        cmd.append('list')
-        cmd.append('-t all')
-        cmd.append(self.name)
         (rc, out, err) = self.module.run_command(' '.join(cmd))
         if rc == 0:
             return True
@@ -265,7 +116,9 @@ class Zfs(object):
         volsize = properties.pop('volsize', None)
         volblocksize = properties.pop('volblocksize', None)
         origin = properties.pop('origin', None)
-        createparent = properties.pop('createparent', None)
+        createparent = self.module.params.get('createparent')
+        cmd = [self.zfs_cmd]
+
         if "@" in self.name:
             action = 'snapshot'
         elif origin:
@@ -273,135 +126,83 @@ class Zfs(object):
         else:
             action = 'create'
 
-        cmd = [self.module.get_bin_path('zfs', True)]
         cmd.append(action)
 
+        if action in ['create', 'clone']:
             if createparent:
-            cmd.append('-p')
+                cmd += ['-p']
 
+        if volsize:
+            cmd += ['-V', volsize]
         if volblocksize:
-            cmd.append('-b %s' % volblocksize)
+            cmd += ['-b', 'volblocksize']
         if properties:
             for prop, value in properties.iteritems():
-                cmd.append('-o %s="%s"' % (prop, value))
+                cmd += ['-o', '%s="%s"' % (prop, value)]
-        if volsize:
-            cmd.append('-V')
-            cmd.append(volsize)
         if origin:
             cmd.append(origin)
         cmd.append(self.name)
-        (rc, err, out) = self.module.run_command(' '.join(cmd))
+        (rc, out, err) = self.module.run_command(' '.join(cmd))
         if rc == 0:
             self.changed = True
         else:
-            self.module.fail_json(msg=out)
+            self.module.fail_json(msg=err)
 
     def destroy(self):
         if self.module.check_mode:
             self.changed = True
             return
-        cmd = [self.module.get_bin_path('zfs', True)]
+        cmd = [self.zfs_cmd, 'destroy', '-R', self.name]
-        cmd.append('destroy')
+        (rc, out, err) = self.module.run_command(' '.join(cmd))
-        cmd.append(self.name)
-        (rc, err, out) = self.module.run_command(' '.join(cmd))
         if rc == 0:
             self.changed = True
         else:
-            self.module.fail_json(msg=out)
+            self.module.fail_json(msg=err)
 
     def set_property(self, prop, value):
         if self.module.check_mode:
             self.changed = True
             return
-        cmd = self.module.get_bin_path('zfs', True)
+        cmd = [self.zfs_cmd, 'set', prop + '=' + str(value), self.name]
-        args = [cmd, 'set', prop + '=' + value, self.name]
+        (rc, out, err) = self.module.run_command(cmd)
-        (rc, err, out) = self.module.run_command(args)
         if rc == 0:
             self.changed = True
         else:
-            self.module.fail_json(msg=out)
+            self.module.fail_json(msg=err)
 
     def set_properties_if_changed(self):
         current_properties = self.get_current_properties()
         for prop, value in self.properties.iteritems():
+            if prop not in current_properties:
+                self.module.fail_json(msg="invalid property '%s'" % prop)
             if current_properties[prop] != value:
-                if prop in self.immutable_properties:
-                    self.module.fail_json(msg='Cannot change property %s after creation.' % prop)
-                else:
                 self.set_property(prop, value)
 
     def get_current_properties(self):
-        def get_properties_by_name(propname):
+        cmd = [self.zfs_cmd, 'get', '-H']
-            cmd = [self.module.get_bin_path('zfs', True)]
+        if self.enhanced_sharing:
-            cmd += ['get', '-H', propname, self.name]
+            cmd += ['-e']
-            rc, out, err = self.module.run_command(cmd)
+        cmd += ['all', self.name]
-            return [l.split('\t')[1:3] for l in out.splitlines()]
+        rc, out, err = self.module.run_command(" ".join(cmd))
-        properties = dict(get_properties_by_name('all'))
+        properties = dict()
-        if 'share.*' in properties:
+        for p, v in [l.split('\t')[1:3] for l in out.splitlines()]:
-            # Some ZFS pools list the sharenfs and sharesmb properties
+            properties[p] = v
-            # hierarchically as share.nfs and share.smb respectively.
+        # Add alias for enhanced sharing properties
-            del properties['share.*']
+        properties['sharenfs'] = properties.get('share.nfs', None)
-            for p, v in get_properties_by_name('share.all'):
+        properties['sharesmb'] = properties.get('share.smb', None)
-                alias = p.replace('.', '')  # share.nfs -> sharenfs (etc)
-                properties[alias] = v
         return properties
 
-    def run_command(self, cmd):
-        progname = cmd[0]
-        cmd[0] = module.get_bin_path(progname, True)
-        return module.run_command(cmd)
 
 def main():
 
-    # FIXME: should use dict() constructor like other modules, required=False is default
     module = AnsibleModule(
-        argument_spec = {
+        argument_spec = dict(
-            'name':            {'required': True},
+            name =         dict(type='str', required=True),
-            'state':           {'required': True,  'choices':['present', 'absent']},
+            state =        dict(type='str', required=True, choices=['present', 'absent']),
-            'aclinherit':      {'required': False, 'choices':['discard', 'noallow', 'restricted', 'passthrough', 'passthrough-x']},
+            createparent = dict(type='bool', required=False, default=True),
-            'aclmode':         {'required': False, 'choices':['discard', 'groupmask', 'passthrough']},
+            ),
-            'atime':           {'required': False, 'choices':['on', 'off']},
+        supports_check_mode=True,
-            'canmount':        {'required': False, 'choices':['on', 'off', 'noauto']},
+        check_invalid_arguments=False
-            'casesensitivity': {'required': False, 'choices':['sensitive', 'insensitive', 'mixed']},
-            'checksum':        {'required': False, 'choices':['on', 'off', 'fletcher2', 'fletcher4', 'sha256']},
-            'compression':     {'required': False, 'choices':['on', 'off', 'lzjb', 'gzip', 'gzip-1', 'gzip-2', 'gzip-3', 'gzip-4', 'gzip-5', 'gzip-6', 'gzip-7', 'gzip-8', 'gzip-9', 'lz4', 'zle']},
-            'copies':          {'required': False, 'choices':['1', '2', '3']},
-            'createparent':    {'required': False, 'choices':['on', 'off']},
-            'dedup':           {'required': False, 'choices':['on', 'off']},
-            'devices':         {'required': False, 'choices':['on', 'off']},
-            'exec':            {'required': False, 'choices':['on', 'off']},
-            # Not supported
-            #'groupquota':      {'required': False},
-            'jailed':          {'required': False, 'choices':['on', 'off']},
-            'logbias':         {'required': False, 'choices':['latency', 'throughput']},
-            'mountpoint':      {'required': False},
-            'nbmand':          {'required': False, 'choices':['on', 'off']},
-            'normalization':   {'required': False, 'choices':['none', 'formC', 'formD', 'formKC', 'formKD']},
-            'origin':          {'required': False},
-            'primarycache':    {'required': False, 'choices':['all', 'none', 'metadata']},
-            'quota':           {'required': False},
-            'readonly':        {'required': False, 'choices':['on', 'off']},
-            'recordsize':      {'required': False},
-            'refquota':        {'required': False},
-            'refreservation':  {'required': False},
-            'reservation':     {'required': False},
-            'secondarycache':  {'required': False, 'choices':['all', 'none', 'metadata']},
-            'setuid':          {'required': False, 'choices':['on', 'off']},
-            'shareiscsi':      {'required': False, 'choices':['on', 'off']},
-            'sharenfs':        {'required': False},
-            'sharesmb':        {'required': False},
-            'snapdir':         {'required': False, 'choices':['hidden', 'visible']},
-            'sync':            {'required': False, 'choices':['standard', 'always', 'disabled']},
-            # Not supported
-            #'userquota':       {'required': False},
-            'utf8only':        {'required': False, 'choices':['on', 'off']},
-            'volsize':         {'required': False},
-            'volblocksize':    {'required': False},
-            'vscan':           {'required': False, 'choices':['on', 'off']},
-            'xattr':           {'required': False, 'choices':['on', 'off']},
-            'zoned':           {'required': False, 'choices':['on', 'off']},
-            },
-        supports_check_mode=True
         )
 
     state = module.params.pop('state')
@@ -410,9 +211,15 @@ def main():
     # Get all valid zfs-properties
     properties = dict()
     for prop, value in module.params.iteritems():
-        if prop in ['CHECKMODE']:
+        # All freestyle params are zfs properties
-            continue
+        if prop not in module.argument_spec:
-        if value:
+            # Reverse the boolification of freestyle zfs properties
+            if type(value) == bool:
+                if value is True:
+                    properties[prop] = 'on'
+                else:
+                    properties[prop] = 'off'
+            else:
                 properties[prop] = value
 
     result = {}