daemon: let shim binary of runtime configurable by liubin · Pull Request #43736 · moby/moby

liubin · 2022-06-22T08:33:51Z

- What I did
Let users configure shim's binary for additional runtimes

- How I did it
Set json tag of the golang struct definition.

- How to verify it

Edit daemon.json.

{
  "default-runtime": "runc",
  "runtimes": {
    "kata": {
      "shim": {
        "binary": " io.containerd.kata.v2"
      }
    }
  }
}

and then run docker run --net none --runtime kata -it alpine:3.15 sh.
Note: you need Kata Containers shim has been installed.

- Description for the changelog

daemon: let shim binary of runtime configurable

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

liubin · 2022-06-22T08:34:03Z

Additional note to reviewers:

If this idea is acceptable, I will go on the next steps, including process add-runtime and documentation.

First of all, I would like to hear your thoughts on this approach.

cpuguy83 · 2022-06-22T22:56:02Z

This seems mostly ok.
Shims generally take a set of options that are specific to the shim, is it ok for kata to not receive any such options?

liubin · 2022-06-23T02:58:31Z

Thank you @cpuguy83, Kata Containers shim has its configuration file, users can use this file or annotations to set shim/runtime options.

AkihiroSuda · 2022-06-23T11:34:15Z

I feel this should be called "name" rather than a "binary" as the value is not a binary file path

liubin · 2022-06-24T07:03:01Z

@AkihiroSuda though the name of binary is somewhat ambiguous, it really ALSO supports passing the absolute path of the shim binary:

https://github.com/containerd/containerd/blob/10c12954828e7c7c9b6e0ea9b0c02b01407d3ae1/runtime/v2/manager.go#L237-L245

thaJeztah · 2022-06-24T23:33:04Z

api/types/types.go

-	// This is exposed here only for internal use
-	// It is not currently supported to specify custom shim configs
-	Shim *ShimConfig `json:"-"`
+	Shim *ShimConfig `json:"shim"`


This type is also used in the Info type, which is used for the /info response;

moby/api/types/types.go

Line 302 in 9ccedde

Runtimes map[string]Runtime

So;

some changes are needed in the Runtime definition in the Swagger file

and an entry to be added to the API version-history.md

Fixed, please take a look again.

thaJeztah · 2022-06-24T23:35:51Z

api/types/types.go

