Merge pull request #117896 from kolyshkin/mount-utils-spring-cleaning

Mount utils spring cleaning and optimization

staging/src/k8s.io/mount-utils/go.mod

@@ -7,6 +7,7 @@ go 1.20
 require (
 	github.com/moby/sys/mountinfo v0.6.2
 	github.com/stretchr/testify v1.8.2
+	golang.org/x/sys v0.8.0
 	k8s.io/klog/v2 v2.100.1
 	k8s.io/utils v0.0.0-20230406110748-d93618cff8a2
 )
@@ -16,7 +17,6 @@ require (
 	github.com/go-logr/logr v1.2.4 // indirect
 	github.com/kr/pretty v0.3.0 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
-	golang.org/x/sys v0.8.0 // indirect
 	gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
 )

staging/src/k8s.io/mount-utils/mount_helper_test.go

@@ -18,7 +18,6 @@ package mount
 
 import (
 	"fmt"
-	"io/ioutil"
 	"os"
 	"path/filepath"
 	"runtime"
@@ -27,13 +26,12 @@ import (
 )
 
 func TestDoCleanupMountPoint(t *testing.T) {
-
 	if runtime.GOOS == "darwin" {
 		t.Skipf("not supported on GOOS=%s", runtime.GOOS)
 	}
 
 	const testMount = "test-mount"
-	const defaultPerm = 0750
+	const defaultPerm = 0o750
 
 	tests := map[string]struct {
 		corruptedMnt bool
@@ -43,8 +41,7 @@ func TestDoCleanupMountPoint(t *testing.T) {
 		// and error if the prepare function encountered a fatal error.
 		prepareMnt func(base string) (MountPoint, error, error)
 		// Function that prepares the FakeMounter for the test.
-		// Returns error if prepareMntr function encountered a fatal error.
-		prepareMntr func(mntr *FakeMounter) error
+		prepareMntr func(mntr *FakeMounter)
 		expectErr   bool
 	}{
 		"mount-ok": {
@@ -96,22 +93,16 @@ func TestDoCleanupMountPoint(t *testing.T) {
 				}
 				return MountPoint{Device: "/dev/sdb", Path: path}, nil, nil
 			},
-			prepareMntr: func(mntr *FakeMounter) error {
+			prepareMntr: func(mntr *FakeMounter) {
 				mntr.WithSkipMountPointCheck()
-				return nil
 			},
 			expectErr: false,
 		},
 	}
 
 	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)
+			tmpDir := t.TempDir()
 
 			if tt.prepareMnt == nil {
 				t.Fatalf("prepareMnt function required")
@@ -152,15 +143,12 @@ func TestDoCleanupMountPoint(t *testing.T) {
 }
 
 func validateDirExists(dir string) error {
-	_, err := ioutil.ReadDir(dir)
-	if err != nil {
-		return err
-	}
-	return nil
+	_, err := os.ReadDir(dir)
+	return err
 }
 
 func validateDirNotExists(dir string) error {
-	_, err := ioutil.ReadDir(dir)
+	_, err := os.ReadDir(dir)
 	if os.IsNotExist(err) {
 		return nil
 	}

staging/src/k8s.io/mount-utils/mount_helper_unix.go

@@ -20,14 +20,17 @@ limitations under the License.
 package mount
 
 import (
+	"bytes"
 	"errors"
 	"fmt"
 	"io/fs"
 	"os"
 	"strconv"
 	"strings"
+	"sync"
 	"syscall"
 
+	"golang.org/x/sys/unix"
 	"k8s.io/klog/v2"
 	utilio "k8s.io/utils/io"
 )
@@ -91,7 +94,7 @@ type MountInfo struct { // nolint: golint
 
 // ParseMountInfo parses /proc/xxx/mountinfo.
 func ParseMountInfo(filename string) ([]MountInfo, error) {
-	content, err := utilio.ConsistentRead(filename, maxListTries)
+	content, err := readMountInfo(filename)
 	if err != nil {
 		return []MountInfo{}, err
 	}
@@ -173,8 +176,7 @@ func splitMountOptions(s string) []string {
 // isMountPointMatch returns true if the path in mp is the same as dir.
 // Handles case where mountpoint dir has been renamed due to stale NFS mount.
 func isMountPointMatch(mp MountPoint, dir string) bool {
-	deletedDir := fmt.Sprintf("%s\\040(deleted)", dir)
-	return ((mp.Path == dir) || (mp.Path == deletedDir))
+	return strings.TrimSuffix(mp.Path, "\\040(deleted)") == dir
 }
 
 // PathExists returns true if the specified path exists.
@@ -199,3 +201,50 @@ func PathExists(path string) (bool, error) {
 	}
 	return false, err
 }
+
+// These variables are used solely by kernelHasMountinfoBug.
+var (
+	hasMountinfoBug       bool
+	checkMountinfoBugOnce sync.Once
+)
+
+// kernelHasMountinfoBug checks if the kernel bug that can lead to incomplete
+// mountinfo being read is fixed. It does so by checking the kernel version.
+//
+// The bug was fixed by the kernel commit 9f6c61f96f2d97 (since Linux 5.8).
+// Alas, there is no better way to check if the bug is fixed other than to
+// rely on the kernel version returned by uname.
+func kernelHasMountinfoBug() bool {
+	checkMountinfoBugOnce.Do(func() {
+		// Assume old kernel.
+		hasMountinfoBug = true
+
+		uname := unix.Utsname{}
+		err := unix.Uname(&uname)
+		if err != nil {
+			return
+		}
+
+		end := bytes.IndexByte(uname.Release[:], 0)
+		v := bytes.SplitN(uname.Release[:end], []byte{'.'}, 3)
+		if len(v) != 3 {
+			return
+		}
+		major, _ := strconv.Atoi(string(v[0]))
+		minor, _ := strconv.Atoi(string(v[1]))
+
+		if major > 5 || (major == 5 && minor >= 8) {
+			hasMountinfoBug = false
+		}
+	})
+
+	return hasMountinfoBug
+}
+
+func readMountInfo(path string) ([]byte, error) {
+	if kernelHasMountinfoBug() {
+		return utilio.ConsistentRead(path, maxListTries)
+	}
+
+	return os.ReadFile(path)
+}

staging/src/k8s.io/mount-utils/mount_helper_unix_test.go

@@ -20,30 +20,23 @@ limitations under the License.
 package mount
 
 import (
-	"io/ioutil"
 	"os"
 	"path/filepath"
 	"reflect"
 	"testing"
 )
 
-func writeFile(content string) (string, string, error) {
-	tempDir, err := ioutil.TempDir("", "mounter_shared_test")
+func writeFile(t *testing.T, content string) string {
+	filename := filepath.Join(t.TempDir(), "mountinfo")
+	err := os.WriteFile(filename, []byte(content), 0o600)
 	if err != nil {
-		return "", "", err
+		t.Fatal(err)
 	}
-	filename := filepath.Join(tempDir, "mountinfo")
-	err = ioutil.WriteFile(filename, []byte(content), 0600)
-	if err != nil {
-		os.RemoveAll(tempDir)
-		return "", "", err
-	}
-	return tempDir, filename, nil
+	return filename
 }
 
 func TestParseMountInfo(t *testing.T) {
-	info :=
-		`62 0 253:0 / / rw,relatime shared:1 - ext4 /dev/mapper/ssd-root rw,seclabel,data=ordered
+	info := `62 0 253:0 / / rw,relatime shared:1 - ext4 /dev/mapper/ssd-root rw,seclabel,data=ordered
 78 62 0:41 / /tmp rw,nosuid,nodev shared:30 - tmpfs tmpfs rw,seclabel
 80 62 0:42 / /var/lib/nfs/rpc_pipefs rw,relatime shared:31 - rpc_pipefs sunrpc rw
 82 62 0:43 / /var/lib/foo rw,relatime shared:32 - tmpfs tmpfs rw
@@ -85,11 +78,7 @@ func TestParseMountInfo(t *testing.T) {
 40 28 0:36 / /sys/fs/cgroup/perf_event rw,nosuid,nodev,noexec,relatime shared:22 - cgroup cgroup rw,perf_event
 761 60 8:0 / /var/lib/kubelet/plugins/kubernetes.io/iscsi/iface-default/127.0.0.1:3260-iqn.2003-01.org.linux-iscsi.f21.x8664:sn.4b0aae584f7c-lun-0 rw,relatime shared:421 - ext4 /dev/sda rw,context="system_u:object_r:container_file_t:s0:c314,c894",data=ordered
 `
-	tempDir, filename, err := writeFile(info)
-	if err != nil {
-		t.Fatalf("cannot create temporary file: %v", err)
-	}
-	defer os.RemoveAll(tempDir)
+	filename := writeFile(t, info)
 
 	tests := []struct {
 		name         string
@@ -304,11 +293,7 @@ func TestBadParseMountInfo(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		tempDir, filename, err := writeFile(test.info)
-		if err != nil {
-			t.Fatalf("cannot create temporary file: %v", err)
-		}
-		defer os.RemoveAll(tempDir)
+		filename := writeFile(t, test.info)
 
 		infos, err := ParseMountInfo(filename)
 		if err != nil {
@@ -333,3 +318,35 @@ func TestBadParseMountInfo(t *testing.T) {
 		}
 	}
 }
+
+func testIsMountPointMatch(t testing.TB) {
+	mpCases := []struct {
+		mp, dir string
+		res     bool
+	}{
+		{"", "", true},
+		{"/", "/", true},
+		{"/some/path", "/some/path", true},
+		{"/a/different/kind/of/path\\040(deleted)", "/a/different/kind/of/path", true},
+		{"one", "two", false},
+		{"a somewhat long path that ends with A", "a somewhat long path that ends with B", false},
+	}
+
+	for _, tc := range mpCases {
+		mp := MountPoint{Path: tc.mp}
+		res := isMountPointMatch(mp, tc.dir)
+		if res != tc.res {
+			t.Errorf("mp: %q, dir: %q, expected %v, got %v", tc.mp, tc.dir, tc.res, res)
+		}
+	}
+}
+
+func TestIsMountPointMatch(t *testing.T) {
+	testIsMountPointMatch(t)
+}
+
+func BenchmarkIsMountPointMatch(b *testing.B) {
+	for i := 0; i < b.N; i++ {
+		testIsMountPointMatch(b)
+	}
+}

staging/src/k8s.io/mount-utils/mount_linux.go

@@ -24,7 +24,6 @@ import (
 	"errors"
 	"fmt"
 	"io/fs"
-	"io/ioutil"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -37,7 +36,6 @@ import (
 
 	"k8s.io/klog/v2"
 	utilexec "k8s.io/utils/exec"
-	utilio "k8s.io/utils/io"
 )
 
 const (
@@ -271,7 +269,7 @@ func detectSafeNotMountedBehavior() bool {
 // detectSafeNotMountedBehaviorWithExec is for testing with FakeExec.
 func detectSafeNotMountedBehaviorWithExec(exec utilexec.Interface) bool {
 	// create a temp dir and try to umount it
-	path, err := ioutil.TempDir("", "kubelet-detect-safe-umount")
+	path, err := os.MkdirTemp("", "kubelet-detect-safe-umount")
 	if err != nil {
 		klog.V(4).Infof("Cannot create temp dir to detect safe 'not mounted' behavior: %v", err)
 		return false
@@ -633,7 +631,7 @@ func (mounter *SafeFormatAndMount) GetDiskFormat(disk string) (string, error) {
 
 // ListProcMounts is shared with NsEnterMounter
 func ListProcMounts(mountFilePath string) ([]MountPoint, error) {
-	content, err := utilio.ConsistentRead(mountFilePath, maxListTries)
+	content, err := readMountInfo(mountFilePath)
 	if err != nil {
 		return nil, err
 	}
@@ -766,7 +764,7 @@ func (mounter *Mounter) IsMountPoint(file string) (bool, error) {
 	// Resolve any symlinks in file, kernel would do the same and use the resolved path in /proc/mounts.
 	resolvedFile, err := filepath.EvalSymlinks(file)
 	if err != nil {
-		if errors.Is(isMntErr, fs.ErrNotExist) {
+		if errors.Is(err, fs.ErrNotExist) {
 			return false, fs.ErrNotExist
 		}
 		return false, err
@@ -810,7 +808,6 @@ func tryUnmount(target string, withSafeNotMountedBehavior bool, unmountTimeout t
 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)
 	}