[kea-dev] Call for comments about the Lease Expiration design

[Marcin Siodelski]

Shawn,

Thanks for review and comments. My responses inline.

On 23.07.2015 07:24, Shawn Routhier wrote:
> 
>> On Jul 6, 2015, at 5:43 AM, Marcin Siodelski <marcin at isc.org
>> <mailto:marcin at isc.org>> wrote:
>>
>> All,
>>
>> I have put together the document
>> http://kea.isc.org/wiki/LeaseExpirationDesign, which presents the design
>> for the Lease Expiration in Kea 1.0.
>>
>> This is the first pass for this document and it is still considered
>> early draft. In particular there are two sections about extending the
>> MySQL and PostgreSQL backend, which are marked "TBD". I am still
>> wondering if I should put anything into these sections, given that the
>> changes are straight forward and writing what we want to do may take
>> more time and actually implementing it. Nonetheless, the document is in
>> the shape in which people can review it and provide useful comments.
>> Please to so!
>>
>> Marcin
>> _______________________________________________
>> kea-dev mailing list
>> kea-dev at lists.isc.org <mailto:kea-dev at lists.isc.org>
>> https://lists.isc.org/mailman/listinfo/kea-dev
> 
> From Tomek’s mail:
> 
>     Approach 3
>     Everything happens in the main thread. The code is allowed to create new
>     timers wherever needed, but it must use the IOService created in the
>     Daemon class. Right now the main DHCPv{4,6} loop calls
>     LeaseMgrFactory::instance().getIOServiceExecInterval(); to determine the
>     timeout, but it would call TimerMgr::getTimeout() instead. (That would
>     also solve the problem described in #3696). That method would go over
>     all registered timers an find the shortest timeout. That's not a big
>     deal, as I don't think we'll ever have more than 3 or 4 timers (one for
>     LFC, second for expiration, maybe a third one for failover and maybe one
>     more custom for hooks). In this approach, TimerMgr is little more than a
>     container for timers with couple utility methods. No WatchSocket would
>     be necessary.
> 
> I find this approach more appealing in general.  For what it’s worth it
> is also more
> like what the current ISC DHCP code does.  
> 
> How many timers we might have will depend heavily on how much work each
> timer
> is responsible for, but as long as we don’t end up with timers per lease
> the number
> is probably tractable.  I believe in the failover case the ISC DHCP code
> has several
> timers per failover peer.  Kea could choose a similar approach or could
> have something
> more like one failover timer and the code within that timer would be
> responsible for
> handling all of the items associated with failover.
> 
> If we do go with approach 3 we can always start off simple and if we
> decide to have
> a large number of timers in the future move to some sort of list ordered
> by next timeout
> 

I have responded to this in my reply to Tomek’s email. I think we
shouldn’t use isc-dhcp as a reference here, simply because isc-dhcp was
designed to work in the non-concurrent model, whereas Kea should, IMHO,
head towards concurrent and asynchronous model. Note that this is why we
implemented D2 using the asynchronous model. This is also what we have
discussed several times in various situations.

I think that the architecture for timers should be done in the scalable
way today, rather than later. It will ultimatelly save our time, when
adding new timers.

> I think adding a brief description of the worker thread for Approach 2
> might also be useful
> 

Yep. It will be added.

> From Thomas’s mail:
> 
>     I find "most early" and "least early" confusing phrases.  The word
>     "early" means "before an expected time" or "in the beginning part of an
>     event".
> 
> 
> I also find these terms confusing.  Thomas’s suggestions are okay, I
> also like
> “less recently used” and “most recently used”.
> 

Note taken.

> Other
> 
> SKIP FLAG
> I am somewhat torn on the skip flag issue.  It seems to me that once we
> have gotten
> a lease into the DB we probably should be able to get it out at expire
> time arguing
> against a skip flag on the expiration but I do like all of the hooks
> being consistent.
> 
> In Marcin’s mail on the requirements he had this:
> 
>     I think we can, at some point, decide that the expired and reclaimed
>     lease may remain in the database. For example, if the client gets back
>     to get the new lease, the allocation engine may first check if there is
>     a lease in the database which the client already used. If the lease gets
>     removed, there is no way to do it.
> 
> 
> Well reading the skip flag discussions I was thinking he was considering
> using
> the skip to implement this for address affinity, but as the design
> document describes
> doing so will cause the expiration code to be re-processed every cycle
> and break
> the expiration so I don’t think we can go that route.  I think if we
> want address affinity
> we will need another flag or state for the lease to indicate that it is
> expired but has
> already been processed and can be re-used but hopefully only by the
> current client.
> 
> Note that many people do seem to wan address affinity so I think we will
> need to
> consider ways to provide it in the future.  (An option that avoids
> saving state would
> be to generate the IP address based on information from the client.
>  This probably
> works for v6 with a large address space but probably doesn’t work so
> well for v4
> with it’s smaller address space.)
> 

 On reflection, and after several discussions around this issue during
the most recent IETF, I think the design should better account for the
address affinity. So, it seems we may need a flag in the lease database
that would mark the lease reclaimed. This means that the query for the
expired leases should expose the parameter which optionally excludes
processed leases from the query result. Alternatively, we can make it a
configuration parameter of the LeaseMgr, to say whether the reclaimed
lease is deleted or left in the database. Again, if this parameter is to
be implemented, the deletion should be the only available option for 1.0.

> Do we provide a mechanism for the admin to have a hook send the name
> change request
> to add the name to the DNS?
> 

I don’t think we have one.

> LOGGING
> I see text about a warning message for unwarned-cycles but I don’t see a
> description of the
> message for it, should that be in the doc?  
> 

It should be.

> I would also add the number of leases not processed to the
> RECLAMATION_COMPLETE
> message.  So something like
> RECLAMATION_COPMLETE: time: <execution time>, reclaimed: <number>,
> didn’t process: <number>
> 
> 

There is one potential issue, though. If you want to optimize the SQL
query, you could use the LIMIT clause in it. In this case all that the
SQL would return is the number of leases you want to reclaim in a single
cycle. The number of all remaining expired leases in the lease database
is lost in this query. Not sure what we care about more?

Marcin