From 7be430d7743b4a5c83653d9ba5ea227ff89b90c4 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 9b3281228f0..d4554a83b45 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; @@ -269,6 +270,7 @@ BgpServer::BgpServer(EventManager *evm) } BgpServer::~BgpServer() { + assert(srt_manager_list_.empty()); } string BgpServer::ToString() const { @@ -458,3 +460,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 687f8d0c73d..ed1733917f5 100644 --- a/src/bgp/bgp_server.h +++ b/src/bgp/bgp_server.h @@ -11,6 +11,7 @@ #include #include +#include #include #include @@ -41,12 +42,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(); @@ -156,6 +159,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; @@ -177,6 +184,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 d4251453fa8..e7793ed6a71 100644 --- a/src/bgp/routing-instance/routepath_replicator.cc +++ b/src/bgp/routing-instance/routepath_replicator.cc @@ -340,6 +340,8 @@ void RoutePathReplicator::Join(BgpTable *table, const RouteTarget &rt, if (import) { first = group->AddImportTable(family(), table); server()->rtarget_group_mgr()->NotifyRtGroup(rt); + if (family_ == Address::INETVPN) + server_->NotifyAllStaticRoutes(); BOOST_FOREACH(BgpTable *sec_table, group->GetExportTables(family())) { if (sec_table->IsVpnTable() || sec_table->empty()) continue; @@ -376,6 +378,8 @@ void RoutePathReplicator::Leave(BgpTable *table, const RouteTarget &rt, if (import) { group->RemoveImportTable(family(), table); server()->rtarget_group_mgr()->NotifyRtGroup(rt); + if (family_ == Address::INETVPN) + server_->NotifyAllStaticRoutes(); BOOST_FOREACH(BgpTable *sec_table, group->GetExportTables(family())) { if (sec_table->IsVpnTable() || sec_table->empty()) continue; diff --git a/src/bgp/routing-instance/static_route.cc b/src/bgp/routing-instance/static_route.cc index e99a8070672..4b3196aa1f4 100644 --- a/src/bgp/routing-instance/static_route.cc +++ b/src/bgp/routing-instance/static_route.cc @@ -141,10 +141,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); @@ -162,17 +161,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); @@ -199,15 +187,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; @@ -466,6 +464,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) @@ -533,6 +541,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()) { resolve_trigger_->Set(); @@ -554,6 +564,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()) { resolve_trigger_->Set(); @@ -611,6 +623,8 @@ StaticRouteMgr::LocateStaticRoutePrefix(const StaticRouteConfig &cfg) { cfg.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(), @@ -717,6 +731,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 1436055d957..c4983740d39 100644 --- a/src/bgp/routing-instance/static_route.h +++ b/src/bgp/routing-instance/static_route.h @@ -59,6 +59,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 d709ec290bb..7881ccac46b 100644 --- a/src/bgp/test/static_route_test.cc +++ b/src/bgp/test/static_route_test.cc @@ -347,6 +347,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_; @@ -1508,6 +1513,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