[kea-dev] Call for comments about the Decline design

[Thomas Markwalder]

On 7/28/15 7:06 AM, Thomas Markwalder wrote:
> On 7/28/15 3:38 AM, Marcin Siodelski wrote:
>> On 17.07.2015 17:01, Thomas Markwalder wrote:
>>> On 7/13/15 10:54 AM, Tomek Mrugalski wrote:
>>>> Hi everyone,
>>>> As part of the Kea 1.0 preparation, I wrote a short document about our
>>>> intended design for Decline support. It is described here:
>>>>
>>>> http://kea.isc.org/wiki/DeclineDesign
>>>>
>>>> The major idea is to use special hardware address or duid values to
>>>> indicate a declined address and keep it in the regular lease database.
>>>> With this approach, the amount of work is greatly reduced, there is
>>>> almost no performance degradation and this approach is proven
>>>> (implemented years ago in Dibbler) to work well.
>>>>
>>>> I'd like to hear your opinions on this proposal. We plan to conclude the
>>>> design discussions around end of July.
>>>>
>>>> Tomek
>>>>
>>>> _______________________________________________
>>>> kea-dev mailing list
>>>> kea-dev at lists.isc.org
>>>> https://lists.isc.org/mailman/listinfo/kea-dev
>>> I am not crazy about special purposing HWADDR/DUID to indicate a that lease
>>> has been declined. I get that it is "less expensive" in the short term
>>> from a development standpoint. I understand the appeal of that rationale but
>>> if that is the only motivation it isn't enough.
>>>
>>> From an analysis standpoint HWADDR/DUID are values that identify DHCP
>>> clients not a indicator of lease state.  We all know that proper API design
>>> frowns upon multi-meaning parameters and the reasons why. 
>>>
>>> We must also ask ourselves if there are any other circumstances under
>>> which we might persist a lease other than when it has been granted or
>>> declined?  If so, would we need to retain the actual client values for
>>> HWADDR/DUID or would we define more special values?  If we think there
>>> is a reasonable possibility of other cases where we would persist leases,
>>> then we should design for that now.  A more extensible approach would be
>>> to add a state or status column.
>>>
>>> I realize that would leave us with a question of what values would we then
>>> store in these columns when a lease is declined.  Again, from an analysis
>>> standpoint the most meaningful values would be those of the declining
>>> client.
>>> Suppose we did retain the value(s) from the declining client, what are the
>>> ramifications there?  It is already conceivable that a returning client
>>> could
>>> find a previous but expired lease right? We must be checking for that
>>> now right? 
>>> Bear in mind too, that using the "extra" effort it takes to test a flag or
>>> state column is no more than the effort the it takes to to test for a
>>> special
>>> value in HWADDR/DUID.
>>>
>>> Similarly, using the viewpoint of "don't change the schema" if we can
>>> squeeze
>>> it in somewhere else is short-sighted thinking.  Not just in this matter
>>> but in
>>> any other.  Eventually short-cutting the solution to "save time" ALWAYS
>>> comes
>>> back to haunt you.  It may be faster now but we will feel the pain
>>> later.  We
>>> have schema-upgrading now built into the project so we should not be afraid
>>> to change it. We should strive to do it right the first time.
>>>
>>> My gut instinct is that lease should have a state or status column and we
>>> should use it to indicate that a lease has been declined.
>>> ----------------------------------------------------------
>>>
>>> Regarding declined address processing via expiration - Should we really call
>>> both the decline_recycle and expiration_reclaim hooks?  Seems to me that it
>>> should only call the decline_recycle hook. From a business logic
>>> standpoint it
>>> wasn't an active lease so it cannot "expire" and therefore cannot be
>>> reclaimed from
>>> expiration.  If we call both then hooks developers may have to implement
>>> logic
>>> in their callouts to distinguish between the two business cases.
>>>
>>> What about the skip flag? How/where does this fit in?
>>>
>>> ----------------------------------------------------------
>>>
>>> Regarding the optional command to get "all declined", there needs to be
>>> some guard
>>> against how many we will return, if the number of leases is large it
>>> could be an issue
>>>
>>> ----------------------------------------------------------
>>>
>>>
>> Given all the comments for the lease expiration design and the decline
>> design, I would like to propose that we actually implement the
>> additional column 'state' for the Kea 1.0.
>>
>> The lease expiration, as it is described  now, marks the lease as
>> 'reclaimed' by simply removing it from the lease database.  This
>> minimizes changes to the lease database backends etc. However, I am
>> quite confident (after some discussions I had so far) that at some point
>> we will want 'address affinity'. In that case, removing the lease on
>> expiration is not a right approach.
>>
>> Since we need to add new functions and indexes to retrieve expired
>> leases, *now* seems to be the best time to add the new 'state' field if
>> we want to go down this path.
>>
>> The reason I bring it in this thread is that at this point we may also
>> consider if this field can be used for declined addresses.
>>
>> The big question is how to encode the state of the lease. I'd be in
>> favor of encoding a single state on an individual bit, in which case it
>> would be possible to encode multiple states with a single INT value,
>> i.e. the lease could be in multiple states simultaneously.
>>
>> I am seeking for an answer whether we want this or not before I further
>> work on the lease expiration design, as almost all further updates are
>> going to be based on this decision.
>>
>> Marcin
> A bitmask as you suggest, is probably the most flexible.  And I believe
> that the time to add the new field is now, assuming that is the consensus.
>
> Thomas
>
>
>
Tomek:

The decline design now states that declined leases will be marked as
declined
by setting the lease attribute, state, to indicate declined AND by
overwriting
the the hardware address/duid with a special value.

This doesn't make a lot of sense.  One of the primary reasons for creating
and using lease::state was that it allows us to preserve who declined the
address.  You may argue that is would be in the logs but that is true only
if the logging level emits the log message and only if the logs
themselves go
back far enough in time to contain it.

In addition it isn't good practice to store the same information twice. It
ought to be one or the other, not both.

I think we can reduce the impact to the getLeases() queries by simply
modifying
them to exclude declined leases (i.e. those leases whose state indicates
declined).  This would not alter the API at all.  Assuming the query
does the
comparison by address first, testing the state value won't cost much at all.

An alternative is to allow the queries to return them, along with the active
and expired leases they can return now, and let the caller cope with them.

Both declined and expired reclamations will require a new specialized
queries
anyway.   If we in the future, we find a need to create a getLeases()
variant which
accepts a state parameter we can do so then.

Thomas