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: mounting issue with single character volume on windows #25323

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion pkg/specgen/volumes.go
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,9 @@ func GenVolumeMounts(volumeFlag []string) (map[string]spec.Mount, map[string]*Na
return mounts, volumes, overlayVolumes, nil
}

// Splits a volume string, accounting for Win drive paths
// SplitVolumeString Splits a volume string, accounting for Win drive paths
// when running as a WSL linux guest or Windows client
// Format: [[SOURCE-VOLUME|HOST-DIR:]CONTAINER-DIR[:OPTIONS]]
func SplitVolumeString(vol string) []string {
parts := strings.Split(vol, ":")
if !shouldResolveWinPaths() {
Expand All @@ -217,6 +218,20 @@ func SplitVolumeString(vol string) []string {
n = 4
}

// Determine if the last part is an option (e.g., "ro", "z")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mheon really want your review on this ...

hasOption := !strings.HasPrefix(parts[len(parts)-1], "/")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment and code are rather confusing, it took me a while to understand what you are doing. I think that is a valid solution but the comment should that we actually check if the last parts is an absolute path. If it is not we know it must be options. Generally speaking there is no need to invert that here at all. Just name the var lastPartisPath and then switch the conditions below. That removes the negation here,

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the logic, to follow your recommendation


// Case: Volume or relative host path (e.g., "vol-name:/container" or "./hello:/container")
if !hasOption && len(parts) == 2 {
return parts
}

// Case: Volume or relative host path with options (e.g., "vol-name:/container:ro" or "./hello:/container:ro")
if hasOption && len(parts) == 3 {
return parts
}

// Case: Windows absolute path (e.g., "C:/Users:/mnt:ro")
if hasWinDriveScheme(vol, n) {
first := parts[0] + ":" + parts[1]
parts = parts[1:]
Expand Down
72 changes: 72 additions & 0 deletions pkg/specgen/volumes_linux_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
package specgen

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestSplitVolumeString(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tyvm for adding a unit test, kudos

tests := []struct {
name string
volume string
expect []string
}{
// relative host paths
{
name: "relative host path",
volume: "./hello:/container",
expect: []string{"./hello", "/container"},
},
{
name: "relative host path with options",
volume: "./hello:/container:ro",
expect: []string{"./hello", "/container", "ro"},
},
// absolute host path
{
name: "absolute host path",
volume: "/hello:/container",
expect: []string{"/hello", "/container"},
},
{
name: "absolute host path with option",
volume: "/hello:/container:ro",
expect: []string{"/hello", "/container", "ro"},
},
{
name: "absolute host path with option",
volume: "/hello:/container:ro",
expect: []string{"/hello", "/container", "ro"},
},
// volume source
{
name: "volume without option",
volume: "vol-name:/container",
expect: []string{"vol-name", "/container"},
},
{
name: "volume with option",
volume: "vol-name:/container:ro",
expect: []string{"vol-name", "/container", "ro"},
},
{
name: "single letter volume without option",
volume: "a:/container",
expect: []string{"a", "/container"},
},
{
name: "single letter volume with option",
volume: "a:/container:ro",
expect: []string{"a", "/container", "ro"},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
parts := SplitVolumeString(tt.volume)

assert.Equal(t, tt.expect, parts, tt.name)
})
}
}
77 changes: 77 additions & 0 deletions pkg/specgen/volumes_windows_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
package specgen
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

friendly remainder that we never run windows or macos unit tests anywhere in CI, someone must hook that up.
I mentioned to @l0rd nefore.

That is nothing against your test and we keep it but a windows maintainer must manually verify them, I don't have access to such machine so I cannot do that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to run these tests successfully on Windows


import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestSplitVolumeString(t *testing.T) {
tests := []struct {
name string
volume string
expect []string
}{
// relative host paths
{
name: "relative host path",
volume: "./hello:/container",
expect: []string{"./hello", "/container"},
},
{
name: "relative host path with options",
volume: "./hello:/container:ro",
expect: []string{"./hello", "/container", "ro"},
},
// absolute host path
{
name: "absolute host path",
volume: "C:\\hello:/container",
expect: []string{"C:\\hello", "/container"},
},
{
name: "absolute host path with option",
volume: "C:\\hello:/container:ro",
expect: []string{"C:\\hello", "/container", "ro"},
},
{
name: "absolute host path with option",
volume: "C:\\hello:/container:ro",
expect: []string{"C:\\hello", "/container", "ro"},
},
{
name: "absolute extended host path",
volume: `\\?\C:\hello:/container`,
expect: []string{`\\?\C:\hello`, "/container"},
},
// volume source
{
name: "volume without option",
volume: "vol-name:/container",
expect: []string{"vol-name", "/container"},
},
{
name: "volume with option",
volume: "vol-name:/container:ro",
expect: []string{"vol-name", "/container", "ro"},
},
{
name: "single letter volume without option",
volume: "a:/container",
expect: []string{"a", "/container"},
},
{
name: "single letter volume with option",
volume: "a:/container:ro",
expect: []string{"a", "/container", "ro"},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
parts := SplitVolumeString(tt.volume)

assert.Equal(t, tt.expect, parts, tt.name)
})
}
}
13 changes: 13 additions & 0 deletions test/e2e/run_volume_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,19 @@ var _ = Describe("Podman run with volumes", func() {
Expect(session).To(ExitWithError(125, fmt.Sprintf("%s: duplicate mount destination", dest)))
})

It("podman run with single character volume", func() {
// 1. create single character volume
session := podmanTest.Podman([]string{"volume", "create", "a"})
session.WaitWithDefaultTimeout()
volName := session.OutputToString()
Expect(session).Should(ExitCleanly())

// 2. create container with volume
session = podmanTest.Podman([]string{"run", "--volume", volName + ":/data", ALPINE, "sh", "-c", "echo hello world"})
session.WaitWithDefaultTimeout()
Expect(session).Should(ExitCleanly())
})
Comment on lines +148 to +159
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is not actually testing your change, in particular the new code path is never executed on normal linux only when run inside the podman WSL machine because shouldResolveWinPaths().

While this test does not hurt and covers the normal linux flow it is not testing your particular bug/code change, this must be tested in the machine test suite pkg/machine/e2e instead.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test in pkg/machine/e2e/basic_test.go


It("podman run with conflict between image volume and user mount succeeds", func() {
err = podmanTest.RestoreArtifact(REDIS_IMAGE)
Expect(err).ToNot(HaveOccurred())
Expand Down