From 1a49af4bef9bc30f13ca4e7d7f0f909ed7b6f498 Mon Sep 17 00:00:00 2001 From: Nischal Sheth Date: Thu, 18 Jun 2015 23:28:20 -0700 Subject: [PATCH] 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 | 21 ++++ src/bgp/bgp_server.h | 7 ++ .../routing-instance/routepath_replicator.cc | 4 + src/bgp/routing-instance/static_route.cc | 33 ++++++- src/bgp/routing-instance/static_route.h | 1 + src/bgp/test/static_route_test.cc | 98 +++++++++++++++++++ 6 files changed, 160 insertions(+), 4 deletions(-) diff --git a/src/bgp/bgp_server.cc b/src/bgp/bgp_server.cc index a55040000cc..1337f5b416a 100644 --- a/src/bgp/bgp_server.cc +++ b/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 namespace std; @@ -246,6 +247,7 @@ BgpServer::BgpServer(EventManager *evm) } BgpServer::~BgpServer() { + assert(srt_manager_list_.empty()); } string BgpServer::ToString() const { @@ -438,3 +440,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(); + } +} diff --git a/src/bgp/bgp_server.h b/src/bgp/bgp_server.h index 4870b620978..2582d21d7e4 100644 --- a/src/bgp/bgp_server.h +++ b/src/bgp/bgp_server.h @@ -33,12 +33,14 @@ class RoutingInstanceMgr; class RTargetGroupMgr; class SchedulingGroupManager; class ServiceChainMgr; +class StaticRouteMgr; class BgpServer { public: typedef boost::function ASNUpdateCb; typedef boost::function IdentifierUpdateCb; typedef boost::function VisitorFn; + typedef std::set StaticRouteMgrList; explicit BgpServer(EventManager *evm); virtual ~BgpServer(); @@ -133,6 +135,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; @@ -153,6 +159,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_; diff --git a/src/bgp/routing-instance/routepath_replicator.cc b/src/bgp/routing-instance/routepath_replicator.cc index a6de15a5cdf..a46651ea6d4 100644 --- a/src/bgp/routing-instance/routepath_replicator.cc +++ b/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); } diff --git a/src/bgp/routing-instance/static_route.cc b/src/bgp/routing-instance/static_route.cc index 468259dcbc4..3c4d6a79210 100644 --- a/src/bgp/routing-instance/static_route.cc +++ b/src/bgp/routing-instance/static_route.cc @@ -133,10 +133,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); @@ -452,6 +451,16 @@ StaticRoute::AddStaticRoute(NexthopPathIdList *old_path_ids) { } } +void StaticRoute::NotifyRoute() { + InetRoute rt_key(static_route_prefix()); + DBTablePartition *partition = + static_cast(bgp_table()->GetTablePartition(&rt_key)); + BgpRoute *static_route = static_cast(partition->Find(&rt_key)); + if (!static_route) + return; + partition->Notify(static_route); +} + int StaticRouteMgr::static_route_task_id_ = -1; StaticRouteMgr::StaticRouteMgr(RoutingInstance *instance) @@ -520,7 +529,9 @@ bool StaticRouteMgr::StaticRouteEventCallback(StaticRouteRequest *req) { if (!info->num_matchstate()) { listener->UnregisterCondition(table, info); static_route_map_.erase(info->static_route_prefix()); - if (!routing_instance()->deleted() && + if (static_route_map_.empty()) + instance_->server()->RemoveStaticRouteMgr(this); + if (!routing_instance()->deleted() && routing_instance()->config() && routing_instance()->config()->instance_config()) resolve_trigger_->Set(); @@ -541,7 +552,9 @@ bool StaticRouteMgr::StaticRouteEventCallback(StaticRouteRequest *req) { if (!info->num_matchstate() && info->unregistered()) { listener->UnregisterCondition(table, info); static_route_map_.erase(info->static_route_prefix()); - if (!routing_instance()->deleted() && + if (static_route_map_.empty()) + instance_->server()->RemoveStaticRouteMgr(this); + if (!routing_instance()->deleted() && routing_instance()->config() && routing_instance()->config()->instance_config()) resolve_trigger_->Set(); @@ -608,6 +621,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(std::make_pair(prefix, static_route_match)); listener->AddMatchCondition(match->bgp_table(), static_route_match.get(), @@ -719,6 +734,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(it->second.get()); + static_route->NotifyRoute(); + } +} + class ShowStaticRouteHandler { public: static void FillStaticRoutesInfo(std::vector &list, diff --git a/src/bgp/routing-instance/static_route.h b/src/bgp/routing-instance/static_route.h index 07621576883..579c65bd9d0 100644 --- a/src/bgp/routing-instance/static_route.h +++ b/src/bgp/routing-instance/static_route.h @@ -57,6 +57,7 @@ class StaticRouteMgr { bool StaticRouteEventCallback(StaticRouteRequest *req); bool ResolvePendingStaticRouteConfig(); + void NotifyAllRoutes(); RoutingInstance *routing_instance() { return instance_; } diff --git a/src/bgp/test/static_route_test.cc b/src/bgp/test/static_route_test.cc index 684654709a8..16ae7ba70e6 100644 --- a/src/bgp/test/static_route_test.cc +++ b/src/bgp/test/static_route_test.cc @@ -340,6 +340,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_; @@ -1501,6 +1506,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 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(); + + // 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 instance_names = list_of("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(); + + // 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