Skip to content

Make some Raft parameters tunable by environment variables#2566

Open
chungers wants to merge 1 commit into
moby:masterfrom
chungers:tuning-params
Open

Make some Raft parameters tunable by environment variables#2566
chungers wants to merge 1 commit into
moby:masterfrom
chungers:tuning-params

Conversation

@chungers
Copy link
Copy Markdown
Contributor

This PR adds on a previous PR #2564 and make some of the Raft parameter changeable via environment variables. If the environment variables are not specified, then the default values
established in that PR are used instead. This will make it possible to tune the leader election
behavior without recompiling binaries, because these parameters don't seem to be configurable
once swarmkit is vendored into another program.

Signed-off-by: David Chung david.chung@docker.com

@dperny
Copy link
Copy Markdown
Collaborator

dperny commented Mar 22, 2018

Having been bitten pretty hard by environment variables in testkit, I'm inclined to suggest we move checking these environment variables to the highest level in the code feasible. Probably the node package. But maybe not. WDYT?

Comment thread manager/state/raft/raft.go Outdated
// EnvRaftHeartbeatTick is the env to set to customize the Raft heartbeat tick
EnvRaftHeartbeatTick = "SWARMKIT_RAFT_HEARTBEAT_TICK"

// EnvElectionTick is the env to set to customize the Raft leader election delay
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Circleci reports that this comment is missing a Raft in EnvRaftElectionTick and is failing because of it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. will fix this.

@stevvooe
Copy link
Copy Markdown
Contributor

If we are going this far, I'd really like to see this happen in the cluster configuration, so we can be certain the whole cluster is using consistent values.

Let me know if you need more direction on that.

@anshulpundir
Copy link
Copy Markdown
Contributor

anshulpundir commented Mar 22, 2018

Also, what is the motivation behind making these tunable ? Just want to make sure everyone's on the same page.

@dperny
Copy link
Copy Markdown
Collaborator

dperny commented Mar 22, 2018

the motivation makes sense to me, which is that it would let us tweak raft parameters on a live cluster without rebuilds. i don't think it's worth exposing to end-users but could be useful for lots of kinds of debugging and testing.

@chungers
Copy link
Copy Markdown
Contributor Author

I think making these parameters tunable is necessary to make things more serviceable. If we had this we could have had customers / field make changes to tune the system without waiting for a new release. Also, as part of performance engineering, we need to come up with proper ranges of parameters for known/controlled environments and those can be incorporated in the future as defaults or validation rules.

I am ok with exposing these at the level that makes sense -- sorry I am not familiar with how this is vendored/used in the engine so please let me know of a place that makes the most sense. Maybe swarmkit-specific environment variables are actually the way to go without exposing this to the end user.

@anshulpundir
Copy link
Copy Markdown
Contributor

anshulpundir commented Mar 22, 2018

Here's my take on this: I am not yet convinced that these two params need to be tunable in the field. Currently, we don't fully understand how different values of these params effect the system resilience to network partitions an degree of packet loss. I don't think doing this out in the field is a good way to test out the effects of these params.

Also, as part of performance engineering, we need to come up with proper ranges of parameters for known/controlled environments

I agree and my suggestion is to do that before we make this change.

@stevvooe
Copy link
Copy Markdown
Contributor

LGTM

After looking at this, there is already the ability to tune these parameters in several quorum systems (https://coreos.com/etcd/docs/latest/tuning.html is one example; thanks, @cyli ). These will provide a much needed tool for tuning the resilience for varying environments.

@chungers
Copy link
Copy Markdown
Contributor Author

chungers commented Mar 22, 2018

Here's docs from etcd on tuning:

Adjusting these values is a trade off. The value of heartbeat interval is recommended to be around the maximum of average round-trip time (RTT) between members, normally around 0.5-1.5x the round-trip time. If heartbeat interval is too low, etcd will send unnecessary messages that increase the usage of CPU and network resources. On the other side, a too high heartbeat interval leads to high election timeout. Higher election timeout takes longer time to detect a leader failure. The easiest way to measure round-trip time (RTT) is to use PING utility.

The election timeout should be set based on the heartbeat interval and average round-trip time between members. Election timeouts must be at least 10 times the round-trip time so it can account for variance in the network. For example, if the round-trip time between members is 10ms then the election timeout should be at least 100ms.

The upper limit of election timeout is 50000ms (50s), which should only be used when deploying a globally-distributed etcd cluster. A reasonable round-trip time for the continental United States is 130ms, and the time between US and Japan is around 350-400ms. If the network has uneven performance or regular packet delays/loss then it is possible that a couple of retries may be necessary to successfully send a packet. So 5s is a safe upper limit of global round-trip time. As the election timeout should be an order of magnitude bigger than broadcast time, in the case of ~5s for a globally distributed cluster, then 50 seconds becomes a reasonable maximum.

So these are parameters that imho cannot be set to be fixed at the factory. I agree that we shouldn't make the users make guesses and we absolutely need to characterize the system in controlled environments to arrive at baseline recommendations; however, to make the system serviceable by people in the field (support engineers or technical qualified personnel), some degree of tunability is required.

I had a chat with @stevvooe and perhaps making the environment variable names obvious that changing these could void the warranty (like calling it SWARMKIT_UNSUPPORTED_ELECTION_TICK or something) -- could be a reasonable middle ground. These parameters can always make their way into the upper layer config structs once we are convinced they are valuable as config parameters.

Thoughts?

@chungers
Copy link
Copy Markdown
Contributor Author

Updated to use SWARMKIT_X_RAFT_HEARTBEAT_TICK and SWARMKIT_X_RAFT_ELECTION_TICK as environment variables. If these are not set, then the defaults in #2564 will be used.

Signed-off-by: David Chung <david.chung@docker.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 23, 2018

Codecov Report

Merging #2566 into master will increase coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2566      +/-   ##
==========================================
+ Coverage    61.4%   61.49%   +0.08%     
==========================================
  Files         134       49      -85     
  Lines       21800     6329   -15471     
==========================================
- Hits        13387     3892    -9495     
+ Misses       6968     2062    -4906     
+ Partials     1445      375    -1070

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants