[Draft] feat: add Windows support #553

vsiravar · 2023-09-01T20:22:43Z

Draft
Issue #, if available:
#492

Description of changes:
Finch changes to support finch on windows using WSL2.

Refactor main package and add build constraints.
Refactor config package and add build constraints and checks for host configuration.
Add handlers to handle path conversion from windows path to wsl path for nerdctl commands.
Add wrappers for filepath functions in system package for NerdctlCommandSystemDeps.

TODO

Handle filepaths for copy command
Add windows specific unit tests in nerdctl_test.go
Refactor Makefile
Add e2e tests for VM specific to WSL2.

Testing done:

Yes, e2e tests have errors related to copy command.

I've reviewed the guidance in CONTRIBUTING.md

License Acceptance

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Signed-off-by: Vishwas Siravara <[email protected]>

pendo324

Looks great to me, just left some minor comments

pendo324 · 2023-09-01T22:14:47Z

cmd/finch/main_darwin.go

+	optionalDepGroups := []*dependency.Group{
+		credhelper.NewDependencyGroup(ecc, fs, fp, logger, fc, system.NewStdLib().Env("USER"),
+			system.NewStdLib().Arch()),
+	}
+
+	optionalDepGroups = append(optionalDepGroups, vmnet.NewDependencyGroup(ecc, lcc, fs, fp, logger))


Is it possible to only extract the depGroups into a new platform specific file? Seems like everything else is the same

pendo324 · 2023-09-01T22:18:43Z

cmd/finch/virtual_machine_start.go

@@ -79,6 +79,7 @@ func (sva *startVMAction) run() error {
 		return err
 	}

+	// TODO: don't run this on Windows


Working on implementing that in Lima now, so hopefully only a temporary todo

pendo324 · 2023-09-01T22:21:56Z

pkg/config/defaults.go

+	cpuDefault(cfg, deps)
+
+	memoryDefault(cfg, mem)


These settings actually aren't configurable with current versions of WSL2

pendo324 · 2023-09-01T22:24:00Z

pkg/disk/disk_unix.go

Once disks are implemented for Windows in Lima, we'll need this to be back to an all-platform file

ginglis13 · 2023-09-01T22:32:20Z

Makefile

+.PHONY: finch-core-local
+finch-core-local:


TODO on removing this rule. IIRC we added this to make lima.exe and the guestagent sourcing the WSL PR to lima:

https://github.com/vsiravar/finch-core-public/blob/windev/Makefile#L87

https://github.com/vsiravar/finch-core-public/blob/windev/Makefile#L152-L156

But once lima is released with WSL support, we should source similar to how we do on macOS via LIMA_URL: https://github.com/vsiravar/finch-core-public/blob/b82c007d22d4d99f8a78a3db7c6252ce9c5afcc8/Makefile#L36C14-L36C14

ginglis13 · 2023-09-01T22:32:45Z

Makefile

+	FINCH_ROOTFS_URL ?= https://deps.runfinch.com/common/x86-64/finch-rootfs-production-amd64-1690920103.tar.zst
+	FINCH_ROOTFS_BASENAME := $(notdir $(FINCH_ROOTFS_URL))
+	FINCH_ROOTFS_BASENAME := $(subst .zst,,$(FINCH_ROOTFS_BASENAME))
+# TODO ROOTFS URL


nit: can remove

ginglis13 · 2023-09-01T22:32:51Z

Makefile

+	FINCH_ROOTFS_URL ?= https://deps.runfinch.com/common/aarch64/finch-rootfs-production-arm64-1690920104.tar.zst
+	FINCH_ROOTFS_BASENAME := $(notdir $(FINCH_ROOTFS_URL))
+	FINCH_ROOTFS_BASENAME := $(subst .zst,,$(FINCH_ROOTFS_BASENAME))
+# TODO ROOTFS URL


nit: can remove

ginglis13 · 2023-09-01T22:33:38Z

Makefile

+	sed -i.bak -e "s/<finch_image_digest>/$(FINCH_IMAGE_DIGEST)/g" $(OUTDIR)/os/finch.yaml
+
+# TODO: Windows PoC - clean this up / consolidate
+.PHONY: finch.yaml


