Use the WSL convention for the windows machines default volumes by l0rd · Pull Request #2100 · containers/common

l0rd · 2024-07-25T13:29:56Z

Currently on Windows, volumes are mounted differently based on the provider:

WSL automatically mounts the main drive C:/ under /mnt/c on every Linux VM
For HyperV instead, podman mounts C:\Users\name under /Users/name

Issues like this one are the consequence of such behavior.

With this PR getDefaultMachineVolumes() is updated to be consistent with WSL default behavior. That means that on both WSL and HyperV machines, the default volumes will be the %userprofile% drive (e.g. C:/) mounted under /mnt/ (e.g. /mnt/c).

The old Hyper-V mount is still supported. Users can still use the notation podman run -v /Users when the provider is Hyper-V although the notation podman run -v C:\Users should be preferred.

Luap99

This is likely the right thing but it also a breaking change as hyper V users might depend on the current paths and I am not sure how we could deal with that.

l0rd · 2024-07-25T15:25:33Z

This is likely the right thing but it also a breaking change as hyper V users might depend on the current paths and I am not sure how we could deal with that.

This is very true but volumes, with the default mount config, were kind of broken on Hyper-V.

For example:

podman run -v C:\Users\username\folder:/test fedora

fails to start.

So I am wondering, how many podman users are using Hyper-V with the default mount config? Not a lot I guess. But the more we wait there more there will be.

And for those that have a custom mount config (as suggested in the issue for example), this PR doesn't affect them.

Luap99 · 2024-07-25T15:46:29Z

podman run -v /Users/username/folder:/test fedora

But this should work with the current default I assume? And if we have users using that we break them.

I mean overall the number of hyperV users is most likely very small compared to WSL so maybe it is the right thing to do it now to make the volume behaviour consistent and just let people deal with it. Although is there any way we can make it backwards compatible?

Maybe we could add two mounts C:\Users\name:/User/name and C:\Users\name:/mnt/c/Users/name? Or we could configure the machine to have symlink from the old path to the new one but that would require a bunch of extra code.

l0rd · 2024-07-25T15:59:41Z

podman run -v /Users/username/folder:/test fedora
But this should work with the current default I assume? And if we have users using that we break them.

Yes you are correct. That works and users that use that will get broken

I mean overall the number of hyperV users is most likely very small compared to WSL so maybe it is the right thing to do it now to make the volume behaviour consistent and just let people deal with it. Although is there any way we can make it backwards compatible?

Maybe we could add two mounts C:\Users\name:/User/name and C:\Users\name:/mnt/c/Users/name? Or we could configure the machine to have symlink from the old path to the new one but that would require a bunch of extra code.

I have tested adding both mount configs (C:\Users\username:/Users/username and C:\:/mnt/c) and, with that, both formats work. And the code is still pretty simple.

That's not an obvious choice. Do we want to force users to move to a consistent volume format or should we keep maintaining the support for those two inconsistent formats on windows? Considered that Hyper-V is still not widely used I would lean towards the first option. But I am happy to update the PR if the consensus is for the second option. @n1hility @baude what's your opinion on that?

l0rd · 2024-07-29T14:32:06Z

I have pushed a commit with the alternative approach (keeping the old hyper-v default mount and adding a new WSL-consistent mount) to this branch l0rd@2dfaccc.

n1hility · 2024-07-30T03:05:08Z

I think its fine to keep support for both formats you can always deprecate one and remove the code later.

jeffmaury

Shouldn't we scan all local drives (C: D: ...) ?

baude · 2024-07-30T11:30:10Z

im ok with the option to support both ...

l0rd · 2024-07-30T13:09:54Z

@n1hility @baude @Luap99 thank you for your review. I have updated the PR so that both notations are supported on Hyper-V (-v /Users/ and -v C:\Users\). The unit test is updated to test both. Tests are passing.

@jeffmaury thank for looking at this too. This PR addresses an Hyper-V / WSL inconsistency. Supporting -v <arbitrary-drive>:/ is out of scope and worth a (separate) discussion.

Luap99 · 2024-07-30T15:18:30Z

pkg/config/default_windows.go

+// behavior where the host %SystemDrive% (e.g. C:\) is automatically mounted
+// in the guest under /mnt/ (e.g. /mnt/c/)


