Add logic to import/un-import static routes upon instance add/delete

The code in RoutePathReplicator::[Join|Leave] triggers import and
cleanup of routes by scheduling walks of tables that have the import
target being added/deleted in their export target list.

This is not effective for static routes since static routes live in
tables that do not have the route targets assocaited with the static
routes.

The ideal fix would be to maintain a dependency list of static routes
for each route target.

Implement a simpler fix for now, which is to trigger notification of
all static routes when an import target is added/deleted to a table.
This should have acceptable performance since the number of static
routes is usually relatively small.

Maintain a list of non-empty StaticRouteMgrs in the BgpServer so that
we don't have to iterate through all RoutingInstances in order to walk
all static routes.

Change-Id: I559e014fd1fd001c5209dce8ab3e7564048889c7
Closes-bug: 1466755

src/bgp/bgp_server.cc

@@ -23,6 +23,7 @@
 #include "bgp/routing-instance/routepath_replicator.h"
 #include "bgp/routing-instance/rtarget_group_mgr.h"
 #include "bgp/routing-instance/service_chaining.h"
+#include "bgp/routing-instance/static_route.h"
 #include "io/event_manager.h"
 
 using boost::system::error_code;
@@ -271,6 +272,7 @@ BgpServer::BgpServer(EventManager *evm)
 }
 
 BgpServer::~BgpServer() {
+    assert(srt_manager_list_.empty());
 }
 
 string BgpServer::ToString() const {
@@ -460,3 +462,22 @@ void BgpServer::NotifyIdentifierUpdate(Ip4Address old_identifier) {
         }
     }
 }
+
+void BgpServer::InsertStaticRouteMgr(StaticRouteMgr *srt_manager) {
+    CHECK_CONCURRENCY("bgp::Config");
+    srt_manager_list_.insert(srt_manager);
+}
+
+void BgpServer::RemoveStaticRouteMgr(StaticRouteMgr *srt_manager) {
+    CHECK_CONCURRENCY("bgp::StaticRoute");
+    srt_manager_list_.erase(srt_manager);
+}
+
+void BgpServer::NotifyAllStaticRoutes() {
+    CHECK_CONCURRENCY("bgp::Config");
+    for (StaticRouteMgrList::iterator it = srt_manager_list_.begin();
+         it != srt_manager_list_.end(); ++it) {
+        StaticRouteMgr *srt_manager = *it;
+        srt_manager->NotifyAllRoutes();
+    }
+}

src/bgp/bgp_server.h

@@ -11,6 +11,7 @@
 #include <boost/scoped_ptr.hpp>
 
 #include <map>
+#include <set>
 #include <string>
 #include <vector>
 
@@ -36,12 +37,14 @@ class RoutingInstanceMgr;
 class RTargetGroupMgr;
 class SchedulingGroupManager;
 class ServiceChainMgr;
