-
Notifications
You must be signed in to change notification settings - Fork 37
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
mountinfo.Mounted(): fix #90
Conversation
0d24022
to
9ba90c5
Compare
I briefly looked through moby/moby repo code (except Same for github.com/moby/sys/mount, the only user is |
@mkimuram PTAL 🙏🏻 (last two commits) if you have time |
9ba90c5
to
d65f02f
Compare
No longer a draft; PTAL @thaJeztah |
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
} |
Looks like this patch is not needed, since
Maybe for tests, yes (I haven't looked in how unit tests use |
Perhaps we need to document and test it. Let me update the last commit. |
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) |
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]>
d65f02f
to
90da438
Compare
Done. |
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.
@kolyshkin Really clear PR description, patch comments, and codes. Just two comments:
// 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). |
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 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).
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.
Actually, errors.Is
unwraps the error, that's the beauty of it.
Demo code: https://play.golang.org/p/yp5-3eOa9Fa
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.
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
}, | ||
}, | ||
{ | ||
desc: "path whose parent is a symlink to directory on another device", |
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.
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.
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.
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.
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? |
Sorry, I should have clearly commented that it is good to go. It looks good to me. |
Thanks! Just double-checking 😅 Thanks for helping review these changes; it's always good to have more eyes 🤗 |
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
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
, returningan error instead.
ENOENT
, they should filter itout. Amend the function doc accordingly.
There are some corner cases in which
mountedByOpenat2
ormountedByStat
do not work properly. In particular,
mountedByStat
do not work a parent of path is a symlink residing ondifferent filesystem.
To solve these cases, call
normalizePath()
before path is used for any ofmountedBy*
functions, not just beforemountedByMountinfo
like it was done earlier.
Fix
TestMountedBy
The way
prepareMounts
was written was very complicated from themaintainability 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