Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zfs: fix multi-line value in user-defined property #6264

Merged
merged 5 commits into from
Feb 10, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/6264-zfs-multiline-property-value.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
bugfixes:
- zfs - fix handling of multi-line values of user-defined ZFS properties (https://github.com/ansible-collections/community.general/pull/6264).
59 changes: 38 additions & 21 deletions plugins/modules/zfs.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,10 @@

class Zfs(object):

def __init__(self, module, name, properties):
def __init__(self, module, name, extra_zfs_properties):
self.module = module
self.name = name
self.properties = properties
self.extra_zfs_properties = extra_zfs_properties
self.changed = False
self.zfs_cmd = module.get_bin_path('zfs', True)
self.zpool_cmd = module.get_bin_path('zpool', True)
Expand Down Expand Up @@ -142,7 +142,7 @@ def create(self):
if self.module.check_mode:
self.changed = True
return
properties = self.properties
extra_zfs_properties = self.extra_zfs_properties
origin = self.module.params.get('origin')
cmd = [self.zfs_cmd]

Expand All @@ -158,8 +158,8 @@ def create(self):
if action in ['create', 'clone']:
cmd += ['-p']

if properties:
for prop, value in properties.items():
if extra_zfs_properties:
for prop, value in extra_zfs_properties.items():
if prop == 'volsize':
cmd += ['-V', value]
elif prop == 'volblocksize':
Expand Down Expand Up @@ -189,45 +189,62 @@ def set_property(self, prop, value):

def set_properties_if_changed(self):
diff = {'before': {'extra_zfs_properties': {}}, 'after': {'extra_zfs_properties': {}}}
current_properties = self.get_current_properties()
for prop, value in self.properties.items():
current_value = current_properties.get(prop, None)
current_properties = self.list_properties()
for prop, value in self.extra_zfs_properties.items():
current_value = self.get_property(prop, current_properties)
if current_value != value:
self.set_property(prop, value)
diff['before']['extra_zfs_properties'][prop] = current_value
diff['after']['extra_zfs_properties'][prop] = value
if self.module.check_mode:
return diff
updated_properties = self.get_current_properties()
for prop in self.properties:
value = updated_properties.get(prop, None)
updated_properties = self.list_properties()
for prop in self.extra_zfs_properties:
value = self.get_property(prop, updated_properties)
if value is None:
self.module.fail_json(msg="zfsprop was not present after being successfully set: %s" % prop)
if current_properties.get(prop, None) != value:
if self.get_property(prop, current_properties) != value:
self.changed = True
if prop in diff['after']['extra_zfs_properties']:
diff['after']['extra_zfs_properties'][prop] = value
return diff

def get_current_properties(self):
cmd = [self.zfs_cmd, 'get', '-H', '-p', '-o', "property,value,source"]
def list_properties(self):
cmd = [self.zfs_cmd, 'get', '-H', '-p', '-o', "property,source"]
if self.enhanced_sharing:
cmd += ['-e']
cmd += ['all', self.name]
rc, out, err = self.module.run_command(cmd)
properties = dict()
properties = []
for line in out.splitlines():
prop, value, source = line.split('\t')
prop, source = line.split('\t')
# include source '-' so that creation-only properties are not removed
# to avoids errors when the dataset already exists and the property is not changed
# this scenario is most likely when the same playbook is run more than once
if source in ('local', 'received', '-'):
properties[prop] = value
properties.append(prop)
return properties

def get_property(self, name, list_of_properties):
# Add alias for enhanced sharing properties
if self.enhanced_sharing:
properties['sharenfs'] = properties.get('share.nfs', None)
properties['sharesmb'] = properties.get('share.smb', None)
return properties
if name == 'sharenfs':
name = 'share.nfs'
elif name == 'sharesmb':
name = 'share.smb'
if name not in list_of_properties:
return None
cmd = [self.zfs_cmd, 'get', '-H', '-p', '-o', "value"]
if self.enhanced_sharing:
cmd += ['-e']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find -e in the zfs documentation for zfs-get, do you know if this is still a supported flag? (I looked at the old releases and didn't see anything either)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option -e is used in ZFS and doesn't seem to be used in OpenZFS. The output of zfs get can group similar properties (e.g. share*) to single line in output. The -e option expands the output so each property is displayed (on own line).

Man page:
https://docs.oracle.com/cd/E88353_01/html/E72487/zfs-8.html

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's worth mentioning in the docs (notes, likely) that OpenZFS might have this (and possibly other) quirks, advising caution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not the right person to comment about OpenZFS. I know that I don't know it or know very little. The original author of zfs.py module apparently knew lot more than me, have a look at functions check_openzfs() and check_enhanced_sharing(). These functions test pool versions and use some internal knowledge to determine if this is OpenZFS or ZFS and also if the zfs get needs '-e' or not. These tricks are beyond my knowledge, but from the information I can dig, the code of check_enhanced_sharing() looks correct to me. Nowadays, due to Ansible raising its Python version requirements, it only runs on pretty recent Solaris 11.4 systems, so the check_enhanced_sharing() could be now as simple as "return self.is_solaris and not self.is_openzfs", but that could be part of some other project or module cleanup.

Regarding the comment in docs or notes, I'm unsure. Do you mean a) "zfs get -e" or b) the multi-line properties?

Ad a) zfs get -e looks like an implementation detail to me, so I think source code comment is appropriate.
If this is in documentation for users, then I would not know if the user really needs to worry about it,
or what he should be supposed to do with the knowledge.

Ad b) perhaps zfs tool man page or their OpenZFS / ZFS docs is better place for the comment?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking a), but yeah, you got a point. Anyways, we can always add docs later if needed. Both ZFS and OpenZFS are not things I know details about, so I won't press the point.

cmd += [name, self.name]
rc, out, err = self.module.run_command(cmd)
if rc != 0:
return None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if zfs-get fails, that's the same as if the property doesn't exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, if zfs get fails, it's the same as if the property doesn't exist. That's how the original code behaves, it ignores the return code (err) from call to run_command(). See:

rc, out, err = self.module.run_command(cmd)

The variable err is ignored.
It doesn't look right. Should the module fail if zfs get fails? E.g.:

self.module.fail_json(msg="command '%s' failed" % " ".join(cmd))

#
# Strip last newline
#
return out[:-1]


def main():
Expand Down Expand Up @@ -282,7 +299,7 @@ def main():
result['diff']['before_header'] = name
result['diff']['after_header'] = name

result.update(zfs.properties)
result.update(zfs.extra_zfs_properties)
result['changed'] = zfs.changed
module.exit_json(**result)

Expand Down