Merge pull request #43563 from thaJeztah/less_signal_conversions

pass syscall.Signal for stop-signals to reduce type conversions

api/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(config types.ContainerCreateConfig) (container.CreateResponse, error)
-	ContainerKill(name string, sig uint64) error
+	ContainerKill(name string, signal string) error
 	ContainerPause(name string) error
 	ContainerRename(oldName, newName string) error
 	ContainerResize(name string, height, width int) error

api/server/router/container/container_routes.go

@@ -7,7 +7,6 @@ import (
 	"io"
 	"net/http"
 	"strconv"
-	"syscall"
 
 	"github.com/containerd/containerd/platforms"
 	"github.com/docker/docker/api/server/httpstatus"
@@ -254,18 +253,8 @@ func (s *containerRouter) postContainersKill(ctx context.Context, w http.Respons
 		return err
 	}
 
-	var sig syscall.Signal
 	name := vars["name"]
-
-	// If we have a signal, look at it. Otherwise, do nothing
-	if sigStr := r.Form.Get("signal"); sigStr != "" {
-		var err error
-		if sig, err = signal.ParseSignal(sigStr); err != nil {
-			return errdefs.InvalidParameter(err)
-		}
-	}
-
-	if err := s.backend.ContainerKill(name, uint64(sig)); err != nil {
+	if err := s.backend.ContainerKill(name, r.Form.Get("signal")); err != nil {
 		var isStopped bool
 		if errdefs.IsConflict(err) {
 			isStopped = true

builder/builder.go

@@ -65,7 +65,7 @@ type ExecBackend interface {
 	// ContainerRm removes a container specified by `id`.
 	ContainerRm(name string, config *types.ContainerRmConfig) error
 	// ContainerKill stops the container execution abruptly.
-	ContainerKill(containerID string, sig uint64) error
+	ContainerKill(containerID string, sig string) error
 	// ContainerStart starts a new container
 	ContainerStart(containerID string, hostConfig *container.HostConfig, checkpoint string, checkpointDir string) error
 	// ContainerWait stops processing until the given container is stopped.

builder/dockerfile/containerbackend.go

@@ -61,7 +61,7 @@ func (c *containerManager) Run(ctx context.Context, cID string, stdout, stderr i
 		select {
 		case <-ctx.Done():
 			logrus.Debugln("Build cancelled, killing and removing container:", cID)
-			c.backend.ContainerKill(cID, 0)
+			c.backend.ContainerKill(cID, "")
 			c.removeContainer(cID, stdout)
 			cancelErrCh <- errCancelled
 		case <-finished:

builder/dockerfile/mockbackend_test.go

@@ -46,7 +46,7 @@ func (m *MockBackend) CommitBuildStep(c backend.CommitConfig) (image.ID, error)
 	return "", nil
 }
 
-func (m *MockBackend) ContainerKill(containerID string, sig uint64) error {
+func (m *MockBackend) ContainerKill(containerID string, sig string) error {
 	return nil
 }
 
@@ -129,7 +129,7 @@ func (l *mockLayer) NewRWLayer() (builder.RWLayer, error) {
 }
 
 func (l *mockLayer) DiffID() layer.DiffID {
-	return layer.DiffID("abcdef")
+	return "abcdef"
 }
 
 type mockRWLayer struct {

container/container.go

@@ -511,16 +511,16 @@ func (container *Container) IsDestinationMounted(destination string) bool {
 }
 
 // StopSignal returns the signal used to stop the container.
-func (container *Container) StopSignal() int {
+func (container *Container) StopSignal() syscall.Signal {
 	var stopSignal syscall.Signal
 	if container.Config.StopSignal != "" {
 		stopSignal, _ = signal.ParseSignal(container.Config.StopSignal)
 	}
 
-	if int(stopSignal) == 0 {
+	if stopSignal == 0 {
 		stopSignal, _ = signal.ParseSignal(defaultStopSignal)
 	}
-	return int(stopSignal)
+	return stopSignal
 }
 
 // StopTimeout returns the timeout (in seconds) used to stop the container.

container/container_unit_test.go

@@ -24,7 +24,7 @@ func TestContainerStopSignal(t *testing.T) {
 	}
 
 	s := c.StopSignal()
-	if s != int(def) {
+	if s != def {
 		t.Fatalf("Expected %v, got %v", def, s)
 	}
 

daemon/cluster/executor/backend.go

@@ -45,7 +45,7 @@ type Backend interface {
 	ContainerInspectCurrent(name string, size bool) (*types.ContainerJSON, error)
 	ContainerWait(ctx context.Context, name string, condition containerpkg.WaitCondition) (<-chan containerpkg.StateStatus, error)
 	ContainerRm(name string, config *types.ContainerRmConfig) error
-	ContainerKill(name string, sig uint64) error
+	ContainerKill(name string, sig string) error
 	SetContainerDependencyStore(name string, store exec.DependencyGetter) error
 	SetContainerSecretReferences(name string, refs []*swarmtypes.SecretReference) error
 	SetContainerConfigReferences(name string, refs []*swarmtypes.ConfigReference) error

daemon/cluster/executor/container/adapter.go

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

daemon/container_operations_unix.go

@@ -8,6 +8,7 @@ import (
 	"os"
 	"path/filepath"
 	"strconv"
+	"syscall"
 
 	"github.com/docker/docker/container"
 	"github.com/docker/docker/daemon/links"
@@ -336,24 +337,25 @@ func (daemon *Daemon) cleanupSecretDir(c *container.Container) {
 
 func killProcessDirectly(container *container.Container) error {
 	pid := container.GetPID()
-	// Ensure that we don't kill ourselves
 	if pid == 0 {
+		// Ensure that we don't kill ourselves
 		return nil
 	}
 
-	if err := unix.Kill(pid, 9); err != nil {
+	if err := unix.Kill(pid, syscall.SIGKILL); err != nil {
 		if err != unix.ESRCH {
-			return err
+			return errdefs.System(err)
 		}
-		e := errNoSuchProcess{pid, 9}
-		logrus.WithError(e).WithField("container", container.ID).Debug("no such process")
-		return e
+		err = errNoSuchProcess{pid, syscall.SIGKILL}
+		logrus.WithError(err).WithField("container", container.ID).Debug("no such process")
+		return err
 	}
 
 	// In case there were some exceptions(e.g., state of zombie and D)
 	if system.IsProcessAlive(pid) {
 		// Since we can not kill a zombie pid, add zombie check here
 		isZombie, err := system.IsProcessZombie(pid)
+		// TODO(thaJeztah) should we ignore os.IsNotExist() here? ("/proc/<pid>/stat" will be gone if the process exited)
 		if err != nil {
 			logrus.WithError(err).WithField("container", container.ID).Warn("Container state is invalid")
 			return err

daemon/exec.go

@@ -279,7 +279,7 @@ func (daemon *Daemon) ContainerExecStart(ctx context.Context, name string, stdin
 	select {
 	case <-ctx.Done():
 		logrus.Debugf("Sending TERM signal to process %v in container %v", name, c.ID)
-		daemon.containerd.SignalProcess(ctx, c.ID, name, int(signal.SignalMap["TERM"]))
+		daemon.containerd.SignalProcess(ctx, c.ID, name, signal.SignalMap["TERM"])
 
 		timeout := time.NewTimer(termProcessTimeout)
 		defer timeout.Stop()
@@ -287,7 +287,7 @@ func (daemon *Daemon) ContainerExecStart(ctx context.Context, name string, stdin
 		select {
 		case <-timeout.C:
 			logrus.Infof("Container %v, process %v failed to exit within %v of signal TERM - using the force", c.ID, name, termProcessTimeout)
-			daemon.containerd.SignalProcess(ctx, c.ID, name, int(signal.SignalMap["KILL"]))
+			daemon.containerd.SignalProcess(ctx, c.ID, name, signal.SignalMap["KILL"])
 		case <-attachErr:
 			// TERM signal worked
 		}

daemon/kill.go

@@ -17,41 +17,42 @@ import (
 
 type errNoSuchProcess struct {
 	pid    int
-	signal int
+	signal syscall.Signal
 }
 
 func (e errNoSuchProcess) Error() string {
-	return fmt.Sprintf("Cannot kill process (pid=%d) with signal %d: no such process.", e.pid, e.signal)
+	return fmt.Sprintf("cannot kill process (pid=%d) with signal %d: no such process", e.pid, e.signal)
 }
 
 func (errNoSuchProcess) NotFound() {}
 
-// isErrNoSuchProcess returns true if the error
-// is an instance of errNoSuchProcess.
-func isErrNoSuchProcess(err error) bool {
-	_, ok := err.(errNoSuchProcess)
-	return ok
-}
-
 // ContainerKill sends signal to the container
-// If no signal is given (sig 0), then Kill with SIGKILL and wait
+// 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 string, sig uint64) error {
+func (daemon *Daemon) ContainerKill(name, stopSignal string) error {
+	var (
+		err error
+		sig = syscall.SIGKILL
+	)
+	if stopSignal != "" {
+		sig, err = signal.ParseSignal(stopSignal)
+		if err != nil {
+			return errdefs.InvalidParameter(err)
+		}
+		if !signal.ValidSignalForPlatform(sig) {
+			return errdefs.InvalidParameter(errors.Errorf("the %s daemon does not support signal %d", runtime.GOOS, sig))
+		}
+	}
 	container, err := daemon.GetContainer(name)
 	if err != nil {
 		return err
 	}
-
-	if sig != 0 && !signal.ValidSignalForPlatform(syscall.Signal(sig)) {
-		return fmt.Errorf("The %s daemon does not support signal %d", runtime.GOOS, sig)
-	}
-
-	// If no signal is passed, or SIGKILL, perform regular Kill (SIGKILL + wait())
-	if sig == 0 || syscall.Signal(sig) == syscall.SIGKILL {
+	if sig == syscall.SIGKILL {
+		// perform regular Kill (SIGKILL + wait())
 		return daemon.Kill(container)
 	}
-	return daemon.killWithSignal(container, int(sig))
+	return daemon.killWithSignal(container, sig)
 }
 
 // killWithSignal sends the container the given signal. This wrapper for the
@@ -59,8 +60,8 @@ func (daemon *Daemon) ContainerKill(name string, sig uint64) 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, sig int) error {
-	logrus.Debugf("Sending kill signal %d to container %s", sig, container.ID)
+func (daemon *Daemon) killWithSignal(container *containerpkg.Container, stopSignal syscall.Signal) error {
+	logrus.Debugf("Sending kill signal %d to container %s", stopSignal, container.ID)
 	container.Lock()
 	defer container.Unlock()
 
@@ -69,12 +70,12 @@ func (daemon *Daemon) killWithSignal(container *containerpkg.Container, sig int)
 	}
 
 	var unpause bool
-	if container.Config.StopSignal != "" && syscall.Signal(sig) != syscall.SIGKILL {
+	if container.Config.StopSignal != "" && stopSignal != syscall.SIGKILL {
 		containerStopSignal, err := signal.ParseSignal(container.Config.StopSignal)
 		if err != nil {
 			return err
 		}
-		if containerStopSignal == syscall.Signal(sig) {
+		if containerStopSignal == stopSignal {
 			container.ExitOnNext()
 			unpause = container.Paused
 		}
@@ -95,7 +96,8 @@ func (daemon *Daemon) killWithSignal(container *containerpkg.Container, sig int)
 		return nil
 	}
 
-	if err := daemon.kill(container, sig); err != nil {
+	err := daemon.containerd.SignalProcess(context.Background(), container.ID, libcontainerdtypes.InitProcessName, stopSignal)
+	if err != nil {
 		if errdefs.IsNotFound(err) {
 			unpause = false
 			logrus.WithError(err).WithField("container", container.ID).WithField("action", "kill").Debug("container kill failed because of 'container not found' or 'no such process'")
@@ -125,7 +127,7 @@ func (daemon *Daemon) killWithSignal(container *containerpkg.Container, sig int)
 	}
 
 	attributes := map[string]string{
-		"signal": fmt.Sprintf("%d", sig),
+		"signal": fmt.Sprintf("%d", stopSignal),
 	}
 	daemon.LogContainerEventWithAttributes(container, "kill", attributes)
 	return nil
@@ -138,9 +140,9 @@ func (daemon *Daemon) Kill(container *containerpkg.Container) error {
 	}
 
 	// 1. Send SIGKILL
-	if err := daemon.killPossiblyDeadProcess(container, int(syscall.SIGKILL)); err != nil {
+	if err := daemon.killPossiblyDeadProcess(container, syscall.SIGKILL); err != nil {
 		// kill failed, check if process is no longer running.
-		if isErrNoSuchProcess(err) {
+		if errors.As(err, &errNoSuchProcess{}) {
 			return nil
 		}
 	}
@@ -156,7 +158,7 @@ func (daemon *Daemon) Kill(container *containerpkg.Container) error {
 	logrus.WithError(status.Err()).WithField("container", container.ID).Error("Container failed to exit within 10 seconds of kill - trying direct SIGKILL")
 
 	if err := killProcessDirectly(container); err != nil {
-		if isErrNoSuchProcess(err) {
+		if errors.As(err, &errNoSuchProcess{}) {
 			return nil
 		}
 		return err
@@ -173,16 +175,12 @@ func (daemon *Daemon) Kill(container *containerpkg.Container) error {
 }
 
 // killPossibleDeadProcess is a wrapper around killSig() suppressing "no such process" error.
-func (daemon *Daemon) killPossiblyDeadProcess(container *containerpkg.Container, sig int) error {
+func (daemon *Daemon) killPossiblyDeadProcess(container *containerpkg.Container, sig syscall.Signal) error {
 	err := daemon.killWithSignal(container, sig)
 	if errdefs.IsNotFound(err) {
-		e := errNoSuchProcess{container.GetPID(), sig}
-		logrus.Debug(e)
-		return e
+		err = errNoSuchProcess{container.GetPID(), sig}
+		logrus.Debug(err)
+		return err
 	}
 	return err
 }
-
-func (daemon *Daemon) kill(c *containerpkg.Container, sig int) error {
-	return daemon.containerd.SignalProcess(context.Background(), c.ID, libcontainerdtypes.InitProcessName, sig)
-}

daemon/stop.go

@@ -50,7 +50,7 @@ func (daemon *Daemon) containerStop(ctx context.Context, ctr *container.Containe
 		if err != nil {
 			return errdefs.InvalidParameter(err)
 		}
-		stopSignal = int(sig)
+		stopSignal = sig
 	}
 	if options.Timeout != nil {
 		stopTimeout = *options.Timeout

daemon/util_test.go

@@ -5,6 +5,7 @@ package daemon
 
 import (
 	"context"
+	"syscall"
 	"time"
 
 	"github.com/containerd/containerd"
@@ -35,7 +36,7 @@ func (c *MockContainerdClient) Create(ctx context.Context, containerID string, s
 func (c *MockContainerdClient) Start(ctx context.Context, containerID, checkpointDir string, withStdin bool, attachStdio libcontainerdtypes.StdioCallback) (pid int, err error) {
 	return 0, nil
 }
-func (c *MockContainerdClient) SignalProcess(ctx context.Context, containerID, processID string, signal int) error {
+func (c *MockContainerdClient) SignalProcess(ctx context.Context, containerID, processID string, signal syscall.Signal) error {
 	return nil
 }
 func (c *MockContainerdClient) Exec(ctx context.Context, containerID, processID string, spec *specs.Process, withStdin bool, attachStdio libcontainerdtypes.StdioCallback) (int, error) {

libcontainerd/local/local_windows.go

@@ -688,7 +688,7 @@ func (c *client) Exec(ctx context.Context, containerID, processID string, spec *
 // SignalProcess handles `docker stop` on Windows. While Linux has support for
 // the full range of signals, signals aren't really implemented on Windows.
 // We fake supporting regular stop and -9 to force kill.
-func (c *client) SignalProcess(_ context.Context, containerID, processID string, signal int) error {
+func (c *client) SignalProcess(_ context.Context, containerID, processID string, signal syscall.Signal) error {
 	ctr, p, err := c.getProcess(containerID, processID)
 	if err != nil {
 		return err

libcontainerd/remote/client.go

@@ -333,12 +333,12 @@ func (c *client) Exec(ctx context.Context, containerID, processID string, spec *
 	return int(p.Pid()), nil
 }
 
-func (c *client) SignalProcess(ctx context.Context, containerID, processID string, signal int) error {
+func (c *client) SignalProcess(ctx context.Context, containerID, processID string, signal syscall.Signal) error {
 	p, err := c.getProcess(ctx, containerID, processID)
 	if err != nil {
 		return err
 	}
-	return wrapError(p.Kill(ctx, syscall.Signal(signal)))
+	return wrapError(p.Kill(ctx, signal))
 }
 
 func (c *client) ResizeTerminal(ctx context.Context, containerID, processID string, width, height int) error {

libcontainerd/types/types.go

@@ -2,6 +2,7 @@ package types // import "github.com/docker/docker/libcontainerd/types"
 
 import (
 	"context"
+	"syscall"
 	"time"
 
 	"github.com/containerd/containerd"
@@ -54,7 +55,7 @@ type Client interface {
 
 	Create(ctx context.Context, containerID string, spec *specs.Spec, shim string, runtimeOptions interface{}, opts ...containerd.NewContainerOpts) error
 	Start(ctx context.Context, containerID, checkpointDir string, withStdin bool, attachStdio StdioCallback) (pid int, err error)
-	SignalProcess(ctx context.Context, containerID, processID string, signal int) error
+	SignalProcess(ctx context.Context, containerID, processID string, signal syscall.Signal) error
 	Exec(ctx context.Context, containerID, processID string, spec *specs.Process, withStdin bool, attachStdio StdioCallback) (int, error)
 	ResizeTerminal(ctx context.Context, containerID, processID string, width, height int) error
 	CloseStdin(ctx context.Context, containerID, processID string) error

pkg/system/process_unix.go

@@ -33,6 +33,7 @@ func IsProcessZombie(pid int) (bool, error) {
 	statPath := fmt.Sprintf("/proc/%d/stat", pid)
 	dataBytes, err := os.ReadFile(statPath)
 	if err != nil {
+		// TODO(thaJeztah) should we ignore os.IsNotExist() here? ("/proc/<pid>/stat" will be gone if the process exited)
 		return false, err
 	}
 	data := string(dataBytes)

plugin/executor/containerd/containerd.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"io"
 	"sync"
+	"syscall"
 
 	"github.com/containerd/containerd"
 	"github.com/containerd/containerd/cio"
@@ -112,7 +113,7 @@ func (e *Executor) IsRunning(id string) (bool, error) {
 }
 
 // Signal sends the specified signal to the container
-func (e *Executor) Signal(id string, signal int) error {
+func (e *Executor) Signal(id string, signal syscall.Signal) error {
 	return e.client.SignalProcess(context.Background(), id, libcontainerdtypes.InitProcessName, signal)
 }
 

plugin/manager.go

@@ -11,6 +11,7 @@ import (
 	"sort"
 	"strings"
 	"sync"
+	"syscall"
 
 	"github.com/containerd/containerd/content"
 	"github.com/containerd/containerd/content/local"
@@ -37,7 +38,7 @@ type Executor interface {
 	Create(id string, spec specs.Spec, stdout, stderr io.WriteCloser) error
 	IsRunning(id string) (bool, error)
 	Restore(id string, stdout, stderr io.WriteCloser) (alive bool, err error)
-	Signal(id string, signal int) error
+	Signal(id string, signal syscall.Signal) error
 }
 
 // EndpointResolver provides looking up registry endpoints for pulling.

plugin/manager_linux.go

@@ -154,31 +154,30 @@ const shutdownTimeout = 10 * time.Second
 func shutdownPlugin(p *v2.Plugin, ec chan bool, executor Executor) {
 	pluginID := p.GetID()
 
-	err := executor.Signal(pluginID, int(unix.SIGTERM))
-	if err != nil {
+	if err := executor.Signal(pluginID, unix.SIGTERM); err != nil {
 		logrus.Errorf("Sending SIGTERM to plugin failed with error: %v", err)
-	} else {
+		return
+	}
+
+	timeout := time.NewTimer(shutdownTimeout)
+	defer timeout.Stop()
+
+	select {
+	case <-ec:
+		logrus.Debug("Clean shutdown of plugin")
+	case <-timeout.C:
+		logrus.Debug("Force shutdown plugin")
+		if err := executor.Signal(pluginID, unix.SIGKILL); err != nil {
+			logrus.Errorf("Sending SIGKILL to plugin failed with error: %v", err)
+		}
 
-		timeout := time.NewTimer(shutdownTimeout)
-		defer timeout.Stop()
+		timeout.Reset(shutdownTimeout)
 
 		select {
 		case <-ec:
-			logrus.Debug("Clean shutdown of plugin")
+			logrus.Debug("SIGKILL plugin shutdown")
 		case <-timeout.C:
-			logrus.Debug("Force shutdown plugin")
-			if err := executor.Signal(pluginID, int(unix.SIGKILL)); err != nil {
-				logrus.Errorf("Sending SIGKILL to plugin failed with error: %v", err)
-			}
-
-			timeout.Reset(shutdownTimeout)
-
-			select {
-			case <-ec:
-				logrus.Debug("SIGKILL plugin shutdown")
-			case <-timeout.C:
-				logrus.WithField("plugin", p.Name).Warn("Force shutdown plugin FAILED")
-			}
+			logrus.WithField("plugin", p.Name).Warn("Force shutdown plugin FAILED")
 		}
 	}
 }

plugin/manager_linux_test.go

@@ -5,6 +5,7 @@ import (
 	"net"
 	"os"
 	"path/filepath"
+	"syscall"
 	"testing"
 
 	"github.com/docker/docker/api/types"
@@ -87,24 +88,13 @@ func newTestPlugin(t *testing.T, name, cap, root string) *v2.Plugin {
 }
 
 type simpleExecutor struct {
+	Executor
 }
 
 func (e *simpleExecutor) Create(id string, spec specs.Spec, stdout, stderr io.WriteCloser) error {
 	return errors.New("Create failed")
 }
 
-func (e *simpleExecutor) Restore(id string, stdout, stderr io.WriteCloser) (bool, error) {
-	return false, nil
-}
-
-func (e *simpleExecutor) IsRunning(id string) (bool, error) {
-	return false, nil
-}
-
-func (e *simpleExecutor) Signal(id string, signal int) error {
-	return nil
-}
-
 func TestCreateFailed(t *testing.T) {
 	root, err := os.MkdirTemp("", "test-create-failed")
 	if err != nil {
@@ -165,7 +155,7 @@ func (e *executorWithRunning) Restore(id string, stdout, stderr io.WriteCloser)
 	return true, nil
 }
 
-func (e *executorWithRunning) Signal(id string, signal int) error {
+func (e *executorWithRunning) Signal(id string, signal syscall.Signal) error {
 	ch := e.exitChans[id]
 	ch <- struct{}{}
 	<-ch