Do not error on signalling a just-stopped container #14533
Conversation
|
[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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
mheon
force-pushed
the
avoid_error_on_container_stop
branch
from
304ae8a to
bf2c0a0
Compare
| if ctr.state.State == define.ContainerStateExited { | ||
| return nil | ||
| if ctr.ensureState(define.ContainerStateStopped, define.ContainerStateExited) { | ||
| return define.ErrCtrStateInvalid |
There was a problem hiding this comment.
Does this get handled in the upstream, since previous check was no failure.
There was a problem hiding this comment.
Yeah, this is handled by sig-proxy
|
Looks like an issue pulling the system test image. #14529 might help? |
|
LGTM |
Previous PR containers#12394 tried to address this, but made a mistake: containers that have just exited do not move to the Exited state but rather the Stopped state - as such, the code would never have run (there is no way we start `podman kill`, and the container transitions to Exited while we are doing it - that requires holding the container lock, which Kill already does). Fix the code to check Stopped as well (we omit Exited entirely but it's a cheap check and our state logic could change in the future). Also, return an error, instead of exiting cleanly - the Kill failed, after all. ErrCtrStateInvalid is already handled by the sig-proxy logic so there won't be issues. [NO NEW TESTS NEEDED] This fixes a race that I cannot reproduce myself, and I have no idea how we'd repro in CI. Signed-off-by: Matthew Heon <[email protected]>
mheon
force-pushed
the
avoid_error_on_container_stop
branch
from
bf2c0a0 to
c77691f
Compare
|
Rebased and force pushed |
|
@edsantiago Seen this one before? |
|
Oh yeah, known flake, mentioned in #14359 (comment) but not addressed yet |
|
OK, flake, good. Last time all the integration tests failed so I assumed it was an actual infrastructure issue. |
|
Duh, wrong link #14359 (comment) And yes, it's a flake, but I also think it's broken infrastructure. Hard to tell with "internal error" |
|
Looks like this is going to go green. @containers/podman-maintainers LGTM/hold would be appreciated, would like to get a scratchbuild of this out for testing soon. |
openshift-ci
bot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/cherrypick v4.0-rhel |
|
@lsm5: new pull request created: #14727 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
github-actions
bot
added
the
locked - please file new issue/PR
Previous PR #12394 tried to address this, but made a mistake: containers that have just exited do not move to the Exited state but rather the Stopped state - as such, the code would never have run (there is no way we start
podman kill, and the containertransitions to Exited while we are doing it - that requires holding the container lock, which Kill already does).
Fix the code to check Stopped as well (we omit Exited entirely but it's a cheap check and our state logic could change in the
future). Also, return an error, instead of exiting cleanly - the Kill failed, after all. ErrCtrStateInvalid is already handled by the sig-proxy logic so there won't be issues.