Working subset of mount fixes (#4987)

* Fixing bind mount on Linux

* The latest update from jtyr doesn't pass integration tests.

Manually select the changes that are necessary to fix the bug with
unmounting

lib/ansible/modules/system/mount.py

@@ -20,56 +20,77 @@
 # You should have received a copy of the GNU General Public License
 # along with Ansible.  If not, see <http://www.gnu.org/licenses/>.
 
+
+from ansible.module_utils.basic import AnsibleModule
+from ansible.module_utils.basic import get_platform
+from ansible.module_utils.ismount import ismount
+from ansible.module_utils.pycompat24 import get_exception
+from ansible.module_utils.six import iteritems
+import os
+import re
+
+
 DOCUMENTATION = '''
 ---
 module: mount
 short_description: Control active and configured mount points
 description:
-     - This module controls active and configured mount points in C(/etc/fstab).
+  - This module controls active and configured mount points in C(/etc/fstab).
+author:
+  - Ansible Core Team
+  - Seth Vidal
 version_added: "0.6"
 options:
   name:
     description:
-      - "path to the mount point, eg: C(/mnt/files)"
+      - Path to the mount point (e.g. C(/mnt/files))
     required: true
   src:
     description:
-        - device to be mounted on I(name). Required when C(state=present) or C(state=mounted)
+      - Device to be mounted on I(name). Required when I(state) set to
+        C(present) or C(mounted).
     required: false
     default: null
   fstype:
     description:
-      - file-system type. Required when C(state=present) or C(state=mounted)
+      - Filesystem type. Required when I(state) is C(present) or C(mounted).
     required: false
     default: null
   opts:
     description:
-      - mount options (see fstab(5), or vfstab(4) on Solaris)
+      - Mount options (see fstab(5), or vfstab(4) on Solaris).
     required: false
     default: null
   dump:
     description:
-      - "dump (see fstab(5)), Note that if nulled, C(state=present) will cease to work and duplicate entries will be made with subsequent runs."
+      - Dump (see fstab(5)). Note that if set to C(null) and I(state) set to
+        C(present), it will cease to work and duplicate entries will be made
+        with subsequent runs.
       - Has no effect on Solaris systems.
     required: false
     default: 0
   passno:
     description:
-      - "passno (see fstab(5)), Note that if nulled, C(state=present) will cease to work and duplicate entries will be made with subsequent runs."
+      - Passno (see fstab(5)). Note that if set to C(null) and I(state) set to
+        C(present), it will cease to work and duplicate entries will be made
+        with subsequent runs.
       - Deprecated on Solaris systems.
     required: false
     default: 0
   state:
     description:
-      - If C(mounted) or C(unmounted), the device will be actively mounted or unmounted as needed and appropriately configured in I(fstab).
+      - If C(mounted) or C(unmounted), the device will be actively mounted or
-      - C(absent) and C(present) only deal with I(fstab) but will not affect current mounting.
+        unmounted as needed and appropriately configured in I(fstab).
-      - If specifying C(mounted) and the mount point is not present, the mount point will be created. Similarly.
+      - C(absent) and C(present) only deal with I(fstab) but will not affect
+        current mounting.
+      - If specifying C(mounted) and the mount point is not present, the mount
+        point will be created. Similarly.
       - Specifying C(absent) will remove the mount point directory.
     required: true
-    choices: [ "present", "absent", "mounted", "unmounted" ]
+    choices: ["present", "absent", "mounted", "unmounted"]
   fstab:
     description:
-      - file to use instead of C(/etc/fstab). You shouldn't use that option
+      - File to use instead of C(/etc/fstab). You shouldn't use that option
         unless you really know what you are doing. This might be useful if
         you need to configure mountpoints in a chroot environment.
     required: false
@@ -77,65 +98,92 @@ options:
   boot:
     version_added: 2.2
     description:
-      - Determines if the filesystem should be mounted on boot.  Only applies to Solaris systems.
+      - Determines if the filesystem should be mounted on boot.
+      - Only applies to Solaris systems.
     required: false
     default: yes
-    choices: [ "yes", "no" ]
+    choices: ["yes", "no"]
-
-author:
-    - Ansible Core Team
-    - Seth Vidal
 '''
+
 EXAMPLES = '''
-# Mount DVD read-only
+- name: Mount DVD read-only
-- mount: name=/mnt/dvd src=/dev/sr0 fstype=iso9660 opts=ro state=present
+  mount:
+    name: /mnt/dvd
+    src: /dev/sr0
+    fstype: iso9660
+    opts: ro
+    state: present
 
-# Mount up device by label
+- name: Mount up device by label
-- mount: name=/srv/disk src='LABEL=SOME_LABEL' fstype=ext4 state=present
+  mount:
+    name: /srv/disk
+    src: LABEL=SOME_LABEL
+    fstype: ext4
+    state: present
 
-# Mount up device by UUID
+- name: Mount up device by UUID
-- mount: name=/home src='UUID=b3e48f45-f933-4c8e-a700-22a159ec9077' fstype=xfs opts=noatime state=present
+  mount:
+    name: /home
+    src: UUID=b3e48f45-f933-4c8e-a700-22a159ec9077
+    fstype: xfs
+    opts: noatime
+    state: present
 '''
 
-from ansible.module_utils.six import iteritems
 
 def write_fstab(lines, dest):
-
     fs_w = open(dest, 'w')
+
     for l in lines:
         fs_w.write(l)
 
     fs_w.flush()
     fs_w.close()
 
+
 def _escape_fstab(v):
-    """ escape space (040), ampersand (046) and backslash (134) which are invalid in fstab fields """
+    """Escape invalid characters in fstab fields.
+
+    space (040)
+    ampersand (046)
+    backslash (134)
+    """
+
     if isinstance(v, int):
         return v
     else:
-        return v.replace('\\', '\\134').replace(' ', '\\040').replace('&', '\\046')
+        return(
+            v.
+            replace('\\', '\\134').
+            replace(' ', '\\040').
+            replace('&', '\\046'))
+
 
 def set_mount(module, **kwargs):
-    """ set/change a mount point location in fstab """
+    """Set/change a mount point location in fstab."""
 
-    # solaris kwargs: name, src, fstype, opts, boot, passno, state, fstab=/etc/vfstab
+    # solaris kwargs:
-    # linux kwargs: name, src, fstype, opts, dump, passno, state, fstab=/etc/fstab
+    #   name, src, fstype, opts, boot, passno, state, fstab=/etc/vfstab
+    # linux:
+    #   kwargs: name, src, fstype, opts, dump, passno, state, fstab=/etc/fstab
     if get_platform() == 'SunOS':
         args = dict(
-            opts   = '-',
+            opts='-',
-            passno = '-',
+            passno='-',
-            fstab  = '/etc/vfstab',
+            fstab='/etc/vfstab',
-            boot   = 'yes'
+            boot='yes'
         )
-        new_line = '%(src)s - %(name)s %(fstype)s %(passno)s %(boot)s %(opts)s\n'
+        new_line = (
+            '%(src)s - %(name)s %(fstype)s %(passno)s %(boot)s %(opts)s\n')
     else:
         args = dict(
-            opts   = 'defaults',
+            opts='defaults',
-            dump   = '0',
+            dump='0',
-            passno = '0',
+            passno='0',
-            fstab  = '/etc/fstab'
+            fstab='/etc/fstab'
         )
-        new_line = '%(src)s %(name)s %(fstype)s %(opts)s %(dump)s %(passno)s\n'
+        new_line = (
+            '%(src)s %(name)s %(fstype)s %(opts)s %(dump)s %(passno)s\n')
     args.update(kwargs)
 
     to_write = []
@@ -145,35 +193,57 @@ def set_mount(module, **kwargs):
     for line in open(args['fstab'], 'r').readlines():
         if not line.strip():
             to_write.append(line)
+
             continue
+
         if line.strip().startswith('#'):
             to_write.append(line)
+
             continue
+
         if len(line.split()) != 6 and get_platform() != 'SunOS':
-            # not sure what this is or why it is here
+            # Not sure what this is or why it is here but it is not our fault
-            # but it is not our fault so leave it be
+            # so leave it be
             to_write.append(line)
+
             continue
 
         ld = {}
         if get_platform() == 'SunOS':
-            ld['src'], dash, ld['name'], ld['fstype'], ld['passno'], ld['boot'], ld['opts']  = line.split()
+            (
+                ld['src'],
+                dash,
+                ld['name'],
+                ld['fstype'],
+                ld['passno'],
+                ld['boot'],
+                ld['opts']
+            ) = line.split()
         else:
-            ld['src'], ld['name'], ld['fstype'], ld['opts'], ld['dump'], ld['passno']  = line.split()
+            (
+                ld['src'],
+                ld['name'],
+                ld['fstype'],
+                ld['opts'],
+                ld['dump'],
+                ld['passno']
+            ) = line.split()
 
         if ld['name'] != escaped_args['name']:
             to_write.append(line)
+
             continue
 
-        # it exists - now see if what we have is different
+        # It exists - now see if what we have is different
         exists = True
+
         if get_platform() == 'SunOS':
-            for t in ('src', 'fstype','passno', 'boot', 'opts'):
+            for t in ('src', 'fstype', 'passno', 'boot', 'opts'):
                 if ld[t] != escaped_args[t]:
                     changed = True
                     ld[t] = escaped_args[t]
         else:
-            for t in ('src', 'fstype','opts', 'dump', 'passno'):
+            for t in ('src', 'fstype', 'opts', 'dump', 'passno'):
                 if ld[t] != escaped_args[t]:
                     changed = True
                     ld[t] = escaped_args[t]
@@ -194,23 +264,25 @@ def set_mount(module, **kwargs):
 
 
 def unset_mount(module, **kwargs):
-    """ remove a mount point from fstab """
+    """Remove a mount point from fstab."""
 
-    # solaris kwargs: name, src, fstype, opts, boot, passno, state, fstab=/etc/vfstab
+    # solaris kwargs:
-    # linux kwargs: name, src, fstype, opts, dump, passno, state, fstab=/etc/fstab
+    #   name, src, fstype, opts, boot, passno, state, fstab=/etc/vfstab
+    # linux kwargs:
+    #   name, src, fstype, opts, dump, passno, state, fstab=/etc/fstab
     if get_platform() == 'SunOS':
         args = dict(
-            opts   = '-',
+            opts='-',
-            passno = '-',
+            passno='-',
-            fstab  = '/etc/vfstab',
+            fstab='/etc/vfstab',
-            boot   = 'yes'
+            boot='yes'
         )
     else:
         args = dict(
-            opts   = 'default',
+            opts='default',
-            dump   = '0',
+            dump='0',
-            passno = '0',
+            passno='0',
-            fstab  = '/etc/fstab'
+            fstab='/etc/fstab'
         )
     args.update(kwargs)
 
@@ -220,27 +292,49 @@ def unset_mount(module, **kwargs):
     for line in open(args['fstab'], 'r').readlines():
         if not line.strip():
             to_write.append(line)
+
             continue
+
         if line.strip().startswith('#'):
             to_write.append(line)
+
             continue
+
         if len(line.split()) != 6 and get_platform() != 'SunOS':
-            # not sure what this is or why it is here
+            # Not sure what this is or why it is here but it is not our fault
-            # but it is not our fault so leave it be
+            # so leave it be
             to_write.append(line)
+
             continue
 
         ld = {}
+
         if get_platform() == 'SunOS':
-            ld['src'], dash, ld['name'], ld['fstype'], ld['passno'], ld['boot'], ld['opts']  = line.split()
+            (
+                ld['src'],
+                dash,
+                ld['name'],
+                ld['fstype'],
+                ld['passno'],
+                ld['boot'],
+                ld['opts']
+            ) = line.split()
         else:
-            ld['src'], ld['name'], ld['fstype'], ld['opts'], ld['dump'], ld['passno']  = line.split()
+            (
+                ld['src'],
+                ld['name'],
+                ld['fstype'],
+                ld['opts'],
+                ld['dump'],
+                ld['passno']
+            ) = line.split()
 
         if ld['name'] != escaped_name:
             to_write.append(line)
+
             continue
 
-        # if we got here we found a match - continue and mark changed
+        # If we got here we found a match - continue and mark changed
         changed = True
 
     if changed and not module.check_mode:
@@ -250,87 +344,146 @@ def unset_mount(module, **kwargs):
 
 
 def mount(module, **kwargs):
-    """ mount up a path or remount if needed """
+    """Mount up a path or remount if needed."""
 
-    # solaris kwargs: name, src, fstype, opts, boot, passno, state, fstab=/etc/vfstab
+    # solaris kwargs:
-    # linux kwargs: name, src, fstype, opts, dump, passno, state, fstab=/etc/fstab
+    #   name, src, fstype, opts, boot, passno, state, fstab=/etc/vfstab
+    # linux kwargs:
+    #   name, src, fstype, opts, dump, passno, state, fstab=/etc/fstab
     if get_platform() == 'SunOS':
         args = dict(
-            opts   = '-',
+            opts='-',
-            passno = '-',
+            passno='-',
-            fstab  = '/etc/vfstab',
+            fstab='/etc/vfstab',
-            boot   = 'yes'
+            boot='yes'
         )
     else:
         args = dict(
-            opts   = 'default',
+            opts='default',
-            dump   = '0',
+            dump='0',
-            passno = '0',
+            passno='0',
-            fstab  = '/etc/fstab'
+            fstab='/etc/fstab'
         )
     args.update(kwargs)
 
-    mount_bin = module.get_bin_path('mount')
+    mount_bin = module.get_bin_path('mount', required=True)
-
     name = kwargs['name']
-
+    cmd = [mount_bin]
-    cmd = [ mount_bin, ]
 
     if ismount(name):
-        cmd += [ '-o', 'remount', ]
+        cmd += ['-o', 'remount']
 
     if get_platform().lower() == 'freebsd':
-        cmd += [ '-F', args['fstab'], ]
+        cmd += ['-F', args['fstab']]
+    elif get_platform().lower() == 'linux':
+        cmd += ['-T', args['fstab']]
 
-    if get_platform().lower() == 'linux':
+    cmd += [name]
-        cmd += [ '-T', args['fstab'], ]
-
-    cmd += [ name, ]
 
     rc, out, err = module.run_command(cmd)
+
     if rc == 0:
         return 0, ''
     else:
         return rc, out+err
 
-def umount(module, **kwargs):
-    """ unmount a path """
 
-    umount_bin = module.get_bin_path('umount')
+def umount(module, **kwargs):
+    """Unmount a path."""
+
+    umount_bin = module.get_bin_path('umount', required=True)
     name = kwargs['name']
     cmd = [umount_bin, name]
 
     rc, out, err = module.run_command(cmd)
+
     if rc == 0:
         return 0, ''
     else:
         return rc, out+err
 
-def main():
 
+# Note if we wanted to put this into module_utils we'd have to get permission
+# from @jupeter -- https://github.com/ansible/ansible-modules-core/pull/2923
+# @jtyr -- https://github.com/ansible/ansible-modules-core/issues/4439
+# and @abadger to relicense from GPLv3+
+def is_bind_mounted(module, dest, src=None, fstype=None):
+    """Return whether the dest is bind mounted
+
+    :arg module: The AnsibleModule (used for helper functions)
+    :arg dest: The directory to be mounted under. This is the primary means
+        of identifying whether the destination is mounted.
+    :kwarg src: The source directory. If specified, this is used to help
+        ensure that we are detecting that the correct source is mounted there.
+    :kwarg fstype: The filesystem type. If specified this is also used to
+        help ensure that we are detecting the right mount.
+    :returns: True if the dest is mounted with src otherwise False.
+    """
+
+    is_mounted = False
+    bin_path = module.get_bin_path('mount', required=True)
+    cmd = '%s -l' % bin_path
+
+    if get_platform().lower() == 'linux':
+        bin_path = module.get_bin_path('findmnt', required=True)
+        cmd = '%s -nr %s' % (bin_path, dest)
+
+    rc, out, err = module.run_command(cmd)
+    mounts = []
+
+    if len(out):
+        mounts = out.strip().split('\n')
+
+    mount_pattern = re.compile('\[(.*)\]')
+
+    for mnt in mounts:
+        arguments = mnt.split()
+
+        if get_platform().lower() == 'linux':
+            result = mount_pattern.search(arguments[1])
+
+            if len(result.groups()) == 1:
+                if arguments[0] == dest:
+                    is_mounted = True
+        elif (
+                (arguments[0] == src or src is None) and
+                arguments[2] == dest and
+                (arguments[4] == fstype or fstype is None)):
+            is_mounted = True
+
+        if is_mounted:
+            break
+
+    return is_mounted
+
+
+def main():
     module = AnsibleModule(
-        argument_spec = dict(
+        argument_spec=dict(
-            state  = dict(required=True, choices=['present', 'absent', 'mounted', 'unmounted']),
+            boot=dict(default='yes', choices=['yes', 'no']),
-            name   = dict(required=True),
+            dump=dict(),
-            opts   = dict(default=None),
+            fstab=dict(default='/etc/fstab'),
-            passno = dict(default=None, type='str'),
+            fstype=dict(),
-            dump   = dict(default=None),
+            name=dict(required=True, type='path'),
-            src    = dict(required=False),
+            opts=dict(),
-            fstype = dict(required=False),
+            passno=dict(type='str'),
-            boot   = dict(default='yes', choices=['yes', 'no']),
+            src=dict(type='path'),
-            fstab  = dict(default='/etc/fstab')
+            state=dict(
+                required=True,
+                choices=['present', 'absent', 'mounted', 'unmounted']),
         ),
         supports_check_mode=True,
-        required_if = (
+        required_if=(
             ['state', 'mounted', ['src', 'fstype']],
             ['state', 'present', ['src', 'fstype']]
         )
     )
 
-
     changed = False
-    rc = 0
+    args = {
-    args = {'name': module.params['name']}
+        'name': module.params['name']
+    }
+
     if module.params['src'] is not None:
         args['src'] = module.params['src']
     if module.params['fstype'] is not None:
@@ -346,27 +499,37 @@ def main():
     elif module.params['fstab'] is not None:
         args['fstab'] = module.params['fstab']
 
-    # if fstab file does not exist, we first need to create it. This mainly
+    # If fstab file does not exist, we first need to create it. This mainly
-    # happens when fstab optin is passed to the module.
+    # happens when fstab option is passed to the module.
     if not os.path.exists(args['fstab']):
         if not os.path.exists(os.path.dirname(args['fstab'])):
             os.makedirs(os.path.dirname(args['fstab']))
-        open(args['fstab'],'a').close()
 
-    # absent == remove from fstab and unmounted
+        open(args['fstab'], 'a').close()
-    # unmounted == do not change fstab state, but unmount
+
-    # present == add to fstab, do not change mount state
+    # absent:
-    # mounted == add to fstab if not there and make sure it is mounted, if it has changed in fstab then remount it
+    #   Remove from fstab and unmounted.
+    # unmounted:
+    #   Do not change fstab state, but unmount.
+    # present:
+    #   Add to fstab, do not change mount state.
+    # mounted:
+    #   Add to fstab if not there and make sure it is mounted. If it has
+    #   changed in fstab then remount it.
 
     state = module.params['state']
-    name  = module.params['name']
+    name = module.params['name']
+
     if state == 'absent':
         name, changed = unset_mount(module, **args)
+
         if changed and not module.check_mode:
-            if ismount(name):
+            if ismount(name) or is_bind_mounted(module, name):
-                res,msg  = umount(module, **args)
+                res, msg = umount(module, **args)
+
                 if res:
-                    module.fail_json(msg="Error unmounting %s: %s" % (name, msg))
+                    module.fail_json(
+                        msg="Error unmounting %s: %s" % (name, msg))
 
             if os.path.exists(name):
                 try:
@@ -374,61 +537,55 @@ def main():
                 except (OSError, IOError):
                     e = get_exception()
                     module.fail_json(msg="Error rmdir %s: %s" % (name, str(e)))
-
+    elif state == 'unmounted':
-        module.exit_json(changed=changed, **args)
+        if ismount(name) or is_bind_mounted(module, name):
-
-    if state == 'unmounted':
-        if ismount(name):
             if not module.check_mode:
-                res,msg  = umount(module, **args)
+                res, msg = umount(module, **args)
+
                 if res:
-                    module.fail_json(msg="Error unmounting %s: %s" % (name, msg))
+                    module.fail_json(
+                        msg="Error unmounting %s: %s" % (name, msg))
+
             changed = True
-
+    elif state == 'mounted':
-        module.exit_json(changed=changed, **args)
+        if not os.path.exists(name) and not module.check_mode:
-
+            try:
-    if state in ['mounted', 'present']:
+                os.makedirs(name)
-        if state == 'mounted':
+            except (OSError, IOError):
-            if not os.path.exists(name) and not module.check_mode:
+                e = get_exception()
-                try:
+                module.fail_json(
-                    os.makedirs(name)
+                    msg="Error making dir %s: %s" % (name, str(e)))
-                except (OSError, IOError):
-                    e = get_exception()
-                    module.fail_json(msg="Error making dir %s: %s" % (name, str(e)))
 
         name, changed = set_mount(module, **args)
-        if state == 'mounted':
+        res = 0
-            res = 0
+
-            if ismount(name):
+        if ismount(name):
-                if changed and not module.check_mode:
+            if not module.check_mode:
-                    res,msg = mount(module, **args)
+                res, msg = mount(module, **args)
-            elif 'bind' in args.get('opts', []):
                 changed = True
-                cmd = 'mount -l'
+        elif 'bind' in args.get('opts', []):
-                rc, out, err = module.run_command(cmd)
+            changed = True
-                allmounts = out.split('\n')
+
-                for mounts in allmounts[:-1]:
+            if is_bind_mounted(module, name, args['src'], args['fstype']):
-                    arguments = mounts.split()
+                changed = False
-                    if arguments[0] == args['src'] and arguments[2] == args['name'] and arguments[4] == args['fstype']:
+
-                        changed = False
+            if changed and not module.check_mode:
-                if changed:
+                res, msg = mount(module, **args)
-                    res,msg = mount(module, **args)
+        else:
-            else:
+            changed = True
-                changed = True
+
-                if not module.check_mode:
+            if not module.check_mode:
-                    res,msg = mount(module, **args)
+                res, msg = mount(module, **args)
+
+        if res:
+            module.fail_json(msg="Error mounting %s: %s" % (name, msg))
+    elif state == 'present':
+        name, changed = set_mount(module, **args)
+    else:
+        module.fail_json(msg='Unexpected position reached')
+
+    module.exit_json(changed=changed, **args)
 
 
-            if res:
+if __name__ == '__main__':
-                module.fail_json(msg="Error mounting %s: %s" % (name, msg))
+    main()
-
-
-        module.exit_json(changed=changed, **args)
-
-    module.fail_json(msg='Unexpected position reached')
-
-# import module snippets
-from ansible.module_utils.basic import *
-from ansible.module_utils.ismount import *
-
-main()