[3985] Comments updated after review.

diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc
index 630ea401cb91f79c30d86445e73493e9027f252a..a05c770ddc825f36e3305e3d406553380bd4bee4 100644 (file)
--- a/src/bin/dhcp6/dhcp6_srv.cc
+++ b/src/bin/dhcp6/dhcp6_srv.cc
@@ -2746,6 +2746,14 @@ Dhcpv6Srv::setStatusCode(boost::shared_ptr<isc::dhcp::Option6IA>& container,
 void
 Dhcpv6Srv::declineLease(const Pkt6Ptr& decline, const Lease6Ptr lease,
                         boost::shared_ptr<Option6IA> ia_rsp) {
+    // We do not want to decrease the assigned-nas at this time. While
+    // technically a declined address is no longer allocated, the primary usage
+    // of the assigned-addresses statistic is to monitor pool utilization. Most
+    // people would forget to include declined-addresses in the calculation,
+    // and simply do assigned-addresses/total-addresses. This would have a bias
+    // towards under-representing pool utilization, if we decreased allocated
+    // immediately after receiving DHCPDECLINE, rather than later when we recover
+    // the address.
 
     // Check if a lease has flags indicating that the FQDN update has
     // been performed. If so, create NameChangeRequest which removes

diff --git a/src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc
index 95bc2f00eba97c46d200a8affa71306476a06aba..60b41f20b1a2ef1d59408adcb5d79bc0dcf2caa2 100644 (file)
--- a/src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc
+++ b/src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc
@@ -1767,10 +1767,10 @@ TEST_F(AllocEngine6Test, solicitReuseDeclinedLease6Stats) {
 }
 
 // This test checks if statistics are not updated when expired declined lease
-// is reused when responding to REQUEST (fake allocation)
+// is reused when responding to REQUEST (actual allocation)
 TEST_F(AllocEngine6Test, requestReuseDeclinedLease6Stats) {
 
-    // Now prepare for SOLICIT processing
+    // Prepare for REQUEST processing.
     AllocEnginePtr engine(new AllocEngine(AllocEngine::ALLOC_ITERATIVE,
                                           100, true));
     ASSERT_TRUE(engine);

diff --git a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc
index dedcad1024adb54fc9339905c736665a95de484f..96de08bf16a9d4b6ceb9facfd8a77b5e1df7f6a0 100644 (file)
--- a/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc
+++ b/src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc
@@ -943,10 +943,12 @@ public:
     ///        declined expired leases are processed by AllocEngine.
     ///
     /// This method works for both v4 and v6. Just make sure the correct
-    /// statistic name is passed.
+    /// statistic name is passed. This is the name of the assigned addresses,
+    /// that is expected to be decreased once the reclaimation procedure
+    /// is complete.
     ///
-    /// @param stat_name name of the statistic for declined addresses statistic
-    ///        ("declined-addresses" for v4 and "declined-nas" for v6)
+    /// @param stat_name name of the statistic for assigned addresses statistic
+    ///        ("assgined-addresses" for both v4 and "assigned-nas" for v6)
     void testReclaimDeclinedStats(const std::string& stat_name) {
 
         // Leases by default all belong to subnet_id_ = 1. Let's count the
@@ -973,6 +975,7 @@ public:
         }
 
         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
@@ -1012,11 +1015,13 @@ public:
         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.
+        // of DHCPDECLINE(v4)/DECLINE(v6) reception and declined expired lease
+        // reclaimation, we count this address as assigned-addresses. We decrease
+        // assigned-addresses(v4)/assgined-nas(v6) when we reclaim the lease,
+        // not when the packet is received. For explanation, see Duplicate
+        // Addresses (DHCPDECLINE support) (v4) or Duplicate Addresses (DECLINE
+        // support) sections in the User's Guide or a comment in
+        // Dhcpv4Srv::declineLease or Dhcpv6Srv::declineLease.
         testStatistics("subnet[1]." + stat_name, 1000 - subnet1_cnt);
         testStatistics("subnet[2]." + stat_name, 2000 - subnet2_cnt);
 
@@ -1293,14 +1298,14 @@ TEST_F(ExpirationAllocEngine6Test, reclaimExpiredLeasesShortTimeout) {
     testReclaimExpiredLeasesTimeout(1);
 }
 
-/// This test verifies that @ref AllocEngine::reclaimExpiredLeases4 properly
+/// This test verifies that @ref AllocEngine::reclaimExpiredLeases6 properly
 /// handles declined leases that have expired in case when it is told to
 /// remove leases.
 TEST_F(ExpirationAllocEngine6Test, reclaimDeclined1) {
     testReclaimDeclined(true);
 }
 
-/// This test verifies that @ref AllocEngine::reclaimExpiredLeases4 properly
+/// This test verifies that @ref AllocEngine::reclaimExpiredLeases6 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.

diff --git a/src/lib/dhcpsrv/tests/alloc_engine_utils.h b/src/lib/dhcpsrv/tests/alloc_engine_utils.h
index 2d3f84231decf8a2779f2978c40487dffeffbe9f..e1e35a083c4353dedd8fc599fb59fd3c4bc0cb9e 100644 (file)
--- a/src/lib/dhcpsrv/tests/alloc_engine_utils.h
+++ b/src/lib/dhcpsrv/tests/alloc_engine_utils.h
@@ -295,7 +295,7 @@ public:
     /// @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 fake_allocation true = SOLICIT, false = REQUEST
     /// @param exp_result expected result
     /// @param result [out] allocated lease
     void testReuseLease6(const AllocEnginePtr& alloc_engine,
@@ -428,7 +428,7 @@ public:
         /// @todo: check cltt
     }
 
-    /// @brief Generic test used for lease allocation and reuse
+    /// @brief Generic test used for IPv4 lease allocation and reuse
     ///
     /// This test inserts existing_lease (if specified, may be null) into the
     /// LeaseMgr, then conducts lease allocation (pretends that client