[#1418] Addressed first comments

diff --git a/doc/sphinx/arm/dhcp4-srv.rst b/doc/sphinx/arm/dhcp4-srv.rst
index a2f021a0b5ec79c6fe3c166e5045d65efc851f0c..2a43fe830659d940d6b01b07e60242ec7c5fdb07 100644 (file)
--- a/doc/sphinx/arm/dhcp4-srv.rst
+++ b/doc/sphinx/arm/dhcp4-srv.rst
@@ -3998,12 +3998,12 @@ Lease Caching
 -------------
 
 Clients that attempt renewal frequently can cause the server to update
-and write the database frequently resulting in a performance impact on
-the server. The cache parameters instructs the DHCP server to avoid
+and write to the database frequently resulting in a performance impact
+on the server. The cache parameters instruct the DHCP server to avoid
 updating leases too frequently thus avoiding this behavior. Instead
 the server assigns the same lease (i.e. reuses it) with no
-modifications except for CLTT (Client Last Transmission Time)
-which does not require disk operations.
+modifications except for CLTT (Client Last Transmission Time) which
+does not require disk operations.
 
 The two parameters are the ``cache-threshold`` double and the
 ``cache-max-age`` integer and have no default, i.e. the lease caching
@@ -4019,7 +4019,7 @@ as last resort. For example:
             "pools": [ { "pool":  "192.0.2.1 - 192.0.2.200" } ],
             "subnet": "192.0.2.0/24",
             "cache-threshold": .25,
-           "cache-max-age": 600,
+            "cache-max-age": 600,
             "valid-lifetime": 2000,
             ...
         }
@@ -4040,16 +4040,17 @@ When an already assigned lease can fulfill a client query:
   - if the ``cache-threshold`` is configured and is between 0. and 1.
     it expresses the percentage of the lease valid lifetime which is
     allowed for the lease age. Values below and including 0. and
-    values greater than 1. disables the lease cache feature.
-    
+    values greater than 1. disable the lease cache feature.
+
 In the example a lease with a valid lifetime of 2000 seconds can be
 reused if it was committed less than 500 seconds ago. With a lifetime
 of 3000 seconds the maximum age of 600 seconds applies.
 
-In responses the ``dhcp-lease-time`` option is set to the remaining
-valid lifetime i.e. the expiration date does not change. Other options
-based on the valid lifetime e.g. ``dhcp-renewal-time`` and
-``dhcp-rebinding-time`` also use the remaining lifetime.
+In outbound client responses (e.g. DHCPACK messages) the
+``dhcp-lease-time`` option is set to the remaining valid lifetime
+i.e. the expiration date does not change. Other options based on the
+valid lifetime e.g. ``dhcp-renewal-time`` and ``dhcp-rebinding-time``
+also use the remaining lifetime.
 
 .. _host-reservation-v4:
 

diff --git a/doc/sphinx/arm/dhcp6-srv.rst b/doc/sphinx/arm/dhcp6-srv.rst
index 1268a63f5719de7d44a5bc85fb44da34d3da8d70..e921a8f101a83c31cf23cdb2d8e0ff8ad12c9401 100644 (file)
--- a/doc/sphinx/arm/dhcp6-srv.rst
+++ b/doc/sphinx/arm/dhcp6-srv.rst
@@ -3471,12 +3471,12 @@ Lease Caching
 -------------
 
 Clients that attempt renewal frequently can cause the server to update
-and write the database frequently resulting in a performance impact on
-the server. The cache parameters instructs the DHCP server to avoid
+and write to the database frequently resulting in a performance impact
+on the server. The cache parameters instruct the DHCP server to avoid
 updating leases too frequently thus avoiding this behavior. Instead
 the server assigns the same lease (i.e. reuses it) with no
-modifications except for CLTT (Client Last Transmission Time)
-which does not require disk operations.
+modifications except for CLTT (Client Last Transmission Time) which
+does not require disk operations.
 
 The two parameters are the ``cache-threshold`` double and the
 ``cache-max-age`` integer and have no default, i.e. the lease caching
@@ -3492,7 +3492,7 @@ as last resort. For example:
             "subnet": "2001:db8:1:1::/64",
             "pools": [ { "pool": "2001:db8:1:1::1:0/112" } ],
             "cache-threshold": .25,
