[3564] Improved commentary for the allocation engine.

src/lib/dhcpsrv/alloc_engine.cc

@@ -494,23 +494,28 @@ AllocEngine::allocateLease4(const SubnetPtr& subnet, const ClientIdPtr& clientid
 
     try {
 
+        // Set allocator.
         AllocatorPtr allocator = getAllocator(Lease::TYPE_V4);
 
-        // Allocator is always created in AllocEngine constructor and there is
-        // currently no other way to set it, so that check is not really necessary.
-        if (!allocator) {
-            isc_throw(InvalidOperation, "No allocator selected");
-        }
-
         if (!subnet) {
-            isc_throw(InvalidOperation, "Can't allocate IPv4 address without subnet");
+            isc_throw(BadValue, "Can't allocate IPv4 address without subnet");
         }
 
         if (!hwaddr) {
-            isc_throw(InvalidOperation, "HWAddr must be defined");
+            isc_throw(BadValue, "HWAddr must be defined");
         }
 
-        // Build the processing context.
+        /// @todo The context for lease allocation should really be created
+        /// by the DHCPv4 server and passed to this function. The reason for
+        /// this is that the server should retrieve the Host object for the
+        /// client because the Host object contains the data not only useful
+        /// for the address allocation but also hostname and DHCP options
+        /// for the client. The Host object should be passed in the context.
+        /// Making this change would require a change to the allocateLease4
+        /// API which would in turn require lots of changes in unit tests.
+        /// The ticket introducing a context and host reservation in the
+        /// allocation engine is complex enough by itself to warrant that
+        /// the API change is done with a separate ticket.
         Context4 ctx;
         ctx.subnet_ = subnet;
         ctx.clientid_ = clientid;
@@ -650,22 +655,23 @@ AllocEngine::allocateLease4(const SubnetPtr& subnet, const ClientIdPtr& clientid
             }
         }
 
-        // Hint is in the pool but is not available. Search the pool until first of
-        // the following occurs:
+        // No address was requested, requested address was not in pool or the
+        // allocation was not successful so far. Let's try to find a diffferent
+        // address for the client.  Search the pool until first of the following
+        // occurs:
         // - we find a free address
         // - we find an address for which the lease has expired
         // - we exhaust the number of tries
         //
-        // @todo: Current code does not handle pool exhaustion well. It will be
-        // improved. Current problems:
-        // 1. with attempts set to too large value (e.g. 1000) and a small pool (e.g.
-        // 10 addresses), we will iterate over it 100 times before giving up
-        // 2. attempts 0 mean unlimited (this is really UINT_MAX, not infinite)
-        // 3. the whole concept of infinite attempts is just asking for infinite loop
-        // We may consider some form or reference counting (this pool has X addresses
-        // left), but this has one major problem. We exactly control allocation
-        // moment, but we currently do not control expiration time at all
-
+        /// @todo: Current code does not handle pool exhaustion well. It will be
+        /// improved. Current problems:
+        /// 1. with attempts set to too large value (e.g. 1000) and a small pool (e.g.
+        /// 10 addresses), we will iterate over it 100 times before giving up
+        /// 2. attempts 0 mean unlimited (this is really UINT_MAX, not infinite)
+        /// 3. the whole concept of infinite attempts is just asking for infinite loop
+        /// We may consider some form or reference counting (this pool has X addresses
+        /// left), but this has one major problem. We exactly control allocation
+        /// moment, but we currently do not control expiration time at all
         unsigned int i = attempts_;
         do {
             IOAddress candidate = allocator->pickAddress(subnet, clientid,
@@ -733,13 +739,9 @@ AllocEngine::renewLease4(const Lease4Ptr& lease,
     if (!ctx.host_) {
         ConstHostPtr host = HostMgr::instance().get4(ctx.subnet_->getID(),
                                                      lease->addr_);
-/*        if ((host && ctx.hwaddr_ && (*host->getHWAddress() != *ctx.hwaddr_)) ||
-            ((ctx.requested_address_ != IOAddress("0.0.0.0")) &&
-             !ctx.subnet_->inPool(Lease::TYPE_V4, requested_address_))) {
-            ctx.interrupt_processing_ = !ctx.fake_allocation_;
-            return (Lease4Ptr());
-        } */
-
+        // Do not renew the lease if:
+        // - If address is reserved for someone else or ...
+        // - renewed address doesn't belong to a pool.
         if ((host && ctx.hwaddr_ && (*host->getHWAddress() != *ctx.hwaddr_)) ||
             (!ctx.subnet_->inPool(Lease::TYPE_V4, lease->addr_))) {
             ctx.interrupt_processing_ = !ctx.fake_allocation_;
@@ -975,20 +977,19 @@ AllocEngine::replaceClientLease(Lease4Ptr& lease, Context4& ctx) {
                   " replaceClientLease");
     }
 
+    // Remember the previous address for this lease.
     IOAddress prev_address = lease->addr_;
-    if (!ctx.fake_allocation_) {
-        LeaseMgrFactory::instance().deleteLease(prev_address);
-    }
 
     if (!ctx.host_) {
         ConstHostPtr host = HostMgr::instance().get4(ctx.subnet_->getID(),
                                                      ctx.requested_address_);
+        // If there is a reservation for the new address and the reservation
+        // is made for another client, do not use this address.
         if (host && ctx.hwaddr_ && (*host->getHWAddress() != *ctx.hwaddr_)) {
             ctx.interrupt_processing_ = true;
             return (Lease4Ptr());
         }
         lease->addr_ = ctx.requested_address_;
-
     } else {
         lease->addr_ = ctx.host_->getIPv4Reservation();
     }
@@ -1024,7 +1025,15 @@ AllocEngine::replaceClientLease(Lease4Ptr& lease, Context4& ctx) {
         ctx.callout_handle_->getArgument("lease4", lease);
     }
 
-    if (!ctx.fake_allocation_) {
+    /// @todo There should be a callout for a deletion of an old lease.
+    /// The lease4_release callout is in appropriate, because by definition
+    /// it is invoked when DHCPRELEASE packet is received.
+
+    if (!ctx.fake_allocation_ && !skip) {
+        // We can't use LeaseMgr::updateLease because it identifies the
+        // lease by an IP address. Instead, we have to delete an old
+        // lease and add a new one.
+        LeaseMgrFactory::instance().deleteLease(prev_address);
         LeaseMgrFactory::instance().addLease(lease);
     }
 

src/lib/dhcpsrv/alloc_engine.h

@@ -305,18 +305,66 @@ protected:
 
     /// @brief Returns IPv4 lease.
     ///
-    /// This method finds the appropriate lease for the client using the
-    /// following algorithm:
-    /// - If lease exists for the combination of the HW address, client id and
-    /// subnet, try to renew a lease and return it.
-    /// - If lease exists for the combination of the client id and subnet, try
-    /// to renew the lease and return it.
-    /// - If client supplied an address hint and this address is available,
-    /// allocate the new lease with this address.
-    /// - If client supplied an address hint and the lease for this address
-    /// exists in the database, return this lease if it is expired.
-    /// - Pick new address from the pool and try to allocate it for the client,
-    /// if expired lease exists for the picked address, try to reuse this lease.
+    /// This method finds a lease for a client using the following algorithm:
+    /// - If a lease exists for the combination of the HW address or client id
+    ///   and a subnet, try to use this lease for the client. If the client
+    ///   has a reservation for an address for which the lease was created or
+    ///   the client desires to renew the lease for this address (ciaddr or
+    ///   requested IP address option), the server renews the lease for the
+    ///   client. If the client desires a different address or the client has
+    ///   a (potentially new) reservation for a different address, the existing
+    ///   lease is replaced with a new lease.
+    /// - If the client has no lease in the lease database the server will try
+    ///   to allocate a new lease. If the client has a reservation for the
+    ///   particular address or if it has specified a desired address the
+    ///   server will check if the particular address is not allocated to
+    ///   other client. If the address is available, the server will allocate
+    ///   this address for the client.
+    /// - If the desired address is unavailable the server checks if the
+    ///   lease for this address has expired. If the lease is expired, the
+    ///   server will allocate this lease to the client. The relevant
+    ///   information will be updated, e.g. new client HW address, host name
+    ///   etc.
+    /// - If the desired address is in use by other client, the server will try
+    ///   to allocate a different address. The server picks addresses from
+    ///   a dynamic pool and checks if the address is available and that
+    ///   it is not reserved for another client. If it is in use by another
+    ///   client or if it is reserved for another client, this address is not
+    ///   allocated. The server picks next address and repeats this check.
+    ///   Note that the server ceases allocation after configured number
+    ///   of unsuccessful attempts.
+    ///
+    /// The lease allocation process is slightly different for the
+    /// DHCPDISCOVER and DHCPREQUEST messages. In the former case, the client
+    /// may specify the requested IP address option with a desired address and
+    /// the server treats this address as hint. This means that the server may
+    /// allocate a different address on its discretion and send it to the
+    /// client in the DHCPOFFER. If the client accepts this offer it specifies
+    /// this address in the requested IP address option in the DHCPREQUEST.
+    /// At this point, the allocation engine will use the request IP address
+    /// as a hard requirement and if this address can't be allocated for
+    /// any reason, the allocation engine returns NULL lease. As a result,
+    /// the DHCP server sends a DHCPNAK to the client and the client
+    /// falls back to the DHCP server discovery.
+    ///
+    /// The only exception from this rule is when the client doesn't specify
+    /// a requested IP address option (invalid behavior) in which case the
+    /// allocation engine will try to allocate any address.
+    ///
+    /// If there is an address reservation specified for the particular client
+    /// the reserved address always takes precedence over addresses from the
+    /// dynamic pool or even an address currently allocated for this client.
+    ///
+    /// It is possible that the address reserved for the particular client
+    /// is in use by other client, e.g. as a result of pools reconfigruation.
+    /// In this case, when the client requests allocation of the reserved
+    /// address and the server determines that it is leased to someone else,
+    /// the allocation engine doesn't allocate a lease for the client having
+    /// a reservation. When the client having a lease returns to renew, the
+    /// allocation engine doesn't extend the lease for it and returns a NULL
+    /// pointer. The client falls back to the 4-way exchange and a different
+    /// lease is allocated. At this point, the reserved address is freed and
+    /// can be allocated to the client which holds this reservation.
     ///
     /// When a server should do DNS updates, it is required that allocation
     /// returns the information how the lease was obtained by the allocation
@@ -331,6 +379,8 @@ protected:
     /// returned, it is an indication that allocation engine reused/renewed an
     /// existing lease.
     ///
+    /// @todo Replace parameters with a single parameter of a @c Context4 type.
+    ///
     /// @param subnet subnet the allocation should come from
     /// @param clientid Client identifier
     /// @param hwaddr Client's hardware address info
@@ -348,7 +398,7 @@ protected:
     ///        lease. The NULL pointer indicates that lease didn't exist prior
     ///        to calling this function (e.g. new lease has been allocated).
     ///
-    /// @return Allocated IPv4 lease (or NULL if allocation failed)
+    /// @return Allocated IPv4 lease (or NULL if allocation failed).
     Lease4Ptr
     allocateLease4(const SubnetPtr& subnet, const ClientIdPtr& clientid,
                    const HWAddrPtr& hwaddr,
@@ -365,6 +415,7 @@ protected:
     ///
     /// The address of the lease being renewed is NOT updated.
     ///
+    /// @param lease A lease to be renewed.
     /// @param ctx Message processing context. It holds various information
     /// extracted from the client's message and required to allocate a lease.
     ///