From 326d4ce072b1176a239f534cf3e961a1f8beea1a Mon Sep 17 00:00:00 2001 From: Keita Mochizuki <37737691+mochizuki875@users.noreply.github.com> Date: Tue, 14 Feb 2023 23:30:27 +0900 Subject: [PATCH 1/3] Revert "Revert #114605: its unit test requires root permission" --- staging/src/k8s.io/mount-utils/mount_linux.go | 67 ++++++++++--------- .../k8s.io/mount-utils/mount_linux_test.go | 25 +++++++ 2 files changed, 59 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 db0beb6d034..f0125fcb489 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 } @@ -799,13 +787,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. @@ -813,22 +801,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)) } 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 0fcb156ee9f..dec91c72050 100644 --- a/staging/src/k8s.io/mount-utils/mount_linux_test.go +++ b/staging/src/k8s.io/mount-utils/mount_linux_test.go @@ -705,3 +705,28 @@ func makeFakeCommandAction(stdout string, err error, cmdFn func()) testexec.Fake 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) + } + } +} From 99d73dfae5757a477fa40fa93d2f0f80e15db8bf Mon Sep 17 00:00:00 2001 From: mochizuki875 Date: Fri, 17 Feb 2023 09:12:46 +0900 Subject: [PATCH 2/3] unit test not requiring priviledge --- .../k8s.io/mount-utils/mount_linux_test.go | 71 ++++++++++++------- 1 file changed, 46 insertions(+), 25 deletions(-) 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 dec91c72050..c34aa0be539 100644 --- a/staging/src/k8s.io/mount-utils/mount_linux_test.go +++ b/staging/src/k8s.io/mount-utils/mount_linux_test.go @@ -24,11 +24,13 @@ import ( "fmt" "io/ioutil" "os" + "os/exec" "reflect" "strings" "sync" "testing" "time" + "unsafe" utilexec "k8s.io/utils/exec" testexec "k8s.io/utils/exec/testing" @@ -607,6 +609,50 @@ func TestDetectSafeNotMountedBehavior(t *testing.T) { } } +func TestCheckUmountError(t *testing.T) { + target := "/test/path" + withSafeNotMountedBehavior := true + + pState := &os.ProcessState{} + var ref_pState reflect.Value = reflect.ValueOf(pState).Elem() + var ref_status reflect.Value = ref_pState.FieldByName("status") + status := (*uint32)(unsafe.Pointer(ref_status.UnsafeAddr())) + *status = 0 + + command := &exec.Cmd{ProcessState: pState} // dummy command return status 0 + + testcases := []struct { + output []byte + err error + expected bool + }{ + { + err: errors.New("wait: no child processes"), + expected: true, + }, + { + output: []byte("umount: /test/path: not mounted."), + err: errors.New("exit status 1"), + expected: true, + }, + { + output: []byte("umount: /test/path: No such file or directory"), + err: errors.New("exit status 1"), + expected: false, + }, + } + + for _, v := range testcases { + if err := checkUmountError(target, command, v.output, v.err, withSafeNotMountedBehavior); (err == nil) != v.expected { + if v.expected { + t.Errorf("Expected to return nil, but did not. err: %s", err) + } else { + t.Errorf("Expected to return error, but did not.") + } + } + } +} + func TestFormat(t *testing.T) { const ( formatCount = 5 @@ -705,28 +751,3 @@ func makeFakeCommandAction(stdout string, err error, cmdFn func()) testexec.Fake 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) - } - } -} From b84ba39a0f13e77fcb41fa1208801d6ac06a1c33 Mon Sep 17 00:00:00 2001 From: mochizuki875 Date: Wed, 22 Feb 2023 16:55:15 +0900 Subject: [PATCH 3/3] run dummy command return status 0 --- staging/src/k8s.io/mount-utils/mount_linux_test.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) 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 c34aa0be539..61823abb9f0 100644 --- a/staging/src/k8s.io/mount-utils/mount_linux_test.go +++ b/staging/src/k8s.io/mount-utils/mount_linux_test.go @@ -30,7 +30,6 @@ import ( "sync" "testing" "time" - "unsafe" utilexec "k8s.io/utils/exec" testexec "k8s.io/utils/exec/testing" @@ -612,14 +611,11 @@ func TestDetectSafeNotMountedBehavior(t *testing.T) { func TestCheckUmountError(t *testing.T) { target := "/test/path" withSafeNotMountedBehavior := true + command := exec.Command("uname", "-r") // dummy command return status 0 - pState := &os.ProcessState{} - var ref_pState reflect.Value = reflect.ValueOf(pState).Elem() - var ref_status reflect.Value = ref_pState.FieldByName("status") - status := (*uint32)(unsafe.Pointer(ref_status.UnsafeAddr())) - *status = 0 - - command := &exec.Cmd{ProcessState: pState} // dummy command return status 0 + if err := command.Run(); err != nil { + t.Errorf("Faild to exec dummy command. err: %s", err) + } testcases := []struct { output []byte