From 16754102560f5c26527c8a181c17dc3016b4c9cf Mon Sep 17 00:00:00 2001 From: Sebastien Boeuf Date: Tue, 18 Sep 2018 22:42:18 -0700 Subject: [PATCH 1/2] pkg: signals: Factorize signals handling through a Go package In order to reuse the same scheme across several components of the runtime repository, we need to factorize the code handling signalling through a common package. The immediate use case will be to use this package from both the CLI and the network monitor. Signed-off-by: Sebastien Boeuf --- cli/kill.go | 8 +- cli/main.go | 23 +++-- cli/signals.go | 99 --------------------- pkg/signals/signals.go | 127 +++++++++++++++++++++++++++ {cli => pkg/signals}/signals_test.go | 37 +++++--- 5 files changed, 173 insertions(+), 121 deletions(-) delete mode 100644 cli/signals.go create mode 100644 pkg/signals/signals.go rename {cli => pkg/signals}/signals_test.go (76%) diff --git a/cli/kill.go b/cli/kill.go index 0e79d441d..0f47e3b13 100644 --- a/cli/kill.go +++ b/cli/kill.go @@ -58,7 +58,7 @@ EXAMPLE: }, } -var signals = map[string]syscall.Signal{ +var signalList = map[string]syscall.Signal{ "SIGABRT": syscall.SIGABRT, "SIGALRM": syscall.SIGALRM, "SIGBUS": syscall.SIGBUS, @@ -159,13 +159,13 @@ func kill(ctx context.Context, containerID, signal string, all bool) error { } func processSignal(signal string) (syscall.Signal, error) { - signum, signalOk := signals[signal] + signum, signalOk := signalList[signal] if signalOk { return signum, nil } // Support for short name signals (INT) - signum, signalOk = signals["SIG"+signal] + signum, signalOk = signalList["SIG"+signal] if signalOk { return signum, nil } @@ -178,7 +178,7 @@ func processSignal(signal string) (syscall.Signal, error) { signum = syscall.Signal(s) // Check whether signal is valid or not - for _, sig := range signals { + for _, sig := range signalList { if sig == signum { // signal is a valid signal return signum, nil diff --git a/cli/main.go b/cli/main.go index 546062b95..2611306c8 100644 --- a/cli/main.go +++ b/cli/main.go @@ -17,6 +17,7 @@ import ( "strings" "syscall" + "github.com/kata-containers/runtime/pkg/signals" vc "github.com/kata-containers/runtime/virtcontainers" vf "github.com/kata-containers/runtime/virtcontainers/factory" "github.com/kata-containers/runtime/virtcontainers/pkg/oci" @@ -178,12 +179,18 @@ func init() { // first (root) span must be created in beforeSubcommands()): it is simply // used to pass to the crash handling functions to finalise tracing. func setupSignalHandler(ctx context.Context) { + signals.SetLogger(kataLog) + sigCh := make(chan os.Signal, 8) - for _, sig := range handledSignals() { + for _, sig := range signals.HandledSignals() { signal.Notify(sigCh, sig) } + dieCb := func() { + stopTracing(ctx) + } + go func() { for { sig := <-sigCh @@ -195,12 +202,12 @@ func setupSignalHandler(ctx context.Context) { continue } - if fatalSignal(nativeSignal) { + if signals.FatalSignal(nativeSignal) { kataLog.WithField("signal", sig).Error("received fatal signal") - die(ctx) - } else if debug && nonFatalSignal(nativeSignal) { + signals.Die(dieCb) + } else if debug && signals.NonFatalSignal(nativeSignal) { kataLog.WithField("signal", sig).Debug("handling signal") - backtrace() + signals.Backtrace() } } }() @@ -521,7 +528,11 @@ func main() { // create a new empty context ctx := context.Background() - defer handlePanic(ctx) + dieCb := func() { + stopTracing(ctx) + } + + defer signals.HandlePanic(dieCb) createRuntime(ctx) } diff --git a/cli/signals.go b/cli/signals.go deleted file mode 100644 index b4b7b2b32..000000000 --- a/cli/signals.go +++ /dev/null @@ -1,99 +0,0 @@ -// Copyright 2018 Intel Corporation. -// -// SPDX-License-Identifier: Apache-2.0 -// - -package main - -import ( - "bytes" - "context" - "fmt" - "os/signal" - "runtime/pprof" - "strings" - "syscall" -) - -// List of handled signals. -// -// The value is true if receiving the signal should be fatal. -var handledSignalsMap = map[syscall.Signal]bool{ - syscall.SIGABRT: true, - syscall.SIGBUS: true, - syscall.SIGILL: true, - syscall.SIGQUIT: true, - syscall.SIGSEGV: true, - syscall.SIGSTKFLT: true, - syscall.SIGSYS: true, - syscall.SIGTRAP: true, - syscall.SIGUSR1: false, -} - -func handlePanic(ctx context.Context) { - r := recover() - - if r != nil { - msg := fmt.Sprintf("%s", r) - kataLog.WithField("panic", msg).Error("fatal error") - - die(ctx) - } -} - -func backtrace() { - profiles := pprof.Profiles() - - buf := &bytes.Buffer{} - - for _, p := range profiles { - // The magic number requests a full stacktrace. See - // https://golang.org/pkg/runtime/pprof/#Profile.WriteTo. - pprof.Lookup(p.Name()).WriteTo(buf, 2) - } - - for _, line := range strings.Split(buf.String(), "\n") { - kataLog.Error(line) - } -} - -func fatalSignal(sig syscall.Signal) bool { - s, exists := handledSignalsMap[sig] - if !exists { - return false - } - - return s -} - -func nonFatalSignal(sig syscall.Signal) bool { - s, exists := handledSignalsMap[sig] - if !exists { - return false - } - - return !s -} - -func handledSignals() []syscall.Signal { - var signals []syscall.Signal - - for sig := range handledSignalsMap { - signals = append(signals, sig) - } - - return signals -} - -func die(ctx context.Context) { - stopTracing(ctx) - - backtrace() - - if crashOnError { - signal.Reset(syscall.SIGABRT) - syscall.Kill(0, syscall.SIGABRT) - } - - exit(1) -} diff --git a/pkg/signals/signals.go b/pkg/signals/signals.go new file mode 100644 index 000000000..a405ad09d --- /dev/null +++ b/pkg/signals/signals.go @@ -0,0 +1,127 @@ +// Copyright 2018 Intel Corporation. +// +// SPDX-License-Identifier: Apache-2.0 +// + +package signals + +import ( + "bytes" + "fmt" + "os" + "os/signal" + "runtime/pprof" + "strings" + "syscall" + + "github.com/sirupsen/logrus" +) + +var signalLog = logrus.WithField("default-signal-logger", true) + +// CrashOnError causes a coredump to be produced when an internal error occurs +// or a fatal signal is received. +var CrashOnError = false + +// List of handled signals. +// +// The value is true if receiving the signal should be fatal. +var handledSignalsMap = map[syscall.Signal]bool{ + syscall.SIGABRT: true, + syscall.SIGBUS: true, + syscall.SIGILL: true, + syscall.SIGQUIT: true, + syscall.SIGSEGV: true, + syscall.SIGSTKFLT: true, + syscall.SIGSYS: true, + syscall.SIGTRAP: true, + syscall.SIGUSR1: false, +} + +// DieCb is the callback function type that needs to be defined for every call +// into the Die() function. This callback will be run as the first function of +// the Die() implementation. +type DieCb func() + +// SetLogger sets the custom logger to be used by this package. If not called, +// the package will create its own logger. +func SetLogger(logger *logrus.Entry) { + signalLog = logger +} + +// HandlePanic writes a message to the logger and then calls Die(). +func HandlePanic(dieCb DieCb) { + r := recover() + + if r != nil { + msg := fmt.Sprintf("%s", r) + signalLog.WithField("panic", msg).Error("fatal error") + + Die(dieCb) + } +} + +// Backtrace writes a multi-line backtrace to the logger. +func Backtrace() { + profiles := pprof.Profiles() + + buf := &bytes.Buffer{} + + for _, p := range profiles { + // The magic number requests a full stacktrace. See + // https://golang.org/pkg/runtime/pprof/#Profile.WriteTo. + pprof.Lookup(p.Name()).WriteTo(buf, 2) + } + + for _, line := range strings.Split(buf.String(), "\n") { + signalLog.Error(line) + } +} + +// FatalSignal returns true if the specified signal should cause the program +// to abort. +func FatalSignal(sig syscall.Signal) bool { + s, exists := handledSignalsMap[sig] + if !exists { + return false + } + + return s +} + +// NonFatalSignal returns true if the specified signal should simply cause the +// program to Backtrace() but continue running. +func NonFatalSignal(sig syscall.Signal) bool { + s, exists := handledSignalsMap[sig] + if !exists { + return false + } + + return !s +} + +// HandledSignals returns a list of signals the package can deal with. +func HandledSignals() []syscall.Signal { + var signals []syscall.Signal + + for sig := range handledSignalsMap { + signals = append(signals, sig) + } + + return signals +} + +// Die causes a backtrace to be produced. If CrashOnError is set a coredump +// will be produced, else the program will exit. +func Die(dieCb DieCb) { + dieCb() + + Backtrace() + + if CrashOnError { + signal.Reset(syscall.SIGABRT) + syscall.Kill(0, syscall.SIGABRT) + } + + os.Exit(1) +} diff --git a/cli/signals_test.go b/pkg/signals/signals_test.go similarity index 76% rename from cli/signals_test.go rename to pkg/signals/signals_test.go index b0b76f65e..1ea68546f 100644 --- a/cli/signals_test.go +++ b/pkg/signals/signals_test.go @@ -3,10 +3,11 @@ // SPDX-License-Identifier: Apache-2.0 // -package main +package signals import ( "bytes" + "os" "reflect" goruntime "runtime" "sort" @@ -14,6 +15,7 @@ import ( "syscall" "testing" + "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" ) @@ -21,7 +23,7 @@ func TestSignalFatalSignal(t *testing.T) { assert := assert.New(t) for sig, fatal := range handledSignalsMap { - result := nonFatalSignal(sig) + result := NonFatalSignal(sig) if fatal { assert.False(result) } else { @@ -34,7 +36,7 @@ func TestSignalHandledSignalsMap(t *testing.T) { assert := assert.New(t) for sig, fatal := range handledSignalsMap { - result := fatalSignal(sig) + result := FatalSignal(sig) if fatal { assert.True(result) } else { @@ -52,7 +54,7 @@ func TestSignalHandledSignals(t *testing.T) { expected = append(expected, sig) } - got := handledSignals() + got := HandledSignals() sort.Slice(expected, func(i, j int) bool { return int(expected[i]) < int(expected[j]) @@ -69,7 +71,7 @@ func TestSignalNonFatalSignal(t *testing.T) { assert := assert.New(t) for sig, fatal := range handledSignalsMap { - result := nonFatalSignal(sig) + result := NonFatalSignal(sig) if fatal { assert.False(result) } else { @@ -83,7 +85,7 @@ func TestSignalFatalSignalInvalidSignal(t *testing.T) { sig := syscall.SIGXCPU - result := fatalSignal(sig) + result := FatalSignal(sig) assert.False(result) } @@ -92,23 +94,34 @@ func TestSignalNonFatalSignalInvalidSignal(t *testing.T) { sig := syscall.SIGXCPU - result := nonFatalSignal(sig) + result := NonFatalSignal(sig) assert.False(result) } func TestSignalBacktrace(t *testing.T) { assert := assert.New(t) + savedLog := signalLog + defer func() { + signalLog = savedLog + }() + + signalLog = logrus.WithFields(logrus.Fields{ + "name": "name", + "pid": os.Getpid(), + "source": "throttler", + "test-logger": true}) + // create buffer to save logger output buf := &bytes.Buffer{} - savedOut := kataLog.Logger.Out + savedOut := signalLog.Logger.Out defer func() { - kataLog.Logger.Out = savedOut + signalLog.Logger.Out = savedOut }() // capture output to buffer - kataLog.Logger.Out = buf + signalLog.Logger.Out = buf // determine name of *this* function pc := make([]uintptr, 1) @@ -116,12 +129,12 @@ func TestSignalBacktrace(t *testing.T) { fn := goruntime.FuncForPC(pc[0]) name := fn.Name() - backtrace() + Backtrace() b := buf.String() // very basic tests to check if a backtrace was produced assert.True(strings.Contains(b, "contention:")) - assert.True(strings.Contains(b, `"level":"error"`)) + assert.True(strings.Contains(b, `level=error`)) assert.True(strings.Contains(b, name)) } From 048616fb8df9b9601b2551da0052d598c98b2b47 Mon Sep 17 00:00:00 2001 From: Sebastien Boeuf Date: Tue, 18 Sep 2018 23:10:51 -0700 Subject: [PATCH 2/2] netmon: Add signals handler After the signals package has been created and shared with the CLI, this commit calls into it in order to properly handle the signals directed to the network monitor process. Fixes #718 Signed-off-by: Sebastien Boeuf --- netmon/netmon.go | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/netmon/netmon.go b/netmon/netmon.go index 9ba6d0ecc..d401e75ad 100644 --- a/netmon/netmon.go +++ b/netmon/netmon.go @@ -7,6 +7,7 @@ package main import ( "encoding/json" + "errors" "flag" "fmt" "io/ioutil" @@ -14,10 +15,13 @@ import ( "net" "os" "os/exec" + "os/signal" "path/filepath" "strings" + "syscall" "time" + "github.com/kata-containers/runtime/pkg/signals" "github.com/sirupsen/logrus" lSyslog "github.com/sirupsen/logrus/hooks/syslog" "github.com/vishvananda/netlink" @@ -203,6 +207,39 @@ func (n *netmon) cleanup() { close(n.rtDoneCh) } +// setupSignalHandler sets up signal handling, starting a go routine to deal +// with signals as they arrive. +func (n *netmon) setupSignalHandler() { + signals.SetLogger(n.logger()) + + sigCh := make(chan os.Signal, 8) + + for _, sig := range signals.HandledSignals() { + signal.Notify(sigCh, sig) + } + + go func() { + for { + sig := <-sigCh + + nativeSignal, ok := sig.(syscall.Signal) + if !ok { + err := errors.New("unknown signal") + netmonLog.WithError(err).WithField("signal", sig.String()).Error() + continue + } + + if signals.FatalSignal(nativeSignal) { + netmonLog.WithField("signal", sig).Error("received fatal signal") + signals.Die(nil) + } else if n.debug && signals.NonFatalSignal(nativeSignal) { + netmonLog.WithField("signal", sig).Debug("handling signal") + signals.Backtrace() + } + } + }() +} + func (n *netmon) logger() *logrus.Entry { fields := logrus.Fields{ "name": netmonName, @@ -641,6 +678,9 @@ func main() { os.Exit(1) } + // Setup signal handlers + n.setupSignalHandler() + // Scan the current interfaces. if err := n.scanNetwork(); err != nil { n.logger().WithError(err).Fatal("scanNetwork()")