From 2b18d3b6f71a1786c68dee630b0452d20c71f844 Mon Sep 17 00:00:00 2001 From: Ian Chakeres Date: Sat, 1 Jul 2017 15:51:28 -0500 Subject: [PATCH] Fixes bind-mount teardown failure with non-mount point Local volumes Added IsNotMountPoint method to mount utils (pkg/util/mount/mount.go) Added UnmountMountPoint method to volume utils (pkg/volume/util/util.go) Call UnmountMountPoint method from local storage (pkg/volume/local/local.go) IsLikelyNotMountPoint behavior was not modified, so the logic/behavior for UnmountPath is not modified --- .../cm/container_manager_linux_test.go | 8 ++++ .../cm/container_manager_unsupported_test.go | 8 ++++ pkg/util/mount/fake.go | 8 ++++ pkg/util/mount/mount.go | 46 ++++++++++++++++++- pkg/util/mount/mount_linux.go | 13 ++++-- pkg/util/mount/mount_unsupported.go | 12 +++-- pkg/util/mount/nsenter_mount.go | 10 ++++ pkg/util/mount/nsenter_mount_unsupported.go | 8 ++++ pkg/util/removeall/removeall_test.go | 6 +++ pkg/volume/local/local.go | 12 ++--- pkg/volume/util/util.go | 21 ++++++++- 11 files changed, 136 insertions(+), 16 deletions(-) diff --git a/pkg/kubelet/cm/container_manager_linux_test.go b/pkg/kubelet/cm/container_manager_linux_test.go index 12219d0b9b9c..d1cba37ae472 100644 --- a/pkg/kubelet/cm/container_manager_linux_test.go +++ b/pkg/kubelet/cm/container_manager_linux_test.go @@ -47,6 +47,14 @@ func (mi *fakeMountInterface) List() ([]mount.MountPoint, error) { return mi.mountPoints, nil } +func (mi *fakeMountInterface) IsMountPointMatch(mp mount.MountPoint, dir string) bool { + return (mp.Path == dir) +} + +func (mi *fakeMountInterface) IsNotMountPoint(dir string) (bool, error) { + return false, fmt.Errorf("unsupported") +} + func (mi *fakeMountInterface) IsLikelyNotMountPoint(file string) (bool, error) { return false, fmt.Errorf("unsupported") } diff --git a/pkg/kubelet/cm/container_manager_unsupported_test.go b/pkg/kubelet/cm/container_manager_unsupported_test.go index 11d70260e9d9..d1451946fc0e 100644 --- a/pkg/kubelet/cm/container_manager_unsupported_test.go +++ b/pkg/kubelet/cm/container_manager_unsupported_test.go @@ -40,6 +40,14 @@ func (mi *fakeMountInterface) List() ([]mount.MountPoint, error) { return mi.mountPoints, nil } +func (f *fakeMountInterface) IsMountPointMatch(mp mount.MountPoint, dir string) bool { + return (mp.Path == dir) +} + +func (f *fakeMountInterface) IsNotMountPoint(dir string) (bool, error) { + return false, fmt.Errorf("unsupported") +} + func (mi *fakeMountInterface) IsLikelyNotMountPoint(file string) (bool, error) { return false, fmt.Errorf("unsupported") } diff --git a/pkg/util/mount/fake.go b/pkg/util/mount/fake.go index 972bff26a10f..2b71fa0a7285 100644 --- a/pkg/util/mount/fake.go +++ b/pkg/util/mount/fake.go @@ -124,6 +124,14 @@ func (f *FakeMounter) List() ([]MountPoint, error) { return f.MountPoints, nil } +func (f *FakeMounter) IsMountPointMatch(mp MountPoint, dir string) bool { + return (mp.Path == dir) +} + +func (f *FakeMounter) IsNotMountPoint(dir string) (bool, error) { + return IsNotMountPoint(f, dir) +} + func (f *FakeMounter) IsLikelyNotMountPoint(file string) (bool, error) { f.mutex.Lock() defer f.mutex.Unlock() diff --git a/pkg/util/mount/mount.go b/pkg/util/mount/mount.go index 44058042d508..0c458d64b499 100644 --- a/pkg/util/mount/mount.go +++ b/pkg/util/mount/mount.go @@ -44,8 +44,21 @@ type Interface interface { // it could change between chunked reads). This is guaranteed to be // consistent. List() ([]MountPoint, error) - // IsLikelyNotMountPoint determines if a directory is a mountpoint. + // IsMountPointMatch determines if the mountpoint matches the dir + IsMountPointMatch(mp MountPoint, dir string) bool + // IsNotMountPoint determines if a directory is a mountpoint. // It should return ErrNotExist when the directory does not exist. + // IsNotMountPoint is more expensive than IsLikelyNotMountPoint. + // IsNotMountPoint detects bind mounts in linux. + // IsNotMountPoint enumerates all the mountpoints using List() and + // the list of mountpoints may be large, then it uses + // IsMountPointMatch to evaluate whether the directory is a mountpoint + IsNotMountPoint(file string) (bool, error) + // IsLikelyNotMountPoint uses heuristics to determine if a directory + // is a mountpoint. + // It should return ErrNotExist when the directory does not exist. + // IsLikelyNotMountPoint does NOT properly detect all mountpoint types + // most notably linux bind mounts. IsLikelyNotMountPoint(file string) (bool, error) // DeviceOpened determines if the device is in use elsewhere // on the system, i.e. still mounted. @@ -199,3 +212,34 @@ func getDeviceNameFromMount(mounter Interface, mountPath, pluginDir string) (str return path.Base(mountPath), nil } + +// IsNotMountPoint determines if a directory is a mountpoint. +// It should return ErrNotExist when the directory does not exist. +// This method uses the List() of all mountpoints +// It is more extensive than IsLikelyNotMountPoint +// and it detects bind mounts in linux +func IsNotMountPoint(mounter Interface, file string) (bool, error) { + // IsLikelyNotMountPoint provides a quick check + // to determine whether file IS A mountpoint + notMnt, notMntErr := mounter.IsLikelyNotMountPoint(file) + if notMntErr != nil { + return notMnt, notMntErr + } + // identified as mountpoint, so return this fact + if notMnt == false { + return notMnt, nil + } + // check all mountpoints since IsLikelyNotMountPoint + // is not reliable for some mountpoint types + mountPoints, mountPointsErr := mounter.List() + if mountPointsErr != nil { + return notMnt, mountPointsErr + } + for _, mp := range mountPoints { + if mounter.IsMountPointMatch(mp, file) { + notMnt = false + break + } + } + return notMnt, nil +} diff --git a/pkg/util/mount/mount_linux.go b/pkg/util/mount/mount_linux.go index 1685ecb48f08..4c141ad5b0a9 100644 --- a/pkg/util/mount/mount_linux.go +++ b/pkg/util/mount/mount_linux.go @@ -161,6 +161,15 @@ func (*Mounter) List() ([]MountPoint, error) { return listProcMounts(procMountsPath) } +func (mounter *Mounter) IsMountPointMatch(mp MountPoint, dir string) bool { + deletedDir := fmt.Sprintf("%s\\040(deleted)", dir) + return ((mp.Path == dir) || (mp.Path == deletedDir)) +} + +func (mounter *Mounter) IsNotMountPoint(dir string) (bool, error) { + return IsNotMountPoint(mounter, dir) +} + // IsLikelyNotMountPoint determines if a directory is not a mountpoint. // It is fast but not necessarily ALWAYS correct. If the path is in fact // a bind mount from one part of a mount to another it will not be detected. @@ -168,10 +177,6 @@ func (*Mounter) List() ([]MountPoint, error) { // will return true. When in fact /tmp/b is a mount point. If this situation // if of interest to you, don't use this function... func (mounter *Mounter) IsLikelyNotMountPoint(file string) (bool, error) { - return IsNotMountPoint(file) -} - -func IsNotMountPoint(file string) (bool, error) { stat, err := os.Stat(file) if err != nil { return true, err diff --git a/pkg/util/mount/mount_unsupported.go b/pkg/util/mount/mount_unsupported.go index f9abab813dc2..632ad0606ee2 100644 --- a/pkg/util/mount/mount_unsupported.go +++ b/pkg/util/mount/mount_unsupported.go @@ -34,6 +34,14 @@ func (mounter *Mounter) List() ([]MountPoint, error) { return []MountPoint{}, nil } +func (mounter *Mounter) IsMountPointMatch(mp MountPoint, dir string) bool { + return (mp.Path == dir) +} + +func (mounter *Mounter) IsNotMountPoint(dir string) (bool, error) { + return IsNotMountPoint(mounter, dir) +} + func (mounter *Mounter) IsLikelyNotMountPoint(file string) (bool, error) { return true, nil } @@ -57,7 +65,3 @@ func (mounter *SafeFormatAndMount) formatAndMount(source string, target string, func (mounter *SafeFormatAndMount) diskLooksUnformatted(disk string) (bool, error) { return true, nil } - -func IsNotMountPoint(file string) (bool, error) { - return true, nil -} diff --git a/pkg/util/mount/nsenter_mount.go b/pkg/util/mount/nsenter_mount.go index f3a4afc1b0b2..4af8ef0d82ea 100644 --- a/pkg/util/mount/nsenter_mount.go +++ b/pkg/util/mount/nsenter_mount.go @@ -19,6 +19,7 @@ limitations under the License. package mount import ( + "fmt" "os" "path/filepath" "strings" @@ -162,6 +163,15 @@ func (*NsenterMounter) List() ([]MountPoint, error) { return listProcMounts(hostProcMountsPath) } +func (m *NsenterMounter) IsNotMountPoint(dir string) (bool, error) { + return IsNotMountPoint(m, dir) +} + +func (*NsenterMounter) IsMountPointMatch(mp MountPoint, dir string) bool { + deletedDir := fmt.Sprintf("%s\\040(deleted)", dir) + return ((mp.Path == dir) || (mp.Path == deletedDir)) +} + // IsLikelyNotMountPoint determines whether a path is a mountpoint by calling findmnt // in the host's root mount namespace. func (n *NsenterMounter) IsLikelyNotMountPoint(file string) (bool, error) { diff --git a/pkg/util/mount/nsenter_mount_unsupported.go b/pkg/util/mount/nsenter_mount_unsupported.go index dcf19edefd26..e955e1b781b4 100644 --- a/pkg/util/mount/nsenter_mount_unsupported.go +++ b/pkg/util/mount/nsenter_mount_unsupported.go @@ -38,6 +38,14 @@ func (*NsenterMounter) List() ([]MountPoint, error) { return []MountPoint{}, nil } +func (m *NsenterMounter) IsNotMountPoint(dir string) (bool, error) { + return IsNotMountPoint(m, dir) +} + +func (*NsenterMounter) IsMountPointMatch(mp MountPoint, dir string) bool { + return (mp.Path == dir) +} + func (*NsenterMounter) IsLikelyNotMountPoint(file string) (bool, error) { return true, nil } diff --git a/pkg/util/removeall/removeall_test.go b/pkg/util/removeall/removeall_test.go index 938ef08e3585..a5b19fe41a20 100644 --- a/pkg/util/removeall/removeall_test.go +++ b/pkg/util/removeall/removeall_test.go @@ -49,6 +49,12 @@ func (mounter *fakeMounter) PathIsDevice(pathname string) (bool, error) { func (mounter *fakeMounter) GetDeviceNameFromMount(mountPath, pluginDir string) (string, error) { return "", errors.New("not implemented") } +func (mounter *fakeMounter) IsMountPointMatch(mp mount.MountPoint, dir string) bool { + return (mp.Path == dir) +} +func (mounter *fakeMounter) IsNotMountPoint(dir string) (bool, error) { + return mount.IsNotMountPoint(mounter, dir) +} func (mounter *fakeMounter) IsLikelyNotMountPoint(file string) (bool, error) { name := path.Base(file) if strings.HasPrefix(name, "mount") { diff --git a/pkg/volume/local/local.go b/pkg/volume/local/local.go index b09451e6f17d..25f266d936b2 100644 --- a/pkg/volume/local/local.go +++ b/pkg/volume/local/local.go @@ -198,7 +198,7 @@ func (m *localVolumeMounter) SetUpAt(dir string, fsGroup *int64) error { return fmt.Errorf("invalid path: %s %v", m.globalPath, err) } - notMnt, err := m.mounter.IsLikelyNotMountPoint(dir) + notMnt, err := m.mounter.IsNotMountPoint(dir) glog.V(4).Infof("LocalVolume mount setup: PodDir(%s) VolDir(%s) Mounted(%t) Error(%v), ReadOnly(%t)", dir, m.globalPath, !notMnt, err, m.readOnly) if err != nil && !os.IsNotExist(err) { glog.Errorf("cannot validate mount point: %s %v", dir, err) @@ -223,9 +223,9 @@ func (m *localVolumeMounter) SetUpAt(dir string, fsGroup *int64) error { err = m.mounter.Mount(m.globalPath, dir, "", options) if err != nil { glog.Errorf("Mount of volume %s failed: %v", dir, err) - notMnt, mntErr := m.mounter.IsLikelyNotMountPoint(dir) + notMnt, mntErr := m.mounter.IsNotMountPoint(dir) if mntErr != nil { - glog.Errorf("IsLikelyNotMountPoint check failed: %v", mntErr) + glog.Errorf("IsNotMountPoint check failed: %v", mntErr) return err } if !notMnt { @@ -233,9 +233,9 @@ func (m *localVolumeMounter) SetUpAt(dir string, fsGroup *int64) error { glog.Errorf("Failed to unmount: %v", mntErr) return err } - notMnt, mntErr = m.mounter.IsLikelyNotMountPoint(dir) + notMnt, mntErr = m.mounter.IsNotMountPoint(dir) if mntErr != nil { - glog.Errorf("IsLikelyNotMountPoint check failed: %v", mntErr) + glog.Errorf("IsNotMountPoint check failed: %v", mntErr) return err } if !notMnt { @@ -269,5 +269,5 @@ func (u *localVolumeUnmounter) TearDown() error { // TearDownAt unmounts the bind mount func (u *localVolumeUnmounter) TearDownAt(dir string) error { glog.V(4).Infof("Unmounting volume %q at path %q\n", u.volName, dir) - return util.UnmountPath(dir, u.mounter) + return util.UnmountMountPoint(dir, u.mounter, true) /* extensiveMountPointCheck = true */ } diff --git a/pkg/volume/util/util.go b/pkg/volume/util/util.go index 08f0776c27af..98e436c31af2 100644 --- a/pkg/volume/util/util.go +++ b/pkg/volume/util/util.go @@ -75,6 +75,15 @@ func SetReady(dir string) { // UnmountPath is a common unmount routine that unmounts the given path and // deletes the remaining directory if successful. func UnmountPath(mountPath string, mounter mount.Interface) error { + return UnmountMountPoint(mountPath, mounter, false /* extensiveMountPointCheck */) +} + +// UnmountMountPoint 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. +func UnmountMountPoint(mountPath string, mounter mount.Interface, extensiveMountPointCheck bool) error { if pathExists, pathErr := PathExists(mountPath); pathErr != nil { return fmt.Errorf("Error checking if path exists: %v", pathErr) } else if !pathExists { @@ -82,16 +91,26 @@ func UnmountPath(mountPath string, mounter mount.Interface) error { return nil } - notMnt, err := mounter.IsLikelyNotMountPoint(mountPath) + 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 { glog.Warningf("Warning: %q is not a mountpoint, deleting", mountPath) return os.Remove(mountPath) } // Unmount the mount path + glog.V(4).Infof("%q is a mountpoint, unmounting", mountPath) if err := mounter.Unmount(mountPath); err != nil { return err }