fix wsl install workflow on machine init command #26201

lstocchi · 2025-05-26T13:07:55Z

this patch changes how the detection of wsl works. The old way of using wsl --status command output to detect some missing features required by WSL is not fully reliable. WSL checks if the wsl feature is enabled and if the vmcompute service do exist. However, this is not enough to identify if the virtual machine platform feature is enabled. The vmcompute service could exist because it has been installed by other tools or it could exist but being stopped.

The way proposed by this patch is to try execute the import command and, if it fails, check the error and if it is related to the Host Compute Service try to install all features required by WSL.

The flow is the same as before, the user is asked to execute the podman machine init command with elevated privileges. Eventually, after enabling WSL and VMP features, the user is asked to reboot the machine.

When the machine restarts, the powershell gets invoked again and execute the command init.

The code also fixes some issues that could cause misbehaviors when invoking recursively the elevated shell, like an unreleased lock, or a missing file.

it fixes #25523

Fix the check and the automatic installation of WSL2 on Windows when the command `podman machine init` is executed.

pkg/machine/wsl/machine.go

l0rd

.\winmake lint is failing. It's not related to this PR but you need to rebase to current main branch to make it work.

Luap99

I cannot comment on any windows/wsl specifics but the locking doe snot seem right

Luap99 · 2025-05-26T16:00:05Z

pkg/machine/shim/host.go

+	// avoid using defer as the command could be re-executed in elevated mode (on WSL)
+	// and it could get stuck because of the lock
+	machineLock.Unlock()


this is missing an unlock call on the error condition above now.
And it is not clear at all that this is correct, you have just removed a major critical section and I See no reason why things like CreateVM() should not be locked, calls like mc.Write() 100% must be locked and most others.
Unfortunately the locking mode of podman machine is competently undocumented so it is hard to tell what should be locked and what can be done without holding the lock.

thanks for pointing to it. At first i thought it was superfluous as the machine does not exist and there was no risk other operations like start/stop/rm would try to access the same resources... but, effectively, it could happen a double init operation on the same machine or something else i didn't think at.

I tried to refactor it by not revolutionizing the code.
It works like before, except that when init tries to invoke another init in elevated mode, it unlocks sp that the child is free to perform a complete init workflow.

Luap99 · 2025-05-26T16:00:33Z

pkg/machine/shim/host.go

+	err = mp.CreateVM(createOpts, mc, &ignBuilder)
+	if err != nil {
+		return err
+	}


I don't see any reason of why this was moved. IF the order is important then it needs a big comment explaining why.

Added a comment. Basically i want to avoid creating any resources before the CreateVM that could be responsible to invoke a new init that will perform the setup. So i switched the order with the AddSSHConnectionsToPodmanSocket

Luap99 · 2025-05-26T16:03:54Z

pkg/machine/wsl/machine.go

 	}

-	return os.Truncate(name, 0)
+	_, err = os.Create(name)


the function is called truncate but now it no longer truncates which is confusing

renamed 👍

l0rd · 2025-05-26T16:41:43Z

@lstocchi, thank you for this PR. The approach is the right one. I hope you find a way to release the machine lock later in the process (i.e., before restarting in elevated mode), or we should consider another way to check for WSL before the machine lock is acquired. It's also important to find out the different error codes that can be returned when the features are missing (see this comment too), but this is probably less critical.

Adding an end to end test for the WSL installation, that runs in our CI, is complicated. I don't think it's worth it.

packit-as-a-service · 2025-05-27T10:27:47Z

