From c8837f49d0bd0ccb235fd2636033ba483e4cd170 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Tue, 8 Sep 2015 16:25:06 +0200 Subject: [PATCH 1/9] [3973] Implemented first version of the lease reclamation routine. --- src/lib/dhcpsrv/alloc_engine.cc | 65 ++ src/lib/dhcpsrv/alloc_engine.h | 19 + src/lib/dhcpsrv/tests/Makefile.am | 1 + .../tests/alloc_engine_expiration_unittest.cc | 641 ++++++++++++++++++ 4 files changed, 726 insertions(+) create mode 100644 src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index fa66d2e35a..8afc1182ac 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -14,8 +14,12 @@ #include +#include +#include #include #include +#include +#include #include #include #include @@ -34,6 +38,7 @@ using namespace isc::asiolink; using namespace isc::dhcp; +using namespace isc::dhcp_ddns; using namespace isc::hooks; using namespace isc::stats; @@ -1272,6 +1277,66 @@ AllocEngine::updateLeaseData(ClientContext6& ctx, const Lease6Collection& leases return (updated_leases); } +void +AllocEngine::reclaimExpiredLeases6(const size_t max_leases, const uint16_t timeout, + const bool remove_lease) { + LeaseMgr& lease_mgr = LeaseMgrFactory::instance(); + + Lease6Collection leases; + lease_mgr.getExpiredLeases6(leases, max_leases); + + for (Lease6Collection::const_iterator lease_it = leases.begin(); + lease_it != leases.end(); ++lease_it) { + + /// @todo execute a lease6_expire hook here + + /// @todo perform DNS update here + queueNameChangeRequest(*lease_it, *(*lease_it)->duid_); + + // Reclaim the lease - depending on the configuration, set the + // expired-reclaimed state or simply remove it. + if (remove_lease) { + LeaseMgrFactory::instance().deleteLease((*lease_it)->addr_); + + } else { + (*lease_it)->state_ = Lease::STATE_EXPIRED_RECLAIMED; + LeaseMgrFactory::instance().updateLease6(*lease_it); + } + } +} + +template +void +AllocEngine::queueNameChangeRequest(const LeasePtrType& lease, + const IdentifierType& identifier) const { + + if (lease->hostname_.empty() || !lease->fqdn_fwd_ || !lease->fqdn_rev_) { + return; + } + + if (!CfgMgr::instance().getD2ClientMgr().ddnsEnabled()) { + return; + } + + std::vector hostname_wire; + try { + OptionDataTypeUtil::writeFqdn(lease->hostname_, hostname_wire, true); + + } catch (const std::exception& ex) { + + } + + isc::dhcp_ddns::D2Dhcid dhcid(identifier, hostname_wire); + NameChangeRequestPtr ncr; + ncr.reset(new NameChangeRequest(isc::dhcp_ddns::CHG_REMOVE, + lease->fqdn_fwd_, lease->fqdn_rev_, + lease->hostname_, + lease->addr_.toText(), + dhcid, 0, lease->valid_lft_)); + + CfgMgr::instance().getD2ClientMgr().sendRequest(ncr); +} + } // end of isc::dhcp namespace } // end of isc namespace diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index cd8d7774c2..0bf1db7894 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -490,6 +490,21 @@ public: /// @return Returns renewed lease. Lease6Collection renewLeases6(ClientContext6& ctx); + /// @brief Reclaims expired leases. + /// + /// This method retrieves a collection of expired leases and reclaims them. + /// See http://kea.isc.org/wiki/LeaseExpirationDesign#LeasesReclamationRoutine + /// for the details. + /// + /// @param max_leases Maximum number of leases to be reclaimed. + /// @param timeout Maximum amount of time that the reclaimation routine + /// may be processing expired leases, expressed in seconds. + /// @param remove_lease A boolean value indicating if the lease should + /// be removed when it is reclaimed (if true) or it should be left in the + /// database in the "expired-reclaimed" state (if false). + void reclaimExpiredLeases6(const size_t max_leases, const uint16_t timeout, + const bool remove_lease); + /// @brief Attempts to find appropriate host reservation. /// /// Attempts to find appropriate host reservation in HostMgr. If found, it @@ -663,6 +678,10 @@ private: /// @param lease IPv6 lease to be extended. void extendLease6(ClientContext6& ctx, Lease6Ptr lease); + template + void queueNameChangeRequest(const LeasePtrType& lease, + const IdentifierType& identifier) const; + public: /// @brief Context information for the DHCPv4 lease allocation. diff --git a/src/lib/dhcpsrv/tests/Makefile.am b/src/lib/dhcpsrv/tests/Makefile.am index baad1d2ed6..210aece60a 100644 --- a/src/lib/dhcpsrv/tests/Makefile.am +++ b/src/lib/dhcpsrv/tests/Makefile.am @@ -56,6 +56,7 @@ TESTS += libdhcpsrv_unittests libdhcpsrv_unittests_SOURCES = run_unittests.cc libdhcpsrv_unittests_SOURCES += addr_utilities_unittest.cc libdhcpsrv_unittests_SOURCES += alloc_engine_utils.cc alloc_engine_utils.h +libdhcpsrv_unittests_SOURCES += alloc_engine_expiration_unittest.cc libdhcpsrv_unittests_SOURCES += alloc_engine_hooks_unittest.cc libdhcpsrv_unittests_SOURCES += alloc_engine4_unittest.cc libdhcpsrv_unittests_SOURCES += alloc_engine6_unittest.cc diff --git a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc new file mode 100644 index 0000000000..b8d8dc8636 --- /dev/null +++ b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc @@ -0,0 +1,641 @@ +// Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC") +// +// Permission to use, copy, modify, and/or distribute this software for any +// purpose with or without fee is hereby granted, provided that the above +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE +// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +using namespace std; +using namespace isc; +using namespace isc::asiolink; +using namespace isc::dhcp; +using namespace isc::dhcp::test; +using namespace isc::dhcp_ddns; + +namespace { + +/// @brief Number of leases to be initialized for each test. +/// +/// This value is expected by some of the tests to be multiple +/// of 10. +const unsigned int TEST_LEASES_NUM = 100; + +/// @brief Structure wrapping a lower limit within the collection +/// of leases. +/// +/// We're using this structure rather than a size_t value directly +/// to make API of the test fixture class more readable, i.e. the +/// struct name indicates the purpose of the value being held. +struct LowerBound { + /// @brief Constructor. + /// + /// @param lower_bound Interger value wrapped by the structure. + explicit LowerBound(const size_t lower_bound) + : lower_bound_(lower_bound) { }; + + /// @brief Operator returning the size_t value wrapped. + operator size_t() const { + return (lower_bound_); + } + + /// @brief Value wrapped in the structure. + size_t lower_bound_; +}; + +/// @brief Structure wrapping an upper limit within the collection +/// of leases. +/// +/// We're using this structure rather than a size_t value directly +/// to make API of the test fixture class more readable, i.e. the +/// struct name indicates the purpose of the value being held. +struct UpperBound { + /// @brief Constructor. + /// + /// @param lower_bound Interger value wrapped by the structure. + explicit UpperBound(const size_t upper_bound) + : upper_bound_(upper_bound) { }; + + /// @brief Operator returning the size_t value wrapped. + operator size_t() const { + return (upper_bound_); + } + + /// @brief Value wrapped in the structure. + size_t upper_bound_; +}; + +/// @brief Test fixture class for the lease reclamation routines in the +/// @c AllocEngine. +/// +/// This class implements infrastructure for testing leases reclamation +/// routines. The lease reclamation routine has the following +/// characteristic: +/// - it processes multiple leases, +/// - leases are processed in certain order, +/// - number of processed leases may be limited by the parameters, +/// - maxium duration of the lease reclamation routine may be limited, +/// - reclaimed leases may be marked as reclaimed or deleted, +/// - DNS records for some of the leases must be removed when the lease +/// is reclaimed and DNS updates are enabled, +/// - hooks must be invoked (if installed) for each reclaimed lease +/// +/// The typical test requires many leases to be initialized and stored +/// in the lease database for the test. The test fixture class creates +/// these leases upon construction. It is possible to modify these +/// leases to test various properties of the lease reclamation routine +/// as listed above. For example: some of the leases may be marked +/// as expired or hostname may be cleared for some of the leases to +/// check that DNS updates are not generated for them. +/// +/// The tests are built around the +/// @c ExpirationAllocEngine6Test::testLeases methods. These methods +/// verify that the certain operations have been performed by the +/// lease reclamation routine on selected leases. The leases for which +/// certain conditions should be met are selected using the "index +/// algorithms". Various index algorithms are implemented in the +/// test fixture class as static functions and the algorithm is +/// selected by passing function pointer to the @c testLeases method. +/// +/// Examples of index algorithms are: +/// - evenLeaseIndex(index) - picks even index numbers, +/// - oddLeaseIndex(index) - picks odd index numbers, +/// - allLeasesIndexes(index) - picks all index number. +/// +/// For example, the test may use the @c evenLeaseIndex algorithm +/// to mark leases with even indexes as expired and then test whether +/// leases with even indexes have been successfully reclaimed. +/// +/// The "lease algorithm" verifies if the given lease fulfils the +/// specific conditions after reclamation. These are the examples of +/// the lease algorithms: +/// - leaseExists - lease still exists in the database, +/// - leaseDoesntExist - lease removed from the database, +/// - leaseReclaimed - lease exists but has reclaimed status, +/// - leaseNotReclaimed - lease exists and is not in the reclaimed status, +/// - dnsUpdateGeneratedForLease - DNS updates generated for lease, +/// - dnsUpdateNotGeneratedForLease - DNS updates not generated for lease +/// +/// The combination of index algorithm and lease algorithm allows for +/// verifying that the whole sets of leases in the lease database fulfil +/// certain conditions. For example, it is possible to verify that +/// after lease reclamation leases with even indexes have state set to +/// "expired-reclaimed". +/// +/// See @c ExpirationAllocEngine6Test::testLeases for further details. +class ExpirationAllocEngine6Test : public AllocEngine6Test { +public: + + /// @brief Type definition for the lease algorithm. + typedef boost::function LeaseAlgorithmFun; + /// @brief type definition for the lease index algorithm. + typedef boost::function IndexAlgorithmFun; + + /// @brief Class constructor. + /// + /// This constructor initializes @c TEST_LEASES_NUM leases and + /// stores them in the lease manager. + ExpirationAllocEngine6Test(); + + /// @brief Class destructor. + /// + /// It stops the D2 client, if it is running. + virtual ~ExpirationAllocEngine6Test(); + + /// @brief Creates collection of leases for a test. + /// + /// It is called internally at the construction time. + void createLeases(); + + /// @brief Starts D2 client. + void enableDDNS() const; + + /// @brief No-op error handler for the D2 client. + static void d2ErrorHandler(const dhcp_ddns::NameChangeSender::Result, + dhcp_ddns::NameChangeRequestPtr&) { + } + + /// @brief Marks a lease as expired. + /// + /// @param lease_index Lease index. Must be between 0 and + /// @c TEST_LEASES_NUM. + /// @param secs Offset of the expiration time since now. For example + /// a value of 2 would set the lease expiration time to 2 seconds ago. + void expire(const unsigned int lease_index, const time_t secs); + + /// @brief Test selected leases using the specified algorithms. + /// + /// This function picks leases from the range of 0 thru + /// @c TEST_LEASES_NUM and selects the ones to be verified using the + /// specified index algorithm. Selected leases are tested using + /// the specified lease algorithm. + /// + /// @param lease_algorithm Pointer to the lease algorithm function. + /// @param index_algorithm Pointer to the index algorithm function. + bool testLeases(const LeaseAlgorithmFun& lease_algorithm, + const IndexAlgorithmFun& index_algorithm) const; + + + /// @brief Test selected leases using the specified algorithms. + /// + /// This function picks leases from the range of @c lower_bound + /// thru @c upper_bound and selects the ones to be verified using the + /// specified index algorithm. Selected leases are tested using the + /// specified lease algorithm. + /// + /// @param lease_algorithm Pointer to the lease algorithm function. + /// @param index_algorithm Pointer to the index algorithm function. + /// @param lower_bound First index in the range. + /// @param upper_bound Last + 1 index in the range. + bool testLeases(const LeaseAlgorithmFun& lease_algorithm, + const IndexAlgorithmFun& index_algorithm, + const LowerBound& lower_bound, + const UpperBound& upper_bound) const; + + /// @brief Index algorithm selecting even indexes. + /// + /// @param index Lease index. + /// @return true if index is an even number. + static bool evenLeaseIndex(const size_t index) { + return (index % 2); + } + + /// @brief Index algorithm selecting odd indexes. + /// + /// @param index Lease index. + /// @return true if index is an odd number. + static bool oddLeaseIndex(const size_t index) { + return (!evenLeaseIndex(index)); + } + + /// @brief Index algorithm selecting all indexes. + /// + /// @param index Lease index. + /// @return true if the index is in the range of [0 .. TEST_LEASES_NUM). + static bool allLeaseIndexes(const size_t index) { + return (index < TEST_LEASES_NUM); + } + + /// @brief Lease algorithm checking if lease exists. + /// + /// @param lease Pointer to lease. + /// @return true if lease pointer is non-null. + static bool leaseExists(const Lease6Ptr& lease) { + return (static_cast(lease)); + } + + /// @brief Lease algorithm checking if lease doesn't exist. + /// + /// @param lease Pointer to lease. + /// @return true if lease pointer is null. + static bool leaseDoesntExist(const Lease6Ptr& lease) { + return (static_cast(!lease)); + } + + /// @brief Lease algorithm checking if lease state is expired-reclaimed. + /// + /// @param lease Pointer to lease. + /// @return true if lease state is "expired-reclaimed". + static bool leaseReclaimed(const Lease6Ptr& lease) { + return (lease && lease->stateExpiredReclaimed()); + } + + /// @brief Lease algorithm checking if lease state is not + /// expired-reclaimed. + /// + /// @param lease Pointer to lease. + /// @return true if lease state is not "expired-reclaimed". + static bool leaseNotReclaimed(const Lease6Ptr& lease) { + return (lease && !lease->stateExpiredReclaimed()); + } + + /// @brief Lease algorithm checking if removal name change request + /// has been generated for lease. + /// + /// @param lease Pointer to lease. + /// @return true if NCR has been generated for the lease. + static bool dnsUpdateGeneratedForLease(const Lease6Ptr& lease); + + /// @brief Lease algorithm checking if removal name change request + /// hasn't been generated for lease. + /// + /// @param lease Pointer to lease. + /// @return true if NCR has not been generated for the lease. + static bool dnsUpdateNotGeneratedForLease(const Lease6Ptr& lease); + + /// @brief Collection of leases created at construction time. + Lease6Collection leases_; + +}; + +ExpirationAllocEngine6Test::ExpirationAllocEngine6Test() + : AllocEngine6Test() { + createLeases(); +} + +ExpirationAllocEngine6Test::~ExpirationAllocEngine6Test() { + // Stop D2 client if running and remove all queued name change + // requests. + D2ClientMgr& mgr = CfgMgr::instance().getD2ClientMgr(); + if (mgr.amSending()) { + mgr.stopSender(); + mgr.clearQueue(); + } +} + +void +ExpirationAllocEngine6Test::createLeases() { + // Create TEST_LEASES_NUM leases. + for (int i = 0; i < TEST_LEASES_NUM; ++i) { + // DUID + std::ostringstream duid_s; + duid_s << "01020304050607" << std::setw(4) << std::setfill('0') << i; + DuidPtr duid(new DUID(DUID::fromText(duid_s.str()).getDuid())); + + // Address. + std::ostringstream address_s; + address_s << "2001:db8:1::" << std::setw(4) << std::setfill('0') << i; + IOAddress address(address_s.str()); + + // Hostname. + std::ostringstream hostname_s; + hostname_s << "host" << std::setw(4) << std::setfill('0') << i + << ".example.org"; + + // Create lease. + Lease6Ptr lease(new Lease6(Lease::TYPE_NA, address, duid, 1, 50, 60, 10, 20, + SubnetID(1), true, true, hostname_s.str())); + leases_.push_back(lease); + LeaseMgrFactory::instance().addLease(lease); + } +} + +void +ExpirationAllocEngine6Test::enableDDNS() const { + // Start DDNS and assign no-op error handler. + D2ClientMgr& mgr = CfgMgr::instance().getD2ClientMgr(); + D2ClientConfigPtr cfg(new D2ClientConfig()); + cfg->enableUpdates(true); + mgr.setD2ClientConfig(cfg); + mgr.startSender(boost::bind(&ExpirationAllocEngine6Test::d2ErrorHandler, _1, _2)); +} + +void +ExpirationAllocEngine6Test::expire(const unsigned int lease_index, + const time_t secs) { + ASSERT_GT(leases_.size(), lease_index); + // We set the expiration time indirectly by modifying the cltt value. + leases_[lease_index]->cltt_ = time(NULL) - secs - leases_[lease_index]->valid_lft_; + ASSERT_NO_THROW(LeaseMgrFactory::instance().updateLease6(leases_[lease_index])); +} + +bool +ExpirationAllocEngine6Test::testLeases(const LeaseAlgorithmFun& lease_algorithm, + const IndexAlgorithmFun& index_algorithm) const { + // No limits are specified, so test all leases in the range of + // 0 .. TEST_LEASES_NUM. + return (testLeases(lease_algorithm, index_algorithm, LowerBound(0), + UpperBound(TEST_LEASES_NUM))); +} + +bool +ExpirationAllocEngine6Test::testLeases(const LeaseAlgorithmFun& lease_algorithm, + const IndexAlgorithmFun& index_algorithm, + const LowerBound& lower_bound, + const UpperBound& upper_bound) const { + // Select leases between the lower_bound and upper_bound. + for (size_t i = lower_bound; i < upper_bound; ++i) { + // Get the lease from the lease database. + Lease6Ptr lease = LeaseMgrFactory::instance().getLease6(leases_[i]->type_, + leases_[i]->addr_); + // index_algorithm(i) checks if the lease should be checked. + // If so, check if the lease_algorithm indicates that the + // lease fulfils a given condition, e.g. is present in the + // database. If not, return false. + if (index_algorithm(i) && !lease_algorithm(lease)) { + return (false); + } + } + // All leases checked, so return true. + return (true); +} + +bool +ExpirationAllocEngine6Test::dnsUpdateGeneratedForLease(const Lease6Ptr& lease) { + try { + // Iterate over the generated name change requests and try + // to find the match with our lease (using IP address). If + D2ClientMgr& mgr = CfgMgr::instance().getD2ClientMgr(); + for (size_t i = 0; i < mgr.getQueueSize(); ++i) { + const NameChangeRequestPtr& ncr = mgr.peekAt(i); + // If match found, return true. + if (ncr->getIpAddress() == lease->addr_.toText()) { + return (true); + } + } + } catch (...) { + // If error occurred, treat it as no match. + return (false); + } + + // All leases checked - no match. + return (false); +} + +bool +ExpirationAllocEngine6Test::dnsUpdateNotGeneratedForLease(const Lease6Ptr& lease) { + try { + // Iterate over the generated name change requests and try + // to find the match with our lease (using IP address). If + D2ClientMgr& mgr = CfgMgr::instance().getD2ClientMgr(); + for (size_t i = 0; i < mgr.getQueueSize(); ++i) { + const NameChangeRequestPtr& ncr = mgr.peekAt(i); + // If match found, we treat it as if the test fails + // because we expected no NCR. + if (ncr->getIpAddress() == lease->addr_.toText()) { + return (false); + } + } + } catch (...) { + return (false); + } + + // No match found, so we're good. + return (true); +} + +// This test verifies that the leases can be reclaimed without being removed +// from the database. In such case, the leases' state is set to +// "expired-reclaimed". +TEST_F(ExpirationAllocEngine6Test, reclaimExpiredLeases6UpdateState) { + boost::scoped_ptr engine; + ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, + 100, false))); + ASSERT_TRUE(engine); + + for (int i = 0; i < TEST_LEASES_NUM; ++i) { + // Mark leases with even indexes as expired. + if (evenLeaseIndex(i)) { + // The higher the index, the more expired the lease. + expire(i, 10 + i); + } + } + + // Run leases reclamation routine on all leases. This should result + // in setting "expired-reclaimed" state for all leases with even + // indexes. + ASSERT_NO_THROW(engine->reclaimExpiredLeases6(0, 0, false)); + + // Leases with even indexes should be marked as reclaimed. + EXPECT_TRUE(testLeases(&leaseReclaimed, &evenLeaseIndex)); + // Leases with odd indexes shouldn't be marked as reclaimed. + EXPECT_TRUE(testLeases(&leaseNotReclaimed, &oddLeaseIndex)); + +} + +// This test verifies that the reclaimed leases are deleted when requested. +TEST_F(ExpirationAllocEngine6Test, reclaimExpiredLeases6Delete) { + boost::scoped_ptr engine; + ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, + 100, false))); + ASSERT_TRUE(engine); + + for (int i = 0; i < TEST_LEASES_NUM; ++i) { + // Mark leases with even indexes as expired. + if (evenLeaseIndex(i)) { + // The higher the index, the more expired the lease. + expire(i, 10 + i); + } + } + + // Run leases reclamation routine on all leases. This should result + // in removal of all leases with even indexes. + ASSERT_NO_THROW(engine->reclaimExpiredLeases6(0, 0, true)); + + // Leases with odd indexes should be retained and their state + // shouldn't be "expired-reclaimed". + EXPECT_TRUE(testLeases(&leaseNotReclaimed, &oddLeaseIndex)); + // Leases with even indexes should have been removed. + EXPECT_TRUE(testLeases(&leaseDoesntExist, &evenLeaseIndex)); +} + +// This test verifies that it is possible to specify the limit for the +// number of reclaimed leases. +TEST_F(ExpirationAllocEngine6Test, reclaimExpiredLeases6Limit) { + boost::scoped_ptr engine; + ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, + 100, false))); + ASSERT_TRUE(engine); + + for (int i = 0; i < TEST_LEASES_NUM; ++i) { + // Mark all leaes as expired. The higher the index the less + // expired the lease. + expire(i, 1000 - i); + } + + // We will be performing lease reclamation on lease groups of 10. + // Hence, it is convenient if the number of test leases is a + // multiple of 10. + const size_t reclamation_group_size = 10; + BOOST_STATIC_ASSERT(TEST_LEASES_NUM % reclamation_group_size == 0); + + // Leases will be reclaimed in groups of 10. + for (int i = reclamation_group_size; i < TEST_LEASES_NUM; + i += reclamation_group_size) { + + // Reclaim 10 most expired leases out of TEST_LEASES_NUM. Since + // leases are ordered from the most expired to the least expired + // this should reclaim leases between 0 and 9, then 10 and 19 etc. + ASSERT_NO_THROW(engine->reclaimExpiredLeases6(reclamation_group_size, 0, + false)); + + // Check that leases having all indexes between 0 and 9, 19, 29 etc. + // have been reclaimed. + EXPECT_TRUE(testLeases(&leaseReclaimed, &allLeaseIndexes, + LowerBound(0), UpperBound(i))) + << "check failed for i = " << i; + + // Check that all remaining leases haven't been reclaimed. + EXPECT_TRUE(testLeases(&leaseNotReclaimed, &allLeaseIndexes, + LowerBound(i), UpperBound(TEST_LEASES_NUM))) + << "check failed for i = " << i; + } +} + +// This test verifies that DNS updates are generated for the leases +// for which the DNS records exist. +TEST_F(ExpirationAllocEngine6Test, reclaimExpiredLeases6WithDDNS) { + boost::scoped_ptr engine; + ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, + 100, false))); + ASSERT_TRUE(engine); + + // DNS must be started for the D2 client to accept NCRs. + ASSERT_NO_THROW(enableDDNS()); + + for (int i = 0; i < TEST_LEASES_NUM; ++i) { + // Expire all leases with even indexes. + if (evenLeaseIndex(i)) { + // The higher the index, the more expired the lease. + expire(i, 10 + i); + } + } + + // Reclaim all expired leases. + ASSERT_NO_THROW(engine->reclaimExpiredLeases6(0, 0, false)); + + // Leases with odd indexes shouldn't be reclaimed. + EXPECT_TRUE(testLeases(&leaseNotReclaimed, &oddLeaseIndex)); + // Leases with even indexes should be reclaimed. + EXPECT_TRUE(testLeases(&leaseReclaimed, &evenLeaseIndex)); + // DNS updates (removal NCRs) should be generated for leases with even + // indexes. + EXPECT_TRUE(testLeases(&dnsUpdateGeneratedForLease, &evenLeaseIndex)); + // DNS updates (removal NCRs) shouldn't be generated for leases with + // odd indexes. + EXPECT_TRUE(testLeases(&dnsUpdateNotGeneratedForLease, &oddLeaseIndex)); +} + +// This test verifies that it is DNS updates are generated only for the +// reclaimed expired leases. In this case we limit the number of leases +// reclaimed during a single call to reclamation routine. +TEST_F(ExpirationAllocEngine6Test, reclaimExpiredLeases6WithDDNSAndLimit) { + boost::scoped_ptr engine; + ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, + 100, false))); + ASSERT_TRUE(engine); + + // DNS must be started for the D2 client to accept NCRs. + ASSERT_NO_THROW(enableDDNS()); + + for (int i = 0; i < TEST_LEASES_NUM; ++i) { + // Expire only leases with even indexes. + if (evenLeaseIndex(i)) { + // The higher the index, the more expired the lease. + expire(i, 10 + i); + } + } + + const size_t reclamation_group_size = 10; + BOOST_STATIC_ASSERT(TEST_LEASES_NUM % reclamation_group_size == 0); + + // Leases will be reclaimed in groups of 10 + for (int i = 10; i < TEST_LEASES_NUM; i += reclamation_group_size) { + // Reclaim 10 most expired leases. Note that the leases with the higher + // index are more expired. For example, if the TEST_LEASES_NUM is equal + // to 100, the most expired lease will be 98, then 96, 94 etc. + ASSERT_NO_THROW(engine->reclaimExpiredLeases6(reclamation_group_size, 0, + false)); + + // After the first iteration the lower bound is 80, because there will + // be 10 the most expired leases in this group: 80, 82, 84, 86, 88, 90, + // 92, 94, 96, 98. For subsequent iterations accordingly. + int reclaimed_lower_bound = TEST_LEASES_NUM - 2 * i; + // At some point the lower bound will hit the negative value, which + // must be corrected to 0. + if (reclaimed_lower_bound < 0) { + reclaimed_lower_bound = 0; + } + + // Leases between the lower bound calculated above and the upper bound + // of all leases, and having even indexes should have been reclaimed. + EXPECT_TRUE(testLeases(&leaseReclaimed, &evenLeaseIndex, + LowerBound(reclaimed_lower_bound), + UpperBound(TEST_LEASES_NUM))) + << "check failed for i = " << i; + + // For the same leases we should have generated DNS updates (removal NCRs). + EXPECT_TRUE(testLeases(&dnsUpdateGeneratedForLease, &evenLeaseIndex, + LowerBound(reclaimed_lower_bound), + UpperBound(TEST_LEASES_NUM))) + << "check failed for i = " << i; + + // Leases with odd indexes (falling between the reclaimed ones) shouldn't + // have been reclaimed, because they are not expired. + EXPECT_TRUE(testLeases(&leaseNotReclaimed, &oddLeaseIndex, + LowerBound(reclaimed_lower_bound), + UpperBound(TEST_LEASES_NUM))) + << "check failed for i = " << i; + + EXPECT_TRUE(testLeases(&dnsUpdateNotGeneratedForLease, &oddLeaseIndex, + LowerBound(reclaimed_lower_bound), + UpperBound(TEST_LEASES_NUM))) + << "check failed for i = " << i; + + + // At early stages of iterations, there should be conitnuous group of leases + // (expired and not expired) which haven't been reclaimed. + if (reclaimed_lower_bound > 0) { + EXPECT_TRUE(testLeases(&leaseNotReclaimed, &allLeaseIndexes, + LowerBound(0), + UpperBound(reclaimed_lower_bound))) + << "check failed for i = " << i; + + EXPECT_TRUE(testLeases(&dnsUpdateNotGeneratedForLease, &oddLeaseIndex, + LowerBound(0), UpperBound(reclaimed_lower_bound))); + + } + } +} + + + +}; // end of anonymous namespace From d9d36098a7832320b9890bc2fc9a20e67192e0bc Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Wed, 9 Sep 2015 15:44:07 +0200 Subject: [PATCH 2/9] [3973] Added logging to the lease expiration routine. Also added better exception handling. --- src/lib/dhcpsrv/alloc_engine.cc | 100 +++++++++++++----- src/lib/dhcpsrv/alloc_engine.h | 15 ++- src/lib/dhcpsrv/alloc_engine_messages.mes | 32 ++++++ .../tests/alloc_engine_expiration_unittest.cc | 52 ++++++++- 4 files changed, 167 insertions(+), 32 deletions(-) diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 8afc1182ac..85794e7bc2 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -26,6 +26,7 @@ #include #include #include +#include #include #include @@ -1280,6 +1281,16 @@ AllocEngine::updateLeaseData(ClientContext6& ctx, const Lease6Collection& leases void AllocEngine::reclaimExpiredLeases6(const size_t max_leases, const uint16_t timeout, const bool remove_lease) { + + LOG_DEBUG(alloc_engine_logger, ALLOC_ENGINE_DBG_TRACE, + ALLOC_ENGINE_V6_LEASES_RECLAMATION_START) + .arg(max_leases) + .arg(timeout); + + // Create stopwatch and automatically start it to measure the time + // taken by the routine. + util::Stopwatch stopwatch; + LeaseMgr& lease_mgr = LeaseMgrFactory::instance(); Lease6Collection leases; @@ -1288,53 +1299,84 @@ AllocEngine::reclaimExpiredLeases6(const size_t max_leases, const uint16_t timeo for (Lease6Collection::const_iterator lease_it = leases.begin(); lease_it != leases.end(); ++lease_it) { - /// @todo execute a lease6_expire hook here + try { + /// @todo execute a lease6_expire hook here. - /// @todo perform DNS update here - queueNameChangeRequest(*lease_it, *(*lease_it)->duid_); + // Generate removal name change request for D2, if required. + // This will return immediatelly if the DNS wasn't updated + // when the lease was created. + queueRemovalNameChangeRequest(*lease_it, *(*lease_it)->duid_); - // Reclaim the lease - depending on the configuration, set the - // expired-reclaimed state or simply remove it. - if (remove_lease) { - LeaseMgrFactory::instance().deleteLease((*lease_it)->addr_); + // Reclaim the lease - depending on the configuration, set the + // expired-reclaimed state or simply remove it. + if (remove_lease) { + lease_mgr.deleteLease((*lease_it)->addr_); - } else { - (*lease_it)->state_ = Lease::STATE_EXPIRED_RECLAIMED; - LeaseMgrFactory::instance().updateLease6(*lease_it); + } else { + // Clear FQDN information as we have already sent the + // name change request to remove the DNS record. + (*lease_it)->hostname_.clear(); + (*lease_it)->fqdn_fwd_ = false; + (*lease_it)->fqdn_rev_ = false; + (*lease_it)->state_ = Lease::STATE_EXPIRED_RECLAIMED; + lease_mgr.updateLease6(*lease_it); + } + + // Lease has been reclaimed. + LOG_DEBUG(alloc_engine_logger, ALLOC_ENGINE_DBG_TRACE, + ALLOC_ENGINE_V6_LEASE_RECLAIMED) + .arg((*lease_it)->addr_.toText()); + + } catch (const std::exception& ex) { + LOG_ERROR(alloc_engine_logger, ALLOC_ENGINE_V6_LEASE_RECLAMATION_FAILED) + .arg((*lease_it)->addr_.toText()) + .arg(ex.what()); } } + + // Stop measuring the time. + stopwatch.stop(); + + // Mark completion of the lease reclamation routine and present some stats. + LOG_DEBUG(alloc_engine_logger, ALLOC_ENGINE_DBG_TRACE, + ALLOC_ENGINE_V6_LEASES_RECLAMATION_COMPLETE) + .arg(leases.size()) + .arg(stopwatch.logFormatTotalDuration()); } + + template void -AllocEngine::queueNameChangeRequest(const LeasePtrType& lease, - const IdentifierType& identifier) const { +AllocEngine::queueRemovalNameChangeRequest(const LeasePtrType& lease, + const IdentifierType& identifier) const { - if (lease->hostname_.empty() || !lease->fqdn_fwd_ || !lease->fqdn_rev_) { + // Check if there is a need for update. + if (!lease || lease->hostname_.empty() || (!lease->fqdn_fwd_ && !lease->fqdn_rev_) + || !CfgMgr::instance().getD2ClientMgr().ddnsEnabled()) { return; } - if (!CfgMgr::instance().getD2ClientMgr().ddnsEnabled()) { - return; - } - - std::vector hostname_wire; try { + // Create DHCID + std::vector hostname_wire; OptionDataTypeUtil::writeFqdn(lease->hostname_, hostname_wire, true); + dhcp_ddns::D2Dhcid dhcid = D2Dhcid(identifier, hostname_wire); + + // Create name change request. + NameChangeRequestPtr ncr(new NameChangeRequest(isc::dhcp_ddns::CHG_REMOVE, + lease->fqdn_fwd_, lease->fqdn_rev_, + lease->hostname_, + lease->addr_.toText(), + dhcid, 0, lease->valid_lft_)); + // Send name change request. + CfgMgr::instance().getD2ClientMgr().sendRequest(ncr); } catch (const std::exception& ex) { - + LOG_ERROR(alloc_engine_logger, ALLOC_ENGINE_REMOVAL_NCR_FAILED) + .arg(lease->addr_.toText()) + .arg(ex.what()); } - - isc::dhcp_ddns::D2Dhcid dhcid(identifier, hostname_wire); - NameChangeRequestPtr ncr; - ncr.reset(new NameChangeRequest(isc::dhcp_ddns::CHG_REMOVE, - lease->fqdn_fwd_, lease->fqdn_rev_, - lease->hostname_, - lease->addr_.toText(), - dhcid, 0, lease->valid_lft_)); - - CfgMgr::instance().getD2ClientMgr().sendRequest(ncr); } } // end of isc::dhcp namespace diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index 0bf1db7894..c6c2756c61 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -678,9 +678,20 @@ private: /// @param lease IPv6 lease to be extended. void extendLease6(ClientContext6& ctx, Lease6Ptr lease); + /// @brief Sends removal name change reuqest to D2. + /// + /// This method is exception safe. + /// + /// @param lease Pointer to a lease for which NCR should be sent. + /// @param identifier Identifier to be used to generate DHCID for + /// the DNS update. For DHCPv4 it will be hardware address, client + /// identifier. For DHCPv6 it will be a DUID. + /// + /// @tparam LeasePtrType Pointer to a lease. + /// @tparam Identifier HW Address, Client Identifier or DUID. template - void queueNameChangeRequest(const LeasePtrType& lease, - const IdentifierType& identifier) const; + void queueRemovalNameChangeRequest(const LeasePtrType& lease, + const IdentifierType& identifier) const; public: diff --git a/src/lib/dhcpsrv/alloc_engine_messages.mes b/src/lib/dhcpsrv/alloc_engine_messages.mes index 8488c9f69a..6abf3d5d85 100644 --- a/src/lib/dhcpsrv/alloc_engine_messages.mes +++ b/src/lib/dhcpsrv/alloc_engine_messages.mes @@ -14,6 +14,14 @@ $NAMESPACE isc::dhcp +% ALLOC_ENGINE_REMOVAL_NCR_FAILED sending removal name change request failed for lease %1: %2 +This error message is logged when sending a removal name change request +to D2 failed. This name change request is usually generated when the +lease reclamation routine acts upon expired leases. If a lease being +reclaimed has a corresponding DNS entry it needs to be removed. +This message indicates that removal of the DNS entry has failed. +Nevertheless the lease will be reclaimed. + % ALLOC_ENGINE_V4_ALLOC_ERROR %1: error during attempt to allocate an IPv4 address: %2 An error occurred during an attempt to allocate an IPv4 address, the reason for the failure being contained in the message. The server will @@ -246,6 +254,30 @@ reserved for it. This informational message signals that the specified client was assigned the prefix reserved for it. +% ALLOC_ENGINE_V6_LEASE_RECLAMATION_FAILED failed to reclaim the lease %1: %2 +This error message is logged when the allocation engine fails to +reclaim an expired lease. The reason for the failure is included in the +message. The error may be triggered in the lease expiration hook or +while performing the operation on the lease database. + +% ALLOC_ENGINE_V6_LEASE_RECLAIMED successfully reclaimed IPv6 lease %1 +This debug message is logged when the allocation engine successfully +reclaims a lease. + +% ALLOC_ENGINE_V6_LEASES_RECLAMATION_COMPLETE reclaimed %1 leases in %2 +This debug message is logged when the allocation engine completes +reclamation of a set of expired leases. The maximum number of leases +to be reclaimed in a single pass of the lease reclamation routine +is configurable. However, the number of reclaimed leases may also +be limited by the timeout value. The message includes the number +of reclaimed leases and the total time. + +% ALLOC_ENGINE_V6_LEASES_RECLAMATION_START starting reclamation of expired leases (limit = %1 leases, timeout = %2 seconds) +This debug message is issued when the allocation engine starts the +reclamation of the expired leases. The maximum number of leases to +be reclaimed and the timeout is included in the message. If any of +these values is 0, it means "unlimited". + % ALLOC_ENGINE_V6_RENEW_HR allocating leases reserved for the client %1 as a result of Renew This debug message is issued when the allocation engine tries to allocate reserved leases for the client sending a Renew message. diff --git a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc index b8d8dc8636..4f1aaffa73 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc @@ -252,10 +252,15 @@ public: /// @brief Lease algorithm checking if lease state is expired-reclaimed. /// + /// This algorithm also checks that the FQDN information has been removed + /// from the lease. + /// /// @param lease Pointer to lease. /// @return true if lease state is "expired-reclaimed". static bool leaseReclaimed(const Lease6Ptr& lease) { - return (lease && lease->stateExpiredReclaimed()); + return (lease && lease->stateExpiredReclaimed() && + lease->hostname_.empty() && !lease->fqdn_fwd_ && + !lease->fqdn_rev_); } /// @brief Lease algorithm checking if lease state is not @@ -299,6 +304,11 @@ ExpirationAllocEngine6Test::~ExpirationAllocEngine6Test() { mgr.stopSender(); mgr.clearQueue(); } + // Reset configuration. This is important because the CfgMgr is + // a singleton and test configuration would affect all subsequent + // tests. + D2ClientConfigPtr cfg(new D2ClientConfig()); + mgr.setD2ClientConfig(cfg); } void @@ -636,6 +646,46 @@ TEST_F(ExpirationAllocEngine6Test, reclaimExpiredLeases6WithDDNSAndLimit) { } } +// This test verifies that if some leases have invalid hostnames, the +// lease reclamation routine continues with reclamation of leases anyway. +TEST_F(ExpirationAllocEngine6Test, reclaimExpiredLeases6InvalidHostname) { + boost::scoped_ptr engine; + ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, + 100, false))); + ASSERT_TRUE(engine); + // DNS must be started for the D2 client to accept NCRs. + ASSERT_NO_THROW(enableDDNS()); + + for (size_t i = 0; i < TEST_LEASES_NUM; ++i) { + // Generate invalid hostname for every other lease. + if (evenLeaseIndex(i)) { + // Hostname with two consecutive dots is invalid and may result + // in exception if the reclamation routine doesn't protect + // aginst such exceptions. + std::ostringstream hostname_s; + hostname_s << "invalid-host" << i << "..example.com"; + leases_[i]->hostname_ = hostname_s.str(); + ASSERT_NO_THROW(LeaseMgrFactory::instance().updateLease6(leases_[i])); + } + // Every lease is expired. + expire(i, 10 + i); + } + + // Although we know that some hostnames are broken we don't want the + // reclamation process to break when it finds a broken record. + // It should rather continue to process other leases. + ASSERT_NO_THROW(engine->reclaimExpiredLeases6(0, 0, false)); + + // All leases should have been reclaimed. Broken DNS entry doesn't + // warrant that we don't reclaim the lease. + EXPECT_TRUE(testLeases(&leaseReclaimed, &allLeaseIndexes)); + // The routine should not generate DNS updates for the leases with broken + // hostname. + EXPECT_TRUE(testLeases(&dnsUpdateNotGeneratedForLease, &evenLeaseIndex)); + // But it should generate DNS updates for the leases with the correct + // hostname. + EXPECT_TRUE(testLeases(&dnsUpdateGeneratedForLease, &oddLeaseIndex)); +} }; // end of anonymous namespace From 1f2dea1bb0260b6ad22155b5b63a5bcc82cbba19 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Wed, 9 Sep 2015 21:12:59 +0200 Subject: [PATCH 3/9] [3973] Implemented tests for DHCPv4 lease reclamation routine. --- src/lib/dhcpsrv/alloc_engine.cc | 65 ++ src/lib/dhcpsrv/alloc_engine.h | 17 +- .../tests/alloc_engine_expiration_unittest.cc | 908 +++++++++++------- 3 files changed, 628 insertions(+), 362 deletions(-) diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 85794e7bc2..c6306548d5 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -1344,6 +1344,71 @@ AllocEngine::reclaimExpiredLeases6(const size_t max_leases, const uint16_t timeo .arg(stopwatch.logFormatTotalDuration()); } +void +AllocEngine::reclaimExpiredLeases4(const size_t max_leases, const uint16_t timeout, + const bool remove_lease) { + + LOG_DEBUG(alloc_engine_logger, ALLOC_ENGINE_DBG_TRACE, + ALLOC_ENGINE_V6_LEASES_RECLAMATION_START) + .arg(max_leases) + .arg(timeout); + + // Create stopwatch and automatically start it to measure the time + // taken by the routine. + util::Stopwatch stopwatch; + + LeaseMgr& lease_mgr = LeaseMgrFactory::instance(); + + Lease4Collection leases; + lease_mgr.getExpiredLeases4(leases, max_leases); + + for (Lease4Collection::const_iterator lease_it = leases.begin(); + lease_it != leases.end(); ++lease_it) { + + try { + /// @todo execute a lease6_expire hook here. + + // Generate removal name change request for D2, if required. + // This will return immediatelly if the DNS wasn't updated + // when the lease was created. + queueRemovalNameChangeRequest(*lease_it, (*lease_it)->hwaddr_); + + // Reclaim the lease - depending on the configuration, set the + // expired-reclaimed state or simply remove it. + if (remove_lease) { + lease_mgr.deleteLease((*lease_it)->addr_); + + } else { + // Clear FQDN information as we have already sent the + // name change request to remove the DNS record. + (*lease_it)->hostname_.clear(); + (*lease_it)->fqdn_fwd_ = false; + (*lease_it)->fqdn_rev_ = false; + (*lease_it)->state_ = Lease::STATE_EXPIRED_RECLAIMED; + lease_mgr.updateLease4(*lease_it); + } + + // Lease has been reclaimed. + LOG_DEBUG(alloc_engine_logger, ALLOC_ENGINE_DBG_TRACE, + ALLOC_ENGINE_V6_LEASE_RECLAIMED) + .arg((*lease_it)->addr_.toText()); + + } catch (const std::exception& ex) { + LOG_ERROR(alloc_engine_logger, ALLOC_ENGINE_V6_LEASE_RECLAMATION_FAILED) + .arg((*lease_it)->addr_.toText()) + .arg(ex.what()); + } + } + + // Stop measuring the time. + stopwatch.stop(); + + // Mark completion of the lease reclamation routine and present some stats. + LOG_DEBUG(alloc_engine_logger, ALLOC_ENGINE_DBG_TRACE, + ALLOC_ENGINE_V6_LEASES_RECLAMATION_COMPLETE) + .arg(leases.size()) + .arg(stopwatch.logFormatTotalDuration()); +} template diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index c6c2756c61..fc8f662a2b 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -490,7 +490,7 @@ public: /// @return Returns renewed lease. Lease6Collection renewLeases6(ClientContext6& ctx); - /// @brief Reclaims expired leases. + /// @brief Reclaims expired IPv6 leases. /// /// This method retrieves a collection of expired leases and reclaims them. /// See http://kea.isc.org/wiki/LeaseExpirationDesign#LeasesReclamationRoutine @@ -505,6 +505,21 @@ public: void reclaimExpiredLeases6(const size_t max_leases, const uint16_t timeout, const bool remove_lease); + /// @brief Reclaims expired IPv4 leases. + /// + /// This method retrieves a collection of expired leases and reclaims them. + /// See http://kea.isc.org/wiki/LeaseExpirationDesign#LeasesReclamationRoutine + /// for the details. + /// + /// @param max_leases Maximum number of leases to be reclaimed. + /// @param timeout Maximum amount of time that the reclaimation routine + /// may be processing expired leases, expressed in seconds. + /// @param remove_lease A boolean value indicating if the lease should + /// be removed when it is reclaimed (if true) or it should be left in the + /// database in the "expired-reclaimed" state (if false). + void reclaimExpiredLeases4(const size_t max_leases, const uint16_t timeout, + const bool remove_lease); + /// @brief Attempts to find appropriate host reservation. /// /// Attempts to find appropriate host reservation in HostMgr. If found, it diff --git a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc index 4f1aaffa73..3227854185 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc @@ -23,6 +23,7 @@ #include #include #include +#include using namespace std; using namespace isc; @@ -83,7 +84,7 @@ struct UpperBound { size_t upper_bound_; }; -/// @brief Test fixture class for the lease reclamation routines in the +/// @brief Base test fixture class for the lease reclamation routines in the /// @c AllocEngine. /// /// This class implements infrastructure for testing leases reclamation @@ -107,7 +108,7 @@ struct UpperBound { /// check that DNS updates are not generated for them. /// /// The tests are built around the -/// @c ExpirationAllocEngine6Test::testLeases methods. These methods +/// @c ExpirationAllocEngineTest::testLeases methods. These methods /// verify that the certain operations have been performed by the /// lease reclamation routine on selected leases. The leases for which /// certain conditions should be met are selected using the "index @@ -140,33 +141,55 @@ struct UpperBound { /// after lease reclamation leases with even indexes have state set to /// "expired-reclaimed". /// -/// See @c ExpirationAllocEngine6Test::testLeases for further details. -class ExpirationAllocEngine6Test : public AllocEngine6Test { +/// See @c ExpirationAllocEngineTest::testLeases for further details. +template +class ExpirationAllocEngineTest : public ::testing::Test { public: /// @brief Type definition for the lease algorithm. - typedef boost::function LeaseAlgorithmFun; + typedef boost::function LeaseAlgorithmFun; /// @brief type definition for the lease index algorithm. typedef boost::function IndexAlgorithmFun; - /// @brief Class constructor. + /// @brief Constructor. /// - /// This constructor initializes @c TEST_LEASES_NUM leases and - /// stores them in the lease manager. - ExpirationAllocEngine6Test(); + /// Clears configuration, creates new lease manager and allocation engine + /// instances. + ExpirationAllocEngineTest(const std::string& lease_mgr_params) { + CfgMgr::instance().clear(); + LeaseMgrFactory::create(lease_mgr_params); - /// @brief Class destructor. - /// - /// It stops the D2 client, if it is running. - virtual ~ExpirationAllocEngine6Test(); + engine_.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, + 100, false)); - /// @brief Creates collection of leases for a test. + } + + /// @brief Destructor /// - /// It is called internally at the construction time. - void createLeases(); + /// Stops D2 client (if running), clears configuration and removes + /// an instance of the lease manager. + virtual ~ExpirationAllocEngineTest() { + // Stop D2 client if running and remove all queued name change + // requests. + D2ClientMgr& mgr = CfgMgr::instance().getD2ClientMgr(); + if (mgr.amSending()) { + mgr.stopSender(); + mgr.clearQueue(); + } + + CfgMgr::instance().clear(); + LeaseMgrFactory::destroy(); + } /// @brief Starts D2 client. - void enableDDNS() const; + void enableDDNS() const { + // Start DDNS and assign no-op error handler. + D2ClientMgr& mgr = CfgMgr::instance().getD2ClientMgr(); + D2ClientConfigPtr cfg(new D2ClientConfig()); + cfg->enableUpdates(true); + mgr.setD2ClientConfig(cfg); + mgr.startSender(boost::bind(&ExpirationAllocEngineTest::d2ErrorHandler, _1, _2)); + } /// @brief No-op error handler for the D2 client. static void d2ErrorHandler(const dhcp_ddns::NameChangeSender::Result, @@ -179,7 +202,35 @@ public: /// @c TEST_LEASES_NUM. /// @param secs Offset of the expiration time since now. For example /// a value of 2 would set the lease expiration time to 2 seconds ago. - void expire(const unsigned int lease_index, const time_t secs); + void expire(const unsigned int lease_index, const time_t secs) { + ASSERT_GT(leases_.size(), lease_index); + // We set the expiration time indirectly by modifying the cltt value. + leases_[lease_index]->cltt_ = time(NULL) - secs - + leases_[lease_index]->valid_lft_; + ASSERT_NO_THROW(updateLease(lease_index)); + } + + /// @brief Updates lease in the lease database. + /// + /// @param lease_index Index of the lease. + virtual void updateLease(const unsigned int lease_index) = 0; + + /// @brief Retrieves lease from the database. + /// + /// @param lease_index Index of the lease. + virtual LeasePtrType getLease(const unsigned int lease_index) const = 0; + + /// @brief Wrapper method running lease reclamation routine. + /// + /// @param max_leases Maximum number of leases to be reclaimed. + /// @param timeout Maximum amount of time that the reclaimation routine + /// may be processing expired leases, expressed in seconds. + /// @param remove_lease A boolean value indicating if the lease should + /// be removed when it is reclaimed (if true) or it should be left in the + /// database in the "expired-reclaimed" state (if false). + virtual void reclaimExpiredLeases(const size_t max_leases, + const uint16_t timeout, + const bool remove_lease) = 0; /// @brief Test selected leases using the specified algorithms. /// @@ -191,7 +242,12 @@ public: /// @param lease_algorithm Pointer to the lease algorithm function. /// @param index_algorithm Pointer to the index algorithm function. bool testLeases(const LeaseAlgorithmFun& lease_algorithm, - const IndexAlgorithmFun& index_algorithm) const; + const IndexAlgorithmFun& index_algorithm) const { + // No limits are specified, so test all leases in the range of + // 0 .. TEST_LEASES_NUM. + return (testLeases(lease_algorithm, index_algorithm, LowerBound(0), + UpperBound(TEST_LEASES_NUM))); + } /// @brief Test selected leases using the specified algorithms. @@ -208,7 +264,22 @@ public: bool testLeases(const LeaseAlgorithmFun& lease_algorithm, const IndexAlgorithmFun& index_algorithm, const LowerBound& lower_bound, - const UpperBound& upper_bound) const; + const UpperBound& upper_bound) const { + // Select leases between the lower_bound and upper_bound. + for (size_t i = lower_bound; i < upper_bound; ++i) { + // Get the lease from the lease database. + LeasePtrType lease = getLease(i); + // index_algorithm(i) checks if the lease should be checked. + // If so, check if the lease_algorithm indicates that the + // lease fulfils a given condition, e.g. is present in the + // database. If not, return false. + if (index_algorithm(i) && !lease_algorithm(lease)) { + return (false); + } + } + // All leases checked, so return true. + return (true); + } /// @brief Index algorithm selecting even indexes. /// @@ -238,7 +309,7 @@ public: /// /// @param lease Pointer to lease. /// @return true if lease pointer is non-null. - static bool leaseExists(const Lease6Ptr& lease) { + static bool leaseExists(const LeasePtrType& lease) { return (static_cast(lease)); } @@ -246,7 +317,7 @@ public: /// /// @param lease Pointer to lease. /// @return true if lease pointer is null. - static bool leaseDoesntExist(const Lease6Ptr& lease) { + static bool leaseDoesntExist(const LeasePtrType& lease) { return (static_cast(!lease)); } @@ -257,7 +328,7 @@ public: /// /// @param lease Pointer to lease. /// @return true if lease state is "expired-reclaimed". - static bool leaseReclaimed(const Lease6Ptr& lease) { + static bool leaseReclaimed(const LeasePtrType& lease) { return (lease && lease->stateExpiredReclaimed() && lease->hostname_.empty() && !lease->fqdn_fwd_ && !lease->fqdn_rev_); @@ -268,7 +339,7 @@ public: /// /// @param lease Pointer to lease. /// @return true if lease state is not "expired-reclaimed". - static bool leaseNotReclaimed(const Lease6Ptr& lease) { + static bool leaseNotReclaimed(const LeasePtrType& lease) { return (lease && !lease->stateExpiredReclaimed()); } @@ -277,40 +348,344 @@ public: /// /// @param lease Pointer to lease. /// @return true if NCR has been generated for the lease. - static bool dnsUpdateGeneratedForLease(const Lease6Ptr& lease); + static bool dnsUpdateGeneratedForLease(const LeasePtrType& lease) { + try { + // Iterate over the generated name change requests and try + // to find the match with our lease (using IP address). If + D2ClientMgr& mgr = CfgMgr::instance().getD2ClientMgr(); + for (size_t i = 0; i < mgr.getQueueSize(); ++i) { + const NameChangeRequestPtr& ncr = mgr.peekAt(i); + // If match found, return true. + if (ncr->getIpAddress() == lease->addr_.toText()) { + return (true); + } + } + } catch (...) { + // If error occurred, treat it as no match. + return (false); + } + + // All leases checked - no match. + return (false); + + } /// @brief Lease algorithm checking if removal name change request /// hasn't been generated for lease. /// /// @param lease Pointer to lease. /// @return true if NCR has not been generated for the lease. - static bool dnsUpdateNotGeneratedForLease(const Lease6Ptr& lease); + static bool dnsUpdateNotGeneratedForLease(const LeasePtrType& lease) { + try { + // Iterate over the generated name change requests and try + // to find the match with our lease (using IP address). If + D2ClientMgr& mgr = CfgMgr::instance().getD2ClientMgr(); + for (size_t i = 0; i < mgr.getQueueSize(); ++i) { + const NameChangeRequestPtr& ncr = mgr.peekAt(i); + // If match found, we treat it as if the test fails + // because we expected no NCR. + if (ncr->getIpAddress() == lease->addr_.toText()) { + return (false); + } + } + } catch (...) { + return (false); + } + + // No match found, so we're good. + return (true); + } + + /// @brief Test that leases can be reclaimed without being removed. + void testReclaimExpiredLeasesUpdateState() { + for (int i = 0; i < TEST_LEASES_NUM; ++i) { + // Mark leases with even indexes as expired. + if (evenLeaseIndex(i)) { + // The higher the index, the more expired the lease. + expire(i, 10 + i); + } + } + + // Run leases reclamation routine on all leases. This should result + // in setting "expired-reclaimed" state for all leases with even + // indexes. + ASSERT_NO_THROW(reclaimExpiredLeases(0, 0, false)); + + // Leases with even indexes should be marked as reclaimed. + EXPECT_TRUE(testLeases(&leaseReclaimed, &evenLeaseIndex)); + // Leases with odd indexes shouldn't be marked as reclaimed. + EXPECT_TRUE(testLeases(&leaseNotReclaimed, &oddLeaseIndex)); + } + + /// @brief Test that the leases may be reclaimed by being deleted. + void testReclaimExpiredLeasesDelete() { + for (int i = 0; i < TEST_LEASES_NUM; ++i) { + // Mark leases with even indexes as expired. + if (evenLeaseIndex(i)) { + // The higher the index, the more expired the lease. + expire(i, 10 + i); + } + } + + // Run leases reclamation routine on all leases. This should result + // in removal of all leases with even indexes. + ASSERT_NO_THROW(reclaimExpiredLeases(0, 0, true)); + + // Leases with odd indexes should be retained and their state + // shouldn't be "expired-reclaimed". + EXPECT_TRUE(testLeases(&leaseNotReclaimed, &oddLeaseIndex)); + // Leases with even indexes should have been removed. + EXPECT_TRUE(testLeases(&leaseDoesntExist, &evenLeaseIndex)); + } + + /// @brief Test that it is possible to specify the limit for the number + /// of reclaimed leases. + void testReclaimExpiredLeasesLimit() { + for (int i = 0; i < TEST_LEASES_NUM; ++i) { + // Mark all leaes as expired. The higher the index the less + // expired the lease. + expire(i, 1000 - i); + } + + // We will be performing lease reclamation on lease groups of 10. + // Hence, it is convenient if the number of test leases is a + // multiple of 10. + const size_t reclamation_group_size = 10; + BOOST_STATIC_ASSERT(TEST_LEASES_NUM % reclamation_group_size == 0); + + // Leases will be reclaimed in groups of 10. + for (int i = reclamation_group_size; i < TEST_LEASES_NUM; + i += reclamation_group_size) { + + // Reclaim 10 most expired leases out of TEST_LEASES_NUM. Since + // leases are ordered from the most expired to the least expired + // this should reclaim leases between 0 and 9, then 10 and 19 etc. + ASSERT_NO_THROW(reclaimExpiredLeases(reclamation_group_size, + 0, false)); + + // Check that leases having all indexes between 0 and 9, 19, 29 etc. + // have been reclaimed. + EXPECT_TRUE(testLeases(&leaseReclaimed, &allLeaseIndexes, + LowerBound(0), UpperBound(i))) + << "check failed for i = " << i; + + // Check that all remaining leases haven't been reclaimed. + EXPECT_TRUE(testLeases(&leaseNotReclaimed, &allLeaseIndexes, + LowerBound(i), UpperBound(TEST_LEASES_NUM))) + << "check failed for i = " << i; + } + } + + /// @brief Test that DNS updates are generated for the leases for which + /// the DNS records exist. + void testReclaimExpiredLeasesWithDDNS() { + // DNS must be started for the D2 client to accept NCRs. + ASSERT_NO_THROW(enableDDNS()); + + for (int i = 0; i < TEST_LEASES_NUM; ++i) { + // Expire all leases with even indexes. + if (evenLeaseIndex(i)) { + // The higher the index, the more expired the lease. + expire(i, 10 + i); + } + } + + // Reclaim all expired leases. + ASSERT_NO_THROW(reclaimExpiredLeases(0, 0, false)); + + // Leases with odd indexes shouldn't be reclaimed. + EXPECT_TRUE(testLeases(&leaseNotReclaimed, &oddLeaseIndex)); + // Leases with even indexes should be reclaimed. + EXPECT_TRUE(testLeases(&leaseReclaimed, &evenLeaseIndex)); + // DNS updates (removal NCRs) should be generated for leases with even + // indexes. + EXPECT_TRUE(testLeases(&dnsUpdateGeneratedForLease, &evenLeaseIndex)); + // DNS updates (removal NCRs) shouldn't be generated for leases with + // odd indexes. + EXPECT_TRUE(testLeases(&dnsUpdateNotGeneratedForLease, &oddLeaseIndex)); + } + + /// @brief Test that DNS updates are only generated for the reclaimed + /// leases (not for all leases with hostname stored). + void testReclaimExpiredLeasesWithDDNSAndLimit() { + // DNS must be started for the D2 client to accept NCRs. + ASSERT_NO_THROW(enableDDNS()); + + for (int i = 0; i < TEST_LEASES_NUM; ++i) { + // Expire only leases with even indexes. + if (evenLeaseIndex(i)) { + // The higher the index, the more expired the lease. + expire(i, 10 + i); + } + } + + const size_t reclamation_group_size = 10; + BOOST_STATIC_ASSERT(TEST_LEASES_NUM % reclamation_group_size == 0); + + // Leases will be reclaimed in groups of 10 + for (int i = 10; i < TEST_LEASES_NUM; i += reclamation_group_size) { + // Reclaim 10 most expired leases. Note that the leases with the + // higher index are more expired. For example, if the + // TEST_LEASES_NUM is equal to 100, the most expired lease will + // be 98, then 96, 94 etc. + ASSERT_NO_THROW(reclaimExpiredLeases(reclamation_group_size, 0, + false)); + + // After the first iteration the lower bound is 80, because there + // will be 10 the most expired leases in this group: 80, 82, 84, + // 86, 88, 90, 92, 94, 96, 98. For subsequent iterations + // accordingly. + int reclaimed_lower_bound = TEST_LEASES_NUM - 2 * i; + // At some point the lower bound will hit the negative value, which + // must be corrected to 0. + if (reclaimed_lower_bound < 0) { + reclaimed_lower_bound = 0; + } + + // Leases between the lower bound calculated above and the upper + // bound of all leases, and having even indexes should have been + // reclaimed. + EXPECT_TRUE(testLeases(&leaseReclaimed, &evenLeaseIndex, + LowerBound(reclaimed_lower_bound), + UpperBound(TEST_LEASES_NUM))) + << "check failed for i = " << i; + + // For the same leases we should have generated DNS updates + // (removal NCRs). + EXPECT_TRUE(testLeases(&dnsUpdateGeneratedForLease, &evenLeaseIndex, + LowerBound(reclaimed_lower_bound), + UpperBound(TEST_LEASES_NUM))) + << "check failed for i = " << i; + + // Leases with odd indexes (falling between the reclaimed ones) + // shouldn't have been reclaimed, because they are not expired. + EXPECT_TRUE(testLeases(&leaseNotReclaimed, &oddLeaseIndex, + LowerBound(reclaimed_lower_bound), + UpperBound(TEST_LEASES_NUM))) + << "check failed for i = " << i; + + EXPECT_TRUE(testLeases(&dnsUpdateNotGeneratedForLease, + &oddLeaseIndex, + LowerBound(reclaimed_lower_bound), + UpperBound(TEST_LEASES_NUM))) + << "check failed for i = " << i; + + + // At early stages of iterations, there should be conitnuous + // group of leases (expired and not expired) which haven't been + // reclaimed. + if (reclaimed_lower_bound > 0) { + EXPECT_TRUE(testLeases(&leaseNotReclaimed, &allLeaseIndexes, + LowerBound(0), + UpperBound(reclaimed_lower_bound))) + << "check failed for i = " << i; + + EXPECT_TRUE(testLeases(&dnsUpdateNotGeneratedForLease, + &oddLeaseIndex, + LowerBound(0), + UpperBound(reclaimed_lower_bound))); + } + } + } + + /// @brief This test verifies that reclamation routine continues if the + /// DNS update has failed for some leases. + void testReclaimExpiredLeasesInvalidHostname() { + // DNS must be started for the D2 client to accept NCRs. + ASSERT_NO_THROW(enableDDNS()); + + for (size_t i = 0; i < TEST_LEASES_NUM; ++i) { + // Generate invalid hostname for every other lease. + if (evenLeaseIndex(i)) { + // Hostname with two consecutive dots is invalid and may result + // in exception if the reclamation routine doesn't protect + // aginst such exceptions. + std::ostringstream hostname_s; + hostname_s << "invalid-host" << i << "..example.com"; + leases_[i]->hostname_ = hostname_s.str(); + ASSERT_NO_THROW(LeaseMgrFactory::instance().updateLease6(leases_[i])); + } + // Every lease is expired. + expire(i, 10 + i); + } + + // Although we know that some hostnames are broken we don't want the + // reclamation process to break when it finds a broken record. + // It should rather continue to process other leases. + ASSERT_NO_THROW(reclaimExpiredLeases(0, 0, false)); + + // All leases should have been reclaimed. Broken DNS entry doesn't + // warrant that we don't reclaim the lease. + EXPECT_TRUE(testLeases(&leaseReclaimed, &allLeaseIndexes)); + // The routine should not generate DNS updates for the leases with + // broken hostname. + EXPECT_TRUE(testLeases(&dnsUpdateNotGeneratedForLease, + &evenLeaseIndex)); + // But it should generate DNS updates for the leases with the correct + // hostname. + EXPECT_TRUE(testLeases(&dnsUpdateGeneratedForLease, &oddLeaseIndex)); + } /// @brief Collection of leases created at construction time. - Lease6Collection leases_; + std::vector leases_; + + /// @brief Allocation engine instance used for tests. + AllocEnginePtr engine_; + +}; + +/// @brief Specialization of the @c ExpirationAllocEngineTest class to test +/// reclamation of the IPv6 leases. +class ExpirationAllocEngine6Test : public ExpirationAllocEngineTest { +public: + + /// @brief Class constructor. + /// + /// This constructor initializes @c TEST_LEASES_NUM leases and + /// stores them in the lease manager. + ExpirationAllocEngine6Test(); + + /// @brief Creates collection of leases for a test. + /// + /// It is called internally at the construction time. + void createLeases(); + + /// @brief Updates lease in the lease database. + /// + /// @param lease_index Index of the lease. + virtual void updateLease(const unsigned int lease_index) { + LeaseMgrFactory::instance().updateLease6(leases_[lease_index]); + } + + /// @brief Retrieves lease from the database. + /// + /// @param lease_index Index of the lease. + virtual Lease6Ptr getLease(const unsigned int lease_index) const { + return (LeaseMgrFactory::instance().getLease6(leases_[lease_index]->type_, + leases_[lease_index]->addr_)); + } + + /// @brief Wrapper method running lease reclamation routine. + /// + /// @param max_leases Maximum number of leases to be reclaimed. + /// @param timeout Maximum amount of time that the reclaimation routine + /// may be processing expired leases, expressed in seconds. + /// @param remove_lease A boolean value indicating if the lease should + /// be removed when it is reclaimed (if true) or it should be left in the + /// database in the "expired-reclaimed" state (if false). + virtual void reclaimExpiredLeases(const size_t max_leases, + const uint16_t timeout, + const bool remove_lease) { + engine_->reclaimExpiredLeases6(max_leases, timeout, remove_lease); + } }; ExpirationAllocEngine6Test::ExpirationAllocEngine6Test() - : AllocEngine6Test() { + : ExpirationAllocEngineTest("type=memfile universe=6 persist=false") { createLeases(); } -ExpirationAllocEngine6Test::~ExpirationAllocEngine6Test() { - // Stop D2 client if running and remove all queued name change - // requests. - D2ClientMgr& mgr = CfgMgr::instance().getD2ClientMgr(); - if (mgr.amSending()) { - mgr.stopSender(); - mgr.clearQueue(); - } - // Reset configuration. This is important because the CfgMgr is - // a singleton and test configuration would affect all subsequent - // tests. - D2ClientConfigPtr cfg(new D2ClientConfig()); - mgr.setD2ClientConfig(cfg); -} - void ExpirationAllocEngine6Test::createLeases() { // Create TEST_LEASES_NUM leases. @@ -338,354 +713,165 @@ ExpirationAllocEngine6Test::createLeases() { } } -void -ExpirationAllocEngine6Test::enableDDNS() const { - // Start DDNS and assign no-op error handler. - D2ClientMgr& mgr = CfgMgr::instance().getD2ClientMgr(); - D2ClientConfigPtr cfg(new D2ClientConfig()); - cfg->enableUpdates(true); - mgr.setD2ClientConfig(cfg); - mgr.startSender(boost::bind(&ExpirationAllocEngine6Test::d2ErrorHandler, _1, _2)); -} - -void -ExpirationAllocEngine6Test::expire(const unsigned int lease_index, - const time_t secs) { - ASSERT_GT(leases_.size(), lease_index); - // We set the expiration time indirectly by modifying the cltt value. - leases_[lease_index]->cltt_ = time(NULL) - secs - leases_[lease_index]->valid_lft_; - ASSERT_NO_THROW(LeaseMgrFactory::instance().updateLease6(leases_[lease_index])); -} - -bool -ExpirationAllocEngine6Test::testLeases(const LeaseAlgorithmFun& lease_algorithm, - const IndexAlgorithmFun& index_algorithm) const { - // No limits are specified, so test all leases in the range of - // 0 .. TEST_LEASES_NUM. - return (testLeases(lease_algorithm, index_algorithm, LowerBound(0), - UpperBound(TEST_LEASES_NUM))); -} - -bool -ExpirationAllocEngine6Test::testLeases(const LeaseAlgorithmFun& lease_algorithm, - const IndexAlgorithmFun& index_algorithm, - const LowerBound& lower_bound, - const UpperBound& upper_bound) const { - // Select leases between the lower_bound and upper_bound. - for (size_t i = lower_bound; i < upper_bound; ++i) { - // Get the lease from the lease database. - Lease6Ptr lease = LeaseMgrFactory::instance().getLease6(leases_[i]->type_, - leases_[i]->addr_); - // index_algorithm(i) checks if the lease should be checked. - // If so, check if the lease_algorithm indicates that the - // lease fulfils a given condition, e.g. is present in the - // database. If not, return false. - if (index_algorithm(i) && !lease_algorithm(lease)) { - return (false); - } - } - // All leases checked, so return true. - return (true); -} - -bool -ExpirationAllocEngine6Test::dnsUpdateGeneratedForLease(const Lease6Ptr& lease) { - try { - // Iterate over the generated name change requests and try - // to find the match with our lease (using IP address). If - D2ClientMgr& mgr = CfgMgr::instance().getD2ClientMgr(); - for (size_t i = 0; i < mgr.getQueueSize(); ++i) { - const NameChangeRequestPtr& ncr = mgr.peekAt(i); - // If match found, return true. - if (ncr->getIpAddress() == lease->addr_.toText()) { - return (true); - } - } - } catch (...) { - // If error occurred, treat it as no match. - return (false); - } - - // All leases checked - no match. - return (false); -} - -bool -ExpirationAllocEngine6Test::dnsUpdateNotGeneratedForLease(const Lease6Ptr& lease) { - try { - // Iterate over the generated name change requests and try - // to find the match with our lease (using IP address). If - D2ClientMgr& mgr = CfgMgr::instance().getD2ClientMgr(); - for (size_t i = 0; i < mgr.getQueueSize(); ++i) { - const NameChangeRequestPtr& ncr = mgr.peekAt(i); - // If match found, we treat it as if the test fails - // because we expected no NCR. - if (ncr->getIpAddress() == lease->addr_.toText()) { - return (false); - } - } - } catch (...) { - return (false); - } - - // No match found, so we're good. - return (true); -} // This test verifies that the leases can be reclaimed without being removed // from the database. In such case, the leases' state is set to // "expired-reclaimed". TEST_F(ExpirationAllocEngine6Test, reclaimExpiredLeases6UpdateState) { - boost::scoped_ptr engine; - ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, - 100, false))); - ASSERT_TRUE(engine); - - for (int i = 0; i < TEST_LEASES_NUM; ++i) { - // Mark leases with even indexes as expired. - if (evenLeaseIndex(i)) { - // The higher the index, the more expired the lease. - expire(i, 10 + i); - } - } - - // Run leases reclamation routine on all leases. This should result - // in setting "expired-reclaimed" state for all leases with even - // indexes. - ASSERT_NO_THROW(engine->reclaimExpiredLeases6(0, 0, false)); - - // Leases with even indexes should be marked as reclaimed. - EXPECT_TRUE(testLeases(&leaseReclaimed, &evenLeaseIndex)); - // Leases with odd indexes shouldn't be marked as reclaimed. - EXPECT_TRUE(testLeases(&leaseNotReclaimed, &oddLeaseIndex)); - + testReclaimExpiredLeasesUpdateState(); } // This test verifies that the reclaimed leases are deleted when requested. -TEST_F(ExpirationAllocEngine6Test, reclaimExpiredLeases6Delete) { - boost::scoped_ptr engine; - ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, - 100, false))); - ASSERT_TRUE(engine); - - for (int i = 0; i < TEST_LEASES_NUM; ++i) { - // Mark leases with even indexes as expired. - if (evenLeaseIndex(i)) { - // The higher the index, the more expired the lease. - expire(i, 10 + i); - } - } - - // Run leases reclamation routine on all leases. This should result - // in removal of all leases with even indexes. - ASSERT_NO_THROW(engine->reclaimExpiredLeases6(0, 0, true)); - - // Leases with odd indexes should be retained and their state - // shouldn't be "expired-reclaimed". - EXPECT_TRUE(testLeases(&leaseNotReclaimed, &oddLeaseIndex)); - // Leases with even indexes should have been removed. - EXPECT_TRUE(testLeases(&leaseDoesntExist, &evenLeaseIndex)); +TEST_F(ExpirationAllocEngine6Test, reclaimExpiredLeasesDelete) { + testReclaimExpiredLeasesDelete(); } // This test verifies that it is possible to specify the limit for the // number of reclaimed leases. -TEST_F(ExpirationAllocEngine6Test, reclaimExpiredLeases6Limit) { - boost::scoped_ptr engine; - ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, - 100, false))); - ASSERT_TRUE(engine); - - for (int i = 0; i < TEST_LEASES_NUM; ++i) { - // Mark all leaes as expired. The higher the index the less - // expired the lease. - expire(i, 1000 - i); - } - - // We will be performing lease reclamation on lease groups of 10. - // Hence, it is convenient if the number of test leases is a - // multiple of 10. - const size_t reclamation_group_size = 10; - BOOST_STATIC_ASSERT(TEST_LEASES_NUM % reclamation_group_size == 0); - - // Leases will be reclaimed in groups of 10. - for (int i = reclamation_group_size; i < TEST_LEASES_NUM; - i += reclamation_group_size) { - - // Reclaim 10 most expired leases out of TEST_LEASES_NUM. Since - // leases are ordered from the most expired to the least expired - // this should reclaim leases between 0 and 9, then 10 and 19 etc. - ASSERT_NO_THROW(engine->reclaimExpiredLeases6(reclamation_group_size, 0, - false)); - - // Check that leases having all indexes between 0 and 9, 19, 29 etc. - // have been reclaimed. - EXPECT_TRUE(testLeases(&leaseReclaimed, &allLeaseIndexes, - LowerBound(0), UpperBound(i))) - << "check failed for i = " << i; - - // Check that all remaining leases haven't been reclaimed. - EXPECT_TRUE(testLeases(&leaseNotReclaimed, &allLeaseIndexes, - LowerBound(i), UpperBound(TEST_LEASES_NUM))) - << "check failed for i = " << i; - } +TEST_F(ExpirationAllocEngine6Test, reclaimExpiredLeasesLimit) { + testReclaimExpiredLeasesLimit(); } // This test verifies that DNS updates are generated for the leases // for which the DNS records exist. -TEST_F(ExpirationAllocEngine6Test, reclaimExpiredLeases6WithDDNS) { - boost::scoped_ptr engine; - ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, - 100, false))); - ASSERT_TRUE(engine); - - // DNS must be started for the D2 client to accept NCRs. - ASSERT_NO_THROW(enableDDNS()); - - for (int i = 0; i < TEST_LEASES_NUM; ++i) { - // Expire all leases with even indexes. - if (evenLeaseIndex(i)) { - // The higher the index, the more expired the lease. - expire(i, 10 + i); - } - } - - // Reclaim all expired leases. - ASSERT_NO_THROW(engine->reclaimExpiredLeases6(0, 0, false)); - - // Leases with odd indexes shouldn't be reclaimed. - EXPECT_TRUE(testLeases(&leaseNotReclaimed, &oddLeaseIndex)); - // Leases with even indexes should be reclaimed. - EXPECT_TRUE(testLeases(&leaseReclaimed, &evenLeaseIndex)); - // DNS updates (removal NCRs) should be generated for leases with even - // indexes. - EXPECT_TRUE(testLeases(&dnsUpdateGeneratedForLease, &evenLeaseIndex)); - // DNS updates (removal NCRs) shouldn't be generated for leases with - // odd indexes. - EXPECT_TRUE(testLeases(&dnsUpdateNotGeneratedForLease, &oddLeaseIndex)); +TEST_F(ExpirationAllocEngine6Test, reclaimExpiredLeasesWithDDNS) { + testReclaimExpiredLeasesWithDDNS(); } // This test verifies that it is DNS updates are generated only for the // reclaimed expired leases. In this case we limit the number of leases // reclaimed during a single call to reclamation routine. -TEST_F(ExpirationAllocEngine6Test, reclaimExpiredLeases6WithDDNSAndLimit) { - boost::scoped_ptr engine; - ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, - 100, false))); - ASSERT_TRUE(engine); - - // DNS must be started for the D2 client to accept NCRs. - ASSERT_NO_THROW(enableDDNS()); - - for (int i = 0; i < TEST_LEASES_NUM; ++i) { - // Expire only leases with even indexes. - if (evenLeaseIndex(i)) { - // The higher the index, the more expired the lease. - expire(i, 10 + i); - } - } - - const size_t reclamation_group_size = 10; - BOOST_STATIC_ASSERT(TEST_LEASES_NUM % reclamation_group_size == 0); - - // Leases will be reclaimed in groups of 10 - for (int i = 10; i < TEST_LEASES_NUM; i += reclamation_group_size) { - // Reclaim 10 most expired leases. Note that the leases with the higher - // index are more expired. For example, if the TEST_LEASES_NUM is equal - // to 100, the most expired lease will be 98, then 96, 94 etc. - ASSERT_NO_THROW(engine->reclaimExpiredLeases6(reclamation_group_size, 0, - false)); - - // After the first iteration the lower bound is 80, because there will - // be 10 the most expired leases in this group: 80, 82, 84, 86, 88, 90, - // 92, 94, 96, 98. For subsequent iterations accordingly. - int reclaimed_lower_bound = TEST_LEASES_NUM - 2 * i; - // At some point the lower bound will hit the negative value, which - // must be corrected to 0. - if (reclaimed_lower_bound < 0) { - reclaimed_lower_bound = 0; - } - - // Leases between the lower bound calculated above and the upper bound - // of all leases, and having even indexes should have been reclaimed. - EXPECT_TRUE(testLeases(&leaseReclaimed, &evenLeaseIndex, - LowerBound(reclaimed_lower_bound), - UpperBound(TEST_LEASES_NUM))) - << "check failed for i = " << i; - - // For the same leases we should have generated DNS updates (removal NCRs). - EXPECT_TRUE(testLeases(&dnsUpdateGeneratedForLease, &evenLeaseIndex, - LowerBound(reclaimed_lower_bound), - UpperBound(TEST_LEASES_NUM))) - << "check failed for i = " << i; - - // Leases with odd indexes (falling between the reclaimed ones) shouldn't - // have been reclaimed, because they are not expired. - EXPECT_TRUE(testLeases(&leaseNotReclaimed, &oddLeaseIndex, - LowerBound(reclaimed_lower_bound), - UpperBound(TEST_LEASES_NUM))) - << "check failed for i = " << i; - - EXPECT_TRUE(testLeases(&dnsUpdateNotGeneratedForLease, &oddLeaseIndex, - LowerBound(reclaimed_lower_bound), - UpperBound(TEST_LEASES_NUM))) - << "check failed for i = " << i; - - - // At early stages of iterations, there should be conitnuous group of leases - // (expired and not expired) which haven't been reclaimed. - if (reclaimed_lower_bound > 0) { - EXPECT_TRUE(testLeases(&leaseNotReclaimed, &allLeaseIndexes, - LowerBound(0), - UpperBound(reclaimed_lower_bound))) - << "check failed for i = " << i; - - EXPECT_TRUE(testLeases(&dnsUpdateNotGeneratedForLease, &oddLeaseIndex, - LowerBound(0), UpperBound(reclaimed_lower_bound))); - - } - } +TEST_F(ExpirationAllocEngine6Test, reclaimExpiredLeasesWithDDNSAndLimit) { + testReclaimExpiredLeasesWithDDNSAndLimit(); } // This test verifies that if some leases have invalid hostnames, the // lease reclamation routine continues with reclamation of leases anyway. -TEST_F(ExpirationAllocEngine6Test, reclaimExpiredLeases6InvalidHostname) { - boost::scoped_ptr engine; - ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, - 100, false))); - ASSERT_TRUE(engine); +TEST_F(ExpirationAllocEngine6Test, reclaimExpiredLeasesInvalidHostname) { + testReclaimExpiredLeasesInvalidHostname(); +} - // DNS must be started for the D2 client to accept NCRs. - ASSERT_NO_THROW(enableDDNS()); +// ******************************************************* +// +// DHCPv4 lease reclamation routine tests start here! +// +// ******************************************************* - for (size_t i = 0; i < TEST_LEASES_NUM; ++i) { - // Generate invalid hostname for every other lease. - if (evenLeaseIndex(i)) { - // Hostname with two consecutive dots is invalid and may result - // in exception if the reclamation routine doesn't protect - // aginst such exceptions. - std::ostringstream hostname_s; - hostname_s << "invalid-host" << i << "..example.com"; - leases_[i]->hostname_ = hostname_s.str(); - ASSERT_NO_THROW(LeaseMgrFactory::instance().updateLease6(leases_[i])); - } - // Every lease is expired. - expire(i, 10 + i); +/// @brief Specialization of the @c ExpirationAllocEngineTest class to test +/// reclamation of the IPv4 leases. +class ExpirationAllocEngine4Test : public ExpirationAllocEngineTest { +public: + + /// @brief Class constructor. + /// + /// This constructor initializes @c TEST_LEASES_NUM leases and + /// stores them in the lease manager. + ExpirationAllocEngine4Test(); + + /// @brief Creates collection of leases for a test. + /// + /// It is called internally at the construction time. + void createLeases(); + + /// @brief Updates lease in the lease database. + /// + /// @param lease_index Index of the lease. + virtual void updateLease(const unsigned int lease_index) { + LeaseMgrFactory::instance().updateLease4(leases_[lease_index]); } - // Although we know that some hostnames are broken we don't want the - // reclamation process to break when it finds a broken record. - // It should rather continue to process other leases. - ASSERT_NO_THROW(engine->reclaimExpiredLeases6(0, 0, false)); + /// @brief Retrieves lease from the database. + /// + /// @param lease_index Index of the lease. + virtual Lease4Ptr getLease(const unsigned int lease_index) const { + return (LeaseMgrFactory::instance().getLease4(leases_[lease_index]->addr_)); + } - // All leases should have been reclaimed. Broken DNS entry doesn't - // warrant that we don't reclaim the lease. - EXPECT_TRUE(testLeases(&leaseReclaimed, &allLeaseIndexes)); - // The routine should not generate DNS updates for the leases with broken - // hostname. - EXPECT_TRUE(testLeases(&dnsUpdateNotGeneratedForLease, &evenLeaseIndex)); - // But it should generate DNS updates for the leases with the correct - // hostname. - EXPECT_TRUE(testLeases(&dnsUpdateGeneratedForLease, &oddLeaseIndex)); + /// @brief Wrapper method running lease reclamation routine. + /// + /// @param max_leases Maximum number of leases to be reclaimed. + /// @param timeout Maximum amount of time that the reclaimation routine + /// may be processing expired leases, expressed in seconds. + /// @param remove_lease A boolean value indicating if the lease should + /// be removed when it is reclaimed (if true) or it should be left in the + /// database in the "expired-reclaimed" state (if false). + virtual void reclaimExpiredLeases(const size_t max_leases, + const uint16_t timeout, + const bool remove_lease) { + engine_->reclaimExpiredLeases4(max_leases, timeout, remove_lease); + } +}; + +ExpirationAllocEngine4Test::ExpirationAllocEngine4Test() + : ExpirationAllocEngineTest("type=memfile universe=4 persist=false") { + createLeases(); +} + +void +ExpirationAllocEngine4Test::createLeases() { + // Create TEST_LEASES_NUM leases. + for (uint32_t i = 0; i < TEST_LEASES_NUM; ++i) { + // HW address + std::ostringstream hwaddr_s; + hwaddr_s << "01:02:03:04:" << std::setw(2) << std::setfill('0') + << (i >> 16) << ":" << std::setw(2) << std::setfill('0') + << (i & 0x00FF); + HWAddrPtr hwaddr(new HWAddr(HWAddr::fromText(hwaddr_s.str(), HTYPE_ETHER))); + + // Address. + std::ostringstream address_s; + address_s << "10.0." << (i >> 16) << "." << (i & 0x00FF); + IOAddress address(address_s.str()); + + // Hostname. + std::ostringstream hostname_s; + hostname_s << "host" << std::setw(4) << std::setfill('0') << i + << ".example.org"; + + // Create lease. + Lease4Ptr lease(new Lease4(address, hwaddr, ClientIdPtr(), 60, 10, 20, + time(NULL), SubnetID(1), true, true, + hostname_s.str())); + leases_.push_back(lease); + LeaseMgrFactory::instance().addLease(lease); + } +} + +// This test verifies that the leases can be reclaimed without being removed +// from the database. In such case, the leases' state is set to +// "expired-reclaimed". +TEST_F(ExpirationAllocEngine4Test, reclaimExpiredLeasesUpdateState) { + testReclaimExpiredLeasesUpdateState(); +} + +// This test verifies that the reclaimed leases are deleted when requested. +TEST_F(ExpirationAllocEngine4Test, reclaimExpiredLeasesDelete) { + testReclaimExpiredLeasesDelete(); +} + +// This test verifies that it is possible to specify the limit for the +// number of reclaimed leases. +TEST_F(ExpirationAllocEngine4Test, reclaimExpiredLeasesLimit) { + testReclaimExpiredLeasesLimit(); +} + +// This test verifies that DNS updates are generated for the leases +// for which the DNS records exist. +TEST_F(ExpirationAllocEngine4Test, reclaimExpiredLeasesWithDDNS) { + testReclaimExpiredLeasesWithDDNS(); +} + +// This test verifies that it is DNS updates are generated only for the +// reclaimed expired leases. In this case we limit the number of leases +// reclaimed during a single call to reclamation routine. +TEST_F(ExpirationAllocEngine4Test, reclaimExpiredLeasesWithDDNSAndLimit) { + testReclaimExpiredLeasesWithDDNSAndLimit(); +} + +// This test verifies that if some leases have invalid hostnames, the +// lease reclamation routine continues with reclamation of leases anyway. +TEST_F(ExpirationAllocEngine4Test, reclaimExpiredLeasesInvalidHostname) { + testReclaimExpiredLeasesInvalidHostname(); } }; // end of anonymous namespace From 0ea15c0d021f0eaf1b437846820ef4d39c85ba50 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Thu, 10 Sep 2015 12:43:37 +0200 Subject: [PATCH 4/9] [3973] Use client identifier to generate DHCID for lease reclamation. --- src/lib/dhcpsrv/alloc_engine.cc | 102 ++++---- src/lib/dhcpsrv/alloc_engine.h | 25 ++ src/lib/dhcpsrv/alloc_engine_messages.mes | 28 ++- .../tests/alloc_engine_expiration_unittest.cc | 235 +++++++++++++++--- 4 files changed, 304 insertions(+), 86 deletions(-) diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index c6306548d5..4009b1e512 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -30,6 +30,8 @@ #include #include +#include + #include #include #include @@ -1296,8 +1298,7 @@ AllocEngine::reclaimExpiredLeases6(const size_t max_leases, const uint16_t timeo Lease6Collection leases; lease_mgr.getExpiredLeases6(leases, max_leases); - for (Lease6Collection::const_iterator lease_it = leases.begin(); - lease_it != leases.end(); ++lease_it) { + BOOST_FOREACH(Lease6Ptr lease, leases) { try { /// @todo execute a lease6_expire hook here. @@ -1305,31 +1306,17 @@ AllocEngine::reclaimExpiredLeases6(const size_t max_leases, const uint16_t timeo // Generate removal name change request for D2, if required. // This will return immediatelly if the DNS wasn't updated // when the lease was created. - queueRemovalNameChangeRequest(*lease_it, *(*lease_it)->duid_); + queueRemovalNameChangeRequest(lease, *(lease->duid_)); // Reclaim the lease - depending on the configuration, set the // expired-reclaimed state or simply remove it. - if (remove_lease) { - lease_mgr.deleteLease((*lease_it)->addr_); - - } else { - // Clear FQDN information as we have already sent the - // name change request to remove the DNS record. - (*lease_it)->hostname_.clear(); - (*lease_it)->fqdn_fwd_ = false; - (*lease_it)->fqdn_rev_ = false; - (*lease_it)->state_ = Lease::STATE_EXPIRED_RECLAIMED; - lease_mgr.updateLease6(*lease_it); - } - - // Lease has been reclaimed. - LOG_DEBUG(alloc_engine_logger, ALLOC_ENGINE_DBG_TRACE, - ALLOC_ENGINE_V6_LEASE_RECLAIMED) - .arg((*lease_it)->addr_.toText()); + reclaimLeaseInDatabase(lease, remove_lease, + boost::bind(&LeaseMgr::updateLease6, + &lease_mgr, _1)); } catch (const std::exception& ex) { LOG_ERROR(alloc_engine_logger, ALLOC_ENGINE_V6_LEASE_RECLAMATION_FAILED) - .arg((*lease_it)->addr_.toText()) + .arg(lease->addr_.toText()) .arg(ex.what()); } } @@ -1349,7 +1336,7 @@ AllocEngine::reclaimExpiredLeases4(const size_t max_leases, const uint16_t timeo const bool remove_lease) { LOG_DEBUG(alloc_engine_logger, ALLOC_ENGINE_DBG_TRACE, - ALLOC_ENGINE_V6_LEASES_RECLAMATION_START) + ALLOC_ENGINE_V4_LEASES_RECLAMATION_START) .arg(max_leases) .arg(timeout); @@ -1362,40 +1349,33 @@ AllocEngine::reclaimExpiredLeases4(const size_t max_leases, const uint16_t timeo Lease4Collection leases; lease_mgr.getExpiredLeases4(leases, max_leases); - for (Lease4Collection::const_iterator lease_it = leases.begin(); - lease_it != leases.end(); ++lease_it) { + BOOST_FOREACH(Lease4Ptr lease, leases) { try { - /// @todo execute a lease6_expire hook here. + /// @todo execute a lease4_expire hook here. // Generate removal name change request for D2, if required. // This will return immediatelly if the DNS wasn't updated // when the lease was created. - queueRemovalNameChangeRequest(*lease_it, (*lease_it)->hwaddr_); + if (lease->client_id_) { + // Client id takes precedence over HW address. + queueRemovalNameChangeRequest(lease, lease->client_id_->getClientId()); + + } else { + // Client id is not specified for the lease. Use HW address + // instead. + queueRemovalNameChangeRequest(lease, lease->hwaddr_); + } // Reclaim the lease - depending on the configuration, set the // expired-reclaimed state or simply remove it. - if (remove_lease) { - lease_mgr.deleteLease((*lease_it)->addr_); - - } else { - // Clear FQDN information as we have already sent the - // name change request to remove the DNS record. - (*lease_it)->hostname_.clear(); - (*lease_it)->fqdn_fwd_ = false; - (*lease_it)->fqdn_rev_ = false; - (*lease_it)->state_ = Lease::STATE_EXPIRED_RECLAIMED; - lease_mgr.updateLease4(*lease_it); - } - - // Lease has been reclaimed. - LOG_DEBUG(alloc_engine_logger, ALLOC_ENGINE_DBG_TRACE, - ALLOC_ENGINE_V6_LEASE_RECLAIMED) - .arg((*lease_it)->addr_.toText()); + reclaimLeaseInDatabase(lease, remove_lease, + boost::bind(&LeaseMgr::updateLease4, + &lease_mgr, _1)); } catch (const std::exception& ex) { - LOG_ERROR(alloc_engine_logger, ALLOC_ENGINE_V6_LEASE_RECLAMATION_FAILED) - .arg((*lease_it)->addr_.toText()) + LOG_ERROR(alloc_engine_logger, ALLOC_ENGINE_V4_LEASE_RECLAMATION_FAILED) + .arg(lease->addr_.toText()) .arg(ex.what()); } } @@ -1405,12 +1385,11 @@ AllocEngine::reclaimExpiredLeases4(const size_t max_leases, const uint16_t timeo // Mark completion of the lease reclamation routine and present some stats. LOG_DEBUG(alloc_engine_logger, ALLOC_ENGINE_DBG_TRACE, - ALLOC_ENGINE_V6_LEASES_RECLAMATION_COMPLETE) + ALLOC_ENGINE_V4_LEASES_RECLAMATION_COMPLETE) .arg(leases.size()) .arg(stopwatch.logFormatTotalDuration()); } - template void AllocEngine::queueRemovalNameChangeRequest(const LeasePtrType& lease, @@ -1444,6 +1423,35 @@ AllocEngine::queueRemovalNameChangeRequest(const LeasePtrType& lease, } } +template +void AllocEngine::reclaimLeaseInDatabase(const LeasePtrType& lease, + const bool remove_lease, + const boost::function& + lease_update_fun) const { + LeaseMgr& lease_mgr = LeaseMgrFactory::instance(); + + // Reclaim the lease - depending on the configuration, set the + // expired-reclaimed state or simply remove it. + if (remove_lease) { + lease_mgr.deleteLease(lease->addr_); + + } else { + // Clear FQDN information as we have already sent the + // name change request to remove the DNS record. + lease->hostname_.clear(); + lease->fqdn_fwd_ = false; + lease->fqdn_rev_ = false; + lease->state_ = Lease::STATE_EXPIRED_RECLAIMED; + lease_update_fun(lease); + } + + // Lease has been reclaimed. + LOG_DEBUG(alloc_engine_logger, ALLOC_ENGINE_DBG_TRACE, + ALLOC_ENGINE_LEASE_RECLAIMED) + .arg(lease->addr_.toText()); +} + + } // end of isc::dhcp namespace } // end of isc namespace diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index fc8f662a2b..e5f1c53212 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -26,6 +26,7 @@ #include #include +#include #include #include @@ -708,6 +709,30 @@ private: void queueRemovalNameChangeRequest(const LeasePtrType& lease, const IdentifierType& identifier) const; + /// @brief Marks lease as reclaimed in the database. + /// + /// This method is called internally by the leases reclaimation routines. + /// Depending on the value of the @c remove_lease parameter this method + /// will delete the reclaimed lease from the database or set its sate + /// to "expired-reclaimed". In the latter case it will also clear the + /// FQDN information. + /// + /// This method may throw exceptions if the operation on the lease database + /// fails for any reason. + /// + /// @param lease Pointer to the lease. + /// @param remove_lease Boolean flag indicating if the lease should be + /// removed from the database (if true). + /// @param lease_update_fun Pointer to the function in the @c LeaseMgr to + /// be used to update the lease if the @c remove_lease is set to false. + /// + /// @tparam LeasePtrType One of the @c Lease6Ptr or @c Lease4Ptr. + template + void reclaimLeaseInDatabase(const LeasePtrType& lease, + const bool remove_lease, + const boost::function& + lease_update_fun) const; + public: /// @brief Context information for the DHCPv4 lease allocation. diff --git a/src/lib/dhcpsrv/alloc_engine_messages.mes b/src/lib/dhcpsrv/alloc_engine_messages.mes index 6abf3d5d85..4a60ae76b9 100644 --- a/src/lib/dhcpsrv/alloc_engine_messages.mes +++ b/src/lib/dhcpsrv/alloc_engine_messages.mes @@ -14,6 +14,10 @@ $NAMESPACE isc::dhcp +% ALLOC_ENGINE_LEASE_RECLAIMED successfully reclaimed lease %1 +This debug message is logged when the allocation engine successfully +reclaims a lease. + % ALLOC_ENGINE_REMOVAL_NCR_FAILED sending removal name change request failed for lease %1: %2 This error message is logged when sending a removal name change request to D2 failed. This name change request is usually generated when the @@ -57,6 +61,26 @@ client sending the DHCPDISCOVER has a reservation for the specified address. The allocation engine will try to offer this address to the client. +% ALLOC_ENGINE_V4_LEASE_RECLAMATION_FAILED failed to reclaim the lease %1: %2 +This error message is logged when the allocation engine fails to +reclaim an expired lease. The reason for the failure is included in the +message. The error may be triggered in the lease expiration hook or +while performing the operation on the lease database. + +% ALLOC_ENGINE_V4_LEASES_RECLAMATION_COMPLETE reclaimed %1 leases in %2 +This debug message is logged when the allocation engine completes +reclamation of a set of expired leases. The maximum number of leases +to be reclaimed in a single pass of the lease reclamation routine +is configurable. However, the number of reclaimed leases may also +be limited by the timeout value. The message includes the number +of reclaimed leases and the total time. + +% ALLOC_ENGINE_V4_LEASES_RECLAMATION_START starting reclamation of expired leases (limit = %1 leases, timeout = %2 seconds) +This debug message is issued when the allocation engine starts the +reclamation of the expired leases. The maximum number of leases to +be reclaimed and the timeout is included in the message. If any of +these values is 0, it means "unlimited". + % ALLOC_ENGINE_V4_OFFER_EXISTING_LEASE allocation engine will try to offer existing lease to the client %1 This message is issued when the allocation engine determines that the client has a lease in the lease database, it doesn't have @@ -260,10 +284,6 @@ reclaim an expired lease. The reason for the failure is included in the message. The error may be triggered in the lease expiration hook or while performing the operation on the lease database. -% ALLOC_ENGINE_V6_LEASE_RECLAIMED successfully reclaimed IPv6 lease %1 -This debug message is logged when the allocation engine successfully -reclaims a lease. - % ALLOC_ENGINE_V6_LEASES_RECLAMATION_COMPLETE reclaimed %1 leases in %2 This debug message is logged when the allocation engine completes reclamation of a set of expired leases. The maximum number of leases diff --git a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc index 3227854185..2b7077bec8 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc @@ -14,6 +14,8 @@ #include #include +#include +#include #include #include #include @@ -202,7 +204,7 @@ public: /// @c TEST_LEASES_NUM. /// @param secs Offset of the expiration time since now. For example /// a value of 2 would set the lease expiration time to 2 seconds ago. - void expire(const unsigned int lease_index, const time_t secs) { + void expire(const uint16_t lease_index, const time_t secs) { ASSERT_GT(leases_.size(), lease_index); // We set the expiration time indirectly by modifying the cltt value. leases_[lease_index]->cltt_ = time(NULL) - secs - @@ -286,7 +288,7 @@ public: /// @param index Lease index. /// @return true if index is an even number. static bool evenLeaseIndex(const size_t index) { - return (index % 2); + return (index % 2 == 0); } /// @brief Index algorithm selecting odd indexes. @@ -350,24 +352,12 @@ public: /// @return true if NCR has been generated for the lease. static bool dnsUpdateGeneratedForLease(const LeasePtrType& lease) { try { - // Iterate over the generated name change requests and try - // to find the match with our lease (using IP address). If - D2ClientMgr& mgr = CfgMgr::instance().getD2ClientMgr(); - for (size_t i = 0; i < mgr.getQueueSize(); ++i) { - const NameChangeRequestPtr& ncr = mgr.peekAt(i); - // If match found, return true. - if (ncr->getIpAddress() == lease->addr_.toText()) { - return (true); - } - } + return (static_cast(getNCRForLease(lease))); + } catch (...) { // If error occurred, treat it as no match. return (false); } - - // All leases checked - no match. - return (false); - } /// @brief Lease algorithm checking if removal name change request @@ -396,6 +386,57 @@ public: return (true); } + /// @brief Returns Name Change Request from the D2 client queue. + /// + /// @param lease Pointer to the lease to be matched with NCR. + /// + /// @return null pointer if no match found. + static NameChangeRequestPtr getNCRForLease(const LeasePtrType& lease) { + // Iterate over the generated name change requests and try + // to find the match with our lease (using IP address). If + D2ClientMgr& mgr = CfgMgr::instance().getD2ClientMgr(); + for (size_t i = 0; i < mgr.getQueueSize(); ++i) { + const NameChangeRequestPtr& ncr = mgr.peekAt(i); + // If match found, return true. + if (ncr->getIpAddress() == lease->addr_.toText()) { + return (ncr); + } + } + return (NameChangeRequestPtr()); + } + + /// @brief Returns index of the lease from the address. + /// + /// This method assumes that leases are ordered from the smallest to + /// the highest address, e.g. 10.0.0.0, 10.0.0.1, 10.0.0.2 etc. The + /// last two bytes can be used to extract index. + /// + /// @param address Address. + /// + /// @return index + static uint16_t getLeaseIndexFromAddress(const IOAddress& address) { + std::vector bytes = address.toBytes(); + std::vector::reverse_iterator bytes_it = bytes.rbegin(); + uint16_t index = static_cast(*bytes_it) | + (static_cast(*(bytes_it + 1)) << 8); + return (index); + } + + /// @brief Generates hostname for lease index. + /// + /// Generates hostname in the form of "hostXXXX.example.org", where + /// XXXX is a lease index. + /// + /// @param index Lease index. + /// + /// @return Generated hostname. + static std::string generateHostnameForLeaseIndex(const uint16_t index) { + std::ostringstream hostname_s; + hostname_s << "host" << std::setw(4) << std::setfill('0') + << index << ".example.org"; + return (hostname_s.str()); + } + /// @brief Test that leases can be reclaimed without being removed. void testReclaimExpiredLeasesUpdateState() { for (int i = 0; i < TEST_LEASES_NUM; ++i) { @@ -603,7 +644,7 @@ public: std::ostringstream hostname_s; hostname_s << "invalid-host" << i << "..example.com"; leases_[i]->hostname_ = hostname_s.str(); - ASSERT_NO_THROW(LeaseMgrFactory::instance().updateLease6(leases_[i])); + ASSERT_NO_THROW(updateLease(i)); } // Every lease is expired. expire(i, 10 + i); @@ -689,7 +730,7 @@ ExpirationAllocEngine6Test::ExpirationAllocEngine6Test() void ExpirationAllocEngine6Test::createLeases() { // Create TEST_LEASES_NUM leases. - for (int i = 0; i < TEST_LEASES_NUM; ++i) { + for (uint16_t i = 0; i < TEST_LEASES_NUM; ++i) { // DUID std::ostringstream duid_s; duid_s << "01020304050607" << std::setw(4) << std::setfill('0') << i; @@ -700,14 +741,10 @@ ExpirationAllocEngine6Test::createLeases() { address_s << "2001:db8:1::" << std::setw(4) << std::setfill('0') << i; IOAddress address(address_s.str()); - // Hostname. - std::ostringstream hostname_s; - hostname_s << "host" << std::setw(4) << std::setfill('0') << i - << ".example.org"; - // Create lease. - Lease6Ptr lease(new Lease6(Lease::TYPE_NA, address, duid, 1, 50, 60, 10, 20, - SubnetID(1), true, true, hostname_s.str())); + Lease6Ptr lease(new Lease6(Lease::TYPE_NA, address, duid, 1, 50, 60, 10, + 20, SubnetID(1), true, true, + generateHostnameForLeaseIndex(i))); leases_.push_back(lease); LeaseMgrFactory::instance().addLease(lease); } @@ -773,6 +810,11 @@ public: /// It is called internally at the construction time. void createLeases(); + /// @brief Generates unique client identifier from lease index. + /// + /// @param index lease index. + void setUniqueClientId(const uint16_t index); + /// @brief Updates lease in the lease database. /// /// @param lease_index Index of the lease. @@ -800,6 +842,20 @@ public: const bool remove_lease) { engine_->reclaimExpiredLeases4(max_leases, timeout, remove_lease); } + + /// @brief Lease algorithm checking if NCR has been generated from client + /// identifier. + /// + /// @param lease Pointer to the lease for which the NCR needs to be checked. + static bool dnsUpdateGeneratedFromClientId(const Lease4Ptr& lease); + + /// @brief Lease algorithm checking if NCR has been generated from + /// HW address. + static bool dnsUpdateGeneratedFromHWAddress(const Lease4Ptr& lease); + + /// @brief Test that DNS updates are properly generated when the + /// reclaimed leases contain client identifier. + void testReclaimExpiredLeasesWithDDNSAndClientId(); }; ExpirationAllocEngine4Test::ExpirationAllocEngine4Test() @@ -810,33 +866,136 @@ ExpirationAllocEngine4Test::ExpirationAllocEngine4Test() void ExpirationAllocEngine4Test::createLeases() { // Create TEST_LEASES_NUM leases. - for (uint32_t i = 0; i < TEST_LEASES_NUM; ++i) { + for (uint16_t i = 0; i < TEST_LEASES_NUM; ++i) { // HW address std::ostringstream hwaddr_s; hwaddr_s << "01:02:03:04:" << std::setw(2) << std::setfill('0') - << (i >> 16) << ":" << std::setw(2) << std::setfill('0') + << (i >> 8) << ":" << std::setw(2) << std::setfill('0') << (i & 0x00FF); - HWAddrPtr hwaddr(new HWAddr(HWAddr::fromText(hwaddr_s.str(), HTYPE_ETHER))); + HWAddrPtr hwaddr(new HWAddr(HWAddr::fromText(hwaddr_s.str(), + HTYPE_ETHER))); // Address. std::ostringstream address_s; - address_s << "10.0." << (i >> 16) << "." << (i & 0x00FF); + address_s << "10.0." << (i >> 8) << "." << (i & 0x00FF); IOAddress address(address_s.str()); - // Hostname. - std::ostringstream hostname_s; - hostname_s << "host" << std::setw(4) << std::setfill('0') << i - << ".example.org"; - // Create lease. Lease4Ptr lease(new Lease4(address, hwaddr, ClientIdPtr(), 60, 10, 20, time(NULL), SubnetID(1), true, true, - hostname_s.str())); + generateHostnameForLeaseIndex(i))); leases_.push_back(lease); LeaseMgrFactory::instance().addLease(lease); } } +void +ExpirationAllocEngine4Test::setUniqueClientId(const uint16_t index) { + std::ostringstream clientid_s; + clientid_s << "AA:BB:" << std::setw(2) << std::setfill('0') + << (index >> 16) << ":" << std::setw(2) << std::setfill('0') + << (index & 0x00FF); + ClientIdPtr client_id(ClientId::fromText(clientid_s.str())); + leases_[index]->client_id_ = client_id; + LeaseMgrFactory::instance().updateLease4(leases_[index]); +} + +bool +ExpirationAllocEngine4Test::dnsUpdateGeneratedFromClientId(const Lease4Ptr& lease) { + try { + NameChangeRequestPtr ncr = getNCRForLease(lease); + if (ncr) { + if (lease->client_id_) { + // Generate hostname for this lease. Note that the lease + // in the database doesn't have the hostname because it + // has been removed by the lease reclamation routine. + std::string hostname = generateHostnameForLeaseIndex( + getLeaseIndexFromAddress(lease->addr_)); + + // Get DHCID from NCR. + const D2Dhcid& dhcid = ncr->getDhcid(); + // Generate reference DHCID to compare with the one from + // the NCR. + std::vector fqdn_wire; + OptionDataTypeUtil::writeFqdn(hostname, fqdn_wire, true); + D2Dhcid clientid_dhcid(lease->client_id_->getClientId(), + fqdn_wire); + // Return true if they match. + return (dhcid == clientid_dhcid); + } + } + + } catch (...) { + // If error occurred, treat it as no match. + return (false); + } + + // All leases checked - no match. + return (false); +} + +bool +ExpirationAllocEngine4Test::dnsUpdateGeneratedFromHWAddress(const Lease4Ptr& lease) { + try { + NameChangeRequestPtr ncr = getNCRForLease(lease); + if (ncr) { + if (lease->hwaddr_) { + // Generate hostname for this lease. Note that the lease + // in the database doesn't have the hostname because it + // has been removed by the lease reclamation routine. + std::string hostname = generateHostnameForLeaseIndex( + getLeaseIndexFromAddress(lease->addr_)); + + // Get DHCID from NCR. + const D2Dhcid& dhcid = ncr->getDhcid(); + // Generate reference DHCID to compare with the one from + // the NCR. + std::vector fqdn_wire; + OptionDataTypeUtil::writeFqdn(hostname, fqdn_wire, true); + D2Dhcid hwaddr_dhcid(lease->hwaddr_, fqdn_wire); + // Return true if they match. + return (dhcid == hwaddr_dhcid); + } + } + + } catch (...) { + // If error occurred, treat it as no match. + return (false); + } + + // All leases checked - no match. + return (false); +} + +void +ExpirationAllocEngine4Test::testReclaimExpiredLeasesWithDDNSAndClientId() { + // DNS must be started for the D2 client to accept NCRs. + ASSERT_NO_THROW(enableDDNS()); + + for (int i = 0; i < TEST_LEASES_NUM; ++i) { + // Set client identifiers for leases with even indexes only. + if (evenLeaseIndex(i)) { + setUniqueClientId(i); + } + // Expire all leases. The higher the index, the more expired the lease. + expire(i, 10 + i); + } + + // Reclaim all expired leases. + ASSERT_NO_THROW(reclaimExpiredLeases(0, 0, false)); + + // Leases with even indexes should be reclaimed. + EXPECT_TRUE(testLeases(&leaseReclaimed, &evenLeaseIndex)); + // DNS updates (removal NCRs) should be generated for all leases. + EXPECT_TRUE(testLeases(&dnsUpdateGeneratedForLease, &allLeaseIndexes)); + // Leases with even indexes include client identifiers so the DHCID should + // be generated from the client identifiers. + EXPECT_TRUE(testLeases(&dnsUpdateGeneratedFromClientId, &evenLeaseIndex)); + // Leases with odd indexes do not include client identifiers so their + // DHCID should be generated from the HW address. + EXPECT_TRUE(testLeases(&dnsUpdateGeneratedFromHWAddress, &oddLeaseIndex)); +} + // This test verifies that the leases can be reclaimed without being removed // from the database. In such case, the leases' state is set to // "expired-reclaimed". @@ -874,4 +1033,10 @@ TEST_F(ExpirationAllocEngine4Test, reclaimExpiredLeasesInvalidHostname) { testReclaimExpiredLeasesInvalidHostname(); } +// This test verifies that DNS updates are properly generated when the +// client id is used as a primary identifier in the lease. +TEST_F(ExpirationAllocEngine4Test, reclaimExpiredLeasesWithDDNSAndClientId) { + testReclaimExpiredLeasesWithDDNSAndClientId(); +} + }; // end of anonymous namespace From 216d4107cbdb37e1831a7652e1db573b435ff101 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Thu, 10 Sep 2015 16:27:32 +0200 Subject: [PATCH 5/9] [3973] Update statistics when the lease is reclaimed. --- src/lib/dhcpsrv/alloc_engine.cc | 46 ++++ .../tests/alloc_engine_expiration_unittest.cc | 244 +++++++++++++++++- 2 files changed, 289 insertions(+), 1 deletion(-) diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 4009b1e512..af97beb83f 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -1314,6 +1314,35 @@ AllocEngine::reclaimExpiredLeases6(const size_t max_leases, const uint16_t timeo boost::bind(&LeaseMgr::updateLease6, &lease_mgr, _1)); + // Update statistics. + + // Decrease number of assigned leases. + if (lease->type_ == Lease::TYPE_NA) { + // IA_NA + StatsMgr::instance().addValue(StatsMgr::generateName("subnet", + lease->subnet_id_, + "assigned-nas"), + int64_t(-1)); + + } else if (lease->type_ == Lease::TYPE_PD) { + // IA_PD + StatsMgr::instance().addValue(StatsMgr::generateName("subnet", + lease->subnet_id_, + "assigned-pds"), + int64_t(-1)); + + } + + // Increase total number of reclaimed leases. + StatsMgr::instance().addValue("reclaimed-leases", int64_t(1)); + + // Increase number of reclaimed leases for a subnet. + StatsMgr::instance().addValue(StatsMgr::generateName("subnet", + lease->subnet_id_, + "reclaimed-leases"), + int64_t(1)); + + } catch (const std::exception& ex) { LOG_ERROR(alloc_engine_logger, ALLOC_ENGINE_V6_LEASE_RECLAMATION_FAILED) .arg(lease->addr_.toText()) @@ -1373,6 +1402,23 @@ AllocEngine::reclaimExpiredLeases4(const size_t max_leases, const uint16_t timeo boost::bind(&LeaseMgr::updateLease4, &lease_mgr, _1)); + // Update statistics. + + // Decrease number of assigned addresses. + StatsMgr::instance().addValue(StatsMgr::generateName("subnet", + lease->subnet_id_, + "assigned-addresses"), + int64_t(-1)); + + // Increase total number of reclaimed leases. + StatsMgr::instance().addValue("reclaimed-leases", int64_t(1)); + + // Increase number of reclaimed leases for a subnet. + StatsMgr::instance().addValue(StatsMgr::generateName("subnet", + lease->subnet_id_, + "reclaimed-leases"), + int64_t(1)); + } catch (const std::exception& ex) { LOG_ERROR(alloc_engine_logger, ALLOC_ENGINE_V4_LEASE_RECLAMATION_FAILED) .arg(lease->addr_.toText()) diff --git a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc index 2b7077bec8..93ff635367 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -33,6 +34,7 @@ using namespace isc::asiolink; using namespace isc::dhcp; using namespace isc::dhcp::test; using namespace isc::dhcp_ddns; +using namespace isc::stats; namespace { @@ -158,12 +160,20 @@ public: /// Clears configuration, creates new lease manager and allocation engine /// instances. ExpirationAllocEngineTest(const std::string& lease_mgr_params) { + // Clear configuration. CfgMgr::instance().clear(); + D2ClientConfigPtr cfg(new D2ClientConfig()); + CfgMgr::instance().setD2ClientConfig(cfg); + + // Remove all statistics. + StatsMgr::instance().resetAll(); + + // Create lease manager. LeaseMgrFactory::create(lease_mgr_params); + // Create allocation engine instance. engine_.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100, false)); - } /// @brief Destructor @@ -179,7 +189,15 @@ public: mgr.clearQueue(); } + // Clear configuration. CfgMgr::instance().clear(); + D2ClientConfigPtr cfg(new D2ClientConfig()); + CfgMgr::instance().setD2ClientConfig(cfg); + + // Remove all statistics. + StatsMgr::instance().resetAll(); + + // Kill lease manager. LeaseMgrFactory::destroy(); } @@ -222,6 +240,14 @@ public: /// @param lease_index Index of the lease. virtual LeasePtrType getLease(const unsigned int lease_index) const = 0; + /// @brief Sets subnet id for a lease. + /// + /// It also updates statistics of assigned leases in the stats manager. + /// + /// @param lease_index Lease index. + /// @param id New subnet id. + virtual void setSubnetId(const uint16_t lease_index, const SubnetID& id) = 0; + /// @brief Wrapper method running lease reclamation routine. /// /// @param max_leases Maximum number of leases to be reclaimed. @@ -234,6 +260,30 @@ public: const uint16_t timeout, const bool remove_lease) = 0; + /// @brief Test that statistic manager holds a given value. + /// + /// @param stat_name Statistic name. + /// @param exp_value Expected value. + bool testStatistics(const std::string& stat_name, const int64_t exp_value) const { + try { + ObservationPtr observation = StatsMgr::instance().getObservation(stat_name); + if (observation) { + if (observation->getInteger().first != exp_value) { + ADD_FAILURE() + << "value of the observed statistics '" + << stat_name << "' " << "(" + << observation->getInteger().first << ") " + << "doesn't match expected value (" << exp_value << ")"; + } + return (observation->getInteger().first == exp_value); + } + + } catch (...) { + ; + } + return (false); + } + /// @brief Test selected leases using the specified algorithms. /// /// This function picks leases from the range of 0 thru @@ -698,6 +748,22 @@ public: LeaseMgrFactory::instance().updateLease6(leases_[lease_index]); } + /// @brief Sets subnet id for a lease. + /// + /// It also updates statistics of assigned leases in the stats manager. + /// + /// @param lease_index Lease index. + /// @param id New subnet id. + virtual void setSubnetId(const uint16_t lease_index, const SubnetID& id); + + /// @brief Sets type of a lease. + /// + /// It also updates statistics of assigned leases in the stats manager. + /// + /// @param lease_index Lease index. + /// @param lease_type Lease type. + void setLeaseType(const uint16_t lease_index, const Lease6::Type& lease_type); + /// @brief Retrieves lease from the database. /// /// @param lease_index Index of the lease. @@ -720,6 +786,9 @@ public: engine_->reclaimExpiredLeases6(max_leases, timeout, remove_lease); } + /// @brief Test that statistics is updated when leases are reclaimed. + void testReclaimExpiredLeasesStats(); + }; ExpirationAllocEngine6Test::ExpirationAllocEngine6Test() @@ -747,9 +816,96 @@ ExpirationAllocEngine6Test::createLeases() { generateHostnameForLeaseIndex(i))); leases_.push_back(lease); LeaseMgrFactory::instance().addLease(lease); + + // Note in the statistics that this lease has been added. + StatsMgr& stats_mgr = StatsMgr::instance(); + std::string stat_name = + lease->type_ == Lease::TYPE_NA ? "assigned-nas" : "assigned-pds"; + stats_mgr.addValue(stats_mgr.generateName("subnet", lease->subnet_id_, stat_name), + int64_t(1)); } } +void +ExpirationAllocEngine6Test::setSubnetId(const uint16_t lease_index, const SubnetID& id) { + ASSERT_GT(leases_.size(), lease_index); + if (leases_[lease_index]->subnet_id_ != id) { + StatsMgr& stats_mgr = StatsMgr::instance(); + std::string stats_name = (leases_[lease_index]->type_ == Lease::TYPE_NA ? + "assigned-nas" : "assigned-pds"); + stats_mgr.addValue(stats_mgr.generateName("subnet", id, stats_name), + int64_t(1)); + stats_mgr.addValue(stats_mgr.generateName("subnet", + leases_[lease_index]->subnet_id_, + stats_name), + int64_t(-1)); + leases_[lease_index]->subnet_id_ = id; + ASSERT_NO_THROW(updateLease(lease_index)); + } +} + +void +ExpirationAllocEngine6Test::setLeaseType(const uint16_t lease_index, + const Lease6::Type& lease_type) { + ASSERT_GT(leases_.size(), lease_index); + if (leases_[lease_index]->type_ != lease_type) { + StatsMgr& stats_mgr = StatsMgr::instance(); + std::string stats_name = (lease_type == Lease::TYPE_NA ? + "assigned-nas" : "assigned-pds"); + stats_mgr.addValue(stats_mgr.generateName("subnet", + leases_[lease_index]->subnet_id_, + stats_name), + int64_t(1)); + stats_name = (leases_[lease_index]->type_ == Lease::TYPE_NA ? + "assigned-nas" : "assigned-pds"); + stats_mgr.addValue(stats_mgr.generateName("subnet", + leases_[lease_index]->subnet_id_, + stats_name), + int64_t(-1)); + leases_[lease_index]->type_ = lease_type; + ASSERT_NO_THROW(updateLease(lease_index)); + } +} + + +void +ExpirationAllocEngine6Test::testReclaimExpiredLeasesStats() { + // This test requires that the number of leases is an even number. + BOOST_STATIC_ASSERT(TEST_LEASES_NUM % 2 == 0); + + for (int i = 0; i < TEST_LEASES_NUM; ++i) { + // Mark all leaes as expired. The higher the index the less + // expired the lease. + expire(i, 1000 - i); + // Modify subnet ids and lease types for some leases. + if (evenLeaseIndex(i)) { + setSubnetId(i, SubnetID(2)); + setLeaseType(i, Lease::TYPE_PD); + } + } + + // Leases will be reclaimed in groups of 8. + const size_t reclamation_group_size = 8; + for (int i = reclamation_group_size; i < TEST_LEASES_NUM; + i += reclamation_group_size) { + + // Reclaim 8 most expired leases out of TEST_LEASES_NUM. + ASSERT_NO_THROW(reclaimExpiredLeases(reclamation_group_size, + 0, false)); + + // Number of reclaimed leases should increase as we loop. + EXPECT_TRUE(testStatistics("reclaimed-leases", i)); + // Make sure that the number of reclaimed leases is also distributed + // across two subnets. + EXPECT_TRUE(testStatistics("subnet[1].reclaimed-leases", i / 2)); + EXPECT_TRUE(testStatistics("subnet[2].reclaimed-leases", i / 2)); + // Number of assigned leases should decrease as we reclaim them. + EXPECT_TRUE(testStatistics("subnet[1].assigned-nas", + (TEST_LEASES_NUM - i) / 2)); + EXPECT_TRUE(testStatistics("subnet[2].assigned-pds", + (TEST_LEASES_NUM - i) / 2)); + } +} // This test verifies that the leases can be reclaimed without being removed // from the database. In such case, the leases' state is set to @@ -788,6 +944,12 @@ TEST_F(ExpirationAllocEngine6Test, reclaimExpiredLeasesInvalidHostname) { testReclaimExpiredLeasesInvalidHostname(); } +// This test verifies that statistics is correctly updated when the leases +// are reclaimed. +TEST_F(ExpirationAllocEngine6Test, reclaimExpiredLeasesStats) { + testReclaimExpiredLeasesStats(); +} + // ******************************************************* // // DHCPv4 lease reclamation routine tests start here! @@ -829,6 +991,14 @@ public: return (LeaseMgrFactory::instance().getLease4(leases_[lease_index]->addr_)); } + /// @brief Sets subnet id for a lease. + /// + /// It also updates statistics of assigned leases in the stats manager. + /// + /// @param lease_index Lease index. + /// @param id New subnet id. + virtual void setSubnetId(const uint16_t lease_index, const SubnetID& id); + /// @brief Wrapper method running lease reclamation routine. /// /// @param max_leases Maximum number of leases to be reclaimed. @@ -856,6 +1026,10 @@ public: /// @brief Test that DNS updates are properly generated when the /// reclaimed leases contain client identifier. void testReclaimExpiredLeasesWithDDNSAndClientId(); + + /// @brief Test that statistics is updated when leases are reclaimed.. + void testReclaimExpiredLeasesStats(); + }; ExpirationAllocEngine4Test::ExpirationAllocEngine4Test() @@ -886,6 +1060,12 @@ ExpirationAllocEngine4Test::createLeases() { generateHostnameForLeaseIndex(i))); leases_.push_back(lease); LeaseMgrFactory::instance().addLease(lease); + + // Note in the statistics that this lease has been added. + StatsMgr& stats_mgr = StatsMgr::instance(); + std::string stat_name = "assigned-addresses"; + stats_mgr.addValue(stats_mgr.generateName("subnet", lease->subnet_id_, stat_name), + int64_t(1)); } } @@ -900,6 +1080,23 @@ ExpirationAllocEngine4Test::setUniqueClientId(const uint16_t index) { LeaseMgrFactory::instance().updateLease4(leases_[index]); } +void +ExpirationAllocEngine4Test::setSubnetId(const uint16_t lease_index, const SubnetID& id) { + ASSERT_GT(leases_.size(), lease_index); + if (leases_[lease_index]->subnet_id_ != id) { + StatsMgr& stats_mgr = StatsMgr::instance(); + stats_mgr.addValue(stats_mgr.generateName("subnet", id, "assigned-addresses"), + int64_t(1)); + stats_mgr.addValue(stats_mgr.generateName("subnet", + leases_[lease_index]->subnet_id_, + "assigned-addresses"), + int64_t(-1)); + leases_[lease_index]->subnet_id_ = id; + ASSERT_NO_THROW(updateLease(lease_index)); + } +} + + bool ExpirationAllocEngine4Test::dnsUpdateGeneratedFromClientId(const Lease4Ptr& lease) { try { @@ -996,6 +1193,45 @@ ExpirationAllocEngine4Test::testReclaimExpiredLeasesWithDDNSAndClientId() { EXPECT_TRUE(testLeases(&dnsUpdateGeneratedFromHWAddress, &oddLeaseIndex)); } +void +ExpirationAllocEngine4Test::testReclaimExpiredLeasesStats() { + // This test requires that the number of leases is an even number. + BOOST_STATIC_ASSERT(TEST_LEASES_NUM % 2 == 0); + + for (int i = 0; i < TEST_LEASES_NUM; ++i) { + // Mark all leaes as expired. The higher the index the less + // expired the lease. + expire(i, 1000 - i); + // Modify subnet ids of some leases. + if (evenLeaseIndex(i)) { + setSubnetId(i, 2); + } + } + + // Leases will be reclaimed in groups of 8. + const size_t reclamation_group_size = 8; + for (int i = reclamation_group_size; i < TEST_LEASES_NUM; + i += reclamation_group_size) { + + // Reclaim 8 most expired leases out of TEST_LEASES_NUM. + ASSERT_NO_THROW(reclaimExpiredLeases(reclamation_group_size, + 0, false)); + + // Number of reclaimed leases should increase as we loop. + EXPECT_TRUE(testStatistics("reclaimed-leases", i)); + // Make sure that the number of reclaimed leases is also distributed + // across two subnets. + EXPECT_TRUE(testStatistics("subnet[1].reclaimed-leases", i / 2)); + EXPECT_TRUE(testStatistics("subnet[2].reclaimed-leases", i / 2)); + // Number of assigned leases should decrease as we reclaim them. + EXPECT_TRUE(testStatistics("subnet[1].assigned-addresses", + (TEST_LEASES_NUM - i) / 2)); + EXPECT_TRUE(testStatistics("subnet[2].assigned-addresses", + (TEST_LEASES_NUM - i) / 2)); + } +} + + // This test verifies that the leases can be reclaimed without being removed // from the database. In such case, the leases' state is set to // "expired-reclaimed". @@ -1039,4 +1275,10 @@ TEST_F(ExpirationAllocEngine4Test, reclaimExpiredLeasesWithDDNSAndClientId) { testReclaimExpiredLeasesWithDDNSAndClientId(); } +// This test verifies that statistics is correctly updated when the leases +// are reclaimed. +TEST_F(ExpirationAllocEngine4Test, reclaimExpiredLeasesStats) { + testReclaimExpiredLeasesStats(); +} + }; // end of anonymous namespace From d172fb500896515acd07fb9005453fb65e61ed69 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Thu, 10 Sep 2015 19:58:55 +0200 Subject: [PATCH 6/9] [3973] Updated descriptions of lease reclamation routines in alloc_engine.h --- src/lib/dhcpsrv/alloc_engine.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index e5f1c53212..b15d19cc8a 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -497,6 +497,14 @@ public: /// See http://kea.isc.org/wiki/LeaseExpirationDesign#LeasesReclamationRoutine /// for the details. /// + /// This method is executed periodically to act upon expired leases. This + /// includes for each lease: + /// - executing "lease_expire6" hook, + /// - removing DNS record for a lease, + /// - reclaiming a lease in the database, i.e. setting its state to + /// "expired-reclaimed" or removing it from the lease databse, + /// - updating statistics of assigned and reclaimed leases + /// /// @param max_leases Maximum number of leases to be reclaimed. /// @param timeout Maximum amount of time that the reclaimation routine /// may be processing expired leases, expressed in seconds. @@ -512,6 +520,14 @@ public: /// See http://kea.isc.org/wiki/LeaseExpirationDesign#LeasesReclamationRoutine /// for the details. /// + /// This method is executed periodically to act upon expired leases. This + /// includes for each lease: + /// - executing "lease_expire6" hook, + /// - removing DNS record for a lease, + /// - reclaiming a lease in the database, i.e. setting its state to + /// "expired-reclaimed" or removing it from the lease databse, + /// - updating statistics of assigned and reclaimed leases + /// /// @param max_leases Maximum number of leases to be reclaimed. /// @param timeout Maximum amount of time that the reclaimation routine /// may be processing expired leases, expressed in seconds. From 8b01bb335fbd105773be1d5364a7ad4d84ab6e59 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Thu, 10 Sep 2015 20:25:21 +0200 Subject: [PATCH 7/9] [3973] Explicit types in the base class constructor. --- src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc index 93ff635367..819e8fcfbe 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc @@ -792,7 +792,7 @@ public: }; ExpirationAllocEngine6Test::ExpirationAllocEngine6Test() - : ExpirationAllocEngineTest("type=memfile universe=6 persist=false") { + : ExpirationAllocEngineTest("type=memfile universe=6 persist=false") { createLeases(); } @@ -1033,7 +1033,7 @@ public: }; ExpirationAllocEngine4Test::ExpirationAllocEngine4Test() - : ExpirationAllocEngineTest("type=memfile universe=4 persist=false") { + : ExpirationAllocEngineTest("type=memfile universe=4 persist=false") { createLeases(); } From 2ead25cbea048aa1a01c773e878e07fcbe279cec Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Fri, 11 Sep 2015 09:34:37 +0200 Subject: [PATCH 8/9] [3973] Added todos for the lease reclamation routines. --- src/lib/dhcpsrv/alloc_engine.cc | 4 ++++ .../dhcpsrv/tests/alloc_engine_expiration_unittest.cc | 10 ++++++++++ 2 files changed, 14 insertions(+) diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index af97beb83f..35cf3c49be 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -1291,6 +1291,8 @@ AllocEngine::reclaimExpiredLeases6(const size_t max_leases, const uint16_t timeo // Create stopwatch and automatically start it to measure the time // taken by the routine. + /// @todo Monitor time elapsed and return from the lease reclamation routine + /// if it hits the timeout value. util::Stopwatch stopwatch; LeaseMgr& lease_mgr = LeaseMgrFactory::instance(); @@ -1371,6 +1373,8 @@ AllocEngine::reclaimExpiredLeases4(const size_t max_leases, const uint16_t timeo // Create stopwatch and automatically start it to measure the time // taken by the routine. + /// @todo Monitor time elapsed and return from the lease reclamation routine + /// if it hits the timeout value. util::Stopwatch stopwatch; LeaseMgr& lease_mgr = LeaseMgrFactory::instance(); diff --git a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc index 819e8fcfbe..3347553d00 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc @@ -146,6 +146,16 @@ struct UpperBound { /// "expired-reclaimed". /// /// See @c ExpirationAllocEngineTest::testLeases for further details. +/// +/// @todo These tests should be extended to cover the following cases: +/// - timeout value in the lease reclamation routines - the most reliable +/// way to test it will be by attaching a lease4_expire/lease6_expire +/// hooks which would block for a specific period of time. This will +/// allow for calculating the approximate number of reclaimed leases +/// within a given timeout. See ticket #3972. +/// - declined leases - declined leases expire and should be removed +/// from the lease database by the lease reclamation routine. See +/// ticket #3976. template class ExpirationAllocEngineTest : public ::testing::Test { public: From e588aa4a9f7a13fc3a3cbd09b4b52ef167470448 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Wed, 16 Sep 2015 14:02:31 +0200 Subject: [PATCH 9/9] [3973] Addressed review comments. --- src/lib/dhcpsrv/alloc_engine.cc | 4 +++- src/lib/dhcpsrv/alloc_engine.h | 4 ++-- src/lib/dhcpsrv/alloc_engine_messages.mes | 24 ++++++++++--------- .../tests/alloc_engine_expiration_unittest.cc | 13 +++++++--- 4 files changed, 28 insertions(+), 17 deletions(-) diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 35cf3c49be..44a648976e 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -1308,7 +1308,9 @@ AllocEngine::reclaimExpiredLeases6(const size_t max_leases, const uint16_t timeo // Generate removal name change request for D2, if required. // This will return immediatelly if the DNS wasn't updated // when the lease was created. - queueRemovalNameChangeRequest(lease, *(lease->duid_)); + if (lease->duid_) { + queueRemovalNameChangeRequest(lease, *(lease->duid_)); + } // Reclaim the lease - depending on the configuration, set the // expired-reclaimed state or simply remove it. diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index b15d19cc8a..7596eecae1 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -716,11 +716,11 @@ private: /// /// @param lease Pointer to a lease for which NCR should be sent. /// @param identifier Identifier to be used to generate DHCID for - /// the DNS update. For DHCPv4 it will be hardware address, client + /// the DNS update. For DHCPv4 it will be hardware address or client /// identifier. For DHCPv6 it will be a DUID. /// /// @tparam LeasePtrType Pointer to a lease. - /// @tparam Identifier HW Address, Client Identifier or DUID. + /// @tparam IdentifierType HW Address, Client Identifier or DUID. template void queueRemovalNameChangeRequest(const LeasePtrType& lease, const IdentifierType& identifier) const; diff --git a/src/lib/dhcpsrv/alloc_engine_messages.mes b/src/lib/dhcpsrv/alloc_engine_messages.mes index 4a60ae76b9..1a9f748220 100644 --- a/src/lib/dhcpsrv/alloc_engine_messages.mes +++ b/src/lib/dhcpsrv/alloc_engine_messages.mes @@ -16,12 +16,12 @@ $NAMESPACE isc::dhcp % ALLOC_ENGINE_LEASE_RECLAIMED successfully reclaimed lease %1 This debug message is logged when the allocation engine successfully -reclaims a lease. +reclaims a lease. The lease is now available for assignment. % ALLOC_ENGINE_REMOVAL_NCR_FAILED sending removal name change request failed for lease %1: %2 This error message is logged when sending a removal name change request -to D2 failed. This name change request is usually generated when the -lease reclamation routine acts upon expired leases. If a lease being +to DHCP DDNS failed. This name change request is usually generated when +the lease reclamation routine acts upon expired leases. If a lease being reclaimed has a corresponding DNS entry it needs to be removed. This message indicates that removal of the DNS entry has failed. Nevertheless the lease will be reclaimed. @@ -71,11 +71,12 @@ while performing the operation on the lease database. This debug message is logged when the allocation engine completes reclamation of a set of expired leases. The maximum number of leases to be reclaimed in a single pass of the lease reclamation routine -is configurable. However, the number of reclaimed leases may also -be limited by the timeout value. The message includes the number -of reclaimed leases and the total time. +is configurable using 'max-reclaim-leases' parameter. However, +the number of reclaimed leases may also be limited by the timeout +value, configured with 'max-reclaim-time'. The message includes the +number of reclaimed leases and the total time. -% ALLOC_ENGINE_V4_LEASES_RECLAMATION_START starting reclamation of expired leases (limit = %1 leases, timeout = %2 seconds) +% ALLOC_ENGINE_V4_LEASES_RECLAMATION_START starting reclamation of expired leases (limit = %1 leases or %2 seconds) This debug message is issued when the allocation engine starts the reclamation of the expired leases. The maximum number of leases to be reclaimed and the timeout is included in the message. If any of @@ -288,11 +289,12 @@ while performing the operation on the lease database. This debug message is logged when the allocation engine completes reclamation of a set of expired leases. The maximum number of leases to be reclaimed in a single pass of the lease reclamation routine -is configurable. However, the number of reclaimed leases may also -be limited by the timeout value. The message includes the number -of reclaimed leases and the total time. +is configurable using 'max-reclaim-leases' parameter. However, +the number of reclaimed leases may also be limited by the timeout +value, configured with 'max-reclaim-time'. The message includes the +number of reclaimed leases and the total time. -% ALLOC_ENGINE_V6_LEASES_RECLAMATION_START starting reclamation of expired leases (limit = %1 leases, timeout = %2 seconds) +% ALLOC_ENGINE_V6_LEASES_RECLAMATION_START starting reclamation of expired leases (limit = %1 leases or %2 seconds) This debug message is issued when the allocation engine starts the reclamation of the expired leases. The maximum number of leases to be reclaimed and the timeout is included in the message. If any of diff --git a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc index 3347553d00..797d171580 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc @@ -102,6 +102,8 @@ struct UpperBound { /// - DNS records for some of the leases must be removed when the lease /// is reclaimed and DNS updates are enabled, /// - hooks must be invoked (if installed) for each reclaimed lease +/// - statistics must be updated to increase the number of reclaimed +/// leases and decrease the number of allocated leases /// /// The typical test requires many leases to be initialized and stored /// in the lease database for the test. The test fixture class creates @@ -274,6 +276,9 @@ public: /// /// @param stat_name Statistic name. /// @param exp_value Expected value. + /// + /// @return true if the statistic manager holds a particular value, + /// false otherwise. bool testStatistics(const std::string& stat_name, const int64_t exp_value) const { try { ObservationPtr observation = StatsMgr::instance().getObservation(stat_name); @@ -389,7 +394,8 @@ public: /// from the lease. /// /// @param lease Pointer to lease. - /// @return true if lease state is "expired-reclaimed". + /// @return true if lease state is "expired-reclaimed" and the FQDN + /// information has been removed from the lease. static bool leaseReclaimed(const LeasePtrType& lease) { return (lease && lease->stateExpiredReclaimed() && lease->hostname_.empty() && !lease->fqdn_fwd_ && @@ -446,7 +452,7 @@ public: return (true); } - /// @brief Returns Name Change Request from the D2 client queue. + /// @brief Returns removal name change request from the D2 client queue. /// /// @param lease Pointer to the lease to be matched with NCR. /// @@ -458,7 +464,8 @@ public: for (size_t i = 0; i < mgr.getQueueSize(); ++i) { const NameChangeRequestPtr& ncr = mgr.peekAt(i); // If match found, return true. - if (ncr->getIpAddress() == lease->addr_.toText()) { + if ((ncr->getIpAddress() == lease->addr_.toText()) && + (ncr->getChangeType() == CHG_REMOVE)) { return (ncr); } }