daemon: use shim v2 by default #41115

AkihiroSuda · 2020-06-16T05:11:22Z

- What I did

containerd has three shim binaries:

containerd-shim: implements shim API v1.
containerd-shim-runc-v1: implements shim API v2. Introduced in containerd v1.2.
containerd-shim-runc-v2: implements shim API v2. Introduced in containerd v1.3.

We have been using containerd-shim on cgroup v1 mode, containerd-shim-runc-v2 on cgroup v2 mode.

This commit changes the daemon to use containerd-shim-runc-v2 by default regardless to the cgroup mode.

To provide a workaround for potential regression issues, the daemon supports switching back to shim v1 mode via an env var MOBY_DISABLE_SHIM_V2=1.

The env var is only intended to be used as a workaround and is deprecated from its birth.
(So there is no daemon.json flag and no output in docker info)

The env var is discarded on cgroup v2 hosts, as shim v1 does not support cgroup v2.

Fix #41107

- How I did it
Updated *Daemon.useShimV2() to use shim v2 by default.

Requires containerd v1.3.0 or later. Using v1.3.5 or later is recommended. v1.3.0-v1.3.4 doesn't pass TestContainerStartOnDaemonRestart due to lack of containerd/containerd#4329 .

- How to verify it

Run some containers, and make sure containerd-shim-runc-v2 processes are launched.
Make sure it can be switched back to containerd-shim by launching dockerd with MOBY_DISABLE_SHIM_V2=1.

- Description for the changelog

daemon: use shim v2 by default. Requires containerd v1.3.0 or later. Using v1.3.5 or later is recommended.

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

AkihiroSuda · 2020-06-16T06:43:48Z

=== RUN   TestContainerStartOnDaemonRestart
=== PAUSE TestContainerStartOnDaemonRestart
=== CONT  TestContainerStartOnDaemonRestart
--- FAIL: TestContainerStartOnDaemonRestart (3.14s)
    daemon_linux_test.go:67: assertion failed: error is not nil: Error response from daemon: OCI runtime create failed: container with id exists: d7a842b6fb87a16b16d8b887237a653c89a4733bf50a51f5491d7914c348edff: unknown: failed to start test container

Fix: containerd/containerd#4327

thaJeztah

left some suggestions

daemon/daemon_unix.go

daemon/daemon.go

cpuguy83 · 2020-06-17T17:04:54Z

I think we need to revisit how we are handling this.

I would prefer to rely on the containerd config for this.
This might be able to be shoe-horned into the runtime concept that we already have.
Maybe add a field to the runtiume config we already have for shim?

Packagers can set the preferred runtime configuration in the containerd packaging, or rely on containerd's default.

AkihiroSuda · 2020-06-18T06:20:08Z

I would prefer to rely on the containerd config for this.

No, containerd daemon doesn't have any concept of "default runtime".

This might be able to be shoe-horned into the runtime concept that we already have.

Yes, but it is a separate topic.

cpuguy83 · 2020-06-18T16:45:06Z

Right, the default is determined by the client. But I can create and run a container without manually specifying a runtime. What I'm really getting at, is all the extra env var and function arguments to determine which shim to use does not seem right. We should be able to change the default and if someone wants something different they should be able to configure that in the runtime config.

cpuguy83 · 2020-06-18T18:42:01Z

Discussed on the maintainers call with @thaJeztah and @tonistiigi

I think we agreed it would be best to remove all the configuration we have regarding v1 vs v2 shim. Just use the v2 shim only, and then we can follow up by allowing the runtime config to specify a shim name.

containerd has three shim binaries: * containerd-shim: implements shim API v1. * containerd-shim-runc-v1: implements shim API v2. Introduced in containerd v1.2. * containerd-shim-runc-v2: implements shim API v2. Introduced in containerd v1.3. We have been using containerd-shim on cgroup v1 mode, containerd-shim-runc-v2 on cgroup v2 mode. This commit changes the daemon to use containerd-shim-runc-v2 by default regardless to the cgroup mode. To provide a workaround for potential regression issues, the daemon supports switching back to shim v1 mode via an env var `MOBY_DISABLE_SHIM_V2=1`. The env var is only intended to be used as a workaround and is deprecated from its birth. (So there is no daemon.json flag and no output in `docker info`) Fix moby#41107 Needs containerd v1.3.0 or later. v1.3.5 or later is recommended. Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

AkihiroSuda · 2020-06-30T04:17:52Z

Ready for review/merge

thaJeztah · 2020-06-30T10:30:36Z

I think we agreed it would be best to remove all the configuration we have regarding v1 vs v2 shim. Just use the v2 shim only, and then we can follow up by allowing the runtime config to specify a shim name.

Did you want that in this PR? Looks like the current implementation still adds the env-var

AkihiroSuda · 2020-06-30T10:34:01Z

Looks like the current implementation still adds the env-var

I interpreted remove all the configuration to just remove daemon.json fields

cpuguy83 · 2020-06-30T16:45:24Z

I interpreted remove all the configuration to just remove daemon.json fields

I was referring to the env var to disable, and passing bools around on what shim to use.
So, let's just use shim v2 then we can provide a way to configure that per runtime.

AkihiroSuda · 2020-06-30T21:36:36Z

Don't we need to provide a workaround for potential regression?

cpuguy83 · 2020-06-30T23:07:30Z

The work-around would be to allow configuring it at the runtime level.

AkihiroSuda · 2020-06-30T23:14:16Z

What is runtime level?

