[ovs-dev] [PATCH] ovs-test: Enhancments to the ovs-test tool

[Ansis Atteka]

On Mon, Apr 2, 2012 at 1:28 PM, Ben Pfaff <blp at nicira.com> wrote:

> On Fri, Mar 30, 2012 at 01:10:26PM -0700, Ansis Atteka wrote:
> > -Implemented support for ovs-test client, so that it could automatically
> > spawn an ovs-test server process from itself. This reduces the number of
> > commands the user have to type to get tests running.
> > -Automated creation of OVS bridges and ports (for VLAN and GRE tests),
> so that
> > user would not need to invoke ovs-vsctl manually to switch from direct,
> 802.1Q
> > and GRE tests.
> > -Fixed some pylint reported warnings.
> > -Fixed ethtool invocation so that we always try to query the physical
> interface
> > to get the driver name and version.
> > -and some others enhancments.
> >
> > The new usage:
> > Node1:ovs-test -s 15531
> > Node2:ovs-test -c 127.0.0.1,1.1.1.1 192.168.122.151,1.1.1.2 -d -l 125 -t
> gre
> >
> > Signed-off-by: Ansis Atteka <aatteka at nicira.com>
>
> If we're lucky, Reid will review this also.
>
> NEWS should mention the enhancements.
>
> Is the special case for 127.0.0.1 useful?  (Why would I want to run
> this test against localhost?)
>
"127.0.0.1 case" allows you to type 2 commands:

Node1:ovs-test -s 15531
> Node2:ovs-test -c 127.0.0.1,1.1.1.1 192.<Node1>,1.1.1.2 -d -l 125 -t gre
>

instead of 3 commands:

Node1:ovs-test -s 15531
> Node2:ovs-test -s 15531
> Node2:ovs-test -c <Node2>,1.1.1.1 <Node1>,1.1.1.2 -d -l 125 -t gre
>


I think that we usually use the style of two blank lines between
> top-level definitions, but ip_optional_mask() only has one.
>
> vlan_tag(): Google Python style also says "if x or y", not "if (x) or (y)".
>
> vlan_tag(): "Should" => "should"
>
> xmlrpc_get_my_address_from(): "th" => "the"
>
> xmlrpc_create_test_bridge(): I don't understand in what sense this
> function is atomic.

Maybe "atomic" meaning is not appropriate here. With "atomic" I
meant that we must do everything in one shot. Otherwise, If we would do
multiple RPC calls just to create the Physical bridge (e.g. 1. creating
bridge; 2. attaching physical interface to it; 3. moving IP config), then
after step #2 the ovs-test client wouldn't be able to connect to ovs-test
server anymore.



> But it also seems like it should un-assign the IP
> address from the original interface.
>
I though about the same thing, but this "lazy way" of not unassigning
the IP address from the physical interface seemed to always work fine.
But I do have to agree with you that this does not look nice, so I will
try to fix it.


> "bridge" is misspelled as "brige" in several places (even function
> names).
>
> xmlrpc_create_bridge() and other functions check for != -1 but I'd
> expect that the correct check is == 0.
>
> I would make xmlrpc_is_ovs_bridge() return True or False, not 0 or 1,
> because the usage "if server.is_ovs_bridge(interface_name)== 0:" looks
> funny.
>
> python/ovstest/vswitch.py has a ton of code duplication, some shared
> with python/ovstest/util.py.  I'd factor it out and add error
> reporting.
>
> In the documentation, please write ethtool as \fBethtool\fR(8).
>
> In the documentation, I think that it would be nice to explain why one
> would pick a particular innerip or innerport.  My guess, without
> thinking too hard, is that it is important that these inner ips do not
> interfere with any IP addresses in use on either the server or the
> client, because even though the data is tunnelled the inner IP traffic
> still goes through the network stack on the client and the server.  I
> guess that the inner ports are not as important.
>
> In the examples, I think that it would be nice to explain the
> suggested client command line a bit more.  I think that it will run
> three tests, one for direct mode, one for vlan mode, and one for gre
> tunnel mode, but it would be nice to say that.
>
> On the same note, the manpage doesn't really say (that I noticed) what
> happens when you give multiple mode options.  It's common for
> utilities to only support a single mode in one invocation, but I don't
> think that this is the case here, so it might be good to say that.
> You could put the options that add another mode into a subsection
> (.SS) that mentions that each one introduces a new mode.
>
> Thanks,
>
> Ben.
>
Ok, to everything else and I will resubmit the patch. Sorry that this time
I did not create multiple logical patches to make the review easier.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20120404/c6c18f5a/attachment-0003.html>