BIND 10 #2320: Allocation engine v4: Address assignment

[BIND 10 Development]

#2320: Allocation engine v4: Address assignment
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:
                Type:  task          |  stephen
            Priority:  medium        |                       Status:
           Component:  dhcp4         |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130122
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => stephen


Comment:

 Replying to [comment:10 stephen]:
 > Reviewed commit 21794c5df650ecf28f55ed95cb12d278945ab3a5
 >
 > '''doc/guide/bind10-guide.xml'''[[BR]]
 > I've made a tiny change here (s/NACK/NAK/) and pushed the result.
 Thanks.

 > '''src/bin/dhcp4/dhcp4_messages.mes'''[[BR]]
 > Again a textual change here in DHCP4_RELEASE_EXCEPTION: I've replaced
 the work "exception" in the explanation with "error", on the grounds that
 for this usage, "exception" is really a programming term but the audience
 may not have programming experience.
 Ok. The text now refers to the incident as "error", but the message name
 is still exception. I suppose that is ok, as the explanation text is
 addressed at the end user, but message id is more addressed toward
 developers (or perhaps support staff).

 > '''src/bin/dhcp4/dhcp4_srv.cc'''[[BR]]
 > > It is currently an ERROR message as it indicates database
 inconsistency, some race conditions once we have multi-process capability
 or someone tampering with the database while we are processing this
 release.
 > The severity guidelines in src/lib/log/README state:
 > {{{
 > ERROR
 > -----
 > Something has happened such that the program can continue but the
 > results for the current (or future) operations cannot be guaranteed to
 > be correct, or the results will be correct but the service is impaired.
 > For example, the program started but attempts to open one or more
 network
 > interfaces failed.
 I believe that is the case. We do not know the reasons why delete failed.
 We know that the lease was there just a couple microseconds ago, but its
 deletion failed. The reason can be anywhere between benign (sysadmin just
 deleted a lease) to catastrophic (MySQL server crashed). According to our
 understanding the lease should not be there anymore. If it is, this will
 impair our future operation. We can continue to operate and provide
 meaningful service. I think ERROR adequately represents this. Also, if
 this were non-recoverable error, FATAL would be more appropriate.

 > In this case I would argue that the result for the current operation is
 correct and the program will continue working normally, hence a WARN.
 True, there may be problems in the future but we don't know that - it may
 be just this record is affected.
 It very much depends. I can find criteria where this is anything between
 barely noticable (Ok, we've lost one IPv6 address from /64 pool) to
 catastrophic (we have /32 IPv4 pool, i.e. one address and we failed to
 delete it, rendering the whole setup unusable).

 Finally, I think we should not second guess MySQL developers. Their
 documentation clearly refers to non-zero return codes as an error. My
 understanding is that non-zero return code is the only case where
 LeaseMgr::deleteLease() can throw.

 This is yet another case where we discuss different logging levels.
 Personally I think that the deployments who are scared of DoS attacks
 should have tweaked their logging to turn everything off (or have a robust
 logging system capable of handling such an attack). We should get back
 sometime after the release.

 > '''src/lib/dhcpsrv/subnet.cc'''[[BR]]
 > Fair point about the default values.  However, getting the default value
 through a virtual method would work:
 > {{{
 > class Subnet {
 >       virtual PoolPtr getPool(isc::asiolink::IOAddress hint);
 >       virtual PoolPtr getPool() {
 >               return (getPool(default_pool()));
 >       }
 >       virtual isc::asiolink::IOAddress default_pool() = 0;
 > };
 >
 > class Subnet4 : public Subnet {
 >       virtual isc::asiolink::IOAddress default_pool() {
 >               return (isc::asiolink::IOAddress("0.0.0.0"));
 >       }
 > };
 >
 > class Subnet6 : public Subnet {
 >       virtual isc::asiolink::IOAddress default_pool() {
 >               return (isc::asiolink::IOAddress("::"));
 >       }
 > };
 > }}}
 > Although a bit more complicated than the ugly hack, it does have the
 advantage of insulating the base Subnet class from any knowledge of
 subclasses derived from it.
 It is more complicated. It would be more appealing trade-off if there were
 other cases of Subnets planned in the future. I'm sure we won't see
 Subnet8. Anyway, I've added this code and the dynamic_cast is gone.

 Changes pushed, the ticket is back with you. If you have no other
 comments, I think it is ready to merge.

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