From 5ac5a8ccfcadb79116e95c36b8f9704c9bdf2d26 Mon Sep 17 00:00:00 2001 From: Adam Wolfe Gordon Date: Tue, 4 Oct 2016 09:36:41 -0600 Subject: [PATCH] agent: Stop reaping child processes (resolves #1988) The consul docker image now uses dumb-init to reap child processes, so there's no need to reap them ourselves. --- command/agent/agent.go | 9 -------- command/agent/command.go | 32 ++--------------------------- command/agent/remote_exec.go | 6 ------ command/agent/watch_handler.go | 9 +------- command/agent/watch_handler_test.go | 3 +-- 5 files changed, 4 insertions(+), 55 deletions(-) diff --git a/command/agent/agent.go b/command/agent/agent.go index 6284a25f02..e04c52d339 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -116,14 +116,6 @@ type Agent struct { // agent methods use this, so use with care and never override // outside of a unit test. endpoints map[string]string - - // reapLock is used to prevent child process reaping from interfering - // with normal waiting for subprocesses to complete. Any time you exec - // and wait, you should take a read lock on this mutex. Only the reaper - // takes the write lock. This setup prevents us from serializing all the - // child process management with each other, it just serializes them - // with the child process reaper. - reapLock sync.RWMutex } // Create is used to create a new Agent. Returns @@ -1042,7 +1034,6 @@ func (a *Agent) AddCheck(check *structs.HealthCheck, chkType *CheckType, persist Interval: chkType.Interval, Timeout: chkType.Timeout, Logger: a.logger, - ReapLock: &a.reapLock, } monitor.Start() a.checkMonitors[check.CheckID] = monitor diff --git a/command/agent/command.go b/command/agent/command.go index 35420e9172..a417b02964 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -20,7 +20,6 @@ import ( "github.com/hashicorp/consul/lib" "github.com/hashicorp/consul/watch" "github.com/hashicorp/go-checkpoint" - "github.com/hashicorp/go-reap" "github.com/hashicorp/go-syslog" "github.com/hashicorp/logutils" scada "github.com/hashicorp/scada-client/scada" @@ -771,33 +770,6 @@ func (c *Command) Run(args []string) int { defer server.Shutdown() } - // Enable child process reaping - if (config.Reap != nil && *config.Reap) || (config.Reap == nil && os.Getpid() == 1) { - if !reap.IsSupported() { - c.Ui.Error("Child process reaping is not supported on this platform (set reap=false)") - return 1 - } else { - logger := c.agent.logger - logger.Printf("[DEBUG] Automatically reaping child processes") - - pids := make(reap.PidCh, 1) - errors := make(reap.ErrorCh, 1) - go func() { - for { - select { - case pid := <-pids: - logger.Printf("[DEBUG] Reaped child process %d", pid) - case err := <-errors: - logger.Printf("[ERR] Error reaping child process: %v", err) - case <-c.agent.shutdownCh: - return - } - } - }() - go reap.ReapChildren(pids, errors, c.agent.shutdownCh, &c.agent.reapLock) - } - } - // Check and shut down the SCADA listeners at the end defer func() { if c.scadaHttp != nil { @@ -829,7 +801,7 @@ func (c *Command) Run(args []string) int { // Register the watches for _, wp := range config.WatchPlans { go func(wp *watch.WatchPlan) { - wp.Handler = makeWatchHandler(logOutput, wp.Exempt["handler"], &c.agent.reapLock) + wp.Handler = makeWatchHandler(logOutput, wp.Exempt["handler"]) wp.LogOutput = c.logOutput if err := wp.Run(httpAddr.String()); err != nil { c.Ui.Error(fmt.Sprintf("Error running watch: %v", err)) @@ -1017,7 +989,7 @@ func (c *Command) handleReload(config *Config) *Config { // Register the new watches for _, wp := range newConf.WatchPlans { go func(wp *watch.WatchPlan) { - wp.Handler = makeWatchHandler(c.logOutput, wp.Exempt["handler"], &c.agent.reapLock) + wp.Handler = makeWatchHandler(c.logOutput, wp.Exempt["handler"]) wp.LogOutput = c.logOutput if err := wp.Run(httpAddr.String()); err != nil { c.Ui.Error(fmt.Sprintf("Error running watch: %v", err)) diff --git a/command/agent/remote_exec.go b/command/agent/remote_exec.go index 7c8268dca8..20c4bb0b91 100644 --- a/command/agent/remote_exec.go +++ b/command/agent/remote_exec.go @@ -135,12 +135,6 @@ func (a *Agent) handleRemoteExec(msg *UserEvent) { return } - // Disable child process reaping so that we can get this command's - // return value. Note that we take the read lock here since we are - // waiting on a specific PID and don't need to serialize all waits. - a.reapLock.RLock() - defer a.reapLock.RUnlock() - // Ensure we write out an exit code exitCode := 0 defer a.remoteExecWriteExitCode(&event, &exitCode) diff --git a/command/agent/watch_handler.go b/command/agent/watch_handler.go index 5ed210500c..afc4fb94d2 100644 --- a/command/agent/watch_handler.go +++ b/command/agent/watch_handler.go @@ -8,7 +8,6 @@ import ( "log" "os" "strconv" - "sync" "github.com/armon/circbuf" "github.com/hashicorp/consul/watch" @@ -34,16 +33,10 @@ func verifyWatchHandler(params interface{}) error { } // makeWatchHandler returns a handler for the given watch -func makeWatchHandler(logOutput io.Writer, params interface{}, reapLock *sync.RWMutex) watch.HandlerFunc { +func makeWatchHandler(logOutput io.Writer, params interface{}) watch.HandlerFunc { script := params.(string) logger := log.New(logOutput, "", log.LstdFlags) fn := func(idx uint64, data interface{}) { - // Disable child process reaping so that we can get this command's - // return value. Note that we take the read lock here since we are - // waiting on a specific PID and don't need to serialize all waits. - reapLock.RLock() - defer reapLock.RUnlock() - // Create the command cmd, err := ExecScript(script) if err != nil { diff --git a/command/agent/watch_handler_test.go b/command/agent/watch_handler_test.go index 3980fd26c0..28f1e425f5 100644 --- a/command/agent/watch_handler_test.go +++ b/command/agent/watch_handler_test.go @@ -3,7 +3,6 @@ package agent import ( "io/ioutil" "os" - "sync" "testing" ) @@ -26,7 +25,7 @@ func TestMakeWatchHandler(t *testing.T) { defer os.Remove("handler_out") defer os.Remove("handler_index_out") script := "echo $CONSUL_INDEX >> handler_index_out && cat >> handler_out" - handler := makeWatchHandler(os.Stderr, script, &sync.RWMutex{}) + handler := makeWatchHandler(os.Stderr, script) handler(100, []string{"foo", "bar", "baz"}) raw, err := ioutil.ReadFile("handler_out") if err != nil {