Windows support #164
Windows support #164
Conversation
| @@ -57,19 +57,19 @@ install: | |||
| TAR_FLAGS=--transform 's/.*\///g' --owner=0 --group=0 | |||
|
|
|||
| artifacts: clean | |||
| GOOS=linux GOARCH=amd64 make -C $(CURDIR) binaries | |||
| GOOS=$(GOOS) GOARCH=amd64 make -C $(CURDIR) binaries | |||
There was a problem hiding this comment.
This produces nerdctl-$(VERSION_TRIMMED)-linux-amd64.tar.gz, so GOOS has to be hard-coded to linux here.
Instead, we should add GOOS=windows that produces nerdctl-$(VERSION_TRIMMED)-windows-amd64.zip
|
|
||
| // getDataStore returns a string like "/var/lib/nerdctl/1935db59". | ||
| // "1935db9" is from `$(echo -n "/run/containerd/containerd.sock" | sha256sum | cut -c1-8)`` | ||
| func getDataStore(clicontext *cli.Context) (string, error) { |
There was a problem hiding this comment.
How is this different from Linux implementation?
| "github.com/urfave/cli/v2" | ||
| ) | ||
|
|
||
| func newClient(clicontext *cli.Context) (*containerd.Client, context.Context, context.CancelFunc, error) { |
There was a problem hiding this comment.
This does not necessarily need to be a separate file.
The containerd address can be changed with if runtime.GOOS == "windows"{}.
| const AppArmorProfileName = "" | ||
|
|
||
| func DataRoot() string { | ||
| return "C:\\ProgramData\\containerd\\root" |
There was a problem hiding this comment.
Should read %ALLUSERSPROFILE (%ProgramData%) ?
There was a problem hiding this comment.
Specifically,
filepath.Join(os.Getenv("ProgramData"), "containerd", "root")
There was a problem hiding this comment.
The "data root" of nerdctl has nothing to do with containerd's data root.
So this should be like filepath.Join(os.Getenv("ProgramData"), "nerdctl")
| } | ||
|
|
||
| func CNIPath() string { | ||
| return "C:\\Program Files\\containerd\\cni\\bin" |
There was a problem hiding this comment.
Should read %ProgramFiles%?
There was a problem hiding this comment.
filepath.Join(os.Getenv("ProgramFiles"), "containerd", "cni", "bin")
is how containerd.pkg.cri.config.DefaultConfig().CniConfig.NetworkPluginBinDir is defined.
| } | ||
| return s | ||
| } | ||
|
|
||
| const UnameO = "GNU/Linux" |
There was a problem hiding this comment.
Should be in infoutil_linux.go
| "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| // LockFile from https://github.com/boltdb/bolt/blob/master/bolt_windows.go using MIT |
| const ( | ||
| DefaultNetworkName = "nat" | ||
| DefaultID = 0 | ||
| DefaultCIDR = "10.1.0.0/24" |
There was a problem hiding this comment.
Why is this different from Linux?
There was a problem hiding this comment.
I don't think it needs to be, I was using the same values that I had manually configured to test
| package netutil | ||
|
|
||
| const ( | ||
| DefaultNetworkName = "nat" |
There was a problem hiding this comment.
Why is this different from Linux?
There was a problem hiding this comment.
There isn't a "bridge" network type for Windows AFAIK, and the Windows CNI plugins (at least the ones from https://github.com/microsoft/windows-container-networking: nat, sdnoverlay, and sdnbridge) require that the network name equals the network type: microsoft/windows-container-networking#57
sdnoverlay and sdnbridge require extra system setup beyond the CNI config file, so they seem like poor choices for default usage. Third-party CNI plugins are equally likely to require extra system configuration.
There was a problem hiding this comment.
The network name is expected to match Docker's default network, not the name of CNI plugin.
There was a problem hiding this comment.
Ah, I misunderstood. I thought this was what ended up substituted as as .Name in ConfigListTemplate.
Although it's not clear to me why we even have a CNI config if we're going to be using the Docker-provided network setup.
There was a problem hiding this comment.
When using Docker the default network on Windows is called nat so I called it that here. Are you saying this should stay bridge?
| "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| func loadapparmor() { |
| @@ -59,7 +51,7 @@ var runCommand = &cli.Command{ | |||
| Name: "run", | |||
| Usage: "Run a command in a new container", | |||
| Action: runAction, | |||
| BashComplete: runBashComplete, | |||
| BashComplete: runComplete, | |||
| @@ -173,7 +165,7 @@ var runCommand = &cli.Command{ | |||
| &cli.StringFlag{ | |||
| Name: "runtime", | |||
| Usage: "Runtime to use for this container, e.g. \"crun\", or \"io.containerd.runsc.v1\"", | |||
| Value: plugin.RuntimeRuncV2, | |||
| Value: "io.containerd.runhcs.v1", | |||
There was a problem hiding this comment.
Linux runtime shouldn't change
There was a problem hiding this comment.
yup! it was just to make it work. This will need to be extracted so when built for Windows it is defaulted correctly.
| //if len(args) > 0 { | ||
| // opts = append(opts, oci.WithProcessArgs(args...)) | ||
| //} | ||
| if cwd := clicontext.String("cwd"); cwd != "" { |
There was a problem hiding this comment.
Flag name looks different from Linux
|
closing in favor of a more iterative approach. I did take much of the feedback here into consideration. Thanks for the early reviews! |
This refactors to allow building a Windows binary as well as adds
nerdctl runsupport. This a draft to figure out/show the amount of work. This should be broken down into smaller PRs that can be reviewed easier. See #28 for discussion