daemon: Pass context through container kill operations

Drop cancellation at the kill boundary because killing starts
irreversible task and cleanup work that must finish after request
cancellation.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

daemon/cluster/executor/backend.go

@@ -46,7 +46,7 @@ type Backend interface {
 	ContainerInspect(ctx context.Context, name string, options backend.ContainerInspectOptions) (_ *container.InspectResponse, desiredMACAddress network.HardwareAddr, _ error)
 	ContainerWait(ctx context.Context, name string, condition container.WaitCondition) (<-chan containerpkg.StateStatus, error)
 	ContainerRm(name string, config *backend.ContainerRmConfig) error
-	ContainerKill(name string, sig string) error
+	ContainerKill(ctx context.Context, name string, sig string) error
 	SetContainerDependencyStore(name string, store exec.DependencyGetter) error
 	SetContainerSecretReferences(name string, refs []*swarm.SecretReference) error
 	SetContainerConfigReferences(name string, refs []*swarm.ConfigReference) error

daemon/cluster/executor/container/adapter.go

@@ -437,7 +437,7 @@ func (c *containerAdapter) shutdown(ctx context.Context) error {
 }
 
 func (c *containerAdapter) terminate(ctx context.Context) error {
-	return c.backend.ContainerKill(c.container.name(), syscall.SIGKILL.String())
+	return c.backend.ContainerKill(ctx, c.container.name(), syscall.SIGKILL.String())
 }
 
 func (c *containerAdapter) remove(ctx context.Context) error {

daemon/delete.go

@@ -95,7 +95,7 @@ func (daemon *Daemon) cleanupContainer(ctr *container.Container, config backend.
 				return errdefs.Conflict(fmt.Errorf("container is %s: stop the container before removing or force remove", ctr.State.State()))
 			}
 		}
-		if err := daemon.Kill(ctr); err != nil && !isNotRunning(err) {
+		if err := daemon.kill(context.TODO(), ctr); err != nil && !isNotRunning(err) {
 			return fmt.Errorf("could not kill container: %w", err)
 		}
 	}

daemon/kill.go

@@ -33,7 +33,7 @@ func (errNoSuchProcess) NotFound() {}
 // If no signal is given, then Kill with SIGKILL and wait
 // for the container to exit.
 // If a signal is given, then just send it to the container and return.
-func (daemon *Daemon) ContainerKill(name, stopSignal string) error {
+func (daemon *Daemon) ContainerKill(ctx context.Context, name, stopSignal string) error {
 	var (
 		err error
 		sig = syscall.SIGKILL
@@ -53,9 +53,9 @@ func (daemon *Daemon) ContainerKill(name, stopSignal string) error {
 	}
 	if sig == syscall.SIGKILL {
 		// perform regular Kill (SIGKILL + wait())
-		return daemon.Kill(container)
+		return daemon.kill(ctx, container)
 	}
-	return daemon.killWithSignal(container, sig)
+	return daemon.killWithSignal(ctx, container, sig)
 }
 
 // killWithSignal sends the container the given signal. This wrapper for the
@@ -63,8 +63,9 @@ func (daemon *Daemon) ContainerKill(name, stopSignal string) error {
 // to send the signal. An error is returned if the container is paused
 // or not running, or if there is a problem returned from the
 // underlying kill command.
-func (daemon *Daemon) killWithSignal(container *containerpkg.Container, stopSignal syscall.Signal) error {
-	log.G(context.TODO()).WithFields(log.Fields{
+func (daemon *Daemon) killWithSignal(ctx context.Context, container *containerpkg.Container, stopSignal syscall.Signal) error {
+	ctx = context.WithoutCancel(ctx)
+	log.G(ctx).WithFields(log.Fields{
 		"signal":    int(stopSignal),
 		"container": container.ID,
 	}).Debugf("sending signal %[1]d (%[1]s) to container", stopSignal)
@@ -93,8 +94,8 @@ func (daemon *Daemon) killWithSignal(container *containerpkg.Container, stopSign
 
 	if !daemon.IsShuttingDown() {
 		container.HasBeenManuallyStopped = true
-		if err := container.CheckpointTo(context.WithoutCancel(context.TODO()), daemon.containersReplica); err != nil {
-			log.G(context.TODO()).WithFields(log.Fields{
+		if err := container.CheckpointTo(ctx, daemon.containersReplica); err != nil {
+			log.G(ctx).WithFields(log.Fields{
 				"error":     err,
 				"container": container.ID,
 			}).Warn("error checkpointing container state")
@@ -108,10 +109,10 @@ func (daemon *Daemon) killWithSignal(container *containerpkg.Container, stopSign
 		return nil
 	}
 
-	if err := task.Kill(context.Background(), stopSignal); err != nil {
+	if err := task.Kill(ctx, stopSignal); err != nil {
 		if cerrdefs.IsNotFound(err) {
 			unpause = false
-			log.G(context.TODO()).WithFields(log.Fields{
+			log.G(ctx).WithFields(log.Fields{
 				"error":     err,
 				"container": container.ID,
 				"action":    "kill",
@@ -126,19 +127,19 @@ func (daemon *Daemon) killWithSignal(container *containerpkg.Container, stopSign
 				if container.Config.StopTimeout != nil {
 					stopTimeout = *container.Config.StopTimeout
 				}
-				var ctx context.Context
+				var waitCtx context.Context
 				var cancel context.CancelFunc
 				if stopTimeout >= 0 {
-					ctx, cancel = context.WithTimeout(context.TODO(), time.Duration(stopTimeout)*time.Second)
+					waitCtx, cancel = context.WithTimeout(ctx, time.Duration(stopTimeout)*time.Second)
 				} else {
-					ctx, cancel = context.WithCancel(context.TODO())
+					waitCtx, cancel = context.WithCancel(ctx)
 				}
 
 				defer cancel()
-				s := <-container.State.Wait(ctx, containertypes.WaitConditionNotRunning)
+				s := <-container.State.Wait(waitCtx, containertypes.WaitConditionNotRunning)
 				if s.Err() != nil {
 					if err := daemon.handleContainerExit(container, nil); err != nil {
-						log.G(context.TODO()).WithFields(log.Fields{
+						log.G(waitCtx).WithFields(log.Fields{
 							"error":     err,
 							"container": container.ID,
 							"action":    "kill",
@@ -153,8 +154,8 @@ func (daemon *Daemon) killWithSignal(container *containerpkg.Container, stopSign
 
 	if unpause {
 		// above kill signal will be sent once resume is finished
-		if err := task.Resume(context.Background()); err != nil {
-			log.G(context.TODO()).WithFields(log.Fields{
+		if err := task.Resume(ctx); err != nil {
+			log.G(ctx).WithFields(log.Fields{
 				"error":     err,
 				"container": container.ID,
 				"action":    "kill",
@@ -168,14 +169,14 @@ func (daemon *Daemon) killWithSignal(container *containerpkg.Container, stopSign
 	return nil
 }
 
-// Kill forcefully terminates a container.
-func (daemon *Daemon) Kill(container *containerpkg.Container) error {
+func (daemon *Daemon) kill(ctx context.Context, container *containerpkg.Container) error {
+	ctx = context.WithoutCancel(ctx)
 	if !container.State.IsRunning() {
 		return errNotRunning(container.ID)
 	}
 
 	// 1. Send SIGKILL
-	if err := daemon.killPossiblyDeadProcess(container, syscall.SIGKILL); err != nil {
+	if err := daemon.killPossiblyDeadProcess(ctx, container, syscall.SIGKILL); err != nil {
 		// kill failed, check if process is no longer running.
 		if errors.As(err, &errNoSuchProcess{}) {
 			return nil
@@ -187,15 +188,15 @@ func (daemon *Daemon) Kill(container *containerpkg.Container) error {
 		waitTimeout = 75 * time.Second // runhcs can be sloooooow.
 	}
 
-	ctx, cancel := context.WithTimeout(context.Background(), waitTimeout)
+	waitCtx, cancel := context.WithTimeout(ctx, waitTimeout)
 	defer cancel()
 
-	status := <-container.State.Wait(ctx, containertypes.WaitConditionNotRunning)
+	status := <-container.State.Wait(waitCtx, containertypes.WaitConditionNotRunning)
 	if status.Err() == nil {
 		return nil
 	}
 
-	log.G(ctx).WithFields(log.Fields{"error": status.Err(), "container": container.ID}).Warnf("Container failed to exit within %v of kill - trying direct SIGKILL", waitTimeout)
+	log.G(waitCtx).WithFields(log.Fields{"error": status.Err(), "container": container.ID}).Warnf("Container failed to exit within %v of kill - trying direct SIGKILL", waitTimeout)
 
 	if err := killProcessDirectly(container); err != nil {
 		if errors.As(err, &errNoSuchProcess{}) {
@@ -205,21 +206,22 @@ func (daemon *Daemon) Kill(container *containerpkg.Container) error {
 	}
 
 	// wait for container to exit one last time, if it doesn't then kill didn't work, so return error
-	ctx2, cancel2 := context.WithTimeout(context.Background(), 2*time.Second)
-	defer cancel2()
+	finalWaitCtx, cancel := context.WithTimeout(ctx, 2*time.Second)
+	defer cancel()
 
-	if status := <-container.State.Wait(ctx2, containertypes.WaitConditionNotRunning); status.Err() != nil {
+	if status := <-container.State.Wait(finalWaitCtx, containertypes.WaitConditionNotRunning); status.Err() != nil {
 		return errors.New("tried to kill container, but did not receive an exit event")
 	}
 	return nil
 }
 
 // killPossiblyDeadProcess is a wrapper around killSig() suppressing "no such process" error.
-func (daemon *Daemon) killPossiblyDeadProcess(container *containerpkg.Container, sig syscall.Signal) error {
-	err := daemon.killWithSignal(container, sig)
+func (daemon *Daemon) killPossiblyDeadProcess(ctx context.Context, container *containerpkg.Container, sig syscall.Signal) error {
+	ctx = context.WithoutCancel(ctx)
+	err := daemon.killWithSignal(ctx, container, sig)
 	if cerrdefs.IsNotFound(err) {
 		err = errNoSuchProcess{container.State.GetPID(), sig}
-		log.G(context.TODO()).Debug(err)
+		log.G(ctx).Debug(err)
 		return err
 	}
 	return err

daemon/kill_test.go

@@ -20,6 +20,22 @@ type notFoundKillTask struct {
 	deleteCalled chan struct{}
 }
 
+type killContextKey struct{}
+
+type contextKillTask struct {
+	libcontainerdtypes.Task
+	killContext chan context.Context
+}
+
+func (t *contextKillTask) Pid() uint32 {
+	return 1
+}
+
+func (t *contextKillTask) Kill(ctx context.Context, _ syscall.Signal) error {
+	t.killContext <- ctx
+	return nil
+}
+
 func (t *notFoundKillTask) Pid() uint32 {
 	return 1
 }
@@ -49,7 +65,7 @@ func TestKillWithSignalWaitsIndefinitelyForDelayedExit(t *testing.T) {
 		shutdown:      true,
 	}
 
-	err := daemon.killWithSignal(ctr, syscall.SIGKILL)
+	err := daemon.killWithSignal(t.Context(), ctr, syscall.SIGKILL)
 	assert.NilError(t, err)
 
 	select {
@@ -62,3 +78,30 @@ func TestKillWithSignalWaitsIndefinitelyForDelayedExit(t *testing.T) {
 	ctr.State.SetStopped(&container.ExitStatus{})
 	ctr.Unlock()
 }
+
+func TestKillWithSignalDetachesContext(t *testing.T) {
+	task := &contextKillTask{killContext: make(chan context.Context, 1)}
+	ctr := container.NewBaseContainer(t.Name(), t.TempDir())
+	ctr.Config = &containertypes.Config{}
+	ctr.HostConfig = &containertypes.HostConfig{}
+	ctr.Lock()
+	ctr.State.SetRunning(nil, task, time.Now())
+	ctr.Unlock()
+
+	daemon := &Daemon{
+		EventsService: events.New(),
+		shutdown:      true,
+	}
+
+	const contextValue = "value"
+	ctx := context.WithValue(t.Context(), killContextKey{}, contextValue)
+	ctx, cancel := context.WithCancel(ctx)
+	cancel()
+
+	err := daemon.killWithSignal(ctx, ctr, syscall.SIGKILL)
+	assert.NilError(t, err)
+
+	killCtx := <-task.killContext
+	assert.NilError(t, killCtx.Err())
+	assert.Equal(t, killCtx.Value(killContextKey{}), contextValue)
+}

daemon/server/router/container/backend.go

@@ -33,7 +33,7 @@ type copyBackend interface {
 // stateBackend includes functions to implement to provide container state lifecycle functionality.
 type stateBackend interface {
 	ContainerCreate(ctx context.Context, config backend.ContainerCreateConfig) (container.CreateResponse, error)
-	ContainerKill(name string, signal string) error
+	ContainerKill(ctx context.Context, name string, signal string) error
 	ContainerPause(name string) error
 	ContainerRename(oldName, newName string) error
 	ContainerResize(ctx context.Context, name string, height, width uint32) error

daemon/server/router/container/container_routes.go

@@ -336,13 +336,13 @@ func (c *containerRouter) postContainersStop(ctx context.Context, w http.Respons
 	return nil
 }
 
-func (c *containerRouter) postContainersKill(_ context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error {
+func (c *containerRouter) postContainersKill(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error {
 	if err := httputils.ParseForm(r); err != nil {
 		return err
 	}
 
 	name := vars["name"]
-	if err := c.backend.ContainerKill(name, r.Form.Get("signal")); err != nil {
+	if err := c.backend.ContainerKill(ctx, name, r.Form.Get("signal")); err != nil {
 		return errors.Wrapf(err, "cannot kill container: %s", name)
 	}
 

daemon/stop.go

@@ -83,7 +83,7 @@ func (daemon *Daemon) containerStop(ctx context.Context, ctr *container.Containe
 	}()
 
 	// 1. Send a stop signal
-	err := daemon.killPossiblyDeadProcess(ctr, stopSignal)
+	err := daemon.killPossiblyDeadProcess(ctx, ctr, stopSignal)
 	if err != nil {
 		wait = 2 * time.Second
 	}
@@ -115,7 +115,7 @@ func (daemon *Daemon) containerStop(ctx context.Context, ctr *container.Containe
 	log.G(ctx).WithField("container", ctr.ID).Infof("Container failed to exit within %s of signal %d - using the force", wait, stopSignal)
 
 	// Stop either failed or container didn't exit, so fallback to kill.
-	if err := daemon.Kill(ctr); err != nil {
+	if err := daemon.kill(ctx, ctr); err != nil {
 		// got a kill error, but give container 2 more seconds to exit just in case
 		subCtx, cancel := context.WithTimeout(ctx, 2*time.Second)
 		defer cancel()