BIND 10 #2342: MySQL lease database access functions

[BIND 10 Development]

#2342: MySQL lease database access functions
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  tomek
  stephen                            |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:         |  DHCP-20121101
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  dhcpdb                             |           Sub-Project:  DHCP
                   Keywords:         |  Estimated Difficulty:  0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by tomek):

 '''configure.ac'''
 Error message "Specified mysql_config program ${MYSQL_CONFIG} is a
 directory" should also explain that it should point to the mysql_config
 program. (I think the second error message will not be printed if the use
 points out to a directory.

 Is the mysql_config program called differently on different systems?
 Pehaps specifying a directory is sufficient? On the other hand, there
 could be multiple versions instaled, so this tool can possibly by named
 mysql55_config or similar...

 Please write it clearly somewhere that MySQL is currently used for DHCP
 only. Users can easily get an impression that DNS part of BIND10 may use
 MySQL as well. I'm not sure how to show that? Perhaps adding something
 like "DHCP backend: MySQL" to the status page at the end of configure
 script? BTW are there any plans to implement different backends for DNS?
 Perhaps we could also put "DNS backend: SQLite"?

 Why did you change order of the files in lib10_dhcp__la_SOURCES? If your
 intention was to make it alphabetically ordered, then iface_mgr_bsd.cc is
 in a wrong place. I'm not opposing that change, just curious about it.

 '''dhcpdb_create.mysql'''
 Is the type VARBINARY portable? I've just checked that it is not part of
 SQL92:
 http://www.contrib.andrew.cmu.edu/~shadow/sql/sql1992.txt
 We may keep it, but let's add a comments section at the end of the schema
 file about likely portability issues.

 Length of the client-id field in lease4 table. There is no explicit length
 restriction in RFC2131 or RFC2132 as far as I can tell. However, you
 should ask Shawn about this. I based its length in benchmark on three
 facts. First, the benchmarks were limited to v4, but I wanted to include
 some characteristics of IPv6. DUID (v6 concept) allows up to 128 bytes.
 There is RFC4361 that specify how DHCPv4 client can use DUIDs. I haven't
 read it, but I assume that DUID replaced or is sent as client-id, so from
 client identification perspective that sets the upper limit at 128 bytes.

 lease_type TINYINT - you can add a comment "see lease6_types for possible
 values".

 Comment "IA ID" for a field iaid does not give any information. "value of
 IA_ID field in IA_NA/IA_PD option" would be useful. "See section 10 of
 RFC3315" is another possible comment here. (Section 10 explains the
 concept of identity association".

 Why do you use COMMIT at the end of the script? My understanding is that
 COMMIT finalizes transaction that is in progress, but there is no START
 TRANSACTION or BEGIN anywhere in the script. Also, it seems valid, but a
 bit odd to require doing everything in one transaction here.

 Comment for comment about indexes. I thought the most likely index to
 appear first will be for addresses.

 '''lease_mgr.h'''
 Please do not use defines with two leading underscores. These are reserved
 for compiler. LEASE_MGR_H will just fine instead.

 Lease6() constructor: That is wrong. It passes 0 to IOAddress(uint32_t
 v4address), which in turn creates address 0.0.0.0. That is very wrong.

 As for both Lease4 and Lease6 constructors, I think there should be one,
 but let's not make it default. I think ultimately there should be a
 constructor that will take all required parameters (in particular at least
 address and duid/client-id). If you think that current default constructor
 is useful for testing, we can leave it for now, but please add a todo that
 is should be removed eventually. Also, see code on trac2324 for an example
 of such constructor for Lease6:

 {{{
     Lease6(LeaseType type, const isc::asiolink::IOAddress& addr, DuidPtr
 duid,
            uint32_t iaid, uint32_t preferred, uint32_t valid, uint32_t t1,
            uint32_t t2, SubnetID subnet_id);
 }}}

 What's your opinion on keeping T1 and T2 in the lease structure? It is
 technically a property of the IA container in v6 or DHCP exchange in v4
 and not part of the lease. It is also not kept in the database. I think we
 either should remove it from the Lease4/6 structure or add it to Lease4/6
 tables in MySQL.

 LeaseMgr() constructor. Defining extra type (ParameterMap) makes the
 interface less robust and actually requires more code in every place where
 LeaseMgr is instantiated. Although there's like to be only one such place
 in the production code, there will be many more in the testing code. Also,
 passing a string is a very generic approach that could be extended later
 for some specific backends. Personally, I liked previous approach better,
 but if my arguments do not convince you, feel free to keep the new code.

 Thanks for adding the consts and references. I'm not sure how that slipped
 through previous reviews :)

 There is a new method commit(). I'm fine with it, but isn't there another
 one needed like beginTransaction()? Or perhaps that is controlled by the
 backend itself? Is the developer supposed to call commit() after every
 operation (like getLease4()) or only after write operations? Is it
 mandatory for some backends or completely optional? Some comment about
 planned operation should be added.

 I noticed that there is no page in Developer's Guide about the backend.
 There should be one that explains the core concepts. See
 src/bin/dhcp6/dhcp6.dox for an example. It doesn't have to be very
 thorough. It is addressed at developer's who want to understand the
 architecture. Existing method documentation is fine regarding the details.

 LeaseMgrPtr - I very strongly oppose to it. LeaseMgr should be a
 singleton. In fact, I wrote an implementation for it. I'm not sure if such
 a construct has a name, but I'm calling it meta-signleton. It allows only
 one instance of any classes derived from the base LeaseMgr. See code on
 #2324.

 End of review, part 1. Review of the remaining files will follow.

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