[ovs-dev] [PATCHv2] ovs-test: Enhancements to the ovs-test tool
Ansis Atteka
aatteka at nicira.com
Sat Apr 14 02:26:45 UTC 2012
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.
>
>
>> + 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()
>
?
>
> 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://mail.openvswitch.org/pipermail/ovs-dev/attachments/20120413/c841f9f2/attachment-0003.html>
More information about the dev
mailing list