From 67ec00d6b8e032e2275f093c1d8d93832120ad3d Mon Sep 17 00:00:00 2001 From: Janario Oliveira Date: Fri, 13 Sep 2019 11:12:22 +0200 Subject: [PATCH 1/5] Unmount subpath should only scan the first level dir --- pkg/util/mount/fake.go | 11 +++++- pkg/volume/util/subpath/subpath_linux.go | 3 +- pkg/volume/util/subpath/subpath_linux_test.go | 34 ++++++++++++++++++- 3 files changed, 45 insertions(+), 3 deletions(-) diff --git a/pkg/util/mount/fake.go b/pkg/util/mount/fake.go index 66b877064d0..063973527f1 100644 --- a/pkg/util/mount/fake.go +++ b/pkg/util/mount/fake.go @@ -32,9 +32,12 @@ type FakeMounter struct { MountCheckErrors map[string]error // Some tests run things in parallel, make sure the mounter does not produce // any golang's DATA RACE warnings. - mutex sync.Mutex + mutex sync.Mutex + UmountFunc UmountFunc } +type UmountFunc func(path string) error + var _ Interface = &FakeMounter{} const ( @@ -117,6 +120,12 @@ func (f *FakeMounter) Unmount(target string) error { newMountpoints := []MountPoint{} for _, mp := range f.MountPoints { if mp.Path == absTarget { + if f.UmountFunc != nil { + err := f.UmountFunc(absTarget) + if err != nil { + return err + } + } klog.V(5).Infof("Fake mounter: unmounted %s from %s", mp.Device, absTarget) // Don't copy it to newMountpoints continue diff --git a/pkg/volume/util/subpath/subpath_linux.go b/pkg/volume/util/subpath/subpath_linux.go index b497a810dc2..59c8128409e 100644 --- a/pkg/volume/util/subpath/subpath_linux.go +++ b/pkg/volume/util/subpath/subpath_linux.go @@ -241,7 +241,8 @@ func doCleanSubPaths(mounter mount.Interface, podDir string, volumeName string) if err = doCleanSubPath(mounter, fullContainerDirPath, filepath.Base(path)); err != nil { return err } - return nil + // skip subdirs of the volume: it only matters the first level to unmount, otherwise it would try to unmount subdir of the volume + return filepath.SkipDir }) if err != nil { return fmt.Errorf("error processing %s: %s", fullContainerDirPath, err) diff --git a/pkg/volume/util/subpath/subpath_linux_test.go b/pkg/volume/util/subpath/subpath_linux_test.go index 954a134157a..f950ed8f821 100644 --- a/pkg/volume/util/subpath/subpath_linux_test.go +++ b/pkg/volume/util/subpath/subpath_linux_test.go @@ -409,6 +409,7 @@ func TestCleanSubPaths(t *testing.T) { // Function that validates directory structure after the test validate func(base string) error expectError bool + umount func(path string) error }{ { name: "not-exists", @@ -539,6 +540,37 @@ func TestCleanSubPaths(t *testing.T) { return validateDirExists(baseSubdir) }, }, + { + name: "subpath-with-files", + prepare: func(base string) ([]mount.MountPoint, error) { + path := filepath.Join(base, containerSubPathDirectoryName, testVol, "container1", "0") + path2 := filepath.Join(base, containerSubPathDirectoryName, testVol, "container1", "1") + if err := os.MkdirAll(filepath.Join(path, "my-dir-1"), defaultPerm); err != nil { + return nil, err + } + if err := os.MkdirAll(filepath.Join(path2, "my-dir-2"), defaultPerm); err != nil { + return nil, err + } + mounts := []mount.MountPoint{ + {Device: "/dev/sdb", Path: path}, + {Device: "/dev/sdc", Path: path2}, + } + return mounts, nil + }, + umount: func(mountpath string) error { + fileInfo, err := ioutil.ReadDir(mountpath) + for _, file := range fileInfo { + if err = os.RemoveAll(filepath.Join(mountpath, file.Name())); err != nil { + return err + } + } + + return nil + }, + validate: func(base string) error { + return validateDirNotExists(filepath.Join(base, containerSubPathDirectoryName)) + }, + }, } for _, test := range tests { @@ -553,7 +585,7 @@ func TestCleanSubPaths(t *testing.T) { t.Fatalf("failed to prepare test %q: %v", test.name, err.Error()) } - fm := &mount.FakeMounter{MountPoints: mounts} + fm := &mount.FakeMounter{MountPoints: mounts, UmountFunc: test.umount} err = doCleanSubPaths(fm, base, testVol) if err != nil && !test.expectError { From 439ce51441f0429c0b7853aa35fc4503954cb712 Mon Sep 17 00:00:00 2001 From: Janario Oliveira Date: Tue, 17 Sep 2019 07:24:50 +0200 Subject: [PATCH 2/5] Changed test case to use `filepath.Walk` --- pkg/volume/util/subpath/subpath_linux_test.go | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/pkg/volume/util/subpath/subpath_linux_test.go b/pkg/volume/util/subpath/subpath_linux_test.go index f950ed8f821..8a730a090bf 100644 --- a/pkg/volume/util/subpath/subpath_linux_test.go +++ b/pkg/volume/util/subpath/subpath_linux_test.go @@ -558,11 +558,19 @@ func TestCleanSubPaths(t *testing.T) { return mounts, nil }, umount: func(mountpath string) error { - fileInfo, err := ioutil.ReadDir(mountpath) - for _, file := range fileInfo { - if err = os.RemoveAll(filepath.Join(mountpath, file.Name())); err != nil { + err := filepath.Walk(mountpath, func(path string, info os.FileInfo, err error) error { + if path == mountpath { + // Skip top level directory + return nil + } + + if err = os.RemoveAll(path); err != nil { return err } + return filepath.SkipDir + }) + if err != nil { + return fmt.Errorf("error processing %s: %s", mountpath, err) } return nil From 2ca213579d4bc9591dbd4a9047c6e09549e1deb5 Mon Sep 17 00:00:00 2001 From: Janario Oliveira Date: Tue, 17 Sep 2019 07:30:10 +0200 Subject: [PATCH 3/5] Renamed function --- pkg/util/mount/fake.go | 10 +++++----- pkg/volume/util/subpath/subpath_linux_test.go | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/util/mount/fake.go b/pkg/util/mount/fake.go index 063973527f1..0400f3b1812 100644 --- a/pkg/util/mount/fake.go +++ b/pkg/util/mount/fake.go @@ -32,11 +32,11 @@ type FakeMounter struct { MountCheckErrors map[string]error // Some tests run things in parallel, make sure the mounter does not produce // any golang's DATA RACE warnings. - mutex sync.Mutex - UmountFunc UmountFunc + mutex sync.Mutex + UnmountFunc UnmountFunc } -type UmountFunc func(path string) error +type UnmountFunc func(path string) error var _ Interface = &FakeMounter{} @@ -120,8 +120,8 @@ func (f *FakeMounter) Unmount(target string) error { newMountpoints := []MountPoint{} for _, mp := range f.MountPoints { if mp.Path == absTarget { - if f.UmountFunc != nil { - err := f.UmountFunc(absTarget) + if f.UnmountFunc != nil { + err := f.UnmountFunc(absTarget) if err != nil { return err } diff --git a/pkg/volume/util/subpath/subpath_linux_test.go b/pkg/volume/util/subpath/subpath_linux_test.go index 8a730a090bf..ca0c0d3744d 100644 --- a/pkg/volume/util/subpath/subpath_linux_test.go +++ b/pkg/volume/util/subpath/subpath_linux_test.go @@ -409,7 +409,7 @@ func TestCleanSubPaths(t *testing.T) { // Function that validates directory structure after the test validate func(base string) error expectError bool - umount func(path string) error + unmount func(path string) error }{ { name: "not-exists", @@ -557,7 +557,7 @@ func TestCleanSubPaths(t *testing.T) { } return mounts, nil }, - umount: func(mountpath string) error { + unmount: func(mountpath string) error { err := filepath.Walk(mountpath, func(path string, info os.FileInfo, err error) error { if path == mountpath { // Skip top level directory @@ -593,7 +593,7 @@ func TestCleanSubPaths(t *testing.T) { t.Fatalf("failed to prepare test %q: %v", test.name, err.Error()) } - fm := &mount.FakeMounter{MountPoints: mounts, UmountFunc: test.umount} + fm := &mount.FakeMounter{MountPoints: mounts, UnmountFunc: test.unmount} err = doCleanSubPaths(fm, base, testVol) if err != nil && !test.expectError { From cb0ab22b2e3838042b991fd526fb2c0ae69394de Mon Sep 17 00:00:00 2001 From: Janario Oliveira Date: Mon, 4 Nov 2019 19:43:55 +0100 Subject: [PATCH 4/5] Added test case for subpath mount with file --- pkg/volume/util/subpath/subpath_linux.go | 9 ++++-- pkg/volume/util/subpath/subpath_linux_test.go | 31 +++++++++++++++---- 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/pkg/volume/util/subpath/subpath_linux.go b/pkg/volume/util/subpath/subpath_linux.go index 59c8128409e..164ed51a64f 100644 --- a/pkg/volume/util/subpath/subpath_linux.go +++ b/pkg/volume/util/subpath/subpath_linux.go @@ -241,8 +241,13 @@ func doCleanSubPaths(mounter mount.Interface, podDir string, volumeName string) if err = doCleanSubPath(mounter, fullContainerDirPath, filepath.Base(path)); err != nil { return err } - // skip subdirs of the volume: it only matters the first level to unmount, otherwise it would try to unmount subdir of the volume - return filepath.SkipDir + + if info.IsDir() { + // skip subdirs of the volume: it only matters the first level to unmount, otherwise it would try to unmount subdir of the volume + return filepath.SkipDir + } + + return nil }) if err != nil { return fmt.Errorf("error processing %s: %s", fullContainerDirPath, err) diff --git a/pkg/volume/util/subpath/subpath_linux_test.go b/pkg/volume/util/subpath/subpath_linux_test.go index ca0c0d3744d..c61b1270211 100644 --- a/pkg/volume/util/subpath/subpath_linux_test.go +++ b/pkg/volume/util/subpath/subpath_linux_test.go @@ -543,17 +543,36 @@ func TestCleanSubPaths(t *testing.T) { { name: "subpath-with-files", prepare: func(base string) ([]mount.MountPoint, error) { - path := filepath.Join(base, containerSubPathDirectoryName, testVol, "container1", "0") - path2 := filepath.Join(base, containerSubPathDirectoryName, testVol, "container1", "1") - if err := os.MkdirAll(filepath.Join(path, "my-dir-1"), defaultPerm); err != nil { + containerPath := filepath.Join(base, containerSubPathDirectoryName, testVol, "container1") + if err := os.MkdirAll(containerPath, defaultPerm); err != nil { return nil, err } - if err := os.MkdirAll(filepath.Join(path2, "my-dir-2"), defaultPerm); err != nil { + + file0 := filepath.Join(containerPath, "0") + if err := ioutil.WriteFile(file0, []byte{}, defaultPerm); err != nil { return nil, err } + + dir1 := filepath.Join(containerPath, "1") + if err := os.MkdirAll(filepath.Join(dir1, "my-dir-1"), defaultPerm); err != nil { + return nil, err + } + + dir2 := filepath.Join(containerPath, "2") + if err := os.MkdirAll(filepath.Join(dir2, "my-dir-2"), defaultPerm); err != nil { + return nil, err + } + + file3 := filepath.Join(containerPath, "3") + if err := ioutil.WriteFile(file3, []byte{}, defaultPerm); err != nil { + return nil, err + } + mounts := []mount.MountPoint{ - {Device: "/dev/sdb", Path: path}, - {Device: "/dev/sdc", Path: path2}, + {Device: "/dev/sdb", Path: file0}, + {Device: "/dev/sdc", Path: dir1}, + {Device: "/dev/sdd", Path: dir2}, + {Device: "/dev/sde", Path: file3}, } return mounts, nil }, From c9e97151b395fa2b79c615e2a4427f2754ac8edf Mon Sep 17 00:00:00 2001 From: Janario Oliveira Date: Thu, 7 Nov 2019 09:44:31 +0100 Subject: [PATCH 5/5] Changed unmount function for subpath with dirs --- pkg/volume/util/subpath/subpath_linux_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/volume/util/subpath/subpath_linux_test.go b/pkg/volume/util/subpath/subpath_linux_test.go index c61b1270211..cb046bfd277 100644 --- a/pkg/volume/util/subpath/subpath_linux_test.go +++ b/pkg/volume/util/subpath/subpath_linux_test.go @@ -583,7 +583,7 @@ func TestCleanSubPaths(t *testing.T) { return nil } - if err = os.RemoveAll(path); err != nil { + if err = os.Remove(path); err != nil { return err } return filepath.SkipDir