[ovs-dev] [PATCHv2] ovs-test: Enhancements to the ovs-test tool

Reid Price reid at nicira.com
Mon Apr 16 13:17:49 PDT 2012


On Mon, Apr 16, 2012 at 1:16 PM, Ansis Atteka <aatteka at nicira.com> wrote:

>
>
> On Mon, Apr 16, 2012 at 1:00 PM, Reid Price <reid at nicira.com> wrote:
>
>> On Mon, Apr 16, 2012 at 12:32 PM, Ansis Atteka <aatteka at nicira.com>wrote:
>>
>>>
>>>
>>> On Fri, Apr 13, 2012 at 8:00 PM, Reid Price <reid at nicira.com> wrote:
>>>
>>>>
>>>>
>>>> On Fri, Apr 13, 2012 at 7:26 PM, Ansis Atteka <aatteka at nicira.com>wrote:
>>>>
>>>>>
>>>>>
>>>>> On Wed, Apr 11, 2012 at 3:10 PM, Reid Price <reid at nicira.com> wrote:
>>>>>
>>>>>> Sure thing, a few notes inline.  Looks good overall.
>>>>>>
>>>>>> On Tue, Apr 10, 2012 at 6:02 PM, Ansis Atteka <aatteka at nicira.com>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 enhancements.
>>>>>>>
>>>>>>> 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>
>>>>>>> ---
>>>>>>>  NEWS                        |    4 +
>>>>>>>  python/automake.mk          |    3 +-
>>>>>>>  python/ovstest/args.py      |  156 +++++++++++++++++++------
>>>>>>>  python/ovstest/rpcserver.py |  156 ++++++++++++++++++++++++-
>>>>>>>  python/ovstest/tcp.py       |   23 +----
>>>>>>>  python/ovstest/udp.py       |   10 +--
>>>>>>>  python/ovstest/util.py      |   82 +++++++++++--
>>>>>>>  python/ovstest/vswitch.py   |   97 ++++++++++++++++
>>>>>>>  utilities/ovs-test.8.in     |  152 ++++++++++++++-----------
>>>>>>>  utilities/ovs-test.in       |  262
>>>>>>> ++++++++++++++++++++++++++++++++++++++-----
>>>>>>>  10 files changed, 763 insertions(+), 182 deletions(-)
>>>>>>>  create mode 100644 python/ovstest/vswitch.py
>>>>>>>
>>>>>>> diff --git a/NEWS b/NEWS
>>>>>>> index ed3fc88..ac00335 100644
>>>>>>> --- a/NEWS
>>>>>>> +++ b/NEWS
>>>>>>> @@ -8,6 +8,10 @@ post-v1.6.0
>>>>>>>       Internetwork Control (0xc0).
>>>>>>>     - Added the granular link health statistics, 'cfm_health', to an
>>>>>>>       interface.
>>>>>>> +    - ovs-test:
>>>>>>> +        - Added support for spawning ovs-test server from the
>>>>>>> client.
>>>>>>> +        - Now ovs-test is able to automatically create test bridges
>>>>>>> and ports.
>>>>>>> +
>>>>>>>
>>>>>>>
>>>>>>>  v1.6.0 - xx xxx xxxx
>>>>>>> diff --git a/python/automake.mk b/python/automake.mk
>>>>>>> index b60e26b..59db000 100644
>>>>>>> --- a/python/automake.mk
>>>>>>> +++ b/python/automake.mk
>>>>>>> @@ -6,7 +6,8 @@ ovstest_pyfiles = \
>>>>>>>        python/ovstest/rpcserver.py \
>>>>>>>        python/ovstest/tcp.py \
>>>>>>>        python/ovstest/udp.py \
>>>>>>> -       python/ovstest/util.py
>>>>>>> +       python/ovstest/util.py \
>>>>>>> +       python/ovstest/vswitch.py
>>>>>>>
>>>>>>>  ovs_pyfiles = \
>>>>>>>        python/ovs/__init__.py \
>>>>>>> diff --git a/python/ovstest/args.py b/python/ovstest/args.py
>>>>>>> index d6b4756..39a32b3 100644
>>>>>>> --- a/python/ovstest/args.py
>>>>>>> +++ b/python/ovstest/args.py
>>>>>>> @@ -1,4 +1,4 @@
>>>>>>> -# Copyright (c) 2011 Nicira Networks
>>>>>>> +# Copyright (c) 2011, 2012 Nicira Networks
>>>>>>>  #
>>>>>>>  # Licensed under the Apache License, Version 2.0 (the "License");
>>>>>>>  # you may not use this file except in compliance with the License.
>>>>>>> @@ -19,9 +19,10 @@ ovsargs provide argument parsing for ovs-test
>>>>>>> utility
>>>>>>>  import argparse
>>>>>>>  import socket
>>>>>>>  import re
>>>>>>> +import sys
>>>>>>>
>>>>>> Google style likes these alphabetized (one line higher)
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> -def ip(string):
>>>>>>> +def ip_address(string):
>>>>>>>     """Verifies if string is a valid IP address"""
>>>>>>>     try:
>>>>>>>         socket.inet_aton(string)
>>>>>>> @@ -30,8 +31,28 @@ def ip(string):
>>>>>>>     return string
>>>>>>>
>>>>>>>
>>>>>>> +def ip_optional_mask(string):
>>>>>>> +    """
>>>>>>> +    Verifies if string contains a valid IP address and an optional
>>>>>>> mask in
>>>>>>> +    CIDR notation.
>>>>>>> +    """
>>>>>>> +    token = string.split("/")
>>>>>>> +    if len(token) > 2:
>>>>>>> +        raise argparse.ArgumentTypeError("IP address and netmask
>>>>>>> must be "
>>>>>>> +                                         "separated by a single
>>>>>>> slash")
>>>>>>> +    elif len(token) == 2:
>>>>>>> +        try:
>>>>>>> +            mask = int(token[1])
>>>>>>> +        except ValueError:
>>>>>>> +            raise argparse.ArgumentTypeError("Netmask is not a
>>>>>>> valid integer")
>>>>>>> +        if mask < 0 or mask > 31:
>>>>>>> +            raise argparse.ArgumentTypeError("Netmask must be in
>>>>>>> range 0..31")
>>>>>>> +    ip_address(token[0])
>>>>>>> +    return string
>>>>>>> +
>>>>>>> +
>>>>>>>  def port(string):
>>>>>>> -    """Convert a string into a Port (integer)"""
>>>>>>> +    """Convert a string into a TCP/UDP Port (integer)"""
>>>>>>>     try:
>>>>>>>         port_number = int(string)
>>>>>>>         if port_number < 1 or port_number > 65535:
>>>>>>> @@ -41,39 +62,76 @@ def port(string):
>>>>>>>     return port_number
>>>>>>>
>>>>>>>
>>>>>>> -def ip_optional_port(string, default_port):
>>>>>>> +def ip_optional_port(string, default_port, ip_callback):
>>>>>>>     """Convert a string into IP and Port pair. If port was absent
>>>>>>> then use
>>>>>>> -    default_port as the port"""
>>>>>>> +    default_port as the port. The third argument is a callback that
>>>>>>> verifies
>>>>>>> +    whether IP address is given in correct format."""
>>>>>>>     value = string.split(':')
>>>>>>>     if len(value) == 1:
>>>>>>> -        return (ip(value[0]), default_port)
>>>>>>> +        return (ip_callback(value[0]), default_port)
>>>>>>>     elif len(value) == 2:
>>>>>>> -        return (ip(value[0]), port(value[1]))
>>>>>>> +        return (ip_callback(value[0]), port(value[1]))
>>>>>>>     else:
>>>>>>>         raise argparse.ArgumentTypeError("IP address from the
>>>>>>> optional Port "
>>>>>>>                                          "must be colon-separated")
>>>>>>>
>>>>>>>
>>>>>>> +def vlan_tag(string):
>>>>>>> +    """
>>>>>>> +    This function verifies whether given string is a correct VLAN
>>>>>>> tag.
>>>>>>> +    """
>>>>>>> +    try:
>>>>>>> +        value = int(string)
>>>>>>> +        if value < 1 or value > 4094:
>>>>>>> +            raise argparse.ArgumentTypeError("Not a valid VLAN tag.
>>>>>>> "
>>>>>>> +                                             "VLAN tag should be in
>>>>>>> the "
>>>>>>> +                                             "range 1..4094.")
>>>>>>> +        return string
>>>>>>> +    except ValueError:
>>>>>>> +        raise argparse.ArgumentTypeError("VLAN tag is not a valid
>>>>>>> integer")
>>>>>>>
>>>>>> Nit, would shrink the try/except to cover just the integer casting
>>>>>>
>>>>>>
>>>>>>> +
>>>>>>>
>>>>>>>  def server_endpoint(string):
>>>>>>> -    """Converts a string in
>>>>>>> ControlIP[:ControlPort][,TestIP[:TestPort]] format
>>>>>>> +    """Converts a string
>>>>>>> OuterIP[:OuterPort],InnerIP[/Mask][:InnerPort]
>>>>>>>     into a 4-tuple, where:
>>>>>>> -    1. First element is ControlIP
>>>>>>> -    2. Second element is ControlPort (if omitted will use default
>>>>>>> value 15531)
>>>>>>> -    3  Third element is TestIP (if omitted will be the same as
>>>>>>> ControlIP)
>>>>>>> -    4. Fourth element is TestPort (if omitted will use default
>>>>>>> value 15532)"""
>>>>>>> +    1. First element is OuterIP
>>>>>>> +    2. Second element is OuterPort (if omitted will use default
>>>>>>> value 15531)
>>>>>>> +    3  Third element is InnerIP with optional mask
>>>>>>> +    4. Fourth element is InnerPort (if omitted will use default
>>>>>>> value 15532)
>>>>>>> +    """
>>>>>>>     value = string.split(',')
>>>>>>> -    if len(value) == 1: #  TestIP and TestPort are not present
>>>>>>> -        ret = ip_optional_port(value[0], 15531)
>>>>>>> -        return (ret[0], ret[1], ret[0], 15532)
>>>>>>> -    elif len(value) == 2:
>>>>>>> -        ret1 = ip_optional_port(value[0], 15531)
>>>>>>> -        ret2 = ip_optional_port(value[1], 15532)
>>>>>>> +    if len(value) == 2:
>>>>>>> +        ret1 = ip_optional_port(value[0], 15531, ip_address)
>>>>>>> +        ret2 = ip_optional_port(value[1], 15532, ip_optional_mask)
>>>>>>>
>>>>>> Might be worth storing some of these magic values as variables
>>>>>> somewhere,
>>>>>> if you ever reference them again.
>>>>>>
>>>>>>          return (ret1[0], ret1[1], ret2[0], ret2[1])
>>>>>>>     else:
>>>>>>> -        raise argparse.ArgumentTypeError("ControlIP:ControlPort and
>>>>>>> TestIP:"
>>>>>>> -                                         "TestPort must be comma "
>>>>>>> -                                         "separated")
>>>>>>> +        raise argparse.ArgumentTypeError("OuterIP:OuterPort and
>>>>>>> InnerIP/Mask:"
>>>>>>> +                                         "InnerPort must be comma
>>>>>>> separated")
>>>>>>> +
>>>>>>> +
>>>>>>> +class UniqueServerAction(argparse.Action):
>>>>>>> +    """
>>>>>>> +    This custom action class will prevent user from entering
>>>>>>> multiple ovs-test
>>>>>>> +    servers with the same OuterIP. If there is an server with
>>>>>>> 127.0.0.1 outer
>>>>>>> +    IP address then it will be inserted in the front of the list.
>>>>>>> +    """
>>>>>>> +    def __call__(self, parser, namespace, values,
>>>>>>> option_string=None):
>>>>>>> +        outer_ips = set()
>>>>>>> +        endpoints = []
>>>>>>> +        for server in values:
>>>>>>> +            try:
>>>>>>> +                endpoint = server_endpoint(server)
>>>>>>> +            except argparse.ArgumentTypeError:
>>>>>>> +                raise argparse.ArgumentError(self,
>>>>>>> str(sys.exc_info()[1]))
>>>>>>> +            if endpoint[0] in outer_ips:
>>>>>>> +                raise argparse.ArgumentError(self, "Duplicate
>>>>>>> OuterIPs found")
>>>>>>> +            else:
>>>>>>> +                outer_ips.add(endpoint[0])
>>>>>>> +                if endpoint[0] == "127.0.0.1":
>>>>>>> +                    endpoints.insert(0, endpoint)
>>>>>>> +                else:
>>>>>>> +                    endpoints.append(endpoint)
>>>>>>> +        setattr(namespace, self.dest, endpoints)
>>>>>>>
>>>>>>>
>>>>>>>  def bandwidth(string):
>>>>>>> @@ -86,30 +144,54 @@ def bandwidth(string):
>>>>>>>     return long(bwidth) / 8 #  Convert from bits to bytes
>>>>>>>
>>>>>>>
>>>>>>> +def tunnel_types(string):
>>>>>>> +    """
>>>>>>> +    This function converts a string into a list that contains all
>>>>>>> tunnel types
>>>>>>> +    that user intended to test.
>>>>>>> +    """
>>>>>>> +    return string.split(',')
>>>>>>> +
>>>>>>> +
>>>>>>>  def ovs_initialize_args():
>>>>>>> -    """Initialize args for ovstest utility"""
>>>>>>> -    parser = argparse.ArgumentParser(description = 'Test ovs
>>>>>>> connectivity')
>>>>>>> +    """
>>>>>>> +    Initialize argument parsing for ovs-test utility.
>>>>>>> +    """
>>>>>>>
>>>>>>
>>>>>> For all of the kwargs below, the usual style is to omit the spaces on
>>>>>> either
>>>>>> side of key=value pairs.
>>>>>>
>>>>>>
>>>>>>> +    parser = argparse.ArgumentParser(description = 'Test
>>>>>>> connectivity '
>>>>>>> +                                                'between two Open
>>>>>>> vSwitches.')
>>>>>>> +
>>>>>>>     parser.add_argument('-v', '--version', action = 'version',
>>>>>>>                 version = 'ovs-test (Open vSwitch) @VERSION@')
>>>>>>> +
>>>>>>>     parser.add_argument("-b", "--bandwidth", action = 'store',
>>>>>>>                 dest = "targetBandwidth", default = "1M", type =
>>>>>>> bandwidth,
>>>>>>> -                help = 'target bandwidth for UDP tests in
>>>>>>> bits/second. Use '
>>>>>>> +                help = 'Target bandwidth for UDP tests in
>>>>>>> bits/second. Use '
>>>>>>>                 'postfix M or K to alter unit magnitude.')
>>>>>>> +    parser.add_argument("-i", "--interval", action = 'store',
>>>>>>> +                dest = "testInterval", default = "5", type = int,
>>>>>>>
>>>>>> Nit, 5 doesn't need to be in string format
>>>>>>
>>>>>>
>>>>>>> +                help = 'Interval for how long to run each test in
>>>>>>> seconds.')
>>>>>>> +
>>>>>>> +    parser.add_argument("-t", "--tunnel-modes", action = 'store',
>>>>>>> +                dest = "tunnelModes", default = [], type =
>>>>>>> tunnel_types,
>>>>>>>
>>>>>> Do you ever parse arguments multiple times, or modify this?  If it
>>>>>> doesn't
>>>>>> break anything else, might want to use () instead of [].  I'm mostly
>>>>>> just
>>>>>> paranoid, those bugs are tricky to catch if they do happen.
>>>>>>
>>>>>>
>>>>>>> +                help = 'Do L3 tests with the given tunnel modes.')
>>>>>>> +    parser.add_argument("-l", "--vlan-tag", action = 'store',
>>>>>>> +                dest = "vlanTag", default = None, type = vlan_tag,
>>>>>>> +                help = 'Do VLAN tests and use the given VLAN tag.')
>>>>>>> +    parser.add_argument("-d", "--direct", action = 'store_true',
>>>>>>> +                dest = "direct", default = None,
>>>>>>> +                help = 'Do direct tests between both ovs-test
>>>>>>> servers.')
>>>>>>> +
>>>>>>>     group = parser.add_mutually_exclusive_group(required = True)
>>>>>>>     group.add_argument("-s", "--server", action = "store", dest =
>>>>>>> "port",
>>>>>>>                 type = port,
>>>>>>> -                help = 'run in server mode and wait client to
>>>>>>> connect to this '
>>>>>>> -                'port')
>>>>>>> -    group.add_argument('-c', "--client", action = "store", nargs =
>>>>>>> 2,
>>>>>>> -                dest = "servers", type = server_endpoint,
>>>>>>> +                help = 'Run in server mode and wait for the client
>>>>>>> to '
>>>>>>> +                'connect to this port.')
>>>>>>> +    group.add_argument('-c', "--client", nargs = 2,
>>>>>>> +                dest = "servers", action = UniqueServerAction,
>>>>>>>                 metavar = ("SERVER1", "SERVER2"),
>>>>>>> -                help = 'run in client mode and do tests between
>>>>>>> these '
>>>>>>> -                'two servers. Each server must be specified in
>>>>>>> following '
>>>>>>> -                'format -
>>>>>>> ControlIP[:ControlPort][,TestIP[:TestPort]]. If '
>>>>>>> -                'TestIP is omitted then ovs-test server will also
>>>>>>> use the '
>>>>>>> -                'ControlIP for testing purposes. ControlPort is TCP
>>>>>>> port '
>>>>>>> -                'where server will listen for incoming XML/RPC
>>>>>>> control '
>>>>>>> -                'connections to schedule tests (by default 15531).
>>>>>>> TestPort '
>>>>>>> -                'is port which will be used by server to send test
>>>>>>> traffic '
>>>>>>> -                '(by default 15532)')
>>>>>>> +                help = 'Run in client mode and do tests between
>>>>>>> these '
>>>>>>> +                'two ovs-test servers. Each server must be
>>>>>>> specified in '
>>>>>>> +                'following format -
>>>>>>> OuterIP:OuterPort,InnerIP[/mask] '
>>>>>>> +                ':InnerPort. It is possible to start local instance
>>>>>>> of '
>>>>>>> +                'ovs-test server in the client mode by using
>>>>>>> 127.0.0.1 as '
>>>>>>> +                'OuterIP.')
>>>>>>>     return parser.parse_args()
>>>>>>> diff --git a/python/ovstest/rpcserver.py
>>>>>>> b/python/ovstest/rpcserver.py
>>>>>>> index 41d2569..9271c2b 100644
>>>>>>> --- a/python/ovstest/rpcserver.py
>>>>>>> +++ b/python/ovstest/rpcserver.py
>>>>>>> @@ -1,4 +1,4 @@
>>>>>>> -# Copyright (c) 2011 Nicira Networks
>>>>>>> +# Copyright (c) 2011, 2012 Nicira Networks
>>>>>>>  #
>>>>>>>  # Licensed under the Apache License, Version 2.0 (the "License");
>>>>>>>  # you may not use this file except in compliance with the License.
>>>>>>> @@ -21,13 +21,15 @@ from twisted.web import xmlrpc, server
>>>>>>>  from twisted.internet.error import CannotListenError
>>>>>>>  import udp
>>>>>>>  import tcp
>>>>>>> -import args
>>>>>>>  import util
>>>>>>> -
>>>>>>> +import vswitch
>>>>>>> +import xmlrpclib
>>>>>>> +import sys
>>>>>>> +import exceptions
>>>>>>>
>>>>>> Alphabetize again.  I have a slight personal preference for
>>>>>> segregating
>>>>>> builtin modules from local ones with a blank line, think that is in
>>>>>> google
>>>>>> style too, not sure about pep8.
>>>>>>
>>>>>>
>>>>>>>  class TestArena(xmlrpc.XMLRPC):
>>>>>>>     """
>>>>>>> -    This class contains all the functions that ovstest will call
>>>>>>> +    This class contains all the functions that ovs-test client will
>>>>>>> call
>>>>>>>     remotely. The caller is responsible to use designated handleIds
>>>>>>>     for designated methods (e.g. do not mix UDP and TCP handles).
>>>>>>>     """
>>>>>>> @@ -36,6 +38,9 @@ class TestArena(xmlrpc.XMLRPC):
>>>>>>>         xmlrpc.XMLRPC.__init__(self)
>>>>>>>         self.handle_id = 1
>>>>>>>         self.handle_map = {}
>>>>>>> +        self.bridges = set()
>>>>>>> +        self.ports = set()
>>>>>>> +        self.request = None
>>>>>>>
>>>>>>>     def __acquire_handle(self, value):
>>>>>>>         """
>>>>>>> @@ -58,6 +63,40 @@ class TestArena(xmlrpc.XMLRPC):
>>>>>>>         """
>>>>>>>         del self.handle_map[handle]
>>>>>>>
>>>>>>> +    def cleanup(self):
>>>>>>> +        """
>>>>>>> +        Delete all remaining bridges and ports if ovs-test client
>>>>>>> did not had
>>>>>>> +        a chance to remove them. It is necessary to call this
>>>>>>> function if
>>>>>>> +        ovs-test server is abruptly terminated when doing the tests.
>>>>>>> +        """
>>>>>>> +        for port in self.ports:
>>>>>>> +            vswitch.ovs_vsctl_del_port_from_bridge(port)
>>>>>>> +
>>>>>>> +        for bridge in self.bridges:
>>>>>>> +            vswitch.ovs_vsctl_del_bridge(bridge)
>>>>>>> +
>>>>>>> +    def render(self, request):
>>>>>>> +        """
>>>>>>> +        This method overrides the original XMLRPC.render method so
>>>>>>> that it
>>>>>>> +        would be possible to get the XML RPC client IP address from
>>>>>>> the
>>>>>>> +        request object.
>>>>>>> +        """
>>>>>>> +        self.request = request
>>>>>>> +        return xmlrpc.XMLRPC.render(self, request)
>>>>>>> +
>>>>>>> +    def xmlrpc_get_my_address(self):
>>>>>>> +        """
>>>>>>> +        Returns the RPC client's IP address.
>>>>>>> +        """
>>>>>>> +        return self.request.getClientIP()
>>>>>>>
>>>>>> I assume this is in a fairly structured environment where you aren't
>>>>>> worried
>>>>>> about thread-safety or other trickery.
>>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +    def xmlrpc_get_my_address_from(self, his_ip, his_port):
>>>>>>> +        """
>>>>>>> +        Returns the ovs-test server IP address that the other
>>>>>>> ovs-test server
>>>>>>> +        with the given ip will see.
>>>>>>> +        """
>>>>>>> +        server1 = xmlrpclib.Server("http://%s:%u/" % (his_ip,
>>>>>>> his_port))
>>>>>>> +        return server1.get_my_address()
>>>>>>
>>>>>>
>>>>>>>     def xmlrpc_create_udp_listener(self, port):
>>>>>>>         """
>>>>>>> @@ -171,6 +210,98 @@ class TestArena(xmlrpc.XMLRPC):
>>>>>>>             return -1
>>>>>>>         return 0
>>>>>>>
>>>>>>> +    def xmlrpc_create_test_bridge(self, bridge, iface):
>>>>>>> +        """
>>>>>>> +        This function creates a physical bridge from iface. It
>>>>>>> moves the
>>>>>>> +        IP configuration from the physical interface to the bridge.
>>>>>>> +        """
>>>>>>> +        ret = vswitch.ovs_vsctl_add_bridge(bridge)
>>>>>>> +        if ret == 0:
>>>>>>> +            self.bridges.add(bridge)
>>>>>>> +            util.interface_up(bridge)
>>>>>>> +            (ip_addr, mask) = util.interface_get_ip(iface)
>>>>>>> +            util.interface_assign_ip(bridge, ip_addr, mask)
>>>>>>> +            util.interface_assign_ip(iface, "0.0.0.0",
>>>>>>> "255.255.255.255")
>>>>>>> +            ret = vswitch.ovs_vsctl_add_port_to_bridge(bridge,
>>>>>>> iface)
>>>>>>> +            if ret == 0:
>>>>>>> +                self.ports.add(iface)
>>>>>>>
>>>>>> Is it the responsibility of this function to set the IP back to the
>>>>>> interface
>>>>>> if the add_port call fails?  I don't see anything in create_bridge to
>>>>>> do so.
>>>>>>
>>>>>>
>>>>>>> +        return ret
>>>>>>> +
>>>>>>> +    def xmlrpc_del_test_bridge(self, bridge, iface):
>>>>>>> +        """
>>>>>>> +        This function deletes the test bridge and moves its IP
>>>>>>> configuration
>>>>>>> +        back to the physical interface.
>>>>>>> +        """
>>>>>>> +        (ip_addr, mask) = util.interface_get_ip(bridge)
>>>>>>> +        util.interface_assign_ip(iface, ip_addr, mask)
>>>>>>> +        ret = vswitch.ovs_vsctl_del_bridge(bridge)
>>>>>>> +        return ret
>>>>>>> +
>>>>>>> +    def xmlrpc_get_iface_from_bridge(self, brname):
>>>>>>> +        """
>>>>>>> +        Tries to figure out physical interface from bridge.
>>>>>>> +        """
>>>>>>> +        return vswitch.ovs_get_physical_interface(brname)
>>>>>>> +
>>>>>>> +    def xmlrpc_create_bridge(self, brname):
>>>>>>> +        """
>>>>>>> +        Creates an OVS bridge.
>>>>>>> +        """
>>>>>>> +        ret = vswitch.ovs_vsctl_add_bridge(brname)
>>>>>>> +        if ret == 0:
>>>>>>> +            self.bridges.add(brname)
>>>>>>> +        return ret
>>>>>>> +
>>>>>>> +    def xmlrpc_del_bridge(self, brname):
>>>>>>> +        """
>>>>>>> +        Deletes an OVS bridge.
>>>>>>> +        """
>>>>>>> +        ret = vswitch.ovs_vsctl_del_bridge(brname)
>>>>>>> +        if ret == 0:
>>>>>>> +            self.bridges.discard(brname)
>>>>>>> +        return ret
>>>>>>> +
>>>>>>> +    def xmlrpc_is_ovs_bridge(self, bridge):
>>>>>>> +        """
>>>>>>> +        This function verifies whether given interface is an ovs
>>>>>>> bridge.
>>>>>>> +        """
>>>>>>> +        return vswitch.ovs_vsctl_is_ovs_bridge(bridge)
>>>>>>> +
>>>>>>> +    def xmlrpc_add_port_to_bridge(self, bridge, port):
>>>>>>> +        """
>>>>>>> +        Adds a port to the OVS bridge.
>>>>>>> +        """
>>>>>>> +        ret = vswitch.ovs_vsctl_add_port_to_bridge(bridge, port)
>>>>>>> +        if ret == 0:
>>>>>>> +            self.ports.add(port)
>>>>>>> +        return ret
>>>>>>> +
>>>>>>> +    def xmlrpc_del_port_from_bridge(self, port):
>>>>>>> +        """
>>>>>>> +        Removes a port from OVS bridge.
>>>>>>> +        """
>>>>>>> +        ret = vswitch.ovs_vsctl_del_port_from_bridge(port)
>>>>>>> +        if ret == 0:
>>>>>>> +            self.ports.discard(port)
>>>>>>> +        return ret
>>>>>>> +
>>>>>>> +    def xmlrpc_ovs_vsctl_set(self, table, record, column, key,
>>>>>>> value):
>>>>>>> +        """
>>>>>>> +        This function allows to alter OVS database.
>>>>>>> +        """
>>>>>>> +        return vswitch.ovs_vsctl_set(table, record, column, key,
>>>>>>> value)
>>>>>>> +
>>>>>>> +    def xmlrpc_interface_up(self, iface):
>>>>>>> +        """
>>>>>>> +        This function brings up given interface.
>>>>>>> +        """
>>>>>>> +        return util.interface_up(iface)
>>>>>>> +
>>>>>>> +    def xmlrpc_interface_assign_ip(self, iface, ip_address, mask):
>>>>>>> +        """
>>>>>>> +        This function allows to assing ip address to the given
>>>>>>> interface.
>>>>>>> +        """
>>>>>>> +        return util.interface_assign_ip(iface, ip_address, mask)
>>>>>>>
>>>>>>>     def xmlrpc_get_interface(self, address):
>>>>>>>         """
>>>>>>> @@ -198,6 +329,17 @@ class TestArena(xmlrpc.XMLRPC):
>>>>>>>
>>>>>>>
>>>>>>>  def start_rpc_server(port):
>>>>>>> -    RPC_SERVER = TestArena()
>>>>>>> -    reactor.listenTCP(port, server.Site(RPC_SERVER))
>>>>>>> -    reactor.run()
>>>>>>> +    """
>>>>>>> +    This function creates a RPC server and adds it to the Twisted
>>>>>>> Reactor.
>>>>>>> +    """
>>>>>>> +    rpc_server = TestArena()
>>>>>>> +    reactor.listenTCP(port, server.Site(rpc_server))
>>>>>>> +    try:
>>>>>>> +        print "Starting RPC server\n"
>>>>>>> +        sys.stdout.flush()
>>>>>>> +         # If this server was started from ovs-test client then we
>>>>>>> must flush
>>>>>>> +         # STDOUT so that client would know that server is ready to
>>>>>>> accept
>>>>>>> +         # XML RPC connections.
>>>>>>> +        reactor.run()
>>>>>>> +    finally:
>>>>>>> +        rpc_server.cleanup()
>>>>>>>
>>>>>> May want to add a cleaning up message, don't know if that is obvious
>>>>>> in context.
>>>>>>
>>>>>>
>>>>>>> diff --git a/python/ovstest/tcp.py b/python/ovstest/tcp.py
>>>>>>> index 33dc719..738d06b 100644
>>>>>>> --- a/python/ovstest/tcp.py
>>>>>>> +++ b/python/ovstest/tcp.py
>>>>>>> @@ -1,4 +1,4 @@
>>>>>>> -# Copyright (c) 2011 Nicira Networks
>>>>>>> +# Copyright (c) 2011, 2012 Nicira Networks
>>>>>>>  #
>>>>>>>  # Licensed under the Apache License, Version 2.0 (the "License");
>>>>>>>  # you may not use this file except in compliance with the License.
>>>>>>> @@ -29,14 +29,10 @@ class TcpListenerConnection(Protocol):
>>>>>>>     def __init__(self):
>>>>>>>         self.stats = 0
>>>>>>>
>>>>>>> -    def connectionMade(self):
>>>>>>> -        print "Started TCP Listener connection"
>>>>>>> -
>>>>>>>     def dataReceived(self, data):
>>>>>>>         self.stats += len(data)
>>>>>>>
>>>>>>>     def connectionLost(self, reason):
>>>>>>> -        print "Stopped TCP Listener connection"
>>>>>>
>>>>>>         self.factory.stats += self.stats
>>>>>>>
>>>>>>>
>>>>>>> @@ -50,12 +46,6 @@ class TcpListenerFactory(Factory):
>>>>>>>     def __init__(self):
>>>>>>>         self.stats = 0
>>>>>>>
>>>>>>> -    def startFactory(self):
>>>>>>> -        print "Starting TCP listener factory"
>>>>>>> -
>>>>>>> -    def stopFactory(self):
>>>>>>> -        print "Stopping TCP listener factory"
>>>>>>> -
>>>>>>>     def getResults(self):
>>>>>>>         """ returns the number of bytes received as string"""
>>>>>>>         #XML RPC does not support 64bit int (
>>>>>>> http://bugs.python.org/issue2985)
>>>>>>> @@ -104,18 +94,13 @@ class TcpSenderConnection(Protocol):
>>>>>>>     """
>>>>>>>
>>>>>>>     def connectionMade(self):
>>>>>>> -        print "Started TCP sender connection"
>>>>>>>         producer = Producer(self, self.factory.duration)
>>>>>>>         self.transport.registerProducer(producer, True)
>>>>>>>         producer.resumeProducing()
>>>>>>>
>>>>>>>     def dataReceived(self, data):
>>>>>>> -        print "Sender received data!", data
>>>>>>>         self.transport.loseConnection()
>>>>>>>
>>>>>>> -    def connectionLost(self, reason):
>>>>>>> -        print "Stopped TCP sender connection"
>>>>>>> -
>>>>>>>
>>>>>>>  class TcpSenderFactory(ClientFactory):
>>>>>>>     """
>>>>>>> @@ -128,12 +113,6 @@ class TcpSenderFactory(ClientFactory):
>>>>>>>         self.duration = duration
>>>>>>>         self.stats = 0
>>>>>>>
>>>>>>> -    def startFactory(self):
>>>>>>> -        print "Starting TCP sender factory"
>>>>>>> -
>>>>>>> -    def stopFactory(self):
>>>>>>> -        print "Stopping TCP sender factory"
>>>>>>> -
>>>>>>>     def getResults(self):
>>>>>>>         """Returns amount of bytes sent to the Listener (as a
>>>>>>> string)"""
>>>>>>>         return str(self.stats)
>>>>>>> diff --git a/python/ovstest/udp.py b/python/ovstest/udp.py
>>>>>>> index e09569d..a4d85b9 100644
>>>>>>> --- a/python/ovstest/udp.py
>>>>>>> +++ b/python/ovstest/udp.py
>>>>>>> @@ -1,4 +1,4 @@
>>>>>>> -# Copyright (c) 2011 Nicira Networks
>>>>>>> +# Copyright (c) 2011, 2012 Nicira Networks
>>>>>>>  #
>>>>>>>  # Licensed under the Apache License, Version 2.0 (the "License");
>>>>>>>  # you may not use this file except in compliance with the License.
>>>>>>> @@ -28,12 +28,6 @@ class UdpListener(DatagramProtocol):
>>>>>>>     def __init__(self):
>>>>>>>         self.stats = []
>>>>>>>
>>>>>>> -    def startProtocol(self):
>>>>>>> -        print "Starting UDP listener"
>>>>>>> -
>>>>>>> -    def stopProtocol(self):
>>>>>>> -        print "Stopping UDP listener"
>>>>>>> -
>>>>>>>     def datagramReceived(self, data, (_1, _2)):
>>>>>>>         """This function is called each time datagram is received"""
>>>>>>>         try:
>>>>>>> @@ -61,13 +55,11 @@ class UdpSender(DatagramProtocol):
>>>>>>>         self.data = array.array('c', 'X' * size)
>>>>>>>
>>>>>>>     def startProtocol(self):
>>>>>>> -        print "Starting UDP sender"
>>>>>>>         self.looper = LoopingCall(self.sendData)
>>>>>>>         period = self.duration / float(self.count)
>>>>>>>         self.looper.start(period , now = False)
>>>>>>>
>>>>>>>     def stopProtocol(self):
>>>>>>> -        print "Stopping UDP sender"
>>>>>>>         if (self.looper is not None):
>>>>>>>             self.looper.stop()
>>>>>>>             self.looper = None
>>>>>>> diff --git a/python/ovstest/util.py b/python/ovstest/util.py
>>>>>>> index 3321e69..8def507 100644
>>>>>>> --- a/python/ovstest/util.py
>>>>>>> +++ b/python/ovstest/util.py
>>>>>>> @@ -1,4 +1,4 @@
>>>>>>> -# Copyright (c) 2011 Nicira Networks
>>>>>>> +# Copyright (c) 2011, 2012 Nicira Networks
>>>>>>>  #
>>>>>>>  # Licensed under the Apache License, Version 2.0 (the "License");
>>>>>>>  # you may not use this file except in compliance with the License.
>>>>>>> @@ -15,13 +15,21 @@
>>>>>>>  """
>>>>>>>  util module contains some helper function
>>>>>>>  """
>>>>>>> -import socket, struct, fcntl, array, os, subprocess, exceptions
>>>>>>> +import socket, struct, fcntl, array, os, subprocess, exceptions, re
>>>>>>>
>>>>>> Style prefers one import per line
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> -def str_ip(ip):
>>>>>>> -    (x1, x2, x3, x4) = struct.unpack("BBBB", ip)
>>>>>>> +
>>>>>>> +def str_ip(ip_address):
>>>>>>> +    """
>>>>>>> +    Converts an IP address from binary format to a string.
>>>>>>> +    """
>>>>>>> +    (x1, x2, x3, x4) = struct.unpack("BBBB", ip_address)
>>>>>>>     return ("%u.%u.%u.%u") % (x1, x2, x3, x4)
>>>>>>>
>>>>>>> +
>>>>>>>  def get_interface_mtu(iface):
>>>>>>> +    """
>>>>>>> +    Returns MTU of the given interface.
>>>>>>> +    """
>>>>>>>     s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
>>>>>>>     indata = iface + ('\0' * (32 - len(iface)))
>>>>>>>     try:
>>>>>>> @@ -32,6 +40,7 @@ def get_interface_mtu(iface):
>>>>>>>
>>>>>>>     return mtu
>>>>>>>
>>>>>>> +
>>>>>>>  def get_interface(address):
>>>>>>>     """
>>>>>>>     Finds first interface that has given address
>>>>>>> @@ -52,23 +61,70 @@ def get_interface(address):
>>>>>>>             return name
>>>>>>>     return "" #  did not find interface we were looking for
>>>>>>>
>>>>>>> +
>>>>>>>  def uname():
>>>>>>>     os_info = os.uname()
>>>>>>>     return os_info[2] #return only the kernel version number
>>>>>>>
>>>>>>> -def get_driver(iface):
>>>>>>> +
>>>>>>> +def start_process(args):
>>>>>>>     try:
>>>>>>> -        p = subprocess.Popen(
>>>>>>> -            ["ethtool", "-i", iface],
>>>>>>> +        p = subprocess.Popen(args,
>>>>>>>             stdin = subprocess.PIPE,
>>>>>>>             stdout = subprocess.PIPE,
>>>>>>>             stderr = subprocess.PIPE)
>>>>>>>         out, err = p.communicate()
>>>>>>> -        if p.returncode == 0:
>>>>>>> -            lines = out.split("\n")
>>>>>>> -            driver = "%s(%s)" % (lines[0], lines[1]) #driver name +
>>>>>>> version
>>>>>>> -        else:
>>>>>>> -            driver = "no support for ethtool"
>>>>>>> +        return (p.returncode, out, err)
>>>>>>>     except exceptions.OSError:
>>>>>>> -        driver = ""
>>>>>>> +        return (-1, None, None)
>>>>>>> +
>>>>>>> +
>>>>>>> +def get_driver(iface):
>>>>>>> +    ret, out, _err = start_process(["ethtool", "-i", iface])
>>>>>>> +    if ret == 0:
>>>>>>> +        lines = out.split("\n")
>>>>>>>
>>>>>> Could use splitlines, doesn't matter in this context
>>>>>>
>>>>>>
>>>>>>> +        driver = "%s(%s)" % (lines[0], lines[1]) #driver name +
>>>>>>> version
>>>>>>> +    else:
>>>>>>> +        driver = "<unable to get driver information from ethtool>"
>>>>>>>     return driver
>>>>>>> +
>>>>>>> +
>>>>>>> +def interface_up(iface):
>>>>>>> +    """
>>>>>>> +    This function brings given iface up.
>>>>>>> +    """
>>>>>>> +    ret, _out, _err = start_process(["ifconfig", iface, "up"])
>>>>>>> +    return ret
>>>>>>> +
>>>>>>> +
>>>>>>> +def interface_assign_ip(iface, ip_addr, mask):
>>>>>>> +    """
>>>>>>> +    This function allows to assign IP address to an interface. If
>>>>>>> mask is an
>>>>>>> +    empty string then ifconfig will decide what kind of mask to
>>>>>>> use. The
>>>>>>> +    caller can also specify the mask by using CIDR notation in ip
>>>>>>> argument by
>>>>>>> +    leaving the mask argument as an empty string. In case of
>>>>>>> success this
>>>>>>> +    function returns 0.
>>>>>>> +    """
>>>>>>> +    args = ["ifconfig", iface, ip_addr]
>>>>>>> +    if mask != "":
>>>>>>> +        args.append("netmask")
>>>>>>> +        args.append(mask)
>>>>>>> +    ret, _out, _err = start_process(args)
>>>>>>> +    return ret
>>>>>>> +
>>>>>>> +
>>>>>>> +def interface_get_ip(iface):
>>>>>>> +    """
>>>>>>> +    This function returns tuple - ip and mask that was assigned to
>>>>>>> the
>>>>>>> +    interface.
>>>>>>> +    """
>>>>>>> +    args = ["ifconfig", iface]
>>>>>>> +    ret, out, _err = start_process(args)
>>>>>>> +
>>>>>>> +    if ret == 0:
>>>>>>> +        ip = re.search(r'inet addr:(\S+)', out)
>>>>>>> +        mask = re.search(r'Mask:(\S+)', out)
>>>>>>> +        if ip is not None and mask is not None:
>>>>>>> +            return (ip.group(1), mask.group(1))
>>>>>>> +    else:
>>>>>>> +        return ret
>>>>>>> diff --git a/python/ovstest/vswitch.py b/python/ovstest/vswitch.py
>>>>>>> new file mode 100644
>>>>>>> index 0000000..5bd15d0
>>>>>>> --- /dev/null
>>>>>>> +++ b/python/ovstest/vswitch.py
>>>>>>> @@ -0,0 +1,97 @@
>>>>>>> +# Copyright (c) 2012 Nicira Networks
>>>>>>> +#
>>>>>>> +# Licensed under the Apache License, Version 2.0 (the "License");
>>>>>>> +# you may not use this file except in compliance with the License.
>>>>>>> +# You may obtain a copy of the License at:
>>>>>>> +#
>>>>>>> +#     http://www.apache.org/licenses/LICENSE-2.0
>>>>>>> +#
>>>>>>> +# Unless required by applicable law or agreed to in writing,
>>>>>>> software
>>>>>>> +# distributed under the License is distributed on an "AS IS" BASIS,
>>>>>>> +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
>>>>>>> implied.
>>>>>>> +# See the License for the specific language governing permissions
>>>>>>> and
>>>>>>> +# limitations under the License.
>>>>>>> +
>>>>>>> +"""
>>>>>>> +vswitch module allows its callers to interact with OVS DB.
>>>>>>> +"""
>>>>>>> +import subprocess
>>>>>>> +import exceptions
>>>>>>> +import util
>>>>>>>
>>>>>> alphabetize, maybe split builtins
>>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +
>>>>>>> +def ovs_vsctl_add_bridge(bridge):
>>>>>>> +    """
>>>>>>> +    This function creates an OVS bridge.
>>>>>>> +    """
>>>>>>> +    ret, _out, _err = util.start_process(["ovs-vsctl", "add-br",
>>>>>>> bridge])
>>>>>>> +    return ret
>>>>>>> +
>>>>>>> +
>>>>>>> +def ovs_vsctl_del_bridge(bridge):
>>>>>>> +    """
>>>>>>> +    This function deletes the OVS bridge.
>>>>>>> +    """
>>>>>>> +    ret, _out, _err = util.start_process(["ovs-vsctl", "del-br",
>>>>>>> bridge])
>>>>>>> +    return ret
>>>>>>> +
>>>>>>> +
>>>>>>> +def ovs_vsctl_is_ovs_bridge(bridge):
>>>>>>> +    """
>>>>>>> +    This function verifies whether given port is an OVS bridge. If
>>>>>>> it is an
>>>>>>> +    OVS bridge then it will return True.
>>>>>>> +    """
>>>>>>> +    ret, _out, _err = util.start_process(["ovs-vsctl", "br-exists",
>>>>>>> bridge])
>>>>>>> +    return ret == 0
>>>>>>> +
>>>>>>> +
>>>>>>> +def ovs_vsctl_add_port_to_bridge(bridge, iface):
>>>>>>> +    """
>>>>>>> +    This function adds given interface to the bridge.
>>>>>>> +    """
>>>>>>> +    ret, _out, _err = util.start_process(["ovs-vsctl", "add-port",
>>>>>>> bridge,
>>>>>>> +                                          iface])
>>>>>>> +    return ret
>>>>>>> +
>>>>>>> +
>>>>>>> +def ovs_vsctl_del_port_from_bridge(port):
>>>>>>> +    """
>>>>>>> +    This function removes given port from a OVS bridge.
>>>>>>> +    """
>>>>>>> +    ret, _out, _err = util.start_process(["ovs-vsctl", "del-port",
>>>>>>> port])
>>>>>>> +    return ret
>>>>>>> +
>>>>>>> +
>>>>>>> +def ovs_vsctl_set(table, record, column, key, value):
>>>>>>> +    """
>>>>>>> +    This function allows to alter OVS database. If column is a map,
>>>>>>> then
>>>>>>>
>>>>>> s/alter OVS/alter the OVS/
>>>>>>
>>>>>>
>>>>>>> +    caller should also set the key, otherwise the key should be
>>>>>>> left as an
>>>>>>> +    empty string.
>>>>>>> +    """
>>>>>>> +    if key == "":
>>>>>>>
>>>>>> A few places here you are using "" as the empty value, you may want
>>>>>> to use
>>>>>> None instead (not sure if these are the result of string parsing
>>>>>> elsewhere).
>>>>>> Regardless, you could support both by just doing "if foo:" in these
>>>>>> places.
>>>>>>
>>>>> Actually I decided that it would be better to use "" instead of None,
>>>>> because this value is passed from RPC client to the server (RPC
>>>>> by default do not handle them).
>>>>>
>>>>> Though, I guess I could try to enable None on Twisted by
>>>>> setting allowNone to True. And by enabling allow_none in the
>>>>> xmlrpclib.
>>>>>
>>>>
>>>>  Ah, I forgot this was through RPC.  None works fine if you make those
>>>> two
>>>> changes, but it isn't a big deal.
>>>>
>>>>
>>>>>
>>>>>>
>>>>>>> +        index = column
>>>>>>> +    else:
>>>>>>> +        index = "%s:%s" % (column, key)
>>>>>>> +    index_value = "%s=%s" % (index, value)
>>>>>>> +    ret, _out, _err = util.start_process(["ovs-vsctl", "set",
>>>>>>> table, record,
>>>>>>> +                                          index_value])
>>>>>>> +    return ret
>>>>>>> +
>>>>>>> +
>>>>>>> +def ovs_get_physical_interface(bridge):
>>>>>>> +    """
>>>>>>> +    This function tries to figure out which is the physical
>>>>>>> interface that
>>>>>>> +    belongs to the bridge. If there are multiple physical
>>>>>>> interfaces assigned
>>>>>>> +    to this bridge then it will return the first match.
>>>>>>> +    """
>>>>>>> +    ret, out, _err = util.start_process(["ovs-vsctl",
>>>>>>> "list-ifaces", bridge])
>>>>>>> +
>>>>>>> +    if ret == 0:
>>>>>>> +        ifaces = out.split("\n")
>>>>>>>
>>>>>> splitlines might be slightly more useful here, since it would drop a
>>>>>> trailing newline
>>>>>>
>>>>>>
>>>>>>> +        for iface in ifaces:
>>>>>>> +            ret, out, _err = util.start_process(["ovs-vsctl", "get",
>>>>>>> +                                                 "Interface",
>>>>>>> iface, "type"])
>>>>>>> +            if ret == 0:
>>>>>>> +                if ('""' in out) or ('system' in out):
>>>>>>> +                    return iface #this should be the physical
>>>>>>> interface
>>>>>>>
>>>>>> Formatting nit, two spaces before '#', one after:  "<code>  #
>>>>>> Comment"
>>>>>>
>>>>>> +    return -1
>>>>>>>
>>>>>> May want to use None as the blank value here
>>>>>>
>>>>>>
>>>>>>> diff --git a/utilities/ovs-test.8.in b/utilities/ovs-test.8.in
>>>>>>> index 87c8944..d5f9f20 100644
>>>>>>> --- a/utilities/ovs-test.8.in
>>>>>>> +++ b/utilities/ovs-test.8.in
>>>>>>> @@ -3,41 +3,47 @@
>>>>>>>  .  ns
>>>>>>>  .  IP "\\$1"
>>>>>>>  ..
>>>>>>> -.TH ovs\-test 1 "October 2011" "Open vSwitch" "Open vSwitch Manual"
>>>>>>> +.TH ovs\-test 1 "March 2012" "Open vSwitch" "Open vSwitch Manual"
>>>>>>>
>>>>>> April?
>>>>>>
>>>>>>
>>>>>>>  .
>>>>>>>  .SH NAME
>>>>>>> -\fBovs\-test\fR \- check Linux drivers for performance and vlan
>>>>>>> problems
>>>>>>> +\fBovs\-test\fR \- check Linux drivers for performance, vlan and L3
>>>>>>> tunneling
>>>>>>> +problems
>>>>>>>  .
>>>>>>>  .SH SYNOPSIS
>>>>>>>  \fBovs\-test\fR \fB\-s\fR \fIport\fR
>>>>>>>  .PP
>>>>>>> -\fBovs\-test\fR \fB\-c\fR \fIserver1\fR
>>>>>>> -\fIserver2\fR [\fB\-b\fR \fIbandwidth\fR]
>>>>>>> +\fBovs\-test\fR \fB\-c\fR \fIserver1\fR \fIserver2\fR
>>>>>>> +[\fB\-b\fR \fItargetbandwidth\fR] [\fB\-i\fR \fItestinterval\fR]
>>>>>>> +[\fB\-d\fR]
>>>>>>> +[\fB\-l\fR \fIvlantag\fR]
>>>>>>> +[\fB\-t\fR \fItunnelmodes\fR]
>>>>>>>  .so lib/common-syn.man
>>>>>>>  .
>>>>>>>  .SH DESCRIPTION
>>>>>>>  The \fBovs\-test\fR program may be used to check for problems
>>>>>>> sending
>>>>>>> -802.1Q traffic that Open vSwitch may uncover. These problems can
>>>>>>> -occur when Open vSwitch is used to send 802.1Q traffic through
>>>>>>> physical
>>>>>>> -interfaces running certain drivers of certain Linux kernel
>>>>>>> versions. To run a
>>>>>>> -test, configure Open vSwitch to tag traffic originating from
>>>>>>> \fIserver1\fR and
>>>>>>> -forward it to the \fIserver2\fR. On both servers run \fBovs\-test\fR
>>>>>>> -in server mode. Then, on any other host, run the \fBovs\-test\fR in
>>>>>>> client
>>>>>>> -mode. The client will connect to both \fBovs\-test\fR servers and
>>>>>>> schedule
>>>>>>> -tests between them. \fBovs\-test\fR will perform UDP and TCP tests.
>>>>>>> +802.1Q or GRE traffic that Open vSwitch may uncover. These problems,
>>>>>>> +for example, can occur when Open vSwitch is used to send 802.1Q
>>>>>>> traffic
>>>>>>> +through physical interfaces running certain drivers of certain
>>>>>>> Linux kernel
>>>>>>> +versions. To run a test, configure IP addresses on \fIserver1\fR and
>>>>>>> +\fIserver2\fR for interfaces you intended to test. These interfaces
>>>>>>> could
>>>>>>> +also be already configured OVS bridges that have a physical
>>>>>>> interface attached
>>>>>>> +to them. Then, on one of the nodes, run \fBovs\-test\fR in server
>>>>>>> mode and on
>>>>>>> +the other node run it in client mode. The client will connect to
>>>>>>> +\fBovs\-test\fR server and schedule tests between both of them. The
>>>>>>> +\fBovs\-test\fR client will perform UDP and TCP tests.
>>>>>>>  .PP
>>>>>>> -UDP tests can report packet loss and achieved bandwidth, because
>>>>>>> UDP flow
>>>>>>> -control is done inside \fBovs\-test\fR. It is also possible to
>>>>>>> specify target
>>>>>>> -bandwidth for UDP. By default it is 1Mbit/s.
>>>>>>> +UDP tests can report packet loss and achieved bandwidth for various
>>>>>>> +datagram sizes. By default target bandwidth for UDP tests is
>>>>>>> 1Mbit/s.
>>>>>>>  .PP
>>>>>>>  TCP tests report only achieved bandwidth, because kernel TCP stack
>>>>>>>  takes care of flow control and packet loss. TCP tests are essential
>>>>>>> to detect
>>>>>>> -potential TSO related VLAN issues.
>>>>>>> +potential TSO related issues.
>>>>>>>  .PP
>>>>>>> -To determine whether Open vSwitch is encountering any 802.1Q
>>>>>>> related problems,
>>>>>>> +To determine whether Open vSwitch is encountering any 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,
>>>>>>> +traffic is being directly sent and in one where it is not. If in the
>>>>>>> +802.1Q or L3 tunneled tests both \fBovs\-test\fR processes are
>>>>>>> unable to
>>>>>>> +communicate or the achieved bandwidth is much lower compared to
>>>>>>> direct setup,
>>>>>>>  then, most likely, Open vSwitch has encountered a pre-existing
>>>>>>> kernel or
>>>>>>>  driver bug.
>>>>>>>  .PP
>>>>>>> @@ -46,71 +52,87 @@ Some examples of the types of problems that may
>>>>>>> be encountered are:
>>>>>>>  .
>>>>>>>  .SS "Client Mode"
>>>>>>>  An \fBovs\-test\fR client will connect to two \fBovs\-test\fR
>>>>>>> servers and
>>>>>>> -will ask them to exchange traffic.
>>>>>>> +will ask them to exchange test traffic. It is also possible to
>>>>>>> spawn an
>>>>>>> +\fBovs\-test\fR server automatically from the client.
>>>>>>>  .
>>>>>>>  .SS "Server Mode"
>>>>>>>  To conduct tests, two \fBovs\-test\fR servers must be running on
>>>>>>> two different
>>>>>>> -hosts where client can connect. The actual test traffic is
>>>>>>> exchanged only
>>>>>>> -between both \fBovs\-test\fR server test IP addresses. It is
>>>>>>> recommended that
>>>>>>> -both servers have their test IP addresses in the same subnet,
>>>>>>> otherwise one
>>>>>>> -will need to change routing so that the test traffic actually goes
>>>>>>> through the
>>>>>>> -interface that he originally intended to test.
>>>>>>> +hosts where the client can connect. The actual test traffic is
>>>>>>> exchanged only
>>>>>>> +between both \fBovs\-test\fR servers. It is recommended that both
>>>>>>> servers have
>>>>>>> +their IP addresses in the same subnet, otherwise one would have to
>>>>>>> make sure
>>>>>>> +that routing is set up correctly.
>>>>>>>  .
>>>>>>>  .SH OPTIONS
>>>>>>>  .
>>>>>>>  .IP "\fB\-s \fIport\fR"
>>>>>>>  .IQ "\fB\-\-server\fR \fIport\fR"
>>>>>>> -Run in server mode and wait for a client to establish XML RPC
>>>>>>> Control
>>>>>>> -Connection on TCP \fIport\fR. It is recommended to have ethtool
>>>>>>> installed on
>>>>>>> -the server so that it could retrieve information about NIC driver.
>>>>>>> +Run in server mode and wait for the client to establish XML RPC
>>>>>>> Control
>>>>>>> +Connection on this TCP \fIport\fR. It is recommended to have
>>>>>>> \fBethtool\fR(8)
>>>>>>> +installed on the server so that it could retrieve information about
>>>>>>> NIC
>>>>>>> +driver.
>>>>>>>
>>>>>> i'd go with either "NIC drivers" or "the NIC driver"
>>>>>>
>>>>>>
>>>>>>>  .
>>>>>>>  .IP "\fB\-c \fIserver1\fR \fIserver2\fR"
>>>>>>>  .IQ "\fB\-\-client \fIserver1\fR \fIserver2\fR"
>>>>>>>  Run in client mode and schedule tests between \fIserver1\fR and
>>>>>>> \fIserver2\fR,
>>>>>>> -where each \fIserver\fR must be given in following format -
>>>>>>> -ControlIP[:ControlPort][,TestIP[:TestPort]]. If TestIP is omitted
>>>>>>> then
>>>>>>> -ovs-test server will use the ControlIP for testing purposes.
>>>>>>> ControlPort is
>>>>>>> -TCP port where server will listen for incoming XML/RPC control
>>>>>>> -connections to schedule tests (by default it is 15531). TestPort
>>>>>>> -is port which will be used by server to listen for test traffic
>>>>>>> -(by default it is 15532).
>>>>>>> -.
>>>>>>> -.IP "\fB\-b \fIbandwidth\fR"
>>>>>>> -.IQ "\fB\-\-bandwidth\fR \fIbandwidth\fR"
>>>>>>> -Target bandwidth for UDP tests. The \fIbandwidth\fR must be given
>>>>>>> in bits per
>>>>>>> -second. It is possible to use postfix M or K to alter the target
>>>>>>> bandwidth
>>>>>>> -magnitude.
>>>>>>> +where each \fIserver\fR must be given in the following format -
>>>>>>> +\fIOuterIP[:OuterPort],InnerIP[/Mask][:InnerPort]\fR. The
>>>>>>> \fIOuterIP\fR must
>>>>>>> +be already assigned to the physical interface which is going to be
>>>>>>> tested.
>>>>>>> +This is the IP address where client will try to establish XML RPC
>>>>>>> connection.
>>>>>>> +If \fIOuterIP\fR is 127.0.0.1 then client will automatically spawn
>>>>>>> a local
>>>>>>> +instance of \fBovs\-test\fR server. \fIOuterPort\fR is TCP port
>>>>>>> where server
>>>>>>> +is listening for incoming XML/RPC control connections to schedule
>>>>>>> tests (by
>>>>>>> +default it is 15531). The \fBovs\-test\fR will automatically assign
>>>>>>> +\fIInnerIP[/Mask]\fR to the interfaces that will be created on the
>>>>>>> fly for
>>>>>>> +testing purposes. It is important that \fIInnerIP[/Mask]\fR does
>>>>>>> not interfere
>>>>>>> +with already existing IP addresses on both \fBovs\-test\fR servers
>>>>>>> and client.
>>>>>>> +\fIInnerPort\fR is port which will be used by server to listen for
>>>>>>> test
>>>>>>> +traffic that will be encapsulated (by default it is 15532).
>>>>>>> +.
>>>>>>> +.IP "\fB\-b \fItargetbandwidth\fR"
>>>>>>> +.IQ "\fB\-\-bandwidth\fR \fItargetbandwidth\fR"
>>>>>>> +Target bandwidth for UDP tests. The \fItargetbandwidth\fR must be
>>>>>>> given in
>>>>>>> +bits per second. It is possible to use postfix M or K to alter the
>>>>>>> target
>>>>>>> +bandwidth magnitude.
>>>>>>> +.
>>>>>>> +.IP "\fB\-i \fItestinterval\fR"
>>>>>>> +.IQ "\fB\-\-interval\fR \fItestinterval\fR"
>>>>>>> +For how long each test should run. By default 5 seconds.
>>>>>>>
>>>>>> I think s/For how long/How long/
>>>>>>
>>>>>>
>>>>>>>  .
>>>>>>>  .so lib/common.man
>>>>>>> -.SH EXAMPLES
>>>>>>> -.PP
>>>>>>> -Set up a bridge which forwards traffic originating from
>>>>>>> \fB1.2.3.4\fR out
>>>>>>> -\fBeth1\fR with VLAN tag 10.
>>>>>>> -.IP
>>>>>>> -.B ovs\-vsctl \-\- add\-br vlan\-br \(rs
>>>>>>> -.IP
>>>>>>> -.B \-\- add\-port vlan\-br eth1 \(rs
>>>>>>> -.IP
>>>>>>> -.B \-\- add\-port vlan\-br vlan\-br\-tag tag=10 \(rs
>>>>>>> -.IP
>>>>>>> -.B \-\- set Interface vlan\-br\-tag type=internal
>>>>>>> -.IP
>>>>>>> -.B ifconfig vlan\-br\-tag up 1.2.3.4
>>>>>>>  .
>>>>>>> +.SH "Test Modes"
>>>>>>> +The following test modes are supported by \fBovs\-test\fR. It is
>>>>>>> possible
>>>>>>> +to combine multiple of them in a single \fBovs\-test\fR invocation.
>>>>>>> +.
>>>>>>> +.IP "\fB\-d \fR"
>>>>>>> +.IQ "\fB\-\-direct\fR"
>>>>>>> +Perform direct tests between both \fIOuterIP\fR addresses. These
>>>>>>> tests could
>>>>>>> +be used as a reference to compare 802.1Q or L3 tunneling test
>>>>>>> results.
>>>>>>> +.
>>>>>>> +.IP "\fB\-l \fIvlantag\fR"
>>>>>>> +.IQ "\fB\-\-vlan\-tag\fR \fIvlantag\fR"
>>>>>>> +Perform 802.1Q tests between both servers. These tests will create
>>>>>>> a temporary
>>>>>>> +OVS bridge, if necessary, and attach a VLAN tagged port to it for
>>>>>>> testing
>>>>>>> +purposes.
>>>>>>> +.
>>>>>>> +.IP "\fB\-t \fItunnelmodes\fR"
>>>>>>> +.IQ "\fB\-\-tunnel\-modes\fR \fItunnelmodes\fR"
>>>>>>> +Perform L3 tunneling tests. The given argument is a comma separated
>>>>>>> string
>>>>>>> +that specifies all the L3 tunnel modes that should be tested (e.g.
>>>>>>> gre). The L3
>>>>>>> +tunnels are terminated on interface that has the \fIOuterIP\fR
>>>>>>> address
>>>>>>> +assigned.
>>>>>>> +.
>>>>>>> +.SH EXAMPLES
>>>>>>>  .PP
>>>>>>> -On two different hosts start \fBovs\-test\fR in server mode and
>>>>>>> tell them to
>>>>>>> -listen on port 15531 for incoming client control connections:
>>>>>>> -.IP
>>>>>>> -.B 1.2.3.4: ovs\-test \-s 15531
>>>>>>> +On host 1.2.3.4 start \fBovs\-test\fR in server mode:
>>>>>>>  .IP
>>>>>>> -.B 1.2.3.5: ovs\-test \-s 15531
>>>>>>> +.B ovs\-test \-s 15531
>>>>>>>
>>>>>> Is 15531 implied/default now?  Omitting it here would mean less
>>>>>> documentation
>>>>>> changes to make if you ever modify the default =)
>>>>>>
>>>>>>
>>>>>>>  .
>>>>>>>  .PP
>>>>>>> -On any other host start \fBovs\-test\fR in client mode and ask it
>>>>>>> to connect
>>>>>>> -to those two servers - one at 1.2.3.4 and another at 1.2.3.5 (by
>>>>>>> default
>>>>>>> -client will use TCP port 15531 to establish control channel).
>>>>>>> +On host 1.2.3.5 start \fBovs\-test\fR in client mode and do direct,
>>>>>>> VLAN and
>>>>>>> +GRE tests between both nodes:
>>>>>>>  .IP
>>>>>>> -.B ovs\-test -c 1.2.3.4 1.2.3.5
>>>>>>> +.B ovs\-test \-c 127.0.0.1,1.1.1.1/30 1.2.3.4,1.1.1.2/30 -d -l 123
>>>>>>> -t gre
>>>>>>>  .
>>>>>>>  .SH SEE ALSO
>>>>>>>  .
>>>>>>> @@ -119,4 +141,4 @@ client will use TCP port 15531 to establish
>>>>>>> control channel).
>>>>>>>  .BR ovs\-vsctl (8),
>>>>>>>  .BR ovs\-vlan\-test (8),
>>>>>>>  .BR ethtool (8),
>>>>>>> -.BR uname (1)
>>>>>>> +.BR uname (1)
>>>>>>> \ No newline at end of file
>>>>>>> diff --git a/utilities/ovs-test.in b/utilities/ovs-test.in
>>>>>>> index 6518dbc..f6a5350 100644
>>>>>>> --- a/utilities/ovs-test.in
>>>>>>> +++ b/utilities/ovs-test.in
>>>>>>> @@ -1,4 +1,4 @@
>>>>>>> -#! @PYTHON@
>>>>>>> +#!/usr/bin/python
>>>>>>>  #
>>>>>>>  # Licensed under the Apache License, Version 2.0 (the "License");
>>>>>>>  # you may not use this file except in compliance with the License.
>>>>>>> @@ -21,11 +21,74 @@ import xmlrpclib
>>>>>>>  import time
>>>>>>>  import socket
>>>>>>>  import math
>>>>>>> +import os
>>>>>>> +import subprocess
>>>>>>> +import fcntl
>>>>>>> +import select
>>>>>>> +import signal
>>>>>>> +import argparse
>>>>>>> +import sys
>>>>>>>
>>>>>> sort
>>>>>>
>>>>>>
>>>>>> +
>>>>>>>  from ovstest import args, rpcserver
>>>>>>>
>>>>>> Google style prefers
>>>>>> import ovstest.args as args
>>>>>> import ovstest.rpcserver as rpcserver
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> +DEFAULT_TEST_BRIDGE="ovstestbr0"
>>>>>>> +DEFAULT_TEST_PORT="ovstestport0"
>>>>>>> +DEFAULT_TEST_TUN="ovstesttun0"
>>>>>>> +
>>>>>>> +
>>>>>>> +def sigint_intercept():
>>>>>>> +    """
>>>>>>> +    Intercept SIGINT from child (the local ovs-test server process).
>>>>>>> +    """
>>>>>>> +    signal.signal(signal.SIGINT, signal.SIG_IGN)
>>>>>>> +
>>>>>>> +
>>>>>>> +def start_local_server(port):
>>>>>>> +    """
>>>>>>> +    This function spawns an ovs-test server that listens on
>>>>>>> specified port
>>>>>>> +    and blocks till the spawned ovs-test server is ready to accept
>>>>>>> XML RPC
>>>>>>> +    connections.
>>>>>>> +    """
>>>>>>> +    p = subprocess.Popen(["/usr/bin/ovs-test", "-s", str(port)],
>>>>>>> +                         stdout=subprocess.PIPE,
>>>>>>> stderr=subprocess.PIPE,
>>>>>>> +                         preexec_fn = sigint_intercept)
>>>>>>>
>>>>>> Neat, didn't ever notice preexec_fn before.  Not sure why it is
>>>>>> ignored, but
>>>>>> I'm sure it's for a good cause
>>>>>>
>>>>>>
>>>>>>> +    fcntl.fcntl( p.stdout.fileno(),fcntl.F_SETFL,
>>>>>>> +        fcntl.fcntl(p.stdout.fileno(), fcntl.F_GETFL) |
>>>>>>> os.O_NONBLOCK)
>>>>>>> +
>>>>>>> +    out = ""
>>>>>>> +    while p.poll() == None:
>>>>>>>
>>>>>> Style nit:  s/ == None/ is None/
>>>>>>
>>>>>>
>>>>>> +        fd = select.select([p.stdout.fileno()], [], [])[0]
>>>>>>> +        if fd:
>>>>>>> +            out += p.stdout.readline()
>>>>>>> +            if out.startswith("Starting RPC server"):
>>>>>>> +                break
>>>>>>> +    if p.poll() != None:
>>>>>>>
>>>>>> s/ != None/ is not None/
>>>>>>
>>>>>>
>>>>>> +        raise RuntimeError("Couldn't start local instance of
>>>>>>> ovs-test server")
>>>>>>> +    return p
>>>>>>> +
>>>>>>> +
>>>>>>> +def get_datagram_sizes(mtu1, mtu2):
>>>>>>> +    """
>>>>>>> +    This function calculates all the "interesting" datagram sizes
>>>>>>> so that
>>>>>>> +    we test both - receive and send side with different packets
>>>>>>> sizes.
>>>>>>> +    """
>>>>>>> +    s1 = set([8, mtu1 - 100, mtu1 - 28, mtu1])
>>>>>>> +    s2 = set([8, mtu2 - 100, mtu2 - 28, mtu2])
>>>>>>> +    l = sorted(list(s1.union(s2)))
>>>>>>> +    return l
>>>>>>>
>>>>>> I'd replace the last two lines with
>>>>>>
>>>>>>   return sorted(s1.union(s2))
>>>>>>
>>>>>> partially to avoid using a variable named 'l' (or is it I or | or !
>>>>>> or 1)?
>>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +
>>>>>>> +def ip_from_cidr(string):
>>>>>>> +    """
>>>>>>> +    This function removes the netmask (if present) from the given
>>>>>>> string and
>>>>>>> +    returns the IP address.
>>>>>>> +    """
>>>>>>> +    token = string.split("/")
>>>>>>> +    return token[0]
>>>>>>> +
>>>>>>>
>>>>>>>  def bandwidth_to_string(bwidth):
>>>>>>> -    """Convert bandwidth from long to string and add units"""
>>>>>>> +    """Convert bandwidth from long to string and add units."""
>>>>>>>     bwidth = bwidth * 8 #  Convert back to bits/second
>>>>>>>     if bwidth >= 10000000:
>>>>>>>         return str(int(bwidth / 1000000)) + "Mbps"
>>>>>>> @@ -38,23 +101,27 @@ def bandwidth_to_string(bwidth):
>>>>>>>  def collect_information(node):
>>>>>>
>>>>>>     """Print information about hosts that will do testing"""
>>>>>>>     print "Node %s:%u " % (node[0], node[1])
>>>>>>> -    server1 = xmlrpclib.Server("http://%s:%u/" % (node[0],
>>>>>>> node[1]))
>>>>>>> -    interface_name = server1.get_interface(node[2])
>>>>>>> -    uname = server1.uname()
>>>>>>> +    server = xmlrpclib.Server("http://%s:%u/" % (node[0], node[1]))
>>>>>>> +    interface_name = server.get_interface(node[0])
>>>>>>>
>>>>>> Was this a bug before, or has the behavior changed ([2] -> [0])?
>>>>>> Based upon the use throughout below, I'm guessing this is just
>>>>>> the fallout of the new argument specification format.
>>>>>>
>>>>>>
>>>>>> +    uname = server.uname()
>>>>>>>     mtu = 1500
>>>>>>>
>>>>>>>     if interface_name == "":
>>>>>>>         print ("Could not find interface that has %s IP address."
>>>>>>> -               "Make sure that you specified correct Test IP." %
>>>>>>> (node[2]))
>>>>>>> +               "Make sure that you specified correct Outer IP." %
>>>>>>> (node[0]))
>>>>>>>     else:
>>>>>>> -        mtu = server1.get_interface_mtu(interface_name)
>>>>>>> -        driver = server1.get_driver(interface_name)
>>>>>>> -        print "Will be using %s(%s) with MTU %u" % (interface_name,
>>>>>>> node[2],
>>>>>>> +        if server.is_ovs_bridge(interface_name):
>>>>>>> +            phys_iface =
>>>>>>> server.get_iface_from_bridge(interface_name)
>>>>>>> +        else:
>>>>>>> +            phys_iface = interface_name
>>>>>>> +
>>>>>>> +        driver = server.get_driver(phys_iface)
>>>>>>> +        print "Will be using %s (%s) with MTU %u" % (phys_iface,
>>>>>>> node[0],
>>>>>>>                                                     mtu)
>>>>>>>         if driver == "":
>>>>>>>             print "Install ethtool on this host to get NIC driver
>>>>>>> information"
>>>>>>>         else:
>>>>>>> -            print "On this host %s has %s." % (interface_name,
>>>>>>> driver)
>>>>>>> +            print "On this host %s has %s." % (phys_iface, driver)
>>>>>>>
>>>>>>>     if uname == "":
>>>>>>>         print "Unable to retrieve kernel information. Is this Linux?"
>>>>>>> @@ -64,7 +131,7 @@ def collect_information(node):
>>>>>>>     return mtu
>>>>>>>
>>>>>>>
>>>>>>> -def do_udp_tests(receiver, sender, tbwidth, duration, sender_mtu):
>>>>>>> +def do_udp_tests(receiver, sender, tbwidth, duration, ps):
>>>>>>>
>>>>>> Could get some free self-documenting action by calling this port_sizes
>>>>>>
>>>>>>
>>>>>>
>>>>>>>     """Schedule UDP tests between receiver and sender"""
>>>>>>>     server1 = xmlrpclib.Server("http://%s:%u/" % (receiver[0],
>>>>>>> receiver[1]))
>>>>>>>     server2 = xmlrpclib.Server("http://%s:%u/" % (sender[0],
>>>>>>> sender[1]))
>>>>>>> @@ -77,7 +144,7 @@ def do_udp_tests(receiver, sender, tbwidth,
>>>>>>> duration, sender_mtu):
>>>>>>>     print udpformat.format("Datagram Size", "Snt Datagrams", "Rcv
>>>>>>> Datagrams",
>>>>>>>                             "Datagram Loss", "Bandwidth")
>>>>>>>
>>>>>>> -    for size in [8, sender_mtu - 100, sender_mtu - 28, sender_mtu]:
>>>>>>> +    for size in ps:
>>>>>>>         listen_handle = -1
>>>>>>>         send_handle = -1
>>>>>>>         try:
>>>>>>> @@ -89,8 +156,9 @@ def do_udp_tests(receiver, sender, tbwidth,
>>>>>>> duration, sender_mtu):
>>>>>>>                         " %u. Try to restart the server.\n" %
>>>>>>> receiver[3])
>>>>>>>                 return
>>>>>>>             send_handle = server2.create_udp_sender(
>>>>>>> -                                            (receiver[2],
>>>>>>> receiver[3]),
>>>>>>> -                                            packetcnt, size,
>>>>>>> duration)
>>>>>>> +
>>>>>>>  (ip_from_cidr(receiver[2]),
>>>>>>> +                                             receiver[3]),
>>>>>>> packetcnt, size,
>>>>>>> +                                             duration)
>>>>>>>
>>>>>>>             #Using sleep here because there is no other
>>>>>>> synchronization source
>>>>>>>             #that would notify us when all sent packets were received
>>>>>>> @@ -131,8 +199,8 @@ def do_tcp_tests(receiver, sender, duration):
>>>>>>>             print ("Server was unable to open TCP listening socket
>>>>>>> on port"
>>>>>>>                     " %u. Try to restart the server.\n" %
>>>>>>> receiver[3])
>>>>>>>             return
>>>>>>> -        send_handle = server2.create_tcp_sender(receiver[2],
>>>>>>> receiver[3],
>>>>>>> -                                                    duration)
>>>>>>> +        send_handle =
>>>>>>> server2.create_tcp_sender(ip_from_cidr(receiver[2]),
>>>>>>> +                                                receiver[3],
>>>>>>> duration)
>>>>>>>
>>>>>>>         time.sleep(duration + 1)
>>>>>>>
>>>>>>> @@ -151,30 +219,168 @@ def do_tcp_tests(receiver, sender, duration):
>>>>>>>     print "\n"
>>>>>>>
>>>>>>>
>>>>>>> +def do_l3_tests(node1, node2, bandwidth, duration, ps, type):
>>>>>>> +    """
>>>>>>> +    Do L3 tunneling tests.
>>>>>>> +    """
>>>>>>> +    server1 = xmlrpclib.Server("http://%s:%u/" % (node1[0],
>>>>>>> node1[1]))
>>>>>>> +    server2 = xmlrpclib.Server("http://%s:%u/" % (node2[0],
>>>>>>> node2[1]))
>>>>>>> +    try:
>>>>>>> +        server1.create_bridge(DEFAULT_TEST_BRIDGE)
>>>>>>> +        server2.create_bridge(DEFAULT_TEST_BRIDGE)
>>>>>>> +
>>>>>>> +        server1.interface_up(DEFAULT_TEST_BRIDGE)
>>>>>>> +        server2.interface_up(DEFAULT_TEST_BRIDGE)
>>>>>>> +
>>>>>>> +        server1.interface_assign_ip(DEFAULT_TEST_BRIDGE, node1[2],
>>>>>>> "")
>>>>>>> +        server2.interface_assign_ip(DEFAULT_TEST_BRIDGE, node2[2],
>>>>>>> "")
>>>>>>> +
>>>>>>> +        server1.add_port_to_bridge(DEFAULT_TEST_BRIDGE,
>>>>>>> DEFAULT_TEST_TUN)
>>>>>>> +        server2.add_port_to_bridge(DEFAULT_TEST_BRIDGE,
>>>>>>> DEFAULT_TEST_TUN)
>>>>>>> +
>>>>>>> +        server1.ovs_vsctl_set("Interface", DEFAULT_TEST_TUN,
>>>>>>> "type", "", type)
>>>>>>> +        server2.ovs_vsctl_set("Interface", DEFAULT_TEST_TUN,
>>>>>>> "type", "", type)
>>>>>>> +        server1.ovs_vsctl_set("Interface", DEFAULT_TEST_TUN,
>>>>>>> "options",
>>>>>>> +                              "remote_ip", node2[0])
>>>>>>> +        server2.ovs_vsctl_set("Interface", DEFAULT_TEST_TUN,
>>>>>>> "options",
>>>>>>> +                              "remote_ip", node1[0])
>>>>>>> +
>>>>>>> +        do_udp_tests(node1, node2, bandwidth, duration, ps)
>>>>>>> +        do_udp_tests(node2, node1, bandwidth, duration, ps)
>>>>>>> +        do_tcp_tests(node1, node2, duration)
>>>>>>> +        do_tcp_tests(node2, node1, duration)
>>>>>>> +
>>>>>>> +    finally:
>>>>>>> +        server1.del_bridge(DEFAULT_TEST_BRIDGE)
>>>>>>> +        server2.del_bridge(DEFAULT_TEST_BRIDGE)
>>>>>>>
>>>>>> Slight nit, you will lose your original error if create bridge fails
>>>>>> I think.  Not sure
>>>>>> what the expectations are, or whether create_bridge can fail in that
>>>>>> way.  I tend
>>>>>> to structure these things like
>>>>>>
>>>>>>   do_stateful_operation()  # Atomic
>>>>>>   try:
>>>>>>       other_things()  # Might fail
>>>>>>   finally:
>>>>>>       undo_stateful_operation()  # Can't fail
>>>>>>
>>>>>
>>>>> Maybe I did not understand your approach as intended, but
>>>>> did you mean something like this:
>>>>>
>>>>>     # Make 1&2 together to look like a single atomic operation.
>>>>>>
>>>>>>     try:
>>>>>>       do_atomic_operation1()
>>>>>>       do_atomic_operation2()
>>>>>>     except:
>>>>>>       undo_atomic_operation1()
>>>>>>       undo_atomic_operation2()
>>>>>>       raise Something  # If something went wrong - better quit early
>>>>>> by raising Something
>>>>>>
>>>>>>     try:
>>>>>>       do_something1()
>>>>>>       do_something2()
>>>>>>     finally:
>>>>>>       undo_atomic_operation1()
>>>>>>       undo_atomic_operation2()
>>>>>>
>>>>>
>>>>>
>>>>> instead of this:
>>>>>
>>>>>     try:
>>>>>>       do_atomic_operation1()
>>>>>>       do_atomic_operation2()
>>>>>>       do_something1()
>>>>>>       do_something2()
>>>>>>     finally:
>>>>>>       undo_atomic_operation1() # This would get cleaned if
>>>>>> do_atomic_operation2() failed
>>>>>>       undo_atomic_operation2()
>>>>>>
>>>>>
>>>>> ?
>>>>>
>>>>
>>>> More or less, probably with helper functions.  I think what I suggest
>>>> below is
>>>> probably too much trouble either way, but I'll make a note anyway.
>>>>
>>>> In this case I am only worried about a bridge being deleted that was
>>>> not created.
>>>> If server.create_bridge is not transactional, you will have funky
>>>> problems, but I
>>>> think it is fine.  So you can just do
>>>>
>>>>    servers_with_bridges = []
>>>>    try:
>>>>      server1.create_bridge(DEFAULT_TEST_BRIDGE)
>>>>      servers_with_bridges.append(server1)
>>>>      create_bridge(server2, DEFAULT_TEST_BRIDGE)
>>>>      servers_with_bridges.append(server2)
>>>>      do_other_stuff()
>>>>    finally:
>>>>      for server in servers_with_bridges:
>>>>       server.del_bridge(DEFAULT_TEST_BRIDGE)
>>>>
>>> Thanks. I somehow prefer this approach over the first one.
>>> Would you mind reviewing the v3 patch?
>>>
>>
>> Has it been sent?  I don't think I see it
>>
> Yes, I sent it out 45 minutes ago. The openvswitch.org does not seem to
> work, so I cant copy+paste the link to the mailing list archive.
> Anyway, v3 patch went under the subject "[PATCHv3] ovs-test: Enhancements
> to the ovs-test tool" and I see it in my Inbox/openvswitch folder. Let me
> know if you still do not see it.
>

Yup, see it now.  Thank you.


>
>
>>
>>
>>>
>>>> or just add replace the del_bridge with some try/except
>>>>
>>>>    def try_del_bridge(server, bridge)
>>>>         try:
>>>>             server.del_bridge(bridge)
>>>>         except Exception, e:
>>>>             print "Some warning about %s" % e
>>>>
>>>> and don't bother with the set
>>>>
>>>>
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> If I can't avoid potential failure in the undo portion with
>>>>>> conditionals, or some
>>>>>> other form of graceful cleanup. I would add a try/except inside the
>>>>>> finally clause,
>>>>>> or add something to note any original exception in passing.
>>>>>>
>>>>>>   try:
>>>>>>     <do stuff>
>>>>>>   except Exception:
>>>>>>      <note original exception>
>>>>>>      raise  # Don't forget this part!
>>>>>>   finally:
>>>>>>      server1.del_bridge(DEFAULT_TEST_BRIDGE)
>>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +
>>>>>>> +def do_vlan_tests(node1, node2, bandwidth, duration, ps, tag):
>>>>>>> +    """
>>>>>>> +    Do VLAN tests between node1 and node2.
>>>>>>> +    """
>>>>>>> +    server1 = xmlrpclib.Server("http://%s:%u/" % (node1[0],
>>>>>>> node1[1]))
>>>>>>> +    server2 = xmlrpclib.Server("http://%s:%u/" % (node2[0],
>>>>>>> node2[1]))
>>>>>>> +
>>>>>>> +    br_name1 = None
>>>>>>> +    br_name2 = None
>>>>>>> +
>>>>>>> +    try:
>>>>>>> +        interface_node1 = server1.get_interface(node1[0])
>>>>>>> +        interface_node2 = server2.get_interface(node2[0])
>>>>>>> +
>>>>>>> +        if server1.is_ovs_bridge(interface_node1):
>>>>>>> +            br_name1 = interface_node1
>>>>>>> +        else:
>>>>>>> +            br_name1 = DEFAULT_TEST_BRIDGE
>>>>>>> +            server1.create_test_bridge(br_name1, interface_node1)
>>>>>>> +
>>>>>>> +        if server2.is_ovs_bridge(interface_node2):
>>>>>>> +            br_name2 = interface_node2
>>>>>>> +        else:
>>>>>>> +            br_name2 = DEFAULT_TEST_BRIDGE
>>>>>>> +            server2.create_test_bridge(br_name2, interface_node2)
>>>>>>> +
>>>>>>> +        server1.add_port_to_bridge(br_name1, DEFAULT_TEST_PORT)
>>>>>>> +        server2.add_port_to_bridge(br_name2, DEFAULT_TEST_PORT)
>>>>>>> +
>>>>>>> +        server1.ovs_vsctl_set("Port", DEFAULT_TEST_PORT, "tag", "",
>>>>>>> tag)
>>>>>>> +        server2.ovs_vsctl_set("Port", DEFAULT_TEST_PORT, "tag", "",
>>>>>>> tag)
>>>>>>> +
>>>>>>> +        server1.ovs_vsctl_set("Interface", DEFAULT_TEST_PORT,
>>>>>>> "type", "",
>>>>>>> +                              "internal")
>>>>>>> +        server2.ovs_vsctl_set("Interface", DEFAULT_TEST_PORT,
>>>>>>> "type", "",
>>>>>>> +                              "internal")
>>>>>>> +
>>>>>>> +        server1.interface_assign_ip(DEFAULT_TEST_PORT, node1[2], "")
>>>>>>> +        server2.interface_assign_ip(DEFAULT_TEST_PORT, node2[2], "")
>>>>>>> +
>>>>>>> +        server1.interface_up(DEFAULT_TEST_PORT)
>>>>>>> +        server2.interface_up(DEFAULT_TEST_PORT)
>>>>>>> +
>>>>>>> +        do_udp_tests(node1, node2, bandwidth, duration, ps)
>>>>>>> +        do_udp_tests(node2, node1, bandwidth, duration, ps)
>>>>>>> +        do_tcp_tests(node1, node2, duration)
>>>>>>> +        do_tcp_tests(node2, node1, duration)
>>>>>>> +
>>>>>>> +    finally:
>>>>>>> +        server1.del_port_from_bridge(DEFAULT_TEST_PORT)
>>>>>>> +        server2.del_port_from_bridge(DEFAULT_TEST_PORT)
>>>>>>> +        if br_name1 == DEFAULT_TEST_BRIDGE:
>>>>>>> +            server1.del_test_bridge(br_name1, interface_node1)
>>>>>>> +        if br_name2 == DEFAULT_TEST_BRIDGE:
>>>>>>> +            server2.del_test_bridge(br_name2, interface_node2)
>>>>>>> +
>>>>>>> +
>>>>>>> +def do_direct_tests(node1, node2, bandwidth, duration, ps):
>>>>>>> +    """
>>>>>>> +    Do tests between outer IPs without involving Open vSwitch
>>>>>>> +    """
>>>>>>> +    n1 = (node1[0], node1[1], node1[0], node1[3])
>>>>>>> +    n2 = (node2[0], node2[1], node2[0], node2[3])
>>>>>>> +
>>>>>>> +    do_udp_tests(n1, n2, bandwidth, duration, ps)
>>>>>>> +    do_udp_tests(n2, n1, bandwidth, duration, ps)
>>>>>>> +    do_tcp_tests(n1, n2, duration)
>>>>>>> +    do_tcp_tests(n2, n1, duration)
>>>>>>> +
>>>>>>> +
>>>>>>>  if __name__ == '__main__':
>>>>>>> +    local_server = None
>>>>>>>     try:
>>>>>>>         ovs_args = args.ovs_initialize_args()
>>>>>>>
>>>>>>> -        if ovs_args.port is not None: #  Start in server mode
>>>>>>> -            print "Starting RPC server"
>>>>>>> -            try:
>>>>>>> -                rpcserver.start_rpc_server(ovs_args.port)
>>>>>>> -            except twisted.internet.error.CannotListenError:
>>>>>>> -                print "Couldn't start XMLRPC server on port %u" %
>>>>>>> ovs_args.port
>>>>>>> +        if ovs_args.port is not None: #  Start in pure server mode
>>>>>>>
>>>>>> Not part of your change, just noticed the 2-before 1-after for #.
>>>>>>
>>>>>>
>>>>>>>  +            rpcserver.start_rpc_server(ovs_args.port)
>>>>>>>
>>>>>>>         elif ovs_args.servers is not None: #  Run in client mode
>>>>>>>             node1 = ovs_args.servers[0]
>>>>>>>             node2 = ovs_args.servers[1]
>>>>>>> -            bandwidth = ovs_args.targetBandwidth
>>>>>>>
>>>>>>> -            mtu_node1 = collect_information(node1)
>>>>>>> +            # Verify whether client will need to spawn a local
>>>>>>> instance of
>>>>>>> +            # ovs-test server by looking at the first OuterIP. if
>>>>>>> it is a
>>>>>>> +            # 127.0.0.1 then spawn local ovs-test server.
>>>>>>> +            if node1[0] == "127.0.0.1":
>>>>>>> +                local_server = start_local_server(node1[1])
>>>>>>> +                #We must determine the IP address that local
>>>>>>> ovs-test server
>>>>>>> +                #will use:
>>>>>>> +                me = xmlrpclib.Server("http://%s:%u/" % (node1[0],
>>>>>>> node1[1]))
>>>>>>> +                my_ip = me.get_my_address_from(node2[0], node2[1])
>>>>>>> +                node1 = (my_ip, node1[1], node1[2], node1[3])
>>>>>>> +
>>>>>>>             mtu_node2 = collect_information(node2)
>>>>>>> +            mtu_node1 = collect_information(node1)
>>>>>>> +
>>>>>>> +            bandwidth = ovs_args.targetBandwidth
>>>>>>> +            interval = ovs_args.testInterval
>>>>>>> +            ps = get_datagram_sizes(mtu_node1, mtu_node2)
>>>>>>> +
>>>>>>> +            direct = ovs_args.direct
>>>>>>> +            vlan_tag = ovs_args.vlanTag
>>>>>>> +            tunnel_modes = ovs_args.tunnelModes
>>>>>>> +
>>>>>>> +            if direct is not None:
>>>>>>> +                print "Performing direct tests"
>>>>>>> +                do_direct_tests(node2, node1, bandwidth, interval,
>>>>>>> ps)
>>>>>>> +
>>>>>>> +            if vlan_tag is not None:
>>>>>>> +                print "Performing VLAN tests"
>>>>>>> +                do_vlan_tests(node2, node1, bandwidth, interval,
>>>>>>> ps, vlan_tag)
>>>>>>> +
>>>>>>> +            for tmode in tunnel_modes:
>>>>>>> +                print "Performing", tmode, "tests"
>>>>>>> +                do_l3_tests(node2, node1, bandwidth, interval, ps,
>>>>>>> tmode)
>>>>>>>
>>>>>>> -            do_udp_tests(node1, node2, bandwidth, 5, mtu_node1)
>>>>>>> -            do_udp_tests(node2, node1, bandwidth, 5, mtu_node2)
>>>>>>> -            do_tcp_tests(node1, node2, 5)
>>>>>>> -            do_tcp_tests(node2, node1, 5)
>>>>>>>     except KeyboardInterrupt:
>>>>>>>         pass
>>>>>>> +    except xmlrpclib.Fault:
>>>>>>> +        print "Couldn't establish XMLRPC control channel"
>>>>>>>     except socket.error:
>>>>>>>         print "Couldn't establish XMLRPC control channel"
>>>>>>> +    except xmlrpclib.ProtocolError:
>>>>>>> +        print "XMLRPC control channel was abruptly terminated"
>>>>>>> +    except twisted.internet.error.CannotListenError:
>>>>>>> +        print "Couldn't start XMLRPC server on port %u" %
>>>>>>> ovs_args.port
>>>>>>> +    finally:
>>>>>>> +        if local_server is not None:
>>>>>>> +            local_server.terminate()
>>>>>>> --
>>>>>>> 1.7.9.1
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> dev mailing list
>>>>>>> dev at openvswitch.org
>>>>>>> http://openvswitch.org/mailman/listinfo/dev
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://openvswitch.org/pipermail/dev/attachments/20120416/0a666501/attachment.htm>


More information about the dev mailing list