mountinfo.Mounted(): fix #90

kolyshkin · 2021-10-28T00:28:37Z

Fix Mounted() and its tests.

In case the argument does not exist, Mounted now returns an
appropriate error. Previously, this condition was treated as
"not a mount point", with no error returned, but in fact such
path could be a mount point, overshadowed by another mount.
Since the mount is still unreachable, it can not be unmounted
anyway, so we do not fall back to mountedByMountinfo, returning
an error instead.

⚠️ In case callers are not interested in ENOENT, they should filter it
out. Amend the function doc accordingly.
There are some corner cases in which mountedByOpenat2 or mountedByStat
do not work properly. In particular,
- neither work if there's an ending slash;
- neither work for a symlink to a mounted directory;
- mountedByStat do not work a parent of path is a symlink residing on
  different filesystem.
To solve these cases, call normalizePath() before path is used for any of
mountedBy* functions, not just before mountedByMountinfo
like it was done earlier.
Fix TestMountedBy

The way prepareMounts was written was very complicated from the
maintainability perspective, with all the test cases in the same basket.

Rewrite this in a much cleaner way, with individual subtests.

Add some more test cases that were failing before this PR.

Fixes: #88

kolyshkin · 2021-10-28T00:47:43Z

In case callers are not interested in ENOENT, they should filter it out.

I briefly looked through moby/moby repo code (except *_test.go files) and it looks like there are no users of mountinfo.Mounted that need to be modified because of this change.

Same for github.com/moby/sys/mount, the only user is MakeMount and looks like it will work equally well with this change.

kolyshkin · 2021-10-28T00:48:55Z

@mkimuram PTAL 🙏🏻 (last two commits) if you have time

kolyshkin · 2021-10-28T15:58:08Z

No longer a draft; PTAL @thaJeztah

thaJeztah · 2021-10-28T16:19:02Z

I was wondering if the graph driver needed to be updated; do you think we need a draft PR that vendors this branch? 🤔

--- a/daemon/graphdriver/driver_linux.go
+++ b/daemon/graphdriver/driver_linux.go
@@ -1,6 +1,9 @@
 package graphdriver // import "github.com/docker/docker/daemon/graphdriver"

 import (
+       "errors"
+       "os"
+
        "github.com/moby/sys/mountinfo"
        "golang.org/x/sys/unix"
 )
@@ -113,7 +116,10 @@ type defaultChecker struct {
 }

 func (c *defaultChecker) IsMounted(path string) bool {
-       m, _ := mountinfo.Mounted(path)
+       m, err := mountinfo.Mounted(path)
+       if err != nil && errors.Is(err, os.ErrNotExist) {
+               return false
+       }
        return m
 }

kolyshkin · 2021-10-28T16:22:27Z

I was wondering if the graph driver needed to be updated

Looks like this patch is not needed, since mountinfo.Mounted() always returns false when there's an error (although it's not enforced in the code).

do you think we need a draft PR that vendors this branch?

