daemon: use 'private' ipc mode by default by kolyshkin · Pull Request #35621 · moby/moby

kolyshkin · 2017-11-28T01:11:37Z

(this is a follow-up to #34087)

This changes the default ipc mode of daemon/engine to be private, meaning the containers will not have their /dev/shm filesystem mounted on the host and then bind-mounted from host to a container by default.

The benefits of doing this are:

No leaked mounts. This eliminates a possibility to leak mounts into other namespaces (and therefore unfortunate errors like "Unable to remove filesystem for $ID: remove /var/lib/docker/containers/$ID/shm: device or resource busy").
Working checkpoint/restore. Make docker checkpoint not lose the contents of /dev/shm, but save it to the dump, and be restored back upon docker start --checkpoint.
Better security. Currently any container is opened to share its /dev/shm with any other container.

The downside is, obviously, this change will break the following usage scenario:

 $ docker run -d --name donor busybox top
 $ docker run --rm -it --ipc container:donor busybox sh
 Error response from daemon: linux spec namespaces: can't join IPC
 of  container <ID>: non-shareable IPC (hint: use --ipc shareable
 for the donor container)

The solution, as hinted by the (amended) error message, is to explicitly enable donor sharing by using --ipc shareable option to docker run or docker create:

 $ docker run -d --name donor --ipc shareable busybox top

Compatibility notes

This only applies to containers created after this change. Existing containers are not affected and will work fine as their ipc mode is stored in HostConfig.
Old (bad, but backward-compatible) behavior (i.e. "shareable" containers by default) can be enabled by either using --default-ipc-mode shareable daemon command line option, or by adding a "default-ipc-mode": shareable" line in docker.json configuration file.
If an older client (API < 1.40) is used, a "shareable" container is created. A test to check that is added.
An item in API version history explaining the new default, and a way to change it back, is added.
As a side effect, this PR fixes bug originally addressed in containers/create: use shareable ipc by default for older clients #38700 (that is obsoleted by this one).

- A picture of a cute animal (not mandatory but encouraged)

First part of this saga: Implement private and shareable ipc modes #34087
Documentation updates: Introduce/document new IPC modes docker/cli#367
Test case fixes that were originally part of this PR: Misc test improvements #35840
Kubernetes patch to not rely on IpcMode:shared being the default: Use shareable IPC for sandbox container kubernetes/kubernetes#70826
Some more test case updates: https://github.com/moby/moby/pull/38843/commits

A tiger cub from the 1984 USSR cartoon https://www.youtube.com/watch?v=0DnjD7w-2FI

kolyshkin · 2017-11-30T21:29:36Z

Need to figure out why TestCleanupMountsAfterDaemonAndContainerKill and TestCleanupMountsAfterDaemonCrash fail

kolyshkin · 2017-12-02T00:57:50Z

OK, I got it now why these two tests are failing!

The reason is, all the mounts are now in the private namespace, except for one used for container's /dev/shm. With this patch, this last mount is gone so test is not finding anything.

The solution is to check whether there are any container mounts visible from the host system after container start -- if not, skip the test.

kolyshkin · 2017-12-04T21:25:54Z

~~This one is now ready for the prime time~~

kolyshkin · 2017-12-05T19:31:47Z

Powerpc test case failure is unrelated, nevertheless I figured it out and fixed the test case.

kolyshkin · 2017-12-05T22:36:01Z

The following failure is most probably unrelated as I am seeing it in other PRs. I'll take a look nevertheless.

12:29:56 ----------------------------------------------------------------------
12:29:56 FAIL: check_test.go:366: DockerSwarmSuite.TearDownTest
12:29:56 
12:29:56 check_test.go:371:
12:29:56     d.Stop(c)
12:29:56 daemon/daemon.go:395:
12:29:56     t.Fatalf("Error while stopping the daemon %s : %v", d.id, err)
12:29:56 ... Error: Error while stopping the daemon d704ce7bc07d4 : exit status 130
12:29:56 
12:29:56 
12:29:56 ----------------------------------------------------------------------
12:29:56 PANIC: docker_api_swarm_service_test.go:201: DockerSwarmSuite.TestAPISwarmServicesUpdateStartFirst
12:29:56 
12:29:56 [d704ce7bc07d4] waiting for daemon to start
12:29:56 [d704ce7bc07d4] daemon started
12:29:56 
12:29:56 [d704ce7bc07d4] daemon started
12:29:56 Attempt #2: daemon is still running with pid 19775
12:29:56 Attempt #3: daemon is still running with pid 19775
12:29:56 Attempt #4: daemon is still running with pid 19775
12:29:56 [d704ce7bc07d4] exiting daemon
12:29:56 ... Panic: Fixture has panicked (see related PANIC)
12:29:56 
12:29:56 ----------------------------------------------------------------------

cpuguy83 · 2017-12-06T14:53:57Z

I worry about changing the default.
If the goal is to eliminate issues with EBUSY, shouldn't we dig deeper into the cause of these leaks? This isn't really fixing mount leaks, just making it less likely in the default configuration.

daemon/container_operations_unix.go

cpuguy83 · 2017-12-07T19:41:15Z

We discussed this in the maintainers meeting and it came up "well is Kubernetes depending on shared IPC" ... I believe it is and this will break them... and even if not it's a good example of how this could break people.

note: I mentioned on the call maybe they are using exec but that just doesn't make any sense because they couldn't start containers with different images in the pod, which is required for the pod scenario.

cpuguy83 · 2019-02-09T01:50:44Z

@kolyshkin This wasn't confusing at all. Just that it's wrong to break existing users, which is what this does.

kolyshkin · 2019-02-14T19:30:27Z

We disscused this with @thaJeztah and @cpuguy83 on maintainers meeting today. What needs to be added is:

a changelog entry explaining the new default;
a fix for clients API < 1.41: if new container ipc mode is empty AND compiled-in daemon default is not changed, use "shareable".

thaJeztah · 2019-02-14T20:05:58Z

For the API version; upcoming release will be API v1.40, so if it gets merged before that, it's gonna be API < 1.40

kolyshkin · 2019-02-19T03:41:15Z

I'll revisit this one once #38700 is merged.

kolyshkin · 2019-03-09T00:36:34Z

OK I just came to realization that #38700 needs to be merged into this one, for the reasons that I am too lazy to describe right now. Also that @cpuguy83 and @thaJeztah was right from the start asking to forcefully set IpcMode: shareable for older clients.

I have also added an item into API version history explaining the new default, and a way to change it back.

I think this PR is ready for prime time, aside from the fact that it currently includes #38843; I will rebase once it's merged.

kolyshkin · 2019-03-09T19:51:23Z

01:16:03.313 FAIL: docker_api_swarm_test.go:297: DockerSwarmSuite.TestAPISwarmLeaderElection

is a well-known flaky test (#32673), rebuilding

kolyshkin · 2019-03-09T19:51:52Z

Derek add label: rebuild/janky

There are two if statements checking for exactly same conditions: > if hostConfig != nil && versions.LessThan(version, "1.40") Merge these. Signed-off-by: Kir Kolyshkin <[email protected]>

This changes the default ipc mode of daemon/engine to be private, meaning the containers will not have their /dev/shm bind-mounted from the host by default. The benefits of doing this are: 1. No leaked mounts. Eliminate a possibility to leak mounts into other namespaces (and therefore unfortunate errors like "Unable to remove filesystem for <ID>: remove /var/lib/docker/containers/<ID>/shm: device or resource busy"). 2. Working checkpoint/restore. Make `docker checkpoint` not lose the contents of `/dev/shm`, but save it to the dump, and be restored back upon `docker start --checkpoint` (currently it is lost -- while CRIU handles tmpfs mounts, the "shareable" mount is seen as external to container, and thus rightfully ignored). 3. Better security. Currently any container is opened to share its /dev/shm with any other container. Obviously, this change will break the following usage scenario: $ docker run -d --name donor busybox top $ docker run --rm -it --ipc container:donor busybox sh Error response from daemon: linux spec namespaces: can't join IPC of container <ID>: non-shareable IPC (hint: use IpcMode:shareable for the donor container) The soution, as hinted by the (amended) error message, is to explicitly enable donor sharing by using --ipc shareable: $ docker run -d --name donor --ipc shareable busybox top Compatibility notes: 1. This only applies to containers created _after_ this change. Existing containers are not affected and will work fine as their ipc mode is stored in HostConfig. 2. Old backward compatible behavior ("shareable" containers by default) can be enabled by either using `--default-ipc-mode shareable` daemon command line option, or by adding a `"default-ipc-mode": "shareable"` line in `/etc/docker/daemon.json` configuration file. 3. If an older client (API < 1.40) is used, a "shareable" container is created. A test to check that is added. Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin · 2019-03-10T03:00:34Z

Rebased on top of merged #38843

cpuguy83

LGTM

thaJeztah

LGTM

…>=19.03.0 (see moby/moby#35621)

GordonTheTurtle added the status/0-triage label Nov 28, 2017

kolyshkin force-pushed the ipc-private branch from 79e72cb to 3ed3cbc Compare November 30, 2017 19:45

kolyshkin requested review from dnephin and vdemeester as code owners November 30, 2017 19:45

kolyshkin force-pushed the ipc-private branch from 3ed3cbc to d2a914c Compare November 30, 2017 21:23

kolyshkin changed the title ~~daemon: use 'private' ipc mode by default~~ [WIP] daemon: use 'private' ipc mode by default Nov 30, 2017

kolyshkin force-pushed the ipc-private branch 4 times, most recently from cbe64bf to 7870484 Compare December 1, 2017 22:41

kolyshkin force-pushed the ipc-private branch from 7870484 to ab7aeb2 Compare December 2, 2017 19:08

kolyshkin changed the title ~~[WIP] daemon: use 'private' ipc mode by default~~ daemon: use 'private' ipc mode by default Dec 4, 2017

kolyshkin force-pushed the ipc-private branch 2 times, most recently from cd3f5d2 to 737176d Compare December 4, 2017 19:56

kolyshkin force-pushed the ipc-private branch 3 times, most recently from 07cc25c to 09e427e Compare December 5, 2017 05:39

kolyshkin force-pushed the ipc-private branch 2 times, most recently from 91a7089 to 0a1c4c0 Compare December 6, 2017 07:00

kolyshkin requested a review from tianon as a code owner December 6, 2017 07:13

kolyshkin force-pushed the ipc-private branch from 837dd0c to aeb04a3 Compare December 6, 2017 23:36

thaJeztah reviewed Dec 7, 2017

View reviewed changes

daemon/container_operations_unix.go Outdated Show resolved Hide resolved

thaJeztah added the impact/changelog label Dec 7, 2017

kolyshkin force-pushed the ipc-private branch from f3b81b4 to bb7f72e Compare March 9, 2019 00:31

kolyshkin force-pushed the ipc-private branch 2 times, most recently from c8ae56b to 3e2748a Compare March 9, 2019 01:24

kolyshkin mentioned this pull request Mar 9, 2019

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

Closed

derek bot added the rebuild/janky label Mar 9, 2019

kolyshkin added 2 commits March 9, 2019 18:57

postContainersCreate: minor nitpick

ce7528e

There are two if statements checking for exactly same conditions: > if hostConfig != nil && versions.LessThan(version, "1.40") Merge these. Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin force-pushed the ipc-private branch from 3e2748a to 596ca14 Compare March 10, 2019 02:58

thaJeztah added status/2-code-review and removed status/0-triage status/failing-ci Indicates that the PR in its current state fails the test suite labels Mar 11, 2019

GordonTheTurtle removed the rebuild/janky label Mar 11, 2019

thaJeztah added the rebuild/* label Mar 11, 2019

GordonTheTurtle removed the rebuild/* label Mar 11, 2019

cpuguy83 approved these changes Mar 14, 2019

View reviewed changes

thaJeztah approved these changes Mar 14, 2019

View reviewed changes

thaJeztah merged commit ca0b64e into moby:master Mar 14, 2019

thaJeztah mentioned this pull request Mar 15, 2019

Start containers in their own cgroup namespaces #38377

Merged

doubliez added a commit to doubliez/docker-slaves-plugin that referenced this pull request Sep 3, 2019

Use --ipc=shareable on remoting container to make it work with Docker…

7f26105

…>=19.03.0 (see moby/moby#35621)

doubliez mentioned this pull request Sep 3, 2019

Support latest Docker Engine and Jenkins remoting image jenkinsci/docker-slaves-plugin#28

Open

rycus86 mentioned this pull request Nov 5, 2019

can't join IPC of container error rycus86/podlike#5

Closed

margamanterola mentioned this pull request Apr 14, 2020

Kola test google.kubernetes.basic.docker.v1.3.4_coreos.0 fails with new Alpha release flatcar/Flatcar#79

Closed

Conversation

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

Uh oh!

Compatibility notes

See also

Uh oh!

kolyshkin commented Nov 30, 2017

Uh oh!

kolyshkin commented Dec 2, 2017

Uh oh!

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

Uh oh!

Uh oh!

kolyshkin commented Dec 5, 2017

Uh oh!

kolyshkin commented Dec 5, 2017

Uh oh!

cpuguy83 commented Dec 6, 2017

Uh oh!

Uh oh!

cpuguy83 commented Dec 7, 2017

Uh oh!

cpuguy83 commented Feb 9, 2019

Uh oh!

kolyshkin commented Feb 14, 2019

Uh oh!

thaJeztah commented Feb 14, 2019

Uh oh!

kolyshkin commented Feb 19, 2019

Uh oh!

kolyshkin commented Mar 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kolyshkin commented Mar 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kolyshkin commented Mar 9, 2019

Uh oh!

kolyshkin commented Mar 10, 2019

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

kolyshkin commented Nov 28, 2017 •

edited

Loading

kolyshkin commented Dec 4, 2017 •

edited

Loading

kolyshkin commented Mar 9, 2019 •

edited

Loading

kolyshkin commented Mar 9, 2019 •

edited

Loading