podman rm --all not working with containers that have dependencies

[Luap99]

Issue Description

Try to use podman rm -fa when you have containers with dependencies, e.g. --network container:xxx or any other namespace flag which supports container:.

Steps to reproduce the issue

1. podman run --name test1 -d alpine top
2. podman run --network container:test1 -d alpine top
3. podman rm -fa

Describe the results you received

a32636dc3e18c522d3f16340801ffb76d4e8acad8f02b32723ce91721dbc50a0
Error: container 8c9cbe82f02577e166187ec899afc6497e3bd551fae1b05df5a437b8ef90d081 has dependent containers which must be removed before it: a32636dc3e18c522d3f16340801ffb76d4e8acad8f02b32723ce91721dbc50a0: container already exists

Describe the results you expected

All containers removed without errors.

podman info output

tested with latest main

Podman in a container

No

Privileged Or Rootless

None

Upstream Latest Release

Yes

Additional environment details

No response

Additional information

This is causing issues in the e2e integration tests because we only do rm -fa as cleanup thus some processes are leaked.

[vrothberg]

Ouch!

Can you point out where this fails in the e2e tests? I think this error should be fatal (i.e., let the e2e tests fail).

[Luap99]

I pushed some changes for it to #18163.
The problem is in (p *PodmanTestIntegration) Cleanup() where we only run pod rm -fa and rm -fa

[vrothberg]

Excellent, nice work!

[Luap99]

I think we need to order the container so that they are removed in the right order, looks like we already handle this for pods (e19e0de)

Following the comment it should not be used for containers:

podman/libpod/container_graph.go

Lines 285 to 291 in 5e6c064

	// Visit a node on the container graph and remove it, or set an error if it
	// failed to remove. Only intended for use in pod removal; do *not* use when
	// removing individual containers.
	// All containers are assumed to be *UNLOCKED* on running this function.
	// Container locks will be acquired as necessary.
	// Pod and infraID are optional. If a pod is given it must be *LOCKED*.
	func removeNode(ctx context.Context, node *containerNode, pod *Pod, force bool, timeout *uint, setError bool, ctrErrors map[string]error, ctrsVisited map[string]bool, ctrNamedVolumes map[string]*ContainerNamedVolume) {

I am not sure why?
@mheon Can you take a look please? This is pretty important to fix broken e2e tests.

[edsantiago]

OMG if this gets fixed, can you add like actual error checking to cleanup? This has been a flake nightmare for years. #11597

[Luap99]

I already have it in my ginkgo update PR, so I could find the problematic ones.

[mheon]

We could definitely restructure remove-all to do an ordered removal, but in the meantime, you could just add --depends to remove all dependencies and solve the errors that way?

[mheon]

Alternatively, we could make the combination of --all and --force imply --depends because if you want every container removed, dependencies are included in "every container".

[Luap99]

I only see a --depend option and it is not working:

$ bin/podman run --name test1 -d alpine top
8d09a1feea5963ecacf888264abd24563ee9f6224b4f39e01f5294494a54385c
$ bin/podman run --network container:test1 -d alpine top
617d47aa6ae7e40abed4dd71cc617b98e2b56890a5f8db6b592376a6bd31c1fa
$ bin/podman rm -fa --depend
617d47aa6ae7e40abed4dd71cc617b98e2b56890a5f8db6b592376a6bd31c1fa
Error: container 8d09a1feea5963ecacf888264abd24563ee9f6224b4f39e01f5294494a54385c has dependent containers which must be removed before it: 617d47aa6ae7e40abed4dd71cc617b98e2b56890a5f8db6b592376a6bd31c1fa: container already exists
$ bin/podman ps
CONTAINER ID  IMAGE                            COMMAND     CREATED         STATUS         PORTS       NAMES
8d09a1feea59  docker.io/library/alpine:latest  top         33 seconds ago  Up 34 seconds              test1

[mheon]

I checked, and it's already set by default on rm --all - so our actual bug is that --depend is broken.

[mheon]

--depend works fine outside of rm -fa - interesting.

[mheon]

Self-assigning, the code here is a bit of a mess and probably ought to be refactored into normal RemoveContainer.

[Luap99]

@mheon any chance you can finish this? I would love to get my ginkgo v2 PR in.

[edsantiago]

@Luap99 as a stopgap, WDYT of just running two sequential podman rm -fas in cleanup?

[Luap99]

@mheon When you open a PR please also revert: c4b9f4b

[mheon]

Ack