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 <patrakov@gmail.com>
Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>

Co-authored-by: Alexander E. Patrakov <patrakov@gmail.com>

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']):
+        dirs_created = []
-            module.fail_json(msg="Unable to mount %s as it does not exist" % args['src'])
-
         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)

tests/integration/targets/mount/aliases

@@ -2,4 +2,3 @@ needs/privileged
 needs/root
 shippable/posix/group1
 skip/aix
-disabled # fixme

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)