Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix WSL systemd detection #19994

Merged

Conversation

Syquel
Copy link
Contributor

@Syquel Syquel commented Sep 15, 2023

Does this PR introduce a user-facing change?

Fixes the detection of systemd in WSL.
Users who use WSL with systemd must recreate their machine to apply this fix.

If WSL is configured to use systemd by setting boot.systemd=truein /etc/wsl.conf, WSL creates a Symlink from /sbin/init to /usr/lib/systemd/systemd and starts systemd via /sbin/init.
podman machine start, podman machine ssh and other commands check if systemd is running via isSystemdRunning() and fail if it is not detected.

Because isSystemdRunning() incorrectly checks for the command name "/lib/systemd/systemd" instead of the executable path it does not detect systemd running as "/sbin/init".

@Luap99
Copy link
Member

Luap99 commented Sep 15, 2023

@n1hility PTAL

@Syquel Syquel force-pushed the fix-wsl-systemd-detection branch 2 times, most recently from 32b1f89 to 6eb32f9 Compare September 16, 2023 11:45
pkg/machine/wsl/machine.go Outdated Show resolved Hide resolved
@n1hility
Copy link
Member

Thanks for the PR @Syquel !

Is there a use-case you have for wanting to enable the built-in systemd? To support this properly a different bootstrap/start of machine would be required. The reason we don't yet have built-in support for this mode is because of a WSL bug that breaks user systemd services (used by podman), which is only fixed in preview releases (1.3.14+)

@Syquel
Copy link
Contributor Author

Syquel commented Sep 20, 2023

Hi! I have been using WSL with systemd and podman for ~9 months now successfully (even before the official support) and after applying this patch the podman windows client works too.

My use case is the cgroupv2 support provided by systemd.

You are right that simply bootstrapping the WSL machine via podman machine init is not enough at the moment, but with a few minor customizations it works without any issues.
In my opinion podman machine init does way too much at the moment and it's way easier to deploy a "raw" fedora image to WSL and customize it to get postman to run properly with systemd, but that's an unrelated issue.

This patch simply enables the podman windows client to acknowledge that the podman "server" is running on a WSL machine with systemd enabled.
The current commands to determine if systemd is running are incorrect, because they look at the executable name in argv[0] instead of looking at the actually invoked executable.

As stated in my last review comment I am not sure about only checking PID 1 for systemd, but the general approach to check the actually invoked executable should be correct.

// EDIT
Currently I use the podman.exe generate by the build of this PR and I can finally use podman machine start, podman machine ssh etc again without any issues.

[NO NEW TESTS NEEDED]

Signed-off-by: Frederik Boster <[email protected]>
@Syquel Syquel force-pushed the fix-wsl-systemd-detection branch from 6eb32f9 to 5b990c3 Compare September 28, 2023 10:19
@Syquel
Copy link
Contributor Author

Syquel commented Sep 28, 2023

Could someone take a look, please?

@baude
Copy link
Member

baude commented Sep 28, 2023

code looks fine, what say you @n1hility

@TomSweeneyRedHat
Copy link
Member

LGTM
but would like a head nod from @n1hility

@n1hility
Copy link
Member

/lgtm

The change itself is fine so will merge it but until we have formal native systemd support can't promise there won't be other issues (don't consider it supported).

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 28, 2023
@rhatdan
Copy link
Member

rhatdan commented Sep 28, 2023

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 28, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, Syquel

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 28, 2023
@openshift-merge-robot openshift-merge-robot merged commit 744e09d into containers:main Sep 28, 2023
n1hility added a commit to n1hility/podman that referenced this pull request Oct 4, 2023
This reverts commit 5b990c3.
PR containers#19994

Causes wsl nsenter script to infinitely loop in standard operation

[NO NEW TESTS NEEDED]

Signed-off-by: Jason T. Greene <[email protected]>
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Dec 28, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants