CI says:

Please add the relevant 'impact/' label to the PR or remove the changelog description

But I do not have permissions to change PR labels =/

Looks like something is perhaps validating too eagerly if it's not required:

InvalidArgument: create container failed validation: container.Runtime.Name must be set: invalid argument
Error: Process completed with exit code 1.

Whoops. I'll put this back then.

Rebased to resolve merge conflicts.

I'm not sure I can quickly find alternative runtime that can be easily distinguised by its external behavior from hcsshim... To be honest, I discovered #50542 when was trying to use my custom build of hcsshim to debug some issues.

The best candidate would be runwasi, because we can easily test "either we can run a WASM image or not", but it currently does not compile on Windows.

I did a quick rebase, because there were merged conflicts in the imports due to the modules being merged.

Thanks! I'm happy to see some of the kludge-y code disappear. I did make a quick pass at trying to follow the logic we have in place now, but could possibly use some more eyes.

I wrote down my first pass below;

I did a compare between the Unix and Windows implementations of this code, and there's one thing I noticed; on Linux, we do some extra steps;

• If no runtime is set, we take the default from the daemon config (on Windows, we take the containerd default)
• After the runtime to use is resolved, we store the container's hostConfig on disk
• There's also a step to validate if the given runtime could be found, and if not, an error is returned.

Diff below;

diff --git a/./daemon/start_unix.go b/./daemon/start_windows.go
index 8e3c4ab07c..bc4f04c5d6 100644
--- a/./daemon/start_unix.go
+++ b/./daemon/start_windows.go
@@ -1,25 +1,23 @@
-//go:build !windows
-
 package daemon
 
 import (
-	"context"
-
+	"github.com/containerd/containerd/v2/defaults"
 	"github.com/moby/moby/v2/daemon/container"
 )
 
-// getLibcontainerdCreateOptions callers must hold a lock on the container
 func (daemon *Daemon) getLibcontainerdCreateOptions(daemonCfg *configStore, container *container.Container) (string, interface{}, error) {
-	// Ensure a runtime has been assigned to this container
-	if container.HostConfig.Runtime == "" {
-		container.HostConfig.Runtime = daemonCfg.Runtimes.Default
-		container.CheckpointTo(context.WithoutCancel(context.TODO()), daemon.containersReplica)
+	if container.HostConfig.Runtime != "" {
+		return container.HostConfig.Runtime, nil, nil
+	}
+
+	if daemonCfg.DefaultRuntime != "" {
+		return daemonCfg.DefaultRuntime, nil, nil
 	}
 
-	shim, opts, err := daemonCfg.Runtimes.Get(container.HostConfig.Runtime)
-	if err != nil {
-		return "", nil, setExitCodeFromError(container.SetExitCode, err)
+	if daemon.containerdClient == nil {
+		// We're running in legacy non-containerd mode, runtime doesn't affect anything
+		return "", nil, nil
 	}
 
-	return shim, opts, nil
+	return defaults.DefaultRuntime, nil, nil
 }

👇 👉 Not all related to this PR, also looking at the Unix implementation / logic 👈

While looking at the Unix implementation👇

• this code is called by daemon.containerStart, not daemon.containerCreate, so if we're persisting this as (host) config, I'm slightly wondering if it should've been resolved on container create, so that the container could be rejected if no valid runtime was found, instead of creating an (at create-time) invalid container.
• ❓ (i.e., when is the right moment to resolve the config and to "bake" it into the container state?)
• 👉 the Linux validation step somewhat matches that; if the container was started before, it would've had its runtime "baked" in the config. The validation would invalidate the runtime in the container-config on disk before proceeding.
• 🤔 but on Windows, the resolved runtime would not be persisted, so depending on if any call to container.CheckpointTo() or container.toDisk() happened, it would either re-resolve the runtime to use, or use what's persisted to disk.

Not sure what's correct here, but wondering if the Windows and Unix implementations should be aligned here (and persist whatever was resolved to Disk?)

For the validation, there's some interesting observations as well; I wondered what the interface{} output var was (why an interface{});

It looks like that's shimConfig, which currently is only defined on non-Windows.

❓ Was it left out for Windows with the assumption "Windows doesn't use containerd" (which now no longer holds true), or was this because it's legacy?

If no runtime is set, we take the default from the daemon config (on Windows, we take the containerd default)

I am returning daemonCfg.DefaultRuntime if it is set before fallbacking to containerd default runtime.

After the runtime to use is resolved, we store the container's hostConfig on disk

I saw that code but was not sure whether it is needed. Will add saving.

Thanks! Definitely take my comments with a pinch of salt; parts of this code became quite convoluted over time to facilitate all the different situations (and/or stubbing things on Windows). I started to unravel some bits a while back as part of #49551 and #50398, but there were still (too) many abstractions in some spots making it hard to follow the logic. 😅

I'm slightly wondering if it should've been resolved on container create, so that the container could be rejected if no valid runtime was found, instead of creating an (at create-time) invalid container.

Not sure about this. One might want to create a container and never start it (but instead just docker cp files from it, for example). I'd leave things as they are.

Forgot to say: updated PR to save container after assigning a runtime shim to it. Not sure why saving neither uses context from outer code nor why saving error is ignored, just did it the same way as in unix version.

Thanks!

Not sure why saving neither uses context from outer code nor why saving error is ignored, just did it the same way as in unix version.

I would have to look; my assumptions;

• Code that was written before context exists (or common in use)
• Possibly the save should not terminate if the context is cancelled (but could now use context.WithoutCancel()
• Error handling; no idea!

I asked a colleague to have a closer look at this PR as well 🤗

Thx @dmcgowan

Did a rebase as there was a tiny merge-conflict because of #50672 (changed interface{} to any

Let's bring this one in once CI completes

It seems, this change broke one usecase: when dockerd is started with --containerd-address=<value> --default-runtime=com.docker.hcsshim.v1, docker attempts to pass that runtime name to containerd, what results in container startup failue due to nonexistent containerd-shim-hcsshim-v1.exe on PATH.