Improve WSL 2 experience #57

douglascamata · 2022-11-30T14:15:09Z

With proper detection of WSL 2 we can give it the special treatment it needs.

Changes

On detection of WSL 2, the following happens:

Fail fast if cAdvisor is also enabled: it's not supported in WSL 2.
The check for networking from container to host will listen only on tcp4 (IPv4). I tested with tcp (should be IPv4 and v6) but it ended up only opening a tcp6 port and the check happens via IPv4, which fails.
The hostAddr is set to host.docker.local, as Windows/WSL 2 also use it.
The OpenInBrowser will open the URLs in the Windows browser.

I tested this out in my Windows box and it works flawlessly.

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

douglascamata · 2022-11-30T14:16:04Z

interactive/interactive.go

+func WSL2() (bool, error) {
+	if runtime.GOOS != "linux" {
+		return false, nil
+	}
+	out, err := exec.Command("cat", "/proc/version").CombinedOutput()
+	if err != nil {
+		return false, errors.Wrapf(err, "detecting WSL2: %s", string(out))
+	}
+	return bytes.Contains(out, []byte("WSL2")), nil
+}


I would love to move this WSL2 function to https://github.com/efficientgo/core. What do you think? cc @matej-g @bwplotka

Also considering adding context support there to enable quick work cancellation. Not sure whether it would be valuable.

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

saswatamcode · 2022-11-30T14:27:45Z

interactive/interactive.go

+	if runtime.GOOS != "linux" {
+		return false, nil
+	}
+	out, err := exec.Command("cat", "/proc/version").CombinedOutput()


I think we can just do grep here instead of reading the entire contents and doing bytes.Contains?

I am afraid grep might not be thoroughly available in Linux distros by default, especially the more slim ones. /proc/version contains very little text, so I don't think the performance is a problem. It has this "shape": Linux version 2.6.31.12-0.2-default (geeko@buildhost) (gcc version 4.4.1 [gcc-4_4-branch revision 150839] (SUSE Linux) ) #1 SMP 2010-03-16 21:25:39 +0100.

I even thought about not using cat and directly reading /proc/version. 😄

Actually now I consider simply reading the file better. 100% agnostic of whatever is installed in the OS and hides the error from callers. If the file doesn't exist it means not WSL 2.

saswatamcode · 2022-11-30T14:28:49Z

interactive/interactive.go

+	if err != nil {
+		return false, errors.Wrapf(err, "detecting WSL2: %s", string(out))
+	}
+	return bytes.Contains(out, []byte("WSL2")), nil


Do we need to check if it contains WSL2 or Microsoft? Referriing to similar issue microsoft/WSL#4071

I'm aware of that issue. I believe it is better to check for WSL2 because:

I don't know and didn't test if this works with WSL 1. Likely it doesn't as networking is very different between WSL 1 and 2.

We are future proof in case Microsoft releases WSL 3 and it requires a different workaround (😂 😭 )

No false positives if one day a wild Microsoft Linux distribution appears.

I'm just imagining someone having to come back to this code in 5 years like 'wth is a WSL?'

Documented what is WSL at ad6f833

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

moadz · 2022-11-30T15:11:07Z

env.go

 	// Close shutdowns isolated environment and cleans its resources.
 	Close()
+	// WSL2 returns true if the environment is WSL2.
+	WSL2() bool


I'm not sure i understand why we need this. If WSL2 is not compatible with cadvisor we should either:

Remove cadvisor (to remove a dependency on linux for e2e)

Keep cadvisor, and encourage people who use linux subsystem on linux to disable cadvisor.

Adding a field to Environment configuration for this very specific edge case stinks of 'leaky abstraction'.

Conversely you can invert this and have a 'useCadvisor' env variable that will determine if it should be enabled or not so there isn't this conceptual hop from:
' Why is it relevant if i'm using WSL2?'
greps WSL2
'Oh, WSL doesn't support cadvisor.'
'I'll just disable cadvisor'

