From 78b330848ca26bc84a5c3dbe86cc34698d1d5b39 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 | 8 ++ .../routing-instance/routepath_replicator.cc | 4 + src/bgp/routing-instance/static_route.cc | 60 ++++++++---- src/bgp/routing-instance/static_route.h | 1 + src/bgp/test/static_route_test.cc | 98 +++++++++++++++++++ 6 files changed, 174 insertions(+), 18 deletions(-) diff --git a/src/bgp/bgp_server.cc b/src/bgp/bgp_server.cc index 90521171507..3a2df7f3489 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 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(); + } +} diff --git a/src/bgp/bgp_server.h b/src/bgp/bgp_server.h index 6139f23f51c..1d1f00416c2 100644 --- a/src/bgp/bgp_server.h +++ b/src/bgp/bgp_server.h @@ -11,6 +11,7 @@ #include #include +#include #include #include @@ -36,12 +37,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(); @@ -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_; diff --git a/src/bgp/routing-instance/routepath_replicator.cc b/src/bgp/routing-instance/routepath_replicator.cc index 72d6540f556..2c4c1bd13a9 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 0b2d1416a7f..33aa174a611 100644 --- a/src/bgp/routing-instance/static_route.cc +++ b/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(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 &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 &rtargets, IpAddress nexthop) + : routing_instance_(rtinst), + static_route_prefix_(static_route), + nexthop_(nexthop), + nexthop_route_(NULL), + unregistered_(false) { for (vector::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(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) @@ -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(it->second.get()); + static_route->NotifyRoute(); + } +} + class ShowStaticRouteHandler { public: static void FillStaticRoutesInfo(vector *list, diff --git a/src/bgp/routing-instance/static_route.h b/src/bgp/routing-instance/static_route.h index f41cfea733a..5393368fd65 100644 --- a/src/bgp/routing-instance/static_route.h +++ b/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_; } diff --git a/src/bgp/test/static_route_test.cc b/src/bgp/test/static_route_test.cc index 2fed7952e4e..56c765b2ec8 100644 --- a/src/bgp/test/static_route_test.cc +++ b/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 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