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/database_backends.dox'''[[BR]]
 >
 > This file should mention that MySQL is disabled by default and --with-
 dhcp-mysql is needed.
 >
 > Comment on memfile that nothing is written to external storage should
 emphasise somehow that this is a temporary limitation ...
 The file has been updated to include this information.


 '''src/lib/dhcpsrv/dhcpdb_create.mysql'''[[BR]]
 > Why are we increasing only minor version? There are incompatible changes
 (lease_time => valid_leasetime), so major version should be bumped.
 Fair point - version changed from 0.2 to 1.0 (in both dhcpdb_create.mysql
 and tests/schema_copy.h).

 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
 commonaility between the DNS and DHCP update programs.  I would prefer
 that we don't duplicate code.)


 '''src/lib/dhcpsrv/lease_mgr.h'''[[BR]]
 > "This is pecified" => "This is specified"
 Fixed.

 > Comment for Lease6 is truncated. There seem to be missing a line
 Fixed - appropriate line copied from Lease4.

 (Although as a note, if we were to put the definitions of getters and
 setters in the class definition, the compiler should inline them and there
 should be no penalty.  However, I think a "struct" is warranted in this
 case: I tend to use a "struct" where the intention is to collect data
 items together, and a "class" where the contained data is less important
 than the functionality the class provides.  In this case, both Lease4 and
 Lease6 are principally data stores, so a "struct" is appropriate.)

 > Comment for addr_ field in Lease6: It should be commented that it may
 contain prefix in case of IA_PD.
 Comment added.

 > Comment for iaid field: "Most containers" => "All containers"
 changed.

 '''src/lib/dhcpsrv/mysql_lease_mgr.cc'''[[BR]]
 > CLIENT_ID_MAX_LEN constant: Please comment that this is arbitrarily
 chosen value as RFC2131 does not specify upper bounds. It just follows 128
 byte limit from DHCPv6.

 > We can possibly add a comment that a minor performance boost may be
 achieved with shortening this field.
 Is that actually the case?  VARCHAR fields in the database are variable
 length and only store the data passed to them.  I'm not sure that changing
 the maximum size affects performance.

 > On a side note, it may be a good idea to mark such comments somehow. It
 will be useful when a customer comes to us and says that extra perfomance
 is needed even if corners are cut. Note that it is "when", not "if". Not
 all performance improvements are sane in general case, but it would be
 useful to have pointers to start.
 We could just ensure that such sections start "Performance:", although it
 might be better to define a suitable tag: the Doxygen command \xrefitem
 may be our friend here.  However, as adding a suitable tag such as
 "\performance" means altering the Doxygen configuration file to include a
 suitable ALIAS statement, I would prefer to get agreement with the DNS
 team first.

 > "variables n header" => "variables in header"
 Fixed.

 > "nonly to satisfy cppcheck" => "only to satisfy cppcheck"
 Fixed.

 > "structre" => "structure"
 Fixed.

 > It would be nice to add maximum field lengths in createBindForSend
 comments, e.g. hwaddr: varbinary(128).
 Done.

 > Please define const int value for bind_[] array size.
 Done.  Also added some static asserts for additional checking about array
 size limits.:w

 > "strictures" => "structures"
 Fixed

 > ADDRESS6_TEXT_MAX_LEN is 42 bytes. Comment for address:varchar states
 that we define the field as "couple bytes longer", so we can null
 terminate it. Why couple bytes and not just one? Also, is the MySQL API
 require null-termination? In that case why string length is passed as
 well?
 The documentation is not as clear as it could be, but on reflection I
 think you are correct - the number of bytes transferred from the database
 for a VARCHAR is the number of characters in the field.  As such, the
 buffer need only allocate one additional byte to allow for a trailing
 NULL.

 > Why did you remove error checking (bind_[X].error values)? There
 shouldn't be any overflows, but it is better to check it anyway. One
 example that comes to mind is overflow my happen with IDN hostnames and
 odd UTF-8 encoding. That encoding may be set by the user and internal
 MySQL conversion is outside of our control. Let's keep it.
 Mainly because it wasn't used.  But you are right, we might as well check
 for truncation now (it was a "todo").  The error variables have been
 restored and truncated data causes an exception to be thrown.

 > Why MySqlLease6Exchange::getLeaseData() does not set T1 and T2 and uses
 zeros? I haven't reviewed the tests yet, but aren't tests supposed to
 catch that?
 Should T1 and T2 be stored in the database?  At the moment, they aren't.

 > "flushed disk in the background every second or so." comment: I think
 you should remove "or so". I think (haven't checked that, so I may be
 wrong) that innodb_flush_log_at_trx_commit set to 2 means exactly 1 second
 (with the standard granularity of non-real time systems).
 Done.

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

 > Why do we need semicolons in openDatabase() in catch() sections? Is
 there a compiler that complains about empty { } sections?
 Hangover from an old programming style.  Removed.


 > MySqlLeaseMgr::addLeaseCommon comment: Please explicitly say that this
 is for Lease4 and Lease6. Also, since this is file local class (no header
 for it), it makes sense to comment its methods using doxygen, including
 @param and @return tags.
 It is part of !MySqlLeaseManager, so the header is in the .h file.

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

 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.

 > I think we should check errors in fields (inbind[0].error should be set
 to something and its value verified).
 The error flags are only set if the fetch returns a truncation error.
 This is now checked.

 > getName() method is supposed to return backend type. That is required
 for Kea to report which backend is used. See DHCP6_DB_BACKEND_STARTED log
 message and memfile implementation.
 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.)

 '''src/lib/dhcpsrv/mysql_lease_mgr.h'''
 > Major version should be increased, not minor as the changes are not
 backwards-compatible.
 Changed


 > deleteLease4/6 returns boolean value, but updateLease4/6 status is
 reported as exception (or lack of thereof). This should be consistent. I
 think exeptions are they way to go (there may be other exeptions as well,
 e.g. db error), but I'm ok with whatever consistent approach you'll
 propose.
 It's really up to you as you are writing the code that uses it.  But I
 would have thought that the cases are different:

 The desired post-condition of deleteLease() is that the lease is not in
 the database.  Whether it was there beforehand is not really relevant as
 the aim is to get rid of it.  Therefore, deleting a lease that is not
 there may be worthy of a note somewhere but it does not affect the flow of
 control of whatever is deleting the lease.  (If the pre-condition that
 presence of the lease is important, getLease should have been used to
 check.)

 The postCondition of updateLease is that a version of the lease should
 exist in the databse after the operation.  The lease not being in the
 database beforehand causes the operation to fail and the post-condition
 not to be satisfied.  This is an error.

 So in the former, status return that is option and can easily be ignored
 appears to be the way to go.  For the latter, an exception seems
 appropriate.

 > 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 argumewnt 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?

 > !StatementIndex enum: Comment for GET_LEASE4_CLIENTID_SUBID should be
 "// Get lease4 by Client ID and subnet ID".
 Updated.

 > addLeaseCommon(): it may not be obvious what the two flavours are.
 Adding simple "(v4 or v6)" will clarify that.
 Updated.



 > getLeaseCollection(): "any data in the collection is cleared before new
 data is added.". Why? I think it *may* be useful to obtain sum of all
 leases that meet certain criteria...
 I've removed the clear() call.  In the existing code it was unecessary
 anyway as all calls to getLeaseCollection() declare the !LeaseCollection
 object immediately before it.  The header has been updated.

 > We could query several subnets and just keep adding results to the same
 collection. I'm not sure if that is how we will implement leasequery, but
 I'm somewhat doubtful about result.clear(). If you have strong opinion
 about keeping it as it is now, I won't object.
 As noted above, as currently written the clear() was unecessary.  If we do
 go for this idea, then we will have to rewrite some of the getLease()
 methods to modify a !LeaseCollection object passed to them as opposed to
 returning one via the method return value.

 > I like the templated getLeaseCollection approach. Do you think
 specifying 3 parameter getLeaseCollection methods are needed? Templated
 getLeaseCollection could have been called directly. The only (minor)
 drawback is that you'd need to use exchange4_ and exchange6_ explicitly in
 every call. Again, that is only a suggestion. I'm fine with keeping the
 code as it is now.
 I added them more for ease of use and understandability than anything
 else.  Without them, at the call to getLeaseCollection() you would need to
 introduce the correct exchange object, which has not appeared anywhere in
 the method up to then.  With them, you don't worry about the exchange
 objects, they only appear in the common code.  The three-parameter methods
 are defined in the header, so the compiler should inline them.


 '''src/lib/dhcpsrv/tests/schema_copy.h'''
 > I think that keeping this file completely up to date with
 dhcpdb_create.mysql will be a challenging task. We will see how it goes.
 One possible approach would be to have the schema defined in one place and
 the other being
 generated out of it.
 I agree, and "But let's keep things simple for now" was my approach.  In
 the long-term I was envisioning something like the DNS "dbutil" utility
 for creating and updating the schema.  It may be possible to add something
 to that to generate the .h file.

 > Please just add a statement to dhcpdb_create.mysql that any changes to
 the schema require also schema_copy.h update. Please add is somewhere
 close to the schema_version statements.
 Done.

 '''src/lib/dhcpsrv/tests/lease_mgr_unittest.cc'''
 > Please include a one or two line comment before each test to explain the
 scope of a test. That is Shawn's idea that I very much support. We will
 convert them to a more structured test documentation one day.
 Done.

 > Lease4.Lease4Constructor: I would use much bigger values for ADDRESS.
 Currently it is 0.1.some.thing. It would be convenient to check that
 values with most significant bit set (greater than 0x7fffffff) work as
 well.
 The LeaseXConstructors are now tested with a variety of addresses.

 > Please update copyright header to 2012. !LeaseMgr is so advanced that
 nobody even dared to think about it in 2011 :)
 Done.

 '''src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc'''
 > You indented "if (name != NULL) {" line. Previous indentation was
 correct. Please revert the change.
 Done.

 > boost::shared_ptr<DUID> is used in many places. There is !DuidPtr type
 defined for it.
 Changed.

 > Also, we probably should defined !ClientIdPtr (or perhaps use !DuidPtr
 for as a !ClientId is derived from Duid class).
 I leave that up to you.

 > checkLeasesDifferent(): I agree with the first loop using ASSERT_TRUE
 instead of EXPECT_TRUE, but I question the second double loop: It will
 print out the first error and abort. It would be more informational if all
 combinations were printed...
 Done by adding a SCOPED_TRACE statement prior to the call.

 > ... Also, it should be EXPECT_EQ or ASSERT_EQ, so leases would be
 printed in case of mismatch.
 That will only work where the lease can be converted to a string.  For the
 moment, I think that just identifying which leases are equal is
 sufficient.

 > Finally, I think it would be good to compare not the shared pointers,
 but the objects they point to. This would catch the case where you have 2
 exact copies with all parameters the same.
 Good catch.  Done - of course, it did mean adding Lease4::operator==() and
 then writing unit tests for both Lease4::Operator==() and
 Lease6::operator==().


 > getLease4AddressSubnetId: Please define constants for the values used in
 tests (73, 74).
 Done.

 > getLease4ClientId, getLease4HwaddrSubnetId, getLease4Hwaddr: Please
 check that the functions do not crash when empty client-id and/or hw
 address is provided. (Remember the fun we had with zero length client-
 ids?) Please test maximum sizes of the client-id (128) and hwaddr (20).
 Done.  Also tried lengths in excess of these limits to verify they are
 truncated (although I can't find a way to get that information when the
 data is added to the database). In addition, the tests were extended to
 check DUID lengths in the Lease6 tests.

 > One generic comment about client-id: Shawn suggested that we should keep
 hardware type as the first byte in hwaddr. That is something we are doing
 in isc-dhcp. I'm not saying that we should do it within this ticket, but
 we should keep in mind that something like this will likely be
 implemented. The best course of action would be to talk about if briefly
 and the probably create a ticket for it.
 Agreed.  Or define a !HardwareAddress class which contains both type and
 value, and wrote both into separate columns in the database.

 > Generic comment about getLeaseX family of tests. They should be
 eventually refactored to cover other backends. I think we need one common
 class for all backends. As this is a huge task, we should probably create
 a separate ticket for it. I for one would like to avoid replicating this
 huge work you've done here when implementing similar tests for memfile.
 Agreed, but I don't think it's a huge task.  The tests are virtually
 completely backend agnostic, so it is really just getting them to run with
 different backends.  Gtest has "parameterised tests", which may meet that
 need.  I've created ticket #2533 for this.

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