Implement private and shareable ipc modes by kolyshkin · Pull Request #34087 · moby/moby

kolyshkin · 2017-07-12T21:49:34Z

Since the commit d88fe44 ("Add support for sharing /dev/shm/ and
/dev/mqueue between containers") container's /dev/shm is mounted on the
host first, then bind-mounted inside the container. This is done that
way in order to be able to share this container's IPC namespace
(and the /dev/shm mount point) with another container.

Unfortunately, this functionality breaks container checkpoint/restore
(even if IPC is not shared). Since /dev/shm is an external mount, its
contents is not saved by criu checkpoint, and so upon restore any
application that tries to access data under /dev/shm is severily
disappointed as the data is not there (which usually results in a fatal crash). For example, see
https://github.com/xemul/criu/issues/300 and https://github.com/xemul/criu/issues/320.

This commit solves the issue by introducing new IPC modes for containers
(in addition to host and container:ID). The new modes are:

shareable: enables sharing this container's IPC with others
(this used to be a default behavior)
private: disables sharing this container's IPC.

In private mode, container's /dev/shm is truly mounted inside the
container, without any bind-mounting from the host, which solves the
issue for checkpoint/restore.

Surely, to ultimately solve it, we'd need to make private the default
mode, but it breaks the backward compatibility. So, let's make the
default container IPC mode per-daemon configurable (with built-in
default set to shareable for now). The default can be changed
either via a daemon CLI option (--default-shm-mode) or configuration
file parameter of the same name.

Surely, one can only set either shareable or private IPC modes as a
daemon default. Other modes (like 'host' or 'container') do not make
sense in this context and are therefore not allowed.

Some other changes this patch introduces are:

A mount for /dev/shm is added to default OCI Linux spec.
IpcMode.Valid() is simplified to remove duplicated code that parsed
'container:ID' form. Note the old version used to check that ID does
not contain a semicolon -- this is no longer the case (tests are
modified accordingly). The motivation is we should either do a
proper check for container ID validity, or don't check it at all
(since it is checked in other places anyway). I chose the latter.
IPC mode tests are modified to add checks for newly added values.

I did some manual testing, and it appears to be working.

~~TODO: write integration tests~~
Test cases come in the second patch, and they cover the scenarios I can think of.

kolyshkin · 2017-07-20T20:40:05Z

ping @thaJeztah @cpuguy83 @justincormack @rhatdan @mrunalp

This builds (and depends) on moby/moby#34087 Signed-off-by: Kir Kolyshkin <[email protected]>

thaJeztah

Thanks @kolyshkin - I left some comments (but interested in other reviews as well)

Wondering would this also reduce the chance of /shm "device in use" problems?

thaJeztah · 2017-07-20T22:22:59Z

cmd/dockerd/config_unix.go

Should the default be set here? ("shareable" - perhaps a constant defaultIPCMode)

Perhaps mention the possible options as well, given that they're limited;

`Default mode for containers ipc ("debug" | "info")`

thaJeztah · 2017-07-20T22:28:04Z

daemon/container_operations_unix.go

Unfortunately, there's already a couple of these, but we should try to avoid the API being imported into the daemon package. The daemon should not be aware of the API.

thaJeztah · 2017-07-20T22:59:47Z

daemon/daemon_unix.go

I'd change the wording a bit here.. something like;

"IPC mode (%v) is not supported as default value. Select either 'shareable' or 'private')".

thaJeztah · 2017-07-20T23:28:51Z

daemon/container_operations_unix.go

This doesn't look "right". The IpcMode should be stored in the container's hostconfig, not requested from the daemon. When creating the container, we should;

check if docker run --ipc=.. is set

if not, get the daemon's default, and set it to that value

Doing so prevents the container's IPC-mode to be updated if the daemon defaults are changed and the container restarted. Possibly have a similar function as runconfig.SetDefaultNetModeIfBlank(), and apply the defaults in daemon.create()

Unrelated to this PR, but looks like we should really evaluate where defaults should be set. I noticed the SetDefaultNetModeIfBlank() method is called in three different locations(!);

daemon.create()

daemon.setHostConfig() (which is called by daemon.create()), and

ContainerConfigWrapper.getHostConfig(), which is ultimately called through postContainersCreate() and postContainersStart() (last one for legacy reasons))

