containerd-shim: Do not remount root MS_SLAVE by ijc · Pull Request #1141 · containerd/containerd

ijc · 2017-07-07T14:54:59Z

Mounting as MS_SLAVE here breaks use cases which want to use
rootPropagation=shared in order to expose mounts to the host (and other
containers binding the same subtree).

Note that runc will also setup root as required by rootPropagation, defaulting
to MS_PRIVATE.

Fixes #1132.

Signed-off-by: Ian Campbell [email protected]

codecov-io · 2017-07-07T15:01:16Z

Codecov Report

Merging #1141 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #1141   +/-   ##
======================================
  Coverage    28.1%   28.1%           
======================================
  Files          28      28           
  Lines        2832    2832           
======================================
  Hits          796     796           
  Misses       1887    1887           
  Partials      149     149

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b3e572...d42cb88. Read the comment docs.

crosbymichael · 2017-07-07T16:41:39Z

Because the shim mounts the rootfs, we also need to make sure it is unmounted before the shim exits with this change.

ijc · 2017-07-10T13:55:46Z

Thanks, I'll look into that. Perhaps that need also explains the EBUSY I was seeing when I tried the option of removing the mount altogether in #1132 (comment).

ijc · 2017-07-11T14:27:38Z

Updated with a version which removes the mount completely and also cleans up the rootfs mount. PTAL.

