From 9eaffc456f214ef8854c118404340f1124f1d7b0 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Thu, 31 Mar 2016 17:45:14 -0700 Subject: [PATCH 1/3] skip_leave_on_int's default changes based on agent mode `skip_leave_on_int`'s behavior now changes based on whether or not the agent is acting as a client or server. Fixes: 1687 --- command/agent/command.go | 13 +++- command/agent/command_test.go | 63 ++++++++++++++++--- command/agent/config.go | 11 ++-- command/agent/config_test.go | 11 ++-- .../source/docs/agent/options.html.markdown | 16 +++-- 5 files changed, 90 insertions(+), 24 deletions(-) diff --git a/command/agent/command.go b/command/agent/command.go index c11b90c5c..86db5eb73 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -175,6 +175,17 @@ func (c *Command) readConfig() *Config { return nil } + // Make sure SkipLeaveOnInt is set to the right default based on the + // agent's mode (client or server) + if config.SkipLeaveOnInt == nil { + config.SkipLeaveOnInt = new(bool) + if config.Server { + *config.SkipLeaveOnInt = true + } else { + *config.SkipLeaveOnInt = false + } + } + // Ensure we have a data directory if config.DataDir == "" && !dev { c.Ui.Error("Must specify data directory using -data-dir") @@ -810,7 +821,7 @@ WAIT: // Check if we should do a graceful leave graceful := false - if sig == os.Interrupt && !config.SkipLeaveOnInt { + if sig == os.Interrupt && !(*config.SkipLeaveOnInt) { graceful = true } else if sig == syscall.SIGTERM && config.LeaveOnTerm { graceful = true diff --git a/command/agent/command_test.go b/command/agent/command_test.go index cc0ec21e3..773a510d8 100644 --- a/command/agent/command_test.go +++ b/command/agent/command_test.go @@ -110,17 +110,17 @@ func TestRetryJoin(t *testing.T) { } func TestReadCliConfig(t *testing.T) { + tmpDir, err := ioutil.TempDir("", "consul") + if err != nil { + t.Fatalf("err: %s", err) + } + defer os.RemoveAll(tmpDir) shutdownCh := make(chan struct{}) defer close(shutdownCh) // Test config parse { - tmpDir, err := ioutil.TempDir("", "consul") - if err != nil { - t.Fatalf("err: %s", err) - } - cmd := &Command{ args: []string{ "-data-dir", tmpDir, @@ -137,12 +137,59 @@ func TestReadCliConfig(t *testing.T) { } } + // Test SkipLeaveOnInt default for server mode + { + ui := new(cli.MockUi) + cmd := &Command{ + args: []string{ + "-node", `"server1"`, + "-server", + "-data-dir", tmpDir, + }, + ShutdownCh: shutdownCh, + Ui: ui, + } + + config := cmd.readConfig() + if config == nil { + t.Fatalf(`Expected non-nil config object: %s`, ui.ErrorWriter.String()) + } + if config.Server != true { + t.Errorf(`Expected -server to be true`) + } + if (*config.SkipLeaveOnInt) != true { + t.Errorf(`Expected SkipLeaveOnInt to be true in server mode`) + } + } + + // Test SkipLeaveOnInt default for client mode + { + ui := new(cli.MockUi) + cmd := &Command{ + args: []string{ + "-data-dir", tmpDir, + "-node", `"client"`, + }, + ShutdownCh: shutdownCh, + Ui: ui, + } + + config := cmd.readConfig() + if config == nil { + t.Fatalf(`Expected non-nil config object: %s`, ui.ErrorWriter.String()) + } + if config.Server != false { + t.Errorf(`Expected server to be false`) + } + if *config.SkipLeaveOnInt != false { + t.Errorf(`Expected SkipLeaveOnInt to be false in client mode`) + } + } + // Test empty node name { cmd := &Command{ - args: []string{ - "-node", `""`, - }, + args: []string{"-node", `""`}, ShutdownCh: shutdownCh, Ui: new(cli.MockUi), } diff --git a/command/agent/config.go b/command/agent/config.go index eb3e659b2..2e22dcf98 100644 --- a/command/agent/config.go +++ b/command/agent/config.go @@ -227,9 +227,10 @@ type Config struct { // the TERM signal. Defaults false. This can be changed on reload. LeaveOnTerm bool `mapstructure:"leave_on_terminate"` - // SkipLeaveOnInt controls if Serf skips a graceful leave when receiving - // the INT signal. Defaults false. This can be changed on reload. - SkipLeaveOnInt bool `mapstructure:"skip_leave_on_interrupt"` + // SkipLeaveOnInt controls if Serf skips a graceful leave when + // receiving the INT signal. Defaults false on clients, true on + // servers. This can be changed on reload. + SkipLeaveOnInt *bool `mapstructure:"skip_leave_on_interrupt"` Telemetry Telemetry `mapstructure:"telemetry"` @@ -1018,8 +1019,8 @@ func MergeConfig(a, b *Config) *Config { if b.LeaveOnTerm == true { result.LeaveOnTerm = true } - if b.SkipLeaveOnInt == true { - result.SkipLeaveOnInt = true + if b.SkipLeaveOnInt != nil { + result.SkipLeaveOnInt = b.SkipLeaveOnInt } if b.Telemetry.DisableHostname == true { result.Telemetry.DisableHostname = true diff --git a/command/agent/config_test.go b/command/agent/config_test.go index e78736702..72cae23a5 100644 --- a/command/agent/config_test.go +++ b/command/agent/config_test.go @@ -74,8 +74,8 @@ func TestDecodeConfig(t *testing.T) { t.Fatalf("bad: %#v", config) } - if config.SkipLeaveOnInt != DefaultConfig().SkipLeaveOnInt { - t.Fatalf("bad: %#v", config) + if config.SkipLeaveOnInt != nil { + t.Fatalf("bad: expected nil SkipLeaveOnInt") } if config.LeaveOnTerm != DefaultConfig().LeaveOnTerm { @@ -290,7 +290,7 @@ func TestDecodeConfig(t *testing.T) { t.Fatalf("err: %s", err) } - if config.SkipLeaveOnInt != true { + if *config.SkipLeaveOnInt != true { t.Fatalf("bad: %#v", config) } @@ -1239,7 +1239,7 @@ func TestMergeConfig(t *testing.T) { AdvertiseAddr: "127.0.0.1", Server: false, LeaveOnTerm: false, - SkipLeaveOnInt: false, + SkipLeaveOnInt: new(bool), EnableDebug: false, CheckUpdateIntervalRaw: "8m", RetryIntervalRaw: "10s", @@ -1294,7 +1294,7 @@ func TestMergeConfig(t *testing.T) { }, Server: true, LeaveOnTerm: true, - SkipLeaveOnInt: true, + SkipLeaveOnInt: new(bool), EnableDebug: true, VerifyIncoming: true, VerifyOutgoing: true, @@ -1368,6 +1368,7 @@ func TestMergeConfig(t *testing.T) { }, Reap: Bool(true), } + *b.SkipLeaveOnInt = true c := MergeConfig(a, b) diff --git a/website/source/docs/agent/options.html.markdown b/website/source/docs/agent/options.html.markdown index 2b8c6b81b..fffeba52b 100644 --- a/website/source/docs/agent/options.html.markdown +++ b/website/source/docs/agent/options.html.markdown @@ -621,11 +621,17 @@ Consul will not enable TLS for the HTTP API unless the `https` port has been ass at or above the default to encourage clients to send infrequent heartbeats. Defaults to 10s. -* `skip_leave_on_interrupt` - This is similar to [`leave_on_terminate`](#leave_on_terminate) but - only affects interrupt handling. By default, an interrupt (such as hitting - Control-C in a shell) causes Consul to gracefully leave. Setting this to true - disables that. Defaults to false. +* `skip_leave_on_interrupt` This is + similar to [`leave_on_terminate`](#leave_on_terminate) but only affects + interrupt handling. When Consul receives an interrupt signal (such as + hitting Control-C in a terminal), Consul will gracefully leave the cluster. + Setting this to `true` disables that behavior. The default behavior for + this feature varies based on whether or not the agent is running as a + client or a server. On agents in client-mode, this defaults to `false` and + for agents in server-mode, this defaults to `true` (i.e. Ctrl-C on a server + will keep the server in the cluster and therefore quorum, and Ctrl-C on a + client will gracefully leave). * `start_join` An array of strings specifying addresses of nodes to [`-join`](#_join) upon startup. From 62231bae6b8abb5da961668c0cf3b384a1a0faac Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Thu, 31 Mar 2016 17:58:06 -0700 Subject: [PATCH 2/3] Add changelog entry for skip_leave_on_interrupt default behavior changing --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 19382488d..999ae4d97 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,9 @@ IMPROVEMENTS: [agent options](https://www.consul.io/docs/agent/options.html#udp_answer_limit) documentation for additional details for when this should be used. [GH-1712] +* `skip_leave_on_interrupt`'s default behavior is now dependent on whether or + not the agent is acting as a server or client. When Consul is started as a + server the default is `true` and `false` when a client. [GH-1909] ## 0.6.4 (March 16, 2016) From 8156eb9953b35be4d939e361383f4d228e069b36 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Thu, 31 Mar 2016 18:06:58 -0700 Subject: [PATCH 3/3] Add a note re: pre-0.7 behavior --- website/source/docs/agent/options.html.markdown | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/website/source/docs/agent/options.html.markdown b/website/source/docs/agent/options.html.markdown index fffeba52b..df9085897 100644 --- a/website/source/docs/agent/options.html.markdown +++ b/website/source/docs/agent/options.html.markdown @@ -628,10 +628,11 @@ Consul will not enable TLS for the HTTP API unless the `https` port has been ass hitting Control-C in a terminal), Consul will gracefully leave the cluster. Setting this to `true` disables that behavior. The default behavior for this feature varies based on whether or not the agent is running as a - client or a server. On agents in client-mode, this defaults to `false` and - for agents in server-mode, this defaults to `true` (i.e. Ctrl-C on a server - will keep the server in the cluster and therefore quorum, and Ctrl-C on a - client will gracefully leave). + client or a server (prior to Consul 0.7 the default value was + unconditionally set to `false`). On agents in client-mode, this defaults + to `false` and for agents in server-mode, this defaults to `true` + (i.e. Ctrl-C on a server will keep the server in the cluster and therefore + quorum, and Ctrl-C on a client will gracefully leave). * `start_join` An array of strings specifying addresses of nodes to [`-join`](#_join) upon startup.