Remove container from storage on --force #2281
Conversation
|
Will allow you to cleanup from #2240 |
|
[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:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
cmd/podman/rm.go
Outdated
| @@ -42,6 +46,20 @@ Running containers will not be removed without the -f option. | |||
| } | |||
| ) | |||
|
|
|||
| // Attempt to remove containers | |||
| func removeContainersFromStorage(runtime *libpod.Runtime, ctrs []string) { | |||
| storageService := runtime.StorageService() | |||
There was a problem hiding this comment.
If we really want to do this, let's do it in libpod, so we don't need to expose these things
docs/podman-rm.1.md
Outdated
| @@ -17,7 +17,9 @@ Remove all containers. Can be used in conjunction with -f as well. | |||
|
|
|||
| **--force, f** | |||
|
|
|||
| Force the removal of a running and paused containers | |||
| Force the removal of a running and paused containers. Forcing a container removal will also | |||
There was a problem hiding this comment.
"a running" to just "running" or perhaps "any running" or "all running"
docs/podman-rm.1.md
Outdated
| Force the removal of a running and paused containers | ||
| Force the removal of a running and paused containers. Forcing a container removal will also | ||
| remove containers from container storage, if the container is not known to podman. The container | ||
| could have been created by a different container engine. |
There was a problem hiding this comment.
I'm hemming and hawing about whether or not to add something like: "different container engine and due to that this option should be used cautiously." IDK if that's beating over the head too much or not.
There was a problem hiding this comment.
Yes I was going down that path. I also was thinking that this can happen when you are using just podman in some weird situations. Where the database gets updated and for some reason storage does not get removed.
|
Unhappy tests, changes LGTM in general and I concur with @mheon, would like to see the code in libpod it would probably be useful there for other API's. |
rhatdan
force-pushed
the
deleteContainer
branch
from
6c1e245 to
3de0fec
Compare
|
bot, retest this please |
|
LGTM |
| // if the container does not exist in database, attempt to remove it from storage | ||
| if _, err := r.LookupContainer(i); err != nil && errors.Cause(err) == image.ErrNoSuchCtr { | ||
| r.storageService.UnmountContainerImage(i, true) | ||
| if err := r.storageService.DeleteContainer(i); err != nil && errors.Cause(err) != storage.ErrContainerUnknown { |
There was a problem hiding this comment.
I think returning ErrContainerUnknown is probably better than dropping it - useful to know if we did nothing in these cases, I think.
There was a problem hiding this comment.
I think this is going to be a common case, since we might have attempted to remove 5 containers, and 4 succeeded and one failed on the normal path. Then running through this path would complain about the 4 successful ones.
I think it is better to just ignore containers that are already removed. Similar to rm -f /etc/foobar, will not care if /etc/foobar exists.
|
☔ The latest upstream changes (presumably #2274) made this pull request unmergeable. Please resolve the merge conflicts. |
openshift-ci-robot
added
the
needs-rebase
Currently we can get into a state where a container exists in storage but does not exist in libpod. If the user forces a removal of this container, then we should remove it from storage even if the container is owned by another tool. Signed-off-by: Daniel J Walsh <[email protected]>
rhatdan
force-pushed
the
deleteContainer
branch
from
3de0fec to
233ba5b
Compare
openshift-ci-robot
removed
the
needs-rebase
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
Currently we can get into a state where a container exists in
storage but does not exist in libpod. If the user forces a
removal of this container, then we should remove it from storage
even if the container is owned by another tool.
Signed-off-by: Daniel J Walsh [email protected]