BIND 10 #3183: perfdhcp: Add support to renew prefixes and measure server response rate

[BIND 10 Development]

#3183: perfdhcp: Add support to renew prefixes and measure server response rate
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:
                Type:  enhancement   |  marcin
            Priority:  high          |                       Status:
           Component:  perfdhcp      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20131016
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * owner:  tmark => marcin


Comment:

 This was an intricate bit of work.  I think you wove it in rather nicely.

 packet_storage.h

 "list of Rely messages sent"  change Rely to Reply.

 ----------------------------------------------

 General:

 You seemed concerned with random access performance.  Seems like a
 std::vector
 might be faster than a std::list in this case.  You are always adding to
 the
 end, you could preallocate to a large size.  Random access of vectors is
 much faster.

 Just a thought.

 ----------------------------------------------

 packet_storage_unittest.cc

 TEST_F(PacketStorageTest, getNext)

 1. line 70 in TEST_F(PacketStorageTest, getNext), seems to be an
 extraneous call
 to storage_.getNext().

 2. What is the purpose of the lines 72-76?  What is tested here that is
 not tested above them?

 ----------------------------------------------

 packet_storage_unittest.cc

 General comment, there are no tests yet for Pkt4.

 ----------------------------------------------

 packet_storage_unittest.cc

 TEST_F(PacketStorageTest, getRandom)

 1. line 93 seems to be an extraneous call to storage_.getNext().

 2. What is the purpose of the lines 75-100?  What is tested here that is
 not tested above them?

 ----------------------------------------------

 packet_storage_unittest.cc

 TEST_F(PacketStorageTest, getNextAndRandom)

 1. line 113 is not needed.

 2. At line 128 you expect the transid of the packet to be 100. How is this
 reliable if the access is random?  Shouldn't it be about 50/50 between the
 two ids?  If its always the first id then how is it random?

 Maybe you need a test the demonstrates access is actually random.
 Since you create packets with transids 0 to STORAGE_SIZE - 1, you could do
 something like this:
 {{{
     cnt_equals = 0;
     for (int i = 0; i < STORAGE_SIZE; ++i) {
         Pkt6Ptr packet = storage_.getRandom();
         cnt_equals += (i == packet->getTransId() ? 1 : 0);
     }

     // if the number of times id is equal to i, is the same as the number
     // of elements then they were NOT accessed randomly.
     // The odds of 20 elements being randomly  accessed sequential order
     // is nil isn't it?
     EXPECT_NE(cnt_equals, STORAGE_SIZE);
 }}}

 ----------------------------------------------

 packet_storage_unittest.cc

 TEST_F(PacketStorageTest, clear)

 This test should verify that retrieval still works following a partial
 clear.
 In other words, it should verify that you can still do a getNext() and a
 getRandom() after clearing out the first 10.

 ----------------------------------------------

 command_options.cc

 line 733  change "freater" to "greater".

 ----------------------------------------------

 command_options.cc

 1. Why is renew rate only allowed for IPv6?  Is IPv4 going to be added?

 2. Shouldn't -i and -f be mutually exclusive?  If we aren't accepting
 leases,
 we wont get DHCP6_REPLYs, so we won't generate renewals.

 --------------------------------------------------

 test_control.h

 Commentary for sendRenew() should mention the use of the cached replies.
 Rather than:

 "This function will try to identify an existing lease for which a
 Renew..."

 something like:

 "This function will select an existing lease from the Reply packet
 cache..."

 Makes it a bit clearer.

 --------------------------------------------------
 test_control.cc

 TestControl::sendRenew()

 This method updates last_renew_ whether it sends or not. So really, this
 value
 is "last time I called sendRenew".  If you intend for it to accurately
 reflect
 the last time a renew was sent it shouldn't be updated until after the
 send()
 succeeds.

 --------------------------------------------------

 test_control.cc

 General, there is a fair amount of complexity in clearing out the renewal
 "cache".  An alternative would be not create more if they are not needed.
 Perhaps determine a cap at runtime based on the rate(s) and if the cap is
 reached stop adding new ones?  Seems like it would be faster and easier to
 stop adding unless you until you  more than to create too many and delete
 them
 periodically.

 --------------------------------------------------

 test_control_unittests.cc

 TEST_F(TestControlTest, createRenew)

 This test could use a bit more inline commentary.

 --------------------------------------------------

 Unit tests pass, and under valgrind show no leaks.
 Builds with clang on OS-X without issues
 cppcheck was clean.

 I tested the -f option against a BIND10 server under Debian6 and it works.
 KEA sees the renewals and responds accordingly, perfdhcp records and
 reports the
 results as expected.

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