mount: replace mountinfo handling with moby/sys/mountinfo by thaJeztah · Pull Request #4578 · containerd/containerd

thaJeztah · 2020-09-22T11:01:29Z

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#34~~ merged

~~With this change, the only consumer of mount.Self() is containerd/cri. mount.PID() looks to be unused altogether, so both could potentially be removed if containerd/cri is updated.~~ removed

k8s-ci-robot · 2020-09-22T11:01:30Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

theopenlab-ci · 2020-09-22T11:07:06Z

Build succeeded.

containerd-build-arm64 : FAILURE in 4m 14s (non-voting)

theopenlab-ci · 2020-09-22T11:18:40Z

Build succeeded.

containerd-build-arm64 : FAILURE in 5m 24s (non-voting)

theopenlab-ci · 2020-09-22T11:28:52Z

Build succeeded.

containerd-build-arm64 : FAILURE in 5m 21s (non-voting)

fuweid · 2020-09-22T16:18:54Z

But it brings new dependencies into containerd 😂

estesp · 2020-09-22T16:22:04Z

New dependencies, but fewer total lines of code? "+5,059 −8,426" ❓ 😂

thaJeztah · 2020-09-22T16:24:09Z

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

theopenlab-ci · 2020-09-28T09:27:54Z

Build succeeded.

containerd-build-arm64 : FAILURE in 5m 32s (non-voting)

theopenlab-ci · 2020-09-28T09:45:54Z

Build succeeded.

containerd-build-arm64 : FAILURE in 5m 32s (non-voting)

kolyshkin · 2020-09-28T15:43:15Z

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.

I also think we can implement this in mountinfo itself, as it's been used from a few places.

updated; PTAL

theopenlab-ci · 2020-10-01T22:51:06Z

Build succeeded.

containerd-build-arm64 : FAILURE in 5m 32s (non-voting)

theopenlab-ci · 2020-10-06T23:46:07Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 48s (non-voting)

theopenlab-ci · 2020-10-29T22:35:15Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 47s (non-voting)

thaJeztah · 2020-10-29T22:48:45Z

Moving this out of draft; removed the remaining uses in the CRI code now that it was integrated

@estesp @kolyshkin @cpuguy83 PTAL

theopenlab-ci · 2020-11-10T15:37:00Z

Build succeeded.

containerd-build-arm64 : FAILURE in 5m 10s (non-voting)

theopenlab-ci · 2020-11-13T17:54:16Z

Build succeeded.

containerd-build-arm64 : FAILURE in 6m 34s (non-voting)

theopenlab-ci · 2020-12-03T15:46:09Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 53s (non-voting)

kolyshkin · 2020-12-03T17:48:31Z

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) (or mount.UnmountAll(target, unix.MNT_DETACH)) -- provided that target 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.

@kolyshkin would that mean we can replace this whole function with moby/mount.RecursiveUnmount(), or would we still need some of the logic below?

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

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.

kolyshkin · 2020-12-03T17:49:35Z

@thaJeztah thanks for working on this! LGTM overall, maybe makes sense to squash the first two (or three?) commits.

thaJeztah · 2020-12-07T10:44:35Z

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

IOW this function can be replaced with unix.Umount(target, unix.MNT_DETACH) (or mount.UnmountAll(target, unix.MNT_DETACH))

Do you think we should replace it here? (mostly wondering if the current logic was addressing specific (corner) cases)

theopenlab-ci · 2020-12-07T10:51:56Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 6m 45s (non-voting)

theopenlab-ci · 2020-12-07T21:07:28Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 6m 03s (non-voting)

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 <[email protected]>

Signed-off-by: Sebastiaan van Stijn <[email protected]>

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 <[email protected]>

thaJeztah · 2021-01-08T16:36:31Z

Rebased to get a fresh run of CI

@AkihiroSuda @estesp @kolyshkin @cpuguy83 PTAL

theopenlab-ci · 2021-01-08T16:44:01Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 6m 13s (non-voting)

kolyshkin · 2021-01-08T18:45:36Z

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


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.

kolyshkin

LGTM

dmcgowan

LGTM

estesp

LGTM

thaJeztah force-pushed the use_moby_sys branch from 9c3ba3f to 4d618ff Compare September 22, 2020 11:11

thaJeztah force-pushed the use_moby_sys branch from 4d618ff to 528144f Compare September 22, 2020 11:22

thaJeztah force-pushed the use_moby_sys branch from 528144f to 218e473 Compare September 28, 2020 09:20

thaJeztah force-pushed the use_moby_sys branch 2 times, most recently from 9ad3dfd to 6363e49 Compare September 28, 2020 09:38