(transcribed from #1132 (comment) which I confusingly put in the wrong place)

crosbymichael · 2017-07-11T15:50:06Z

LGTM

estesp

Seems reasonable; just a few thoughts on the cleanup function

estesp · 2017-07-12T13:36:53Z

relatively insignificant, but maybe it's simpler just to do nrRootMounts = len(r.Rootfs) here at the top and not worry about counting in the loop?

This was deliberate in order to only undo the work we succeeded with. Although that is somewhat undermined by my not actually calling cleanupMounts in the error path for the Mount call itself!

Actually, the more i look at this why don't you just loop until you receive a mount error that the path is not mounted instead of keeping this count around?

You can just loop until you hit EINVAL

TL;DR, yes, I should loop until EINVAL.

I was thinking it was possible that the caller might have also mounted some stuff on the rootfs before calling create? It'd be weird for containerd to undo that. But thinking some more the rootfs here is entirely internal to containerd(-shim) and the user doesn't really formally know where it would be, which I suppose is why were have the rootfs slice in the first place...

EINVAL covers a multitude of sins according to umount(2), including "bad flags" in addition to "not a mount point" (I'm not too worried about "called with MNT_EXPIRE and either MNT_DETACH or MNT_FORCE").

I think it's still the right way to go though.

estesp · 2017-07-12T13:38:35Z

what about making this a defered function with a success variable declared as false before the defer definition. then the unmount can be gated by if !success {, and only set success to true at the one success path at the bottom before returning the pid?

In this case, wouldn't success be defined as success := err == nil. If so, can jut check if the err is nil at the top of the closure.

The err captured by the closure may not be the same err as gets assigned to further down the function, since err can be shadowed (e.g. if err := foo(); err != nil { return err } shadows the previous err, I think)

Yeah, my assumption is the err == nil defer style would require a careful undoing of err scoping in all the if blocks with a function-scoped var err error at the top, right?

Even then I think you can shadow without the compiler warning about it if the new declaration happens in a nested scope, so there would be a danger of forgetting and doing an err := somewhere.

cpuguy83 · 2017-07-12T13:49:09Z

On error cases, won't this ultimately give a partially unwound stack of mounts anyway?
Should it just return on the first error?

I figured there was no harm in trying to do all of them, but yes it is likely not to do much in practice after the first failure and will certainly not unmount the full stack if one of them fails.

I think it is still best to try.

We expect this (or something very similar) to be merged soon, it fixes linuxkit#2131 so moving ahead now. The new alpine mirror is linuxkit/alpine:ce99c286eec27d0e722fc0e6ac2a3d2b1abc874e Signed-off-by: Ian Campbell <[email protected]>

ijc · 2017-07-12T16:50:18Z

I made the change to use defer and a success flag.

ijc · 2017-07-13T08:10:01Z

Rebased onto alpha0.

We expect this (or something very similar) to be merged soon, it fixes linuxkit#2131 so moving ahead now. The new alpine mirror is linuxkit/alpine:6832775a7e861ee2d7842e157688ece52d007142 Signed-off-by: Ian Campbell <[email protected]>

estesp

One minor nit: it seems idiomatic in Go to not assign zero values; so var success bool is usually more accepted than success := false

Regardless, LGTM

Any other maintainers want to weigh in? I believe @ijc has responded to all feedback

ijc · 2017-07-19T13:58:37Z

Thanks @estesp I fixed the zero val assign.

Mounting as MS_SLAVE here breaks use cases which want to use rootPropagation=shared in order to expose mounts to the host (and other containers binding the same subtree), mounting as e.g. MS_SHARED is pointless in this context so just remove. Having done this we also need to arrange to manually clean up the mounts on delete, so do so. Note that runc will also setup root as required by rootPropagation, defaulting to MS_PRIVATE. Fixes containerd#1132. Signed-off-by: Ian Campbell <[email protected]>

Signed-off-by: Ian Campbell <[email protected]>

This avoids someone adding a new error path and forgetting to call the cleanup function. We prefer to use an explicit flag to gate the clean rather than relying on `err != nil` so we don't have to rely on people never accidentally shadowing the `err` as seen by the closure. Signed-off-by: Ian Campbell <[email protected]>

This is simpler than trying to count how many successful mounts we made. Signed-off-by: Ian Campbell <[email protected]>

ijc · 2017-07-20T10:10:26Z

Rebased onto alpha1.

stevvooe · 2017-07-21T18:59:27Z

LGTM

This looks okay. The only area that I'm worried about is unmount mounts that weren't mounted by the shim.

Since 'containerd/containerd#1141' has been merged into containerd git repo, so we don't need the below repo 'https://github.com/ijc/containerd.git' as the containerd git tree in that pkg. Moreover, with the 'ijc' containerd git tree, we'll have below error message when build the pkg/containerd: fatal: reference is not a tree: d42cb88ba2b08d2ca6c8c017d629b394bf1dd08c The command '/bin/sh -c git checkout $CONTAINERD_COMMIT' returned a non-zero code: 128. Except that above issue, we need to update the "github.com/Sirupsen/logrus" pkg imported, because the latest code has change the 'Sirupsen' to 'sirupsen', else the build process will complain that that pkg can't be found. We also need to update the caller of NewTask(.) adapting to the latest implementation of NewTask() to avoid the type conflict issue. Signed-off-by: Dennis Chen <[email protected]>

crosbymichael modified the milestones: containerd alpha, containerd beta Jul 10, 2017

ijc mentioned this pull request Jul 11, 2017

Shim unconditionally mounts rootfs as MS_SLAVE #1132

Closed

ijc force-pushed the rootfsPropagation branch from a13cb2f to f2fac1e Compare July 11, 2017 13:36

ijc changed the title ~~container-shim: Setup root as MS_SHARED instead of MS_SLAVE~~ containerd-shim: Do not remount root MS_SLAVE Jul 11, 2017

ijc force-pushed the rootfsPropagation branch 2 times, most recently from 1d46787 to 1f93aac Compare July 11, 2017 14:36

estesp reviewed Jul 12, 2017

View reviewed changes

cpuguy83 reviewed Jul 12, 2017

View reviewed changes

ijc mentioned this pull request Jul 12, 2017

Update containerd linuxkit/linuxkit#2159

Merged

ijc force-pushed the rootfsPropagation branch from 1f93aac to 6ade6f4 Compare July 12, 2017 16:15

ijc force-pushed the rootfsPropagation branch from 6ade6f4 to a8c2686 Compare July 13, 2017 08:09

ijc force-pushed the rootfsPropagation branch from 508eb0a to 3455ffc Compare July 13, 2017 09:15

estesp approved these changes Jul 19, 2017

View reviewed changes

ijc force-pushed the rootfsPropagation branch from 3455ffc to b37bb5e Compare July 19, 2017 13:58

Ian Campbell added 3 commits July 20, 2017 10:50

Cleanup mounts if we fail to mount one element of rootfs

300f083

Signed-off-by: Ian Campbell <[email protected]>

Loop umount'ing rootfs until there are no more mounts

d42cb88

This is simpler than trying to count how many successful mounts we made. Signed-off-by: Ian Campbell <[email protected]>

ijc force-pushed the rootfsPropagation branch from b37bb5e to d42cb88 Compare July 20, 2017 10:09

ijc mentioned this pull request Jul 20, 2017

Containerd v1.0.0 alpha1 linuxkit/linuxkit#2258

Merged

estesp merged commit a2df6d1 into containerd:master Jul 21, 2017

stevvooe modified the milestones: containerd alpha2, containerd beta Jul 21, 2017

ijc deleted the rootfsPropagation branch July 24, 2017 08:32

arm64b mentioned this pull request Jul 26, 2017

pkg/containerd: Fix the type conflict issue in containerd pkg linuxkit/linuxkit#2297

Closed

ijc mentioned this pull request Jul 27, 2017

Containerd v1.0.0 alpha2 linuxkit/linuxkit#2306

Merged

hinshun mentioned this pull request Jan 25, 2023

Use mount.Target to specify subdirectory of rootfs mount #7840

Merged

Conversation

ijc commented Jul 7, 2017

Uh oh!

codecov-io commented Jul 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

crosbymichael commented Jul 7, 2017

Uh oh!

ijc commented Jul 10, 2017

Uh oh!

ijc commented Jul 11, 2017

Uh oh!

crosbymichael commented Jul 11, 2017

Uh oh!

estesp left a comment

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!

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!

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!

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!

ijc commented Jul 12, 2017

Uh oh!

ijc commented Jul 13, 2017

Uh oh!

estesp left a comment

Choose a reason for hiding this comment

Uh oh!

ijc commented Jul 19, 2017

Uh oh!

ijc commented Jul 20, 2017

Uh oh!

stevvooe commented Jul 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov-io commented Jul 7, 2017 •

edited

Loading