Opened 7 years ago

Closed 7 years ago

#2342 closed task (complete)

MySQL IPv6 lease database access functions

Reported by: stephen Owned by: stephen
Priority: medium Milestone: Sprint-DHCP-20121115
Component: database-all Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description (last modified by stephen)

Implementation of the concrete MySQL implementation of the abstract lease database API. This ticket is only for IPv6 functionality.

Subtickets

Change History (29)

comment:1 Changed 7 years ago by stephen

  • Owner set to stephen
  • Status changed from new to assigned

comment:2 Changed 7 years ago by stephen

  • Owner changed from stephen to UnAssigned

This is in for an intermediate review:

  • for synchronising with working being done on the allocation engine (for which a backend database is needed).
  • preventing the amount of unreviewed code getting too large.

Notes:

  • The database creation script of #2142 is included as part of this ticket (although it has been somewhat edited). It turns out to be less work to include it here than to merge #2142.
  • The Lease6.address field is a string for the moment - this simplified debugging. It will be converted back to a byte array before this ticket is merged.
  • Building the code requires that configure be run with the --with-mysql-config flag pointing to the mysql_config program (default /usr/bin/mysql_config). Without this flag, the build of the DHCP server will omit all MySQL code and tests.
  • The test programs assume that a database "keatest" (user: keatest, password: keatest) be present. This database must have been created using the dhcpdb_create.mysql script, and the lease6 table should be empty.

comment:3 Changed 7 years ago by stephen

  • Status changed from assigned to reviewing

comment:4 Changed 7 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:5 Changed 7 years ago 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_dhcpla_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.

comment:6 Changed 7 years ago 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.

comment:7 Changed 7 years ago by tomek

  • Owner changed from tomek to stephen

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:

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

comment:8 follow-up: Changed 7 years ago by stephen

Split the modifications into three parts to address the three parts of the review.

Corrections to comment:5 addressed in commit b0ed08d21421ca321502cda1e9811a4bfe560275

configure.ac

Error message "Specified mysql_config program ${MYSQL_CONFIG} is a directory" ...

Combined the two checks into one and modified the error message.

Is the mysql_config program called differently on different systems? Pehaps specifying a directory is sufficient? ...

The --with-botan-config switch points to a configuration program, and I followed that example.

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.

The switch has been renamed --with-dhcp-mysql and the help text tries to make the "DHCP only" usage clear.

To avoid confusion, I've also removed the MySQL listing from the final report.

Why did you change order of the files in lib10_dhcpla_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.

I use vim, so when I sort a list like that I just run it through the external "sort" program. Unfortunately although LC_ALL was set in my .bashrc, it wasn't exported - so sort ignored it and treated "." and "_" as equivalent. This has been corrected.

dhcpdb_create.mysql

Is the type VARBINARY portable?...We may keep it, but let's add a comments section at the end of the schema file about likely portability issues.

Possibly not, but the field is binary, "VARBINARY" seemed proper. From what I understand, this means that sorting and comparison is based on byte values instead of a character set. If we put an index on the column, then I guess VARBINARY will offer slightly better performance (no collation table to check). I've noted this down, but it should be trivial to change if we ever need to.

Length of the client-id field in lease4 table....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.

We have to specify something, and 128 characters seems a suitable maximum. We can change this if we need. I've moved the definition of the sizes of the various buffers to the head of the file and added some documentation (to both the .cc file and the schema definition file) to make it clear where changes are needed if the lengths are altered.

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

Done.

Comment "IA ID" for a field iaid does not give any information..."See section 10 of RFC3315" is another possible comment here.

Changed.

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.

My omission - corrected.

Also, it seems valid, but a bit odd to require doing everything in one transaction here.

Changed.

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

By default, the first field in the table is the primary key and is implictly indexed. I've made this clear by adding the PRIMARY KEY description to that field. :wNote that indexing on a text field - currently used to store an IPv6 address - may be slow. When completed, the address will be stored in a BINARY(16) field, and access will be much faster.

lease_mgr.h

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

Unfortunately that is used elsewhere in BIND 10 - see the .h files in other directories.

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.

This is not really an issue. The problem is that in getLeaseData(), a Lease6 object has to be created and filled in, and it was the lack of a default constructor for the addr_ object that was preventing this. The addr_ object is replaced. However, I've changed the constructor to make it a V6 address by initialising with the string "::".

As for both Lease4 and Lease6 constructors, I think there should be one, but let's not make it default...

It depends what the easiest thing to do is. There are a lot of fields and a constructor in which all those fields have to be set is cumbersome to use. However, if we need one, I don't mind. The only place where one is used is the MySQL code is in MySqlLease6Exchange::getLeaseData() (so far), so adding the constructor is not really an issue. How many other places in the code will it be used?

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.

I think that the lease tables should hold lease data only. Therefore, T1/T2 should probably be removed from the lease structure.

LeaseMgr() constructor. Defining extra type (ParameterMap) makes the interface less robust and actually requires more code in every place where LeaseMgr...

(Comment ignored - overridden by a later comment.)

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

Nor am I.

There is a new method commit(). I'm fine with it, but isn't there another one needed like beginTransaction()?