thaJeztah · 2017-07-20T23:31:05Z

daemon/container_operations_unix.go

Same comment here as above; at this point, c.HostConfig.IpcMode should already be set to a value

thaJeztah · 2017-07-20T23:42:07Z

daemon/daemon_unix.go

If the IpcMode is set during create, this will have to be updated to (e.g.) containertypes.IpcMode(config.IpcMode).Valid(). At this point we should not be checking the daemon defaults, but the container's setting.

verifyDefaultIpcMode() could still be needed, but only when verifying the daemon configuration (i.e. when starting the daemon and/or (re)loading the daemon configuration

thaJeztah · 2017-07-20T23:43:05Z

daemon/oci_linux.go

same remarks apply here as I made in container_operations_unix.go. The container should already have this configuration set at this point

thaJeztah · 2017-07-20T23:45:29Z

daemon/oci_linux.go

I think this should also be set earlier, so that it's baked into the container's config

thaJeztah · 2017-07-20T23:46:26Z

daemon/oci_linux.go

c.HostConfig.IpcMode.IsPrivate() (related to earlier comments)

thaJeztah · 2017-07-21T00:27:35Z

runconfig/hostconfig_test.go

nit: would it be more readable to use a struct for these? The second loop for parsing container names could be combined in that case;

func TestIpcModeTest(t *testing.T) { ipcModes := map[container.IpcMode]struct { private bool host bool container bool shareable bool valid bool ctrName string }{ "": {valid: true}, "private": {private: true, valid: true}, "something": {}, "something:weird": {ctrName: "weird"}, ":weird": {ctrName: "weird"}, "host": {host: true, valid: true}, "container": {}, "container:name": {container: true, ctrName: "name", valid: true}, "container:name1:name2": {container: true, ctrName: "name1:name2", valid: true}, "container:": {container: true, valid: true}, "shareable": {shareable: true, valid: true}, } for ipcMode, state := range ipcModes { assert.Equal(t, state.private, ipcMode.IsPrivate(), "IpcMode.IsPrivate() parsing failed for %q", ipcMode) assert.Equal(t, state.host, ipcMode.IsHost(), "IpcMode.IsHost() parsing failed for %q", ipcMode) assert.Equal(t, state.container, ipcMode.IsContainer(), "IpcMode.IsContainer() parsing failed for %q", ipcMode) assert.Equal(t, state.shareable, ipcMode.IsShareable(), "IpcMode.IsShareable() parsing failed for %q", ipcMode) assert.Equal(t, state.valid, ipcMode.Valid(), "IpcMode.Valid() parsing failed for %q", ipcMode) assert.Equal(t, state.ctrName, ipcMode.Container(), "IpcMode.Container() parsing failed for %q", ipcMode) } }

rhatdan · 2017-07-22T10:38:43Z

The way we have gone after this is to add a RUNC hook oci-umount which cleans up leaked mount points into a container.

https://github.com/projectatomic/oci-umount

kolyshkin · 2017-07-26T04:43:05Z

@thaJeztah thank you for review! I think I addressed all your comments in the updated patches, can you please take another look?

thaJeztah · 2017-07-26T21:44:20Z

Thank you @kolyshkin ! I gave it a quick glance, and definitely looks better; I'll do a full review soon.

This builds (and depends) on moby/moby#34087 Version 2: - remove --ipc argument validation (it is now done by daemon) - add/document 'none' value - docs/reference/run.md: add a table with better modes description - dockerd(8) typesetting fixes Version 3: - remove ipc mode tests from cli/command/container/opts_test.go Signed-off-by: Kir Kolyshkin <[email protected]>

thaJeztah · 2017-08-06T00:57:23Z

ping @justincormack PTAL

It was noted[1] that container's HostConfig.ShmSize, if not set, should be initialized to daemon default value during container creation. In fact, it is already done in daemon.adaptContainerSettings, so we can use value from container.HostConfig directly. [1] moby#34087 (comment) Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin · 2017-08-11T13:35:42Z

The last push is just a trivial rebase to newer master.

justincormack · 2017-08-12T13:38:10Z

integration-cli/docker_api_ipcmode_test.go

typo cat->can

One less cat! I feel the urge to compensate! Here:

justincormack · 2017-08-12T13:43:44Z

Thanks for adding none, I think thats a good idea. It could do with a test (make sure it has no mount at least). Otherwise this looks good to me.

Since the commit d88fe44 ("Add support for sharing /dev/shm/ and /dev/mqueue between containers") container's /dev/shm is mounted on the host first, then bind-mounted inside the container. This is done that way in order to be able to share this container's IPC namespace (and the /dev/shm mount point) with another container. Unfortunately, this functionality breaks container checkpoint/restore (even if IPC is not shared). Since /dev/shm is an external mount, its contents is not saved by `criu checkpoint`, and so upon restore any application that tries to access data under /dev/shm is severily disappointed (which usually results in a fatal crash). This commit solves the issue by introducing new IPC modes for containers (in addition to 'host' and 'container:ID'). The new modes are: - 'shareable': enables sharing this container's IPC with others (this used to be the implicit default); - 'private': disables sharing this container's IPC. In 'private' mode, container's /dev/shm is truly mounted inside the container, without any bind-mounting from the host, which solves the issue. While at it, let's also implement 'none' mode. The motivation, as eloquently put by Justin Cormack, is: > I wondered a while back about having a none shm mode, as currently it is > not possible to have a totally unwriteable container as there is always > a /dev/shm writeable mount. It is a bit of a niche case (and clearly > should never be allowed to be daemon default) but it would be trivial to > add now so maybe we should... ...so here's yet yet another mode: - 'none': no /dev/shm mount inside the container (though it still has its own private IPC namespace). Now, to ultimately solve the abovementioned checkpoint/restore issue, we'd need to make 'private' the default mode, but unfortunately it breaks the backward compatibility. So, let's make the default container IPC mode per-daemon configurable (with the built-in default set to 'shareable' for now). The default can be changed either via a daemon CLI option (--default-shm-mode) or a daemon.json configuration file parameter of the same name. Note one can only set either 'shareable' or 'private' IPC modes as a daemon default (i.e. in this context 'host', 'container', or 'none' do not make much sense). Some other changes this patch introduces are: 1. A mount for /dev/shm is added to default OCI Linux spec. 2. IpcMode.Valid() is simplified to remove duplicated code that parsed 'container:ID' form. Note the old version used to check that ID does not contain a semicolon -- this is no longer the case (tests are modified accordingly). The motivation is we should either do a proper check for container ID validity, or don't check it at all (since it is checked in other places anyway). I chose the latter. 3. IpcMode.Container() is modified to not return container ID if the mode value does not start with "container:", unifying the check to be the same as in IpcMode.IsContainer(). 3. IPC mode unit tests (runconfig/hostconfig_test.go) are modified to add checks for newly added values. [v2: addressed review at moby#34087 (review)] [v3: addressed review at moby#34087 (review)] [v4: addressed the case of upgrading from older daemon, in this case container.HostConfig.IpcMode is unset and this is valid] [v5: document old and new IpcMode values in api/swagger.yaml] [v6: add the 'none' mode, changelog entry to docs/api/version-history.md] Signed-off-by: Kir Kolyshkin <[email protected]>

These test cases cover various arguments for docker create/run --ipc option, as well as daemon's --default-ipc-mode cli option and configuration file parameter. For the description of container IPC modes, see previous commit. To run these: TESTFLAGS='-check.f IpcMode' make test-integration-cli [v2: simplify TestDaemonEvents(), add default-ipc-mode presense check] [v3: add TestDaemonIpcModeVSRestart] [v4: ipcmode test now uses client lib instead of CLI (except for exec)] [v5: nitpicks in comments] [v6: add test case for "none"; fix a typo; simplify TestDaemonEvents() more] Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin · 2017-08-14T08:07:11Z

It could do with a test (make sure it has no mount at least).

Done. Thank you for reviewing!

kolyshkin · 2017-08-14T10:18:37Z

Windows test failure seems to be caused by some tests (or execution environment) race, not related to the patchset. Will restart

thaJeztah · 2017-08-14T13:52:16Z

All green now; merging 👍

It was noted[1] that container's HostConfig.ShmSize, if not set, should be initialized to daemon default value during container creation. In fact, it is already done in daemon.adaptContainerSettings, so we can use value from container.HostConfig directly. [1] moby/moby#34087 (comment) Signed-off-by: Kir Kolyshkin <[email protected]> Upstream-commit: 0fb1fb1 Component: engine

Since the commit d88fe44 ("Add support for sharing /dev/shm/ and /dev/mqueue between containers") container's /dev/shm is mounted on the host first, then bind-mounted inside the container. This is done that way in order to be able to share this container's IPC namespace (and the /dev/shm mount point) with another container. Unfortunately, this functionality breaks container checkpoint/restore (even if IPC is not shared). Since /dev/shm is an external mount, its contents is not saved by `criu checkpoint`, and so upon restore any application that tries to access data under /dev/shm is severily disappointed (which usually results in a fatal crash). This commit solves the issue by introducing new IPC modes for containers (in addition to 'host' and 'container:ID'). The new modes are: - 'shareable': enables sharing this container's IPC with others (this used to be the implicit default); - 'private': disables sharing this container's IPC. In 'private' mode, container's /dev/shm is truly mounted inside the container, without any bind-mounting from the host, which solves the issue. While at it, let's also implement 'none' mode. The motivation, as eloquently put by Justin Cormack, is: > I wondered a while back about having a none shm mode, as currently it is > not possible to have a totally unwriteable container as there is always > a /dev/shm writeable mount. It is a bit of a niche case (and clearly > should never be allowed to be daemon default) but it would be trivial to > add now so maybe we should... ...so here's yet yet another mode: - 'none': no /dev/shm mount inside the container (though it still has its own private IPC namespace). Now, to ultimately solve the abovementioned checkpoint/restore issue, we'd need to make 'private' the default mode, but unfortunately it breaks the backward compatibility. So, let's make the default container IPC mode per-daemon configurable (with the built-in default set to 'shareable' for now). The default can be changed either via a daemon CLI option (--default-shm-mode) or a daemon.json configuration file parameter of the same name. Note one can only set either 'shareable' or 'private' IPC modes as a daemon default (i.e. in this context 'host', 'container', or 'none' do not make much sense). Some other changes this patch introduces are: 1. A mount for /dev/shm is added to default OCI Linux spec. 2. IpcMode.Valid() is simplified to remove duplicated code that parsed 'container:ID' form. Note the old version used to check that ID does not contain a semicolon -- this is no longer the case (tests are modified accordingly). The motivation is we should either do a proper check for container ID validity, or don't check it at all (since it is checked in other places anyway). I chose the latter. 3. IpcMode.Container() is modified to not return container ID if the mode value does not start with "container:", unifying the check to be the same as in IpcMode.IsContainer(). 3. IPC mode unit tests (runconfig/hostconfig_test.go) are modified to add checks for newly added values. [v2: addressed review at moby/moby#34087 (review)] [v3: addressed review at moby/moby#34087 (review)] [v4: addressed the case of upgrading from older daemon, in this case container.HostConfig.IpcMode is unset and this is valid] [v5: document old and new IpcMode values in api/swagger.yaml] [v6: add the 'none' mode, changelog entry to docs/api/version-history.md] Signed-off-by: Kir Kolyshkin <[email protected]> Upstream-commit: 7120976 Component: engine

GordonTheTurtle added the status/0-triage label Jul 12, 2017

kolyshkin mentioned this pull request Jul 19, 2017

/dev should be constrained in size just like /dev/shm #34166

Merged

kolyshkin force-pushed the ipcmode branch 2 times, most recently from 42827a6 to ea5b4eb Compare July 20, 2017 17:45

kolyshkin changed the title ~~[WIP] [RFC] Implement private and shareable ipc modes~~ Implement private and shareable ipc modes Jul 20, 2017

kolyshkin added a commit to kolyshkin/cli that referenced this pull request Jul 20, 2017

Introduce/document new IPC modes

b8707d9

This builds (and depends) on moby/moby#34087 Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin mentioned this pull request Jul 20, 2017

Introduce/document new IPC modes docker/cli#367

Merged

thaJeztah requested changes Jul 21, 2017

View reviewed changes

thaJeztah added status/2-code-review and removed status/0-triage labels Jul 21, 2017

kolyshkin force-pushed the ipcmode branch from ea5b4eb to 3417f34 Compare July 25, 2017 01:35

kolyshkin force-pushed the ipcmode branch from 3417f34 to 04e3f41 Compare July 25, 2017 01:36

kolyshkin force-pushed the ipcmode branch 4 times, most recently from 1db6766 to aa438d4 Compare July 25, 2017 05:03

kolyshkin force-pushed the ipcmode branch 5 times, most recently from ab5cac0 to e3be977 Compare July 25, 2017 23:46

kolyshkin force-pushed the ipcmode branch from ffe44ad to b870d09 Compare August 11, 2017 10:33

justincormack reviewed Aug 12, 2017

View reviewed changes

kolyshkin force-pushed the ipcmode branch from b870d09 to c4c59df Compare August 14, 2017 07:37

kolyshkin added 2 commits August 14, 2017 10:50

kolyshkin force-pushed the ipcmode branch from c4c59df to 9a60e1c Compare August 14, 2017 08:00

justincormack approved these changes Aug 14, 2017

View reviewed changes

thaJeztah merged commit bb6fc72 into moby:master Aug 14, 2017

thaJeztah mentioned this pull request Dec 18, 2017

start container without mounting shm into container #21609

Closed

thaJeztah mentioned this pull request Mar 12, 2018

starting a container read-only with IPC private mounts /dev/shm read-only #36503

Closed

kolyshkin mentioned this pull request Mar 12, 2018

daemon/setMounts(): do not make /dev/shm ro #36526

Merged

kolyshkin mentioned this pull request Nov 8, 2018

Use shareable IPC for sandbox container kubernetes/kubernetes#70826

Merged

This was referenced Feb 4, 2019

daemon: use 'private' ipc mode by default #35621

Merged

containers/create: use shareable ipc by default for older clients #38700

Closed

thaJeztah mentioned this pull request May 11, 2020

docker pull does download and extraction sequentially #37957

Closed

thaJeztah mentioned this pull request May 8, 2023

Do we need a more accurate validation of namespace mode #33325

Closed

Conversation

kolyshkin commented Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kolyshkin commented Jul 20, 2017

Uh oh!

thaJeztah 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!

rhatdan commented Jul 22, 2017

Uh oh!

kolyshkin commented Jul 26, 2017

Uh oh!

thaJeztah commented Jul 26, 2017

Uh oh!

thaJeztah commented Aug 6, 2017

Uh oh!

kolyshkin commented Aug 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justincormack commented Aug 12, 2017

Uh oh!

kolyshkin commented Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kolyshkin commented Aug 14, 2017

Uh oh!

thaJeztah commented Aug 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kolyshkin commented Jul 12, 2017 •

edited

Loading

kolyshkin commented Aug 14, 2017 •

edited

Loading