[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore.

lstocchi · 2025-05-27T10:53:19Z

@lstocchi, thank you for this PR. The approach is the right one. I hope you find a way to release the machine lock later in the process (i.e., before restarting in elevated mode), or we should consider another way to check for WSL before the machine lock is acquired.

it's quite a mess with the current workflow. I tried not to change the code too much so if you have any suggestions, feel free to let me know.

It's also important to find out the different error codes that can be returned when the features are missing (see this comment too), but this is probably less critical.

Yes i agree, if we find an agreement on this PR, we could always enhance the list of errors related to WSL features later on when users face them. BTW I'll give a look at the WSL source code to see if i can find out some other known errors

l0rd · 2025-05-27T16:39:15Z

I tried again to run machine init on my laptop with the Virtual Machine Platform disabled, and it behaved as expected: WSL got installed, etc. I will continue the review tomorrow.

l0rd

I have added some new comments. Other suggestions:

avoiding pulling the WSL machine OS image at every invocation of machine init (3 times) but only the first time
better rendering of the progress bars when the Windows features are installed (I am not sure if we can do much)
provide a message to the user mentioning that podman won't be available until Windows is rebooted (instead of showing an error). This is particularly important if the user accepts to install WSL but refuse to boot the machine imediately.

None of these are blocking from my point view for this PR (automatically WSL installation is fixed) but I would apreciate if you could work at least on avoiding returning an error before the reboot.

pkg/machine/define/errors.go

pkg/machine/shim/host.go

pkg/machine/wsl/util_windows.go

lstocchi · 2025-06-03T10:33:14Z

I have added some new comments. Other suggestions:

avoiding pulling the WSL machine OS image at every invocation of machine init (3 times) but only the first time

better rendering of the progress bars when the Windows features are installed (I am not sure if we can do much)

provide a message to the user mentioning that podman won't be available until Windows is rebooted (instead of showing an error). This is particularly important if the user accepts to install WSL but refuse to boot the machine imediately.

None of these are blocking from my point view for this PR (automatically WSL installation is fixed) but I would apreciate if you could work at least on avoiding returning an error before the reboot.

I would open two separate issues for the first 2 points.
Mainly the first one is quite intrusive. I guess it would change the workflow for other OSes as well

pkg/machine/wsl/util_windows.go

this patch changes how the detection of wsl works. The old way of using wsl --status command output to detect some missing features required by WSL is not fully reliable. WSL checks if the wsl feature is enabled and if the vmcompute service do exist. However, this is not enough to identify if the virtual machine platform feature is enabled. The vmcompute service could exist because it has been installed by other tools or it could exist but being stopped. The way proposed by this patch is to try execute the import command and, if it fails, check the error and if it is related to the Host Compute Service try to install all features required by WSL. The flow is the same as before, the user is asked to execute the podman machine init command with elevated privileges. Eventually, after enabling WSL and VMP features, the user is asked to reboot the machine. When the machine restarts, the powershell gets invoked again and execute the command init. The code also fixes some issues that could cause misbehaviors when invoking recursively the elevated shell, like an unreleased lock, or a missing file. Signed-off-by: lstocchi <lstocchi@redhat.com>

l0rd · 2025-06-05T13:41:39Z

/lgtm
/hold

@Luap99 @baude PTAL

@lstocchi you should add a release notes section in the description. Something like:

```release-note
Fix the check and the automatic installation of WSL2 on Windows when the command `podman machine init` is executed.
```

l0rd · 2025-06-13T09:35:32Z

/hold cancel

openshift-ci · 2025-06-13T09:39:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: l0rd, lstocchi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [l0rd]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels May 26, 2025

github-actions bot added the machine label May 26, 2025

lstocchi force-pushed the wsl branch from ad45e2a to 7f41c3e Compare May 26, 2025 14:43

l0rd reviewed May 26, 2025

View reviewed changes

pkg/machine/wsl/machine.go Outdated Show resolved Hide resolved

This comment was marked as duplicate.

Sign in to view

l0rd reviewed May 26, 2025

View reviewed changes

Luap99 reviewed May 26, 2025

View reviewed changes

lstocchi force-pushed the wsl branch 3 times, most recently from ef0bffc to 17bc0ee Compare May 27, 2025 10:27

lstocchi force-pushed the wsl branch from 17bc0ee to 8bfc419 Compare May 27, 2025 10:36

l0rd added the No New Tests Allow PR to proceed without adding regression tests label May 27, 2025

lstocchi force-pushed the wsl branch from 8bfc419 to 3b4fc66 Compare May 28, 2025 15:38

lstocchi marked this pull request as ready for review May 28, 2025 15:38

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 28, 2025

l0rd reviewed Jun 2, 2025

View reviewed changes

pkg/machine/define/errors.go Outdated Show resolved Hide resolved

pkg/machine/shim/host.go Outdated Show resolved Hide resolved

pkg/machine/wsl/util_windows.go Outdated Show resolved Hide resolved

lstocchi force-pushed the wsl branch from 3b4fc66 to baf3db3 Compare June 3, 2025 10:28

lstocchi requested a review from l0rd June 3, 2025 10:33

l0rd reviewed Jun 3, 2025

View reviewed changes

pkg/machine/wsl/util_windows.go Outdated Show resolved Hide resolved

lstocchi force-pushed the wsl branch from baf3db3 to 8532ecb Compare June 4, 2025 12:27

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 5, 2025

openshift-ci bot assigned l0rd Jun 5, 2025

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. release-note and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Jun 5, 2025

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 13, 2025

l0rd approved these changes Jun 13, 2025

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 13, 2025

openshift-merge-bot bot merged commit 60859b0 into containers:main Jun 13, 2025
82 of 83 checks passed

stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 12, 2025

stale-locking-app bot locked as resolved and limited conversation to collaborators Sep 12, 2025

fix wsl install workflow on machine init command #26201

fix wsl install workflow on machine init command #26201

Uh oh!

Conversation

lstocchi commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

This comment was marked as duplicate.

Uh oh!

l0rd left a comment

Choose a reason for hiding this comment

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

Luap99 May 26, 2025

Choose a reason for hiding this comment

Uh oh!

lstocchi May 27, 2025

Choose a reason for hiding this comment

Uh oh!

Luap99 May 26, 2025

Choose a reason for hiding this comment

Uh oh!

lstocchi May 27, 2025

Choose a reason for hiding this comment

Uh oh!

Luap99 May 26, 2025

Choose a reason for hiding this comment

Uh oh!

lstocchi May 27, 2025

Choose a reason for hiding this comment

Uh oh!

l0rd commented May 26, 2025

Uh oh!

packit-as-a-service bot commented May 27, 2025

Uh oh!

lstocchi commented May 27, 2025

Uh oh!

l0rd commented May 27, 2025

Uh oh!

l0rd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lstocchi commented Jun 3, 2025

Uh oh!

Uh oh!

l0rd commented Jun 5, 2025

Uh oh!

l0rd commented Jun 13, 2025

Uh oh!

openshift-ci bot commented Jun 13, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lstocchi commented May 26, 2025 •

edited

Loading