BIND 10 #2140: Create abstract API for lease database access

[BIND 10 Development]

#2140: Create abstract API for lease database access
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  sar
  stephen                            |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:         |  DHCP-20121018
  medium                             |            Resolution:
                  Component:  dhcp   |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DHCP
            Defect Severity:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by sar):

 The changes look fine.  I have snipped out anything I don't need to
 clarify.

 Replying to [comment:6 tomek]:
 > Replying to [comment:5 sar]:
 >
 > >         Comments on DUID (dhid.h, dhid.cc)
 > > The return of a pointer to the private vector in the get functions
 causes me
 > > a certain amount of concern.  Howerver I don't have any concrete
 problems with
 > > it and therefore it's probably fine as is until how the rest of code
 might
 > > usae the pointer becomes more clear.
 > I've changed them to return values rather than references. That means
 that they are safer, but much less efficient, so those methods should be
 used only sporadically. If we find a frequent use for them, we co could
 toss them altogether, but we will have to replace them with with
 storeSelf() or similar that will store the actual content somewhere (in a
 buffer?)
 > when sending out a packet.

 That looks fine, if we find that it's a performance issue we can do
 something else - including bringing back the code you removed.  Until we
 know how it will be used I think it's best to be safe.

 >
 > > I have a larger concern about the typing in the structures.  Perhaps
 it won't
 > > happen but it seems like there is some potential for confusion between
 the
 > > different types if the structures can be mixed.
 > Do you mean DUID and ClientID? No, not really. At first I thought if we
 could define DHCPv4 client-id as yet another DUID type and then use DUIDs
 in both v4 and v6. However, I decided that is not that useful and would
 make the use a bit problematic, e.g. what type should getType() return?

 Yes I was thinking of DUID vs ClientID.  If you haven't already you may
 wish to add an item to construct a client iD that is based on a DUID (see
 rfc4361).  I think mostly that would be useful for a client built from Kea
 so there's no hurry for it.


 > > Are you planning on including the type of the hardware addess as the
 > > first element in the hardware address vector?
 > I haven't really thought about it, but since we are doing this in ISC-
 DHCP and there are no obvious issues with it, we can do it again.

 It seems like a convenient way to keep the type associated with the
 address.  Another item that's not an immediate issue but perhaps should be
 mentioned is that the hardware address may not be used in some cases.  For
 example when using Infiniband hardware the hardware address is too long to
 fit into a DHCPv4 header.

 >
 > > There are several additional timestamps associated with failover.
 > > I suppose they can be deferred for now and added if/when we do
 failover.
 > Yes. I have added a comment that the lease structure will be expanded
 with the failover specific things.
 >
 > > I'm curious about the choice for valid_lft.  Making it seconds since
 cltt
 > > means that most uses of it will require getting cltt and doing an
 addition.
 > > Storing it as a time_t in its own right might take more space but
 might make
 > > the code more efficient.
 > Making it time_t would also mean that you need to update it every time
 the lease is refreshed. With the current approach, unless you do something
 weird (like a first renewal after reconfiguration) the only thing you need
 to do with renewal is to update cltt. That may have impact on DB
 performance. Stephen (who is currently working on DB backend) suggested
 that we should reverse the logic and keep the timestamp of expiration and
 cltt as a difference to that. That will make his idea for housekeeping
 process that sweeps through database and removes expired leases a bit
 simpler to implement.
 >
 > Also, the argument about avoiding a simple arithmetic is somewhat weak.
 We will have to use subtraction to calculate the valid-lifetime field. If
 we decide to keep everything as time_t values, we will get into much worse
 troubles, as we could get inconsistent values. I can agree to any solution
 that has one timestamp with the other fields specified as diffs to that. I
 don't really care about which one that is. Keeping cltt as a timestamp was
 the simplest approach as then all other fields are expressed as always
 positive differences. Also, they can then be directly used when creating a
 message.
 >
 > If you don't agree with that approach, we (Stephen, you and me) should
 have a discussion about it and make a compromise.

 I don't have a strong opinion at this time.  Your argument is reasonable
 but I still have some concerns.  I think we can start with this and adjust
 it as necessary as we progress.  The two areas I have concerns about are:

 1) in failover the two peers exchange time information and converting it
 to a delta might be difficult or annoying.  But we aren't doing failover
 yet so I don't think that's a reason to delay.

 2) I think there may be some optimizations we can use to avoid updating
 the DB and I'm not sure how they will fit into a scheme of cltt and
 deltas.  For example we may choose to do some form of lazy update where
 what is written into the DB is longer than what is passed to the client
 such that the next time the client makes a request the server doesn't need
 to write to the db before replying.  Again we aren't doing this yet so
 this isn't a reason to delay either.

 One side note we may elect to change the lease duration several times
 during a client's lifetime.  This happens in failover as the server
 switches from MCLT to the actual lease time and we may want to consider
 some sort of adaptive lease times as well.  There was a discussion of this
 on the dhcp-users mailing list that I found interesting.  In any case the
 number of times we change the lease duration is likely to be a good deal
 less than the number of times clot gets updated.

 > > I'm not sure we need the DDNS flags here.  In the current DDNS
 requirements it
 > > appears that there will probably be a separate DDNS structure to track
 DDNS
 > > information - the names in use, if A, AAAA or PTR have been updated
 etc.
 > > (Such an arrangement is likely required by the desire to be able to
 remove DDNS
 > > information when a lease expires even if the range has already been
 removed.
 > > We might be able to keep the lease structures around but that seems
 like it
 > > might be overly complicated.)  In my mind I'm thinking of the DDNS
 module as
 > > a somewhat separate entity.  It would be informed of all updates to
 the lease
 > Fully agree here. However, we need to provide that info to DDNS module
 somehow. I envisage the module as a relatively simple stateless (sort of
 his state would be limited to currently ongoing updates) machine.
 >
 > I do not want it to keep separate table with information that is closely
 coupled with lease information. First, such a separation would allow
 inconsistency in the database (some administrator deleted leases, but
 forgot to delete dns transactions related to it). Second, many
 implementations do fqdn in renewals. That means that the number of queries
 required for handling renewal would double. Third, we need to take into
 consideration that this information must survive restarts. If we keep the
 lease and the necessary DNS info (fqdn, was A/AAAA updated, was PTR
 updated) separate, we will need to do costly sanity checks between lease
 database and some table that hold dns info.
 >
 > > (requests, renews, rebinds, releases, expires etc) and would update
 its own db
 > > and the DNS server as necessary.  In this model the flags woudld go in
 that
 > > other structure not in the lease.  One downside of this is that the
 lease won't
 > > always have the same name information as the DNS.
 > I was more imagining DNS module as something very simple, little more
 than a wrapper around nsupdate (functionally speaking, the actual
 implementation will be completely different).

 I think we probably need to have a discussion about this, perhaps in the a
 Kea conference call.  In the DHCP4 code it would probably be better to
 split the two for several reasons.  But the DB is held in memory so doing
 an extra "lookup" for the ddns bits isn't very costly.  In the Kea code
 the tradeoffs will be different and the utility of a single lookup may
 outweigh the pain of keeping the various pieces in sync.

-- 
Ticket URL: <http://bind10.isc.org/ticket/2140#comment:7>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development