BIND 10 #3146: PD: Abstract LeaseMgr support for Lease6 type PD

[BIND 10 Development]

#3146: PD: Abstract LeaseMgr support for Lease6 type PD
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tmark
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp6         |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130918
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => tmark


Comment:

 I had this response fully written yesterday, but got distracted, and then
 forgot about it and shutdown my computer. :(

 Replying to [comment:5 tmark]:
 > Review Comments:
 >
 > General:
 >
 > If no two leases may have the same address, regardless of type, why do
 we pass in lease type on getLease6 methods that formerly only required an
 address?
 Passing type has two functions. It may be used a filter, but also as a
 sanity check. Under normal (non-error) circumstances, the type should
 always match.

 The other error handling alternative would be to implement type checks in
 every place where getLease6(addr) is used and that would be much more
 difficult to maintain. I do not have any specific preference regarding its
 operation. It can either log error message and return NULL or perhaps
 throw. In both cases the error handling should be vocal enough, so it
 should be noticed by sysadmin relatively quickly.

 > From a back end perspective, it would be inefficient to filter by lease
 type
 > address is guaranteed to be unique, so at it would likely be ignored.
 It is unique, but may have different type than expected. I'm aware of 2
 cases when this could happen. First is a sysadmin that manually changes
 lease type. Another one is a misconfiguration where address and prefix
 pools overlap.

 > General:
 >
 > Not to be picky, but the v4 methods which return collections are not
 "getLeases4".  I think if you are going to make a semantic change in the
 names like this, you should do it for both v4 and v6, or not do it.
 Personally, I like the change, in fact it is almost too subtle,
 getLease6Collection would be even better.  Maybe open a ticket to rename
 the appropriate lease4 methods in similar fashion?
 Created ticket #3163. We should keep in mind that there's no parity
 between DHCPv4 and DHCPv6. v4 allows exactly one address for a single
 client+subnet. For DHCPv6, there may be many addresses for each DUID/iaid
 /subnet-id combination. For now, we used to support only a single address,
 but the protocol allows more than one (this is mostly used in
 renumbering).

 > General unit tests:
 >
 > Tests such as TEST_F(!MySqlLeaseMgrTest, getLease6DuidIaid) are going to
 fail
 > once the back ends implement filtering by type.   I think we need
 @todo's on
 > these.
 I found two tests and added @todos there. If I missed any other, they will
 reveal themselves once the filtering is implemented.

 > /dhcpsrv/tests/test_utils.h
 >
 > Did you assigned specific values to !LeaseType enums solely for unit
 testing?  I see
 > that !GenericLeaseMgrTest's constructor relies on the values being 0,1,2
 with the "i%3" trick.  This seems like an abuse of enums that you yourself
 would normally complain about it
 > and is it even portable?  It also makes it less than obvious which lease
 type is in which test
 > lease.
 Updated as requested. The values used in dhcpdb_create.mysql are matching
 values in Lease6::LeaseType. Added an appropriate comment about it.

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