Gossip tuneables #4444

mkeeler · 2018-07-25T16:09:02Z

This PR exposes a number of the serf/memberlist/gossip tuneables. Previously all of these were being forced to default values. For users with extremely large clusters it would be nice to tune these to provide for quicker convergence and gossip message propagation (at the expense of increased network bandwidth).

The only changes here are to expose the configuration entries to the user. They end up just modifying being set on the agent's Config struct as Config.SerfConfig.MemberlistConfig..

The runtime tests were updated to test having these configurations in the hcl/json config and making sure they end up with the correct values in the RuntimeConfig. I then did a little manual testing to verify that the values get set properly in the agent's Config from the RuntimeConfig.

banks

Looks great. I had a couple of questions inline more for my understanding than because I think anything is wrong.

I have just a couple of minor suggestions on the docs which are worth changing.

banks · 2018-07-25T18:59:20Z

agent/config/default.go

+			gossip_interval = "100ms"
+			probe_interval = "100ms"
+			probe_timeout = "100ms"
+			suspicion_mult = 3


Are retransmit_mult and gossip_nodes not set here for a good reason? I suspect because we already use the default value for them in memberlist here but is it worth hard coding it for clarity?

See my other comment. We don't want to do that here. Only items specifically overridden for dev mode go here.

banks · 2018-07-25T19:01:19Z

agent/config/default.go

