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

wking · 2018-03-06T23:26:15Z

This example initially came in with b3918a2, 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, #771). Since documenting (r)bind seems to be too difficult (for reasons I don't understand), we should at least stop setting type in the example to stop confusing users.

Runc still checks .Type instead of .Options for bind-ness in a few places, but we can address all of those by setting .Device to bind depending on .Options here.

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>

wking · 2018-03-06T23:42:14Z

Runc still checks .Type instead of .Options for bind-ness in a few places, but we can address all of those by setting .Device to bind depending on .Options here.

Filed as opencontainers/runc#1753.

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>

Nabarun · 2018-03-07T05:03:35Z

config.md

@@ -115,7 +115,6 @@ For POSIX platforms the `mounts` structure has the following fields:
    },
    {
        "destination": "/data",
-        "type": "bind",
        "source": "/volumes/testing",
        "options": ["rbind","rw"]


Does it makes sense to have rbind in the options if type:bind is removed

Yes, rbind/bind are options and are not meant to be the "type". The type is not used when performing bind-mounts.

@cyphar please help me understand this, if I remove rbind from my options it will then not do the binding right?

... if I remove rbind from my options it will then not do the binding right?

You need either bind or rbind in options. You do not need to set type for binds per the spec, and if/when opencontainers/runc#1753 lands, you won't need to set type for runc binds either.

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>

vbatts · 2018-04-04T21:58:37Z

i reject this. 'bind' is suitable type.

wking · 2018-04-04T22:03:35Z

i reject this. 'bind' is suitable type.

I'm fine with folks including bind as their type if they want, but I don't see a need to when we also require them to set (r)bind in options. And mount(2) works fine without requiring "bind" for filesystemtype. Can you give more details about why you'd like to keep it? Especially since it diverges from the specified values? If you want to keep it, would you be comfortable documenting it in the spec?

vbatts · 2018-04-04T22:13:06Z

that is just an e.g. not a mandated list ... "bind" is a type of mount

…

On 4/4/18 6:03 PM, W. Trevor King wrote: i reject this. 'bind' is suitable type. I'm fine with folks including |bind| as their type if they want, but I don't see a need to when we /also/ require them to set |(r)bind| in |options|. And |mount(2)| works fine without requiring |"bind"| for |filesystemtype|. Can you give more details about why you'd like to keep it? Especially since it diverges from the specified values <https://github.com/opencontainers/runtime-spec/blame/v1.0.1/config.md#L103>? If you want to keep it, would you be comfortable documenting it in the spec? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#954 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEF6Wmly1_YFyAKf6AwvP47T4D8rGYUks5tlUM8gaJpZM4SfnD8>.

wking · 2018-04-04T22:30:37Z

On Wed, Apr 04, 2018 at 10:13:08PM +0000, Vincent Batts wrote: that is just an e.g. not a mandated list ... "bind" is a type of mount

Right. But the mount(2) API shows that you don't have to explicitly pass it in when you are separately setting MS_BIND or similar. And if you expect runtimes to *require* type set to ‘bind’ for bind mounts, I think you really want to land wording to that effect in the spec. I certainly don't see it now, and portable syntax for bind mounts seems like an important part of compliance testing.

wking · 2018-04-12T22:38:34Z

Cross-linking #878 (which I'd forgotten was open). That PR is in flight with docs that would support (but not require) the example change I'd proposed here.

wking · 2018-04-18T14:00:46Z

Any chance of revisiting this now that opencontainers/runc#1753 has landed?

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 mentioned this pull request Mar 6, 2018

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

Merged

Nabarun reviewed Mar 7, 2018

View reviewed changes

vbatts closed this Apr 4, 2018

wking mentioned this pull request Apr 12, 2018

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

Closed

wking mentioned this pull request May 9, 2018

bind is not a valid mount type #967

Closed

flx42 mentioned this pull request Jul 27, 2018

config: clarify source mount #981

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

wking commented Mar 6, 2018

wking commented Mar 6, 2018

Nabarun Mar 7, 2018

cyphar Mar 7, 2018

Nabarun Mar 7, 2018

wking Mar 7, 2018

vbatts commented Apr 4, 2018

wking commented Apr 4, 2018

vbatts commented Apr 4, 2018 via email

wking commented Apr 4, 2018 via email

wking commented Apr 12, 2018

wking commented Apr 18, 2018

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

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

Conversation

wking commented Mar 6, 2018

wking commented Mar 6, 2018

Nabarun Mar 7, 2018

Choose a reason for hiding this comment

cyphar Mar 7, 2018

Choose a reason for hiding this comment

Nabarun Mar 7, 2018

Choose a reason for hiding this comment

wking Mar 7, 2018

Choose a reason for hiding this comment

vbatts commented Apr 4, 2018

wking commented Apr 4, 2018

vbatts commented Apr 4, 2018 via email

wking commented Apr 4, 2018 via email

wking commented Apr 12, 2018

wking commented Apr 18, 2018