config: Document 'rbind' and 'bind' mount options extensions #771

wking · 2017-04-14T05:15:39Z

My last attempt to get (recursive) bind mounts documented was in #530, which was rejected based on “MS_REC makes no sense what-so-ever outside of a bind mount”. That doesn't make sense to me, with mount(8) exposing MS_REC | MS_SHARED as --make-rshared, MS_REC | MS_SLAVE as --make-rslave, etc., but here's a reroll that uses rbind and bind instead, with a goal of specifying the (recursive) bind syntax so runtime-tools can rely on that instead of blindly assuming runtimes support an unspecified bind syntax.

mrunalp · 2017-04-26T15:17:25Z

I think we should link to the kernel documents instead.

dqminh · 2017-04-26T15:28:34Z

I dont think there's a kernel doc for this. Since we are converting the list of known strings values (i.e. rbind,noatime,slave etc to a list of mount flags), maybe having a table of required mappings to refer to here is useful.

For example taken from https://github.com/opencontainers/runc/blob/653207bc29a6d2d62b5d4f55b596467cb715a128/libcontainer/specconv/spec_linux.go#L630

flags	syscall
bind	syscall.MS_BIND
rbind	syscall.MS_BIND \| syscall.MS_REC

wking · 2017-04-26T16:50:41Z

On Wed, Apr 26, 2017 at 08:28:36AM -0700, Daniel, Dao Quang Minh wrote: rbind | syscall.MS_BIND | syscall.MS_REC

Now that we're becoming very clear about the mapping between ‘options’ and MS_* constants, do maintainers want to revisit a separate string for MS_REC (like the ‘recursive’ I floated in #530)? Or are we forging ahead with r* options for common MS_REC & MS_* pairings? We could just link to mount(2) or [1] to define the MS_* values instead of including a table. ‘rlimit’ would no longer be supported, which has a backwards-compat cost, but we'd be able to get out of the business of defining a wrapper around the kernel API (and based on #780 that's a business we want to get out of). [1]: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/include/uapi/linux/fs.h?id=refs/tags/v4.10.12#n104

crosbymichael · 2017-04-26T19:01:19Z

@wking NO

When I said rec makes no sense outside of binds, that was because you used binds in your examples and I try to simplify things for you because you take everything so literally.

rec is never used by itself and the format and options are spec'd based on fstab style that are common between /etc/fstab and mount(8).

I like @dqminh suggestion on the table if you feel like you must write more docs on this. Just follow the list that is already in the code and don't make up anything, document as is.

wking · 2017-04-26T21:19:23Z

I like @dqminh suggestion on the table…

That's pretty much what I have now, with the list here. It currently only covers bind and rbind. Do I need to convert that list to a table? Do I need to fill it out with the other entries from runC?

I expect we do need to fill it out somewhat, but don't think that has to happen in this PR. This PR can establish the pattern (table or list, wording pattern for each entry, etc.) and then once we get something landed I'll file a followup adding the other entries that aren't covered by the mount(8) references.

crosbymichael · 2017-04-26T21:24:25Z

I'm not a fan of the current PR contents and would rather not specify rbind and bind without the others. They should be no different than any of the others.

wking · 2017-04-27T21:01:33Z

I'm not a fan of the current PR contents…

Is there anything other than which values are explicitly listed in the spec you would like to see changed?

… and would rather not specify rbind and bind without the others. They should be no different than any of the others.

Of the options listed in runC's flags, bind and rbind are the only ones not covered by mount(8) filesystem-independent options (source here). RunC differs from the mount page in defining defaults as a no-op, while the man page defines it as rw (clear MS_RDONLY), suid (clear MS_NOSUID), dev (clear MS_NODEV), exec (clear MS_NOEXEC), auto (clear MS_NOAUTO), nouser (clear MS_USER), and async (clear MS_SYNCHRONOUS). The man page also defines silent and loud (set and clear MS_SILENT), which are not supported in runC (at least as of 1.0.0-rc3). The other filesystem-independent options from the man page look like they're backed by data and not mountflags in the mount(2) call.

So if this PR lands as it stands in bd28d9b, I expect runC will need to fix default and add silent and loud. If we want runC to be compliant as it stands, we'll have to drop the filesystem-independent mount(8) reference and inline a list with all of the MS_*-backed entries and then say something like “any unrecognized options are passed to mount(2) as data”. Or is there a third approach I'm missing that still allows compliance testing? And which approach sounds best to you?

They are not filesystem types, so they don't belong in 'type'. The specs claim mount(2) as inspiration for this modeling (which makes sense, since that's the syscall Linux runtimes will make to implement it), and there (recursive) bind is represented by mountflags (MS_REC | MS_BIND). Currently the 'options' property handles both mount(2)'s mountflags and data, so 'options' is a good spot for these two settings. Before this commit, we were punting this sort of table to mount(8)'s filesystem-independent mount options. With this commit we drop the mount(8) reference and replace it with explicit requirements based on mount(2), as approved by Michael [1]. Personally, I prefer the old mount(8) punt, but have been unable to get (recursive) bind documented without removing it. The option strings still come from mount(8)'s filesytem-independent mount options with the following exceptions: * move, rbind, rprivate, rshared, rslave, and runbindable are exposed in mount(8) through long options (e.g. --move). * (no)acl is listed under filesystem-specific mount options (e.g. for ext2). This commit covers the MS_* entries from [2] with the following exceptions: * MS_VERBOSE, which has been deprecated in favor of MS_SILENT. * MS_KERNMOUNT, since the mount(2) calls won't be kern_mount calls and they are not covered in mount(8). * MS_SUBMOUNT and other flags documented as "internal to the kernel". * MS_RMT_MASK, since it's a mask and not a flag. * MS_MGC_*, since the magic mount flag is ignored since Linux 2.4 according to mount(2). The example I'm touching used: "type": "bind", ... "options": ["rbind", ...] but I don't see a point to putting 'bind' in 'type' when it's not a filesystem type and you already have 'rbind' in 'options'. We could have stuck closer mount(2) by using: "options": ["recursive", "bind", ...] but while that approach extends more conveniently to the other recursive mounts (recursive shared, slave, private, and unbindable mounts), there has been resistance to a direct MS_REC analog [3,4]. I think that resistance is ungrounded (obviously the kernel and mount(2) feels like a composable MS_REC makes sense), but I'm not a mainainer. Since there are existing consumers using the old example format and similar things like runtime-tools: $ git log --oneline -1 | cat 03e8b89 Merge pull request opencontainers#176 from hmeng-19/set_oom_score_adj $ ./ocitools generate --template <(echo '{}') --bind ab:cd:ro | jq '.mounts[0]' { "destination": "cd", "type": "bind", "source": "ab", "options": [ "bind", "ro" ] } this may be a breaking change for some spec consumers (although that ocitools example will still work, because 'options' contains 'bind', which means the 'type' will be ignored). But even if there are broken consumers, we're still pre-1.0, the spec never explained what bind/rbind meant before this commit, and consolidating the Linux mount spec around mount(2) now will make life less confusing in the future. [1]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2017/opencontainers.2017-05-09-20.07.log.html#l-24 [2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/fs.h?id=refs/tags/v4.11#n105 [3]: opencontainers#530 (comment) [4]: opencontainers#771 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>

