Build CRI Plugin on Windows and Add Presubmit #1258

Random-Liu · 2019-09-02T01:02:33Z

This PR moves linux specific logic into _unix.go files, and make this buildable on windows.

This PR also adds appveyor presubmit test. We don't use travis windows support because travis only supports windows 1803 (https://docs.travis-ci.com/user/reference/windows/#windows-version), but we need RS5 (17655+ build) for the containerd integration to work. appveyor supports it https://www.appveyor.com/docs/windows-images-software/#visual-studio-2019.

Note that this PR leaves all windows implementation empty. I have a local version which makes it work, I'll send out it one by one for easy reviewing and testing.

The first commit includes #1259.

mikebrow

not sure how to review, it looks from this one (main) commit that we're forking most of the tree for things that might have some differences and setting the windows version of the fork to ... not implemented for now. Concerned that having two mostly different versions of the tree will make it harder to support the product. For example, you'll need to read both container_create_windows and container_create_unix before knowing how to modify either, and will have to test both independently, vs being able to just ignore simple path differences or sections of the code that say something like unix only or windows only.

mikebrow · 2019-09-03T13:55:56Z

.appveyor.yml

+  PATH: C:\go\bin;C:\tools\mingw64\bin;$(PATH)
+  CGO_ENABLED: 1
+  matrix:
+    - GO_VERSION: 1.12.9


has to be hard coded to 1.12.9? this appveyor test is going to be a pain isn't it :)

other than the hard coding of the go version this presubmit test LGTM

Hard coded because of this containerd/containerd#3541

mikebrow · 2019-09-03T14:17:51Z

pkg/config/config_windows.go

+
+// DefaultConfig returns default configurations of cri plugin.
+func DefaultConfig() PluginConfig {
+	return PluginConfig{}


i was only expecting the paths to be different for the most part in things like configs... which would require a two different sets of paths approach based on windows/linux ... not two copies of config. For example, StreamServerAddress: "127.0.0.1", local host the same on windows? As another example, sandbox image, is it multi arch? If so should be the same image. Still another, docker registry should be the same right?

Put another way... are we just forking entire sets of functions if there might be a difference of one path or one feature in a function, or are we forking because we know it will be very unique and no simple separation of paths and tools used could be determined. What's the plan?

It works in place :). Mostly haha. @Random-Liu - Should you fill this out with the defaults that matter from our fork for the checkin or do you want to do that as a followup?

@mikebrow Here is the diff:

< // +build !windows --- > // +build windows 30,32c30,32 < NetworkPluginBinDir: "/opt/cni/bin", < NetworkPluginConfDir: "/etc/cni/net.d", < NetworkPluginMaxConfNum: 1, // only one CNI plugin config file will be loaded --- > NetworkPluginBinDir: "C:\\Program Files\\containerd\\cni\\bin", > NetworkPluginConfDir: "C:\\Program Files\\containerd\\cni\\conf", > NetworkPluginMaxConfNum: 1, 37c37 < DefaultRuntimeName: "runc", --- > DefaultRuntimeName: "runhcs", 40,41c40,41 < "runc": { < Type: "io.containerd.runc.v1", --- > "runhcs": { > Type: "io.containerd.runhcs.v1", 49d48 < EnableSelinux: false, 55c54 < SandboxImage: "k8s.gcr.io/pause:3.1", --- > SandboxImage: "mcr.microsoft.com/k8s/core/pause:1.0.0", 57d55 < SystemdCgroup: false, 67d64 < DisableProcMount: false,

Let's see whether we can further clean this up as part of #1245.

Added #1245 (comment)

@Random-Liu - Knowing there is a future here can we not add "runhcs" and use "runhcs-wcow-process". It makes more sense and is more descriptive once the other two: "runhcs-wcow-hypervisor" and "runhcs-lcow" are added

Sure. Will do in the next PR when actually adding the default config.

mikebrow

vendor commit LGTM

jterry75 · 2019-09-03T20:43:25Z

cmd/containerd/builtins_windows.go

