Support volume bind mounts for rootless containers #12687
Conversation
openshift-ci
bot
added
the
approved
|
@mheon PTAL |
libpod/runtime_volume_linux.go
Outdated
| switch key { | ||
| case "device", "o", "type", "UID", "GID", "SIZE", "INODES": | ||
| case "device": | ||
| if _, err := os.Stat(val); err != nil { |
There was a problem hiding this comment.
I think this might be undesirable - e.g. for tmpfs device can be set to anything, so long as the destination path is valid
libpod/volume.go
Outdated
| var rootlessVolume = map[string]bool{ | ||
| "": true, | ||
| "local": true, | ||
| "bind": true, |
There was a problem hiding this comment.
Why do we validate this at all? Rootless users could also use fuse based file systems. Can we just skip the check and handle errors from the mount command?
There was a problem hiding this comment.
That is what I am trying to figure out.
rhatdan
force-pushed
the
volume
branch
4 times, most recently
from
b6a4131 to
8d137ec
Compare
|
@mheon @vrothberg @baude PTAL |
libpod/runtime_volume_linux.go
Outdated
| for key, val := range volume.config.Options { | ||
| switch strings.ToUpper(key) { | ||
| case "DEVICE": | ||
| switch volume.config.Options["type"] { |
There was a problem hiding this comment.
Probably want to ToLower() this to ensure no case-comparison issues
libpod/runtime_volume_linux.go
Outdated
| return nil, errors.Wrapf(err, "invalid volume option %s for driver 'local'", key) | ||
| } | ||
| case "tmpfs": | ||
| if val != "tmpfs" { |
There was a problem hiding this comment.
Do we enforce this? I think this might break existing volumes, tmpfs used to be able to use any string here
libpod/volume_internal_linux.go
Outdated
| @@ -33,7 +33,7 @@ func (v *Volume) mount() error { | |||
| } | |||
|
|
|||
| // We cannot mount 'local' volumes as rootless. | |||
| if !v.UsesVolumeDriver() && rootless.IsRootless() { | |||
| if rootless.IsRootless() && v.RequiresRoot() { | |||
There was a problem hiding this comment.
You need to retain the !UsesVolumeDriver() - that catches volume plugins
There was a problem hiding this comment.
@mheon what drivers are not supported in rootless mode?
I think "local", "bind", "tmpfs" and "" are all supported. If all UserVolumeMounts are supported, or at least allow them to fail themselves. I
There was a problem hiding this comment.
Bind and tmpfs are not drivers, they're methods the local plugin can use to mount.
The local driver (and "", an alias to it) is supported rootless, in a limited fashion - only the kernel mount types that can be used in a rootless user namespace will work.
Any drivers that aren't local are supported (these would be volume plugins), though we make no promises that a given volume plugin actually works with rootless Podman.
There was a problem hiding this comment.
I removed both checks, just allow the mount and unmount commands to fail. Don't try to decide what is supported and what is not.
Fix handling of "bind" and "tmpfs" olumes to actually work. Allow bind, tmpfs local volumes to work in rootless mode. Also removed the string "error" from all error messages that begine with it. All Podman commands are printed with Error:, so this causes an ugly stutter. Fixes: containers#12013 Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
|
LGTM |
|
Before this PR does Following PR fixes this. One side effect which I found is: Dont know if its expected or not but once we Reproducer Try running same example in test
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@flouthoc can you give the steps to reproduce. podman volume create -o type=bind -o device=/somepath vol-name |
github-actions
bot
added
the
locked - please file new issue/PR
Fix handling of "bind" volumes to actually work.
Allow bind volumes to work in rootless mode.
Also removed the string "error" from all error messages that begine with it.
All Podman commands are printed with Error:, so this causes an ugly
stutter.
Fixes: #12013
Signed-off-by: Daniel J Walsh dwalsh@redhat.com