-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() { | ||
|
@@ -217,6 +218,20 @@ func SplitVolumeString(vol string) []string { | |
n = 4 | ||
} | ||
|
||
// Determine if the last part is an option (e.g., "ro", "z") | ||
hasOption := !strings.HasPrefix(parts[len(parts)-1], "/") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:] | ||
|
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
package specgen | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a test in |
||
|
||
It("podman run with conflict between image volume and user mount succeeds", func() { | ||
err = podmanTest.RestoreArtifact(REDIS_IMAGE) | ||
Expect(err).ToNot(HaveOccurred()) | ||
|
There was a problem hiding this comment.
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 ...