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 https://github.com/kubernetes/kubernetes/pull/47999

The existing method IsLikelyNotMountPoint purposely does not check mountpoints reliability (4c5b22d4c6/pkg/util/mount/mount_linux.go (L161)), since the number of mountpoints can be large. 4c5b22d4c6/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 https://github.com/kubernetes/kubernetes/issues/48331).
```

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")
 }

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")
 }

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()

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
+}

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

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
-}

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) {

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
 }

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") {

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 */
 }

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
 	}