There is no mysql_start_transaction() call, only mysql_commit() and mysql_rollback(). And the MySQL documentation is annoyingly obtuse on this point. From various sources it appears that with autocommit disabled, a new transaction automatically starts with the first statement after the last commit. So it appears that we don't need one.

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.

#2403 created for that.

LeaseMgrPtr - I very strongly oppose to it. LeaseMgr should be a singleton.

Fair point. The LeaseMgr creation code is now split into two: LeaseMgrFactory::create(dbconfig) to create the lease manager for the specified backend, and LeaseMgrFactory::instance() to return the current LeaseMgr. A destroy() method is also present to clean up - it should be called at program end.

comment:9 Changed 7 years ago by stephen

  • Description modified (diff)
  • Summary changed from MySQL lease database access functions to MySQL IPv6 lease database access functions

comment:10 follow-up: Changed 7 years ago by stephen

Corrections to comment:6 addressed in commit 08daa03beac5d16305734fc434edcc28962205e9

dhcpdb_create.mysql

The field client_id should be called duid.

Changed.

lease_time should be called valid_lifetime

Changed.

lease_mgr.cc

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

(Ignored, see below)

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

What headers?

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.

Will do.

I don't like the idea of shared_ptr to LeaseMgr...

See above regarding the splitting of LeaseMgr creation and access (all now in LeaseMgrFactory).

Please add a comment that we will eventually need to develop some kind of registration mechanism for different backends

Added to lease_mgr_factory.h (which seemed the natural place for the comment).

mysql_lease_mgr.h

Please do not use two leading underscores in MYSQL_LEASE_MGR_H define.

See above regarding what appears to be the norm in BIND 10.

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

Too much cutting and ppasting and not enough attention. Fixed.

Do we want to have default values for the host,db name,user and password?

I've set a default for "host" ("localhost"), but I'd like to leave the rest. "Kea" is our current code name and it is entirely possible that it may be changed before release. Default values for "user" and "password" tend to be discouraged as people leave the defaults set, opening security holes.

Comment for MySqlLeaseMgr() should clarify if the parameters are mandatory or optional.

Done.

It should also mention that DbOperationError or DbOpenError may be thrown.

Done.

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

Added for methods implemented so far. Headers will be updated as more methods are implemented.

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.

I've changed this ticket description to talk about method for V6 operations. Another ticket (#2404) has been created for V4.

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.

Agreed. If a method is virtual in the base class, I like to declare it virtual in derived classes, for documentation purposes.

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.

MySQL stores timestamps in local time (this is documented in the MySQL documentation). I'll make a note to include that in the developer documentation.

StatementIndex. I think the GET_VERSION should come on the list first, as this is the first call to be ever called...

I was going to put the list of statements in alphabetical order.

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

Thanks.

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.

Comment has been updated to explain that the class only handles the MYSQL_BIND structures describing data exchanged with the database.

MySqlLease6Exchange::CreateBindForSend(): Why bind_[7].is_unsigned is set in such a strange way? Using true_ would work.

Missed that one when I created true_ and false_ for the my_bool type. Fixed.

What does "The MySQL documentation" mean in the CreateBindForSend() comment?

Nothing - removed.

MySqlLease6Exchange::getLeaseData() - The exception that throws "invalid lease type returned" should provide more details.

Added more information to the exception.

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

IIRC that was what is was called in the performance test program. It has been changed to "duid".

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.

It uses local time and comments have been added to note this.

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.

Added.

MySqlLeaseMgr::MySqlLeaseMgr(): We should check if mysql_init() returned NULL.

Done - it throws a DbOpenError exception with a suitable message.

Lease4 methods: I would prefer them to throw until they are implemented...

All unimplemented methods (both Lease6 and Lease6) now throw a NotImplemented exception.

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

As "name" is used to denote the database, getName() should return "keatest", which is how it has been implemented. (A "getType()" should return "mysql".) We may want to check

General comments for MySqlLeaseMgr class:

Would it be possible to rename raw_statements_ to text_statements_? Raw may mean different things in this context...

Changed.

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

Not at the moment. The underlying mysql_xxxx() calls can be thread-safe, but you need to use a different database connection for each thread. If we go this way, we may want to relax the idea of a singleton lease manager - we could have multiple lease managers, each with a different connection.

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.

Probably not very signal safe. There are no handlers I am aware of.

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

Files moved to libb10-dhcpsrv

comment:11 follow-up: Changed 7 years ago by stephen

  • Owner changed from stephen to tomek

comment:7 address in commit 89c8076272fb81a9e49b3bdbcd704103707db83f

src/lib/dhcp/tests/Makefile.am

Make sure that the following lines are cleaned up before merging the code:

Removed.

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

Moved.

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.

A message is printed out with the first test stating that if it fails, the environment is broken.

As an aside, I have extended the code to auto-reconnect - although this is mainly because the client will disconnect from the database if inactive for too long.

BasicLease6 test fails for me:

This was due to the timezone problem (described above), now corrected.

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.

Done.

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.

Will do in the associated documentation. Good point about the password.

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.

