Add option to specify containerd runtime #4141
Conversation
slonopotamus
force-pushed
the
containerd-runtime-option
branch
from
aca0577
to
d9b22ac
Compare
|
slonopotamus
force-pushed
the
containerd-runtime-option
branch
from
d9b22ac
to
d1ad968
Compare
|
Added missing import. |
|
I believe that |
| @@ -112,6 +112,7 @@ type ContainerdConfig struct { | |||
| Labels map[string]string `toml:"labels"` | |||
| Platforms []string `toml:"platforms"` | |||
| Namespace string `toml:"namespace"` | |||
| Runtime string `toml:"runtime"` | |||
There was a problem hiding this comment.
Shouldn't this support options for runc binary path, cgroup driver, etc?
I also guess this should be map[string]interface{} to support multiple runtime classes in the future.
i.e., The config structure should be similar to the config structure of containerd/CRI.
There was a problem hiding this comment.
I am not sure how to pass that to containerd. What I just wanted to achieve is to be able to override client.defaultRuntime. Because there are cough, cough platforms where runtime isn't mature enough to be declared The Default.
The lack of such configuration option leaded to FreeBSD runtime being hardcoded in buildkit. I'm in the same boat with macOS.
There was a problem hiding this comment.
So, my idea is: with these changes, one can configure runtime options in containerd config, while using buildkit to select needed runtime. Or, option added here can just be used to specify path path to runtime binary. Exposing the full runtime configuration through buildkit config is too much extending the scope of the task.
There was a problem hiding this comment.
Some related work in moby;
I need to refresh my memory (but perhaps the code describes it), but ISTR, we only allowed paths etc to be configured for the legacy bits, and otherwise only allow a (fully qualified?) reference to be specified, or at least from the client side (to prevent a arbitrary paths to be passed that are not in the server's PATH).
There was a problem hiding this comment.
I think I missed this originally. We should probably make sure to cover this is in a first pass - I wrote up a possible implementation: jedevc@22d9bb1. @AkihiroSuda @tonistiigi wdyt? (I took inspiration from how CRI does it today). @slonopotamus you happy to pull that change into your branch (or I can force-push)?
Without a way to configure the runtime, I think the feature is quite limited (e.g. I have a use case for wanting to override the runc binary used by the io.containerd.runc.v2 runtime, which is possible by using the binary_name option). Containerd doesn't seem to have any other way to configure this, so we need to allow this configuration in buildkit if we want to allow users this kind of freedom.
Even if we don't take this now, we need to provide a place in config fields to allow this kind of configuration. Previously we had:
[worker.containerd]
runtime = "io.containerd.runc.v2"I think something like this should work easily:
[worker.containerd.runtime]
name = "io.containerd.runc.v2"
options = { binary_name = "foobar" }There was a problem hiding this comment.
@jedevc I'm perfectly fine if you create a separate PR. I just need a way to specify which runtime to use, it isn't really important what lines one would need to write into config file, it is easy to adapt.
slonopotamus
force-pushed
the
containerd-runtime-option
branch
from
d1ad968
to
e9f1eab
Compare
There was a problem hiding this comment.
LGTM
maybe we'll need to use containerd's WithRuntime if we want to allow users fully configuring the shim behaviour (as commented in #4141 (comment))
|
User already can fully configure runtime in containerd config. I don't see much value adding a second place with exactly the same settings. And then the third, to allow configuration of all that through Moby config? |
The first one only applies to CRI, not to BuildKit and Moby. |
slonopotamus
force-pushed
the
containerd-runtime-option
branch
from
e9f1eab
to
b79ce6d
Compare
|
BTW, Moby already has map[string,Runtime]. So, both containerd and Moby can use different runtimes. It's only |
slonopotamus
force-pushed
the
containerd-runtime-option
branch
from
b79ce6d
to
a96327d
Compare
Signed-off-by: Marat Radchenko <[email protected]>
slonopotamus
force-pushed
the
containerd-runtime-option
branch
from
a96327d
to
ae8e604
Compare
| @@ -46,6 +48,14 @@ func init() { | |||
| defaultConf.Workers.Containerd.Namespace = defaultContainerdNamespace | |||
| } | |||
|
|
|||
| if defaultConf.Workers.Containerd.Runtime == "" { | |||
| if runtime.GOOS == "freebsd" { | |||
| defaultConf.Workers.Containerd.Runtime = "wtf.sbk.runj.v1" | |||
There was a problem hiding this comment.
Maybe add a TODO saying it should be removed once containerd/containerd#8964 comes with a containerd release?
|
@jedevc thanks for pushing this to the end! |
Thoughts: it could just be
runtimeordefault runtime, hinting that in the future specific build run can override it.