From 379daa6aff3a4a2d18986761041e2e990e8ac840 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Tue, 24 Nov 2020 21:30:35 -0500 Subject: [PATCH 1/2] Use force umount for nfs volumes --- pkg/volume/nfs/nfs.go | 9 ++- staging/src/k8s.io/mount-utils/mount.go | 8 +++ .../k8s.io/mount-utils/mount_helper_common.go | 70 +++++++++++++++---- staging/src/k8s.io/mount-utils/mount_linux.go | 49 +++++++++++++ 4 files changed, 120 insertions(+), 16 deletions(-) diff --git a/pkg/volume/nfs/nfs.go b/pkg/volume/nfs/nfs.go index 34f838a8276..9f46118f556 100644 --- a/pkg/volume/nfs/nfs.go +++ b/pkg/volume/nfs/nfs.go @@ -20,6 +20,7 @@ import ( "fmt" "os" "runtime" + "time" "k8s.io/klog/v2" "k8s.io/mount-utils" @@ -61,7 +62,8 @@ var _ volume.PersistentVolumePlugin = &nfsPlugin{} var _ volume.RecyclableVolumePlugin = &nfsPlugin{} const ( - nfsPluginName = "kubernetes.io/nfs" + nfsPluginName = "kubernetes.io/nfs" + unMountTimeout = time.Minute ) func (plugin *nfsPlugin) Init(host volume.VolumeHost) error { @@ -302,6 +304,11 @@ func (c *nfsUnmounter) TearDownAt(dir string) error { // Use extensiveMountPointCheck to consult /proc/mounts. We can't use faster // IsLikelyNotMountPoint (lstat()), since there may be root_squash on the // NFS server and kubelet may not be able to do lstat/stat() there. + forceUmounter, ok := c.mounter.(mount.MounterForceUnmounter) + if ok { + klog.V(4).Infof("Using force unmounter interface") + return mount.CleanupMountWithForce(dir, forceUmounter, true /* extensiveMountPointCheck */, unMountTimeout) + } return mount.CleanupMountPoint(dir, c.mounter, true /* extensiveMountPointCheck */) } diff --git a/staging/src/k8s.io/mount-utils/mount.go b/staging/src/k8s.io/mount-utils/mount.go index c78cf13df91..93b60d3f922 100644 --- a/staging/src/k8s.io/mount-utils/mount.go +++ b/staging/src/k8s.io/mount-utils/mount.go @@ -24,6 +24,7 @@ import ( "os" "path/filepath" "strings" + "time" utilexec "k8s.io/utils/exec" ) @@ -78,6 +79,13 @@ type Interface interface { // the mount interface. var _ Interface = &Mounter{} +type MounterForceUnmounter interface { + Interface + // UnmountWithForce unmounts given target but will retry unmounting with force option + // after given timeout. + UnmountWithForce(target string, umountTimeout time.Duration) error +} + // MountPoint represents a single line in /proc/mounts or /etc/fstab. type MountPoint struct { // nolint: golint Device string diff --git a/staging/src/k8s.io/mount-utils/mount_helper_common.go b/staging/src/k8s.io/mount-utils/mount_helper_common.go index 1d40549b5f4..5691edeabce 100644 --- a/staging/src/k8s.io/mount-utils/mount_helper_common.go +++ b/staging/src/k8s.io/mount-utils/mount_helper_common.go @@ -19,6 +19,7 @@ package mount import ( "fmt" "os" + "time" "k8s.io/klog/v2" ) @@ -40,6 +41,39 @@ func CleanupMountPoint(mountPath string, mounter Interface, extensiveMountPointC return doCleanupMountPoint(mountPath, mounter, extensiveMountPointCheck, corruptedMnt) } +func CleanupMountWithForce(mountPath string, mounter MounterForceUnmounter, extensiveMountPointCheck bool, umountTimeout time.Duration) error { + pathExists, pathErr := PathExists(mountPath) + if !pathExists { + klog.Warningf("Warning: Unmount skipped because path does not exist: %v", mountPath) + return nil + } + corruptedMnt := IsCorruptedMnt(pathErr) + if pathErr != nil && !corruptedMnt { + return fmt.Errorf("Error checking path: %v", pathErr) + } + var notMnt bool + var err error + if !corruptedMnt { + _, err = removePathIfNotMountPoint(mountPath, mounter, extensiveMountPointCheck) + if err != nil { + return err + } + } + + // Unmount the mount path + klog.V(4).Infof("%q is a mountpoint, unmounting", mountPath) + if err := mounter.UnmountWithForce(mountPath, umountTimeout); err != nil { + return err + } + + notMnt, err = removePathIfNotMountPoint(mountPath, mounter, extensiveMountPointCheck) + // mountPath is not a mount point we should return whatever error we saw + if notMnt { + return err + } + return fmt.Errorf("Failed to unmount path %v", mountPath) +} + // doCleanupMountPoint unmounts the given path and // deletes the remaining directory if successful. // if extensiveMountPointCheck is true @@ -51,20 +85,10 @@ func doCleanupMountPoint(mountPath string, mounter Interface, extensiveMountPoin var notMnt bool var err error if !corruptedMnt { - if extensiveMountPointCheck { - notMnt, err = IsNotMountPoint(mounter, mountPath) - } else { - notMnt, err = mounter.IsLikelyNotMountPoint(mountPath) - } - + _, err = removePathIfNotMountPoint(mountPath, mounter, extensiveMountPointCheck) if err != nil { return err } - - if notMnt { - klog.Warningf("Warning: %q is not a mountpoint, deleting", mountPath) - return os.Remove(mountPath) - } } // Unmount the mount path @@ -73,19 +97,35 @@ func doCleanupMountPoint(mountPath string, mounter Interface, extensiveMountPoin return err } + notMnt, err = removePathIfNotMountPoint(mountPath, mounter, extensiveMountPointCheck) + // mountPath is not a mount point we should return whatever error we saw + if notMnt { + return err + } + return fmt.Errorf("Failed to unmount path %v", mountPath) +} + +// removePathIfNotMountPoint verifies if given mountPath is a mount point if not it attempts +// to remove the directory. Returns true and nil if directory was not a mount point and removed. +func removePathIfNotMountPoint(mountPath string, mounter Interface, extensiveMountPointCheck bool) (bool, error) { + var notMnt bool + var err error + if extensiveMountPointCheck { notMnt, err = IsNotMountPoint(mounter, mountPath) } else { notMnt, err = mounter.IsLikelyNotMountPoint(mountPath) } + if err != nil { - return err + return notMnt, err } + if notMnt { - klog.V(4).Infof("%q is unmounted, deleting the directory", mountPath) - return os.Remove(mountPath) + klog.Warningf("Warning: %q is not a mountpoint, deleting", mountPath) + return notMnt, os.Remove(mountPath) } - return fmt.Errorf("Failed to unmount path %v", mountPath) + return notMnt, nil } // PathExists returns true if the specified path exists. diff --git a/staging/src/k8s.io/mount-utils/mount_linux.go b/staging/src/k8s.io/mount-utils/mount_linux.go index 20993cf06b3..f4a45adf244 100644 --- a/staging/src/k8s.io/mount-utils/mount_linux.go +++ b/staging/src/k8s.io/mount-utils/mount_linux.go @@ -19,6 +19,7 @@ limitations under the License. package mount import ( + "context" "fmt" "os" "os/exec" @@ -26,6 +27,7 @@ import ( "strconv" "strings" "syscall" + "time" "k8s.io/klog/v2" utilexec "k8s.io/utils/exec" @@ -53,6 +55,8 @@ type Mounter struct { withSystemd bool } +var _ MounterForceUnmounter = &Mounter{} + // New returns a mount.Interface for the current system. // It provides options to override the default mounter behavior. // mounterPath allows using an alternative to `/bin/mount` for mounting. @@ -268,6 +272,20 @@ func (mounter *Mounter) Unmount(target string) error { return nil } +// UnmountWithForce unmounts given target but will retry unmounting with force option +// after given timeout. +func (mounter *Mounter) UnmountWithForce(target string, umountTimeout time.Duration) error { + err := tryUnmount(target, umountTimeout) + if err != nil { + if err == context.DeadlineExceeded { + klog.V(2).Infof("Timed out waiting for unmount of %s, trying with -f", target) + err = forceUmount(target) + } + return err + } + return nil +} + // List returns a list of all mounted filesystems. func (*Mounter) List() ([]MountPoint, error) { return ListProcMounts(procMountsPath) @@ -573,3 +591,34 @@ func SearchMountPoints(hostSource, mountInfoPath string) ([]string, error) { return refs, nil } + +// tryUnmount calls plain "umount" and waits for unmountTimeout for it to finish. +func tryUnmount(path string, unmountTimeout time.Duration) error { + klog.V(4).Infof("Unmounting %s", path) + ctx, cancel := context.WithTimeout(context.Background(), unmountTimeout) + defer cancel() + + cmd := exec.CommandContext(ctx, "umount", path) + out, cmderr := cmd.CombinedOutput() + + // CombinedOutput() does not return DeadlineExceeded, make sure it's + // propagated on timeout. + if ctx.Err() != nil { + return ctx.Err() + } + + if cmderr != nil { + return fmt.Errorf("unmount failed: %v\nUnmounting arguments: %s\nOutput: %s", cmderr, path, string(out)) + } + return nil +} + +func forceUmount(path string) error { + cmd := exec.Command("umount", "-f", path) + out, cmderr := cmd.CombinedOutput() + + if cmderr != nil { + return fmt.Errorf("unmount failed: %v\nUnmounting arguments: %s\nOutput: %s", cmderr, path, string(out)) + } + return nil +} From 95852d7b8ec6bff61f6bad456ba4a0e9ad4356e1 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Mon, 14 Dec 2020 21:51:33 -0500 Subject: [PATCH 2/2] Only skip unmount if path does not exist and there is no error --- .../k8s.io/mount-utils/mount_helper_common.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/staging/src/k8s.io/mount-utils/mount_helper_common.go b/staging/src/k8s.io/mount-utils/mount_helper_common.go index 5691edeabce..a7987485562 100644 --- a/staging/src/k8s.io/mount-utils/mount_helper_common.go +++ b/staging/src/k8s.io/mount-utils/mount_helper_common.go @@ -30,7 +30,7 @@ import ( // but properly handles bind mounts within the same fs. func CleanupMountPoint(mountPath string, mounter Interface, extensiveMountPointCheck bool) error { pathExists, pathErr := PathExists(mountPath) - if !pathExists { + if !pathExists && pathErr == nil { klog.Warningf("Warning: Unmount skipped because path does not exist: %v", mountPath) return nil } @@ -43,7 +43,7 @@ func CleanupMountPoint(mountPath string, mounter Interface, extensiveMountPointC func CleanupMountWithForce(mountPath string, mounter MounterForceUnmounter, extensiveMountPointCheck bool, umountTimeout time.Duration) error { pathExists, pathErr := PathExists(mountPath) - if !pathExists { + if !pathExists && pathErr == nil { klog.Warningf("Warning: Unmount skipped because path does not exist: %v", mountPath) return nil } @@ -54,8 +54,10 @@ func CleanupMountWithForce(mountPath string, mounter MounterForceUnmounter, exte var notMnt bool var err error if !corruptedMnt { - _, err = removePathIfNotMountPoint(mountPath, mounter, extensiveMountPointCheck) - if err != nil { + notMnt, err = removePathIfNotMountPoint(mountPath, mounter, extensiveMountPointCheck) + // if mountPath was not a mount point - we would have attempted to remove mountPath + // and hence return errors if any. + if err != nil || notMnt { return err } } @@ -85,8 +87,10 @@ func doCleanupMountPoint(mountPath string, mounter Interface, extensiveMountPoin var notMnt bool var err error if !corruptedMnt { - _, err = removePathIfNotMountPoint(mountPath, mounter, extensiveMountPointCheck) - if err != nil { + notMnt, err = removePathIfNotMountPoint(mountPath, mounter, extensiveMountPointCheck) + // if mountPath was not a mount point - we would have attempted to remove mountPath + // and hence return errors if any. + if err != nil || notMnt { return err } }