Support for Windows paths in the source position of the volume mounts #17113

arixmkii · 2023-01-13T21:52:12Z

Implementation demo isolated https://go.dev/play/p/Do-tBPHESLE (~~Original outdated version~~)

There are 2 things added. First there is added support for handling drive letters while doing value split. If drive letter is detected, then max number of elements will be increased by one, but then first two will be concatenated to reconstruct the path. Second part is basic, but working conversion of Windows path to Unix to be used, when target path is not explicitly specified.

Used regex for drive letter detection, because init would be never used in the hotpath, so, performance wise it affordable.

Signed-off-by: Arthur Sengileyev [email protected]

[NO NEW TESTS NEEDED]

Does this PR introduce a user-facing change?

None

arixmkii · 2023-01-13T21:53:07Z

Added NO NEW TESTS NEEDED, because obviously there are no tests for QEMU machine on Windows as of now and all the code changes are isolated behind runtime.GOOS == "windows" condition.

arixmkii · 2023-01-13T21:57:04Z

Tested with QEMU on Windows enabled build with the following commands for machine init.

// explicitly mounting to /home/core/ps
podman machine init --image-path testing --username core --cpus 4 --memory 8192 -v C:\Temp\PodmanStorage:/home/core/ps
podman machine init --image-path testing --username core --cpus 4 --memory 8192 -v C:/Temp/PodmanStorage:/home/core/ps

// automatically mounting to /C/Temp/PodmanStorage
podman machine init --image-path testing --username core --cpus 4 --memory 8192 -v C:\Temp\PodmanStorage
podman machine init --image-path testing --username core --cpus 4 --memory 8192 -v C:/Temp/PodmanStorage

// automatically mounting to /Temp/PodmanStorage
// usage of pahts w/o drive letter should be strictly discouraged, but it is still a supported option
podman machine init --image-path testing --username core --cpus 4 --memory 8192 -v \Temp\PodmanStorage
podman machine init --image-path testing --username core --cpus 4 --memory 8192 -v /Temp/PodmanStorage

After init machines were always launched and mounts being operational were verifies using ssh into machine.

mheon · 2023-01-17T21:40:52Z

@n1hility @baude PTAL

TomSweeneyRedHat · 2023-01-20T00:51:40Z

pkg/machine/qemu/machine.go

