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

kill: record exit code #14821

Closed
wants to merge 1 commit into from
Closed

Conversation

vrothberg
Copy link
Member

Make sure to record the exit code after killing a container.
Otherwise, a concurrent stop may not record the exit code
and yield the container unusable.

Fixes: #14761
Signed-off-by: Valentin Rothberg [email protected]

Does this PR introduce a user-facing change?

No release note as it's fixing a bug that never made it into any version.

None

@mheon @baude PTAL

Make sure to record the exit code after killing a container.
Otherwise, a concurrent `stop` may not record the exit code
and yield the container unusable.

Fixes: containers#14761
Signed-off-by: Valentin Rothberg <[email protected]>
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 4, 2022

[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:

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 4, 2022
c.newContainerEvent(events.Kill)

return c.save()
return c.recordExitCode()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

podman kill can be used to send any signal to the container, unless I am missing something c.recordExitCode() would block and wait for conmon to exit which seems wrong. Am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that is a fair point. I'll check what I can do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conmon spawns the cleanup process, that should record the exit code IMO and not kill.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I concur. It seems there are still some farts in the current logic. Will continue digging tomorrow.

libpod/container_internal.go Show resolved Hide resolved
@vrothberg
Copy link
Member Author

Closing as it's not the right way to go.

@vrothberg vrothberg closed this Jul 4, 2022
@vrothberg vrothberg deleted the fix-14761 branch July 4, 2022 15:05
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
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. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

podman wait: new timeout, possibly deadlock
2 participants