-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Add Warning for Sharing Volumes Not Mounted to Machine #27767
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
base: main
Are you sure you want to change the base?
Add Warning for Sharing Volumes Not Mounted to Machine #27767
Conversation
Signed-off-by: Bruce Fan <[email protected]>
Signed-off-by: Bruce Fan <[email protected]>
Signed-off-by: Bruce Fan <[email protected]>
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 like the idea of adding a warning. I have a suggestion to make this more DRY, but I have some concerns about symlink evaluation. For example, on Mac, /tmp is actually /private/tmp.
|
@baude Feelings on this one? |
Signed-off-by: Bruce Fan <[email protected]>
What are you thinking of for symlink evaluation? From my understanding, symlinks aren't evaluated in the actual volume share codepath. If we evaluated symlinks before checking their machine path, then the original example would have its path evaluated to Conversely, if we evaluated symlinks after checking the machine path, I think the warning would say that I could definitely be missing something, though. |
I checked the implementation, and it seems that |
Signed-off-by: Bruce Fan <[email protected]>
|
If I'm understanding this correctly, then trying to mount from "./filepath" and "/filepath" could result in two different outcomes. In fact, by being at the root directory, I've updated the warning check with the eval symlinks, but the behavior I described above feels weird, so I wanted to check in. |
|
|
||
| // WarnIfMachineVolumesUnavailable inspects bind mounts requested via --volume | ||
| // and warns if the source paths are not shared with the active Podman machine. | ||
| func WarnIfMachineVolumesUnavailable(machineMode bool, connectionURI string, volumeSpecs []string) { |
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 would wrap CheckPathOnRunningMachine inside the WarnIfMachineVolumesUnavailable function. You can get the context with registry.context(). This will simplify the code for the caller and reduce the overall function size.
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.
When I tried this, it seemed that CheckPathOnRunningMachine didn't recognize that podman was running in machine mode. I have a hunch that the registry context at the current call site isn't fully set up at that point.
Signed-off-by: Bruce Fan <[email protected]>
On macOS and Windows, podman runs in remote mode, which uses a VM to run containers. Volume sharing is done using this VM's filesystem, rather than the host filesystem. However, since the VM default mounts include some common folders, this might not be clear to some users.
To address this, when a user tries to share a volume that isn't mounted to the podman machine, a warning is generated linking to the podman machine docs.
Example (macOS):
Fixes: #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. See CONTRIBUTING.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?