diff --git a/pkg/util/mount/BUILD b/pkg/util/mount/BUILD index 221afb7a9c6..c9b9ce8a47e 100644 --- a/pkg/util/mount/BUILD +++ b/pkg/util/mount/BUILD @@ -9,6 +9,7 @@ go_library( "exec_mount_unsupported.go", "fake.go", "mount.go", + "mount_helper.go", "mount_linux.go", "mount_unsupported.go", "mount_windows.go", @@ -67,6 +68,7 @@ go_test( name = "go_default_test", srcs = [ "exec_mount_test.go", + "mount_helper_test.go", "mount_linux_test.go", "mount_test.go", "mount_windows_test.go", diff --git a/pkg/util/mount/fake.go b/pkg/util/mount/fake.go index 06e0fcccdc1..0e2952f3e02 100644 --- a/pkg/util/mount/fake.go +++ b/pkg/util/mount/fake.go @@ -30,6 +30,8 @@ type FakeMounter struct { MountPoints []MountPoint Log []FakeAction Filesystem map[string]FileType + // Error to return for a path when calling IsLikelyNotMountPoint + MountCheckErrors map[string]error // Some tests run things in parallel, make sure the mounter does not produce // any golang's DATA RACE warnings. mutex sync.Mutex @@ -119,6 +121,7 @@ func (f *FakeMounter) Unmount(target string) error { } f.MountPoints = newMountpoints f.Log = append(f.Log, FakeAction{Action: FakeActionUnmount, Target: absTarget}) + delete(f.MountCheckErrors, target) return nil } @@ -141,7 +144,12 @@ func (f *FakeMounter) IsLikelyNotMountPoint(file string) (bool, error) { f.mutex.Lock() defer f.mutex.Unlock() - _, err := os.Stat(file) + err := f.MountCheckErrors[file] + if err != nil { + return false, err + } + + _, err = os.Stat(file) if err != nil { return true, err } diff --git a/pkg/util/mount/mount_helper.go b/pkg/util/mount/mount_helper.go new file mode 100644 index 00000000000..a06871e4789 --- /dev/null +++ b/pkg/util/mount/mount_helper.go @@ -0,0 +1,124 @@ +/* +Copyright 2018 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package mount + +import ( + "fmt" + "os" + "syscall" + + "k8s.io/klog" +) + +// CleanupMountPoint unmounts the given path and +// deletes the remaining directory if successful. +// if extensiveMountPointCheck is true +// IsNotMountPoint will be called instead of IsLikelyNotMountPoint. +// IsNotMountPoint is more expensive but properly handles bind mounts within the same fs. +func CleanupMountPoint(mountPath string, mounter Interface, extensiveMountPointCheck bool) error { + // mounter.ExistsPath cannot be used because for containerized kubelet, we need to check + // the path in the kubelet container, not on the host. + 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) + } + return doCleanupMountPoint(mountPath, mounter, extensiveMountPointCheck, corruptedMnt) +} + +// doCleanupMountPoint unmounts the given path and +// deletes the remaining directory if successful. +// if extensiveMountPointCheck is true +// IsNotMountPoint will be called instead of IsLikelyNotMountPoint. +// IsNotMountPoint is more expensive but properly handles bind mounts within the same fs. +// if corruptedMnt is true, it means that the mountPath is a corrupted mountpoint, and the mount point check +// will be skipped +func doCleanupMountPoint(mountPath string, mounter Interface, extensiveMountPointCheck bool, corruptedMnt bool) error { + if !corruptedMnt { + var notMnt bool + var err error + if extensiveMountPointCheck { + notMnt, err = IsNotMountPoint(mounter, mountPath) + } else { + notMnt, err = mounter.IsLikelyNotMountPoint(mountPath) + } + + 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 + klog.V(4).Infof("%q is a mountpoint, unmounting", mountPath) + if err := mounter.Unmount(mountPath); err != nil { + return err + } + + notMnt, mntErr := mounter.IsLikelyNotMountPoint(mountPath) + if mntErr != nil { + return mntErr + } + if notMnt { + klog.V(4).Infof("%q is unmounted, deleting the directory", mountPath) + return os.Remove(mountPath) + } + return fmt.Errorf("Failed to unmount path %v", mountPath) +} + +// TODO: clean this up to use pkg/util/file/FileExists +// PathExists returns true if the specified path exists. +func PathExists(path string) (bool, error) { + _, err := os.Stat(path) + if err == nil { + return true, nil + } else if os.IsNotExist(err) { + return false, nil + } else if IsCorruptedMnt(err) { + return true, err + } else { + return false, err + } +} + +// IsCorruptedMnt return true if err is about corrupted mount point +func IsCorruptedMnt(err error) bool { + if err == nil { + return false + } + var underlyingError error + switch pe := err.(type) { + case nil: + return false + case *os.PathError: + underlyingError = pe.Err + case *os.LinkError: + underlyingError = pe.Err + case *os.SyscallError: + underlyingError = pe.Err + } + + return underlyingError == syscall.ENOTCONN || underlyingError == syscall.ESTALE || underlyingError == syscall.EIO +} diff --git a/pkg/util/mount/mount_helper_test.go b/pkg/util/mount/mount_helper_test.go new file mode 100644 index 00000000000..3afcca37986 --- /dev/null +++ b/pkg/util/mount/mount_helper_test.go @@ -0,0 +1,152 @@ +/* +Copyright 2018 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package mount + +import ( + "fmt" + "io/ioutil" + "os" + "path/filepath" + "syscall" + "testing" +) + +func TestDoCleanupMountPoint(t *testing.T) { + const testMount = "test-mount" + const defaultPerm = 0750 + + tests := map[string]struct { + corruptedMnt bool + // Function that prepares the directory structure for the test under + // the given base directory. + // Returns a fake MountPoint, a fake error for the mount point, + // and error if the prepare function encountered a fatal error. + prepare func(base string) (MountPoint, error, error) + expectErr bool + }{ + "mount-ok": { + prepare: func(base string) (MountPoint, error, error) { + path := filepath.Join(base, testMount) + if err := os.MkdirAll(path, defaultPerm); err != nil { + return MountPoint{}, nil, err + } + return MountPoint{Device: "/dev/sdb", Path: path}, nil, nil + }, + }, + "mount-corrupted": { + prepare: func(base string) (MountPoint, error, error) { + path := filepath.Join(base, testMount) + if err := os.MkdirAll(path, defaultPerm); err != nil { + return MountPoint{}, nil, err + } + return MountPoint{Device: "/dev/sdb", Path: path}, os.NewSyscallError("fake", syscall.ESTALE), nil + }, + corruptedMnt: true, + }, + "mount-err-not-corrupted": { + prepare: func(base string) (MountPoint, error, error) { + path := filepath.Join(base, testMount) + if err := os.MkdirAll(path, defaultPerm); err != nil { + return MountPoint{}, nil, err + } + return MountPoint{Device: "/dev/sdb", Path: path}, os.NewSyscallError("fake", syscall.ETIMEDOUT), nil + }, + expectErr: true, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + + tmpDir, err := ioutil.TempDir("", "unmount-mount-point-test") + if err != nil { + t.Fatalf("failed to create tmpdir: %v", err) + } + defer os.RemoveAll(tmpDir) + + if tt.prepare == nil { + t.Fatalf("prepare function required") + } + + mountPoint, mountError, err := tt.prepare(tmpDir) + if err != nil { + t.Fatalf("failed to prepare test: %v", err) + } + + fake := &FakeMounter{ + MountPoints: []MountPoint{mountPoint}, + MountCheckErrors: map[string]error{mountPoint.Path: mountError}, + } + + err = doCleanupMountPoint(mountPoint.Path, fake, true, tt.corruptedMnt) + if tt.expectErr { + if err == nil { + t.Errorf("test %s failed, expected error, got none", name) + } + if err := validateDirExists(mountPoint.Path); err != nil { + t.Errorf("test %s failed, mount path doesn't exist: %v", name, err) + } + } + if !tt.expectErr { + if err != nil { + t.Errorf("test %s failed: %v", name, err) + } + if err := validateDirNotExists(mountPoint.Path); err != nil { + t.Errorf("test %s failed, mount path still exists: %v", name, err) + } + } + }) + } +} + +func validateDirEmpty(dir string) error { + files, err := ioutil.ReadDir(dir) + if err != nil { + return err + } + + if len(files) != 0 { + return fmt.Errorf("Directory %q is not empty", dir) + } + return nil +} + +func validateDirExists(dir string) error { + _, err := ioutil.ReadDir(dir) + if err != nil { + return err + } + return nil +} + +func validateDirNotExists(dir string) error { + _, err := ioutil.ReadDir(dir) + if os.IsNotExist(err) { + return nil + } + if err != nil { + return err + } + return fmt.Errorf("dir %q still exists", dir) +} + +func validateFileExists(file string) error { + if _, err := os.Stat(file); err != nil { + return err + } + return nil +} diff --git a/pkg/util/mount/mount_linux.go b/pkg/util/mount/mount_linux.go index f3574f884a1..85a90169687 100644 --- a/pkg/util/mount/mount_linux.go +++ b/pkg/util/mount/mount_linux.go @@ -891,15 +891,22 @@ func doCleanSubPaths(mounter Interface, podDir string, volumeName string) error // scan /var/lib/kubelet/pods//volume-subpaths///* fullContainerDirPath := filepath.Join(subPathDir, containerDir.Name()) - subPaths, err := ioutil.ReadDir(fullContainerDirPath) - if err != nil { - return fmt.Errorf("error reading %s: %s", fullContainerDirPath, err) - } - for _, subPath := range subPaths { - if err = doCleanSubPath(mounter, fullContainerDirPath, subPath.Name()); err != nil { + err = filepath.Walk(fullContainerDirPath, func(path string, info os.FileInfo, err error) error { + if path == fullContainerDirPath { + // Skip top level directory + return nil + } + + // pass through errors and let doCleanSubPath handle them + if err = doCleanSubPath(mounter, fullContainerDirPath, filepath.Base(path)); err != nil { return err } + return nil + }) + if err != nil { + return fmt.Errorf("error processing %s: %s", fullContainerDirPath, err) } + // Whole container has been processed, remove its directory. if err := os.Remove(fullContainerDirPath); err != nil { return fmt.Errorf("error deleting %s: %s", fullContainerDirPath, err) @@ -926,22 +933,12 @@ func doCleanSubPath(mounter Interface, fullContainerDirPath, subPathIndex string // process /var/lib/kubelet/pods//volume-subpaths/// klog.V(4).Infof("Cleaning up subpath mounts for subpath %v", subPathIndex) fullSubPath := filepath.Join(fullContainerDirPath, subPathIndex) - notMnt, err := IsNotMountPoint(mounter, fullSubPath) - if err != nil { - return fmt.Errorf("error checking %s for mount: %s", fullSubPath, err) + + if err := CleanupMountPoint(fullSubPath, mounter, true); err != nil { + return fmt.Errorf("error cleaning subpath mount %s: %s", fullSubPath, err) } - // Unmount it - if !notMnt { - if err = mounter.Unmount(fullSubPath); err != nil { - return fmt.Errorf("error unmounting %s: %s", fullSubPath, err) - } - klog.V(5).Infof("Unmounted %s", fullSubPath) - } - // Remove it *non*-recursively, just in case there were some hiccups. - if err = os.Remove(fullSubPath); err != nil { - return fmt.Errorf("error deleting %s: %s", fullSubPath, err) - } - klog.V(5).Infof("Removed %s", fullSubPath) + + klog.V(4).Infof("Successfully cleaned subpath directory %s", fullSubPath) return nil } diff --git a/pkg/util/mount/mount_linux_test.go b/pkg/util/mount/mount_linux_test.go index 89dbfdf041c..cbd0276322a 100644 --- a/pkg/util/mount/mount_linux_test.go +++ b/pkg/util/mount/mount_linux_test.go @@ -666,44 +666,6 @@ func TestSafeMakeDir(t *testing.T) { } } -func validateDirEmpty(dir string) error { - files, err := ioutil.ReadDir(dir) - if err != nil { - return err - } - - if len(files) != 0 { - return fmt.Errorf("Directory %q is not empty", dir) - } - return nil -} - -func validateDirExists(dir string) error { - _, err := ioutil.ReadDir(dir) - if err != nil { - return err - } - return nil -} - -func validateDirNotExists(dir string) error { - _, err := ioutil.ReadDir(dir) - if os.IsNotExist(err) { - return nil - } - if err != nil { - return err - } - return fmt.Errorf("dir %q still exists", dir) -} - -func validateFileExists(file string) error { - if _, err := os.Stat(file); err != nil { - return err - } - return nil -} - func TestRemoveEmptyDirs(t *testing.T) { defaultPerm := os.FileMode(0750) tests := []struct { diff --git a/pkg/volume/util/BUILD b/pkg/volume/util/BUILD index d53a67c9c11..d2bcab775ee 100644 --- a/pkg/volume/util/BUILD +++ b/pkg/volume/util/BUILD @@ -61,7 +61,6 @@ go_test( "//pkg/apis/core/install:go_default_library", "//pkg/apis/core/v1/helper:go_default_library", "//pkg/kubelet/apis:go_default_library", - "//pkg/util/mount:go_default_library", "//pkg/util/slice:go_default_library", "//pkg/volume:go_default_library", "//staging/src/k8s.io/api/core/v1:go_default_library", @@ -69,8 +68,12 @@ go_test( "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", - "//staging/src/k8s.io/client-go/util/testing:go_default_library", - ], + ] + select({ + "@io_bazel_rules_go//go/platform:linux": [ + "//staging/src/k8s.io/client-go/util/testing:go_default_library", + ], + "//conditions:default": [], + }), ) filegroup( diff --git a/pkg/volume/util/util.go b/pkg/volume/util/util.go index 71a5a473629..18e24d69f05 100644 --- a/pkg/volume/util/util.go +++ b/pkg/volume/util/util.go @@ -23,9 +23,8 @@ import ( "path" "path/filepath" "strings" - "syscall" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" storage "k8s.io/api/storage/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" @@ -128,8 +127,9 @@ func SetReady(dir string) { // UnmountPath is a common unmount routine that unmounts the given path and // deletes the remaining directory if successful. +// TODO: Remove this function and change callers to call mount pkg directly func UnmountPath(mountPath string, mounter mount.Interface) error { - return UnmountMountPoint(mountPath, mounter, false /* extensiveMountPointCheck */) + return mount.CleanupMountPoint(mountPath, mounter, false /* extensiveMountPointCheck */) } // UnmountMountPoint is a common unmount routine that unmounts the given path and @@ -137,93 +137,21 @@ func UnmountPath(mountPath string, mounter mount.Interface) error { // if extensiveMountPointCheck is true // IsNotMountPoint will be called instead of IsLikelyNotMountPoint. // IsNotMountPoint is more expensive but properly handles bind mounts. +// TODO: Change callers to call mount pkg directly func UnmountMountPoint(mountPath string, mounter mount.Interface, extensiveMountPointCheck bool) 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) - } - return doUnmountMountPoint(mountPath, mounter, extensiveMountPointCheck, corruptedMnt) -} - -// doUnmountMountPoint is a common unmount routine that unmounts the given path and -// deletes the remaining directory if successful. -// if extensiveMountPointCheck is true -// IsNotMountPoint will be called instead of IsLikelyNotMountPoint. -// IsNotMountPoint is more expensive but properly handles bind mounts. -// if corruptedMnt is true, it means that the mountPath is a corrupted mountpoint, Take it as an argument for convenience of testing -func doUnmountMountPoint(mountPath string, mounter mount.Interface, extensiveMountPointCheck bool, corruptedMnt bool) error { - if !corruptedMnt { - var notMnt bool - var err error - if extensiveMountPointCheck { - notMnt, err = mount.IsNotMountPoint(mounter, mountPath) - } else { - notMnt, err = mounter.IsLikelyNotMountPoint(mountPath) - } - - 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 - klog.V(4).Infof("%q is a mountpoint, unmounting", mountPath) - if err := mounter.Unmount(mountPath); err != nil { - return err - } - notMnt, mntErr := mounter.IsLikelyNotMountPoint(mountPath) - if mntErr != nil { - return mntErr - } - if notMnt { - klog.V(4).Infof("%q is unmounted, deleting the directory", mountPath) - return os.Remove(mountPath) - } - return fmt.Errorf("Failed to unmount path %v", mountPath) + return mount.CleanupMountPoint(mountPath, mounter, extensiveMountPointCheck) } // PathExists returns true if the specified path exists. +// TODO: Change callers to call mount pkg directly func PathExists(path string) (bool, error) { - _, err := os.Stat(path) - if err == nil { - return true, nil - } else if os.IsNotExist(err) { - return false, nil - } else if IsCorruptedMnt(err) { - return true, err - } - - return false, err + return mount.PathExists(path) } // IsCorruptedMnt return true if err is about corrupted mount point +// TODO: Change callers to call mount pkg directly func IsCorruptedMnt(err error) bool { - if err == nil { - return false - } - var underlyingError error - switch pe := err.(type) { - case nil: - return false - case *os.PathError: - underlyingError = pe.Err - case *os.LinkError: - underlyingError = pe.Err - case *os.SyscallError: - underlyingError = pe.Err - } - - return underlyingError == syscall.ENOTCONN || underlyingError == syscall.ESTALE || underlyingError == syscall.EIO + return mount.IsCorruptedMnt(err) } // GetSecretForPod locates secret by name in the pod's namespace and returns secret map diff --git a/pkg/volume/util/util_test.go b/pkg/volume/util/util_test.go index c7b7d1a1fb2..257410e226f 100644 --- a/pkg/volume/util/util_test.go +++ b/pkg/volume/util/util_test.go @@ -22,16 +22,14 @@ import ( "runtime" "testing" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/sets" - utiltesting "k8s.io/client-go/util/testing" // util.go uses api.Codecs.LegacyCodec so import this package to do some // resource initialization. "hash/fnv" _ "k8s.io/kubernetes/pkg/apis/core/install" - "k8s.io/kubernetes/pkg/util/mount" "reflect" "strings" @@ -354,45 +352,6 @@ func TestZonesToSet(t *testing.T) { } } -func TestDoUnmountMountPoint(t *testing.T) { - - tmpDir1, err1 := utiltesting.MkTmpdir("umount_test1") - if err1 != nil { - t.Fatalf("error creating temp dir: %v", err1) - } - defer os.RemoveAll(tmpDir1) - - tmpDir2, err2 := utiltesting.MkTmpdir("umount_test2") - if err2 != nil { - t.Fatalf("error creating temp dir: %v", err2) - } - defer os.RemoveAll(tmpDir2) - - // Second part: want no error - tests := []struct { - mountPath string - corruptedMnt bool - }{ - { - mountPath: tmpDir1, - corruptedMnt: true, - }, - { - mountPath: tmpDir2, - corruptedMnt: false, - }, - } - - fake := &mount.FakeMounter{} - - for _, tt := range tests { - err := doUnmountMountPoint(tt.mountPath, fake, false, tt.corruptedMnt) - if err != nil { - t.Errorf("err Expected nil, but got: %v", err) - } - } -} - func TestCalculateTimeoutForVolume(t *testing.T) { pv := &v1.PersistentVolume{ Spec: v1.PersistentVolumeSpec{ diff --git a/test/e2e/storage/testsuites/subpath.go b/test/e2e/storage/testsuites/subpath.go index bdf4a75c61e..f7343b680c5 100644 --- a/test/e2e/storage/testsuites/subpath.go +++ b/test/e2e/storage/testsuites/subpath.go @@ -22,7 +22,7 @@ import ( "regexp" "strings" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/rand" "k8s.io/apimachinery/pkg/util/wait" @@ -377,6 +377,32 @@ func testSubPath(input *subPathTestInput) { // Pod should fail testPodFailSubpath(input.f, input.pod, true) }) + + It("should be able to unmount after the subpath directory is deleted", func() { + // Change volume container to busybox so we can exec later + input.pod.Spec.Containers[1].Image = imageutils.GetE2EImage(imageutils.BusyBox) + input.pod.Spec.Containers[1].Command = []string{"/bin/sh", "-ec", "sleep 100000"} + + By(fmt.Sprintf("Creating pod %s", input.pod.Name)) + pod, err := input.f.ClientSet.CoreV1().Pods(input.f.Namespace.Name).Create(input.pod) + Expect(err).ToNot(HaveOccurred(), "while creating pod") + defer func() { + By(fmt.Sprintf("Deleting pod %s", pod.Name)) + framework.DeletePodWithWait(input.f, input.f.ClientSet, pod) + }() + + // Wait for pod to be running + err = framework.WaitForPodRunningInNamespace(input.f.ClientSet, pod) + Expect(err).ToNot(HaveOccurred(), "while waiting for pod to be running") + + // Exec into container that mounted the volume, delete subpath directory + rmCmd := fmt.Sprintf("rm -rf %s", input.subPathDir) + _, err = podContainerExec(pod, 1, rmCmd) + Expect(err).ToNot(HaveOccurred(), "while removing subpath directory") + + // Delete pod (from defer) and wait for it to be successfully deleted + }) + // TODO: add a test case for the same disk with two partitions }