From 664ac1eabaeea7fda4a53e2a2a32b21076bbb128 Mon Sep 17 00:00:00 2001 From: Tomek Mrugalski Date: Tue, 22 Sep 2015 20:36:47 +0200 Subject: [PATCH] [3984] Declined IPv4 leases reclaimation implemented --- src/lib/dhcpsrv/alloc_engine.cc | 56 +++++- src/lib/dhcpsrv/alloc_engine.h | 33 ++++ src/lib/dhcpsrv/alloc_engine_messages.mes | 6 + src/lib/dhcpsrv/lease.cc | 5 + src/lib/dhcpsrv/lease.h | 5 + .../dhcpsrv/tests/alloc_engine4_unittest.cc | 169 ++++++++++++++++ .../tests/alloc_engine_expiration_unittest.cc | 181 +++++++++++++++--- src/lib/dhcpsrv/tests/alloc_engine_utils.cc | 76 ++++++++ src/lib/dhcpsrv/tests/alloc_engine_utils.h | 55 +++++- .../tests/generic_lease_mgr_unittest.cc | 157 ++++++++++++++- .../tests/generic_lease_mgr_unittest.h | 10 + .../tests/memfile_lease_mgr_unittest.cc | 7 + 12 files changed, 729 insertions(+), 31 deletions(-) diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 44a648976e..e3ac0696ee 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -1402,9 +1402,22 @@ AllocEngine::reclaimExpiredLeases4(const size_t max_leases, const uint16_t timeo queueRemovalNameChangeRequest(lease, lease->hwaddr_); } + bool remove_tmp = remove_lease; + if (lease->state_ == Lease::STATE_DECLINED) { + // There's no point keep declined lease after its reclaimation. + // Declined lease doesn't have any client identifying information + // anymore. + remove_tmp = true; + + // Do extra steps required for declined lease reclaimation: + // - bump decline-related stats + // - log separate message + reclaimDeclined(lease); + } + // Reclaim the lease - depending on the configuration, set the // expired-reclaimed state or simply remove it. - reclaimLeaseInDatabase(lease, remove_lease, + reclaimLeaseInDatabase(lease, remove_tmp, boost::bind(&LeaseMgr::updateLease4, &lease_mgr, _1)); @@ -1442,6 +1455,36 @@ AllocEngine::reclaimExpiredLeases4(const size_t max_leases, const uint16_t timeo .arg(stopwatch.logFormatTotalDuration()); } +void +AllocEngine::reclaimDeclined(const Lease4Ptr& lease) { + + if (!lease || (lease->state_ != Lease::STATE_DECLINED) ) { + return; + } + + LOG_INFO(alloc_engine_logger, ALLOC_ENGINE_V4_DECLINED_RECOVERED). + arg(lease->addr_.toText()).arg(lease->valid_lft_); + + StatsMgr& stats_mgr = StatsMgr::instance(); + + // Decrease subnet specific counter for currently declined addresses + stats_mgr.addValue(StatsMgr::generateName("subnet", lease->subnet_id_, + "declined-addresses"), static_cast(-1)); + + // Decrease global counter for declined addresses + stats_mgr.addValue("declined-addresses", static_cast(-1)); + + stats_mgr.addValue("reclaimed-declined-addresses", static_cast(1)); + + stats_mgr.addValue(StatsMgr::generateName("subnet", lease->subnet_id_, + "reclaimed-declined-addresses"), static_cast(1)); + + // Note that we do not touch assigned-addresses counters. Those are + // modified in whatever code calls this method. + + /// @todo: call lease4_decline_recycle hook here. +} + template void AllocEngine::queueRemovalNameChangeRequest(const LeasePtrType& lease, @@ -2142,6 +2185,17 @@ AllocEngine::reuseExpiredLease4(Lease4Ptr& expired, isc_throw(BadValue, "null subnet specified for the reuseExpiredLease"); } + if ( (expired->state_ == Lease::STATE_DECLINED) && + (ctx.fake_allocation_ == false)) { + // If this is a declined lease that expired, we need to conduct + // extra steps for it. However, we do want to conduct those steps + // only once. In paricular, if we have an expired declined lease + // and client sent DHCPDISCOVER and will later send DHCPREQUEST, + // we only want to call this method once when responding to + // DHCPREQUEST (when the actual reclaimation takes place). + reclaimDeclined(expired); + } + updateLease4Information(expired, ctx); LOG_DEBUG(alloc_engine_logger, ALLOC_ENGINE_DBG_TRACE_DETAIL_DATA, diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index 7596eecae1..c0f5468c93 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -505,6 +505,29 @@ public: /// "expired-reclaimed" or removing it from the lease databse, /// - updating statistics of assigned and reclaimed leases /// + /// Note: declined leases fall under the same expiration/reclaimation + /// processing as normal leases. In principle, it would more elegant + /// to have a separate processing for declined leases reclaimation. However, + /// due to performance reasons we decided to use them together. Several + /// aspects were taken into consideration. First, normal leases are expected + /// to expire frequently, so in a typical deployment this method will have + /// some leases to process. Second, declined leases are expected to be very + /// rare event, so in most cases there won't be any declined expired leases. + /// Third, the calls to LeaseMgr to obtain all leases of specific expiration + /// criteria are expensive, so it is better to have one call rather than + /// two, especially if one of those calls is expected to usually return no + /// leases. + /// + /// It doesn't make sense to retain declined leases that are reclaimed, + /// because those leases don't contain any useful information (all client + /// identifying information was stripped when the leave was moved to the + /// declined state). Therefore remove_leases parameter is ignored for + /// declined leases. They are always removed. + /// + /// Also, for delined leases @ref reclaimDeclined is called. It conducts + /// several declined specific operation (extra log entry, stats dump, + /// hooks). + /// /// @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. @@ -749,6 +772,16 @@ private: const boost::function& lease_update_fun) const; + /// @brief Conducts steps necessary for reclaiming declined lease. + /// + /// These are the additional steps required when recoving a declined lease: + /// - bump decline recovered stat + /// - log lease recovery + /// - call hook (@todo) + /// + /// @param lease Lease to be reclaimed from Declined state + void reclaimDeclined(const Lease4Ptr& lease); + 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 1a9f748220..e490139ca0 100644 --- a/src/lib/dhcpsrv/alloc_engine_messages.mes +++ b/src/lib/dhcpsrv/alloc_engine_messages.mes @@ -48,6 +48,12 @@ consider reducing the lease lifetime. In this way, addresses allocated to clients that are no longer active on the network will become available sooner. +% ALLOC_ENGINE_V4_DECLINED_RECOVERED Address %1 was recovered after %2 seconds of probation-period +This informational message indicates that the specified address was reported +as duplicate (client sent DECLINE) and the server marked this address as +unvailable for a period of time. This time now has elapsed and the address +is now returned to available pool. This step concludes decline recovery process. + % ALLOC_ENGINE_V4_DISCOVER_ADDRESS_CONFLICT %1: conflicting reservation for address %2 with existing lease %3 This warning message is issued when the DHCP server finds that the address reserved for the client can't be offered because this address diff --git a/src/lib/dhcpsrv/lease.cc b/src/lib/dhcpsrv/lease.cc index 2320e51d74..712986f561 100644 --- a/src/lib/dhcpsrv/lease.cc +++ b/src/lib/dhcpsrv/lease.cc @@ -85,6 +85,11 @@ Lease::stateExpiredReclaimed() const { return (state_ == STATE_EXPIRED_RECLAIMED); } +bool +Lease::stateDeclined() const { + return (state_ == STATE_DECLINED); +} + int64_t Lease::getExpirationTime() const { return (static_cast(cltt_) + valid_lft_); diff --git a/src/lib/dhcpsrv/lease.h b/src/lib/dhcpsrv/lease.h index 18d19be29c..a8b65be51a 100644 --- a/src/lib/dhcpsrv/lease.h +++ b/src/lib/dhcpsrv/lease.h @@ -177,6 +177,11 @@ struct Lease { /// otherwise. bool stateExpiredReclaimed() const; + /// @brief Indicates if the lease is in the "declined" state. + /// + /// @return true if the lease is in the "declined" state, false otherwise. + bool stateDeclined() const; + /// @brief Returns true if the other lease has equal FQDN data. /// /// @param other Lease which FQDN data is to be compared with our lease. diff --git a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc index fa222cf45f..2168a64c09 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc @@ -553,6 +553,175 @@ TEST_F(AllocEngine4Test, requestReuseExpiredLease4) { EXPECT_TRUE(*ctx.old_lease_ == original_lease); } +// This test checks if an expired declined lease can be reused when responding +// to DHCPDISCOVER (fake allocation) +TEST_F(AllocEngine4Test, discoverReuseDeclinedLease4) { + + AllocEnginePtr engine(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, + 100, false)); + ASSERT_TRUE(engine); + + // Now prepare a configuration with single address pool. + IOAddress addr("192.0.2.15"); + CfgMgr& cfg_mgr = CfgMgr::instance(); + cfg_mgr.clear(); + subnet_ = Subnet4Ptr(new Subnet4(IOAddress("192.0.2.0"), 24, 1, 2, 3)); + pool_ = Pool4Ptr(new Pool4(addr, addr)); // just a single address + subnet_->addPool(pool_); + cfg_mgr.getStagingCfg()->getCfgSubnets4()->add(subnet_); + + // Now create a declined lease, decline it and rewind its cltt, so it + // is expired. + Lease4Ptr declined = generateDeclinedLease("192.0.2.15", 100, -10); + + // CASE 1: Ask for any address + Lease4Ptr assigned; + testReuseLease4(engine, declined, "0.0.0.0", true, SHOULD_PASS, assigned); + + // Check that we got that single lease + ASSERT_TRUE(assigned); + EXPECT_EQ(addr, assigned->addr_); + + // CASE 2: Asking specifically for this address + testReuseLease4(engine, declined, "192.0.2.15", true, SHOULD_PASS, assigned); + + // Check that we get it again + ASSERT_TRUE(assigned); + EXPECT_EQ(addr, assigned->addr_); +} + +// This test checks if statistics are not updated when expired declined lease +// is resued when responding to DHCPDISCOVER (fake allocation) +TEST_F(AllocEngine4Test, discoverReuseDeclinedLease4Stats) { + + // Now prepare for DISCOVER processing + AllocEnginePtr engine(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, + 100, false)); + ASSERT_TRUE(engine); + + // Now prepare a configuration with single address pool. + IOAddress addr("192.0.2.15"); + CfgMgr& cfg_mgr = CfgMgr::instance(); + cfg_mgr.clear(); + subnet_ = Subnet4Ptr(new Subnet4(IOAddress("192.0.2.0"), 24, 1, 2, 3)); + pool_ = Pool4Ptr(new Pool4(addr, addr)); // just a single address + subnet_->addPool(pool_); + cfg_mgr.getStagingCfg()->getCfgSubnets4()->add(subnet_); + + // Now create a declined lease, decline it and rewind its cltt, so it + // is expired. + Lease4Ptr declined = generateDeclinedLease("192.0.2.15", 100, -10); + + // Let's fix some global stats... + StatsMgr& stats_mgr = StatsMgr::instance(); + stats_mgr.setValue("declined-addresses", static_cast(1000)); + stats_mgr.setValue("reclaimed-declined-addresses", static_cast(1000)); + + // ...and subnet specific stats as well. + string stat1 = StatsMgr::generateName("subnet", subnet_->getID(), + "declined-addresses"); + string stat2 = StatsMgr::generateName("subnet", subnet_->getID(), + "reclaimed-declined-addresses"); + stats_mgr.setValue(stat1, static_cast(1000)); + stats_mgr.setValue(stat2, static_cast(1000)); + + // Ask for any address. There's only one address in the pool, so it doesn't + // matter much. + Lease4Ptr assigned; + testReuseLease4(engine, declined, "0.0.0.0", true, SHOULD_PASS, assigned); + + // Check that the stats were not modified + testStatistics("declined-addresses", 1000); + testStatistics("reclaimed-declined-addresses", 1000); + + testStatistics(stat1, 1000); + testStatistics(stat2, 1000); +} + +// This test checks if an expired declined lease can be reused when responding +// to REQUEST (actual allocation) +TEST_F(AllocEngine4Test, requestReuseDeclinedLease4) { + + AllocEnginePtr engine(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, + 100, false)); + ASSERT_TRUE(engine); + + // Now prepare a configuration with single address pool. + IOAddress addr("192.0.2.15"); + CfgMgr& cfg_mgr = CfgMgr::instance(); + cfg_mgr.clear(); + subnet_ = Subnet4Ptr(new Subnet4(IOAddress("192.0.2.0"), 24, 1, 2, 3)); + pool_ = Pool4Ptr(new Pool4(addr, addr)); // just a single address + subnet_->addPool(pool_); + cfg_mgr.getStagingCfg()->getCfgSubnets4()->add(subnet_); + + // Now create a declined lease, decline it and rewind its cltt, so it + // is expired. + Lease4Ptr declined = generateDeclinedLease("192.0.2.15", 100, -10); + + // Asking specifically for this address + Lease4Ptr assigned; + testReuseLease4(engine, declined, "192.0.2.15", false, SHOULD_PASS, assigned); + // Check that we got it. + ASSERT_TRUE(assigned); + EXPECT_EQ(addr, assigned->addr_); + + // Check that the lease is indeed updated in LeaseMgr + Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(addr); + ASSERT_TRUE(from_mgr); + + // Now check that the lease in LeaseMgr has the same parameters + detailCompareLease(assigned, from_mgr); +} + +// This test checks if statistics are not updated when expired declined lease +// is resued when responding to DHCPREQUEST (actual allocation) +TEST_F(AllocEngine4Test, requestReuseDeclinedLease4Stats) { + + AllocEnginePtr engine(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, + 100, false)); + ASSERT_TRUE(engine); + + // Now prepare a configuration with single address pool. + IOAddress addr("192.0.2.15"); + CfgMgr& cfg_mgr = CfgMgr::instance(); + cfg_mgr.clear(); + subnet_ = Subnet4Ptr(new Subnet4(IOAddress("192.0.2.0"), 24, 1, 2, 3)); + pool_ = Pool4Ptr(new Pool4(addr, addr)); // just a single address + subnet_->addPool(pool_); + cfg_mgr.getStagingCfg()->getCfgSubnets4()->add(subnet_); + + // Now create a declined lease, decline it and rewind its cltt, so it + // is expired. + Lease4Ptr declined = generateDeclinedLease("192.0.2.15", 100, -10); + + // Let's fix some global stats... + StatsMgr& stats_mgr = StatsMgr::instance(); + stats_mgr.setValue("declined-addresses", static_cast(1000)); + stats_mgr.setValue("reclaimed-declined-addresses", static_cast(1000)); + + // ...and subnet specific stats as well. + string stat1 = StatsMgr::generateName("subnet", subnet_->getID(), + "declined-addresses"); + string stat2 = StatsMgr::generateName("subnet", subnet_->getID(), + "reclaimed-declined-addresses"); + stats_mgr.setValue(stat1, static_cast(1000)); + stats_mgr.setValue(stat2, static_cast(1000)); + + // Asking specifically for this address + Lease4Ptr assigned; + testReuseLease4(engine, declined, "192.0.2.15", false, SHOULD_PASS, assigned); + // Check that we got it. + ASSERT_TRUE(assigned); + + // Check that the stats were modified + testStatistics("declined-addresses", 999); + testStatistics("reclaimed-declined-addresses", 1001); + + testStatistics(stat1, 999); + testStatistics(stat2, 1001); +} + // This test checks that the Allocation Engine correcly identifies the // existing client's lease in the lease database, using the client // identifier and HW address. diff --git a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc index 797d171580..87175f9b01 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc @@ -138,6 +138,7 @@ struct UpperBound { /// - leaseDoesntExist - lease removed from the database, /// - leaseReclaimed - lease exists but has reclaimed status, /// - leaseNotReclaimed - lease exists and is not in the reclaimed status, +/// - leaseDeclined - lease exists and is in declined state, /// - dnsUpdateGeneratedForLease - DNS updates generated for lease, /// - dnsUpdateNotGeneratedForLease - DNS updates not generated for lease /// @@ -242,6 +243,17 @@ public: ASSERT_NO_THROW(updateLease(lease_index)); } + /// @brief Declines specified lease + /// + /// Sets specified lease to declined state and sets its probation-period. + /// @param lease_index Index of the lease. + /// @param probation_time value of probation period to be set (in seconds) + void decline(const uint16_t lease_index, const time_t probation_time) { + ASSERT_GT(leases_.size(), lease_index); + leases_[lease_index]->decline(probation_time); + ASSERT_NO_THROW(updateLease(lease_index)); + } + /// @brief Updates lease in the lease database. /// /// @param lease_index Index of the lease. @@ -272,33 +284,6 @@ 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. - /// - /// @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); - 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 @@ -402,6 +387,14 @@ public: !lease->fqdn_rev_); } + /// @brief Lease algorithm checking if lease state is Declined. + /// + /// @param lease Pointer to lease. + /// @return true if lease state is "declined". + static bool leaseDeclined(const LeasePtrType& lease) { + return (lease && lease->stateDeclined()); + } + /// @brief Lease algorithm checking if lease state is not /// expired-reclaimed. /// @@ -734,6 +727,119 @@ public: EXPECT_TRUE(testLeases(&dnsUpdateGeneratedForLease, &oddLeaseIndex)); } + /// @brief Test that declined expired leases can be removed. + /// + /// This method allows controlling remove_leases parameter when calling + /// @ref AllocEngine::reclaimExpiredLeases4. This should not matter, as + /// the address affinity doesn't make sense for declined leases (they don't + /// have any useful information in them anymore), so AllocEngine should + /// remove them all the time. + /// + /// @param remove see description above + void testReclaimDeclined4(bool remove) { + for (int i = 0; i < TEST_LEASES_NUM; ++i) { + + // Mark leases with even indexes as expired. + if (evenLeaseIndex(i)) { + + // Mark lease as declined with 100 seconds of probation-period + // (i.e. lease is supposed to be off limits for 100 seconds) + decline(i, 100); + + // The higher the index, the more expired the lease. + expire(i, 10 + i); + } + } + + // Run leases reclamation routine on all leases. This should result + // in removing all leases with status = declined, i.e. all + // even leases should be gone. + ASSERT_NO_THROW(reclaimExpiredLeases(0, 0, remove)); + + // Leases with even indexes should not exist in the DB + EXPECT_TRUE(testLeases(&leaseDoesntExist, &evenLeaseIndex)); + } + + /// @brief Test that appropriate statistics are updated when + /// declined expired leases are processed by AllocEngine. + void testReclaimDeclined4Stats() { + + // Leases by default all belong to subnet_id_ = 1. Let's count the + // number of declined leases. + int subnet1_cnt = 0; + int subnet2_cnt = 0; + + // Let's move all leases to declined,expired state. + for (int i = 0; i < TEST_LEASES_NUM; ++i) { + + // Move the lease to declined state + decline(i, 100); + + // And expire it, so it will be reclaimed + expire(i, 10 + 1); + + // Move every other lease to subnet-id = 2. + if (evenLeaseIndex(i)) { + subnet1_cnt++; + } else { + subnet2_cnt++; + setSubnetId(i, 2); + } + } + + StatsMgr& stats_mgr = StatsMgr::instance(); + // Let's set the global statistic. Values are arbitrary and can + // be used to easily detect whether a given stat was decreased or + // increased. They are sufficiently high compared to number of leases + // to avoid any chances of going into negative. + stats_mgr.setValue("declined-addresses", static_cast(1000)); + + // Let's set global the counter for reclaimed declined addresses. + stats_mgr.setValue("reclaimed-declined-addresses", + static_cast(2000)); + + // And those subnet specific as well + stats_mgr.setValue(stats_mgr.generateName("subnet", 1, + "assigned-addresses"), int64_t(1000)); + stats_mgr.setValue(stats_mgr.generateName("subnet", 2, + "assigned-addresses"), int64_t(2000)); + + stats_mgr.setValue(stats_mgr.generateName("subnet", 1, + "reclaimed-declined-addresses"), int64_t(3000)); + stats_mgr.setValue(stats_mgr.generateName("subnet", 2, + "reclaimed-declined-addresses"), int64_t(4000)); + + stats_mgr.setValue(stats_mgr.generateName("subnet", 1, + "declined-addresses"), int64_t(10)); + stats_mgr.setValue(stats_mgr.generateName("subnet", 2, + "declined-addresses"), int64_t(20)); + + // 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)); + + // Declined-addresses should be decreased from its initial value (1000) + // for both recovered addresses from subnet1 and subnet2. + testStatistics("declined-addresses", 1000 - subnet1_cnt - subnet2_cnt); + + // The code should bump up global counter for reclaimed declined + // addresses. + testStatistics("reclaimed-declined-addresses", 2000 + subnet1_cnt + subnet2_cnt); + + // subnet[X].assigned-addresses should go down. Between the time + // of DHCPDECLINE reception and declined expired lease reclaimation, + // we count this address as assigned-addresses. We decrease assigned- + // addresses when we reclaim the lease, not when the packet is received. + // For explanation, see Duplicate Addresses (DHCPDECLINE support) + // section in the User's Guide or a comment in Dhcpv4Srv::declineLease. + testStatistics("subnet[1].assigned-addresses", 1000 - subnet1_cnt); + testStatistics("subnet[2].assigned-addresses", 2000 - subnet2_cnt); + + // subnet[X].reclaimed-declined-addresses should go up in each subnet + testStatistics("subnet[1].reclaimed-declined-addresses", 3000 + subnet1_cnt); + testStatistics("subnet[2].reclaimed-declined-addresses", 4000 + subnet1_cnt); + } + /// @brief Collection of leases created at construction time. std::vector leases_; @@ -1298,4 +1404,25 @@ TEST_F(ExpirationAllocEngine4Test, reclaimExpiredLeasesStats) { testReclaimExpiredLeasesStats(); } +/// This test verifies that @ref AllocEngine::reclaimExpiredLeases4 properly +/// handles declined leases that have expired in case when it is told to +/// remove leases. +TEST_F(ExpirationAllocEngine4Test, reclaimDeclined1) { + testReclaimDeclined4(true); +} + +/// This test verifies that @ref AllocEngine::reclaimExpiredLeases4 properly +/// handles declined leases that have expired in case when it is told to +/// not remove leases. This flag should not matter and declined expired +/// leases should always be removed. +TEST_F(ExpirationAllocEngine4Test, reclaimDeclined2) { + testReclaimDeclined4(false); +} + +/// This test verifies that statistics are modified correctly after +/// reclaim expired leases is called. +TEST_F(ExpirationAllocEngine4Test, reclaimDeclinedStats) { + testReclaimDeclined4Stats(); +} + }; // end of anonymous namespace diff --git a/src/lib/dhcpsrv/tests/alloc_engine_utils.cc b/src/lib/dhcpsrv/tests/alloc_engine_utils.cc index 53227c773d..402827e9f7 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_utils.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine_utils.cc @@ -47,6 +47,82 @@ namespace isc { namespace dhcp { namespace test { +bool testStatistics(const std::string& stat_name, const int64_t exp_value) { + 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); +} + +void +AllocEngine4Test::testReuseLease4(const AllocEnginePtr& engine, + Lease4Ptr& existing_lease, + const std::string& addr, + const bool fake_allocation, + ExpectedResult exp_result, + Lease4Ptr& result) { + ASSERT_TRUE(engine); + + if (existing_lease) { + // If old lease was specified, we need to add it to the database. + // Let's wipe any leases for that address (if any). We ignore + // any errors (previous lease may not exist) + LeaseMgrFactory::instance().deleteLease(existing_lease->addr_); + + // Let's add it. + ASSERT_TRUE(LeaseMgrFactory::instance().addLease(existing_lease)); + } + + // A client comes along, asking specifically for a given address + AllocEngine::ClientContext4 ctx(subnet_, clientid_, hwaddr_, + IOAddress(addr), false, false, + "", fake_allocation); + if (fake_allocation) { + ctx.query_.reset(new Pkt4(DHCPDISCOVER, 1234)); + } else { + ctx.query_.reset(new Pkt4(DHCPREQUEST, 1234)); + } + result = engine->allocateLease4(ctx); + + switch (exp_result) { + case SHOULD_PASS: + ASSERT_TRUE(result); + + checkLease4(result); + break; + + case SHOULD_FAIL: + ASSERT_FALSE(result); + break; + } +} + +Lease4Ptr +AllocEngine4Test::generateDeclinedLease(const std::string& addr, + time_t probation_period, + int32_t expired) { + HWAddrPtr hwaddr(new HWAddr()); + time_t now = time(NULL); + Lease4Ptr declined(new Lease4(addr, hwaddr, ClientIdPtr(), 495, + 100, 200, now, subnet_->getID())); + declined->decline(probation_period); + declined->cltt_ = time(NULL) - probation_period + expired; + return (declined); +} + AllocEngine6Test::AllocEngine6Test() { CfgMgr::instance().clear(); diff --git a/src/lib/dhcpsrv/tests/alloc_engine_utils.h b/src/lib/dhcpsrv/tests/alloc_engine_utils.h index 128c5060d7..6c9aa59de2 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_utils.h +++ b/src/lib/dhcpsrv/tests/alloc_engine_utils.h @@ -39,6 +39,19 @@ namespace test { /// alloc_engine6_unittest.cc - all unit-tests dedicated to IPv6 /// alloc_engine_hooks_unittest.cc - all unit-tests dedicated to hooks + +/// @brief Test that statistic manager holds a given value. +/// +/// This function may be used in many allocation tests and there's no +/// single base class for it. @todo consider moving it src/lib/util. +/// +/// @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); + /// @brief Allocation engine with some internal methods exposed class NakedAllocEngine : public AllocEngine { public: @@ -333,6 +346,12 @@ public: class AllocEngine4Test : public ::testing::Test { public: + /// @brief Specified expected result of a given operation + enum ExpectedResult { + SHOULD_PASS, + SHOULD_FAIL + }; + /// @brief Default constructor /// /// Sets clientid_, hwaddr_, subnet_, pool_ fields to example values @@ -366,7 +385,41 @@ public: } EXPECT_TRUE(*lease->hwaddr_ == *hwaddr_); /// @todo: check cltt - } + } + + /// @brief Generic test used for lease allocation and reuse + /// + /// This test inserts old_lease (if specified, may be null) into the + /// LeaseMgr, then conducts lease allocation (pretends that client + /// sent either Discover or Request, depending on fake_allocation). + /// Allocated lease is then returned (using result) for further inspection. + /// + /// @param alloc_engine allocation engine + /// @param existing_lease optional lease to be inserted in the database + /// @param addr address to be requested by client + /// @param exp_result expected result + /// @param result [out] allocated lease + void testReuseLease4(const AllocEnginePtr& alloc_engine, + Lease4Ptr& existing_lease, + const std::string& addr, + const bool fake_allocation, + ExpectedResult exp_result, + Lease4Ptr& result); + + /// @brief Creates a declined lease with specified expiration time + /// + /// expired parameter controls probation period. Positive value + /// means that the lease will expire in X seconds. Negative means + /// that the lease had expired X seconds ago. 0 means it expires now. + /// Probation period is a parameter that specifies for how long + /// a lease will become unavailable after decline. + /// + /// @param addr address of the lease + /// @param probation_period expressed in seconds + /// @param expired number of seconds where it will expire + Lease4Ptr generateDeclinedLease(const std::string& addr, + time_t probation_period, + int32_t expired); /// @brief Create a subnet with a specified pool of addresses. /// diff --git a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc index 43d2b7c98b..0e5ba491b9 100644 --- a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc @@ -1729,7 +1729,7 @@ GenericLeaseMgrTest::testGetExpiredLeases4() { // Remember expired leases returned. std::vector saved_expired_leases = expired_leases; - // Remove expired leases again. + // Remove expired leases again. expired_leases.clear(); // Limit the number of leases to be returned to 2. @@ -1849,7 +1849,7 @@ GenericLeaseMgrTest::testGetExpiredLeases6() { // Remember expired leases returned. std::vector saved_expired_leases = expired_leases; - // Remove expired leases again. + // Remove expired leases again. expired_leases.clear(); // Limit the number of leases to be returned to 2. @@ -2084,6 +2084,159 @@ GenericLeaseMgrTest::testDeleteExpiredReclaimedLeases6() { } } +void +GenericLeaseMgrTest::testGetDeclinedLeases4() { + // Get the leases to be used for the test. + vector leases = createLeases4(); + + // Make sure we have at least 8 leases there. + ASSERT_GE(leases.size(), 8); + + // Use the same current time for all leases. + time_t current_time = time(NULL); + + // Add them to the database + for (size_t i = 0; i < leases.size(); ++i) { + + // Mark the first half of the leases as DECLINED + if (i < leases.size()/2) { + // Mark as declined with 1000 seconds of probation-period + leases[i]->decline(1000); + } + + // Mark every other lease as expired. + if (i % 2 == 0) { + // Set client last transmission time to the value older than the + // valid lifetime to make it expired. The expiration time also + // depends on the lease index, so as we can later check that the + // leases are ordered by the expiration time. + leases[i]->cltt_ = current_time - leases[i]->valid_lft_ - 10 - i; + + } else { + // Set current time as cltt for remaining leases. These leases are + // not expired. + leases[i]->cltt_ = current_time; + } + + ASSERT_TRUE(lmptr_->addLease(leases[i])); + } + + // The leases are: + // 0 - declined, expired + // 1 - declined, not expired + // 2 - declined, expired + // 3 - declined, not expired + // 4 - default, expired + // 5 - default, not expired + // 6 - default, expired + // 7 - default, not expired + + // Retrieve at most 1000 expired leases. + Lease4Collection expired_leases; + ASSERT_NO_THROW(lmptr_->getExpiredLeases4(expired_leases, 1000)); + + // Leases with even indexes should be returned as expired. It shouldn't + // matter if the state is default or expired. The getExpiredLeases4 does + // not pay attention to state, just expiration timers. + ASSERT_EQ(static_cast(leases.size() / 2), expired_leases.size()); + + int declined_state = 0; + int default_state = 0; + + // The expired leases should be returned from the most to least expired. + // This matches the reverse order to which they have been added. + for (Lease4Collection::reverse_iterator lease = expired_leases.rbegin(); + lease != expired_leases.rend(); ++lease) { + int index = static_cast(std::distance(expired_leases.rbegin(), lease)); + // Multiple current index by two, because only leases with even indexes + // should have been returned. + EXPECT_EQ(leases[2 * index]->addr_, (*lease)->addr_); + + // Count leases in default and declined states + if ((*lease)->state_ == Lease::STATE_DEFAULT) { + default_state++; + } else if ((*lease)->state_ == Lease::STATE_DECLINED) { + declined_state++; + } + } + + // LeaseMgr is supposed to return both default and declined leases + EXPECT_NE(0, declined_state); + EXPECT_NE(0, default_state); + + // Update current time for the next test. + current_time = time(NULL); + // Also, remove expired leases collected during the previous test. + expired_leases.clear(); + + // This time let's reverse the expiration time and see if they will be returned + // in the correct order. + leases = createLeases4(); + for (int i = 0; i < leases.size(); ++i) { + + // Mark the second half of the leases as DECLINED + if (i >= leases.size()/2) { + // Mark as declined with 1000 seconds of probation-period + leases[i]->decline(1000); + } + + // Update the time of expired leases with even indexes. + if (i % 2 == 0) { + leases[i]->cltt_ = current_time - leases[i]->valid_lft_ - 1000 + i; + + } else { + // Make sure remaining leases remain unexpired. + leases[i]->cltt_ = current_time + 100; + } + ASSERT_NO_THROW(lmptr_->updateLease4(leases[i])); + } + + // Retrieve expired leases again. The limit of 0 means return all expired + // leases. + ASSERT_NO_THROW(lmptr_->getExpiredLeases4(expired_leases, 0)); + // The same leases should be returned. + ASSERT_EQ(static_cast(leases.size() / 2), expired_leases.size()); + + // This time leases should be returned in the non-reverse order. + declined_state = 0; + default_state = 0; + for (Lease4Collection::iterator lease = expired_leases.begin(); + lease != expired_leases.end(); ++lease) { + int index = static_cast(std::distance(expired_leases.begin(), lease)); + EXPECT_EQ(leases[2 * index]->addr_, (*lease)->addr_); + + // Count leases in default and declined states + if ((*lease)->state_ == Lease::STATE_DEFAULT) { + default_state++; + } else if ((*lease)->state_ == Lease::STATE_DECLINED) { + declined_state++; + } + } + + // Check that both declined and default state leases were returned. + EXPECT_NE(0, declined_state); + EXPECT_NE(0, default_state); + + // Remember expired leases returned. + std::vector saved_expired_leases = expired_leases; + + // Remove expired leases again. + expired_leases.clear(); + + // Limit the number of leases to be returned to 2. + ASSERT_NO_THROW(lmptr_->getExpiredLeases4(expired_leases, 2)); + + // Make sure we have exactly 2 leases returned. + ASSERT_EQ(2, expired_leases.size()); + + // Test that most expired leases have been returned. + for (Lease4Collection::iterator lease = expired_leases.begin(); + lease != expired_leases.end(); ++lease) { + int index = static_cast(std::distance(expired_leases.begin(), lease)); + EXPECT_EQ(leases[2 * index]->addr_, (*lease)->addr_); + } +} + }; // namespace test }; // namespace dhcp }; // namespace isc diff --git a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h index 320c347b72..581bd76d26 100644 --- a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h +++ b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h @@ -285,6 +285,16 @@ public: /// - reclaimed leases are not returned. void testGetExpiredLeases6(); + /// @brief Checks that declined DHCPv4 leases that have expired can be retrieved. + /// + /// This test checks that the following: + /// - all expired and not reclaimed leases are returned, regardless if + /// they're normal or declined + /// - the order in which they're updated in LeaseMgr doesn't matter + /// - leases are returned in the order from most expired to the least + /// expired + void testGetDeclinedLeases4(); + /// @brief Checks that selected expired-reclaimed DHCPv6 leases /// are removed. /// diff --git a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc index 1f3f023ae7..d6fbd5a258 100644 --- a/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc @@ -1024,6 +1024,13 @@ TEST_F(MemfileLeaseMgrTest, versionCheck) { LeaseMgrFactory::destroy(); } +// Checks that declined leases can be returned correctly. +TEST_F(MemfileLeaseMgrTest, getDeclined4) { + + startBackend(V4); + testGetDeclinedLeases4(); +} + // This test checks that the backend reads DHCPv4 lease data from multiple // files. TEST_F(MemfileLeaseMgrTest, load4MultipleLeaseFiles) {