BIND 10 #1959: Implement perfdhcp control logic

[BIND 10 Development]

#1959: Implement perfdhcp control logic
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  marcin
  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 marcin):

 Replying to [comment:12 tomek]:
 > '''tests/tools/perfdhcp/tests/command_options_helper.h'''
 > represinting => representing

 Fixed.

 >
 > 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.

 >
 > In the following code in CommandOptionsTest.Base:
 > {{{
 >     // "t" is invalid character in DUID
 >     EXPECT_THROW(process("perfdhcp -6 -l ethx -b "
 >                          "duiD=010101010101010101t110111F14 all"),
 >                  isc::InvalidParameter);
 > }}}
 > is the duid capitalization (duiD) made on purpose?

 It was but it was at the same time checking if invalid digit ('t')
 produces exception so if there was an error in parsing duiD string it
 would not be clear whether exception comes from case sensitive (invalid)
 treatment of this option or 't' in the middle of DUID. I changed the
 "duiD" to "duid' in this particular case to avoid this issue.

 >
 > Is the parsing case-insensitive? I assume it is, but there seem to be no
 test for it. For parameters longer than single character, please check at
 least couple combinations, -Duid= -DuId= -DUID= etc.

 There are now a couple of new test cases that cover this.
 >
 > What about odd length DUID, e.g. 01234? Is it padded with a leading
 zero? Please test it.

 It is not padded. If odd number of digits is entered the parser will throw
 exception. It is now tested.

 >
 > What about too short DUIDs, e.g. 12? That should be allowed for server
 testing purposes, but a warning should be added that DUID not-conformant
 to 3315 is used. If it is not allowed in the current code, an appropriate
 @TODO should be added. (it is minor and time should not be spent of
 implementing such a support now if it is missing).

 Current implementation uses MAC generation function to generate unique
 DUIDs. For this reason I assume that DUID has to be at least 6 octets
 long. If it is not then exception is thrown. This is now tested.

 >
 > Please make sure that you test CommandOptionsTest.Interface on several
 non-linux systems. IfaceMgr detects interfaces on Linux only. On other
 systems it uses very simple approach: it tries to find interface index of
 "lo" interface. If that fails, it repeats with "lo0". This allows
 detecting loopback interfaces on all systems, except Windows. That is a
 bizarre OS that do not feature loopback concept. Fortunately, I think
 nobody pays any attention to this OS in BIND10 context.

 ok.

 >
 > 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 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.

 >
 > '''tests/tools/perfdhcp/tests/test_control_unittest.cc'''
 > Let me just say that the trick with using in NamedTestControl is
 brilliant!

 Congratulations to somebody who documented Google Test! :-)

 >
 > 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.

 >
 > matchRequestedOptions(): layed => laid

 Fixed. There are a couple of words in English that confuse me every time I
 write them.

 >
 > unequalOctetPosition(): the comment for parameter random_size lacks
 parameter name.
 > The comment for randomization range suggests that it is always 6. If
 that is the case, why not making is a fixed value? And it is not really
 needed anyway.

 I removed it. It was not needed.

 >
 > 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. :-)

 >
 > 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.

 >
 > testDuid(): The conditions checked in testDuid() are suspicious. Why the
 test passes if the total_dist is 0? It should only pass if all X DUIDs are
 different.

 Ok. I now changed this test quite a lot. it should not be confusing
 anymore.

 >
 > Something along the lines:
 > {{{
 > Duid duid[clients_num];
 > // fill in DUIDs
 > for (i = 0; i < clients_num; ++i) {
 >     for (j = i + 1; j<clients_num; ++j) {
 >         if (duid[i] == duid[j]))
 >             FAIL();
 >     }
 > }
 > }}}
 >
 > The same comment applies to testMacAddress().

 The test for MAC address generation has been changed as well.

 >
 > testPkt4Exchange(): reponses => responses

 Fixed.

 >
 > createOffcePkt4(), creatAdvertisePkt6(): doxygen descriptions are
 missing.

 Added.

 >
 > 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.

 >
 > 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.

 >
 > matchRequestedOptions(): break in the loop should be removed. This will
 detect option code duplicates.

 Ok. Removed.

 >
 > TestControlTest.Options6: TODO should look like this /// @todo to be
 picked up by Doxygen TODO list generator.

 Fixed.

 >
 > TestControlTest.PacketTemplates: The test should verify that the
 templates were read correctly.

 Test now uses random binary data to create the template files in flight.
 Then it initializes CommandOptions with those template files and compares
 that data read from the files is equal to what has been written to files.

 >
 > TestControlTest.RateControl: Checks for xchgs_num should be a bit more
 thorough. This is very broad subject, so I proposed to add TODO and don't
 spend too much time on it now.

 I added todo cause it might be quite complex to create more thorough test.

 >
 > Ok. That's it. End of the review.

 Thank you for the effort. I will try to create smaller tickets from now
 on. I can see that you haven't omitted a single line of code. Good review.

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