Merge pull request saltstack#65351 from garethgreenaway/18907_lazy_un…

…mount_when_fails

[master] Add fallback for when "remounting"  NFS or FUSE initially fails

changelog/18907.fixed.md

@@ -0,0 +1 @@
+When an NFS or FUSE mount fails to unmount when mount options have changed, try again with a lazy umount before mounting again.

salt/modules/mount.py

@@ -1383,7 +1383,7 @@ def remount(name, device, mkmnt=False, fstype="", opts="defaults", user=None):
     return mount(name, device, mkmnt, fstype, opts, user=user)
 
 
-def umount(name, device=None, user=None, util="mount"):
+def umount(name, device=None, user=None, util="mount", lazy=False):
     """
     Attempt to unmount a device by specifying the directory it is mounted on
 
@@ -1413,10 +1413,14 @@ def umount(name, device=None, user=None, util="mount"):
     if name not in mnts:
         return f"{name} does not have anything mounted"
 
+    cmd = "umount"
+    if lazy:
+        cmd = f"{cmd} -l"
     if not device:
-        cmd = f"umount '{name}'"
+        cmd = f"{cmd} '{name}'"
     else:
-        cmd = f"umount '{device}'"
+        cmd = f"{cmd}'{device}'"
+
     out = __salt__["cmd.run_all"](cmd, runas=user, python_shell=False)
     if out["retcode"]:
         return out["stderr"]

salt/states/mount.py

@@ -71,7 +71,7 @@ def mounted(
     extra_mount_translate_options=None,
     hidden_opts=None,
     bind_mount_copy_active_opts=True,
-    **kwargs
+    **kwargs,
 ):
     """
     Verify that a device is mounted