@@ -657,15 +657,16 @@ type Runtime struct {
 	Path string   `json:"path"`


Curious; your example shows a runtime configuration that ONLY has shim.binary set (but no path); is that a valid configuration?

If so (and the path is optional), we should consider adding an omitempty to this field.

If that means that at least path and/or shim.binary must be specified, then we should probably have a validation for that (error if none are set?).

Thank you for the review, @thaJeztah
The path is used for configuring OCI runtime for io.containerd.runc.v2 shim, for other shims, they may or may not need the path property, so I added the omitempty attribute.

thaJeztah · 2022-06-24T23:38:01Z

As these fields are now added to the API, and were previously "internal" (so not part of the JSON output), I guess we should also look what we want the API response to look like. Currently (in this PR, the response will be something like;

{
  "path": "crun",
  "shim": {
    "binary": "io.containerd.runc.v2"
  }
}

Question is; do we want the shim to be a struct in that response, or do we prefer a "flat" output? If we prefer "flat", we could embed the ShimConfig directly in Runtime; perhaps rename binary to shim (which could somewhat address @AkihiroSuda's comment about binary vs name). Perhaps add an omitempty for the field as well?

type Runtime struct {
	Path string   `json:"path"`
	Args []string `json:"runtimeArgs,omitempty"`
	*ShimConfig
}

// ShimConfig is used by runtime to configure containerd shims
type ShimConfig struct {
	Binary string `json:"shim,omitempty"`

	// This is exposed here only for internal use
	// It is not currently supported to specify custom shim options
	Opts interface{} `json:"-"`
}

Which would look like;

{
  "path": "runc",
  "shim": "io.containerd.runc.v2"
}

^^^ input/suggestions welcome on the above; wdyt @AkihiroSuda @cpuguy83 ?

Users can add additional runtimes by adding configurations to daemon.json, but it can only be used for specific OCI runtimes under io.containerd.runc.v2. By exposing shim and its binary property, users can set containerd shim v2 runtime directly: { "default-runtime": "runc", "runtimes": { "kata": { "shim": { "binary": " io.containerd.kata.v2" } } } } Now users can use `--runtime kata` to start containers run inside Kata Containers. Signed-off-by: liubin <liubin0329@gmail.com>

liubin · 2022-06-27T08:14:50Z

I updated the code, and now for such a daemon.json:

{
  "runtimes": {
    "runc-1": {},
    "kata": {
      "shim": {
        "binary": "io.containerd.kata.v2"
      }
    },
    "runx": {
      "path": "runc",
      "runtimeArgs": [
        "--debug"
      ]
    }
  }
}

The /Info API of the daemon will return:

{
  "io.containerd.runc.v2": {
    "path": "runc",
    "shim": {
      "binary": "io.containerd.runc.v2"
    }
  },
  "kata": {
    "shim": {
      "binary": "io.containerd.kata.v2"
    }
  },
  "runc": {
    "path": "runc",
    "shim": {
      "binary": "io.containerd.runc.v2"
    }
  },
  "runc-1": {},
  "runx": {
    "path": "runc",
    "runtimeArgs": [
      "--debug"
    ]
  }
}

liubin · 2022-07-06T06:38:55Z

@cpuguy83 @AkihiroSuda @thaJeztah PATL, thanks.

cpuguy83 · 2022-07-06T20:40:43Z

api/types/types.go

 type ShimConfig struct {
-	Binary string
-	Opts   interface{}
+	Binary string `json:"binary"`


Nit: Maybe call this Type which, as I recall, is generally how they are referred to in containerd.
Then we can have another field like Path so people can customize the path to the binary.

Name could also work.

The alternative would be to flatten the output, and show it as shim; #43736 (comment)

{ "path": "runc", "shim": "io.containerd.runc.v2" }

Or would that be simplifying it too much?

cpuguy83 · 2022-07-06T20:42:03Z

api/swagger.yaml

        example: ["--debug", "--systemd-cgroup=false"]
+      shim:
+        description: |
+          Specify the containerd shim v2 name or binary when invoked.


Configuration options for containerd shims.

cpuguy83 · 2022-07-06T20:42:44Z

api/swagger.yaml

+        properties:
+          binary:
+            description: |
+              A containerd shim v2 name or the absolute path of the binary.


Maybe we can forgo the v2 here.

corhere · 2022-07-12T22:15:56Z

api/types/types.go

-	Path string   `json:"path"`
+	Path string   `json:"path,omitempty"`
 	Args []string `json:"runtimeArgs,omitempty"`

-	// This is exposed here only for internal use
-	// It is not currently supported to specify custom shim configs
-	Shim *ShimConfig `json:"-"`
+	Shim *ShimConfig `json:"shim,omitempty"`


The Path and Args fields are effectively ignored when Shim != nil so users who set both might be surprised to find some of their configuration is ignored. The technical reason is that the Path and Args fields are passed as options to the runc shim. Other shims are unlikely to accept a runc options message and the daemon wouldn't know how to marshal the options to the protobuf message type the other shim is expecting, so the daemon makes no attempt to mutate a provided shim config.

moby/daemon/runtime_unix.go

Lines 122 to 135 in be62811

if len(rt.Args) > 0 {

p, err := daemon.rewriteRuntimePath(name, rt.Path, rt.Args)

if err != nil {

return nil, err

}

rt.Path = p

rt.Args = nil

}

if rt.Shim == nil {

rt.Shim = defaultV2ShimConfig(daemon.configStore, rt.Path)

}

return rt, nil

Since the daemon can't marshal the options for arbitrary shims from JSON, the shim name is the only thing which could be configured from the daemon for shims which are not io.containerd.runc.v2. It's one string, and the docker run --runtime flag also happens to be one string... No daemon.json configuration would be required to use alternative runtimes if the daemon was to interpret --runtime values (which do not correspond to any runtimes property in daemon.json) as the desired shim name! This is how containerd's ctr and nerdctl commands behave, to great effect.

From a UX perspective, the --runtime option was meant to be the more "convenient" option, where the user can use docker info to see which runtimes are available/configured on the daemon, and use those as option (--runtime=runc, --runtime=kata), and the daemon-configuratioin can contain additional config options (IIRC, this could even be multiple versions of runc, e.g.).

No daemon.json configuration would be required to use alternative runtimes if the daemon was to interpret --runtime values

I know that access to the API already is equivalent to root, but if the --runtime value passed can be (as is being discussed here) either a name (io.containerd.runc.v2) or a path to a binary, that could potentially open up some "interesting" things (--runtime=/bin/rm, --runtime=/some/arbitrary/script.sh)

corhere · 2022-07-14T16:29:15Z

FYI, this PR appears to be a duplicate of

RFC: runtime: enable kata as optional runtime #42986

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

thaJeztah added impact/api docs/revisit labels Jun 24, 2022

thaJeztah requested changes Jun 24, 2022

View reviewed changes

liubin force-pushed the let-shim-binary-configurable branch from 29b347a to 778a776 Compare June 27, 2022 08:06

cpuguy83 reviewed Jul 6, 2022

View reviewed changes

corhere reviewed Jul 12, 2022

View reviewed changes

corhere mentioned this pull request Jul 13, 2022

daemon: support alternative runtimes MVP #43800

Merged

cpuguy83 closed this in #43800 Jul 29, 2022

thaJeztah mentioned this pull request Jul 29, 2022

[22.06 backport] daemon: support other containerd runtimes (MVP) #43887

Merged

thaJeztah removed impact/api impact/changelog docs/revisit labels Aug 20, 2022

		@@ -657,15 +657,16 @@ type Runtime struct {
		Path string `json:"path"`

	if len(rt.Args) > 0 {
	p, err := daemon.rewriteRuntimePath(name, rt.Path, rt.Args)
	if err != nil {
	return nil, err
	}
	rt.Path = p
	rt.Args = nil
	}

	if rt.Shim == nil {
	rt.Shim = defaultV2ShimConfig(daemon.configStore, rt.Path)
	}

	return rt, nil

Conversation

liubin commented Jun 22, 2022

Uh oh!

liubin commented Jun 22, 2022

Uh oh!

cpuguy83 commented Jun 22, 2022

Uh oh!

liubin commented Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented Jun 23, 2022

Uh oh!

liubin commented Jun 24, 2022

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!

thaJeztah commented Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liubin commented Jun 27, 2022

Uh oh!

liubin commented Jul 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

corhere commented Jul 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

liubin commented Jun 23, 2022 •

edited

Loading

thaJeztah commented Jun 24, 2022 •

edited

Loading

thaJeztah Jul 7, 2022 •

edited

Loading