containers/create: use shareable ipc by default for older clients by kolyshkin · Pull Request #38700 · moby/moby

kolyshkin · 2019-02-09T01:42:13Z

Older (API < 1.32) clients are not aware of new IPC modes
(shareable, private, and none, introduced in commit 7120976 (PR #34087)
and expect a new container to be created with IPC mode that
is now known as shareable.

Since the above commit, daemon default can be set to private,
essentially locking out such older clients from being able to
share IPC between containers (i.e. using --ipc container:NNN).

So, with this patch, a client which does not set any value
for IPC mode, defaults to:

for API < 1.32: shareable;
for API >= 1.32: daemon's default-ipc-mode

AkihiroSuda · 2019-02-10T23:08:36Z

CLI test failing

02:44:37 ----------------------------------------------------------------------
02:44:37 FAIL: docker_cli_daemon_test.go:2973: DockerDaemonSuite.TestDaemonRestartIpcMode
02:44:37 
02:44:37 [d6050514c6c8f] waiting for daemon to start
02:44:37 [d6050514c6c8f] daemon started
02:44:37 
02:44:37 docker_cli_daemon_test.go:2990:
02:44:37     c.Assert(m, checker.Equals, "private")
02:44:37 ... obtained string = "shareable"
02:44:37 ... expected string = "private"
02:44:37 
02:44:37 [d6050514c6c8f] exiting daemon

cpuguy83 · 2019-02-11T00:39:03Z

Let's take a step back here. I don't see why we should modify the client, are we doing something similar for other options?

thaJeztah

LGTM

cpuguy83 · 2019-02-14T19:19:23Z

19:03:28 FAIL: docker_cli_daemon_test.go:2973: DockerDaemonSuite.TestDaemonRestartIpcMode
19:03:28 
19:03:28 [d77aa0cf0b918] waiting for daemon to start
19:03:28 [d77aa0cf0b918] daemon started
19:03:28 
19:03:28 docker_cli_daemon_test.go:2990:
19:03:28     c.Assert(m, checker.Equals, "private")
19:03:28 ... obtained string = "shareable"
19:03:28 ... expected string = "private"
19:03:28 
19:03:28 [d77aa0cf0b918] exiting daemon
19:03:29

kolyshkin · 2019-02-14T21:55:02Z

19:03:28 FAIL: docker_cli_daemon_test.go:2973: DockerDaemonSuite.TestDaemonRestartIpcMode

This test is failing as the API used is < 1.32 (basically confirming the patch works). Fix is coming :)

codecov · 2019-02-19T03:17:22Z

Codecov Report

❗ No coverage uploaded for pull request base (master@54dddad). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #38700   +/-   ##
=========================================
  Coverage          ?   36.55%           
=========================================
  Files             ?      610           
  Lines             ?    45394           
  Branches          ?        0           
=========================================
  Hits              ?    16593           
  Misses            ?    26507           
  Partials          ?     2294

kolyshkin · 2019-02-19T03:21:36Z

OK here's the test fix; please see individual commits description.

kolyshkin · 2019-02-19T17:57:39Z

exp ci failure in

01:05:06.642 FAIL: docker_api_swarm_test.go:297: DockerSwarmSuite.TestAPISwarmLeaderElection

is a known flaky test

janky ci failure in

01:19:34.071 FAIL: docker_cli_daemon_test.go:118: DockerDaemonSuite.TestDaemonRestartUnlessStopped

is a recent flaky test (#38720)

Restarting both

kolyshkin · 2019-02-19T17:58:10Z

(reserved for Derek commands)

thaJeztah · 2019-02-20T13:17:17Z

Was the last commit only moving the test to the new test-suite, or were changes made as well related to this PR?

Perhaps it's better to move the general test-changes to a separate PR

kolyshkin · 2019-02-20T22:13:31Z

Was the last commit only moving the test to the new test-suite, or were changes made as well related to this PR?

TL;DR: this change is needed here and can't be separated out (unless we merge it first).

Perhaps I was too brief in my commit description:

Move the test case from integration-cli to integration.
This makes it use the current API version.

The test logic itself has not changed, except these two things:

the new test sets default-ipc-mode via command line rather than via daemon.json (it doesn't matter);
the new test uses current API version.

The problem this move solves is, old integration-cli tests use old API version, and so the following code stopped working (see #38700 (comment)) after the changes in this PR:

start a daemon with "private" default-ipc-mode;
create a container without explicitly specifying its ipc mode;
check its mode is shareable.

When old API is used, check in (3) fails.

@thaJeztah I can split the last commit to a separate PR if you want

thaJeztah · 2019-02-20T22:59:37Z

My thinking was; do the general modernization of TestDaemonRestartIpcMode in a separate PR, and (in this PR) add the new test-case to the test

Older (API < 1.32) clients are not aware of new IPC modes (`shareable`, `private`, and `none`, introduced in commit 7120976) and expect a new container to be created with IPC mode that is now known as `shareable`. Since the above commit, daemon default can be set to `private`, essentially locking out such older clients from being able to share IPC between containers (i.e. using `--ipc container:NNN`). So, with this patch, a client which does not set any value for IPC mode, defaults to: * for API < 1.32: 'shareable'; * for API >= 1.32: daemon's default-ipc-mode An integration test is added to check this works as intended. Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin · 2019-03-08T18:25:46Z

do the general modernization of TestDaemonRestartIpcMode in a separate PR, and (in this PR) add the new test-case to the test

Done, test move separated out to #38843, so this one is simple to review. The only catch is #38843 need to be merged first for this PR to pass CI.

kolyshkin · 2019-03-09T01:38:56Z

Closing this in favor of last version of #35621

kolyshkin mentioned this pull request Feb 9, 2019

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

Merged

GordonTheTurtle added the status/0-triage label Feb 9, 2019

AkihiroSuda approved these changes Feb 10, 2019

View reviewed changes

thaJeztah approved these changes Feb 14, 2019

View reviewed changes

thaJeztah added status/2-code-review and removed status/0-triage labels Feb 14, 2019

kolyshkin force-pushed the fix-older-clients-wrt-ipc branch from 41fc1be to b472526 Compare February 19, 2019 03:17

kolyshkin force-pushed the fix-older-clients-wrt-ipc branch from b472526 to 3237c17 Compare February 19, 2019 03:20

derek bot added the rebuild/experimental label Feb 19, 2019

GordonTheTurtle removed the rebuild/experimental label Feb 19, 2019

derek bot added the rebuild/janky label Feb 19, 2019

GordonTheTurtle removed the rebuild/janky label Feb 19, 2019

GordonTheTurtle assigned mlaventure Feb 24, 2019

kolyshkin mentioned this pull request Mar 8, 2019

TestDaemonRestartIpcMode: move to integration #38843

Merged

kolyshkin force-pushed the fix-older-clients-wrt-ipc branch from 3237c17 to 9f939ed Compare March 8, 2019 18:24

kolyshkin closed this Mar 9, 2019

thaJeztah mentioned this pull request Mar 15, 2019

Start containers in their own cgroup namespaces #38377

Merged

Conversation

kolyshkin commented Feb 9, 2019

Uh oh!

AkihiroSuda commented Feb 10, 2019

Uh oh!

cpuguy83 commented Feb 11, 2019

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Feb 14, 2019

Uh oh!

kolyshkin commented Feb 14, 2019

Uh oh!

codecov bot commented Feb 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kolyshkin commented Feb 19, 2019

Uh oh!

kolyshkin commented Feb 19, 2019

Uh oh!

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

Uh oh!

Uh oh!

thaJeztah commented Feb 20, 2019

Uh oh!

kolyshkin commented Feb 20, 2019

Uh oh!

thaJeztah commented Feb 20, 2019

Uh oh!

kolyshkin commented Mar 8, 2019

Uh oh!

kolyshkin commented Mar 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Feb 19, 2019 •

edited

Loading

kolyshkin commented Feb 19, 2019 •

edited

Loading