wking · 2017-05-10T17:43:02Z

Rerolled in bd28d9b → 4b1b93f to target mount(2) and replace the list with a table as discussed in yesterday's meeting. Since I was clarifying the mountflags and data values, I went ahead and clearly tied the other properties to mount(2) as well. This should be a no-op as far as spec-consumers are concerned unless they were using the unspecified bind type without putting (r)bind in options.

There are still a number of differences vs. runC where runC diverges from mount(8)'s filesystem-independent options:

The effect of defaults, as described earlier. We could follow runC and make defaults a no-op, but I don't see a point to that (who would put defaults in their options knowing it is a no-op?).
Missing support: silent, loud (as described earlier), (no)iversion, (no)lazytime. And the mount(8) filesystem-specific options which are represented by MS_* flags: (no)acl. And the mount(8) double-dash options which are represented by MS_* flags: move. We could follow runC and not define options for the missing MS_* settings, but that just breaks consumers who try to use them (since runtimes would have to pass lazytime, etc. through in data instead of setting MS_LAZYTIME).

I'm happy to submit patches to runC to correct those issues. But if runC patches are not acceptable and maintainers won't accept this PR unless I strictly match the current runC implementation, please say so again (ideally with some sort of motivation) and I'll spin them off into follow-up PRs.

More details in the commit message for those who are interested.

