- Notifications
You must be signed in to change notification settings - Fork2.9k
macOS: resolve volume source symlinks to fix /tmp mounts#27622
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
Signed-off-by: Bruce Fan <brucexfan@gmail.com>
Signed-off-by: Bruce Fan <brucexfan@gmail.com>
[APPROVALNOTIFIER] This PR isNOT APPROVED This pull-request has been approved by:Banana-Cultist The full list of commands accepted by this bot can be foundhere. Needs approval from an approver in each of these files: Approvers can indicate their approval by writing |
Honny1 left a comment
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.
I have just one nit about the test. Also, I realized that this issue also impacts thelocal API, but it is unrelated to this PR. I will create an issue for that.
| ) | ||
| func TestResolveVolumeSourcePathTmpSymlink(t *testing.T) { | ||
| dir, err := os.MkdirTemp("/tmp", "podman-vol-") |
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.
Uset.TempDir() for automatic cleanup. According to theimplementation, this callsMkdirTemp using the default temporary path derived from theTMPDIR environment variable (MkdirTemp,TempDir() string)). You can uset.Setenv to force this to/tmp.
| dir,err:=os.MkdirTemp("/tmp","podman-vol-") | |
| dir:=t.TempDir() |
Luap99 left a comment
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.
Thanks, but I am not sure this make sense.
First of all this changes the behavior for macos only making it rather confusing if you compare it to podman-remote on linux.
But second, most importantly this is a a breaking change. In general there is not direct relation between the path on the client and on the server. The actual mounts happen server side so if we resolve the path on the client it means it is impossible to mount/tmp/somedir on the server because the client always resolves it first.
On the other hand if we don't resolve like currently we pass the path as is and it works just fine. And if a users wants to resolve the link they can just do it themselves before calling podman.
Our docs never claim to mount /tmp with podman machine, we do mount /private so I think saying we must fix /tmp mounts is wrong. There is also no guarantee that the symlink target exists on the server.
As such I would recommend to close the issue as wontifx but I am interesting in@baude@mheon@l0rd and@ashley-cui opinions on this.
This PR fixes an issue where mounting volumes from directories that are symbolic links on the host (specifically /tmp on macOS) would fail or result in mounting the wrong directory inside the VM.
On macOS, /tmp is a symlink to /private/tmp. When a user runs podman run -v /tmp/foo:/mnt ..., the Podman client sends the path /tmp/foo to the Linux VM. The Linux VM has its own /tmp directory while also mounting /private from the host. This results in a statfs error or the container mounting the VM's local /tmp instead of the user's intended host directory (/private/tmp).
This PR updates the volume spec generation logic in pkg/specgen to resolve symbolic links for absolute host paths on macOS before the spec is sent to the backend. This ensures that /tmp/foo is converted to /private/tmp/foo, which correctly maps to the shared file system mount inside the Podman machine.
A new unit test TestResolveVolumeSourcePathTmpSymlink has been added in pkg/specgen.
I also manually verified the fix using a
podman-remotebinary built from this branch:Reproduction Command:
Before:
Error: statfs /tmp/test-vol/data: no such file or directoryAfter:
hello.txtFixes#27468
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. SeeCONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?