Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unmount subpath should only scan the first level of files/directories #82698

Merged
merged 5 commits into from Nov 8, 2019
Merged

Unmount subpath should only scan the first level of files/directories #82698

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
67ec00d
Unmount subpath should only scan the first level dir
janario Sep 13, 2019
439ce51
Changed test case to use `filepath.Walk`
janario Sep 17, 2019
2ca2135
Renamed function
janario Sep 17, 2019
cb0ab22
Added test case for subpath mount with file
janario Nov 4, 2019
c9e9715
Changed unmount function for subpath with dirs
janario Nov 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 10 additions & 1 deletion pkg/util/mount/fake.go
View file
Open in desktop
Expand Up @@ -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
UnmountFunc UnmountFunc
}

type UnmountFunc func(path string) error

var _ Interface = &FakeMounter{}

const (
Expand Down Expand Up @@ -117,6 +120,12 @@ func (f *FakeMounter) Unmount(target string) error {
newMountpoints := []MountPoint{}
for _, mp := range f.MountPoints {
if mp.Path == absTarget {
if f.UnmountFunc != nil {
err := f.UnmountFunc(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
Expand Down
6 changes: 6 additions & 0 deletions pkg/volume/util/subpath/subpath_linux.go
View file
Open in desktop
Expand Up @@ -241,6 +241,12 @@ func doCleanSubPaths(mounter mount.Interface, podDir string, volumeName string)
if err = doCleanSubPath(mounter, fullContainerDirPath, filepath.Base(path)); err != nil {
return err
}

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 {
Expand Down
61 changes: 60 additions & 1 deletion pkg/volume/util/subpath/subpath_linux_test.go
View file
Open in desktop
Expand Up @@ -409,6 +409,7 @@ func TestCleanSubPaths(t *testing.T) {
// Function that validates directory structure after the test
validate func(base string) error
expectError bool
unmount func(path string) error
}{
{
name: "not-exists",
Expand Down Expand Up @@ -539,6 +540,64 @@ func TestCleanSubPaths(t *testing.T) {
return validateDirExists(baseSubdir)
},
},
{
name: "subpath-with-files",
prepare: func(base string) ([]mount.MountPoint, error) {
containerPath := filepath.Join(base, containerSubPathDirectoryName, testVol, "container1")
if err := os.MkdirAll(containerPath, defaultPerm); err != nil {
return nil, err
}

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: file0},
{Device: "/dev/sdc", Path: dir1},
{Device: "/dev/sdd", Path: dir2},
{Device: "/dev/sde", Path: file3},
}
return mounts, nil
},
unmount: func(mountpath string) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why we need this unmount function? The RemoveAll is especially scary if the test gets the path wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is set to test with the FakeMounter

The unmount process expects the directory to be empty after the unmount which will then remove the directory

Using the FakeMounter I do a clean up, to avoid it to fail in the remove

The bug itself is that without the skipDir it would start to try to unmount all the were inside my volume, which won't exist after the root unmount

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where I'm confused is that there's nothing under the "0", "1", etc test directories?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from the latest changes this is how the files look like:

janario@ubuntu:/tmp/clean-subpaths-subpath-with-files-825360851/volume-subpaths/vol1$ tree .
.
└── container1
    ├── 0
    ├── 1
    │   └── my-dir-1
    ├── 2
    │   └── my-dir-2
    └── 3

5 directories, 2 files

then the filepath.Walk starts with

  • 0 (file): unmount function doesn't do anything because it is a file and the file gets removed here
  • 1 (dir with sub dirs): unmount function will remove my-dir-1 but not 1
  • 2 (dir with sub dirs): same as dir 1
  • 3 (file): same as file 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I think I got your point, the scenario is created from my test case, so in the unmount from my scenario should know that my-dir-1 and my-dir-2 will always be empty

so there is no need for a recursive remove

I've just changed it to not recursive remove

let me know what do you think

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation! I didn't realize there were directories created underneath, so this makes sense to me.

I contemplated whether or not we should just add the cleanup dir logic to the FakeMounter, but then we would need the RemoveAll to be more generic, and I'm very wary of using RemoveAll anywhere. So I think this is fine.

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.Remove(path); err != nil {
return err
}
return filepath.SkipDir
})
if err != nil {
return fmt.Errorf("error processing %s: %s", mountpath, err)
}

return nil
},
validate: func(base string) error {
return validateDirNotExists(filepath.Join(base, containerSubPathDirectoryName))
},
},
}

for _, test := range tests {
Expand All @@ -553,7 +612,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, UnmountFunc: test.unmount}

err = doCleanSubPaths(fm, base, testVol)
if err != nil && !test.expectError {
Expand Down