BIND 10 #1959: Implement perfdhcp control logic

[BIND 10 Development]

#1959: Implement perfdhcp control logic
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  tomek
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:  Sprint-
  enhancement                        |  DHCP-20120917
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:         |           Sub-Project:  DHCP
  perfdhcp                           |  Estimated Difficulty:  0
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by tomek):

 Replying to [comment:13 marcin]:
 > > CommandOptionsHelper seems like a useful class that could be shared
 between other components. dhcp4 and dhcp6 come to mind, but I'm sure there
 are others. You should investigate, if it is feasible to move it to
 src/lib/testutils.
 >
 > I will investigate this in the course of #1960. I just don't want to
 complicate things more than they are now.
 Partially agree. That should not be handled in 1959, but stuffing it into
 1960 will just make another bloated 2-weeks/3 days review ticket. Please
 create a separate ticket for it. It is ok to have small (like half a day)
 ticket.

 > > We do have plans to implement interface detection on other OSes, but
 such a ticket is not currently planned. Do you think it would be useful to
 implement it soon? There is a getifaddrs() interface that seems to be
 supported on Linux, all BSDs and surprisingly even Solaris 11.
 >
 > I think it would be very useful so as we can run tests on multiple OSes.
 I've just created ticket #2246 for it. If you think it is useful to have
 it sooner rather than later, you may suggest that it should be included in
 the next sprint.

 > > I don't like the fact that if no interfaces are detected that the test
 does nothing. In my opinion it should fail in such case.
 >
 > I understand your point but I would rather want to have reliable
 interface detection on all supported OSes. The interface detection should
 be checked elsewhere (IfaceMgr). Here I rely on this fact but I keep this
 safety valve as long as I am not sure if it is 100% reliable and well
 tested.
 Fair enough.

 > > getLocalLoopback(): Loopback interfaces should have
 IfaceMgr::Iface::flag_loopback_ set to true.
 >
 > Nice! I now use this although I don't like the fact that this field is
 public.
 I was too lazy/busy to implement getters for it.

 > > If I understand the method correctly, in its current it will never
 return anything above 2. The first iteration will reduce the value to
 something in 0..255 range. The second iteration will set unequal_pos to 2
 and break the loop. For that reason it doesn't work for 65537 and above
 (returns 2 instead of 3).
 > >
 > > The code for this method is surprisingly convoluted. If I understand
 it correctly, this method should return number of bytes that have to be
 different to accommodate specified number of  clients. So what you really
 want is:
 > >
 > > log256(clients_num - 1);
 > >
 > > Here are 2 proposals to do this:
 > > {{{
 > >     if (clients_num < 2) // degenerated case. There's nothing to
 randomize
 > >         return 0;
 > >
 > >     return 1 + log(clients_num - 1)/log(256);
 > > }}}
 > > It is very short, but has the disadvantage of using floating point
 arithmetic. Another code that is similar to the code under review would
 be:
 > > {{{
 > >     if (!clients_num)
 > >         return 0;
 > >     clients_num--;
 > >
 > >     int cnt = 0;
 > >     while (clients_num) {
 > >         clients_num >>= 8;
 > >         ++cnt;
 > >     }
 > >
 > >     return cnt;
 > > }}}
 > > The advantage here is not using floating point calculations. As this
 is a test code, I personally prefer simplicity over performance to avoid
 possible bugs.
 >
 > Since you approved both options I picked the second one. I don't like
 floating points. :-)
 Ok.

 > > On a related note, how does this method correlate to what is happening
 in generateDuid()? If you have 256 clients, those clients will have unique
 DUID only if the generateMacAddress() manages to allocate 256 unique
 numbers.
 > >
 > > I would like to see a test that generates 256 DUIDs and verifies that
 they are indeed unique. They should differ only on a single byte, right?
 >
 > I used random() function to generate "unique" numbers. The point is that
 they might not be unique in the short period of time because repetitions
 are possible. We discussed this on jabber and I think what strikes me here
 is that "if somebody sets he wants to simulate 100 clients and run 100
 iterations he might not get 100 leases and he will be unhappy trying to
 debug why it is the case". My understanding was that if you run the test
 for a long period of time (BTW, this is how performance should be
 measured) the number of leases will reach 100 or will be close enough.
 However, after our discussion I think it is good point to guarantee the
 unique DUIDs and MACs within number of iterations equal to number of
 simulated clients. I do it now with sequential number generators.
 Ok, great.

 > > TestControlTest.Options4,6: Those tests are generic to all options.
 Most of the test code is generic, only the call to
 registerOptionFactories() is specific to TestControl. You should consider
 splitting those tests and moving generic parts to src/lib/dhcp/ somewhere.
 >
 > I don't agree. I have a set of proprietary factory functions that I need
 within perfdhcp. I test only those functions. What we want in src/lib/dhcp
 should be generic not proprietary to perfdhcp.
 Ok, fair enough. Nevertheless, parts of the existing code will be reused
 option definition work will progress to the stage that factory method will
 be used in src/lib/dhcp as well.

 > > TestControlTest.Options6: How does the code for requestes_options
 work? It uses matchRequestedOptions() that compares the requests on byte
 by byte basis. DHCPv6 options are  coded on 2 bytes.
 matchRequestdOptions6() is needed.
 >
 > Ooops! I missed the fact that they are 2 bytes long for DHCPv6.
 matchRequestedOptions6() look ok.

 Ok. That deals with the response to 4th part of the review. I'm afraid
 during the process of answering them, I've found couple new things. I will
 post them in a separate comment. Don't worry, they are very minor.

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