On 12/17/2015 04:37 AM, Ofer Ben Yacov wrote:
> From: Ofer Ben-Yacov <ofer.benya...@gmail.com>
>
> Currently the IDL does not support passive TCP connection,
> i.e. when the OVSDB connects to its manager.
>
> This patch enables IDL to use an already-open session
> (the one which was previously used for retrieving the db schema).
> In addition, it enables IDL to go back to "listen mode" in case the connection
> is lost.
Thanks for the patch. I was able to apply this version. As discussed
off-list, I had problems applying previous submissions of this patch.
I'll start looking this over.
>
> LIMITATIONS:
> ----------------------
>
> This patch enables a **SINGLE** TCP connection from an OVSDB server to an
> agent that uses IDL with {IP,PORT} pair. Therefore, the agent will support
> only **ONE** OVSDB server using {IP,PORT} pair.
>
> Future development may add multi-session server capability that will allow
> an agent to use single {IP,PORT} pair to connect to multiple OVSDB servers.
I'm curious, what's your use case for this? If your use case is only
connecting to a single ovsdb server, what's the advantage of this mode?
> TESTING:
> ---------------
>
> To be able to test passive TCP to the OVSDB, a helper program was used.
> The program flow is:
> 1. Open TCP connection to OVSDB
> 2. Open TCP connection to the agent application that uses IDL
> 3. Forward data between connections (Proxy)
> 4. Simulate a connection problem by stopping and restarting the program
Would you be willing to add a test case for this? The ovs Python
library actually has pretty good test coverage when running "make check"
for ovs.
Take a look at the files in the tests/ directory. In particular, see
test-ovsdb.py and the *.at files that call it such as ovsdb-idl.at. How
about adding a test case that exercises this? That would also help make
sure I don't break it in the Python 3 port I'm doing.
> Requested-by: Ben Pfaff <blp at nicira.com>, "D M, Vikas" <vikas....@hpe.com>,
> "Kamat, Maruti Haridas" <maruti.ka...@hpe.com>,
> "Sukhdev Kapur" <sukh...@arista.com>,
> "Migliaccio, Armando" <armando.migliac...@hpe.com>
This should be one email address per line.
Some more fairly minor comments inline.
>
> ---
> python/ovs/db/idl.py | 18 +++++++++++++++---
> python/ovs/jsonrpc.py | 19 +++++++++++--------
> python/ovs/stream.py | 47 +++++++++++++++++++++++++++++++----------------
> 3 files changed, 57 insertions(+), 27 deletions(-)
>
> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
> index c8990c7..4b492fe 100644
> --- a/python/ovs/db/idl.py
> +++ b/python/ovs/db/idl.py
> @@ -83,7 +83,7 @@ class Idl(object):
> currently being constructed, if there is one, or None otherwise.
> """
>
> - def __init__(self, remote, schema):
> + def __init__(self, remote, schema, session = None):
Please remove the spaces around the '='. (PEP8)
Try running flake8 against the code before and after your patch to make
sure you don't introduce any new issues. I have a separate patch series
cleaning up most of it.
> """Creates and returns a connection to the database named 'db_name'
> on
> 'remote', which should be in a form acceptable to
> ovs.jsonrpc.session.open(). The connection will maintain an
> in-memory
> @@ -101,7 +101,16 @@ class Idl(object):
> As a convenience to users, 'schema' may also be an instance of the
> SchemaHelper class.
>
> - The IDL uses and modifies 'schema' directly."""
> + The IDL uses and modifies 'schema' directly.
> +
> + In passive mode ( where the OVSDB connects to its manager ),
> + we first need to wait for the OVSDB to connect and then
> + pass the 'session' object (while the it is still open ) and
> + the schema we retrieved from the open session to the IDL to use it.
> +
> + If in active mode, do not pass 'session' and it will be created
> + by IDL by using 'remote'.
> + """
>
> assert isinstance(schema, SchemaHelper)
> schema = schema.get_idl_schema()
> @@ -109,7 +118,10 @@ class Idl(object):
> self.tables = schema.tables
> self.readonly = schema.readonly
> self._db = schema
> - self._session = ovs.jsonrpc.Session.open(remote)
> + if session:
> + self._session = session
> + else:
> + self._session = ovs.jsonrpc.Session.open(remote)
> self._monitor_request_id = None
> self._last_seqno = None
> self.change_seqno = 0
> diff --git a/python/ovs/jsonrpc.py b/python/ovs/jsonrpc.py
> index d54d74b..1b68d3f 100644
> --- a/python/ovs/jsonrpc.py
> +++ b/python/ovs/jsonrpc.py
> @@ -418,23 +418,25 @@ class Session(object):
> self.__disconnect()
>
> name = self.reconnect.get_name()
> - if not self.reconnect.is_passive():
> - error, self.stream = ovs.stream.Stream.open(name)
> + if self.reconnect.is_passive():
> + if self.pstream is not None:
> + self.pstream.close()
> + error, self.pstream = ovs.stream.PassiveStream.open(name)
> if not error:
> - self.reconnect.connecting(ovs.timeval.msec())
> + self.reconnect.listening(ovs.timeval.msec())
> else:
> self.reconnect.connect_failed(ovs.timeval.msec(), error)
> - elif self.pstream is not None:
> - error, self.pstream = ovs.stream.PassiveStream.open(name)
> + else:
> + error, self.stream = ovs.stream.Stream.open(name)
> if not error:
> - self.reconnect.listening(ovs.timeval.msec())
> + self.reconnect.connecting(ovs.timeval.msec())
> else:
> self.reconnect.connect_failed(ovs.timeval.msec(), error)
>
> self.seqno += 1
>
> def run(self):
> - if self.pstream is not None:
> + if self.pstream is not None and self.stream is None:
> error, stream = self.pstream.accept()
> if error == 0:
> if self.rpc or self.stream:
> @@ -444,11 +446,11 @@ class Session(object):
> self.__disconnect()
> self.reconnect.connected(ovs.timeval.msec())
> self.rpc = Connection(stream)
> + self.stream = stream
> elif error != errno.EAGAIN:
> self.reconnect.listen_error(ovs.timeval.msec(), error)
> self.pstream.close()
> self.pstream = None
> -
> if self.rpc:
> backlog = self.rpc.get_backlog()
> self.rpc.run()
> @@ -559,3 +561,4 @@ class Session(object):
>
> def force_reconnect(self):
> self.reconnect.force_reconnect(ovs.timeval.msec())
> +
This looks like some unintended added whitespace. I get a warning about
this patch introducing a whitespace error when I apply it.
> diff --git a/python/ovs/stream.py b/python/ovs/stream.py
> index fb083ee..a8be6e0 100644
> --- a/python/ovs/stream.py
> +++ b/python/ovs/stream.py
> @@ -15,7 +15,6 @@
> import errno
> import os
> import socket
> -
Please drop this line removal.
> import ovs.poller
> import ovs.socket_util
> import ovs.vlog
> @@ -261,11 +260,11 @@ class PassiveStream(object):
> @staticmethod
> def is_valid_name(name):
> """Returns True if 'name' is a passive stream name in the form
> - "TYPE:ARGS" and TYPE is a supported passive stream type (currently
> only
> - "punix:"), otherwise False."""
> - return name.startswith("punix:")
> + "TYPE:ARGS" and TYPE is a supported passive stream type,
> + otherwise False."""
> + return name.startswith("punix:") or name.startswith("ptcp:")
>
> - def __init__(self, sock, name, bind_path):
> + def __init__(self, sock, name, bind_path=None):
> self.name = name
> self.socket = sock
> self.bind_path = bind_path
> @@ -274,22 +273,31 @@ class PassiveStream(object):
> def open(name):
> """Attempts to start listening for remote stream connections. 'name'
> is a connection name in the form "TYPE:ARGS", where TYPE is an
> passive
> - stream class's name and ARGS are stream class-specific. Currently
> the
> - only supported TYPE is "punix".
> + stream class's name and ARGS are stream class-specific.
>
> Returns (error, pstream): on success 'error' is 0 and 'pstream' is
> the
> new PassiveStream, on failure 'error' is a positive errno value and
> 'pstream' is None."""
> if not PassiveStream.is_valid_name(name):
> return errno.EAFNOSUPPORT, None
> -
> - bind_path = name[6:]
> + bind_path = None
> if name.startswith("punix:"):
> + bind_path = name[6:]
> bind_path = ovs.util.abs_file_name(ovs.dirs.RUNDIR, bind_path)
> - error, sock = ovs.socket_util.make_unix_socket(socket.SOCK_STREAM,
> - True, bind_path, None)
> - if error:
> - return error, None
> + error, sock =
> ovs.socket_util.make_unix_socket(socket.SOCK_STREAM,
> + True, bind_path,
> + None)
> + if error:
> + return error, None
> +
> + elif name.startswith("ptcp:"):
> + sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
> + sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
> + remote = name.split(':')
> + sock.bind((remote[1], int(remote[2])))
How about IPv6?
I'm not sure if the existing code supports it though. If so, it'd be
nice to include it here.
> +
> + else:
> + raise Exception('Unknown connection string')
>
> try:
> sock.listen(10)
> @@ -320,7 +328,10 @@ class PassiveStream(object):
> try:
> sock, addr = self.socket.accept()
> ovs.socket_util.set_nonblocking(sock)
> - return 0, Stream(sock, "unix:%s" % addr, 0)
> + if (sock.family == socket.AF_UNIX):
> + return 0, Stream(sock, "unix:%s" % addr, 0)
> + return 0, Stream(sock, 'ptcp:' + addr[0] + ':' +
> str(addr[1]),
> + 0)
> except socket.error, e:
> error = ovs.socket_util.get_exception_errno(e)
> if error != errno.EAGAIN:
> @@ -350,8 +361,10 @@ class UnixStream(Stream):
> @staticmethod
> def _open(suffix, dscp):
> connect_path = suffix
> - return ovs.socket_util.make_unix_socket(socket.SOCK_STREAM,
> - True, None, connect_path)
> + return ovs.socket_util.make_unix_socket(socket.SOCK_STREAM,
> + True, None, connect_path)
> +
> +
> Stream.register_method("unix", UnixStream)
>
>
> @@ -363,4 +376,6 @@ class TCPStream(Stream):
> if not error:
> sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1)
> return error, sock
> +
> +
> Stream.register_method("tcp", TCPStream)
>
--
Russell Bryant
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
