[master] Merge branch 'trac3984' (v4 declined lease recovery)

Conflicts:
	src/lib/dhcpsrv/alloc_engine.cc
	src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc

src/lib/dhcpsrv/alloc_engine.cc

@@ -1465,9 +1465,25 @@ AllocEngine::reclaimExpiredLeases4(const size_t max_leases, const uint16_t timeo
                     queueRemovalNameChangeRequest(lease, lease->hwaddr_);
                 }
 
+                // Let's check if the lease that just expired is in DECLINED state.
+                // If it is, we need to conduct couple extra steps and also force
+                // its removal.
+                bool remove_tmp = remove_lease;
+                if (lease->state_ == Lease::STATE_DECLINED) {
+                    // There's no point in keeping 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<Lease4Ptr>(lease, remove_lease,
+                reclaimLeaseInDatabase<Lease4Ptr>(lease, remove_tmp,
                                                   boost::bind(&LeaseMgr::updateLease4,
                                                               &lease_mgr, _1));
             }
@@ -1518,6 +1534,37 @@ 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<int64_t>(-1));
+
+    // Decrease global counter for declined addresses
+    stats_mgr.addValue("declined-addresses", static_cast<int64_t>(-1));
+
+    stats_mgr.addValue("reclaimed-declined-addresses", static_cast<int64_t>(1));
+
+    stats_mgr.addValue(StatsMgr::generateName("subnet", lease->subnet_id_,
+        "reclaimed-declined-addresses"), static_cast<int64_t>(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<typename LeasePtrType, typename IdentifierType>
 void
 AllocEngine::queueRemovalNameChangeRequest(const LeasePtrType& lease,
@@ -2218,6 +2265,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,

src/lib/dhcpsrv/alloc_engine.h

@@ -528,6 +528,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 milliseconds.
@@ -749,6 +772,16 @@ private:
                                 const boost::function<void (const LeasePtrType&)>&
                                 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.

src/lib/dhcpsrv/alloc_engine_messages.mes

@@ -48,6 +48,13 @@ 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
+has been returned to the 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

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<int64_t>(cltt_) + valid_lft_);

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.

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 reused 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<int64_t>(1000));
+    stats_mgr.setValue("reclaimed-declined-addresses", static_cast<int64_t>(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<int64_t>(1000));
+    stats_mgr.setValue(stat2, static_cast<int64_t>(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 reused 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<int64_t>(1000));
+    stats_mgr.setValue("reclaimed-declined-addresses", static_cast<int64_t>(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<int64_t>(1000));
+    stats_mgr.setValue(stat2, static_cast<int64_t>(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.

src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc

@@ -147,6 +147,7 @@ std::string callout_argument_name("lease4");
 /// - 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
 ///
@@ -249,6 +250,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.
@@ -279,33 +291,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
@@ -409,6 +394,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.
     ///
@@ -912,6 +905,119 @@ public:
                                UpperBound(TEST_LEASES_NUM)));
     }
 
+    /// @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 (unsigned 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 (unsigned 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<int64_t>(1000));
+
+        // Let's set global the counter for reclaimed declined addresses.
+        stats_mgr.setValue("reclaimed-declined-addresses",
+                           static_cast<int64_t>(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<LeasePtrType> leases_;
 
@@ -1541,4 +1647,25 @@ TEST_F(ExpirationAllocEngine4Test, reclaimExpiredLeasesShortTimeout) {
     testReclaimExpiredLeasesTimeout(1);
 }
 
+/// 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

src/lib/dhcpsrv/tests/alloc_engine_utils.cc

@@ -47,6 +47,84 @@ 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 an existing 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) {
+    // There's an assumption that hardware address is always present for IPv4
+    // packet (always non-null). Client-id is optional (may be null).
+    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_ = now - probation_period + expired;
+    return (declined);
+}
+
 AllocEngine6Test::AllocEngine6Test() {
     CfgMgr::instance().clear();
 

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,42 @@ public:
         }
         EXPECT_TRUE(*lease->hwaddr_ == *hwaddr_);
         /// @todo: check cltt
-     }
+    }
+
+    /// @brief Generic test used for lease allocation and reuse
+    ///
+    /// This test inserts existing_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 fake_allocation true = DISCOVER, false = REQUEST
+    /// @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 expired X seconds ago. 0 means it expires now.
+    /// Probation period is a parameter that specifies for how long
+    /// a lease will stay 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.
     ///

src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc

@@ -1691,6 +1691,7 @@ GenericLeaseMgrTest::testGetExpiredLeases4() {
         int index = static_cast<int>(std::distance(expired_leases.rbegin(), lease));
         // Multiple current index by two, because only leases with even indexes
         // should have been returned.
+        ASSERT_LE(2 * index, leases.size());
         EXPECT_EQ(leases[2 * index]->addr_, (*lease)->addr_);
     }
 
@@ -1723,13 +1724,14 @@ GenericLeaseMgrTest::testGetExpiredLeases4() {
     for (Lease4Collection::iterator lease = expired_leases.begin();
          lease != expired_leases.end(); ++lease) {
         int index = static_cast<int>(std::distance(expired_leases.begin(), lease));
+        ASSERT_LE(2 * index, leases.size());
         EXPECT_EQ(leases[2 * index]->addr_, (*lease)->addr_);
     }
 
     // Remember expired leases returned.
     std::vector<Lease4Ptr> 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.
@@ -1742,6 +1744,7 @@ GenericLeaseMgrTest::testGetExpiredLeases4() {
     for (Lease4Collection::iterator lease = expired_leases.begin();
          lease != expired_leases.end(); ++lease) {
         int index = static_cast<int>(std::distance(expired_leases.begin(), lease));
+        ASSERT_LE(2 * index, leases.size());
         EXPECT_EQ(leases[2 * index]->addr_, (*lease)->addr_);
     }
 
@@ -1849,7 +1852,7 @@ GenericLeaseMgrTest::testGetExpiredLeases6() {
     // Remember expired leases returned.
     std::vector<Lease6Ptr> 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 +2087,159 @@ GenericLeaseMgrTest::testDeleteExpiredReclaimedLeases6() {
     }
 }
 
+void
+GenericLeaseMgrTest::testGetDeclinedLeases4() {
+    // Get the leases to be used for the test.
+    vector<Lease4Ptr> 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 second 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<size_t>(leases.size() / 2), expired_leases.size());
+
+    unsigned int declined_state = 0;
+    unsigned 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<int>(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<size_t>(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<int>(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<Lease4Ptr> 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<int>(std::distance(expired_leases.begin(), lease));
+        EXPECT_EQ(leases[2 * index]->addr_, (*lease)->addr_);
+    }
+}
+
 }; // namespace test
 }; // namespace dhcp
 }; // namespace isc

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.
     ///

src/lib/dhcpsrv/tests/memfile_lease_mgr_unittest.cc

@@ -992,6 +992,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) {