From ea5c94cdb94f74808958732d8b3254921f52be4f Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 15 Jul 2021 17:33:55 +0200 Subject: [PATCH 1/3] pkg/signal: move signal.DumpStacks() to a separate package It is not directly related to signal-handling, so can well live in its own package. Also added a variant that doesn't take a directory to write files to, for easier consumption / better match to how it's used. Signed-off-by: Sebastiaan van Stijn --- daemon/cluster/cluster.go | 4 +-- daemon/cluster/swarm.go | 4 +-- daemon/debugtrap_unix.go | 4 +-- daemon/debugtrap_windows.go | 4 +-- libnetwork/diagnostic/server.go | 4 +-- pkg/signal/signal_deprecated.go | 8 +++++ pkg/signal/trap.go | 43 ++----------------------- pkg/signal/trap_linux_test.go | 17 ---------- pkg/stack/stackdump.go | 57 +++++++++++++++++++++++++++++++++ pkg/stack/stackdump_test.go | 31 ++++++++++++++++++ 10 files changed, 108 insertions(+), 68 deletions(-) create mode 100644 pkg/signal/signal_deprecated.go create mode 100644 pkg/stack/stackdump.go create mode 100644 pkg/stack/stackdump_test.go diff --git a/daemon/cluster/cluster.go b/daemon/cluster/cluster.go index b47fad5936..38a3822362 100644 --- a/daemon/cluster/cluster.go +++ b/daemon/cluster/cluster.go @@ -54,7 +54,7 @@ import ( "github.com/docker/docker/daemon/cluster/controllers/plugin" executorpkg "github.com/docker/docker/daemon/cluster/executor" lncluster "github.com/docker/docker/libnetwork/cluster" - "github.com/docker/docker/pkg/signal" + "github.com/docker/docker/pkg/stack" swarmapi "github.com/docker/swarmkit/api" swarmnode "github.com/docker/swarmkit/node" "github.com/pkg/errors" @@ -393,7 +393,7 @@ func (c *Cluster) Cleanup() { if err := node.Stop(); err != nil { logrus.Errorf("failed to shut down cluster node: %v", err) - signal.DumpStacks("") + stack.Dump() } c.mu.Lock() diff --git a/daemon/cluster/swarm.go b/daemon/cluster/swarm.go index 30418c7f68..e7bb7cc21d 100644 --- a/daemon/cluster/swarm.go +++ b/daemon/cluster/swarm.go @@ -13,7 +13,7 @@ import ( "github.com/docker/docker/daemon/cluster/convert" "github.com/docker/docker/errdefs" "github.com/docker/docker/opts" - "github.com/docker/docker/pkg/signal" + "github.com/docker/docker/pkg/stack" swarmapi "github.com/docker/swarmkit/api" "github.com/docker/swarmkit/manager/encryption" swarmnode "github.com/docker/swarmkit/node" @@ -399,7 +399,7 @@ func (c *Cluster) Leave(force bool) error { // release readers in here if err := nr.Stop(); err != nil { logrus.Errorf("failed to shut down cluster node: %v", err) - signal.DumpStacks("") + stack.Dump() return err } diff --git a/daemon/debugtrap_unix.go b/daemon/debugtrap_unix.go index bb0adfcc08..a29e8c2d1b 100644 --- a/daemon/debugtrap_unix.go +++ b/daemon/debugtrap_unix.go @@ -6,7 +6,7 @@ import ( "os" "os/signal" - stackdump "github.com/docker/docker/pkg/signal" + "github.com/docker/docker/pkg/stack" "github.com/sirupsen/logrus" "golang.org/x/sys/unix" ) @@ -16,7 +16,7 @@ func (daemon *Daemon) setupDumpStackTrap(root string) { signal.Notify(c, unix.SIGUSR1) go func() { for range c { - path, err := stackdump.DumpStacks(root) + path, err := stack.DumpToFile(root) if err != nil { logrus.WithError(err).Error("failed to write goroutines dump") } else { diff --git a/daemon/debugtrap_windows.go b/daemon/debugtrap_windows.go index 19554dd7ae..56e505e49b 100644 --- a/daemon/debugtrap_windows.go +++ b/daemon/debugtrap_windows.go @@ -5,7 +5,7 @@ import ( "os" "unsafe" - "github.com/docker/docker/pkg/signal" + "github.com/docker/docker/pkg/stack" "github.com/sirupsen/logrus" "golang.org/x/sys/windows" ) @@ -34,7 +34,7 @@ func (daemon *Daemon) setupDumpStackTrap(root string) { logrus.Debugf("Stackdump - waiting signal at %s", event) for { windows.WaitForSingleObject(h, windows.INFINITE) - path, err := signal.DumpStacks(root) + path, err := stack.DumpToFile(root) if err != nil { logrus.WithError(err).Error("failed to write goroutines dump") } else { diff --git a/libnetwork/diagnostic/server.go b/libnetwork/diagnostic/server.go index bab792b575..08c3e875af 100644 --- a/libnetwork/diagnostic/server.go +++ b/libnetwork/diagnostic/server.go @@ -9,7 +9,7 @@ import ( "sync/atomic" "github.com/docker/docker/libnetwork/internal/caller" - stackdump "github.com/docker/docker/pkg/signal" + "github.com/docker/docker/pkg/stack" "github.com/sirupsen/logrus" ) @@ -169,7 +169,7 @@ func stackTrace(ctx interface{}, w http.ResponseWriter, r *http.Request) { log := logrus.WithFields(logrus.Fields{"component": "diagnostic", "remoteIP": r.RemoteAddr, "method": caller.Name(0), "url": r.URL.String()}) log.Info("stack trace") - path, err := stackdump.DumpStacks("/tmp/") + path, err := stack.DumpToFile("/tmp/") if err != nil { log.WithError(err).Error("failed to write goroutines dump") HTTPReply(w, FailCommand(err), json) // nolint:errcheck diff --git a/pkg/signal/signal_deprecated.go b/pkg/signal/signal_deprecated.go new file mode 100644 index 0000000000..ded247ce35 --- /dev/null +++ b/pkg/signal/signal_deprecated.go @@ -0,0 +1,8 @@ +package signal + +import "github.com/docker/docker/pkg/stack" + +// DumpStacks appends the runtime stack into file in dir and returns full path +// to that file. +// Deprecated: use github.com/docker/docker/pkg/stack.Dump instead. +var DumpStacks = stack.DumpToFile diff --git a/pkg/signal/trap.go b/pkg/signal/trap.go index a277b95629..a838b3e7b9 100644 --- a/pkg/signal/trap.go +++ b/pkg/signal/trap.go @@ -4,14 +4,10 @@ import ( "fmt" "os" gosignal "os/signal" - "path/filepath" - "runtime" - "strings" "sync/atomic" "syscall" - "time" - "github.com/pkg/errors" + "github.com/docker/docker/pkg/stack" ) // Trap sets up a simplified signal "trap", appropriate for common @@ -58,7 +54,7 @@ func Trap(cleanup func(), logger interface { logger.Info("Forcing docker daemon shutdown without cleanup; 3 interrupts received") } case syscall.SIGQUIT: - DumpStacks("") + stack.Dump() logger.Info("Forcing docker daemon shutdown without cleanup on SIGQUIT") } // for the SIGINT/TERM, and SIGQUIT non-clean shutdown case, exit with 128 + signal # @@ -67,38 +63,3 @@ func Trap(cleanup func(), logger interface { } }() } - -const stacksLogNameTemplate = "goroutine-stacks-%s.log" - -// DumpStacks appends the runtime stack into file in dir and returns full path -// to that file. -func DumpStacks(dir string) (string, error) { - var ( - buf []byte - stackSize int - ) - bufferLen := 16384 - for stackSize == len(buf) { - buf = make([]byte, bufferLen) - stackSize = runtime.Stack(buf, true) - bufferLen *= 2 - } - buf = buf[:stackSize] - var f *os.File - if dir != "" { - path := filepath.Join(dir, fmt.Sprintf(stacksLogNameTemplate, strings.Replace(time.Now().Format(time.RFC3339), ":", "", -1))) - var err error - f, err = os.OpenFile(path, os.O_CREATE|os.O_WRONLY, 0666) - if err != nil { - return "", errors.Wrap(err, "failed to open file to write the goroutine stacks") - } - defer f.Close() - defer f.Sync() - } else { - f = os.Stderr - } - if _, err := f.Write(buf); err != nil { - return "", errors.Wrap(err, "failed to write goroutine stacks") - } - return f.Name(), nil -} diff --git a/pkg/signal/trap_linux_test.go b/pkg/signal/trap_linux_test.go index 781ca5a354..3dce0cd238 100644 --- a/pkg/signal/trap_linux_test.go +++ b/pkg/signal/trap_linux_test.go @@ -64,20 +64,3 @@ func TestTrap(t *testing.T) { } } - -func TestDumpStacks(t *testing.T) { - directory, err := ioutil.TempDir("", "test-dump-tasks") - assert.Check(t, err) - defer os.RemoveAll(directory) - dumpPath, err := DumpStacks(directory) - assert.Check(t, err) - readFile, _ := ioutil.ReadFile(dumpPath) - fileData := string(readFile) - assert.Check(t, is.Contains(fileData, "goroutine")) -} - -func TestDumpStacksWithEmptyInput(t *testing.T) { - path, err := DumpStacks("") - assert.Check(t, err) - assert.Check(t, is.Equal(os.Stderr.Name(), path)) -} diff --git a/pkg/stack/stackdump.go b/pkg/stack/stackdump.go new file mode 100644 index 0000000000..f65c186c14 --- /dev/null +++ b/pkg/stack/stackdump.go @@ -0,0 +1,57 @@ +package stack // import "github.com/docker/docker/pkg/stack" + +import ( + "fmt" + "os" + "path/filepath" + "runtime" + "strings" + "time" + + "github.com/pkg/errors" +) + +const stacksLogNameTemplate = "goroutine-stacks-%s.log" + +// Dump outputs the runtime stack to os.StdErr. +func Dump() { + _ = dump(os.Stderr) +} + +// DumpToFile appends the runtime stack into a file named "goroutine-stacks-.log" +// in dir and returns the full path to that file. If no directory name is +// provided, it outputs to os.Stderr. +func DumpToFile(dir string) (string, error) { + var f *os.File + if dir != "" { + path := filepath.Join(dir, fmt.Sprintf(stacksLogNameTemplate, strings.Replace(time.Now().Format(time.RFC3339), ":", "", -1))) + var err error + f, err = os.OpenFile(path, os.O_CREATE|os.O_WRONLY, 0666) + if err != nil { + return "", errors.Wrap(err, "failed to open file to write the goroutine stacks") + } + defer f.Close() + defer f.Sync() + } else { + f = os.Stderr + } + return f.Name(), dump(f) +} + +func dump(f *os.File) error { + var ( + buf []byte + stackSize int + ) + bufferLen := 16384 + for stackSize == len(buf) { + buf = make([]byte, bufferLen) + stackSize = runtime.Stack(buf, true) + bufferLen *= 2 + } + buf = buf[:stackSize] + if _, err := f.Write(buf); err != nil { + return errors.Wrap(err, "failed to write goroutine stacks") + } + return nil +} diff --git a/pkg/stack/stackdump_test.go b/pkg/stack/stackdump_test.go new file mode 100644 index 0000000000..ae58ba0a89 --- /dev/null +++ b/pkg/stack/stackdump_test.go @@ -0,0 +1,31 @@ +package stack // import "github.com/docker/docker/pkg/stack" + +import ( + "io/ioutil" + "os" + "testing" + + "gotest.tools/v3/assert" + is "gotest.tools/v3/assert/cmp" +) + +func TestDump(t *testing.T) { + Dump() +} + +func TestDumpToFile(t *testing.T) { + directory, err := ioutil.TempDir("", "test-dump-tasks") + assert.Check(t, err) + defer os.RemoveAll(directory) + dumpPath, err := DumpToFile(directory) + assert.Check(t, err) + readFile, _ := ioutil.ReadFile(dumpPath) + fileData := string(readFile) + assert.Check(t, is.Contains(fileData, "goroutine")) +} + +func TestDumpToFileWithEmptyInput(t *testing.T) { + path, err := DumpToFile("") + assert.Check(t, err) + assert.Check(t, is.Equal(os.Stderr.Name(), path)) +} From 0880df4644dcd6a6a79db27b7d0092c74bf90487 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 15 Jul 2021 18:08:17 +0200 Subject: [PATCH 2/3] pkg/signal: move Trap() to cmd/dockerd It's the only location where this is used, and it's quite specific to dockerd (not really a reusable function for external use), so moving it into that package. Signed-off-by: Sebastiaan van Stijn --- cmd/dockerd/daemon.go | 4 ++-- {pkg/signal => cmd/dockerd/trap}/testfiles/main.go | 4 ++-- {pkg/signal => cmd/dockerd/trap}/trap.go | 2 +- {pkg/signal => cmd/dockerd/trap}/trap_linux_test.go | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) rename {pkg/signal => cmd/dockerd/trap}/testfiles/main.go (91%) rename {pkg/signal => cmd/dockerd/trap}/trap.go (96%) rename {pkg/signal => cmd/dockerd/trap}/trap_linux_test.go (95%) diff --git a/cmd/dockerd/daemon.go b/cmd/dockerd/daemon.go index 1841253a06..a42ccfac76 100644 --- a/cmd/dockerd/daemon.go +++ b/cmd/dockerd/daemon.go @@ -32,6 +32,7 @@ import ( buildkit "github.com/docker/docker/builder/builder-next" "github.com/docker/docker/builder/dockerfile" "github.com/docker/docker/cli/debug" + "github.com/docker/docker/cmd/dockerd/trap" "github.com/docker/docker/daemon" "github.com/docker/docker/daemon/cluster" "github.com/docker/docker/daemon/config" @@ -44,7 +45,6 @@ import ( "github.com/docker/docker/pkg/jsonmessage" "github.com/docker/docker/pkg/pidfile" "github.com/docker/docker/pkg/plugingetter" - "github.com/docker/docker/pkg/signal" "github.com/docker/docker/pkg/sysinfo" "github.com/docker/docker/pkg/system" "github.com/docker/docker/plugin" @@ -183,7 +183,7 @@ func (cli *DaemonCli) start(opts *daemonOptions) (err error) { stopc := make(chan bool) defer close(stopc) - signal.Trap(func() { + trap.Trap(func() { cli.stop() <-stopc // wait for daemonCli.start() to return }, logrus.StandardLogger()) diff --git a/pkg/signal/testfiles/main.go b/cmd/dockerd/trap/testfiles/main.go similarity index 91% rename from pkg/signal/testfiles/main.go rename to cmd/dockerd/trap/testfiles/main.go index e56854c7c3..22cb60bd00 100644 --- a/pkg/signal/testfiles/main.go +++ b/cmd/dockerd/trap/testfiles/main.go @@ -5,7 +5,7 @@ import ( "syscall" "time" - "github.com/docker/docker/pkg/signal" + "github.com/docker/docker/cmd/dockerd/trap" "github.com/sirupsen/logrus" ) @@ -15,7 +15,7 @@ func main() { "QUIT": syscall.SIGQUIT, "INT": os.Interrupt, } - signal.Trap(func() { + trap.Trap(func() { time.Sleep(time.Second) os.Exit(99) }, logrus.StandardLogger()) diff --git a/pkg/signal/trap.go b/cmd/dockerd/trap/trap.go similarity index 96% rename from pkg/signal/trap.go rename to cmd/dockerd/trap/trap.go index a838b3e7b9..fa21d99d04 100644 --- a/pkg/signal/trap.go +++ b/cmd/dockerd/trap/trap.go @@ -1,4 +1,4 @@ -package signal // import "github.com/docker/docker/pkg/signal" +package trap // import "github.com/docker/docker/cmd/dockerd/trap" import ( "fmt" diff --git a/pkg/signal/trap_linux_test.go b/cmd/dockerd/trap/trap_linux_test.go similarity index 95% rename from pkg/signal/trap_linux_test.go rename to cmd/dockerd/trap/trap_linux_test.go index 3dce0cd238..b283c7da0a 100644 --- a/pkg/signal/trap_linux_test.go +++ b/cmd/dockerd/trap/trap_linux_test.go @@ -1,6 +1,6 @@ // +build linux -package signal // import "github.com/docker/docker/pkg/signal" +package trap // import "github.com/docker/docker/cmd/dockerd/trap" import ( "io/ioutil" From 6ff6913ac46dfa6748f1c6a2dd34ad6dee80b495 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 15 Jul 2021 18:24:23 +0200 Subject: [PATCH 3/3] pkg/signal: remove gotest.tools dependency Signed-off-by: Sebastiaan van Stijn --- pkg/signal/signal_linux_test.go | 23 +++++++++++++--------- pkg/signal/signal_test.go | 35 ++++++++++++++++++++++----------- 2 files changed, 37 insertions(+), 21 deletions(-) diff --git a/pkg/signal/signal_linux_test.go b/pkg/signal/signal_linux_test.go index decd454de3..5b30547273 100644 --- a/pkg/signal/signal_linux_test.go +++ b/pkg/signal/signal_linux_test.go @@ -7,9 +7,6 @@ import ( "syscall" "testing" "time" - - "gotest.tools/v3/assert" - is "gotest.tools/v3/assert/cmp" ) func TestCatchAll(t *testing.T) { @@ -28,9 +25,11 @@ func TestCatchAll(t *testing.T) { for sigStr := range listOfSignals { if signal, ok := SignalMap[sigStr]; ok { - syscall.Kill(syscall.Getpid(), signal) + _ = syscall.Kill(syscall.Getpid(), signal) s := <-sigs - assert.Check(t, is.Equal(s.String(), signal.String())) + if s.String() != signal.String() { + t.Errorf("expected: %q, got: %q", signal, s) + } } } } @@ -41,7 +40,9 @@ func TestCatchAllIgnoreSigUrg(t *testing.T) { defer StopCatch(sigs) err := syscall.Kill(syscall.Getpid(), syscall.SIGURG) - assert.NilError(t, err) + if err != nil { + t.Fatal(err) + } timer := time.NewTimer(1 * time.Second) defer timer.Stop() select { @@ -55,11 +56,15 @@ func TestStopCatch(t *testing.T) { signal := SignalMap["HUP"] channel := make(chan os.Signal, 1) CatchAll(channel) - syscall.Kill(syscall.Getpid(), signal) + _ = syscall.Kill(syscall.Getpid(), signal) signalString := <-channel - assert.Check(t, is.Equal(signalString.String(), signal.String())) + if signalString.String() != signal.String() { + t.Errorf("expected: %q, got: %q", signal, signalString) + } StopCatch(channel) _, ok := <-channel - assert.Check(t, is.Equal(ok, false)) + if ok { + t.Error("expected: !ok, got: ok") + } } diff --git a/pkg/signal/signal_test.go b/pkg/signal/signal_test.go index c275345b8b..c9fe394606 100644 --- a/pkg/signal/signal_test.go +++ b/pkg/signal/signal_test.go @@ -3,32 +3,43 @@ package signal // import "github.com/docker/docker/pkg/signal" import ( "syscall" "testing" - - "gotest.tools/v3/assert" - is "gotest.tools/v3/assert/cmp" ) func TestParseSignal(t *testing.T) { - _, checkAtoiError := ParseSignal("0") - assert.Check(t, is.Error(checkAtoiError, "Invalid signal: 0")) + _, err := ParseSignal("0") + expectedErr := "Invalid signal: 0" + if err == nil || err.Error() != expectedErr { + t.Errorf("expected %q, but got %v", expectedErr, err) + } - _, error := ParseSignal("SIG") - assert.Check(t, is.Error(error, "Invalid signal: SIG")) + _, err = ParseSignal("SIG") + expectedErr = "Invalid signal: SIG" + if err == nil || err.Error() != expectedErr { + t.Errorf("expected %q, but got %v", expectedErr, err) + } for sigStr := range SignalMap { - responseSignal, error := ParseSignal(sigStr) - assert.Check(t, error) + responseSignal, err := ParseSignal(sigStr) + if err != nil { + t.Error(err) + } signal := SignalMap[sigStr] - assert.Check(t, is.DeepEqual(signal, responseSignal)) + if responseSignal != signal { + t.Errorf("expected: %q, got: %q", signal, responseSignal) + } } } func TestValidSignalForPlatform(t *testing.T) { isValidSignal := ValidSignalForPlatform(syscall.Signal(0)) - assert.Check(t, is.Equal(false, isValidSignal)) + if isValidSignal { + t.Error("expected !isValidSignal") + } for _, sigN := range SignalMap { isValidSignal = ValidSignalForPlatform(sigN) - assert.Check(t, is.Equal(true, isValidSignal)) + if !isValidSignal { + t.Error("expected isValidSignal") + } } }