diff --git a/src/bgp/bgp_server.cc b/src/bgp/bgp_server.cc index 99085bca71f..8b08a9721ac 100644 --- a/src/bgp/bgp_server.cc +++ b/src/bgp/bgp_server.cc @@ -484,35 +484,42 @@ BgpPeer *BgpServer::FindPeer(const string &name) { return (loc != peer_list_.end() ? loc->second : NULL); } +BgpPeer *BgpServer::FindNextPeer(const string &name) { + BgpPeerList::iterator loc = peer_list_.upper_bound(name); + return (loc != peer_list_.end() ? loc->second : NULL); +} + void BgpServer::InsertPeer(TcpSession::Endpoint remote, BgpPeer *peer) { if (!remote.port() && remote.address().is_unspecified()) return; - - EndpointToBgpPeerList::iterator loc = endpoint_peer_list_.find(remote); - if (loc != endpoint_peer_list_.end()) { - loc->second->Clear(BgpProto::Notification::PeerDeconfigured); - endpoint_peer_list_.erase(loc); - } endpoint_peer_list_.insert(make_pair(remote, peer)); } void BgpServer::RemovePeer(TcpSession::Endpoint remote, BgpPeer *peer) { - EndpointToBgpPeerList::iterator loc = endpoint_peer_list_.find(remote); - if (loc != endpoint_peer_list_.end() && loc->second == peer) { - endpoint_peer_list_.erase(loc); + EndpointPeerList::iterator loc = endpoint_peer_list_.lower_bound(remote); + while (loc != endpoint_peer_list_.end() && loc->first == remote) { + if (loc->second == peer) { + endpoint_peer_list_.erase(loc); + break; + } + ++loc; } } BgpPeer *BgpServer::FindPeer(TcpSession::Endpoint remote) const { - EndpointToBgpPeerList::const_iterator loc = - endpoint_peer_list_.find(remote); + EndpointPeerList::const_iterator loc = endpoint_peer_list_.find(remote); return (loc == endpoint_peer_list_.end() ? NULL : loc->second); } -BgpPeer *BgpServer::FindNextPeer(TcpSession::Endpoint remote) const { - EndpointToBgpPeerList::const_iterator loc = - endpoint_peer_list_.upper_bound(remote); - return (loc == endpoint_peer_list_.end() ? NULL : loc->second); +BgpPeer *BgpServer::FindExactPeer(const BgpPeer *peer) const { + EndpointPeerList::const_iterator loc = + endpoint_peer_list_.lower_bound(peer->endpoint()); + while (loc != endpoint_peer_list_.end() && loc->first == peer->endpoint()) { + if (loc->second == peer) + return loc->second; + ++loc; + } + return NULL; } const string &BgpServer::localname() const { diff --git a/src/bgp/bgp_server.h b/src/bgp/bgp_server.h index 373a39c0d8d..9797d222af9 100644 --- a/src/bgp/bgp_server.h +++ b/src/bgp/bgp_server.h @@ -73,11 +73,11 @@ class BgpServer { int RegisterPeer(BgpPeer *peer); void UnregisterPeer(BgpPeer *peer); BgpPeer *FindPeer(const std::string &name); + BgpPeer *FindNextPeer(const std::string &name = std::string()); void InsertPeer(TcpSession::Endpoint remote, BgpPeer *peer); void RemovePeer(TcpSession::Endpoint remote, BgpPeer *peer); BgpPeer *FindPeer(TcpSession::Endpoint remote) const; - BgpPeer *FindNextPeer( - TcpSession::Endpoint remote = TcpSession::Endpoint()) const; + BgpPeer *FindExactPeer(const BgpPeer *peer) const; void Shutdown(); @@ -263,7 +263,7 @@ class BgpServer { typedef std::vector AdminDownListenersList; typedef std::vector ASNUpdateListenersList; typedef std::vector IdentifierUpdateListenersList; - typedef std::map EndpointToBgpPeerList; + typedef std::multimap EndpointPeerList; void RoutingInstanceMgrDeletionComplete(RoutingInstanceMgr *mgr); void FillPeerStats(const BgpPeer *peer) const; @@ -293,7 +293,7 @@ class BgpServer { tbb::atomic num_up_bgpaas_peer_; tbb::atomic deleting_bgpaas_count_; BgpPeerList peer_list_; - EndpointToBgpPeerList endpoint_peer_list_; + EndpointPeerList endpoint_peer_list_; boost::scoped_ptr lifetime_manager_; boost::scoped_ptr deleter_; diff --git a/src/bgp/bgp_session_manager.cc b/src/bgp/bgp_session_manager.cc index cf4f44a9987..40cc8a0fabe 100644 --- a/src/bgp/bgp_session_manager.cc +++ b/src/bgp/bgp_session_manager.cc @@ -99,7 +99,7 @@ bool BgpSessionManager::ProcessWriteReady(TcpSessionPtr tcp_session) { // // Search for a matching BgpPeer. // First look for a matching address in the master instance. -// Then look for a matching port in the EndpointToBgpPeerList in BgpServer. +// Then look for a matching port in the EndpointPeerList in BgpServer. // BgpPeer *BgpSessionManager::FindPeer(Endpoint remote) { BgpPeer *peer = NULL; diff --git a/src/bgp/bgp_show_neighbor.cc b/src/bgp/bgp_show_neighbor.cc index 6eaa26d183f..4637dbdd067 100644 --- a/src/bgp/bgp_show_neighbor.cc +++ b/src/bgp/bgp_show_neighbor.cc @@ -18,30 +18,25 @@ using std::string; using std::vector; // -// Build the list of BgpPeers in one shot for now. Look at the master instance -// and at all peers in the BgpServer's EndpointToBgpPeerList. The latter is the -// list of all bgpaas peers in non-master instances. +// Build the list of BgpPeers in one shot for now. // static bool FillBgpNeighborInfoList(const BgpSandeshContext *bsc, bool summary, uint32_t page_limit, uint32_t iter_limit, const string &start_neighbor, const string &search_string, vector *show_list, string *next_instance) { - const RoutingInstanceMgr *rim = bsc->bgp_server->routing_instance_mgr(); - const RoutingInstance *rtinstance = rim->GetDefaultRoutingInstance(); - rtinstance->peer_manager()->FillBgpNeighborInfo( - bsc, show_list, search_string, summary); - regex search_expr(search_string); - const BgpPeer *peer = bsc->bgp_server->FindNextPeer(); - while (peer) { - if ((regex_search(peer->peer_basename(), search_expr)) || - (regex_search(peer->peer_address_string(), search_expr)) || - (search_string == "deleted" && peer->IsDeleted())) { - BgpNeighborResp bnr; - peer->FillNeighborInfo(bsc, &bnr, summary); - show_list->push_back(bnr); + + for (const BgpPeer *peer = bsc->bgp_server->FindNextPeer(); peer != NULL; + peer = bsc->bgp_server->FindNextPeer(peer->peer_name())) { + if ((!regex_search(peer->peer_basename(), search_expr)) && + (!regex_search(peer->peer_address_string(), search_expr)) && + (search_string != "deleted" || !peer->IsDeleted())) { + continue; } - peer = bsc->bgp_server->FindNextPeer(peer->endpoint()); + + BgpNeighborResp bnr; + peer->FillNeighborInfo(bsc, &bnr, summary); + show_list->push_back(bnr); } return true; } diff --git a/src/bgp/routing-instance/peer_manager.cc b/src/bgp/routing-instance/peer_manager.cc index d1b743f64fa..6769aa9d90e 100644 --- a/src/bgp/routing-instance/peer_manager.cc +++ b/src/bgp/routing-instance/peer_manager.cc @@ -59,7 +59,7 @@ BgpPeer *PeerManager::PeerLocate(BgpServer *server, // // Resurrect the BgpPeer with given name if we have configuration for it. -// Also insert it into the BgpServer's EndpointToBgpPeerList - in case it's +// Also insert it into the BgpServer's EndpointPeerList - in case it is // a BGPaaS peer. This needs to happen here since we would not have been // able to do it from BgpServer::ConfigUpdater::ProcessNeighborConfig since // old incarnation of the BgpPeer still existed at that point. @@ -260,20 +260,3 @@ const string &PeerManager::name() const { BgpServer *PeerManager::server() const { return instance_->server(); } - -void PeerManager::FillBgpNeighborInfo(const BgpSandeshContext *bsc, - vector *bnr_list, const string &search_string, - bool summary) const { - regex search_expr(search_string); - BgpPeerKey key = BgpPeerKey(); - while (const BgpPeer *peer = NextPeer(key)) { - if ((regex_search(peer->peer_basename(), search_expr)) || - (regex_search(peer->peer_address_string(), search_expr)) || - (search_string == "deleted" && peer->IsDeleted())) { - BgpNeighborResp bnr; - peer->FillNeighborInfo(bsc, &bnr, summary); - bnr_list->push_back(bnr); - } - key = peer->peer_key(); - } -} diff --git a/src/bgp/routing-instance/peer_manager.h b/src/bgp/routing-instance/peer_manager.h index fc0a9013afd..a36c40d92a1 100644 --- a/src/bgp/routing-instance/peer_manager.h +++ b/src/bgp/routing-instance/peer_manager.h @@ -40,11 +40,6 @@ class PeerManager { void ClearAllPeers(); const BgpPeer *NextPeer(const BgpPeerKey &key) const; - - void FillBgpNeighborInfo(const BgpSandeshContext *bsc, - std::vector *sbnr_list, - const std::string &search_string, bool summary) const; - size_t GetNeighborCount(std::string up_or_down); size_t size() { return peers_by_key_.size(); } diff --git a/src/bgp/test/bgp_config_test.cc b/src/bgp/test/bgp_config_test.cc index 1b9645e5313..1fca068a43e 100644 --- a/src/bgp/test/bgp_config_test.cc +++ b/src/bgp/test/bgp_config_test.cc @@ -521,7 +521,7 @@ TEST_F(BgpConfigTest, BGPaaSNeighbors2) { TASK_UTIL_EXPECT_EQ(0, server_.num_bgpaas_peer()); } -TEST_F(BgpConfigTest, BGPaaSNeighbors3) { +TEST_F(BgpConfigTest, BGPaaSNeighbors3a) { string content; content = FileRead("controller/src/bgp/testdata/config_test_36a.xml"); EXPECT_TRUE(parser_.Parse(content)); @@ -548,9 +548,6 @@ TEST_F(BgpConfigTest, BGPaaSNeighbors3) { TASK_UTIL_EXPECT_EQ(1025, peer2->peer_port()); TASK_UTIL_EXPECT_EQ(peer2, server_.FindPeer(peer2->endpoint())); - // Save the old endpoint for peer1. - TcpSession::Endpoint old_peer1_endpoint = peer1->endpoint(); - // Set test::vm1 port to be same as port for test:vm2. content = FileRead("controller/src/bgp/testdata/config_test_36c.xml"); EXPECT_TRUE(parser_.Parse(content)); @@ -562,12 +559,11 @@ TEST_F(BgpConfigTest, BGPaaSNeighbors3) { // Verify that the port is identical for test:vm2. TASK_UTIL_EXPECT_EQ(1025, peer2->peer_port()); - // Verify that test:vm1 is inserted into BgpServer::EndpointToBgpPeerList. - // Verify that there's no entry for the old remote endpoint for test:vm1. - // Note that test:vm2 is removed from BgpServer::EndpointToBgpPeerList when - // test:vm1 is inserted with the same remote endpoint. - TASK_UTIL_EXPECT_EQ(peer1, server_.FindPeer(peer1->endpoint())); - TASK_UTIL_EXPECT_TRUE(server_.FindPeer(old_peer1_endpoint) == NULL); + // Verify that test:vm1 is inserted into BgpServer::EndpointPeerList. + // Verify that test:vm2 is inserted into BgpServer::EndpointPeerList. + // Both peers have the same endpoint. + TASK_UTIL_EXPECT_EQ(peer1, server_.FindExactPeer(peer1)); + TASK_UTIL_EXPECT_EQ(peer2, server_.FindExactPeer(peer2)); // Set test::vm2 port to be same as old port for test:vm1. content = FileRead("controller/src/bgp/testdata/config_test_36d.xml"); @@ -577,8 +573,81 @@ TEST_F(BgpConfigTest, BGPaaSNeighbors3) { // Verify that the port is updated for test:vm2. TASK_UTIL_EXPECT_EQ(1024, peer2->peer_port()); - // Verify that test:vm1 is inserted into BgpServer::EndpointToBgpPeerList. - // Verify that test:vm2 is inserted into BgpServer::EndpointToBgpPeerList. + // Verify that test:vm1 is inserted into BgpServer::EndpointPeerList. + // Verify that test:vm2 is inserted into BgpServer::EndpointPeerList. + TASK_UTIL_EXPECT_EQ(peer1, server_.FindPeer(peer1->endpoint())); + TASK_UTIL_EXPECT_EQ(peer2, server_.FindPeer(peer2->endpoint())); + + // Cleanup. + boost::replace_all(content, "", ""); + boost::replace_all(content, "", ""); + EXPECT_TRUE(parser_.Parse(content)); + task_util::WaitForIdle(); + + // Ensure that the instance is deleted + TASK_UTIL_EXPECT_EQ(static_cast(NULL), + server_.routing_instance_mgr()->GetRoutingInstance("test")); + TASK_UTIL_EXPECT_EQ(0, server_.num_bgp_peer()); + TASK_UTIL_EXPECT_EQ(0, server_.num_bgpaas_peer()); +} + +TEST_F(BgpConfigTest, BGPaaSNeighbors3b) { + string content; + content = FileRead("controller/src/bgp/testdata/config_test_36a.xml"); + EXPECT_TRUE(parser_.Parse(content)); + task_util::WaitForIdle(); + + RoutingInstance *rti = + server_.routing_instance_mgr()->GetRoutingInstance("test"); + TASK_UTIL_ASSERT_TRUE(rti != NULL); + TASK_UTIL_EXPECT_EQ(2, rti->peer_manager_size()); + TASK_UTIL_EXPECT_EQ(0, server_.num_bgp_peer()); + TASK_UTIL_EXPECT_EQ(2, server_.num_bgpaas_peer()); + + // Verify that the port is set for test:vm1. + TASK_UTIL_EXPECT_TRUE( + rti->peer_manager()->PeerLookup("test:vm1:0") != NULL); + BgpPeer *peer1 = rti->peer_manager()->PeerLookup("test:vm1:0"); + TASK_UTIL_EXPECT_EQ(1024, peer1->peer_port()); + TASK_UTIL_EXPECT_EQ(peer1, server_.FindPeer(peer1->endpoint())); + + // Verify that the port is set for test:vm2. + TASK_UTIL_EXPECT_TRUE( + rti->peer_manager()->PeerLookup("test:vm2:0") != NULL); + BgpPeer *peer2 = rti->peer_manager()->PeerLookup("test:vm2:0"); + TASK_UTIL_EXPECT_EQ(1025, peer2->peer_port()); + TASK_UTIL_EXPECT_EQ(peer2, server_.FindPeer(peer2->endpoint())); + + // Set test::vm1 port to be same as port for test:vm2. + content = FileRead("controller/src/bgp/testdata/config_test_36c.xml"); + EXPECT_TRUE(parser_.Parse(content)); + task_util::WaitForIdle(); + + // Verify that the port is updated for test:vm1. + TASK_UTIL_EXPECT_EQ(1025, peer1->peer_port()); + + // Verify that the port is identical for test:vm2. + TASK_UTIL_EXPECT_EQ(1025, peer2->peer_port()); + + // Verify that test:vm1 is inserted into BgpServer::EndpointPeerList. + // Verify that test:vm2 is inserted into BgpServer::EndpointPeerList. + // Both peers have the same endpoint. + TASK_UTIL_EXPECT_EQ(peer1, server_.FindExactPeer(peer1)); + TASK_UTIL_EXPECT_EQ(peer2, server_.FindExactPeer(peer2)); + + // Set test::vm1 port back to be same as original port for test:vm1. + content = FileRead("controller/src/bgp/testdata/config_test_36a.xml"); + EXPECT_TRUE(parser_.Parse(content)); + task_util::WaitForIdle(); + + // Verify that the port is updated for test:vm1. + TASK_UTIL_EXPECT_EQ(1024, peer1->peer_port()); + + // Verify that the port is unchanged for test:vm2. + TASK_UTIL_EXPECT_EQ(1025, peer2->peer_port()); + + // Verify that test:vm1 is inserted into BgpServer::EndpointPeerList. + // Verify that test:vm2 is inserted into BgpServer::EndpointPeerList. TASK_UTIL_EXPECT_EQ(peer1, server_.FindPeer(peer1->endpoint())); TASK_UTIL_EXPECT_EQ(peer2, server_.FindPeer(peer2->endpoint())); @@ -922,7 +991,7 @@ TEST_F(BgpConfigTest, BGPaaSNeighbors9) { TASK_UTIL_EXPECT_TRUE(server_.FindPeer(peer2->endpoint()) == NULL); // Recreate neighbor config. The old peers should still be around - // but should not be in the BgpServer::EndpointToBgpPeerList. + // but should not be in the BgpServer::EndpointPeerList. content = FileRead("controller/src/bgp/testdata/config_test_36a.xml"); EXPECT_TRUE(parser_.Parse(content)); task_util::WaitForIdle();