Add options to specify containerd runtime (alternative)

[jedevc]

Alternative to #4141 (cc @slonopotamus)

See #4141 (comment) - in addition to just adding a new containerd runtime config options, it allows passing options to that runtime.

[jedevc]

This is pulled pretty much directly from https://github.com/containerd/containerd/blob/c33249cbe6cb5a5e877647626472d16174b83f85/pkg/cri/sbserver/helpers.go#L289-L299.

Because of how this works, this means that we'd need to add support for each containerd runtime whose options we want to support, which is a little bit fiddly IMO.

@AkihiroSuda is there a different way to work around this? Or alternatively, could we potentially expose this logic as public in containerd?

[AkihiroSuda]

Or alternatively, could we potentially expose this logic as public in containerd?

SGTM

[slonopotamus]

Ideally, options should be an opaque type. Otherwise, this is just a leaky abstraction and we only pretend that runtimes are easily swappable.

[TBBle]

Just came across this (because it broke Windows RUN due to the aforementioned fiddly changes being needed, see #4364)

The only way I can see of avoiding this fiddlyness (which as-noted, containerd/cri also suffers from) would be to amend the containerd v2 runtime ABI to require runtimes to support both their own Options type, and accept runtimeoptions.Options and parse their own type out of either the ConfigPath or ConfigBody.

I suggested that hcsshim support that in microsoft/hcsshim#1941 and have more notes on the idea there, but I'm not super-sold on it being a great approach. It also doesn't help a lot when an application needs to do more than reserialise a user-provided config block into an Options struct. But for use-cases like this (and containerd/cri) that's what we're trying to do, so it fits the bill well.

One improvement I did suggest is that the thing that defines the Options struct should also export the name, so that if-chains like this are correctly coupled. But that assumes the struct is containerd-specific; it is in hcsshim, but I don't know off-hand if runc also uses this struct elsewhere and the runtime name would be irrelevant. (plugin.RuntimeRuncV2 is historical legacy, AFAIK; the list of runtimes in plugin shouldn't be growing.)

I suspect that if the default option was nil, then at least it would not fail when a runtime whose options type is not known is used, as long as that runtime does not require any settings; hcsshim meets this criteria, it correctly handles the "No options struct" case, it only barfs when it gets one it cannot deserialize.

In the end, making something like this public from containerd isn't a bad idea, but it only helps for cases that containerd knows about, and there'll certainly be situations where you need to handle a new runtime locally like this either because the containerd change is too new to vendor, or because the update to containerd is stuck in a PR.

Right now, containerd itself doesn't know about any of these types; containerd/cri does because it does the same thing we do here (except it also populates runtimeoptions.Options.ConfigBytes) to handle its own config, and ctr happens to know how to pass "Debug" into hcsshim. And similar to my comment above, containerd probably shouldn't start to know about all the possible runtime types, but no specifically-better solution comes to mind.

[slonopotamus]

BUG: this should be name = "..."

[neersighted]

We have support for configuring this on the moby/moby side; I'm wondering if BuildKit should inherit the default runtime + options from Moby when compiled into dockerd.

If so, where is the correct place in the lifecycle to inject the runtime config?

[jedevc]

@neersighted buildkit in moby seems to still use the runc executor, not the containerd executor: https://github.com/moby/moby/blob/a1d966c492c9f3941e1b6627f3f1128e76a27a2d/builder/builder-next/executor_linux.go#L58-L68.

So application of the containerd config wouldn't apply here. But if the containerd executor would be used, the runtime config could be injected in that function linked above.

[neersighted]

It does use the runc executor, but with this being configurable now, I see a strong justification for potentially using the containerd executor (as users may want to e.g. run their builds with Kata, just like their regular containers).

Thanks for the pointer; for some reason even though I worked on this code last month, I forgot how we handle the executor lifecycle in Moby 😅

[slonopotamus]

And btw, docker build doesn't have --runtime option. Only docker run has one.

[neersighted]

Yes, that's because only the runc executor is used in Moby. In a world where BuildKit supports multiple runtimes with the containerd backend, we can potentially revisit that as well. However lots of design/UX considerations come up there; easier to change the default first, and then think about new command line flags.

[slonopotamus]

Interestingly, if you enable containerd-snapshotter feature, then this creates containerd executor, but it is later gets overwritten with https://github.com/moby/moby/blob/a1d966c492c9f3941e1b6627f3f1128e76a27a2d/builder/builder-next/controller.go#L112-L116

[TBBle]

It does use the runc executor, but with this being configurable now, I see a strong justification for potentially using the containerd executor (as users may want to e.g. run their builds with Kata, just like their regular containers).

WCOW has entered the chat.