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

Tomek Mrugalski tomasz at isc.org
Mon Jul 13 17:11:05 UTC 2015


On 06.07.2015 14:43, Marcin Siodelski wrote:
> I have put together the document
> http://kea.isc.org/wiki/LeaseExpirationDesign, which presents the design
> for the Lease Expiration in Kea 1.0.
I reviewed your proposal and let me be the first one to say: good work!
Expiration is a tricky topic and I'm sure there were temptations to do
things with threads, processes or other tricky things. We may get there
eventually, but 1.0 is not the right time to step into this muddy area. :)

Anyway, on to the details:

Lease Reclaimation Routine
Can you consider renaming reclaimLeases4() to processExpiredLeases4? It
is maybe obvious what reclaim means in the lease expiration context, but
when you look at it some time in the future, it won't be that easy to
guess what "reclaim" means. It could as well be confused as reclaim
declined leases or reclaim leases that after reconfiguration no longer
belong to a subnet. On the other hand, if the decline design is accepted
as proposed, it may reclaim also declined leases, so maybe that name is
not that bad after all?

One thing is not clear to me. In the initial implementation with a
configuration of 100 subnets, how many times the reclaimLeases4 would be
called? One or 100? While I can certainly see a benefit of having per
subnet tweaks in the future, I think the base approach should be to
reclaim leases uniformly.

On a related note, it brings us to the interesting problem of measuring
timeout. If you plan to implement only global (not per subnet)
expiration in 1.0, this discussion becomes academic and we can skip it
in 1.0 timeframe. Anyway, if you have 100 subnets and the user had
specified that expiration can take no longer than 10 milliseconds, would
the code favor expiration for lease from subnet 1 over those from subnet
100? I think the answer is "no" (which is good) if there will be one
call for all subnets. The answer would likely be "yes" if there were
separate calls for each subnet. That's another reason why getting per
subnet tweaks may fragile. The code would have to implement some sort of
round robin with memory (the last expiration processing stopped at
subnet X, so this one continues from X or X+1).

Comment for unexpected shutdown:
I think the problem with unexpected shutdown is really non-issue if you
order the steps in appropriate order. In particular, the actual lease
removal must be done after hooks and name change request is sent. If the
server experiences an unexpected shutdown, the lease will will stay in
the database. After restart a possibly extra hook call and D2 name
removal request will be issued, but that's ok. The data will be
consistent. So I agree with your comment that this aspect (not really an
issue in my opinion) should be documented. Describing it in detail in
the Guide for Hooks Developers is fine, but a brief mention in the
User's Guide is also necessary. Even without hooks, you can get
duplicate DNS update attempts (with the latter failing) if you shutdown
your server abruptly.

Discussion regarding skip flag in the lease{4,6}_expire hook. I think
the skip flag should work as you described. Whether they will "pile up"
or not really depends on the client patterns. If you have a limited
number of clients (e.g. in a company that does not permit visitors), you
may want to skip expiration as a way to provide stable addresses.
There are better ways to do it (very long lease times for example), but
there may be extra reasons (monitoring when devices are up, thus short
lifetimes and desire to keep addresses stable). Anyway, explaining this
in the hooks guide would be very useful. BTW keep in mind ticket #3499
about replacing skip flag with an enum. 1.0 is the last moment where we
can do such a change.

Sending Name Change Requests from callouts
I have mixed feelings whether that is really that useful and if it's
really part of the expiration design. The same thing could be done for
lease assignment ("callout implementor would want to send a name change
request on his own"). I'm ok keeping it as part of the design, but the
tickets associated with this particular part should be considered
optional and have low priority.

Periodically executing tasks
>From the tasks listed at the end of your design, I presume that you
favor approach 2. I like it more than approach 1, so given choice of 1
or 2, I would go with 2. There's another possible approach, though.
Let's call it approach 3. You may think of it as simplified approach 2.

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.

Regarding approach 2 and 3
Do you want to register timers by string? It would be more efficient if
each timer had a unique integer value assigned upon registration. We had
similar discussion about statistics access patterns. In that case the
decision was to stick with strings as it's more natural. Also, hook
users may ask how they can ensure that their timer name is unique. A
method that returns timer id would ensure that. But I don't insist on
it, if you prefer to keep access by name pattern.

Configuration structure
- the parameter unwarned-cycles doesn't make sense, at least not in the
form you propose. Here's an example. I have the thing configured to run
expiration every minute. I also have clients that come and go and every
minute there's one lease that expires. Each expiration fully processes
all expired leases, yet after 5 minutes I start getting warnings that
something is wrong in my setup. Instead, I would put a log on INFO that
the expiration was not completed, because of allowed time budget was
reached and there are still X leases awaiting reclaimation.

- I think idle-time should be specified in seconds, not milliseconds.
Even in the most agressive setups ("I need to know about the expiration
exactly the second when it happens"), the smallest value used would be
1000. The name is also confusing, as it suggests that the server will do
expiration only if nothing else is happening. People may afraid that if
it's set to 1 second and there's a packet coming in every second the
expiration will never be triggered. Better name would be
"run-every-x-seconds", "expiration-interval" or simply "interval". The
documentation should clarify that it won't happen strictly every X
seconds, but every X+(time it took to process the last expiration cycle).

The design does not explain if it will be possible to run expiration
procedure during start-up. There should be a switch for it and the
default should be to do full expiration of all leases at startup.
That would be useful when recovering after server downtime.

New server command
You may consider renaming the command to "leases-expire" for the reasons
stated above.

Implementation tasks
I like this list. I need to update decline design with a similar one.

Extra steps for recovering declined addresses
If the Decline design I proposed gets through, there will be one extra
minor step to do. See point 4 in Implementation details here:
http://kea.isc.org/wiki/DeclineDesign.

Tomek



More information about the kea-dev mailing list