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

Comment (by tmark):

 Replying to [comment:6 tomek]:
 > 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.


 I'm good with this answer.

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

 Good enough.  The parity in this case isn't so much from a protocol
 standpoint as it is from an API standpoint.  To use a plural form in case
 and not another is inconsistent.


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

 Thanks.

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

 Don't you feel better now?  You do, don't you?



 Changes are good to go.

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