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 <aatt...@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?) 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. But it also seems like it should un-assign the IP address from the original interface. "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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
