Add podman volume mount support #13318

rhatdan · 2022-02-22T20:06:21Z

Signed-off-by: Daniel J Walsh [email protected]

edsantiago

Nice new feature. Some suggestions for the tests.

edsantiago · 2022-02-22T20:13:38Z

test/system/160-volumes.bats

+	touch $mnt/$myfile
+        run_podman run -v ${myvolume}:/vol1:z $IMAGE ls /vol1
+	is "$output" "$myfile" "$myfile should exists within the containers volume"


inconsistent indentation; mix of tabs and spaces. Also, s/exists/exist/

Added a small test for rootless.

edsantiago · 2022-02-22T20:14:46Z

test/system/160-volumes.bats

@@ -387,4 +387,27 @@ NeedsChown    | true
    run_podman volume rm $myvolume
 }

+@test "podman volume mount" {
+    skip_if_remote "podman --remote volume mount not supported"


Test doesn't really do anything when rootless. Why not just skip_if_rootless?

test/system/160-volumes.bats

edsantiago · 2022-02-22T20:24:14Z

oh, and linter is unhappy

pkg/domain/infra/abi/volumes.go:182:1: receiver name ir should be consistent with previous receiver name ic for ContainerEngine (golint)
func (ir *ContainerEngine) VolumeMount(ctx context.Context, nameOrIDs []string) ([]*entities.ContainerMountReport, error) {
^
pkg/domain/infra/abi/volumes.go:198:1: receiver name ir should be consistent with previous receiver name ic for ContainerEngine (golint)
func (ir *ContainerEngine) VolumeUnmount(ctx context.Context, nameOrIDs []string) ([]*entities.ContainerUnmountReport, error) {
^
pkg/domain/infra/abi/auto-update.go:10:28: ST1016: methods on the same type should have the same receiver name (seen 2x "ir", 97x "ic") (stylecheck)
func (ic *ContainerEngine) AutoUpdate(ctx context.Context, options entities.AutoUpdateOptions) ([]*entities.AutoUpdateReport, []error) {
                           ^
make: *** [Makefile:251: golangci-lint] Error 1

docs/source/markdown/podman-volume-mount.1.md

docs/source/markdown/podman-volume-unmount.1.md

mheon · 2022-02-23T18:04:35Z

libpod/volume.go

@@ -255,3 +255,11 @@ func (v *Volume) IsDangling() (bool, error) {
 func (v *Volume) UsesVolumeDriver() bool {
 	return !(v.config.Driver == define.VolumeDriverLocal || v.config.Driver == "")
 }
+
+func (v *Volume) Mount() (string, error) {
+	return v.config.MountPoint, v.mount()


Need to v.lock.Lock(); defer v.lock.Unlock() here and Unmount

Thinking about this a bit more - v.config.MountPoint may not be populated until v.mount() runs (I think this is mostly a volume plugins thing), so it's safer to do err := v.mount(); return v.config.MountPoint, err

mheon · 2022-02-23T18:06:13Z

If you do a podman run using a volume, then do a podman volume unmount on it when the container is running - does that break anything? Want to make sure we don't need to add extra checks around that.

rhatdan · 2022-02-23T19:54:43Z

Since it is in a different mount namespace the unmount will not effect the running container.

Tested this out with a tmpfs container.

mheon · 2022-02-23T20:43:35Z

That isn't true? The volume mounts are made before the container is created, and then we bind-mount the mountpoint into the container - so there is no difference between a container requesting an unmount, and a user requesting an unmount, save for the fact that the container is still potentially running. I'm worried that we'll still set the mount counter to 0 if the unmount fails because the volume is in use, which would result in the volume being considered unmounted when it is still mounted.

rhatdan · 2022-02-23T21:19:12Z

What I see is the mount point in the podman unshare aria shows the tmpfs is no longer mounted, but if I exec into the container I see the tmpfs is still mounted and the data I created on it it is still available. This is because in the containers mount namespace the file system is still mounted.
If I run another container on the volume, I will see different content because the tmpfs will get mounted again.

edsantiago · 2022-02-23T22:18:58Z

@mheon from my testing, podman volume unmount is a NOP. Actually, volume mount is, too, all it does is display a path to an existing volume. It does not seem to increment refcnt. podman is perfectly happy to volume rm while I still "have" the volume "mounted":

$ sudo bin/podman volume mount myvolume
/var/lib/containers/storage/volumes/myvolume/_data
$ sudo touch files in there, etc etc
$ sudo bin/podman volume rm myvolume
myvolume
$ sudo ls /var/lib/containers/storage/volumes/myvolume
ls: cannot access '/var/lib/containers/storage/volumes/myvolume': No such file or directory

mheon · 2022-02-23T22:32:02Z

@edsantiago Did you make the volume with options that would require a mount take place - e.g. podman volume create --opt type=tmpfs --opt device=tmpfs myvolume or similar (options may not work perfectly, I'm going off memory here)?

edsantiago · 2022-02-23T22:39:07Z

Uh, no, it was just a plain simple volume create. I was just surprised that podman pulled the rug out from under me while I thought I had the volume "mounted". Not a big deal though.

mheon · 2022-02-23T23:16:52Z

Ah - yeah, it's a short-circuit operation unless the volume actually needs a mount. Should probably make that clear in the manpages.

It should always be safe to invoke mount and unmount, even on volumes that don't require a mount, to be clear; it's just that some volumes don't require them, and others do. I think the recommended workflow after the patches would always involve using them.

rhatdan · 2022-02-24T01:18:06Z

I think the bind mount happens after the tmpfs mount which causes the tmpfs mount to leak into the mount namespace.
Bottom line I did the following

$ podman unshare
# ./bin/podman volume create --opt device=tmpfs --opt type=tmpfs --opt o=uid=1000,gid=1000 testvol
# ./bin/podman run -d -v testvol:/vol:Z fedora sleep 1000
52fee416c7a0ac40f085654f4564693d4b5ffc181ec48c174e272378f0283fab
# mount | grep _data
tmpfs on /home/dwalsh/.local/share/containers/storage/volumes/testvol/_data type tmpfs (rw,relatime,seclabel,uid=100999,gid=100999,inode64)
# ./bin/podman exec -l touch /vol/dan
# ./bin/podman volume unmount testvol
testvol
# mount | grep _data
# ./bin/podman exec -l ls /vol
dan

cdoern · 2022-02-28T14:49:21Z

pkg/domain/infra/abi/volumes.go

@@ -178,3 +178,35 @@ func (ic *ContainerEngine) VolumeMounted(ctx context.Context, nameOrID string) (
 	}
 	return &entities.BoolReport{Value: false}, nil
 }
+
+func (ic *ContainerEngine) VolumeMount(ctx context.Context, nameOrIDs []string) ([]*entities.ContainerMountReport, error) {


Do we want to use ContainerMountReport or should we make a new type for VolumeMount for consistency?

cdoern · 2022-02-28T14:49:35Z

pkg/domain/infra/abi/volumes.go

+	return reports, nil
+}
+
+func (ic *ContainerEngine) VolumeUnmount(ctx context.Context, nameOrIDs []string) ([]*entities.ContainerUnmountReport, error) {


Same as above, type consistency.

Fixes: containers#12768 Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan · 2022-02-28T23:42:14Z

@containers/podman-maintainers PTAL

giuseppe

LGTM

openshift-ci · 2022-03-01T08:43:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, giuseppe, 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:

~~OWNERS~~ [edsantiago,giuseppe,rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vrothberg

/lgtm

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 22, 2022

edsantiago approved these changes Feb 22, 2022

View reviewed changes

rhatdan force-pushed the volume branch from a69f8e3 to 76146ed Compare February 22, 2022 20:42

TomSweeneyRedHat reviewed Feb 23, 2022

View reviewed changes

docs/source/markdown/podman-volume-mount.1.md Outdated Show resolved Hide resolved

TomSweeneyRedHat reviewed Feb 23, 2022

View reviewed changes

docs/source/markdown/podman-volume-unmount.1.md Outdated Show resolved Hide resolved

rhatdan force-pushed the volume branch from 7ae7155 to 76a6b22 Compare February 23, 2022 18:03

mheon reviewed Feb 23, 2022

View reviewed changes

rhatdan force-pushed the volume branch 2 times, most recently from e7ce688 to 026821e Compare February 23, 2022 19:54

rhatdan force-pushed the volume branch from 026821e to 8404677 Compare February 24, 2022 01:24

cdoern reviewed Feb 28, 2022

View reviewed changes

Add podman volume mount support

3dc1b8e

Fixes: containers#12768 Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the volume branch from 8404677 to 3dc1b8e Compare February 28, 2022 19:26

giuseppe approved these changes Mar 1, 2022

View reviewed changes

vrothberg reviewed Mar 1, 2022

View reviewed changes

openshift-ci bot assigned vrothberg Mar 1, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 1, 2022

openshift-merge-robot merged commit 87d22e1 into containers:main Mar 1, 2022

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 21, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add podman volume mount support #13318

Add podman volume mount support #13318

rhatdan commented Feb 22, 2022

edsantiago left a comment

edsantiago Feb 22, 2022

rhatdan Feb 22, 2022

edsantiago Feb 22, 2022

edsantiago commented Feb 22, 2022

mheon Feb 23, 2022

mheon Feb 23, 2022

mheon commented Feb 23, 2022

rhatdan commented Feb 23, 2022

mheon commented Feb 23, 2022

rhatdan commented Feb 23, 2022

edsantiago commented Feb 23, 2022

mheon commented Feb 23, 2022

edsantiago commented Feb 23, 2022

mheon commented Feb 23, 2022

rhatdan commented Feb 24, 2022

cdoern Feb 28, 2022

cdoern Feb 28, 2022

rhatdan commented Feb 28, 2022

giuseppe left a comment

openshift-ci bot commented Mar 1, 2022

vrothberg left a comment

Add podman volume mount support #13318

Add podman volume mount support #13318

Conversation

rhatdan commented Feb 22, 2022

edsantiago left a comment

Choose a reason for hiding this comment

edsantiago Feb 22, 2022

Choose a reason for hiding this comment

rhatdan Feb 22, 2022

Choose a reason for hiding this comment

edsantiago Feb 22, 2022

Choose a reason for hiding this comment

edsantiago commented Feb 22, 2022

mheon Feb 23, 2022

Choose a reason for hiding this comment

mheon Feb 23, 2022

Choose a reason for hiding this comment

mheon commented Feb 23, 2022

rhatdan commented Feb 23, 2022

mheon commented Feb 23, 2022

rhatdan commented Feb 23, 2022

edsantiago commented Feb 23, 2022

mheon commented Feb 23, 2022

edsantiago commented Feb 23, 2022

mheon commented Feb 23, 2022

rhatdan commented Feb 24, 2022

cdoern Feb 28, 2022

Choose a reason for hiding this comment

cdoern Feb 28, 2022

Choose a reason for hiding this comment

rhatdan commented Feb 28, 2022

giuseppe left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 1, 2022

vrothberg left a comment

Choose a reason for hiding this comment