Allow switching Windows runtimes. by cpuguy83 · Pull Request #42089 · moby/moby

cpuguy83 · 2021-02-26T23:26:59Z

This adds support for 2 runtimes on Windows, one that uses the built-in
HCSv1 integration and another which uses containerd with the runhcs
shim.

Related to #41455

olljanat · 2021-03-01T06:10:50Z

Looks that finding runtime binaries with this logic fails to error:

specified default runtime 'io.containerd.runhcs.v1' does not exist

Way how I set it on #41479 works.

Which is enabling experimental mode + DOCKER_WINDOWS_CONTAINERD_RUNTIME which are called on:

moby/pkg/system/init_windows.go

Lines 15 to 29 in 46cdcd2

    
           // InitContainerdRuntime sets whether to use ContainerD for runtime 
        
           // on Windows. This is an experimental feature still in development, and 
        
           // also requires an environment variable to be set (so as not to turn the 
        
           // feature on from simply experimental which would also mean LCOW. 
        
           func InitContainerdRuntime(experimental bool, cdPath string) { 
        
           	if experimental && len(cdPath) > 0 && len(os.Getenv("DOCKER_WINDOWS_CONTAINERD_RUNTIME")) > 0 { 
        
           		logrus.Warnf("Using ContainerD runtime. This feature is experimental") 
        
           		containerdRuntimeSupported = true 
        
           	} 
        
           } 
        
           // ContainerdRuntimeSupported returns true if the use of ContainerD runtime is supported. 
        
           func ContainerdRuntimeSupported() bool { 
        
           	return containerdRuntimeSupported 
        
           }

and setting containerd runtime name on:

moby/daemon/start_windows.go

Line 15 in 46cdcd2

return "", opts, nil

cpuguy83 · 2021-09-09T00:21:28Z

Closing this one as I think the work @olljanat is exactly the right way to handle this.
If you want v1, don't specify a containerd socket.

cpuguy83 · 2021-09-10T22:42:09Z

Actually, I take that back. There's some other work I'd like to do (namely embed containerd instead of relying on an external one when no --containerd= is set) that needs this work.

cpuguy83 · 2021-09-14T20:57:11Z

RS5 failures look unrelated.

tianon · 2021-09-15T19:40:32Z

daemon/daemon_windows.go

+
+	rt := daemon.configStore.GetDefaultRuntimeName()
+	if rt == "" {
+		if daemon.configStore.ContainerdAddr == "" {


No cases where we use a default value for the containerd address or anything like that? ISTR some talk about trying to default to V2 on 2022+? (I guess that ship sailed?)

So my plan is to follow-up with a PR to embed containerd and use that if no address is specified on both linux and windows.

That said, even on Linux we don't default a socket here.
On Windows we just don't start a containerd process like we do on Linux.

Ahhh, got it; thanks for the added detail.

tianon

Looks pretty straightforward to me 😇

thaJeztah

LGTM

thaJeztah · 2021-09-22T16:09:23Z

daemon/config/config_windows.go

+	// This is used by the `default-runtime` flag in dockerd as the default value.
+	// On windows we'd prefer to keep this empty so the value is auto-detected based on other options.
+	StockRuntimeName = ""


The whole StockRuntimeName vs DefaultRuntimeName is a bit confusing.

Wondering, will setting this to an empty string cause some odd behavior if somehow the option is set to an empty string?

moby/opts/runtime.go

Lines 44 to 46 in 7b9275c

if parts[0] == o.stockRuntimeName {

return fmt.Errorf("runtime name '%s' is reserved", o.stockRuntimeName)

}

Ah, never mind; looks like we first check if it's not empty;

moby/opts/runtime.go

Lines 39 to 41 in 7b9275c

if parts[0] == "" || parts[1] == "" {

return fmt.Errorf("invalid runtime argument: %s", val)

}

thaJeztah

oh! looks like CI is failing; looks relevant;

ERRO Running error: buildssa: analysis skipped: errors in package: [/go/src/github.com/docker/docker/daemon/config/config_linux_test.go:153:5: unknown field DefaultRuntime in struct literal]

thaJeztah · 2021-09-22T16:16:35Z

Could you also open a PR in the CLI repository to update the daemon.json for Windows? (it shows an example with "all options supported by Windows"); https://github.com/docker/cli/blob/master/docs/reference/commandline/dockerd.md#on-windows

thaJeztah · 2021-09-23T07:00:41Z

This should fix the failure;

diff --git a/daemon/config/config_linux_test.go b/daemon/config/config_linux_test.go
index 424bfc00e6..cfc635567d 100644
--- a/daemon/config/config_linux_test.go
+++ b/daemon/config/config_linux_test.go
@@ -150,7 +150,9 @@ func TestUnixValidateConfigurationErrors(t *testing.T) {
                                Runtimes: map[string]types.Runtime{
                                        "foo": {},
                                },
-                               DefaultRuntime: "bar",
+                               CommonConfig: CommonConfig{
+                                       DefaultRuntime: "bar",
+                               },
                        },
                },
        }

