Fix volume handling in podman #2229

rhatdan · 2019-01-29T15:42:53Z

Fix builtin volumes to work with podman volume

Currently builtin volumes are not recored in podman volumes when
they are created automatically. This patch fixes this.

Remove container volumes when requested

Currently the --volume option on podman remove does nothing.
This will implement the changes needed to remove the volumes
if the user requests it.

When removing a volume make sure that no container uses the volume.

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

openshift-ci-robot · 2019-01-29T15:43:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [rhatdan]

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

TomSweeneyRedHat · 2019-01-29T16:06:27Z

libpod/runtime_ctr.go

+	if removeVolume {
+		volumes, err = c.namedVolumes()
+		if err != nil {
+			logrus.Errorf("unable to retreive builtin volumes: %v", err)


s/retreive/retrieve

It would probably be good to add the container id to the message:
`logrus.Errorf("unable to retrieve builtin volumes %v for container %v", err, c.ID())

TomSweeneyRedHat · 2019-01-29T16:09:43Z

@rhatdan looks like you've some test unhappiness

TomSweeneyRedHat · 2019-01-29T17:46:24Z

libpod/container_internal.go

+		if err != nil {
+			return errors.Wrapf(ErrCtrStateInvalid, "failed to create volume for %s in %s for container %s", k, mountPoint, c.ID())
+		}
+		volumePath := filepath.Join(volume.config.MountPoint, k)


Even better error, ty.

TomSweeneyRedHat

LGTM, assuming happy tests

rhatdan · 2019-01-29T18:03:44Z

@umohnani8 PTAL
Fixes #2170

rhatdan · 2019-01-29T18:41:39Z

/test images

tvass · 2019-01-29T18:51:05Z

Hi, I have tested this PR and this looks good to me. Volume is now listed under "podman volume ls" while the container is running.

However, the created volume is NOT always removed when the container exits (but was launched with --rm option).

ie:

$ podman volume prune
WARNING! This will remove all volumes not used by at least one container.
Are you sure you want to continue? [y/N] y
$ podman run --rm -ti test sh -c "sleep 5"
$ podman run --rm -ti test sh -c "sleep 5"
$ podman run --rm -ti test sh -c "sleep 5"
$ podman run --rm -ti test sh -c "sleep 5"
$ podman run --rm -ti test sh -c "sleep 5"
$ podman volume ls 
DRIVER   VOLUME NAME
local    a5254033fb420e7c3495b30f869ac07df307b3ff0701be7613dcc306e73d2048
local    e34bb6cf87eefe4683b1a0c18f0b4ca1fc4085dd208d6a0c9b045248255f0fca
local    eaa4c5b1a89e951484d1b068977e339edf8b1125596361413db2b149b2493e8a

rhatdan · 2019-01-29T19:07:32Z

Might be a race condition. Weird, I was trying that out in testing. Lets get this merged and then we can continue playing with it. I have seen another issue where you can remove a builtin volume and it does not complain if their is a container using it.

mheon · 2019-01-29T19:38:29Z

The DB backend for built-in volumes needs an partial rewrite on top of that. Leaving them as volumes in the spec is reallllllly iffy and can help lead to these situations.

…

On Tue, Jan 29, 2019, 14:07 Daniel J Walsh ***@***.*** wrote: Might be a race condition. Weird, I was trying that out in testing. Lets get this merged and then we can continue playing with it. I have seen another issue where you can remove a builtin volume and it does not complain if their is a container using it. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#2229 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHYHCIlAj98JwRVlZEf-Vwk6_lrgR1YIks5vIJv2gaJpZM4aYVED> .

mheon · 2019-01-29T19:39:28Z

I'm not sure this patch is safe until that happens. Honestly not sure I want anything built in volume work merged until the backend cleanup can land.

…

On Tue, Jan 29, 2019, 14:38 Matthew Heon ***@***.*** wrote: The DB backend for built-in volumes needs an partial rewrite on top of that. Leaving them as volumes in the spec is reallllllly iffy and can help lead to these situations. On Tue, Jan 29, 2019, 14:07 Daniel J Walsh ***@***.*** wrote: > Might be a race condition. Weird, I was trying that out in testing. Lets > get this merged and then we can continue playing with it. I have seen > another issue where you can remove a builtin volume and it does not > complain if their is a container using it. > > — > You are receiving this because your review was requested. > Reply to this email directly, view it on GitHub > <#2229 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AHYHCIlAj98JwRVlZEf-Vwk6_lrgR1YIks5vIJv2gaJpZM4aYVED> > . >

mheon · 2019-01-29T21:11:37Z

I don't think we want to completely drop the old volume handling here, either. The per container dirs and tmpfs volume drivers have merit. This needs to be switchable. Libpod.conf? More detailed review tomorrow, but I think this is probably safe to go in even without those DB changes. Does need some changes.

…

On Tue, Jan 29, 2019, 14:39 Matthew Heon ***@***.*** wrote: I'm not sure this patch is safe until that happens. Honestly not sure I want anything built in volume work merged until the backend cleanup can land. On Tue, Jan 29, 2019, 14:38 Matthew Heon ***@***.*** wrote: > The DB backend for built-in volumes needs an partial rewrite on top of > that. Leaving them as volumes in the spec is reallllllly iffy and can help > lead to these situations. > > On Tue, Jan 29, 2019, 14:07 Daniel J Walsh ***@***.*** > wrote: > >> Might be a race condition. Weird, I was trying that out in testing. Lets >> get this merged and then we can continue playing with it. I have seen >> another issue where you can remove a builtin volume and it does not >> complain if their is a container using it. >> >> — >> You are receiving this because your review was requested. >> Reply to this email directly, view it on GitHub >> <#2229 (comment)>, >> or mute the thread >> <https://github.com/notifications/unsubscribe-auth/AHYHCIlAj98JwRVlZEf-Vwk6_lrgR1YIks5vIJv2gaJpZM4aYVED> >> . >> >

mheon · 2019-01-29T21:58:04Z

libpod/runtime_ctr.go

@@ -393,6 +402,14 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force bool)
 		}
 	}

+	for _, v := range volumes {
+		if volume, err := runtime.state.Volume(v); err == nil {
+			if err := runtime.removeVolume(ctx, volume, true, true); err != nil {


If we get an error that the volume is still in use, we should log nothing

Fixed to ignore error if I get back the errnovolume or errcontainerbeingused

mheon · 2019-01-29T21:58:46Z

libpod/runtime_ctr.go

@@ -221,17 +221,18 @@ func (r *Runtime) newContainer(ctx context.Context, rSpec *spec.Spec, options ..

 // RemoveContainer removes the given container
 // If force is specified, the container will be stopped first
+// If removeVolume is specified, the volume contents will be removed also


"If removeVolume is specified, named volumes used by the container will be removed also if and only if the container is the sole user"

mheon · 2019-01-29T21:59:41Z

cmd/podman/start.go

@@ -147,7 +147,7 @@ func startCmd(c *cli.Context) error {
 					logrus.Errorf("unable to detect if container %s should be deleted", ctr.ID())
 				}
 				if createArtifact.Rm {
-					if rmErr := runtime.RemoveContainer(ctx, ctr, true); rmErr != nil {
+					if rmErr := runtime.RemoveContainer(ctx, ctr, true, true); rmErr != nil {


I don't think this is safe at all - what if there are actual named volumes people are using, not image volumes, attached? We're risking killing people's data here

Yes this is questionable. We need a mechanism to know if the volume was created just for the container or not. But I agree we should remove the volume removal.

mheon · 2019-01-29T22:00:54Z

libpod/container_internal.go

+		if err != nil {
+			return errors.Wrapf(ErrCtrStateInvalid, "failed to create volume for %s in %s for container %s", k, mountPoint, c.ID())
+		}
+		volumePath := filepath.Join(volume.config.MountPoint, k)


Nuke the filepath.Join() - why do we need it? There's only one thing in the volume, no need to make a subdir in it

The k in this case is the directory that is being volume mounted in.

In my tests It happens to be /myvol. I believe this is necessary.

Shouldn't be - we're bind-mounting it into the given path in the container, no need to have the volume path appended again after the path for the named volume

Right, we have already created a new volume with a generated name, which will be bind-mounted to /myvol inside the container. So no need to do the filepath.Join() and create a sub-dir inside that.

mheon · 2019-01-29T22:01:12Z

libpod/container_internal.go

@@ -1082,8 +1082,11 @@ func (c *Container) addLocalVolumes(ctx context.Context, g *generate.Generator,
 		if MountExists(g.Mounts(), k) {
 			continue
 		}
-		volumePath := filepath.Join(c.config.StaticDir, "volumes", k)
-
+		volume, err := c.runtime.NewVolume(ctx, []VolumeCreateOption{}...)


We need to give the volume a name, at least - something based on the container's name. containername-volumepath?

The volume gets named automatically if there is no name specified. This is how it works in Docker also. The problem with us naming it, is that there could be multiple built in volumes within an image.

mheon · 2019-01-29T22:02:18Z

libpod/container_internal.go

@@ -1290,3 +1293,23 @@ func getExcludedCGroups() (excludes []string) {
 	excludes = []string{"rdma"}
 	return
 }
+
+// namedVolumes returns named volumes for the container
+func (c *Container) namedVolumes() ([]string, error) {


It's a fundamental problem that we don't have the actual name of the volume in here, just the path it's mounted to in the container

Can't fix in this PR, just making a note for myself to go over when I do the DB stuff

I agree. Long range we need to revisit this, especially with the eventual adding of non local Docker volumes.

mheon · 2019-01-29T22:03:06Z

libpod/container_internal.go

+// namedVolumes returns named volumes for the container
+func (c *Container) namedVolumes() ([]string, error) {
+	var volumes []string
+	spec, err := c.specFromState()


Use c.config.spec instead, specFromState is expensive and pointless, we don't add any named volumes to the generated spec on disk

That is what I had originally but the volume is not listed in the spec, it is only listed when I load from state.

Wait, what? We're not adding these when we generate the spec... At least, we shouldn't be... This is in a really bad spot if we can't find volumes without the running spec

umohnani8 · 2019-01-30T15:18:35Z

LGTM once comments are addressed.

rhatdan · 2019-02-09T15:11:26Z

Also fixes #2301

rhatdan · 2019-02-10T15:09:59Z

bot, retest this please

mheon · 2019-02-11T19:17:41Z

I think this is only a partial solution, but a full solution is a ways off, and I really want this in 1.0

LGTM
@giuseppe @TomSweeneyRedHat @baude @vrothberg PTAL

mheon · 2019-02-11T19:17:59Z

Sorry, 1.1 not 1.0

rh-atomic-bot · 2019-02-11T19:59:41Z

☔ The latest upstream changes (presumably #2307) made this pull request unmergeable. Please resolve the merge conflicts.

vrothberg

Code LGTM (just a minor nit).

But note that I am not very familiar with those parts of the code. Tests would be nice. Currently, both commits seem to be merge commits which seems odd.

libpod/container_internal.go

rh-atomic-bot · 2019-02-12T14:47:07Z

☔ The latest upstream changes (presumably #2309) made this pull request unmergeable. Please resolve the merge conflicts.

rh-atomic-bot · 2019-02-13T21:56:39Z

☔ The latest upstream changes (presumably #2319) made this pull request unmergeable. Please resolve the merge conflicts.

vrothberg · 2019-02-14T11:15:42Z

@rhatdan code LGTM but the merge commits in the PR may indicate that you need to rebase the branch and repush.

iFix builtin volumes to work with podman volume Currently builtin volumes are not recored in podman volumes when they are created automatically. This patch fixes this. Remove container volumes when requested Currently the --volume option on podman remove does nothing. This will implement the changes needed to remove the volumes if the user requests it. When removing a volume make sure that no container uses the volume. Signed-off-by: Daniel J Walsh dwalsh@redhat.com Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

baude · 2019-02-14T20:43:40Z

/lgtm

openshift-ci-robot requested review from mheon and umohnani8 January 29, 2019 15:43

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L labels Jan 29, 2019

TomSweeneyRedHat reviewed Jan 29, 2019

View reviewed changes

rhatdan force-pushed the volumes branch from 45a1b89 to f389c03 Compare January 29, 2019 17:48

TomSweeneyRedHat reviewed Jan 29, 2019

View reviewed changes

rhatdan mentioned this pull request Jan 29, 2019

Volume is not created when using VOLUME in Dockerfile #2170

Closed

mheon reviewed Jan 29, 2019

View reviewed changes

rhatdan force-pushed the volumes branch from f389c03 to 943b0dc Compare January 30, 2019 12:13

rhatdan force-pushed the volumes branch 3 times, most recently from e05393e to b484cc5 Compare February 1, 2019 14:46

rhatdan force-pushed the volumes branch 2 times, most recently from 5e68469 to 9732931 Compare February 9, 2019 15:10

rhatdan mentioned this pull request Feb 9, 2019

'podman run --volume .:/whatever' panics (relative path source) #2301

Closed

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 11, 2019

rhatdan force-pushed the volumes branch from 9732931 to d9eee16 Compare February 12, 2019 02:06

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2019

vrothberg reviewed Feb 12, 2019

View reviewed changes

libpod/container_internal.go Show resolved Hide resolved

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2019

rhatdan force-pushed the volumes branch from 26f10c7 to 9598a73 Compare February 13, 2019 06:43

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2019

rhatdan force-pushed the volumes branch from 9598a73 to 1238900 Compare February 13, 2019 20:41

rhatdan force-pushed the volumes branch from 126f2a7 to 52df1fa Compare February 14, 2019 18:22

openshift-ci-robot assigned baude Feb 14, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2019

openshift-merge-robot merged commit 8ec9eb0 into containers:master Feb 14, 2019

This was referenced Feb 14, 2019

OpenTracing First Impl #1692

Merged

Enable --rm with --detach #2335

Merged

Add support to migrate containers #2272

Merged

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 27, 2023

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

Fix volume handling in podman #2229

Fix volume handling in podman #2229

Conversation

rhatdan commented Jan 29, 2019

openshift-ci-robot commented Jan 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomSweeneyRedHat commented Jan 29, 2019

Choose a reason for hiding this comment

TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

rhatdan commented Jan 29, 2019

rhatdan commented Jan 29, 2019

tvass commented Jan 29, 2019

rhatdan commented Jan 29, 2019

mheon commented Jan 29, 2019 via email

mheon commented Jan 29, 2019 via email

mheon commented Jan 29, 2019 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umohnani8 commented Jan 30, 2019

rhatdan commented Feb 9, 2019

rhatdan commented Feb 10, 2019

mheon commented Feb 11, 2019

mheon commented Feb 11, 2019

rh-atomic-bot commented Feb 11, 2019

vrothberg left a comment

Choose a reason for hiding this comment

rh-atomic-bot commented Feb 12, 2019

rh-atomic-bot commented Feb 13, 2019

vrothberg commented Feb 14, 2019

baude commented Feb 14, 2019