From 9407a5752250c83f5565fa43ab294e2d7c47be41 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 9 Oct 2019 12:55:25 +0200 Subject: [PATCH 1/8] hack/make: don't attempt to unmount non-existing daemon root-dir Before: DONE 2 tests in 12.272s ---> Making bundle: .integration-daemon-stop (in bundles/test-integration) umount: bundles/test-integration/root: mountpoint not found After: DONE 2 tests in 14.650s ---> Making bundle: .integration-daemon-stop (in bundles/test-integration) Signed-off-by: Sebastiaan van Stijn --- hack/make/.integration-daemon-stop | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/hack/make/.integration-daemon-stop b/hack/make/.integration-daemon-stop index 63f7f361b4..4f7c8acb8e 100644 --- a/hack/make/.integration-daemon-stop +++ b/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 From 1fe7a9552c5997d64a2180897923b52285eedfb7 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 9 Oct 2019 13:00:21 +0200 Subject: [PATCH 2/8] testutil/daemon: daemon.Cleanup(): unmount daemon root-dir as part of cleanup test-daemons remove their docker.pid when stopped, so the `.integration-daemon-stop` script did not find the mounts for those daemons, and therefore was not unmounting them. As a result, cleaning up the bundles directory on consecutive runs of the tests would fail; rm: cannot remove 'bundles/test-integration/TestDockerSwarmSuite/TestSwarmInit/d1f188f3f5472/root': Device or resource busy This patch unmounts the root directory of the daemon as part of the cleanup step. Signed-off-by: Sebastiaan van Stijn --- testutil/daemon/daemon.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/testutil/daemon/daemon.go b/testutil/daemon/daemon.go index de948ecdc2..50a46958d4 100644 --- a/testutil/daemon/daemon.go +++ b/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" @@ -213,6 +214,7 @@ 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() + cleanupMount(t, d) // Cleanup swarmkit wal files if present cleanupRaftDir(t, d.Root) cleanupNetworkNamespace(t, d.execRoot) @@ -710,6 +712,15 @@ func (d *Daemon) Info(t testing.TB) types.Info { return info } +// 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) + } +} + func cleanupRaftDir(t testing.TB, rootPath string) { t.Helper() for _, p := range []string{"wal", "wal-v3-encrypted", "snap-v3-encrypted"} { From 2b3957d0b168a697c7f1bb6fcfa4b2c66c9b8ae0 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 9 Oct 2019 13:17:52 +0200 Subject: [PATCH 3/8] testutil/daemon: prefix all logs with daemon-id This makes it easier to debug issues with tests that start multiple daemons. Signed-off-by: Sebastiaan van Stijn --- testutil/daemon/daemon.go | 54 +++++++++++++++---------------- testutil/daemon/daemon_unix.go | 6 ++-- testutil/daemon/daemon_windows.go | 3 +- 3 files changed, 31 insertions(+), 32 deletions(-) diff --git a/testutil/daemon/daemon.go b/testutil/daemon/daemon.go index 50a46958d4..781864f017 100644 --- a/testutil/daemon/daemon.go +++ b/testutil/daemon/daemon.go @@ -196,7 +196,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 } @@ -215,16 +215,15 @@ func (d *Daemon) NewClient(extraOpts ...client.Opt) (*client.Client, error) { func (d *Daemon) Cleanup(t testing.TB) { t.Helper() cleanupMount(t, d) - // Cleanup swarmkit wal files if present - cleanupRaftDir(t, d.Root) - cleanupNetworkNamespace(t, d.execRoot) + 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, args, err) } } @@ -233,7 +232,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...) @@ -437,9 +436,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) } } } @@ -453,10 +452,10 @@ 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) } d.logFile.Close() d.cmd = nil @@ -467,13 +466,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.Errorf("[%s] could not send signal: %v", d.id, err) } out1: @@ -483,7 +482,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 } } @@ -496,18 +495,18 @@ 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.Errorf("[%s] attempt #%d/5 could not send signal: %v", d.id, i, err) } } } 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 } @@ -578,15 +577,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.Errorf("[%s] error signaling daemon reload: %v", d.id, err) } 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 } @@ -595,19 +594,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() } @@ -721,12 +720,13 @@ func cleanupMount(t testing.TB, d *Daemon) { } } -func cleanupRaftDir(t testing.TB, rootPath string) { +// 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) } } } diff --git a/testutil/daemon/daemon_unix.go b/testutil/daemon/daemon_unix.go index 17191c687f..6ef860b962 100644 --- a/testutil/daemon/daemon_unix.go +++ b/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 diff --git a/testutil/daemon/daemon_windows.go b/testutil/daemon/daemon_windows.go index 0492d387c6..7e7709e6c5 100644 --- a/testutil/daemon/daemon_windows.go +++ b/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 { From 22662cac570486ecb514de263854aa883c627d98 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 9 Oct 2019 13:20:47 +0200 Subject: [PATCH 4/8] testutil/daemon: wrap errors Signed-off-by: Sebastiaan van Stijn --- integration-cli/docker_cli_daemon_test.go | 2 +- testutil/daemon/daemon.go | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/integration-cli/docker_cli_daemon_test.go b/integration-cli/docker_cli_daemon_test.go index 6a874eb981..fa2b89bdda 100644 --- a/integration-cli/docker_cli_daemon_test.go +++ b/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 diff --git a/testutil/daemon/daemon.go b/testutil/daemon/daemon.go index 781864f017..1f1e7bb44c 100644 --- a/testutil/daemon/daemon.go +++ b/testutil/daemon/daemon.go @@ -90,7 +90,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())) @@ -101,7 +101,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 @@ -298,7 +298,7 @@ func (d *Daemon) StartWithLogFile(out *os.File, providedArgs ...string) error { 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) @@ -338,9 +338,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() @@ -365,7 +365,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 } @@ -472,7 +472,7 @@ func (d *Daemon) StopWithError() (err error) { if strings.Contains(err.Error(), "os: process already finished") { return errDaemonNotStarted } - return errors.Errorf("[%s] could not send signal: %v", d.id, err) + return errors.Wrapf(err, "[%s] could not send signal", d.id) } out1: @@ -500,7 +500,7 @@ out2: } 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("[%s] attempt #%d/5 could not send signal: %v", d.id, i, err) + return errors.Wrapf(err, "[%s] attempt #%d/5 could not send signal", d.id, i) } } } @@ -577,7 +577,7 @@ func (d *Daemon) ReloadConfig() error { <-started if err := signalDaemonReload(d.cmd.Process.Pid); err != nil { - return errors.Errorf("[%s] error signaling daemon reload: %v", d.id, err) + return errors.Wrapf(err, "[%s] error signaling daemon reload", d.id) } select { case err := <-errCh: From b843b1ffe3a19b93b40c7193b895ff81ba64d836 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 9 Oct 2019 13:22:59 +0200 Subject: [PATCH 5/8] testutil/daemon: store pidfile-path, and ignore errors when removing This patch stores the location of the pidfile, so that we can use the same path that was set to create it. If no pidfile was created, we'll not try to remove it. We're now also ignoring errors when removing the pidfile, as they should not fail the test (especialy if no pidfile was created in the first place, as that could potentially hide the actual failure). This may help with "failures" such as the one below: ``` FAIL: check_test.go:347: DockerSwarmSuite.TearDownTest check_test.go:352: d.Stop(c) /go/src/github.com/docker/docker/internal/test/daemon/daemon.go:414: t.Fatalf("Error while stopping the daemon %s : %v", d.id, err) ... Error: Error while stopping the daemon d1512c423813a : remove /go/src/github.com/docker/docker/bundles/test-integration/DockerSwarmSuite.TestServiceLogs/d1512c423813a/docker.pid: no such file or directory ``` Signed-off-by: Sebastiaan van Stijn --- testutil/daemon/daemon.go | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/testutil/daemon/daemon.go b/testutil/daemon/daemon.go index 1f1e7bb44c..1dacfd694d 100644 --- a/testutil/daemon/daemon.go +++ b/testutil/daemon/daemon.go @@ -72,6 +72,7 @@ type Daemon struct { init bool dockerdBinary string log logT + pidFile string // swarm related field swarmListenAddr string @@ -246,11 +247,15 @@ func (d *Daemon) StartWithLogFile(out *os.File, providedArgs ...string) error { return errors.Wrapf(err, "[%s] could not find docker binary in $PATH", d.id) } + if d.pidFile == "" { + d.pidFile = filepath.Join(d.Folder, "docker.pid") + } + 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", @@ -395,7 +400,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 @@ -512,7 +520,10 @@ out2: d.cmd.Wait() - return os.Remove(fmt.Sprintf("%s/docker.pid", d.Folder)) + if d.pidFile != "" { + _ = os.Remove(d.pidFile) + } + return nil } // Restart will restart the daemon by first stopping it and the starting it. From f6842327b03e124a218f20fc6d0d007df86f7fb2 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 9 Oct 2019 13:25:21 +0200 Subject: [PATCH 6/8] testutil/daemon: print all arguments when failing to start daemon Signed-off-by: Sebastiaan van Stijn --- testutil/daemon/daemon.go | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/testutil/daemon/daemon.go b/testutil/daemon/daemon.go index 1dacfd694d..0c1ecc19b2 100644 --- a/testutil/daemon/daemon.go +++ b/testutil/daemon/daemon.go @@ -73,6 +73,7 @@ type Daemon struct { dockerdBinary string log logT pidFile string + args []string // swarm related field swarmListenAddr string @@ -224,7 +225,7 @@ func (d *Daemon) Cleanup(t testing.TB) { func (d *Daemon) Start(t testing.TB, args ...string) { t.Helper() if err := d.StartWithError(args...); err != nil { - t.Fatalf("[%s] failed to start daemon with arguments %v : %v", d.id, args, err) + t.Fatalf("[%s] failed to start daemon with arguments %v : %v", d.id, d.args, err) } } @@ -251,7 +252,7 @@ func (d *Daemon) StartWithLogFile(out *os.File, providedArgs ...string) error { d.pidFile = filepath.Join(d.Folder, "docker.pid") } - args := append(d.GlobalFlags, + d.args = append(d.GlobalFlags, "--containerd", containerdSocket, "--data-root", d.Root, "--exec-root", d.execRoot, @@ -261,19 +262,19 @@ func (d *Daemon) StartWithLogFile(out *os.File, providedArgs ...string) error { "--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 @@ -289,14 +290,14 @@ 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 From c56bfdf10a4874701fe53e82d209ec7e5d36d497 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 9 Oct 2019 14:45:37 +0200 Subject: [PATCH 7/8] testutil/daemon: always remove pidfile after daemon is stopped If the daemon was stopped successfully in one of the retry-loops, the function would return early; ```go for { select { case err := <-d.Wait: ---> the function returns here, both on "success" and on "fail" return err case <-time.After(20 * time.Second): ... ``` In that case, the pidfile would not be cleaned up. This patch changes the function to clean-up the pidfile in a defer, so that it will always be removed after succesfully stopping the daemon. Signed-off-by: Sebastiaan van Stijn --- testutil/daemon/daemon.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/testutil/daemon/daemon.go b/testutil/daemon/daemon.go index 0c1ecc19b2..943e6b7cb9 100644 --- a/testutil/daemon/daemon.go +++ b/testutil/daemon/daemon.go @@ -465,8 +465,13 @@ func (d *Daemon) StopWithError() (err error) { d.log.Logf("[%s] error while stopping daemon: %v", d.id, err) } else { 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 }() @@ -519,12 +524,7 @@ out2: return err } - d.cmd.Wait() - - if d.pidFile != "" { - _ = os.Remove(d.pidFile) - } - return nil + return d.cmd.Wait() } // Restart will restart the daemon by first stopping it and the starting it. From 293c1a27a21fd2c0ee0dbff4f348a36aa7a10e28 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 9 Oct 2019 22:20:08 +0200 Subject: [PATCH 8/8] testutil/daemon: remove redundant d.cmd.Wait() `daemon.StartWithLogFile()` already creates a goroutine that calls `d.cmd.Waits()` and sends its return to the channel, `d.Wait`. This code called `d.cmd.Wait()` one more time, and returns the error, which may produce an error _because_ it's called a second time, and potentially cause an incorrect test-result. (thanks to Kir Kolyshkin for spotting this) Signed-off-by: Sebastiaan van Stijn --- testutil/daemon/daemon.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testutil/daemon/daemon.go b/testutil/daemon/daemon.go index 943e6b7cb9..d6a7a07276 100644 --- a/testutil/daemon/daemon.go +++ b/testutil/daemon/daemon.go @@ -524,7 +524,7 @@ out2: return err } - return d.cmd.Wait() + return nil } // Restart will restart the daemon by first stopping it and the starting it.