From b02403738ddc78c880eb53ed9b85b2e2214442b1 Mon Sep 17 00:00:00 2001 From: Prakash M Bailkeri Date: Thu, 21 Jan 2016 12:11:59 +0530 Subject: [PATCH] Route aggregation code review comments Handle review comments from Nischal and fix issues reported by cpplint Change-Id: I5cebd064f0832ecb870ff872e40065b347e49203 Related-bug: #1500698 --- src/bgp/bgp_condition_listener.cc | 3 +- src/bgp/bgp_path.h | 5 +- src/bgp/bgp_route.cc | 4 + src/bgp/bgp_table.cc | 20 ++- src/bgp/bgp_table.h | 6 + src/bgp/routing-instance/route_aggregate.cc | 155 ++++++++++-------- src/bgp/routing-instance/route_aggregate.h | 50 ++++-- .../routing-instance/routepath_replicator.cc | 2 +- src/bgp/test/route_aggregation_test.cc | 2 + 9 files changed, 148 insertions(+), 99 deletions(-) diff --git a/src/bgp/bgp_condition_listener.cc b/src/bgp/bgp_condition_listener.cc index 3eaf62551b4..58bce256bdd 100644 --- a/src/bgp/bgp_condition_listener.cc +++ b/src/bgp/bgp_condition_listener.cc @@ -350,7 +350,8 @@ bool BgpConditionListener::BgpRouteNotify(BgpServer *server, DBEntryBase *entry) { BgpTable *bgptable = static_cast(root->parent()); BgpRoute *rt = static_cast (entry); - bool del_rt = rt->IsDeleted(); + // Either the route is deleted or no valid path exists + bool del_rt = !rt->IsUsable(); TableMap::iterator loc = map_.find(bgptable); assert(loc != map_.end()); diff --git a/src/bgp/bgp_path.h b/src/bgp/bgp_path.h index 2f187ccf3df..b775fdba304 100644 --- a/src/bgp/bgp_path.h +++ b/src/bgp/bgp_path.h @@ -34,7 +34,7 @@ class BgpPath : public Path { ResolvedRoute = 2, ServiceChain = 3, StaticRoute = 4, - Aggregation = 5, + Aggregate = 5, Local = 6, }; @@ -61,7 +61,8 @@ class BgpPath : public Path { bool IsVrfOriginated() const { if (IsReplicated()) return false; - if (source_ != ResolvedRoute && source_ != BGP_XMPP && source_ != Local) + if (source_ != ResolvedRoute && source_ != Aggregate && + source_ != BGP_XMPP && source_ != Local) return false; return true; } diff --git a/src/bgp/bgp_route.cc b/src/bgp/bgp_route.cc index b29e66b0885..5aafcf9c94f 100644 --- a/src/bgp/bgp_route.cc +++ b/src/bgp/bgp_route.cc @@ -299,6 +299,10 @@ void BgpRoute::FillRouteInfo(const BgpTable *table, srp.set_protocol("ServiceChain"); } else if (path->GetSource() == BgpPath::StaticRoute) { srp.set_protocol("StaticRoute"); + } else if (path->GetSource() == BgpPath::Aggregate) { + srp.set_protocol("Aggregate"); + } else if (path->GetSource() == BgpPath::ResolvedRoute) { + srp.set_protocol("ResolvedRoute"); } const BgpAttr *attr = path->GetAttr(); diff --git a/src/bgp/bgp_table.cc b/src/bgp/bgp_table.cc index 6d6f771bb40..6385f245f76 100644 --- a/src/bgp/bgp_table.cc +++ b/src/bgp/bgp_table.cc @@ -134,12 +134,10 @@ UpdateInfo *BgpTable::GetUpdateInfo(RibOut *ribout, BgpRoute *route, if (!path->IsFeasible()) return NULL; - if (IsRouteAggregationSupported()) { - // Check whether the route is contributing route - if (routing_instance()->IsContributingRoute(this, route)) { - return NULL; - } - } + // Check whether the route is contributing route + if (IsRouteAggregationSupported() && IsContributingRoute(route)) + return NULL; + // Needs to be outside the if block so it's not destroyed prematurely. BgpAttrPtr attr_ptr; const BgpAttr *attr = path->GetAttr(); @@ -544,3 +542,13 @@ void BgpTable::UpdatePathCount(const BgpPath *path, int count) { infeasible_path_count_ += count; } } + +// Check whether the route is aggregate route +bool BgpTable::IsAggregateRoute(const BgpRoute *route) const { + return routing_instance()->IsAggregateRoute(this, route); +} + +// Check whether the route is contributing route to aggregate route +bool BgpTable::IsContributingRoute(const BgpRoute *route) const { + return routing_instance()->IsContributingRoute(this, route); +} diff --git a/src/bgp/bgp_table.h b/src/bgp/bgp_table.h index 87ad17db536..20a41476213 100644 --- a/src/bgp/bgp_table.h +++ b/src/bgp/bgp_table.h @@ -132,6 +132,12 @@ class BgpTable : public RouteTable { return infeasible_path_count_; } + // Check whether the route is aggregate route + bool IsAggregateRoute(const BgpRoute *route) const; + + // Check whether the route is contributing route to aggregate route + bool IsContributingRoute(const BgpRoute *route) const; + private: class DeleteActor; friend class BgpTableTest; diff --git a/src/bgp/routing-instance/route_aggregate.cc b/src/bgp/routing-instance/route_aggregate.cc index f512a221a6c..fd127495f11 100644 --- a/src/bgp/routing-instance/route_aggregate.cc +++ b/src/bgp/routing-instance/route_aggregate.cc @@ -4,10 +4,11 @@ #include "bgp/routing-instance/route_aggregate.h" +#include + #include #include - -#include +#include #include "base/lifetime.h" #include "base/task_annotations.h" @@ -17,27 +18,31 @@ using std::make_pair; using std::string; -class AggregateRouteState : public DBState { +class RouteAggregatorState : public DBState { public: - AggregateRouteState() : contributor_(false), aggregator_(false) { + RouteAggregatorState() : contributor_(false), aggregator_(false) { } void set_aggregating_info(AggregateRoutePtr aggregator) { + assert(!aggregating_info_); aggregating_info_ = aggregator; aggregator_ = true; } void reset_aggregating_info() { + assert(aggregating_info_); aggregating_info_ = NULL; aggregator_ = false; } void set_contributing_info(AggregateRoutePtr aggregator) { + assert(!contributing_info_); contributing_info_ = aggregator; contributor_ = true; } void reset_contributing_info() { + assert(contributing_info_); contributing_info_ = NULL; contributor_ = false; } @@ -57,12 +62,13 @@ class AggregateRouteState : public DBState { bool aggregator() const { return aggregator_; } + private: AggregateRoutePtr contributing_info_; bool contributor_; AggregateRoutePtr aggregating_info_; bool aggregator_; - DISALLOW_COPY_AND_ASSIGN(AggregateRouteState); + DISALLOW_COPY_AND_ASSIGN(RouteAggregatorState); }; template @@ -79,8 +85,7 @@ class AggregateRoute : public ConditionMatch { enum CompareResult { NoChange = 0, - PrefixChange = 1, - NexthopChange = 2, + NexthopChange = 1, }; AggregateRoute(RoutingInstance *rtinstance, AggregateRouteMgrT *manager, @@ -96,7 +101,7 @@ class AggregateRoute : public ConditionMatch { } // Compare config and return whether cfg has updated - CompareResult CompareAggregateRouteCfg(const AggregateRouteConfig &cfg); + CompareResult CompareConfig(const AggregateRouteConfig &cfg); const PrefixT &aggregate_route_prefix() const { return aggregate_route_prefix_; @@ -167,6 +172,12 @@ class AggregateRoute : public ConditionMatch { return false; } + bool IsContributingRoute(BgpRoute *route) const { + uint32_t part_id = route->get_table_partition()->index(); + return (contributors_[part_id].find(route) != + contributors_[part_id].end()); + } + void NotifyContributingRoute(BgpRoute *route) { DBRequest req; req.oper = DBRequest::DB_ENTRY_NOTIFY; @@ -176,34 +187,36 @@ class AggregateRoute : public ConditionMatch { bgp_table()->Enqueue(&req); } - AggregateRouteState *LocateRouteState(BgpRoute *route) { - AggregateRouteState *state = static_cast + RouteAggregatorState *LocateRouteState(BgpRoute *route) { + RouteAggregatorState *state = static_cast (route->GetState(bgp_table(), manager_->listener_id())); if (state == NULL) { - state = new AggregateRouteState(); + state = new RouteAggregatorState(); route->SetState(bgp_table(), manager_->listener_id(), state); } return state; } - void AddContributingRoute(BgpRoute *route) { - contributors_[route->get_table_partition()->index()].insert(route); - AggregateRouteState *state = LocateRouteState(route); + bool AddContributingRoute(BgpRoute *route) { + uint32_t part_id = route->get_table_partition()->index(); + contributors_[part_id].insert(route); + RouteAggregatorState *state = LocateRouteState(route); state->set_contributing_info(AggregateRoutePtr(this)); NotifyContributingRoute(route); + return (contributors_[part_id].size() == 1); } - void ClearRouteState(BgpRoute *route, AggregateRouteState *state) { + void ClearRouteState(BgpRoute *route, RouteAggregatorState *state) { if (!state->aggregator() && !state->contributor()) { route->ClearState(bgp_table(), manager_->listener_id()); delete state; } } - void RemoveContributingRoute(BgpRoute *route) { - int num_deleted = - contributors_[route->get_table_partition()->index()].erase(route); - AggregateRouteState *state = static_cast + bool RemoveContributingRoute(BgpRoute *route) { + uint32_t part_id = route->get_table_partition()->index(); + int num_deleted = contributors_[part_id].erase(route); + RouteAggregatorState *state = static_cast (route->GetState(bgp_table(), manager_->listener_id())); if (state) { state->reset_contributing_info(); @@ -212,6 +225,7 @@ class AggregateRoute : public ConditionMatch { } else { assert(num_deleted != 1); } + return contributors_[part_id].empty(); } private: @@ -239,13 +253,11 @@ AggregateRoute::AggregateRoute(RoutingInstance *rtinstance, // Compare config and return whether cfg has updated template -typename AggregateRoute::CompareResult AggregateRoute::CompareAggregateRouteCfg( +typename AggregateRoute::CompareResult AggregateRoute::CompareConfig( const AggregateRouteConfig &cfg) { AddressT address = this->GetAddress(cfg.aggregate); PrefixT prefix(address, cfg.prefix_length); - if (aggregate_route_prefix_ != prefix) { - return PrefixChange; - } + assert(aggregate_route_prefix_ == prefix); if (nexthop_ != cfg.nexthop) { return NexthopChange; } @@ -300,42 +312,43 @@ bool AggregateRoute::Match(BgpServer *server, BgpTable *table, // route. As part of the notification, route will become contributing to // most specific aggregate route prefix. // - if (contributors_[route->get_table_partition()->index()].find(route) != - contributors_[route->get_table_partition()->index()].end()) { + if (IsContributingRoute(route)) { if (!IsBestMatch(route)) deleted = true; - // - // If the route is already contributing route of other aggregate prefix - // of this bgp-table, ignore it - // - } else if (table->routing_instance()->IsContributingRoute(table, route)) + } else if (table->IsContributingRoute(route)) { + // + // If the route is already contributing route of other aggregate + // prefix of this bgp-table, ignore it + // return false; + } } // // Consider route only if it matches most specific aggregate prefix - // configured on the routing instance. e.g. if routing instance has following - // prefixes configured, 1/8, 1.1/16 and 1.1.1/24, 1.1.1.1/32 should match to - // 1.1.1/24 as most specific route. + // configured on the routing instance. e.g. if routing instance has + // following prefixes configured, 1/8, 1.1/16 and 1.1.1/24, + // 1.1.1.1/32 should match to 1.1.1/24 as most specific route. // if (!deleted && !IsBestMatch(route)) return false; BgpConditionListener *listener = server->condition_listener(GetFamily()); bool state_added = listener->CheckMatchState(table, route, this); + bool trigger_eval = false; if (!deleted) { if (!state_added) { listener->SetMatchState(table, route, this); - AddContributingRoute(route); + trigger_eval = AddContributingRoute(route); } } else { if (!state_added) { // Not seen ADD ignore DELETE return false; } - RemoveContributingRoute(route); + trigger_eval = RemoveContributingRoute(route); listener->RemoveMatchState(table, route, this); } - manager_->EvaluateRouteAggregate(this); + if (trigger_eval) manager_->EvaluateAggregateRoute(this); return true; } @@ -357,14 +370,14 @@ void AggregateRoute::AddAggregateRoute() { aggregate_route->ClearDelete(); } - BgpPath *existing_path = aggregate_route->FindPath(BgpPath::Aggregation, 0); + BgpPath *existing_path = aggregate_route->FindPath(BgpPath::Aggregate, 0); assert(existing_path == NULL); BgpAttrSpec attrs; BgpAttrNextHop attr_nexthop(this->GetAddress(nexthop())); attrs.push_back(&attr_nexthop); BgpAttrPtr attr = routing_instance()->server()->attr_db()->Locate(attrs); - BgpPath *new_path = new BgpPath(BgpPath::Aggregation, + BgpPath *new_path = new BgpPath(BgpPath::Aggregate, attr.get(), BgpPath::ResolveNexthop, 0); bgp_table()->path_resolver()->StartPathResolution(partition->index(), new_path, aggregate_route); @@ -385,17 +398,17 @@ void AggregateRoute::UpdateAggregateRoute() { aggregate_route_->ClearDelete(); - BgpPath *existing_path = aggregate_route_->FindPath(BgpPath::Aggregation, 0); + BgpPath *existing_path = aggregate_route_->FindPath(BgpPath::Aggregate, 0); if (existing_path) bgp_table()->path_resolver()->StopPathResolution(partition->index(), existing_path); - aggregate_route_->RemovePath(BgpPath::Aggregation); + aggregate_route_->RemovePath(BgpPath::Aggregate); BgpAttrSpec attrs; BgpAttrNextHop attr_nexthop(this->GetAddress(nexthop())); attrs.push_back(&attr_nexthop); BgpAttrPtr attr = routing_instance()->server()->attr_db()->Locate(attrs); - BgpPath *new_path = new BgpPath(BgpPath::Aggregation, + BgpPath *new_path = new BgpPath(BgpPath::Aggregate, attr.get(), BgpPath::ResolveNexthop, 0); bgp_table()->path_resolver()->StartPathResolution(partition->index(), new_path, aggregate_route_); @@ -415,12 +428,12 @@ void AggregateRoute::RemoveAggregateRoute() { (bgp_table()->GetTablePartition(aggregate_route_)); BgpPath *existing_path = - aggregate_route->FindPath(BgpPath::Aggregation, 0); + aggregate_route->FindPath(BgpPath::Aggregate, 0); assert(existing_path != NULL); bgp_table()->path_resolver()->StopPathResolution(partition->index(), existing_path); - aggregate_route->RemovePath(BgpPath::Aggregation); + aggregate_route->RemovePath(BgpPath::Aggregate); if (!aggregate_route->BestPath()) { partition->Delete(aggregate_route); @@ -434,11 +447,11 @@ template void AggregateRoute::set_aggregate_route(BgpRoute *aggregate) { if (aggregate) { assert(aggregate_route_ == NULL); - AggregateRouteState *state = LocateRouteState(aggregate); + RouteAggregatorState *state = LocateRouteState(aggregate); state->set_aggregating_info(AggregateRoutePtr(this)); } else { assert(aggregate_route_ != NULL); - AggregateRouteState *state = static_cast + RouteAggregatorState *state = static_cast (aggregate_route_->GetState(bgp_table(), manager_->listener_id())); assert(state); state->reset_aggregating_info(); @@ -475,12 +488,12 @@ RouteAggregator::RouteAggregator(RoutingInstance *rtinstance) : rtinstance_(rtinstance), condition_listener_(rtinstance_->server()->condition_listener(GetFamily())), listener_id_(DBTableBase::kInvalidId), - add_remove_contributing_route_trigger_(new TaskTrigger( - boost::bind(&RouteAggregator::ProcessRouteAggregateUpdate, this), + update_list_trigger_(new TaskTrigger( + boost::bind(&RouteAggregator::ProcessUpdateList, this), TaskScheduler::GetInstance()->GetTaskId("bgp::RouteAggregation"), 0)), - resolve_trigger_(new TaskTrigger( - boost::bind(&RouteAggregator::ProcessUnregisterResolveConfig, this), + unregister_list_trigger_(new TaskTrigger( + boost::bind(&RouteAggregator::ProcessUnregisterList, this), TaskScheduler::GetInstance()->GetTaskId("bgp::Config"), 0)), deleter_(new DeleteActor(this)), instance_delete_ref_(this, rtinstance->deleter()) { @@ -521,7 +534,7 @@ void RouteAggregator::UpdateAggregateRouteConfig() { sort(aggregate_route_list.begin(), aggregate_route_list.end(), CompareAggregateRouteConfig); - // TODO templatize the sync operation + // TODO(prakashmb): templatize the sync operation AggregateRouteList::const_iterator aggregate_route_cfg_it = aggregate_route_list.begin(); typename AggregateRouteMap::iterator oper_it = aggregate_route_map_.begin(); @@ -625,10 +638,10 @@ void RouteAggregator::RetryDelete() { } template -void RouteAggregator::EvaluateRouteAggregate(AggregateRoutePtr entry) { +void RouteAggregator::EvaluateAggregateRoute(AggregateRoutePtr entry) { tbb::mutex::scoped_lock lock(mutex_); update_aggregate_list_.insert(entry); - add_remove_contributing_route_trigger_->Set(); + update_list_trigger_->Set(); } template @@ -636,22 +649,22 @@ void RouteAggregator::UnregisterAndResolveRouteAggregate( AggregateRoutePtr entry) { tbb::mutex::scoped_lock lock(mutex_); unregister_aggregate_list_.insert(entry); - resolve_trigger_->Set(); + unregister_list_trigger_->Set(); } template bool RouteAggregator::IsAggregateRoute(const BgpRoute *route) const { - AggregateRouteState *state = static_cast + RouteAggregatorState *state = static_cast (route->GetState(bgp_table(), listener_id())); if (state) { - return (state->contributor() == false); + return (state->aggregator()); } return false; } template bool RouteAggregator::IsContributingRoute(const BgpRoute *route) const { - AggregateRouteState *state = static_cast + RouteAggregatorState *state = static_cast (route->GetState(bgp_table(), listener_id())); if (state) { return state->contributor(); @@ -676,7 +689,7 @@ void RouteAggregator::LocateAggregateRoutePrefix(const AggregateRouteConfig static_cast(it->second.get()); // Check whether the config has got updated typename AggregateRouteT::CompareResult change = - match->CompareAggregateRouteCfg(cfg); + match->CompareConfig(cfg); // No change.. if (change == AggregateRouteT::NoChange) return; @@ -720,16 +733,16 @@ void RouteAggregator::StopAggregateRouteDone(BgpTable *table, } template -bool RouteAggregator::ProcessUnregisterResolveConfig() { +bool RouteAggregator::ProcessUnregisterList() { CHECK_CONCURRENCY("bgp::Config"); for (AggregateRouteProcessList::iterator it = unregister_aggregate_list_.begin(); it != unregister_aggregate_list_.end(); ++it) { - AggregateRoutePtr aggregate = *it; - AggregateRouteT *info = static_cast(aggregate.get()); - aggregate_route_map_.erase(info->aggregate_route_prefix()); - condition_listener_->UnregisterMatchCondition(info->bgp_table(), info); + AggregateRouteT *aggregate = static_cast(it->get()); + aggregate_route_map_.erase(aggregate->aggregate_route_prefix()); + condition_listener_->UnregisterMatchCondition(aggregate->bgp_table(), + aggregate); } unregister_aggregate_list_.clear(); @@ -742,7 +755,7 @@ bool RouteAggregator::ProcessUnregisterResolveConfig() { } template -bool RouteAggregator::ProcessRouteAggregateUpdate() { +bool RouteAggregator::ProcessUpdateList() { CHECK_CONCURRENCY("bgp::RouteAggregation"); for (AggregateRouteProcessList::iterator @@ -774,31 +787,31 @@ bool RouteAggregator::RouteListener(DBTablePartBase *root, // Enable/Disable task triggers template void RouteAggregator::DisableRouteAggregateUpdate() { - add_remove_contributing_route_trigger_->set_disable(); + update_list_trigger_->set_disable(); } template void RouteAggregator::EnableRouteAggregateUpdate() { - add_remove_contributing_route_trigger_->set_enable(); + update_list_trigger_->set_enable(); } template -size_t RouteAggregator::GetUpdateAggregateListSize() const{ +size_t RouteAggregator::GetUpdateAggregateListSize() const { return update_aggregate_list_.size(); } template -void RouteAggregator::DisableUnregResolveTask(){ - resolve_trigger_->set_disable(); +void RouteAggregator::DisableUnregResolveTask() { + unregister_list_trigger_->set_disable(); } template -void RouteAggregator::EnableUnregResolveTask(){ - resolve_trigger_->set_enable(); +void RouteAggregator::EnableUnregResolveTask() { + unregister_list_trigger_->set_enable(); } template -size_t RouteAggregator::GetUnregResolveListSize() const{ +size_t RouteAggregator::GetUnregResolveListSize() const { return unregister_aggregate_list_.size(); } diff --git a/src/bgp/routing-instance/route_aggregate.h b/src/bgp/routing-instance/route_aggregate.h index 1d9be651f18..6c59cff9c37 100644 --- a/src/bgp/routing-instance/route_aggregate.h +++ b/src/bgp/routing-instance/route_aggregate.h @@ -73,15 +73,15 @@ typedef ConditionMatchPtr AggregateRoutePtr; // // On the successful match, AggregateRoute calls AddContributingRoute or // RemoveContributingRoute based the state and puts the AggregateRoute object -// in update_aggregate_list_ and trigger add_remove_contributing_route_trigger_ +// in update_aggregate_list_ and trigger update_list_trigger_ // task trigger to process the aggregate route. // -// In task trigger method for add_remove_contributing_route_trigger_ is +// In task trigger method for update_list_trigger_ is // responsible for creating and deleting the Aggregate route. // Aggregate route is added when first contributing route is added to // contributors_ and removed when last contributing route is removed // -// AggregatorRoute method creates the aggregate route with RouteAggregation as +// RouteAggregator creates the aggregate route with Aggregate as // path source and ResolveNexthop as flags. The BgpAttribute on the aggregate // route contains all the property as specified in the config. Currently, // config supports specifying only the nexthop for the aggregate route. @@ -105,29 +105,33 @@ typedef ConditionMatchPtr AggregateRoutePtr; // When route-aggregate is removed from the routing instance for a given prefix, // RouteAggregator invokes RemoveMatchCondition to initiate the delete process. // StopAggregateRouteDone callback indicates the RouteAggregator about -// completion of remove process and it triggers resolve_trigger_ to unregister -// the match condition. StopAggregateRouteDone callback puts the +// completion of remove process and it triggers unregister_list_trigger_ to +// unregister the match condition. StopAggregateRouteDone callback puts the // AggregateRoute object in unregister_aggregate_list_. // // When the route-aggregate for a given prefix is in delete process, the // AggregateRoute is still maintained in the aggregate_route_map_. This will // avoid/ensure new route-aggregate config with same object is handled only // after successful delete completion of previous AggregateRoute match object -// resolve_trigger_ is executed in bgp::Config task and walks the +// unregister_list_trigger_ is executed in bgp::Config task and walks the // unregister_aggregate_list_ to complete the deletion process by calling // UnregisterMatchCondition(). It also triggers ProcessAggregateRouteConfig to // apply new config for pending delete prefixes. // -// DBState: AggregateRouteState: +// DBState: RouteAggregatorState: // ============================ // // Route agggregator registers with the BgpTable to set the DBState. -// The AggregateRouteState implements the DBState. +// The RouteAggregatorState implements the DBState. // The DBState is added on both matching/contributing route and aggregate route. -// Boolean in this object identify whether the state is added for aggregat route -// or not. IsContributingRoute() API exposed by the RouteAggregator access the -// DBState on the BgpRoute to return the state. Similarly RouteAggregator -// provides API to query whether route is aggregated route, IsAggregateRoute(). +// A route can be both contributing and aggregating route at the same time. +// So two boolean fields are used to indicate the state of the route. +// IsContributingRoute()/IsAggregateRoute API exposed by the RouteAggregator +// access the DBState on the BgpRoute to return the state. +// In addition to the boolean, a reference to the match object is stored in the +// DBState. "aggregating_info_" is the match condition object valid for +// aggregating route and "contributing_info_" refers to the match condition to +// which the route is contributing. // // Lifetime management // =================== @@ -139,9 +143,19 @@ typedef ConditionMatchPtr AggregateRoutePtr; // 2. update_aggregate_list_ is not empty [contributing routes are processed] // 3. unregister_aggregate_list_ is not empty [unregister of Match condition // is complete] -// Task triggers add_remove_contributing_route_trigger_ and resolve_trigger_ will +// Task triggers update_list_trigger_ and unregister_list_trigger_ will // call RetryDelete() to complete the delete RouteAggregator object // +// Concurrency +// =========== +// bgp::RouteAggregation task runs in execlusion to any task that adds/deletes +// path from route. i.e. db::DBTable, bgp::ServiceChain, bgp::StaticRoute and +// bgp::ResolverPath. +// bgp::RouteAggregation runs in exclusion to bgp::Config task +// Match() function of the AggregateRoute class is run in per partition +// db::DBTable task. Hence the "contributors_" maintains the contributing routes +// in per partition list to allow concurrent access +// template class RouteAggregator : public IRouteAggregator { public: @@ -178,7 +192,7 @@ class RouteAggregator : public IRouteAggregator { void ManagedDelete(); void RetryDelete(); - void EvaluateRouteAggregate(AggregateRoutePtr entry); + void EvaluateAggregateRoute(AggregateRoutePtr entry); void UnregisterAndResolveRouteAggregate(AggregateRoutePtr entry); virtual bool IsAggregateRoute(const BgpRoute *route) const; @@ -193,8 +207,8 @@ class RouteAggregator : public IRouteAggregator { void RemoveAggregateRoutePrefix(const PrefixT &static_route); void StopAggregateRouteDone(BgpTable *table, ConditionMatch *info); - bool ProcessUnregisterResolveConfig(); - bool ProcessRouteAggregateUpdate(); + bool ProcessUnregisterList(); + bool ProcessUpdateList(); bool RouteListener(DBTablePartBase *root, DBEntryBase *entry); @@ -213,8 +227,8 @@ class RouteAggregator : public IRouteAggregator { BgpConditionListener *condition_listener_; DBTableBase::ListenerId listener_id_; AggregateRouteMap aggregate_route_map_; - boost::scoped_ptr add_remove_contributing_route_trigger_; - boost::scoped_ptr resolve_trigger_; + boost::scoped_ptr update_list_trigger_; + boost::scoped_ptr unregister_list_trigger_; tbb::mutex mutex_; AggregateRouteProcessList update_aggregate_list_; AggregateRouteProcessList unregister_aggregate_list_; diff --git a/src/bgp/routing-instance/routepath_replicator.cc b/src/bgp/routing-instance/routepath_replicator.cc index 47804b2b332..bf0a2d510d0 100644 --- a/src/bgp/routing-instance/routepath_replicator.cc +++ b/src/bgp/routing-instance/routepath_replicator.cc @@ -480,7 +480,7 @@ bool RoutePathReplicator::RouteListener(TableState *ts, // if (!rt->IsUsable() || (table->IsRouteAggregationSupported() && !rtinstance->deleted() && - rtinstance->IsContributingRoute(table, rt))) { + table->IsContributingRoute(rt))) { if (!dbstate) { return true; } diff --git a/src/bgp/test/route_aggregation_test.cc b/src/bgp/test/route_aggregation_test.cc index 895e52f5916..968b63ced4c 100644 --- a/src/bgp/test/route_aggregation_test.cc +++ b/src/bgp/test/route_aggregation_test.cc @@ -1580,6 +1580,8 @@ TEST_F(RouteAggregationTest, ConfigDelete_DelayedRouteProcessing) { "route-aggregate", "vn_subnet", "route-aggregate-routing-instance"); task_util::WaitForIdle(); + TASK_UTIL_EXPECT_EQ(GetUnregResolveListSize("test", Address::INET), 0); + EnableRouteAggregateUpdate("test", Address::INET); TASK_UTIL_EXPECT_EQ(GetUpdateAggregateListSize("test", Address::INET), 0);