nit: finch.windows.yaml

ginglis13 · 2023-09-01T22:35:23Z

Makefile

 finch.yaml: finch-core
 	mkdir -p $(OUTDIR)/os
 	cp finch.yaml $(OUTDIR)/os
 	# using -i.bak is very intentional, it allows the following commands to succeed for both GNU / BSD sed
 	# this sed command uses the alternative separator of "|" because the image location uses "/"
-	sed -i.bak -e "s|<finch_image_location>|$(FINCH_OS_IMAGE_LOCATION)|g" $(OUTDIR)/os/finch.yaml
+	sed -i.bak -e "s|<finch_image_location>|$(FINCH_IMAGE_LOCATION)|g" $(OUTDIR)/os/finch.yaml
+	sed -i.bak -e "s/<finch_image_arch>/$(LIMA_ARCH)/g" $(OUTDIR)/os/finch.yaml
+	sed -i.bak -e "s/<finch_image_digest>/$(FINCH_IMAGE_DIGEST)/g" $(OUTDIR)/os/finch.yaml


maybe we can consolidate finch.yaml and finch.windows.yaml rules to be something like:

WINDOWS ?= CONF_FILE ?= ifeq ($(GOOS),windows) WINDOWS=.windows CONF_FILE=finch.windows.yaml else CONF_FILE=finch.yaml endif finch$(WINDOWS).yaml: finch-core mkdir -p $(OUTDIR)/os cp $(CONF_FILE) $(OUTDIR)/os # using -i.bak is very intentional, it allows the following commands to succeed for both GNU / BSD sed # this sed command uses the alternative separator of "|" because the image location uses "/" sed -i.bak -e "s|<finch_image_location>|$(FINCH_IMAGE_LOCATION)|g" $(OUTDIR)/os/finch.yaml sed -i.bak -e "s/<finch_image_arch>/$(LIMA_ARCH)/g" $(OUTDIR)/os/finch.yaml sed -i.bak -e "s/<finch_image_digest>/$(FINCH_IMAGE_DIGEST)/g" $(OUTDIR)/os/finch.yaml

ginglis13 · 2023-09-01T22:36:13Z

Makefile

@@ -254,7 +311,7 @@ check-licenses:

 .PHONY: test-unit
 test-unit:
-	go test $(shell go list ./... | grep -v e2e) -shuffle on -race
+	go test $(shell go list ./... | grep -v e2e) -shuffle on


why remove race condition checks?

ginglis13 · 2023-09-01T22:39:13Z

cmd/finch/nerdctl_windows.go

+	"strings"
+)
+
+func convertToWSLPath(systemDeps NerdctlCommandSystemDeps, winPath string) (string, error) {


Could you add a comment here with an example or two of what this conversion looks like?

ginglis13 · 2023-09-01T22:46:11Z

finch.yaml

@@ -5,6 +5,8 @@
 # Default values in this YAML file are specified by `null` instead of Lima's "builtin default" values,
 # so they can be overridden by the $LIMA_HOME/_config/default.yaml mechanism documented at the end of this file.

+#vmType: null


nit: uncomment - this was leftover from my poc :)

ginglis13 · 2023-09-01T22:52:18Z

pkg/config/lima_config_applier_darwin_test.go

@@ -1,6 +1,9 @@
 // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0

+//go:build darwin
+// +build darwin


nit: // +build <tag> tags obsolete after Go 1.18 https://tip.golang.org/doc/go1.18 . this applies to a few other places in this PR. go fix should remove them automatically for you

Signed-off-by: Justin Alvarez <[email protected]>

install packages * qemu-user-static-aarch64 * qemu-user-static-arm * qemu-user-static-x86 * iptables * fuse-sshfs depended on by lima to decrease boot time of Finch VM on Windows Issue #, if available: *Description of changes:* *Testing done:* * EC2 Windows dev instance * build rootfs locally, scp up * run Finch w/ changes in runfinch/finch#553 * basic commands tests -> `finch vm init`, `finch vm pull alpine:latest`, etc. - [x] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Signed-off-by: Gavin Inglis <[email protected]>