kolyshkin reviewed Sep 28, 2020

View reviewed changes

thaJeztah force-pushed the use_moby_sys branch from 6363e49 to f6afb96 Compare October 1, 2020 22:44

thaJeztah force-pushed the use_moby_sys branch from f6afb96 to 0e71811 Compare October 6, 2020 23:38

thaJeztah force-pushed the use_moby_sys branch 2 times, most recently from b7b0ed3 to d92d3a3 Compare October 29, 2020 22:28

thaJeztah changed the title ~~[draft] mount: replace mountinfo handling with moby/sys/mountinfo~~ mount: replace mountinfo handling with moby/sys/mountinfo Oct 29, 2020

thaJeztah marked this pull request as ready for review October 29, 2020 22:47

thaJeztah force-pushed the use_moby_sys branch from d92d3a3 to a9bdd1e Compare November 10, 2020 15:30

thaJeztah force-pushed the use_moby_sys branch from a9bdd1e to 79f93bc Compare November 13, 2020 17:45

thaJeztah force-pushed the use_moby_sys branch from 79f93bc to e8fb2c3 Compare December 3, 2020 15:38

kolyshkin reviewed Dec 3, 2020

View reviewed changes

Comment thread mount/mountinfo.go Outdated

kolyshkin suggested changes Dec 3, 2020

View reviewed changes

thaJeztah force-pushed the use_moby_sys branch from e8fb2c3 to b5532da Compare December 7, 2020 10:42

thaJeztah force-pushed the use_moby_sys branch from b5532da to aec5325 Compare December 7, 2020 20:59

thaJeztah added 6 commits January 8, 2021 17:31

mount: mount.PID(), mount.Self(): change signature to return pointers

43578a9

Signed-off-by: Sebastiaan van Stijn <[email protected]>

mount.CleanupTempMounts() use github.com/moby/sys/mountinfo

f6a0ec7

Signed-off-by: Sebastiaan van Stijn <[email protected]>

mount: remove unused mount.PID()

474abe5

Signed-off-by: Sebastiaan van Stijn <[email protected]>

mount: remove remaining uses of mount.Self()

7572919

Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah force-pushed the use_moby_sys branch from aec5325 to 2374178 Compare January 8, 2021 16:36

kolyshkin reviewed Jan 8, 2021

View reviewed changes

Comment thread mount/mountinfo.go Outdated

kolyshkin reviewed Jan 8, 2021

View reviewed changes

Comment thread mount/mountinfo_bsd.go Outdated

kolyshkin reviewed Jan 8, 2021

View reviewed changes

kolyshkin approved these changes Jan 8, 2021

View reviewed changes

dmcgowan approved these changes Jan 12, 2021

View reviewed changes

estesp approved these changes Jan 12, 2021

View reviewed changes

estesp merged commit e62d03b into containerd:master Jan 12, 2021

thaJeztah deleted the use_moby_sys branch January 12, 2021 16:33

Conversation

thaJeztah commented Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Sep 22, 2020

Uh oh!

theopenlab-ci Bot commented Sep 22, 2020

Uh oh!

theopenlab-ci Bot commented Sep 22, 2020

Uh oh!

theopenlab-ci Bot commented Sep 22, 2020

Uh oh!

fuweid commented Sep 22, 2020

Uh oh!

estesp commented Sep 22, 2020

Uh oh!

thaJeztah commented Sep 22, 2020

Uh oh!

theopenlab-ci Bot commented Sep 28, 2020

Uh oh!

theopenlab-ci Bot commented Sep 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theopenlab-ci Bot commented Oct 1, 2020

Uh oh!

theopenlab-ci Bot commented Oct 6, 2020

Uh oh!

theopenlab-ci Bot commented Oct 29, 2020

Uh oh!

thaJeztah commented Oct 29, 2020

Uh oh!

theopenlab-ci Bot commented Nov 10, 2020

Uh oh!

theopenlab-ci Bot commented Nov 13, 2020

Uh oh!

theopenlab-ci Bot commented Dec 3, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Dec 3, 2020

Uh oh!

thaJeztah commented Dec 7, 2020

Uh oh!

theopenlab-ci Bot commented Dec 7, 2020

Uh oh!

theopenlab-ci Bot commented Dec 7, 2020

Uh oh!

thaJeztah commented Jan 8, 2021

Uh oh!

theopenlab-ci Bot commented Jan 8, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

dmcgowan left a comment

Choose a reason for hiding this comment

Uh oh!

estesp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

thaJeztah commented Sep 22, 2020 •

edited

Loading