Pod Rm Infra Handling Improvements

[cdoern]

What this PR does / why we need it:

Made it so that the pod is removed if the only running container is infra. An error was being triggered where we have exited containers and only infra running, the pod was failing to remove.

Which issue(s) this PR fixes:

resolves #11713

Signed-off-by: cdoern [email protected]

[Luap99]

I think you should just add ctr.IsInfra() to line 202, e.g.

if force || ctr.IsInfra() {
	continue
}

[cdoern]

I think you should just add ctr.IsInfra() to line 202, e.g.

if force || ctr.IsInfra() {
	continue
}

I will add this @Luap99 but I think the main thing we want to do here is catch it in the first check because numCtrs should represent the non-exited containers.

[rhatdan]

LGTM
Tests are not happy though

[cdoern]

@rhatdan any idea why that prune test would be failing? seems to have one extra image listed in the local storage

I see, I think the exited containers were no longer being removed because they weren't in the ctr array, leading the image to not be pruned

[mheon]

I'm still really unclear as to how this is actually triggered. Has anyone managed to reproduce this locally? If so, can we figure out exactly how a pod with multiple containers is giving incorrect error messages? Because the logic in main looks solid to me as-is.

[cdoern]

Yeah I was able to recreate it just by making a pod and running two containers with alpine, and then removing the pod. I think we either should modify the checkReadyForRemoval function to do what I implemented above or just keep what I have. The issue is infra is running and is not recognized as the infra container so the code rejects the pod rm

[mheon]

But how are we getting that far in the first place? It looks like we should definitely be stopping on the existing numCtrs > 0 check. Unless removeCtrs is set to true, which I don't think it should be without --force specified?

[cdoern]

so @mheon what do you expect the outcome here to be? should we allow removal when all containers are exited and only infra is running? or should this first error "pod %s contains containers and cannot be removed" be triggered. I was thinking we should successfully remove the pod but it seems like you think instead this err msg should trigger? If that is the case then PodContainers might be giving us the wrong response in terms of exited containers, that is the only thing I can think of it numCtrs > 0 is false.

[mheon]

I think the expected outcome is that we do not remove, and print a list of the containers present in the pod preventing it from being removed.

[mheon]

OK, maybe I'm wrong here... the podman pod rm manpage says it will remove "stopped pods and their containers".

[rhatdan]

podman pod create --name dan
podman create --pod dan fedora
podman pod rm dan

Everything works.

podman pod create --name dan
podman run --pod dan fedora
podman stop -l
podman pod rm dan
Error: pod 63e83af70712988b25ef68ebc3bb00c270993ab1578ca7207fb3f0646122c30d has containers that are not ready to be removed: cannot remove container e294a2e21c12813eb1a428dc7bc4d4cfed34cc6efefc7e4ff1b18342bc88c5c3 as it is running - running or paused containers cannot be removed without force: container state improper

I would have thought these were the same.

[mheon]

Can I suggest that we talk about this at cabal tomorrow, ensure we have consensus? Also, whatever we do, let's improve the manpages so it's actually clear what's going on.

[cdoern]

Sure, just let me know what you guys decide and I'll implement. I have class from 11-1:45 so I won't be able to come to the cabal.

Actually, class from 11-12:15 is canceled tomorrow so I'll be there to discuss

[cdoern]

@mheon does this cover what we discussed in cabal?

[cdoern]

@containers/podman-maintainers PTAL, ended up being a simple fix where we just needed to add a check for IsInfra in checkReadyForRemoval() I put the check within this function because if you put it within removePod we still fail because checkReadForRemoval gets called again later when the container is actually removed.

[Luap99]

LGTM

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdoern, Luap99

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 [Luap99]

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

[Luap99]

@mheon PTAL

[vrothberg]

LGTM

[mheon]

/lgtm