[3973] Addressed review comments.

diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc
index 35cf3c49be86301d9ca5bf609f6912034d41853f..44a648976e9b62aa79293670e8439cd45d91e473 100644 (file)
--- 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 b15d19cc8a49f84fb7ffc5a4ed3a50f035f5322e..7596eecae138cb71307044be7c77dee417360023 100644 (file)
--- 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<typename LeasePtrType, typename IdentifierType>
     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 4a60ae76b908a877e47f9840916faac7b076c5b5..1a9f748220ac14d26eb5eddbad75105e88f29c61 100644 (file)
--- 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 3347553d0037ae50a9ab2382969051ba1f7a51aa..797d171580ad18c53c177384aaca23248da8865d 100644 (file)
--- 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);
             }
         }