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
mount: replace mountinfo handling with moby/sys/mountinfo #4578
Conversation
Skipping CI for Draft Pull Request. |
Build succeeded.
|
9c3ba3f
to
4d618ff
Compare
Build succeeded.
|
4d618ff
to
528144f
Compare
Build succeeded.
|
But it brings new dependencies into containerd 😂 |
New dependencies, but fewer total lines of code? "+5,059 −8,426" ❓ 😂 |
Yeah, most of that is currently because of golang.org/x/sys and containerd/console. If we tag a new release of containerd/console, I can vendor it separately (nudge nudge 😉) |
528144f
to
218e473
Compare
Build succeeded.
|
9ad3dfd
to
6363e49
Compare
Build succeeded.
|
mount/lookup_unix.go
Outdated
"github.com/pkg/errors" | ||
) | ||
|
||
// Lookup returns the mount info corresponds to the path. | ||
func Lookup(dir string) (Info, error) { | ||
dir = filepath.Clean(dir) | ||
|
||
mounts, err := Self() | ||
m, err := mountinfo.GetMounts(mountinfo.SingleEntryFilter(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.
Alas, this is wrong. Previously this function was capable of returning info for e.g. mount point /mnt
given the argument of /mnt/some/thing
. Currently it looks for /mnt/some/thing
.
You need mountinfo.ParentsFilter(dir)
, sort, and take the longest one -- same as in getSourceMount
in moby/moby.
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 also think we can implement this in mountinfo itself, as it's been used from a few places.
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.
updated; PTAL
6363e49
to
f6afb96
Compare
Build succeeded.
|
f6afb96
to
0e71811
Compare
Build succeeded.
|
b7b0ed3
to
d92d3a3
Compare
Build succeeded.
|
Moving this out of draft; removed the remaining uses in the CRI code now that it was integrated @estesp @kolyshkin @cpuguy83 PTAL |
d92d3a3
to
a9bdd1e
Compare
Build succeeded.
|
a9bdd1e
to
79f93bc
Compare
Build succeeded.
|
79f93bc
to
e8fb2c3
Compare
Build succeeded.
|
pkg/cri/server/helpers_linux.go
Outdated
@@ -161,7 +162,7 @@ func openLogFile(path string) (*os.File, error) { | |||
// unmountRecursive unmounts the target and all mounts underneath, starting with | |||
// the deepest mount first. | |||
func unmountRecursive(ctx context.Context, target string) error { | |||
mounts, err := mount.Self() | |||
mounts, err := mountinfo.GetMounts(nil) |
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.
Two things.
-
This might benefit from using PrefixFilter (and when the logic will be simplified -- there's no need to do filepath.Rel and strings.HasPrefix below).
-
Not really related to this, but there's a little known fact that umount(MNT_DETACH) is actually recursive in Linux, IOW this function can be replaced with
unix.Umount(target, unix.MNT_DETACH)
(ormount.UnmountAll(target, unix.MNT_DETACH)
) -- provided thattarget
itself is a mount point. Related: mount.RecursiveUnmount: add a fast path moby/sys#26 -
Definitely not related to this, but I took a look at
pkg/mount.UnmountAll
and 😮 it retries umount without a delay on any error rather than EINVAL forever. This means, for example, if someone denies umount syscall using seccomp, we'll end up calling umount in a busy loop indefinitely.
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 would that mean we can replace this whole function with moby/mount.RecursiveUnmount()
, or would we still need some of the logic below?
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.
it retries umount without a delay on any error rather than EINVAL
I see it returns on errors (only returns nil
if it was an EINVAL
), or is that not the part you were looking at?
containerd/mount/mount_linux.go
Lines 179 to 182 in 45d8a7e
if err == unix.EINVAL { | |
return nil | |
} | |
return err |
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 see it returns on errors (only returns nil if it was an EINVAL), or is that not the part you were looking at?
You're right, I misread the code in there :) so item 3 above can be crossed out.
Re. item 2, I finally understood the logic of containerd's UnmountAll()
(it works around overmounts, i.e. when the same mountpoint being mounted to multiple times) and I think we should do something like this in moby/sys/mount, too. In the meantime, let's leave this as is, IOW ACK.
@thaJeztah thanks for working on this! LGTM overall, maybe makes sense to squash the first two (or three?) commits. |
e8fb2c3
to
b5532da
Compare
@kolyshkin I added the prefix-filter, and removed the local prefix checks; kept it as a separate commit for easier reviewing, but I can squash.
Do you think we should replace it here? (mostly wondering if the current logic was addressing specific (corner) cases) |
Build succeeded.
|
b5532da
to
aec5325
Compare
Build succeeded.
|
Trying to reduce duplicated effort in maintaining a mountinfo parser, this patch replaces the local implementation with the implementation in github.com/moby/sys, which is actively maintained and contains various optimizations. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Use a PrefixFilter() to get only the mounts we're interested in, which removes the need to manually filter mounts from the mountinfo results. Additional optimizations can be made, as: > ... there's a little known fact that `umount(MNT_DETACH)` is actually > recursive in Linux, IOW this function can be replaced with > `unix.Umount(target, unix.MNT_DETACH)` (or `mount.UnmountAll(target, unix.MNT_DETACH)` > (provided that target itself is a mount point). containerd@e8fb2c3#r535450446 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
aec5325
to
2374178
Compare
Rebased to get a fresh run of CI |
Build succeeded.
|
@@ -40,23 +41,20 @@ func SetTempMountLocation(root string) error { | |||
|
|||
// CleanupTempMounts all temp mounts and remove the directories | |||
func CleanupTempMounts(flags int) (warnings []error, err error) { | |||
mounts, err := Self() | |||
mounts, err := mountinfo.GetMounts(mountinfo.PrefixFilter(tempMountLocation)) |
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.
Alas we can't have a "fast path" I talked about earlier in here, since tempMountLocation
is not a mount point per se.
In case containerd itself is doing all those mounts, those can be saved in a list, and we should go over that list in here (instead of reading mountinfo) -- that would be a worthwhile optimization.
In case a user does those mounts, this is unfortunate and there's nothing we can do but read mountinfo.
In any case, this can be addressed later.
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
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
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
Trying to reduce duplicated effort in maintaining a mountinfo parser, this patch replaces the local implementation with the implementation in github.com/moby/sys, which is actively maintained and contains various optimizations.
Using a temporary vendor, pending moby/sys#32 and moby/sys#34mergedWith this change, the only consumer ofremovedmount.Self()
is containerd/cri.mount.PID()
looks to be unused altogether, so both could potentially be removed if containerd/cri is updated.