Skip to content

Commit

Permalink
container kill: handle stopped/exited container
Browse files Browse the repository at this point in the history
The container lock is released before stopping/killing which implies
certain race conditions with, for instance, the cleanup process changing
the container state to stopped, exited or other states.

The (remaining) flakes seen in containers#16142 and containers#15367 strongly indicate a
race in between the stopping/killing a container and the cleanup
process.  To fix the flake make sure to ignore invalid-state errors.
An alternative fix would be to change `KillContainer` to not return such
errors at all but commit c77691f indicates an explicit desire to
have these errors being reported in the sig proxy.

[NO NEW TESTS NEEDED] as it's a race already covered by the system
tests.

Fixes: containers#16142
Fixes: containers#15367
Signed-off-by: Valentin Rothberg <[email protected]>
  • Loading branch information
vrothberg committed Jan 16, 2023
1 parent 6f919af commit 067442b
Showing 1 changed file with 13 additions and 8 deletions.
21 changes: 13 additions & 8 deletions libpod/oci_conmon_common.go
Original file line number Diff line number Diff line change
Expand Up @@ -429,15 +429,20 @@ func (r *ConmonOCIRuntime) StopContainer(ctr *Container, timeout uint, all bool)
}

if err := r.KillContainer(ctr, uint(unix.SIGKILL), all); err != nil {
// If the PID is 0, then the container is already stopped.
if ctr.state.PID == 0 {
return nil
}
// Again, check if the container is gone. If it is, exit cleanly.
if aliveErr := unix.Kill(ctr.state.PID, 0); errors.Is(aliveErr, unix.ESRCH) {
return nil
// Ignore the error if KillContainer complains about it already
// being stopped or exited. There's an inherent race with the
// cleanup process (see #16142).
if !(errors.Is(err, define.ErrCtrStateInvalid) && ctr.ensureState(define.ContainerStateStopped, define.ContainerStateExited)) {
// If the PID is 0, then the container is already stopped.
if ctr.state.PID == 0 {
return nil
}
// Again, check if the container is gone. If it is, exit cleanly.
if aliveErr := unix.Kill(ctr.state.PID, 0); errors.Is(aliveErr, unix.ESRCH) {
return nil
}
return fmt.Errorf("sending SIGKILL to container %s: %w", ctr.ID(), err)
}
return fmt.Errorf("sending SIGKILL to container %s: %w", ctr.ID(), err)
}

// Give runtime a few seconds to make it happen
Expand Down

0 comments on commit 067442b

Please sign in to comment.