On Mon, Oct 31, 2011 at 04:38:51PM -0700, Ansis Atteka wrote:
> This tool will be a replacement for the current ovs-vlan-test
> utility. Besides from connectivity issues it will also be able
> to detect performance related issues in Open vSwitch setups.
> Currently it uses UDP and TCP protocols for stressing.
>
> Issue #6976
"git am" says:
Applying: ovs-test: A new tool that allows to diagnose connectivity
and performance issues
/home/blp/db/.git/rebase-apply/patch:326: trailing whitespace.
This class contains all the functions that ovstest will call
/home/blp/db/.git/rebase-apply/patch:395: trailing whitespace.
Releases UdpListener and all its resources
/home/blp/db/.git/rebase-apply/patch:448: trailing whitespace.
Releases UdpListener and all its resources
/home/blp/db/.git/rebase-apply/patch:505: trailing whitespace.
This per-listening socket class is used to
/home/blp/db/.git/rebase-apply/patch:580: trailing whitespace.
This factory is responsible to instantiate TcpSenderConnection
classes
warning: squelched 5 whitespace errors
warning: 10 lines add whitespace errors.
The NEWS and debian/changelog entries add this to the 1.3.0 section.
Are we planning to include this in 1.3.0, then? If not (it's missed
the normal 1.3.0 deadline) then the NEWS entry should be in the
post-1.3.0 section and the debian/changelog entry can be omitted for
now.
"automake" says:
python/ovstest/automake.mk:1: run_python multiply defined in condition
TRUE ...
Makefile.am:199: `python/ovstest/automake.mk' included from here
python/ovs/automake.mk:1: ... `run_python' previously defined here
Makefile.am:197: `python/ovs/automake.mk' included from here
"lintian" pointed out that
A new distributed testing tool that allows to diagnose performance
is better written:
A new distributed testing tool that allows one to diagnose performance
The python/ovstest hierarchy duplicates a bit of the content
(dirs.py) and Makefile mechanisms from python/ovs. Is there a good
reason for that? I wonder why it can't just go in, say,
python/ovs/test and reuse the same infrastructure?
The new module names start with ovs, e.g. "ovsargs", but they're
already in a package named ovstest. The second ovs prefix seems
redundant; is there value in it?
It looks like all of the bandwidth values are accepted and printed in
kilobytes per second. This is a little surprising because usually
bandwidth is expressed in kilobits per second.
The usage message says "-c SERVERS SERVERS". The manpage says "-c
SERVER1 SERVER2". The latter seems more accurate; should the usage
message be updated to match?
The output has header lines like:
UDP test from 127.0.0.1 to 127.0.0.1 with target BANDWIDTH 1000KBps
which is a little confusing if one is doing a test on a single machine
(I used localhost ports 1234 and 1235). Maybe the output could
include the port numbers too?
It seems odd to write BANDWIDTH in all-caps in output.
I saw the following output from the ovs-test server when the client
started the test:
/usr/lib/python2.6/dist-packages/ovstest/ovsrpcserver.py:87:
DeprecationWarning: Please use stopListening() to disconnect port
listener.transport.loseConnection()
I see a lot more horizontal white space in the Python code than we
usually use. We usually write (x) not ( x ). I think that our
style generally matches the Google Python style guidelines:
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Whitespace#Whitespace
Occasionally we do find ourselves lining things up though.
ovsargs.py
----------
I see some unnecessary use of \ line splicing. I don't think that any
of the \s is needed.
In ip_port(), I think that this:
if len( value ) == 2:
ip( value[0] )
else:
raise argparse.ArgumentTypeError( "IP address and Port must be "\
"colon-separated" )
return ( value[0], port( value[1] ) )
can be written slightly more simply as:
if len( value ) != 2:
raise argparse.ArgumentTypeError( "IP address and Port must be "\
"colon-separated" )
return ( ip(value[0]), port( value[1] ) )
The bandwidth() function is clever, I wouldn't have thought to do it
that way :-)
ovsrpcserver.py
---------------
I see some code like this:
( listener ) = self.__get_handle_resources( handle )
Are the () on the left side of the assignment functional? My
understanding is that Python only parses () as designating a list if
there's a comma. If these () don't do anything, then, please omit
them.
ovstcp.py
---------
I think that Producer should derive from object.
Here the indentation of the comments is odd:
def getResults( self ):
""" returns the number of bytes received as string"""
#XML RPC does not support 64bit ints (http://bugs.python.org/issue2985)
#so we have to convert the amount of bytes into a string
return str( self.stats )
ovs-test.8.in
-------------
The synopsis has some double spaces that should be single spaces, e.g.:
\fBovs\-test\fR \fB\-s\fR \fIport\fR
->
\fBovs\-test\fR \fB\-s\fR \fIport\fR
I'd like to make it crystal-clear that the problems that ovs-test
finds aren't bugs in OVS, e.g. change:
The \fBovs\-test\fR program may be used to check for problems sending
802.1Q traffic which may occur when running Open vSwitch. These problems can
to
The \fBovs\-test\fR program may be used to check for problems sending
802.1Q traffic that Open vSwitch may uncover. These problems can
and change:
To determine whether Open vSwitch is encountering any 802.1Q
related problems, the user must compare packet loss and achieved
bandwidth in a setup where traffic is being tagged against one
where it is not. If in the tagged setup both servers are unable
to communicate or the achieved bandwidth is lower, then, most
likely, Open vSwitch has encountered a pre-existing kernel or
driver bug.
I'd also reword:
in server mode. And then on any other host run the \fBovs\-test\fR in client
to
in server mode. Then, on any other host, run the \fBovs\-test\fR in client
There's a missing period at the end of:
Run in server mode and wait a client to connect to \fIport\fR
The description of the argument to --client is hard to understand. I
think that the Xs make it harder to read; it makes it look like IPX
protocol is somehow involved. I'd just delete the Xs. I don't
understand the distinction between controlIP and testIP, or when one
would want to specify just one or both; the text doesn't say.
Would it be easy to display the Linux kernel version and "ethtool -i"
output from each side in the ovs-test output? Then we wouldn't have
to show people how to display it themselves, and we might get better
bug reports.
The EXAMPLES say that a single ovs-test command starts two servers. I
think that it means that you should run that command on each server,
but it doesn't say that.
ovs-test.in
-----------
The Nicira copyright notice is missing.
The string .format method is new in Python 2.6, so we'll need to
change it to % or something else to get this to work on XenServers or
RHEL5, which have Python 2.4.
I'd use parentheses instead of \ line continuation.
50, 500, 1000, 1500 seems like an odd sequence of UDP packet sizes.
What motivated these choices? I would have guessed at including 18
bytes (14 Eth + 20 IP + 8 UDP + 18 payload + 4 FCS == 64 is a
min-length Ethernet frame, right?) and 1472 (I think that's the
max-length UDP that doesn't require fragmentation) somewhere in there.
I'm a little surprised by the port 15532 that pops up. I would have
expected that all the port numbers used were the ones configured by
the user.
I'm not sure why the variable names in the __main__ block at the end
are in all-caps.
It would be good to include a few words in the ovs-vlan-test and
ovs-test manpages about the differences between the two utilities and
when one would choose to use one or the other.
Thanks,
Ben.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
