From afc620fc7465f4ce3f7f1fb98f761e756fe0cf11 Mon Sep 17 00:00:00 2001 From: "patchback[bot]" <45432694+patchback[bot]@users.noreply.github.com> Date: Mon, 10 May 2021 18:16:49 +0200 Subject: [PATCH] Avoid incorrectly marking zfs tasks as changed (#2454) (#2483) * Avoid incorrectly marking zfs tasks as changed The zfs module will incorrectly mark certain tasks as having been changed. For example, if a dataset has a quota of "1G" and the user changes it to "1024M", the actual quota vale has not changed, but since the module is doing a simple string comparison between "1G" and "1024M", it marks the step as "changed". Instead of trying to handle all the corner cases of zfs (another example is when the zpool "altroot" property has been set), this change simply compares the output of "zfs-get" from before and after "zfs-set" is called * update changelog format * Update changelogs/fragments/2454-detect_zfs_changed.yml Co-authored-by: Felix Fontein * add note about check_mode * Update plugins/modules/storage/zfs/zfs.py Co-authored-by: Felix Fontein * Update plugins/modules/storage/zfs/zfs.py Co-authored-by: Felix Fontein * clarify check mode qualifications * rephrase to avoid hypothetical Co-authored-by: Felix Fontein (cherry picked from commit 8e7aff00b5f3af2ed7dbc377255832da68817144) Co-authored-by: sam-lunt --- .../fragments/2454-detect_zfs_changed.yml | 2 ++ plugins/modules/storage/zfs/zfs.py | 24 +++++++++++++++---- 2 files changed, 21 insertions(+), 5 deletions(-) create mode 100644 changelogs/fragments/2454-detect_zfs_changed.yml diff --git a/changelogs/fragments/2454-detect_zfs_changed.yml b/changelogs/fragments/2454-detect_zfs_changed.yml new file mode 100644 index 0000000000..0604278f6b --- /dev/null +++ b/changelogs/fragments/2454-detect_zfs_changed.yml @@ -0,0 +1,2 @@ +bugfixes: + - zfs - certain ZFS properties, especially sizes, would lead to a task being falsely marked as "changed" even when no actual change was made (https://github.com/ansible-collections/community.general/issues/975, https://github.com/ansible-collections/community.general/pull/2454). diff --git a/plugins/modules/storage/zfs/zfs.py b/plugins/modules/storage/zfs/zfs.py index fe693a5045..2d5d4487dd 100644 --- a/plugins/modules/storage/zfs/zfs.py +++ b/plugins/modules/storage/zfs/zfs.py @@ -37,6 +37,12 @@ options: - A dictionary of zfs properties to be set. - See the zfs(8) man page for more information. type: dict +notes: + - C(check_mode) is supported, but in certain situations it may report a task + as changed that will not be reported as changed when C(check_mode) is disabled. + For example, this might occur when the zpool C(altroot) option is set or when + a size is written using human-readable notation, such as C(1M) or C(1024K), + instead of as an unqualified byte count, such as C(1048576). author: - Johan Wiren (@johanwiren) ''' @@ -184,9 +190,7 @@ class Zfs(object): return cmd = [self.zfs_cmd, 'set', prop + '=' + str(value), self.name] (rc, out, err) = self.module.run_command(cmd) - if rc == 0: - self.changed = True - else: + if rc != 0: self.module.fail_json(msg=err) def set_properties_if_changed(self): @@ -194,15 +198,25 @@ class Zfs(object): for prop, value in self.properties.items(): if current_properties.get(prop, None) != value: self.set_property(prop, value) + if self.module.check_mode: + return + updated_properties = self.get_current_properties() + for prop in self.properties: + value = updated_properties.get(prop, None) + 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: + self.changed = True def get_current_properties(self): - cmd = [self.zfs_cmd, 'get', '-H'] + cmd = [self.zfs_cmd, 'get', '-H', '-p', '-o', "property,value,source"] if self.enhanced_sharing: cmd += ['-e'] cmd += ['all', self.name] rc, out, err = self.module.run_command(" ".join(cmd)) properties = dict() - for prop, value, source in [l.split('\t')[1:4] for l in out.splitlines()]: + for line in out.splitlines(): + prop, value, 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