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