Handle multipath for bgpaas when paths have same nexthop

Following changes are implemented:

- Tweak resolver to retain IPeer from original BgpPath
- Add PathResolver tests for multipath with same nexthop

Change-Id: I4c4afd0393cceef089a0ede9acb8ccda1901bad4
Closes-Bug: 1578504

src/base/task.cc

@@ -13,6 +13,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>
@@ -430,6 +431,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

@@ -180,6 +180,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);
@@ -243,7 +245,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_path.h

@@ -104,6 +104,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

@@ -271,10 +271,19 @@ 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->());
-        if (path->GetPeer() != peer_) continue;
+
+        // Skip paths from other peers.
+        if (path->GetPeer() != peer_)
+            continue;
+
+        // Skip resolved paths - PathResolver is responsible for them.
+        if (path->IsResolved())
+            continue;
+
+        // Skip secondary paths.
+        if (dynamic_cast<BgpSecondaryPath *>(path))
+            continue;
 
         switch (action) {
             case MembershipRequest::RIBIN_SWEEP:

src/bgp/bgp_server.cc

@@ -259,6 +259,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

@@ -408,12 +408,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);
     }
 

src/bgp/routing-instance/path_resolver.h

@@ -27,6 +27,7 @@ class BgpTable;
 class DBEntryBase;
 class DBTablePartBase;
 class DeleteActor;
+class IPeer;
 class PathResolverPartition;
 class ResolverNexthop;
 class ResolverPath;
@@ -144,6 +145,8 @@ class PathResolver {
 
     void DisableResolverPathUpdateProcessing();
     void EnableResolverPathUpdateProcessing();
+    void PauseResolverPathUpdateProcessing();
+    void ResumeResolverPathUpdateProcessing();
     size_t GetResolverPathUpdateListSize() const;
 
     BgpTable *table_;
@@ -214,6 +217,8 @@ class PathResolverPartition {
 
     void DisableResolverPathUpdateProcessing();
     void EnableResolverPathUpdateProcessing();
+    void PauseResolverPathUpdateProcessing();
+    void ResumeResolverPathUpdateProcessing();
     size_t GetResolverPathUpdateListSize() const;
 
     int part_id_;
@@ -307,8 +312,8 @@ class ResolverPath {
 
     void AddResolvedPath(ResolvedPathList::const_iterator it);
     void DeleteResolvedPath(ResolvedPathList::const_iterator it);
-    BgpPath *LocateResolvedPath(uint32_t path_id, const BgpAttr *attr,
-        uint32_t label);
+    BgpPath *LocateResolvedPath(const IPeer *peer, uint32_t path_id,
+        const BgpAttr *attr, uint32_t label);
 
     PathResolverPartition *partition_;
     const BgpPath *path_;

src/bgp/test/bgp_server_test_util.cc

@@ -78,8 +78,9 @@ void BgpServerTest::PostShutdown() {
     config_db_->Clear();
 }
 
-void BgpServerTest::Shutdown(bool verify) {
-    task_util::WaitForIdle();
+void BgpServerTest::Shutdown(bool verify, bool wait_for_idle) {
+    if (wait_for_idle)
+        task_util::WaitForIdle();
     BgpServer::Shutdown();
     if (verify)
         VerifyShutdown();

src/bgp/test/bgp_server_test_util.h

@@ -195,7 +195,7 @@ class BgpServerTest : public BgpServer {
                               const std::string &name);
     void DisableAllPeers();
     void EnableAllPeers();
-    void Shutdown(bool verify = true);
+    void Shutdown(bool verify = true, bool wait_for_idle = true);
     void VerifyShutdown() const;
 
     DB *config_db() { return config_db_.get(); }