[mheon]

Every exec session run attached will, on exit, do two things: it will signal the associated podman exec that it is finished (to allow Podman to collect the exit code and exit), and spawn a cleanup process to clean up the exec session (in case the podman exec process died, we still need to clean up). If an exec session is created that exits almost instantly, but generates a large amount of output (e.g. prints thousands of lines), the cleanup process can potentially execute before podman exec has a chance to read the exit code, resulting in errors. Handle this by detecting if the cleanup process has already removed the exec session before handling the error from reading the exec exit code.

[NO NEW TESTS NEEDED] I have no idea how to test this in CI.

Fixes #13227

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [mheon]

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

[Luap99]

please start using errors.Is for new code

[Luap99]

same here use errors.Is instead of os.IsNotExist

[mheon]

Adding WIP this isn't doing what I expect

[mheon]

I think I fixed it, going to repush

[mheon]

@edsantiago PTAL, I have this running on a 1MT and it seems to be running through the reproducer without issue.

[edsantiago]

@thrix FYI

[edsantiago]

There's no way I can review the locking flow; and I shudder at the thought of building this in a CentOS container. I'll trust your testing. Two very minor comments.

[edsantiago]

I'm guessing that's a typo for "info"?

[edsantiago]

Repetitive comment; maybe clearer as "Container is entirely removed, so there's no need to save status. Exit without error."

[mheon]

@containers/podman-maintainers PTAL

[giuseppe]

If an exec session is created that exits almost instantly, but generates a large amount of output (e.g. prints thousands of lines), the cleanup process can potentially execute before podman exec has a chance to read the exit code, resulting in errors.

is it a race in conmon? I don' think it should call the cleanup process before reading all the container output and writing the exit code

[mheon]

@giuseppe I don't think so? My suspicion is this: Conmon sends all the output (a considerable amount, probably 10kb?) over the unix socket to Podman, detects that the container has exited (it does so almost instantly), and spawns the cleanup process. Podman, meanwhile, is still reading the container's output from the Unix socket and writing it to the terminal - which, per Ed's experimentation, seems to be the limiting factor (is writing to TTYs rate-limited in some way? I should look into this) and takes more time than it does for Conmon to finish sending output, write the exit file, and invoke the cleanup process. The cleanup process wakes up and runs in the background, reads (and removes) the exit file, and writes the exit code to the DB. Podman presumably finishes writing sometime during this, but the container lock forces it to wait until after the cleanup process is finished; by which point there was no exit file, hence error messages.

[mheon]

Can we merge this, or are there further questions?

[mheon]

@baude @Luap99 @vrothberg PTAL

[rhatdan]

LGTM

[baude]

/lgtm