Merge pull request #2320 from hashicorp/f-leave

Changes default for `leave_on_terminate` based on server or client mode.

command/agent/command.go

@@ -179,15 +179,13 @@ func (c *Command) readConfig() *Config {
 		return nil
 	}
 
-	// Make sure SkipLeaveOnInt is set to the right default based on the
+	// Make sure LeaveOnTerm and SkipLeaveOnInt are set to the right
-	// agent's mode (client or server)
+	// defaults based on the agent's mode (client or server).
+	if config.LeaveOnTerm == nil {
+		config.LeaveOnTerm = Bool(!config.Server)
+	}
 	if config.SkipLeaveOnInt == nil {
-		config.SkipLeaveOnInt = new(bool)
+		config.SkipLeaveOnInt = Bool(config.Server)
-		if config.Server {
-			*config.SkipLeaveOnInt = true
-		} else {
-			*config.SkipLeaveOnInt = false
-		}
 	}
 
 	// Ensure we have a data directory
@@ -922,7 +920,7 @@ WAIT:
 	graceful := false
 	if sig == os.Interrupt && !(*config.SkipLeaveOnInt) {
 		graceful = true
-	} else if sig == syscall.SIGTERM && config.LeaveOnTerm {
+	} else if sig == syscall.SIGTERM && (*config.LeaveOnTerm) {
 		graceful = true
 	}
 

command/agent/command_test.go

@@ -137,7 +137,7 @@ func TestReadCliConfig(t *testing.T) {
 		}
 	}
 
