BIND 10 #3183: perfdhcp: Add support to renew prefixes and measure server response rate
BIND 10 Development
do-not-reply at isc.org
Fri Oct 25 10:08:21 UTC 2013
#3183: perfdhcp: Add support to renew prefixes and measure server response rate
-------------------------------------+-------------------------------------
Reporter: marcin | Owner: tmark
Type: enhancement | Status:
Priority: high | reviewing
Component: perfdhcp | Milestone:
Keywords: | Sprint-DHCP-20131016
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:4 tmark]:
> 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.
Fixed.
>
> ----------------------------------------------
>
> 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.
I had considered that but I was planning to access container elements
randomly. Each accessed element I planned to remove from the storage
(actually both getNext() and getRandom() remove the returned element).
When you erase the element from the vector, other than its end(), you end
up reallocating all elements after removed one. This is not the case for
list so I decided to use list instead. This however has other penalties.
>
> ----------------------------------------------
>
> 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().
I added a comment to say why I am doing it. I was thinking that the
attempt to getNext() or getRandom() when a container is empty may result
in a change to the internal state of the container so as the subsequent
call will crash or return garbage. The purpose of the test is to verify
that it doesn't.
>
> 2. What is the purpose of the lines 72-76? What is tested here that is
not tested above them?
I am testing the case, that the container got empty after multiple calls
to getNext and that I can add new elements to it. Added a comment.
>
> ----------------------------------------------
>
> packet_storage_unittest.cc
>
> General comment, there are no tests yet for Pkt4.
This is true but the class is Packet type agnostic. There are no methods
which would call Pkt4 or Pkt6 methods. It just holds objects. It could
actually hold any object.
>
> ----------------------------------------------
>
> 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?
Explained above.
>
> ----------------------------------------------
>
> 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?
Good catch. I now swapped call to getNext() and getRandom() to make test
reliable.
>
> 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);
> }}}
Very good suggestion. I added it to the getRandom test.
>
> ----------------------------------------------
>
> 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.
Added.
>
> ----------------------------------------------
>
> command_options.cc
>
> line 733 change "freater" to "greater".
Fixed.
>
> ----------------------------------------------
>
> command_options.cc
>
> 1. Why is renew rate only allowed for IPv6? Is IPv4 going to be added?
I just needed IPv6 for now so I didn't bother IPv4. In the future we will
implement it.
>
> 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.
Actually they should. Added a check.
>
> --------------------------------------------------
>
> 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.
Fixed.
>
> --------------------------------------------------
> 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.
True. I modified the comment in the header.
>
> --------------------------------------------------
>
> 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.
I will rethink that when I will be implementing releases. But In general I
concur with that. It would be easier to stop adding packets, rather than
clear.
>
> --------------------------------------------------
>
> test_control_unittests.cc
>
> TEST_F(TestControlTest, createRenew)
>
> This test could use a bit more inline commentary.
It has now.
>
> --------------------------------------------------
>
> 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.
>
>
>
Thanks a lot.
--
Ticket URL: <http://bind10.isc.org/ticket/3183#comment:5>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list