Remove containers when pod prune & pod rm. #4443
Conversation
QiWang19
force-pushed
the
prune_pod
branch
3 times, most recently
from
5cd2425 to
69c10ab
Compare
QiWang19
force-pushed
the
prune_pod
branch
10 times, most recently
from
baa14cb to
b3d8b99
Compare
| @@ -247,7 +247,7 @@ func (i *LibpodAPI) RemovePod(call iopodman.VarlinkCall, name string, force bool | |||
| if err != nil { | |||
| return call.ReplyPodNotFound(name, err.Error()) | |||
| } | |||
| if err = i.Runtime.RemovePod(ctx, pod, force, force); err != nil { | |||
| if err = i.Runtime.RemovePod(ctx, pod, true, force); err != nil { | |||
There was a problem hiding this comment.
We should, that is the change we made to podman pod rm.
There was a problem hiding this comment.
From the tests failures podman pod rm doesn't remove a pod with a container https://github.com/containers/libpod/blob/b3d8b99b82d466ab9a325e66c1a240ff989eeb66/test/e2e/pod_rm_test.go#L80-L89
There was a problem hiding this comment.
Then remove this test. Podman pod will always have a container. I don't see a reason to not remove the pod. Too much work on the user to have to remove all containers before removing the pod. Force says remove the pod and containers if they are running.
Containers in Pods do not standalone, so pod includes containers.
There was a problem hiding this comment.
We handle the infra container explicitly. I would prefer that we keep the current behavior - --force to remove pods with anything other than an infra container
There was a problem hiding this comment.
Why. Containers are part of the Pod and should be treated as such. Similar to built in volumes. I don't want to add yet another option that will confuse the users.
When people think in terms of pods, they do not consider the containers as separate.
There was a problem hiding this comment.
And podman rm doesn't remove volumes unless you tell it to - you need --volumes to do that.
There was a problem hiding this comment.
Honestly, we should just get a session together about pods at the F2F - it's a good opportunity to revisit these decisions
There was a problem hiding this comment.
podman rm removes the builtin volumes, IE Volumes that are not used by other entities, I see Pod containers the same way. But yes we can talk about this at the Face2Face.
@haircommander @mrunalp What does kubernetes do. Does it first remove all of the containers from a pod, before removing the pod, or when you tell CRI-O to remove the pod, then it removes all of the containers and finally the POD?
There was a problem hiding this comment.
if the kubelet asks cri-o to remove a pod, it is removed, regardless of containers that live in it, I am pretty sure. The kubelet making a request of cri-o is a bit different than a user typing cli, though. I think ultimately I'm with @rhatdan here, a pod should be treated as a unit in removal.
|
@rhatdan @mheon @haircommander PTAL |
QiWang19
changed the title
|
LGTM |
|
/lgtm |
|
Eh. Not the biggest fan of removing containers without |
This path allows pod prune & pod rm to remove stopped containers in the pod before deleting the pod. PrunePods and RemovePod should be able to remove containers without force removal of stopped pods. Signed-off-by: Qi Wang <[email protected]>
|
update man pages. Needs approved, lgtm labels |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mheon, QiWang19 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
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
fix #4346
This path allows pod prune & pod rm to remove stopped containers in the pod before deleting the pod.
PrunePods and RemovePod should be able to remove containers without force removal of stopped pods.
Signed-off-by: Qi Wang [email protected]