From a0d84422df11f7ba7737c9fa1f5835b4b431ade9 Mon Sep 17 00:00:00 2001 From: Prakash M Bailkeri Date: Fri, 7 Aug 2015 16:18:14 +0530 Subject: [PATCH] Routepath Replicator should unregister from BgpTable only after all replicated routes are deleted When the routing instance is deleted, route path replicator walks the route table as part of Leave of rtgroup in import and export Route targets. During the walk, it deletes all the replicated path/routes. On walk complete, it unregisters from the DBTable. In the static route scenario, the static route is added on the internal routing instance that has "static-route-entries" property attached to it. Such generated static route is replicated to destination VRF based on "route-target-list" config in "static-route-entries. Note: such internal routing instance doesn't have these route targets in its export_rt. If this internal routing instance is deleted, route path replicator starts the table walk as part of Leave of RtGroup in import and export. In case static route module has not processed the config delete of static route entries, it would not have deleted the static route added to inet route table. On walk complete, it would unregister from the routing table. Hence the replicated routes of Static route will never be deleted as RouteReplicator module is no longer TableListener and doesn't process delete of StaticRoute which happens when static route module process the delete request. Proposed fix: Routereplicator to keep track of replicated routes/dbstate added, and unregister from DBTable only after all DBStates are clear (or after all replicated routes from this table is deleted). Added unit test code to Static route test and route replication test to simulate the error condition and validate the fix Change-Id: I5ddb05425401a36bf117a7971a3ab7758494d39b Closes-bug: #1482277 --- .../routing-instance/routepath_replicator.cc | 36 ++++- .../routing-instance/routepath_replicator.h | 19 ++- src/bgp/test/routepath_replicator_test.cc | 134 +++++++++++++++++- src/bgp/test/static_route_test.cc | 75 ++++++++++ 4 files changed, 256 insertions(+), 8 deletions(-) diff --git a/src/bgp/routing-instance/routepath_replicator.cc b/src/bgp/routing-instance/routepath_replicator.cc index a46651ea6d4..c41217c1741 100644 --- a/src/bgp/routing-instance/routepath_replicator.cc +++ b/src/bgp/routing-instance/routepath_replicator.cc @@ -52,6 +52,7 @@ do { \ TableState::TableState(BgpTable *table, DBTableBase::ListenerId id) : id_(id), table_delete_ref_(this, table->deleter()) { assert(table->deleter() != NULL); + route_count_ = 0; } TableState::~TableState() { @@ -163,9 +164,10 @@ RoutePathReplicator::BulkReplicationDone(DBTableBase *table) { RouteReplicatorTableState::iterator ts_it = table_state_.find(bgptable); assert(ts_it != table_state_.end()); TableState *ts = ts_it->second; - if (ts->GetGroupList().empty()) { - unreg_table_list_.insert(bgptable); + if (!ts->GetGroupList().empty() || ts->route_count()) { + return; } + unreg_table_list_.insert(bgptable); unreg_trigger_->Set(); } @@ -289,7 +291,7 @@ void RoutePathReplicator::Leave(BgpTable *table, const RouteTarget &rt, } void -RoutePathReplicator::DBStateSync(BgpTable *table, BgpRoute *rt, +RoutePathReplicator::DBStateSync(BgpTable *table, TableState *ts, BgpRoute *rt, DBTableBase::ListenerId id, RtReplicated *dbstate, RtReplicated::ReplicatedRtPathList ¤t) { @@ -331,11 +333,34 @@ RoutePathReplicator::DBStateSync(BgpTable *table, BgpRoute *rt, dbstate->GetMutableList()->erase(dbstate_it); } if (dbstate->GetList().empty()) { + uint32_t prev = ts->decrement_route_count(); rt->ClearState(table, id); delete dbstate; + if (prev == 1) { + RetryUnregisterTableState(ts, table); + } } } +// +// Attempt to unregister the table listener after deleting all DBState or +// replicated routes. +// Check whether +// 1. all RouteTargets are deleted from the list and +// 2. No pending TableWalk requested by RoutePathReplicator on this table +// +void RoutePathReplicator::RetryUnregisterTableState(TableState *ts, + BgpTable *table) { + CHECK_CONCURRENCY("db::DBTable"); + // Mutex to protect unreg_table_list_ and bulk_sync_ + tbb::mutex::scoped_lock lock(mutex_); + if (!ts->GetGroupList().empty() || + (bulk_sync_.find(table) != bulk_sync_.end())) + return; + unreg_table_list_.insert(table); + unreg_trigger_->Set(); +} + // // Update the ExtCommunity with the RouteTargets from the export list // and the OriginVn. The OriginVn is derived from the RouteTargets in @@ -415,13 +440,14 @@ bool RoutePathReplicator::BgpTableListener(DBTablePartBase *root, if (!dbstate) { return true; } - DBStateSync(table, rt, id, dbstate, replicated_path_list); + DBStateSync(table, ts, rt, id, dbstate, replicated_path_list); return true; } if (dbstate == NULL) { dbstate = new RtReplicated(); rt->SetState(table, id, dbstate); + ts->increment_route_count(); } // Get the export route target list from the routing instance @@ -528,7 +554,7 @@ bool RoutePathReplicator::BgpTableListener(DBTablePartBase *root, } } - DBStateSync(table, rt, id, dbstate, replicated_path_list); + DBStateSync(table, ts, rt, id, dbstate, replicated_path_list); return true; } diff --git a/src/bgp/routing-instance/routepath_replicator.h b/src/bgp/routing-instance/routepath_replicator.h index df7b312ebf7..812a36e24b0 100644 --- a/src/bgp/routing-instance/routepath_replicator.h +++ b/src/bgp/routing-instance/routepath_replicator.h @@ -8,6 +8,7 @@ #include #include +#include #include #include "bgp/bgp_table.h" @@ -41,10 +42,23 @@ class TableState { return id_; } + uint32_t route_count() const { return route_count_; } + + uint32_t increment_route_count() const { + return route_count_.fetch_and_increment(); + } + + uint32_t decrement_route_count() const { + uint32_t prev = route_count_.fetch_and_decrement(); + assert(prev); + return prev; + } + private: DBTableBase::ListenerId id_; LifetimeRef table_delete_ref_; GroupList list_; + mutable tbb::atomic route_count_; DISALLOW_COPY_AND_ASSIGN(TableState); }; @@ -176,6 +190,7 @@ class RoutePathReplicator { SandeshTraceBufferPtr trace_buffer() const { return trace_buf_; } bool UnregisterTables(); + void RetryUnregisterTableState(TableState *ts, BgpTable *table); private: typedef std::map RouteReplicatorTableState; @@ -188,8 +203,8 @@ class RoutePathReplicator { void DeleteSecondaryPath(BgpTable *table, BgpRoute *rt, const RtReplicated::SecondaryRouteInfo &rtinfo); - void DBStateSync(BgpTable *table, BgpRoute *rt, DBTableBase::ListenerId id, - RtReplicated *dbstate, + void DBStateSync(BgpTable *table, TableState *ts, BgpRoute *rt, + DBTableBase::ListenerId id, RtReplicated *dbstate, RtReplicated::ReplicatedRtPathList ¤t); // Mutex to protect unreg_table_list_, table_state_ and bulk_sync_ diff --git a/src/bgp/test/routepath_replicator_test.cc b/src/bgp/test/routepath_replicator_test.cc index b6d4a710ece..5f41a8a8e14 100644 --- a/src/bgp/test/routepath_replicator_test.cc +++ b/src/bgp/test/routepath_replicator_test.cc @@ -134,8 +134,28 @@ class ReplicationTest : public ::testing::Test { parser->Receive(&config_db_, netconf.data(), netconf.length(), 0); } + void DeleteRoutingInstance(const string &instance_name, const string &rt_name) { + ifmap_test_util::IFMapMsgUnlink(&config_db_, "routing-instance", instance_name, + "virtual-network", instance_name, "virtual-network-routing-instance"); + ifmap_test_util::IFMapMsgUnlink(&config_db_, "routing-instance", instance_name, + "route-target", rt_name, "instance-target"); + ifmap_test_util::IFMapMsgNodeDelete( + &config_db_, "virtual-network", instance_name); + ifmap_test_util::IFMapMsgNodeDelete( + &config_db_, "routing-instance", instance_name); + ifmap_test_util::IFMapMsgNodeDelete( + &config_db_, "route-target", rt_name); + task_util::WaitForIdle(); + } + + void VerifyTableNoExists(const string &table_name) { + TASK_UTIL_EXPECT_TRUE( + bgp_server_->database()->FindTable(table_name) == NULL); + } + void AddInetRoute(IPeer *peer, const string &instance_name, - const string &prefix, int localpref, string rd = "") { + const string &prefix, int localpref, string rd = "", + const vector &rtarget_list = vector()) { boost::system::error_code error; Ip4Prefix nlri = Ip4Prefix::FromString(prefix, &error); EXPECT_FALSE(error); @@ -150,6 +170,15 @@ class ReplicationTest : public ::testing::Test { if (!rd.empty()) { attr_spec.push_back(&rd_spec); } + ExtCommunitySpec spec; + if (!rtarget_list.empty()) { + BOOST_FOREACH(string tgt, rtarget_list) { + RouteTarget rt(RouteTarget::FromString(tgt)); + spec.communities.push_back(get_value(rt.GetExtCommunity().begin(), 8)); + } + attr_spec.push_back(&spec); + } + BgpAttrPtr attr = bgp_server_->attr_db()->Locate(attr_spec); request.data.reset(new BgpTable::RequestData(attr, 0, 0)); BgpTable *table = static_cast( @@ -1900,6 +1929,109 @@ TEST_F(ReplicationTest, OriginVn4) { VERIFY_EQ(0, RouteCount("red")); } +// Test a case where routing instance is deleted with replicated route +// In this case the route is replicated due to rtarget of the route not due +// to export_rt of the VRF. Simulate the static route scenario +TEST_F(ReplicationTest, DeleteInstanceWithReplicatedRoute) { + vector instance_names = list_of("blue")("red"); + multimap connections; + NetworkConfig(instance_names, connections); + task_util::WaitForIdle(); + + boost::system::error_code ec; + peers_.push_back( + new BgpPeerMock(Ip4Address::from_string("192.168.0.1", ec))); + + // Add route to blue table with the Rtarget that red imports. + AddInetRoute(peers_[0], "blue", "10.0.1.1/32", 100, "192.168.0.1:1", + list_of("target:64496:2")); + task_util::WaitForIdle(); + + DeleteRoutingInstance("blue", "target:64496:1"); + task_util::WaitForIdle(); + + DeleteInetRoute(peers_[0], "blue", "10.0.1.1/32"); + task_util::WaitForIdle(); + + DeleteRoutingInstance("red", "target:64496:2"); + task_util::WaitForIdle(); + + // Make sure that the blue inet table is gone. + VerifyTableNoExists("blue.inet.0"); + VerifyTableNoExists("red.inet.0"); +} + +// Test a case where routing instance is deleted with replicated route +// In this case the route is replicated due to rtarget of the route not due +// to export_rt of the VRF. Delete the routing instance that importing the route +TEST_F(ReplicationTest, DeleteInstanceWithReplicatedRoute_1) { + vector instance_names = list_of("blue")("red"); + multimap connections; + NetworkConfig(instance_names, connections); + task_util::WaitForIdle(); + + boost::system::error_code ec; + peers_.push_back( + new BgpPeerMock(Ip4Address::from_string("192.168.0.1", ec))); + + // Add route to blue table with the Rtarget that red imports. + AddInetRoute(peers_[0], "blue", "10.0.1.1/32", 100, "192.168.0.1:1", + list_of("target:64496:2")); + task_util::WaitForIdle(); + + DeleteRoutingInstance("red", "target:64496:2"); + task_util::WaitForIdle(); + + DeleteInetRoute(peers_[0], "blue", "10.0.1.1/32"); + task_util::WaitForIdle(); + + DeleteRoutingInstance("blue", "target:64496:1"); + task_util::WaitForIdle(); + + // Make sure that the blue inet table is gone. + VerifyTableNoExists("blue.inet.0"); + VerifyTableNoExists("red.inet.0"); +} + +// Test a case where routing instance is deleted with replicated routes +// Same as DeleteInstanceWithReplicatedRoute with multiple routes in +// different partition +TEST_F(ReplicationTest, DeleteInstanceWithReplicatedRoute_2) { + vector instance_names = list_of("blue")("red"); + multimap connections; + NetworkConfig(instance_names, connections); + task_util::WaitForIdle(); + + boost::system::error_code ec; + peers_.push_back( + new BgpPeerMock(Ip4Address::from_string("192.168.0.1", ec))); + + // Add multiple routes to blue table with the Rtarget that red imports. + AddInetRoute(peers_[0], "blue", "10.0.1.1/32", 100, "192.168.0.1:1", + list_of("target:64496:2")); + AddInetRoute(peers_[0], "blue", "10.0.1.2/32", 100, "192.168.0.1:1", + list_of("target:64496:2")); + AddInetRoute(peers_[0], "blue", "10.0.1.3/32", 100, "192.168.0.1:1", + list_of("target:64496:2")); + task_util::WaitForIdle(); + + DeleteRoutingInstance("blue", "target:64496:1"); + task_util::WaitForIdle(); + + DeleteInetRoute(peers_[0], "blue", "10.0.1.1/32"); + DeleteInetRoute(peers_[0], "blue", "10.0.1.2/32"); + DeleteInetRoute(peers_[0], "blue", "10.0.1.3/32"); + task_util::WaitForIdle(); + + DeleteRoutingInstance("red", "target:64496:2"); + task_util::WaitForIdle(); + + // Make sure that the blue inet table is gone. + VerifyTableNoExists("blue.inet.0"); + VerifyTableNoExists("red.inet.0"); +} + + class TestEnvironment : public ::testing::Environment { virtual ~TestEnvironment() { } }; diff --git a/src/bgp/test/static_route_test.cc b/src/bgp/test/static_route_test.cc index 16ae7ba70e6..8e81ba3e8f1 100644 --- a/src/bgp/test/static_route_test.cc +++ b/src/bgp/test/static_route_test.cc @@ -152,6 +152,7 @@ class StaticRouteTest : public ::testing::Test { bool IsQueueEmpty(const string &instance_name) { RoutingInstance *rtinstance = ri_mgr_->GetRoutingInstance(instance_name); + if (rtinstance == NULL) return true; return rtinstance->static_route_mgr()->IsQueueEmpty(); } @@ -222,6 +223,18 @@ class StaticRouteTest : public ::testing::Test { table->Enqueue(&request); } + void DeleteRoutingInstance(const string &instance_name, const string &rt) { + ifmap_test_util::IFMapMsgUnlink(&config_db_, "routing-instance", instance_name, + "virtual-network", instance_name, "virtual-network-routing-instance"); + ifmap_test_util::IFMapMsgUnlink(&config_db_, "routing-instance", instance_name, + "route-target", rt, "instance-target"); + ifmap_test_util::IFMapMsgNodeDelete( + &config_db_, "virtual-network",instance_name); + ifmap_test_util::IFMapMsgNodeDelete( + &config_db_, "routing-instance", instance_name); + ifmap_test_util::IFMapMsgNodeDelete( + &config_db_, "route-target", rt); + } BgpRoute *InetRouteLookup(const string &instance_name, const string &prefix) { @@ -1673,6 +1686,68 @@ TEST_F(StaticRouteTest, SandeshTest) { task_util::WaitForIdle(); } +// +// Delete the internal routing instance before deleting the routing instances +// which import the static route. +// To simulate the delay in static route processing disable the static route +// Queue and enable it. +// Expected: All the routing instances and bgp tables deleted cleanly +// +TEST_F(StaticRouteTest, DeleteNatInstance) { + vector instance_names = list_of("blue")("nat"); + multimap connections; + NetworkConfig(instance_names, connections); + task_util::WaitForIdle(); + + std::auto_ptr params = + GetStaticRouteConfig("controller/src/bgp/testdata/static_route_1.xml"); + + ifmap_test_util::IFMapMsgPropertyAdd(&config_db_, "routing-instance", + "nat", "static-route-entries", params.release(), 0); + task_util::WaitForIdle(); + + // Check for Static route + TASK_UTIL_WAIT_EQ_NO_MSG(InetRouteLookup("blue", "192.168.1.0/24"), + NULL, 1000, 10000, + "Wait for Static route in blue.."); + + set encap = list_of("gre")("vxlan"); + // Add Nexthop Route + AddInetRoute(NULL, "nat", "192.168.1.254/32", 100, "2.3.4.5", encap); + task_util::WaitForIdle(); + + // Check for Static route + TASK_UTIL_WAIT_NE_NO_MSG(InetRouteLookup("blue", "192.168.1.0/24"), + NULL, 1000, 10000, + "Wait for Static route in blue.."); + TASK_UTIL_WAIT_NE_NO_MSG(InetRouteLookup("nat", "192.168.1.0/24"), + NULL, 1000, 10000, + "Wait for Static route in nat.."); + + // Stop the static route processing + DisableStaticRouteQ("nat"); + // Delete nexthop route + DeleteInetRoute(NULL, "nat", "192.168.1.254/32"); + + // Delete the routing instance where static route originates + DeleteRoutingInstance("nat", "target:64496:2"); + ifmap_test_util::IFMapMsgPropertyDelete(&config_db_, "routing-instance", + "nat", "static-route-entries"); + task_util::WaitForIdle(); + + // Delete the routing instance that imports the static route + DeleteRoutingInstance("blue", "target:64496:1"); + task_util::WaitForIdle(); + + // Re-enable the static route module. From this point on the static route + // get deleted as StaticRouteManager starts processing nexthop route delete + EnableStaticRouteQ("nat"); + TASK_UTIL_EXPECT_TRUE(IsQueueEmpty("nat")); + + VerifyTableNoExists("blue.inet.0"); + VerifyTableNoExists("nat.inet.0"); +} + class TestEnvironment : public ::testing::Environment { virtual ~TestEnvironment() { } };