From 75a5f83602e7cbd9a708e793e329a30bacc5878d Mon Sep 17 00:00:00 2001 From: Abhijeet Kasurde Date: Sat, 30 May 2020 02:04:53 +0530 Subject: [PATCH] Partially revert "mount: Check if src exists before mounted (ansible/ansible#61752)" (#33) This reverts part of ansible commit 72023d7462e78635264fd12bfdb23894b4163cba. The immediate reason is that it breaks mounts where src is not a path. Examples of such mounts are network-based filesystems such as nfs, cifs, glusterfs, ceph, virtual filesystems such as tmpfs or overlayfs, and also UUID-based mounts. It is too hard to come with an exhaustive list, especially if we take non-Linux systems into account, so don't even try. Additionally, it did not really fix the issue (ansible/ansible#59183) that it intended to fix, because the mount could fail but leave a non-working fstab entry for reasons other than non-existing src path. Restore fstab and remove the mount point after a failed mount Add a reminder that not only devices can be mounted Fixes: ansible/ansible#65855 Fixes: ansible/ansible#67588 Fixes: ansible/ansible#67966 Signed-off-by: Alexander E. Patrakov Signed-off-by: Abhijeet Kasurde Co-authored-by: Alexander E. Patrakov --- plugins/modules/mount.py | 58 +++++++++++++++++++++---- tests/integration/targets/mount/aliases | 1 - tests/unit/modules/system/test_mount.py | 36 ++++++++++++++- 3 files changed, 85 insertions(+), 10 deletions(-) diff --git a/plugins/modules/mount.py b/plugins/modules/mount.py index 98b2d05..1fbf716 100644 --- a/plugins/modules/mount.py +++ b/plugins/modules/mount.py @@ -29,7 +29,7 @@ options: aliases: [ name ] src: description: - - Device to be mounted on I(path). + - Device (or NFS volume, or something else) to be mounted on I(path). - Required when I(state) set to C(present) or C(mounted). type: path fstype: @@ -171,13 +171,14 @@ EXAMPLES = r''' ''' +import errno import os import platform from ansible.module_utils.basic import AnsibleModule from ansible_collections.ansible.posix.plugins.module_utils.ismount import ismount from ansible.module_utils.six import iteritems -from ansible.module_utils._text import to_native +from ansible.module_utils._text import to_bytes, to_native def write_fstab(module, lines, path): @@ -213,8 +214,15 @@ def _escape_fstab(v): def set_mount(module, args): """Set/change a mount point location in fstab.""" + name, backup_lines, changed = _set_mount_save_old(module, args) + return name, changed + + +def _set_mount_save_old(module, args): + """Set/change a mount point location in fstab. Save the old fstab contents.""" to_write = [] + old_lines = [] exists = False changed = False escaped_args = dict([(k, _escape_fstab(v)) for k, v in iteritems(args)]) @@ -225,6 +233,8 @@ def set_mount(module, args): '%(src)s - %(name)s %(fstype)s %(passno)s %(boot)s %(opts)s\n') for line in open(args['fstab'], 'r').readlines(): + old_lines.append(line) + if not line.strip(): to_write.append(line) @@ -307,7 +317,7 @@ def set_mount(module, args): if changed and not module.check_mode: write_fstab(module, to_write, args['fstab']) - return (args['name'], changed) + return (args['name'], old_lines, changed) def unset_mount(module, args): @@ -750,17 +760,34 @@ def main(): changed = True elif state == 'mounted': - if not os.path.exists(args['src']): - module.fail_json(msg="Unable to mount %s as it does not exist" % args['src']) - + dirs_created = [] if not os.path.exists(name) and not module.check_mode: try: - os.makedirs(name) + # Something like mkdir -p but with the possibility to undo. + # Based on some copy-paste from the "file" module. + curpath = '' + for dirname in name.strip('/').split('/'): + curpath = '/'.join([curpath, dirname]) + # Remove leading slash if we're creating a relative path + if not os.path.isabs(name): + curpath = curpath.lstrip('/') + + b_curpath = to_bytes(curpath, errors='surrogate_or_strict') + if not os.path.exists(b_curpath): + try: + os.mkdir(b_curpath) + dirs_created.append(b_curpath) + except OSError as ex: + # Possibly something else created the dir since the os.path.exists + # check above. As long as it's a dir, we don't need to error out. + if not (ex.errno == errno.EEXIST and os.path.isdir(b_curpath)): + raise + except (OSError, IOError) as e: module.fail_json( msg="Error making dir %s: %s" % (name, to_native(e))) - name, changed = set_mount(module, args) + name, backup_lines, changed = _set_mount_save_old(module, args) res = 0 if ( @@ -777,6 +804,21 @@ def main(): res, msg = mount(module, args) if res: + # Not restoring fstab after a failed mount was reported as a bug, + # ansible/ansible#59183 + # A non-working fstab entry may break the system at the reboot, + # so undo all the changes if possible. + try: + write_fstab(module, backup_lines, args['fstab']) + except Exception: + pass + + try: + for dirname in dirs_created[::-1]: + os.rmdir(dirname) + except Exception: + pass + module.fail_json(msg="Error mounting %s: %s" % (name, msg)) elif state == 'present': name, changed = set_mount(module, args) diff --git a/tests/integration/targets/mount/aliases b/tests/integration/targets/mount/aliases index 7ea1a65..fa6da2b 100644 --- a/tests/integration/targets/mount/aliases +++ b/tests/integration/targets/mount/aliases @@ -2,4 +2,3 @@ needs/privileged needs/root shippable/posix/group1 skip/aix -disabled # fixme diff --git a/tests/unit/modules/system/test_mount.py b/tests/unit/modules/system/test_mount.py index f25e6e1..893a651 100644 --- a/tests/unit/modules/system/test_mount.py +++ b/tests/unit/modules/system/test_mount.py @@ -2,9 +2,14 @@ import os import tempfile from ansible_collections.ansible.posix.tests.unit.compat import unittest +from ansible_collections.ansible.posix.tests.unit.compat.mock import MagicMock from ansible.module_utils._text import to_bytes -from ansible_collections.ansible.posix.plugins.modules.mount import get_linux_mounts +from ansible_collections.ansible.posix.plugins.modules.mount import ( + get_linux_mounts, + _set_mount_save_old, + set_mount, +) class LinuxMountsTestCase(unittest.TestCase): @@ -23,3 +28,32 @@ class LinuxMountsTestCase(unittest.TestCase): ) mounts = get_linux_mounts(None, path) self.assertEqual(mounts['/tmp/bbb']['src'], '/tmp/aaa') + + def test_set_mount_save_old(self): + module = MagicMock(name='AnsibleModule') + module.check_mode = True + module.params = {'backup': False} + + fstab_data = [ + 'UUID=8ac075e3-1124-4bb6-bef7-a6811bf8b870 / xfs defaults 0 0\n', + '/swapfile none swap defaults 0 0\n' + ] + path = self._create_file("".join(fstab_data)) + args = { + 'fstab': path, + 'name': '/data', + 'src': '/dev/sdb1', + 'fstype': 'ext4', + 'opts': 'defaults', + 'dump': '0', + 'passno': '0', + } + + name, changed = set_mount(module, args) + self.assertEqual(name, '/data') + self.assertTrue(changed) + + name, backup_lines, changed = _set_mount_save_old(module, args) + self.assertEqual(backup_lines, fstab_data) + self.assertEqual(name, '/data') + self.assertTrue(changed)