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 }