Merge pull request #44304 from thaJeztah/sys_move_process

pkg/system: move process-functions to pkg/process, improve pkg/pidfile and reduce overlap

cmd/dockerd/daemon.go

@@ -139,8 +139,11 @@ func (cli *DaemonCli) start(opts *daemonOptions) (err error) {
 	potentiallyUnderRuntimeDir := []string{cli.Config.ExecRoot}
 
 	if cli.Pidfile != "" {
-		if err := pidfile.Write(cli.Pidfile); err != nil {
-			return errors.Wrap(err, "failed to start daemon")
+		if err = system.MkdirAll(filepath.Dir(cli.Pidfile), 0o755); err != nil {
+			return errors.Wrap(err, "failed to create pidfile directory")
+		}
+		if err = pidfile.Write(cli.Pidfile, os.Getpid()); err != nil {
+			return errors.Wrapf(err, "failed to start daemon, ensure docker is not running or delete %s", cli.Pidfile)
 		}
 		potentiallyUnderRuntimeDir = append(potentiallyUnderRuntimeDir, cli.Pidfile)
 		defer func() {

daemon/container_operations_unix.go

@@ -15,8 +15,8 @@ import (
 	"github.com/docker/docker/errdefs"
 	"github.com/docker/docker/libnetwork"
 	"github.com/docker/docker/pkg/idtools"
+	"github.com/docker/docker/pkg/process"
 	"github.com/docker/docker/pkg/stringid"
-	"github.com/docker/docker/pkg/system"
 	"github.com/docker/docker/runconfig"
 	"github.com/moby/sys/mount"
 	"github.com/opencontainers/selinux/go-selinux/label"
@@ -352,10 +352,9 @@ func killProcessDirectly(container *container.Container) error {
 	}
 
 	// In case there were some exceptions(e.g., state of zombie and D)
-	if system.IsProcessAlive(pid) {
+	if process.Alive(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)
+		isZombie, err := process.Zombie(pid)
 		if err != nil {
 			logrus.WithError(err).WithField("container", container.ID).Warn("Container state is invalid")
 			return err

libcontainerd/supervisor/remote_daemon.go

@@ -2,18 +2,18 @@ package supervisor // import "github.com/docker/docker/libcontainerd/supervisor"
 
 import (
 	"context"
-	"io"
 	"os"
 	"os/exec"
 	"path/filepath"
 	"runtime"
-	"strconv"
 	"strings"
 	"time"
 
 	"github.com/containerd/containerd"
 	"github.com/containerd/containerd/services/server/config"
 	"github.com/containerd/containerd/sys"
+	"github.com/docker/docker/pkg/pidfile"
+	"github.com/docker/docker/pkg/process"
 	"github.com/docker/docker/pkg/system"
 	"github.com/pelletier/go-toml"
 	"github.com/pkg/errors"
@@ -124,34 +124,6 @@ func (r *remote) WaitTimeout(d time.Duration) error {
 func (r *remote) Address() string {
 	return r.GRPC.Address
 }
-func (r *remote) getContainerdPid() (int, error) {
-	f, err := os.OpenFile(r.pidFile, os.O_RDWR, 0600)
-	if err != nil {
-		if os.IsNotExist(err) {
-			return -1, nil
-		}
-		return -1, err
-	}
-	defer f.Close()
-
-	b := make([]byte, 8)
-	n, err := f.Read(b)
-	if err != nil && err != io.EOF {
-		return -1, err
-	}
-
-	if n > 0 {
-		pid, err := strconv.ParseUint(string(b[:n]), 10, 64)
-		if err != nil {
-			return -1, err
-		}
-		if system.IsProcessAlive(int(pid)) {
-			return int(pid), nil
-		}
-	}
-
-	return -1, nil
-}
 
 func (r *remote) getContainerdConfig() (string, error) {
 	f, err := os.OpenFile(r.configFile, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0600)
@@ -167,23 +139,22 @@ func (r *remote) getContainerdConfig() (string, error) {
 }
 
 func (r *remote) startContainerd() error {
-	pid, err := r.getContainerdPid()
-	if err != nil {
+	pid, err := pidfile.Read(r.pidFile)
+	if err != nil && !errors.Is(err, os.ErrNotExist) {
 		return err
 	}
 
-	if pid != -1 {
+	if pid > 0 {
 		r.daemonPid = pid
 		r.logger.WithField("pid", pid).Infof("%s is still running", binaryName)
 		return nil
 	}
 
-	configFile, err := r.getContainerdConfig()
+	cfgFile, err := r.getContainerdConfig()
 	if err != nil {
 		return err
 	}
-
-	args := []string{"--config", configFile}
+	args := []string{"--config", cfgFile}
 
 	if r.logLevel != "" {
 		args = append(args, "--log-level", r.logLevel)
@@ -236,9 +207,9 @@ func (r *remote) startContainerd() error {
 		r.logger.WithError(err).Warn("failed to adjust OOM score")
 	}
 
-	err = os.WriteFile(r.pidFile, []byte(strconv.Itoa(r.daemonPid)), 0660)
+	err = pidfile.Write(r.pidFile, r.daemonPid)
 	if err != nil {
-		system.KillProcess(r.daemonPid)
+		process.Kill(r.daemonPid)
 		return errors.Wrap(err, "libcontainerd: failed to save daemon pid to disk")
 	}
 
@@ -357,7 +328,7 @@ func (r *remote) monitorDaemon(ctx context.Context) {
 			r.logger.WithError(err).WithField("binary", binaryName).Debug("daemon is not responding")
 
 			transientFailureCount++
-			if transientFailureCount < maxConnectionRetryCount || system.IsProcessAlive(r.daemonPid) {
+			if transientFailureCount < maxConnectionRetryCount || process.Alive(r.daemonPid) {
 				delay = time.Duration(transientFailureCount) * 200 * time.Millisecond
 				continue
 			}
@@ -365,7 +336,7 @@ func (r *remote) monitorDaemon(ctx context.Context) {
 			client = nil
 		}
 
-		if system.IsProcessAlive(r.daemonPid) {
+		if process.Alive(r.daemonPid) {
 			r.logger.WithField("pid", r.daemonPid).Info("killing and restarting containerd")
 			r.killDaemon()
 		}

libcontainerd/supervisor/remote_daemon_linux.go

@@ -7,7 +7,7 @@ import (
 	"time"
 
 	"github.com/containerd/containerd/defaults"
-	"github.com/docker/docker/pkg/system"
+	"github.com/docker/docker/pkg/process"
 )
 
 const (
@@ -35,13 +35,13 @@ func (r *remote) stopDaemon() {
 	syscall.Kill(r.daemonPid, syscall.SIGTERM)
 	// Wait up to 15secs for it to stop
 	for i := time.Duration(0); i < shutdownTimeout; i += time.Second {
-		if !system.IsProcessAlive(r.daemonPid) {
+		if !process.Alive(r.daemonPid) {
 			break
 		}
 		time.Sleep(time.Second)
 	}
 
-	if system.IsProcessAlive(r.daemonPid) {
+	if process.Alive(r.daemonPid) {
 		r.logger.WithField("pid", r.daemonPid).Warn("daemon didn't stop within 15 secs, killing it")
 		syscall.Kill(r.daemonPid, syscall.SIGKILL)
 	}
@@ -51,7 +51,7 @@ func (r *remote) killDaemon() {
 	// Try to get a stack trace
 	syscall.Kill(r.daemonPid, syscall.SIGUSR1)
 	<-time.After(100 * time.Millisecond)
-	system.KillProcess(r.daemonPid)
+	process.Kill(r.daemonPid)
 }
 
 func (r *remote) platformCleanup() {

libcontainerd/supervisor/remote_daemon_windows.go

@@ -3,7 +3,7 @@ package supervisor // import "github.com/docker/docker/libcontainerd/supervisor"
 import (
 	"os"
 
-	"github.com/docker/docker/pkg/system"
+	"github.com/docker/docker/pkg/process"
 )
 
 const (
@@ -40,7 +40,7 @@ func (r *remote) stopDaemon() {
 }
 
 func (r *remote) killDaemon() {
-	system.KillProcess(r.daemonPid)
+	process.Kill(r.daemonPid)
 }
 
 func (r *remote) platformCleanup() {

pkg/pidfile/pidfile.go

@@ -7,36 +7,46 @@ import (
 	"bytes"
 	"fmt"
 	"os"
-	"path/filepath"
 	"strconv"
 
-	"github.com/docker/docker/pkg/system"
+	"github.com/docker/docker/pkg/process"
 )
 
-func checkPIDFileAlreadyExists(path string) error {
+// Read reads the "PID file" at path, and returns the PID if it contains a
+// valid PID of a running process, or 0 otherwise. It returns an error when
+// failing to read the file, or if the file doesn't exist, but malformed content
+// is ignored. Consumers should therefore check if the returned PID is a non-zero
+// value before use.
+func Read(path string) (pid int, err error) {
 	pidByte, err := os.ReadFile(path)
 	if err != nil {
-		if os.IsNotExist(err) {
-			return nil
-		}
-		return err
+		return 0, err
+	}
+	pid, err = strconv.Atoi(string(bytes.TrimSpace(pidByte)))
+	if err != nil {
+		return 0, nil
 	}
-	pid, err := strconv.Atoi(string(bytes.TrimSpace(pidByte)))
-	if err == nil && processExists(pid) {
-		return fmt.Errorf("pid file found, ensure docker is not running or delete %s", path)
+	if pid != 0 && process.Alive(pid) {
+		return pid, nil
 	}
-	return nil
+	return 0, nil
 }
 
 // Write writes a "PID file" at the specified path. It returns an error if the
 // file exists and contains a valid PID of a running process, or when failing
 // to write the file.
-func Write(path string) error {
-	if err := checkPIDFileAlreadyExists(path); err != nil {
-		return err
+func Write(path string, pid int) error {
+	if pid < 1 {
+		// We might be running as PID 1 when running docker-in-docker,
+		// but 0 or negative PIDs are not acceptable.
+		return fmt.Errorf("invalid PID (%d): only positive PIDs are allowed", pid)
 	}
-	if err := system.MkdirAll(filepath.Dir(path), 0o755); err != nil {
+	oldPID, err := Read(path)
+	if err != nil && !os.IsNotExist(err) {
 		return err
 	}
-	return os.WriteFile(path, []byte(strconv.Itoa(os.Getpid())), 0o644)
+	if oldPID != 0 {
+		return fmt.Errorf("process with PID %d is still running", oldPID)
+	}
+	return os.WriteFile(path, []byte(strconv.Itoa(pid)), 0o644)
 }

pkg/pidfile/pidfile_darwin.go

@@ -1,15 +0,0 @@
-//go:build darwin
-// +build darwin
-
-package pidfile // import "github.com/docker/docker/pkg/pidfile"
-
-import (
-	"golang.org/x/sys/unix"
-)
-
-func processExists(pid int) bool {
-	// OS X does not have a proc filesystem.
-	// Use kill -0 pid to judge if the process exists.
-	err := unix.Kill(pid, 0)
-	return err == nil
-}

pkg/pidfile/pidfile_test.go

@@ -1,19 +1,143 @@
 package pidfile // import "github.com/docker/docker/pkg/pidfile"
 
 import (
+	"errors"
+	"os"
+	"os/exec"
 	"path/filepath"
+	"runtime"
+	"strconv"
 	"testing"
 )
 
 func TestWrite(t *testing.T) {
 	path := filepath.Join(t.TempDir(), "testfile")
-	err := Write(path)
+
+	err := Write(path, 0)
+	if err == nil {
+		t.Fatal("writing PID < 1 should fail")
+	}
+
+	err = Write(path, os.Getpid())
 	if err != nil {
 		t.Fatal("Could not create test file", err)
 	}
 
-	err = Write(path)
+	err = Write(path, os.Getpid())
 	if err == nil {
-		t.Fatal("Test file creation not blocked")
+		t.Error("Test file creation not blocked")
 	}
+
+	pid, err := Read(path)
+	if err != nil {
+		t.Error(err)
+	}
+	if pid != os.Getpid() {
+		t.Errorf("expected pid %d, got %d", os.Getpid(), pid)
+	}
+}
+
+func TestRead(t *testing.T) {
+	tmpDir := t.TempDir()
+
+	t.Run("non-existing pidFile", func(t *testing.T) {
+		_, err := Read(filepath.Join(tmpDir, "nosuchfile"))
+		if !errors.Is(err, os.ErrNotExist) {
+			t.Errorf("expected an os.ErrNotExist, got: %+v", err)
+		}
+	})
+
+	// Verify that we ignore a malformed PID in the file.
+	t.Run("malformed pid", func(t *testing.T) {
+		// Not using Write here, to test Read in isolation.
+		pidFile := filepath.Join(tmpDir, "pidfile-malformed")
+		err := os.WriteFile(pidFile, []byte("something that's not an integer"), 0o644)
+		if err != nil {
+			t.Fatal(err)
+		}
+		pid, err := Read(pidFile)
+		if err != nil {
+			t.Error(err)
+		}
+		if pid != 0 {
+			t.Errorf("expected pid %d, got %d", 0, pid)
+		}
+	})
+
+	t.Run("zero pid", func(t *testing.T) {
+		// Not using Write here, to test Read in isolation.
+		pidFile := filepath.Join(tmpDir, "pidfile-zero")
+		err := os.WriteFile(pidFile, []byte(strconv.Itoa(0)), 0o644)
+		if err != nil {
+			t.Fatal(err)
+		}
+		pid, err := Read(pidFile)
+		if err != nil {
+			t.Error(err)
+		}
+		if pid != 0 {
+			t.Errorf("expected pid %d, got %d", 0, pid)
+		}
+	})
+
+	t.Run("negative pid", func(t *testing.T) {
+		// Not using Write here, to test Read in isolation.
+		pidFile := filepath.Join(tmpDir, "pidfile-negative")
+		err := os.WriteFile(pidFile, []byte(strconv.Itoa(-1)), 0o644)
+		if err != nil {
+			t.Fatal(err)
+		}
+		pid, err := Read(pidFile)
+		if err != nil {
+			t.Error(err)
+		}
+		if pid != 0 {
+			t.Errorf("expected pid %d, got %d", 0, pid)
+		}
+	})
+
+	t.Run("current process pid", func(t *testing.T) {
+		// Not using Write here, to test Read in isolation.
+		pidFile := filepath.Join(tmpDir, "pidfile")
+		err := os.WriteFile(pidFile, []byte(strconv.Itoa(os.Getpid())), 0o644)
+		if err != nil {
+			t.Fatal(err)
+		}
+		pid, err := Read(pidFile)
+		if err != nil {
+			t.Error(err)
+		}
+		if pid != os.Getpid() {
+			t.Errorf("expected pid %d, got %d", os.Getpid(), pid)
+		}
+	})
+
+	// Verify that we don't return a PID if the process exited.
+	t.Run("exited process", func(t *testing.T) {
+		if runtime.GOOS == "windows" {
+			t.Skip("TODO: make this work on Windows")
+		}
+
+		// Get a PID of an exited process.
+		cmd := exec.Command("echo", "hello world")
+		err := cmd.Run()
+		if err != nil {
+			t.Fatal(err)
+		}
+		exitedPID := cmd.ProcessState.Pid()
+
+		// Not using Write here, to test Read in isolation.
+		pidFile := filepath.Join(tmpDir, "pidfile-exited")
+		err = os.WriteFile(pidFile, []byte(strconv.Itoa(exitedPID)), 0o644)
+		if err != nil {
+			t.Fatal(err)
+		}
+		pid, err := Read(pidFile)
+		if err != nil {
+			t.Error(err)
+		}
+		if pid != 0 {
+			t.Errorf("expected pid %d, got %d", 0, pid)
+		}
+	})
 }

pkg/pidfile/pidfile_unix.go

@@ -1,17 +0,0 @@
-//go:build !windows && !darwin
-// +build !windows,!darwin
-
-package pidfile // import "github.com/docker/docker/pkg/pidfile"
-
-import (
-	"os"
-	"path/filepath"
-	"strconv"
-)
-
-func processExists(pid int) bool {
-	if _, err := os.Stat(filepath.Join("/proc", strconv.Itoa(pid))); err == nil {
-		return true
-	}
-	return false
-}

pkg/process/doc.go

@@ -0,0 +1,3 @@
+// Package process provides a set of basic functions to manage individual
+// processes.
+package process

pkg/process/process_test.go

@@ -0,0 +1,40 @@
+package process
+
+import (
+	"fmt"
+	"os"
+	"os/exec"
+	"runtime"
+	"testing"
+)
+
+func TestAlive(t *testing.T) {
+	for _, pid := range []int{0, -1, -123} {
+		t.Run(fmt.Sprintf("invalid process (%d)", pid), func(t *testing.T) {
+			if Alive(pid) {
+				t.Errorf("PID %d should not be alive", pid)
+			}
+		})
+	}
+	t.Run("current process", func(t *testing.T) {
+		if pid := os.Getpid(); !Alive(pid) {
+			t.Errorf("current PID (%d) should be alive", pid)
+		}
+	})
+	t.Run("exited process", func(t *testing.T) {
+		if runtime.GOOS == "windows" {
+			t.Skip("TODO: make this work on Windows")
+		}
+
+		// Get a PID of an exited process.
+		cmd := exec.Command("echo", "hello world")
+		err := cmd.Run()
+		if err != nil {
+			t.Fatal(err)
+		}
+		exitedPID := cmd.ProcessState.Pid()
+		if Alive(exitedPID) {
+			t.Errorf("PID %d should not be alive", exitedPID)
+		}
+	})
+}

pkg/process/process_unix.go

@@ -0,0 +1,82 @@
+//go:build !windows
+// +build !windows
+
+package process
+
+import (
+	"bytes"
+	"fmt"
+	"os"
+	"path/filepath"
+	"runtime"
+	"strconv"
+
+	"golang.org/x/sys/unix"
+)
+
+// Alive returns true if process with a given pid is running. It only considers
+// positive PIDs; 0 (all processes in the current process group), -1 (all processes
+// with a PID larger than 1), and negative (-n, all processes in process group
+// "n") values for pid are never considered to be alive.
+func Alive(pid int) bool {
+	if pid < 1 {
+		return false
+	}
+	switch runtime.GOOS {
+	case "darwin":
+		// OS X does not have a proc filesystem. Use kill -0 pid to judge if the
+		// process exists. From KILL(2): https://www.freebsd.org/cgi/man.cgi?query=kill&sektion=2&manpath=OpenDarwin+7.2.1
+		//
+		// Sig may be one of the signals specified in sigaction(2) or it may
+		// be 0, in which case error checking is performed but no signal is
+		// actually sent. This can be used to check the validity of pid.
+		err := unix.Kill(pid, 0)
+
+		// Either the PID was found (no error) or we get an EPERM, which means
+		// the PID exists, but we don't have permissions to signal it.
+		return err == nil || err == unix.EPERM
+	default:
+		_, err := os.Stat(filepath.Join("/proc", strconv.Itoa(pid)))
+		return err == nil
+	}
+}
+
+// Kill force-stops a process. It only considers positive PIDs; 0 (all processes
+// in the current process group), -1 (all processes with a PID larger than 1),
+// and negative (-n, all processes in process group "n") values for pid are
+// ignored. Refer to [KILL(2)] for details.
+//
+// [KILL(2)]: https://man7.org/linux/man-pages/man2/kill.2.html
+func Kill(pid int) error {
+	if pid < 1 {
+		return fmt.Errorf("invalid PID (%d): only positive PIDs are allowed", pid)
+	}
+	err := unix.Kill(pid, unix.SIGKILL)
+	if err != nil && err != unix.ESRCH {
+		return err
+	}
+	return nil
+}
+
+// Zombie return true if process has a state with "Z". It only considers positive
+// PIDs; 0 (all processes in the current process group), -1 (all processes with
+// a PID larger than 1), and negative (-n, all processes in process group "n")
+// values for pid are ignored. Refer to [PROC(5)] for details.
+//
+// [PROC(5)]: https://man7.org/linux/man-pages/man5/proc.5.html
+func Zombie(pid int) (bool, error) {
+	if pid < 1 {
+		return false, nil
+	}
+	data, err := os.ReadFile(fmt.Sprintf("/proc/%d/stat", pid))
+	if err != nil {
+		if os.IsNotExist(err) {
+			return false, nil
+		}
+		return false, err
+	}
+	if cols := bytes.SplitN(data, []byte(" "), 4); len(cols) >= 3 && string(cols[2]) == "Z" {
+		return true, nil
+	}
+	return false, nil
+}

pkg/pidfile/pidfile_windows.go → pkg/process/process_windows.go

@@ -1,10 +1,13 @@
-package pidfile // import "github.com/docker/docker/pkg/pidfile"
+package process
 
 import (
+	"os"
+
 	"golang.org/x/sys/windows"
 )
 
-func processExists(pid int) bool {
+// Alive returns true if process with a given pid is running.
+func Alive(pid int) bool {
 	h, err := windows.OpenProcess(windows.PROCESS_QUERY_LIMITED_INFORMATION, false, uint32(pid))
 	if err != nil {
 		return false
@@ -28,3 +31,22 @@ func processExists(pid int) bool {
 	}
 	return true
 }
+
+// Kill force-stops a process.
+func Kill(pid int) error {
+	p, err := os.FindProcess(pid)
+	if err == nil {
+		err = p.Kill()
+		if err != nil && err != os.ErrProcessDone {
+			return err
+		}
+	}
+	return nil
+}
+
+// Zombie is not supported on Windows.
+//
+// TODO(thaJeztah): remove once we remove the stubs from pkg/system.
+func Zombie(_ int) (bool, error) {
+	return false, nil
+}

pkg/system/process_deprecated.go

@@ -0,0 +1,27 @@
+//go:build linux || freebsd || darwin || windows
+// +build linux freebsd darwin windows
+
+package system
+
+import "github.com/docker/docker/pkg/process"
+
+var (
+	// IsProcessAlive returns true if process with a given pid is running.
+	//
+	// Deprecated: use [process.Alive].
+	IsProcessAlive = process.Alive
+
+	// IsProcessZombie return true if process has a state with "Z"
+	//
+	// Deprecated: use [process.Zombie].
+	//
+	// TODO(thaJeztah): remove the Windows implementation in process once we remove this stub.
+	IsProcessZombie = process.Zombie
+)
+
+// KillProcess force-stops a process.
+//
+// Deprecated: use [process.Kill].
+func KillProcess(pid int) {
+	_ = process.Kill(pid)
+}

pkg/system/process_unix.go

@@ -1,46 +0,0 @@
-//go:build linux || freebsd || darwin
-// +build linux freebsd darwin
-
-package system // import "github.com/docker/docker/pkg/system"
-
-import (
-	"fmt"
-	"os"
-	"strings"
-	"syscall"
-
-	"golang.org/x/sys/unix"
-)
-
-// IsProcessAlive returns true if process with a given pid is running.
-func IsProcessAlive(pid int) bool {
-	err := unix.Kill(pid, syscall.Signal(0))
-	if err == nil || err == unix.EPERM {
-		return true
-	}
-
-	return false
-}
-
-// KillProcess force-stops a process.
-func KillProcess(pid int) {
-	unix.Kill(pid, unix.SIGKILL)
-}
-
-// IsProcessZombie return true if process has a state with "Z"
-// http://man7.org/linux/man-pages/man5/proc.5.html
-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)
-	sdata := strings.SplitN(data, " ", 4)
-	if len(sdata) >= 3 && sdata[2] == "Z" {
-		return true, nil
-	}
-
-	return false, nil
-}

pkg/system/process_windows.go

@@ -1,18 +0,0 @@
-package system // import "github.com/docker/docker/pkg/system"
-
-import "os"
-
-// IsProcessAlive returns true if process with a given pid is running.
-func IsProcessAlive(pid int) bool {
-	_, err := os.FindProcess(pid)
-
-	return err == nil
-}
-
-// KillProcess force-stops a process.
-func KillProcess(pid int) {
-	p, err := os.FindProcess(pid)
-	if err == nil {
-		_ = p.Kill()
-	}
-}