@@ -282,10 +282,10 @@ def mounted(
         real_device = device.split("=")[1].strip('"').lower()
     elif device.upper().startswith("LABEL="):
         _label = device.split("=")[1]
-        cmd = "blkid -t LABEL={}".format(_label)
-        res = __salt__["cmd.run_all"]("{}".format(cmd))
+        cmd = f"blkid -t LABEL={_label}"
+        res = __salt__["cmd.run_all"](f"{cmd}")
         if res["retcode"] > 0:
-            ret["comment"] = "Unable to find device with label {}.".format(_label)
+            ret["comment"] = f"Unable to find device with label {_label}."
             ret["result"] = False
             return ret
         else:
@@ -432,7 +432,7 @@ def mounted(
                     )
                     if size_match:
                         converted_size = _size_convert(size_match)
-                        opt = "size={}k".format(converted_size)
+                        opt = f"size={converted_size}k"
                     # make cifs option user synonym for option username which is reported by /proc/mounts
                     if fstype in ["cifs"] and opt.split("=")[0] == "user":
                         opt = "username={}".format(opt.split("=")[1])
@@ -460,9 +460,9 @@ def mounted(
                         )
                         if size_match:
                             converted_size = _size_convert(size_match)
-                            opt = "size={}k".format(converted_size)
+                            opt = f"size={converted_size}k"
                             _active_superopts.remove(_active_opt)
-                            _active_opt = "size={}k".format(converted_size)
+                            _active_opt = f"size={converted_size}k"
                             _active_superopts.append(_active_opt)
 
                     if (
@@ -504,11 +504,34 @@ def mounted(
                                 )
                                 ret["result"] = mount_result
                             else:
-                                ret["result"] = False
-                                ret["comment"] = "Unable to unmount {}: {}.".format(
-                                    real_name, unmount_result
+                                # If the first attempt at unmounting fails,
+                                # run again as a lazy umount.
+                                ret["changes"]["umount"] = (
+                                    "Forced a lazy unmount and mount "
+                                    + "because the previous unmount failed "
+                                    + "and because the "
+                                    + "options ({}) changed".format(
+                                        ",".join(sorted(trigger_remount))
+                                    )
+                                )
+                                unmount_result = __salt__["mount.umount"](
+                                    real_name, lazy=True
                                 )
-                                return ret
+                                if unmount_result is True:
+                                    mount_result = __salt__["mount.mount"](
+                                        real_name,
+                                        device,
+                                        mkmnt=mkmnt,
+                                        fstype=fstype,
+                                        opts=opts,
+                                    )
+                                    ret["result"] = mount_result
+                                else:
+                                    ret["result"] = False
+                                    ret["comment"] = "Unable to unmount {}: {}.".format(
+                                        real_name, unmount_result
+                                    )
+                                    return ret
                         else:
                             ret["changes"]["umount"] = (
                                 "Forced remount because "
@@ -551,7 +574,7 @@ def mounted(
                             if fstype in ["nfs", "cvfs"] or fstype.startswith("fuse"):
                                 ret["changes"]["umount"] = (
                                     "Forced unmount and mount because "
-                                    + "options ({}) changed".format(opt)
+                                    + f"options ({opt}) changed"
                                 )
                                 unmount_result = __salt__["mount.umount"](real_name)
                                 if unmount_result is True:
@@ -564,15 +587,32 @@ def mounted(
                                     )
                                     ret["result"] = mount_result
                                 else:
-                                    ret["result"] = False
-                                    ret["comment"] = "Unable to unmount {}: {}.".format(
-                                        real_name, unmount_result
+                                    # If the first attempt at unmounting fails,
+                                    # run again as a lazy umount.
+                                    unmount_result = __salt__["mount.umount"](
+                                        real_name, lazy=True
                                     )
-                                    return ret
+                                    if unmount_result is True:
+                                        mount_result = __salt__["mount.mount"](
+                                            real_name,
+                                            device,
+                                            mkmnt=mkmnt,
+                                            fstype=fstype,
+                                            opts=opts,
+                                        )
+                                        ret["result"] = mount_result
+                                    else:
+                                        ret["result"] = False
+                                        ret[
+                                            "comment"
+                                        ] = "Unable to unmount {}: {}.".format(
+                                            real_name, unmount_result
+                                        )
+                                        return ret
                             else:
                                 ret["changes"]["umount"] = (
                                     "Forced remount because "
-                                    + "options ({}) changed".format(opt)
+                                    + f"options ({opt}) changed"
                                 )
                                 remount_result = __salt__["mount.remount"](
                                     real_name,
@@ -638,13 +678,11 @@ def mounted(
             if __opts__["test"]:
                 ret["result"] = None
                 if os.path.exists(name):
-                    ret["comment"] = "{} would be mounted".format(name)
+                    ret["comment"] = f"{name} would be mounted"
                 elif mkmnt:
-                    ret["comment"] = "{} would be created and mounted".format(name)
+                    ret["comment"] = f"{name} would be created and mounted"
                 else:
-                    ret[
-                        "comment"
-                    ] = "{} does not exist and would not be created".format(name)
+                    ret["comment"] = f"{name} does not exist and would not be created"
                 return ret
 
             if not os.path.exists(name) and not mkmnt:
@@ -668,7 +706,7 @@ def mounted(
             if __opts__["test"]:
                 ret["result"] = None
                 if mkmnt:
-                    ret["comment"] = "{} would be created, but not mounted".format(name)
+                    ret["comment"] = f"{name} would be created, but not mounted"
                 else:
                     ret[
                         "comment"
@@ -677,14 +715,14 @@ def mounted(
                     )
             elif mkmnt:
                 __salt__["file.mkdir"](name, user=user)
-                ret["comment"] = "{} was created, not mounted".format(name)
+                ret["comment"] = f"{name} was created, not mounted"
             else:
-                ret["comment"] = "{} not present and not mounted".format(name)
+                ret["comment"] = f"{name} not present and not mounted"
         else:
             if __opts__["test"]:
-                ret["comment"] = "{} would not be mounted".format(name)
+                ret["comment"] = f"{name} would not be mounted"
             else:
-                ret["comment"] = "{} not mounted".format(name)
+                ret["comment"] = f"{name} not mounted"
 
     if persist:
         if "/etc/fstab" == config:
@@ -745,7 +783,7 @@ def mounted(
                             name
                         )
                     else:
-                        comment = "The {} fstab entry must be updated.".format(name)
+                        comment = f"The {name} fstab entry must be updated."
                 else:
                     ret["result"] = False
                     comment = (
@@ -824,25 +862,25 @@ def swap(name, persist=True, config="/etc/fstab"):
     if __salt__["file.is_link"](name):
         real_swap_device = __salt__["file.readlink"](name)
         if not real_swap_device.startswith("/"):
-            real_swap_device = "/dev/{}".format(os.path.basename(real_swap_device))
+            real_swap_device = f"/dev/{os.path.basename(real_swap_device)}"
     else:
         real_swap_device = name
 
     if real_swap_device in on_:
-        ret["comment"] = "Swap {} already active".format(name)
+        ret["comment"] = f"Swap {name} already active"
     elif __opts__["test"]:
         ret["result"] = None
-        ret["comment"] = "Swap {} is set to be activated".format(name)
+        ret["comment"] = f"Swap {name} is set to be activated"
     else:
         __salt__["mount.swapon"](real_swap_device)
 
         on_ = __salt__["mount.swaps"]()
 
         if real_swap_device in on_:
-            ret["comment"] = "Swap {} activated".format(name)
+            ret["comment"] = f"Swap {name} activated"
             ret["changes"] = on_[real_swap_device]
         else:
-            ret["comment"] = "Swap {} failed to activate".format(name)
+            ret["comment"] = f"Swap {name} failed to activate"
             ret["result"] = False
 
     if persist:
@@ -949,7 +987,7 @@ def unmounted(
         # The mount is present! Unmount it
         if __opts__["test"]:
             ret["result"] = None
-            ret["comment"] = "Mount point {} is mounted but should not be".format(name)
+            ret["comment"] = f"Mount point {name} is mounted but should not be"
             return ret
         if device:
             out = __salt__["mount.umount"](name, device, user=user)
@@ -1041,10 +1079,10 @@ def mod_watch(name, user=None, **kwargs):
             name, kwargs["device"], False, kwargs["fstype"], kwargs["opts"], user=user
         )
         if out:
-            ret["comment"] = "{} remounted".format(name)
+            ret["comment"] = f"{name} remounted"
         else:
             ret["result"] = False
-            ret["comment"] = "{} failed to remount: {}".format(name, out)
+            ret["comment"] = f"{name} failed to remount: {out}"
     else:
         ret["comment"] = "Watch not supported in {} at this time".format(kwargs["sfun"])
     return ret
@@ -1064,7 +1102,7 @@ def _convert_to(maybe_device, convert_to):
     if (
         not convert_to
         or (convert_to == "device" and maybe_device.startswith("/"))
-        or maybe_device.startswith("{}=".format(convert_to.upper()))
+        or maybe_device.startswith(f"{convert_to.upper()}=")
     ):
         return maybe_device
 
@@ -1080,7 +1118,7 @@ def _convert_to(maybe_device, convert_to):
             result = next(iter(blkid))
         else:
             key = convert_to.upper()
-            result = "{}={}".format(key, next(iter(blkid.values()))[key])
+            result = f"{key}={next(iter(blkid.values()))[key]}"
 
     return result
 
@@ -1232,7 +1270,7 @@ def fstab_present(
             msg = "{} entry will be written in {}."
             ret["comment"].append(msg.format(fs_file, config))
             if mount:
-                msg = "Will mount {} on {}".format(name, fs_file)
+                msg = f"Will mount {name} on {fs_file}"
                 ret["comment"].append(msg)
         elif out == "change":
             msg = "{} entry will be updated in {}."
@@ -1290,7 +1328,7 @@ def fstab_present(
                 ret["result"] = False
                 msg = "Error while mounting {}".format(out.split(":", maxsplit=1)[1])
             else:
-                msg = "Mounted {} on {}".format(name, fs_file)
+                msg = f"Mounted {name} on {fs_file}"
             ret["comment"].append(msg)
     elif out == "change":
         ret["changes"]["persist"] = out

tests/pytests/unit/modules/test_mount.py

@@ -669,6 +669,21 @@ def test_umount():
         mock.assert_called_once_with("/mountpoint", disk="/path/to/my.qcow")
 
 
+def test_umount_lazy_true():
+    """
+    Attempt to lazy unmount a device by specifying the
+    directory it is mounted on
+    """
+    mock_mount_active = MagicMock(return_value={"name": "name"})
+    with patch.object(mount, "active", mock_mount_active):
+        mock_cmd = MagicMock(return_value={"retcode": True, "stderr": True})
+        with patch.dict(mount.__salt__, {"cmd.run_all": mock_cmd}):
+            mount.umount("name", lazy=True)
+            mock_cmd.assert_called_once_with(
+                "umount -l 'name'", runas=None, python_shell=False
+            )
+
+
 def test_is_fuse_exec():
     """
     Returns true if the command passed is a fuse mountable application