Agreed.

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

I'm a bit wary of doing that because of DST issues - setting a "struct tm" and converting to a time_t requires knowledge of whether DST is in effect at that time. And the reverse conversion will use the local tables to calculate DST. However, I've modified the test to check that the MYSQL_TIME produced is the one that I would calculate by adding cltt and the valid lifetime and converting using localtime_r. The reverse conversion checks that it reproduces the original time_t value.

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

Done.

printLease6() seems very useful. We could consider moving it to LeaseMgr.

Added it as lease6::toText() to fit it with other text conversion stuff.

compareLease6() - can this be implemented as an operator?

Done. Added both operator==() and operator!=().

I think we should defined two == operators:
one for comparing two Lease6Ptr objects (it should throw as this operation does not make much sense)

I don't think we need this - if we want to compare two pointed-to leases for equality, we can dereference them before comparison.

one for comparing Lease6 structures (it will do what compareLease6() does.

Done - see above.

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

Probably not. Remove. (Err.. BTW, you were the one who wrote lease_mgr.h and first put it in the Lease6 structure!)

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.

Done.

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.

Done. Added clearAll() to the test fixture class. This is run both before the test and after it.

The test doesn't actually put anything in the database...

Now checked - added reopen() to the test fixture class.

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.

Done - see above with the clearAll() function.

Please add boundary checks. Verify that:
232-1 can be stored in iaid_, preferred and valid lifetimes.

Checked. Values near 232-1 have been used to ensure that the different fields can be differentiated.

128 byte long DUID can be stored and retrieved and that its content is intact

Done.

that hardware address of 20 bytes can be stored and retrieved

A few lines ago, you suggested that the Lease6 hardware address should be removed. This has been done.

check if the backend refuses to accept larger values.

For what - apart from prefix_len, for everything the maximum value is set by the data type.

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.

The backend accepts data in time_t format and will convert it to the appropriate local time. Similarly, when it retrieves data, the result is converted to time_t format. So these checks really boil down to whether MySQL can handle leap years, which is out of scope for these checks.

check that prefix_len larger than 128 will be refused.
check that IPv4 address will be refused in lease6.

These properly belong in the Lease6 class (and it is becoming a class, rather than a struct). The database layer should assume that given a Lease6 object, the data in it is correct. When it retrieved data, it should be that the Lease6 object being created from the data detects that it is incorrect. Otherwise, knowledge of what constitutes valid data in Lease4/6 is split into several places. A new ticket (#2405) has been created for it.

comment:12 in reply to: ↑ 8 Changed 7 years ago by tomek

Replying to stephen:

The --with-botan-config switch points to a configuration program, and I followed that example.

Ok.

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.

The switch has been renamed --with-dhcp-mysql and the help text tries to make the "DHCP only" usage clear.

To avoid confusion, I've also removed the MySQL listing from the final report.

Why? Printing out MySQL parameters is very useful, we just need to make them as DHCP-only. It makes sense to also label existing SQLite as DNS-only. Perhaps existing "Features:" could be renamed in "DNS features:" and the MySQL section your removed could be named "DHCP Features:".

I use vim, so when I sort a list like that I just run it through the external "sort" program. Unfortunately although LC_ALL was set in my .bashrc, it wasn't exported - so sort ignored it and treated "." and "_" as equivalent. This has been corrected.

I didn't know that you can sort such things automatically. My emacs-fu seems weak today :)

dhcpdb_create.mysql

Is the type VARBINARY portable?...We may keep it, but let's add a comments section at the end of the schema file about likely portability issues.

Possibly not, but the field is binary, "VARBINARY" seemed proper. From what I understand, this means that sorting and comparison is based on byte values instead of a character set. If we put an index on the column, then I guess VARBINARY will offer slightly better performance (no collation table to check). I've noted this down, but it should be trivial to change if we ever need to.

Our customer asks to do two conflicting things at the same time: keep portability and be as efficient as possible (with implies using MySQL specific things). Since we decided to focus on performance, I think with the portability we just need to document what we think will be the issues to solve once someone asks us to run Oracle or PostgreSQL. A simple list of potential issues will do just fine. Otherwise we could spend months on finding super portable solutions without even what we are trying port to.

lease_mgr.h

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

Unfortunately that is used elsewhere in BIND 10 - see the .h files in other directories.

Mistakes made elsewhere does not warrant creating new mistakes. Double underscore defines are reserved for compiler's internal use and no coding guidelines or existing practices can overrule that. This question may be useful:
http://stackoverflow.com/questions/224397/why-do-people-use-double-underscore-so-much-in-c

As for both Lease4 and Lease6 constructors, I think there should be one, but let's not make it default...

It depends what the easiest thing to do is. There are a lot of fields and a constructor in which all those fields have to be set is cumbersome to use. However, if we need one, I don't mind. The only place where one is used is the MySQL code is in MySqlLease6Exchange::getLeaseData() (so far), so adding the constructor is not really an issue. How many other places in the code will it be used?

There is another one in allocation engine. I think that adding a constructor will be easier to add new fields later (remember that we still need to extend the lease structures with DDNS and failover stuff). It will be easy to spot places where the structure is initialized once you add new parameter. However, having said that I've added a constructor in #2324, so you don't need to do it.

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.

I think that the lease tables should hold lease data only. Therefore, T1/T2 should probably be removed from the lease structure.

Ok. Let's do that once we merge #2324 and #2342.

There is a new method commit(). I'm fine with it, but isn't there another one needed like beginTransaction()?

There is no mysql_start_transaction() call, only mysql_commit() and mysql_rollback(). And the MySQL documentation is annoyingly obtuse on this point. From various sources it appears that with autocommit disabled, a new transaction automatically starts with the first statement after the last commit. So it appears that we don't need one.

Remember that this interface is generic, so we will likely have to take a look at how other backends do it.

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.

#2403 created for that.

Ok.

LeaseMgrPtr - I very strongly oppose to it. LeaseMgr should be a singleton.

Fair point. The LeaseMgr creation code is now split into two: LeaseMgrFactory::create(dbconfig) to create the lease manager for the specified backend, and LeaseMgrFactory::instance() to return the current LeaseMgr. A destroy() method is also present to clean up - it should be called at program end.

Ok, great. Another two possible cases when we want to destroy leasemgr is when we are changing backends (not likely to happen in 2012 or 2013) and also when running several tests (happened already for allocation engine).

comment:13 in reply to: ↑ 10 Changed 7 years ago by tomek

Replying to stephen:

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

What headers?

There are 11 includes in lease_mgr.cc. I seriously doubt we need them all, especially that we are not doing anything fancy there.

mysql_lease_mgr.h

Please do not use two leading underscores in MYSQL_LEASE_MGR_H define.

See above regarding what appears to be the norm in BIND 10.

"what appears to be the norm" in a project cannot overrule standard restrictions.

StatementIndex. I think the GET_VERSION should come on the list first, as this is the first call to be ever called...

I was going to put the list of statements in alphabetical order.

I was overthinking this. For a moment I got the (presumably false) impression that this will be an enum exported in a library and that you could say send query X. If that were true, the first query you ever call having value of 1 would be beneficial. Since we are not likely to do it, you can safely ignore my comment. Those enum values will change between versions when we add new calls, but this will not be a problem.

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

IIRC that was what is was called in the performance test program. It has been changed to "duid".

Ah yes. Dhcp-ubench was a strange mix. I tried to mimic additional load imposed by DHCPv6 data, while keeping the simplicity of 4 byte address.

It uses local time and comments have been added to note this.

Ok, great. ISC-DHCP users will like that.

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

As "name" is used to denote the database, getName() should return "keatest", which is how it has been implemented. (A "getType()" should return "mysql".) We may want to check

Ok. We can stick with that, but my original impression was that getName() is really getBackendName(). See existing comemnts in LeaseMgr::getName().

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

Not at the moment. The underlying mysql_xxxx() calls can be thread-safe, but you need to use a different database connection for each thread. If we go this way, we may want to relax the idea of a singleton lease manager - we could have multiple lease managers, each with a different connection.

Ok. Let's keep it as they are now. I think the simplest way to go for now would be multi-process. We could start several server processes at the same time and each would create its own singleton and do allocations. I wrote allocation engine with such a model in mind. But let's not spend time on it. Keep things as they are now.

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.

Probably not very signal safe. There are no handlers I am aware of.

It's not only a matter or handlers. Some functions may fail if they are interrupted by a call (even when signal handler is implemented).

comment:14 in reply to: ↑ 11 Changed 7 years ago by tomek

Replying to stephen:

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

I'm a bit wary of doing that because of DST issues - setting a "struct tm" and converting to a time_t requires knowledge of whether DST is in effect at that time. And the reverse conversion will use the local tables to calculate DST. However, I've modified the test to check that the MYSQL_TIME produced is the one that I would calculate by adding cltt and the valid lifetime and converting using localtime_r. The reverse conversion checks that it reproduces the original time_t value.

Ok. We'll have the opportunity to check that next week - Poland (and most of Europe as well) is moving to winter time.

I'm afraid that is something we'll need to sort out before first release, because clients losing leases twice a year (and on end of February every 4 years) would be very bad.
The amount of coding here is minimal, but non-trivial research is needed. I think it is a worthy candidate for a separate ticket.

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

Probably not. Remove. (Err.. BTW, you were the one who wrote lease_mgr.h and first put it in the Lease6 structure!)

I honestly admit to my mistakes. At first I thought that the lease6 structure would look empty.

that hardware address of 20 bytes can be stored and retrieved

A few lines ago, you suggested that the Lease6 hardware address should be removed. This has been done.

Ok, great.

check if the backend refuses to accept larger values.

For what - apart from prefix_len, for everything the maximum value is set by the data type.

Does the backend allow storing prefixes /129 and larger? That shouldn't be allowed.

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.

The backend accepts data in time_t format and will convert it to the appropriate local time. Similarly, when it retrieves data, the result is converted to time_t format. So these checks really boil down to whether MySQL can handle leap years, which is out of scope for these checks.

Ok, so we're covered here. Great.

check that prefix_len larger than 128 will be refused.
check that IPv4 address will be refused in lease6.

These properly belong in the Lease6 class (and it is becoming a class, rather than a struct). The database layer should assume that given a Lease6 object, the data in it is correct. When it retrieved data, it should be that the Lease6 object being created from the data detects that it is incorrect. Otherwise, knowledge of what constitutes valid data in Lease4/6 is split into several places. A new ticket (#2405) has been created for it.

Ok, fair point. The only reason I started lease4/6 as a structure is that I didn't want to spend many hours writing trivial getters/setters, tests for them, then dealing with lifetime issues for references returned to internal vectors etc.

Ok, there's one more issue: BasicLease6 test started failing for me. Since the schema changed, I manually drop table'd all tables and then run dhcpdb_schema.mysql again.

[ RUN ] MySqlLeaseMgrTest?.BasicLease6
mysql_lease_mgr_unittest.cc:264: Failure
Value of: second->cltt_

Actual: -66464956333

Expected: first->cltt_
Which is: 234567
[ FAILED ] MySqlLeaseMgrTest?.BasicLease6 (116 ms)

I just looked at the BasicLease6 test and you seem to use EXPECT_TRUE(l_returned); If the lease is NULL, you print a failure, but continue to use the value. It should be ASSERT_TRUE.

It fails with the l3 compare in line 367. I'll try to investigate it and see if I'll be able to fix it.

comment:15 Changed 7 years ago by tomek

  • Owner changed from tomek to stephen

comment:16 Changed 7 years ago by stephen

  • Owner changed from stephen to tomek

configure.ac

To avoid confusion, I've also removed the MySQL listing from the final report.

Why? Printing out MySQL parameters is very useful, we just need to make them as DHCP-only. It makes sense to also...

I wanted to avoid over-complicating configure.ac, but it turned out not to be too much trouble to add this. It only appears if the --with-dhcp-mysql has been specified, to avoid confusing people building DNS.

dhcpdb_create.mysql

Our customer asks to do two conflicting things at the same time: keep portability and be as efficient as possible (with implies using MySQL specific things).

I think the aim is that we don't use anything in MySQL that we can't duplicate elsewhere. Here, all we are talking about is the format by which the data is stored in the database. That can vary between databases: providing we write the database layer appropriately, the code will work on any database. However, if we were to rely on stored procedures, we would have difficultly with something like SQLite3 which doesn't have that feature.

lease_mgr.h

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

Unfortunately that is used elsewhere in BIND 10 - see the .h files in other directories.

Mistakes made elsewhere does not warrant creating new mistakes. Double underscore defines are reserved for compiler's internal use ...

Good point. Double underscores removed.

lease_mgr.cc

There are 11 includes in lease_mgr.cc. I seriously doubt we need them all, especially that we are not doing anything fancy there.

Turns out that all but lease_mgr.h can be removed. Done.

mysql_lease_mgr_unittest.cc

Ok, there's one more issue: BasicLease6 test started failing for me.

That appears to be down to overflows when adding a large number of seconds to the cltt field - the time can go beyond 2038. Ticket #2413 has been created for that. In the meanwhile, the tests deliberately don't cause an overflow and a TODO has been added to the code.

General
The code now has the basic add/get/update lease6 capability. Variants of these are defined using fields other than the address as the key. However, if this is enough for now, I suggest we merge the current ticket and create a new ticket for the remainder.

If you agree, the suggested ChangeLog entry is:

nnn.	[func]		stephen
	Add initial version of a MySQL backend for the DHCP code. (This
        is enabled by specifying --with-dhcp-mysql on the "configure"
        command line.)  The backend implements the basic IPv6 lease access
        functions - add lease, get lease, delete lease, update lease.
	(Trac #2342, git xxx)

comment:17 Changed 7 years ago by stephen

  • Owner changed from tomek to stephen

comment:18 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

A review is needed of the the changes in commit a0f0141e8e52b1ebc6f0bfa17b13d6b98da3e72c and beyond. The changes comprise:

  • Fixing of the above issues
  • Implementation of new getLease6() methods
  • Reorganisation of the way the SQL statements are defined
  • Having unit tests create the schema in the database (instead of having it already defined).
  • Adding documentation

To run the unit tests, MySQL must be installed on the system, and the "configure" run with the --with-dhcp-mysql switch. The file src/lib/dhcp/database_backends.dox contains instructions for running the unit test.

comment:19 Changed 7 years ago by marcin

Reviewed commit f25544eeedb66a4f5bb0e0c41387e2e58555daa9

database_backends.dox
Typo here: A lease manager is instantiated through a the LeaseMgrFactory?....
(a or the should be left)

Typo here: ... is is controlled by the argument.

Typo here: ...there are certain pre-requisites for scuccessfully running the unit tests.

Awkward formatting when enumerating pre-requisites for setting up different (currently MySQL only) databases. In particular, These are database-specific: with colon which makes me expect some form of bullets or at least indentation before MYSQL and other databases which may be added in the future. Why not to add sub-sections like Pre-requisites for MYSQL etc. and replace '' These are database-specific: '' with '' Database-specific setup for running unit tests are given in the following sub-sections. '' ?

Consider adding references/links to the MySQL documentation on how to create MySQL users and schema:

Also, suggest that whenever you write keatest in a sentence it is taken between quotation marks. This will make it clear for a reader that it is user name, password or data base name rather than two separate words: kea and test without accidentally deleted space between them.

lease_mgr.h
updateLease6: This function is pure and thus it doesn't throw any exception so I suggest that @exception tags are removed from the documentation. Also, since we may have multiple lease managers derived from LeaseMgr? base class, listed exceptions may be not relevant to them (e.g. DbOperationError?).

updateLease6: Although not mandated in style guidelines, I suggest that @exception tags are replaced with @throw tags for two reasons:

  • @throw appears to be more common in BIND10 code
  • @throw is simply shorter

This applies to the following files as well:

  • mysql_lease_mgr.h
  • lease_mgr_factory.h

mysql_lease_mgr.cc
Minor: Included headers: according to BIND10 coding style, if possible we should be first including project headers, then library headers and lastly system headers.

MySqlLease6Exchange doxygen documentation: There is extra new line between doxygen description of this class and line starting with Note - .... In such case the note is not incorporated into the doxygen description of the class. Is this intentional? I think should be. If you agree, please also consider using @note tag for the note.

MySqlLease6Exchange constructor documentation: Thanks for putting the note why you perform memset on addr6_buffer_ etc. However when I read it in the html form I get confused what ''other initializations'' it is referring to. I suggest that you clarify this comment so it can be understood without a need to read the code.

getLease6: It is not necessary to do complex cast and usually removing "constness" is bad idea too. Can't you just initialize duid_vector by value:

vector<uint8_t> duid_vector = duid.getDuid();
...
inbind[0].buffer = &duid_vector[0];

This may have some performance implications but is safer and simpler.

getLease6: In the try-catch statement you catch isc::BadValue? exception (which may be emitted by getLeaseData()) and free some allocated resources if this exception is caught. However it appears to me that getLeaseData() may throw other exceptions too. That is for example: bad_alloc if new operator fails here:

result->duid_.reset(new DUID(duid_buffer_, duid_length_));

Also DUID's constructor emits isc::OutOfRange? when DUID has invalid length. These exceptions will not be caught and mysql_stmt_free_result(...) will not be called in getLease6 making it prone to resource leaks.

mysql_lease_mgr.h
updateLease6 doxygen: invalid parameter name lease4 (instead of lease6). Also similar issue in @brief description.

deleteLease6 doxygen: same as above

mysql_lease_mgr_unittest.cc
MysqlLeaseMgrTest?: suggest that you try-catch around this:

LeaseMgrFactory::create(validConnectionString());

and output some more information about the reason of failure just like you do in the same file in line 416. Otherwise, when MySQL is not configured, constructor will emit exception which will be logged similar to this:

unknown file: Failure
C++ exception with description "Access denied for user ..."

which doesn't mention that it was a problem to access database.

comment:20 Changed 7 years ago by marcin

  • Owner changed from marcin to stephen

comment:21 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

database_backends.dox

Typo here: A lease manager is instantiated through a the LeaseMgrFactory....

Typo here: ... is is controlled by the argument.

Typo here: ...there are certain pre-requisites for scuccessfully running the unit tests.

Done.

Awkward formatting when enumerating pre-requisites for setting up different (currently MySQL only) databases. In particular, These are database-specific...

Done

Consider adding references/links to the MySQL documentation on how to create MySQL users and schema...

I've avoided doing that, partly because anyone installing Kea into a MySQL system should know how MySQL works (presumably they had to install MySQL to start with) but mainly because the references suggested are MySQL version-specific, so will become outdated and could be removed.

Also, suggest that whenever you write keatest in a sentence it is taken between quotation marks.

I've chosen to use an emphasis instead of quotation marks (so there is no confusion as to whether the quotation marks are part of the string the user should enter). In addition, I've emphasised some other keywords.

lease_mgr.h

updateLease6: This function is pure and thus it doesn't throw any exception so I suggest that @exception tags are removed from the documentation...

Done.

updateLease6: Although not mandated in style guidelines, I suggest that @exception tags are replaced with @throw tags for two reasons...

All occurrences of "@exception" in the DHCP library replaced with "@throw". In addition, all exceptions now include the namespace to ensure they appear as hyperlinks in the Doxygen documentation.

mysql_lease_mgr.cc

Minor: Included headers: according to BIND10 coding style, if possible we should be first including project headers, then library headers and lastly system headers.

Done.

MySqlLease6Exchange doxygen documentation: There is extra new line between doxygen description of this class and line starting with Note -

@note tag used

MySqlLease6Exchange constructor documentation: Thanks for putting the note why you perform memset on addr6_buffer_ etc. However when I read it in the html form I get confused what other initializations it is referring to. I suggest that you clarify this comment so it can be understood without a need to read the code.

Done.

getLease6: It is not necessary to do complex cast and usually removing "constness" is bad idea too. Can't you just initialize duid_vector by value:...

It is possible, but as you note, this involves a redundant copy. As we have to focus on performance, I'm wary about introducing such penalties: not because this one will have a significant impact, but because the cumulative effect of all these copies through the code may do. I've kept the "const_cast" in, but have added a an explanatory note describing why the "const" is being removed (because the C interface to MySQL does not use "const").

getLease6: In the try-catch statement you catch isc::BadValue exception (which may be emitted by getLeaseData()) and free some allocated resources if this exception is caught. However it appears to me that getLeaseData() may throw other exceptions too...

Good point. I've put the call to mysql_stmt_free_result in the destructor of an object that is created whenever
mysql_stmt_fetch is used. That way, mysql_stmt_free_result should be called however getLease6() or any other method exits.

mysql_lease_mgr.h

updateLease6 doxygen: invalid parameter name lease4 (instead of lease6). Also similar issue in @brief description.

deleteLease6 doxygen: same as above

Fixed.

mysql_lease_mgr_unittest.cc

MysqlLeaseMgrTest: suggest that you try-catch around this:

LeaseMgrFactory::create(validConnectionString());

...

Done. All exception are caught, the message output and the exception re-thrown. As the exception may not derive from an ISC exception (and so may not have the "what" message), this seems the way to give most information.

comment:22 Changed 7 years ago by marcin

  • Owner changed from marcin to stephen

Reviewed commit b0ad16d99eb0bc2148976a31c4dbf7ff0ed5516a

As we discussed on jabber... when I tried multithreaded build I found race condition occuring between two threads that were creating libdhcp++ and libdhcp_srv. This is because of the dependency between these libraries which has been introduced with commit:

[2342] Add programmer documentation.

As you suggested I moved the duid{.h, .cc} to libdhcp_srv and removed the dependency. However once we come up with the permanent solution for the race conditions we should revert this change.
The #2475 has been submitted to address it.

I fixed some remaining typos in lease_mgr.h and pushed changes.

With respect to complex cast...
Added comments are good but if the performance considerations are your major motivation not to do redundant copy it should be included in the comment too. If you agree, please add extra comment.

comment:23 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

Makefile.am(s)/lease_mgr.h
Reviewed, all OK

const_cast (in mysql_lease_mgr.cc)
Comments updated.

comment:24 Changed 7 years ago by marcin

  • Owner changed from marcin to stephen

All ok. Please merge.

comment:25 Changed 7 years ago by marcin

  • Owner changed from stephen to marcin

comment:26 Changed 7 years ago by marcin

  • Owner changed from marcin to stephen

After the merge of master branch to trac2342 there were a number of files modified to resolve conflicts. Thus this recent commit has been reviewed: 4e7a8f7575704052e70432f1e2508d310e8f6bb0

dhcp6_srv.cc
@todo comments in L42 and L92: comments refer to the code without support for MySQL_LeaseMgr but the support is now being added so comments should be removed.

Dhcp6Srv ctor: Dhcp6Srv is using different database, username and password than MySQL_LeaseMgr. This will confuse people trying to run unit tests with --with-dhcp-mysql. If we are going to release DHCP without concrete solution can't we simply require database (etc.) named "kea" instead of "keatest" in the MySQL_LeaseMgr unit test? That should be farily trivial change. If there is a good reason not to do it, documentation should be updated to point out that "kea" database needs to be created (apart from "keatest" db) to run dhcp6_srv unit tests. I couldn't find such information anywhere. Also, can you create a ticket to reolve this inconsistency and put the reference to it in #2342?

Dhcp6Srv ctor: This constructor may now throw a few exceptions: isc::InvalidParameter? or isc::InvalidType? emitted by LeaseMgrFactory::create. It may also throw a bunch of exceptions from MySqlLeaseMgr? ctor: DbOpenError?, DbOperationError?. Whenever error occurs earlier in the constructor the shudown_ member is reset (true). Shouldn't you catch exceptions emitted by the factory function and set shutdown_? Another good reason to catch exceptions would be to log the error message just like it is done for other errors occuring in the constructor.
As a side note: emitted exceptions are not documented in the constructor's header (would require a few @throw tags there if you decide not to catch exceptions as I suggested).

lib/dhcp/Makefile.am
Your merge brought back the dependency between libdhcp_srv and libdhcp++ libraries that has been earlier removed with the commit 1cee186c837d355ef28b00773bbb145e75444b85. This is causing multi-threaded build issues that we dicussed last week.

lib/dhcp/lease_mgr.cc
getParameter: can parameter be returned by reference?

lib/dhcp/lease_mgr.cc
LeaseMgr? destructor: since destructor is empty, why not to remove it from the implementation and put empty brackets in the header file?

lib/dhcp/lease_mgr.h
LeaseMgr? destructor: other classes will derive from this class so the destructor must be virtual.

lib/dhcp/lease_mgr_factory.h
LeaseMgrFactory? description: @TODO is not a doxygen tag. You should probably replace it with @todo.

Headers inclusion: it appears to me that neither string nor exceptions/exceptions.h has to be included. They are pulled indirectly by lease_mgr.h.

create(): Note: could be replaced with @note in the documentation.

parse(): typo in the @return tag: std::map<>std::string, std::string>

lib/dhcp/lease_mgr_factory.cc
According to BIND10 coding style we should first include project headers, then library headers and finally system headers. The headers are included in the reverse order. Also this whole section could have been removed as those headers are included indirectly anyway:

#include <algorithm>
#include <iostream>
#include <iterator>
#include <map>
#include <sstream>
#include <string>
#include <utility>

parse(): Extra space between exclamation mark and dbconfig

if (! dbconfig.empty())

lib/dhcp/memfile_lease_mgr.h

#endif // MEMFILE_LEASE_MGR_HSE4

garbage HSE4.

There are no tests for LeaseMgrFactory?. I presume that this is because this class is tested indirectly but comment in its header file would be useful to point this out.

Also please run:

cd doc
make devel
less html/doxygen-errors.log

There are many errors there. I did not go through all of them but I think some may be related to your work.

comment:27 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

dhcp6_srv.cc

@todo comments in L42 and L92: comments refer to the code without support for MySQL_LeaseMgr but the support is now being added so comments should be removed.

Removed.

Dhcp6Srv ctor: Dhcp6Srv is using different database, username and password than MySQL_LeaseMgr. This will confuse people trying to run unit tests with --with-dhcp-mysql...

Modified. The parameters to determine what lease manager to use are now passed as an argument to the constructor. The default is the memfile lease manager, which is used for tests.

Dhcp6Srv ctor: This constructor may now throw a few exceptions: isc::InvalidParameter or isc::InvalidType emitted by LeaseMgrFactory::create. It may also throw a bunch of exceptions from MySqlLeaseMgr ctor: DbOpenError, DbOperationError. Whenever error occurs earlier in the constructor the shutdown_ member is reset (true)...

shutdown_ now defaults to true and is only reset to false on successful exit from the constructor. Also, more of the code is now enclosed in a try...catch block, the latter of which logs a message make if the code fails.

lib/dhcp/Makefile.am

Your merge brought back the dependency between libdhcp_srv and libdhcp++ libraries that has been earlier removed with the commit 1cee186c837d355ef28b00773bbb145e75444b85. This is causing multi-threaded build issues that we discussed last week.

Corrected.

lib/dhcp/lease_mgr.cc

getParameter: can parameter be returned by reference?

Possibly, but it is usual to return string parameters by value. Note that this is a non-time-critical operation - the parameters are only accessed when the lease manager is created, which is normally once per program.

lib/dhcp/lease_mgr.cc

LeaseMgr destructor: since destructor is empty, why not to remove it from the implementation and put empty brackets in the header file?

Done. The constructor has also been moved to the header as it is simple.

lib/dhcp/lease_mgr.h

LeaseMgr destructor: other classes will derive from this class so the destructor must be virtual.

Done - this was the cause of the memory leak reported by valgrind.

lib/dhcp/lease_mgr_factory.h

LeaseMgrFactory description: @TODO is not a doxygen tag. You should probably replace it with @todo.

Done

Headers inclusion: it appears to me that neither string nor exceptions/exceptions.h has to be included. They are pulled indirectly by lease_mgr.h.

Removed.

create(): Note: could be replaced with @note in the documentation.

Replaced.

parse(): typo in the @return tag: std::map<>std::string, std::string>

Corrected.

lib/dhcp/lease_mgr_factory.cc

According to BIND10 coding style we should first include project headers, then library headers and finally system headers.

Reordered.

The headers are included in the reverse order. Also this whole section could have been removed as those headers are included indirectly anyway:

I've modified the order but prefer to leave them in: I think that for a .h/.cc file pair, the .cc file should include all headers it needs for variable and function declarations, with the exception of those declared in the corresponding .h file. To rely on other .h files for the inclusion of a header leaves the software open to something suddently not compiling because a #include has been removed from an apparently unrelated header file.

parse(): Extra space between exclamation mark and dbconfig

Removed.

lib/dhcp/memfile_lease_mgr.h

#endif MEMFILE_LEASE_MGR_HSE4

garbage HSE4.

The result of typing when the focus is elsewhere...

There are no tests for LeaseMgrFactory...

There are some, but only for the parameter parsing. A note has been added to the LeaseMgrFactory unit test file to not that the tests for create/instance/destroy are carried out implicitly in the tests for the various concrete lease manager.s

Also please run:
:
There are many errors there. I did not go through all of them but I think some may be related to your work.

Ones related to this change have been corrected.

comment:28 Changed 7 years ago by marcin

  • Owner changed from marcin to stephen

The Dhcp6Srv ctor is taking dbconfig value which defaults to type=memfile and this is ok for tests. The actual config is not initialized for the server to startup right now and I understand that this is because it will be resolved with other ticket. It would be useful to have the reference to this ticket here or in the code.

Otherwise, changes are ok and can be merged.

comment:29 Changed 7 years ago by stephen

  • Resolution set to complete
  • Status changed from reviewing to closed

It would be useful to have the reference to this ticket here or in the code.

This is ticket #2472

Merged into master in commit c7defffb89bd0f3fdd7ad2437c78950bcb86ad37

Note: See TracTickets for help on using tickets.