They are not filesystem types, so they don't belong in 'type'. The specs claim mount(2) as inspiration for this modeling (which makes sense, since that's the syscall Linux runtimes will make to implement it), and there (recursive) bind is represented by mountflags (MS_REC | MS_BIND). Currently the 'options' property handles both mount(2)'s mountflags and data, so 'options' is a good spot for these two settings. Before this commit, we were punting this sort of table to mount(8)'s filesystem-independent mount options. With this commit we drop the mount(8) reference and replace it with explicit requirements based on mount(2), as approved by Michael [1]. Personally, I prefer the old mount(8) punt, but have been unable to get (recursive) bind documented without removing it. The option strings still come from mount(8)'s filesytem-independent mount options with the following exceptions: * move, rbind, rprivate, rshared, rslave, and runbindable are exposed in mount(8) through long options (e.g. --move). * (no)acl is listed under filesystem-specific mount options (e.g. for ext2). This commit covers the MS_* entries from [2] with the following exceptions: * MS_VERBOSE, which has been deprecated in favor of MS_SILENT. * MS_KERNMOUNT, since the mount(2) calls won't be kern_mount calls and they are not covered in mount(8). * MS_SUBMOUNT and other flags documented as "internal to the kernel". * MS_RMT_MASK, since it's a mask and not a flag. * MS_MGC_*, since the magic mount flag is ignored since Linux 2.4 according to mount(2). The example I'm touching used: "type": "bind", ... "options": ["rbind", ...] but I don't see a point to putting 'bind' in 'type' when it's not a filesystem type and you already have 'rbind' in 'options'. We could have stuck closer mount(2) by using: "options": ["recursive", "bind", ...] but while that approach extends more conveniently to the other recursive mounts (recursive shared, slave, private, and unbindable mounts), there has been resistance to a direct MS_REC analog [3,4]. I think that resistance is ungrounded (obviously the kernel and mount(2) feels like a composable MS_REC makes sense), but I'm not a mainainer. Since there are existing consumers using the old example format and similar things like runtime-tools: $ git log --oneline -1 | cat 03e8b89 Merge pull request opencontainers#176 from hmeng-19/set_oom_score_adj $ ./ocitools generate --template <(echo '{}') --bind ab:cd:ro | jq '.mounts[0]' { "destination": "cd", "type": "bind", "source": "ab", "options": [ "bind", "ro" ] } this may be a breaking change for some spec consumers (although that ocitools example will still work, because 'options' contains 'bind', which means the 'type' will be ignored). But even if there are broken consumers, we're still pre-1.0, the spec never explained what bind/rbind meant before this commit, and consolidating the Linux mount spec around mount(2) now will make life less confusing in the future. [1]: http://ircbot.wl.linuxfoundation.org/meetings/opencontainers/2017/opencontainers.2017-05-09-20.07.log.html#l-24 [2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/fs.h?id=refs/tags/v4.11#n105 [3]: opencontainers#530 (comment) [4]: opencontainers#771 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>

