From 1e4aa28c9daf1ab221b67bea158a0c31d82cec67 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Mon, 21 Dec 2015 21:47:35 -0800 Subject: [PATCH 1/4] Adds child process reaping when Consul is running as PID 1. --- command/agent/command.go | 27 +++++++++++++++++++ command/agent/config.go | 10 +++++++ command/agent/config_test.go | 13 +++++++++ .../source/docs/agent/options.html.markdown | 4 +++ 4 files changed, 54 insertions(+) diff --git a/command/agent/command.go b/command/agent/command.go index 0993ccfa1..a5f59bee9 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -18,6 +18,7 @@ import ( "github.com/armon/go-metrics/datadog" "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" @@ -641,6 +642,32 @@ func (c *Command) Run(args []string) int { defer server.Shutdown() } + // Enable child process reaping + if !config.DisableReap && (os.Getpid() == 1) { + logger := c.agent.logger + if !reap.IsSupported() { + logger.Printf("[WARN] Running as PID 1 but child process reaping is not supported on this platform, disabling") + } else { + 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) + } + } + // Check and shut down the SCADA listeners at the end defer func() { if c.scadaHttp != nil { diff --git a/command/agent/config.go b/command/agent/config.go index c03659116..50db3244f 100644 --- a/command/agent/config.go +++ b/command/agent/config.go @@ -422,6 +422,12 @@ type Config struct { // Minimum Session TTL SessionTTLMin time.Duration `mapstructure:"-"` SessionTTLMinRaw string `mapstructure:"session_ttl_min"` + + // DisableReap controls automatic reaping of child processes, useful if + // running as PID 1 in a Docker container. This defaults to false, and + // reaping will be automatically enabled if this is false and Consul's + // PID is 1. + DisableReap bool `mapstructure:"disable_reap"` } // UnixSocketPermissions contains information about a unix socket, and @@ -1140,6 +1146,10 @@ func MergeConfig(a, b *Config) *Config { result.RetryJoinWan = append(result.RetryJoinWan, a.RetryJoinWan...) result.RetryJoinWan = append(result.RetryJoinWan, b.RetryJoinWan...) + if b.DisableReap { + result.DisableReap = true + } + return &result } diff --git a/command/agent/config_test.go b/command/agent/config_test.go index 000567b26..8ce8236ad 100644 --- a/command/agent/config_test.go +++ b/command/agent/config_test.go @@ -777,6 +777,17 @@ func TestDecodeConfig(t *testing.T) { if config.SessionTTLMin != 5*time.Second { t.Fatalf("bad: %s %#v", config.SessionTTLMin.String(), config) } + + // DisableReap + input = `{"disable_reap": true}` + config, err = DecodeConfig(bytes.NewReader([]byte(input))) + if err != nil { + t.Fatalf("err: %s", err) + } + + if config.DisableReap != true { + t.Fatalf("bad: reap not disabled: %#v", config) + } } func TestDecodeConfig_invalidKeys(t *testing.T) { @@ -1157,6 +1168,7 @@ func TestMergeConfig(t *testing.T) { CheckUpdateIntervalRaw: "8m", RetryIntervalRaw: "10s", RetryIntervalWanRaw: "10s", + DisableReap: false, } b := &Config{ @@ -1266,6 +1278,7 @@ func TestMergeConfig(t *testing.T) { RPC: &net.TCPAddr{}, RPCRaw: "127.0.0.5:1233", }, + DisableReap: true, } c := MergeConfig(a, b) diff --git a/website/source/docs/agent/options.html.markdown b/website/source/docs/agent/options.html.markdown index cadd50621..aac394f32 100644 --- a/website/source/docs/agent/options.html.markdown +++ b/website/source/docs/agent/options.html.markdown @@ -407,6 +407,10 @@ definitions support being updated during a reload. `disable_anonymous_signature` Disables providing an anonymous signature for de-duplication with the update check. See [`disable_update_check`](#disable_update_check). +* + `disable_reap` will prevent Consul from automatically reaping child processes if it + detects it is running as PID 1, such as in a Docker container. + * `disable_remote_exec` Disables support for remote execution. When set to true, the agent will ignore any incoming remote exec requests. From e1d456a0791d6de2b7d8f970fa37fdbd9b2b6756 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Tue, 22 Dec 2015 10:28:10 -0800 Subject: [PATCH 2/4] Changes sense of option to "reap" and uses nil for "not set". --- command/agent/command.go | 5 +++-- command/agent/config.go | 14 ++++++------ command/agent/config_test.go | 22 ++++++++++++++----- .../source/docs/agent/options.html.markdown | 9 ++++---- 4 files changed, 31 insertions(+), 19 deletions(-) diff --git a/command/agent/command.go b/command/agent/command.go index a5f59bee9..314b60612 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -643,10 +643,11 @@ func (c *Command) Run(args []string) int { } // Enable child process reaping - if !config.DisableReap && (os.Getpid() == 1) { + if (config.Reap != nil && *config.Reap) || (config.Reap == nil && os.Getpid() == 1) { logger := c.agent.logger if !reap.IsSupported() { - logger.Printf("[WARN] Running as PID 1 but child process reaping is not supported on this platform, disabling") + c.Ui.Error("Child process reaping is not supported on this platform (set reap=false)") + return 1 } else { logger.Printf("[DEBUG] Automatically reaping child processes") diff --git a/command/agent/config.go b/command/agent/config.go index 50db3244f..76149d31f 100644 --- a/command/agent/config.go +++ b/command/agent/config.go @@ -423,11 +423,11 @@ type Config struct { SessionTTLMin time.Duration `mapstructure:"-"` SessionTTLMinRaw string `mapstructure:"session_ttl_min"` - // DisableReap controls automatic reaping of child processes, useful if - // running as PID 1 in a Docker container. This defaults to false, and - // reaping will be automatically enabled if this is false and Consul's - // PID is 1. - DisableReap bool `mapstructure:"disable_reap"` + // Reap controls automatic reaping of child processes, useful if running + // as PID 1 in a Docker container. This defaults to nil which will make + // Consul reap only if it detects it's running as PID 1. If non-nil, + // then this will be used to decide if reaping is enabled. + Reap *bool `mapstructure:"reap"` } // UnixSocketPermissions contains information about a unix socket, and @@ -1146,8 +1146,8 @@ func MergeConfig(a, b *Config) *Config { result.RetryJoinWan = append(result.RetryJoinWan, a.RetryJoinWan...) result.RetryJoinWan = append(result.RetryJoinWan, b.RetryJoinWan...) - if b.DisableReap { - result.DisableReap = true + if b.Reap != nil { + result.Reap = b.Reap } return &result diff --git a/command/agent/config_test.go b/command/agent/config_test.go index 8ce8236ad..99a5b398e 100644 --- a/command/agent/config_test.go +++ b/command/agent/config_test.go @@ -778,15 +778,25 @@ func TestDecodeConfig(t *testing.T) { t.Fatalf("bad: %s %#v", config.SessionTTLMin.String(), config) } - // DisableReap - input = `{"disable_reap": true}` + // Reap + input = `{"reap": true}` config, err = DecodeConfig(bytes.NewReader([]byte(input))) if err != nil { t.Fatalf("err: %s", err) } - if config.DisableReap != true { - t.Fatalf("bad: reap not disabled: %#v", config) + if config.Reap == nil || *config.Reap != true { + t.Fatalf("bad: reap not enabled: %#v", config) + } + + input = `{}` + config, err = DecodeConfig(bytes.NewReader([]byte(input))) + if err != nil { + t.Fatalf("err: %s", err) + } + + if config.Reap != nil { + t.Fatalf("bad: reap not tri-stated: %#v", config) } } @@ -1168,7 +1178,6 @@ func TestMergeConfig(t *testing.T) { CheckUpdateIntervalRaw: "8m", RetryIntervalRaw: "10s", RetryIntervalWanRaw: "10s", - DisableReap: false, } b := &Config{ @@ -1278,8 +1287,9 @@ func TestMergeConfig(t *testing.T) { RPC: &net.TCPAddr{}, RPCRaw: "127.0.0.5:1233", }, - DisableReap: true, + Reap: new(bool), } + *b.Reap = true c := MergeConfig(a, b) diff --git a/website/source/docs/agent/options.html.markdown b/website/source/docs/agent/options.html.markdown index aac394f32..049d43463 100644 --- a/website/source/docs/agent/options.html.markdown +++ b/website/source/docs/agent/options.html.markdown @@ -407,10 +407,6 @@ definitions support being updated during a reload. `disable_anonymous_signature` Disables providing an anonymous signature for de-duplication with the update check. See [`disable_update_check`](#disable_update_check). -* - `disable_reap` will prevent Consul from automatically reaping child processes if it - detects it is running as PID 1, such as in a Docker container. - * `disable_remote_exec` Disables support for remote execution. When set to true, the agent will ignore any incoming remote exec requests. @@ -508,6 +504,11 @@ definitions support being updated during a reload. * `protocol` Equivalent to the [`-protocol` command-line flag](#_protocol). +* `reap` controls Consul's automatic reaping of child processes, which + is useful if Consul is running as PID 1 in a Docker container. If this isn't specified, then Consul will + automatically reap child processes if it detects it is running as PID 1. If this is specified, then it + controls reaping regardless of Consul's PID. + * `recursor` Provides a single recursor address. This has been deprecated, and the value is appended to the [`recursors`](#recursors) list for backwards compatibility. From d71036c4f01dec5131f447ca2f0911aabd8e57a8 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Tue, 22 Dec 2015 10:29:55 -0800 Subject: [PATCH 3/4] Moves logger down where it's used for reaping. --- command/agent/command.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/agent/command.go b/command/agent/command.go index 314b60612..5b3c08a39 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -644,11 +644,11 @@ func (c *Command) Run(args []string) int { // Enable child process reaping if (config.Reap != nil && *config.Reap) || (config.Reap == nil && os.Getpid() == 1) { - logger := c.agent.logger 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) From 4dd7a5fbaaddd26c5a3886b63236dba835c04550 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Tue, 22 Dec 2015 10:43:32 -0800 Subject: [PATCH 4/4] Adds a Bool helper function. --- command/agent/config.go | 5 +++++ command/agent/config_test.go | 3 +-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/command/agent/config.go b/command/agent/config.go index 76149d31f..d0c68df4c 100644 --- a/command/agent/config.go +++ b/command/agent/config.go @@ -430,6 +430,11 @@ type Config struct { Reap *bool `mapstructure:"reap"` } +// Bool is used to initialize bool pointers in struct literals. +func Bool(b bool) *bool { + return &b +} + // UnixSocketPermissions contains information about a unix socket, and // implements the FilePermissions interface. type UnixSocketPermissions struct { diff --git a/command/agent/config_test.go b/command/agent/config_test.go index 99a5b398e..72af551ab 100644 --- a/command/agent/config_test.go +++ b/command/agent/config_test.go @@ -1287,9 +1287,8 @@ func TestMergeConfig(t *testing.T) { RPC: &net.TCPAddr{}, RPCRaw: "127.0.0.5:1233", }, - Reap: new(bool), + Reap: Bool(true), } - *b.Reap = true c := MergeConfig(a, b)