From db6b291a5a1afbfdae60d3a48a2b09abf6cd0ae1 Mon Sep 17 00:00:00 2001
From: Preetha Appan <preetha@hashicorp.com>
Date: Tue, 7 May 2019 16:23:32 -0500
Subject: [PATCH] code review feedback

---
 client/client.go               | 20 --------------------
 client/servers/manager.go      | 31 ++++++++++++++++++++++++++-----
 client/servers/manager_test.go |  8 ++++++++
 dev/cluster/client3.hcl        |  3 +++
 dev/cluster/server1.hcl        |  2 +-
 5 files changed, 38 insertions(+), 26 deletions(-)

diff --git a/client/client.go b/client/client.go
index bf767a58f..43343f2cd 100644
--- a/client/client.go
+++ b/client/client.go
@@ -2427,26 +2427,6 @@ DISCOLOOP:
 
 	consulLogger.Info("discovered following servers", "servers", nomadServers)
 
-	// Check if the list of servers discovered is identical to the list we already have
-	// If so, we don't need to reset the server list unnecessarily
-	knownServers := make(map[string]struct{})
-	serverList := c.servers.GetServers()
-	for _, s := range serverList {
-		knownServers[s.Addr.String()] = struct{}{}
-	}
-
-	allFound := true
-	for _, s := range nomadServers {
-		_, known := knownServers[s.Addr.String()]
-		if !known {
-			allFound = false
-			break
-		}
-	}
-	if allFound && len(nomadServers) == len(serverList) {
-		c.logger.Info("Not replacing server list, current server list is identical to servers discovered in Consul")
-		return nil
-	}
 	// Fire the retry trigger if we have updated the set of servers.
 	if c.servers.SetServers(nomadServers) {
 		// Start rebalancing
diff --git a/client/servers/manager.go b/client/servers/manager.go
index fea945dc5..d65fd43e0 100644
--- a/client/servers/manager.go
+++ b/client/servers/manager.go
@@ -201,12 +201,16 @@ func (m *Manager) SetServers(servers Servers) bool {
 	m.Lock()
 	defer m.Unlock()
 
-	// Sort both the  existing and incoming servers
-	servers.Sort()
-	m.servers.Sort()
-
 	// Determine if they are equal
-	equal := servers.Equal(m.servers)
+	equal := m.serversAreEqual(servers)
+
+	// If server list is equal don't change the list and return immediatly
+	// This prevents unnecessary shuffling of a failed server that was moved to the
+	// bottom of the list
+	if equal {
+		m.logger.Debug("Not replacing server list, current server list is identical to servers discovered in Consul")
+		return !equal
+	}
 
 	// Randomize the incoming servers
 	servers.shuffle()
@@ -215,6 +219,23 @@ func (m *Manager) SetServers(servers Servers) bool {
 	return !equal
 }
 
+// Method to check if the arg list of servers is equal to the one we already have
+func (m *Manager) serversAreEqual(servers Servers) bool {
+	// We use a copy of the server list here because determining
+	// equality requires a sort step which modifies the order of the server list
+	var copy Servers
+	copy = make([]*Server, 0, len(m.servers))
+	for _, s := range m.servers {
+		copy = append(copy, s.Copy())
+	}
+
+	// Sort both the  existing and incoming servers
+	copy.Sort()
+	servers.Sort()
+
+	return copy.Equal(servers)
+}
+
 // FindServer returns a server to send an RPC too. If there are no servers, nil
 // is returned.
 func (m *Manager) FindServer() *Server {
diff --git a/client/servers/manager_test.go b/client/servers/manager_test.go
index e33ab4119..8442d1e6f 100644
--- a/client/servers/manager_test.go
+++ b/client/servers/manager_test.go
@@ -66,6 +66,14 @@ func TestServers_SetServers(t *testing.T) {
 	require.True(m.SetServers([]*servers.Server{s1}))
 	require.Equal(1, m.NumServers())
 	require.Len(m.GetServers(), 1)
+
+	// Test that the list of servers does not get shuffled
+	// as a side effect when incoming list is equal
+	require.True(m.SetServers([]*servers.Server{s1, s2}))
+	before := m.GetServers()
+	require.False(m.SetServers([]*servers.Server{s1, s2}))
+	after := m.GetServers()
+	require.Equal(before, after)
 }
 
 func TestServers_FindServer(t *testing.T) {
diff --git a/dev/cluster/client3.hcl b/dev/cluster/client3.hcl
index 2702b79b0..ceb09715a 100644
--- a/dev/cluster/client3.hcl
+++ b/dev/cluster/client3.hcl
@@ -7,6 +7,9 @@ data_dir = "/tmp/client3"
 # Give the agent a unique name. Defaults to hostname
 name = "client3"
 
+# Enable debugging
+enable_debug = true
+
 # Enable the client
 client {
   enabled = true
diff --git a/dev/cluster/server1.hcl b/dev/cluster/server1.hcl
index d9a22e364..62b2c792a 100644
--- a/dev/cluster/server1.hcl
+++ b/dev/cluster/server1.hcl
@@ -16,5 +16,5 @@ server {
   }
 
   # Self-elect, should be 3 or 5 for production
-  bootstrap_expect = 1
+  bootstrap_expect = 3
 }