Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

podman wait: allow waiting for removal of containers #23646

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Aug 16, 2024

By default wait only waits for the exit of a container, there is really no way to make it wait for the removal too when the container was created with --rm. I though I found a clever way in 8a94331 but this is not working race free. While it works most of the time any other parallel process might call syncContainer() before the cleanup process holds the lock until it removes it. As such the wait hack to only update the state and not sync the exit file did not work so we can drop that.

However the test wants to wait for the removal to happen by the cleanup process and we can already say --condition=removing to do this but this will throw an error if the ctr was removed instead of counting this as success so fix that as well.

Fixes #23640

Does this PR introduce a user-facing change?

podman wait --condition=removing will no longer throw an error when the container is removed as this is expected.

By default wait only waits for the exit of a container, there is really
no way to make it wait for the removal too when the container was
created with --rm. I though I found a clever way in 8a94331 but this
is not working race free. While it works most of the time any other
parallel process might call syncContainer() before the cleanup process
holds the lock until it removes it. As such the wait hack to only update
the state and not sync the exit file did not work so we can drop that.

However the test wants to wait for the removal to happen by the cleanup
process and we can already say --condition=removing to do this but this
will throw an error if the ctr was removed instead of counting this as
success so fix that as well.

Fixes containers#23640

Signed-off-by: Paul Holzinger <[email protected]>
@openshift-ci openshift-ci bot added the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Aug 16, 2024
Copy link
Contributor

openshift-ci bot commented Aug 16, 2024

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@openshift-ci openshift-ci bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Aug 16, 2024
@Luap99
Copy link
Member Author

Luap99 commented Aug 16, 2024

@mheon PTAL, some follow up to my wait fixes. I thought my fix may work but now I know there is no way to have the normal wait wait for the removal first

@mheon
Copy link
Member

mheon commented Aug 16, 2024

LGTM

@rhatdan
Copy link
Member

rhatdan commented Aug 16, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 16, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 98d52b6 into containers:main Aug 16, 2024
83 checks passed
@Luap99 Luap99 deleted the wait-remove branch August 16, 2024 15:21
@stale-locking-app 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 15, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Nov 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

race(?) in podman run --rm then podman rm?
3 participants