NDT-DEV email list created

[Sebastian Kostuch <>]

Hi Peter,
regarding work on multiple streams feature then more than only 1 person was involved in implementing this and changes were also tested
internally so it wasn't for sure pushing changes and merging them without any review etc. Also this branch has been added many months ago
and was then ready for another developer's insight and review (thanks to Aaron for all his comments here). And even with that there are still
possibilities to merge changes which provide some bugs now or in the future (that's the one reason among others that there were and still are
some things that need fixes in current master branch). Websocket client is still new (and does not support all features which another clients have),
to add support for new tests it needs some changes in its code, the work I've done was to ensure that nothing that worked so far will break after
our changes and I've tested also websocket client with old tests and it worked good. I really appreciate your comments regarding found bugs
and will address them soon. Thank you very much for whole feedback!

Best regards
Sebastian

On 29.06.2015 23:51, Peter Boothe wrote:

It is always preferable when the person saying "this code is good code and fine to merge into master" is distinct from the person who wrote the code. Two sets of eyes are much better than one.

This change introduced a lot of complexity (and two new goto statements!) into the already-intricate test_c2s() and test_s2c() routines.  

For example: The new code depends on socket file descriptors being strictly increasing integers.

If the final accept() in the new code fails to return a file descriptor that is a larger integer than every other in the set, then the select() on line 398 in test_c2s_srv.c will have undetermined behavior.  It is likely, but definitely not promised by the OS, that socket file descriptors will be increasing. This new code introduces the potential for a subtle bug in long-running servers, because long-running servers are the ones that are most likely to re-use file descriptors, and that re-use will break the select().

Also, select()'s behavior when the last socket created is not the last socket to be closed and removed from the FD_SET is similarly undefined, which means some new logic needs to be added near lines 424-431.

The new code also needlessly breaks websocket support for the new tests, because the 0 on lines 257 and 258 of test_c2s_srv.c should be an i.

And that's what I've found so far in my first attempt to go through the changes in that one file when I was trying to integrate these changes into my pull request.

  -Peter

On Fri, Jun 26, 2015 at 10:56 AM, Sebastian Kostuch <> wrote:

Hi
as wrote yesterday, I have merged MultiplePorts branch into master. This version is stable and I've ensured that
it does not break basic functionality.

Regards
Sebastian

On 25.06.2015 14:46, Sebastian Kostuch wrote:

Hi Aaron,
I haven't look in detail on Peter's changes yet but after a brief look the major critical
conflicts will be probably related to connection handling in S2C and C2S tests. When it comes
to changes in web100srv file then the one on MultiplePorts are rather small and don't touch
critical pieces of code. MultiplePorts branch has been added some months ago and there were
few merges being done from master meanwhile (which also provided many conflicts due to major changes in master code) so I think it would be good to first merge these changes and then if needed
I could help with resolving conflicts with Peter's changes (they also require some deep review while
ours are ready to merge as they are).
Protocol changes work ok with or without json usage, actually there are
small changes to protocol at this moment as all major features which were discussed in another
thread are planned to be implemented later as merging our branch does not require them to be
ready for now (I've just added 2 new tests as separated ones and when it comes to protocol they
differ from basic throughput tests with additional parameters being sent by server at the beginning
of test, which are also contained in json 'msg' value).

Regards
Sebastian

On 25.06.2015 14:22, Aaron Brown wrote:

Hey Sevastian,

I'm preparing to move to Virginia so haven't been able to look at the various requests. I'm presuming this work and the work Peter has been doing are incompatible. Is that accurate? If so, is there a plan to get them to work together so they can both be merged? Also, are the protocol changes you've proposed compatible with the json protocol and what do they look like?

Cheers,

Aaron

On Thursday, June 25, 2015, Sebastian Kostuch <> wrote:

Hi,
I've added additional test run where server is run with extended test options and client requests those tests.
I've also tested MultiplePorts changes when using old client/server with our new one to ensure that nothing
will break when connecting old client with new server and new client with old server. It seems like everything
is working ok and there should be no issues with using these versions so if there won't be any feedback blocking
from merging these changes into master I will do so tomorrow.

Regards
Sebastian

On 24.06.2015 18:14, Peter Boothe wrote:

A unit-testing framework was introduced to NDT a little while back.  It can be seen in action in all the files named *_unit_tests.c, and the complete source code of the unit testing framework can be found in unit_testing.c and unit_testing.h.  It's compatible with Automake and Autoconf, which actually have built-in support for unit testing.

This means that we can now type "make check", and pieces of the code will be unit-tested and end-to-end tested. Not all the code is tested, but hopefully over time an increasing percentage of the code will be. This way we can make sure, as part of the build process, that old bugs haven't reappeared and that new features work as advertised.

I recognize that the MultiplePorts features were not built with unit-testing in mind, so instead of trying to shim unit-testing onto its component parts (which looks like it would be a big pain), can we add some end-to-end tests of the new functionality (and keep the unit tests of the old functionality) in web100srv_unit_tests.c?  That way we can be sure that these changes (a) do actually work and (b) don't break old clients, and we can encode those guarantees in code to make sure they stay true.  It shouldn't require too much new work to do - I think the code would be an almost line-for-line copy of the test_e2e() function, but with a different command line for starting the NDT client.

  -Peter

On Wed, Jun 24, 2015 at 7:37 AM, Sebastian Kostuch <> wrote:

Hi all,
I've ended up with fixing issues that Aaron suggested in comments to multiple streams support commit (link here), thanks for all of them.
All changes are available on MultiplePorts branch. We look forward to merge this branch into master so NDT will gain some new functionalities
regarding extended throughput tests.

These changes are ready to review and it would be very appreciated if someone could check if code looks ok and all works as expected
(I've tested it on my own however it would be good if someone would confirm that there are no new issues after these changes).

If you have any comments then please leave them as a line note in proper commit or write as an answer to this mail and I will address
them all.

Below are listed the most important changes that MultiplePorts branch include:
    * added 2 new tests which extend current throughput ones (Java client runs them by default, in C client you need to use --enables2cext or/and --enablec2sext options)
    *  options related to extended tests (for details please see 'Extended tests code' section in server manual):
        - setting duration of throughput test
        - enable throughput snapshots with possibility to specify its delay and offset
        - setting number of streams used in mentioned tests (up to 7)
    * added possibility to save web100/web10g variables related to each stream in separated file (--savewebvalues option)
    * packet trace include packets from all streams however only the ones from first stream are used for network testing (there is already separated thread on extending this functionality in future)
    * every stream is being snapshotted, however due to some high CPU usage on some configurations I've added possibility to disable snapshots at all (--disablesnaps option), they are enabled by default as they were so far

Thank you for any feedback.

Regards
Sebastian

--

ᴹ̶LAB | Measure the Internet, save the data, and make it universally accessible and useful.

--

ᴹ̶LAB | Measure the Internet, save the data, and make it universally accessible and useful.