From 167252fb5e0eb14336ce7e5d00ad57a365800d1b Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 9 May 2023 15:01:30 -0700 Subject: [PATCH 1/8] mount-utils: format with gofumpt gofumpt is a superset of go fmt, enabling some more strict formatting rules, mostly to improve code readability. No functional or code change, just formatting. Signed-off-by: Kir Kolyshkin --- .../k8s.io/mount-utils/mount_helper_test.go | 4 +-- .../mount-utils/mount_helper_unix_test.go | 5 ++-- staging/src/k8s.io/mount-utils/mount_linux.go | 1 - .../k8s.io/mount-utils/mount_linux_test.go | 27 +++++++++---------- staging/src/k8s.io/mount-utils/mount_test.go | 4 --- .../src/k8s.io/mount-utils/mount_windows.go | 4 +-- .../k8s.io/mount-utils/mount_windows_test.go | 6 ++--- .../src/k8s.io/mount-utils/resizefs_linux.go | 3 +-- .../k8s.io/mount-utils/resizefs_linux_test.go | 18 +++++-------- .../mount-utils/safe_format_and_mount_test.go | 2 +- 10 files changed, 29 insertions(+), 45 deletions(-) diff --git a/staging/src/k8s.io/mount-utils/mount_helper_test.go b/staging/src/k8s.io/mount-utils/mount_helper_test.go index 288fff803e1..1e506bb2edf 100644 --- a/staging/src/k8s.io/mount-utils/mount_helper_test.go +++ b/staging/src/k8s.io/mount-utils/mount_helper_test.go @@ -27,13 +27,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 @@ -106,7 +105,6 @@ func TestDoCleanupMountPoint(t *testing.T) { 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) diff --git a/staging/src/k8s.io/mount-utils/mount_helper_unix_test.go b/staging/src/k8s.io/mount-utils/mount_helper_unix_test.go index 6fbc79e3e4d..973b8b640b8 100644 --- a/staging/src/k8s.io/mount-utils/mount_helper_unix_test.go +++ b/staging/src/k8s.io/mount-utils/mount_helper_unix_test.go @@ -33,7 +33,7 @@ func writeFile(content string) (string, string, error) { return "", "", err } filename := filepath.Join(tempDir, "mountinfo") - err = ioutil.WriteFile(filename, []byte(content), 0600) + err = ioutil.WriteFile(filename, []byte(content), 0o600) if err != nil { os.RemoveAll(tempDir) return "", "", err @@ -42,8 +42,7 @@ func writeFile(content string) (string, string, error) { } 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 diff --git a/staging/src/k8s.io/mount-utils/mount_linux.go b/staging/src/k8s.io/mount-utils/mount_linux.go index f0125fcb489..dc861734ad5 100644 --- a/staging/src/k8s.io/mount-utils/mount_linux.go +++ b/staging/src/k8s.io/mount-utils/mount_linux.go @@ -810,7 +810,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) } diff --git a/staging/src/k8s.io/mount-utils/mount_linux_test.go b/staging/src/k8s.io/mount-utils/mount_linux_test.go index 5f7a2246668..5ea22e1ac4c 100644 --- a/staging/src/k8s.io/mount-utils/mount_linux_test.go +++ b/staging/src/k8s.io/mount-utils/mount_linux_test.go @@ -36,8 +36,7 @@ import ( ) func TestReadProcMountsFrom(t *testing.T) { - successCase := - `/dev/0 /path/to/0 type0 flags 0 0 + successCase := `/dev/0 /path/to/0 type0 flags 0 0 /dev/1 /path/to/1 type1 flags 1 1 /dev/2 /path/to/2 type2 flags,1,2=3 2 2 ` @@ -148,10 +147,14 @@ func setEquivalent(set1, set2 []string) bool { func TestGetDeviceNameFromMount(t *testing.T) { fm := NewFakeMounter( []MountPoint{ - {Device: "/dev/disk/by-path/prefix-lun-1", - Path: "/mnt/111"}, - {Device: "/dev/disk/by-path/prefix-lun-1", - Path: "/mnt/222"}, + { + Device: "/dev/disk/by-path/prefix-lun-1", + Path: "/mnt/111", + }, + { + Device: "/dev/disk/by-path/prefix-lun-1", + Path: "/mnt/222", + }, }) tests := []struct { @@ -203,7 +206,6 @@ func TestGetMountRefsByDev(t *testing.T) { } for i, test := range tests { - if refs, err := getMountRefsByDev(fm, test.mountPath); err != nil || !setEquivalent(test.expectedRefs, refs) { t.Errorf("%d. getMountRefsByDev(%q) = %v, %v; expected %v, nil", i, test.mountPath, refs, err, test.expectedRefs) } @@ -294,7 +296,6 @@ func TestPathWithinBase(t *testing.T) { if PathWithinBase(test.fullPath, test.basePath) != test.expected { t.Errorf("test %q failed: expected %v", test.name, test.expected) } - } } @@ -422,8 +423,10 @@ func TestSearchMountPoints(t *testing.T) { 62 25 7:1 / /var/lib/kubelet/pods/f19fe4e2-5a63-11e8-962f-000c29bb0377/volumes/kubernetes.io~local-volume/local-pv-test rw,relatime shared:38 - ext4 /dev/loop1 rw,data=ordered 95 25 7:1 / /var/lib/kubelet/pods/4854a48b-5a64-11e8-962f-000c29bb0377/volumes/kubernetes.io~local-volume/local-pv-test rw,relatime shared:38 - ext4 /dev/loop1 rw,data=ordered `, - []string{"/var/lib/kubelet/pods/f19fe4e2-5a63-11e8-962f-000c29bb0377/volumes/kubernetes.io~local-volume/local-pv-test", - "/var/lib/kubelet/pods/4854a48b-5a64-11e8-962f-000c29bb0377/volumes/kubernetes.io~local-volume/local-pv-test"}, + []string{ + "/var/lib/kubelet/pods/f19fe4e2-5a63-11e8-962f-000c29bb0377/volumes/kubernetes.io~local-volume/local-pv-test", + "/var/lib/kubelet/pods/4854a48b-5a64-11e8-962f-000c29bb0377/volumes/kubernetes.io~local-volume/local-pv-test", + }, nil, }, } @@ -459,7 +462,6 @@ func TestSensitiveMountOptions(t *testing.T) { mountFlags []string }{ { - source: "mySrc", target: "myTarget", fstype: "myFS", @@ -468,7 +470,6 @@ func TestSensitiveMountOptions(t *testing.T) { mountFlags: []string{}, }, { - source: "mySrc", target: "myTarget", fstype: "myFS", @@ -477,7 +478,6 @@ func TestSensitiveMountOptions(t *testing.T) { mountFlags: []string{}, }, { - source: "mySrc", target: "myTarget", fstype: "myFS", @@ -486,7 +486,6 @@ func TestSensitiveMountOptions(t *testing.T) { mountFlags: []string{}, }, { - source: "mySrc", target: "myTarget", fstype: "myFS", diff --git a/staging/src/k8s.io/mount-utils/mount_test.go b/staging/src/k8s.io/mount-utils/mount_test.go index 4fa6484d59e..5823bbcce19 100644 --- a/staging/src/k8s.io/mount-utils/mount_test.go +++ b/staging/src/k8s.io/mount-utils/mount_test.go @@ -38,7 +38,6 @@ func TestMakeBindOpts(t *testing.T) { []string{}, }, { - []string{"bind", "vers=2", "ro", "_netdev"}, true, []string{"bind", "_netdev"}, @@ -80,7 +79,6 @@ func TestMakeBindOptsSensitive(t *testing.T) { expectedSensitiveRemountOpts: []string{"user=foo", "pass=bar"}, }, { - mountOptions: []string{"vers=2", "ro", "_netdev"}, sensitiveMountOptions: []string{"user=foo", "pass=bar", "bind"}, isBind: true, @@ -105,7 +103,6 @@ func TestMakeBindOptsSensitive(t *testing.T) { expectedSensitiveRemountOpts: []string{"user=foo", "pass=bar"}, }, { - mountOptions: []string{"vers=2", "bind", "ro", "_netdev"}, sensitiveMountOptions: []string{"user=foo", "remount", "pass=bar"}, isBind: true, @@ -114,7 +111,6 @@ func TestMakeBindOptsSensitive(t *testing.T) { expectedSensitiveRemountOpts: []string{"user=foo", "pass=bar"}, }, { - mountOptions: []string{"vers=2", "bind", "ro", "_netdev"}, sensitiveMountOptions: []string{"user=foo", "remount", "pass=bar"}, isBind: true, diff --git a/staging/src/k8s.io/mount-utils/mount_windows.go b/staging/src/k8s.io/mount-utils/mount_windows.go index 4f2b7aee873..e5a03ecff83 100644 --- a/staging/src/k8s.io/mount-utils/mount_windows.go +++ b/staging/src/k8s.io/mount-utils/mount_windows.go @@ -82,11 +82,11 @@ func (mounter *Mounter) MountSensitive(source string, target string, fstype stri if source == "tmpfs" { klog.V(3).Infof("mounting source (%q), target (%q), with options (%q)", source, target, sanitizedOptionsForLogging) - return os.MkdirAll(target, 0755) + return os.MkdirAll(target, 0o755) } parentDir := filepath.Dir(target) - if err := os.MkdirAll(parentDir, 0755); err != nil { + if err := os.MkdirAll(parentDir, 0o755); err != nil { return err } diff --git a/staging/src/k8s.io/mount-utils/mount_windows_test.go b/staging/src/k8s.io/mount-utils/mount_windows_test.go index 2e1b2d7cc02..524ee5f7bae 100644 --- a/staging/src/k8s.io/mount-utils/mount_windows_test.go +++ b/staging/src/k8s.io/mount-utils/mount_windows_test.go @@ -147,7 +147,7 @@ func TestIsLikelyNotMountPoint(t *testing.T) { "Dir", "", func(base, fileName, targetLinkName string) error { - return os.Mkdir(filepath.Join(base, fileName), 0750) + return os.Mkdir(filepath.Join(base, fileName), 0o750) }, true, false, @@ -166,7 +166,7 @@ func TestIsLikelyNotMountPoint(t *testing.T) { "targetSymLink", func(base, fileName, targetLinkName string) error { targeLinkPath := filepath.Join(base, targetLinkName) - if err := os.Mkdir(targeLinkPath, 0750); err != nil { + if err := os.Mkdir(targeLinkPath, 0o750); err != nil { return err } @@ -184,7 +184,7 @@ func TestIsLikelyNotMountPoint(t *testing.T) { "targetSymLink2", func(base, fileName, targetLinkName string) error { targeLinkPath := filepath.Join(base, targetLinkName) - if err := os.Mkdir(targeLinkPath, 0750); err != nil { + if err := os.Mkdir(targeLinkPath, 0o750); err != nil { return err } diff --git a/staging/src/k8s.io/mount-utils/resizefs_linux.go b/staging/src/k8s.io/mount-utils/resizefs_linux.go index 81386fef878..3a5fa1be7cb 100644 --- a/staging/src/k8s.io/mount-utils/resizefs_linux.go +++ b/staging/src/k8s.io/mount-utils/resizefs_linux.go @@ -45,7 +45,6 @@ func NewResizeFs(exec utilexec.Interface) *ResizeFs { // Resize perform resize of file system func (resizefs *ResizeFs) Resize(devicePath string, deviceMountPath string) (bool, error) { format, err := getDiskFormat(resizefs.exec, devicePath) - if err != nil { formatErr := fmt.Errorf("ResizeFS.Resize - error checking format for device %s: %v", devicePath, err) return false, formatErr @@ -78,7 +77,6 @@ func (resizefs *ResizeFs) extResize(devicePath string) (bool, error) { resizeError := fmt.Errorf("resize of device %s failed: %v. resize2fs output: %s", devicePath, err, string(output)) return false, resizeError - } func (resizefs *ResizeFs) xfsResize(deviceMountPath string) (bool, error) { @@ -161,6 +159,7 @@ func (resizefs *ResizeFs) NeedResize(devicePath string, deviceMountPath string) } return true, nil } + func (resizefs *ResizeFs) getDeviceSize(devicePath string) (uint64, error) { output, err := resizefs.exec.Command(blockDev, "--getsize64", devicePath).CombinedOutput() outStr := strings.TrimSpace(string(output)) diff --git a/staging/src/k8s.io/mount-utils/resizefs_linux_test.go b/staging/src/k8s.io/mount-utils/resizefs_linux_test.go index 98f13d3cfe4..5acc1632e46 100644 --- a/staging/src/k8s.io/mount-utils/resizefs_linux_test.go +++ b/staging/src/k8s.io/mount-utils/resizefs_linux_test.go @@ -28,8 +28,7 @@ import ( ) func TestGetFileSystemSize(t *testing.T) { - cmdOutputSuccessXfs := - ` + cmdOutputSuccessXfs := ` statfs.f_bsize = 4096 statfs.f_blocks = 1832448 statfs.f_bavail = 1822366 @@ -50,8 +49,7 @@ func TestGetFileSystemSize(t *testing.T) { counts.freeino = 61 counts.allocino = 64 ` - cmdOutputNoDataXfs := - ` + cmdOutputNoDataXfs := ` statfs.f_bsize = 4096 statfs.f_blocks = 1832448 statfs.f_bavail = 1822366 @@ -70,8 +68,7 @@ func TestGetFileSystemSize(t *testing.T) { counts.freeino = 61 counts.allocino = 64 ` - cmdOutputSuccessExt4 := - ` + cmdOutputSuccessExt4 := ` Filesystem volume name: cloudimg-rootfs Last mounted on: / Filesystem UUID: testUUID @@ -121,8 +118,7 @@ Journal start: 1 Journal checksum type: crc32c Journal checksum: 0xb7df3c6e ` - cmdOutputNoDataExt4 := - `Filesystem volume name: cloudimg-rootfs + cmdOutputNoDataExt4 := `Filesystem volume name: cloudimg-rootfs Last mounted on: / Filesystem UUID: testUUID Filesystem magic number: 0xEF53 @@ -169,8 +165,7 @@ Journal start: 1 Journal checksum type: crc32c Journal checksum: 0xb7df3c6e ` - cmdOutputSuccessBtrfs := - `superblock: bytenr=65536, device=/dev/loop0 + cmdOutputSuccessBtrfs := `superblock: bytenr=65536, device=/dev/loop0 --------------------------------------------------------- csum_type 0 (crc32c) csum_size 4 @@ -279,8 +274,7 @@ backup_roots[4]: backup_num_devices: 1 ` - cmdOutputNoDataBtrfs := - `superblock: bytenr=65536, device=/dev/loop0 + cmdOutputNoDataBtrfs := `superblock: bytenr=65536, device=/dev/loop0 --------------------------------------------------------- csum_type 0 (crc32c) csum_size 4 diff --git a/staging/src/k8s.io/mount-utils/safe_format_and_mount_test.go b/staging/src/k8s.io/mount-utils/safe_format_and_mount_test.go index 7d70eb0a98a..99a87ae43e2 100644 --- a/staging/src/k8s.io/mount-utils/safe_format_and_mount_test.go +++ b/staging/src/k8s.io/mount-utils/safe_format_and_mount_test.go @@ -261,7 +261,7 @@ func TestSafeFormatAndMount(t *testing.T) { t.Errorf("test \"%s\" the directory was not mounted", test.description) } - //check that the correct device was mounted + // check that the correct device was mounted mountedDevice, _, err := GetDeviceNameFromMount(fakeMounter.FakeMounter, dest) if err != nil || mountedDevice != device { t.Errorf("test \"%s\" the correct device was not mounted", test.description) From 4bb023927936ae8ac0de06347c68a70c682d53b8 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 9 May 2023 15:10:17 -0700 Subject: [PATCH 2/8] mount-utils: IsMountPoint: fix Commit 44bea358045e0 added a code to return unwrapped fs.ErrNotExist error in case filepath.EvalSymlinks failed a wrapped one. This never worked because of a copy/paste bug. Fix this. Fixes: 44bea358045e0 Cc: Manu Gupta Signed-off-by: Kir Kolyshkin --- staging/src/k8s.io/mount-utils/mount_linux.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/staging/src/k8s.io/mount-utils/mount_linux.go b/staging/src/k8s.io/mount-utils/mount_linux.go index dc861734ad5..005f447d9d8 100644 --- a/staging/src/k8s.io/mount-utils/mount_linux.go +++ b/staging/src/k8s.io/mount-utils/mount_linux.go @@ -766,7 +766,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 From 404e844468bc3f9b2211598b489b922b833f80a4 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 9 May 2023 15:12:32 -0700 Subject: [PATCH 3/8] mount-utils: add isMountPointMatch test and benchmark Add some test cases for isMountPointMatch, to prepare for its rework. Signed-off-by: Kir Kolyshkin --- .../mount-utils/mount_helper_unix_test.go | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/staging/src/k8s.io/mount-utils/mount_helper_unix_test.go b/staging/src/k8s.io/mount-utils/mount_helper_unix_test.go index 973b8b640b8..3baea376ee2 100644 --- a/staging/src/k8s.io/mount-utils/mount_helper_unix_test.go +++ b/staging/src/k8s.io/mount-utils/mount_helper_unix_test.go @@ -332,3 +332,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) + } +} From 8c79a911331e7b504ae3c6a4e03edc6629728ce3 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 9 May 2023 15:29:03 -0700 Subject: [PATCH 4/8] mount-utils: isMountPointMatch: simplify and speedup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Here's before/after comparison. [kir@kir-rhat mount-utils]$ benchstat before after name old time/op new time/op delta IsMountPointMatch-4 707ns ± 1% 40ns ± 1% -94.39% (p=0.008 n=5+5) name old alloc/op new alloc/op delta IsMountPointMatch-4 264B ± 0% 0B -100.00% (p=0.008 n=5+5) name old allocs/op new allocs/op delta IsMountPointMatch-4 11.0 ± 0% 0.0 -100.00% (p=0.008 n=5+5) Signed-off-by: Kir Kolyshkin --- staging/src/k8s.io/mount-utils/mount_helper_unix.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/staging/src/k8s.io/mount-utils/mount_helper_unix.go b/staging/src/k8s.io/mount-utils/mount_helper_unix.go index cb8732fce74..7b1eb6af1b7 100644 --- a/staging/src/k8s.io/mount-utils/mount_helper_unix.go +++ b/staging/src/k8s.io/mount-utils/mount_helper_unix.go @@ -173,8 +173,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. From b690450e8462841a2b5ed6dc79a4cc23d548ecc2 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 9 May 2023 15:52:07 -0700 Subject: [PATCH 5/8] mount-utils: don't reread mountinfo on newer kernels 1. Background. Since the dawn of times mount-utils package tries to work around the bug in the Linux kernel, which results in occasional incomplete read of mountinfo entries (from either /proc/mounts or /proc/PID/mountinfo). The workaround used is to read the whole file twice and compare the two blobs. If they differ, try again. The kernel bug is manifesting when mountinfo read is performed concurrently with an unmount, and can easily be reproduced by running lots of mounts and unmounts in parallel with the code reading mountinfo. For one such reproducer, see https://github.com/kolyshkin/procfs-test. On a Kubernetes node with lots of short-lived containers, mounts and unmounts are quite frequent. This leads to the occasional bug, and surely results in much more re-reads of mountinfo, because the workaround assumes its content is more-or-less static. The good news is, this bug was finally fixed by kernel commit 9f6c61f96f2d97, which made its way into Linux 5.8. 2. The issue. The code still read every file at least twice, and up to 10 times. The chance of re-reading is higher if there is a mount or unmount going on at the same time. The result is higher system and kernel load, and degraded performance. 3. The fix. As the re-reading is not necessary for newer kernels, let's check the kernel version and skip the workaround if running Linux >= 5.8. Signed-off-by: Kir Kolyshkin --- staging/src/k8s.io/mount-utils/go.mod | 2 +- .../k8s.io/mount-utils/mount_helper_unix.go | 52 ++++++++++++++++++- staging/src/k8s.io/mount-utils/mount_linux.go | 3 +- 3 files changed, 53 insertions(+), 4 deletions(-) diff --git a/staging/src/k8s.io/mount-utils/go.mod b/staging/src/k8s.io/mount-utils/go.mod index e2dec9a41ff..f3fe8a08c4c 100644 --- a/staging/src/k8s.io/mount-utils/go.mod +++ b/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 ) diff --git a/staging/src/k8s.io/mount-utils/mount_helper_unix.go b/staging/src/k8s.io/mount-utils/mount_helper_unix.go index 7b1eb6af1b7..82210aaa23a 100644 --- a/staging/src/k8s.io/mount-utils/mount_helper_unix.go +++ b/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 } @@ -198,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) +} diff --git a/staging/src/k8s.io/mount-utils/mount_linux.go b/staging/src/k8s.io/mount-utils/mount_linux.go index 005f447d9d8..4ce73b6c598 100644 --- a/staging/src/k8s.io/mount-utils/mount_linux.go +++ b/staging/src/k8s.io/mount-utils/mount_linux.go @@ -37,7 +37,6 @@ import ( "k8s.io/klog/v2" utilexec "k8s.io/utils/exec" - utilio "k8s.io/utils/io" ) const ( @@ -633,7 +632,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 } From 699d118d85f8986d957618822ea510f9b81aa7c7 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 7 Jun 2023 14:02:40 -0700 Subject: [PATCH 6/8] mount-utils: stop using ioutil It has been deprecated since Go 1.16. Signed-off-by: Kir Kolyshkin --- staging/src/k8s.io/mount-utils/mount_linux.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/staging/src/k8s.io/mount-utils/mount_linux.go b/staging/src/k8s.io/mount-utils/mount_linux.go index 4ce73b6c598..7d180723047 100644 --- a/staging/src/k8s.io/mount-utils/mount_linux.go +++ 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" @@ -270,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 From 8ced101db53475c0162401b3b6638ffee0eef6f7 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 7 Jun 2023 14:03:06 -0700 Subject: [PATCH 7/8] mount-utils: stop using ioutil in tests io/ioutil is deprecated since Go 1.16. Besides, we now have a nice t.TempDir() function which simplifies things a lot. Signed-off-by: Kir Kolyshkin --- .../k8s.io/mount-utils/mount_helper_test.go | 16 +++-------- .../mount-utils/mount_helper_unix_test.go | 28 +++++-------------- .../k8s.io/mount-utils/mount_linux_test.go | 3 +- .../k8s.io/mount-utils/mount_windows_test.go | 16 ++--------- .../mount-utils/safe_format_and_mount_test.go | 9 +----- 5 files changed, 15 insertions(+), 57 deletions(-) diff --git a/staging/src/k8s.io/mount-utils/mount_helper_test.go b/staging/src/k8s.io/mount-utils/mount_helper_test.go index 1e506bb2edf..abbc3199828 100644 --- a/staging/src/k8s.io/mount-utils/mount_helper_test.go +++ b/staging/src/k8s.io/mount-utils/mount_helper_test.go @@ -18,7 +18,6 @@ package mount import ( "fmt" - "io/ioutil" "os" "path/filepath" "runtime" @@ -105,11 +104,7 @@ func TestDoCleanupMountPoint(t *testing.T) { 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") @@ -150,15 +145,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 } diff --git a/staging/src/k8s.io/mount-utils/mount_helper_unix_test.go b/staging/src/k8s.io/mount-utils/mount_helper_unix_test.go index 3baea376ee2..71662b58315 100644 --- a/staging/src/k8s.io/mount-utils/mount_helper_unix_test.go +++ b/staging/src/k8s.io/mount-utils/mount_helper_unix_test.go @@ -20,25 +20,19 @@ 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), 0o600) - if err != nil { - os.RemoveAll(tempDir) - return "", "", err - } - return tempDir, filename, nil + return filename } func TestParseMountInfo(t *testing.T) { @@ -84,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 @@ -303,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 { diff --git a/staging/src/k8s.io/mount-utils/mount_linux_test.go b/staging/src/k8s.io/mount-utils/mount_linux_test.go index 5ea22e1ac4c..7dae6cb73bd 100644 --- a/staging/src/k8s.io/mount-utils/mount_linux_test.go +++ b/staging/src/k8s.io/mount-utils/mount_linux_test.go @@ -22,7 +22,6 @@ package mount import ( "errors" "fmt" - "io/ioutil" "os" "os/exec" "reflect" @@ -430,7 +429,7 @@ func TestSearchMountPoints(t *testing.T) { nil, }, } - tmpFile, err := ioutil.TempFile("", "test-get-filetype") + tmpFile, err := os.CreateTemp("", "test-get-filetype") if err != nil { t.Fatal(err) } diff --git a/staging/src/k8s.io/mount-utils/mount_windows_test.go b/staging/src/k8s.io/mount-utils/mount_windows_test.go index 524ee5f7bae..226af98e618 100644 --- a/staging/src/k8s.io/mount-utils/mount_windows_test.go +++ b/staging/src/k8s.io/mount-utils/mount_windows_test.go @@ -21,7 +21,6 @@ package mount import ( "fmt" - "io/ioutil" "os" "os/exec" "path/filepath" @@ -200,12 +199,7 @@ func TestIsLikelyNotMountPoint(t *testing.T) { } for _, test := range tests { - base, err := ioutil.TempDir("", test.fileName) - if err != nil { - t.Fatalf(err.Error()) - } - - defer os.RemoveAll(base) + base := t.TempDir() if err := test.setUp(base, test.fileName, test.targetLinkName); err != nil { t.Fatalf("unexpected error in setUp(%s, %s): %v", test.fileName, test.targetLinkName, err) @@ -280,13 +274,7 @@ func TestFormatAndMount(t *testing.T) { Interface: &fakeMounter, Exec: fakeExec, } - base, err := ioutil.TempDir("", test.device) - if err != nil { - t.Fatalf(err.Error()) - } - defer os.RemoveAll(base) - - target := filepath.Join(base, test.target) + target := filepath.Join(t.TempDir(), test.target) err = mounter.FormatAndMount(test.device, target, test.fstype, test.mountOptions) if test.expectError { assert.NotNil(t, err, "Expect error during FormatAndMount(%s, %s, %s, %v)", test.device, test.target, test.fstype, test.mountOptions) diff --git a/staging/src/k8s.io/mount-utils/safe_format_and_mount_test.go b/staging/src/k8s.io/mount-utils/safe_format_and_mount_test.go index 99a87ae43e2..07279554a4f 100644 --- a/staging/src/k8s.io/mount-utils/safe_format_and_mount_test.go +++ b/staging/src/k8s.io/mount-utils/safe_format_and_mount_test.go @@ -18,8 +18,6 @@ package mount import ( "fmt" - "io/ioutil" - "os" "runtime" "strings" "testing" @@ -58,11 +56,6 @@ func TestSafeFormatAndMount(t *testing.T) { if runtime.GOOS == "darwin" || runtime.GOOS == "windows" { t.Skipf("not supported on GOOS=%s", runtime.GOOS) } - mntDir, err := ioutil.TempDir(os.TempDir(), "mount") - if err != nil { - t.Fatalf("failed to create tmp dir: %v", err) - } - defer os.RemoveAll(mntDir) tests := []struct { description string fstype string @@ -241,7 +234,7 @@ func TestSafeFormatAndMount(t *testing.T) { } device := "/dev/foo" - dest := mntDir + dest := t.TempDir() var err error if len(test.formatOptions) > 0 { err = mounter.FormatAndMountSensitiveWithFormatOptions(device, dest, test.fstype, test.mountOptions, test.sensitiveMountOptions, test.formatOptions) From cfbc5dc54f86274aa87df7ab7957f83d833b8fc2 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 7 Jun 2023 14:18:08 -0700 Subject: [PATCH 8/8] mount-utils: fix linter warnings in tests Mostly "return value is not checked". Signed-off-by: Kir Kolyshkin --- .../k8s.io/mount-utils/mount_helper_test.go | 6 ++--- .../k8s.io/mount-utils/mount_linux_test.go | 23 +++++++++++++------ 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/staging/src/k8s.io/mount-utils/mount_helper_test.go b/staging/src/k8s.io/mount-utils/mount_helper_test.go index abbc3199828..3293da66b75 100644 --- a/staging/src/k8s.io/mount-utils/mount_helper_test.go +++ b/staging/src/k8s.io/mount-utils/mount_helper_test.go @@ -41,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": { @@ -94,9 +93,8 @@ 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, }, diff --git a/staging/src/k8s.io/mount-utils/mount_linux_test.go b/staging/src/k8s.io/mount-utils/mount_linux_test.go index 7dae6cb73bd..ba575e0c469 100644 --- a/staging/src/k8s.io/mount-utils/mount_linux_test.go +++ b/staging/src/k8s.io/mount-utils/mount_linux_test.go @@ -30,6 +30,7 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" utilexec "k8s.io/utils/exec" testexec "k8s.io/utils/exec/testing" ) @@ -436,15 +437,17 @@ func TestSearchMountPoints(t *testing.T) { defer os.Remove(tmpFile.Name()) defer tmpFile.Close() for _, v := range testcases { - tmpFile.Truncate(0) - tmpFile.Seek(0, 0) - tmpFile.WriteString(v.mountInfos) - tmpFile.Sync() + assert.NoError(t, tmpFile.Truncate(0)) + _, err := tmpFile.Seek(0, 0) + assert.NoError(t, err) + _, err = tmpFile.WriteString(v.mountInfos) + assert.NoError(t, err) + assert.NoError(t, tmpFile.Sync()) refs, err := SearchMountPoints(v.source, tmpFile.Name()) if !reflect.DeepEqual(refs, v.expectedRefs) { t.Errorf("test %q: expected Refs: %#v, got %#v", v.name, v.expectedRefs, refs) } - if !reflect.DeepEqual(err, v.expectedErr) { + if err != v.expectedErr { t.Errorf("test %q: expected err: %v, got %v", v.name, v.expectedErr, err) } } @@ -703,7 +706,10 @@ func TestFormatConcurrency(t *testing.T) { // for one to be released for i := 0; i < tc.max+1; i++ { go func() { - mounter.format(fstype, nil) + _, err := mounter.format(fstype, nil) + if err != nil { + t.Errorf("format(%q): %v", fstype, err) + } }() } @@ -778,7 +784,10 @@ func TestFormatTimeout(t *testing.T) { for i := 0; i < maxConcurrency+1; i++ { go func() { - mounter.format(fstype, nil) + _, err := mounter.format(fstype, nil) + if err != nil { + t.Errorf("format(%q): %v", fstype, err) + } }() }