[ovs-dev] [PATCH] Add Passive TCP connection to IDL

Ofer Ben-Yacov ofer.benyacov at gmail.com
Sun Dec 20 13:12:32 UTC 2015


Hi Russel,

to answer your questions:
1. limitation: the use case for passive mode is that I'm integrating IDL to
L2GW project and passive mode is prerequisite. The change that will enable
usage of the same IP and PORT (full blown server) will require much more
code changes, probably to Neutron also ( Connection object). I thought it
would be better to to it in 2 phases as this phase is good enough to start
with and without it I will not be able to add the IDL to the L2GW. The only
limitation is that if you have more than one OVSDB server, you will need to
use different port for each server.

2. Testing: I run system tests to test the code before I sent the patch. I
will take a look at the tests and will try to add some unit tests to make
sure that future development will not break it.

3. One email address per line: gotcha.

4. PEP8 check with flake8: I fixed the minor changes you've sent and also
fixed other issues flake8 has found on the files I modified (including one
small bug: idl.py line 1224).

5. IPv6: currently, there is no support for IPv6 in the existing code. The
new code I added does not add other complexity to prevent or delay such
support.


I will submit the new code with the modifications again.

Ofer.


On Thu, Dec 17, 2015 at 11:48 PM, Russell Bryant <russell at ovn.org> wrote:

> On 12/17/2015 04:37 AM, Ofer Ben Yacov wrote:
> > From: Ofer Ben-Yacov <ofer.benyacov at 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.d-m at hpe.com>,
> >       "Kamat, Maruti Haridas" <maruti.kamat at hpe.com>,
> >       "Sukhdev Kapur" <sukhdev at arista.com>,
> >       "Migliaccio, Armando" <armando.migliaccio at 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
>



More information about the dev mailing list