BIND 10 #2404: MySQL IPv4 lease database access functions

[BIND 10 Development]

#2404: MySQL IPv4 lease database access functions
-------------------------------------+-------------------------------------
            Reporter:  stephen       |                        Owner:  tomek
                Type:  task          |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcpdb        |                    Milestone:
            Keywords:                |  Sprint-DHCP-20121213
           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 stephen):

 * owner:  stephen => tomek


Comment:

 '''src/lib/dhcpsrv/dhcpdb_create.mysql'''
 >> In the longer term, I would like to use a utility like "dbutil" to
 update the schema. This requires discussion with the DNS team as although
 the schema and backend database is different, there will be a lot of
 commonality between the DNS and DHCP update programs. I would prefer that
 we don't duplicate code.)
 > Feel free to initiate such discussion. If you need more time for this
 than you can dedicate now, please submit a new ticket for that. Currently
 the schema is rather simple, so we can update it manually. But it will get
 much more complicated in the future.
 Ticket #2543 created for this.

 '''src/lib/dhcpsrv/lease_mgr.h'''
 > The only real difference between class and a struct is that struct
 cannot have private members.
 Not true - a "struct" can have private members.  The only real difference
 is that elements in a class are "private" by default, whereas elements in
 an struct default to "public".

 '''src/lib/dhcpsrv/mysql_lease_mgr.cc'''
 > I'm not saying that we should this now. Just marking it as "potential
 (minor) performance improvement" will be sufficient.
 I've added this comment where MAX_CLIENT_ID_LEN is defined (in duid.h).

 >>> You may add @todo here noting that innodb_flush_log_at_trx_commit
 parameter will be tweakable from bind10 configuration one day.
 >> Is that wise? I'm not certain if the parameter can be altered from the
 C API and besides, it is a database tuning parameter, not really in the
 scope of BIND 10.
 > Ok. I thought that it will be controllable via bind10 config, but
 changing it directly in MySQL is fine as well. So how is it changed? Using
 mysql client? Or in some config file?
 The settings are stored in a configuration file, but can be set on a per-
 session or per-user basis if required.  See "mysql --help --verbose" for
 more details.

 >>> getLease4: I think we should throw exception if the subnet-id field
 does not match, and not silently ignore it. This means database
 corruption.
 >> I was a puzzled when I saw this method in the !LeaseMgr class: the
 address is the unique primary key of the table so we can find at most one
 lease with that address. The subnet_id is just an attribute of the row
 returned, so getting by address and subnet ID is really over-constraining
 the retrieval operation.
 > I agree to some degree. Unless someone tries to assign several
 overlapping RFC1918 address in different subnets. But that will not work
 for other reasons...
 >
 > Yes, you are right. Please remove this call.
 As this means alterations to a number of files, I've created a separate
 ticket (#2546)

 > Also, I've found a minor bug in the getLease4(addr) comment. It mentions
 DUID instead of client-id, which is likely a copy-paste error.
 Fixed.

 >> Really, I don't think this method belongs in the lease manager: it is
 the caller that is expecting a particular relationship between address and
 subnet ID, not the database layer. So it seems more logical for the caller
 to perform the check. Having said that, a performance optimisation could
 be for the database to perform the search using SQL: if the combination of
 address and subnet ID is not found, it would save the transfer of a record
 from the database.
 > Agree. Feel free to remove it as part of this ticket or submit separate
 ticket for that.
 Part of ticket #2546

 >> getType() returns the type of backend (memfile, !MySql etc.) getName()
 returns the name of the instance being used. For !MySql, getType returns
 "mysql", getName returns the name of the database ("kea", "keatest" etc.)
 > Fair enough. I have either forgotten about getType() method or someone
 else added it. Can I then ask you to update getName() in
 memfile_lease_mgr.h? It's a single line change.
 As memfile currently doesn't have a backing file, the name returned is
 "memfile".  When we back it up with a file, it should return the name of
 the file.

 >>> deleteLease4/6 todo comment: I'm not sure about unifying them. I think
 it would be better from the consistency perspective to keep the API
 clearly label protocol number.
 >> addlease does not include a numeric suffix, the version of the method
 being chosen by argument type. With deleteLease, the version of the method
 to be used can be inferred from the type of address passed to it. So why
 introduce a complication?
 > Fair enough.
 I've added this as part of ticket #2546

 '''src/lib/dhcpsrv/tests/lease_mgr_unittest.cc'''
 > Let's define !ClientIdPtr. !ClientId field will probably get extra
 methods, e.g. like getHwType(), when we add hardware type to it, as Shawn
 suggested.
 Part of ticket #2546

 > I was thinking about writing Lease6Ptr::operator== that will always
 throw exception to easier trigger cases, when developer wants to compare
 leases, but actually compares pointers to them. But I haven't found a way
 to do that.
 Providing a specialisation of "boost::shared_ptr<Lease6>::operator==()"
 (within lease_mgr.h) to throw an exception when called is probably the way
 to go.

 Suggested !ChangeLog entry for this change is:
 {{{
 xxx.    [func]          stephen
         Extend DHCP MySQL backend to handle IPv4 addresses.
         (Trac #2404, git xxx)
 }}}

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