From 3c32bac100852a7b71362bed5ae559687dbd3e3e Mon Sep 17 00:00:00 2001 From: Nischal Sheth Date: Mon, 22 Jun 2015 17:33:21 -0700 Subject: [PATCH] Don't delete BgpTable till all walkers are done Add infra to keep track of number of walkers per DBTableBase and invoke the RetryDelete method when the last walker is done. Modify BgpTable to use the new infra. Change-Id: Id33c84c7fac76ab99fdf919045da280b0b5038b4 Closes-Bug: 1468052 --- src/bgp/bgp_table.cc | 22 ++++++++----- src/bgp/bgp_table.h | 2 +- src/bgp/test/bgp_xmpp_deferq_test.cc | 48 ++++++++++++++++++++++++++++ src/db/db_table.cc | 16 +++++++++- src/db/db_table.h | 7 ++++ src/db/db_table_walker.cc | 7 ++++ 6 files changed, 92 insertions(+), 10 deletions(-) diff --git a/src/bgp/bgp_table.cc b/src/bgp/bgp_table.cc index 7f6a8921d37..50cf86ab132 100644 --- a/src/bgp/bgp_table.cc +++ b/src/bgp/bgp_table.cc @@ -405,26 +405,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 e1c5c747737..79341e6b652 100644 --- a/src/bgp/test/bgp_xmpp_deferq_test.cc +++ b/src/bgp/test/bgp_xmpp_deferq_test.cc @@ -405,10 +405,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 0c9bb9782b1..b07726a7c36 100644 --- a/src/db/db_table.cc +++ b/src/db/db_table.cc @@ -102,7 +102,7 @@ class DBTableBase::ListenerInfo { }; DBTableBase::DBTableBase(DB *db, const string &name) - : db_(db), name_(name), info_(new ListenerInfo()) { + : db_(db), name_(name), info_(new ListenerInfo()), walker_count_(0) { } DBTableBase::~DBTableBase() { @@ -136,6 +136,20 @@ void DBTableBase::RunNotify(DBTablePartBase *tpart, DBEntryBase *entry) { info_->RunNotify(tpart, entry); } +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 6a42d851bd7..bc2214390ee 100644 --- a/src/db/db_table.h +++ b/src/db/db_table.h @@ -80,9 +80,11 @@ class DBTableBase { // Calculate the size across all partitions. virtual size_t Size() const { return 0; } + bool empty() const { return (Size() == 0); } // Suspended deletion resume hook for user function virtual void RetryDelete() { } + virtual bool MayDelete() const; DB *database() { return db_; } const DB *database() const { return db_; } @@ -91,11 +93,16 @@ class DBTableBase { bool HasListeners() const; + bool HasWalkers() const { return walker_count_ != 0; } + void incr_walker_count() { walker_count_++; } + uint64_t decr_walker_count() { return --walker_count_; } + private: class ListenerInfo; DB *db_; std::string name_; std::auto_ptr info_; + uint64_t walker_count_; }; // An implementation of DBTableBase that uses boost::set as data-store diff --git a/src/db/db_table_walker.cc b/src/db/db_table_walker.cc index bf5aeed9808..7fa48bd9280 100644 --- a/src/db/db_table_walker.cc +++ b/src/db/db_table_walker.cc @@ -204,6 +204,7 @@ DBTableWalker::WalkId DBTableWalker::WalkTable(DBTable *table, walkerfn, walk_complete); walkers_[i] = walker; } + table->incr_walker_count(); return i; } @@ -217,6 +218,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) { @@ -232,4 +234,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(); + } }