From c4e93b0b5fe3cc43c8e1dd933ad0444136acc01f Mon Sep 17 00:00:00 2001 From: "patchback[bot]" <45432694+patchback[bot]@users.noreply.github.com> Date: Tue, 27 Oct 2020 05:47:47 +0100 Subject: [PATCH] filesystem: fix 355 state absent (#1149) (#1184) * add support for filesystem removal (fix #355) - Add 'state' option, defaults to 'present'. - When state=absent, ignore other options (even 'dev' if the device doesn't exist) * test filesystem state=absent (+ check_mode + idempotency) * fix doc-required-mismatch * add changelog fragment * fix blkid return code * ext4dev may be deprecated * base checks on UUID instead * Update changelogs/fragments/1149-filesystem-fix-355-state-absent.yml Co-authored-by: Felix Fontein * Update plugins/modules/system/filesystem.py (version_added) Co-authored-by: Felix Fontein * use array for new run_command() calls; do not wipefs if no fs found * use dd as a fallback * do not use bare 'except' (pep8) * force string type * use dd anyway (wipefs not supported everywhere, possibly buggy with vfat, etc.) * do not truncate regular files; update changelog fragment * doc: update state description and an example; notice check_mode support * do not wipe mounted fs, fail instead * back to wipefs implementation * update test's main conditions * update changelog fragment * explicit types * fail state=absent on freebsd * remove doc-missing-type exceptions (2.9, 2.10, 2.11) Co-authored-by: Felix Fontein (cherry picked from commit a5ca990857f4c59af36335a717f14cd4704d0428) Co-authored-by: quidame --- .../1149-filesystem-fix-355-state-absent.yml | 4 + plugins/modules/system/filesystem.py | 116 +++++++++++++----- .../targets/filesystem/tasks/main.yml | 7 +- .../targets/filesystem/tasks/remove_fs.yml | 98 +++++++++++++++ tests/sanity/ignore-2.10.txt | 1 - tests/sanity/ignore-2.11.txt | 1 - tests/sanity/ignore-2.9.txt | 1 - 7 files changed, 194 insertions(+), 34 deletions(-) create mode 100644 changelogs/fragments/1149-filesystem-fix-355-state-absent.yml create mode 100644 tests/integration/targets/filesystem/tasks/remove_fs.yml diff --git a/changelogs/fragments/1149-filesystem-fix-355-state-absent.yml b/changelogs/fragments/1149-filesystem-fix-355-state-absent.yml new file mode 100644 index 0000000000..e969c9bead --- /dev/null +++ b/changelogs/fragments/1149-filesystem-fix-355-state-absent.yml @@ -0,0 +1,4 @@ +--- +bugfixes: + - filesystem - add option ``state`` with default ``present``. When set to ``absent``, filesystem signatures are removed + (https://github.com/ansible-collections/community.general/issues/355). diff --git a/plugins/modules/system/filesystem.py b/plugins/modules/system/filesystem.py index 3707d800db..fca7f2c56d 100644 --- a/plugins/modules/system/filesystem.py +++ b/plugins/modules/system/filesystem.py @@ -16,10 +16,24 @@ short_description: Makes a filesystem description: - This module creates a filesystem. options: + state: + description: + - If C(state=present), the filesystem is created if it doesn't already + exist, that is the default behaviour if I(state) is omitted. + - If C(state=absent), filesystem signatures on I(dev) are wiped if it + contains a filesystem (as known by C(blkid)). + - When C(state=absent), all other options but I(dev) are ignored, and the + module doesn't fail if the device I(dev) doesn't actually exist. + - C(state=absent) is not supported and will fail on FreeBSD systems. + type: str + choices: [ present, absent ] + default: present + version_added: 1.3.0 fstype: choices: [ btrfs, ext2, ext3, ext4, ext4dev, f2fs, lvm, ocfs2, reiserfs, xfs, vfat, swap ] description: - - Filesystem type to be created. + - Filesystem type to be created. This option is required with + C(state=present) (or if I(state) is omitted). - reiserfs support was added in 2.2. - lvm support was added in 2.5. - since 2.5, I(dev) can be an image file. @@ -27,11 +41,12 @@ options: - ocfs2 support was added in 2.6 - f2fs support was added in 2.7 - swap support was added in 2.8 - required: yes + type: str aliases: [type] dev: description: - Target path to device or image file. + type: path required: yes aliases: [device] force: @@ -43,18 +58,22 @@ options: description: - If C(yes), if the block device and filesystem size differ, grow the filesystem into the space. - Supported for C(ext2), C(ext3), C(ext4), C(ext4dev), C(f2fs), C(lvm), C(xfs), C(vfat), C(swap) filesystems. - - XFS Will only grow if mounted. + - XFS Will only grow if mounted. Currently, the module is based on commands + from C(util-linux) package to perform operations, so resizing of XFS is + not supported on FreeBSD systems. - vFAT will likely fail if fatresize < 1.04. type: bool default: 'no' opts: description: - List of options to be passed to mkfs command. + type: str requirements: - Uses tools related to the I(fstype) (C(mkfs)) and C(blkid) command. When I(resizefs) is enabled, C(blockdev) command is required too. notes: - Potential filesystem on I(dev) are checked using C(blkid), in case C(blkid) isn't able to detect an existing filesystem, this filesystem is overwritten even if I(force) is C(no). + - This module supports I(check_mode). ''' EXAMPLES = ''' @@ -68,6 +87,11 @@ EXAMPLES = ''' fstype: ext4 dev: /dev/sdb1 opts: -cc + +- name: Blank filesystem signature on /dev/sdb1 + community.general.filesystem: + dev: /dev/sdb1 + state: absent ''' from distutils.version import LooseVersion @@ -144,6 +168,22 @@ class Filesystem(object): cmd = "%s %s %s '%s'" % (mkfs, self.MKFS_FORCE_FLAGS, opts, dev) self.module.run_command(cmd, check_rc=True) + def wipefs(self, dev): + if platform.system() == 'FreeBSD': + msg = "module param state=absent is currently not supported on this OS (FreeBSD)." + self.module.fail_json(msg=msg) + + if self.module.check_mode: + return + + # wipefs comes with util-linux package (as 'blockdev' & 'findmnt' above) + # so it is not supported on FreeBSD. Even the use of dd as a fallback is + # not doable here if it needs get_mountpoint() (to prevent corruption of + # a mounted filesystem), since 'findmnt' is not available on FreeBSD. + wipefs = self.module.get_bin_path('wipefs', required=True) + cmd = [wipefs, "--all", dev.__str__()] + self.module.run_command(cmd, check_rc=True) + def grow_cmd(self, dev): cmd = self.module.get_bin_path(self.GROW, required=True) return [cmd, str(dev)] @@ -369,34 +409,35 @@ def main(): # There is no "single command" to manipulate filesystems, so we map them all out and their options module = AnsibleModule( argument_spec=dict( - fstype=dict(required=True, aliases=['type'], - choices=list(fstypes)), - dev=dict(required=True, aliases=['device']), - opts=dict(), + state=dict(type='str', default='present', choices=['present', 'absent']), + fstype=dict(type='str', aliases=['type'], choices=list(fstypes)), + dev=dict(type='path', required=True, aliases=['device']), + opts=dict(type='str'), force=dict(type='bool', default=False), resizefs=dict(type='bool', default=False), ), + required_if=[ + ('state', 'present', ['fstype']) + ], supports_check_mode=True, ) + state = module.params['state'] dev = module.params['dev'] fstype = module.params['fstype'] opts = module.params['opts'] force = module.params['force'] resizefs = module.params['resizefs'] - if fstype in friendly_names: - fstype = friendly_names[fstype] - changed = False - try: - klass = FILESYSTEMS[fstype] - except KeyError: - module.fail_json(changed=False, msg="module does not support this filesystem (%s) yet." % fstype) - if not os.path.exists(dev): - module.fail_json(msg="Device %s not found." % dev) + msg = "Device %s not found." % dev + if state == "present": + module.fail_json(msg=msg) + else: + module.exit_json(msg=msg) + dev = Device(module, dev) cmd = module.get_bin_path('blkid', required=True) @@ -405,24 +446,39 @@ def main(): # then this existing filesystem would be overwritten even if force isn't enabled. fs = raw_fs.strip() - filesystem = klass(module) + if state == "present": + if fstype in friendly_names: + fstype = friendly_names[fstype] - same_fs = fs and FILESYSTEMS.get(fs) == FILESYSTEMS[fstype] - if same_fs and not resizefs and not force: - module.exit_json(changed=False) - elif same_fs and resizefs: - if not filesystem.GROW: - module.fail_json(changed=False, msg="module does not support resizing %s filesystem yet." % fstype) + try: + klass = FILESYSTEMS[fstype] + except KeyError: + module.fail_json(changed=False, msg="module does not support this filesystem (%s) yet." % fstype) - out = filesystem.grow(dev) + filesystem = klass(module) - module.exit_json(changed=True, msg=out) - elif fs and not force: - module.fail_json(msg="'%s' is already used as %s, use force=yes to overwrite" % (dev, fs), rc=rc, err=err) + same_fs = fs and FILESYSTEMS.get(fs) == FILESYSTEMS[fstype] + if same_fs and not resizefs and not force: + module.exit_json(changed=False) + elif same_fs and resizefs: + if not filesystem.GROW: + module.fail_json(changed=False, msg="module does not support resizing %s filesystem yet." % fstype) - # create fs - filesystem.create(opts, dev) - changed = True + out = filesystem.grow(dev) + + module.exit_json(changed=True, msg=out) + elif fs and not force: + module.fail_json(msg="'%s' is already used as %s, use force=yes to overwrite" % (dev, fs), rc=rc, err=err) + + # create fs + filesystem.create(opts, dev) + changed = True + + elif fs: + # wipe fs signatures + filesystem = Filesystem(module) + filesystem.wipefs(dev) + changed = True module.exit_json(changed=changed) diff --git a/tests/integration/targets/filesystem/tasks/main.yml b/tests/integration/targets/filesystem/tasks/main.yml index 1a3cbc1a12..ba79b1e95f 100644 --- a/tests/integration/targets/filesystem/tasks/main.yml +++ b/tests/integration/targets/filesystem/tasks/main.yml @@ -43,7 +43,12 @@ - 'not (item.0.key == "f2fs" and ansible_distribution == "Ubuntu" and ansible_distribution_version is version("14.04", "<="))' - 'not (item.1 == "overwrite_another_fs" and ansible_system == "FreeBSD")' + - 'not (item.1 == "remove_fs" and ansible_system == "FreeBSD")' # util-linux not available on FreeBSD + # On CentOS 6 shippable containers, wipefs seems unable to remove vfat signatures + - 'not (item.1 == "remove_fs" and item.0.key == "vfat" and ansible_distribution == "CentOS" and + ansible_distribution_version is version("7.0", "<"))' + # The xfsprogs package on newer versions of OpenSUSE (15+) require Python 3, we skip this on our Python 2 container # OpenSUSE 42.3 Python2 and the other py3 containers are not affected so we will continue to run that - 'not (item.0.key == "xfs" and ansible_os_family == "Suse" and ansible_python.version.major == 2 and ansible_distribution_major_version|int != 42)' - loop: "{{ query('dict', tested_filesystems)|product(['create_fs', 'overwrite_another_fs'])|list }}" + loop: "{{ query('dict', tested_filesystems)|product(['create_fs', 'overwrite_another_fs', 'remove_fs'])|list }}" diff --git a/tests/integration/targets/filesystem/tasks/remove_fs.yml b/tests/integration/targets/filesystem/tasks/remove_fs.yml new file mode 100644 index 0000000000..7d1ca2a19c --- /dev/null +++ b/tests/integration/targets/filesystem/tasks/remove_fs.yml @@ -0,0 +1,98 @@ +--- +# We assume 'create_fs' tests have passed. + +- name: filesystem creation + filesystem: + dev: '{{ dev }}' + fstype: '{{ fstype }}' + +- name: get filesystem UUID with 'blkid' + command: + cmd: 'blkid -c /dev/null -o value -s UUID {{ dev }}' + changed_when: false + register: blkid_ref + +- name: Assert that a filesystem exists on top of the device + assert: + that: + - blkid_ref.stdout | length > 0 + + +# Test check_mode first +- name: filesystem removal (check mode) + filesystem: + dev: '{{ dev }}' + state: absent + register: wipefs + check_mode: yes + +- name: get filesystem UUID with 'blkid' (should remain the same) + command: + cmd: 'blkid -c /dev/null -o value -s UUID {{ dev }}' + changed_when: false + register: blkid + +- name: Assert that the state changed but the filesystem still exists + assert: + that: + - wipefs is changed + - blkid.stdout == blkid_ref.stdout + +# Do it +- name: filesystem removal + filesystem: + dev: '{{ dev }}' + state: absent + register: wipefs + +- name: get filesystem UUID with 'blkid' (should be empty) + command: + cmd: 'blkid -c /dev/null -o value -s UUID {{ dev }}' + changed_when: false + failed_when: false + register: blkid + +- name: Assert that the state changed and the device has no filesystem + assert: + that: + - wipefs is changed + - blkid.stdout | length == 0 + - blkid.rc == 2 + +# Do it again +- name: filesystem removal (idempotency) + filesystem: + dev: '{{ dev }}' + state: absent + register: wipefs + +- name: Assert that the state did not change + assert: + that: + - wipefs is not changed + +# and again +- name: filesystem removal (idempotency, check mode) + filesystem: + dev: '{{ dev }}' + state: absent + register: wipefs + check_mode: yes + +- name: Assert that the state did not change + assert: + that: + - wipefs is not changed + + +# By the way, test removal of a filesystem on unexistent device +- name: filesystem removal (unexistent device) + filesystem: + dev: '/dev/unexistent_device' + state: absent + register: wipefs + +- name: Assert that the state did not change + assert: + that: + - wipefs is not changed diff --git a/tests/sanity/ignore-2.10.txt b/tests/sanity/ignore-2.10.txt index 49bc15a528..5f127ebb95 100644 --- a/tests/sanity/ignore-2.10.txt +++ b/tests/sanity/ignore-2.10.txt @@ -1169,7 +1169,6 @@ plugins/modules/system/dconf.py pylint:blacklisted-name plugins/modules/system/dconf.py validate-modules:doc-missing-type plugins/modules/system/dconf.py validate-modules:parameter-type-not-in-doc plugins/modules/system/filesystem.py pylint:blacklisted-name -plugins/modules/system/filesystem.py validate-modules:doc-missing-type plugins/modules/system/gconftool2.py pylint:blacklisted-name plugins/modules/system/gconftool2.py validate-modules:parameter-state-invalid-choice plugins/modules/system/gconftool2.py validate-modules:parameter-type-not-in-doc diff --git a/tests/sanity/ignore-2.11.txt b/tests/sanity/ignore-2.11.txt index 49bc15a528..5f127ebb95 100644 --- a/tests/sanity/ignore-2.11.txt +++ b/tests/sanity/ignore-2.11.txt @@ -1169,7 +1169,6 @@ plugins/modules/system/dconf.py pylint:blacklisted-name plugins/modules/system/dconf.py validate-modules:doc-missing-type plugins/modules/system/dconf.py validate-modules:parameter-type-not-in-doc plugins/modules/system/filesystem.py pylint:blacklisted-name -plugins/modules/system/filesystem.py validate-modules:doc-missing-type plugins/modules/system/gconftool2.py pylint:blacklisted-name plugins/modules/system/gconftool2.py validate-modules:parameter-state-invalid-choice plugins/modules/system/gconftool2.py validate-modules:parameter-type-not-in-doc diff --git a/tests/sanity/ignore-2.9.txt b/tests/sanity/ignore-2.9.txt index 2f17ba269e..8f353c19e2 100644 --- a/tests/sanity/ignore-2.9.txt +++ b/tests/sanity/ignore-2.9.txt @@ -921,7 +921,6 @@ plugins/modules/system/dconf.py pylint:blacklisted-name plugins/modules/system/dconf.py validate-modules:doc-missing-type plugins/modules/system/dconf.py validate-modules:parameter-type-not-in-doc plugins/modules/system/filesystem.py pylint:blacklisted-name -plugins/modules/system/filesystem.py validate-modules:doc-missing-type plugins/modules/system/gconftool2.py pylint:blacklisted-name plugins/modules/system/gconftool2.py validate-modules:parameter-type-not-in-doc plugins/modules/system/interfaces_file.py pylint:blacklisted-name