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() {