Runtime configure shim by cpuguy83 · Pull Request #41182 · moby/moby

cpuguy83 · 2020-07-07T21:05:25Z

- What I did

Move all shim config into "runtime" type, and adds 2 new built-in runtimes to allow explicitly switching from runc+shimv1 and runc+shimv2

- How I did it

Add a ShimConfig type to types.Runtime which stores the shim config.
The daemon then uses that instead of having things spread between daemon and libcontainerd, some bool passing, etc.

This option is only exposed internally, i.e. it cannot be set from daemon.json.
This is due to issues being able to support unknown shims which requires knowing types to pass to those shims (e.g. does it need a runc v2 shim config, a runc v1 shim config, or some other entirely different shim).

- How to verify it

Set --default-runtime=com.docker.runtime.runc.v2, run a container (and/or a plugin), check the process tree and see the container was created with /usr/local/bin/containerd-shim-runc-v2

- Description for the changelog

Add support new built-in runtime for using the containerd v2 shim by using the runtime com.docker.runtime.runc.v2

daemon/start_unix.go

cpuguy83 · 2020-07-07T21:08:45Z

plugin/executor/containerd/containerd.go

Note here I did not carry through with this custom runtime root path either since it doesn't really seem necessary.

AkihiroSuda · 2020-07-08T00:00:28Z

daemon/runtime_unix.go

Why not just use containerd runtime name?

I did this because we provide our own configuration... but maybe it's not needed.

FYI this code moved in my last push, but is currently unchanged.

Let's just use containerd names to avoid confusion.

AkihiroSuda · 2020-07-08T00:55:58Z

daemon/runtime_unix.go

You mean: why a script? I recall there was a discussion about that in #37978 (comment)

This is old code, I just moved it out of a huge file to group it with other runtime code.

It's here because containerd does not accept extra arguments passed to the oci runtime.

Unrelated to this PR but we should do a validation of the permissions of this path every time it is used. Otherwise, this looks quite scary.

daemon/runtime_unix.go

daemon/config/config.go

daemon/config/config_common_unix.go

daemon/runtime_unix.go

thaJeztah · 2020-07-08T09:18:17Z

daemon/config/config.go

Should we rename to Default ? (and deprecate the old one?)

The variable name?

libcontainerd/remote/client.go

thaJeztah · 2020-07-09T18:11:57Z

@AkihiroSuda @tonistiigi PTAL

cpuguy83 · 2020-07-09T18:15:56Z

Updated this to add deprecation warnings for the v1 runtime.

Re: naming the runtimes

The containerd names are confusing. I'm not sure if it's good to use them or not. Just thinking about it.

Signed-off-by: Brian Goff <[email protected]>

cpuguy83 · 2020-07-11T00:02:35Z

Tests are all green now, at least.

fuweid

LGTM(nb)

fuweid · 2020-07-13T15:36:39Z

The containerd names are confusing. I'm not sure if it's good to use them or not. Just thinking about it.

Legacy shim doesn't have version. Both runc.v1 and runc.v2 are shimv2 lol

AkihiroSuda · 2020-07-13T16:03:47Z

My concern is that runc.v1 with the com.docker. prefix is shim API v1...

cpuguy83 · 2020-07-13T17:16:50Z

Yeah, perhaps I should just use the containerd shim names as the runtime name.
This is only included as a fallback for edge cases anyway, and we can document that.

In dockerd we already have a concept of a "runtime", which specifies the OCI runtime to use (e.g. runc). This PR extends that config to add containerd shim configuration. This option is only exposed within the daemon itself (cannot be configured in daemon.json). This is due to issues in supporting unknown shims which will require more design work. What this change allows us to do is keep all the runtime config in one place. So the default "runc" runtime will just have it's already existing shim config codified within the runtime config alone. I've also added 2 more "stock" runtimes which are basically runc+shimv1 and runc+shimv2. These new runtime configurations are: - io.containerd.runtime.v1.linux - runc + v1 shim using the V1 shim API - io.containerd.runc.v2 - runc + shim v2 These names coincide with the actual names of the containerd shims. This allows the user to essentially control what shim is going to be used by either specifying these as a `--runtime` on container create or by setting `--default-runtime` on the daemon. For custom/user-specified runtimes, the default shim config (currently shim v1) is used. Signed-off-by: Brian Goff <[email protected]>

cpuguy83 · 2020-07-13T21:20:51Z

I've updated this to use the shim names as our runtime name.

thaJeztah

LGTM

@tonistiigi PTAL

tonistiigi

SGTM

thaJeztah · 2020-12-23T13:48:09Z

Looks like this may have introduced a regression (panic when configuring custom runtime); docker/for-linux#1169