+class StaticRouteMgr;
 
 class BgpServer {
 public:
     typedef boost::function<void(as_t, as_t)> ASNUpdateCb;
     typedef boost::function<void(Ip4Address)> IdentifierUpdateCb;
     typedef boost::function<void(BgpPeer *)> VisitorFn;
+    typedef std::set<StaticRouteMgr *> StaticRouteMgrList;
     explicit BgpServer(EventManager *evm);
     virtual ~BgpServer();
 
@@ -137,6 +140,10 @@ class BgpServer {
     void UnregisterIdentifierUpdateCallback(int listener);
     void NotifyIdentifierUpdate(Ip4Address old_identifier);
 
+    void InsertStaticRouteMgr(StaticRouteMgr *srt_manager);
+    void RemoveStaticRouteMgr(StaticRouteMgr *srt_manager);
+    void NotifyAllStaticRoutes();
+
 private:
     class ConfigUpdater;
     class DeleteActor;
@@ -158,6 +165,7 @@ class BgpServer {
     IdentifierUpdateListenersList id_listeners_;
     boost::dynamic_bitset<> id_bmap_;      // free list.
     uint16_t hold_time_;
+    StaticRouteMgrList srt_manager_list_;
 
     DB db_;
     boost::dynamic_bitset<> peer_bmap_;

src/bgp/routing-instance/routepath_replicator.cc

@@ -200,6 +200,8 @@ void RoutePathReplicator::Join(BgpTable *table, const RouteTarget &rt,
 
     RPR_TRACE(TableJoin, table->name(), rt.ToString(), import);
     if (import) {
+        if (family_ == Address::INETVPN)
+            server_->NotifyAllStaticRoutes();
         BOOST_FOREACH(BgpTable *bgptable, group->GetExportTables(family())) {
             RequestWalk(bgptable);
         }
@@ -236,6 +238,8 @@ void RoutePathReplicator::Leave(BgpTable *table, const RouteTarget &rt,
 
     if (import) {
         group->RemoveImportTable(family(), table);
+        if (family_ == Address::INETVPN)
+            server_->NotifyAllStaticRoutes();
         BOOST_FOREACH(BgpTable *bgptable, group->GetExportTables(family())) {
             RequestWalk(bgptable);
         }

src/bgp/routing-instance/static_route.cc

@@ -142,10 +142,9 @@ class StaticRoute : public ConditionMatch {
     }
 
     void AddStaticRoute(NexthopPathIdList *list);
-
     void UpdateStaticRoute();
-
     void RemoveStaticRoute();
+    void NotifyRoute();
 
     virtual bool Match(BgpServer *server, BgpTable *table,
                        BgpRoute *route, bool deleted);
@@ -159,17 +158,6 @@ class StaticRoute : public ConditionMatch {
     }
 
 private:
-    RoutingInstance *routing_instance_;
-
-    Ip4Prefix static_route_prefix_;
-    IpAddress nexthop_;
-    BgpRoute *nexthop_route_;
-    NexthopPathIdList nexthop_path_ids_;
-
-    RouteTargetList rtarget_list_;
-
-    bool unregistered_;
-
     // Helper function to match
     bool is_nexthop_route(BgpRoute *route) {
         InetRoute *inet_route = dynamic_cast<InetRoute *>(route);
@@ -196,15 +184,25 @@ class StaticRoute : public ConditionMatch {
         return new_ext_community;
     }
 
+    RoutingInstance *routing_instance_;
+    Ip4Prefix static_route_prefix_;
+    IpAddress nexthop_;
+    BgpRoute *nexthop_route_;
+    NexthopPathIdList nexthop_path_ids_;
+    RouteTargetList rtarget_list_;
+    bool unregistered_;
+
     DISALLOW_COPY_AND_ASSIGN(StaticRoute);
 };
 
 
-StaticRoute::StaticRoute(RoutingInstance *rtinst,
-            const Ip4Prefix &static_route, const vector<string> &rtargets,
-            IpAddress nexthop)
-    : routing_instance_(rtinst), static_route_prefix_(static_route),
-    nexthop_(nexthop), nexthop_route_(NULL), unregistered_(false) {
+StaticRoute::StaticRoute(RoutingInstance *rtinst, const Ip4Prefix &static_route,
+    const vector<string> &rtargets, IpAddress nexthop)
+    : routing_instance_(rtinst),
+      static_route_prefix_(static_route),
+      nexthop_(nexthop),
+      nexthop_route_(NULL),
+      unregistered_(false) {
     for (vector<string>::const_iterator it = rtargets.begin();
         it != rtargets.end(); it++) {
         error_code ec;
@@ -462,6 +460,16 @@ StaticRoute::AddStaticRoute(NexthopPathIdList *old_path_ids) {
     }
 }
 
+void StaticRoute::NotifyRoute() {
+    InetRoute rt_key(static_route_prefix());
+    DBTablePartition *partition =
+       static_cast<DBTablePartition *>(bgp_table()->GetTablePartition(&rt_key));
+    BgpRoute *static_route = static_cast<BgpRoute *>(partition->Find(&rt_key));
+    if (!static_route)
+        return;
+    partition->Notify(static_route);
+}
+
 int StaticRouteMgr::static_route_task_id_ = -1;
 
 StaticRouteMgr::StaticRouteMgr(RoutingInstance *instance)
@@ -530,6 +538,8 @@ bool StaticRouteMgr::StaticRouteEventCallback(StaticRouteRequest *req) {
             if (!info->num_matchstate()) {
                 listener->UnregisterCondition(table, info);
                 static_route_map_.erase(info->static_route_prefix());
+                if (static_route_map_.empty())
+                    instance_->server()->RemoveStaticRouteMgr(this);
                 if (!routing_instance()->deleted() &&
                     routing_instance()->config() &&
                     routing_instance()->config()->instance_config())
@@ -551,6 +561,8 @@ bool StaticRouteMgr::StaticRouteEventCallback(StaticRouteRequest *req) {
             if (!info->num_matchstate() && info->unregistered()) {
                 listener->UnregisterCondition(table, info);
                 static_route_map_.erase(info->static_route_prefix());
+                if (static_route_map_.empty())
+                    instance_->server()->RemoveStaticRouteMgr(this);
                 if (!routing_instance()->deleted() &&
                     routing_instance()->config() &&
                     routing_instance()->config()->instance_config())
@@ -617,6 +629,8 @@ StaticRouteMgr::LocateStaticRoutePrefix(const autogen::StaticRouteType &cfg) {
         new StaticRoute(routing_instance(), prefix, cfg.route_target, nexthop);
     StaticRoutePtr static_route_match = StaticRoutePtr(match);
 
+    if (static_route_map_.empty())
+        instance_->server()->InsertStaticRouteMgr(this);
     static_route_map_.insert(make_pair(prefix, static_route_match));
 
     listener->AddMatchCondition(match->bgp_table(), static_route_match.get(),
@@ -728,6 +742,16 @@ void StaticRouteMgr::FlushStaticRouteConfig() {
     }
 }
 
+void StaticRouteMgr::NotifyAllRoutes() {
+    CHECK_CONCURRENCY("bgp::Config");
+    for (StaticRouteMap::iterator it = static_route_map_.begin();
+         it != static_route_map_.end(); ++it) {
+        StaticRoute *static_route =
+             static_cast<StaticRoute *>(it->second.get());
+        static_route->NotifyRoute();
+    }
+}
+
 class ShowStaticRouteHandler {
 public:
     static void FillStaticRoutesInfo(vector<StaticRouteEntriesInfo> *list,

src/bgp/routing-instance/static_route.h

@@ -61,6 +61,7 @@ class StaticRouteMgr {
     bool StaticRouteEventCallback(StaticRouteRequest *req);
 
     bool ResolvePendingStaticRouteConfig();
+    void NotifyAllRoutes();
 
     RoutingInstance *routing_instance() { return instance_; }
 

src/bgp/test/static_route_test.cc

@@ -343,6 +343,11 @@ class StaticRouteTest : public ::testing::Test {
         TASK_UTIL_EXPECT_EQ(1, validate_done_);
     }
 
+    void VerifyTableNoExists(const string &table_name) {
+        TASK_UTIL_EXPECT_TRUE(
+            bgp_server_->database()->FindTable(table_name) == NULL);
+    }
+
     EventManager evm_;
     DB config_db_;
     DBGraph config_graph_;
@@ -1504,6 +1509,99 @@ TEST_F(StaticRouteTest, EntryAfterStop) {
                              "Wait for Static route in blue..");
 }
 
+//
+// Delete the routing instance that imports the static route and make sure
+// the inet table gets deleted. Objective is to check that the static route
+// is removed from the table even though the static route config has not
+// changed.
+//
+TEST_F(StaticRouteTest, DeleteRoutingInstance) {
+    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();
+
+    // Add Nexthop Route
+    AddInetRoute(NULL, "nat", "192.168.1.254/32", 100, "2.3.4.5");
+    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..");
+
+    // Delete the configuration for the blue instance.
+    ifmap_test_util::IFMapMsgUnlink(&config_db_, "routing-instance", "blue",
+        "virtual-network", "blue", "virtual-network-routing-instance");
+    ifmap_test_util::IFMapMsgUnlink(&config_db_, "routing-instance", "blue",
+        "route-target", "target:64496:1", "instance-target");
+    ifmap_test_util::IFMapMsgNodeDelete(
+        &config_db_, "virtual-network", "blue");
+    ifmap_test_util::IFMapMsgNodeDelete(
+        &config_db_, "routing-instance", "blue");
+    ifmap_test_util::IFMapMsgNodeDelete(
+        &config_db_, "route-target", "target:64496:1");
+    task_util::WaitForIdle();
+
+    // Make sure that the blue inet table is gone.
+    VerifyTableNoExists("blue.inet.0");
+
+    // Delete nexthop route
+    DeleteInetRoute(NULL, "nat", "192.168.1.254/32");
+    task_util::WaitForIdle();
+}
+
+//
+// Add the routing instance that imports the static route after the static
+// route has already been added. Objective is to check that the static route
+// is replicated to the table in the new instance without any triggers to
+// the static route module.
+//
+TEST_F(StaticRouteTest, AddRoutingInstance) {
+    vector<string> instance_names = list_of("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();
+
+    // Add Nexthop Route
+    AddInetRoute(NULL, "nat", "192.168.1.254/32", 100, "2.3.4.5");
+    task_util::WaitForIdle();
+
+    // Check for Static route
+    TASK_UTIL_WAIT_NE_NO_MSG(InetRouteLookup("nat", "192.168.1.0/24"),
+                             NULL, 1000, 10000,
+                             "Wait for Static route in nat..");
+
+    // Add the blue instance.
+    // Make sure that the id and route target for nat instance don't change.
+    instance_names = list_of("nat")("blue");
+    NetworkConfig(instance_names, connections);
+    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..");
+
+    // Delete nexthop route
+    DeleteInetRoute(NULL, "nat", "192.168.1.254/32");
+    task_util::WaitForIdle();
+}
+
 // Sandesh introspect test
 // Verify http introspect output
 //   1. After creating the config and before nexthop route is published