From f7c76a0d8fbaabd7de81faf6902dd4103399d57a Mon Sep 17 00:00:00 2001 From: Nischal Sheth Date: Mon, 9 May 2016 10:41:56 -0700 Subject: [PATCH] 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 | 14 ++ src/base/task.h | 4 +- src/base/task_trigger.cc | 5 + src/base/task_trigger.h | 14 ++ src/bgp/bgp_path.h | 4 + src/bgp/bgp_peer_close.cc | 15 +- src/bgp/bgp_server.cc | 14 ++ src/bgp/bgp_table.cc | 13 +- src/bgp/routing-instance/path_resolver.cc | 50 ++++- src/bgp/routing-instance/path_resolver.h | 9 +- src/bgp/test/bgp_server_test_util.cc | 5 +- src/bgp/test/bgp_server_test_util.h | 2 +- src/bgp/test/path_resolver_test.cc | 215 +++++++++++++++++++++- 13 files changed, 345 insertions(+), 19 deletions(-) diff --git a/src/base/task.cc b/src/base/task.cc index e550c6cd9c4..13306576362 100644 --- a/src/base/task.cc +++ b/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 #include @@ -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); diff --git a/src/base/task.h b/src/base/task.h index 68790f48640..2eeeeff1543 100644 --- a/src/base/task.h +++ b/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_; diff --git a/src/base/task_trigger.cc b/src/base/task_trigger.cc index 6213407f54e..a4d9b9a50fb 100644 --- a/src/base/task_trigger.cc +++ b/src/base/task_trigger.cc @@ -16,6 +16,9 @@ class TaskTrigger::WorkerTask : public Task { parent_->Reset(); return true; } + if (parent_->deferred()) { + return false; + } bool done = (parent_->func_)(); if (done) { parent_->Reset(); @@ -23,6 +26,7 @@ class TaskTrigger::WorkerTask : public Task { 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() { diff --git a/src/base/task_trigger.h b/src/base/task_trigger.h index 0df070efb7a..0bb6c9afd48 100644 --- a/src/base/task_trigger.h +++ b/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 trigger_; tbb::atomic disabled_; + tbb::atomic deferred_; }; #endif /* defined(__ctrlplane__task_trigger__) */ diff --git a/src/bgp/bgp_path.h b/src/bgp/bgp_path.h index 1294c4869fc..159d45e22ed 100644 --- a/src/bgp/bgp_path.h +++ b/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_; } diff --git a/src/bgp/bgp_peer_close.cc b/src/bgp/bgp_peer_close.cc index 0641d445091..cbbb79b8713 100644 --- a/src/bgp/bgp_peer_close.cc +++ b/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(it.operator->())) continue; BgpPath *path = static_cast(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(path)) + continue; switch (action) { case MembershipRequest::RIBIN_SWEEP: diff --git a/src/bgp/bgp_server.cc b/src/bgp/bgp_server.cc index ded2a9b13f0..a6a19848f3b 100644 --- a/src/bgp/bgp_server.cc +++ b/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; diff --git a/src/bgp/bgp_table.cc b/src/bgp/bgp_table.cc index 29e731f4aa6..aa0ea6df36f 100644 --- a/src/bgp/bgp_table.cc +++ b/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(it.operator->()); + + // Skip resolved paths. + if (path->IsResolved()) + continue; + // Skip secondary paths. - if (dynamic_cast(it.operator->())) continue; + if (dynamic_cast(path)) + continue; - BgpPath *path = static_cast(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)); } } diff --git a/src/bgp/routing-instance/path_resolver.cc b/src/bgp/routing-instance/path_resolver.cc index cbfaab18e89..c12d17d479f 100644 --- a/src/bgp/routing-instance/path_resolver.cc +++ b/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); } diff --git a/src/bgp/routing-instance/path_resolver.h b/src/bgp/routing-instance/path_resolver.h index 79fecf84d4f..13c4e74981f 100644 --- a/src/bgp/routing-instance/path_resolver.h +++ b/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_; diff --git a/src/bgp/test/bgp_server_test_util.cc b/src/bgp/test/bgp_server_test_util.cc index 7e5c2bdbaea..f854c7d9fb9 100644 --- a/src/bgp/test/bgp_server_test_util.cc +++ b/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(); diff --git a/src/bgp/test/bgp_server_test_util.h b/src/bgp/test/bgp_server_test_util.h index b903fc6e740..f780dc3cd08 100644 --- a/src/bgp/test/bgp_server_test_util.h +++ b/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(); } diff --git a/src/bgp/test/path_resolver_test.cc b/src/bgp/test/path_resolver_test.cc index 8ee1d9a098a..a68cd823545 100644 --- a/src/bgp/test/path_resolver_test.cc +++ b/src/bgp/test/path_resolver_test.cc @@ -518,6 +518,16 @@ class PathResolverTest : public ::testing::Test { table->path_resolver()->EnableResolverPathUpdateProcessing(); } + void PauseResolverPathUpdateProcessing(const string &instance) { + BgpTable *table = GetTable(instance); + table->path_resolver()->PauseResolverPathUpdateProcessing(); + } + + void ResumeResolverPathUpdateProcessing(const string &instance) { + BgpTable *table = GetTable(instance); + table->path_resolver()->ResumeResolverPathUpdateProcessing(); + } + size_t ResolverPathUpdateListSize(const string &instance) { BgpTable *table = GetTable(instance); return table->path_resolver()->GetResolverPathUpdateListSize(); @@ -1501,7 +1511,7 @@ TYPED_TEST(PathResolverTest, SinglePrefixWithEcmp) { // BGP has multiple paths for same prefix, each with a different nexthop. // Add and remove paths for the prefix. // -TYPED_TEST(PathResolverTest, SinglePrefixWithMultipath) { +TYPED_TEST(PathResolverTest, SinglePrefixWithMultipath1) { PeerMock *bgp_peer1 = this->bgp_peer1_; PeerMock *bgp_peer2 = this->bgp_peer2_; PeerMock *xmpp_peer1 = this->xmpp_peer1_; @@ -1562,11 +1572,55 @@ TYPED_TEST(PathResolverTest, SinglePrefixWithMultipath) { this->DeleteBgpPath(bgp_peer2, "blue", this->BuildPrefix(1)); } +// +// BGP has multiple paths for same prefix, each with the same nexthop. +// Add and remove paths for the prefix. +// +TYPED_TEST(PathResolverTest, SinglePrefixWithMultipath2) { + PeerMock *bgp_peer1 = this->bgp_peer1_; + PeerMock *bgp_peer2 = this->bgp_peer2_; + PeerMock *xmpp_peer1 = this->xmpp_peer1_; + + this->AddBgpPath(bgp_peer1, "blue", this->BuildPrefix(1), + this->BuildHostAddress("192.168.1.100")); + this->AddBgpPath(bgp_peer2, "blue", this->BuildPrefix(1), + this->BuildHostAddress("192.168.1.100")); + + this->AddXmppPath(xmpp_peer1, "blue", + this->BuildPrefix("192.168.1.100", 32), + this->BuildNextHopAddress("172.16.1.1"), 10001); + this->VerifyPathAttributes("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.1.1"), 10001); + + this->DeleteBgpPath(bgp_peer1, "blue", this->BuildPrefix(1)); + this->VerifyPathAttributes("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.1.1"), 10001); + + this->DeleteBgpPath(bgp_peer2, "blue", this->BuildPrefix(1)); + this->VerifyPathNoExists("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.1.1")); + + this->AddBgpPath(bgp_peer1, "blue", this->BuildPrefix(1), + this->BuildHostAddress("192.168.1.100")); + this->AddBgpPath(bgp_peer2, "blue", this->BuildPrefix(1), + this->BuildHostAddress("192.168.1.100")); + this->VerifyPathAttributes("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.1.1"), 10001); + + this->DeleteXmppPath(xmpp_peer1, "blue", + this->BuildPrefix("192.168.1.100", 32)); + this->VerifyPathNoExists("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.1.1")); + + this->DeleteBgpPath(bgp_peer1, "blue", this->BuildPrefix(1)); + this->DeleteBgpPath(bgp_peer2, "blue", this->BuildPrefix(1)); +} + // // BGP has multiple paths for same prefix, each with a different nexthop. // XMPP route for the nexthop is ECMP. // -TYPED_TEST(PathResolverTest, SinglePrefixWithMultipathAndEcmp) { +TYPED_TEST(PathResolverTest, SinglePrefixWithMultipathAndEcmp1) { PeerMock *bgp_peer1 = this->bgp_peer1_; PeerMock *bgp_peer2 = this->bgp_peer2_; PeerMock *xmpp_peer1 = this->xmpp_peer1_; @@ -1675,6 +1729,119 @@ TYPED_TEST(PathResolverTest, SinglePrefixWithMultipathAndEcmp) { this->DeleteBgpPath(bgp_peer2, "blue", this->BuildPrefix(1)); } +// +// BGP has multiple paths for same prefix, each with the same nexthop. +// XMPP route for the nexthop is ECMP. +// +TYPED_TEST(PathResolverTest, SinglePrefixWithMultipathAndEcmp2) { + PeerMock *bgp_peer1 = this->bgp_peer1_; + PeerMock *bgp_peer2 = this->bgp_peer2_; + PeerMock *xmpp_peer1 = this->xmpp_peer1_; + PeerMock *xmpp_peer2 = this->xmpp_peer2_; + + this->AddBgpPath(bgp_peer1, "blue", this->BuildPrefix(1), + this->BuildHostAddress("192.168.1.100")); + this->AddBgpPath(bgp_peer2, "blue", this->BuildPrefix(1), + this->BuildHostAddress("192.168.1.100")); + + this->AddXmppPath(xmpp_peer1, "blue", + this->BuildPrefix("192.168.1.100", 32), + this->BuildNextHopAddress("172.16.1.1"), 10001, + this->BuildNextHopAddress("172.16.2.1")); + this->AddXmppPath(xmpp_peer2, "blue", + this->BuildPrefix("192.168.1.100", 32), + this->BuildNextHopAddress("172.16.1.2"), 10002, + this->BuildNextHopAddress("172.16.2.2")); + this->VerifyPathAttributes("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.1.1"), 10001); + this->VerifyPathAttributes("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.2.1"), 10001); + this->VerifyPathAttributes("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.1.2"), 10002); + this->VerifyPathAttributes("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.2.2"), 10002); + + this->AddXmppPath(xmpp_peer1, "blue", + this->BuildPrefix("192.168.1.100", 32), + this->BuildNextHopAddress("172.16.2.1"), 10001); + this->AddXmppPath(xmpp_peer2, "blue", + this->BuildPrefix("192.168.1.100", 32), + this->BuildNextHopAddress("172.16.2.2"), 10002); + this->VerifyPathNoExists("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.1.1")); + this->VerifyPathAttributes("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.2.1"), 10001); + this->VerifyPathNoExists("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.1.2")); + this->VerifyPathAttributes("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.2.2"), 10002); + + this->AddXmppPath(xmpp_peer1, "blue", + this->BuildPrefix("192.168.1.100", 32), + this->BuildNextHopAddress("172.16.1.1"), 10001, + this->BuildNextHopAddress("172.16.2.1")); + this->AddXmppPath(xmpp_peer2, "blue", + this->BuildPrefix("192.168.1.100", 32), + this->BuildNextHopAddress("172.16.1.2"), 10002, + this->BuildNextHopAddress("172.16.2.2")); + this->VerifyPathAttributes("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.1.1"), 10001); + this->VerifyPathAttributes("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.2.1"), 10001); + this->VerifyPathAttributes("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.1.2"), 10002); + this->VerifyPathAttributes("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.2.2"), 10002); + + this->AddXmppPath(xmpp_peer1, "blue", + this->BuildPrefix("192.168.1.100", 32), + this->BuildNextHopAddress("172.16.1.1"), 10001); + this->AddXmppPath(xmpp_peer2, "blue", + this->BuildPrefix("192.168.1.100", 32), + this->BuildNextHopAddress("172.16.1.2"), 10002); + this->VerifyPathAttributes("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.1.1"), 10001); + this->VerifyPathNoExists("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.2.1")); + this->VerifyPathAttributes("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.1.2"), 10002); + this->VerifyPathNoExists("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.2.2")); + + this->AddXmppPath(xmpp_peer1, "blue", + this->BuildPrefix("192.168.1.100", 32), + this->BuildNextHopAddress("172.16.1.1"), 10003, + this->BuildNextHopAddress("172.16.2.1")); + this->AddXmppPath(xmpp_peer2, "blue", + this->BuildPrefix("192.168.1.100", 32), + this->BuildNextHopAddress("172.16.1.2"), 10004, + this->BuildNextHopAddress("172.16.2.2")); + this->VerifyPathAttributes("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.1.1"), 10003); + this->VerifyPathAttributes("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.2.1"), 10003); + this->VerifyPathAttributes("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.1.2"), 10004); + this->VerifyPathAttributes("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.2.2"), 10004); + + this->DeleteXmppPath(xmpp_peer1, "blue", + this->BuildPrefix("192.168.1.100", 32)); + this->DeleteXmppPath(xmpp_peer2, "blue", + this->BuildPrefix("192.168.1.100", 32)); + this->VerifyPathNoExists("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.1.1")); + this->VerifyPathNoExists("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.2.1")); + this->VerifyPathNoExists("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.1.2")); + this->VerifyPathNoExists("blue", this->BuildPrefix(1), + this->BuildNextHopAddress("172.16.2.2")); + + this->DeleteBgpPath(bgp_peer1, "blue", this->BuildPrefix(1)); + this->DeleteBgpPath(bgp_peer2, "blue", this->BuildPrefix(1)); +} + // // BGP has multiple prefixes, each with the same nexthop. // @@ -2189,6 +2356,50 @@ TYPED_TEST(PathResolverTest, Shutdown4) { TASK_UTIL_EXPECT_EQ(0, bgp_server->routing_instance_mgr()->count()); } +// +// Shutdown server and resolver before all resolver paths are deleted. +// Deletion does not finish till resolver paths are deleted and nexthop +// gets unregistered from condition listener. +// +TYPED_TEST(PathResolverTest, Shutdown5) { + BgpServerTestPtr bgp_server = this->bgp_server_; + PeerMock *bgp_peer1 = this->bgp_peer1_; + PeerMock *bgp_peer2 = this->bgp_peer2_; + + this->AddBgpPath(bgp_peer1, "blue", this->BuildPrefix(1), + this->BuildHostAddress(bgp_peer1->ToString())); + this->AddBgpPath(bgp_peer2, "blue", this->BuildPrefix(1), + this->BuildHostAddress(bgp_peer2->ToString())); + + task_util::WaitForIdle(); + this->PauseResolverPathUpdateProcessing("blue"); + + this->DeleteBgpPath(bgp_peer1, "blue", this->BuildPrefix(1)); + this->DeleteBgpPath(bgp_peer2, "blue", this->BuildPrefix(1)); + TASK_UTIL_EXPECT_EQ(2, this->ResolverPathUpdateListSize("blue")); + + TASK_UTIL_EXPECT_EQ(3, bgp_server->routing_instance_mgr()->count()); + bgp_server->Shutdown(false, false); + + // Verify that all instances are intact. + for (int idx = 0; idx < 100; ++idx) { + usleep(10000); + TASK_UTIL_EXPECT_EQ(3, bgp_server->routing_instance_mgr()->count()); + } + + // Ensure that all bgp::ResolverPath tasks are resumed before any of + // them run. Otherwise, it's possible that some of them run, trigger + // creation of bgp::ResolverNexthop task which runs and triggers the + // creation of bgp::Config task, which in turn destroys PathResolver. + // If this sequence happens before all the bgp::ResolverPath tasks + // are resumed, then would be accessing freed memory. + TaskScheduler::GetInstance()->Stop(); + this->ResumeResolverPathUpdateProcessing("blue"); + TaskScheduler::GetInstance()->Start(); + + TASK_UTIL_EXPECT_EQ(0, bgp_server->routing_instance_mgr()->count()); +} + class TestEnvironment : public ::testing::Environment { virtual ~TestEnvironment() { } };