From: Marcin Siodelski Date: Wed, 22 Feb 2023 10:56:12 +0000 (+0100) Subject: [#2764] Preserve lease mgr callbacks on recreate X-Git-Tag: Kea-2.3.6~87 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e9110c05b747f6762d3486e8633f9a0494db962a;p=thirdparty%2Fkea.git [#2764] Preserve lease mgr callbacks on recreate --- diff --git a/src/lib/dhcpsrv/cfg_db_access.cc b/src/lib/dhcpsrv/cfg_db_access.cc index 5e04b085f0..5fdda531b5 100644 --- a/src/lib/dhcpsrv/cfg_db_access.cc +++ b/src/lib/dhcpsrv/cfg_db_access.cc @@ -55,9 +55,8 @@ CfgDbAccess::getHostDbAccessStringList() const { void CfgDbAccess::createManagers() const { - // Recreate lease manager. - LeaseMgrFactory::destroy(); - LeaseMgrFactory::create(getLeaseDbAccessString()); + // Recreate lease manager without preserving the registered callbacks. + LeaseMgrFactory::recreate(getLeaseDbAccessString(), false); // Recreate host data source. HostMgr::create(); diff --git a/src/lib/dhcpsrv/lease_mgr_factory.cc b/src/lib/dhcpsrv/lease_mgr_factory.cc index f5334dd7da..a7c0e5fb9a 100644 --- a/src/lib/dhcpsrv/lease_mgr_factory.cc +++ b/src/lib/dhcpsrv/lease_mgr_factory.cc @@ -32,10 +32,10 @@ using namespace std; namespace isc { namespace dhcp { -boost::scoped_ptr& +boost::scoped_ptr& LeaseMgrFactory::getLeaseMgrPtr() { - static boost::scoped_ptr leaseMgrPtr; - return (leaseMgrPtr); + static boost::scoped_ptr lease_mgr_ptr; + return (lease_mgr_ptr); } void @@ -101,14 +101,33 @@ LeaseMgrFactory::destroy() { getLeaseMgrPtr().reset(); } +void +LeaseMgrFactory::recreate(const std::string& dbaccess, bool preserve_callbacks) { + TrackingLeaseMgr::CallbackContainerPtr callbacks; + // Preserve the callbacks if needed. + if (preserve_callbacks && haveInstance()) { + callbacks = instance().callbacks_; + } + + // Re-create the manager. + destroy(); + create(dbaccess); + + if (callbacks) { + // Copy the callbacks to the new instance. It should be fast + // because we merely copy the pointer. + instance().callbacks_ = callbacks; + } +} + bool LeaseMgrFactory::haveInstance() { return (getLeaseMgrPtr().get()); } -LeaseMgr& +TrackingLeaseMgr& LeaseMgrFactory::instance() { - LeaseMgr* lmptr = getLeaseMgrPtr().get(); + TrackingLeaseMgr* lmptr = getLeaseMgrPtr().get(); if (lmptr == NULL) { isc_throw(NoLeaseManager, "no current lease manager is available"); } diff --git a/src/lib/dhcpsrv/lease_mgr_factory.h b/src/lib/dhcpsrv/lease_mgr_factory.h index 82cf0dfaae..69fef85dc4 100644 --- a/src/lib/dhcpsrv/lease_mgr_factory.h +++ b/src/lib/dhcpsrv/lease_mgr_factory.h @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2020 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2023 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -8,7 +8,7 @@ #define LEASE_MGR_FACTORY_H #include -#include +#include #include #include @@ -76,6 +76,19 @@ public: /// lease manager is available. static void destroy(); + /// @brief Recreate an instance of a lease manager with optionally + /// preserving registered callbacks. + /// + /// @param dbaccess Database access parameters. These are in the form of + /// "keyword=value" pairs, separated by spaces. They are backend- + /// -end specific, although must include the "type" keyword which + /// gives the backend in use. + /// @param preserve_callbacks a boolean flag indicating if all registered + /// @c TrackingLeaseMgr callbacks should be copied to the new + /// instance. + static void recreate(const std::string& dbaccess, + bool preserve_callbacks = true); + /// @brief Return current lease manager /// /// Returns an instance of the "current" lease manager. An exception @@ -83,7 +96,7 @@ public: /// /// @throw isc::dhcp::NoLeaseManager No lease manager is available: use /// create() to create one before calling this method. - static LeaseMgr& instance(); + static TrackingLeaseMgr& instance(); /// @brief Indicates if the lease manager has been instantiated. /// @@ -96,7 +109,7 @@ private: /// Holds a pointer to the singleton lease manager. The singleton /// is encapsulated in this method to avoid a "static initialization /// fiasco" if defined in an external static variable. - static boost::scoped_ptr& getLeaseMgrPtr(); + static boost::scoped_ptr& getLeaseMgrPtr(); }; diff --git a/src/lib/dhcpsrv/mysql_lease_mgr.cc b/src/lib/dhcpsrv/mysql_lease_mgr.cc index 572dafc42a..7175bec171 100644 --- a/src/lib/dhcpsrv/mysql_lease_mgr.cc +++ b/src/lib/dhcpsrv/mysql_lease_mgr.cc @@ -1890,8 +1890,7 @@ MySqlLeaseMgr::dbReconnect(ReconnectCtlPtr db_reconnect_ctl) { // At least one connection was lost. try { CfgDbAccessPtr cfg_db = CfgMgr::instance().getCurrentCfg()->getCfgDbAccess(); - LeaseMgrFactory::destroy(); - LeaseMgrFactory::create(cfg_db->getLeaseDbAccessString()); + LeaseMgrFactory::recreate(cfg_db->getLeaseDbAccessString()); reopened = true; } catch (const std::exception& ex) { LOG_ERROR(dhcpsrv_logger, DHCPSRV_MYSQL_LEASE_DB_RECONNECT_ATTEMPT_FAILED) diff --git a/src/lib/dhcpsrv/pgsql_lease_mgr.cc b/src/lib/dhcpsrv/pgsql_lease_mgr.cc index b42d66b396..420c58d878 100644 --- a/src/lib/dhcpsrv/pgsql_lease_mgr.cc +++ b/src/lib/dhcpsrv/pgsql_lease_mgr.cc @@ -1368,8 +1368,7 @@ PgSqlLeaseMgr::dbReconnect(ReconnectCtlPtr db_reconnect_ctl) { // At least one connection was lost. try { CfgDbAccessPtr cfg_db = CfgMgr::instance().getCurrentCfg()->getCfgDbAccess(); - LeaseMgrFactory::destroy(); - LeaseMgrFactory::create(cfg_db->getLeaseDbAccessString()); + LeaseMgrFactory::recreate(cfg_db->getLeaseDbAccessString()); reopened = true; } catch (const std::exception& ex) { LOG_ERROR(dhcpsrv_logger, DHCPSRV_PGSQL_LEASE_DB_RECONNECT_ATTEMPT_FAILED) diff --git a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc index 8f6a1720bd..615677e5b7 100644 --- a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc @@ -4399,6 +4399,50 @@ GenericLeaseMgrTest::testTrackDeleteLease6(bool expect_locked, bool expect_mt_sa EXPECT_FALSE(lmptr_->isLocked(lease)); } +void +GenericLeaseMgrTest::testRecreateWithCallbacks(const std::string& access) { + // Register a callback. + lmptr_->registerCallback(TrackingLeaseMgr::TRACK_ADD_LEASE, 0, "flq", + std::bind(&GenericLeaseMgrTest::logCallback, + this, + TrackingLeaseMgr::TRACK_ADD_LEASE, + 0, + ph::_1, + ph::_2)); + + // Recreate the lease manager with the callbacks. + ASSERT_NO_THROW(LeaseMgrFactory::recreate(access, true)); + lmptr_ = &(LeaseMgrFactory::instance()); + + // Add a lease. It should trigger the callback. + Lease4Ptr lease = initializeLease4(straddress4_[1]); + EXPECT_TRUE(lmptr_->addLease(lease)); + + // Make sure that the callback has been invoked. + EXPECT_EQ(1, logs_.size()); +} + +void +GenericLeaseMgrTest::testRecreateWithoutCallbacks(const std::string& access) { + // Register a callback. + lmptr_->registerCallback(TrackingLeaseMgr::TRACK_ADD_LEASE, 0, "flq", + std::bind(&GenericLeaseMgrTest::logCallback, + this, + TrackingLeaseMgr::TRACK_ADD_LEASE, + 0, + ph::_1, + ph::_2)); + + // Recreate the lease manager without the callbacks. + ASSERT_NO_THROW(LeaseMgrFactory::recreate(access, false)); + lmptr_ = &(LeaseMgrFactory::instance()); + + // Add a lease. It should not trigger the callback. + Lease4Ptr lease = initializeLease4(straddress4_[1]); + EXPECT_TRUE(lmptr_->addLease(lease)); + EXPECT_TRUE(logs_.empty()); +} + } // namespace test } // namespace dhcp } // namespace isc diff --git a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h index 82f7e41684..bd5ac03be9 100644 --- a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h +++ b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h @@ -626,6 +626,20 @@ public: /// expect that the callbacks are called in the MT-safe context. void testTrackDeleteLease6(bool expect_locked, bool expect_mt_safe); + /// @brief Checks that the lease manager can be recreated and its + /// registered callbacks preserved, if desired. + /// + /// @param access database connection string used for recreating the + /// lease manager. + void testRecreateWithCallbacks(const std::string& access); + + /// @brief Checks that the lease manager can be recreated without the + /// previously registered callbacks. + /// + /// @param access database connection string used for recreating the + /// lease manager. + void testRecreateWithoutCallbacks(const std::string& access); + /// @brief String forms of IPv4 addresses std::vector straddress4_; diff --git a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc index 94d5d4d9d4..403aacced9 100644 --- a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc @@ -235,7 +235,7 @@ public: " lease database backend.\n"; throw; } - lmptr_ = static_cast(&(LeaseMgrFactory::instance())); + lmptr_ = &(LeaseMgrFactory::instance()); } /// @brief Runs IOService and stops after a specified time. @@ -2017,7 +2017,7 @@ TEST_F(MemfileLeaseMgrTest, lease4ContainerIndexUpdate) { // Recreate Memfile_LeaseMgr. LeaseMgrFactory::destroy(); ASSERT_NO_THROW(LeaseMgrFactory::create(dbaccess)); - lmptr_ = static_cast(&(LeaseMgrFactory::instance())); + lmptr_ = &(LeaseMgrFactory::instance()); // We will store addresses here, so it will be easier to randomly // pick a lease. @@ -2060,7 +2060,7 @@ TEST_F(MemfileLeaseMgrTest, lease4ContainerIndexUpdate) { // Recreate Memfile_LeaseMgr. LeaseMgrFactory::destroy(); LeaseMgrFactory::create(dbaccess); - lmptr_ = static_cast(&(LeaseMgrFactory::instance())); + lmptr_ = &(LeaseMgrFactory::instance()); }); // Ok, let's check if the leases are really accessible. @@ -2147,7 +2147,7 @@ TEST_F(MemfileLeaseMgrTest, lease6ContainerIndexUpdate) { // Recreate Memfile_LeaseMgr. LeaseMgrFactory::destroy(); ASSERT_NO_THROW(LeaseMgrFactory::create(dbaccess)); - lmptr_ = static_cast(&(LeaseMgrFactory::instance())); + lmptr_ = &(LeaseMgrFactory::instance()); // We will store addresses here, so it will be easier to randomly // pick a lease. @@ -2191,7 +2191,7 @@ TEST_F(MemfileLeaseMgrTest, lease6ContainerIndexUpdate) { // Recreate Memfile_LeaseMgr. LeaseMgrFactory::destroy(); LeaseMgrFactory::create(dbaccess); - lmptr_ = static_cast(&(LeaseMgrFactory::instance())); + lmptr_ = &(LeaseMgrFactory::instance()); }); // Ok, let's check if the leases are really accessible. @@ -4339,5 +4339,18 @@ TEST_F(MemfileLeaseMgrTest, trackDeleteLease6MultiThreading) { testTrackDeleteLease6(false, true); } +/// @brief Checks that the lease manager can be recreated and its +/// registered callbacks preserved, if desired. +TEST_F(MemfileLeaseMgrTest, recreateWithCallbacks) { + startBackend(V4); + testRecreateWithCallbacks(getConfigString(V4)); +} + +/// @brief Checks that the lease manager can be recreated without the +/// previously registered callbacks. +TEST_F(MemfileLeaseMgrTest, recreateWithoutCallbacks) { + startBackend(V4); + testRecreateWithoutCallbacks(getConfigString(V4)); +} } // namespace diff --git a/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc index 291c72f620..da14ff2baf 100644 --- a/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc @@ -64,7 +64,7 @@ public: throw; } - lmptr_ = static_cast(&(LeaseMgrFactory::instance())); + lmptr_ = &(LeaseMgrFactory::instance()); MultiThreadingMgr::instance().setMode(false); } @@ -104,7 +104,7 @@ public: void reopen(Universe) { LeaseMgrFactory::destroy(); LeaseMgrFactory::create(validMySQLConnectionString()); - lmptr_ = static_cast(&(LeaseMgrFactory::instance())); + lmptr_ = &(LeaseMgrFactory::instance()); } }; @@ -1276,4 +1276,16 @@ TEST_F(MySqlLeaseMgrTest, trackDeleteLease6MultiThreading) { testTrackDeleteLease6(true, false); } +/// @brief Checks that the lease manager can be recreated and its +/// registered callbacks preserved, if desired. +TEST_F(MySqlLeaseMgrTest, recreateWithCallbacks) { + testRecreateWithCallbacks(validMySQLConnectionString()); +} + +/// @brief Checks that the lease manager can be recreated without the +/// previously registered callbacks. +TEST_F(MySqlLeaseMgrTest, recreateWithoutCallbacks) { + testRecreateWithoutCallbacks(validMySQLConnectionString()); +} + } // namespace diff --git a/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc index 42d47bf03c..b18cb91715 100644 --- a/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc @@ -64,7 +64,7 @@ public: throw; } - lmptr_ = static_cast(&(LeaseMgrFactory::instance())); + lmptr_ = &(LeaseMgrFactory::instance()); MultiThreadingMgr::instance().setMode(false); } @@ -104,7 +104,7 @@ public: void reopen(Universe) { LeaseMgrFactory::destroy(); LeaseMgrFactory::create(validPgSQLConnectionString()); - lmptr_ = static_cast(&(LeaseMgrFactory::instance())); + lmptr_ = &(LeaseMgrFactory::instance()); } }; @@ -1265,4 +1265,17 @@ TEST_F(PgSqlLeaseMgrTest, trackDeleteLease6MultiThreading) { testTrackDeleteLease6(true, false); } +/// @brief Checks that the lease manager can be recreated and its +/// registered callbacks preserved, if desired. +TEST_F(PgSqlLeaseMgrTest, recreateWithCallbacks) { + testRecreateWithCallbacks(validPgSQLConnectionString()); +} + +/// @brief Checks that the lease manager can be recreated without the +/// previously registered callbacks. +TEST_F(PgSqlLeaseMgrTest, recreateWithoutCallbacks) { + testRecreateWithoutCallbacks(validPgSQLConnectionString()); +} + + } // namespace diff --git a/src/lib/dhcpsrv/tracking_lease_mgr.cc b/src/lib/dhcpsrv/tracking_lease_mgr.cc index 0a8e7402d7..2be6752435 100644 --- a/src/lib/dhcpsrv/tracking_lease_mgr.cc +++ b/src/lib/dhcpsrv/tracking_lease_mgr.cc @@ -19,7 +19,7 @@ namespace isc { namespace dhcp { TrackingLeaseMgr::TrackingLeaseMgr() - : LeaseMgr() { + : LeaseMgr(), callbacks_(new TrackingLeaseMgr::CallbackContainer()) { } bool @@ -61,7 +61,7 @@ TrackingLeaseMgr::registerCallback(TrackingLeaseMgr::CallbackType type, std::string owner, TrackingLeaseMgr::CallbackFn callback_fn) { // The first index filters the callbacks by type and subnet_id. - auto& idx = callbacks_.get<0>(); + auto& idx = callbacks_->get<0>(); auto range = idx.equal_range(boost::make_tuple(type, subnet_id)); if (range.first != range.second) { // Make sure that the callback for this owner does not exist. @@ -75,7 +75,7 @@ TrackingLeaseMgr::registerCallback(TrackingLeaseMgr::CallbackType type, } } TrackingLeaseMgr::Callback callback{type, owner, subnet_id, callback_fn}; - callbacks_.insert(callback); + callbacks_->insert(callback); } void @@ -88,18 +88,18 @@ TrackingLeaseMgr::registerCallback(TrackingLeaseMgr::CallbackType type, void TrackingLeaseMgr::unregisterCallbacks(SubnetID subnet_id) { // The second index filters the callbacks by the subnet identifier. - auto& idx = callbacks_.get<1>(); + auto& idx = callbacks_->get<1>(); idx.erase(subnet_id); } void TrackingLeaseMgr::unregisterAllCallbacks() { - callbacks_.clear(); + callbacks_->clear(); } bool TrackingLeaseMgr::hasCallbacks() const { - return (!callbacks_.empty()); + return (!callbacks_->empty()); } std::string @@ -127,7 +127,7 @@ void TrackingLeaseMgr::runCallbacksForSubnetID(CallbackType type, SubnetID subnet_id, const LeasePtr& lease, bool mt_safe) { // The first index filters by callback type and subnet_id. - auto& idx_by_type = callbacks_.get<0>(); + auto& idx_by_type = callbacks_->get<0>(); auto cbs = idx_by_type.equal_range(boost::make_tuple(type, subnet_id)); if (cbs.first == cbs.second) { return; diff --git a/src/lib/dhcpsrv/tracking_lease_mgr.h b/src/lib/dhcpsrv/tracking_lease_mgr.h index 0aefc79e8d..017a29b79c 100644 --- a/src/lib/dhcpsrv/tracking_lease_mgr.h +++ b/src/lib/dhcpsrv/tracking_lease_mgr.h @@ -16,7 +16,7 @@ #include #include #include -#include +#include #include #include #include @@ -76,6 +76,12 @@ namespace dhcp { class TrackingLeaseMgr : public LeaseMgr { public: + /// The @c LeaseMgrFactory manages the @c LeaseMgr instances and has + /// to be able to move installed callbacks between them. No other external + /// class can have access to the callbacks container. Thus, we can't make + /// the container public. The friend declaration deals with it cleanly. + friend class LeaseMgrFactory; + /// @brief An enumeration differentiating between lease write operations. typedef enum { TRACK_ADD_LEASE, @@ -104,6 +110,8 @@ public: CallbackFn fn; } Callback; +protected: + /// @brief A multi-index container holding registered callbacks. /// /// The callbacks are accessible via two indexes. The first composite index @@ -125,7 +133,8 @@ public: > > CallbackContainer; -protected: + /// @brief Pointer to the callback container. + typedef boost::shared_ptr CallbackContainerPtr; /// @brief Constructor. TrackingLeaseMgr(); @@ -273,7 +282,7 @@ protected: const LeasePtr& lease, bool mt_safe); /// @brief The multi-index container holding registered callbacks. - CallbackContainer callbacks_; + CallbackContainerPtr callbacks_; /// @brief A set of locked leases. ///