From 38237b107e4972ae1cadcd7e1b84a2ca00111568 Mon Sep 17 00:00:00 2001 From: Manish Singh Date: Wed, 30 Mar 2016 07:18:50 +0530 Subject: [PATCH] Route present wih no paths. Problem: When only stale paths in headless mode were present and CN goes down, then walk was issued to squash all of them. However after squashing it didnt check if route has no paths and delete the same. So route was present with no paths. Solution: Re-org the squashing code in walk to go via proper removal of path and check for zero path to delete route. Closes-bug: #1562961 Conflicts: src/vnsw/agent/test/test_l2route.cc Conflicts: src/vnsw/agent/test/test_l2route.cc Conflicts: src/vnsw/agent/test/test_l2route.cc Double path delete request in succession. Problem: In stale path cleanup, if no stale path was found function used to clean last path seen. This resulted in not related path getting deleted. In case of OVS delete the path to be deleted was already gone because of bug and table deleted, resulting in ceash. Solution: Delete if relevant path is found. Closes-bug: #1571598 Change-Id: I25bd7cec4c0774d0a041286c15af99bc5a2d1ada --- src/vnsw/agent/oper/agent_route.cc | 54 ++++++++++------ src/vnsw/agent/oper/agent_route.h | 3 +- src/vnsw/agent/test/test_l2route.cc | 96 +++++++++++++++++++++++++++++ 3 files changed, 133 insertions(+), 20 deletions(-) diff --git a/src/vnsw/agent/oper/agent_route.cc b/src/vnsw/agent/oper/agent_route.cc index 0029ff88ca0..117647ec815 100644 --- a/src/vnsw/agent/oper/agent_route.cc +++ b/src/vnsw/agent/oper/agent_route.cc @@ -146,7 +146,7 @@ bool AgentRouteTable::DeleteAllBgpPath(DBTablePartBase *part, DeletePathFromPeer(part, route, path); to_be_deleted_path_list_it++; } - route->SquashStalePaths(NULL); + SquashStalePaths(route, NULL); } return true; } @@ -255,7 +255,10 @@ void AgentRouteTable::DeletePathFromPeer(DBTablePartBase *part, // For active peers reflector will remove the route but for // non active peers explicitly squash the paths. if (peer && (peer->GetType() != Peer::BGP_PEER)) { - rt->SquashStalePaths(NULL); + path = rt->FindStalePath(); + if (path) { + rt->RemovePath(path); + } } // Delete route if no more paths @@ -519,6 +522,23 @@ void AgentRouteTable::NotifyEntry(AgentRoute *e) { tpart->Notify(e); } +void AgentRouteTable::SquashStalePaths(AgentRoute *route, + const AgentPath *exception_path) { + Route::PathList::iterator it = route->GetPathList().begin(); + AgentPath *path = NULL; + while (it != route->GetPathList().end()) { + path = static_cast(it.operator->()); + // Delete all stale path except for the path sent(exception_path) + if (path->is_stale() && (path != exception_path)) { + // Since we squash stales, at any point of time there should be only + // one stale other than exception_path in list + DeletePathFromPeer(route->get_table_partition(), route, path); + return; + } + it++; + } +} + uint32_t AgentRoute::GetActiveLabel() const { return GetActivePath()->label(); }; @@ -581,6 +601,18 @@ AgentPath *AgentRoute::FindLocalVmPortPath() const { return NULL; } +AgentPath *AgentRoute::FindStalePath() { + Route::PathList::iterator it = GetPathList().begin(); + while (it != GetPathList().end()) { + AgentPath *path = static_cast(it.operator->()); + if (path->is_stale()) { + return path; + } + it++; + } + return NULL; +} + void AgentRoute::DeletePathInternal(AgentPath *path) { AgentRouteTable *table = static_cast(get_table()); table->DeletePathFromPeer(get_table_partition(), this, path); @@ -616,22 +648,6 @@ AgentPath *AgentRoute::FindPath(const Peer *peer) const { return NULL; } -void AgentRoute::SquashStalePaths(const AgentPath *exception_path) { - Route::PathList::iterator it = GetPathList().begin(); - - while (it != GetPathList().end()) { - // Delete all stale path except for the path sent(exception_path) - AgentPath *path = static_cast(it.operator->()); - if (path->is_stale() && (path != exception_path)) { - // Since we squash stales, at any point of time there should be only - // one stale other than exception_path in list - RemovePath(path); - return; - } - it++; - } -} - // First path in list is always treated as active path. const AgentPath *AgentRoute::GetActivePath() const { return static_cast(front()); @@ -753,7 +769,7 @@ void AgentRouteTable::StalePathFromPeer(DBTablePartBase *part, AgentRoute *rt, if (rt && (rt->IsDeleted() == false)) { path->set_is_stale(true); // Remove all stale path except the path received - rt->SquashStalePaths(path); + SquashStalePaths(rt, path); rt->GetPathList().sort(&AgentRouteTable::PathSelection); rt->Sync(); part->Notify(rt); diff --git a/src/vnsw/agent/oper/agent_route.h b/src/vnsw/agent/oper/agent_route.h index 48468ad249a..77281ddfe67 100644 --- a/src/vnsw/agent/oper/agent_route.h +++ b/src/vnsw/agent/oper/agent_route.h @@ -181,6 +181,7 @@ class AgentRouteTable : public RouteTable { //Stale path handling void StalePathFromPeer(DBTablePartBase *part, AgentRoute *rt, const Peer *peer); + void SquashStalePaths(AgentRoute *rt, const AgentPath *path); private: class DeleteActor; @@ -253,6 +254,7 @@ class AgentRoute : public Route { uint32_t vrf_id() const; AgentPath *FindLocalVmPortPath() const; + AgentPath *FindStalePath(); const AgentPath *GetActivePath() const; const NextHop *GetActiveNextHop() const; const std::string &dest_vn_name() const; @@ -262,7 +264,6 @@ class AgentRoute : public Route { void ResyncTunnelNextHop(); bool HasUnresolvedPath(); bool Sync(void); - void SquashStalePaths(const AgentPath *path); //TODO Move dependantroutes and nh to inet4 void UpdateDependantRoutes();// analogous to updategatewayroutes diff --git a/src/vnsw/agent/test/test_l2route.cc b/src/vnsw/agent/test/test_l2route.cc index c11d00b7a5e..3f9d2234a83 100644 --- a/src/vnsw/agent/test/test_l2route.cc +++ b/src/vnsw/agent/test/test_l2route.cc @@ -1545,6 +1545,102 @@ TEST_F(RouteTest, add_local_peer_and_then_vm) { bgp_peer.reset(); } +//Bug# 1562961 +TEST_F(RouteTest, StalePathDeleteRouteDelete) { + struct PortInfo input[] = { + {"vnet1", 1, "1.1.1.10", "00:00:01:01:01:10", 1, 1}, + }; + + client->Reset(); + CreateVmportEnv(input, 1); + client->WaitForIdle(); + + //Add a peer and keep a reference of same. + BgpPeer *bgp_peer_ptr = CreateBgpPeer(Ip4Address(1), "BGP Peer1"); + boost::shared_ptr bgp_peer = + bgp_peer_ptr->GetBgpXmppPeer()->bgp_peer_id_ref(); + + BridgeTunnelRouteAdd(bgp_peer_ptr, vrf_name_, TunnelType::MplsType(), + server1_ip_, MplsTable::kStartLabel + 10, + remote_vm_mac_, remote_vm_ip4_, 32); + client->WaitForIdle(); + + EvpnRouteEntry *rt = EvpnRouteGet(vrf_name_, remote_vm_mac_, + remote_vm_ip4_, 0); + EXPECT_TRUE(rt != NULL); + AgentPath *path = rt->FindPath(bgp_peer_ptr); + path->set_is_stale(true); + DelVrf(vrf_name_.c_str()); + client->WaitForIdle(); + rt = EvpnRouteGet(vrf_name_, remote_vm_mac_, + remote_vm_ip4_, 0); + EXPECT_TRUE(rt == NULL); + + DeleteBgpPeer(bgp_peer.get()); + client->WaitForIdle(); + DeleteVmportEnv(input, 1, true); + client->WaitForIdle(); + //Release VRF reference + bgp_peer.reset(); + client->WaitForIdle(); +} + +class SetupTask : public Task { + public: + SetupTask(RouteTest *test, std::string name) : + Task((TaskScheduler::GetInstance()-> + GetTaskId("db::DBTable")), 0), test_(test), + test_name_(name) { + } + + virtual bool Run() { + if (test_name_ == "SquashPathTest_1") { + char local_vm_mac_str_[100]; + MacAddress local_vm_mac_; + Ip4Address local_vm_ip4_; + char local_vm_ip4_str_[100]; + strcpy(local_vm_ip4_str_, "1.1.1.10"); + local_vm_ip4_ = Ip4Address::from_string(local_vm_ip4_str_); + strcpy(local_vm_mac_str_, "00:00:01:01:01:10"); + local_vm_mac_ = MacAddress::FromString(local_vm_mac_str_); + EvpnRouteEntry *rt = EvpnRouteGet("vrf1", + local_vm_mac_, + local_vm_ip4_, + 0); + const VrfEntry *vrf = VrfGet("vrf1"); + vrf->GetEvpnRouteTable()->SquashStalePaths(rt, NULL); + } + return true; + } + std::string Description() const { return "SetupTask"; } + private: + RouteTest *test_; + std::string test_name_; +}; + +//Bug# 1571598 +TEST_F(RouteTest, SquashPathTest_1) { + struct PortInfo input[] = { + {"vnet1", 1, "1.1.1.10", "00:00:01:01:01:10", 1, 1}, + }; + + client->Reset(); + CreateVmportEnv(input, 1); + client->WaitForIdle(); + + EvpnRouteEntry *rt = EvpnRouteGet("vrf1", local_vm_mac_, local_vm_ip4_, 0); + EXPECT_TRUE(rt != NULL); + EXPECT_TRUE(rt->GetActivePath() != NULL); + SetupTask * task = new SetupTask(this, "SquashPathTest_1"); + TaskScheduler::GetInstance()->Enqueue(task); + client->WaitForIdle(); + rt = EvpnRouteGet("vrf1", local_vm_mac_, local_vm_ip4_, 0); + EXPECT_TRUE(rt != NULL); + + DeleteVmportEnv(input, 1, true); + client->WaitForIdle(); +} + int main(int argc, char *argv[]) { ::testing::InitGoogleTest(&argc, argv); GETUSERARGS();