BIND 10 #3181: perfdhcp: Add support to release prefixes and measure server response rate

[BIND 10 Development]

#3181: perfdhcp: Add support to release prefixes and measure server response rate
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:
                Type:  enhancement   |  marcin
            Priority:  medium        |                       Status:
           Component:  perfdhcp      |  reviewing
            Keywords:                |                    Milestone:  DHCP-
           Sensitive:  0             |  Kea1.0-alpha
         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:

 ---------------------------------------------------------------------
 command.cc

 The usage text is a littel confusing:

 {{{
         "-f<renew-rate>: A rate at which IPv6 Renew requests are sent
 to\n"
         "    a server. The sum of this value and release-rate must be
 equal\n"
         "    or lower than the rate specified as -r<rate>. If -r<rate>
 is\n"
         "    not specified, this parameter must not be specified too.\n"
         "-F<release-rate>: A rate at which IPv6 Release requests are sent
 to\n"
         "    a server. The sum of this value and renew-rate must be equal
 or\n"
         "    lower than the rate specified as -r<rate>. If -r<rate> is
 not\n"
         "    specified, this parameter must not be specified too.\n"
 }}}

 How about something like this:

 {{{
         "-f<renew-rate>: Rate at which IPv6 Renew requests are sent to\n"
         "    a server. This value is only valid when used in conjunction
 with
         "    the exchange rate (given by -r<rate>).  Furthermore the sum
 of
         "    this value and the release-rate (given by -F<rate) must be
 equal
         "    to or less than the exchange rate.\n"
         "-F<release-rate>: Rate at which IPv6 Release requests are sent
 to\n"
         "    a server. This value is only valid when used in conjunction
 with
         "    the exchange rate (given by -r<rate>).  Furthermore the sum
 of
         "    this value and the renew-rate (given by -f<rate) must be
 equal
         "    to or less than the exchange rate.\n"
 }}}


 Insert "exchange" before the word "rate"  in  "than the rate specified",
 I think makes this a little easier to keep straight.

 {{{
 +    check((getRate() != 0) && (getRenewRate() + getReleaseRate() >
 getRate()),
 +          "The sum of Renew rate (-f<renew-rate>) and Release rate"
 +          " (-F<release-rate>) must not be greater than the rate
 specified"
 +          " as -r<rate>");


 +    check((getRate() == 0) && (getReleaseRate() != 0),
 +          "Release rate specified as -F<release-rate> must not be
 specified"
 +          " when -r<rate> parameter is not specified");
 }}}

 This is up to you.

 --------------------------------------------------------------------
 command_options_unittest.cc
 You are missing tests for "negative" rate values, such as:

    process("perfdhcp -6 -r 10 -f -5 -l ethx all");

 This will throw.

 --------------------------------------------------------------------
 test_control_unittests.c

 Brace on line 781 is tabbed wrong. ;)

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

 processReceivedPacket6

 The logic within in th DHCPV6_REPLY block, could use some commentary. It
 is
 a clever arrangement but it is not at all obvious what's going on.

 --------------------------------------------------------------------
 stats_mgr_unittests.cc

 In this test, TEST_F(StatsMgrTest, ExchangeToString),

 Why do you add the exchanges?

 {{{
     // Test DHCPv6 specific exchange names.
     StatsMgr6 stats_mgr6;
     stats_mgr6.addExchangeStats(StatsMgr6::XCHG_SA);
     stats_mgr6.addExchangeStats(StatsMgr6::XCHG_RR);
     stats_mgr6.addExchangeStats(StatsMgr6::XCHG_RN);
     stats_mgr6.addExchangeStats(StatsMgr6::XCHG_RL);
 }}}

 Ostensibly this test is verifying the exchangeToString() method, which
 does
 not need an exchange to exist to decode the name.  On the other hand, if
 you
 are also testing the ability to add an exchange of each type, then perhaps
 each of the above should be wrapped with an EXPECT_NO_THROW.

 ------------------------------------------------------------------
 rate_control.h

 I think you should explain the basic rate calculation and the role of
 aggressivity either in the file commentary or in getOutboundMessageCount
 commentary.  It wasn't until I got into the logic of this method that I
 understood what it did.


 line 29, "in the same time" should be "at the same time" ;)


 line 37,  "on the same level"  should be "at the same level"

 line 163 change:
 "updateSendDue has been in the past"
  to  this
 "updateSendDue is in the past"

 line 44, remove the word "Typically".

 ----------------------------------------------------------------------
 rate_control.cc

 The second constructor only validates aggressivity. Shouldn't it also
 validate rate?  Rate cannot be negative can it?

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

 rate_control.h - setAggressivity() does no validation, neither doee
 setRate().
 Shouldn't they?

 ----------------------------------------------------------------------
 rate_control_unittests.cc

 line wrap at line 101.

 ----------------------------------------------------------------------
 rate_control_unittests.cc

 at line 137,  you have EXPECT_GT(count, 0).  Since this is using the
 default
 constructor with rate of 0 and aggressivity of 1, the result is always
 going
 to be 1 right?

 Since you know the formula for the "due exchanges" you should be able to
 construct a simple test that verifies you get the right number of due
 exchanges
 , given an amount of time and the ticks_per_second.  The latter is the
 only
 value that may vary from platform to platform.   Perhaps you could devise
 such a test?

 ----------------------------------------------------------------------
 rate_control_unittests.cc

 at line 172, I think this check that rc.send_due is greater than
 last_send_due.
 It should always be a more recent time than last_send_due, not just a
 different
 time.

 at line 185, this test should be less than, not less or equal.  If it
 comes out
 equal the test ought to fail because the time calculations are way off.

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

 The unit tests pass on OS-X, debian, and FreeBSD.  Also passes with
 valgrind.
 It also passes cppcheck.


 Perhaps you should create a ticket for the orphan counting issue.  Also
 since this is
 visible behavior, you should mention it in the !ChangeLog entry, yes?

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