fix: mounting issue with single character volume on windows #25323
Conversation
openshift-ci
bot
added
release-note-none
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: axel7083 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: axel7083 <[email protected]>
axel7083
force-pushed
the
fix/single-character-volume
branch
from
a593ab2 to
021d30c
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestSplitVolumeString(t *testing.T) { |
There was a problem hiding this comment.
tyvm for adding a unit test, kudos
pkg/specgen/volumes.go
Outdated
| @@ -217,6 +218,20 @@ func SplitVolumeString(vol string) []string { | |||
| n = 4 | |||
| } | |||
|
|
|||
| // Determine if the last part is an option (e.g., "ro", "z") | |||
There was a problem hiding this comment.
@mheon really want your review on this ...
|
@axel7083 thanks for the PR ... kudos for adding system tests and unittests. really appreciate that. I did a flyby and it looked with the expectation that others will be doing a more indepth review. |
| 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()) | ||
| }) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I added a test in pkg/machine/e2e/basic_test.go
| @@ -0,0 +1,77 @@ | |||
| package specgen | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I was able to run these tests successfully on Windows
pkg/specgen/volumes.go
Outdated
| // 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.
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,
There was a problem hiding this comment.
I updated the logic, to follow your recommendation
|
Also somewhat related, we should consider blocking creating single letter volumes, docker doesn't allow this ref #23943. That of course is a breaking change so we cannot do that right now so the fix here still is great to have in the meantime but I would generally strongly recommend against using single letter volume names for the problems shown in the isue |
Signed-off-by: axel7083 <[email protected]>
|
is it worth noting somewhere this will likely be deprecated or at least in the man page something that says dont do this. |
|
Code LGTM |
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(volumeCreate).To(Exit(0)) | ||
|
|
||
| run, err := mb.setCmd(bm.withPodmanCommand([]string{"run", "-v", "a:/test:Z", "quay.io/libpod/alpine_nginx"})).run() |
There was a problem hiding this comment.
This is failing. The container default command doesn't return, and the test times out.
You can run the test locally (and I recommend it):
# Build podman
./winmake.ps1 podman
# Install ginkgo
cd ./test/tools
go build -o build/ginkgo.exe ./vendor/github.com/onsi/ginkgo/v2/ginkgo
cd -
# Run the test
$remotetags = "remote exclude_graphdriver_btrfs btrfs_noversion exclude_graphdriver_devicemapper containers_image_openpgp"
$focus_file = "basic_test.go"
$focus_test = "Single character volume mount"
./test/tools/build/ginkgo.exe `
-v --tags "$remotetags" -timeout=90m --trace --no-color `
--focus-file $focus_file `
--focus "$focus_test" `
./pkg/machine/e2e/.| @@ -0,0 +1,77 @@ | |||
| package specgen | |||
There was a problem hiding this comment.
I was able to run these tests successfully on Windows
Fixes #25218
Does this PR introduce a user-facing change?
cc @l0rd