-	// Test SkipLeaveOnInt default for server mode
+	// Test LeaveOnTerm and SkipLeaveOnInt defaults for server mode
 	{
 		ui := new(cli.MockUi)
 		cmd := &Command{
@@ -157,12 +157,15 @@ func TestReadCliConfig(t *testing.T) {
 		if config.Server != true {
 			t.Errorf(`Expected -server to be true`)
 		}
+		if (*config.LeaveOnTerm) != false {
+			t.Errorf(`Expected LeaveOnTerm to be false in server mode`)
+		}
 		if (*config.SkipLeaveOnInt) != true {
 			t.Errorf(`Expected SkipLeaveOnInt to be true in server mode`)
 		}
 	}
 
-	// Test SkipLeaveOnInt default for client mode
+	// Test LeaveOnTerm and SkipLeaveOnInt defaults for client mode
 	{
 		ui := new(cli.MockUi)
 		cmd := &Command{
@@ -181,6 +184,9 @@ func TestReadCliConfig(t *testing.T) {
 		if config.Server != false {
 			t.Errorf(`Expected server to be false`)
 		}
+		if (*config.LeaveOnTerm) != true {
+			t.Errorf(`Expected LeaveOnTerm to be true in client mode`)
+		}
 		if *config.SkipLeaveOnInt != false {
 			t.Errorf(`Expected SkipLeaveOnInt to be false in client mode`)
 		}

command/agent/config.go

@@ -303,8 +303,9 @@ type Config struct {
 	TaggedAddresses map[string]string
 
 	// LeaveOnTerm controls if Serf does a graceful leave when receiving
-	// the TERM signal. Defaults false. This can be changed on reload.
+	// the TERM signal. Defaults true on clients, false on servers. This can
-	LeaveOnTerm bool `mapstructure:"leave_on_terminate"`
+	// 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 on clients, true on
@@ -1170,8 +1171,8 @@ func MergeConfig(a, b *Config) *Config {
 	if b.Server == true {
 		result.Server = b.Server
 	}
-	if b.LeaveOnTerm == true {
+	if b.LeaveOnTerm != nil {
-		result.LeaveOnTerm = true
+		result.LeaveOnTerm = b.LeaveOnTerm
 	}
 	if b.SkipLeaveOnInt != nil {
 		result.SkipLeaveOnInt = b.SkipLeaveOnInt

command/agent/config_test.go

@@ -78,8 +78,8 @@ func TestDecodeConfig(t *testing.T) {
 		t.Fatalf("bad: expected nil SkipLeaveOnInt")
 	}
 
-	if config.LeaveOnTerm != DefaultConfig().LeaveOnTerm {
+	if config.LeaveOnTerm != nil {
-		t.Fatalf("bad: %#v", config)
+		t.Fatalf("bad: expected nil LeaveOnTerm")
 	}
 
 	// Server bootstrap
@@ -279,7 +279,7 @@ func TestDecodeConfig(t *testing.T) {
 		t.Fatalf("err: %s", err)
 	}
 
-	if config.LeaveOnTerm != true {
+	if *config.LeaveOnTerm != true {
 		t.Fatalf("bad: %#v", config)
 	}
 
@@ -1382,7 +1382,7 @@ func TestMergeConfig(t *testing.T) {
 		BindAddr:               "127.0.0.1",
 		AdvertiseAddr:          "127.0.0.1",
 		Server:                 false,
-		LeaveOnTerm:            false,
+		LeaveOnTerm:            new(bool),
 		SkipLeaveOnInt:         new(bool),
 		EnableDebug:            false,
 		CheckUpdateIntervalRaw: "8m",
@@ -1441,8 +1441,8 @@ func TestMergeConfig(t *testing.T) {
 			HTTPS: "127.0.0.4",
 		},
 		Server:                 true,
-		LeaveOnTerm:            true,
+		LeaveOnTerm:            Bool(true),
-		SkipLeaveOnInt:         new(bool),
+		SkipLeaveOnInt:         Bool(true),
 		EnableDebug:            true,
 		VerifyIncoming:         true,
 		VerifyOutgoing:         true,
@@ -1521,7 +1521,6 @@ func TestMergeConfig(t *testing.T) {
 		},
 		Reap: Bool(true),
 	}
-	*b.SkipLeaveOnInt = true
 
 	c := MergeConfig(a, b)
 

consul/server_test.go

@@ -7,18 +7,17 @@ import (
 	"net"
 	"os"
 	"strings"
+	"sync/atomic"
 	"testing"
 	"time"
 
 	"github.com/hashicorp/consul/testutil"
 )
 
-var nextPort = 15000
+var nextPort int32 = 15000
 
 func getPort() int {
-	p := nextPort
+	return int(atomic.AddInt32(&nextPort, 1))
-	nextPort++
-	return p
 }
 
 func tmpDir(t *testing.T) string {

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

@@ -567,9 +567,11 @@ Consul will not enable TLS for the HTTP API unless the `https` port has been ass
     ```
 
 * <a name="leave_on_terminate"></a><a href="#leave_on_terminate">`leave_on_terminate`</a> If
-  enabled, when the agent receives a TERM signal,
+  enabled, when the agent receives a TERM signal, it will send a `Leave` message to the rest
-  it will send a `Leave` message to the rest of the cluster and gracefully
+  of the cluster and gracefully leave. The default behavior for this feature varies based on
-  leave. Defaults to false.
+  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 `true`
+  and for agents in server-mode, this defaults to `false`.
 
 * <a name="log_level"></a><a href="#log_level">`log_level`</a> Equivalent to the
   [`-log-level` command-line flag](#_log_level).
@@ -581,20 +583,21 @@ Consul will not enable TLS for the HTTP API unless the `https` port has been ass
   later, this is a nested object that allows tuning the performance of different subsystems in
   Consul. See the [Server Performance](/docs/guides/performance.html) guide for more details. The
   following parameters are available:
-    * <a name="raft_multiplier"></a><a href="#raft_multiplier">`raft_multiplier`</a> - An integer
+
-      multiplier used by Consul servers to scale key Raft timing parameters. Omitting this value
+* <a name="raft_multiplier"></a><a href="#raft_multiplier">`raft_multiplier`</a> - An integer
-      or setting it to 0 uses default timing described below. Lower values are used to tighten
+  multiplier used by Consul servers to scale key Raft timing parameters. Omitting this value
-      timing and increase sensitivity while higher values relax timings and reduce sensitivity.
+  or setting it to 0 uses default timing described below. Lower values are used to tighten
-      Tuning this affects the time it takes Consul to detect leader failures and to perform
+  timing and increase sensitivity while higher values relax timings and reduce sensitivity.
-      leader elections, at the expense of requiring more network and CPU resources for better
+  Tuning this affects the time it takes Consul to detect leader failures and to perform
-      performance.<br><br>By default, Consul will use a lower-performance timing that's suitable
+  leader elections, at the expense of requiring more network and CPU resources for better
-      for [minimal Consul servers](/docs/guides/performance.html#minumum), currently equivalent
+  performance.<br><br>By default, Consul will use a lower-performance timing that's suitable
-      to setting this to a value of 5 (this default may be changed in future versions of Consul,
+  for [minimal Consul servers](/docs/guides/performance.html#minumum), currently equivalent
-      depending if the target minimum server profile changes). Setting this to a value of 1 will
+  to setting this to a value of 5 (this default may be changed in future versions of Consul,
-      configure Raft to its highest-performance mode, equivalent to the default timing of Consul
+  depending if the target minimum server profile changes). Setting this to a value of 1 will
-      prior to 0.7, and is recommended for [production Consul servers](/docs/guides/performance.html#production).
+  configure Raft to its highest-performance mode, equivalent to the default timing of Consul
-      See the note on [last contact](/docs/guides/performance.html#last-contact) timing for more
+  prior to 0.7, and is recommended for [production Consul servers](/docs/guides/performance.html#production).
-      details on tuning this parameter. The maximum allowed value is 10.
+  See the note on [last contact](/docs/guides/performance.html#last-contact) timing for more
+  details on tuning this parameter. The maximum allowed value is 10.
 
 * <a name="ports"></a><a href="#ports">`ports`</a> This is a nested object that allows setting
   the bind ports for the following keys:

website/source/docs/upgrade-specific.html.markdown

@@ -19,7 +19,7 @@ standard upgrade flow.
 Consul version 0.7 is a very large release with many important changes. Changes
 to be aware of during an upgrade are categorized below.
 
-#### Defaults Changed for Better Performance
+#### Performance Timing Defaults and Tuning
 
 Consul 0.7 now defaults the DNS configuration to allow for stale queries by defaulting
 [`allow_stale`](/docs/agent/options.html#allow_stale) to true for better utilization
@@ -53,11 +53,22 @@ to all Consul servers when upgrading:
 
 See the [Server Performance](/docs/guides/performance.html) guide for more details.
 
-#### Servers No Longer Default to Leave on Interrupt
+#### Leave-Related Configuration Defaults
 
-The default behavior of [`skip_leave_on_interrupt`](/docs/agent/options.html#skip_leave_on_interrupt)
+The default behavior of [`leave_on_terminate`](/docs/agent/options.html#leave_on_terminate)
-is now dependent on whether or not the agent is acting as a server or client. When Consul is started as a
+and [`skip_leave_on_interrupt`](/docs/agent/options.html#skip_leave_on_interrupt)
-server the default is `true` and `false` when a client.
+are now dependent on whether or not the agent is acting as a server or client:
+
+* For servers, `leave_on_terminate` defaults to "false" and `skip_leave_on_interrupt`
+defaults to "true".
+
+* For clients, `leave_on_terminate` defaults to "true" and `skip_leave_on_interrupt`
+defaults to "false".
+
+These defaults are designed to be safer for servers so that you must explicitly
+configure them to leave the cluster. This also results in a better experience for
+clients, especially in cloud environments where they may be created and destroyed
+often and users prefer not to wait for the 72 hour reap time for cleanup.
 
 #### Dropped Support for Protocol Version 1
 
@@ -69,7 +80,7 @@ to upgrade all agents to a newer version of Consul before upgrading to Consul
 #### Prepared Query Changes
 
 Consul version 0.7 adds a feature which allows prepared queries to store a
-["Near" parameter](/docs/agent/http/query.html#near) in the query definition
+[`Near` parameter](/docs/agent/http/query.html#near) in the query definition
 itself. This feature enables using the distance sorting features of prepared
 queries without explicitly providing the node to sort near in requests, but
 requires the agent servicing a request to send additional information about
@@ -88,19 +99,19 @@ Consul version 0.7 added support for translating WAN addresses in certain
 and the agents need to be running version 0.7 or later in order to use this
 feature.
 
-These translated addresses could break clients that are expecting local
+These translated addresses could break HTTP endpoint consumers that are
-addresses. A new [`X-Consul-Translate-Addresses`](/docs/agent/http.html#translate_header)
+expecting local addresses, so a new [`X-Consul-Translate-Addresses`](/docs/agent/http.html#translate_header)
 header was added to allow clients to detect if translation is enabled for HTTP
-responses, and a "lan" tag was added to `TaggedAddresses` for clients that need
+responses. A "lan" tag was added to `TaggedAddresses` for clients that need
 the local address regardless of translation.
 
-#### Changes to Outage Recovery and `peers.json`
+#### Outage Recovery and `peers.json` Changes
 
 The `peers.json` file is no longer present by default and is only used when
 performing recovery. This file will be deleted after Consul starts and ingests
-this file. Consul 0.7 also uses a new, automatically-created raft/peers.info file
+the file. Consul 0.7 also uses a new, automatically-created raft/peers.info file
-to avoid ingesting the `peers.json` file on the first start after upgrading (it
+to avoid ingesting the `peers.json` file on the first start after upgrading (the
-is simply deleted on the first start after upgrading).
+`peers.json` file is simply deleted on the first start after upgrading).
 
 Please be sure to review the [Outage Recovery Guide](/docs/guides/outage.html)
 before upgrading for more details.