diff --git a/src/bgp/bgp_table.cc b/src/bgp/bgp_table.cc index e808e5fda16..f0c4bb9f978 100644 --- a/src/bgp/bgp_table.cc +++ b/src/bgp/bgp_table.cc @@ -413,26 +413,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 d02533c62f3..f276883b9eb 100644 --- a/src/bgp/bgp_table.h +++ b/src/bgp/bgp_table.h @@ -94,7 +94,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 201c95d3218..bf29114c3e4 100644 --- a/src/bgp/test/bgp_xmpp_deferq_test.cc +++ b/src/bgp/test/bgp_xmpp_deferq_test.cc @@ -389,10 +389,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(); + } }