Skip to content

Commit

Permalink
docker: kill signal API should include timeout context
Browse files Browse the repository at this point in the history
When the Docker driver kills as task, we send a request via the Docker API for
dockerd to fire the signal. We send that signal and then block for the
`kill_timeout` waiting for the container to exit. But if the Docker API
blocks, we will block indefinitely because we haven't configured the API call
with the same timeout.

This changeset is a minimal intervention to add the timeout to the Docker API
call _only_ when we have the `kill_timeout` set. Future work should examine
whether we should be threading contexts through other `go-dockerclient` API
calls.
  • Loading branch information
tgross committed Dec 2, 2020
1 parent cf6055e commit 3f9cf73
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 6 deletions.
2 changes: 1 addition & 1 deletion drivers/docker/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -1461,7 +1461,7 @@ func (d *Driver) SignalTask(taskID string, signal string) error {
return fmt.Errorf("failed to parse signal: %v", err)
}

return h.Signal(sig)
return h.Signal(context.Background(), sig)
}

func (d *Driver) ExecTask(taskID string, cmd []string, timeout time.Duration) (*drivers.ExecTaskResult, error) {
Expand Down
14 changes: 9 additions & 5 deletions drivers/docker/handle.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ func (h *taskHandle) Exec(ctx context.Context, cmd string, args []string) (*driv
return execResult, nil
}

func (h *taskHandle) Signal(s os.Signal) error {
func (h *taskHandle) Signal(ctx context.Context, s os.Signal) error {
// Convert types
sysSig, ok := s.(syscall.Signal)
if !ok {
Expand All @@ -116,8 +116,9 @@ func (h *taskHandle) Signal(s os.Signal) error {

dockerSignal := docker.Signal(sysSig)
opts := docker.KillContainerOptions{
ID: h.containerID,
Signal: dockerSignal,
ID: h.containerID,
Signal: dockerSignal,
Context: ctx,
}
return h.client.KillContainer(opts)

Expand All @@ -127,7 +128,10 @@ func (h *taskHandle) Signal(s os.Signal) error {
func (h *taskHandle) Kill(killTimeout time.Duration, signal os.Signal) error {
// Only send signal if killTimeout is set, otherwise stop container
if killTimeout > 0 {
if err := h.Signal(signal); err != nil {
ctx, cancel := context.WithTimeout(context.Background(), killTimeout)
defer cancel()

if err := h.Signal(ctx, signal); err != nil {
// Container has already been removed.
if strings.Contains(err.Error(), NoSuchContainerError) {
h.logger.Debug("attempted to signal nonexistent container")
Expand All @@ -146,7 +150,7 @@ func (h *taskHandle) Kill(killTimeout time.Duration, signal os.Signal) error {
select {
case <-h.waitCh:
return nil
case <-time.After(killTimeout):
case <-ctx.Done():
}
}

Expand Down

0 comments on commit 3f9cf73

Please sign in to comment.