cpuguy83 · 2020-06-30T23:59:46Z

dockerd has a config for custom runtimes, and users can specify which runtime with docker run --runtime

AkihiroSuda · 2020-07-01T03:36:52Z

--runtime flags could be extended for specifying third party shim v2 runtimes such as io.containerd.kata.v2, but I don't think we should mix up the workaround flag for rolling back shim API to v1 there.

If we are confident that shim v2 is already matured enough, we can go ahead without any rollback knob.

cpuguy83 · 2020-07-01T16:55:53Z

@AkihiroSuda I don't think we're mixing anything up here.
We are switching to the v2 shim, if people want to roll back then they should specify the v1 shim.

thaJeztah · 2020-07-02T18:26:53Z

Discussing in the maintainers meeting with @cpuguy83 @tonistiigi.

the (already existing) booleans aren't great, and we should try to get rid of them
the env-vars could (potentially) be useful during RC's for 20.0x, but ideally should be gone in 20.0x-GA, and replaced by a runtime option

@cpuguy83 wants to have a look at a runtime option; if that works out, I'd say wait with merging until we have the alternative

cpuguy83 · 2020-07-07T21:11:55Z

#41182

After #41182, default becomes a one line change and fallback to v1 is possible by specifying --runtime=com.docker.runtime.runc.v1

The previous default runtime `io.containerd.runtime.v1.linux` is being deprecated (containerd/containerd#4365) `io.containerd.runc.v2` is available since containerd v1.3.0. Using v1.3.5 or later is recommended. v1.3.0-v1.3.4 doesn't pass `TestContainerStartOnDaemonRestart`. Fix moby#41107 Replace moby#41115 Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

AkihiroSuda · 2020-07-15T05:07:17Z

new PR: #41210

AkihiroSuda mentioned this pull request Jun 16, 2020

cgroup1: use shim v2 #41107

Closed

This was referenced Jun 16, 2020

test-integration: support cgroup2 #41065

Merged

shim v2 doesn't pass Moby test TestContainerStartOnDaemonRestart (because shim v2 uses hard-coded RuncRoot) containerd/containerd#4326

Closed

thaJeztah requested changes Jun 16, 2020

View reviewed changes

daemon/daemon_unix.go Outdated Show resolved Hide resolved

daemon/daemon_unix.go Outdated Show resolved Hide resolved

daemon/daemon_unix.go Outdated Show resolved Hide resolved

thaJeztah added area/runtime Runtime impact/changelog status/2-code-review labels Jun 16, 2020

thaJeztah added this to the 20.03.0 milestone Jun 16, 2020

cpuguy83 reviewed Jun 16, 2020

View reviewed changes

daemon/daemon_unix.go Outdated Show resolved Hide resolved

AkihiroSuda force-pushed the shim2 branch 2 times, most recently from 0ba160c to cbd6907 Compare June 17, 2020 03:41

thaJeztah reviewed Jun 17, 2020

View reviewed changes

daemon/daemon_unix.go Outdated Show resolved Hide resolved

AkihiroSuda marked this pull request as draft June 17, 2020 10:09

thaJeztah reviewed Jun 17, 2020

View reviewed changes

daemon/daemon.go Outdated Show resolved Hide resolved

AkihiroSuda mentioned this pull request Jun 24, 2020

Prepare release v1.3.5 containerd/containerd#4338

Merged

AkihiroSuda force-pushed the shim2 branch from cbd6907 to 2527dbf Compare June 26, 2020 03:01

AkihiroSuda force-pushed the shim2 branch from 2527dbf to 53a230e Compare June 30, 2020 04:17

AkihiroSuda marked this pull request as ready for review June 30, 2020 04:17

AkihiroSuda closed this Jul 8, 2020

AkihiroSuda mentioned this pull request Jul 15, 2020

cgroup v1: change the default runtime to io.containerd.runc.v2 #41210

Merged

zhangyoufu mentioned this pull request Dec 21, 2020

containerd-shim hangs on reboot/shutdown (live restore + runc v2 runtime) #41831

Closed

daemon: use shim v2 by default #41115

daemon: use shim v2 by default #41115

Uh oh!

Conversation

AkihiroSuda commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpuguy83 commented Jun 17, 2020

Uh oh!

AkihiroSuda commented Jun 18, 2020

Uh oh!

cpuguy83 commented Jun 18, 2020 via email • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 commented Jun 18, 2020

Uh oh!

AkihiroSuda commented Jun 30, 2020

Uh oh!

thaJeztah commented Jun 30, 2020

Uh oh!

AkihiroSuda commented Jun 30, 2020

Uh oh!

cpuguy83 commented Jun 30, 2020

Uh oh!

AkihiroSuda commented Jun 30, 2020

Uh oh!

cpuguy83 commented Jun 30, 2020

Uh oh!

AkihiroSuda commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 commented Jun 30, 2020

Uh oh!

AkihiroSuda commented Jul 1, 2020

Uh oh!

cpuguy83 commented Jul 1, 2020

Uh oh!

thaJeztah commented Jul 2, 2020

Uh oh!

cpuguy83 commented Jul 7, 2020

Uh oh!

AkihiroSuda commented Jul 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AkihiroSuda commented Jun 16, 2020 •

edited

Loading

AkihiroSuda commented Jun 16, 2020 •

edited

Loading

cpuguy83 commented Jun 18, 2020 via email •

edited by thaJeztah

Loading

AkihiroSuda commented Jun 30, 2020 •

edited

Loading