-           "cache-max-age": 600,
+            "cache-max-age": 600,
             "valid-lifetime": 2000,
             ...
         }
@@ -3513,14 +3513,15 @@ When an already assigned lease can fulfill a client query:
   - if the ``cache-threshold`` is configured and is between 0. and 1.
     it expresses the percentage of the lease valid lifetime which is
     allowed for the lease age. Values below and including 0. and
-    values greater than 1. disables the lease cache feature.
-    
+    values greater than 1. disable the lease cache feature.
+
 In the example a lease with a valid lifetime of 2000 seconds can be
 reused if it was committed less than 500 seconds ago. With a lifetime
 of 3000 seconds the maximum age of 600 seconds applies.
 
-In responses used preferred and valid lifetimes are the remaining
-values i.e. the expiration dates do not change.
+In outbound client responses (e.g. DHCPV6_REPLY messages) used
+preferred and valid lifetimes are the remaining values i.e. the
+expiration dates do not change.
 
 .. _host-reservation-v6:
 

diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc
index 652db2e3d51cd6757e49cb03b46784d752dea345..01cd7547392b760c7f60560e44ddf28db5e4fbd3 100644 (file)
--- a/src/lib/dhcpsrv/alloc_engine.cc
+++ b/src/lib/dhcpsrv/alloc_engine.cc
@@ -2139,7 +2139,7 @@ AllocEngine::extendLease6(ClientContext6& ctx, Lease6Ptr lease) {
     Lease6Ptr old_data(new Lease6(*lease));
 
     bool changed = false;
-    lease->remaining_preferred_lft_ = lease->preferred_lft_;
+    uint32_t current_preferred_lft = lease->preferred_lft_;
     if (!ctx.currentIA().hints_.empty() &&
         ctx.currentIA().hints_[0].getPreferred()) {
         uint32_t preferred = ctx.currentIA().hints_[0].getPreferred();
@@ -2147,7 +2147,7 @@ AllocEngine::extendLease6(ClientContext6& ctx, Lease6Ptr lease) {
     } else {
         lease->preferred_lft_ = ctx.subnet_->getPreferred();
     }
-    if (lease->preferred_lft_ < lease->remaining_preferred_lft_) {
+    if (lease->preferred_lft_ < current_preferred_lft) {
         changed = true;
     }
     lease->remaining_valid_lft_ = 0;
@@ -2254,7 +2254,7 @@ AllocEngine::extendLease6(ClientContext6& ctx, Lease6Ptr lease) {
 
         // Try to reuse the lease.
         if (!changed) {
-            setLeaseRemainingLife(lease, ctx);
+            setLeaseRemainingLife(lease, current_preferred_lft, ctx);
         }
 
 
@@ -2331,8 +2331,8 @@ AllocEngine::updateLeaseData(ClientContext6& ctx, const Lease6Collection& leases
 
             lease->cltt_ = time(NULL);
             if (!fqdn_changed) {
-                lease->remaining_preferred_lft_ = lease->preferred_lft_;
-                setLeaseRemainingLife(lease, ctx);
+                uint32_t current_preferred_lft = lease->preferred_lft_;
+                setLeaseRemainingLife(lease, current_preferred_lft, ctx);
             }
             if (lease->remaining_valid_lft_ == 0) {
                 ctx.currentIA().changed_leases_.push_back(*lease_it);
@@ -4538,9 +4538,11 @@ AllocEngine::setLeaseRemainingLife(const Lease4Ptr& lease,
 
 void
 AllocEngine::setLeaseRemainingLife(const Lease6Ptr& lease,
+                                   uint32_t current_preferred_lft,
                                    const ClientContext6& ctx) const {
     // Sanity.
     lease->remaining_valid_lft_ = 0;
+    lease->remaining_preferred_lft_ = 0;
     const Subnet6Ptr& subnet = ctx.subnet_;
     if (!subnet) {
         return;
@@ -4587,11 +4589,12 @@ AllocEngine::setLeaseRemainingLife(const Lease6Ptr& lease,
     }
 
     // Seems to be reusable.
-    if ((lease->remaining_preferred_lft_ == Lease::INFINITY_LFT) ||
-        (lease->remaining_preferred_lft_ == 0)) {
+    if ((current_preferred_lft == Lease::INFINITY_LFT) ||
+        (current_preferred_lft == 0)) {
         // Keep these values.
-    } else if (lease->remaining_preferred_lft_ > age) {
-        lease->remaining_preferred_lft_ -= age;
+        lease->remaining_preferred_lft_ = current_preferred_lft;
+    } else if (current_preferred_lft > age) {
+        lease->remaining_preferred_lft_ = current_preferred_lft - age;
     } else {
         // Can be a misconfiguration so stay safe...
         return;

diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h
index 6203478cf71cf1434834401c536cfbd139e7bd8e..f64eac151c23fc9b99b30a64bafb884f06763b74 100644 (file)
--- a/src/lib/dhcpsrv/alloc_engine.h
+++ b/src/lib/dhcpsrv/alloc_engine.h
@@ -1899,14 +1899,11 @@ private:
     ///  - the lease is not updated in the lease database.
     ///  - the previous value of the lease can be returned to the client.
     ///
-    /// @note: there is no current_preferred_lft_ field in the lease
-    /// so the remaining_preferred_lft_ is used too for this:
-    ///  - it must be set to the previous preferred lifetime before call.
-    ///  - after call it must be ignored if remaining valid lifetime is zero.
-    ///
     /// @param [in,out] lease A pointer to the lease to be updated.
+    /// @param current_preferred_lft Current preferred lease lifetime.
     /// @param subnet A pointer to the lease subnet.
     void setLeaseRemainingLife(const Lease6Ptr& lease,
+                               uint32_t current_preferred_lft,
                                const ClientContext6& ctx) const;
 
 private:

diff --git a/src/lib/dhcpsrv/lease.h b/src/lib/dhcpsrv/lease.h
index 7def545356f2a984b0feefe72efa59f39e8ebd6b..25ca007ff633195646e8e9050b07bfd41f5f42bf 100644 (file)
--- a/src/lib/dhcpsrv/lease.h
+++ b/src/lib/dhcpsrv/lease.h
@@ -132,6 +132,7 @@ struct Lease : public isc::data::UserContext, public isc::data::CfgToElement {
     ///
     /// Expressed as number of seconds since current time, also
     /// valid lifetime - age where age is old cltt - new cltt.
+    /// The value 0 is used for the "cannot be reused" condition.
     uint32_t remaining_valid_lft_;
 
     /// @brief Client last transmission time
@@ -531,6 +532,8 @@ struct Lease6 : public Lease {
     ///
     /// Expressed as number of seconds since current time, also
     /// preferred lifetime - age where age is old cltt - new cltt.
+    /// This parameter is used only when remaining_valid_lft_ is not zero,
+    /// i.e. when the lease can be reused.
     uint32_t remaining_preferred_lft_;
 
     /// @todo: Add DHCPv6 failover related fields here

diff --git a/src/lib/dhcpsrv/tests/alloc_engine_utils.h b/src/lib/dhcpsrv/tests/alloc_engine_utils.h
index fd00d796c9a68c0edd8d8ba94bdaadfd2528b5b3..6c812734fc319b6d28f4b8d9f32c50c873da8767 100644 (file)
--- a/src/lib/dhcpsrv/tests/alloc_engine_utils.h
+++ b/src/lib/dhcpsrv/tests/alloc_engine_utils.h
@@ -93,7 +93,7 @@ public:
     /// @brief Wrapper method for invoking AllocEngine4::updateLease4ExtendedInfo().
     /// @param lease lease to update
     /// @param ctx current packet processing context
-    /// @return the changed returned value
+    /// @return true if extended information was changed
     bool callUpdateLease4ExtendedInfo(const Lease4Ptr& lease,
                                       AllocEngine::ClientContext4& ctx) const {
         return (updateLease4ExtendedInfo(lease, ctx));
@@ -102,7 +102,7 @@ public:
     /// @brief Wrapper method for invoking AllocEngine6::updateLease6ExtendedInfo().
     /// @param lease lease to update
     /// @param ctx current packet processing context
-    /// @return the changed returned value
+    /// @return true if extended information was changed
     bool callUpdateLease6ExtendedInfo(const Lease6Ptr& lease,
                                       AllocEngine::ClientContext6& ctx) const {
         return (updateLease6ExtendedInfo(lease, ctx));