BGPaaSv2: Further progress on AS override implementation

Highlights:

- Schema and config parsing changes to enable AS override
- Implement split horizon check within an eBGP RibOut
- Strip non-transitive attributes when sending routes to eBGP peers
- Tweak resolver to retain IPeer from original BgpPath so that split
  horizon check can work for resolved paths
- Make sure that resolved paths don't get deleted prematurely
- Add logic to prevent BgpLifetimeManager from deleting objects when
  any resolver tasks are on the wait queue

Change-Id: I2872faf1d382192ea401f1948d842a59bfa0a4a8
Partial-Bug: 1552952

src/base/task.cc

@@ -12,6 +12,7 @@
 #include "tbb/enumerable_thread_specific.h"
 #include "base/logging.h"
 #include "base/task.h"
+#include "base/task_annotations.h"
 
 #include <sandesh/sandesh_types.h>
 #include <sandesh/sandesh.h>
@@ -404,6 +405,19 @@ TaskGroup *TaskScheduler::QueryTaskGroup(int task_id) {
     return task_group_db_[task_id];
 }
 
+//
+// Check if there are any Tasks in the given TaskGroup.
+// Assumes that all task ids are mutually exclusive with bgp::Config.
+//
+bool TaskScheduler::IsTaskGroupEmpty(int task_id) const {
+    CHECK_CONCURRENCY("bgp::Config");
+    tbb::mutex::scoped_lock lock(mutex_);
+    TaskGroup *group = task_group_db_[task_id];
+    assert(group);
+    assert(group->TaskRunCount() == 0);
+    return group->IsWaitQEmpty();
+}
+
 // Get TaskGroup for a task_id. Grows task_entry_db_ if necessary
 TaskEntry *TaskScheduler::GetTaskEntry(int task_id, int task_instance) {
     TaskGroup *group = GetTaskGroup(task_id);

src/base/task.h

@@ -175,6 +175,8 @@ class TaskScheduler {
 
     TaskGroup *GetTaskGroup(int task_id);
     TaskGroup *QueryTaskGroup(int task_id);
+    bool IsTaskGroupEmpty(int task_id) const;
+
     TaskEntry *GetTaskEntry(int task_id, int instance_id);
     TaskEntry *QueryTaskEntry(int task_id, int instance_id);
     void OnTaskExit(Task *task);
@@ -233,7 +235,7 @@ class TaskScheduler {
     TaskEntry               *stop_entry_;
 
     tbb::task_scheduler_init task_scheduler_;
-    tbb::mutex              mutex_;
+    mutable tbb::mutex      mutex_;
     bool                    running_;
     uint64_t                seqno_;
     TaskGroupDb             task_group_db_;

src/base/task_trigger.cc

@@ -16,13 +16,17 @@ class TaskTrigger::WorkerTask : public Task {
             parent_->Reset();
             return true;
         }
+        if (parent_->deferred()) {
+            return false;
+        }
         bool done = (parent_->func_)();
         if (done) {
             parent_->Reset();
         }
         return done;
     }
     std::string Description() const { return "TaskTrigger::WorkerTask"; }
+
 private:
     TaskTrigger *parent_;
 };
@@ -31,6 +35,7 @@ TaskTrigger::TaskTrigger(FunctionPtr func, int task_id, int task_instance)
     : func_(func), task_id_(task_id), task_instance_(task_instance) {
     trigger_ = false;
     disabled_ = false;
+    deferred_ = false;
 }
 
 TaskTrigger::~TaskTrigger() {

src/base/task_trigger.h

@@ -29,6 +29,19 @@ class TaskTrigger {
         return disabled_;
     }
 
+    // For Test only
+    void set_deferred() {
+        bool current = deferred_.fetch_and_store(true);
+        assert(!current);
+    }
+    void clear_deferred() {
+        bool current = deferred_.fetch_and_store(false);
+        assert(current);
+    }
+    bool deferred() {
+        return deferred_;
+    }
+
 private:
     class WorkerTask;
 
@@ -37,6 +50,7 @@ class TaskTrigger {
     int task_instance_;
     tbb::atomic<bool> trigger_;
     tbb::atomic<bool> disabled_;
+    tbb::atomic<bool> deferred_;
 };
 
 #endif /* defined(__ctrlplane__task_trigger__) */

src/bgp/bgp_config_ifmap.cc

@@ -243,6 +243,7 @@ static void NeighborSetSessionAttributes(
     }
     if (attributes != NULL) {
         neighbor->set_passive(attributes->passive);
+        neighbor->set_as_override(attributes->as_override);
         neighbor->set_loop_count(attributes->loop_count);
         if (attributes->admin_down) {
             neighbor->set_admin_down(true);

src/bgp/bgp_path.h

@@ -105,6 +105,10 @@ class BgpPath : public Path {
         return ((flags_ & (INFEASIBLE_MASK & ~ResolveNexthop)) == 0);
     }
 
+    bool IsResolved() const {
+        return ((flags_ & ResolvedPath) != 0);
+    }
+
     uint32_t GetFlags() const {
         return flags_;
     }

src/bgp/bgp_peer_close.cc

@@ -306,14 +306,21 @@ void PeerCloseManager::ProcessRibIn(DBTablePartBase *root, BgpRoute *rt,
          it != rt->GetPathList().end(); it = next) {
         next++;
 
-        // Skip secondary paths.
-        if (dynamic_cast<BgpSecondaryPath *>(it.operator->()))
-            continue;
         BgpPath *path = static_cast<BgpPath *>(it.operator->());
+
+        // Skip paths from other peers.
         if (path->GetPeer() != peer_)
             continue;
-        uint32_t stale = 0;
 
+        // Skip resolved paths - PathResolver is responsible for them.
+        if (path->IsResolved())
+            continue;
+
+        // Skip secondary paths.
+        if (dynamic_cast<BgpSecondaryPath *>(path))
+            continue;
+
+        uint32_t stale = 0;
         switch (action) {
             case MembershipRequest::RIBIN_SWEEP:
 

src/bgp/bgp_ribout.cc

@@ -445,6 +445,20 @@ const RibPeerSet &RibOut::PeerSet() const {
     return active_peerset_;
 }
 
+//
+// Clear the bit index corresponding to the specified peer.
+// Used to implement split horizon within an EBGP Ribout.
+//
+void RibOut::GetSubsetPeerSet(RibPeerSet *peerset,
+    const IPeerUpdate *cpeer) const {
+    assert(policy_.type == BgpProto::EBGP);
+    IPeerUpdate *peer = const_cast<IPeerUpdate *>(cpeer);
+    int index = GetPeerIndex(peer);
+    if (index < 0)
+        return;
+    peerset->reset(index);
+}
+
 //
 // Return the peer corresponding to the specified bit index.
 //

src/bgp/bgp_ribout.h

@@ -259,6 +259,7 @@ class RibOut {
 
     // Returns a bitmask with all the peers that are advertising this RibOut.
     const RibPeerSet &PeerSet() const;
+    void GetSubsetPeerSet(RibPeerSet *peerset, const IPeerUpdate *cpeer) const;
 
     BgpTable* table() const { return table_; }
 

src/bgp/bgp_server.cc

@@ -258,6 +258,20 @@ void BgpServer::RetryDelete() {
 bool BgpServer::IsReadyForDeletion() {
     CHECK_CONCURRENCY("bgp::Config");
 
+    static TaskScheduler *scheduler = TaskScheduler::GetInstance();
+    static int resolver_path_task_id =
+        scheduler->GetTaskId("bgp::ResolverPath");
+    static int resolver_nexthop_task_id =
+        scheduler->GetTaskId("bgp::ResolverNexthop");
+
+    // Check if any PathResolver is active.
+    // Need to ensure that there's no pending deletes of BgpPaths added by
+    // PathResolver since they hold pointers to IPeer.
+    if (!scheduler->IsTaskGroupEmpty(resolver_path_task_id) ||
+        !scheduler->IsTaskGroupEmpty(resolver_nexthop_task_id)) {
+        return false;
+    }
+
     // Check if the IPeer membership manager queue is empty.
     if (!membership_mgr_->IsQueueEmpty()) {
         return false;

src/bgp/bgp_table.cc

@@ -205,22 +205,35 @@ UpdateInfo *BgpTable::GetUpdateInfo(RibOut *ribout, BgpRoute *route,
                 return NULL;
             }
 
-            // Sender side AS path loop check.
-            if (!ribout->as_override() && attr->as_path() &&
-                attr->as_path()->path().AsPathLoop(ribout->peer_as())) {
-                return NULL;
-            }
-
             // Handle route-target filtering.
-            if (attr->ext_community() != NULL) {
+            if (IsVpnTable() && attr->ext_community() != NULL) {
                 server()->rtarget_group_mgr()->GetRibOutInterestedPeers(
                     ribout, attr->ext_community(), peerset, &new_peerset);
                 if (new_peerset.empty())
                     return NULL;
             }
 
+            // Sender side AS path loop check and split horizon within RibOut.
+            if (!ribout->as_override()) {
+                if (attr->as_path() &&
+                    attr->as_path()->path().AsPathLoop(ribout->peer_as())) {
+                    return NULL;
+                }
+            } else {
+                if (peer && peer->PeerType() == BgpProto::EBGP) {
+                    ribout->GetSubsetPeerSet(&new_peerset, peer);
+                    if (new_peerset.empty())
+                        return NULL;
+                }
+            }
+
             BgpAttr *clone = new BgpAttr(*attr);
 
+            // Remove non-transitive attributes.
+            // Note that med is handled further down.
+            clone->set_originator_id(Ip4Address());
+            clone->set_cluster_list(NULL);
+
             // Update nexthop.
             if (!ribout->nexthop().is_unspecified())
                 clone->set_nexthop(ribout->nexthop());
@@ -390,12 +403,17 @@ void BgpTable::Input(DBTablePartition *root, DBClient *client,
     // Mark this peer's all paths as deleted.
     for (Route::PathList::iterator it = rt->GetPathList().begin();
          it != rt->GetPathList().end(); ++it) {
+        BgpPath *path = static_cast<BgpPath *>(it.operator->());
+
+        // Skip resolved paths.
+        if (path->IsResolved())
+            continue;
+
         // Skip secondary paths.
-        if (dynamic_cast<BgpSecondaryPath *>(it.operator->())) continue;
+        if (dynamic_cast<BgpSecondaryPath *>(path))
+            continue;
 
-        BgpPath *path = static_cast<BgpPath *>(it.operator->());
-        if (path->GetPeer() == peer &&
-                path->GetSource() == BgpPath::BGP_XMPP) {
+        if (path->GetPeer() == peer && path->GetSource() == BgpPath::BGP_XMPP) {
             deleted_paths.insert(make_pair(path, true));
         }
     }

src/bgp/routing-instance/path_resolver.cc

@@ -476,6 +476,26 @@ void PathResolver::EnableResolverPathUpdateProcessing() {
     }
 }
 
+//
+// Pause processing of the path update list in all partitions.
+// For testing only.
+//
+void PathResolver::PauseResolverPathUpdateProcessing() {
+    for (int part_id = 0; part_id < DB::PartitionCount(); ++part_id) {
+        partitions_[part_id]->PauseResolverPathUpdateProcessing();
+    }
+}
+
+//
+// Resume processing of the path update list in all partitions.
+// For testing only.
+//
+void PathResolver::ResumeResolverPathUpdateProcessing() {
+    for (int part_id = 0; part_id < DB::PartitionCount(); ++part_id) {
+        partitions_[part_id]->ResumeResolverPathUpdateProcessing();
+    }
+}
+
 //
 // Get size of the update list.
 // For testing only.
@@ -713,6 +733,22 @@ void PathResolverPartition::EnableResolverPathUpdateProcessing() {
     rpath_update_trigger_->set_enable();
 }
 
+//
+// Pause processing of the update list.
+// For testing only.
+//
+void PathResolverPartition::PauseResolverPathUpdateProcessing() {
+    rpath_update_trigger_->set_deferred();
+}
+
+//
+// Resume processing of the update list.
+// For testing only.
+//
+void PathResolverPartition::ResumeResolverPathUpdateProcessing() {
+    rpath_update_trigger_->clear_deferred();
+}
+
 //
 // Get size of the update list.
 // For testing only.
@@ -800,10 +836,12 @@ ResolverPath::~ResolverPath() {
 //
 void ResolverPath::AddResolvedPath(ResolvedPathList::const_iterator it) {
     BgpPath *path = *it;
+    const IPeer *peer = path->GetPeer();
     resolved_path_list_.insert(path);
     route_->InsertPath(path);
     BGP_LOG_STR(BgpMessage, SandeshLevel::SYS_DEBUG, BGP_LOG_FLAG_TRACE,
         "Added resolved path " << route_->ToString() <<
+        " peer " << (peer ? peer->ToString() : "None") <<
         " path_id " << BgpPath::PathIdString(path->GetPathId()) <<
         " nexthop " << path->GetAttr()->nexthop().to_string() <<
         " label " << path->GetLabel() <<
@@ -816,8 +854,10 @@ void ResolverPath::AddResolvedPath(ResolvedPathList::const_iterator it) {
 //
 void ResolverPath::DeleteResolvedPath(ResolvedPathList::const_iterator it) {
     BgpPath *path = *it;
+    const IPeer *peer = path->GetPeer();
     BGP_LOG_STR(BgpMessage, SandeshLevel::SYS_DEBUG, BGP_LOG_FLAG_TRACE,
         "Deleted resolved path " << route_->ToString() <<
+        " peer " << (peer ? peer->ToString() : "None") <<
         " path_id " << BgpPath::PathIdString(path->GetPathId()) <<
         " nexthop " << path->GetAttr()->nexthop().to_string() <<
         " label " << path->GetLabel() <<
@@ -829,12 +869,13 @@ void ResolverPath::DeleteResolvedPath(ResolvedPathList::const_iterator it) {
 //
 // Find or create the matching resolved BgpPath.
 //
-BgpPath *ResolverPath::LocateResolvedPath(uint32_t path_id,
+BgpPath *ResolverPath::LocateResolvedPath(const IPeer *peer, uint32_t path_id,
     const BgpAttr *attr, uint32_t label) {
     for (ResolvedPathList::iterator it = resolved_path_list_.begin();
          it != resolved_path_list_.end(); ++it) {
         BgpPath *path = *it;
-        if (path->GetPathId() == path_id &&
+        if (path->GetPeer() == peer &&
+            path->GetPathId() == path_id &&
             path->GetAttr() == attr &&
             path->GetLabel() == label) {
             return path;
@@ -844,7 +885,7 @@ BgpPath *ResolverPath::LocateResolvedPath(uint32_t path_id,
     BgpPath::PathSource src = path_->GetSource();
     uint32_t flags =
         (path_->GetFlags() & ~BgpPath::ResolveNexthop) | BgpPath::ResolvedPath;
-    return (new BgpPath(path_id, src, attr, flags, label));
+    return (new BgpPath(peer, path_id, src, attr, flags, label));
 }
 
 //
@@ -910,6 +951,7 @@ bool ResolverPath::UpdateResolvedPaths() {
     // resolved paths.
     ResolvedPathList future_resolved_path_list;
     const BgpRoute *nh_route = rnexthop_->route();
+    const IPeer *peer = path_ ? path_->GetPeer() : NULL;
     Route::PathList::const_iterator it;
     if (path_ && nh_route)
         it = nh_route->GetPathList().begin();
@@ -954,7 +996,7 @@ bool ResolverPath::UpdateResolvedPaths() {
         // Locate the resolved path.
         uint32_t path_id = nh_path->GetAttr()->nexthop().to_v4().to_ulong();
         BgpPath *resolved_path =
-            LocateResolvedPath(path_id, attr.get(), nh_path->GetLabel());
+            LocateResolvedPath(peer, path_id, attr.get(), nh_path->GetLabel());
         future_resolved_path_list.insert(resolved_path);
     }