kill: resync the container if runtime fails #16320
Conversation
|
[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 |
openshift-ci
bot
added
the
approved
There was a problem hiding this comment.
This will deadlock, at least one caller has already the container lock:
podman/libpod/container_api.go
Lines 218 to 226 in 47bcd10
Yes, SA #15492 |
No caller should have the lock at that point. |
Ah, OK. That Kill needs to be fixed then. Thanks for highlighting that! |
If the runtime fails to kill the container there is fair chance that the container has transitionted to another state or been removed already. Take the lock and resync the container to check for that to prevent reading old and potentially outdated state. [NO NEW TESTS NEEDED] Signed-off-by: Valentin Rothberg <[email protected]>
| @@ -370,15 +372,7 @@ func (r *ConmonOCIRuntime) KillContainer(ctr *Container, signal uint, all bool) | |||
| args = append(args, "kill", ctr.ID(), fmt.Sprintf("%d", signal)) | |||
| } | |||
| if err := utils.ExecCmdWithStdStreams(os.Stdin, os.Stdout, os.Stderr, env, r.path, args...); err != nil { | |||
| // Update container state - there's a chance we failed because | |||
| // the container exited in the meantime. | |||
| if err2 := r.UpdateContainerStatus(ctr); err2 != nil { | |||
There was a problem hiding this comment.
Can't do this, this is going to break things further up the stack re: Sigproxy. The update needs to remain here.
| logrus.Infof("Error updating status for container %s: %v", ctr.ID(), err2) | ||
| } | ||
| if ctr.ensureState(define.ContainerStateStopped, define.ContainerStateExited) { | ||
| return fmt.Errorf("%w: %s", define.ErrCtrStateInvalid, ctr.state.State) |
There was a problem hiding this comment.
This is also going to break SigProxy. We need this to remain ErrCtrStateInvalid.
There was a problem hiding this comment.
Can you elaborate a bit more? At least for kill and stop, the container is not locked, so we should not fiddle with the state.
Maybe we need to make this conditional?
There was a problem hiding this comment.
The container is locked during kill.
There was a problem hiding this comment.
I can see only one place where KillContainer is run unlocked. I'll fix that.
We should not be touching KillContainer as such.
There was a problem hiding this comment.
You're right. It's unlocked during stop but not kill.
There was a problem hiding this comment.
#16323 catches the last case where KillContainer is run unlocked
There was a problem hiding this comment.
I can see only one place where KillContainer is run unlocked. I'll fix that.
So you will fix #16142?
|
Closing, @mheon wants to take a shot at this. |
github-actions
bot
added
the
locked - please file new issue/PR
If the runtime fails to kill the container there is fair chance that the container has transitionted to another state or been removed already. Take the lock and resync the container to check for that to prevent reading old and potentially outdated state.
[NO NEW TESTS NEEDED]
Signed-off-by: Valentin Rothberg [email protected]
Does this PR introduce a user-facing change?
@containers/podman-maintainers PTAL