thaJeztah · 2021-01-07T20:00:14Z

daemon/daemon_unix.go

-	if err != nil {
-		return errors.Wrap(err, "failed to get temp dir to generate runtime scripts")
+	if hostConfig.Runtime == config.LinuxV1RuntimeName || (hostConfig.Runtime == "" && daemon.configStore.DefaultRuntime == config.LinuxV1RuntimeName) {
+		warnings = append(warnings, fmt.Sprintf("Configured runtime %q is deprecated and will be removed in the next release.", config.LinuxV1RuntimeName))


We need to add a line to the deprecation log; https://github.com/docker/cli/blob/master/docs/deprecated.md

cpuguy83 requested review from AkihiroSuda and tonistiigi July 7, 2020 21:05

cpuguy83 commented Jul 7, 2020

View reviewed changes

daemon/start_unix.go Outdated Show resolved Hide resolved

cpuguy83 commented Jul 7, 2020

View reviewed changes

cpuguy83 mentioned this pull request Jul 7, 2020

daemon: use shim v2 by default #41115

Closed

cpuguy83 added this to the 20.03.0 milestone Jul 7, 2020

cpuguy83 added the impact/changelog label Jul 7, 2020

AkihiroSuda requested changes Jul 8, 2020

View reviewed changes

cpuguy83 force-pushed the runtime_configure_shim branch from 1f0ef3d to 32065d5 Compare July 8, 2020 00:44

AkihiroSuda requested changes Jul 8, 2020

View reviewed changes

daemon/runtime_unix.go Outdated Show resolved Hide resolved

thaJeztah added area/daemon Core Engine area/runtime Runtime docs/revisit status/2-code-review labels Jul 8, 2020

thaJeztah reviewed Jul 8, 2020

View reviewed changes

daemon/config/config.go Outdated Show resolved Hide resolved

thaJeztah reviewed Jul 8, 2020

View reviewed changes

daemon/config/config_common_unix.go Outdated Show resolved Hide resolved

thaJeztah requested changes Jul 8, 2020

View reviewed changes

cpuguy83 force-pushed the runtime_configure_shim branch 2 times, most recently from 7354d8c to 794c66d Compare July 9, 2020 17:43

cpuguy83 force-pushed the runtime_configure_shim branch from 794c66d to 481ec2b Compare July 9, 2020 18:14

cpuguy83 force-pushed the runtime_configure_shim branch from 481ec2b to 99ea6b9 Compare July 9, 2020 19:54

Fix lint error on sprintf call for runtime string

6fd94aa

Signed-off-by: Brian Goff <[email protected]>

cpuguy83 force-pushed the runtime_configure_shim branch 3 times, most recently from d9d5945 to dfb02f4 Compare July 10, 2020 22:26

AkihiroSuda approved these changes Jul 11, 2020

View reviewed changes

fuweid approved these changes Jul 13, 2020

View reviewed changes

cpuguy83 force-pushed the runtime_configure_shim branch from dfb02f4 to f63f73a Compare July 13, 2020 21:20

AkihiroSuda approved these changes Jul 14, 2020

View reviewed changes

thaJeztah approved these changes Jul 14, 2020

View reviewed changes

tonistiigi reviewed Jul 14, 2020

View reviewed changes

cpuguy83 merged commit 61b73ee into moby:master Jul 14, 2020

cpuguy83 deleted the runtime_configure_shim branch July 14, 2020 21:16

thaJeztah mentioned this pull request Jul 15, 2020

Remove unused useShimV2() #41214

Merged

thaJeztah mentioned this pull request Dec 23, 2020

[v20.10.1] dockerd panics when configuring default runtime docker/for-linux#1169

Closed

3 tasks

thaJeztah added the impact/deprecation label Jan 7, 2021

thaJeztah reviewed Jan 7, 2021

View reviewed changes

thaJeztah mentioned this pull request May 4, 2021

Docker Daemon is passing the incorrect root directory for storage of container state. #42345

Open

thaJeztah mentioned this pull request Jun 5, 2022

daemon: remove support for deprecated io.containerd.runtime.v1.linux #43695

Merged

Conversation

cpuguy83 commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

thaJeztah commented Jul 9, 2020

Uh oh!

cpuguy83 commented Jul 9, 2020

Uh oh!

cpuguy83 commented Jul 11, 2020

Uh oh!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

fuweid commented Jul 13, 2020

Uh oh!

AkihiroSuda commented Jul 13, 2020

Uh oh!

cpuguy83 commented Jul 13, 2020

Uh oh!

cpuguy83 commented Jul 13, 2020

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

tonistiigi left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Dec 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cpuguy83 commented Jul 7, 2020 •

edited

Loading