Part of catching runC up with the spec, which punts valid options to mount(8) [1,2]. [1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0-rc5/config.md#L68 [2]: opencontainers/runtime-spec#771 Signed-off-by: W. Trevor King <wking@tremily.us>

Part of catching runC up with the spec, which punts valid options to mount(8) [1,2]. This is a filesystem-specific entry in mount(8), but it's represented by a MS_* flag in mount(2) so we need an entry in the translation table. [1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0-rc5/config.md#L68 [2]: opencontainers/runtime-spec#771 Signed-off-by: W. Trevor King <wking@tremily.us>

wking · 2017-05-26T16:28:15Z

I've filed opencontainers/runc#1460, opencontainers/runc#1461, opencontainers/runc#1462, and opencontainers/runc#1463 addressing the missing options from mount(8).

I have not filed a runC PR for MS_MOVE yet, because the current spec does not claim support for it. We'll see how the missing-option PRs play out, and I may file a MS_MOVE PR if they go over well.

Part of catching runC up with the spec, which punts valid options to mount(8) [1,2]. [1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0-rc5/config.md#L68 [2]: opencontainers/runtime-spec#771 Signed-off-by: W. Trevor King <wking@tremily.us>

Part of catching runC up with the spec, which punts valid options to mount(8) [1,2]. This is a filesystem-specific entry in mount(8), but it's represented by a MS_* flag in mount(2) so we need an entry in the translation table. [1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0-rc5/config.md#L68 [2]: opencontainers/runtime-spec#771 Signed-off-by: W. Trevor King <wking@tremily.us>

Part of catching runC up with the spec, which punts valid options to mount(8) [1,2]. [1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0-rc5/config.md#L68 [2]: opencontainers/runtime-spec#771 Signed-off-by: W. Trevor King <wking@tremily.us>

Part of catching runC up with the spec, which punts valid options to mount(8) [1,2]. This is a filesystem-specific entry in mount(8), but it's represented by a MS_* flag in mount(2) so we need an entry in the translation table. [1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0-rc5/config.md#L68 [2]: opencontainers/runtime-spec#771 Signed-off-by: W. Trevor King <wking@tremily.us>

And also silent, loud, (no)iversion, and (no)acl. This is part of catching runC up with the spec, which punts valid options to mount(8) [1,2]. (no)acl is a filesystem-specific entry in mount(8), but it's represented by a MS_* flag in mount(2) so we need an entry in the translation table. [1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0-rc5/config.md#L68 [2]: opencontainers/runtime-spec#771 Signed-off-by: W. Trevor King <wking@tremily.us>

wking · 2017-07-10T18:26:20Z

Can we re-open this now that opencontainers/runc#1460 has landed?

And also silent, loud, (no)iversion, and (no)acl. This is part of catching runC up with the spec, which punts valid options to mount(8) [1,2]. (no)acl is a filesystem-specific entry in mount(8), but it's represented by a MS_* flag in mount(2) so we need an entry in the translation table. [1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0-rc5/config.md#L68 [2]: opencontainers/runtime-spec#771 Signed-off-by: W. Trevor King <wking@tremily.us>

This initially came in with b3918a2 (Add bind mount example, 2015-06-30), but the 'bind' value is not one of the types you can get out of /proc/filesystems. Instead, bind mounts should leave the type unset and put either 'bind' or 'rbind' in options (although neither of those are documented either [1]). Since documenting (r)bind seems to be too difficult, we should at least stop setting type in the example to stop confusing users [2,3]. Runc still checks .Type instead of .Options for bind-ness in a few places [4,5,6], but we can address all of those by setting .Device to "bind" depending on .Options at [4]. [1]: opencontainers#771 [2]: opencontainers/runc#1744 [3]: https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/2gia6t1Dnv0 Subject: Runc default mount type Date: Tue, 6 Mar 2018 14:19:40 -0800 (PST) Message-Id: <57e18bd7-caad-4e21-bd7e-df016fda3efd@opencontainers.org> [4]: https://github.com/opencontainers/runc/blob/v1.0.0-rc5/libcontainer/specconv/spec_linux.go#L272-L276 [5]: https://github.com/opencontainers/runc/blob/v1.0.0-rc5/libcontainer/rootfs_linux.go#L33 [6]: https://github.com/opencontainers/runc/blob/v1.0.0-rc5/libcontainer/rootfs_linux.go#L234 Signed-off-by: W. Trevor King <wking@tremily.us>

From the "Creating a bind mount" section of mount(2) [1]: > If mountflags includes MS_BIND (available since Linux 2.4), then > perform a bind mount... > > The filesystemtype and data arguments are ignored. This commit adds support for configurations that leave the OPTIONAL type [2] unset for bind mounts. There's a related spec-example change in flight with [3], although my personal preference would be a more explicit spec for the whole mount structure [4]. [1]: http://man7.org/linux/man-pages/man2/mount.2.html [2]: https://github.com/opencontainers/runtime-spec/blame/v1.0.1/config.md#L102 [3]: opencontainers/runtime-spec#954 [4]: opencontainers/runtime-spec#771 Signed-off-by: W. Trevor King <wking@tremily.us>

And also silent, loud, (no)iversion, and (no)acl. This is part of catching runC up with the spec, which punts valid options to mount(8) [1,2]. (no)acl is a filesystem-specific entry in mount(8), but it's represented by a MS_* flag in mount(2) so we need an entry in the translation table. [1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0-rc5/config.md#L68 [2]: opencontainers/runtime-spec#771 Signed-off-by: W. Trevor King <wking@tremily.us>

From the "Creating a bind mount" section of mount(2) [1]: > If mountflags includes MS_BIND (available since Linux 2.4), then > perform a bind mount... > > The filesystemtype and data arguments are ignored. This commit adds support for configurations that leave the OPTIONAL type [2] unset for bind mounts. There's a related spec-example change in flight with [3], although my personal preference would be a more explicit spec for the whole mount structure [4]. [1]: http://man7.org/linux/man-pages/man2/mount.2.html [2]: https://github.com/opencontainers/runtime-spec/blame/v1.0.1/config.md#L102 [3]: opencontainers/runtime-spec#954 [4]: opencontainers/runtime-spec#771 Signed-off-by: W. Trevor King <wking@tremily.us>

And also silent, loud, (no)iversion, and (no)acl. This is part of catching runC up with the spec, which punts valid options to mount(8) [1,2]. (no)acl is a filesystem-specific entry in mount(8), but it's represented by a MS_* flag in mount(2) so we need an entry in the translation table. [1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0-rc5/config.md#L68 [2]: opencontainers/runtime-spec#771 Signed-off-by: W. Trevor King <wking@tremily.us>

From the "Creating a bind mount" section of mount(2) [1]: > If mountflags includes MS_BIND (available since Linux 2.4), then > perform a bind mount... > > The filesystemtype and data arguments are ignored. This commit adds support for configurations that leave the OPTIONAL type [2] unset for bind mounts. There's a related spec-example change in flight with [3], although my personal preference would be a more explicit spec for the whole mount structure [4]. [1]: http://man7.org/linux/man-pages/man2/mount.2.html [2]: https://github.com/opencontainers/runtime-spec/blame/v1.0.1/config.md#L102 [3]: opencontainers/runtime-spec#954 [4]: opencontainers/runtime-spec#771 Signed-off-by: W. Trevor King <wking@tremily.us>

wking force-pushed the mount-bind-v2 branch from bd28d9b to 9073ddf Compare May 10, 2017 17:29

wking force-pushed the mount-bind-v2 branch from 9073ddf to 4b1b93f Compare May 10, 2017 17:45

crosbymichael closed this May 10, 2017

This was referenced May 11, 2017

WIP: libcontainer/specconv/spec_linux: defaults should not be a no-op opencontainers/runc#1444

Closed

libcontainer/specconv/spec_linux: Use golang.org/x/sys/unix opencontainers/runc#1445

Closed

config.md: fix typo of context #807

Merged

wking mentioned this pull request May 24, 2017

Mount correction #854

Merged

wking mentioned this pull request May 26, 2017

libcontainer/specconv/spec_linux: Add support for (no)lazytime opencontainers/runc#1460

Merged

wking mentioned this pull request May 26, 2017

libcontainer/specconv/spec_linux: Add support for silent and loud opencontainers/runc#1461

Closed

wking mentioned this pull request May 26, 2017

libcontainer/specconv/spec_linux: Add support for (no)iversion opencontainers/runc#1462

Closed

wking mentioned this pull request May 26, 2017

libcontainer/specconv/spec_linux: Add support for (no)acl opencontainers/runc#1463

Closed

wking mentioned this pull request May 31, 2017

Add readonly option for mount #868

Merged

wking mentioned this pull request Jun 14, 2017

runtimetest: fix mounts check opencontainers/runtime-tools#392

Merged

wking mentioned this pull request Jul 10, 2017

config.md: document what type should be when bind-mount #878

Closed

wking mentioned this pull request Aug 17, 2017

runtimetest: add mounts order validation opencontainers/runtime-tools#444

Merged

wking mentioned this pull request Sep 22, 2017

config-linux: Allowed values for rootfsPropagation (include r* forms)? #923

Open

wking mentioned this pull request Mar 6, 2018

config: Drop 'type' from bind-mount example #954

Closed

wking mentioned this pull request Mar 6, 2018

libcontainer/specconv/spec_linux: Support empty 'type' for bind mounts opencontainers/runc#1753

Merged

wking mentioned this pull request May 9, 2018

bind is not a valid mount type #967

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: Document 'rbind' and 'bind' mount options extensions #771

config: Document 'rbind' and 'bind' mount options extensions #771

wking commented Apr 14, 2017 •

edited

Loading

mrunalp commented Apr 26, 2017

dqminh commented Apr 26, 2017

wking commented Apr 26, 2017 via email

crosbymichael commented Apr 26, 2017

wking commented Apr 26, 2017

crosbymichael commented Apr 26, 2017

wking commented Apr 27, 2017 •

edited

Loading

wking commented May 10, 2017 •

edited

Loading

wking commented May 26, 2017 •

edited

Loading

wking commented Jul 10, 2017

config: Document 'rbind' and 'bind' mount options extensions #771

config: Document 'rbind' and 'bind' mount options extensions #771

Conversation

wking commented Apr 14, 2017 • edited Loading

mrunalp commented Apr 26, 2017

dqminh commented Apr 26, 2017

wking commented Apr 26, 2017 via email

crosbymichael commented Apr 26, 2017

wking commented Apr 26, 2017

crosbymichael commented Apr 26, 2017

wking commented Apr 27, 2017 • edited Loading

wking commented May 10, 2017 • edited Loading

wking commented May 26, 2017 • edited Loading

wking commented Jul 10, 2017

wking commented Apr 14, 2017 •

edited

Loading

wking commented Apr 27, 2017 •

edited

Loading

wking commented May 10, 2017 •

edited

Loading

wking commented May 26, 2017 •

edited

Loading