@@ -26,6 +26,10 @@ func DefaultRPCProtocol() (int, error) {
 // todo(fs): IMO, this should be the definitive default for all configurable values
 // todo(fs): and whatever is in here should clobber every default value. Hence, no sourcing.
 func DefaultSource() Source {
+	cfg := consul.DefaultConfig()
+	serfLAN := cfg.SerfLANConfig.MemberlistConfig
+	serfWAN := cfg.SerfWANConfig.MemberlistConfig


What's the significance of these moving from DefaultConsulSource to here? This seems like a better place but I don't recall offhand the details of how these two methods interact...

In depth explanation in my other comment.

DefaultConsulSource = non user modifiable
DefaultSource = user modifiable

banks · 2018-07-25T19:02:12Z

agent/config/runtime.go

+	// hcl: gossip_lan { gossip_interval = duration}
+	GossipLANGossipInterval time.Duration
+
+	// GossipfLANGossipNodes is the number of random nodes to send gossip messages to


typo: extra f in the variable name.

banks · 2018-07-25T19:02:51Z

agent/config/runtime.go

+	//
+	// The default is: 3
+	//
+	// hcl: gossip_lan { gossip_nodes = int}


nit: space after int

banks · 2018-07-25T19:04:42Z

agent/config/runtime.go

+
+	// GossipLANRetransmitMult is the multiplier for the number of retransmissions
+	// that are attempted for messages broadcasted over gossip. This
+	// configuration only applies to WAN gossip communications. The actual


LAN variable but you said "only applies to WAN"

banks · 2018-07-25T19:04:55Z

agent/config/runtime.go

+	// hcl: gossip_wan { gossip_interval = duration}
+	GossipWANGossipInterval time.Duration
+
+	// GossipfWANGossipNodes is the number of random nodes to send gossip messages to


Extra f again

banks · 2018-07-25T19:05:57Z

agent/config/runtime_test.go

+				rt.GossipWANGossipInterval = 100 * time.Millisecond
+				rt.GossipWANProbeInterval = 100 * time.Millisecond
+				rt.GossipWANProbeTimeout = 100 * time.Millisecond
+				rt.GossipWANSuspicionMult = 3


Why don't we have tests for Gossip*Nodes and Gossip*RetransmitMult here again?

That test is just for -dev mode overrides. I guess we could add validation for some other values not being overridden but other tests already cover that.

banks · 2018-07-25T19:07:19Z

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

+* <a name="gossip_lan"></a><a href="#gossip_lan">`gossip_lan`</a> - (Advanced) This object contains a number of sub-keys
+  which can be set to tune the LAN gossip communications. These are only provided for users running especially large 
+  clusters that need fine tuning and are prepared to spend significant effort correctly tuning them for their
+  environment and workload.


This is great, maybe add a sentence: "The default values are most appropriate in almost all deployments." to hammer it home.

Ha I just repeated this even after that change. 😂

banks · 2018-07-25T19:07:51Z

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

+
+  * <a name="probe_timeout"></a><a href="#probe_timeout">`probe_timeout`</a> - The timeout to wait for an ack from
+    a probed node before assuming it is unhealthy. This should be set to 99-percentile of RTT (round-trip time) on
+     your network.


Should we specify the default? Also maybe the language is confusing "This should be set to " implies that users should fiddle with this when we don't really want to encourage that.

Maybe instead something like "This should be at least the 99-percentile of RTT in you network. The default is 500ms and is a conservative value suitable for almost all realistic deployments."

banks · 2018-07-25T19:10:45Z

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

+  * <a name="retransmit_mult"></a><a href="#retransmit_mult">`retransmit_mult`</a> - The multiplier for the number 
+    of retransmissions that are attempted for messages broadcasted over gossip. The number of retransmits is scaled
+    using this multiplier and the cluster size. The higher the multiplier, the more likely a failed broadcast is to
+    converge at the expense of increased bandwidth.


mkeeler · 2018-07-25T19:38:27Z

Some of this might require a more in depth explanation of how the config is loaded.

Our configuration build (agent/config/builder.go) has a concept of user configuration as well as injected configurations. When given config fragments and config files. Our base configuration fragments to inject are located in agent/config/default.go.

How the builder sets things up is:

DefaultSource
DevSource (only if -dev mode is enabled)
User Defined Sources
NonUserSource
DefaultConsulSource
DefaultEnterpriseSource
DefaultVersionSource
DevConsulSource (if -dev mode is enabled)

Why we have NonUserSource and DefaultConsulSource I am not entirely sure. They both just override anything configured by the user previously to force specific values.

DevSource just overrides default configuration that is setup in DefaultSource and should only contain those items that it specifically wants to override.

Previously the gossip tuneables were in the DefaultConsulSource and thus not user modifiable even though they were technically valid configuration entries. They also existed in DevConsulSource to override them to better values for a -dev environment.

This PR shuffles things around a bit. No longer are the tuneables under the Consul configuration but we also want them to be user modifiable so they were put into the DefaultSource. This sets up sane defaults. A few of them that were previously overridden in DevConsulSource were moved to DevSource to get the override but to still allow user modification.

pearkes

Docs look great here. Just a small note about reminding people why not to touch this stuff.

pearkes · 2018-07-26T00:52:17Z

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

@@ -883,6 +883,74 @@ Consul will not enable TLS for the HTTP API unless the `https` port has been ass
 * <a name="disable_keyring_file"></a><a href="#disable_keyring_file">`disable_keyring_file`</a> - Equivalent to the
  [`-disable-keyring-file` command-line flag](#_disable_keyring_file).

+* <a name="gossip_lan"></a><a href="#gossip_lan">`gossip_lan`</a> - (Advanced) This object contains a number of sub-keys


I'd add:

Tuning these improperly can cause Consul to fail in unexpected ways.

Or something similarly strong language wise about potentially shooting yourself in the foot.

And maybe even bold that line 😄

mkeeler · 2018-07-26T15:37:42Z

Docs update and bolded where appropriate.

pearkes added this to the 1.2.2 milestone Jul 25, 2018

mkeeler requested review from banks and pearkes July 25, 2018 18:46

banks requested changes Jul 25, 2018

View reviewed changes

pearkes reviewed Jul 26, 2018

View reviewed changes

banks approved these changes Jul 26, 2018

View reviewed changes

mkeeler added 4 commits July 26, 2018 11:36

Expose a few gossip tuneables

319984b

Add gossip tuneables documentation

bcd0adb

PR Updates

f092c34

Make the language regarding advanced settings stronger in the docs

046ec10

mkeeler force-pushed the serf-tuneables branch from 007cf14 to 046ec10 Compare July 26, 2018 15:37

mkeeler merged commit 0e02277 into master Jul 26, 2018

mkeeler deleted the serf-tuneables branch September 6, 2018 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gossip tuneables #4444

Gossip tuneables #4444

mkeeler commented Jul 25, 2018 •

edited

Loading

banks left a comment

banks Jul 25, 2018

mkeeler Jul 25, 2018

banks Jul 25, 2018

mkeeler Jul 25, 2018

banks Jul 25, 2018

banks Jul 25, 2018

banks Jul 25, 2018

banks Jul 25, 2018

banks Jul 25, 2018

mkeeler Jul 25, 2018 •

edited

Loading

banks Jul 25, 2018

pearkes Jul 26, 2018

banks Jul 25, 2018

banks Jul 25, 2018

mkeeler commented Jul 25, 2018

pearkes left a comment

pearkes Jul 26, 2018

pearkes Jul 26, 2018

mkeeler commented Jul 26, 2018

Gossip tuneables #4444

Gossip tuneables #4444

Conversation

mkeeler commented Jul 25, 2018 • edited Loading

banks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkeeler Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkeeler commented Jul 25, 2018

pearkes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkeeler commented Jul 26, 2018

mkeeler commented Jul 25, 2018 •

edited

Loading

mkeeler Jul 25, 2018 •

edited

Loading