randomize servers

client/servers/manager.go

@@ -74,6 +74,16 @@ func (s *Server) String() string {
 	return s.addr
 }
 
+func (s *Server) Equal(o *Server) bool {
+	if s == nil && o == nil {
+		return true
+	} else if s == nil && o != nil || s != nil && o == nil {
+		return false
+	}
+
+	return s.Addr.String() == o.Addr.String() && s.DC == o.DC
+}
+
 type Servers []*Server
 
 func (s Servers) String() string {
@@ -160,6 +170,9 @@ func (m *Manager) Start() {
 func (m *Manager) SetServers(servers Servers) {
 	m.Lock()
 	defer m.Unlock()
+
+	// Randomize the incoming servers
+	servers.shuffle()
 	m.servers = servers
 }
 
@@ -204,7 +217,7 @@ func (m *Manager) NotifyFailedServer(s *Server) {
 	// 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, move the server to the end of the list.
-	if len(m.servers) > 1 && m.servers[0] == s {
+	if len(m.servers) > 1 && m.servers[0].Equal(s) {
 		m.servers.cycle()
 	}
 }

client/servers/manager_test.go

@@ -10,6 +10,7 @@ import (
 	"testing"
 
 	"github.com/hashicorp/nomad/client/servers"
+	"github.com/hashicorp/nomad/helper/testlog"
 )
 
 type fauxAddr struct {
@@ -32,22 +33,22 @@ func (cp *fauxConnPool) Ping(net.Addr) error {
 	return fmt.Errorf("bad server")
 }
 
-func testManager() (m *servers.Manager) {
-	logger := log.New(os.Stderr, "", log.LstdFlags)
+func testManager(t *testing.T) (m *servers.Manager) {
+	logger := testlog.Logger(t)
 	shutdownCh := make(chan struct{})
 	m = servers.New(logger, shutdownCh, &fauxConnPool{})
 	return m
 }
 
-func testManagerFailProb(failPct float64) (m *servers.Manager) {
-	logger := log.New(os.Stderr, "", log.LstdFlags)
+func testManagerFailProb(t *testing.T, failPct float64) (m *servers.Manager) {
+	logger := testlog.Logger(t)
 	shutdownCh := make(chan struct{})
 	m = servers.New(logger, shutdownCh, &fauxConnPool{failPct: failPct})
 	return m
 }
 
 func TestServers_SetServers(t *testing.T) {
-	m := testManager()
+	m := testManager(t)
 	var num int
 	num = m.NumServers()
 	if num != 0 {
@@ -66,14 +67,10 @@ func TestServers_SetServers(t *testing.T) {
 	if l := len(all); l != 2 {
 		t.Fatalf("expected 2 servers got %d", l)
 	}
-
-	if all[0] == s1 || all[0] == s2 {
-		t.Fatalf("expected a copy, got actual server")
-	}
 }
 
 func TestServers_FindServer(t *testing.T) {
-	m := testManager()
+	m := testManager(t)
 
 	if m.FindServer() != nil {
 		t.Fatalf("Expected nil return")
@@ -105,20 +102,14 @@ func TestServers_FindServer(t *testing.T) {
 		t.Fatalf("Expected two servers")
 	}
 	s1 = m.FindServer()
-	if s1 == nil || s1.String() != "s1" {
-		t.Fatalf("Expected s1 server (still)")
+
+	for _, srv := range srvs {
+		m.NotifyFailedServer(srv)
 	}
 
-	m.NotifyFailedServer(s1)
 	s2 := m.FindServer()
-	if s2 == nil || s2.String() != "s2" {
-		t.Fatalf("Expected s2 server")
-	}
-
-	m.NotifyFailedServer(s2)
-	s1 = m.FindServer()
-	if s1 == nil || s1.String() != "s1" {
-		t.Fatalf("Expected s1 server")
+	if s1.Equal(s2) {
+		t.Fatalf("Expected different server")
 	}
 }
 
@@ -132,7 +123,7 @@ func TestServers_New(t *testing.T) {
 }
 
 func TestServers_NotifyFailedServer(t *testing.T) {
-	m := testManager()
+	m := testManager(t)
 
 	if m.NumServers() != 0 {
 		t.Fatalf("Expected zero servers to start")
@@ -159,32 +150,39 @@ func TestServers_NotifyFailedServer(t *testing.T) {
 		t.Fatalf("Expected two servers")
 	}
 
-	s1 = m.FindServer()
-	if s1 == nil || s1.String() != "s1" {
-		t.Fatalf("Expected s1 server")
+	// Grab a server
+	first := m.FindServer()
+
+	// Find the other server
+	second := s1
+	if first.Equal(s1) {
+		second = s2
 	}
 
-	m.NotifyFailedServer(s2)
-	s1 = m.FindServer()
-	if s1 == nil || s1.String() != "s1" {
-		t.Fatalf("Expected s1 server (still)")
+	// Fail the other server
+	m.NotifyFailedServer(second)
+	next := m.FindServer()
+	if !next.Equal(first) {
+		t.Fatalf("Expected first server (still)")
 	}
 
-	m.NotifyFailedServer(s1)
-	s2 = m.FindServer()
-	if s2 == nil || s2.String() != "s2" {
-		t.Fatalf("Expected s2 server")
+	// Fail the first
+	m.NotifyFailedServer(first)
+	next = m.FindServer()
+	if !next.Equal(second) {
+		t.Fatalf("Expected second server")
 	}
 
-	m.NotifyFailedServer(s2)
-	s1 = m.FindServer()
-	if s1 == nil || s1.String() != "s1" {
-		t.Fatalf("Expected s1 server")
+	// Fail the second
+	m.NotifyFailedServer(second)
+	next = m.FindServer()
+	if !next.Equal(first) {
+		t.Fatalf("Expected first server")
 	}
 }
 
 func TestServers_NumServers(t *testing.T) {
-	m := testManager()
+	m := testManager(t)
 	var num int
 	num = m.NumServers()
 	if num != 0 {
@@ -201,7 +199,7 @@ func TestServers_NumServers(t *testing.T) {
 
 func TestServers_RebalanceServers(t *testing.T) {
 	const failPct = 0.5
-	m := testManagerFailProb(failPct)
+	m := testManagerFailProb(t, failPct)
 	const maxServers = 100
 	const numShuffleTests = 100
 	const uniquePassRate = 0.5