-
Notifications
You must be signed in to change notification settings - Fork 42
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.
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
Add test for RecursiveUnmount when a submount fails to unmount. #107
Conversation
mount/mount_unix.go
Outdated
func Unmount(target string) error { | ||
var Unmount = func(target string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather not do that for the sake of a single unit test.
Unfortunately I don't know of any other way to make the deferred unmount fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, maybe I do.
Try to create a mount which is shadowed by another mount. This will be shown in mountinfo, but the directory will not be there, so the unmount will fail with ENOENT.
IOW something like
- mount "tmp/dir1/subdir" using tmpfs.
- mount "tmp/dir1" using tmpfs.
Now, RecursiveUnmount will try to unmount "tmp/dir1/subdir", and since it's hidden by the second mount, its unmount will fail with ENOENT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion. I did not know about shadow mounts. I updated the test by using your suggestion.
c7ad270
to
616f9ba
Compare
4aae220
to
8c119c0
Compare
HI @kolyshkin Thanks |
LGTM, except I don't understand why 3 mounts are needed -- it seems that 2 are enough. One other thing, can you please add a short description of what the added case tests (something like "$NAME checks that RecursiveUnmount does return an error when a submount fails to be unmounted") |
Having 3 mounts ensures that we check for the cases when suberr is nil and not nil. https://github.com/moby/sys/blob/main/mount/mount_unix.go#L72-L77 |
mount/mount_unix_test.go
Outdated
// unmount shadowed mounts | ||
shadowedMounts := []string{child, grandchild} | ||
for _, shadowedMount := range shadowedMounts { | ||
// Unmount dir, make sure dir-other is still mounted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment no longer reflects reality -- I don't see "dir" and "dir-other" in here.
mount/mount_unix_test.go
Outdated
child := filepath.Dir(grandchild) | ||
parent := filepath.Dir(child) | ||
|
||
// order is necessary to create shadow mounts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it makes sense to describe the idea here, so that the future readers will get a grasp about what is going on.
// Create a set of mounts that should result in RecursiveUnmount failure,
// caused by the fact that the grandchild mount is shadowed by the child mount,
// and the child mount is shadowed by the parent mount. So. these two mounts
// are listed in mountinfo, but since they are unreachable, unmount will fail.
mount/mount_unix_test.go
Outdated
"golang.org/x/sys/unix" | ||
|
||
"github.com/moby/sys/mountinfo" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit; can you remove the empty line and sort the imports?
@manugupt1 with #109 merged, do you need new releases of |
@thaJeztah I think there might be more changes required; I do not know enough k8s to see if I can base on the latest commit over master and then do follow ups either here or there if reqd. |
Technically you can, but maintainers will complain. For some package which did not do a release for a long time they can make an exception, and of course you can use an untagged version for testing. So, let us know once you finished the testing and need a release, and we'll tag one. |
mount/mount_unix_test.go
Outdated
// unmount shadowed mounts | ||
shadowedMounts := []string{child, grandchild} | ||
for _, shadowedMount := range shadowedMounts { | ||
t.Run("RecursiveUnmount returns an error", func(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, with this t.Run we have two subtests which are similarly named with the same name. I guess either to remove t.Run, or make the subtest name different (e.g. by t.Run(shadowedMount)
).
@thaJeztah WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch; yes, I think that would work; I'd change this to:
t.Run(shadowedMount, func(t *testing.T) {
mount/mount_unix_test.go
Outdated
//nolint:errcheck | ||
defer Unmount(dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't leave it as a comment earlier as it's a minor nit, but if you're updating, perhaps put the //nolint
after the line; slightly cleaner, and makes it slightly clearer which line the //nolint
applies to.
defer Unmount(dir) //nolint:errcheck
mount/mount_unix_test.go
Outdated
grandchild := path.Join(tmp, dir) | ||
child := filepath.Dir(grandchild) | ||
parent := filepath.Dir(child) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems making things more complicated than needed to find paths we already know what they'll be (relative to tmp
that is). Given that this only runs on unix/linux, we don't strictly need filepath
(path separator will always be /
) and because these paths are "fixed", we don't need to care about path-escaping etc. (also grandchild
should probably be grandChild
to be properly camelCase);
perhaps something like:
var (
tmp = t.TempDir()
parent = tmp + "/sub1"
child = tmp + "/sub1/sub2"
grandChild = tmp + "/sub1/sub2/sub3"
)
err := os.MkdirAll(grandChild, 0o700)
if err != nil {
t.Fatal(err)
}
That way, it's obvious at a glance what paths we're dealing with (the var( )
block makes them align nicely, so it's easier to read for this).
Thanks for updating; changes look good - could you squash the commits? (it's fine to squash my suggestions with your commit 👍) |
Uses shadow mounts to get ENOENT for deeper level mounts. moby#45 Co-authored-by: Sebastiaan van Stijn <[email protected]> Signed-off-by: Manu Gupta <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@kolyshkin PTAL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Add test for RecursiveUnmount when a submount fails to unmount.
#45
Signed-off-by: Manu Gupta [email protected]