Merge pull request #48402 from ianchakeres/local-storage-teardown-fix

Automatic merge from submit-queue Local storage teardown fix **What this PR does / why we need it**: Local storage uses bindmounts and the method IsLikelyNotMountPoint does not detect these as mountpoints. Therefore, local PVs are not properly unmounted when they are deleted. **Which issue this PR fixes**: fixes #48331 **Special notes for your reviewer**: You can use these e2e tests to reproduce the issue and validate the fix works appropriately #47999 The existing method IsLikelyNotMountPoint purposely does not check mountpoints reliability (https://github.com/kubernetes/kubernetes/blob/4c5b22d4c6b630ff1d76b1d15d74c6597c0aa037/pkg/util/mount/mount_linux.go#L161), since the number of mountpoints can be large. https://github.com/kubernetes/kubernetes/blob/4c5b22d4c6b630ff1d76b1d15d74c6597c0aa037/pkg/util/mount/mount.go#L46 This implementation changes the behavior for local storage to detect mountpoints reliably, and avoids changing the behavior for any other callers to a UnmountPath. **Release note**: ``` Fixes bind-mount teardown failure with non-mount point Local volumes (issue #48331). ```
kubernetes · Jul 12, 2017 · 03360d7 · 03360d7
2 parents 3e89fe2 + 2b18d3b
commit 03360d7
Show file tree

Hide file tree

Showing 11 changed files with 136 additions and 16 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -161,17 +161,22 @@ 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.
 // mkdir /tmp/a /tmp/b; mount --bin /tmp/a /tmp/b; IsLikelyNotMountPoint("/tmp/b")
 // 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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,19 +223,19 @@ 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 {
 			if mntErr = m.mounter.Unmount(dir); mntErr != nil {
 				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
@@ -75,23 +75,42 @@ 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 {
 		glog.Warningf("Warning: Unmount skipped because path does not exist: %v", mountPath)
 		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
 	}