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      |
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => sar


Comment:

 Replying to [comment:5 sar]:
 > I have one architectural level question and a set of simpler questions
 or
 > comments.
 >
 > The architectural level issue deal with getting information from the db
 > via the lease_mgr api.  In the current code a given key may map to
 several
 > different leases on different subnets.  These could either be somewhat
 fixed,
 > for example a laptop used for testing that has a set of fixed addresses
 > and can be carried from subnet to subnet.  Or they could by dynamic, for
 > example if a laptop is moved around a campus and acquires multiple
 leases.
 Excellent point. I've updated several getLeaseX methods and added several
 new ones. In general, if we are very specific and want a single lease for
 a given subnet, we can get at most one lease, so the method returns a
 single pointer. On the other hand if we don't specify subnet_id, it is
 possible to get more than one lease, so the function returns a collection
 (basically a vector of pointers).

 There are getLease() methods that pass address as a parameter. As for any
 given address there could be at most one lease, they return a single
 pointer.

 > The functions that find a lease via some key (clientid, hwaddr etc)
 > don't seem to allow for either finding a lease within a subnet or
 > walking through all of the leases with that key until one for
 > the proper subnet is found.
 They now do.

 >         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.

 > 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?

 There are two reasons why ClientID was derived from DUID class. First, it
 couldn't stay as vector<uint8_t>, because that is the same type as
 hwaddress - compiler was confused by getLease4(hwaddr) and
 getLease4(client_id). The second reason is that both have exactly the same
 operations and data stored, so it reuses some of the code in both classes.

 > It also seems that deciding a 4 byte id is an IPv4 address might not
 always be true.
 Do you prefer to add a flag that specifies that client-id is really an
 address or get rid of
 {{{
     ClientId(const isc::asiolink::IOAddress& addr);
     isc::asiolink::IOAddress getAddress() const;
     bool isAddress() const;
 }}}
 altogether? I don't have any preference and will do whatever you
 recommend.

 > I'm not sure about putting an IPv4 address into the same structure - do
 you
 > have a use case for that already?
 No, I haven't. That seems to answer my previous question. Ok,
 aforementioned methods has been removed. ClientId is now an opaque value.

 > Lastly did you consider adding functions to create the various types of
 DUIDs?
 > (LL, LLT etc) It seems to be that such functions would be useful and
 would
 > go into this class.
 Yes, we will do that eventually. See Dhcpv6Srv::setServerID() in
 src/bin/dhcp6/dhcp6_srv.cc. It was implemented before we had DUID class.
 I've created ticket #2352 for DUID work.

 >         Comments on lease_mgr (lease_mgr.h, lease_mgr.cc)
 > Is there a description of how the lease structures will be used (in a
 > general sense)?  I'm imagining the lease_mgr  structure being used as a
 > temporary holder to get the data from the backend db, manipulate it
 > and then push the changes back to the backend.  Is that somewhat
 > correct?
 Yes. It is correct. There is no description as that will be worked out
 soon. See scary large number of tickets in the current sprint starting
 with "allocation engine".

 There was a discussion that some of the leases may be kept in memory as a
 form of cache, but that is a performance optimization and it will not be
 implemented in 2012. Another way lease structures will eventually be used
 is in memfile backend - basically a C++ version of what we have in ISC-
 DHCP.

 > 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.

 > 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.

 > Does recylce_time need to be in the structure?  If the lease structures
 aren't
 > permanent I can see the use of this functionality to provide a "grace"
 period
 > for a user to return and reclaim an address.  However I'm a bit iffy on
 needing
 > it on a lease by lease basis vs server wide basis.
 Good point. That would be only useful if we wanted to have the grace
 period changeable on a subnet basis. I have removed recycle_time from both
 lease structures.

 > 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).


 > The v6 lease structure includes t1 and t2 is there a reason not to
 include them
 > in the v4 structure?  The trend in the current code is to handle them in
 a
 > similar fashion and it also makes serving leasequeries easier.
 Good catch. Added.

 > Comments on the test code:
 >
 > It would probably be good to add a short description to each of
 > the tests to indicate what they are trying to test.
 Added comments to all tests. In fact, I think such descriptions should be
 a bit more structured. I have created a ticket #2350 for that purpose.

 > While it wouldn't matter all that much to the tests it might be
 > useful to use something that is closer to the full format
 > for an LLT or LL.
 We will get do that once we implement support for LLT and LL. #2352
 description was updated to cover that as well.

 Thank you for your review. All changes are in git. Please check if they
 address your comments.

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