Fix `podman stop` and `podman run --rmi` #23670

mheon · 2024-08-19T15:39:54Z

This started off as an attempt to make podman stop on a container started with --rm actually remove the container, instead of just cleaning it up and waiting for the cleanup process to finish the removal.

In the process, I realized that podman run --rmi was rather broken. It was only done as part of the Podman CLI, not the cleanup process (meaning it only worked with attached containers) and the way it was wired meant that I was fairly confident that it wouldn't work if I did a podman stop on an attached container run with --rmi. I rewired it to use the same mechanism that podman run --rm uses, so it should be a lot more durable now, and I also wired it into podman inspect so you can tell that a container will remove its image.

Tests have been added for the changes to podman run --rmi. No tests for stop on a run --rm container as that would be racy.

Does this PR introduce a user-facing change?

Fixed a bug where the `podman stop` command would not ensure containers created with `--rm` were removed when it exited.
Fixed a bug where the `--rmi` option to `podman run` did not function correctly with detached containers.
The output of the `podman inspect` command for containers now includes a new field, `HostConfig.AutoRemoveImage`, which shows whether a container was created with the `--rmi` option set.

Luap99 · 2024-08-19T16:26:40Z

Since we were talking about this please add a Fixes #22852 and Fixes for the Jira issue to your commit

Luap99 · 2024-08-19T16:02:52Z

test/system/030-run.bats

+    # Try again with a detached container and verify it works
+    cname=c_$(safename)
+    run_podman run -d --name $cname --rmi $NONLOCAL_IMAGE /bin/true
+    run_podman wait $cname


This will not work, see my latest wait changes. podman wait waits for stop not removal so this will flake a lot.
And you cannot even wait for removing as the image is removed after the ctr is removed so there would still be a race...

I suggest you a sleep loop to wait until image exists command fails and then have some timeout in there

Luap99 · 2024-08-19T16:04:43Z

pkg/domain/infra/abi/containers.go

-			// We still need to clean up since we do not know if the other cleanup process is successful
-			if c.AutoRemove() && (errors.Is(err, define.ErrNoSuchCtr) || errors.Is(err, define.ErrCtrRemoved)) {
-				return nil
+		if c.AutoRemove() && !c.ShouldRestart(ctx) {


do not call c.ShouldRestart(ctx) here, a container that is stopped by hand cannot by definition restart again.

Luap99 · 2024-08-19T16:08:32Z

pkg/domain/infra/abi/containers.go

+				if (errors.Is(err, define.ErrNoSuchCtr) || errors.Is(err, define.ErrCtrRemoved)) {
+					return nil
+				}


this is now wrong, we only ignore the error in case of auto remove removed the ctr in the background.

I guess there is harm harm ignoring it here though but is is a change of logic that isn't needed.

I think ignoring is still safer - there's an inherent race around the cleanup here as we unlock after Stop(), so a podman rm can sneak in before cleanup.

it can yes sure, but is podman stop ctr and then ctr is removed in the middle not an error to the end user? Unless the ctr is configured for auto removed I would argue this will be a user error if they stop/remove in parallel IMO and something they should notice. But I can accept either way.

I don't think it's an error case since the stop actually happened, and the only thing we're erroring on is cleanup - so we did what was asked, and only the cleanup logic is throwing an error.

TomSweeneyRedHat · 2024-08-19T19:12:14Z

Addresses: https://issues.redhat.com/browse/RHEL-39513

mheon · 2024-08-20T12:36:24Z

@containers/podman-maintainers This is ready

edsantiago

LGTM, some suggestions for the test

test/system/030-run.bats

This started off as an attempt to make `podman stop` on a container started with `--rm` actually remove the container, instead of just cleaning it up and waiting for the cleanup process to finish the removal. In the process, I realized that `podman run --rmi` was rather broken. It was only done as part of the Podman CLI, not the cleanup process (meaning it only worked with attached containers) and the way it was wired meant that I was fairly confident that it wouldn't work if I did a `podman stop` on an attached container run with `--rmi`. I rewired it to use the same mechanism that `podman run --rm` uses, so it should be a lot more durable now, and I also wired it into `podman inspect` so you can tell that a container will remove its image. Tests have been added for the changes to `podman run --rmi`. No tests for `stop` on a `run --rm` container as that would be racy. Fixes containers#22852 Fixes RHEL-39513 Signed-off-by: Matt Heon <[email protected]>

edsantiago

Tests LGTM

openshift-ci · 2024-08-20T16:26:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, 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:

~~OWNERS~~ [edsantiago,mheon]

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

rhatdan · 2024-08-20T19:54:13Z

pkg/domain/infra/abi/containers.go

+					return fmt.Errorf("removing container %s image %s: %w", c.ID(), imageName, mErr)
+				}
+			}
+		} else {


This looks like if you specify AutoRemoteImage, we are no longer cleaning up the containers?

Removing the image implies removing the container, and removing the container also unmounts, cleans up the network, etc, so we should be OK.

rhatdan · 2024-08-20T21:41:24Z

/lgtm

openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 19, 2024

github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Aug 19, 2024

mheon mentioned this pull request Aug 19, 2024

'podman stop' does not thoroughly clean up, resulting in 'the container name \"autotests\" is already in use' #22852

Closed

Luap99 reviewed Aug 19, 2024

View reviewed changes

mheon force-pushed the fix_stop_and_rmi branch 5 times, most recently from 4fcbb55 to 4122abb Compare August 19, 2024 19:00

mheon force-pushed the fix_stop_and_rmi branch 2 times, most recently from ba1d729 to 79a3dba Compare August 19, 2024 20:13

edsantiago reviewed Aug 20, 2024

View reviewed changes

test/system/030-run.bats Outdated Show resolved Hide resolved

test/system/030-run.bats Outdated Show resolved Hide resolved

test/system/030-run.bats Outdated Show resolved Hide resolved

mheon force-pushed the fix_stop_and_rmi branch from 79a3dba to 458ba5a Compare August 20, 2024 13:51

edsantiago approved these changes Aug 20, 2024

View reviewed changes

rhatdan reviewed Aug 20, 2024

View reviewed changes

openshift-ci bot assigned rhatdan Aug 20, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 20, 2024

openshift-merge-bot bot merged commit 43fe3eb into containers:main Aug 20, 2024
85 of 86 checks passed

stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Nov 19, 2024

stale-locking-app bot locked as resolved and limited conversation to collaborators Nov 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `podman stop` and `podman run --rmi` #23670

Fix `podman stop` and `podman run --rmi` #23670

mheon commented Aug 19, 2024

Luap99 commented Aug 19, 2024

Luap99 Aug 19, 2024

Luap99 Aug 19, 2024

Luap99 Aug 19, 2024

mheon Aug 19, 2024

Luap99 Aug 19, 2024

mheon Aug 19, 2024

TomSweeneyRedHat commented Aug 19, 2024

mheon commented Aug 20, 2024

edsantiago left a comment

edsantiago left a comment

openshift-ci bot commented Aug 20, 2024

rhatdan Aug 20, 2024

mheon Aug 20, 2024

rhatdan commented Aug 20, 2024

Fix podman stop and podman run --rmi #23670

Fix podman stop and podman run --rmi #23670

Conversation

mheon commented Aug 19, 2024

Does this PR introduce a user-facing change?

Luap99 commented Aug 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomSweeneyRedHat commented Aug 19, 2024

mheon commented Aug 20, 2024

edsantiago left a comment

Choose a reason for hiding this comment

edsantiago left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Aug 20, 2024

Fix `podman stop` and `podman run --rmi` #23670

Fix `podman stop` and `podman run --rmi` #23670