Signed-off-by: Justin Alvarez <[email protected]>

AkihiroSuda · 2023-11-24T07:36:52Z

finch.windows.yaml

+# The vmType can be specified only on creating the instance.
+# The vmType of existing instances cannot be changed.
+# 🟢 Builtin default: "qemu"
+vmType: wsl2


This has to be fixed:

[wsl2] [hostagent] connection to the guest agent was closed unexpectedlydial vsock host(2):662862431: vsock: not implemented on windows lima-vm/lima#2025

pendo324 · 2024-02-20T00:09:15Z

Closed by merging #649

vsiravar added 3 commits August 25, 2023 14:58

Windows support

00727dc

Add path conversions to nerdctl commands and vm commands

4de30df

Signed-off-by: Vishwas Siravara <[email protected]>

Add license

d83f9f1

vsiravar changed the title ~~feat: Vsiravar/windev~~ [Draft] feat: Vsiravar/windev Sep 1, 2023

pendo324 reviewed Sep 1, 2023

View reviewed changes

ginglis13 reviewed Sep 1, 2023

View reviewed changes

pendo324 and others added 8 commits September 13, 2023 18:12

add persistent disk support

5460239

Signed-off-by: Justin Alvarez <[email protected]>

move all platform specific code out of the static yaml

5aabe59

Signed-off-by: Justin Alvarez <[email protected]>

fix persistent disk and nerdctl config applier

12b434d

Signed-off-by: Justin Alvarez <[email protected]>

actually mount persistent disk (inverted logic)

ae45dbd

Signed-off-by: Justin Alvarez <[email protected]>

fix vm start

431a7bc

Signed-off-by: Justin Alvarez <[email protected]>

Fix path for anynymous and named volumes

8124988

Update finch-core submodule

d242607

Fix rootfs suffix in Makefile

73a9beb

ginglis13 mentioned this pull request Sep 14, 2023

perf: add lima dependency pkgs to speed up VM boot runfinch/finch-core#157

Merged

1 task

Vishwas Siravara added 2 commits September 14, 2023 23:54

Update rootfs archive

8b5968f

Update finch-core submodule

46d7a44

pendo324 added 2 commits September 15, 2023 17:52

Fix some path and linting issues

79b136c

Signed-off-by: Justin Alvarez <[email protected]>

Update rootfs and finch-core

2a40338

Signed-off-by: Justin Alvarez <[email protected]>

pendo324 changed the title ~~[Draft] feat: Vsiravar/windev~~ [Draft] feat: add Windows support Sep 18, 2023

Update finch-core

7cd44ef

Signed-off-by: Justin Alvarez <[email protected]>

AkihiroSuda reviewed Nov 24, 2023

View reviewed changes

pendo324 closed this Feb 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Draft] feat: add Windows support #553

[Draft] feat: add Windows support #553

vsiravar commented Sep 1, 2023 •

edited

Loading

pendo324 left a comment

pendo324 Sep 1, 2023

pendo324 Sep 1, 2023

pendo324 Sep 1, 2023

pendo324 Sep 1, 2023

ginglis13 Sep 1, 2023

ginglis13 Sep 1, 2023

ginglis13 Sep 1, 2023

ginglis13 Sep 1, 2023

ginglis13 Sep 1, 2023 •

edited

Loading

ginglis13 Sep 1, 2023

ginglis13 Sep 1, 2023

ginglis13 Sep 1, 2023 •

edited

Loading

ginglis13 Sep 1, 2023

AkihiroSuda Nov 24, 2023

pendo324 commented Feb 20, 2024

[Draft] feat: add Windows support #553

[Draft] feat: add Windows support #553

Conversation

vsiravar commented Sep 1, 2023 • edited Loading

License Acceptance

pendo324 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ginglis13 Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ginglis13 Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pendo324 commented Feb 20, 2024

vsiravar commented Sep 1, 2023 •

edited

Loading

ginglis13 Sep 1, 2023 •

edited

Loading

ginglis13 Sep 1, 2023 •

edited

Loading