From e944370cde6cde959b3b813baec3966e133d089f Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Wed, 30 Aug 2017 12:31:36 -0500 Subject: [PATCH] More cleanup from code review --- agent/consul/rpc.go | 2 +- agent/consul/serf.go | 6 +- agent/consul/server.go | 4 +- agent/consul/server_address_lookup.go | 56 ---------------- agent/consul/server_lookup.go | 66 +++++++++++++++++++ ...s_lookup_test.go => server_lookup_test.go} | 4 +- agent/consul/server_test.go | 4 +- 7 files changed, 76 insertions(+), 66 deletions(-) delete mode 100644 agent/consul/server_address_lookup.go create mode 100644 agent/consul/server_lookup.go rename agent/consul/{server_address_lookup_test.go => server_lookup_test.go} (94%) diff --git a/agent/consul/rpc.go b/agent/consul/rpc.go index baf0c4c61..8afd09d30 100644 --- a/agent/consul/rpc.go +++ b/agent/consul/rpc.go @@ -238,7 +238,7 @@ func (s *Server) getLeader() (bool, *metadata.Server) { } // Lookup the server - server, _ := s.serverLookup.GetServer(leader) + server := s.serverLookup.Server(leader) // Server could be nil return false, server diff --git a/agent/consul/serf.go b/agent/consul/serf.go index 27048a834..779ab0154 100644 --- a/agent/consul/serf.go +++ b/agent/consul/serf.go @@ -261,13 +261,13 @@ 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 := metadata.IsConsulServer(m) + ok, serverMeta := metadata.IsConsulServer(m) if !ok { continue } - s.logger.Printf("[INFO] consul: Removing LAN server %s", parts) + s.logger.Printf("[INFO] consul: Removing LAN server %s", serverMeta) // Update id to address map - s.serverLookup.RemoveServer(parts) + s.serverLookup.RemoveServer(serverMeta) } } diff --git a/agent/consul/server.go b/agent/consul/server.go index 9d63239b9..e0015bb9f 100644 --- a/agent/consul/server.go +++ b/agent/consul/server.go @@ -700,9 +700,9 @@ func (s *Server) setupRPC(tlsWrap tlsutil.DCWrapper) error { return true } - server, ok := s.serverLookup.GetServer(address) + server := s.serverLookup.Server(address) - if !ok { + if server == nil { return false } diff --git a/agent/consul/server_address_lookup.go b/agent/consul/server_address_lookup.go deleted file mode 100644 index ac1d33937..000000000 --- a/agent/consul/server_address_lookup.go +++ /dev/null @@ -1,56 +0,0 @@ -package consul - -import ( - "fmt" - "sync" - - "github.com/hashicorp/consul/agent/metadata" - "github.com/hashicorp/raft" -) - -// ServerLookup encapsulates looking up servers by id and address -type ServerLookup struct { - lock sync.RWMutex - addressToServer map[raft.ServerAddress]*metadata.Server - IdToServer map[raft.ServerID]*metadata.Server -} - -func NewServerLookup() *ServerLookup { - return &ServerLookup{addressToServer: make(map[raft.ServerAddress]*metadata.Server), IdToServer: make(map[raft.ServerID]*metadata.Server)} -} - -func (sa *ServerLookup) AddServer(server *metadata.Server) { - sa.lock.Lock() - defer sa.lock.Unlock() - sa.addressToServer[raft.ServerAddress(server.Addr.String())] = server - sa.IdToServer[raft.ServerID(server.ID)] = server -} - -func (sa *ServerLookup) RemoveServer(server *metadata.Server) { - sa.lock.Lock() - defer sa.lock.Unlock() - delete(sa.addressToServer, raft.ServerAddress(server.Addr.String())) - delete(sa.IdToServer, raft.ServerID(server.ID)) -} - -// Implements the ServerAddressProvider interface -func (sa *ServerLookup) ServerAddr(id raft.ServerID) (raft.ServerAddress, error) { - sa.lock.RLock() - defer sa.lock.RUnlock() - svr, ok := sa.IdToServer[id] - if !ok { - return "", fmt.Errorf("Could not find address for server id %v", id) - } - return raft.ServerAddress(svr.Addr.String()), nil -} - -// GetServer looks up the server by address, returns a boolean if not found -func (sa *ServerLookup) GetServer(addr raft.ServerAddress) (*metadata.Server, bool) { - sa.lock.RLock() - defer sa.lock.RUnlock() - svr, ok := sa.addressToServer[addr] - if !ok { - return nil, false - } - return svr, true -} diff --git a/agent/consul/server_lookup.go b/agent/consul/server_lookup.go new file mode 100644 index 000000000..e163856d7 --- /dev/null +++ b/agent/consul/server_lookup.go @@ -0,0 +1,66 @@ +package consul + +import ( + "fmt" + "sync" + + "github.com/hashicorp/consul/agent/metadata" + "github.com/hashicorp/raft" +) + +// ServerLookup encapsulates looking up servers by id and address +type ServerLookup struct { + lock sync.RWMutex + addressToServer map[raft.ServerAddress]*metadata.Server + idToServer map[raft.ServerID]*metadata.Server +} + +func NewServerLookup() *ServerLookup { + return &ServerLookup{ + addressToServer: make(map[raft.ServerAddress]*metadata.Server), + idToServer: make(map[raft.ServerID]*metadata.Server), + } +} + +func (sl *ServerLookup) AddServer(server *metadata.Server) { + sl.lock.Lock() + defer sl.lock.Unlock() + sl.addressToServer[raft.ServerAddress(server.Addr.String())] = server + sl.idToServer[raft.ServerID(server.ID)] = server +} + +func (sl *ServerLookup) RemoveServer(server *metadata.Server) { + sl.lock.Lock() + defer sl.lock.Unlock() + delete(sl.addressToServer, raft.ServerAddress(server.Addr.String())) + delete(sl.idToServer, raft.ServerID(server.ID)) +} + +// Implements the ServerAddressProvider interface +func (sl *ServerLookup) ServerAddr(id raft.ServerID) (raft.ServerAddress, error) { + sl.lock.RLock() + defer sl.lock.RUnlock() + svr, ok := sl.idToServer[id] + if !ok { + return "", fmt.Errorf("Could not find address for server id %v", id) + } + return raft.ServerAddress(svr.Addr.String()), nil +} + +// Server looks up the server by address, returns a boolean if not found +func (sl *ServerLookup) Server(addr raft.ServerAddress) *metadata.Server { + sl.lock.RLock() + defer sl.lock.RUnlock() + svr, _ := sl.addressToServer[addr] + return svr +} + +func (sl *ServerLookup) Servers() []*metadata.Server { + sl.lock.RLock() + defer sl.lock.RUnlock() + var ret []*metadata.Server + for _, svr := range sl.addressToServer { + ret = append(ret, svr) + } + return ret +} diff --git a/agent/consul/server_address_lookup_test.go b/agent/consul/server_lookup_test.go similarity index 94% rename from agent/consul/server_address_lookup_test.go rename to agent/consul/server_lookup_test.go index d1189bb6e..6e5a93d68 100644 --- a/agent/consul/server_address_lookup_test.go +++ b/agent/consul/server_lookup_test.go @@ -36,8 +36,8 @@ func TestServerLookup(t *testing.T) { t.Fatalf("Expected %v but got %v", addr, got) } - server, ok := lookup.GetServer(raft.ServerAddress(addr)) - if !ok { + server := lookup.Server(raft.ServerAddress(addr)) + if server == nil { t.Fatalf("Expected lookup to return true") } if server.Addr.String() != addr { diff --git a/agent/consul/server_test.go b/agent/consul/server_test.go index 0d921e126..c646f66d1 100644 --- a/agent/consul/server_test.go +++ b/agent/consul/server_test.go @@ -342,7 +342,7 @@ func TestServer_JoinSeparateLanAndWanAddresses(t *testing.T) { if len(s2.router.GetDatacenters()) != 2 { r.Fatalf("remote consul missing") } - if len(s2.serverLookup.addressToServer) != 2 { + if len(s2.serverLookup.Servers()) != 2 { r.Fatalf("local consul fellow s3 for s2 missing") } }) @@ -667,7 +667,7 @@ func testVerifyRPC(s1, s2 *Server, t *testing.T) (bool, error) { // Have s2 make an RPC call to s1 var leader *metadata.Server - for _, server := range s2.serverLookup.addressToServer { + for _, server := range s2.serverLookup.Servers() { if server.Name == s1.config.NodeName { leader = server }