This adds support for 2 runtimes on Windows, one that uses the built-in HCSv1 integration and another which uses containerd with the runhcs shim. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

thaJeztah

LGTM (assuming green)

cpuguy83 mentioned this pull request Feb 26, 2021

Windows: remove direct HCS calls #41455

Open

TBBle mentioned this pull request Mar 1, 2021

Error building 4.25.3 windows server 2019 "Not enough space on disk" adamrehn/ue4-docker#99

Closed

cpuguy83 changed the title ~~[WIP] ALlow switching Windows runtimes.~~ [WIP] Allow switching Windows runtimes. Mar 22, 2021

olljanat mentioned this pull request May 18, 2021

Windows CI: Add support for testing with containerd #41479

Merged

cpuguy83 closed this Sep 9, 2021

cpuguy83 reopened this Sep 10, 2021

cpuguy83 force-pushed the windows_containerd branch 5 times, most recently from d6277dd to 81fb2be Compare September 13, 2021 18:04

cpuguy83 changed the title ~~[WIP] Allow switching Windows runtimes.~~ Allow switching Windows runtimes. Sep 13, 2021

cpuguy83 force-pushed the windows_containerd branch from 81fb2be to 1a72df5 Compare September 13, 2021 21:10

cpuguy83 requested a review from tianon September 15, 2021 18:49

cpuguy83 force-pushed the windows_containerd branch 2 times, most recently from 54ebe0b to e1a1bf3 Compare September 15, 2021 19:38

tianon reviewed Sep 15, 2021

View reviewed changes

tianon approved these changes Sep 15, 2021

View reviewed changes

thaJeztah approved these changes Sep 22, 2021

View reviewed changes

thaJeztah added area/runtime Runtime docs/revisit impact/changelog status/4-merge labels Sep 22, 2021

thaJeztah added this to the 21.xx milestone Sep 22, 2021

thaJeztah requested changes Sep 22, 2021

View reviewed changes

thaJeztah removed the status/4-merge label Sep 22, 2021

thaJeztah added the status/2-code-review label Sep 22, 2021

Allow switching Windows runtimes.

7ccf750

This adds support for 2 runtimes on Windows, one that uses the built-in HCSv1 integration and another which uses containerd with the runhcs shim. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 force-pushed the windows_containerd branch from e1a1bf3 to 7ccf750 Compare September 23, 2021 17:44

thaJeztah approved these changes Sep 23, 2021

View reviewed changes

thaJeztah added status/4-merge and removed status/2-code-review labels Sep 23, 2021

cpuguy83 mentioned this pull request Sep 23, 2021

doc: Add default-runtime opt to daemon.json docker/cli#3307

Merged

thaJeztah merged commit ed83e2e into moby:master Sep 23, 2021

cpuguy83 deleted the windows_containerd branch October 14, 2021 18:23

olljanat mentioned this pull request Jul 31, 2022

Windows: Run containerd as managed process #43893

Closed

CharityKathure mentioned this pull request Jun 11, 2024

Windows: Run containerd as managed process #47955

Merged

	if parts[0] == o.stockRuntimeName {
	return fmt.Errorf("runtime name '%s' is reserved", o.stockRuntimeName)
	}

	if parts[0] == "" \|\| parts[1] == "" {
	return fmt.Errorf("invalid runtime argument: %s", val)
	}

Conversation

cpuguy83 commented Feb 26, 2021

Uh oh!

olljanat commented Mar 1, 2021

Uh oh!

cpuguy83 commented Sep 9, 2021

Uh oh!

cpuguy83 commented Sep 10, 2021

Uh oh!

cpuguy83 commented Sep 14, 2021

Uh oh!

tianon Sep 15, 2021

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Sep 15, 2021

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Sep 15, 2021

Choose a reason for hiding this comment

Uh oh!

tianon Sep 15, 2021

Choose a reason for hiding this comment

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Sep 22, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah Sep 22, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Sep 22, 2021

Uh oh!

thaJeztah commented Sep 23, 2021

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants