Make OCI options cross-platform

[mxpv]

Since VM based runtimes become a thing, at doesn’t make any sense anymore to separate OCI options by platform (e.g. have spec_opts_linux, _windows, etc).
It’s quite legal to use different host and guests (like run Windows container from Linux host or Linux VM from Darwin).
In current implementation a runtime receives a wrong OCI spec if host OS != guest OS.

Instead we should be asking runtime what kind of OS its going to run and depending on that containerd must build proper container spec. So we should not exclude OCI options at compile time.

In the end we should end up with something like this:

func CRI.CreateContainer() {

// same code for all platforms
opts = buildCommonSpecOpts();

// doesn't matter which host OS is:
switch sandboxRuntime.GetTargetOS() {
case Linux:
   opts = appendLinuxOpts();
case Windows:
   opts = appendWindowsOpts();
…

}

This PR:

• a22081c Consolidates OCI spec options and make them available on all platforms (except those which rely on platform specific features).
• 05dc1c0 Updates sandbox API interfaces to return required OS
• 6e9fb18 Do some initial work - build common spec builder.

Doing PRs separately to avoid huge diffs. This one just copy-pastes function to make them available everywhere.

[mxpv]

Turns out there was an issue for this: #7910
/cc @dcantah

[dcantah]

Nice I might not even need to do anything 🤣

[mxpv]

/test pull-containerd-sandboxed-node-e2e

[kzys]

Let's say LCOW implements Platform, does it return Linux?

[dcantah]

That's the tricky part here it seems, LCOW is a part of the same shim as Windows containers. Feel like we'd need something here to tell the shim what guest to launch in the first place (annotation/shim option etc.) Likely a different PR

[egernst]

I wonder if we can add this as part of the runtime configuration, similar to some of the other per-shim configuration fields. Greedily, it makes me think that this needn't be sandbox API only. Similarly, perhaps a single shim can actually support multiple guest targets.

[dcantah]

Yea that was my first thought here #7910, if it's generic though and specified here we still need to forward it to the shim somehow (annotation would be easiest but ideally whatever would denote it would be standardized/part of the runtime spec)

[egernst]

I see -- you're saying that the shim cannot determine the target based on the resulting spec that it receives, and we'd be reliant on an annotation otherwise to indicate?

[kevpar]

Sorry for not getting a chance to comment on this sooner. Adding a few thoughts from my side.

LCOW isn't supported on CRI today, we don't really need to solve for that at this point. However, we should aim for a path where the runhcs shim can implement sandbox support, and right now I don't think it would have a good way to know what platform to report.

Given that we will probably need some way (e.g. a platform field on each runtime handler like in #7910) for CRI to tell the shim what platform to create a sandbox for, it seems a bit odd to me that we then have to turn around and ask the sandbox what its platform is. We just got done telling it what platform it should be, so can we just remember that value (perhaps stored in CRI's sandboxstore) rather than querying it?

This would mean we can avoid adding a new controller/sandbox API to query what platform is running, which simplifies the changes needed here.

Would splitting into two shims even an option?

I'd prefer we don't have to force a change in shim architecture to accomodate this. There are also some technical reasons to want a single shim binary, such as code page sharing between shim processes.

[mxpv]

it seems a bit odd to me that we then have to turn around and ask the sandbox what its platform is.

That's fair. However there is no official way on CRI level to specify target platform, so it's currently up to a runtime how to figure out which target OS to run (configs, annotations, etc). So I'd prefer to avoid hard-coding this on containerd-CRI side for now. This is a good path forward though, I like this, we can update interfaces in 2.0 as sandbox API is still experimental and we can tune interface as we need.

[kevpar]

Well I guess this is okay 😆 Personally feel that what platform is being run is a central enough part that CRI should explicitly specify it, but don't think it's a big deal.

[dcantah]

KEP time? 🤓

[egernst]

Well, it would be a configuration option - not hard coded, similar to runtime's config path or which CNI plugin to use, right?

[mxpv]

/test pull-containerd-sandboxed-node-e2e

[dcantah]

I think a lot of this makes sense and the sandbox API fits very nice here. I'll work on a way for shims that support multiple platforms to get passed what OS the sandbox needs in the first place; I'll reword #7910 a little. My main comment is making that container spec generation a little nicer so I'll wait for that and re-look

[mxpv]

@dcantah @fuweid Updated PR as suggested.
opts/ package files are renamed instead of copy-paste (with minor per-platform fixups)
And there are separate spec builders for each platform (moved from _platfrom_specific files).

[mxpv]

/test pull-containerd-sandboxed-node-e2e

[fuweid]

LGTM. Thanks!