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

 '''dhcpdb_create.mysql'''
 Just noticed two more things:
 - The field client_id should be called duid.
 - lease_time should be called valid_lifetime (it makes sense to stick to
 RFC names where possible, especially that we have a pref_lifetime field).
 That applies to both lease4 and lease6.

 '''lease_mgr.cc'''
 No specific comments, except that ParameterMap is slightly more difficult
 to use, compared to simple string.

 Also, do we need all those 11 headers if most of the code was removed?

 '''lease_mgr_factory.cc'''
 Oh, I see that the code for parsing dbconfig string was just moved. Please
 ignore my previous comments about its disappearance from LeaseMgr then.

 I don't like the idea of shared_ptr to LeaseMgr. LeaseMgr is an essential
 component of the whole solution, so we need to have a direct control over
 its creation and destruction. With shared pointer the destruction is
 somewhat fuzzy. What if some code stores the pointer? Then we will not be
 able to destroy it. That will be essential for doing graceful shutdown or
 switching backends once we'll have more than one to choose from.

 I'm fine with the factory concept, but I'd prefer it to use normal
 pointers and possibly make the constructor protected. See the alternative
 proposal for accessing LeaseMgr in #2324. There's method that returns an
 instance of LeaseMgr:

 {{{
     static LeaseMgr& instance();
 }}}

 It is much more convenient to just call instance() rather than pass
 LeaseMgrPtr to all places that may need it. Furthermore passing
 shared_ptrs is worse from the performance perspective. You may pass const
 references to it, but still that requires passing some data and make the
 interface slightly more difficult to use. Keep in mind that LeaseMgr will
 be accessed from *lots* of places.

 Please add a comment that we will eventually need to develop some kind of
 registration mechanism for different backends (like reading all libraries
 from specific directory). Obviously, that is unlikely to appear even in
 2013, but it is good to have such places marked with @todo. Thanks to
 doxygen, Developer's Guide has a list of such todos generated
 automatically.

 '''mysql_lease_mgr.h'''
 Please do not use two leading underscores in __MYSQL_LEASE_MGR_H define.

 {{{
 /// @brief Abstract Lease Manager
 }}}
 This class is not abstract, at least I really hope it isn't. I plan to use
 it later this week :)

 Do we want to have default values for the host,db name,user and password?
 We can add there either here or in configuration handler. I think adding
 them in CfgMgr would be more convenient as we could then leverage the
 feature of bindctl trat displays default values. I see that openDatabase()
 skips parameters that are not used. I can imagine that some liberal admins
 may set up their database to allow anyone to connect, so no user/password
 is necessary, but is there really a case when no database name is
 specified? And what about host? In that absence of host specfication, what
 is used? Loopback?

 Comment for MySqlLeaseMgr() should clarify if the parameters are mandatory
 or optional.
 It should also mention that DbOperationError or DbOpenError may be thrown.

 Several Lease6 methods do not have possible exceptions mentioned in their
 comments. They should be added.

 Lease4 methods don't have them either, but I think we can skip it for now
 as there is no implementation. We may consider adding @todos marking v4
 code as not implemented, but I'm not sure if there's much benefit to do it
 as those methods will likely to appear in the upcoming weeks or even days.

 Why many methods are virtual here? Do we expect and further
 specialization, like MySQL55LeaseMgr? Those method are virtual anyway,
 since their definitions in base class are virtual. On a second thought, I
 think we can keep the virtual keyword.

 Sorry for opening that can of worms, but what timezone the timestamps are
 specified in? There was a long fight over ISC-DHCP using UTC. I'm not sure
 how it ended, but it was frowned upon by many people around the world. As
 a person who enjoys living close to Greenwich you may underestimate the
 issue ;-). However it was very frustrating for people who wanted to read
 actual expiration time from lease file. My point here is that we should
 make it possible for people to let them use their local timezones. If that
 is already the case, a brief comment that is all that is needed here.

 StatementIndex. I think the GET_VERSION should come on the list first, as
 this is the first call to be ever called. It will be good to keep it as
 always first in case sometime far in the future we decide to merge Lease4
 and Lease6 and there will be no GET_LEASE6 call at all.

 I like the checkError method a lot. It will be very useful for debugging
 as all necessary info in provided in the exception message.

 '''mysql_lease_mgr.cc'''
 Comment says that "On the INSERT, SELECT and UPDATE statements, an array
 of MYSQL_BIND
 structures must be built". I think the same is true for DELETE as you need
 to pass something to WHERE clause.

 MySqlLease6Exchange::CreateBindForSend()
 Why bind_[7].is_unsigned is set in such a strange way? Using true_ would
 work.
 What does "The MySQL documentation" mean in the CreateBindForSend()
 comment?

 MySqlLease6Exchange::getLeaseData()
 The exception that throws "invalid lease type returned" should provide
 more details. If the user messes up his database, we should give him a
 clue what is broken and should be fixed. Something like "Invalid lease
 type X returned for lease ADDRESS. Only 0, 1 and 2 allowed".

 MySqlLease6Exchange fields:
 Why the duid storage is called clientid_ and clientid_length_ rather than
 duid_ and duid_length_?

 MySqlLeaseMgr::converToDatabaseTime()
 See my comments about forcing users to use UTC. Users prefer to store
 their data in their local time. If MySQL allows storing data in local
 time, we should use it.

 MySqlLeaseMgr::openDatabase()
 Does it make sense to continue without database name? There's a comment
 about using defaults for non-specified options. What defaults? Where are
 they specified or what values they have?

 MySqlLeaseMgr::prepareStatement()
 InvalidParameter should specify faulty index in its text.

 MySqlLeaseMgr::MySqlLeaseMgr()
 We should checkf if mysql_init() returned NULL. Doc for it says that it
 may do so if there is not enough memory. That in turn would likely make us
 segfault later, when we try to use it.

 Lease4 methods:
 I would prefer them to throw until they are implemented. That is
 especially true for getLease4(). Returning NULL means that the database
 was checked and there is no such lease, so the allocation engine can act
 based on that information.

 MySqlLeaseMgr::getName()
 should return "mysql";

 General comments for MySqlLeaseMgr class:
 Would it be possible to rename raw_statements_ to text_statements_? Raw
 may mean different things in this context (raw as not compiled yet or raw
 as raw data sent to mysql).

 One final question about the mysql_lease_mgr in general: is the code
 thread-safe?

 One nasty question asked during BIND10 is also adequate here: is the code
 signal safe? I don't think we need to care about it now, just asking out
 of curiosity.

 '''src/lib/dhcp/Makefile.am'''
 One thing occurred to me: lease_mgr files should part of the
 libb10-dhcpsrv (server specific stuff), not the libb10-dhcp++ library
 (stuff shared by clients, servers, relays, performance tools etc). I know
 that this is confusing. We should consider splitting them into separate
 directories one day.

 This concludes part 2 of the review. Review of the remaining files will
 follow.

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