+			if len(paths) > 1 && driveLetterMatcher.MatchString(paths[0]) {
+				paths = strings.SplitN(volume, ":", 4)
+				newpaths := []string{paths[0] + ":" + paths[1]}
+				for _, v := range paths[2:] {


It's late and I can't quickly recall. If paths only has one value in it after the SplitN, i.e. the value in the volume variable had no colons, will this spit up?

There is a check at line 309, that paths will have at least 2 elements len(paths) > 1.

Example of this check isolated https://go.dev/play/p/AhlcW74W5cH

TomSweeneyRedHat · 2023-01-20T00:52:55Z

@arixmkii looks like lint is picking on you:

level=warning msg="[linters_context] sqlclosecheck is disabled because of generics. You can track the evolution of the generics support by following the https://github.com/golangci/golangci-lint/issues/2649."
level=warning msg="[linters_context] wastedassign is disabled because of generics. You can track the evolution of the generics support by following the https://github.com/golangci/golangci-lint/issues/2649."
pkg/machine/qemu/machine.go:312:5: S1011: should replace loop with `newpaths = append(newpaths, paths[2:]...)` (gosimple)
				for _, v := range paths[2:] {
				^
make: *** [Makefile:253: golangci-lint] Error 1

arixmkii · 2023-01-21T16:32:00Z

Had another reading session about paths on Windows and it looks like this version of algorithm will fail for DOS device paths, which could used for referencing files: https://learn.microsoft.com/en-us/dotnet/standard/io/file-path-formats#dos-device-paths I'm not really sure if QEMU will accept them as valid, but I can update the algorithm to handle them (here Regexp will come really handy).

arixmkii · 2023-01-21T17:18:25Z

Added DOS device path support in a separate commit. Will squash if it is accepted or remove it if this is not needed.

Isolated example updated: https://go.dev/play/p/WGT06ek1RmB

n1hility · 2023-01-21T18:08:43Z

pkg/machine/qemu/machine.go

@@ -302,8 +304,24 @@ func (v *MachineVM) Init(opts machine.InitOptions) (bool, error) {
 	for i, volume := range opts.Volumes {
 		tag := fmt.Sprintf("vol%d", i)
 		paths := strings.SplitN(volume, ":", 3)
+		if runtime.GOOS == "windows" {


I know this is a small amount of specific code here, but a general ask from the team is to try to break out any platform-specific code into a build-tag controlled unit (_windows etc), to try to keep code sizes from growing too rapidly.

Sure. Will refactor and take into account for my future contributions.

n1hility · 2023-01-23T21:52:27Z

pkg/machine/qemu/machine.go

 		source := paths[0]
 		target := source
+		if runtime.GOOS == "windows" {


in addition to the above, this should check its even necessary to do so (if paths.len > 1 [ after its been altered above])

Valid point. And simple fix. My idea was to mirror unconditional behavior of the above, but obviously that one was for variable initialization, which is not needed here.

pkg/machine/qemu/machine.go

n1hility · 2023-01-23T22:09:44Z

pkg/machine/qemu/machine.go

+			}
+			target = strings.ReplaceAll(target, ":", "")
+			prefixRemover := regexp.MustCompile(`^//[.?]/`)
+			target = prefixRemover.ReplaceAllString(target, "/")


tiny nit: this could be a match target = target[3:] or ReplaceAllLiteralString

Need to check this. I'm still not as good with Go lang as I would want to. Thank you for the hints.

arixmkii · 2023-01-29T22:59:45Z

Applied some refactoring. Now getting paths and then processing of source, target, options are dedicated functions, which could be shared or platform specific.

Applied some changes to source to target path conversion on Windows. Now algorithm looks more straight forward (at least to me).

replace backslashes with forward ones
replace colon with slash (this is needed to support extremely rare case of relative path for a working directory on another driver - so, C:my\path would become /c/my/path instead of /cmy/path. These form is really discouraged and using relative paths as an argument to mounts would be a bad practice, but it is pretty simple fix anyway.
remove known DOS device prefixes
append leading slash and then eliminate any consecutive multiple slashes //+ replaced with /. They could appear if UNC paths were used (I'm not sure if QEMU will be happy about them, but just to stay on the safe side) https://learn.microsoft.com/en-us/dotnet/standard/io/file-path-formats#unc-paths

arixmkii · 2023-01-29T23:07:47Z

Updated example https://go.dev/play/p/Do-tBPHESLE

arixmkii · 2023-01-29T23:24:16Z

Will fix linters in the morning and resubmit.

rhatdan · 2023-01-30T11:33:27Z

@n1hility PTAL
Do we need this in 4.4?

arixmkii · 2023-01-30T11:51:52Z

Do we need this in 4.4?

Sharing my opinion and giving some more insights.

Code changes are isolated in Windows specific sources (refactored parts should not change current behavior), but Windows parts for now are also cut by the Compiler/Linker process, because QEMU is not yet referenced in the list of available machines for Windows.

From my point of view, changes are safe looking and there is no need to delay it if possible, but there is no any business justification to include it in the current release, because it changes no behavior within the supported feature set.

n1hility · 2023-01-30T19:14:35Z

pkg/machine/qemu/machine_windows.go

+	target := strings.ReplaceAll(paths[0], "\\", "/")
+	target = strings.ReplaceAll(target, ":", "/")
+	target = strings.ReplaceAll(target, "//./", "")
+	target = strings.ReplaceAll(target, "//?/", "")


A minor nit, but not a blocker, is that these namespace prefixes can only occur at the start of the string, so you could just do something like:

if strings.HasPrefix(path, "//./") || strings.HasPrefix(path, "//?/") { path = path[4:] }

Thanks. This is better indeed and more correct as we are eliminating only prefixes.

Applied as part of the same commit. Last line was changed to fix output of gofmt -d -s.

@@ -42,10 +42,11 @@ func extractTargetPath(paths []string) string { if len(paths) > 1 { return paths[1] } target := strings.ReplaceAll(paths[0], "\\", "/") target = strings.ReplaceAll(target, ":", "/") - target = strings.ReplaceAll(target, "//./", "") - target = strings.ReplaceAll(target, "//?/", "") + if strings.HasPrefix(target, "//./") || strings.HasPrefix(target, "//?/") { + target = target[4:] + } dedup := regexp.MustCompile(`//+`) - return dedup.ReplaceAllLiteralString("/" + target, "/") + return dedup.ReplaceAllLiteralString("/"+target, "/") }

n1hility · 2023-01-30T20:54:55Z

Other than the above nit LGTM.

n1hility · 2023-01-30T21:03:03Z

@arixmkii the reason @rhatdan is asking is we have already branched v4.4, so should this be backported after merge? Since as you mention it's not yet usable, I think we can skip the backport and target for a later 4.x, when the other patches land.

There are 2 things added. First there is added support for handling drive letters while doing value split. If drive letter is detected, then max number of elements will be increased by one, but then first two will be concatenated to reconstruct the path. Second part is basic, but working, conversion of Windows path to Unix path to be used, when target path is not explicitly specified. Signed-off-by: Arthur Sengileyev <[email protected]>

arixmkii · 2023-01-30T21:39:10Z

@n1hility Thank you for the context and explaining some background processes to the community contributors. I noticed this already, when my gvproxy/installer PR was cherry-picked for the release branch. Thus, I tried to explain my opinion, that there is no need to backport this changes to the release branch, because they are more of a ground for some future work.

arixmkii · 2023-02-28T06:54:51Z

@rhatdan @n1hility can we move forward with it? It should have no conflicts with latest head, but I can rebase and force push if needed.

rhatdan · 2023-02-28T11:50:50Z

/approve
/lgtm

openshift-ci · 2023-02-28T11:51:00Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [rhatdan]

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 the release-note-none label Jan 13, 2023

arixmkii force-pushed the windows-qemu-machine-volume-mounts branch 2 times, most recently from 74c591a to 3d73606 Compare January 13, 2023 22:03

TomSweeneyRedHat reviewed Jan 20, 2023

View reviewed changes

arixmkii force-pushed the windows-qemu-machine-volume-mounts branch from 3d73606 to 44a0667 Compare January 21, 2023 15:46

arixmkii force-pushed the windows-qemu-machine-volume-mounts branch from 922b1b6 to 6881ca3 Compare January 21, 2023 17:30

n1hility reviewed Jan 23, 2023

View reviewed changes

arixmkii force-pushed the windows-qemu-machine-volume-mounts branch from 6881ca3 to 4ab3beb Compare January 29, 2023 22:52

arixmkii force-pushed the windows-qemu-machine-volume-mounts branch from 4ab3beb to d9b9248 Compare January 29, 2023 23:02

arixmkii requested a review from n1hility January 29, 2023 23:09

arixmkii force-pushed the windows-qemu-machine-volume-mounts branch 2 times, most recently from fcc60c0 to ec39837 Compare January 30, 2023 09:56

n1hility reviewed Jan 30, 2023

View reviewed changes

arixmkii force-pushed the windows-qemu-machine-volume-mounts branch from ec39837 to 952049f Compare January 30, 2023 21:33

arixmkii mentioned this pull request Feb 10, 2023

Support Qemu accelerated podman machine on Windows hosts #13006

Open

arixmkii mentioned this pull request Feb 12, 2023

[Feature]: QEMU Machine should support Windows Source paths for 9pfs on Windows hosts #17098

Closed

openshift-ci bot assigned rhatdan Feb 28, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 28, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 28, 2023

openshift-merge-robot merged commit 77504b2 into containers:main Feb 28, 2023

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 Sep 7, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 7, 2023

arixmkii deleted the windows-qemu-machine-volume-mounts branch March 1, 2024 13:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for Windows paths in the source position of the volume mounts #17113

Support for Windows paths in the source position of the volume mounts #17113

arixmkii commented Jan 13, 2023 •

edited

Loading

arixmkii commented Jan 13, 2023 •

edited

Loading

arixmkii commented Jan 13, 2023

mheon commented Jan 17, 2023

TomSweeneyRedHat Jan 20, 2023

arixmkii Jan 21, 2023

TomSweeneyRedHat commented Jan 20, 2023

arixmkii commented Jan 21, 2023

arixmkii commented Jan 21, 2023 •

edited

Loading

n1hility Jan 21, 2023

arixmkii Jan 23, 2023

n1hility Jan 23, 2023

arixmkii Jan 23, 2023

n1hility Jan 23, 2023

arixmkii Jan 23, 2023

arixmkii commented Jan 29, 2023

arixmkii commented Jan 29, 2023

arixmkii commented Jan 29, 2023

rhatdan commented Jan 30, 2023

arixmkii commented Jan 30, 2023

n1hility Jan 30, 2023

arixmkii Jan 30, 2023

n1hility commented Jan 30, 2023

n1hility commented Jan 30, 2023 •

edited

Loading

arixmkii commented Jan 30, 2023

arixmkii commented Feb 28, 2023

rhatdan commented Feb 28, 2023

openshift-ci bot commented Feb 28, 2023

Support for Windows paths in the source position of the volume mounts #17113

Support for Windows paths in the source position of the volume mounts #17113

Conversation

arixmkii commented Jan 13, 2023 • edited Loading

Does this PR introduce a user-facing change?

arixmkii commented Jan 13, 2023 • edited Loading

arixmkii commented Jan 13, 2023

mheon commented Jan 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomSweeneyRedHat commented Jan 20, 2023

arixmkii commented Jan 21, 2023

arixmkii commented Jan 21, 2023 • edited Loading

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

arixmkii commented Jan 29, 2023

arixmkii commented Jan 29, 2023

arixmkii commented Jan 29, 2023

rhatdan commented Jan 30, 2023

arixmkii commented Jan 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1hility commented Jan 30, 2023

n1hility commented Jan 30, 2023 • edited Loading

arixmkii commented Jan 30, 2023

arixmkii commented Feb 28, 2023

rhatdan commented Feb 28, 2023

openshift-ci bot commented Feb 28, 2023

arixmkii commented Jan 13, 2023 •

edited

Loading

arixmkii commented Jan 13, 2023 •

edited

Loading

arixmkii commented Jan 21, 2023 •

edited

Loading

n1hility commented Jan 30, 2023 •

edited

Loading