From: Francis Dupont Date: Sat, 14 Nov 2020 15:46:27 +0000 (+0100) Subject: [#1418] Addressed first comments X-Git-Tag: Kea-1.9.4~94 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b27118b5f89dea1ddeaae42920294ffa6853d1c6;p=thirdparty%2Fkea.git [#1418] Addressed first comments --- diff --git a/doc/sphinx/arm/dhcp4-srv.rst b/doc/sphinx/arm/dhcp4-srv.rst index a2f021a0b5..2a43fe8306 100644 --- 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 1268a63f57..e921a8f101 100644 --- 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 652db2e3d5..01cd754739 100644 --- 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 6203478cf7..f64eac151c 100644 --- 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 7def545356..25ca007ff6 100644 --- 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 fd00d796c9..6c812734fc 100644 --- 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));