container stop: handle race

[vrothberg]

There is an inherent race when stopping/killing a container with other processes attempting to do the same and also with the container having exited in the meantime. In those cases, the OCI runtime may fail to kill the container as it has already exited.

Handle those races by first checking if the container state has changed before returning the error.

[NO NEW TESTS NEEDED] - as it's a hard to test race.

Fixes: #18452

Does this PR introduce a user-facing change?

Improve error handling when stopping or killing a container.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vrothberg

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

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

[vrothberg]

@Luap99 WDYT?

[Luap99]

sounds good

[NO NEW TESTS NEEDED] - as it's a hard to test race.

I cherry pick this into #18442 which should give us much more insight.

[Luap99]

I still see open pidfd flakes in https://api.cirrus-ci.com/v1/artifact/task/6578178697723904/html/int-podman-fedora-38-root-container-boltdb.log.html so this doe snot seem to help/change much

[vrothberg]

I still see open pidfd flakes in https://api.cirrus-ci.com/v1/artifact/task/6578178697723904/html/int-podman-fedora-38-root-container-boltdb.log.html so this doe snot seem to help/change much

There's probably another call path. I'll take another look.

[vrothberg]

@Luap99 could you try the following patch in your PR?

diff --git a/libpod/container_internal.go b/libpod/container_internal.go
index cf4fa1e0cb8b..f03016322042 100644
--- a/libpod/container_internal.go
+++ b/libpod/container_internal.go
@@ -1358,6 +1358,7 @@ func (c *Container) stop(timeout uint) error {
                                // If the container has already been removed (e.g., via
                                // the cleanup process), set the container state to "stopped".
                                c.state.State = define.ContainerStateStopped
+                               logrus.Errorf("FIX ME HERE, PLEASE! %v", stopErr)
                                return stopErr
                        }

My suspicion is that we need to tweak the return stopErr but I first want to make sure it's the right place.

[Luap99]

@Luap99 could you try the following patch in your PR?

yes, pushed

[Luap99]

I couldn't find FIX ME HERE in the failed logs but I see the open pidfd one. I wonder if #18462 changes the behaviour, I will add this to my PR.

[vrothberg]

That is surprising.

[vrothberg]

@Luap99 how's it going? News on the bug?

[Luap99]

I don't know, this doesn't seem to make any difference. Neither did my cleanup patch.

You can just grep for open pidfd in this PR logs, i.e. https://api.cirrus-ci.com/v1/artifact/task/5572269305495552/html/int-podman-fedora-37-root-container-boltdb.log.html

It is a pretty bad flake, so far no idea how to reproduce though.

[Luap99]

Also keep in mind that this is a crun error log. It doesn't seem to cause podman to exit >0 so it must be some some code path were podman ignores oci runtime errors.