Merge pull request #40062 from thaJeztah/testutil_improvements

Various testutil improvements

hack/make/.integration-daemon-stop

@@ -11,7 +11,9 @@ if [ ! "$(go env GOOS)" = 'windows' ]; then
 			echo >&2 "warning: PID $pid from $pidFile had a nonzero exit code"
 		fi
 		root=$(dirname "$pidFile")/root
-		umount "$root" || true
+		if [ -d "$root" ]; then
+			umount "$root" || true
+		fi
 	done
 
 	if [ -z "$DOCKER_TEST_HOST" ]; then

integration-cli/docker_cli_daemon_test.go

@@ -575,7 +575,7 @@ func (s *DockerDaemonSuite) TestDaemonExitOnFailure(c *testing.T) {
 	//attempt to start daemon with incorrect flags (we know -b and --bip conflict)
 	if err := s.d.StartWithError("--bridge", "nosuchbridge", "--bip", "1.1.1.1"); err != nil {
 		//verify we got the right error
-		if !strings.Contains(err.Error(), "Daemon exited") {
+		if !strings.Contains(err.Error(), "daemon exited") {
 			c.Fatalf("Expected daemon not to start, got %v", err)
 		}
 		// look in the log and make sure we got the message that daemon is shutting down

testutil/daemon/daemon.go

@@ -19,6 +19,7 @@ import (
 	"github.com/docker/docker/client"
 	"github.com/docker/docker/opts"
 	"github.com/docker/docker/pkg/ioutils"
+	"github.com/docker/docker/pkg/mount"
 	"github.com/docker/docker/pkg/stringid"
 	"github.com/docker/docker/testutil/request"
 	"github.com/docker/go-connections/sockets"
@@ -71,6 +72,8 @@ type Daemon struct {
 	init                       bool
 	dockerdBinary              string
 	log                        logT
+	pidFile                    string
+	args                       []string
 
 	// swarm related field
 	swarmListenAddr string
@@ -89,7 +92,7 @@ func NewDaemon(workingDir string, ops ...Option) (*Daemon, error) {
 	storageDriver := os.Getenv("DOCKER_GRAPHDRIVER")
 
 	if err := os.MkdirAll(SockRoot, 0700); err != nil {
-		return nil, fmt.Errorf("could not create daemon socket root: %v", err)
+		return nil, errors.Wrapf(err, "failed to create daemon socket root %q", SockRoot)
 	}
 
 	id := fmt.Sprintf("d%s", stringid.TruncateID(stringid.GenerateRandomID()))
@@ -100,7 +103,7 @@ func NewDaemon(workingDir string, ops ...Option) (*Daemon, error) {
 	}
 	daemonRoot := filepath.Join(daemonFolder, "root")
 	if err := os.MkdirAll(daemonRoot, 0755); err != nil {
-		return nil, fmt.Errorf("could not create daemon root: %v", err)
+		return nil, errors.Wrapf(err, "failed to create daemon root %q", daemonRoot)
 	}
 
 	userlandProxy := true
@@ -195,7 +198,7 @@ func (d *Daemon) NewClientT(t testing.TB, extraOpts ...client.Opt) *client.Clien
 	t.Helper()
 
 	c, err := d.NewClient(extraOpts...)
-	assert.NilError(t, err, "cannot create daemon client")
+	assert.NilError(t, err, "[%s] could not create daemon client", d.id)
 	return c
 }
 
@@ -213,16 +216,16 @@ func (d *Daemon) NewClient(extraOpts ...client.Opt) (*client.Client, error) {
 // Cleanup cleans the daemon files : exec root (network namespaces, ...), swarmkit files
 func (d *Daemon) Cleanup(t testing.TB) {
 	t.Helper()
-	// Cleanup swarmkit wal files if present
-	cleanupRaftDir(t, d.Root)
-	cleanupNetworkNamespace(t, d.execRoot)
+	cleanupMount(t, d)
+	cleanupRaftDir(t, d)
+	cleanupNetworkNamespace(t, d)
 }
 
 // Start starts the daemon and return once it is ready to receive requests.
 func (d *Daemon) Start(t testing.TB, args ...string) {
 	t.Helper()
 	if err := d.StartWithError(args...); err != nil {
-		t.Fatalf("failed to start daemon with arguments %v : %v", args, err)
+		t.Fatalf("[%s] failed to start daemon with arguments %v : %v", d.id, d.args, err)
 	}
 }
 
@@ -231,7 +234,7 @@ func (d *Daemon) Start(t testing.TB, args ...string) {
 func (d *Daemon) StartWithError(args ...string) error {
 	logFile, err := os.OpenFile(filepath.Join(d.Folder, "docker.log"), os.O_RDWR|os.O_CREATE|os.O_APPEND, 0600)
 	if err != nil {
-		return errors.Wrapf(err, "[%s] Could not create %s/docker.log", d.id, d.Folder)
+		return errors.Wrapf(err, "[%s] failed to create logfile", d.id)
 	}
 
 	return d.StartWithLogFile(logFile, args...)
@@ -245,29 +248,33 @@ func (d *Daemon) StartWithLogFile(out *os.File, providedArgs ...string) error {
 		return errors.Wrapf(err, "[%s] could not find docker binary in $PATH", d.id)
 	}
 
-	args := append(d.GlobalFlags,
+	if d.pidFile == "" {
+		d.pidFile = filepath.Join(d.Folder, "docker.pid")
+	}
+
+	d.args = append(d.GlobalFlags,
 		"--containerd", containerdSocket,
 		"--data-root", d.Root,
 		"--exec-root", d.execRoot,
-		"--pidfile", fmt.Sprintf("%s/docker.pid", d.Folder),
+		"--pidfile", d.pidFile,
 		fmt.Sprintf("--userland-proxy=%t", d.userlandProxy),
 		"--containerd-namespace", d.id,
 		"--containerd-plugins-namespace", d.id+"p",
 	)
 	if d.defaultCgroupNamespaceMode != "" {
-		args = append(args, []string{"--default-cgroupns-mode", d.defaultCgroupNamespaceMode}...)
+		d.args = append(d.args, []string{"--default-cgroupns-mode", d.defaultCgroupNamespaceMode}...)
 	}
 	if d.experimental {
-		args = append(args, "--experimental")
+		d.args = append(d.args, "--experimental")
 	}
 	if d.init {
-		args = append(args, "--init")
+		d.args = append(d.args, "--init")
 	}
 	if !(d.UseDefaultHost || d.UseDefaultTLSHost) {
-		args = append(args, []string{"--host", d.Sock()}...)
+		d.args = append(d.args, []string{"--host", d.Sock()}...)
 	}
 	if root := os.Getenv("DOCKER_REMAP_ROOT"); root != "" {
-		args = append(args, []string{"--userns-remap", root}...)
+		d.args = append(d.args, []string{"--userns-remap", root}...)
 	}
 
 	// If we don't explicitly set the log-level or debug flag(-D) then
@@ -283,21 +290,21 @@ func (d *Daemon) StartWithLogFile(out *os.File, providedArgs ...string) error {
 		}
 	}
 	if !foundLog {
-		args = append(args, "--debug")
+		d.args = append(d.args, "--debug")
 	}
 	if d.storageDriver != "" && !foundSd {
-		args = append(args, "--storage-driver", d.storageDriver)
+		d.args = append(d.args, "--storage-driver", d.storageDriver)
 	}
 
-	args = append(args, providedArgs...)
-	d.cmd = exec.Command(dockerdBinary, args...)
+	d.args = append(d.args, providedArgs...)
+	d.cmd = exec.Command(dockerdBinary, d.args...)
 	d.cmd.Env = append(os.Environ(), "DOCKER_SERVICE_PREFER_OFFLINE_IMAGE=1")
 	d.cmd.Stdout = out
 	d.cmd.Stderr = out
 	d.logFile = out
 
 	if err := d.cmd.Start(); err != nil {
-		return errors.Errorf("[%s] could not start daemon container: %v", d.id, err)
+		return errors.Wrapf(err, "[%s] could not start daemon container", d.id)
 	}
 
 	wait := make(chan error, 1)
@@ -337,9 +344,9 @@ func (d *Daemon) StartWithLogFile(out *os.File, providedArgs ...string) error {
 
 		select {
 		case <-ctx.Done():
-			return errors.Errorf("[%s] Daemon exited and never started: %s", d.id, ctx.Err())
+			return errors.Wrapf(ctx.Err(), "[%s] daemon exited and never started", d.id)
 		case err := <-d.Wait:
-			return errors.Errorf("[%s] Daemon exited during startup: %v", d.id, err)
+			return errors.Wrapf(err, "[%s] daemon exited during startup", d.id)
 		default:
 			rctx, rcancel := context.WithTimeout(context.TODO(), 2*time.Second)
 			defer rcancel()
@@ -364,7 +371,7 @@ func (d *Daemon) StartWithLogFile(out *os.File, providedArgs ...string) error {
 			d.log.Logf("[%s] daemon started\n", d.id)
 			d.Root, err = d.queryRootDir()
 			if err != nil {
-				return errors.Errorf("[%s] error querying daemon for root directory: %v", d.id, err)
+				return errors.Wrapf(err, "[%s] error querying daemon for root directory", d.id)
 			}
 			return nil
 		}
@@ -394,7 +401,10 @@ func (d *Daemon) Kill() error {
 		return err
 	}
 
-	return os.Remove(fmt.Sprintf("%s/docker.pid", d.Folder))
+	if d.pidFile != "" {
+		_ = os.Remove(d.pidFile)
+	}
+	return nil
 }
 
 // Pid returns the pid of the daemon
@@ -435,9 +445,9 @@ func (d *Daemon) Stop(t testing.TB) {
 	err := d.StopWithError()
 	if err != nil {
 		if err != errDaemonNotStarted {
-			t.Fatalf("Error while stopping the daemon %s : %v", d.id, err)
+			t.Fatalf("[%s] error while stopping the daemon: %v", d.id, err)
 		} else {
-			t.Logf("Daemon %s is not started", d.id)
+			t.Logf("[%s] daemon is not started", d.id)
 		}
 	}
 }
@@ -451,12 +461,17 @@ func (d *Daemon) StopWithError() (err error) {
 		return errDaemonNotStarted
 	}
 	defer func() {
-		if err == nil {
-			d.log.Logf("[%s] Daemon stopped", d.id)
+		if err != nil {
+			d.log.Logf("[%s] error while stopping daemon: %v", d.id, err)
 		} else {
-			d.log.Logf("[%s] Error when stopping daemon: %v", d.id, err)
+			d.log.Logf("[%s] daemon stopped", d.id)
+			if d.pidFile != "" {
+				_ = os.Remove(d.pidFile)
+			}
+		}
+		if err := d.logFile.Close(); err != nil {
+			d.log.Logf("[%s] failed to close daemon logfile: %v", d.id, err)
 		}
-		d.logFile.Close()
 		d.cmd = nil
 	}()
 
@@ -465,13 +480,13 @@ func (d *Daemon) StopWithError() (err error) {
 	defer ticker.Stop()
 	tick := ticker.C
 
-	d.log.Logf("[%s] Stopping daemon", d.id)
+	d.log.Logf("[%s] stopping daemon", d.id)
 
 	if err := d.cmd.Process.Signal(os.Interrupt); err != nil {
 		if strings.Contains(err.Error(), "os: process already finished") {
 			return errDaemonNotStarted
 		}
-		return errors.Errorf("could not send signal: %v", err)
+		return errors.Wrapf(err, "[%s] could not send signal", d.id)
 	}
 
 out1:
@@ -481,7 +496,7 @@ out1:
 			return err
 		case <-time.After(20 * time.Second):
 			// time for stopping jobs and run onShutdown hooks
-			d.log.Logf("[%s] daemon stop timeout", d.id)
+			d.log.Logf("[%s] daemon stop timed out after 20 seconds", d.id)
 			break out1
 		}
 	}
@@ -494,24 +509,22 @@ out2:
 		case <-tick:
 			i++
 			if i > 5 {
-				d.log.Logf("tried to interrupt daemon for %d times, now try to kill it", i)
+				d.log.Logf("[%s] tried to interrupt daemon for %d times, now try to kill it", d.id, i)
 				break out2
 			}
-			d.log.Logf("Attempt #%d: daemon is still running with pid %d", i, d.cmd.Process.Pid)
+			d.log.Logf("[%d] attempt #%d/5: daemon is still running with pid %d", i, d.cmd.Process.Pid)
 			if err := d.cmd.Process.Signal(os.Interrupt); err != nil {
-				return errors.Errorf("could not send signal: %v", err)
+				return errors.Wrapf(err, "[%s] attempt #%d/5 could not send signal", d.id, i)
 			}
 		}
 	}
 
 	if err := d.cmd.Process.Kill(); err != nil {
-		d.log.Logf("Could not kill daemon: %v", err)
+		d.log.Logf("[%s] failed to kill daemon: %v", d.id, err)
 		return err
 	}
 
-	d.cmd.Wait()
-
-	return os.Remove(fmt.Sprintf("%s/docker.pid", d.Folder))
+	return nil
 }
 
 // Restart will restart the daemon by first stopping it and the starting it.
@@ -576,15 +589,15 @@ func (d *Daemon) ReloadConfig() error {
 
 	<-started
 	if err := signalDaemonReload(d.cmd.Process.Pid); err != nil {
-		return errors.Errorf("error signaling daemon reload: %v", err)
+		return errors.Wrapf(err, "[%s] error signaling daemon reload", d.id)
 	}
 	select {
 	case err := <-errCh:
 		if err != nil {
-			return errors.Errorf("error waiting for daemon reload event: %v", err)
+			return errors.Wrapf(err, "[%s] error waiting for daemon reload event", d.id)
 		}
 	case <-time.After(30 * time.Second):
-		return errors.New("timeout waiting for daemon reload event")
+		return errors.Errorf("[%s] daemon reload event timed out after 30 seconds", d.id)
 	}
 	return nil
 }
@@ -593,19 +606,19 @@ func (d *Daemon) ReloadConfig() error {
 func (d *Daemon) LoadBusybox(t testing.TB) {
 	t.Helper()
 	clientHost, err := client.NewClientWithOpts(client.FromEnv)
-	assert.NilError(t, err, "failed to create client")
+	assert.NilError(t, err, "[%s] failed to create client", d.id)
 	defer clientHost.Close()
 
 	ctx := context.Background()
 	reader, err := clientHost.ImageSave(ctx, []string{"busybox:latest"})
-	assert.NilError(t, err, "failed to download busybox")
+	assert.NilError(t, err, "[%s] failed to download busybox", d.id)
 	defer reader.Close()
 
 	c := d.NewClientT(t)
 	defer c.Close()
 
 	resp, err := c.ImageLoad(ctx, reader, true)
-	assert.NilError(t, err, "failed to load busybox")
+	assert.NilError(t, err, "[%s] failed to load busybox", d.id)
 	defer resp.Body.Close()
 }
 
@@ -710,12 +723,22 @@ func (d *Daemon) Info(t testing.TB) types.Info {
 	return info
 }
 
-func cleanupRaftDir(t testing.TB, rootPath string) {
+// cleanupMount unmounts the daemon root directory, or logs a message if
+// unmounting failed.
+func cleanupMount(t testing.TB, d *Daemon) {
+	t.Helper()
+	if err := mount.Unmount(d.Root); err != nil {
+		d.log.Logf("[%s] unable to unmount daemon root (%s): %v", d.id, d.Root, err)
+	}
+}
+
+// cleanupRaftDir removes swarmkit wal files if present
+func cleanupRaftDir(t testing.TB, d *Daemon) {
 	t.Helper()
 	for _, p := range []string{"wal", "wal-v3-encrypted", "snap-v3-encrypted"} {
-		dir := filepath.Join(rootPath, "swarm/raft", p)
+		dir := filepath.Join(d.Root, "swarm/raft", p)
 		if err := os.RemoveAll(dir); err != nil {
-			t.Logf("error removing %v: %v", dir, err)
+			t.Logf("[%s] error removing %v: %v", d.id, dir, err)
 		}
 	}
 }

testutil/daemon/daemon_unix.go

@@ -13,17 +13,17 @@ import (
 	"gotest.tools/assert"
 )
 
-func cleanupNetworkNamespace(t testing.TB, execRoot string) {
+func cleanupNetworkNamespace(t testing.TB, d *Daemon) {
 	t.Helper()
 	// Cleanup network namespaces in the exec root of this
 	// daemon because this exec root is specific to this
 	// daemon instance and has no chance of getting
 	// cleaned up when a new daemon is instantiated with a
 	// new exec root.
-	netnsPath := filepath.Join(execRoot, "netns")
+	netnsPath := filepath.Join(d.execRoot, "netns")
 	filepath.Walk(netnsPath, func(path string, info os.FileInfo, err error) error {
 		if err := unix.Unmount(path, unix.MNT_DETACH); err != nil && err != unix.EINVAL && err != unix.ENOENT {
-			t.Logf("unmount of %s failed: %v", path, err)
+			t.Logf("[%s] unmount of %s failed: %v", d.id, path, err)
 		}
 		os.Remove(path)
 		return nil

testutil/daemon/daemon_windows.go

@@ -23,8 +23,7 @@ func signalDaemonReload(pid int) error {
 	return fmt.Errorf("daemon reload not supported")
 }
 
-func cleanupNetworkNamespace(t testing.TB, execRoot string) {
-}
+func cleanupNetworkNamespace(_ testing.TB, _ *Daemon) {}
 
 // CgroupNamespace returns the cgroup namespace the daemon is running in
 func (d *Daemon) CgroupNamespace(t testing.TB) string {