+package main
+
+import (
+	_ "github.com/containerd/containerd/diff/lcow"


I'm fine with this but since you are only doing WCOW process for the first round we dont need any lcow/ stuff

The problem is that the lcow differ is required here. https://github.com/containerd/containerd/blob/4e6e61c44e83ba1f69aa37ea603a9f23383821fb/services/diff/service_windows.go#L22

Can we add a no_windows-lcow build tag in containerd?

Oh funny. No problem just leave it. I dont think its worth the tag it wont work here anyways

pkg/netns/netns_windows.go

pkg/os/os.go

pkg/server/container_create_windows.go

jterry75 · 2019-09-03T20:53:42Z

pkg/server/container_stats.go

@@ -25,6 +25,8 @@ import (

 // ContainerStats returns stats of the container. If the container does not
 // exist, the call returns an error.
+// TODO(windows): hcsshim Stats is not implemented, add windows support after


Ah. I have a PR for you. Will link you in

Thanks! I'll file an issue after my next PR, and will add the support once the hcsshim side is ready. :)

pkg/server/container_stats_list_windows.go

pkg/server/container_update_resources_windows.go

jterry75 · 2019-09-03T20:56:22Z

pkg/server/helpers.go

@@ -51,46 +49,24 @@ const (
 	errorStartReason = "StartError"
 	// errorStartExitCode is the exit code when fails to start container.
 	// 128 is the same with Docker's behavior.
+	// TODO(windows): Figure out what should be used for windows.
 	errorStartExitCode = 128


Is this for "command not exist"?

Usually this is caused by command not found.

See this https://github.com/moby/moby/blob/master/daemon/start.go#L127

jterry75 · 2019-09-03T20:57:16Z

pkg/server/helpers.go

@@ -443,6 +340,7 @@ func getRuntimeOptions(c containers.Container) (interface{}, error) {

 const (
 	// unknownExitCode is the exit code when exit reason is unknown.
+	// TODO(windows): Figure out unknown exit status for windows.
 	unknownExitCode = 255


Runtime V2 defined this as 255 so we return that for Windows as well

Nice. Sound good.

Removed the TODO.

pkg/server/io/helpers_windows.go

jterry75

Nice! Minor questions

Random-Liu · 2019-09-03T22:01:00Z

not sure how to review, it looks from this one (main) commit that we're forking most of the tree for things that might have some differences and setting the windows version of the fork to ... not implemented for now. Concerned that having two mostly different versions of the tree will make it harder to support the product. For example, you'll need to read both container_create_windows and container_create_unix before knowing how to modify either, and will have to test both independently, vs being able to just ignore simple path differences or sections of the code that say something like unix only or windows only

@mikebrow I share the same concern. Actually I've done a round of de-duplication. I think we've reduced to a reasonable amount of duplication. :)

I'll send out the actual windows implementation soon. If we still think that some part can be de-duplicated, we can cleanup after that is done.

jterry75

LGTM

mikebrow

LGTM

Random-Liu · 2019-09-03T23:08:20Z

Apply LGTM based on #1258 (review) and #1258 (review)

Signed-off-by: Lantao Liu <lantaol@google.com>

k8s-ci-robot · 2019-09-03T23:24:11Z

New changes are detected. LGTM label has been removed.

Random-Liu · 2019-09-03T23:24:46Z

Did a rebase. Apply LGTM based on #1258 (review) and #1258 (review)

yujuhong · 2019-09-04T21:36:26Z

Late to the party, but lgtm :-)

k8s-ci-robot added the size/XXL label Sep 2, 2019

Random-Liu closed this Sep 2, 2019

Random-Liu reopened this Sep 2, 2019

Random-Liu force-pushed the windows-support-1 branch 11 times, most recently from ab036cc to 60f460d Compare September 3, 2019 05:53

Random-Liu changed the title ~~[WIP] Windows support 1~~ Build CRI Plugin on Windows and Add Presubmit Sep 3, 2019

Random-Liu added this to the v1.4 milestone Sep 3, 2019

Random-Liu assigned jterry75, yujuhong and mikebrow Sep 3, 2019

Random-Liu force-pushed the windows-support-1 branch 4 times, most recently from 11ccfa2 to a604e24 Compare September 3, 2019 07:58

mikebrow reviewed Sep 3, 2019

View reviewed changes