Just my thoughts, not willing to die on this hill though. IMHO i'd rather avoid embedding OS checks in configuration for very specific feature compatibility.

Got rid of the WSL() function in the Environment interface, as indeed I agree it leaks details from the DockerEnvironment type. Instead now I just import e2einteractive and use it from there. Note that as I mentioned in another comment, I would like to move the function to efficientgo/core if maintainers agree. Changes are in 4330eb9.

Keep cadvisor, and encourage people who use linux subsystem on linux to disable cadvisor.

I talked with @bwplotka about this and our conclusion is that we should "panic" and stop execution in the scenario where we're running in WSL 2 and the option to run cAdvisor is set to true.

moadz · 2022-11-30T15:16:18Z

env_docker.go

-		d.hostAddr = dockerMacOSGatewayAddr
+		d.hostAddr = dockerGatewayAddr
 	default:
+		if d.WSL2() {


What does runtime.GOOS return if running on WSL? If it isn't linux what is it?

This switch statement is also redundant, you really shouldn't have to branch conditionals inside of a switch. This should either be in a case or should not be set here.

I also know it was like this before so 🤷🏽

What does runtime.GOOS return if running on WSL? If it isn't linux what is it?

It does return linux, that's why the implementation checks for the kernel version by reading from /proc/version. Also it uses runtime.GOOS != linux as a condition to fail quickly returning false.

moadz · 2022-11-30T15:18:13Z

monitoring/monitoring.go

+	// via IPv4 to confirm Prometheus can scrape the local endpoint.
+	// Explicitly asking for an IPv4 listener works.
+	if env.WSL2() {
+		networkType = "tcp4"


Is tcp4 the more specific option (specifies you want an ipv4 connection)? If so we can just make that the default and get rid of the branching conditionals.

We can do this. Only downside would be that Docker one day might drop support for ipv4, but I bet that's likely going to happen only after our lifetimes. Will change!

Oh, this might also not work on servers/machines with only ipv6 stack (might some cloud/on-prem CI runners?). 😬

What do you think, @moadz?

Can we use IPv6 then for network port later on? If the latter step requires IPv4, we might as well put tcp4 here.

This is only affecting the listener that is used for a quick test of the host-container networking. It won't affect anything after this point.

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

douglascamata · 2022-11-30T16:57:00Z

interactive/interactive.go

+// HostOSPlatform returns the host's OS platform akin to `runtime.GOOS`, with
+// added awareness of Windows Subsystem for Linux (WSL) 2 environments.
+// The possible values are the same as `runtime.GOOS`, plus "WSL2".
+func HostOSPlatform() string {
+	if wsl2() {
+		return "WSL2"
+	}
+	return runtime.GOOS
+}
+
+func wsl2() bool {
+	if runtime.GOOS != "linux" {
+		return false
+	}
+	version, err := os.ReadFile("/proc/version")
+	if err != nil {
+		return false
+	}
+	return bytes.Contains(version, []byte("WSL2"))
+}


Repeating old comment here for visibility after some updates: I would love to move these two functions into https://github.com/efficientgo/core. If maintainers agree, I'll open a PR there and update this one.

Hm... We could consider that. Feel free to add TODO with this, but I ideally we test it's usability (how useful it is for others) in e2e first.

bwplotka

Some thoughts, otherwise LGTM. Thanks!

env_docker.go

interactive/interactive.go

bwplotka · 2022-12-06T13:55:01Z

interactive/interactive.go

+// HostOSPlatform returns the host's OS platform akin to `runtime.GOOS`, with
+// added awareness of Windows Subsystem for Linux (WSL) 2 environments.
+// The possible values are the same as `runtime.GOOS`, plus "WSL2".
+func HostOSPlatform() string {
+	if wsl2() {
+		return "WSL2"
+	}
+	return runtime.GOOS
+}
+
+func wsl2() bool {
+	if runtime.GOOS != "linux" {
+		return false
+	}
+	version, err := os.ReadFile("/proc/version")
+	if err != nil {
+		return false
+	}
+	return bytes.Contains(version, []byte("WSL2"))
+}


Hm... We could consider that. Feel free to add TODO with this, but I ideally we test it's usability (how useful it is for others) in e2e first.

bwplotka · 2022-12-06T13:57:15Z

monitoring/monitoring.go

+	// via IPv4 to confirm Prometheus can scrape the local endpoint.
+	// Explicitly asking for an IPv4 listener works.
+	if env.WSL2() {
+		networkType = "tcp4"


Can we use IPv6 then for network port later on? If the latter step requires IPv4, we might as well put tcp4 here.

bwplotka · 2022-12-06T13:57:38Z

monitoring/monitoring.go

 	env.AddListener(l)

 	if opt.useCadvisor {
+		if e2einteractive.HostOSPlatform() == "WSL2" {


Again, this is ideally in separate package or e2e.

bwplotka

Amazing, thanks! 💪🏽

bwplotka · 2022-12-07T12:27:32Z

host/host.go

+		return false
+	}
+	version, err := os.ReadFile("/proc/version")
+	if err != nil {


I guess we could be more strict on error cases, but we can improve later (:

* Detect WSL2 and open in browser from Windows Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Fix hostAddr in WSL2 Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Fix host-local endpoint test in WSL2 Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Fail fast to start in WSL2 with cadvisor Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Inline some conditionals Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Detect WSL 2 without relying on OS binaries/tools Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Fix var name Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Drop environment.WSL2() and use func from package Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Improve how OS platform is checked Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Document what is WSL Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Fix WSL2 check for network test Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Fix networkType deletion Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Separate OS platform function into its own package * Reorder imports * Add TODO to new `host` package * Please the copyright linter Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

douglascamata added 4 commits November 30, 2022 14:28

Detect WSL2 and open in browser from Windows

91d3219

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

Fix hostAddr in WSL2

cbfdf71

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

Fix host-local endpoint test in WSL2

1ff6b13

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

Fail fast to start in WSL2 with cadvisor

a3d7d9a

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

douglascamata commented Nov 30, 2022

View reviewed changes

Inline some conditionals

18fc218

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

saswatamcode reviewed Nov 30, 2022

View reviewed changes

douglascamata added 2 commits November 30, 2022 15:55

Detect WSL 2 without relying on OS binaries/tools

2615f73

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

Fix var name

ea4ba92

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

moadz reviewed Nov 30, 2022

View reviewed changes

douglascamata added 5 commits November 30, 2022 16:25

Drop environment.WSL2() and use func from package

4330eb9

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

Improve how OS platform is checked

8db8119

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

Document what is WSL

ad6f833

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

Fix WSL2 check for network test

7029b7f

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

Fix networkType deletion

c1c3e5b

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

douglascamata commented Nov 30, 2022

View reviewed changes

bwplotka reviewed Dec 6, 2022

View reviewed changes

douglascamata added 4 commits December 7, 2022 13:06

Separate OS platform function into its own package

2bad9d7

Reorder imports

efdb76f

Add TODO to new host package

aeed012

Please the copyright linter

47b3c0a

bwplotka approved these changes Dec 7, 2022

View reviewed changes

bwplotka merged commit 596a48e into efficientgo:main Dec 7, 2022

Improve WSL 2 experience #57

Improve WSL 2 experience #57

Uh oh!

Conversation

douglascamata commented Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

douglascamata Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

douglascamata Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moadz Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

douglascamata Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

douglascamata commented Nov 30, 2022 •

edited

Loading

douglascamata Nov 30, 2022 •

edited

Loading

douglascamata Nov 30, 2022 •

edited

Loading

moadz Nov 30, 2022 •

edited

Loading

douglascamata Nov 30, 2022 •

edited

Loading