Add test for RecursiveUnmount when a submount fails to unmount. #107

manugupt1 · 2022-03-07T06:24:45Z

Add test for RecursiveUnmount when a submount fails to unmount.

Signed-off-by: Manu Gupta manugupt1@gmail.com

kolyshkin · 2022-03-07T22:34:28Z

mount/mount_unix.go

-func Unmount(target string) error {
+var Unmount = func(target string) error {


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.

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.

Thanks for the suggestion. I did not know about shadow mounts. I updated the test by using your suggestion.

mount/mount_unix_test.go

manugupt1 · 2022-03-19T23:09:00Z

HI @kolyshkin
Will you be able to review this when you get a chance?

Thanks

kolyshkin · 2022-04-06T03:15:02Z

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")

manugupt1 · 2022-04-06T06:01:57Z

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

kolyshkin · 2022-04-06T17:53:25Z

mount/mount_unix_test.go

+	// unmount shadowed mounts
+	shadowedMounts := []string{child, grandchild}
+	for _, shadowedMount := range shadowedMounts {
+		// Unmount dir, make sure dir-other is still mounted.


This comment no longer reflects reality -- I don't see "dir" and "dir-other" in here.

kolyshkin · 2022-04-06T17:58:13Z

mount/mount_unix_test.go

+	child := filepath.Dir(grandchild)
+	parent := filepath.Dir(child)
+
+	// order is necessary to create shadow mounts.


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.

thaJeztah · 2022-04-07T11:11:30Z

mount/mount_unix_test.go

+	"golang.org/x/sys/unix"
+
 	"github.com/moby/sys/mountinfo"


Minor nit; can you remove the empty line and sort the imports?

thaJeztah · 2022-04-07T11:13:31Z

@manugupt1 with #109 merged, do you need new releases of mount and mountinfo after this was merged, or do you anticipate other changes coming before we should do a new release?

manugupt1 · 2022-04-07T17:44:44Z

@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.

kolyshkin · 2022-04-08T01:47:41Z

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.

kolyshkin · 2022-04-08T01:50:27Z

mount/mount_unix_test.go

+	// unmount shadowed mounts
+	shadowedMounts := []string{child, grandchild}
+	for _, shadowedMount := range shadowedMounts {
+		t.Run("RecursiveUnmount returns an error", func(t *testing.T) {


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?

Good catch; yes, I think that would work; I'd change this to:

t.Run(shadowedMount, func(t *testing.T) {

thaJeztah · 2022-04-08T08:47:05Z

mount/mount_unix_test.go

+		//nolint:errcheck
+		defer Unmount(dir)


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

thaJeztah · 2022-04-08T09:05:38Z

mount/mount_unix_test.go

+	grandchild := path.Join(tmp, dir)
+	child := filepath.Dir(grandchild)
+	parent := filepath.Dir(child)


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).

thaJeztah · 2022-04-08T22:05:55Z

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 <thaJeztah@users.noreply.github.com> Signed-off-by: Manu Gupta <manugupt1@gmail.com>

thaJeztah

LGTM, thanks!

@kolyshkin PTAL

kolyshkin

LGTM

manugupt1 force-pushed the add-test branch from fde4511 to 8299665 Compare March 7, 2022 06:27

kolyshkin reviewed Mar 7, 2022

View reviewed changes

mount/mount_unix_test.go Show resolved Hide resolved

manugupt1 force-pushed the add-test branch 2 times, most recently from c7ad270 to 616f9ba Compare March 8, 2022 03:56

manugupt1 requested a review from kolyshkin March 8, 2022 03:57

manugupt1 force-pushed the add-test branch 3 times, most recently from 4aae220 to 8c119c0 Compare March 9, 2022 04:32

manugupt1 force-pushed the add-test branch from 7f651ba to b8cddd9 Compare April 6, 2022 06:01

kolyshkin reviewed Apr 6, 2022

View reviewed changes

manugupt1 force-pushed the add-test branch from b8cddd9 to e43a12e Compare April 7, 2022 00:15

thaJeztah reviewed Apr 7, 2022

View reviewed changes

manugupt1 force-pushed the add-test branch from e43a12e to 352cedc Compare April 7, 2022 15:30

kolyshkin reviewed Apr 8, 2022

View reviewed changes

thaJeztah reviewed Apr 8, 2022

View reviewed changes

mount/mount_unix_test.go Outdated Show resolved Hide resolved

thaJeztah reviewed Apr 8, 2022

View reviewed changes

manugupt1 force-pushed the add-test branch from ddd07c0 to 018d979 Compare April 8, 2022 19:16

Add test for RecursiveUnmount when a submount fails to unmount.

3f01d17

Uses shadow mounts to get ENOENT for deeper level mounts. moby#45 Co-authored-by: Sebastiaan van Stijn <thaJeztah@users.noreply.github.com> Signed-off-by: Manu Gupta <manugupt1@gmail.com>

manugupt1 force-pushed the add-test branch from 018d979 to 3f01d17 Compare April 8, 2022 22:40

thaJeztah approved these changes Apr 8, 2022

View reviewed changes

kolyshkin approved these changes Apr 9, 2022

View reviewed changes

kolyshkin merged commit d75da0c into moby:main Apr 9, 2022

kolyshkin mentioned this pull request Oct 27, 2023

mount: Add test coverage for RecursiveUnmount #45

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test for RecursiveUnmount when a submount fails to unmount. #107

Add test for RecursiveUnmount when a submount fails to unmount. #107

manugupt1 commented Mar 7, 2022

kolyshkin Mar 7, 2022

kolyshkin Mar 7, 2022

manugupt1 Mar 8, 2022

manugupt1 commented Mar 19, 2022

kolyshkin commented Apr 6, 2022

manugupt1 commented Apr 6, 2022 •

edited

kolyshkin Apr 6, 2022

kolyshkin Apr 6, 2022

thaJeztah Apr 7, 2022

thaJeztah commented Apr 7, 2022

manugupt1 commented Apr 7, 2022

kolyshkin commented Apr 8, 2022

kolyshkin Apr 8, 2022 •

edited

thaJeztah Apr 8, 2022

thaJeztah Apr 8, 2022

thaJeztah Apr 8, 2022

thaJeztah commented Apr 8, 2022

thaJeztah left a comment

kolyshkin left a comment

		func Unmount(target string) error {
		var Unmount = func(target string) error {

Add test for RecursiveUnmount when a submount fails to unmount. #107

Add test for RecursiveUnmount when a submount fails to unmount. #107

Conversation

manugupt1 commented Mar 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manugupt1 commented Mar 19, 2022

kolyshkin commented Apr 6, 2022

manugupt1 commented Apr 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Apr 7, 2022

manugupt1 commented Apr 7, 2022

kolyshkin commented Apr 8, 2022

kolyshkin Apr 8, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Apr 8, 2022

thaJeztah left a comment

Choose a reason for hiding this comment

kolyshkin left a comment

Choose a reason for hiding this comment

manugupt1 commented Apr 6, 2022 •

edited

kolyshkin Apr 8, 2022 •

edited