diff --git a/src/bgp/bgp_table.cc b/src/bgp/bgp_table.cc index 2820a73e5e7..33abdd353af 100644 --- a/src/bgp/bgp_table.cc +++ b/src/bgp/bgp_table.cc @@ -414,26 +414,32 @@ void BgpTable::Input(DBTablePartition *root, DBClient *client, bool BgpTable::MayDelete() const { CHECK_CONCURRENCY("bgp::Config"); - // - // The route replicator may be still in the processes of cleaning up - // the table. + // Bail if the table has listeners. if (HasListeners()) { BGP_LOG_TABLE(this, SandeshLevel::SYS_DEBUG, BGP_LOG_FLAG_ALL, "Paused table deletion due to pending listeners"); return false; } - // - // This table cannot be deleted yet if any route is still present. - // + // Bail if the table has walkers. + if (HasWalkers()) { + BGP_LOG_TABLE(this, SandeshLevel::SYS_DEBUG, BGP_LOG_FLAG_ALL, + "Paused table deletion due to pending walkers"); + return false; + } + + // Bail if the table is not empty. size_t size = Size(); if (size > 0) { BGP_LOG_TABLE(this, SandeshLevel::SYS_DEBUG, BGP_LOG_FLAG_ALL, "Paused table deletion due to " << size << - " pending entries"); + " pending routes"); return false; } - return true; + + // Check the base class at the end so that we add custom checks + // before this if needed and to get more informative log message. + return DBTableBase::MayDelete(); } void BgpTable::Shutdown() { diff --git a/src/bgp/bgp_table.h b/src/bgp/bgp_table.h index 5c58b4a2130..6230b9417bc 100644 --- a/src/bgp/bgp_table.h +++ b/src/bgp/bgp_table.h @@ -97,7 +97,7 @@ class BgpTable : public RouteTable { void ManagedDelete(); virtual void RetryDelete(); void Shutdown(); - bool MayDelete() const; + virtual bool MayDelete() const; bool IsDeleted() const { return deleter()->IsDeleted(); } RoutingInstance *routing_instance() { return rtinstance_; } diff --git a/src/bgp/test/bgp_xmpp_deferq_test.cc b/src/bgp/test/bgp_xmpp_deferq_test.cc index f17c4bf30ed..2544fc81229 100644 --- a/src/bgp/test/bgp_xmpp_deferq_test.cc +++ b/src/bgp/test/bgp_xmpp_deferq_test.cc @@ -408,10 +408,58 @@ class BgpXmppSerializeMembershipReqTest : public BgpXmppUnitTest { } }; +static bool DummyWalkFunction(DBTablePartBase *root, DBEntryBase *entry) { + return true; +} + +static void DummyWalkDoneFunction(DBTableBase *table) { +} + int BgpXmppUnitTest::validate_done_; namespace { +TEST_F(BgpXmppUnitTest, TableDeleteWithPendingWalk) { + Configure(); + task_util::WaitForIdle(); + + // create an XMPP client in server A + agent_a_.reset( + new test::NetworkAgentMock(&evm_, SUB_ADDR, xs_a_->GetPort())); + + // Pause deletion for blue inet table. + BgpTable *blue_table = VerifyBgpTable("blue", Address::INET); + PauseDelete(blue_table->deleter()); + + // Unconfigure all instances. + // The blue instance and blue inet table should exist in deleted state. + UnconfigureRoutingInstances(); + task_util::WaitForIdle(); + RoutingInstance *blue = VerifyRoutingInstance("blue"); + TASK_UTIL_EXPECT_TRUE(blue->deleted()); + TASK_UTIL_EXPECT_TRUE(blue_table->IsDeleted()); + + // Stop the scheduler and resume delete of the table. + // This should create the bgp::Config Task to process lifetime manager + // work queue. + TaskScheduler::GetInstance()->Stop(); + blue_table->deleter()->ResumeDelete(); + + // Start a bunch of table walks. + DBTableWalker *walker = a_->database()->GetWalker(); + for (int idx = 0; idx < 128; ++idx) { + walker->WalkTable( + blue_table, NULL, DummyWalkFunction, DummyWalkDoneFunction); + } + + // Start the scheduler. + // Table should get deleted only after all the walks are done. + TaskScheduler::GetInstance()->Start(); + + // The blue instance should have been destroyed. + VerifyNoRoutingInstance("blue"); +} + TEST_F(BgpXmppUnitTest, Connection) { Configure(); task_util::WaitForIdle(); diff --git a/src/db/db_table.cc b/src/db/db_table.cc index 291ff9b58c3..038bf05e43d 100644 --- a/src/db/db_table.cc +++ b/src/db/db_table.cc @@ -158,7 +158,8 @@ class DBTableBase::ListenerInfo { DBTableBase::DBTableBase(DB *db, const string &name) : db_(db), name_(name), info_(new ListenerInfo(name)), - enqueue_count_(0), input_count_(0), notify_count_(0) { + enqueue_count_(0), input_count_(0), notify_count_(0), + walker_count_(0) { walk_request_count_ = 0; walk_complete_count_ = 0; walk_cancel_count_ = 0; @@ -202,6 +203,20 @@ void DBTableBase::AddToDBStateCount(ListenerId listener, int count) { info_->AddToDBStateCount(listener, count); } +bool DBTableBase::MayDelete() const { + if (HasListeners()) { + return false; + } + if (HasWalkers()) { + return false; + } + if (!empty()) { + return false; + } + + return true; +} + bool DBTableBase::HasListeners() const { return !info_->empty(); } diff --git a/src/db/db_table.h b/src/db/db_table.h index caacc7af96d..d815d38189c 100644 --- a/src/db/db_table.h +++ b/src/db/db_table.h @@ -83,7 +83,7 @@ class DBTableBase { void RunNotify(DBTablePartBase *tpart, DBEntryBase *entry); - // manage db state count for a listner + // Manage db state count for a listener. void AddToDBStateCount(ListenerId listener, int count); // Calculate the size across all partitions. @@ -93,6 +93,7 @@ class DBTableBase { // Suspended deletion resume hook for user function virtual void RetryDelete() { } + virtual bool MayDelete() const; DB *database() { return db_; } const DB *database() const { return db_; } @@ -115,6 +116,10 @@ class DBTableBase { void incr_notify_count() { notify_count_++; } void reset_notify_count() { notify_count_ = 0; } + bool HasWalkers() const { return walker_count_ != 0; } + void incr_walker_count() { walker_count_++; } + uint64_t decr_walker_count() { return --walker_count_; } + uint64_t walk_request_count() const { return walk_request_count_; } uint64_t walk_complete_count() const { return walk_complete_count_; } uint64_t walk_cancel_count() const { return walk_cancel_count_; } @@ -130,6 +135,7 @@ class DBTableBase { uint64_t enqueue_count_; uint64_t input_count_; uint64_t notify_count_; + uint64_t walker_count_; tbb::atomic walk_request_count_; tbb::atomic walk_complete_count_; tbb::atomic walk_cancel_count_; diff --git a/src/db/db_table_walker.cc b/src/db/db_table_walker.cc index ea6c4549a58..017388018d6 100644 --- a/src/db/db_table_walker.cc +++ b/src/db/db_table_walker.cc @@ -202,6 +202,7 @@ DBTableWalker::WalkId DBTableWalker::WalkTable(DBTable *table, walkerfn, walk_complete); walkers_[i] = walker; } + table->incr_walker_count(); return i; } @@ -214,6 +215,7 @@ void DBTableWalker::WalkCancel(WalkId id) { void DBTableWalker::PurgeWalker(WalkId id) { tbb::mutex::scoped_lock lock(walkers_mutex_); Walker *walker = walkers_[id]; + DBTable *table = walker->table_; delete walker; walkers_[id] = NULL; if ((size_t) id == walkers_.size() - 1) { @@ -229,4 +231,9 @@ void DBTableWalker::PurgeWalker(WalkId id) { } walker_map_.set(id); } + + // Retry table deletion when the last walker is purged. + if (table->decr_walker_count() == 0) { + table->RetryDelete(); + } }