The comment doesn't quite match the code, we mount the drive where the users homedir is located.
I don't know if there is a way for a homedir to be on a different disk compared to C: (%SystemDrive%) in practise (at least super unlikely) so not really that important I guess but maybe we should actually lookup the SystemDrive env?

Technically the drive of %USERPROFILE% can be different from %SYSTEMDRIVE%. But I still think we should mount %USERPROFILE% drive rather than %SYSTEMDRIVE% as user files are typically there. I have updated the comment.

Luap99 · 2024-07-30T15:20:40Z

pkg/config/default_windows_test.go

+)
+
+func TestGetDefaultMachineVolumes(t *testing.T) {
+	t.Parallel()


this is incompatible with setenv so you need to remove this.

Luap99 · 2024-07-30T15:21:19Z

pkg/config/default_windows_test.go

+		},
+	} {
+		for _, env := range tc.envs {
+			os.Setenv(env[0], env[1])


use t.Setenv() then go should rightfully error out and tell you using setenv with parallel is not supported.

Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>

l0rd · 2024-07-30T17:35:47Z

@Luap99 thank you for the review. I have updated the PR taking into account your feedback.

Luap99

LGTM

openshift-ci · 2024-07-30T17:48:09Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [Luap99]

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

l0rd · 2024-08-06T08:22:39Z

@baude @containers/podman-maintainers PTAL

rhatdan · 2024-08-06T11:53:16Z

@baude @n1hility @ashley-cui PTAL

baude · 2024-08-06T13:36:16Z

/lgtm

This was referenced Jul 25, 2024

Stop skipping machine volume test on Hyper-V containers/podman#23401

Merged

Cannot mount local folder with HyperV containers/podman#21036

Closed

Luap99 reviewed Jul 25, 2024

View reviewed changes

l0rd mentioned this pull request Jul 29, 2024

Error running podman build with option --build-context on Hyper-V containers/podman#23429

Open

jeffmaury reviewed Jul 30, 2024

View reviewed changes

l0rd force-pushed the windows-machines-default-volumes branch from 0f3ba1a to 0282f33 Compare July 30, 2024 12:50

Luap99 reviewed Jul 30, 2024

View reviewed changes

Use the WSL convention for the windows machines default volumes

2a99bbd

Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>

l0rd force-pushed the windows-machines-default-volumes branch from 0282f33 to 2a99bbd Compare July 30, 2024 17:34

Luap99 approved these changes Jul 30, 2024

View reviewed changes

openshift-ci bot added the approved label Jul 30, 2024

openshift-ci bot assigned baude Aug 6, 2024

openshift-ci bot added the lgtm label Aug 6, 2024

openshift-merge-bot bot merged commit 05b2e1f into containers:main Aug 6, 2024

wbrefvem mentioned this pull request Aug 28, 2024

Clarify podman machine volume mounting behavior under WSL containers/podman#23779

Merged

		// behavior where the host %SystemDrive% (e.g. C:\) is automatically mounted
		// in the guest under /mnt/ (e.g. /mnt/c/)

Conversation

l0rd commented Jul 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

l0rd commented Jul 25, 2024

Uh oh!

Luap99 commented Jul 25, 2024

Uh oh!

l0rd commented Jul 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

l0rd commented Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n1hility commented Jul 30, 2024

Uh oh!

jeffmaury left a comment

Choose a reason for hiding this comment

Uh oh!

baude commented Jul 30, 2024

Uh oh!

l0rd commented Jul 30, 2024

Uh oh!

Luap99 Jul 30, 2024

Choose a reason for hiding this comment

Uh oh!

l0rd Jul 30, 2024

Choose a reason for hiding this comment

Uh oh!

Luap99 Jul 30, 2024

Choose a reason for hiding this comment

Uh oh!

Luap99 Jul 30, 2024

Choose a reason for hiding this comment

Uh oh!

l0rd commented Jul 30, 2024

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Jul 30, 2024

Uh oh!

l0rd commented Aug 6, 2024

Uh oh!

rhatdan commented Aug 6, 2024

Uh oh!

baude commented Aug 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

l0rd commented Jul 25, 2024 •

edited

Loading

l0rd commented Jul 25, 2024 •

edited

Loading

l0rd commented Jul 29, 2024 •

edited

Loading