From 368fd9d01e09a667867de0d5bd51782321fa9037 Mon Sep 17 00:00:00 2001 From: mochizuki875 Date: Tue, 20 Dec 2022 22:12:15 +0900 Subject: [PATCH 1/2] check umount result --- staging/src/k8s.io/mount-utils/mount_linux.go | 67 ++++++++++--------- 1 file changed, 34 insertions(+), 33 deletions(-) diff --git a/staging/src/k8s.io/mount-utils/mount_linux.go b/staging/src/k8s.io/mount-utils/mount_linux.go index a7c00ea1b6d..50e9382828f 100644 --- a/staging/src/k8s.io/mount-utils/mount_linux.go +++ b/staging/src/k8s.io/mount-utils/mount_linux.go @@ -363,19 +363,7 @@ func (mounter *Mounter) Unmount(target string) error { command := exec.Command("umount", target) output, err := command.CombinedOutput() if err != nil { - if err.Error() == errNoChildProcesses { - if command.ProcessState.Success() { - // We don't consider errNoChildProcesses an error if the process itself succeeded (see - k/k issue #103753). - return nil - } - // Rewrite err with the actual exit error of the process. - err = &exec.ExitError{ProcessState: command.ProcessState} - } - if mounter.withSafeNotMountedBehavior && strings.Contains(string(output), errNotMounted) { - klog.V(4).Infof("ignoring 'not mounted' error for %s", target) - return nil - } - return fmt.Errorf("unmount failed: %v\nUnmounting arguments: %s\nOutput: %s", err, target, string(output)) + return checkUmountError(target, command, output, err, mounter.withSafeNotMountedBehavior) } return nil } @@ -383,11 +371,11 @@ func (mounter *Mounter) Unmount(target string) error { // 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(mounter, target, umountTimeout) + err := tryUnmount(target, mounter.withSafeNotMountedBehavior, 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) + err = forceUmount(target, mounter.withSafeNotMountedBehavior) } return err } @@ -775,13 +763,13 @@ func (mounter *Mounter) IsMountPoint(file string) (bool, error) { } // tryUnmount calls plain "umount" and waits for unmountTimeout for it to finish. -func tryUnmount(mounter *Mounter, path string, unmountTimeout time.Duration) error { - klog.V(4).Infof("Unmounting %s", path) +func tryUnmount(target string, withSafeNotMountedBehavior bool, unmountTimeout time.Duration) error { + klog.V(4).Infof("Unmounting %s", target) ctx, cancel := context.WithTimeout(context.Background(), unmountTimeout) defer cancel() - cmd := exec.CommandContext(ctx, "umount", path) - out, cmderr := cmd.CombinedOutput() + command := exec.CommandContext(ctx, "umount", target) + output, err := command.CombinedOutput() // CombinedOutput() does not return DeadlineExceeded, make sure it's // propagated on timeout. @@ -789,22 +777,35 @@ func tryUnmount(mounter *Mounter, path string, unmountTimeout time.Duration) err return ctx.Err() } - if cmderr != nil { - if mounter.withSafeNotMountedBehavior && strings.Contains(string(out), errNotMounted) { - klog.V(4).Infof("ignoring 'not mounted' error for %s", path) + if err != nil { + return checkUmountError(target, command, output, err, withSafeNotMountedBehavior) + } + return nil +} + +func forceUmount(target string, withSafeNotMountedBehavior bool) error { + command := exec.Command("umount", "-f", target) + output, err := command.CombinedOutput() + + if err != nil { + return checkUmountError(target, command, output, err, withSafeNotMountedBehavior) + } + return nil +} + +// checkUmountError checks a result of umount command and determine a return value. +func checkUmountError(target string, command *exec.Cmd, output []byte, err error, withSafeNotMountedBehavior bool) error { + if err.Error() == errNoChildProcesses { + if command.ProcessState.Success() { + // We don't consider errNoChildProcesses an error if the process itself succeeded (see - k/k issue #103753). return nil } - return fmt.Errorf("unmount failed: %v\nUnmounting arguments: %s\nOutput: %s", cmderr, path, string(out)) + // Rewrite err with the actual exit error of the process. + err = &exec.ExitError{ProcessState: command.ProcessState} } - 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)) + if withSafeNotMountedBehavior && strings.Contains(string(output), errNotMounted) { + klog.V(4).Infof("ignoring 'not mounted' error for %s", target) + return nil } - return nil + return fmt.Errorf("unmount failed: %v\nUnmounting arguments: %s\nOutput: %s", err, target, string(output)) } From bf77290bbffaffd20e9db06e14fe974d814707ef Mon Sep 17 00:00:00 2001 From: mochizuki875 Date: Wed, 11 Jan 2023 01:54:25 +0900 Subject: [PATCH 2/2] add unit test for check unmounted behavior of Unmount --- .../k8s.io/mount-utils/mount_linux_test.go | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/staging/src/k8s.io/mount-utils/mount_linux_test.go b/staging/src/k8s.io/mount-utils/mount_linux_test.go index 8083656aa47..26291030a45 100644 --- a/staging/src/k8s.io/mount-utils/mount_linux_test.go +++ b/staging/src/k8s.io/mount-utils/mount_linux_test.go @@ -26,6 +26,7 @@ import ( "reflect" "strings" "testing" + "time" utilexec "k8s.io/utils/exec" testexec "k8s.io/utils/exec/testing" @@ -620,3 +621,28 @@ func makeFakeCommandAction(stdout string, err error) testexec.FakeCommandAction return testexec.InitFakeCmd(&c, cmd, args...) } } + +func TestNotMountedBehaviorOfUnmount(t *testing.T) { + target, err := ioutil.TempDir("", "kubelet-umount") + if err != nil { + t.Errorf("Cannot create temp dir: %v", err) + } + + defer os.RemoveAll(target) + + m := Mounter{withSafeNotMountedBehavior: true} + if err = m.Unmount(target); err != nil { + t.Errorf(`Expect complete Unmount(), but it dose not: %v`, err) + } + + if err = tryUnmount(target, m.withSafeNotMountedBehavior, time.Minute); err != nil { + t.Errorf(`Expect complete tryUnmount(), but it does not: %v`, err) + } + + // forceUmount exec "umount -f", so skip this case if user is not root. + if os.Getuid() == 0 { + if err = forceUmount(target, m.withSafeNotMountedBehavior); err != nil { + t.Errorf(`Expect complete forceUnmount(), but it does not: %v`, err) + } + } +}