From: Marcin Siodelski Date: Mon, 15 Dec 2014 13:16:03 +0000 (+0100) Subject: [3564] Improved commentary for the allocation engine. X-Git-Tag: trac3504_base~25^2~4 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=30d482b258e9796c8a4d664b9be5d583b340dce7;p=thirdparty%2Fkea.git [3564] Improved commentary for the allocation engine. --- diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 64d99788c8..ea8ff2b16d 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/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); } diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index 9c01512615..fb3d669ee0 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/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. ///