[ovs-dev] [PATCH] ovs-test: A new tool that allows to diagnose connectivity and performance issues

Ansis Atteka aatteka at nicira.com
Fri Nov 11 01:26:17 UTC 2011


On Tue, Nov 1, 2011 at 11:18 AM, Ben Pfaff <blp at nicira.com> wrote:

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

>
> 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.
>
Fixed. Removed also from Debian/Changelog.

>
> "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
>
Fixed (see two items below)

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

>
> 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?
>
Fixed, but slightly different than you suggested.

If I would move contents of python/ovstest directory under python/ovs/test
then both of these Python modules (ovs and ovs.test) would have
child-parent relations (hence ovs/test would be distributed also with
python-openvswitch debian package). Unless there is a way I could
explicitly exclude ovs/test subdirectory from python-openvswitch.install
file.

I moved the automake.mk file one level up to include both directories -
ovstest and ovs - to reflect their sibling relations and share the same
automake.mk file. Are you ok with this?

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

Fixed.

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

 Fixed.

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

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

>
> It seems odd to write BANDWIDTH in all-caps in output.

Fixed.

>

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()
>
Fixed. This warning showed up only on Python 2.6.

>
> 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.
>
Fixed. There were also some other deviations from Google Python coding
style that I fixed.

>
> ovsargs.py
> ----------
>
> I see some unnecessary use of \ line splicing.  I don't think that any
> of the \s is needed.
>
Fixed.

>
> 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] ) )
>
Fixed.

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

>
> ovstcp.py
> ---------
>
> I think that Producer should derive from object.
>
Fixed.

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

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

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

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

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

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

>
> ovs-test.in
> -----------
>
> The Nicira copyright notice is missing.
>
Fixed

>
> 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.
>
Not Fixed Yet. Can we postpone this till we actually put any effort to
support ovs-test on XenServer? I can do this once I will have XenServer in
my testbed.

>
> I'd use parentheses instead of \ line continuation.
>
Fixed.

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

Fixed by using UDP payloads with following sizes - [8, MTU-28, MTU]. This
would cover following scenarios:
1. When UDP payload alone is not enough to achieve min Ethernet Frame size
of 64-bytes;
2. When whole Ethernet Payload is utilized; without IP fragmentation;
3. When IP fragmentation kicks in;
If anyone thinks this is not enough, let me know.

 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.
>
Fixed. There are actually two IP:Port pairs that come into picture for each
ovs-test server - ControlIP:ControlPort and TestIP:15532. Anyway I just
made 15532 configurable when one starts the the ovs-test in client mode. As
minimum only both server ControlIPs are needed to get started. If one wants
more advanced setup, then he can also override ControlPort, TestIP and
TestPort default values.

>
> I'm not sure why the variable names in the __main__ block at the end
> are in all-caps.
>

Pylint suggested to use all caps: ID:C0103  Invalid name "node1" (should
match (([A-Z_][A-Z0-9_]*)|(__.*__))$) Let me know if you still think that
using lower case would be more appropriate here (*see also Global/Class
Constants in *http://code.google.com/p/soc/wiki/PythonStyleGuide#Naming)?


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

>
> Thanks,
>
> Ben.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20111110/3d4cc0c4/attachment-0003.html>


More information about the dev mailing list