From 146c5b0a597f4f2482815f3978a99c11b5528b4f Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Thu, 18 Feb 2016 15:17:42 -0800 Subject: [PATCH 01/69] Use `rand.Int31n()` to get power of two optimization In cases where i+1 is a power of two, skip one modulo operation. --- consul/structs/structs.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consul/structs/structs.go b/consul/structs/structs.go index 3e7ef5955e..d582560cd8 100644 --- a/consul/structs/structs.go +++ b/consul/structs/structs.go @@ -410,7 +410,7 @@ type CheckServiceNodes []CheckServiceNode // Shuffle does an in-place random shuffle using the Fisher-Yates algorithm. func (nodes CheckServiceNodes) Shuffle() { for i := len(nodes) - 1; i > 0; i-- { - j := rand.Int31() % int32(i+1) + j := rand.Int31n(int32(i + 1)) nodes[i], nodes[j] = nodes[j], nodes[i] } } From a92cda7bcd9ba83f337c0e852115f66977969482 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Thu, 18 Feb 2016 15:19:28 -0800 Subject: [PATCH 02/69] Fix whitespace alignment in a comment --- command/agent/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/agent/config.go b/command/agent/config.go index ba72ddeba1..6d4ebf2bd8 100644 --- a/command/agent/config.go +++ b/command/agent/config.go @@ -360,7 +360,7 @@ type Config struct { // * deny - Deny all requests // * extend-cache - Ignore the cache expiration, and allow cached // ACL's to be used to service requests. This - // is the default. If the ACL is not in the cache, + // is the default. If the ACL is not in the cache, // this acts like deny. ACLDownPolicy string `mapstructure:"acl_down_policy"` From cc86eb0a1a962838769071be92c7c35d0cc9e18c Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Thu, 18 Feb 2016 15:24:40 -0800 Subject: [PATCH 03/69] Rename c.consuls to c.consulServers Prep for breaking out maintenance of consuls into a new goroutine. --- consul/client.go | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/consul/client.go b/consul/client.go index b3fdb080e8..a037f1eed9 100644 --- a/consul/client.go +++ b/consul/client.go @@ -43,9 +43,9 @@ type Client struct { // Connection pool to consul servers connPool *ConnPool - // consuls tracks the locally known servers - consuls []*serverParts - consulLock sync.RWMutex + // consulServers tracks the locally known servers + consulServers []*serverParts + consulLock sync.RWMutex // eventCh is used to receive events from the // serf cluster in the datacenter @@ -254,9 +254,9 @@ func (c *Client) nodeJoin(me serf.MemberEvent) { // Check if this server is known found := false c.consulLock.Lock() - for idx, existing := range c.consuls { + for idx, existing := range c.consulServers { if existing.Name == parts.Name { - c.consuls[idx] = parts + c.consulServers[idx] = parts found = true break } @@ -264,7 +264,7 @@ func (c *Client) nodeJoin(me serf.MemberEvent) { // Add to the list if not known if !found { - c.consuls = append(c.consuls, parts) + c.consulServers = append(c.consulServers, parts) } c.consulLock.Unlock() @@ -286,11 +286,11 @@ func (c *Client) nodeFail(me serf.MemberEvent) { // Remove the server if known c.consulLock.Lock() - n := len(c.consuls) + n := len(c.consulServers) for i := 0; i < n; i++ { - if c.consuls[i].Name == parts.Name { - c.consuls[i], c.consuls[n-1] = c.consuls[n-1], nil - c.consuls = c.consuls[:n-1] + if c.consulServers[i].Name == parts.Name { + c.consulServers[i], c.consulServers[n-1] = c.consulServers[n-1], nil + c.consulServers = c.consulServers[:n-1] break } } @@ -339,13 +339,14 @@ func (c *Client) RPC(method string, args interface{}, reply interface{}) error { // Bail if we can't find any servers c.consulLock.RLock() - if len(c.consuls) == 0 { + numConsulServers = len(c.consulServers) + if numConsulServers == 0 { c.consulLock.RUnlock() return structs.ErrNoServers } // Select a random addr - server = c.consuls[rand.Int31()%int32(len(c.consuls))] + server = c.consulServers[rand.Int31n(int32(numConsulServers))] c.consulLock.RUnlock() // Forward to remote Consul @@ -371,7 +372,7 @@ func (c *Client) Stats() map[string]map[string]string { stats := map[string]map[string]string{ "consul": map[string]string{ "server": "false", - "known_servers": toString(uint64(len(c.consuls))), + "known_servers": toString(uint64(len(c.consulServers))), }, "serf_lan": c.serf.Stats(), "runtime": runtimeStats(), From f9aa968bf4455cd98c8bb0d64b9a28c57906db97 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Thu, 18 Feb 2016 15:38:21 -0800 Subject: [PATCH 04/69] Remove lastRPCTime This mechanism isn't going to provide much value in the future. Preemptively reduce the complexity of future work. --- consul/client.go | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/consul/client.go b/consul/client.go index a037f1eed9..1596f1e5f9 100644 --- a/consul/client.go +++ b/consul/client.go @@ -53,8 +53,7 @@ type Client struct { // lastServer is the last server we made an RPC call to, // this is used to re-use the last connection - lastServer *serverParts - lastRPCTime time.Time + lastServer *serverParts // Logger uses the provided LogOutput logger *log.Logger @@ -328,9 +327,22 @@ func (c *Client) localEvent(event serf.UserEvent) { // RPC is used to forward an RPC call to a consul server, or fail if no servers func (c *Client) RPC(method string, args interface{}, reply interface{}) error { - // Check the last rpc time + // Check to make sure we haven't spent too much time querying a + // single server + now := time.Now() + if !c.connRebalanceTime.IsZero() && now.After(c.connRebalanceTime) { + c.logger.Printf("[DEBUG] consul: connection time to server %s exceeded, rotating server connection", c.lastServer.Addr) + c.lastServer = nil + } + + // Allocate these vars on the stack before the goto + var numConsulServers int + var clusterWideRebalanceConnsPerSec float64 + var connReuseLowWaterMark time.Duration + var numLANMembers int + var server *serverParts - if time.Now().Sub(c.lastRPCTime) < clientRPCCache { + if c.lastServer != nil { server = c.lastServer if server != nil { goto TRY_RPC @@ -352,6 +364,7 @@ func (c *Client) RPC(method string, args interface{}, reply interface{}) error { // Forward to remote Consul TRY_RPC: if err := c.connPool.RPC(c.config.Datacenter, server.Addr, server.Version, method, args, reply); err != nil { + c.connRebalanceTime = time.Time{} c.lastServer = nil c.lastRPCTime = time.Time{} return err @@ -359,7 +372,6 @@ TRY_RPC: // Cache the last server c.lastServer = server - c.lastRPCTime = time.Now() return nil } From 54016f52767b8feee5ddd195cc85618c70fb53c9 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Thu, 18 Feb 2016 15:44:32 -0800 Subject: [PATCH 05/69] Commit miss re: consuls variable rename --- consul/client_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consul/client_test.go b/consul/client_test.go index 02a8db0bc8..1f41eba774 100644 --- a/consul/client_test.go +++ b/consul/client_test.go @@ -95,7 +95,7 @@ func TestClient_JoinLAN(t *testing.T) { // Check we have a new consul testutil.WaitForResult(func() (bool, error) { - return len(c1.consuls) == 1, nil + return len(c1.consulServers) == 1, nil }, func(err error) { t.Fatalf("expected consul server") }) From f6ffbf4e965155d7891448d4519bddfd9819fdb8 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Thu, 18 Feb 2016 17:46:02 -0800 Subject: [PATCH 06/69] Warn if serf events have queued up past 80% of the limit It is theoretically possible that the number of queued serf events can back up. If this happens, emit a warning message if there are more than 200 events in queue. Most notably, this can happen if `c.consulServerLock` is held for an "extended period of time". The probability of anyone ever seeing this log message is hopefully low to nonexistent, but if it happens, the warning message indicating a large number of serf events fired while a lock was held is likely to be helpful (vs serf mysteriously blocking when attempting to add an event to a channel). --- consul/client.go | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/consul/client.go b/consul/client.go index 1596f1e5f9..d6d76f3b28 100644 --- a/consul/client.go +++ b/consul/client.go @@ -24,6 +24,16 @@ const ( // clientMaxStreams controls how many idle streams we keep // open to a server clientMaxStreams = 32 + + // serfEventBacklog is the maximum number of unprocessed Serf Events + // that will be held in queue before new serf events block. A + // blocking serf event queue is a bad thing. + serfEventBacklog = 256 + + // serfEventBacklogWarning is the threshold at which point log + // warnings will be emitted indicating a problem when processing serf + // events. + serfEventBacklogWarning = 200 ) // Interface is used to provide either a Client or Server, @@ -102,8 +112,8 @@ func NewClient(config *Config) (*Client, error) { // Create server c := &Client{ config: config, - connPool: NewPool(config.LogOutput, clientRPCCache, clientMaxStreams, tlsWrap), - eventCh: make(chan serf.Event, 256), + connPool: NewPool(config.LogOutput, clientRPCConnMaxIdle, clientMaxStreams, tlsWrap), + eventCh: make(chan serf.Event, serfEventBacklog), logger: logger, shutdownCh: make(chan struct{}), } @@ -214,7 +224,13 @@ func (c *Client) Encrypted() bool { // lanEventHandler is used to handle events from the lan Serf cluster func (c *Client) lanEventHandler() { + var numQueuedEvents int for { + numQueuedEvents = len(c.eventCh) + if numQueuedEvents > serfEventBacklogWarning { + c.logger.Printf("[WARN] consul: number of queued serf events above warning threshold: %d/%d", numQueuedEvents, serfEventBacklogWarning) + } + select { case e := <-c.eventCh: switch e.EventType() { From fb0bfcc3cfd59c5d88b624cfd34f0580bd76a02b Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Thu, 18 Feb 2016 19:42:06 -0800 Subject: [PATCH 07/69] Introduce GOTEST_FLAGS to conditionally add -v to go test Trivial change that makes it possible for developers to set an environment variable and change the output of `go test` to be detailed (i.e. `GOTEST_FLAGS=-v`). --- scripts/test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/test.sh b/scripts/test.sh index b1ee7add71..a892469f09 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -12,4 +12,4 @@ go build -o $TEMPDIR/consul || exit 1 # Run the tests echo "--> Running tests" -go list ./... | grep -v ^github.com/hashicorp/consul/vendor/ | PATH=$TEMPDIR:$PATH xargs -n1 go test +go list ./... | grep -v ^github.com/hashicorp/consul/vendor/ | PATH=$TEMPDIR:$PATH xargs -n1 go test ${GOTEST_FLAGS:-} From 6af781d9d5399a71b5671b8fb23689093dda1e8b Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Thu, 18 Feb 2016 20:31:36 -0800 Subject: [PATCH 08/69] Rename `lastServer` to `preferredServer` Expanding the domain of lastServer beyond RPC() changes the meaning of this variable. Rename accordingly to match the intent coming in a subsequent commit: a background thread will be in charge of rotating preferredServer. --- consul/client.go | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/consul/client.go b/consul/client.go index d6d76f3b28..b23077034f 100644 --- a/consul/client.go +++ b/consul/client.go @@ -9,6 +9,7 @@ import ( "strconv" "strings" "sync" + "sync/atomic" "time" "github.com/hashicorp/consul/consul/structs" @@ -61,9 +62,9 @@ type Client struct { // serf cluster in the datacenter eventCh chan serf.Event - // lastServer is the last server we made an RPC call to, + // preferredServer is the last server we made an RPC call to, // this is used to re-use the last connection - lastServer *serverParts + preferredServer *serverParts // Logger uses the provided LogOutput logger *log.Logger @@ -347,8 +348,8 @@ func (c *Client) RPC(method string, args interface{}, reply interface{}) error { // single server now := time.Now() if !c.connRebalanceTime.IsZero() && now.After(c.connRebalanceTime) { - c.logger.Printf("[DEBUG] consul: connection time to server %s exceeded, rotating server connection", c.lastServer.Addr) - c.lastServer = nil + c.logger.Printf("[DEBUG] consul: connection time to server %s exceeded, rotating server connection", c.preferredServer.Addr) + c.preferredServer = nil } // Allocate these vars on the stack before the goto @@ -358,11 +359,9 @@ func (c *Client) RPC(method string, args interface{}, reply interface{}) error { var numLANMembers int var server *serverParts - if c.lastServer != nil { - server = c.lastServer - if server != nil { - goto TRY_RPC - } + if c.preferredServer != nil { + server = c.preferredServer + goto TRY_RPC } // Bail if we can't find any servers @@ -381,13 +380,12 @@ func (c *Client) RPC(method string, args interface{}, reply interface{}) error { TRY_RPC: if err := c.connPool.RPC(c.config.Datacenter, server.Addr, server.Version, method, args, reply); err != nil { c.connRebalanceTime = time.Time{} - c.lastServer = nil - c.lastRPCTime = time.Time{} + c.preferredServer = nil return err } - // Cache the last server - c.lastServer = server + // Cache the last server as our preferred server + _ = atomic.StorePointer(&c.preferredServer, server) return nil } From 7b308d8d7e7f4aa1d175af102703645b3603c423 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 19 Feb 2016 00:27:39 -0800 Subject: [PATCH 09/69] Add a flag to denote that a server is disabled A server is not normally disabled, but in the event of an RPC error, we want to mark a server as down to allow for fast failover to a different server. This value must be an int in order to support atomic operations. Additionally, this is the preliminary work required to bring up a server in a disabled state. RPC health checks in the future could mark the server as alive, thereby creating an organic "slow start" feature for Consul. --- consul/util.go | 17 ++++++++++++++--- consul/util_test.go | 13 ++++++++++++- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/consul/util.go b/consul/util.go index 8959ee66a2..d5b4573ba4 100644 --- a/consul/util.go +++ b/consul/util.go @@ -23,7 +23,7 @@ import ( */ var privateBlocks []*net.IPNet -// serverparts is used to return the parts of a server role +// serverParts is used to return the parts of a server role type serverParts struct { Name string Datacenter string @@ -32,6 +32,11 @@ type serverParts struct { Expect int Version int Addr net.Addr + + // Disabled is a uint64 in order to support atomic integer + // operations. Zero means enabled, non-zero is the number of times + // this server has failed without being marked healthy. + Disabled uint64 } func (s *serverParts) String() string { @@ -116,8 +121,8 @@ func CanServersUnderstandProtocol(members []serf.Member, version uint8) (bool, e return (numServers > 0) && (numWhoGrok == numServers), nil } -// Returns if a member is a consul server. Returns a bool, -// the datacenter, and the rpc port +// Returns true if a serf member is a consul server. Returns a bool and a +// pointer to the serverParts. func isConsulServer(m serf.Member) (bool, *serverParts) { if m.Tags["role"] != "consul" { return false, nil @@ -125,6 +130,11 @@ func isConsulServer(m serf.Member) (bool, *serverParts) { datacenter := m.Tags["dc"] _, bootstrap := m.Tags["bootstrap"] + var disabled uint64 = 0 + _, disabledStr := m.Tags["disabled"] + if disabledStr { + disabled = 1 + } expect := 0 expect_str, ok := m.Tags["expect"] @@ -158,6 +168,7 @@ func isConsulServer(m serf.Member) (bool, *serverParts) { Expect: expect, Addr: addr, Version: vsn, + Disabled: disabled, } return true, parts } diff --git a/consul/util_test.go b/consul/util_test.go index 1011ec896a..f2b868d33c 100644 --- a/consul/util_test.go +++ b/consul/util_test.go @@ -217,14 +217,24 @@ func TestIsConsulServer(t *testing.T) { if parts.Bootstrap { t.Fatalf("unexpected bootstrap") } + if parts.Disabled > 0 { + t.Fatalf("unexpected disabled") + } if parts.Expect != 0 { t.Fatalf("bad: %v", parts.Expect) } m.Tags["bootstrap"] = "1" + m.Tags["disabled"] = "1" valid, parts = isConsulServer(m) - if !valid || !parts.Bootstrap { + if !valid { + t.Fatalf("expected a valid consul server") + } + if !parts.Bootstrap { t.Fatalf("expected bootstrap") } + if parts.Disabled == 0 { + t.Fatalf("expected disabled") + } if parts.Addr.String() != "127.0.0.1:10000" { t.Fatalf("bad addr: %v", parts.Addr) } @@ -233,6 +243,7 @@ func TestIsConsulServer(t *testing.T) { } m.Tags["expect"] = "3" delete(m.Tags, "bootstrap") + delete(m.Tags, "disabled") valid, parts = isConsulServer(m) if !valid || parts.Expect != 3 { t.Fatalf("bad: %v", parts.Expect) From d4ca349e21a2ef38802728bed9b530753b7da0b7 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 19 Feb 2016 12:13:17 -0800 Subject: [PATCH 10/69] Refactor out the management of Consul servers Move the management of c.consulServers (fka c.consuls) into consul/server_manager.go. This commit brings in a background task that proactively manages the server list and: *) reshuffles the list *) manages the timer out of the RPC() path *) uses atomics to detect a server has failed This is a WIP, more work in testing needs to be completed. --- consul/client.go | 112 ++++++++------------- consul/client_test.go | 3 +- consul/server_manager.go | 212 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 257 insertions(+), 70 deletions(-) create mode 100644 consul/server_manager.go diff --git a/consul/client.go b/consul/client.go index b23077034f..edf8890dfc 100644 --- a/consul/client.go +++ b/consul/client.go @@ -3,7 +3,6 @@ package consul import ( "fmt" "log" - "math/rand" "os" "path/filepath" "strconv" @@ -54,18 +53,19 @@ type Client struct { // Connection pool to consul servers connPool *ConnPool - // consulServers tracks the locally known servers - consulServers []*serverParts - consulLock sync.RWMutex + // serverConfig provides the necessary load/store semantics to + // serverConfig + serverConfigValue atomic.Value + serverConfigMtx sync.Mutex + + // consulServersCh is used to receive events related to the + // maintenance of the list of consulServers + consulServersCh chan consulServerEventTypes // eventCh is used to receive events from the // serf cluster in the datacenter eventCh chan serf.Event - // preferredServer is the last server we made an RPC call to, - // this is used to re-use the last connection - preferredServer *serverParts - // Logger uses the provided LogOutput logger *log.Logger @@ -119,6 +119,13 @@ func NewClient(config *Config) (*Client, error) { shutdownCh: make(chan struct{}), } + // Create the initial serverConfig + serverCfg := serverConfig{} + c.serverConfigValue.Store(serverCfg) + + // Start consulServers maintenance + go c.consulServersManager() + // Start the Serf listeners to prevent a deadlock go c.lanEventHandler() @@ -266,23 +273,7 @@ func (c *Client) nodeJoin(me serf.MemberEvent) { continue } c.logger.Printf("[INFO] consul: adding server %s", parts) - - // Check if this server is known - found := false - c.consulLock.Lock() - for idx, existing := range c.consulServers { - if existing.Name == parts.Name { - c.consulServers[idx] = parts - found = true - break - } - } - - // Add to the list if not known - if !found { - c.consulServers = append(c.consulServers, parts) - } - c.consulLock.Unlock() + c.AddServer(parts) // Trigger the callback if c.config.ServerUp != nil { @@ -299,18 +290,7 @@ func (c *Client) nodeFail(me serf.MemberEvent) { continue } c.logger.Printf("[INFO] consul: removing server %s", parts) - - // Remove the server if known - c.consulLock.Lock() - n := len(c.consulServers) - for i := 0; i < n; i++ { - if c.consulServers[i].Name == parts.Name { - c.consulServers[i], c.consulServers[n-1] = c.consulServers[n-1], nil - c.consulServers = c.consulServers[:n-1] - break - } - } - c.consulLock.Unlock() + c.RemoveServer(parts) } } @@ -344,61 +324,55 @@ func (c *Client) localEvent(event serf.UserEvent) { // RPC is used to forward an RPC call to a consul server, or fail if no servers func (c *Client) RPC(method string, args interface{}, reply interface{}) error { - // Check to make sure we haven't spent too much time querying a - // single server - now := time.Now() - if !c.connRebalanceTime.IsZero() && now.After(c.connRebalanceTime) { - c.logger.Printf("[DEBUG] consul: connection time to server %s exceeded, rotating server connection", c.preferredServer.Addr) - c.preferredServer = nil + serverCfgPtr := c.serverConfigValue.Load() + if serverCfgPtr == nil { + c.logger.Printf("[ERR] consul: Failed to load a server config") + return structs.ErrNoServers } + serverCfg := serverCfgPtr.(serverConfig) - // Allocate these vars on the stack before the goto - var numConsulServers int - var clusterWideRebalanceConnsPerSec float64 - var connReuseLowWaterMark time.Duration - var numLANMembers int - - var server *serverParts - if c.preferredServer != nil { - server = c.preferredServer - goto TRY_RPC - } - - // Bail if we can't find any servers - c.consulLock.RLock() - numConsulServers = len(c.consulServers) - if numConsulServers == 0 { - c.consulLock.RUnlock() + numServers := len(serverCfg.servers) + if numServers == 0 { + c.logger.Printf("[ERR] consul: No servers found in the server config") return structs.ErrNoServers } - // Select a random addr - server = c.consulServers[rand.Int31n(int32(numConsulServers))] - c.consulLock.RUnlock() + // Find the first non-failing server in the server list. If this is + // not the first server a prior RPC call marked the first server as + // failed and we're waiting for the server management task to reorder + // a working server to the front of the list. + var server *serverParts + for i := range serverCfg.servers { + failCount := atomic.LoadUint64(&(serverCfg.servers[i].Disabled)) + if failCount == 0 { + server = serverCfg.servers[i] + break + } + } // Forward to remote Consul -TRY_RPC: if err := c.connPool.RPC(c.config.Datacenter, server.Addr, server.Version, method, args, reply); err != nil { - c.connRebalanceTime = time.Time{} - c.preferredServer = nil + atomic.AddUint64(&server.Disabled, 1) + c.logger.Printf("[ERR] consul: RPC failed to server %s: %v", server.Addr, err) + c.consulServersCh <- consulServersRPCError return err } - // Cache the last server as our preferred server - _ = atomic.StorePointer(&c.preferredServer, server) return nil } // Stats is used to return statistics for debugging and insight // for various sub-systems func (c *Client) Stats() map[string]map[string]string { + serverCfg := c.serverConfigValue.Load().(serverConfig) + toString := func(v uint64) string { return strconv.FormatUint(v, 10) } stats := map[string]map[string]string{ "consul": map[string]string{ "server": "false", - "known_servers": toString(uint64(len(c.consulServers))), + "known_servers": toString(uint64(len(serverCfg.servers))), }, "serf_lan": c.serf.Stats(), "runtime": runtimeStats(), diff --git a/consul/client_test.go b/consul/client_test.go index 1f41eba774..98852d91ac 100644 --- a/consul/client_test.go +++ b/consul/client_test.go @@ -95,7 +95,8 @@ func TestClient_JoinLAN(t *testing.T) { // Check we have a new consul testutil.WaitForResult(func() (bool, error) { - return len(c1.consulServers) == 1, nil + serverCfg := c1.serverConfigValue.Load().(serverConfig) + return len(serverCfg.servers) == 1, nil }, func(err error) { t.Fatalf("expected consul server") }) diff --git a/consul/server_manager.go b/consul/server_manager.go new file mode 100644 index 0000000000..35023b3464 --- /dev/null +++ b/consul/server_manager.go @@ -0,0 +1,212 @@ +package consul + +import ( + "math/rand" + "sync/atomic" + "time" + + "github.com/hashicorp/consul/lib" +) + +type consulServerEventTypes int + +const ( + // consulServersNodeJoin is used to notify of a new consulServer. + // The primary effect of this is a reshuffling of consulServers and + // finding a new preferredServer. + consulServersNodeJoin = iota + + // consulServersRebalance is used to signal we should rebalance our + // connection load across servers + consulServersRebalance + + // consulServersRPCError is used to signal when a server has either + // timed out or returned an error and we would like to have the + // server manager find a new preferredServer. + consulServersRPCError +) + +// serverCfg is the thread-safe configuration structure that is used to +// maintain the list of consul servers in Client. +// +// NOTE(sean@): We are explicitly relying on the fact that this is copied. +// Please keep this structure light. +type serverConfig struct { + // servers tracks the locally known servers + servers []*serverParts + + // Timer used to control rebalancing of servers + rebalanceTimer *time.Timer +} + +// consulServersManager is used to automatically shuffle and rebalance the +// list of consulServers. This maintenance happens either when a new server +// is added or when a duration has been exceed. +func (c *Client) consulServersManager() { + defaultTimeout := 5 * time.Second // FIXME(sean@): This is a bullshit value + var rebalanceTimer *time.Timer + func(c *Client) { + c.serverConfigMtx.Lock() + defer c.serverConfigMtx.Unlock() + + serverCfgPtr := c.serverConfigValue.Load() + if serverCfgPtr == nil { + panic("server config has not been initialized") + } + var serverCfg serverConfig + serverCfg = serverCfgPtr.(serverConfig) + rebalanceTimer = time.NewTimer(defaultTimeout) + serverCfg.rebalanceTimer = rebalanceTimer + }(c) + + for { + select { + case e := <-c.consulServersCh: + switch e { + case consulServersNodeJoin: + c.logger.Printf("[INFO] consul: new node joined cluster") + c.RebalanceServers() + case consulServersRebalance: + c.logger.Printf("[INFO] consul: rebalancing servers by request") + c.RebalanceServers() + case consulServersRPCError: + c.logger.Printf("[INFO] consul: need to find a new server to talk with") + c.CycleFailedServers() + // FIXME(sean@): wtb preemptive Status.Ping + // of servers, ideally parallel fan-out of N + // nodes, then settle on the first node which + // responds successfully. + // + // Is there a distinction between slow and + // offline? Do we run the Status.Ping with a + // fixed timeout (say 30s) that way we can + // alert administrators that they've set + // their RPC time too low even though the + // Ping did return successfully? + default: + c.logger.Printf("[WARN] consul: unhandled LAN Serf Event: %#v", e) + } + case <-rebalanceTimer.C: + c.logger.Printf("[INFO] consul: server rebalance timeout") + c.RebalanceServers() + + case <-c.shutdownCh: + return + } + } +} + +func (c *Client) AddServer(server *serverParts) { + c.serverConfigMtx.Lock() + defer c.serverConfigMtx.Unlock() + serverCfg := c.serverConfigValue.Load().(serverConfig) + + // Check if this server is known + found := false + for idx, existing := range serverCfg.servers { + if existing.Name == server.Name { + // Overwrite the existing server parts in order to + // possibly update metadata (i.e. server version) + serverCfg.servers[idx] = server + found = true + break + } + } + + // Add to the list if not known + if !found { + serverCfg.servers = append(serverCfg.servers, server) + + // Notify the server maintenance task of a new server + c.consulServersCh <- consulServersNodeJoin + } + + c.serverConfigValue.Store(serverCfg) + +} + +func (c *Client) CycleFailedServers() { + c.serverConfigMtx.Lock() + defer c.serverConfigMtx.Unlock() + serverCfg := c.serverConfigValue.Load().(serverConfig) + + for i := range serverCfg.servers { + failCount := atomic.LoadUint64(&(serverCfg.servers[i].Disabled)) + if failCount == 0 { + break + } else if failCount > 0 { + serverCfg.servers = serverCfg.cycleServer() + } + } + + serverCfg.resetRebalanceTimer(c) + c.serverConfigValue.Store(serverCfg) +} + +func (sc *serverConfig) cycleServer() (servers []*serverParts) { + numServers := len(servers) + if numServers < 2 { + // No action required for zero or one server situations + return servers + } + + var failedNode *serverParts + failedNode, servers = servers[0], servers[1:] + servers = append(servers, failedNode) + return servers +} + +func (c *Client) RebalanceServers() { + c.serverConfigMtx.Lock() + defer c.serverConfigMtx.Unlock() + serverCfg := c.serverConfigValue.Load().(serverConfig) + + // Shuffle the server list on server join. Servers are selected from + // the head of the list and are moved to the end of the list on + // failure. + for i := len(serverCfg.servers) - 1; i > 0; i-- { + j := rand.Int31n(int32(i + 1)) + serverCfg.servers[i], serverCfg.servers[j] = serverCfg.servers[j], serverCfg.servers[i] + } + + serverCfg.resetRebalanceTimer(c) + c.serverConfigValue.Store(serverCfg) +} + +func (c *Client) RemoveServer(server *serverParts) { + c.serverConfigMtx.Lock() + defer c.serverConfigMtx.Unlock() + serverCfg := c.serverConfigValue.Load().(serverConfig) + + // Remove the server if known + n := len(serverCfg.servers) + for i := 0; i < n; i++ { + if serverCfg.servers[i].Name == server.Name { + serverCfg.servers[i], serverCfg.servers[n-1] = serverCfg.servers[n-1], nil + serverCfg.servers = serverCfg.servers[:n-1] + break + } + } + + c.serverConfigValue.Store(serverCfg) + +} + +// resetRebalanceTimer assumes: +// +// 1) the serverConfigMtx is already held by the caller. +// 2) the caller will call serverConfigValue.Store() +func (sc *serverConfig) resetRebalanceTimer(c *Client) { + numConsulServers := len(sc.servers) + // Limit this connection's life based on the size (and health) of the + // cluster. Never rebalance a connection more frequently than + // connReuseLowWatermarkDuration, and make sure we never exceed + // clusterWideRebalanceConnsPerSec operations/s across numLANMembers. + clusterWideRebalanceConnsPerSec := float64(numConsulServers * newRebalanceConnsPerSecPerServer) + connReuseLowWatermarkDuration := clientRPCMinReuseDuration + lib.RandomStagger(clientRPCMinReuseDuration/clientRPCJitterFraction) + numLANMembers := len(c.LANMembers()) + connRebalanceTimeout := lib.RateScaledInterval(clusterWideRebalanceConnsPerSec, connReuseLowWatermarkDuration, numLANMembers) + c.logger.Printf("[DEBUG] consul: connection will be rebalanced in %v", connRebalanceTimeout) + + sc.rebalanceTimer.Reset(connRebalanceTimeout) +} From b1e392405caf944d9e49efb7e1aeb0743ecd2f8b Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 19 Feb 2016 13:17:52 -0800 Subject: [PATCH 11/69] Handle the case where there are no healthy servers Pointed out by: @slackpad --- consul/client.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/consul/client.go b/consul/client.go index edf8890dfc..36bfe7c0da 100644 --- a/consul/client.go +++ b/consul/client.go @@ -350,6 +350,11 @@ func (c *Client) RPC(method string, args interface{}, reply interface{}) error { } } + if server == nil { + c.logger.Printf("[ERR] consul: No healthy servers found in the server config") + return structs.ErrNoServers + } + // Forward to remote Consul if err := c.connPool.RPC(c.config.Datacenter, server.Addr, server.Version, method, args, reply); err != nil { atomic.AddUint64(&server.Disabled, 1) From 5be956c3102c59885788e5bdd3d534bfd0ee2362 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 19 Feb 2016 13:19:13 -0800 Subject: [PATCH 12/69] Rename serverConfigMtx to serverConfigLock Pointed out by: @slackpad --- consul/client.go | 2 +- consul/server_manager.go | 22 +++++++++++----------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/consul/client.go b/consul/client.go index 36bfe7c0da..a22dfb251e 100644 --- a/consul/client.go +++ b/consul/client.go @@ -56,7 +56,7 @@ type Client struct { // serverConfig provides the necessary load/store semantics to // serverConfig serverConfigValue atomic.Value - serverConfigMtx sync.Mutex + serverConfigLock sync.Mutex // consulServersCh is used to receive events related to the // maintenance of the list of consulServers diff --git a/consul/server_manager.go b/consul/server_manager.go index 35023b3464..51b1acf325 100644 --- a/consul/server_manager.go +++ b/consul/server_manager.go @@ -46,8 +46,8 @@ func (c *Client) consulServersManager() { defaultTimeout := 5 * time.Second // FIXME(sean@): This is a bullshit value var rebalanceTimer *time.Timer func(c *Client) { - c.serverConfigMtx.Lock() - defer c.serverConfigMtx.Unlock() + c.serverConfigLock.Lock() + defer c.serverConfigLock.Unlock() serverCfgPtr := c.serverConfigValue.Load() if serverCfgPtr == nil { @@ -97,8 +97,8 @@ func (c *Client) consulServersManager() { } func (c *Client) AddServer(server *serverParts) { - c.serverConfigMtx.Lock() - defer c.serverConfigMtx.Unlock() + c.serverConfigLock.Lock() + defer c.serverConfigLock.Unlock() serverCfg := c.serverConfigValue.Load().(serverConfig) // Check if this server is known @@ -126,8 +126,8 @@ func (c *Client) AddServer(server *serverParts) { } func (c *Client) CycleFailedServers() { - c.serverConfigMtx.Lock() - defer c.serverConfigMtx.Unlock() + c.serverConfigLock.Lock() + defer c.serverConfigLock.Unlock() serverCfg := c.serverConfigValue.Load().(serverConfig) for i := range serverCfg.servers { @@ -157,8 +157,8 @@ func (sc *serverConfig) cycleServer() (servers []*serverParts) { } func (c *Client) RebalanceServers() { - c.serverConfigMtx.Lock() - defer c.serverConfigMtx.Unlock() + c.serverConfigLock.Lock() + defer c.serverConfigLock.Unlock() serverCfg := c.serverConfigValue.Load().(serverConfig) // Shuffle the server list on server join. Servers are selected from @@ -174,8 +174,8 @@ func (c *Client) RebalanceServers() { } func (c *Client) RemoveServer(server *serverParts) { - c.serverConfigMtx.Lock() - defer c.serverConfigMtx.Unlock() + c.serverConfigLock.Lock() + defer c.serverConfigLock.Unlock() serverCfg := c.serverConfigValue.Load().(serverConfig) // Remove the server if known @@ -194,7 +194,7 @@ func (c *Client) RemoveServer(server *serverParts) { // resetRebalanceTimer assumes: // -// 1) the serverConfigMtx is already held by the caller. +// 1) the serverConfigLock is already held by the caller. // 2) the caller will call serverConfigValue.Store() func (sc *serverConfig) resetRebalanceTimer(c *Client) { numConsulServers := len(sc.servers) From 0925b262506eb55ea3d442ceff11d18ea1d8c8b2 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 19 Feb 2016 17:26:45 -0800 Subject: [PATCH 13/69] Refactor consul.serverParts into server_details.ServerDetails This may be short-lived, but it also seems like this is going to lead us down a path where ServerDetails is going to evolve into a more powerful package that will encapsulate more behavior behind a coherent API. --- consul/client.go | 5 +- consul/leader.go | 11 +-- consul/merge.go | 5 +- consul/serf.go | 11 +-- consul/server.go | 9 ++- consul/server_details/server_details.go | 81 ++++++++++++++++++++ consul/server_details/server_details_test.go | 63 +++++++++++++++ consul/util.go | 72 ----------------- consul/util_test.go | 54 ------------- 9 files changed, 167 insertions(+), 144 deletions(-) create mode 100644 consul/server_details/server_details.go create mode 100644 consul/server_details/server_details_test.go diff --git a/consul/client.go b/consul/client.go index a22dfb251e..2de27f0720 100644 --- a/consul/client.go +++ b/consul/client.go @@ -11,6 +11,7 @@ import ( "sync/atomic" "time" + "github.com/hashicorp/consul/consul/server_details" "github.com/hashicorp/consul/consul/structs" "github.com/hashicorp/serf/coordinate" "github.com/hashicorp/serf/serf" @@ -263,7 +264,7 @@ func (c *Client) lanEventHandler() { // nodeJoin is used to handle join events on the serf cluster func (c *Client) nodeJoin(me serf.MemberEvent) { for _, m := range me.Members { - ok, parts := isConsulServer(m) + ok, parts := server_details.IsConsulServer(m) if !ok { continue } @@ -285,7 +286,7 @@ func (c *Client) nodeJoin(me serf.MemberEvent) { // nodeFail is used to handle fail events on the serf cluster func (c *Client) nodeFail(me serf.MemberEvent) { for _, m := range me.Members { - ok, parts := isConsulServer(m) + ok, parts := server_details.IsConsulServer(m) if !ok { continue } diff --git a/consul/leader.go b/consul/leader.go index 55c487b4fd..3f4bc14844 100644 --- a/consul/leader.go +++ b/consul/leader.go @@ -8,6 +8,7 @@ import ( "time" "github.com/armon/go-metrics" + "github.com/hashicorp/consul/consul/server_details" "github.com/hashicorp/consul/consul/structs" "github.com/hashicorp/raft" "github.com/hashicorp/serf/serf" @@ -349,7 +350,7 @@ func (s *Server) shouldHandleMember(member serf.Member) bool { if valid, dc := isConsulNode(member); valid && dc == s.config.Datacenter { return true } - if valid, parts := isConsulServer(member); valid && parts.Datacenter == s.config.Datacenter { + if valid, parts := server_details.IsConsulServer(member); valid && parts.Datacenter == s.config.Datacenter { return true } return false @@ -360,7 +361,7 @@ func (s *Server) shouldHandleMember(member serf.Member) bool { func (s *Server) handleAliveMember(member serf.Member) error { // Register consul service if a server var service *structs.NodeService - if valid, parts := isConsulServer(member); valid { + if valid, parts := server_details.IsConsulServer(member); valid { service = &structs.NodeService{ ID: ConsulServiceID, Service: ConsulServiceName, @@ -496,7 +497,7 @@ func (s *Server) handleDeregisterMember(reason string, member serf.Member) error } // Remove from Raft peers if this was a server - if valid, parts := isConsulServer(member); valid { + if valid, parts := server_details.IsConsulServer(member); valid { if err := s.removeConsulServer(member, parts.Port); err != nil { return err } @@ -523,7 +524,7 @@ func (s *Server) handleDeregisterMember(reason string, member serf.Member) error } // joinConsulServer is used to try to join another consul server -func (s *Server) joinConsulServer(m serf.Member, parts *serverParts) error { +func (s *Server) joinConsulServer(m serf.Member, parts *server_details.ServerDetails) error { // Do not join ourself if m.Name == s.config.NodeName { return nil @@ -533,7 +534,7 @@ func (s *Server) joinConsulServer(m serf.Member, parts *serverParts) error { if parts.Bootstrap { members := s.serfLAN.Members() for _, member := range members { - valid, p := isConsulServer(member) + valid, p := server_details.IsConsulServer(member) if valid && member.Name != m.Name && p.Bootstrap { s.logger.Printf("[ERR] consul: '%v' and '%v' are both in bootstrap mode. Only one node should be in bootstrap mode, not adding Raft peer.", m.Name, member.Name) return nil diff --git a/consul/merge.go b/consul/merge.go index c61b79f418..ed86c69843 100644 --- a/consul/merge.go +++ b/consul/merge.go @@ -3,6 +3,7 @@ package consul import ( "fmt" + "github.com/hashicorp/consul/consul/server_details" "github.com/hashicorp/serf/serf" ) @@ -24,7 +25,7 @@ func (md *lanMergeDelegate) NotifyMerge(members []*serf.Member) error { continue } - ok, parts := isConsulServer(*m) + ok, parts := server_details.IsConsulServer(*m) if ok && parts.Datacenter != md.dc { return fmt.Errorf("Member '%s' part of wrong datacenter '%s'", m.Name, parts.Datacenter) @@ -41,7 +42,7 @@ type wanMergeDelegate struct { func (md *wanMergeDelegate) NotifyMerge(members []*serf.Member) error { for _, m := range members { - ok, _ := isConsulServer(*m) + ok, _ := server_details.IsConsulServer(*m) if !ok { return fmt.Errorf("Member '%s' is not a server", m.Name) } diff --git a/consul/serf.go b/consul/serf.go index 2a2cb6944e..d9f27309c0 100644 --- a/consul/serf.go +++ b/consul/serf.go @@ -4,6 +4,7 @@ import ( "net" "strings" + "github.com/hashicorp/consul/consul/server_details" "github.com/hashicorp/serf/serf" ) @@ -140,7 +141,7 @@ func (s *Server) localEvent(event serf.UserEvent) { // lanNodeJoin is used to handle join events on the LAN pool. func (s *Server) lanNodeJoin(me serf.MemberEvent) { for _, m := range me.Members { - ok, parts := isConsulServer(m) + ok, parts := server_details.IsConsulServer(m) if !ok { continue } @@ -163,7 +164,7 @@ func (s *Server) lanNodeJoin(me serf.MemberEvent) { // wanNodeJoin is used to handle join events on the WAN pool. func (s *Server) wanNodeJoin(me serf.MemberEvent) { for _, m := range me.Members { - ok, parts := isConsulServer(m) + ok, parts := server_details.IsConsulServer(m) if !ok { s.logger.Printf("[WARN] consul: non-server in WAN pool: %s", m.Name) continue @@ -209,7 +210,7 @@ func (s *Server) maybeBootstrap() { members := s.serfLAN.Members() addrs := make([]string, 0) for _, member := range members { - valid, p := isConsulServer(member) + valid, p := server_details.IsConsulServer(member) if !valid { continue } @@ -247,7 +248,7 @@ func (s *Server) maybeBootstrap() { // lanNodeFailed is used to handle fail events on the LAN pool. func (s *Server) lanNodeFailed(me serf.MemberEvent) { for _, m := range me.Members { - ok, parts := isConsulServer(m) + ok, parts := server_details.IsConsulServer(m) if !ok { continue } @@ -262,7 +263,7 @@ func (s *Server) lanNodeFailed(me serf.MemberEvent) { // wanNodeFailed is used to handle fail events on the WAN pool. func (s *Server) wanNodeFailed(me serf.MemberEvent) { for _, m := range me.Members { - ok, parts := isConsulServer(m) + ok, parts := server_details.IsConsulServer(m) if !ok { continue } diff --git a/consul/server.go b/consul/server.go index b9adc88337..347cc6f26a 100644 --- a/consul/server.go +++ b/consul/server.go @@ -15,6 +15,7 @@ import ( "time" "github.com/hashicorp/consul/acl" + "github.com/hashicorp/consul/consul/server_details" "github.com/hashicorp/consul/consul/state" "github.com/hashicorp/consul/tlsutil" "github.com/hashicorp/raft" @@ -97,7 +98,7 @@ type Server struct { // localConsuls is used to track the known consuls // in the local datacenter. Used to do leader forwarding. - localConsuls map[string]*serverParts + localConsuls map[string]*server_details.ServerDetails localLock sync.RWMutex // Logger uses the provided LogOutput @@ -119,7 +120,7 @@ type Server struct { // remoteConsuls is used to track the known consuls in // remote datacenters. Used to do DC forwarding. - remoteConsuls map[string][]*serverParts + remoteConsuls map[string][]*server_details.ServerDetails remoteLock sync.RWMutex // rpcListener is used to listen for incoming connections @@ -216,10 +217,10 @@ func NewServer(config *Config) (*Server, error) { connPool: NewPool(config.LogOutput, serverRPCCache, serverMaxStreams, tlsWrap), eventChLAN: make(chan serf.Event, 256), eventChWAN: make(chan serf.Event, 256), - localConsuls: make(map[string]*serverParts), + localConsuls: make(map[string]*server_details.ServerDetails), logger: logger, reconcileCh: make(chan serf.Member, 32), - remoteConsuls: make(map[string][]*serverParts), + remoteConsuls: make(map[string][]*server_details.ServerDetails), rpcServer: rpc.NewServer(), rpcTLS: incomingTLS, tombstoneGC: gc, diff --git a/consul/server_details/server_details.go b/consul/server_details/server_details.go new file mode 100644 index 0000000000..9b2addc189 --- /dev/null +++ b/consul/server_details/server_details.go @@ -0,0 +1,81 @@ +package server_details + +import ( + "fmt" + "net" + "strconv" + + "github.com/hashicorp/serf/serf" +) + +// ServerDetails is used to return details of a consul server +type ServerDetails struct { + Name string + Datacenter string + Port int + Bootstrap bool + Expect int + Version int + Addr net.Addr + + // Disabled is a uint64 in order to support atomic integer + // operations. Zero means enabled, non-zero is the number of times + // this server has failed without being marked healthy. + Disabled uint64 +} + +func (s *ServerDetails) String() string { + return fmt.Sprintf("%s (Addr: %s) (DC: %s)", s.Name, s.Addr, s.Datacenter) +} + +// IsConsulServer returns true if a serf member is a consul server. Returns a +// bool and a pointer to the ServerDetails. +func IsConsulServer(m serf.Member) (bool, *ServerDetails) { + if m.Tags["role"] != "consul" { + return false, nil + } + + datacenter := m.Tags["dc"] + _, bootstrap := m.Tags["bootstrap"] + var disabled uint64 = 0 + _, disabledStr := m.Tags["disabled"] + if disabledStr { + disabled = 1 + } + + expect := 0 + expect_str, ok := m.Tags["expect"] + var err error + if ok { + expect, err = strconv.Atoi(expect_str) + if err != nil { + return false, nil + } + } + + port_str := m.Tags["port"] + port, err := strconv.Atoi(port_str) + if err != nil { + return false, nil + } + + vsn_str := m.Tags["vsn"] + vsn, err := strconv.Atoi(vsn_str) + if err != nil { + return false, nil + } + + addr := &net.TCPAddr{IP: m.Addr, Port: port} + + parts := &ServerDetails{ + Name: m.Name, + Datacenter: datacenter, + Port: port, + Bootstrap: bootstrap, + Expect: expect, + Addr: addr, + Version: vsn, + Disabled: disabled, + } + return true, parts +} diff --git a/consul/server_details/server_details_test.go b/consul/server_details/server_details_test.go new file mode 100644 index 0000000000..ea3bef0365 --- /dev/null +++ b/consul/server_details/server_details_test.go @@ -0,0 +1,63 @@ +package server_details_test + +import ( + "net" + "testing" + + "github.com/hashicorp/consul/consul/server_details" + "github.com/hashicorp/serf/serf" +) + +func TestIsConsulServer(t *testing.T) { + m := serf.Member{ + Name: "foo", + Addr: net.IP([]byte{127, 0, 0, 1}), + Tags: map[string]string{ + "role": "consul", + "dc": "east-aws", + "port": "10000", + "vsn": "1", + }, + } + valid, parts := server_details.IsConsulServer(m) + if !valid || parts.Datacenter != "east-aws" || parts.Port != 10000 { + t.Fatalf("bad: %v %v", valid, parts) + } + if parts.Name != "foo" { + t.Fatalf("bad: %v", parts) + } + if parts.Bootstrap { + t.Fatalf("unexpected bootstrap") + } + if parts.Disabled > 0 { + t.Fatalf("unexpected disabled") + } + if parts.Expect != 0 { + t.Fatalf("bad: %v", parts.Expect) + } + m.Tags["bootstrap"] = "1" + m.Tags["disabled"] = "1" + valid, parts = server_details.IsConsulServer(m) + if !valid { + t.Fatalf("expected a valid consul server") + } + if !parts.Bootstrap { + t.Fatalf("expected bootstrap") + } + if parts.Disabled == 0 { + t.Fatalf("expected disabled") + } + if parts.Addr.String() != "127.0.0.1:10000" { + t.Fatalf("bad addr: %v", parts.Addr) + } + if parts.Version != 1 { + t.Fatalf("bad: %v", parts) + } + m.Tags["expect"] = "3" + delete(m.Tags, "bootstrap") + delete(m.Tags, "disabled") + valid, parts = server_details.IsConsulServer(m) + if !valid || parts.Expect != 3 { + t.Fatalf("bad: %v", parts.Expect) + } +} diff --git a/consul/util.go b/consul/util.go index d5b4573ba4..02dda3c116 100644 --- a/consul/util.go +++ b/consul/util.go @@ -23,26 +23,6 @@ import ( */ var privateBlocks []*net.IPNet -// serverParts is used to return the parts of a server role -type serverParts struct { - Name string - Datacenter string - Port int - Bootstrap bool - Expect int - Version int - Addr net.Addr - - // Disabled is a uint64 in order to support atomic integer - // operations. Zero means enabled, non-zero is the number of times - // this server has failed without being marked healthy. - Disabled uint64 -} - -func (s *serverParts) String() string { - return fmt.Sprintf("%s (Addr: %s) (DC: %s)", s.Name, s.Addr, s.Datacenter) -} - func init() { // Add each private block privateBlocks = make([]*net.IPNet, 6) @@ -121,58 +101,6 @@ func CanServersUnderstandProtocol(members []serf.Member, version uint8) (bool, e return (numServers > 0) && (numWhoGrok == numServers), nil } -// Returns true if a serf member is a consul server. Returns a bool and a -// pointer to the serverParts. -func isConsulServer(m serf.Member) (bool, *serverParts) { - if m.Tags["role"] != "consul" { - return false, nil - } - - datacenter := m.Tags["dc"] - _, bootstrap := m.Tags["bootstrap"] - var disabled uint64 = 0 - _, disabledStr := m.Tags["disabled"] - if disabledStr { - disabled = 1 - } - - expect := 0 - expect_str, ok := m.Tags["expect"] - var err error - if ok { - expect, err = strconv.Atoi(expect_str) - if err != nil { - return false, nil - } - } - - port_str := m.Tags["port"] - port, err := strconv.Atoi(port_str) - if err != nil { - return false, nil - } - - vsn_str := m.Tags["vsn"] - vsn, err := strconv.Atoi(vsn_str) - if err != nil { - return false, nil - } - - addr := &net.TCPAddr{IP: m.Addr, Port: port} - - parts := &serverParts{ - Name: m.Name, - Datacenter: datacenter, - Port: port, - Bootstrap: bootstrap, - Expect: expect, - Addr: addr, - Version: vsn, - Disabled: disabled, - } - return true, parts -} - // Returns if a member is a consul node. Returns a bool, // and the datacenter. func isConsulNode(m serf.Member) (bool, string) { diff --git a/consul/util_test.go b/consul/util_test.go index f2b868d33c..042a4f6779 100644 --- a/consul/util_test.go +++ b/consul/util_test.go @@ -196,60 +196,6 @@ func TestUtil_CanServersUnderstandProtocol(t *testing.T) { } } -func TestIsConsulServer(t *testing.T) { - m := serf.Member{ - Name: "foo", - Addr: net.IP([]byte{127, 0, 0, 1}), - Tags: map[string]string{ - "role": "consul", - "dc": "east-aws", - "port": "10000", - "vsn": "1", - }, - } - valid, parts := isConsulServer(m) - if !valid || parts.Datacenter != "east-aws" || parts.Port != 10000 { - t.Fatalf("bad: %v %v", valid, parts) - } - if parts.Name != "foo" { - t.Fatalf("bad: %v", parts) - } - if parts.Bootstrap { - t.Fatalf("unexpected bootstrap") - } - if parts.Disabled > 0 { - t.Fatalf("unexpected disabled") - } - if parts.Expect != 0 { - t.Fatalf("bad: %v", parts.Expect) - } - m.Tags["bootstrap"] = "1" - m.Tags["disabled"] = "1" - valid, parts = isConsulServer(m) - if !valid { - t.Fatalf("expected a valid consul server") - } - if !parts.Bootstrap { - t.Fatalf("expected bootstrap") - } - if parts.Disabled == 0 { - t.Fatalf("expected disabled") - } - if parts.Addr.String() != "127.0.0.1:10000" { - t.Fatalf("bad addr: %v", parts.Addr) - } - if parts.Version != 1 { - t.Fatalf("bad: %v", parts) - } - m.Tags["expect"] = "3" - delete(m.Tags, "bootstrap") - delete(m.Tags, "disabled") - valid, parts = isConsulServer(m) - if !valid || parts.Expect != 3 { - t.Fatalf("bad: %v", parts.Expect) - } -} - func TestIsConsulNode(t *testing.T) { m := serf.Member{ Tags: map[string]string{ From 2ca4cc58cee02454e623320d4071c05357d17f89 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 19 Feb 2016 17:32:16 -0800 Subject: [PATCH 14/69] Move consul.serverConfig out of the consul package Relocated to its own package, server_manager. This now greatly simplifies the RPC() call path and appropriately hides the locking behind the package boundary. More work is needed to be done here --- consul/client.go | 66 ++--- consul/client_test.go | 3 +- consul/server_manager.go | 212 ---------------- consul/server_manager/server_manager.go | 323 ++++++++++++++++++++++++ 4 files changed, 342 insertions(+), 262 deletions(-) delete mode 100644 consul/server_manager.go create mode 100644 consul/server_manager/server_manager.go diff --git a/consul/client.go b/consul/client.go index 2de27f0720..a5a4e5e11d 100644 --- a/consul/client.go +++ b/consul/client.go @@ -8,19 +8,23 @@ import ( "strconv" "strings" "sync" - "sync/atomic" "time" "github.com/hashicorp/consul/consul/server_details" + "github.com/hashicorp/consul/consul/server_manager" "github.com/hashicorp/consul/consul/structs" "github.com/hashicorp/serf/coordinate" "github.com/hashicorp/serf/serf" ) const ( - // clientRPCCache controls how long we keep an idle connection - // open to a server - clientRPCCache = 30 * time.Second + // clientRPCConnMaxIdle controls how long we keep an idle connection + // open to a server. 127s was chosen as the first prime above 120s + // (arbitrarily chose to use a prime) with the intent of reusing + // connections who are used by once-a-minute cron(8) jobs *and* who + // use a 60s jitter window (e.g. in vixie cron job execution can + // drift by up to 59s per job, or 119s for a once-a-minute cron job). + clientRPCConnMaxIdle = 127 * time.Second // clientMaxStreams controls how many idle streams we keep // open to a server @@ -54,14 +58,8 @@ type Client struct { // Connection pool to consul servers connPool *ConnPool - // serverConfig provides the necessary load/store semantics to - // serverConfig - serverConfigValue atomic.Value - serverConfigLock sync.Mutex - - // consulServersCh is used to receive events related to the - // maintenance of the list of consulServers - consulServersCh chan consulServerEventTypes + // serverManager + serverMgr *server_manager.ServerManager // eventCh is used to receive events from the // serf cluster in the datacenter @@ -120,12 +118,10 @@ func NewClient(config *Config) (*Client, error) { shutdownCh: make(chan struct{}), } - // Create the initial serverConfig - serverCfg := serverConfig{} - c.serverConfigValue.Store(serverCfg) + c.serverMgr = server_manager.NewServerManager(c.logger, c.shutdownCh) // Start consulServers maintenance - go c.consulServersManager() + go c.serverMgr.StartServerManager() // Start the Serf listeners to prevent a deadlock go c.lanEventHandler() @@ -274,7 +270,7 @@ func (c *Client) nodeJoin(me serf.MemberEvent) { continue } c.logger.Printf("[INFO] consul: adding server %s", parts) - c.AddServer(parts) + c.serverMgr.AddServer(parts) // Trigger the callback if c.config.ServerUp != nil { @@ -291,7 +287,7 @@ func (c *Client) nodeFail(me serf.MemberEvent) { continue } c.logger.Printf("[INFO] consul: removing server %s", parts) - c.RemoveServer(parts) + c.serverMgr.RemoveServer(parts) } } @@ -325,32 +321,7 @@ func (c *Client) localEvent(event serf.UserEvent) { // RPC is used to forward an RPC call to a consul server, or fail if no servers func (c *Client) RPC(method string, args interface{}, reply interface{}) error { - serverCfgPtr := c.serverConfigValue.Load() - if serverCfgPtr == nil { - c.logger.Printf("[ERR] consul: Failed to load a server config") - return structs.ErrNoServers - } - serverCfg := serverCfgPtr.(serverConfig) - - numServers := len(serverCfg.servers) - if numServers == 0 { - c.logger.Printf("[ERR] consul: No servers found in the server config") - return structs.ErrNoServers - } - - // Find the first non-failing server in the server list. If this is - // not the first server a prior RPC call marked the first server as - // failed and we're waiting for the server management task to reorder - // a working server to the front of the list. - var server *serverParts - for i := range serverCfg.servers { - failCount := atomic.LoadUint64(&(serverCfg.servers[i].Disabled)) - if failCount == 0 { - server = serverCfg.servers[i] - break - } - } - + server := c.serverMgr.FindHealthyServer() if server == nil { c.logger.Printf("[ERR] consul: No healthy servers found in the server config") return structs.ErrNoServers @@ -358,9 +329,8 @@ func (c *Client) RPC(method string, args interface{}, reply interface{}) error { // Forward to remote Consul if err := c.connPool.RPC(c.config.Datacenter, server.Addr, server.Version, method, args, reply); err != nil { - atomic.AddUint64(&server.Disabled, 1) + c.serverMgr.NotifyFailedServer(server) c.logger.Printf("[ERR] consul: RPC failed to server %s: %v", server.Addr, err) - c.consulServersCh <- consulServersRPCError return err } @@ -370,7 +340,7 @@ func (c *Client) RPC(method string, args interface{}, reply interface{}) error { // Stats is used to return statistics for debugging and insight // for various sub-systems func (c *Client) Stats() map[string]map[string]string { - serverCfg := c.serverConfigValue.Load().(serverConfig) + numServers := c.serverMgr.GetNumServers() toString := func(v uint64) string { return strconv.FormatUint(v, 10) @@ -378,7 +348,7 @@ func (c *Client) Stats() map[string]map[string]string { stats := map[string]map[string]string{ "consul": map[string]string{ "server": "false", - "known_servers": toString(uint64(len(serverCfg.servers))), + "known_servers": toString(uint64(numServers)), }, "serf_lan": c.serf.Stats(), "runtime": runtimeStats(), diff --git a/consul/client_test.go b/consul/client_test.go index 98852d91ac..ce45e559b2 100644 --- a/consul/client_test.go +++ b/consul/client_test.go @@ -95,8 +95,7 @@ func TestClient_JoinLAN(t *testing.T) { // Check we have a new consul testutil.WaitForResult(func() (bool, error) { - serverCfg := c1.serverConfigValue.Load().(serverConfig) - return len(serverCfg.servers) == 1, nil + return c1.serverMgr.GetNumServers() == 1, nil }, func(err error) { t.Fatalf("expected consul server") }) diff --git a/consul/server_manager.go b/consul/server_manager.go deleted file mode 100644 index 51b1acf325..0000000000 --- a/consul/server_manager.go +++ /dev/null @@ -1,212 +0,0 @@ -package consul - -import ( - "math/rand" - "sync/atomic" - "time" - - "github.com/hashicorp/consul/lib" -) - -type consulServerEventTypes int - -const ( - // consulServersNodeJoin is used to notify of a new consulServer. - // The primary effect of this is a reshuffling of consulServers and - // finding a new preferredServer. - consulServersNodeJoin = iota - - // consulServersRebalance is used to signal we should rebalance our - // connection load across servers - consulServersRebalance - - // consulServersRPCError is used to signal when a server has either - // timed out or returned an error and we would like to have the - // server manager find a new preferredServer. - consulServersRPCError -) - -// serverCfg is the thread-safe configuration structure that is used to -// maintain the list of consul servers in Client. -// -// NOTE(sean@): We are explicitly relying on the fact that this is copied. -// Please keep this structure light. -type serverConfig struct { - // servers tracks the locally known servers - servers []*serverParts - - // Timer used to control rebalancing of servers - rebalanceTimer *time.Timer -} - -// consulServersManager is used to automatically shuffle and rebalance the -// list of consulServers. This maintenance happens either when a new server -// is added or when a duration has been exceed. -func (c *Client) consulServersManager() { - defaultTimeout := 5 * time.Second // FIXME(sean@): This is a bullshit value - var rebalanceTimer *time.Timer - func(c *Client) { - c.serverConfigLock.Lock() - defer c.serverConfigLock.Unlock() - - serverCfgPtr := c.serverConfigValue.Load() - if serverCfgPtr == nil { - panic("server config has not been initialized") - } - var serverCfg serverConfig - serverCfg = serverCfgPtr.(serverConfig) - rebalanceTimer = time.NewTimer(defaultTimeout) - serverCfg.rebalanceTimer = rebalanceTimer - }(c) - - for { - select { - case e := <-c.consulServersCh: - switch e { - case consulServersNodeJoin: - c.logger.Printf("[INFO] consul: new node joined cluster") - c.RebalanceServers() - case consulServersRebalance: - c.logger.Printf("[INFO] consul: rebalancing servers by request") - c.RebalanceServers() - case consulServersRPCError: - c.logger.Printf("[INFO] consul: need to find a new server to talk with") - c.CycleFailedServers() - // FIXME(sean@): wtb preemptive Status.Ping - // of servers, ideally parallel fan-out of N - // nodes, then settle on the first node which - // responds successfully. - // - // Is there a distinction between slow and - // offline? Do we run the Status.Ping with a - // fixed timeout (say 30s) that way we can - // alert administrators that they've set - // their RPC time too low even though the - // Ping did return successfully? - default: - c.logger.Printf("[WARN] consul: unhandled LAN Serf Event: %#v", e) - } - case <-rebalanceTimer.C: - c.logger.Printf("[INFO] consul: server rebalance timeout") - c.RebalanceServers() - - case <-c.shutdownCh: - return - } - } -} - -func (c *Client) AddServer(server *serverParts) { - c.serverConfigLock.Lock() - defer c.serverConfigLock.Unlock() - serverCfg := c.serverConfigValue.Load().(serverConfig) - - // Check if this server is known - found := false - for idx, existing := range serverCfg.servers { - if existing.Name == server.Name { - // Overwrite the existing server parts in order to - // possibly update metadata (i.e. server version) - serverCfg.servers[idx] = server - found = true - break - } - } - - // Add to the list if not known - if !found { - serverCfg.servers = append(serverCfg.servers, server) - - // Notify the server maintenance task of a new server - c.consulServersCh <- consulServersNodeJoin - } - - c.serverConfigValue.Store(serverCfg) - -} - -func (c *Client) CycleFailedServers() { - c.serverConfigLock.Lock() - defer c.serverConfigLock.Unlock() - serverCfg := c.serverConfigValue.Load().(serverConfig) - - for i := range serverCfg.servers { - failCount := atomic.LoadUint64(&(serverCfg.servers[i].Disabled)) - if failCount == 0 { - break - } else if failCount > 0 { - serverCfg.servers = serverCfg.cycleServer() - } - } - - serverCfg.resetRebalanceTimer(c) - c.serverConfigValue.Store(serverCfg) -} - -func (sc *serverConfig) cycleServer() (servers []*serverParts) { - numServers := len(servers) - if numServers < 2 { - // No action required for zero or one server situations - return servers - } - - var failedNode *serverParts - failedNode, servers = servers[0], servers[1:] - servers = append(servers, failedNode) - return servers -} - -func (c *Client) RebalanceServers() { - c.serverConfigLock.Lock() - defer c.serverConfigLock.Unlock() - serverCfg := c.serverConfigValue.Load().(serverConfig) - - // Shuffle the server list on server join. Servers are selected from - // the head of the list and are moved to the end of the list on - // failure. - for i := len(serverCfg.servers) - 1; i > 0; i-- { - j := rand.Int31n(int32(i + 1)) - serverCfg.servers[i], serverCfg.servers[j] = serverCfg.servers[j], serverCfg.servers[i] - } - - serverCfg.resetRebalanceTimer(c) - c.serverConfigValue.Store(serverCfg) -} - -func (c *Client) RemoveServer(server *serverParts) { - c.serverConfigLock.Lock() - defer c.serverConfigLock.Unlock() - serverCfg := c.serverConfigValue.Load().(serverConfig) - - // Remove the server if known - n := len(serverCfg.servers) - for i := 0; i < n; i++ { - if serverCfg.servers[i].Name == server.Name { - serverCfg.servers[i], serverCfg.servers[n-1] = serverCfg.servers[n-1], nil - serverCfg.servers = serverCfg.servers[:n-1] - break - } - } - - c.serverConfigValue.Store(serverCfg) - -} - -// resetRebalanceTimer assumes: -// -// 1) the serverConfigLock is already held by the caller. -// 2) the caller will call serverConfigValue.Store() -func (sc *serverConfig) resetRebalanceTimer(c *Client) { - numConsulServers := len(sc.servers) - // Limit this connection's life based on the size (and health) of the - // cluster. Never rebalance a connection more frequently than - // connReuseLowWatermarkDuration, and make sure we never exceed - // clusterWideRebalanceConnsPerSec operations/s across numLANMembers. - clusterWideRebalanceConnsPerSec := float64(numConsulServers * newRebalanceConnsPerSecPerServer) - connReuseLowWatermarkDuration := clientRPCMinReuseDuration + lib.RandomStagger(clientRPCMinReuseDuration/clientRPCJitterFraction) - numLANMembers := len(c.LANMembers()) - connRebalanceTimeout := lib.RateScaledInterval(clusterWideRebalanceConnsPerSec, connReuseLowWatermarkDuration, numLANMembers) - c.logger.Printf("[DEBUG] consul: connection will be rebalanced in %v", connRebalanceTimeout) - - sc.rebalanceTimer.Reset(connRebalanceTimeout) -} diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go new file mode 100644 index 0000000000..3754e27599 --- /dev/null +++ b/consul/server_manager/server_manager.go @@ -0,0 +1,323 @@ +package server_manager + +import ( + "log" + "math/rand" + "sync" + "sync/atomic" + "time" + + "github.com/hashicorp/consul/consul/server_details" + "github.com/hashicorp/consul/lib" +) + +type consulServerEventTypes int + +const ( + // consulServersNodeJoin is used to notify of a new consulServer. + // The primary effect of this is a reshuffling of consulServers and + // finding a new preferredServer. + consulServersNodeJoin = iota + + // consulServersRebalance is used to signal we should rebalance our + // connection load across servers + consulServersRebalance + + // consulServersRPCError is used to signal when a server has either + // timed out or returned an error and we would like to have the + // server manager find a new preferredServer. + consulServersRPCError +) + +const ( + // clientRPCJitterFraction determines the amount of jitter added to + // clientRPCMinReuseDuration before a connection is expired and a new + // connection is established in order to rebalance load across consul + // servers. The cluster-wide number of connections per second from + // rebalancing is applied after this jitter to ensure the CPU impact + // is always finite. See newRebalanceConnsPerSecPerServer's comment + // for additional commentary. + // + // For example, in a 10K consul cluster with 5x servers, this default + // averages out to ~13 new connections from rebalancing per server + // per second (each connection is reused for 120s to 180s). + clientRPCJitterFraction = 2 + + // clientRPCMinReuseDuration controls the minimum amount of time RPC + // queries are sent over an established connection to a single server + clientRPCMinReuseDuration = 120 * time.Second + + // Limit the number of new connections a server receives per second + // for connection rebalancing. This limit caps the load caused by + // continual rebalancing efforts when a cluster is in equilibrium. A + // lower value comes at the cost of increased recovery time after a + // partition. This parameter begins to take effect when there are + // more than ~48K clients querying 5x servers or at lower server + // values when there is a partition. + // + // For example, in a 100K consul cluster with 5x servers, it will + // take ~5min for all servers to rebalance their connections. If + // 99,995 agents are in the minority talking to only one server, it + // will take ~26min for all servers to rebalance. A 10K cluster in + // the same scenario will take ~2.6min to rebalance. + newRebalanceConnsPerSecPerServer = 64 +) + +// serverCfg is the thread-safe configuration structure that is used to +// maintain the list of consul servers in Client. +// +// NOTE(sean@): We are explicitly relying on the fact that this is copied. +// Please keep this structure light. +type serverConfig struct { + // servers tracks the locally known servers + servers []*server_details.ServerDetails + + // Timer used to control rebalancing of servers + rebalanceTimer *time.Timer +} + +type ServerManager struct { + // serverConfig provides the necessary load/store semantics to + // serverConfig + serverConfigValue atomic.Value + serverConfigLock sync.Mutex + + // consulServersCh is used to receive events related to the + // maintenance of the list of consulServers + consulServersCh chan consulServerEventTypes + + // shutdownCh is a copy of the channel in consul.Client + shutdownCh chan struct{} + + // Logger uses the provided LogOutput + logger *log.Logger +} + +func (sm *ServerManager) AddServer(server *server_details.ServerDetails) { + sm.serverConfigLock.Lock() + defer sm.serverConfigLock.Unlock() + serverCfg := sm.serverConfigValue.Load().(serverConfig) + + // Check if this server is known + found := false + for idx, existing := range serverCfg.servers { + if existing.Name == server.Name { + // Overwrite the existing server parts in order to + // possibly update metadata (i.e. server version) + serverCfg.servers[idx] = server + found = true + break + } + } + + // Add to the list if not known + if !found { + newServers := make([]*server_details.ServerDetails, len(serverCfg.servers)+1) + copy(newServers, serverCfg.servers) + serverCfg.servers = newServers + + // Notify the server maintenance task of a new server + sm.consulServersCh <- consulServersNodeJoin + } + + sm.serverConfigValue.Store(serverCfg) +} + +func (sm *ServerManager) CycleFailedServers() { + sm.serverConfigLock.Lock() + defer sm.serverConfigLock.Unlock() + serverCfg := sm.serverConfigValue.Load().(serverConfig) + + for i := range serverCfg.servers { + failCount := atomic.LoadUint64(&(serverCfg.servers[i].Disabled)) + if failCount == 0 { + break + } else if failCount > 0 { + serverCfg.servers = serverCfg.cycleServer() + } + } + + serverCfg.resetRebalanceTimer(sm) + sm.serverConfigValue.Store(serverCfg) +} + +func (sc *serverConfig) cycleServer() (servers []*server_details.ServerDetails) { + numServers := len(servers) + if numServers < 2 { + // No action required for zero or one server situations + return servers + } + + newServers := make([]*server_details.ServerDetails, len(servers)+1) + var dequeuedServer *server_details.ServerDetails + dequeuedServer, newServers = servers[0], servers[1:] + servers = append(newServers, dequeuedServer) + return servers +} + +func (sm *ServerManager) FindHealthyServer() (server *server_details.ServerDetails) { + serverCfg := sm.getServerConfig() + numServers := len(serverCfg.servers) + if numServers == 0 { + sm.logger.Printf("[ERR] consul: No servers found in the server config") + return nil + } + + // Find the first non-failing server in the server list. If this is + // not the first server a prior RPC call marked the first server as + // failed and we're waiting for the server management task to reorder + // a working server to the front of the list. + for i := range serverCfg.servers { + failCount := atomic.LoadUint64(&(serverCfg.servers[i].Disabled)) + if failCount == 0 { + server = serverCfg.servers[i] + break + } + } + + return server +} + +func (sm *ServerManager) GetNumServers() (numServers int) { + serverCfg := sm.getServerConfig() + numServers = len(serverCfg.servers) + return numServers +} + +// getServerConfig shorthand method to hide the locking semantics of +// atomic.Value +func (sm *ServerManager) getServerConfig() serverConfig { + return sm.serverConfigValue.Load().(serverConfig) +} + +// NewServerManager is the only way to safely create a new ServerManager +// struct. +// +// NOTE(sean@): We don't simply pass in a consul.Client struct to avoid a +// cyclic import +func NewServerManager(logger *log.Logger, shutdownCh chan struct{}) (sm *ServerManager) { + sm = new(ServerManager) + // Create the initial serverConfig + serverCfg := serverConfig{} + sm.logger = logger + sm.shutdownCh = shutdownCh + sm.serverConfigValue.Store(serverCfg) + return sm +} + +func (sm *ServerManager) NotifyFailedServer(server *server_details.ServerDetails) { + atomic.AddUint64(&server.Disabled, 1) + sm.consulServersCh <- consulServersRPCError +} + +func (sm *ServerManager) RebalanceServers() { + sm.serverConfigLock.Lock() + defer sm.serverConfigLock.Unlock() + serverCfg := sm.serverConfigValue.Load().(serverConfig) + + // Shuffle the server list on server join. Servers are selected from + // the head of the list and are moved to the end of the list on + // failure. + for i := len(serverCfg.servers) - 1; i > 0; i-- { + j := rand.Int31n(int32(i + 1)) + serverCfg.servers[i], serverCfg.servers[j] = serverCfg.servers[j], serverCfg.servers[i] + } + + serverCfg.resetRebalanceTimer(sm) + sm.serverConfigValue.Store(serverCfg) +} + +func (sm *ServerManager) RemoveServer(server *server_details.ServerDetails) { + sm.serverConfigLock.Lock() + defer sm.serverConfigLock.Unlock() + serverCfg := sm.serverConfigValue.Load().(serverConfig) + + // Remove the server if known + n := len(serverCfg.servers) + for i := 0; i < n; i++ { + if serverCfg.servers[i].Name == server.Name { + serverCfg.servers[i], serverCfg.servers[n-1] = serverCfg.servers[n-1], nil + serverCfg.servers = serverCfg.servers[:n-1] + break + } + } + + sm.serverConfigValue.Store(serverCfg) +} + +// resetRebalanceTimer assumes: +// +// 1) the serverConfigLock is already held by the caller. +// 2) the caller will call serverConfigValue.Store() +func (sc *serverConfig) resetRebalanceTimer(sm *ServerManager) { + numConsulServers := len(sc.servers) + // Limit this connection's life based on the size (and health) of the + // cluster. Never rebalance a connection more frequently than + // connReuseLowWatermarkDuration, and make sure we never exceed + // clusterWideRebalanceConnsPerSec operations/s across numLANMembers. + clusterWideRebalanceConnsPerSec := float64(numConsulServers * newRebalanceConnsPerSecPerServer) + connReuseLowWatermarkDuration := clientRPCMinReuseDuration + lib.RandomStagger(clientRPCMinReuseDuration/clientRPCJitterFraction) + numLANMembers := 16384 // Assume sufficiently large for now. FIXME: numLanMembers := len(c.LANMembers()) + connRebalanceTimeout := lib.RateScaledInterval(clusterWideRebalanceConnsPerSec, connReuseLowWatermarkDuration, numLANMembers) + sm.logger.Printf("[DEBUG] consul: connection will be rebalanced in %v", connRebalanceTimeout) + + sc.rebalanceTimer.Reset(connRebalanceTimeout) +} + +// StartServerManager is used to start and manage the task of automatically +// shuffling and rebalance the list of consul servers. This maintenance +// happens either when a new server is added or when a duration has been +// exceed. +func (sm *ServerManager) StartServerManager() { + defaultTimeout := 5 * time.Second // FIXME(sean@): This is a bullshit value + var rebalanceTimer *time.Timer + func() { + sm.serverConfigLock.Lock() + defer sm.serverConfigLock.Unlock() + + serverCfgPtr := sm.serverConfigValue.Load() + if serverCfgPtr == nil { + panic("server config has not been initialized") + } + var serverCfg serverConfig + serverCfg = serverCfgPtr.(serverConfig) + rebalanceTimer = time.NewTimer(defaultTimeout) + serverCfg.rebalanceTimer = rebalanceTimer + }() + + for { + select { + case e := <-sm.consulServersCh: + switch e { + case consulServersNodeJoin: + sm.logger.Printf("[INFO] consul: new node joined cluster") + sm.RebalanceServers() + case consulServersRebalance: + sm.logger.Printf("[INFO] consul: rebalancing servers by request") + sm.RebalanceServers() + case consulServersRPCError: + sm.logger.Printf("[INFO] consul: need to find a new server to talk with") + sm.CycleFailedServers() + // FIXME(sean@): wtb preemptive Status.Ping + // of servers, ideally parallel fan-out of N + // nodes, then settle on the first node which + // responds successfully. + // + // Is there a distinction between slow and + // offline? Do we run the Status.Ping with a + // fixed timeout (say 30s) that way we can + // alert administrators that they've set + // their RPC time too low even though the + // Ping did return successfully? + default: + sm.logger.Printf("[WARN] consul: unhandled LAN Serf Event: %#v", e) + } + case <-rebalanceTimer.C: + sm.logger.Printf("[INFO] consul: server rebalance timeout") + sm.RebalanceServers() + + case <-sm.shutdownCh: + return + } + } +} From 075d1b628fd6d9fdd5f98e64e4e1dff2caa7fb03 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Thu, 18 Feb 2016 15:44:32 -0800 Subject: [PATCH 15/69] Commit miss re: consuls variable rename --- consul/client_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consul/client_test.go b/consul/client_test.go index ce45e559b2..59403dab84 100644 --- a/consul/client_test.go +++ b/consul/client_test.go @@ -10,7 +10,7 @@ import ( "github.com/hashicorp/consul/consul/structs" "github.com/hashicorp/consul/testutil" - "github.com/hashicorp/net-rpc-msgpackrpc" + "github.com/hashicorp/consul/vendor/github.com/hashicorp/net-rpc-msgpackrpc" "github.com/hashicorp/serf/serf" ) From 9b8767aa678b70c1e840d36ea2003e0ec43f1e78 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 19 Feb 2016 12:13:17 -0800 Subject: [PATCH 16/69] Refactor out the management of Consul servers Move the management of c.consulServers (fka c.consuls) into consul/server_manager.go. This commit brings in a background task that proactively manages the server list and: *) reshuffles the list *) manages the timer out of the RPC() path *) uses atomics to detect a server has failed This is a WIP, more work in testing needs to be completed. --- consul/server_manager.go | 212 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 212 insertions(+) create mode 100644 consul/server_manager.go diff --git a/consul/server_manager.go b/consul/server_manager.go new file mode 100644 index 0000000000..35023b3464 --- /dev/null +++ b/consul/server_manager.go @@ -0,0 +1,212 @@ +package consul + +import ( + "math/rand" + "sync/atomic" + "time" + + "github.com/hashicorp/consul/lib" +) + +type consulServerEventTypes int + +const ( + // consulServersNodeJoin is used to notify of a new consulServer. + // The primary effect of this is a reshuffling of consulServers and + // finding a new preferredServer. + consulServersNodeJoin = iota + + // consulServersRebalance is used to signal we should rebalance our + // connection load across servers + consulServersRebalance + + // consulServersRPCError is used to signal when a server has either + // timed out or returned an error and we would like to have the + // server manager find a new preferredServer. + consulServersRPCError +) + +// serverCfg is the thread-safe configuration structure that is used to +// maintain the list of consul servers in Client. +// +// NOTE(sean@): We are explicitly relying on the fact that this is copied. +// Please keep this structure light. +type serverConfig struct { + // servers tracks the locally known servers + servers []*serverParts + + // Timer used to control rebalancing of servers + rebalanceTimer *time.Timer +} + +// consulServersManager is used to automatically shuffle and rebalance the +// list of consulServers. This maintenance happens either when a new server +// is added or when a duration has been exceed. +func (c *Client) consulServersManager() { + defaultTimeout := 5 * time.Second // FIXME(sean@): This is a bullshit value + var rebalanceTimer *time.Timer + func(c *Client) { + c.serverConfigMtx.Lock() + defer c.serverConfigMtx.Unlock() + + serverCfgPtr := c.serverConfigValue.Load() + if serverCfgPtr == nil { + panic("server config has not been initialized") + } + var serverCfg serverConfig + serverCfg = serverCfgPtr.(serverConfig) + rebalanceTimer = time.NewTimer(defaultTimeout) + serverCfg.rebalanceTimer = rebalanceTimer + }(c) + + for { + select { + case e := <-c.consulServersCh: + switch e { + case consulServersNodeJoin: + c.logger.Printf("[INFO] consul: new node joined cluster") + c.RebalanceServers() + case consulServersRebalance: + c.logger.Printf("[INFO] consul: rebalancing servers by request") + c.RebalanceServers() + case consulServersRPCError: + c.logger.Printf("[INFO] consul: need to find a new server to talk with") + c.CycleFailedServers() + // FIXME(sean@): wtb preemptive Status.Ping + // of servers, ideally parallel fan-out of N + // nodes, then settle on the first node which + // responds successfully. + // + // Is there a distinction between slow and + // offline? Do we run the Status.Ping with a + // fixed timeout (say 30s) that way we can + // alert administrators that they've set + // their RPC time too low even though the + // Ping did return successfully? + default: + c.logger.Printf("[WARN] consul: unhandled LAN Serf Event: %#v", e) + } + case <-rebalanceTimer.C: + c.logger.Printf("[INFO] consul: server rebalance timeout") + c.RebalanceServers() + + case <-c.shutdownCh: + return + } + } +} + +func (c *Client) AddServer(server *serverParts) { + c.serverConfigMtx.Lock() + defer c.serverConfigMtx.Unlock() + serverCfg := c.serverConfigValue.Load().(serverConfig) + + // Check if this server is known + found := false + for idx, existing := range serverCfg.servers { + if existing.Name == server.Name { + // Overwrite the existing server parts in order to + // possibly update metadata (i.e. server version) + serverCfg.servers[idx] = server + found = true + break + } + } + + // Add to the list if not known + if !found { + serverCfg.servers = append(serverCfg.servers, server) + + // Notify the server maintenance task of a new server + c.consulServersCh <- consulServersNodeJoin + } + + c.serverConfigValue.Store(serverCfg) + +} + +func (c *Client) CycleFailedServers() { + c.serverConfigMtx.Lock() + defer c.serverConfigMtx.Unlock() + serverCfg := c.serverConfigValue.Load().(serverConfig) + + for i := range serverCfg.servers { + failCount := atomic.LoadUint64(&(serverCfg.servers[i].Disabled)) + if failCount == 0 { + break + } else if failCount > 0 { + serverCfg.servers = serverCfg.cycleServer() + } + } + + serverCfg.resetRebalanceTimer(c) + c.serverConfigValue.Store(serverCfg) +} + +func (sc *serverConfig) cycleServer() (servers []*serverParts) { + numServers := len(servers) + if numServers < 2 { + // No action required for zero or one server situations + return servers + } + + var failedNode *serverParts + failedNode, servers = servers[0], servers[1:] + servers = append(servers, failedNode) + return servers +} + +func (c *Client) RebalanceServers() { + c.serverConfigMtx.Lock() + defer c.serverConfigMtx.Unlock() + serverCfg := c.serverConfigValue.Load().(serverConfig) + + // Shuffle the server list on server join. Servers are selected from + // the head of the list and are moved to the end of the list on + // failure. + for i := len(serverCfg.servers) - 1; i > 0; i-- { + j := rand.Int31n(int32(i + 1)) + serverCfg.servers[i], serverCfg.servers[j] = serverCfg.servers[j], serverCfg.servers[i] + } + + serverCfg.resetRebalanceTimer(c) + c.serverConfigValue.Store(serverCfg) +} + +func (c *Client) RemoveServer(server *serverParts) { + c.serverConfigMtx.Lock() + defer c.serverConfigMtx.Unlock() + serverCfg := c.serverConfigValue.Load().(serverConfig) + + // Remove the server if known + n := len(serverCfg.servers) + for i := 0; i < n; i++ { + if serverCfg.servers[i].Name == server.Name { + serverCfg.servers[i], serverCfg.servers[n-1] = serverCfg.servers[n-1], nil + serverCfg.servers = serverCfg.servers[:n-1] + break + } + } + + c.serverConfigValue.Store(serverCfg) + +} + +// resetRebalanceTimer assumes: +// +// 1) the serverConfigMtx is already held by the caller. +// 2) the caller will call serverConfigValue.Store() +func (sc *serverConfig) resetRebalanceTimer(c *Client) { + numConsulServers := len(sc.servers) + // Limit this connection's life based on the size (and health) of the + // cluster. Never rebalance a connection more frequently than + // connReuseLowWatermarkDuration, and make sure we never exceed + // clusterWideRebalanceConnsPerSec operations/s across numLANMembers. + clusterWideRebalanceConnsPerSec := float64(numConsulServers * newRebalanceConnsPerSecPerServer) + connReuseLowWatermarkDuration := clientRPCMinReuseDuration + lib.RandomStagger(clientRPCMinReuseDuration/clientRPCJitterFraction) + numLANMembers := len(c.LANMembers()) + connRebalanceTimeout := lib.RateScaledInterval(clusterWideRebalanceConnsPerSec, connReuseLowWatermarkDuration, numLANMembers) + c.logger.Printf("[DEBUG] consul: connection will be rebalanced in %v", connRebalanceTimeout) + + sc.rebalanceTimer.Reset(connRebalanceTimeout) +} From a482eaef708a716cae4b6bf0fd0fae145127c991 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 19 Feb 2016 13:19:13 -0800 Subject: [PATCH 17/69] Rename serverConfigMtx to serverConfigLock Pointed out by: @slackpad --- consul/server_manager.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/consul/server_manager.go b/consul/server_manager.go index 35023b3464..51b1acf325 100644 --- a/consul/server_manager.go +++ b/consul/server_manager.go @@ -46,8 +46,8 @@ func (c *Client) consulServersManager() { defaultTimeout := 5 * time.Second // FIXME(sean@): This is a bullshit value var rebalanceTimer *time.Timer func(c *Client) { - c.serverConfigMtx.Lock() - defer c.serverConfigMtx.Unlock() + c.serverConfigLock.Lock() + defer c.serverConfigLock.Unlock() serverCfgPtr := c.serverConfigValue.Load() if serverCfgPtr == nil { @@ -97,8 +97,8 @@ func (c *Client) consulServersManager() { } func (c *Client) AddServer(server *serverParts) { - c.serverConfigMtx.Lock() - defer c.serverConfigMtx.Unlock() + c.serverConfigLock.Lock() + defer c.serverConfigLock.Unlock() serverCfg := c.serverConfigValue.Load().(serverConfig) // Check if this server is known @@ -126,8 +126,8 @@ func (c *Client) AddServer(server *serverParts) { } func (c *Client) CycleFailedServers() { - c.serverConfigMtx.Lock() - defer c.serverConfigMtx.Unlock() + c.serverConfigLock.Lock() + defer c.serverConfigLock.Unlock() serverCfg := c.serverConfigValue.Load().(serverConfig) for i := range serverCfg.servers { @@ -157,8 +157,8 @@ func (sc *serverConfig) cycleServer() (servers []*serverParts) { } func (c *Client) RebalanceServers() { - c.serverConfigMtx.Lock() - defer c.serverConfigMtx.Unlock() + c.serverConfigLock.Lock() + defer c.serverConfigLock.Unlock() serverCfg := c.serverConfigValue.Load().(serverConfig) // Shuffle the server list on server join. Servers are selected from @@ -174,8 +174,8 @@ func (c *Client) RebalanceServers() { } func (c *Client) RemoveServer(server *serverParts) { - c.serverConfigMtx.Lock() - defer c.serverConfigMtx.Unlock() + c.serverConfigLock.Lock() + defer c.serverConfigLock.Unlock() serverCfg := c.serverConfigValue.Load().(serverConfig) // Remove the server if known @@ -194,7 +194,7 @@ func (c *Client) RemoveServer(server *serverParts) { // resetRebalanceTimer assumes: // -// 1) the serverConfigMtx is already held by the caller. +// 1) the serverConfigLock is already held by the caller. // 2) the caller will call serverConfigValue.Store() func (sc *serverConfig) resetRebalanceTimer(c *Client) { numConsulServers := len(sc.servers) From b9e558862004a58bf73992f0becfba59926b7b5a Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 19 Feb 2016 17:32:16 -0800 Subject: [PATCH 18/69] Move consul.serverConfig out of the consul package Relocated to its own package, server_manager. This now greatly simplifies the RPC() call path and appropriately hides the locking behind the package boundary. More work is needed to be done here --- consul/server_manager.go | 212 --------------------------------------- 1 file changed, 212 deletions(-) delete mode 100644 consul/server_manager.go diff --git a/consul/server_manager.go b/consul/server_manager.go deleted file mode 100644 index 51b1acf325..0000000000 --- a/consul/server_manager.go +++ /dev/null @@ -1,212 +0,0 @@ -package consul - -import ( - "math/rand" - "sync/atomic" - "time" - - "github.com/hashicorp/consul/lib" -) - -type consulServerEventTypes int - -const ( - // consulServersNodeJoin is used to notify of a new consulServer. - // The primary effect of this is a reshuffling of consulServers and - // finding a new preferredServer. - consulServersNodeJoin = iota - - // consulServersRebalance is used to signal we should rebalance our - // connection load across servers - consulServersRebalance - - // consulServersRPCError is used to signal when a server has either - // timed out or returned an error and we would like to have the - // server manager find a new preferredServer. - consulServersRPCError -) - -// serverCfg is the thread-safe configuration structure that is used to -// maintain the list of consul servers in Client. -// -// NOTE(sean@): We are explicitly relying on the fact that this is copied. -// Please keep this structure light. -type serverConfig struct { - // servers tracks the locally known servers - servers []*serverParts - - // Timer used to control rebalancing of servers - rebalanceTimer *time.Timer -} - -// consulServersManager is used to automatically shuffle and rebalance the -// list of consulServers. This maintenance happens either when a new server -// is added or when a duration has been exceed. -func (c *Client) consulServersManager() { - defaultTimeout := 5 * time.Second // FIXME(sean@): This is a bullshit value - var rebalanceTimer *time.Timer - func(c *Client) { - c.serverConfigLock.Lock() - defer c.serverConfigLock.Unlock() - - serverCfgPtr := c.serverConfigValue.Load() - if serverCfgPtr == nil { - panic("server config has not been initialized") - } - var serverCfg serverConfig - serverCfg = serverCfgPtr.(serverConfig) - rebalanceTimer = time.NewTimer(defaultTimeout) - serverCfg.rebalanceTimer = rebalanceTimer - }(c) - - for { - select { - case e := <-c.consulServersCh: - switch e { - case consulServersNodeJoin: - c.logger.Printf("[INFO] consul: new node joined cluster") - c.RebalanceServers() - case consulServersRebalance: - c.logger.Printf("[INFO] consul: rebalancing servers by request") - c.RebalanceServers() - case consulServersRPCError: - c.logger.Printf("[INFO] consul: need to find a new server to talk with") - c.CycleFailedServers() - // FIXME(sean@): wtb preemptive Status.Ping - // of servers, ideally parallel fan-out of N - // nodes, then settle on the first node which - // responds successfully. - // - // Is there a distinction between slow and - // offline? Do we run the Status.Ping with a - // fixed timeout (say 30s) that way we can - // alert administrators that they've set - // their RPC time too low even though the - // Ping did return successfully? - default: - c.logger.Printf("[WARN] consul: unhandled LAN Serf Event: %#v", e) - } - case <-rebalanceTimer.C: - c.logger.Printf("[INFO] consul: server rebalance timeout") - c.RebalanceServers() - - case <-c.shutdownCh: - return - } - } -} - -func (c *Client) AddServer(server *serverParts) { - c.serverConfigLock.Lock() - defer c.serverConfigLock.Unlock() - serverCfg := c.serverConfigValue.Load().(serverConfig) - - // Check if this server is known - found := false - for idx, existing := range serverCfg.servers { - if existing.Name == server.Name { - // Overwrite the existing server parts in order to - // possibly update metadata (i.e. server version) - serverCfg.servers[idx] = server - found = true - break - } - } - - // Add to the list if not known - if !found { - serverCfg.servers = append(serverCfg.servers, server) - - // Notify the server maintenance task of a new server - c.consulServersCh <- consulServersNodeJoin - } - - c.serverConfigValue.Store(serverCfg) - -} - -func (c *Client) CycleFailedServers() { - c.serverConfigLock.Lock() - defer c.serverConfigLock.Unlock() - serverCfg := c.serverConfigValue.Load().(serverConfig) - - for i := range serverCfg.servers { - failCount := atomic.LoadUint64(&(serverCfg.servers[i].Disabled)) - if failCount == 0 { - break - } else if failCount > 0 { - serverCfg.servers = serverCfg.cycleServer() - } - } - - serverCfg.resetRebalanceTimer(c) - c.serverConfigValue.Store(serverCfg) -} - -func (sc *serverConfig) cycleServer() (servers []*serverParts) { - numServers := len(servers) - if numServers < 2 { - // No action required for zero or one server situations - return servers - } - - var failedNode *serverParts - failedNode, servers = servers[0], servers[1:] - servers = append(servers, failedNode) - return servers -} - -func (c *Client) RebalanceServers() { - c.serverConfigLock.Lock() - defer c.serverConfigLock.Unlock() - serverCfg := c.serverConfigValue.Load().(serverConfig) - - // Shuffle the server list on server join. Servers are selected from - // the head of the list and are moved to the end of the list on - // failure. - for i := len(serverCfg.servers) - 1; i > 0; i-- { - j := rand.Int31n(int32(i + 1)) - serverCfg.servers[i], serverCfg.servers[j] = serverCfg.servers[j], serverCfg.servers[i] - } - - serverCfg.resetRebalanceTimer(c) - c.serverConfigValue.Store(serverCfg) -} - -func (c *Client) RemoveServer(server *serverParts) { - c.serverConfigLock.Lock() - defer c.serverConfigLock.Unlock() - serverCfg := c.serverConfigValue.Load().(serverConfig) - - // Remove the server if known - n := len(serverCfg.servers) - for i := 0; i < n; i++ { - if serverCfg.servers[i].Name == server.Name { - serverCfg.servers[i], serverCfg.servers[n-1] = serverCfg.servers[n-1], nil - serverCfg.servers = serverCfg.servers[:n-1] - break - } - } - - c.serverConfigValue.Store(serverCfg) - -} - -// resetRebalanceTimer assumes: -// -// 1) the serverConfigLock is already held by the caller. -// 2) the caller will call serverConfigValue.Store() -func (sc *serverConfig) resetRebalanceTimer(c *Client) { - numConsulServers := len(sc.servers) - // Limit this connection's life based on the size (and health) of the - // cluster. Never rebalance a connection more frequently than - // connReuseLowWatermarkDuration, and make sure we never exceed - // clusterWideRebalanceConnsPerSec operations/s across numLANMembers. - clusterWideRebalanceConnsPerSec := float64(numConsulServers * newRebalanceConnsPerSecPerServer) - connReuseLowWatermarkDuration := clientRPCMinReuseDuration + lib.RandomStagger(clientRPCMinReuseDuration/clientRPCJitterFraction) - numLANMembers := len(c.LANMembers()) - connRebalanceTimeout := lib.RateScaledInterval(clusterWideRebalanceConnsPerSec, connReuseLowWatermarkDuration, numLANMembers) - c.logger.Printf("[DEBUG] consul: connection will be rebalanced in %v", connRebalanceTimeout) - - sc.rebalanceTimer.Reset(connRebalanceTimeout) -} From 0eac8265737b5b9091d88e5b1949e859466b4d73 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 19 Feb 2016 12:13:17 -0800 Subject: [PATCH 19/69] Refactor out the management of Consul servers Move the management of c.consulServers (fka c.consuls) into consul/server_manager.go. This commit brings in a background task that proactively manages the server list and: *) reshuffles the list *) manages the timer out of the RPC() path *) uses atomics to detect a server has failed This is a WIP, more work in testing needs to be completed. --- consul/server_manager.go | 212 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 212 insertions(+) create mode 100644 consul/server_manager.go diff --git a/consul/server_manager.go b/consul/server_manager.go new file mode 100644 index 0000000000..35023b3464 --- /dev/null +++ b/consul/server_manager.go @@ -0,0 +1,212 @@ +package consul + +import ( + "math/rand" + "sync/atomic" + "time" + + "github.com/hashicorp/consul/lib" +) + +type consulServerEventTypes int + +const ( + // consulServersNodeJoin is used to notify of a new consulServer. + // The primary effect of this is a reshuffling of consulServers and + // finding a new preferredServer. + consulServersNodeJoin = iota + + // consulServersRebalance is used to signal we should rebalance our + // connection load across servers + consulServersRebalance + + // consulServersRPCError is used to signal when a server has either + // timed out or returned an error and we would like to have the + // server manager find a new preferredServer. + consulServersRPCError +) + +// serverCfg is the thread-safe configuration structure that is used to +// maintain the list of consul servers in Client. +// +// NOTE(sean@): We are explicitly relying on the fact that this is copied. +// Please keep this structure light. +type serverConfig struct { + // servers tracks the locally known servers + servers []*serverParts + + // Timer used to control rebalancing of servers + rebalanceTimer *time.Timer +} + +// consulServersManager is used to automatically shuffle and rebalance the +// list of consulServers. This maintenance happens either when a new server +// is added or when a duration has been exceed. +func (c *Client) consulServersManager() { + defaultTimeout := 5 * time.Second // FIXME(sean@): This is a bullshit value + var rebalanceTimer *time.Timer + func(c *Client) { + c.serverConfigMtx.Lock() + defer c.serverConfigMtx.Unlock() + + serverCfgPtr := c.serverConfigValue.Load() + if serverCfgPtr == nil { + panic("server config has not been initialized") + } + var serverCfg serverConfig + serverCfg = serverCfgPtr.(serverConfig) + rebalanceTimer = time.NewTimer(defaultTimeout) + serverCfg.rebalanceTimer = rebalanceTimer + }(c) + + for { + select { + case e := <-c.consulServersCh: + switch e { + case consulServersNodeJoin: + c.logger.Printf("[INFO] consul: new node joined cluster") + c.RebalanceServers() + case consulServersRebalance: + c.logger.Printf("[INFO] consul: rebalancing servers by request") + c.RebalanceServers() + case consulServersRPCError: + c.logger.Printf("[INFO] consul: need to find a new server to talk with") + c.CycleFailedServers() + // FIXME(sean@): wtb preemptive Status.Ping + // of servers, ideally parallel fan-out of N + // nodes, then settle on the first node which + // responds successfully. + // + // Is there a distinction between slow and + // offline? Do we run the Status.Ping with a + // fixed timeout (say 30s) that way we can + // alert administrators that they've set + // their RPC time too low even though the + // Ping did return successfully? + default: + c.logger.Printf("[WARN] consul: unhandled LAN Serf Event: %#v", e) + } + case <-rebalanceTimer.C: + c.logger.Printf("[INFO] consul: server rebalance timeout") + c.RebalanceServers() + + case <-c.shutdownCh: + return + } + } +} + +func (c *Client) AddServer(server *serverParts) { + c.serverConfigMtx.Lock() + defer c.serverConfigMtx.Unlock() + serverCfg := c.serverConfigValue.Load().(serverConfig) + + // Check if this server is known + found := false + for idx, existing := range serverCfg.servers { + if existing.Name == server.Name { + // Overwrite the existing server parts in order to + // possibly update metadata (i.e. server version) + serverCfg.servers[idx] = server + found = true + break + } + } + + // Add to the list if not known + if !found { + serverCfg.servers = append(serverCfg.servers, server) + + // Notify the server maintenance task of a new server + c.consulServersCh <- consulServersNodeJoin + } + + c.serverConfigValue.Store(serverCfg) + +} + +func (c *Client) CycleFailedServers() { + c.serverConfigMtx.Lock() + defer c.serverConfigMtx.Unlock() + serverCfg := c.serverConfigValue.Load().(serverConfig) + + for i := range serverCfg.servers { + failCount := atomic.LoadUint64(&(serverCfg.servers[i].Disabled)) + if failCount == 0 { + break + } else if failCount > 0 { + serverCfg.servers = serverCfg.cycleServer() + } + } + + serverCfg.resetRebalanceTimer(c) + c.serverConfigValue.Store(serverCfg) +} + +func (sc *serverConfig) cycleServer() (servers []*serverParts) { + numServers := len(servers) + if numServers < 2 { + // No action required for zero or one server situations + return servers + } + + var failedNode *serverParts + failedNode, servers = servers[0], servers[1:] + servers = append(servers, failedNode) + return servers +} + +func (c *Client) RebalanceServers() { + c.serverConfigMtx.Lock() + defer c.serverConfigMtx.Unlock() + serverCfg := c.serverConfigValue.Load().(serverConfig) + + // Shuffle the server list on server join. Servers are selected from + // the head of the list and are moved to the end of the list on + // failure. + for i := len(serverCfg.servers) - 1; i > 0; i-- { + j := rand.Int31n(int32(i + 1)) + serverCfg.servers[i], serverCfg.servers[j] = serverCfg.servers[j], serverCfg.servers[i] + } + + serverCfg.resetRebalanceTimer(c) + c.serverConfigValue.Store(serverCfg) +} + +func (c *Client) RemoveServer(server *serverParts) { + c.serverConfigMtx.Lock() + defer c.serverConfigMtx.Unlock() + serverCfg := c.serverConfigValue.Load().(serverConfig) + + // Remove the server if known + n := len(serverCfg.servers) + for i := 0; i < n; i++ { + if serverCfg.servers[i].Name == server.Name { + serverCfg.servers[i], serverCfg.servers[n-1] = serverCfg.servers[n-1], nil + serverCfg.servers = serverCfg.servers[:n-1] + break + } + } + + c.serverConfigValue.Store(serverCfg) + +} + +// resetRebalanceTimer assumes: +// +// 1) the serverConfigMtx is already held by the caller. +// 2) the caller will call serverConfigValue.Store() +func (sc *serverConfig) resetRebalanceTimer(c *Client) { + numConsulServers := len(sc.servers) + // Limit this connection's life based on the size (and health) of the + // cluster. Never rebalance a connection more frequently than + // connReuseLowWatermarkDuration, and make sure we never exceed + // clusterWideRebalanceConnsPerSec operations/s across numLANMembers. + clusterWideRebalanceConnsPerSec := float64(numConsulServers * newRebalanceConnsPerSecPerServer) + connReuseLowWatermarkDuration := clientRPCMinReuseDuration + lib.RandomStagger(clientRPCMinReuseDuration/clientRPCJitterFraction) + numLANMembers := len(c.LANMembers()) + connRebalanceTimeout := lib.RateScaledInterval(clusterWideRebalanceConnsPerSec, connReuseLowWatermarkDuration, numLANMembers) + c.logger.Printf("[DEBUG] consul: connection will be rebalanced in %v", connRebalanceTimeout) + + sc.rebalanceTimer.Reset(connRebalanceTimeout) +} From 117c65dc552275087f53ef262d1f82a2d10eaf5f Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 19 Feb 2016 13:19:13 -0800 Subject: [PATCH 20/69] Rename serverConfigMtx to serverConfigLock Pointed out by: @slackpad --- consul/server_manager.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/consul/server_manager.go b/consul/server_manager.go index 35023b3464..51b1acf325 100644 --- a/consul/server_manager.go +++ b/consul/server_manager.go @@ -46,8 +46,8 @@ func (c *Client) consulServersManager() { defaultTimeout := 5 * time.Second // FIXME(sean@): This is a bullshit value var rebalanceTimer *time.Timer func(c *Client) { - c.serverConfigMtx.Lock() - defer c.serverConfigMtx.Unlock() + c.serverConfigLock.Lock() + defer c.serverConfigLock.Unlock() serverCfgPtr := c.serverConfigValue.Load() if serverCfgPtr == nil { @@ -97,8 +97,8 @@ func (c *Client) consulServersManager() { } func (c *Client) AddServer(server *serverParts) { - c.serverConfigMtx.Lock() - defer c.serverConfigMtx.Unlock() + c.serverConfigLock.Lock() + defer c.serverConfigLock.Unlock() serverCfg := c.serverConfigValue.Load().(serverConfig) // Check if this server is known @@ -126,8 +126,8 @@ func (c *Client) AddServer(server *serverParts) { } func (c *Client) CycleFailedServers() { - c.serverConfigMtx.Lock() - defer c.serverConfigMtx.Unlock() + c.serverConfigLock.Lock() + defer c.serverConfigLock.Unlock() serverCfg := c.serverConfigValue.Load().(serverConfig) for i := range serverCfg.servers { @@ -157,8 +157,8 @@ func (sc *serverConfig) cycleServer() (servers []*serverParts) { } func (c *Client) RebalanceServers() { - c.serverConfigMtx.Lock() - defer c.serverConfigMtx.Unlock() + c.serverConfigLock.Lock() + defer c.serverConfigLock.Unlock() serverCfg := c.serverConfigValue.Load().(serverConfig) // Shuffle the server list on server join. Servers are selected from @@ -174,8 +174,8 @@ func (c *Client) RebalanceServers() { } func (c *Client) RemoveServer(server *serverParts) { - c.serverConfigMtx.Lock() - defer c.serverConfigMtx.Unlock() + c.serverConfigLock.Lock() + defer c.serverConfigLock.Unlock() serverCfg := c.serverConfigValue.Load().(serverConfig) // Remove the server if known @@ -194,7 +194,7 @@ func (c *Client) RemoveServer(server *serverParts) { // resetRebalanceTimer assumes: // -// 1) the serverConfigMtx is already held by the caller. +// 1) the serverConfigLock is already held by the caller. // 2) the caller will call serverConfigValue.Store() func (sc *serverConfig) resetRebalanceTimer(c *Client) { numConsulServers := len(sc.servers) From 01b637114ca86df4e5140b75007435b17a432e8f Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 19 Feb 2016 17:32:16 -0800 Subject: [PATCH 21/69] Move consul.serverConfig out of the consul package Relocated to its own package, server_manager. This now greatly simplifies the RPC() call path and appropriately hides the locking behind the package boundary. More work is needed to be done here --- consul/server_manager.go | 212 --------------------------------------- 1 file changed, 212 deletions(-) delete mode 100644 consul/server_manager.go diff --git a/consul/server_manager.go b/consul/server_manager.go deleted file mode 100644 index 51b1acf325..0000000000 --- a/consul/server_manager.go +++ /dev/null @@ -1,212 +0,0 @@ -package consul - -import ( - "math/rand" - "sync/atomic" - "time" - - "github.com/hashicorp/consul/lib" -) - -type consulServerEventTypes int - -const ( - // consulServersNodeJoin is used to notify of a new consulServer. - // The primary effect of this is a reshuffling of consulServers and - // finding a new preferredServer. - consulServersNodeJoin = iota - - // consulServersRebalance is used to signal we should rebalance our - // connection load across servers - consulServersRebalance - - // consulServersRPCError is used to signal when a server has either - // timed out or returned an error and we would like to have the - // server manager find a new preferredServer. - consulServersRPCError -) - -// serverCfg is the thread-safe configuration structure that is used to -// maintain the list of consul servers in Client. -// -// NOTE(sean@): We are explicitly relying on the fact that this is copied. -// Please keep this structure light. -type serverConfig struct { - // servers tracks the locally known servers - servers []*serverParts - - // Timer used to control rebalancing of servers - rebalanceTimer *time.Timer -} - -// consulServersManager is used to automatically shuffle and rebalance the -// list of consulServers. This maintenance happens either when a new server -// is added or when a duration has been exceed. -func (c *Client) consulServersManager() { - defaultTimeout := 5 * time.Second // FIXME(sean@): This is a bullshit value - var rebalanceTimer *time.Timer - func(c *Client) { - c.serverConfigLock.Lock() - defer c.serverConfigLock.Unlock() - - serverCfgPtr := c.serverConfigValue.Load() - if serverCfgPtr == nil { - panic("server config has not been initialized") - } - var serverCfg serverConfig - serverCfg = serverCfgPtr.(serverConfig) - rebalanceTimer = time.NewTimer(defaultTimeout) - serverCfg.rebalanceTimer = rebalanceTimer - }(c) - - for { - select { - case e := <-c.consulServersCh: - switch e { - case consulServersNodeJoin: - c.logger.Printf("[INFO] consul: new node joined cluster") - c.RebalanceServers() - case consulServersRebalance: - c.logger.Printf("[INFO] consul: rebalancing servers by request") - c.RebalanceServers() - case consulServersRPCError: - c.logger.Printf("[INFO] consul: need to find a new server to talk with") - c.CycleFailedServers() - // FIXME(sean@): wtb preemptive Status.Ping - // of servers, ideally parallel fan-out of N - // nodes, then settle on the first node which - // responds successfully. - // - // Is there a distinction between slow and - // offline? Do we run the Status.Ping with a - // fixed timeout (say 30s) that way we can - // alert administrators that they've set - // their RPC time too low even though the - // Ping did return successfully? - default: - c.logger.Printf("[WARN] consul: unhandled LAN Serf Event: %#v", e) - } - case <-rebalanceTimer.C: - c.logger.Printf("[INFO] consul: server rebalance timeout") - c.RebalanceServers() - - case <-c.shutdownCh: - return - } - } -} - -func (c *Client) AddServer(server *serverParts) { - c.serverConfigLock.Lock() - defer c.serverConfigLock.Unlock() - serverCfg := c.serverConfigValue.Load().(serverConfig) - - // Check if this server is known - found := false - for idx, existing := range serverCfg.servers { - if existing.Name == server.Name { - // Overwrite the existing server parts in order to - // possibly update metadata (i.e. server version) - serverCfg.servers[idx] = server - found = true - break - } - } - - // Add to the list if not known - if !found { - serverCfg.servers = append(serverCfg.servers, server) - - // Notify the server maintenance task of a new server - c.consulServersCh <- consulServersNodeJoin - } - - c.serverConfigValue.Store(serverCfg) - -} - -func (c *Client) CycleFailedServers() { - c.serverConfigLock.Lock() - defer c.serverConfigLock.Unlock() - serverCfg := c.serverConfigValue.Load().(serverConfig) - - for i := range serverCfg.servers { - failCount := atomic.LoadUint64(&(serverCfg.servers[i].Disabled)) - if failCount == 0 { - break - } else if failCount > 0 { - serverCfg.servers = serverCfg.cycleServer() - } - } - - serverCfg.resetRebalanceTimer(c) - c.serverConfigValue.Store(serverCfg) -} - -func (sc *serverConfig) cycleServer() (servers []*serverParts) { - numServers := len(servers) - if numServers < 2 { - // No action required for zero or one server situations - return servers - } - - var failedNode *serverParts - failedNode, servers = servers[0], servers[1:] - servers = append(servers, failedNode) - return servers -} - -func (c *Client) RebalanceServers() { - c.serverConfigLock.Lock() - defer c.serverConfigLock.Unlock() - serverCfg := c.serverConfigValue.Load().(serverConfig) - - // Shuffle the server list on server join. Servers are selected from - // the head of the list and are moved to the end of the list on - // failure. - for i := len(serverCfg.servers) - 1; i > 0; i-- { - j := rand.Int31n(int32(i + 1)) - serverCfg.servers[i], serverCfg.servers[j] = serverCfg.servers[j], serverCfg.servers[i] - } - - serverCfg.resetRebalanceTimer(c) - c.serverConfigValue.Store(serverCfg) -} - -func (c *Client) RemoveServer(server *serverParts) { - c.serverConfigLock.Lock() - defer c.serverConfigLock.Unlock() - serverCfg := c.serverConfigValue.Load().(serverConfig) - - // Remove the server if known - n := len(serverCfg.servers) - for i := 0; i < n; i++ { - if serverCfg.servers[i].Name == server.Name { - serverCfg.servers[i], serverCfg.servers[n-1] = serverCfg.servers[n-1], nil - serverCfg.servers = serverCfg.servers[:n-1] - break - } - } - - c.serverConfigValue.Store(serverCfg) - -} - -// resetRebalanceTimer assumes: -// -// 1) the serverConfigLock is already held by the caller. -// 2) the caller will call serverConfigValue.Store() -func (sc *serverConfig) resetRebalanceTimer(c *Client) { - numConsulServers := len(sc.servers) - // Limit this connection's life based on the size (and health) of the - // cluster. Never rebalance a connection more frequently than - // connReuseLowWatermarkDuration, and make sure we never exceed - // clusterWideRebalanceConnsPerSec operations/s across numLANMembers. - clusterWideRebalanceConnsPerSec := float64(numConsulServers * newRebalanceConnsPerSecPerServer) - connReuseLowWatermarkDuration := clientRPCMinReuseDuration + lib.RandomStagger(clientRPCMinReuseDuration/clientRPCJitterFraction) - numLANMembers := len(c.LANMembers()) - connRebalanceTimeout := lib.RateScaledInterval(clusterWideRebalanceConnsPerSec, connReuseLowWatermarkDuration, numLANMembers) - c.logger.Printf("[DEBUG] consul: connection will be rebalanced in %v", connRebalanceTimeout) - - sc.rebalanceTimer.Reset(connRebalanceTimeout) -} From e48b910f87548d73191935c8bcc5722f4e910309 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 19 Feb 2016 12:13:17 -0800 Subject: [PATCH 22/69] Refactor out the management of Consul servers Move the management of c.consulServers (fka c.consuls) into consul/server_manager.go. This commit brings in a background task that proactively manages the server list and: *) reshuffles the list *) manages the timer out of the RPC() path *) uses atomics to detect a server has failed This is a WIP, more work in testing needs to be completed. --- consul/server_manager.go | 212 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 212 insertions(+) create mode 100644 consul/server_manager.go diff --git a/consul/server_manager.go b/consul/server_manager.go new file mode 100644 index 0000000000..35023b3464 --- /dev/null +++ b/consul/server_manager.go @@ -0,0 +1,212 @@ +package consul + +import ( + "math/rand" + "sync/atomic" + "time" + + "github.com/hashicorp/consul/lib" +) + +type consulServerEventTypes int + +const ( + // consulServersNodeJoin is used to notify of a new consulServer. + // The primary effect of this is a reshuffling of consulServers and + // finding a new preferredServer. + consulServersNodeJoin = iota + + // consulServersRebalance is used to signal we should rebalance our + // connection load across servers + consulServersRebalance + + // consulServersRPCError is used to signal when a server has either + // timed out or returned an error and we would like to have the + // server manager find a new preferredServer. + consulServersRPCError +) + +// serverCfg is the thread-safe configuration structure that is used to +// maintain the list of consul servers in Client. +// +// NOTE(sean@): We are explicitly relying on the fact that this is copied. +// Please keep this structure light. +type serverConfig struct { + // servers tracks the locally known servers + servers []*serverParts + + // Timer used to control rebalancing of servers + rebalanceTimer *time.Timer +} + +// consulServersManager is used to automatically shuffle and rebalance the +// list of consulServers. This maintenance happens either when a new server +// is added or when a duration has been exceed. +func (c *Client) consulServersManager() { + defaultTimeout := 5 * time.Second // FIXME(sean@): This is a bullshit value + var rebalanceTimer *time.Timer + func(c *Client) { + c.serverConfigMtx.Lock() + defer c.serverConfigMtx.Unlock() + + serverCfgPtr := c.serverConfigValue.Load() + if serverCfgPtr == nil { + panic("server config has not been initialized") + } + var serverCfg serverConfig + serverCfg = serverCfgPtr.(serverConfig) + rebalanceTimer = time.NewTimer(defaultTimeout) + serverCfg.rebalanceTimer = rebalanceTimer + }(c) + + for { + select { + case e := <-c.consulServersCh: + switch e { + case consulServersNodeJoin: + c.logger.Printf("[INFO] consul: new node joined cluster") + c.RebalanceServers() + case consulServersRebalance: + c.logger.Printf("[INFO] consul: rebalancing servers by request") + c.RebalanceServers() + case consulServersRPCError: + c.logger.Printf("[INFO] consul: need to find a new server to talk with") + c.CycleFailedServers() + // FIXME(sean@): wtb preemptive Status.Ping + // of servers, ideally parallel fan-out of N + // nodes, then settle on the first node which + // responds successfully. + // + // Is there a distinction between slow and + // offline? Do we run the Status.Ping with a + // fixed timeout (say 30s) that way we can + // alert administrators that they've set + // their RPC time too low even though the + // Ping did return successfully? + default: + c.logger.Printf("[WARN] consul: unhandled LAN Serf Event: %#v", e) + } + case <-rebalanceTimer.C: + c.logger.Printf("[INFO] consul: server rebalance timeout") + c.RebalanceServers() + + case <-c.shutdownCh: + return + } + } +} + +func (c *Client) AddServer(server *serverParts) { + c.serverConfigMtx.Lock() + defer c.serverConfigMtx.Unlock() + serverCfg := c.serverConfigValue.Load().(serverConfig) + + // Check if this server is known + found := false + for idx, existing := range serverCfg.servers { + if existing.Name == server.Name { + // Overwrite the existing server parts in order to + // possibly update metadata (i.e. server version) + serverCfg.servers[idx] = server + found = true + break + } + } + + // Add to the list if not known + if !found { + serverCfg.servers = append(serverCfg.servers, server) + + // Notify the server maintenance task of a new server + c.consulServersCh <- consulServersNodeJoin + } + + c.serverConfigValue.Store(serverCfg) + +} + +func (c *Client) CycleFailedServers() { + c.serverConfigMtx.Lock() + defer c.serverConfigMtx.Unlock() + serverCfg := c.serverConfigValue.Load().(serverConfig) + + for i := range serverCfg.servers { + failCount := atomic.LoadUint64(&(serverCfg.servers[i].Disabled)) + if failCount == 0 { + break + } else if failCount > 0 { + serverCfg.servers = serverCfg.cycleServer() + } + } + + serverCfg.resetRebalanceTimer(c) + c.serverConfigValue.Store(serverCfg) +} + +func (sc *serverConfig) cycleServer() (servers []*serverParts) { + numServers := len(servers) + if numServers < 2 { + // No action required for zero or one server situations + return servers + } + + var failedNode *serverParts + failedNode, servers = servers[0], servers[1:] + servers = append(servers, failedNode) + return servers +} + +func (c *Client) RebalanceServers() { + c.serverConfigMtx.Lock() + defer c.serverConfigMtx.Unlock() + serverCfg := c.serverConfigValue.Load().(serverConfig) + + // Shuffle the server list on server join. Servers are selected from + // the head of the list and are moved to the end of the list on + // failure. + for i := len(serverCfg.servers) - 1; i > 0; i-- { + j := rand.Int31n(int32(i + 1)) + serverCfg.servers[i], serverCfg.servers[j] = serverCfg.servers[j], serverCfg.servers[i] + } + + serverCfg.resetRebalanceTimer(c) + c.serverConfigValue.Store(serverCfg) +} + +func (c *Client) RemoveServer(server *serverParts) { + c.serverConfigMtx.Lock() + defer c.serverConfigMtx.Unlock() + serverCfg := c.serverConfigValue.Load().(serverConfig) + + // Remove the server if known + n := len(serverCfg.servers) + for i := 0; i < n; i++ { + if serverCfg.servers[i].Name == server.Name { + serverCfg.servers[i], serverCfg.servers[n-1] = serverCfg.servers[n-1], nil + serverCfg.servers = serverCfg.servers[:n-1] + break + } + } + + c.serverConfigValue.Store(serverCfg) + +} + +// resetRebalanceTimer assumes: +// +// 1) the serverConfigMtx is already held by the caller. +// 2) the caller will call serverConfigValue.Store() +func (sc *serverConfig) resetRebalanceTimer(c *Client) { + numConsulServers := len(sc.servers) + // Limit this connection's life based on the size (and health) of the + // cluster. Never rebalance a connection more frequently than + // connReuseLowWatermarkDuration, and make sure we never exceed + // clusterWideRebalanceConnsPerSec operations/s across numLANMembers. + clusterWideRebalanceConnsPerSec := float64(numConsulServers * newRebalanceConnsPerSecPerServer) + connReuseLowWatermarkDuration := clientRPCMinReuseDuration + lib.RandomStagger(clientRPCMinReuseDuration/clientRPCJitterFraction) + numLANMembers := len(c.LANMembers()) + connRebalanceTimeout := lib.RateScaledInterval(clusterWideRebalanceConnsPerSec, connReuseLowWatermarkDuration, numLANMembers) + c.logger.Printf("[DEBUG] consul: connection will be rebalanced in %v", connRebalanceTimeout) + + sc.rebalanceTimer.Reset(connRebalanceTimeout) +} From c7c551dbe0e4a56fe1843bf68cbd65ac868cb186 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 19 Feb 2016 13:19:13 -0800 Subject: [PATCH 23/69] Rename serverConfigMtx to serverConfigLock Pointed out by: @slackpad --- consul/server_manager.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/consul/server_manager.go b/consul/server_manager.go index 35023b3464..51b1acf325 100644 --- a/consul/server_manager.go +++ b/consul/server_manager.go @@ -46,8 +46,8 @@ func (c *Client) consulServersManager() { defaultTimeout := 5 * time.Second // FIXME(sean@): This is a bullshit value var rebalanceTimer *time.Timer func(c *Client) { - c.serverConfigMtx.Lock() - defer c.serverConfigMtx.Unlock() + c.serverConfigLock.Lock() + defer c.serverConfigLock.Unlock() serverCfgPtr := c.serverConfigValue.Load() if serverCfgPtr == nil { @@ -97,8 +97,8 @@ func (c *Client) consulServersManager() { } func (c *Client) AddServer(server *serverParts) { - c.serverConfigMtx.Lock() - defer c.serverConfigMtx.Unlock() + c.serverConfigLock.Lock() + defer c.serverConfigLock.Unlock() serverCfg := c.serverConfigValue.Load().(serverConfig) // Check if this server is known @@ -126,8 +126,8 @@ func (c *Client) AddServer(server *serverParts) { } func (c *Client) CycleFailedServers() { - c.serverConfigMtx.Lock() - defer c.serverConfigMtx.Unlock() + c.serverConfigLock.Lock() + defer c.serverConfigLock.Unlock() serverCfg := c.serverConfigValue.Load().(serverConfig) for i := range serverCfg.servers { @@ -157,8 +157,8 @@ func (sc *serverConfig) cycleServer() (servers []*serverParts) { } func (c *Client) RebalanceServers() { - c.serverConfigMtx.Lock() - defer c.serverConfigMtx.Unlock() + c.serverConfigLock.Lock() + defer c.serverConfigLock.Unlock() serverCfg := c.serverConfigValue.Load().(serverConfig) // Shuffle the server list on server join. Servers are selected from @@ -174,8 +174,8 @@ func (c *Client) RebalanceServers() { } func (c *Client) RemoveServer(server *serverParts) { - c.serverConfigMtx.Lock() - defer c.serverConfigMtx.Unlock() + c.serverConfigLock.Lock() + defer c.serverConfigLock.Unlock() serverCfg := c.serverConfigValue.Load().(serverConfig) // Remove the server if known @@ -194,7 +194,7 @@ func (c *Client) RemoveServer(server *serverParts) { // resetRebalanceTimer assumes: // -// 1) the serverConfigMtx is already held by the caller. +// 1) the serverConfigLock is already held by the caller. // 2) the caller will call serverConfigValue.Store() func (sc *serverConfig) resetRebalanceTimer(c *Client) { numConsulServers := len(sc.servers) From 579e536f58145b7439b375dc9ce85666b8e65f9d Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 19 Feb 2016 17:32:16 -0800 Subject: [PATCH 24/69] Move consul.serverConfig out of the consul package Relocated to its own package, server_manager. This now greatly simplifies the RPC() call path and appropriately hides the locking behind the package boundary. More work is needed to be done here --- consul/server_manager.go | 212 --------------------------------------- 1 file changed, 212 deletions(-) delete mode 100644 consul/server_manager.go diff --git a/consul/server_manager.go b/consul/server_manager.go deleted file mode 100644 index 51b1acf325..0000000000 --- a/consul/server_manager.go +++ /dev/null @@ -1,212 +0,0 @@ -package consul - -import ( - "math/rand" - "sync/atomic" - "time" - - "github.com/hashicorp/consul/lib" -) - -type consulServerEventTypes int - -const ( - // consulServersNodeJoin is used to notify of a new consulServer. - // The primary effect of this is a reshuffling of consulServers and - // finding a new preferredServer. - consulServersNodeJoin = iota - - // consulServersRebalance is used to signal we should rebalance our - // connection load across servers - consulServersRebalance - - // consulServersRPCError is used to signal when a server has either - // timed out or returned an error and we would like to have the - // server manager find a new preferredServer. - consulServersRPCError -) - -// serverCfg is the thread-safe configuration structure that is used to -// maintain the list of consul servers in Client. -// -// NOTE(sean@): We are explicitly relying on the fact that this is copied. -// Please keep this structure light. -type serverConfig struct { - // servers tracks the locally known servers - servers []*serverParts - - // Timer used to control rebalancing of servers - rebalanceTimer *time.Timer -} - -// consulServersManager is used to automatically shuffle and rebalance the -// list of consulServers. This maintenance happens either when a new server -// is added or when a duration has been exceed. -func (c *Client) consulServersManager() { - defaultTimeout := 5 * time.Second // FIXME(sean@): This is a bullshit value - var rebalanceTimer *time.Timer - func(c *Client) { - c.serverConfigLock.Lock() - defer c.serverConfigLock.Unlock() - - serverCfgPtr := c.serverConfigValue.Load() - if serverCfgPtr == nil { - panic("server config has not been initialized") - } - var serverCfg serverConfig - serverCfg = serverCfgPtr.(serverConfig) - rebalanceTimer = time.NewTimer(defaultTimeout) - serverCfg.rebalanceTimer = rebalanceTimer - }(c) - - for { - select { - case e := <-c.consulServersCh: - switch e { - case consulServersNodeJoin: - c.logger.Printf("[INFO] consul: new node joined cluster") - c.RebalanceServers() - case consulServersRebalance: - c.logger.Printf("[INFO] consul: rebalancing servers by request") - c.RebalanceServers() - case consulServersRPCError: - c.logger.Printf("[INFO] consul: need to find a new server to talk with") - c.CycleFailedServers() - // FIXME(sean@): wtb preemptive Status.Ping - // of servers, ideally parallel fan-out of N - // nodes, then settle on the first node which - // responds successfully. - // - // Is there a distinction between slow and - // offline? Do we run the Status.Ping with a - // fixed timeout (say 30s) that way we can - // alert administrators that they've set - // their RPC time too low even though the - // Ping did return successfully? - default: - c.logger.Printf("[WARN] consul: unhandled LAN Serf Event: %#v", e) - } - case <-rebalanceTimer.C: - c.logger.Printf("[INFO] consul: server rebalance timeout") - c.RebalanceServers() - - case <-c.shutdownCh: - return - } - } -} - -func (c *Client) AddServer(server *serverParts) { - c.serverConfigLock.Lock() - defer c.serverConfigLock.Unlock() - serverCfg := c.serverConfigValue.Load().(serverConfig) - - // Check if this server is known - found := false - for idx, existing := range serverCfg.servers { - if existing.Name == server.Name { - // Overwrite the existing server parts in order to - // possibly update metadata (i.e. server version) - serverCfg.servers[idx] = server - found = true - break - } - } - - // Add to the list if not known - if !found { - serverCfg.servers = append(serverCfg.servers, server) - - // Notify the server maintenance task of a new server - c.consulServersCh <- consulServersNodeJoin - } - - c.serverConfigValue.Store(serverCfg) - -} - -func (c *Client) CycleFailedServers() { - c.serverConfigLock.Lock() - defer c.serverConfigLock.Unlock() - serverCfg := c.serverConfigValue.Load().(serverConfig) - - for i := range serverCfg.servers { - failCount := atomic.LoadUint64(&(serverCfg.servers[i].Disabled)) - if failCount == 0 { - break - } else if failCount > 0 { - serverCfg.servers = serverCfg.cycleServer() - } - } - - serverCfg.resetRebalanceTimer(c) - c.serverConfigValue.Store(serverCfg) -} - -func (sc *serverConfig) cycleServer() (servers []*serverParts) { - numServers := len(servers) - if numServers < 2 { - // No action required for zero or one server situations - return servers - } - - var failedNode *serverParts - failedNode, servers = servers[0], servers[1:] - servers = append(servers, failedNode) - return servers -} - -func (c *Client) RebalanceServers() { - c.serverConfigLock.Lock() - defer c.serverConfigLock.Unlock() - serverCfg := c.serverConfigValue.Load().(serverConfig) - - // Shuffle the server list on server join. Servers are selected from - // the head of the list and are moved to the end of the list on - // failure. - for i := len(serverCfg.servers) - 1; i > 0; i-- { - j := rand.Int31n(int32(i + 1)) - serverCfg.servers[i], serverCfg.servers[j] = serverCfg.servers[j], serverCfg.servers[i] - } - - serverCfg.resetRebalanceTimer(c) - c.serverConfigValue.Store(serverCfg) -} - -func (c *Client) RemoveServer(server *serverParts) { - c.serverConfigLock.Lock() - defer c.serverConfigLock.Unlock() - serverCfg := c.serverConfigValue.Load().(serverConfig) - - // Remove the server if known - n := len(serverCfg.servers) - for i := 0; i < n; i++ { - if serverCfg.servers[i].Name == server.Name { - serverCfg.servers[i], serverCfg.servers[n-1] = serverCfg.servers[n-1], nil - serverCfg.servers = serverCfg.servers[:n-1] - break - } - } - - c.serverConfigValue.Store(serverCfg) - -} - -// resetRebalanceTimer assumes: -// -// 1) the serverConfigLock is already held by the caller. -// 2) the caller will call serverConfigValue.Store() -func (sc *serverConfig) resetRebalanceTimer(c *Client) { - numConsulServers := len(sc.servers) - // Limit this connection's life based on the size (and health) of the - // cluster. Never rebalance a connection more frequently than - // connReuseLowWatermarkDuration, and make sure we never exceed - // clusterWideRebalanceConnsPerSec operations/s across numLANMembers. - clusterWideRebalanceConnsPerSec := float64(numConsulServers * newRebalanceConnsPerSecPerServer) - connReuseLowWatermarkDuration := clientRPCMinReuseDuration + lib.RandomStagger(clientRPCMinReuseDuration/clientRPCJitterFraction) - numLANMembers := len(c.LANMembers()) - connRebalanceTimeout := lib.RateScaledInterval(clusterWideRebalanceConnsPerSec, connReuseLowWatermarkDuration, numLANMembers) - c.logger.Printf("[DEBUG] consul: connection will be rebalanced in %v", connRebalanceTimeout) - - sc.rebalanceTimer.Reset(connRebalanceTimeout) -} From 9eb6481d734cb958ca7e5361fc578133541dc964 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 19 Feb 2016 18:56:15 -0800 Subject: [PATCH 25/69] Use config convenience method to get config 'cause ELETTHECOMPILERSDOTHEWORK. I don't need that cluttering up the subconscious with more complexity. --- consul/server_manager/server_manager.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index 3754e27599..370fe9a712 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -96,7 +96,7 @@ type ServerManager struct { func (sm *ServerManager) AddServer(server *server_details.ServerDetails) { sm.serverConfigLock.Lock() defer sm.serverConfigLock.Unlock() - serverCfg := sm.serverConfigValue.Load().(serverConfig) + serverCfg := sm.getServerConfig() // Check if this server is known found := false @@ -126,7 +126,7 @@ func (sm *ServerManager) AddServer(server *server_details.ServerDetails) { func (sm *ServerManager) CycleFailedServers() { sm.serverConfigLock.Lock() defer sm.serverConfigLock.Unlock() - serverCfg := sm.serverConfigValue.Load().(serverConfig) + serverCfg := sm.getServerConfig() for i := range serverCfg.servers { failCount := atomic.LoadUint64(&(serverCfg.servers[i].Disabled)) @@ -213,7 +213,7 @@ func (sm *ServerManager) NotifyFailedServer(server *server_details.ServerDetails func (sm *ServerManager) RebalanceServers() { sm.serverConfigLock.Lock() defer sm.serverConfigLock.Unlock() - serverCfg := sm.serverConfigValue.Load().(serverConfig) + serverCfg := sm.getServerConfig() // Shuffle the server list on server join. Servers are selected from // the head of the list and are moved to the end of the list on @@ -230,7 +230,7 @@ func (sm *ServerManager) RebalanceServers() { func (sm *ServerManager) RemoveServer(server *server_details.ServerDetails) { sm.serverConfigLock.Lock() defer sm.serverConfigLock.Unlock() - serverCfg := sm.serverConfigValue.Load().(serverConfig) + serverCfg := sm.getServerConfig() // Remove the server if known n := len(serverCfg.servers) From b4db49a62efaa465abeb5f17a57268b82a4e30e3 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 19 Feb 2016 18:57:15 -0800 Subject: [PATCH 26/69] Document the various functions and their locking --- consul/server_manager/server_manager.go | 32 +++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index 370fe9a712..dd6662b8bc 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -93,6 +93,9 @@ type ServerManager struct { logger *log.Logger } +// AddServer takes out an internal write lock and adds a new server. If the +// server is not known, it adds the new server and schedules a rebalance. If +// it is known, we merge the new server details. func (sm *ServerManager) AddServer(server *server_details.ServerDetails) { sm.serverConfigLock.Lock() defer sm.serverConfigLock.Unlock() @@ -123,6 +126,10 @@ func (sm *ServerManager) AddServer(server *server_details.ServerDetails) { sm.serverConfigValue.Store(serverCfg) } +// CycleFailedServers takes out an internal write lock and dequeues all +// failed servers and re-enqueues them. This method does not reshuffle the +// server list. Because this changed the order of servers, we push out the +// time at which a rebalance occurs. func (sm *ServerManager) CycleFailedServers() { sm.serverConfigLock.Lock() defer sm.serverConfigLock.Unlock() @@ -141,6 +148,9 @@ func (sm *ServerManager) CycleFailedServers() { sm.serverConfigValue.Store(serverCfg) } +// cycleServers returns a new list of servers that has dequeued the first +// server and enqueued it at the end of the list. cycleServers assumes the +// caller is holding the serverConfigLock. func (sc *serverConfig) cycleServer() (servers []*server_details.ServerDetails) { numServers := len(servers) if numServers < 2 { @@ -155,6 +165,8 @@ func (sc *serverConfig) cycleServer() (servers []*server_details.ServerDetails) return servers } +// FindHealthyServer takes out an internal "read lock" and searches through +// the list of servers to find a healthy server. func (sm *ServerManager) FindHealthyServer() (server *server_details.ServerDetails) { serverCfg := sm.getServerConfig() numServers := len(serverCfg.servers) @@ -178,6 +190,8 @@ func (sm *ServerManager) FindHealthyServer() (server *server_details.ServerDetai return server } +// GetNumServers takes out an internal "read lock" and returns the number of +// servers. numServers includes both healthy and unhealthy servers. func (sm *ServerManager) GetNumServers() (numServers int) { serverCfg := sm.getServerConfig() numServers = len(serverCfg.servers) @@ -205,11 +219,23 @@ func NewServerManager(logger *log.Logger, shutdownCh chan struct{}) (sm *ServerM return sm } +// NotifyFailedServer is an exported convenience function that allows callers +// to pass in a server that has failed an RPC request and mark it as failed. +// This will initiate a background task that will optimize the failed server +// to the end of the serer list. No locks are required here because we are +// bypassing the serverConfig and sending a message to ServerManager's +// channel. func (sm *ServerManager) NotifyFailedServer(server *server_details.ServerDetails) { atomic.AddUint64(&server.Disabled, 1) sm.consulServersCh <- consulServersRPCError } +// RebalanceServers takes out an internal write lock and shuffles the list of +// servers on this agent. This allows for a redistribution of work across +// consul servers and provides a guarantee that the order list of +// ServerDetails isn't actually ordered, therefore we can sequentially walk +// the array to pick a server without all agents in the cluster dog piling on +// a single node. func (sm *ServerManager) RebalanceServers() { sm.serverConfigLock.Lock() defer sm.serverConfigLock.Unlock() @@ -227,6 +253,12 @@ func (sm *ServerManager) RebalanceServers() { sm.serverConfigValue.Store(serverCfg) } +// RemoveServer takes out an internal write lock and removes a server from +// the server list. No rebalancing happens as a result of the removed server +// because we do not want a network partition which separated a server from +// this agent to cause an increase in work. Instead we rely on the internal +// already existing semantics to handle failure detection after a server has +// been removed. func (sm *ServerManager) RemoveServer(server *server_details.ServerDetails) { sm.serverConfigLock.Lock() defer sm.serverConfigLock.Unlock() From 00ff8e5307878b82b67a3db6db3ba04257228540 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 19 Feb 2016 18:58:49 -0800 Subject: [PATCH 27/69] Cosmetic and various other wordsmithing cleanups --- consul/server_manager/server_manager.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index dd6662b8bc..cbde08bfb1 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -154,12 +154,12 @@ func (sm *ServerManager) CycleFailedServers() { func (sc *serverConfig) cycleServer() (servers []*server_details.ServerDetails) { numServers := len(servers) if numServers < 2 { - // No action required for zero or one server situations + // No action required return servers } - newServers := make([]*server_details.ServerDetails, len(servers)+1) var dequeuedServer *server_details.ServerDetails + newServers := make([]*server_details.ServerDetails, len(servers)+1) dequeuedServer, newServers = servers[0], servers[1:] servers = append(newServers, dequeuedServer) return servers @@ -198,8 +198,8 @@ func (sm *ServerManager) GetNumServers() (numServers int) { return numServers } -// getServerConfig shorthand method to hide the locking semantics of -// atomic.Value +// getServerConfig is a convenience method to hide the locking semantics of +// atomic.Value from the caller. func (sm *ServerManager) getServerConfig() serverConfig { return sm.serverConfigValue.Load().(serverConfig) } @@ -207,8 +207,7 @@ func (sm *ServerManager) getServerConfig() serverConfig { // NewServerManager is the only way to safely create a new ServerManager // struct. // -// NOTE(sean@): We don't simply pass in a consul.Client struct to avoid a -// cyclic import +// NOTE(sean@): We can not pass in *consul.Client due to an import cycle func NewServerManager(logger *log.Logger, shutdownCh chan struct{}) (sm *ServerManager) { sm = new(ServerManager) // Create the initial serverConfig From a7091b0837f8139cabee15f501fcdf7949b51e11 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 19 Feb 2016 18:59:35 -0800 Subject: [PATCH 28/69] Properly retain a pointer to the rebalanceTimer --- consul/server_manager/server_manager.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index cbde08bfb1..4e95988ee5 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -300,7 +300,6 @@ func (sc *serverConfig) resetRebalanceTimer(sm *ServerManager) { // happens either when a new server is added or when a duration has been // exceed. func (sm *ServerManager) StartServerManager() { - defaultTimeout := 5 * time.Second // FIXME(sean@): This is a bullshit value var rebalanceTimer *time.Timer func() { sm.serverConfigLock.Lock() @@ -312,8 +311,9 @@ func (sm *ServerManager) StartServerManager() { } var serverCfg serverConfig serverCfg = serverCfgPtr.(serverConfig) - rebalanceTimer = time.NewTimer(defaultTimeout) - serverCfg.rebalanceTimer = rebalanceTimer + serverCfg.resetRebalanceTimer(sm) + rebalanceTimer = serverCfg.rebalanceTimer + sm.serverConfigValue.Store(serverCfg) }() for { From e6c27325d9dbcf249754a3c51c6e909b8bc7d03e Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 19 Feb 2016 19:00:23 -0800 Subject: [PATCH 29/69] rebalanceTimer may be nil during initialization When first starting the server manager, it's possible that the rebalanceTimer in serverConfig will be nil, test accordingly. --- consul/server_manager/server_manager.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index 4e95988ee5..63c7d71d3a 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -292,7 +292,11 @@ func (sc *serverConfig) resetRebalanceTimer(sm *ServerManager) { connRebalanceTimeout := lib.RateScaledInterval(clusterWideRebalanceConnsPerSec, connReuseLowWatermarkDuration, numLANMembers) sm.logger.Printf("[DEBUG] consul: connection will be rebalanced in %v", connRebalanceTimeout) - sc.rebalanceTimer.Reset(connRebalanceTimeout) + if sc.rebalanceTimer == nil { + sc.rebalanceTimer = time.NewTimer(connRebalanceTimeout) + } else { + sc.rebalanceTimer.Reset(connRebalanceTimeout) + } } // StartServerManager is used to start and manage the task of automatically From e53704b032e780c20cf882087e1901a73613192f Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 19 Feb 2016 19:01:46 -0800 Subject: [PATCH 30/69] Mutate copies of serverCfg.servers, not original Removing any ambiguity re: ownership of the mutated server lists is a win for maintenance and debugging. --- consul/server_manager/server_manager.go | 31 ++++++++++++++++++------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index 63c7d71d3a..a51ef73bd8 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -105,9 +105,14 @@ func (sm *ServerManager) AddServer(server *server_details.ServerDetails) { found := false for idx, existing := range serverCfg.servers { if existing.Name == server.Name { - // Overwrite the existing server parts in order to - // possibly update metadata (i.e. server version) - serverCfg.servers[idx] = server + newServers := make([]*server_details.ServerDetails, len(serverCfg.servers)) + copy(newServers, serverCfg.servers) + + // Overwrite the existing server details in order to + // possibly update metadata (e.g. server version) + newServers[idx] = server + + serverCfg.servers = newServers found = true break } @@ -240,13 +245,17 @@ func (sm *ServerManager) RebalanceServers() { defer sm.serverConfigLock.Unlock() serverCfg := sm.getServerConfig() + newServers := make([]*server_details.ServerDetails, len(serverCfg.servers)+1) + copy(newServers, serverCfg.servers) + // Shuffle the server list on server join. Servers are selected from // the head of the list and are moved to the end of the list on // failure. for i := len(serverCfg.servers) - 1; i > 0; i-- { j := rand.Int31n(int32(i + 1)) - serverCfg.servers[i], serverCfg.servers[j] = serverCfg.servers[j], serverCfg.servers[i] + newServers[i], newServers[j] = newServers[j], newServers[i] } + serverCfg.servers = newServers serverCfg.resetRebalanceTimer(sm) sm.serverConfigValue.Store(serverCfg) @@ -267,13 +276,17 @@ func (sm *ServerManager) RemoveServer(server *server_details.ServerDetails) { n := len(serverCfg.servers) for i := 0; i < n; i++ { if serverCfg.servers[i].Name == server.Name { - serverCfg.servers[i], serverCfg.servers[n-1] = serverCfg.servers[n-1], nil - serverCfg.servers = serverCfg.servers[:n-1] - break + newServers := make([]*server_details.ServerDetails, len(serverCfg.servers)-1) + copy(newServers, serverCfg.servers) + + newServers[i], newServers[n-1] = newServers[n-1], nil + newServers = newServers[:n-1] + serverCfg.servers = newServers + + sm.serverConfigValue.Store(serverCfg) + return } } - - sm.serverConfigValue.Store(serverCfg) } // resetRebalanceTimer assumes: From 7f55931d02a8b4283bfec7f3f3b06df5e6be8fc3 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Mon, 22 Feb 2016 13:01:44 -0800 Subject: [PATCH 31/69] Commit a handful of refactoring && copy/paste-o fixes --- consul/server_manager/server_manager.go | 32 +++++++++++++------------ 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index a51ef73bd8..c13c0d7be5 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -120,8 +120,9 @@ func (sm *ServerManager) AddServer(server *server_details.ServerDetails) { // Add to the list if not known if !found { - newServers := make([]*server_details.ServerDetails, len(serverCfg.servers)+1) + newServers := make([]*server_details.ServerDetails, len(serverCfg.servers), len(serverCfg.servers)+1) copy(newServers, serverCfg.servers) + newServers = append(newServers, server) serverCfg.servers = newServers // Notify the server maintenance task of a new server @@ -157,17 +158,16 @@ func (sm *ServerManager) CycleFailedServers() { // server and enqueued it at the end of the list. cycleServers assumes the // caller is holding the serverConfigLock. func (sc *serverConfig) cycleServer() (servers []*server_details.ServerDetails) { - numServers := len(servers) + numServers := len(sc.servers) if numServers < 2 { // No action required return servers } - var dequeuedServer *server_details.ServerDetails - newServers := make([]*server_details.ServerDetails, len(servers)+1) - dequeuedServer, newServers = servers[0], servers[1:] - servers = append(newServers, dequeuedServer) - return servers + newServers := make([]*server_details.ServerDetails, 0, numServers) + newServers = append(newServers, sc.servers[1:]...) + newServers = append(newServers, sc.servers[0]) + return newServers } // FindHealthyServer takes out an internal "read lock" and searches through @@ -203,23 +203,25 @@ func (sm *ServerManager) GetNumServers() (numServers int) { return numServers } -// getServerConfig is a convenience method to hide the locking semantics of -// atomic.Value from the caller. +// getServerConfig is a convenience method which hides the locking semantics +// of atomic.Value from the caller. func (sm *ServerManager) getServerConfig() serverConfig { return sm.serverConfigValue.Load().(serverConfig) } // NewServerManager is the only way to safely create a new ServerManager // struct. -// -// NOTE(sean@): We can not pass in *consul.Client due to an import cycle func NewServerManager(logger *log.Logger, shutdownCh chan struct{}) (sm *ServerManager) { + // NOTE(sean@): Can't pass *consul.Client due to an import cycle sm = new(ServerManager) - // Create the initial serverConfig - serverCfg := serverConfig{} sm.logger = logger + sm.consulServersCh = make(chan consulServerEventTypes, maxConsulServerManagerEvents) sm.shutdownCh = shutdownCh - sm.serverConfigValue.Store(serverCfg) + + sc := serverConfig{} + sc.servers = make([]*server_details.ServerDetails, 0) + sc.rebalanceTimer = time.NewTimer(time.Duration(initialRebalanceTimeoutHours * time.Hour)) + sm.serverConfigValue.Store(sc) return sm } @@ -245,7 +247,7 @@ func (sm *ServerManager) RebalanceServers() { defer sm.serverConfigLock.Unlock() serverCfg := sm.getServerConfig() - newServers := make([]*server_details.ServerDetails, len(serverCfg.servers)+1) + newServers := make([]*server_details.ServerDetails, len(serverCfg.servers)) copy(newServers, serverCfg.servers) // Shuffle the server list on server join. Servers are selected from From 74bcbc63f8b37e86faa2f3ae4c2a38073f792d35 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Mon, 22 Feb 2016 13:11:07 -0800 Subject: [PATCH 32/69] Use saveServerConfig vs atomic.Value.Store(config) --- consul/server_manager/server_manager.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index c13c0d7be5..dfe8939dee 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -129,7 +129,7 @@ func (sm *ServerManager) AddServer(server *server_details.ServerDetails) { sm.consulServersCh <- consulServersNodeJoin } - sm.serverConfigValue.Store(serverCfg) + sm.saveServerConfig(serverCfg) } // CycleFailedServers takes out an internal write lock and dequeues all @@ -151,7 +151,7 @@ func (sm *ServerManager) CycleFailedServers() { } serverCfg.resetRebalanceTimer(sm) - sm.serverConfigValue.Store(serverCfg) + sm.saveServerConfig(serverCfg) } // cycleServers returns a new list of servers that has dequeued the first @@ -260,7 +260,7 @@ func (sm *ServerManager) RebalanceServers() { serverCfg.servers = newServers serverCfg.resetRebalanceTimer(sm) - sm.serverConfigValue.Store(serverCfg) + sm.saveServerConfig(serverCfg) } // RemoveServer takes out an internal write lock and removes a server from @@ -285,7 +285,7 @@ func (sm *ServerManager) RemoveServer(server *server_details.ServerDetails) { newServers = newServers[:n-1] serverCfg.servers = newServers - sm.serverConfigValue.Store(serverCfg) + sm.saveServerConfig(serverCfg) return } } @@ -332,7 +332,7 @@ func (sm *ServerManager) StartServerManager() { serverCfg = serverCfgPtr.(serverConfig) serverCfg.resetRebalanceTimer(sm) rebalanceTimer = serverCfg.rebalanceTimer - sm.serverConfigValue.Store(serverCfg) + sm.saveServerConfig(serverCfg) }() for { From bf8c860663fff97bd643d63c1e9d0cbf8dd86e8c Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Tue, 23 Feb 2016 17:50:22 -0800 Subject: [PATCH 33/69] Update Serf to include `serf.NumNodes()` --- .../hashicorp/serf/coordinate/test_util.go | 27 ------------------- vendor/github.com/hashicorp/serf/serf/serf.go | 14 ++++++++-- 2 files changed, 12 insertions(+), 29 deletions(-) delete mode 100644 vendor/github.com/hashicorp/serf/coordinate/test_util.go diff --git a/vendor/github.com/hashicorp/serf/coordinate/test_util.go b/vendor/github.com/hashicorp/serf/coordinate/test_util.go deleted file mode 100644 index 116e949337..0000000000 --- a/vendor/github.com/hashicorp/serf/coordinate/test_util.go +++ /dev/null @@ -1,27 +0,0 @@ -package coordinate - -import ( - "math" - "testing" -) - -// verifyEqualFloats will compare f1 and f2 and fail if they are not -// "equal" within a threshold. -func verifyEqualFloats(t *testing.T, f1 float64, f2 float64) { - const zeroThreshold = 1.0e-6 - if math.Abs(f1-f2) > zeroThreshold { - t.Fatalf("equal assertion fail, %9.6f != %9.6f", f1, f2) - } -} - -// verifyEqualVectors will compare vec1 and vec2 and fail if they are not -// "equal" within a threshold. -func verifyEqualVectors(t *testing.T, vec1 []float64, vec2 []float64) { - if len(vec1) != len(vec2) { - t.Fatalf("vector length mismatch, %d != %d", len(vec1), len(vec2)) - } - - for i, _ := range vec1 { - verifyEqualFloats(t, vec1[i], vec2[i]) - } -} diff --git a/vendor/github.com/hashicorp/serf/serf/serf.go b/vendor/github.com/hashicorp/serf/serf/serf.go index 4f1921f775..613b915dc4 100644 --- a/vendor/github.com/hashicorp/serf/serf/serf.go +++ b/vendor/github.com/hashicorp/serf/serf/serf.go @@ -214,8 +214,8 @@ type queries struct { } const ( - UserEventSizeLimit = 512 // Maximum byte size for event name and payload - snapshotSizeLimit = 128 * 1024 // Maximum 128 KB snapshot + UserEventSizeLimit = 512 // Maximum byte size for event name and payload + snapshotSizeLimit = 128 * 1024 // Maximum 128 KB snapshot ) // Create creates a new Serf instance, starting all the background tasks @@ -1680,3 +1680,13 @@ func (s *Serf) GetCachedCoordinate(name string) (coord *coordinate.Coordinate, o return nil, false } + +// NumNodes returns the number of nodes in the serf cluster, regardless of +// their health or status. +func (s *Serf) NumNodes() (numNodes int) { + s.memberLock.RLock() + numNodes = len(s.members) + s.memberLock.RUnlock() + + return numNodes +} From bad6cb8897c8794b923b320a65549f3d8869de03 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 24 Feb 2016 10:38:37 -0800 Subject: [PATCH 34/69] Comment nits --- consul/server_manager/server_manager.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index dfe8939dee..cbdec6b5b4 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -89,7 +89,7 @@ type ServerManager struct { // shutdownCh is a copy of the channel in consul.Client shutdownCh chan struct{} - // Logger uses the provided LogOutput + // logger uses the provided LogOutput logger *log.Logger } @@ -134,8 +134,8 @@ func (sm *ServerManager) AddServer(server *server_details.ServerDetails) { // CycleFailedServers takes out an internal write lock and dequeues all // failed servers and re-enqueues them. This method does not reshuffle the -// server list. Because this changed the order of servers, we push out the -// time at which a rebalance occurs. +// server list, instead it requests the rebalance duration be refreshed/reset +// further into the future. func (sm *ServerManager) CycleFailedServers() { sm.serverConfigLock.Lock() defer sm.serverConfigLock.Unlock() From 0c87463b7e1b40a40c75b2cce1bfbe8f8d83e52c Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 24 Feb 2016 10:55:04 -0800 Subject: [PATCH 35/69] Introduce asynchronous management of consul server lists Instead of blocking the RPC call path and performing a potentially expensive calculation (including a call to `c.LANMembers()`), introduce a channel to request a rebalance. Some events don't force a reshuffle, instead the extend the duration of the current rebalance window because the environment thrashed enough to redistribute a client's load. --- consul/client.go | 2 +- consul/client_test.go | 9 +- consul/server_manager/server_manager.go | 114 ++++++++++--- .../server_manager_internal_test.go | 156 ++++++++++++++++++ consul/server_manager/server_manager_test.go | 75 +++++++++ 5 files changed, 327 insertions(+), 29 deletions(-) create mode 100644 consul/server_manager/server_manager_internal_test.go create mode 100644 consul/server_manager/server_manager_test.go diff --git a/consul/client.go b/consul/client.go index a5a4e5e11d..bf0b67ec63 100644 --- a/consul/client.go +++ b/consul/client.go @@ -118,7 +118,7 @@ func NewClient(config *Config) (*Client, error) { shutdownCh: make(chan struct{}), } - c.serverMgr = server_manager.NewServerManager(c.logger, c.shutdownCh) + c.serverMgr = server_manager.NewServerManager(c.logger, c.shutdownCh, c.serf) // Start consulServers maintenance go c.serverMgr.StartServerManager() diff --git a/consul/client_test.go b/consul/client_test.go index 59403dab84..2dec18d4c4 100644 --- a/consul/client_test.go +++ b/consul/client_test.go @@ -83,6 +83,12 @@ func TestClient_JoinLAN(t *testing.T) { if _, err := c1.JoinLAN([]string{addr}); err != nil { t.Fatalf("err: %v", err) } + numServers := c1.serverMgr.GetNumServers() + testutil.WaitForResult(func() (bool, error) { + return numServers == 1, nil + }, func(err error) { + t.Fatalf("expected consul server: %d", numServers) + }) // Check the members testutil.WaitForResult(func() (bool, error) { @@ -93,9 +99,10 @@ func TestClient_JoinLAN(t *testing.T) { t.Fatalf("bad len") }) + numServers = c1.serverMgr.GetNumServers() // Check we have a new consul testutil.WaitForResult(func() (bool, error) { - return c1.serverMgr.GetNumServers() == 1, nil + return numServers == 1, nil }, func(err error) { t.Fatalf("expected consul server") }) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index cbdec6b5b4..b287f8189e 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -9,6 +9,7 @@ import ( "github.com/hashicorp/consul/consul/server_details" "github.com/hashicorp/consul/lib" + "github.com/hashicorp/serf/serf" ) type consulServerEventTypes int @@ -23,6 +24,11 @@ const ( // connection load across servers consulServersRebalance + // consulServersRefreshRebalanceDuration is used to signal when we + // should reset the rebalance duration because the server list has + // changed and we don't need to proactively change our connection + consulServersRefreshRebalanceDuration + // consulServersRPCError is used to signal when a server has either // timed out or returned an error and we would like to have the // server manager find a new preferredServer. @@ -47,6 +53,11 @@ const ( // queries are sent over an established connection to a single server clientRPCMinReuseDuration = 120 * time.Second + // initialRebalanceTimeoutHours is the initial value for the + // rebalanceTimer. This value is discarded immediately after the + // client becomes aware of the first server. + initialRebalanceTimeoutHours = 24 + // Limit the number of new connections a server receives per second // for connection rebalancing. This limit caps the load caused by // continual rebalancing efforts when a cluster is in equilibrium. A @@ -61,6 +72,14 @@ const ( // will take ~26min for all servers to rebalance. A 10K cluster in // the same scenario will take ~2.6min to rebalance. newRebalanceConnsPerSecPerServer = 64 + + // maxConsulServerManagerEvents is the size of the consulServersCh + // buffer. + maxConsulServerManagerEvents = 16 + + // defaultClusterSize is the assumed cluster size if no serf cluster + // is available. + defaultClusterSize = 1024 ) // serverCfg is the thread-safe configuration structure that is used to @@ -71,9 +90,6 @@ const ( type serverConfig struct { // servers tracks the locally known servers servers []*server_details.ServerDetails - - // Timer used to control rebalancing of servers - rebalanceTimer *time.Timer } type ServerManager struct { @@ -86,11 +102,20 @@ type ServerManager struct { // maintenance of the list of consulServers consulServersCh chan consulServerEventTypes + // refreshRebalanceDurationCh is used to signal that a refresh should + // occur + refreshRebalanceDurationCh chan bool + // shutdownCh is a copy of the channel in consul.Client shutdownCh chan struct{} // logger uses the provided LogOutput logger *log.Logger + + // serf is used to estimate the approximate number of nodes in a + // cluster and limit the rate at which it rebalances server + // connections + serf *serf.Serf } // AddServer takes out an internal write lock and adds a new server. If the @@ -150,8 +175,8 @@ func (sm *ServerManager) CycleFailedServers() { } } - serverCfg.resetRebalanceTimer(sm) sm.saveServerConfig(serverCfg) + sm.requestRefreshRebalanceDuration() } // cycleServers returns a new list of servers that has dequeued the first @@ -211,16 +236,18 @@ func (sm *ServerManager) getServerConfig() serverConfig { // NewServerManager is the only way to safely create a new ServerManager // struct. -func NewServerManager(logger *log.Logger, shutdownCh chan struct{}) (sm *ServerManager) { +func NewServerManager(logger *log.Logger, shutdownCh chan struct{}, serf *serf.Serf) (sm *ServerManager) { // NOTE(sean@): Can't pass *consul.Client due to an import cycle sm = new(ServerManager) sm.logger = logger + sm.serf = serf sm.consulServersCh = make(chan consulServerEventTypes, maxConsulServerManagerEvents) sm.shutdownCh = shutdownCh + sm.refreshRebalanceDurationCh = make(chan bool, maxConsulServerManagerEvents) + sc := serverConfig{} sc.servers = make([]*server_details.ServerDetails, 0) - sc.rebalanceTimer = time.NewTimer(time.Duration(initialRebalanceTimeoutHours * time.Hour)) sm.serverConfigValue.Store(sc) return sm } @@ -259,8 +286,8 @@ func (sm *ServerManager) RebalanceServers() { } serverCfg.servers = newServers - serverCfg.resetRebalanceTimer(sm) sm.saveServerConfig(serverCfg) + sm.requestRefreshRebalanceDuration() } // RemoveServer takes out an internal write lock and removes a server from @@ -291,27 +318,45 @@ func (sm *ServerManager) RemoveServer(server *server_details.ServerDetails) { } } -// resetRebalanceTimer assumes: -// -// 1) the serverConfigLock is already held by the caller. -// 2) the caller will call serverConfigValue.Store() -func (sc *serverConfig) resetRebalanceTimer(sm *ServerManager) { - numConsulServers := len(sc.servers) +// requestRefreshRebalanceDuration sends a message to which causes a background +// thread to recalc the duration +func (sm *ServerManager) requestRefreshRebalanceDuration() { + sm.refreshRebalanceDurationCh <- true +} + +// requestServerRebalance sends a message to which causes a background thread +// to reshuffle the list of servers +func (sm *ServerManager) requestServerRebalance() { + sm.consulServersCh <- consulServersRebalance +} + +// refreshServerRebalanceTimer is called +func (sm *ServerManager) refreshServerRebalanceTimer(timer *time.Timer) { + serverCfg := sm.getServerConfig() + numConsulServers := len(serverCfg.servers) // Limit this connection's life based on the size (and health) of the // cluster. Never rebalance a connection more frequently than // connReuseLowWatermarkDuration, and make sure we never exceed // clusterWideRebalanceConnsPerSec operations/s across numLANMembers. clusterWideRebalanceConnsPerSec := float64(numConsulServers * newRebalanceConnsPerSecPerServer) connReuseLowWatermarkDuration := clientRPCMinReuseDuration + lib.RandomStagger(clientRPCMinReuseDuration/clientRPCJitterFraction) - numLANMembers := 16384 // Assume sufficiently large for now. FIXME: numLanMembers := len(c.LANMembers()) + + // Assume a moderate sized cluster unless we have an actual serf + // instance we can query. + numLANMembers := defaultClusterSize + if sm.serf != nil { + numLANMembers = sm.serf.NumNodes() + } connRebalanceTimeout := lib.RateScaledInterval(clusterWideRebalanceConnsPerSec, connReuseLowWatermarkDuration, numLANMembers) sm.logger.Printf("[DEBUG] consul: connection will be rebalanced in %v", connRebalanceTimeout) - if sc.rebalanceTimer == nil { - sc.rebalanceTimer = time.NewTimer(connRebalanceTimeout) - } else { - sc.rebalanceTimer.Reset(connRebalanceTimeout) - } + timer.Reset(connRebalanceTimeout) +} + +// saveServerConfig is a convenience method which hides the locking semantics +// of atomic.Value from the caller. +func (sm *ServerManager) saveServerConfig(sc serverConfig) { + sm.serverConfigValue.Store(sc) } // StartServerManager is used to start and manage the task of automatically @@ -319,7 +364,9 @@ func (sc *serverConfig) resetRebalanceTimer(sm *ServerManager) { // happens either when a new server is added or when a duration has been // exceed. func (sm *ServerManager) StartServerManager() { - var rebalanceTimer *time.Timer + var rebalanceTimer *time.Timer = time.NewTimer(time.Duration(initialRebalanceTimeoutHours * time.Hour)) + var rebalanceTaskDispatched int32 + func() { sm.serverConfigLock.Lock() defer sm.serverConfigLock.Unlock() @@ -330,8 +377,6 @@ func (sm *ServerManager) StartServerManager() { } var serverCfg serverConfig serverCfg = serverCfgPtr.(serverConfig) - serverCfg.resetRebalanceTimer(sm) - rebalanceTimer = serverCfg.rebalanceTimer sm.saveServerConfig(serverCfg) }() @@ -340,13 +385,14 @@ func (sm *ServerManager) StartServerManager() { case e := <-sm.consulServersCh: switch e { case consulServersNodeJoin: - sm.logger.Printf("[INFO] consul: new node joined cluster") - sm.RebalanceServers() + sm.logger.Printf("[INFO] server manager: new node joined cluster") + // rebalance on new server + sm.requestServerRebalance() case consulServersRebalance: - sm.logger.Printf("[INFO] consul: rebalancing servers by request") + sm.logger.Printf("[INFO] server manager: rebalancing servers by request") sm.RebalanceServers() case consulServersRPCError: - sm.logger.Printf("[INFO] consul: need to find a new server to talk with") + sm.logger.Printf("[INFO] server manager: need to find a new server to talk with") sm.CycleFailedServers() // FIXME(sean@): wtb preemptive Status.Ping // of servers, ideally parallel fan-out of N @@ -360,7 +406,21 @@ func (sm *ServerManager) StartServerManager() { // their RPC time too low even though the // Ping did return successfully? default: - sm.logger.Printf("[WARN] consul: unhandled LAN Serf Event: %#v", e) + sm.logger.Printf("[WARN] server manager: unhandled LAN Serf Event: %#v", e) + } + case <-sm.refreshRebalanceDurationCh: + chanLen := len(sm.refreshRebalanceDurationCh) + // Drain all messages from the rebalance channel + for i := 0; i < chanLen; i++ { + <-sm.refreshRebalanceDurationCh + } + // Only run one rebalance task at a time, but do + // allow for the channel to be drained + if atomic.CompareAndSwapInt32(&rebalanceTaskDispatched, 0, 1) { + go func() { + defer atomic.StoreInt32(&rebalanceTaskDispatched, 0) + sm.refreshServerRebalanceTimer(rebalanceTimer) + }() } case <-rebalanceTimer.C: sm.logger.Printf("[INFO] consul: server rebalance timeout") diff --git a/consul/server_manager/server_manager_internal_test.go b/consul/server_manager/server_manager_internal_test.go new file mode 100644 index 0000000000..614b56f7b0 --- /dev/null +++ b/consul/server_manager/server_manager_internal_test.go @@ -0,0 +1,156 @@ +package server_manager + +import ( + "bytes" + "log" + "testing" + + "github.com/hashicorp/consul/consul/server_details" +) + +var ( + localLogger *log.Logger + localLogBuffer *bytes.Buffer +) + +func init() { + localLogBuffer = new(bytes.Buffer) + localLogger = log.New(localLogBuffer, "", 0) +} + +func GetBufferedLogger() *log.Logger { + return localLogger +} + +func testServerManager() (sm *ServerManager) { + logger := GetBufferedLogger() + shutdownCh := make(chan struct{}) + sm = NewServerManager(logger, shutdownCh, nil) + return sm +} + +// func (sc *serverConfig) cycleServer() (servers []*server_details.ServerDetails) { +func TestServerManagerInternal_cycleServer(t *testing.T) { + sm := testServerManager() + sc := sm.getServerConfig() + + server0 := &server_details.ServerDetails{Name: "server1"} + server1 := &server_details.ServerDetails{Name: "server2"} + server2 := &server_details.ServerDetails{Name: "server3"} + sc.servers = append(sc.servers, server0, server1, server2) + sm.saveServerConfig(sc) + + sc = sm.getServerConfig() + if len(sc.servers) != 3 { + t.Fatalf("server length incorrect: %d/3", len(sc.servers)) + } + if sc.servers[0] != server0 && + sc.servers[1] != server1 && + sc.servers[2] != server2 { + t.Fatalf("initial server ordering not correct") + } + + sc.servers = sc.cycleServer() + if len(sc.servers) != 3 { + t.Fatalf("server length incorrect: %d/3", len(sc.servers)) + } + if sc.servers[0] != server1 && + sc.servers[1] != server2 && + sc.servers[2] != server0 { + t.Fatalf("server ordering after one cycle not correct") + } + + sc.servers = sc.cycleServer() + if len(sc.servers) != 3 { + t.Fatalf("server length incorrect: %d/3", len(sc.servers)) + } + if sc.servers[0] != server2 && + sc.servers[1] != server0 && + sc.servers[2] != server1 { + t.Fatalf("server ordering after two cycles not correct") + } + + sc.servers = sc.cycleServer() + if len(sc.servers) != 3 { + t.Fatalf("server length incorrect: %d/3", len(sc.servers)) + } + if sc.servers[0] != server0 && + sc.servers[1] != server1 && + sc.servers[2] != server2 { + t.Fatalf("server ordering after three cycles not correct") + } +} + +// func (sm *ServerManager) getServerConfig() serverConfig { +func TestServerManagerInternal_getServerConfig(t *testing.T) { + sm := testServerManager() + sc := sm.getServerConfig() + if sc.servers == nil { + t.Fatalf("serverConfig.servers nil") + } + + if len(sc.servers) != 0 { + t.Fatalf("serverConfig.servers length not zero") + } +} + +// func NewServerManager(logger *log.Logger, shutdownCh chan struct{}) (sm *ServerManager) { +func TestServerManagerInternal_NewServerManager(t *testing.T) { + sm := testServerManager() + if sm == nil { + t.Fatalf("ServerManager nil") + } + + if sm.logger == nil { + t.Fatalf("ServerManager.logger nil") + } + + if sm.consulServersCh == nil { + t.Fatalf("ServerManager.consulServersCh nil") + } + + if sm.shutdownCh == nil { + t.Fatalf("ServerManager.shutdownCh nil") + } +} + +// func (sc *serverConfig) resetRebalanceTimer(sm *ServerManager) { + +// func (sm *ServerManager) saveServerConfig(sc serverConfig) { +func TestServerManagerInternal_saveServerConfig(t *testing.T) { + sm := testServerManager() + + // Initial condition + func() { + sc := sm.getServerConfig() + if len(sc.servers) != 0 { + t.Fatalf("ServerManager.saveServerConfig failed to load init config") + } + + newServer := new(server_details.ServerDetails) + sc.servers = append(sc.servers, newServer) + sm.saveServerConfig(sc) + }() + + // Test that save works + func() { + sc1 := sm.getServerConfig() + t1NumServers := len(sc1.servers) + if t1NumServers != 1 { + t.Fatalf("ServerManager.saveServerConfig failed to save mutated config") + } + }() + + // Verify mutation w/o a save doesn't alter the original + func() { + newServer := new(server_details.ServerDetails) + sc := sm.getServerConfig() + sc.servers = append(sc.servers, newServer) + + sc_orig := sm.getServerConfig() + origNumServers := len(sc_orig.servers) + if origNumServers >= len(sc.servers) { + t.Fatalf("ServerManager.saveServerConfig unsaved config overwrote original") + } + }() +} diff --git a/consul/server_manager/server_manager_test.go b/consul/server_manager/server_manager_test.go new file mode 100644 index 0000000000..46efd0083b --- /dev/null +++ b/consul/server_manager/server_manager_test.go @@ -0,0 +1,75 @@ +package server_manager_test + +import ( + "bytes" + "log" + "testing" + + "github.com/hashicorp/consul/consul/server_details" + "github.com/hashicorp/consul/consul/server_manager" +) + +var ( + localLogger *log.Logger + localLogBuffer *bytes.Buffer +) + +func init() { + localLogBuffer = new(bytes.Buffer) + localLogger = log.New(localLogBuffer, "", 0) +} + +func GetBufferedLogger() *log.Logger { + return localLogger +} + +func makeMockServerManager() (sm *server_manager.ServerManager) { + logger, shutdownCh := mockServerManager() + sm = server_manager.NewServerManager(logger, shutdownCh, nil) + return sm +} + +func mockServerManager() (logger *log.Logger, shutdownCh chan struct{}) { + logger = GetBufferedLogger() + shutdownCh = make(chan struct{}) + return logger, shutdownCh +} + +// func (sm *ServerManager) AddServer(server *server_details.ServerDetails) { + +// func (sm *ServerManager) CycleFailedServers() { + +// func (sm *ServerManager) FindHealthyServer() (server *server_details.ServerDetails) { + +// func (sm *ServerManager) GetNumServers() (numServers int) { +func TestServerManager_GetNumServers(t *testing.T) { + sm := makeMockServerManager() + var num int + num = sm.GetNumServers() + if num != 0 { + t.Fatalf("Expected zero servers to start") + } + + s := &server_details.ServerDetails{} + sm.AddServer(s) + num = sm.GetNumServers() + if num != 1 { + t.Fatalf("Expected one server after AddServer") + } +} + +// func NewServerManager(logger *log.Logger, shutdownCh chan struct{}) (sm *ServerManager) { +func TestServerManager_NewServerManager(t *testing.T) { + sm := makeMockServerManager() + if sm == nil { + t.Fatalf("ServerManager nil") + } +} + +// func (sm *ServerManager) NotifyFailedServer(server *server_details.ServerDetails) { + +// func (sm *ServerManager) RebalanceServers() { + +// func (sm *ServerManager) RemoveServer(server *server_details.ServerDetails) { + +// func (sm *ServerManager) StartServerManager() { From c2c73bfeab035765bb8ed632563555bb5759de41 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 24 Feb 2016 11:54:16 -0800 Subject: [PATCH 36/69] Unbreak client tests by reverting to original test Debugging code crept into the actual test and hung out for much longer than it should have. --- consul/client_test.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/consul/client_test.go b/consul/client_test.go index 2dec18d4c4..1249800949 100644 --- a/consul/client_test.go +++ b/consul/client_test.go @@ -83,11 +83,10 @@ func TestClient_JoinLAN(t *testing.T) { if _, err := c1.JoinLAN([]string{addr}); err != nil { t.Fatalf("err: %v", err) } - numServers := c1.serverMgr.GetNumServers() testutil.WaitForResult(func() (bool, error) { - return numServers == 1, nil + return c1.serverMgr.GetNumServers() == 1, nil }, func(err error) { - t.Fatalf("expected consul server: %d", numServers) + t.Fatalf("expected consul server") }) // Check the members @@ -99,10 +98,9 @@ func TestClient_JoinLAN(t *testing.T) { t.Fatalf("bad len") }) - numServers = c1.serverMgr.GetNumServers() // Check we have a new consul testutil.WaitForResult(func() (bool, error) { - return numServers == 1, nil + return c1.serverMgr.GetNumServers() == 1, nil }, func(err error) { t.Fatalf("expected consul server") }) From fdbb142c3f4a3b5e7cabf2444fe660fce22bafb3 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 24 Feb 2016 14:48:04 -0800 Subject: [PATCH 37/69] Simplify error handling Rely on Serf for liveliness. In the event of a failure, simply cycle the server to the end of the list. If the server is unhealthy, Serf will reap the dead server. Additional simplifications: *) Only rebalance servers based on timers, not when a new server is readded to the cluster. *) Back out the failure count in server_details.ServerDetails --- consul/client.go | 2 +- consul/server_details/server_details.go | 11 -- consul/server_details/server_details_test.go | 6 - consul/server_manager/server_manager.go | 146 ++++--------------- consul/server_manager/server_manager_test.go | 2 +- 5 files changed, 34 insertions(+), 133 deletions(-) diff --git a/consul/client.go b/consul/client.go index bf0b67ec63..bc44cbffa7 100644 --- a/consul/client.go +++ b/consul/client.go @@ -121,7 +121,7 @@ func NewClient(config *Config) (*Client, error) { c.serverMgr = server_manager.NewServerManager(c.logger, c.shutdownCh, c.serf) // Start consulServers maintenance - go c.serverMgr.StartServerManager() + go c.serverMgr.Start() // Start the Serf listeners to prevent a deadlock go c.lanEventHandler() diff --git a/consul/server_details/server_details.go b/consul/server_details/server_details.go index 9b2addc189..e9386e8ca3 100644 --- a/consul/server_details/server_details.go +++ b/consul/server_details/server_details.go @@ -17,11 +17,6 @@ type ServerDetails struct { Expect int Version int Addr net.Addr - - // Disabled is a uint64 in order to support atomic integer - // operations. Zero means enabled, non-zero is the number of times - // this server has failed without being marked healthy. - Disabled uint64 } func (s *ServerDetails) String() string { @@ -37,11 +32,6 @@ func IsConsulServer(m serf.Member) (bool, *ServerDetails) { datacenter := m.Tags["dc"] _, bootstrap := m.Tags["bootstrap"] - var disabled uint64 = 0 - _, disabledStr := m.Tags["disabled"] - if disabledStr { - disabled = 1 - } expect := 0 expect_str, ok := m.Tags["expect"] @@ -75,7 +65,6 @@ func IsConsulServer(m serf.Member) (bool, *ServerDetails) { Expect: expect, Addr: addr, Version: vsn, - Disabled: disabled, } return true, parts } diff --git a/consul/server_details/server_details_test.go b/consul/server_details/server_details_test.go index ea3bef0365..ac24b38450 100644 --- a/consul/server_details/server_details_test.go +++ b/consul/server_details/server_details_test.go @@ -29,9 +29,6 @@ func TestIsConsulServer(t *testing.T) { if parts.Bootstrap { t.Fatalf("unexpected bootstrap") } - if parts.Disabled > 0 { - t.Fatalf("unexpected disabled") - } if parts.Expect != 0 { t.Fatalf("bad: %v", parts.Expect) } @@ -44,9 +41,6 @@ func TestIsConsulServer(t *testing.T) { if !parts.Bootstrap { t.Fatalf("expected bootstrap") } - if parts.Disabled == 0 { - t.Fatalf("expected disabled") - } if parts.Addr.String() != "127.0.0.1:10000" { t.Fatalf("bad addr: %v", parts.Addr) } diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index b287f8189e..819acd73ff 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -14,27 +14,6 @@ import ( type consulServerEventTypes int -const ( - // consulServersNodeJoin is used to notify of a new consulServer. - // The primary effect of this is a reshuffling of consulServers and - // finding a new preferredServer. - consulServersNodeJoin = iota - - // consulServersRebalance is used to signal we should rebalance our - // connection load across servers - consulServersRebalance - - // consulServersRefreshRebalanceDuration is used to signal when we - // should reset the rebalance duration because the server list has - // changed and we don't need to proactively change our connection - consulServersRefreshRebalanceDuration - - // consulServersRPCError is used to signal when a server has either - // timed out or returned an error and we would like to have the - // server manager find a new preferredServer. - consulServersRPCError -) - const ( // clientRPCJitterFraction determines the amount of jitter added to // clientRPCMinReuseDuration before a connection is expired and a new @@ -149,36 +128,11 @@ func (sm *ServerManager) AddServer(server *server_details.ServerDetails) { copy(newServers, serverCfg.servers) newServers = append(newServers, server) serverCfg.servers = newServers - - // Notify the server maintenance task of a new server - sm.consulServersCh <- consulServersNodeJoin } sm.saveServerConfig(serverCfg) } -// CycleFailedServers takes out an internal write lock and dequeues all -// failed servers and re-enqueues them. This method does not reshuffle the -// server list, instead it requests the rebalance duration be refreshed/reset -// further into the future. -func (sm *ServerManager) CycleFailedServers() { - sm.serverConfigLock.Lock() - defer sm.serverConfigLock.Unlock() - serverCfg := sm.getServerConfig() - - for i := range serverCfg.servers { - failCount := atomic.LoadUint64(&(serverCfg.servers[i].Disabled)) - if failCount == 0 { - break - } else if failCount > 0 { - serverCfg.servers = serverCfg.cycleServer() - } - } - - sm.saveServerConfig(serverCfg) - sm.requestRefreshRebalanceDuration() -} - // cycleServers returns a new list of servers that has dequeued the first // server and enqueued it at the end of the list. cycleServers assumes the // caller is holding the serverConfigLock. @@ -197,27 +151,16 @@ func (sc *serverConfig) cycleServer() (servers []*server_details.ServerDetails) // FindHealthyServer takes out an internal "read lock" and searches through // the list of servers to find a healthy server. -func (sm *ServerManager) FindHealthyServer() (server *server_details.ServerDetails) { +func (sm *ServerManager) FindHealthyServer() *server_details.ServerDetails { serverCfg := sm.getServerConfig() numServers := len(serverCfg.servers) if numServers == 0 { sm.logger.Printf("[ERR] consul: No servers found in the server config") return nil + } else { + // Return whatever is at the front of the list + return serverCfg.servers[0] } - - // Find the first non-failing server in the server list. If this is - // not the first server a prior RPC call marked the first server as - // failed and we're waiting for the server management task to reorder - // a working server to the front of the list. - for i := range serverCfg.servers { - failCount := atomic.LoadUint64(&(serverCfg.servers[i].Disabled)) - if failCount == 0 { - server = serverCfg.servers[i] - break - } - } - - return server } // GetNumServers takes out an internal "read lock" and returns the number of @@ -254,13 +197,25 @@ func NewServerManager(logger *log.Logger, shutdownCh chan struct{}, serf *serf.S // NotifyFailedServer is an exported convenience function that allows callers // to pass in a server that has failed an RPC request and mark it as failed. -// This will initiate a background task that will optimize the failed server -// to the end of the serer list. No locks are required here because we are -// bypassing the serverConfig and sending a message to ServerManager's -// channel. +// If the server being failed is not the first server on the list, this is a +// noop. If, however, the server is failed and first on the list, acquire +// the lock, retest, and take the penalty of moving the server to the end of +// the list. func (sm *ServerManager) NotifyFailedServer(server *server_details.ServerDetails) { - atomic.AddUint64(&server.Disabled, 1) - sm.consulServersCh <- consulServersRPCError + serverCfg := sm.getServerConfig() + + if len(serverCfg.servers) > 0 && serverCfg.servers[0] == server { + // Grab a lock, retest, and take the hit of cycling the first + // server to the end. + sm.serverConfigLock.Lock() // FIXME(sean@): wtb TryLock + defer sm.serverConfigLock.Unlock() + serverCfg = sm.getServerConfig() + + if len(serverCfg.servers) > 0 && serverCfg.servers[0] == server { + serverCfg.cycleServer() + sm.saveServerConfig(serverCfg) + } + } } // RebalanceServers takes out an internal write lock and shuffles the list of @@ -287,7 +242,6 @@ func (sm *ServerManager) RebalanceServers() { serverCfg.servers = newServers sm.saveServerConfig(serverCfg) - sm.requestRefreshRebalanceDuration() } // RemoveServer takes out an internal write lock and removes a server from @@ -324,12 +278,6 @@ func (sm *ServerManager) requestRefreshRebalanceDuration() { sm.refreshRebalanceDurationCh <- true } -// requestServerRebalance sends a message to which causes a background thread -// to reshuffle the list of servers -func (sm *ServerManager) requestServerRebalance() { - sm.consulServersCh <- consulServersRebalance -} - // refreshServerRebalanceTimer is called func (sm *ServerManager) refreshServerRebalanceTimer(timer *time.Timer) { serverCfg := sm.getServerConfig() @@ -359,11 +307,10 @@ func (sm *ServerManager) saveServerConfig(sc serverConfig) { sm.serverConfigValue.Store(sc) } -// StartServerManager is used to start and manage the task of automatically -// shuffling and rebalance the list of consul servers. This maintenance -// happens either when a new server is added or when a duration has been -// exceed. -func (sm *ServerManager) StartServerManager() { +// Start is used to start and manage the task of automatically shuffling and +// rebalance the list of consul servers. This maintenance happens either +// when a new server is added or when a duration has been exceed. +func (sm *ServerManager) Start() { var rebalanceTimer *time.Timer = time.NewTimer(time.Duration(initialRebalanceTimeoutHours * time.Hour)) var rebalanceTaskDispatched int32 @@ -382,51 +329,22 @@ func (sm *ServerManager) StartServerManager() { for { select { - case e := <-sm.consulServersCh: - switch e { - case consulServersNodeJoin: - sm.logger.Printf("[INFO] server manager: new node joined cluster") - // rebalance on new server - sm.requestServerRebalance() - case consulServersRebalance: - sm.logger.Printf("[INFO] server manager: rebalancing servers by request") - sm.RebalanceServers() - case consulServersRPCError: - sm.logger.Printf("[INFO] server manager: need to find a new server to talk with") - sm.CycleFailedServers() - // FIXME(sean@): wtb preemptive Status.Ping - // of servers, ideally parallel fan-out of N - // nodes, then settle on the first node which - // responds successfully. - // - // Is there a distinction between slow and - // offline? Do we run the Status.Ping with a - // fixed timeout (say 30s) that way we can - // alert administrators that they've set - // their RPC time too low even though the - // Ping did return successfully? - default: - sm.logger.Printf("[WARN] server manager: unhandled LAN Serf Event: %#v", e) - } - case <-sm.refreshRebalanceDurationCh: - chanLen := len(sm.refreshRebalanceDurationCh) - // Drain all messages from the rebalance channel - for i := 0; i < chanLen; i++ { - <-sm.refreshRebalanceDurationCh - } + case <-rebalanceTimer.C: + sm.logger.Printf("[INFO] server manager: server rebalance timeout") + sm.RebalanceServers() + // Only run one rebalance task at a time, but do // allow for the channel to be drained if atomic.CompareAndSwapInt32(&rebalanceTaskDispatched, 0, 1) { + sm.logger.Printf("[INFO] server manager: Launching rebalance duration task") go func() { defer atomic.StoreInt32(&rebalanceTaskDispatched, 0) sm.refreshServerRebalanceTimer(rebalanceTimer) }() } - case <-rebalanceTimer.C: - sm.logger.Printf("[INFO] consul: server rebalance timeout") - sm.RebalanceServers() case <-sm.shutdownCh: + sm.logger.Printf("[INFO] server manager: shutting down") return } } diff --git a/consul/server_manager/server_manager_test.go b/consul/server_manager/server_manager_test.go index 46efd0083b..5c83701ad1 100644 --- a/consul/server_manager/server_manager_test.go +++ b/consul/server_manager/server_manager_test.go @@ -72,4 +72,4 @@ func TestServerManager_NewServerManager(t *testing.T) { // func (sm *ServerManager) RemoveServer(server *server_details.ServerDetails) { -// func (sm *ServerManager) StartServerManager() { +// func (sm *ServerManager) Start() { From 295af0168088cec4465ef4beeabc0141b405ea28 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 24 Feb 2016 15:04:04 -0800 Subject: [PATCH 38/69] Make use of interfaces Use an interface instead of serf.Serf as arg to NewServerManager. Bonus points for improved testability. Pointed out by: @slackpad --- consul/server_manager/server_manager.go | 18 ++++++++---------- .../server_manager_internal_test.go | 9 ++++++++- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index 819acd73ff..c3c9051d8f 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -9,7 +9,6 @@ import ( "github.com/hashicorp/consul/consul/server_details" "github.com/hashicorp/consul/lib" - "github.com/hashicorp/serf/serf" ) type consulServerEventTypes int @@ -61,6 +60,10 @@ const ( defaultClusterSize = 1024 ) +type ConsulClusterInfo interface { + NumNodes() int +} + // serverCfg is the thread-safe configuration structure that is used to // maintain the list of consul servers in Client. // @@ -94,7 +97,7 @@ type ServerManager struct { // serf is used to estimate the approximate number of nodes in a // cluster and limit the rate at which it rebalances server // connections - serf *serf.Serf + clusterInfo ConsulClusterInfo } // AddServer takes out an internal write lock and adds a new server. If the @@ -179,11 +182,11 @@ func (sm *ServerManager) getServerConfig() serverConfig { // NewServerManager is the only way to safely create a new ServerManager // struct. -func NewServerManager(logger *log.Logger, shutdownCh chan struct{}, serf *serf.Serf) (sm *ServerManager) { +func NewServerManager(logger *log.Logger, shutdownCh chan struct{}, cci ConsulClusterInfo) (sm *ServerManager) { // NOTE(sean@): Can't pass *consul.Client due to an import cycle sm = new(ServerManager) sm.logger = logger - sm.serf = serf + sm.clusterInfo = cci sm.consulServersCh = make(chan consulServerEventTypes, maxConsulServerManagerEvents) sm.shutdownCh = shutdownCh @@ -289,12 +292,7 @@ func (sm *ServerManager) refreshServerRebalanceTimer(timer *time.Timer) { clusterWideRebalanceConnsPerSec := float64(numConsulServers * newRebalanceConnsPerSecPerServer) connReuseLowWatermarkDuration := clientRPCMinReuseDuration + lib.RandomStagger(clientRPCMinReuseDuration/clientRPCJitterFraction) - // Assume a moderate sized cluster unless we have an actual serf - // instance we can query. - numLANMembers := defaultClusterSize - if sm.serf != nil { - numLANMembers = sm.serf.NumNodes() - } + numLANMembers := sm.clusterInfo.NumNodes() connRebalanceTimeout := lib.RateScaledInterval(clusterWideRebalanceConnsPerSec, connReuseLowWatermarkDuration, numLANMembers) sm.logger.Printf("[DEBUG] consul: connection will be rebalanced in %v", connRebalanceTimeout) diff --git a/consul/server_manager/server_manager_internal_test.go b/consul/server_manager/server_manager_internal_test.go index 614b56f7b0..75be1efd33 100644 --- a/consul/server_manager/server_manager_internal_test.go +++ b/consul/server_manager/server_manager_internal_test.go @@ -22,10 +22,17 @@ func GetBufferedLogger() *log.Logger { return localLogger } +type fauxSerf struct { +} + +func (s *fauxSerf) NumNodes() int { + return 16384 +} + func testServerManager() (sm *ServerManager) { logger := GetBufferedLogger() shutdownCh := make(chan struct{}) - sm = NewServerManager(logger, shutdownCh, nil) + sm = NewServerManager(logger, shutdownCh, &fauxSerf{}) return sm } From d13e3c18c9b513c6854ede5b4f3dd300281ecc8a Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 24 Feb 2016 19:11:16 -0800 Subject: [PATCH 39/69] Emulate a TryLock using atomic.CompareAndSwap Prevent possible queueing behind serverConfigLock in the event that a server fails on a busy host. --- consul/server_manager/server_manager.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index c3c9051d8f..ac25cb6d35 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -98,6 +98,10 @@ type ServerManager struct { // cluster and limit the rate at which it rebalances server // connections clusterInfo ConsulClusterInfo + + // notifyFailedServersBarrier is acts as a barrier to prevent + // queueing behind serverConfigLog and acts as a TryLock(). + notifyFailedBarrier int32 } // AddServer takes out an internal write lock and adds a new server. If the @@ -207,10 +211,14 @@ func NewServerManager(logger *log.Logger, shutdownCh chan struct{}, cci ConsulCl func (sm *ServerManager) NotifyFailedServer(server *server_details.ServerDetails) { serverCfg := sm.getServerConfig() - if len(serverCfg.servers) > 0 && serverCfg.servers[0] == server { + // Use atomic.CAS to emulate a TryLock(). + if len(serverCfg.servers) > 0 && serverCfg.servers[0] == server && + atomic.CompareAndSwapInt32(&sm.notifyFailedBarrier, 0, 1) { + defer atomic.StoreInt32(&sm.notifyFailedBarrier, 0) + // Grab a lock, retest, and take the hit of cycling the first // server to the end. - sm.serverConfigLock.Lock() // FIXME(sean@): wtb TryLock + sm.serverConfigLock.Lock() defer sm.serverConfigLock.Unlock() serverCfg = sm.getServerConfig() From d2d55f4bb0e0787a334cc9c067d2e7df86d7f1a8 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 24 Feb 2016 19:51:21 -0800 Subject: [PATCH 40/69] Remove additional cruft from ServerManager's channels No longer needed code. --- consul/server_manager/server_manager.go | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index ac25cb6d35..14a99ec783 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -50,14 +50,6 @@ const ( // will take ~26min for all servers to rebalance. A 10K cluster in // the same scenario will take ~2.6min to rebalance. newRebalanceConnsPerSecPerServer = 64 - - // maxConsulServerManagerEvents is the size of the consulServersCh - // buffer. - maxConsulServerManagerEvents = 16 - - // defaultClusterSize is the assumed cluster size if no serf cluster - // is available. - defaultClusterSize = 1024 ) type ConsulClusterInfo interface { @@ -80,14 +72,6 @@ type ServerManager struct { serverConfigValue atomic.Value serverConfigLock sync.Mutex - // consulServersCh is used to receive events related to the - // maintenance of the list of consulServers - consulServersCh chan consulServerEventTypes - - // refreshRebalanceDurationCh is used to signal that a refresh should - // occur - refreshRebalanceDurationCh chan bool - // shutdownCh is a copy of the channel in consul.Client shutdownCh chan struct{} @@ -191,11 +175,8 @@ func NewServerManager(logger *log.Logger, shutdownCh chan struct{}, cci ConsulCl sm = new(ServerManager) sm.logger = logger sm.clusterInfo = cci - sm.consulServersCh = make(chan consulServerEventTypes, maxConsulServerManagerEvents) sm.shutdownCh = shutdownCh - sm.refreshRebalanceDurationCh = make(chan bool, maxConsulServerManagerEvents) - sc := serverConfig{} sc.servers = make([]*server_details.ServerDetails, 0) sm.serverConfigValue.Store(sc) @@ -283,12 +264,6 @@ func (sm *ServerManager) RemoveServer(server *server_details.ServerDetails) { } } -// requestRefreshRebalanceDuration sends a message to which causes a background -// thread to recalc the duration -func (sm *ServerManager) requestRefreshRebalanceDuration() { - sm.refreshRebalanceDurationCh <- true -} - // refreshServerRebalanceTimer is called func (sm *ServerManager) refreshServerRebalanceTimer(timer *time.Timer) { serverCfg := sm.getServerConfig() From bc62de541ccf55a18e986eb26f50cd892e6664c1 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 24 Feb 2016 19:51:56 -0800 Subject: [PATCH 41/69] Update comments to reflect reality --- consul/server_manager/server_manager.go | 78 ++++++++++++++----------- 1 file changed, 45 insertions(+), 33 deletions(-) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index 14a99ec783..ee3b09b4f8 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -59,23 +59,23 @@ type ConsulClusterInfo interface { // serverCfg is the thread-safe configuration structure that is used to // maintain the list of consul servers in Client. // -// NOTE(sean@): We are explicitly relying on the fact that this is copied. -// Please keep this structure light. +// NOTE(sean@): We are explicitly relying on the fact that serverConfig will +// be copied onto the stack. Please keep this structure light. type serverConfig struct { - // servers tracks the locally known servers + // servers tracks the locally known servers. List membership is + // maintained by Serf. servers []*server_details.ServerDetails } type ServerManager struct { - // serverConfig provides the necessary load/store semantics to - // serverConfig + // serverConfig provides the necessary load/store semantics for the + // server list. serverConfigValue atomic.Value serverConfigLock sync.Mutex // shutdownCh is a copy of the channel in consul.Client shutdownCh chan struct{} - // logger uses the provided LogOutput logger *log.Logger // serf is used to estimate the approximate number of nodes in a @@ -89,8 +89,10 @@ type ServerManager struct { } // AddServer takes out an internal write lock and adds a new server. If the -// server is not known, it adds the new server and schedules a rebalance. If -// it is known, we merge the new server details. +// server is not known, appends the server to the list. The new server will +// begin seeing use after the rebalance timer fires or enough servers fail +// organically. If the server is already known, merge the new server +// details. func (sm *ServerManager) AddServer(server *server_details.ServerDetails) { sm.serverConfigLock.Lock() defer sm.serverConfigLock.Unlock() @@ -130,8 +132,7 @@ func (sm *ServerManager) AddServer(server *server_details.ServerDetails) { func (sc *serverConfig) cycleServer() (servers []*server_details.ServerDetails) { numServers := len(sc.servers) if numServers < 2 { - // No action required - return servers + return servers // No action required } newServers := make([]*server_details.ServerDetails, 0, numServers) @@ -141,7 +142,11 @@ func (sc *serverConfig) cycleServer() (servers []*server_details.ServerDetails) } // FindHealthyServer takes out an internal "read lock" and searches through -// the list of servers to find a healthy server. +// the list of servers to find a "healthy" server. If the server is actually +// unhealthy, we rely on Serf to detect this and remove the node from the +// server list. If the server at the front of the list has failed or fails +// during an RPC call, it is rotated to the end of the list. If there are no +// servers available, return nil. func (sm *ServerManager) FindHealthyServer() *server_details.ServerDetails { serverCfg := sm.getServerConfig() numServers := len(serverCfg.servers) @@ -149,7 +154,10 @@ func (sm *ServerManager) FindHealthyServer() *server_details.ServerDetails { sm.logger.Printf("[ERR] consul: No servers found in the server config") return nil } else { - // Return whatever is at the front of the list + // Return whatever is at the front of the list because it is + // assumed to be the oldest in the server list (unless - + // hypothetically - the server list was rotated right after a + // server was added). return serverCfg.servers[0] } } @@ -170,28 +178,29 @@ func (sm *ServerManager) getServerConfig() serverConfig { // NewServerManager is the only way to safely create a new ServerManager // struct. -func NewServerManager(logger *log.Logger, shutdownCh chan struct{}, cci ConsulClusterInfo) (sm *ServerManager) { +func NewServerManager(logger *log.Logger, shutdownCh chan struct{}, clusterInfo ConsulClusterInfo) (sm *ServerManager) { // NOTE(sean@): Can't pass *consul.Client due to an import cycle sm = new(ServerManager) sm.logger = logger - sm.clusterInfo = cci + sm.clusterInfo = clusterInfo sm.shutdownCh = shutdownCh sc := serverConfig{} sc.servers = make([]*server_details.ServerDetails, 0) - sm.serverConfigValue.Store(sc) + sm.saveServerConfig(sc) return sm } -// NotifyFailedServer is an exported convenience function that allows callers -// to pass in a server that has failed an RPC request and mark it as failed. -// If the server being failed is not the first server on the list, this is a -// noop. If, however, the server is failed and first on the list, acquire -// the lock, retest, and take the penalty of moving the server to the end of -// the list. +// NotifyFailedServer marks the passed in server as "failed" by rotating it +// to the end of the server list. func (sm *ServerManager) NotifyFailedServer(server *server_details.ServerDetails) { serverCfg := sm.getServerConfig() + // If the server being failed is not the first server on the list, + // this is a noop. If, however, the server is failed and first on + // the list, acquire the lock, retest, and take the penalty of moving + // the server to the end of the list. + // Use atomic.CAS to emulate a TryLock(). if len(serverCfg.servers) > 0 && serverCfg.servers[0] == server && atomic.CompareAndSwapInt32(&sm.notifyFailedBarrier, 0, 1) { @@ -212,10 +221,12 @@ func (sm *ServerManager) NotifyFailedServer(server *server_details.ServerDetails // RebalanceServers takes out an internal write lock and shuffles the list of // servers on this agent. This allows for a redistribution of work across -// consul servers and provides a guarantee that the order list of -// ServerDetails isn't actually ordered, therefore we can sequentially walk -// the array to pick a server without all agents in the cluster dog piling on -// a single node. +// consul servers and provides a guarantee that the order of the server list +// isn't related to the age at which the node was added to the cluster. +// Elsewhere we rely on the position in the server list as a hint regarding +// the stability of a server relative to its position in the server list. +// Servers at or near the front of the list are more stable than servers near +// the end of the list. func (sm *ServerManager) RebalanceServers() { sm.serverConfigLock.Lock() defer sm.serverConfigLock.Unlock() @@ -237,11 +248,7 @@ func (sm *ServerManager) RebalanceServers() { } // RemoveServer takes out an internal write lock and removes a server from -// the server list. No rebalancing happens as a result of the removed server -// because we do not want a network partition which separated a server from -// this agent to cause an increase in work. Instead we rely on the internal -// already existing semantics to handle failure detection after a server has -// been removed. +// the server list. func (sm *ServerManager) RemoveServer(server *server_details.ServerDetails) { sm.serverConfigLock.Lock() defer sm.serverConfigLock.Unlock() @@ -264,7 +271,9 @@ func (sm *ServerManager) RemoveServer(server *server_details.ServerDetails) { } } -// refreshServerRebalanceTimer is called +// refreshServerRebalanceTimer is only called once the rebalanceTimer +// expires. Historically this was an expensive routine and is intended to be +// run in isolation in a dedicated, non-concurrent task. func (sm *ServerManager) refreshServerRebalanceTimer(timer *time.Timer) { serverCfg := sm.getServerConfig() numConsulServers := len(serverCfg.servers) @@ -289,8 +298,11 @@ func (sm *ServerManager) saveServerConfig(sc serverConfig) { } // Start is used to start and manage the task of automatically shuffling and -// rebalance the list of consul servers. This maintenance happens either -// when a new server is added or when a duration has been exceed. +// rebalancing the list of consul servers. This maintenance only happens +// periodically based on the expiration of the timer. Failed servers are +// automatically cycled to the end of the list. New servers are appended to +// the list. The order of the server list must be shuffled periodically to +// distribute load across all known and available consul servers. func (sm *ServerManager) Start() { var rebalanceTimer *time.Timer = time.NewTimer(time.Duration(initialRebalanceTimeoutHours * time.Hour)) var rebalanceTaskDispatched int32 From 94f79d2c3dc9fd9737c3a4566edf7d29325def6a Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 24 Feb 2016 19:59:12 -0800 Subject: [PATCH 42/69] Missed unit test cruft --- consul/server_manager/server_manager_internal_test.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/consul/server_manager/server_manager_internal_test.go b/consul/server_manager/server_manager_internal_test.go index 75be1efd33..1a96104194 100644 --- a/consul/server_manager/server_manager_internal_test.go +++ b/consul/server_manager/server_manager_internal_test.go @@ -112,10 +112,6 @@ func TestServerManagerInternal_NewServerManager(t *testing.T) { t.Fatalf("ServerManager.logger nil") } - if sm.consulServersCh == nil { - t.Fatalf("ServerManager.consulServersCh nil") - } - if sm.shutdownCh == nil { t.Fatalf("ServerManager.shutdownCh nil") } From 49a5a1ab848982ef4f712446d303cd653f303572 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 24 Feb 2016 20:25:50 -0800 Subject: [PATCH 43/69] cycleServer is a pure function, save the result --- consul/server_manager/server_manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index ee3b09b4f8..37ae6f700a 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -213,7 +213,7 @@ func (sm *ServerManager) NotifyFailedServer(server *server_details.ServerDetails serverCfg = sm.getServerConfig() if len(serverCfg.servers) > 0 && serverCfg.servers[0] == server { - serverCfg.cycleServer() + serverCfg.servers = serverCfg.cycleServer() sm.saveServerConfig(serverCfg) } } From e932e9a43542442a74e215e2530ee104a7c3228f Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 24 Feb 2016 22:05:05 -0800 Subject: [PATCH 44/69] Rename FindHealthyServer() to FindServer() There is no guarantee the server coming back is healthy. It's apt to be healthy by virtue of its place in the server list, but it's not guaranteed. --- consul/client.go | 2 +- consul/server_manager/server_manager.go | 6 ++-- consul/server_manager/server_manager_test.go | 38 +++++++++++++++++++- 3 files changed, 41 insertions(+), 5 deletions(-) diff --git a/consul/client.go b/consul/client.go index bc44cbffa7..b6e9497cc3 100644 --- a/consul/client.go +++ b/consul/client.go @@ -321,7 +321,7 @@ func (c *Client) localEvent(event serf.UserEvent) { // RPC is used to forward an RPC call to a consul server, or fail if no servers func (c *Client) RPC(method string, args interface{}, reply interface{}) error { - server := c.serverMgr.FindHealthyServer() + server := c.serverMgr.FindServer() if server == nil { c.logger.Printf("[ERR] consul: No healthy servers found in the server config") return structs.ErrNoServers diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index 37ae6f700a..a8be34bb8a 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -141,13 +141,13 @@ func (sc *serverConfig) cycleServer() (servers []*server_details.ServerDetails) return newServers } -// FindHealthyServer takes out an internal "read lock" and searches through -// the list of servers to find a "healthy" server. If the server is actually +// FindServer takes out an internal "read lock" and searches through the list +// of servers to find a "healthy" server. If the server is actually // unhealthy, we rely on Serf to detect this and remove the node from the // server list. If the server at the front of the list has failed or fails // during an RPC call, it is rotated to the end of the list. If there are no // servers available, return nil. -func (sm *ServerManager) FindHealthyServer() *server_details.ServerDetails { +func (sm *ServerManager) FindServer() *server_details.ServerDetails { serverCfg := sm.getServerConfig() numServers := len(serverCfg.servers) if numServers == 0 { diff --git a/consul/server_manager/server_manager_test.go b/consul/server_manager/server_manager_test.go index 5c83701ad1..a19df17df7 100644 --- a/consul/server_manager/server_manager_test.go +++ b/consul/server_manager/server_manager_test.go @@ -38,8 +38,44 @@ func mockServerManager() (logger *log.Logger, shutdownCh chan struct{}) { // func (sm *ServerManager) AddServer(server *server_details.ServerDetails) { // func (sm *ServerManager) CycleFailedServers() { +// func (sm *ServerManager) FindServer() (server *server_details.ServerDetails) { +func TestServerManager_FindServer(t *testing.T) { + sm := testServerManager() -// func (sm *ServerManager) FindHealthyServer() (server *server_details.ServerDetails) { + s1 := sm.FindServer() + if s1 == nil { + t.Fatalf("Expected non-nil server") + } + if s1.Name != "s1" { + t.Fatalf("Expected s1 server") + } + + s1 = sm.FindServer() + if s1 == nil || s1.Name != "s1" { + t.Fatalf("Expected s1 server (still)") + } + + sm.AddServer(&server_details.ServerDetails{Name: "s2"}) + if sm.NumServers() != 2 { + t.Fatalf("Expected two servers") + } + s1 = sm.FindServer() + if s1 == nil || s1.Name != "s1" { + t.Fatalf("Expected s1 server (still)") + } + + sm.NotifyFailedServer(s1) + s2 := sm.FindServer() + if s2 == nil || s2.Name != "s2" { + t.Fatalf("Expected s2 server") + } + + sm.NotifyFailedServer(s2) + s1 = sm.FindServer() + if s1 == nil || s1.Name != "s1" { + t.Fatalf("Expected s1 server") + } +} // func (sm *ServerManager) GetNumServers() (numServers int) { func TestServerManager_GetNumServers(t *testing.T) { From 18f7befba9971a2fa933282a5ecbfb74bdcbf586 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 24 Feb 2016 22:08:13 -0800 Subject: [PATCH 45/69] Rename NewServerManger to just New Follow go style recommendations now that this has been refactored out of the consul package and doesn't need the qualifier in the name. --- consul/client.go | 2 +- consul/server_manager/server_manager.go | 5 ++--- .../server_manager_internal_test.go | 6 +++--- consul/server_manager/server_manager_test.go | 19 +++++++++++-------- 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/consul/client.go b/consul/client.go index b6e9497cc3..78643a34ed 100644 --- a/consul/client.go +++ b/consul/client.go @@ -118,7 +118,7 @@ func NewClient(config *Config) (*Client, error) { shutdownCh: make(chan struct{}), } - c.serverMgr = server_manager.NewServerManager(c.logger, c.shutdownCh, c.serf) + c.serverMgr = server_manager.New(c.logger, c.shutdownCh, c.serf) // Start consulServers maintenance go c.serverMgr.Start() diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index a8be34bb8a..e3342a9236 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -176,9 +176,8 @@ func (sm *ServerManager) getServerConfig() serverConfig { return sm.serverConfigValue.Load().(serverConfig) } -// NewServerManager is the only way to safely create a new ServerManager -// struct. -func NewServerManager(logger *log.Logger, shutdownCh chan struct{}, clusterInfo ConsulClusterInfo) (sm *ServerManager) { +// New is the only way to safely create a new ServerManager struct. +func New(logger *log.Logger, shutdownCh chan struct{}, clusterInfo ConsulClusterInfo) (sm *ServerManager) { // NOTE(sean@): Can't pass *consul.Client due to an import cycle sm = new(ServerManager) sm.logger = logger diff --git a/consul/server_manager/server_manager_internal_test.go b/consul/server_manager/server_manager_internal_test.go index 1a96104194..74277c9228 100644 --- a/consul/server_manager/server_manager_internal_test.go +++ b/consul/server_manager/server_manager_internal_test.go @@ -32,7 +32,7 @@ func (s *fauxSerf) NumNodes() int { func testServerManager() (sm *ServerManager) { logger := GetBufferedLogger() shutdownCh := make(chan struct{}) - sm = NewServerManager(logger, shutdownCh, &fauxSerf{}) + sm = New(logger, shutdownCh, &fauxSerf{}) return sm } @@ -101,8 +101,8 @@ func TestServerManagerInternal_getServerConfig(t *testing.T) { } } -// func NewServerManager(logger *log.Logger, shutdownCh chan struct{}) (sm *ServerManager) { -func TestServerManagerInternal_NewServerManager(t *testing.T) { +// func New(logger *log.Logger, shutdownCh chan struct{}, clusterInfo ConsulClusterInfo) (sm *ServerManager) { +func TestServerManagerInternal_New(t *testing.T) { sm := testServerManager() if sm == nil { t.Fatalf("ServerManager nil") diff --git a/consul/server_manager/server_manager_test.go b/consul/server_manager/server_manager_test.go index a19df17df7..b5285d505d 100644 --- a/consul/server_manager/server_manager_test.go +++ b/consul/server_manager/server_manager_test.go @@ -23,16 +23,19 @@ func GetBufferedLogger() *log.Logger { return localLogger } -func makeMockServerManager() (sm *server_manager.ServerManager) { - logger, shutdownCh := mockServerManager() - sm = server_manager.NewServerManager(logger, shutdownCh, nil) - return sm +type fauxSerf struct { } -func mockServerManager() (logger *log.Logger, shutdownCh chan struct{}) { - logger = GetBufferedLogger() - shutdownCh = make(chan struct{}) - return logger, shutdownCh +func (s *fauxSerf) NumNodes() int { + return 16384 +} + +func testServerManager() (sm *server_manager.ServerManager) { + logger := GetBufferedLogger() + logger = log.New(os.Stderr, "", log.LstdFlags) + shutdownCh := make(chan struct{}) + sm = server_manager.New(logger, shutdownCh, &fauxSerf{}) + return sm } // func (sm *ServerManager) AddServer(server *server_details.ServerDetails) { From 8e3c83a258813094a277339c8cf4d0577cfb4941 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 24 Feb 2016 22:09:49 -0800 Subject: [PATCH 46/69] Rename GetNumServers to NumServers() Matches the style of the rest of the repo --- consul/client.go | 2 +- consul/client_test.go | 4 ++-- consul/server_manager/server_manager.go | 16 ++++++++-------- consul/server_manager/server_manager_test.go | 4 ++-- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/consul/client.go b/consul/client.go index 78643a34ed..4f2de2a191 100644 --- a/consul/client.go +++ b/consul/client.go @@ -340,7 +340,7 @@ func (c *Client) RPC(method string, args interface{}, reply interface{}) error { // Stats is used to return statistics for debugging and insight // for various sub-systems func (c *Client) Stats() map[string]map[string]string { - numServers := c.serverMgr.GetNumServers() + numServers := c.serverMgr.NumServers() toString := func(v uint64) string { return strconv.FormatUint(v, 10) diff --git a/consul/client_test.go b/consul/client_test.go index 1249800949..8caf765249 100644 --- a/consul/client_test.go +++ b/consul/client_test.go @@ -84,7 +84,7 @@ func TestClient_JoinLAN(t *testing.T) { t.Fatalf("err: %v", err) } testutil.WaitForResult(func() (bool, error) { - return c1.serverMgr.GetNumServers() == 1, nil + return c1.serverMgr.NumServers() == 1, nil }, func(err error) { t.Fatalf("expected consul server") }) @@ -100,7 +100,7 @@ func TestClient_JoinLAN(t *testing.T) { // Check we have a new consul testutil.WaitForResult(func() (bool, error) { - return c1.serverMgr.GetNumServers() == 1, nil + return c1.serverMgr.NumServers() == 1, nil }, func(err error) { t.Fatalf("expected consul server") }) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index e3342a9236..b327b20f24 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -162,14 +162,6 @@ func (sm *ServerManager) FindServer() *server_details.ServerDetails { } } -// GetNumServers takes out an internal "read lock" and returns the number of -// servers. numServers includes both healthy and unhealthy servers. -func (sm *ServerManager) GetNumServers() (numServers int) { - serverCfg := sm.getServerConfig() - numServers = len(serverCfg.servers) - return numServers -} - // getServerConfig is a convenience method which hides the locking semantics // of atomic.Value from the caller. func (sm *ServerManager) getServerConfig() serverConfig { @@ -218,6 +210,14 @@ func (sm *ServerManager) NotifyFailedServer(server *server_details.ServerDetails } } +// NumServers takes out an internal "read lock" and returns the number of +// servers. numServers includes both healthy and unhealthy servers. +func (sm *ServerManager) NumServers() (numServers int) { + serverCfg := sm.getServerConfig() + numServers = len(serverCfg.servers) + return numServers +} + // RebalanceServers takes out an internal write lock and shuffles the list of // servers on this agent. This allows for a redistribution of work across // consul servers and provides a guarantee that the order of the server list diff --git a/consul/server_manager/server_manager_test.go b/consul/server_manager/server_manager_test.go index b5285d505d..2fa61a3dc5 100644 --- a/consul/server_manager/server_manager_test.go +++ b/consul/server_manager/server_manager_test.go @@ -84,14 +84,14 @@ func TestServerManager_FindServer(t *testing.T) { func TestServerManager_GetNumServers(t *testing.T) { sm := makeMockServerManager() var num int - num = sm.GetNumServers() + num = sm.NumServers() if num != 0 { t.Fatalf("Expected zero servers to start") } s := &server_details.ServerDetails{} sm.AddServer(s) - num = sm.GetNumServers() + num = sm.NumServers() if num != 1 { t.Fatalf("Expected one server after AddServer") } From a63d5ab96390e2ea71647f22bf6f696b509f48b1 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 24 Feb 2016 22:23:50 -0800 Subject: [PATCH 47/69] Add a handful more unit tests to the public interface --- consul/server_manager/server_manager.go | 10 +- consul/server_manager/server_manager_test.go | 245 ++++++++++++++++++- 2 files changed, 236 insertions(+), 19 deletions(-) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index b327b20f24..892f1f560f 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -151,7 +151,7 @@ func (sm *ServerManager) FindServer() *server_details.ServerDetails { serverCfg := sm.getServerConfig() numServers := len(serverCfg.servers) if numServers == 0 { - sm.logger.Printf("[ERR] consul: No servers found in the server config") + sm.logger.Printf("[WARN] consul: No servers found in the server config") return nil } else { // Return whatever is at the front of the list because it is @@ -257,11 +257,9 @@ func (sm *ServerManager) RemoveServer(server *server_details.ServerDetails) { n := len(serverCfg.servers) for i := 0; i < n; i++ { if serverCfg.servers[i].Name == server.Name { - newServers := make([]*server_details.ServerDetails, len(serverCfg.servers)-1) - copy(newServers, serverCfg.servers) - - newServers[i], newServers[n-1] = newServers[n-1], nil - newServers = newServers[:n-1] + newServers := make([]*server_details.ServerDetails, 0, len(serverCfg.servers)-1) + newServers = append(newServers, serverCfg.servers[:i]...) + newServers = append(newServers, serverCfg.servers[i+1:]...) serverCfg.servers = newServers sm.saveServerConfig(serverCfg) diff --git a/consul/server_manager/server_manager_test.go b/consul/server_manager/server_manager_test.go index 2fa61a3dc5..ee1d60c6d7 100644 --- a/consul/server_manager/server_manager_test.go +++ b/consul/server_manager/server_manager_test.go @@ -2,7 +2,10 @@ package server_manager_test import ( "bytes" + "fmt" "log" + "os" + "strings" "testing" "github.com/hashicorp/consul/consul/server_details" @@ -39,12 +42,48 @@ func testServerManager() (sm *server_manager.ServerManager) { } // func (sm *ServerManager) AddServer(server *server_details.ServerDetails) { +func TestServerManager_AddServer(t *testing.T) { + sm := testServerManager() + var num int + num = sm.NumServers() + if num != 0 { + t.Fatalf("Expected zero servers to start") + } + + s1 := &server_details.ServerDetails{Name: "s1"} + sm.AddServer(s1) + num = sm.NumServers() + if num != 1 { + t.Fatalf("Expected one server") + } + + sm.AddServer(s1) + num = sm.NumServers() + if num != 1 { + t.Fatalf("Expected one server (still)") + } + + s2 := &server_details.ServerDetails{Name: "s2"} + sm.AddServer(s2) + num = sm.NumServers() + if num != 2 { + t.Fatalf("Expected two servers") + } +} -// func (sm *ServerManager) CycleFailedServers() { // func (sm *ServerManager) FindServer() (server *server_details.ServerDetails) { func TestServerManager_FindServer(t *testing.T) { sm := testServerManager() + if sm.FindServer() != nil { + t.Fatalf("Expected nil return") + } + + sm.AddServer(&server_details.ServerDetails{Name: "s1"}) + if sm.NumServers() != 1 { + t.Fatalf("Expected one server") + } + s1 := sm.FindServer() if s1 == nil { t.Fatalf("Expected non-nil server") @@ -80,9 +119,60 @@ func TestServerManager_FindServer(t *testing.T) { } } -// func (sm *ServerManager) GetNumServers() (numServers int) { -func TestServerManager_GetNumServers(t *testing.T) { - sm := makeMockServerManager() +// func New(logger *log.Logger, shutdownCh chan struct{}) (sm *ServerManager) { +func TestServerManager_New(t *testing.T) { + logger := GetBufferedLogger() + logger = log.New(os.Stderr, "", log.LstdFlags) + shutdownCh := make(chan struct{}) + sm := server_manager.New(logger, shutdownCh, &fauxSerf{}) + if sm == nil { + t.Fatalf("ServerManager nil") + } +} + +// func (sm *ServerManager) NotifyFailedServer(server *server_details.ServerDetails) { +func TestServerManager_NotifyFailedServer(t *testing.T) { + sm := testServerManager() + + if sm.NumServers() != 0 { + t.Fatalf("Expected zero servers to start") + } + + s1 := &server_details.ServerDetails{Name: "s1"} + s2 := &server_details.ServerDetails{Name: "s2"} + sm.AddServer(s1) + sm.AddServer(s2) + if sm.NumServers() != 2 { + t.Fatalf("Expected two servers") + } + + s1 = sm.FindServer() + if s1 == nil || s1.Name != "s1" { + t.Fatalf("Expected s1 server") + } + + sm.NotifyFailedServer(s2) + s1 = sm.FindServer() + if s1 == nil || s1.Name != "s1" { + t.Fatalf("Expected s1 server (still)") + } + + sm.NotifyFailedServer(s1) + s2 = sm.FindServer() + if s2 == nil || s2.Name != "s2" { + t.Fatalf("Expected s2 server") + } + + sm.NotifyFailedServer(s2) + s1 = sm.FindServer() + if s1 == nil || s1.Name != "s1" { + t.Fatalf("Expected s1 server") + } +} + +// func (sm *ServerManager) NumServers() (numServers int) { +func TestServerManager_NumServers(t *testing.T) { + sm := testServerManager() var num int num = sm.NumServers() if num != 0 { @@ -97,18 +187,147 @@ func TestServerManager_GetNumServers(t *testing.T) { } } -// func NewServerManager(logger *log.Logger, shutdownCh chan struct{}) (sm *ServerManager) { -func TestServerManager_NewServerManager(t *testing.T) { - sm := makeMockServerManager() - if sm == nil { - t.Fatalf("ServerManager nil") +// func (sm *ServerManager) RebalanceServers() { +func TestServerManager_RebalanceServers(t *testing.T) { + sm := testServerManager() + const maxServers = 100 + const numShuffleTests = 100 + const uniquePassRate = 0.5 + + // Make a huge list of nodes. + for i := 0; i < maxServers; i++ { + nodeName := fmt.Sprintf("s%02d", i) + sm.AddServer(&server_details.ServerDetails{Name: nodeName}) + } + + // Keep track of how many unique shuffles we get. + uniques := make(map[string]struct{}, maxServers) + for i := 0; i < numShuffleTests; i++ { + sm.RebalanceServers() + + var names []string + for j := 0; j < maxServers; j++ { + server := sm.FindServer() + sm.NotifyFailedServer(server) + names = append(names, server.Name) + } + key := strings.Join(names, "|") + uniques[key] = struct{}{} + } + + // We have to allow for the fact that there won't always be a unique + // shuffle each pass, so we just look for smell here without the test + // being flaky. + if len(uniques) < int(maxServers*uniquePassRate) { + t.Fatalf("unique shuffle ratio too low: %d/%d", len(uniques), maxServers) } } -// func (sm *ServerManager) NotifyFailedServer(server *server_details.ServerDetails) { - -// func (sm *ServerManager) RebalanceServers() { - // func (sm *ServerManager) RemoveServer(server *server_details.ServerDetails) { +func TestServerManager_RemoveServer(t *testing.T) { + sm := testServerManager() + + if sm.NumServers() != 0 { + t.Fatalf("Expected zero servers to start") + } + + const maxServers = 19 + servers := make([]*server_details.ServerDetails, maxServers) + for i := maxServers; i > 0; i-- { + nodeName := fmt.Sprintf("s%02d", i) + server := &server_details.ServerDetails{Name: nodeName} + servers = append(servers, server) + sm.AddServer(server) + } + sm.RebalanceServers() + + if sm.NumServers() != maxServers { + t.Fatalf("Expected %d servers", maxServers) + } + + findServer := func(sm *server_manager.ServerManager, server *server_details.ServerDetails) bool { + for i := sm.NumServers(); i > 0; i-- { + s := sm.FindServer() + if s == server { + return true + } + } + return false + } + + expectedNumServers := maxServers + removedServers := make([]*server_details.ServerDetails, 0, maxServers) + + // Remove servers from the front of the list + for i := 3; i > 0; i-- { + server := sm.FindServer() + if server == nil { + t.Fatalf("FindServer returned nil") + } + sm.RemoveServer(server) + expectedNumServers-- + if sm.NumServers() != expectedNumServers { + t.Fatalf("Expected %d servers (got %d)", expectedNumServers, sm.NumServers()) + } + if findServer(sm, server) == true { + t.Fatalf("Did not expect to find server %s after removal from the front", server.Name) + } + removedServers = append(removedServers, server) + } + + // Remove server from the end of the list + for i := 3; i > 0; i-- { + server := sm.FindServer() + sm.NotifyFailedServer(server) + sm.RemoveServer(server) + expectedNumServers-- + if sm.NumServers() != expectedNumServers { + t.Fatalf("Expected %d servers (got %d)", expectedNumServers, sm.NumServers()) + } + if findServer(sm, server) == true { + t.Fatalf("Did not expect to find server %s", server.Name) + } + removedServers = append(removedServers, server) + } + + // Remove server from the middle of the list + for i := 3; i > 0; i-- { + server := sm.FindServer() + sm.NotifyFailedServer(server) + server2 := sm.FindServer() + sm.NotifyFailedServer(server2) // server2 now at end of the list + + sm.RemoveServer(server) + expectedNumServers-- + if sm.NumServers() != expectedNumServers { + t.Fatalf("Expected %d servers (got %d)", expectedNumServers, sm.NumServers()) + } + if findServer(sm, server) == true { + t.Fatalf("Did not expect to find server %s", server.Name) + } + removedServers = append(removedServers, server) + } + + if sm.NumServers()+len(removedServers) != maxServers { + t.Fatalf("Expected %d+%d=%d servers", sm.NumServers(), len(removedServers), maxServers) + } + + // Drain the remaining servers from the middle + for i := sm.NumServers(); i > 0; i-- { + server := sm.FindServer() + sm.NotifyFailedServer(server) + server2 := sm.FindServer() + sm.NotifyFailedServer(server2) // server2 now at end of the list + sm.RemoveServer(server) + removedServers = append(removedServers, server) + } + + if sm.NumServers() != 0 { + t.Fatalf("Expected an empty server list") + } + if len(removedServers) != maxServers { + t.Fatalf("Expected all servers to be in removed server list") + } +} // func (sm *ServerManager) Start() { From da872fee6367fec990e2fe4eafc3a6318bfbb1fa Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Thu, 25 Feb 2016 08:05:15 -0800 Subject: [PATCH 48/69] Test ServerManager.refreshServerRebalanceTimer Change the signature so it returns a value so that this can be tested externally with mock data. See the sample table in TestServerManagerInternal_refreshServerRebalanceTimer() for the rate at which it will back off. This function is mostly used to not cripple large clusters in the event of a partition. --- consul/server_manager/server_manager.go | 5 +- .../server_manager_internal_test.go | 72 ++++++++++++++++++- 2 files changed, 71 insertions(+), 6 deletions(-) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index 892f1f560f..260148324d 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -271,7 +271,7 @@ func (sm *ServerManager) RemoveServer(server *server_details.ServerDetails) { // refreshServerRebalanceTimer is only called once the rebalanceTimer // expires. Historically this was an expensive routine and is intended to be // run in isolation in a dedicated, non-concurrent task. -func (sm *ServerManager) refreshServerRebalanceTimer(timer *time.Timer) { +func (sm *ServerManager) refreshServerRebalanceTimer(timer *time.Timer) time.Duration { serverCfg := sm.getServerConfig() numConsulServers := len(serverCfg.servers) // Limit this connection's life based on the size (and health) of the @@ -280,12 +280,11 @@ func (sm *ServerManager) refreshServerRebalanceTimer(timer *time.Timer) { // clusterWideRebalanceConnsPerSec operations/s across numLANMembers. clusterWideRebalanceConnsPerSec := float64(numConsulServers * newRebalanceConnsPerSecPerServer) connReuseLowWatermarkDuration := clientRPCMinReuseDuration + lib.RandomStagger(clientRPCMinReuseDuration/clientRPCJitterFraction) - numLANMembers := sm.clusterInfo.NumNodes() connRebalanceTimeout := lib.RateScaledInterval(clusterWideRebalanceConnsPerSec, connReuseLowWatermarkDuration, numLANMembers) - sm.logger.Printf("[DEBUG] consul: connection will be rebalanced in %v", connRebalanceTimeout) timer.Reset(connRebalanceTimeout) + return connRebalanceTimeout } // saveServerConfig is a convenience method which hides the locking semantics diff --git a/consul/server_manager/server_manager_internal_test.go b/consul/server_manager/server_manager_internal_test.go index 74277c9228..6ea93c3e9d 100644 --- a/consul/server_manager/server_manager_internal_test.go +++ b/consul/server_manager/server_manager_internal_test.go @@ -2,8 +2,11 @@ package server_manager import ( "bytes" + "fmt" "log" + "os" "testing" + "time" "github.com/hashicorp/consul/consul/server_details" ) @@ -23,16 +26,17 @@ func GetBufferedLogger() *log.Logger { } type fauxSerf struct { + numNodes int } func (s *fauxSerf) NumNodes() int { - return 16384 + return s.numNodes } func testServerManager() (sm *ServerManager) { logger := GetBufferedLogger() shutdownCh := make(chan struct{}) - sm = New(logger, shutdownCh, &fauxSerf{}) + sm = New(logger, shutdownCh, &fauxSerf{numNodes: 16384}) return sm } @@ -108,6 +112,10 @@ func TestServerManagerInternal_New(t *testing.T) { t.Fatalf("ServerManager nil") } + if sm.clusterInfo == nil { + t.Fatalf("ServerManager.clusterInfo nil") + } + if sm.logger == nil { t.Fatalf("ServerManager.logger nil") } @@ -117,7 +125,65 @@ func TestServerManagerInternal_New(t *testing.T) { } } -// func (sc *serverConfig) resetRebalanceTimer(sm *ServerManager) { +// func (sc *serverConfig) refreshServerRebalanceTimer(timer *time.Timer) { +func TestServerManagerInternal_refreshServerRebalanceTimer(t *testing.T) { + sm := testServerManager() + + timer := time.NewTimer(time.Duration(1 * time.Nanosecond)) + time.Sleep(1 * time.Millisecond) + sm.refreshServerRebalanceTimer(timer) + + logger := log.New(os.Stderr, "", log.LstdFlags) + shutdownCh := make(chan struct{}) + + type clusterSizes struct { + numNodes int + numServers int + minRebalance time.Duration + } + clusters := []clusterSizes{ + {0, 3, 2 * time.Minute}, + {1, 0, 2 * time.Minute}, // partitioned cluster + {1, 3, 2 * time.Minute}, + {2, 3, 2 * time.Minute}, + {100, 0, 2 * time.Minute}, // partitioned + {100, 1, 2 * time.Minute}, // partitioned + {100, 3, 2 * time.Minute}, + {1024, 1, 2 * time.Minute}, // partitioned + {1024, 3, 2 * time.Minute}, // partitioned + {1024, 5, 2 * time.Minute}, + {16384, 1, 4 * time.Minute}, // partitioned + {16384, 2, 2 * time.Minute}, // partitioned + {16384, 3, 2 * time.Minute}, // partitioned + {16384, 5, 2 * time.Minute}, + {65535, 0, 2 * time.Minute}, // partitioned + {65535, 1, 8 * time.Minute}, // partitioned + {65535, 2, 3 * time.Minute}, // partitioned + {65535, 3, 5 * time.Minute}, // partitioned + {65535, 5, 3 * time.Minute}, // partitioned + {65535, 7, 2 * time.Minute}, + {1000000, 1, 4 * time.Hour}, // partitioned + {1000000, 2, 2 * time.Hour}, // partitioned + {1000000, 3, 80 * time.Minute}, // partitioned + {1000000, 5, 50 * time.Minute}, // partitioned + {1000000, 11, 20 * time.Minute}, // partitioned + {1000000, 19, 10 * time.Minute}, + } + + for _, s := range clusters { + sm := New(logger, shutdownCh, &fauxSerf{numNodes: s.numNodes}) + + for i := 0; i < s.numServers; i++ { + nodeName := fmt.Sprintf("s%02d", i) + sm.AddServer(&server_details.ServerDetails{Name: nodeName}) + } + + d := sm.refreshServerRebalanceTimer(timer) + if d < s.minRebalance { + t.Fatalf("duration too short for cluster of size %d and %d servers (%s < %s)", s.numNodes, s.numServers, d, s.minRebalance) + } + } +} // func (sm *ServerManager) saveServerConfig(sc serverConfig) { func TestServerManagerInternal_saveServerConfig(t *testing.T) { From 828606232e2a66efc9c12b02ea4a3c3368df7a36 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 23 Mar 2016 22:35:49 -0700 Subject: [PATCH 49/69] Correct a bogus goimport rewrite for tests --- consul/client_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consul/client_test.go b/consul/client_test.go index 8caf765249..5fe6194e35 100644 --- a/consul/client_test.go +++ b/consul/client_test.go @@ -10,7 +10,7 @@ import ( "github.com/hashicorp/consul/consul/structs" "github.com/hashicorp/consul/testutil" - "github.com/hashicorp/consul/vendor/github.com/hashicorp/net-rpc-msgpackrpc" + "github.com/hashicorp/net-rpc-msgpackrpc" "github.com/hashicorp/serf/serf" ) From 4d4806ab02583cf9123bdc29d4e5ddc4dc67d175 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 23 Mar 2016 22:36:12 -0700 Subject: [PATCH 50/69] Add CHANGELOG entry re: agent rebalancing --- CHANGELOG.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a35efd364..c20811a7c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,17 @@ ## 0.7.0 (UNRELEASED) +IMPROVEMENTS: + +* Consul agents will now periodically reconnect to available Consul servers + in order to redistribute their RPC query load. Consul clients will, by + default, attempt to establish a new connection every 120s to 180s unless + the size of the cluster is sufficiently large. The rate at which agents + begin to query new servers is proportional to the size of the Consul + cluster (servers should never receive more than 64 new connections per + second per Consul server as a result of rebalancing). Clusters in stable + environments who use `allow_stale` should see a more even distribution of + query load across all of their Consul servers. [GH-1743] + ## 0.6.4 (March 16, 2016) BACKWARDS INCOMPATIBILITIES: From db7204106389798c0f0a1639e7d7649bfaf661ed Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 25 Mar 2016 11:57:54 -0700 Subject: [PATCH 51/69] Add a comment for Client serverMgr --- consul/client.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/consul/client.go b/consul/client.go index 4f2de2a191..28b5b9eebb 100644 --- a/consul/client.go +++ b/consul/client.go @@ -58,7 +58,8 @@ type Client struct { // Connection pool to consul servers connPool *ConnPool - // serverManager + // serverMgr is responsible for the selection and maintenance of + // Consul servers this agent uses for RPC requests serverMgr *server_manager.ServerManager // eventCh is used to receive events from the From 9daccb8b4145c11ebb91f32d3b752ddd92c5e612 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 25 Mar 2016 12:00:40 -0700 Subject: [PATCH 52/69] Fix stale comment Pointed out by: @slackpad --- consul/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consul/client.go b/consul/client.go index 28b5b9eebb..26db9fc51f 100644 --- a/consul/client.go +++ b/consul/client.go @@ -121,7 +121,7 @@ func NewClient(config *Config) (*Client, error) { c.serverMgr = server_manager.New(c.logger, c.shutdownCh, c.serf) - // Start consulServers maintenance + // Start maintenance task for serverMgr go c.serverMgr.Start() // Start the Serf listeners to prevent a deadlock From 7a2d30d1cff152b80521a1205c5f12db58beaccd Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 25 Mar 2016 12:11:35 -0700 Subject: [PATCH 53/69] Be more Go idiomatic w/ variable names: s/valid/ok/g Cargo culting is bad, m'kay? Pointy Hat: sean- --- consul/server_details/server_details_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/consul/server_details/server_details_test.go b/consul/server_details/server_details_test.go index ac24b38450..c87c4a1532 100644 --- a/consul/server_details/server_details_test.go +++ b/consul/server_details/server_details_test.go @@ -19,9 +19,9 @@ func TestIsConsulServer(t *testing.T) { "vsn": "1", }, } - valid, parts := server_details.IsConsulServer(m) - if !valid || parts.Datacenter != "east-aws" || parts.Port != 10000 { - t.Fatalf("bad: %v %v", valid, parts) + ok, parts := server_details.IsConsulServer(m) + if !ok || parts.Datacenter != "east-aws" || parts.Port != 10000 { + t.Fatalf("bad: %v %v", ok, parts) } if parts.Name != "foo" { t.Fatalf("bad: %v", parts) @@ -34,8 +34,8 @@ func TestIsConsulServer(t *testing.T) { } m.Tags["bootstrap"] = "1" m.Tags["disabled"] = "1" - valid, parts = server_details.IsConsulServer(m) - if !valid { + ok, parts = server_details.IsConsulServer(m) + if !ok { t.Fatalf("expected a valid consul server") } if !parts.Bootstrap { @@ -50,8 +50,8 @@ func TestIsConsulServer(t *testing.T) { m.Tags["expect"] = "3" delete(m.Tags, "bootstrap") delete(m.Tags, "disabled") - valid, parts = server_details.IsConsulServer(m) - if !valid || parts.Expect != 3 { + ok, parts = server_details.IsConsulServer(m) + if !ok || parts.Expect != 3 { t.Fatalf("bad: %v", parts.Expect) } } From 0f3ad9c12011cfa74550ec9ab08f3999b02dfa6d Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 25 Mar 2016 12:20:12 -0700 Subject: [PATCH 54/69] Test to make sure bootstrap is missing --- consul/server_details/server_details_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/consul/server_details/server_details_test.go b/consul/server_details/server_details_test.go index c87c4a1532..6ed2c30ac4 100644 --- a/consul/server_details/server_details_test.go +++ b/consul/server_details/server_details_test.go @@ -54,4 +54,7 @@ func TestIsConsulServer(t *testing.T) { if !ok || parts.Expect != 3 { t.Fatalf("bad: %v", parts.Expect) } + if parts.Bootstrap { + t.Fatalf("unexpected bootstrap") + } } From 3433feb93b76aafd2609a131c50c5a07f389deb2 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 25 Mar 2016 12:22:33 -0700 Subject: [PATCH 55/69] Negative check: test an invalid condition --- consul/server_details/server_details_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/consul/server_details/server_details_test.go b/consul/server_details/server_details_test.go index 6ed2c30ac4..b8aef0abde 100644 --- a/consul/server_details/server_details_test.go +++ b/consul/server_details/server_details_test.go @@ -57,4 +57,10 @@ func TestIsConsulServer(t *testing.T) { if parts.Bootstrap { t.Fatalf("unexpected bootstrap") } + + delete(m.Tags, "consul") + ok, parts = server_details.IsConsulServer(m) + if ok { + t.Fatalf("unexpected ok server") + } } From 68183c43783250a62d075c00a318ab27e76e71b4 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 25 Mar 2016 12:34:12 -0700 Subject: [PATCH 56/69] Change initialReblaanaceTimeout to a time.Duration Pointed out by: @slackpad --- consul/server_manager/server_manager.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index 260148324d..dd36735321 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -31,10 +31,10 @@ const ( // queries are sent over an established connection to a single server clientRPCMinReuseDuration = 120 * time.Second - // initialRebalanceTimeoutHours is the initial value for the + // initialRebalanceTimeout is the initial value for the // rebalanceTimer. This value is discarded immediately after the // client becomes aware of the first server. - initialRebalanceTimeoutHours = 24 + initialRebalanceTimeout = 24 * time.Hour // Limit the number of new connections a server receives per second // for connection rebalancing. This limit caps the load caused by @@ -300,7 +300,7 @@ func (sm *ServerManager) saveServerConfig(sc serverConfig) { // the list. The order of the server list must be shuffled periodically to // distribute load across all known and available consul servers. func (sm *ServerManager) Start() { - var rebalanceTimer *time.Timer = time.NewTimer(time.Duration(initialRebalanceTimeoutHours * time.Hour)) + var rebalanceTimer *time.Timer = time.NewTimer(initialRebalanceTimeout) var rebalanceTaskDispatched int32 func() { From b8bbdb9e7a3d05353c722b3f2633ada961b0ca67 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 25 Mar 2016 12:34:46 -0700 Subject: [PATCH 57/69] Reword comment after moving code into new packages --- consul/server_manager/server_manager.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index dd36735321..0b7cf66372 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -56,8 +56,8 @@ type ConsulClusterInfo interface { NumNodes() int } -// serverCfg is the thread-safe configuration structure that is used to -// maintain the list of consul servers in Client. +// serverCfg is the thread-safe configuration struct used to maintain the +// list of Consul servers in ServerManager. // // NOTE(sean@): We are explicitly relying on the fact that serverConfig will // be copied onto the stack. Please keep this structure light. From b00db393e7279c2b29dcb83d64e916662d0bbb12 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 25 Mar 2016 12:38:40 -0700 Subject: [PATCH 58/69] Clarify that ConsulClusterInfo is an interface over serf An interface was used to break a cyclic import dependency. --- consul/server_manager/server_manager.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index 0b7cf66372..80a8f71f33 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -78,9 +78,9 @@ type ServerManager struct { logger *log.Logger - // serf is used to estimate the approximate number of nodes in a - // cluster and limit the rate at which it rebalances server - // connections + // clusterInfo is used to estimate the approximate number of nodes in + // a cluster and limit the rate at which it rebalances server + // connections. ConsulClusterInfo is an interface that wraps serf. clusterInfo ConsulClusterInfo // notifyFailedServersBarrier is acts as a barrier to prevent From 24eb27486099e937b01e02f3df417408fd07ade5 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 25 Mar 2016 12:41:22 -0700 Subject: [PATCH 59/69] Relocate saveServerConfig next to getServerConfig Requested by: slackpad --- consul/server_manager/server_manager.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index 80a8f71f33..e6d847c5d4 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -168,6 +168,12 @@ func (sm *ServerManager) getServerConfig() serverConfig { return sm.serverConfigValue.Load().(serverConfig) } +// saveServerConfig is a convenience method which hides the locking semantics +// of atomic.Value from the caller. +func (sm *ServerManager) saveServerConfig(sc serverConfig) { + sm.serverConfigValue.Store(sc) +} + // New is the only way to safely create a new ServerManager struct. func New(logger *log.Logger, shutdownCh chan struct{}, clusterInfo ConsulClusterInfo) (sm *ServerManager) { // NOTE(sean@): Can't pass *consul.Client due to an import cycle @@ -287,12 +293,6 @@ func (sm *ServerManager) refreshServerRebalanceTimer(timer *time.Timer) time.Dur return connRebalanceTimeout } -// saveServerConfig is a convenience method which hides the locking semantics -// of atomic.Value from the caller. -func (sm *ServerManager) saveServerConfig(sc serverConfig) { - sm.serverConfigValue.Store(sc) -} - // Start is used to start and manage the task of automatically shuffling and // rebalancing the list of consul servers. This maintenance only happens // periodically based on the expiration of the timer. Failed servers are From 0b3f6932df065a18194eae12fb690a5ea234e84d Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 25 Mar 2016 12:54:36 -0700 Subject: [PATCH 60/69] Only rotate server list with more than one server Fantastic observation by slackpad. This was left over from when there was a boolean for health in the server struct (vs current strategy where we use server position in the list and rely on serf to cleanup the stale members). Pointed out by: slackpad --- consul/server_manager/server_manager.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index e6d847c5d4..0f935c0b62 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -198,8 +198,9 @@ func (sm *ServerManager) NotifyFailedServer(server *server_details.ServerDetails // the list, acquire the lock, retest, and take the penalty of moving // the server to the end of the list. - // Use atomic.CAS to emulate a TryLock(). - if len(serverCfg.servers) > 0 && serverCfg.servers[0] == server && + // Only rotate the server list when there is more than one server + if len(serverCfg.servers) > 1 && serverCfg.servers[0] == server && + // Use atomic.CAS to emulate a TryLock(). atomic.CompareAndSwapInt32(&sm.notifyFailedBarrier, 0, 1) { defer atomic.StoreInt32(&sm.notifyFailedBarrier, 0) @@ -209,7 +210,7 @@ func (sm *ServerManager) NotifyFailedServer(server *server_details.ServerDetails defer sm.serverConfigLock.Unlock() serverCfg = sm.getServerConfig() - if len(serverCfg.servers) > 0 && serverCfg.servers[0] == server { + if len(serverCfg.servers) > 1 && serverCfg.servers[0] == server { serverCfg.servers = serverCfg.cycleServer() sm.saveServerConfig(serverCfg) } From 9c18bb5f1c721a46676a8ee869eab8d51dda6a00 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 25 Mar 2016 13:06:59 -0700 Subject: [PATCH 61/69] Comment updates --- consul/server_manager/server_manager.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index 0f935c0b62..9d68bd14c6 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -232,7 +232,8 @@ func (sm *ServerManager) NumServers() (numServers int) { // Elsewhere we rely on the position in the server list as a hint regarding // the stability of a server relative to its position in the server list. // Servers at or near the front of the list are more stable than servers near -// the end of the list. +// the end of the list. Unhealthy servers are removed when serf notices the +// server has been deregistered. func (sm *ServerManager) RebalanceServers() { sm.serverConfigLock.Lock() defer sm.serverConfigLock.Unlock() @@ -241,9 +242,7 @@ func (sm *ServerManager) RebalanceServers() { newServers := make([]*server_details.ServerDetails, len(serverCfg.servers)) copy(newServers, serverCfg.servers) - // Shuffle the server list on server join. Servers are selected from - // the head of the list and are moved to the end of the list on - // failure. + // Shuffle the server list for i := len(serverCfg.servers) - 1; i > 0; i-- { j := rand.Int31n(int32(i + 1)) newServers[i], newServers[j] = newServers[j], newServers[i] From d9251e30c87f34a14af9bd6c01f3499e86411dac Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 25 Mar 2016 13:08:08 -0700 Subject: [PATCH 62/69] Use range vs for Returning a new array vs mutating an array in place so we can use range now. --- consul/server_manager/server_manager.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index 9d68bd14c6..7e2e213d33 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -260,8 +260,7 @@ func (sm *ServerManager) RemoveServer(server *server_details.ServerDetails) { serverCfg := sm.getServerConfig() // Remove the server if known - n := len(serverCfg.servers) - for i := 0; i < n; i++ { + for i, _ := range serverCfg.servers { if serverCfg.servers[i].Name == server.Name { newServers := make([]*server_details.ServerDetails, 0, len(serverCfg.servers)-1) newServers = append(newServers, serverCfg.servers[:i]...) From 4fec6a9608889843893baee5613590b80edf39cb Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 25 Mar 2016 13:26:29 -0700 Subject: [PATCH 63/69] Guard against very small or negative rates Pointed out by: slackpad --- lib/cluster.go | 4 ++++ lib/cluster_test.go | 11 ++++++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/lib/cluster.go b/lib/cluster.go index 0062c61375..33a7788c78 100644 --- a/lib/cluster.go +++ b/lib/cluster.go @@ -14,6 +14,10 @@ func RandomStagger(intv time.Duration) time.Duration { // order to target an aggregate number of actions per second across the whole // cluster. func RateScaledInterval(rate float64, min time.Duration, n int) time.Duration { + const minRate = 1 / 86400 // 1/(1 * time.Day) + if rate <= minRate { + return min + } interval := time.Duration(float64(time.Second) * float64(n) / rate) if interval < min { return min diff --git a/lib/cluster_test.go b/lib/cluster_test.go index 40949d0204..d517e4677f 100644 --- a/lib/cluster_test.go +++ b/lib/cluster_test.go @@ -16,7 +16,7 @@ func TestRandomStagger(t *testing.T) { } func TestRateScaledInterval(t *testing.T) { - min := 1 * time.Second + const min = 1 * time.Second rate := 200.0 if v := RateScaledInterval(rate, min, 0); v != min { t.Fatalf("Bad: %v", v) @@ -36,4 +36,13 @@ func TestRateScaledInterval(t *testing.T) { if v := RateScaledInterval(rate, min, 10000); v != 50*time.Second { t.Fatalf("Bad: %v", v) } + if v := RateScaledInterval(0, min, 10000); v != min { + t.Fatalf("Bad: %v", v) + } + if v := RateScaledInterval(0.0, min, 10000); v != min { + t.Fatalf("Bad: %v", v) + } + if v := RateScaledInterval(-1, min, 10000); v != min { + t.Fatalf("Bad: %v", v) + } } From 58246fcc0b2e8b5b3d575070dac73c3bcdd928c9 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 25 Mar 2016 13:46:18 -0700 Subject: [PATCH 64/69] Initialize the rebalancce to clientRPCMinReuseDuration In an earlier version there was a channel to notify when a new server was added, however this has long since been removed. Just default to the sane value of 2min before the first rebalance calc takes place. Pointed out by: slackpad --- consul/server_manager/server_manager.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index 7e2e213d33..5fec577e64 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -31,11 +31,6 @@ const ( // queries are sent over an established connection to a single server clientRPCMinReuseDuration = 120 * time.Second - // initialRebalanceTimeout is the initial value for the - // rebalanceTimer. This value is discarded immediately after the - // client becomes aware of the first server. - initialRebalanceTimeout = 24 * time.Hour - // Limit the number of new connections a server receives per second // for connection rebalancing. This limit caps the load caused by // continual rebalancing efforts when a cluster is in equilibrium. A @@ -299,7 +294,7 @@ func (sm *ServerManager) refreshServerRebalanceTimer(timer *time.Timer) time.Dur // the list. The order of the server list must be shuffled periodically to // distribute load across all known and available consul servers. func (sm *ServerManager) Start() { - var rebalanceTimer *time.Timer = time.NewTimer(initialRebalanceTimeout) + var rebalanceTimer *time.Timer = time.NewTimer(clientRPCMinReuseDuration) var rebalanceTaskDispatched int32 func() { From a71fbe57e31cadcc517df784b7d18856db568300 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 25 Mar 2016 13:58:50 -0700 Subject: [PATCH 65/69] Only log in FindServers In FindServer this is a useful warning hinting why its call failed. RPC returns error and leaves it to the higher level caller to do whatever it wants. As an operator, I'd have the detail necessary to know why the RPC call(s) failed. --- consul/client.go | 1 - consul/server_manager/server_manager.go | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/consul/client.go b/consul/client.go index 26db9fc51f..0b536a8d52 100644 --- a/consul/client.go +++ b/consul/client.go @@ -324,7 +324,6 @@ func (c *Client) localEvent(event serf.UserEvent) { func (c *Client) RPC(method string, args interface{}, reply interface{}) error { server := c.serverMgr.FindServer() if server == nil { - c.logger.Printf("[ERR] consul: No healthy servers found in the server config") return structs.ErrNoServers } diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index 5fec577e64..bb4d4fad1c 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -146,7 +146,7 @@ func (sm *ServerManager) FindServer() *server_details.ServerDetails { serverCfg := sm.getServerConfig() numServers := len(serverCfg.servers) if numServers == 0 { - sm.logger.Printf("[WARN] consul: No servers found in the server config") + sm.logger.Printf("[WARN] consul: No servers available") return nil } else { // Return whatever is at the front of the list because it is From a3a0eeeaddf7707873085b8e75d3fd5a3c0428ff Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 25 Mar 2016 14:00:05 -0700 Subject: [PATCH 66/69] Trim residual complexity from server join notifications Now that serf node join events are decoupled from rebalancing activities completely, remove the complixity of draining the channel and ensuring only one go routine was rebalancing the server list. Now that we're no longer initializing a notification channel, we can remove the config load/save from `Start()` --- consul/server_manager/server_manager.go | 27 ++----------------------- 1 file changed, 2 insertions(+), 25 deletions(-) diff --git a/consul/server_manager/server_manager.go b/consul/server_manager/server_manager.go index bb4d4fad1c..504c5ce2bf 100644 --- a/consul/server_manager/server_manager.go +++ b/consul/server_manager/server_manager.go @@ -295,36 +295,13 @@ func (sm *ServerManager) refreshServerRebalanceTimer(timer *time.Timer) time.Dur // distribute load across all known and available consul servers. func (sm *ServerManager) Start() { var rebalanceTimer *time.Timer = time.NewTimer(clientRPCMinReuseDuration) - var rebalanceTaskDispatched int32 - - func() { - sm.serverConfigLock.Lock() - defer sm.serverConfigLock.Unlock() - - serverCfgPtr := sm.serverConfigValue.Load() - if serverCfgPtr == nil { - panic("server config has not been initialized") - } - var serverCfg serverConfig - serverCfg = serverCfgPtr.(serverConfig) - sm.saveServerConfig(serverCfg) - }() for { select { case <-rebalanceTimer.C: - sm.logger.Printf("[INFO] server manager: server rebalance timeout") + sm.logger.Printf("[INFO] server manager: Rebalancing server connections") sm.RebalanceServers() - - // Only run one rebalance task at a time, but do - // allow for the channel to be drained - if atomic.CompareAndSwapInt32(&rebalanceTaskDispatched, 0, 1) { - sm.logger.Printf("[INFO] server manager: Launching rebalance duration task") - go func() { - defer atomic.StoreInt32(&rebalanceTaskDispatched, 0) - sm.refreshServerRebalanceTimer(rebalanceTimer) - }() - } + sm.refreshServerRebalanceTimer(rebalanceTimer) case <-sm.shutdownCh: sm.logger.Printf("[INFO] server manager: shutting down") From b1194e83cb79965cf7317e19c859d3d13f77d994 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 25 Mar 2016 14:10:09 -0700 Subject: [PATCH 67/69] Don't pass in sm, server manager is already in scope Go closures are implicitly capturing lambdas. --- consul/server_manager/server_manager_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/consul/server_manager/server_manager_test.go b/consul/server_manager/server_manager_test.go index ee1d60c6d7..90c5446457 100644 --- a/consul/server_manager/server_manager_test.go +++ b/consul/server_manager/server_manager_test.go @@ -245,7 +245,7 @@ func TestServerManager_RemoveServer(t *testing.T) { t.Fatalf("Expected %d servers", maxServers) } - findServer := func(sm *server_manager.ServerManager, server *server_details.ServerDetails) bool { + findServer := func(server *server_details.ServerDetails) bool { for i := sm.NumServers(); i > 0; i-- { s := sm.FindServer() if s == server { @@ -269,7 +269,7 @@ func TestServerManager_RemoveServer(t *testing.T) { if sm.NumServers() != expectedNumServers { t.Fatalf("Expected %d servers (got %d)", expectedNumServers, sm.NumServers()) } - if findServer(sm, server) == true { + if findServer(server) == true { t.Fatalf("Did not expect to find server %s after removal from the front", server.Name) } removedServers = append(removedServers, server) @@ -284,7 +284,7 @@ func TestServerManager_RemoveServer(t *testing.T) { if sm.NumServers() != expectedNumServers { t.Fatalf("Expected %d servers (got %d)", expectedNumServers, sm.NumServers()) } - if findServer(sm, server) == true { + if findServer(server) == true { t.Fatalf("Did not expect to find server %s", server.Name) } removedServers = append(removedServers, server) @@ -302,7 +302,7 @@ func TestServerManager_RemoveServer(t *testing.T) { if sm.NumServers() != expectedNumServers { t.Fatalf("Expected %d servers (got %d)", expectedNumServers, sm.NumServers()) } - if findServer(sm, server) == true { + if findServer(server) == true { t.Fatalf("Did not expect to find server %s", server.Name) } removedServers = append(removedServers, server) From d18e4d7455f4678580479ed13ab4d0a0e8323c84 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 25 Mar 2016 14:31:48 -0700 Subject: [PATCH 68/69] Delete the right tag "role" != "consul" --- consul/server_details/server_details_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consul/server_details/server_details_test.go b/consul/server_details/server_details_test.go index b8aef0abde..09b87ec165 100644 --- a/consul/server_details/server_details_test.go +++ b/consul/server_details/server_details_test.go @@ -58,7 +58,7 @@ func TestIsConsulServer(t *testing.T) { t.Fatalf("unexpected bootstrap") } - delete(m.Tags, "consul") + delete(m.Tags, "role") ok, parts = server_details.IsConsulServer(m) if ok { t.Fatalf("unexpected ok server") From 5893bd5a350385f9b414733a076e318e4119d4b6 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Fri, 25 Mar 2016 14:40:46 -0700 Subject: [PATCH 69/69] Add additional checks --- consul/server_manager/server_manager_test.go | 32 ++++++++++++++++++-- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/consul/server_manager/server_manager_test.go b/consul/server_manager/server_manager_test.go index 90c5446457..8b292a2ddc 100644 --- a/consul/server_manager/server_manager_test.go +++ b/consul/server_manager/server_manager_test.go @@ -140,7 +140,20 @@ func TestServerManager_NotifyFailedServer(t *testing.T) { s1 := &server_details.ServerDetails{Name: "s1"} s2 := &server_details.ServerDetails{Name: "s2"} + + // Try notifying for a server that is not part of the server manager + sm.NotifyFailedServer(s1) + if sm.NumServers() != 0 { + t.Fatalf("Expected zero servers to start") + } sm.AddServer(s1) + + // Test again w/ a server not in the list + sm.NotifyFailedServer(s2) + if sm.NumServers() != 1 { + t.Fatalf("Expected one server") + } + sm.AddServer(s2) if sm.NumServers() != 2 { t.Fatalf("Expected two servers") @@ -225,16 +238,29 @@ func TestServerManager_RebalanceServers(t *testing.T) { // func (sm *ServerManager) RemoveServer(server *server_details.ServerDetails) { func TestServerManager_RemoveServer(t *testing.T) { + const nodeNameFmt = "s%02d" sm := testServerManager() if sm.NumServers() != 0 { t.Fatalf("Expected zero servers to start") } + // Test removing server before its added + nodeName := fmt.Sprintf(nodeNameFmt, 1) + s1 := &server_details.ServerDetails{Name: nodeName} + sm.RemoveServer(s1) + sm.AddServer(s1) + + nodeName = fmt.Sprintf(nodeNameFmt, 2) + s2 := &server_details.ServerDetails{Name: nodeName} + sm.RemoveServer(s2) + sm.AddServer(s2) + const maxServers = 19 servers := make([]*server_details.ServerDetails, maxServers) - for i := maxServers; i > 0; i-- { - nodeName := fmt.Sprintf("s%02d", i) + // Already added two servers above + for i := maxServers; i > 2; i-- { + nodeName := fmt.Sprintf(nodeNameFmt, i) server := &server_details.ServerDetails{Name: nodeName} servers = append(servers, server) sm.AddServer(server) @@ -242,7 +268,7 @@ func TestServerManager_RemoveServer(t *testing.T) { sm.RebalanceServers() if sm.NumServers() != maxServers { - t.Fatalf("Expected %d servers", maxServers) + t.Fatalf("Expected %d servers, received %d", maxServers, sm.NumServers()) } findServer := func(server *server_details.ServerDetails) bool {