BIND 10 #3181: perfdhcp: Add support to release prefixes and measure server response rate
BIND 10 Development
do-not-reply at isc.org
Tue Dec 10 13:11:08 UTC 2013
#3181: perfdhcp: Add support to release prefixes and measure server response rate
-------------------------------------+-------------------------------------
Reporter: marcin | Owner: tmark
Type: enhancement | Status:
Priority: medium | reviewing
Component: perfdhcp | Milestone: DHCP-
Keywords: | Kea1.0-alpha
Sensitive: 0 | Resolution:
Sub-Project: DHCP | CVSS Scoring:
Estimated Difficulty: 0 | Defect Severity: N/A
Total Hours: 0 | Feature Depending on Ticket:
| Add Hours to Ticket: 0
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by marcin):
* owner: marcin => tmark
Comment:
Replying to [comment:5 tmark]:
> ---------------------------------------------------------------------
> command.cc
>
> The usage text is a littel confusing:
littel? What is that? ;)
>
> {{{
> "-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"
> }}}
>
Yes. This reads better. Thank you.
>
> 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.
added.
>
> --------------------------------------------------------------------
> 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.
>
Added as suggested.
> --------------------------------------------------------------------
> test_control_unittests.c
>
> Brace on line 781 is tabbed wrong. ;)
Fixed.
>
> --------------------------------------------------------------------
> 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.
Added a commentary. Hopefully it is sufficient.
>
> --------------------------------------------------------------------
> 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.
>
What was I thinking?! To be honest I don't know why I thought I had to add
exchanges to test this function. Apparently, I did it for some historical
reasons and later just blindly followed that stupid habit. I now made this
function static.
> ------------------------------------------------------------------
> 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.
>
I added a commentary.
>
> line 29, "in the same time" should be "at the same time" ;)
>
>
> line 37, "on the same level" should be "at the same level"
I think I am going to remember that til end of my life now.
>
> line 163 change:
> "updateSendDue has been in the past"
> to this
> "updateSendDue is in the past"
Fixed.
>
> line 44, remove the word "Typically".
Removed.
>
> ----------------------------------------------------------------------
> rate_control.cc
>
> The second constructor only validates aggressivity. Shouldn't it also
> validate rate? Rate cannot be negative can it?
It should. Throw an exception if it is wrong.
>
> ----------------------------------------------------------------------
>
> rate_control.h - setAggressivity() does no validation, neither doee
setRate().
> Shouldn't they?
>
They do now.
> ----------------------------------------------------------------------
> rate_control_unittests.cc
>
> line wrap at line 101.
Fixed some line wraps in this file.
>
> ----------------------------------------------------------------------
> 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?
Yes. I should have used non-default constructor, which I am doing now.
>
> 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?
>
That is tricky. The number of exchanges depends on the time value which is
not fixed. I remember writing tests of this kind where I tried to
guarantee the certain margin for time values and on slow VMs I had
sporadic failures. I am afraid this test will not be reliable. Besides, I
am thinking to simplify the rate control mechanism. I am not just ready to
do it yet. :)
> ----------------------------------------------------------------------
> 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.
Yes. I corrected that,
>
> 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.
>
No. This function sets the value to a current time and returns. In theory,
it is possible that the check (which happens immediately) happens just
less than microsecond later which effectively means that they are equal.
> ------------------------------------------------------------------------
>
> The unit tests pass on OS-X, debian, and FreeBSD. Also passes with
valgrind.
> It also passes cppcheck.
Thank you QA team! :)
>
>
> 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?
>
I have created #3261 with a low priority.
How about:
{{{
XXX. [func] marcin
perfdhcp: added support for sending DHCPv6 Relese messages at the
specified
rate and measure performance. The orphan messages counters are not
displayed for individual exchanges anymore. The following ticket:
#3261
has been submitted to implement global orphan counting for all
exchange
types.
(Trac #3181, git abcd)
}}}
--
Ticket URL: <http://bind10.isc.org/ticket/3181#comment:6>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list