Merge pull request #1909 from hashicorp/f-toggle-skip-leave-on-int-server

skip_leave_on_int's default changes based on agent mode

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)
 

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

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{
+			args:       []string{"-node", `""`},
-				"-node", `""`,
-			},
 			ShutdownCh: shutdownCh,
 			Ui:         new(cli.MockUi),
 		}

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
+	// SkipLeaveOnInt controls if Serf skips a graceful leave when
-	// the INT signal. Defaults false. This can be changed on reload.
+	// receiving the INT signal. Defaults false on clients, true on
-	SkipLeaveOnInt bool `mapstructure:"skip_leave_on_interrupt"`
+	// 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 {
+	if b.SkipLeaveOnInt != nil {
-		result.SkipLeaveOnInt = true
+		result.SkipLeaveOnInt = b.SkipLeaveOnInt
 	}
 	if b.Telemetry.DisableHostname == true {
 		result.Telemetry.DisableHostname = true

command/agent/config_test.go

@@ -74,8 +74,8 @@ func TestDecodeConfig(t *testing.T) {
 		t.Fatalf("bad: %#v", config)
 	}
 
-	if config.SkipLeaveOnInt != DefaultConfig().SkipLeaveOnInt {
+	if config.SkipLeaveOnInt != nil {
-		t.Fatalf("bad: %#v", config)
+		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)
 

website/source/docs/agent/options.html.markdown

@@ -621,11 +621,18 @@ 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.
 
-* <a name="skip_leave_on_interrupt"></a><a href="#skip_leave_on_interrupt">`skip_leave_on_interrupt`</a>
+* <a name="skip_leave_on_interrupt"></a><a
-  This is similar to [`leave_on_terminate`](#leave_on_terminate) but
+  href="#skip_leave_on_interrupt">`skip_leave_on_interrupt`</a> This is
-  only affects interrupt handling. By default, an interrupt (such as hitting
+  similar to [`leave_on_terminate`](#leave_on_terminate) but only affects
-  Control-C in a shell) causes Consul to gracefully leave. Setting this to true
+  interrupt handling.  When Consul receives an interrupt signal (such as
-  disables that. Defaults to false.
+  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 (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).
 
 * <a name="start_join"></a><a href="#start_join">`start_join`</a> An array of strings specifying addresses
   of nodes to [`-join`](#_join) upon startup.