Routepath Replicator should unregister from BgpTable only after all r…

…eplicated 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

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 &current) {
@@ -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;
 }
 

src/bgp/routing-instance/routepath_replicator.h

@@ -8,6 +8,7 @@
 #include <list>
 
 #include <boost/ptr_container/ptr_map.hpp>
+#include <tbb/atomic.h>
 #include <tbb/mutex.h>
 
 #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<TableState> table_delete_ref_;
     GroupList list_;
+    mutable tbb::atomic<uint32_t> 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<BgpTable *, TableState *> 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 &current);
 
     // Mutex to protect unreg_table_list_, table_state_ and bulk_sync_ 

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<string> &rtarget_list = vector<string>()) {
         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<BgpTable *>(
@@ -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<string> instance_names = list_of("blue")("red");
+    multimap<string, string> 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<string> instance_names = list_of("blue")("red");
+    multimap<string, string> 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<string> instance_names = list_of("blue")("red");
+    multimap<string, string> 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() { }
 };

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<string> instance_names = list_of("blue")("nat");
+    multimap<string, string> connections;
+    NetworkConfig(instance_names, connections);
+    task_util::WaitForIdle();
+
+    std::auto_ptr<autogen::StaticRouteEntriesType> 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<string> 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() { }
 };