Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow switching Windows runtimes. #42089

Merged
merged 1 commit into from Sep 23, 2021
Merged

Conversation

cpuguy83
Copy link
Member

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
Copy link
Contributor

olljanat commented Mar 1, 2021

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:

// 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:

return "", opts, nil

@cpuguy83 cpuguy83 changed the title [WIP] ALlow switching Windows runtimes. [WIP] Allow switching Windows runtimes. Mar 22, 2021
@cpuguy83
Copy link
Member Author

cpuguy83 commented Sep 9, 2021

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 cpuguy83 closed this Sep 9, 2021
@cpuguy83
Copy link
Member Author

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 cpuguy83 reopened this Sep 10, 2021
@cpuguy83 cpuguy83 force-pushed the windows_containerd branch 5 times, most recently from d6277dd to 81fb2be Compare September 13, 2021 18:04
@cpuguy83 cpuguy83 changed the title [WIP] Allow switching Windows runtimes. Allow switching Windows runtimes. Sep 13, 2021
@cpuguy83
Copy link
Member Author

RS5 failures look unrelated.

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

rt := daemon.configStore.GetDefaultRuntimeName()
if rt == "" {
if daemon.configStore.ContainerdAddr == "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh, got it; thanks for the added detail.

Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty straightforward to me 😇

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines +8 to +10
// 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 = ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
}

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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
Copy link
Member

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>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (assuming green)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants