BIND 10 #2342: MySQL lease database access functions

[BIND 10 Development]

#2342: MySQL lease database access functions
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  stephen
  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      |
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => stephen


Comment:

 '''src/lib/dhcp/tests/Makefile.am'''
 Make sure that the following lines are cleaned up before merging the code:
 {{{
 AM_CPPFLAGS += -I/usr/include/mysql -DBIG_JOINS=1 -fno-strict-aliasing
 AM_CPPFLAGS += -DUNIV_LINUX
 }}}

 All LeaseMgr-related files should be in libdhcpsrv_unittests, not
 libdhcp++_unittests.


 '''src/lib/dhcp/tests/mysql_lease_mgr_unittest.cc'''
 You should add some sanity check as a first test that connects to the
 database and prints out extended error message with explanations that the
 test environment is broken if connection fails.

 {{{
     MYSQL* status = mysql_real_connect(mysql_, "localhost", "keatest",
 "keatest", "keattest",
                                        0, NULL, 0);
     if (status != mysql_) {
         // educate user how to set up the environment properly
     }
     mysql_close(mysql_);
 }}}

 BasicLease6 test fails for me:
 {{{
 [ RUN      ] MySqlLeaseMgrTest.BasicLease6
 mysql_lease_mgr_unittest.cc:218: Failure
 Value of: second->cltt_
   Actual: 119856
 Expected: first->cltt_
 Which is: 123456
 mysql_lease_mgr_unittest.cc:218: Failure
 Value of: second->cltt_
   Actual: 230967
 Expected: first->cltt_
 Which is: 234567
 mysql_lease_mgr_unittest.cc:218: Failure
 Value of: second->cltt_
   Actual: 230967
 Expected: first->cltt_
 Which is: 234567
 [  FAILED  ] MySqlLeaseMgrTest.BasicLease6 (66 ms)
 }}}
 It seems that the values are off by 3600 seconds. I'm afraid the concerns
 about timezones were not exaggerated.

 Steps I did to run it:
 mysql -u root -p
 > create database keatest;
 > create user 'keatest'@'localhost' IDENTIFIED by 'keatest';
 > grant all privileges on keatest.* to 'keatest'@'localhost';
 > quit
 mysql -u keatest -p keatest < src/lib/dhcp/dhcp

 OpenDatabase test:
 I would check positive case (all parameters valid) first. It will be
 easier to debug. Now the information about failed connection does not give
 you much information.

 Also you need to precisely describe how to set up the environment. I can
 imagine that some test environment may take liberal approach and allow to
 connect to the database to anyone (or at least skip password
 requirements). That would make OpenDatabase test to fail.

 I think we can leave the test as is, but we need some easy to run script
 that will set up the environment and will print out that MySQL access must
 be configure to require user and password.

 CheckTimeConversion:
 This test does not check that the conversion was done properly, just that
 it is reversible. The code could mess up months with minutes and days with
 seconds and the test would still pass.

 Please check boundary conditions. Verify that the expire time is properly
 calculated if:
 - crossing minutes/hours/days/months/years boundaries
 - the code works on February 28th on regular year
 - the code works on February 29th on leap years

 CheckVersion:
 We should have defines for the current versions somewhere, probably in
 mysql_lease_mgr.h.

 printLease6() seems very useful. We could consider moving it to LeaseMgr
 and use it in exception handling, when experiencing critical errors with
 doing something with the lease.

 compareLease6() - can this be implemented as an operator? I haven't given
 much thought to it, but it seems likely that sooner or later someone will
 compare two Lease6Ptr instead of comparing the objects they point out to.

 I think we should defined two == operators:
 - one for comparing two Lease6Ptr objects (it should throw as this
 operation does not make much sense)
 - one for comparing Lease6 structures (it will do what compareLease6()
 does.

 The first operator will be basically a sanity check.

 Do we really need hwaddr in Lease6? I think we should remove it and the
 sooner we do it the better.

 BasicLease6:
 - Please test that it works for IA_NA leases. You check IA_TA and IA_PD -
 the two types that we don't care about.
 - Before the test begins, make sure you delete everything from the lease6
 tables. There could easily be a leftover e.g. from previous test that
 failed in the middle.
 - Make sure you also clean up after the test.

 - The test doesn't actually put anything in the database! Your changes are
 visible in your connection only. Should a second user connected at the
 same time, he wouldn't see anything in the database. This is essential to
 check it from the multi-process perspective. While it doesn't make sense
 to spend much time on multi-process/threading now, I suggest that you do
 commit, close the connection, open it second time and verify that the data
 is still there.

 Before putting anything in the database, check that there are no leases
 present that you are about to add. Even if you add "DELETE * from Lease6"
 query at the beginning of each test, it is still useful that the backend
 does not return bogus leases.

 Please add boundary checks. Verify that:
 - 2^32-1 can be stored in iaid_, preferred and valid lifetimes.*
 - 128 byte long DUID can be stored and retrieved and that its content is
 intact*
 - that hardware address of 20 bytes can be stored and retrieved*
 - check if the backend refuses to accept larger values.
 - check that CLTT is able to properly store February 29th on leap years
 - check that the backend will refuse dat Feb. 29th on regular year.
 - check that prefix_len larger than 128 will be refused.
 - check that IPv4 address will be refused in lease6.*
 - please check that the time operations are working in timezones different
 than GMT (I'm not sure if UK is on daylight saving or not).

 I think that points marked with * are important and should be implemented
 now, others can be deferred.

 Ok, that concludes the review of all changes up to
 15bc004f2076bd04df69df0efa59728b795ee7c5.

 That's a very good progress. I would like to also have updateLease6()
 implemented. I think it should return false if the update fails. With
 updateLease6() and the current code, I'll be able to start integration
 with the allocation engine code (and focusing on v6). At the same time,
 you could continue your work on remaining v6 calls and v4 code.

 I think it makes sense to split this ticket and merge the code soon,
 rather than wait till all v4 and v6 calls are implemented.

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