Add support for anonymous volumes to podman run -v
#4287
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mheon 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 |
openshift-ci-robot
added
approved
|
Needs tests and manpage adjustments still. Will hit those after lunch. |
mheon
force-pushed
the
anonymous_volumes
branch
2 times, most recently
from
7a1f71d to
86b57a7
Compare
|
Docs done, test added. Want to add a few more around removing containers with -v and volumes being retained / not being retained. |
Previously, when `podman run` encountered a volume mount without separate source and destination (e.g. `-v /run`) we would assume that both were the same - a bind mount of `/run` on the host to `/run` in the container. However, this does not match Docker's behavior - in Docker, this makes an anonymous named volume that will be mounted at `/run`. We already have (more limited) support for these anonymous volumes in the form of image volumes. Extend this support to allow it to be used with user-created volumes coming in from the `-v` flag. This change also affects how named volumes created by the container but given names are treated by `podman run --rm` and `podman rm -v`. Previously, they would be removed with the container in these cases, but this did not match Docker's behaviour. Docker only removed anonymous volumes. With this patch we move to that model as well; `podman run -v testvol:/test` will not have `testvol` survive the container being removed by `podman rm -v`. The sum total of these changes let us turn on volume removal in `--rm` by default. Fixes: containers#4276 Signed-off-by: Matthew Heon <[email protected]>
mheon
force-pushed
the
anonymous_volumes
branch
from
86b57a7 to
0d62391
Compare
|
Alright, tests are all done. I think this is ready. |
rhatdan
reviewed
Oct 19, 2019
| @@ -437,7 +437,7 @@ func (r *LocalRuntime) Run(ctx context.Context, c *cliconfig.RunValues, exitCode | |||
| } | |||
|
|
|||
| if c.IsSet("rm") { | |||
| if err := r.Runtime.RemoveContainer(ctx, ctr, false, false); err != nil { | |||
| if err := r.Runtime.RemoveContainer(ctx, ctr, false, true); err != nil { | |||
There was a problem hiding this comment.
What happens if the volume from this container is shared with another container?
|
Everything looks good, but I am wondering about the force removal of volumes. |
|
It only removes anonymous volumes, so volumes you gave a name to will stick
around. In the current implementation, for anonymous volumes, we refuse to
remove if they are used by another container, but if that container becomes
the only container using the volume, it will remove it with 'podman rm
--volumes'.
…On Sat, Oct 19, 2019, 06:39 Daniel J Walsh ***@***.***> wrote:
Everything looks good, but I am wondering about the force removal of
volumes.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#4287>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB3AOCGSVJS55BJHR4GTXN3QPLPUTANCNFSM4JB3N5YA>
.
|
|
LGTM |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, when
podman runencountered a volume mount without separate source and destination (e.g.-v /run) we would assume that both were the same - a bind mount of/runon the host to/runin the container. However, this does not match Docker's behavior - in Docker, this makes an anonymous named volume that will be mounted at/run.We already have (more limited) support for these anonymous volumes in the form of image volumes. Extend this support to allow it to be used with user-created volumes coming in from the
-vflag.This change also affects how named volumes created by the container but given names are treated by
podman run --rmandpodman rm -v. Previously, they would be removed with the container in these cases, but this did not match Docker's behaviour. Docker only removed anonymous volumes. With this patch we move to that model as well;podman run -v testvol:/testwill not havetestvolsurvive the container being removed bypodman rm -v.The sum total of these changes let us turn on volume removal in
--rmby default.Fixes: #4276