Maybe for tests, yes (I haven't looked in how unit tests use Mounted()).

kolyshkin · 2021-10-28T16:23:10Z

mountinfo.Mounted() always returns false when there's an error

Perhaps we need to document and test it. Let me update the last commit.

thaJeztah · 2021-10-28T16:23:56Z

I saw some places where tests were using it; I may try a draft PR (or if you have time, feel free to open one)

thaJeztah · 2021-10-28T16:24:46Z

That said; probably doesn't need to be a blocker before merging

1. In case the argument does not exist, Mounted now returns an appropriate error. Previously, this condition was treated as "not a mount point", with no error returned, but in fact such path could be a mount point, overshadowed by another mount. Since the mount is still unreachable, it can not be unmounted anyway, so we do not fall back to mountedByMountinfo, returning an error instead. In case callers are not interested in ENOENT, they should filter it out. Amend the function doc accordingly. 2. There are some corner cases in which mountedByOpenat2 or mountedByStat do not work properly. In particular, - neither work if there's an ending slash; - neither work for a symlink to a mounted directory; - mountedByStat do not work a parent of path is a symlink residing on different filesystem. To solve these cases, we have to call normalizePath() before path is used for any of mountedBy* functions, not just before mountedByMountinfo like it was done earlier. More tests are to be added by the next commit. Signed-off-by: Kir Kolyshkin <[email protected]>

The way prepareMounts was written was very complicated from the maintainability perspective, with all the test cases in the same basket. Rewrite this in a much cleaner way, with individual subtests. Add some more test cases that were failing before the previous commit. Signed-off-by: Kir Kolyshkin <[email protected]>

In case of any error, Mounted() always returns false. Document and check that. Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin · 2021-10-28T16:46:37Z

mountinfo.Mounted() always returns false when there's an error

Perhaps we need to document and test it. Let me update the last commit.

Done.

mkimuram

@kolyshkin Really clear PR description, patch comments, and codes. Just two comments:

mkimuram · 2021-10-28T18:23:38Z

mountinfo/mountinfo.go

+// The non-existent path returns an error. If a caller is not interested
+// in this particular error, it should handle it separately using e.g.
+// errors.Is(err, os.ErrNotExist).


I understand that the errors that are returned when neither original path nor resolved real path found will likely be the errors that normalizePath returns. If my understanding is correct, how can users handle error to exclude particular errors, as described?

(normalizePath seems to return wrapped errors, not original errors. And errors.Is doesn't seem to handle the wrapped error the same to the original one).

Actually, errors.Is unwraps the error, that's the beauty of it.

Demo code: https://play.golang.org/p/yp5-3eOa9Fa

Also note that %w is a special directive to wrap an error so it can be unwrapped.

Updated demo code with fmt.Errorf("%w"): https://play.golang.org/p/XQR6weZc-fj

mkimuram · 2021-10-28T18:25:57Z

mountinfo/mounted_linux_test.go

+ },
+ },
+ {
+ desc: "path whose parent is a symlink to directory on another device",


When thinking as closed-box testing, bind mount version of this case, like "path whose parent is a bind mount to directory (on another device)", may be interesting. However, if thinking as open-box testing, it would clearly work, so it may not be needed.

Right. I added this mostly to check that it fails on mountedByStat when a call to normalizePath is commented out.

With normalizePath in place, this indeed does not make much sense.

thaJeztah · 2021-11-03T09:33:35Z

CI on my test PR (moby/moby#42980) didn't explode, so LGTM

@mkimuram @kolyshkin are any changes still needed, for the discussion on #90 (comment) above, or is this good to go?

mkimuram · 2021-11-03T12:21:35Z

Sorry, I should have clearly commented that it is good to go. It looks good to me.

thaJeztah · 2021-11-03T13:13:52Z

Thanks! Just double-checking 😅

Thanks for helping review these changes; it's always good to have more eyes 🤗

thaJeztah

LGTM

kolyshkin marked this pull request as draft October 28, 2021 00:31

kolyshkin mentioned this pull request Oct 28, 2021

mountinfo.Mounted: incorrect results for some corner cases #88

Closed

kolyshkin added the impact/changelog label Oct 28, 2021

kolyshkin mentioned this pull request Oct 28, 2021

CI: enable more linters #91

Closed

kolyshkin force-pushed the fix-mounted branch 2 times, most recently from 0d24022 to 9ba90c5 Compare October 28, 2021 00:40

kolyshkin force-pushed the fix-mounted branch from 9ba90c5 to d65f02f Compare October 28, 2021 15:57

kolyshkin marked this pull request as ready for review October 28, 2021 15:57

kolyshkin mentioned this pull request Oct 28, 2021

CI: add more linters #92

Merged

kolyshkin added 3 commits October 28, 2021 09:37

mountinfo: Mounted: document/check false is returned on error

90da438

In case of any error, Mounted() always returns false. Document and check that. Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin force-pushed the fix-mounted branch from d65f02f to 90da438 Compare October 28, 2021 16:43

mkimuram reviewed Oct 28, 2021

View reviewed changes

thaJeztah mentioned this pull request Nov 1, 2021

vendor: github.com/moby/sys/mount v0.3.0, mountinfo v0.5.0, signal v0.6.0, symlink v0.2.0 moby/moby#42980

Merged

thaJeztah approved these changes Nov 3, 2021

View reviewed changes

thaJeztah merged commit 26b84be into moby:master Nov 3, 2021

kolyshkin mentioned this pull request Nov 5, 2021

Determine mount points without relying on /proc/mounts kubernetes/kubernetes#105833

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mountinfo.Mounted(): fix #90

mountinfo.Mounted(): fix #90

kolyshkin commented Oct 28, 2021 •

edited

Loading

kolyshkin commented Oct 28, 2021

kolyshkin commented Oct 28, 2021

kolyshkin commented Oct 28, 2021

thaJeztah commented Oct 28, 2021

kolyshkin commented Oct 28, 2021

kolyshkin commented Oct 28, 2021

thaJeztah commented Oct 28, 2021

thaJeztah commented Oct 28, 2021

kolyshkin commented Oct 28, 2021

mkimuram left a comment

mkimuram Oct 28, 2021

kolyshkin Oct 28, 2021

kolyshkin Oct 28, 2021

mkimuram Oct 28, 2021 •

edited

Loading

kolyshkin Oct 28, 2021

thaJeztah commented Nov 3, 2021

mkimuram commented Nov 3, 2021

thaJeztah commented Nov 3, 2021

thaJeztah left a comment

mountinfo.Mounted(): fix #90

mountinfo.Mounted(): fix #90

Conversation

kolyshkin commented Oct 28, 2021 • edited Loading

kolyshkin commented Oct 28, 2021

kolyshkin commented Oct 28, 2021

kolyshkin commented Oct 28, 2021

thaJeztah commented Oct 28, 2021

kolyshkin commented Oct 28, 2021

kolyshkin commented Oct 28, 2021

thaJeztah commented Oct 28, 2021

thaJeztah commented Oct 28, 2021

kolyshkin commented Oct 28, 2021

mkimuram left a comment

Choose a reason for hiding this comment

mkimuram Oct 28, 2021

Choose a reason for hiding this comment

kolyshkin Oct 28, 2021

Choose a reason for hiding this comment

kolyshkin Oct 28, 2021

Choose a reason for hiding this comment

mkimuram Oct 28, 2021 • edited Loading

Choose a reason for hiding this comment

kolyshkin Oct 28, 2021

Choose a reason for hiding this comment

thaJeztah commented Nov 3, 2021

mkimuram commented Nov 3, 2021

thaJeztah commented Nov 3, 2021

thaJeztah left a comment

Choose a reason for hiding this comment

kolyshkin commented Oct 28, 2021 •

edited

Loading

mkimuram Oct 28, 2021 •

edited

Loading