[ovs-dev] [PATCH v2 5/5] ovsdb/jsonrpc-server: ovsdb-server closes accepted connections immediately

Isaku Yamahata yamahata at valinux.co.jp
Fri Oct 5 07:07:06 UTC 2012


Hi Mehak. Thank you for taking care of this.
I'm afraid that you missed v3 patch.
Although the changes are minor, please pick v3 patches.

http://openvswitch.org/pipermail/dev/2012-September/021475.html
http://openvswitch.org/pipermail/dev/2012-September/021476.html
http://openvswitch.org/pipermail/dev/2012-September/021477.html

thanks,

On Thu, Oct 04, 2012 at 11:51:55PM -0700, Mehak Mahajan wrote:
> Hey Isaku,
> 
> Thanks for the patch.
> I have applied it to master.
> 
> thanx!
> mehak
> 
> On Wed, Sep 26, 2012 at 12:12 AM, Isaku Yamahata <yamahata at valinux.co.jp>
> wrote:
> 
>     Thus ovsdb-client aborts as follows.
> 
>     > # ovs-vsctl set-manager ptcp:6634
>     > # ovsdb-client get-schema tcp:127.0.0.1:6634
>     > 2012-09-14T05:38:26Z|00001|jsonrpc|WARN|tcp:127.0.0.1:6634: receive
>     error: Connection reset by peer
>     > ovsdb-client: transaction failed (Connection reset by peer)
>     NOTE: This occurs intermittently depending on how ovsdb-server runs.
>           Running ovsdb-client on a remote machine increases the possibility.
> 
>     This is because ovsdb-server closes newly accepted tcp connection.
>     The following changesets caused it. struct jsonrpc_session::dscp isn't set
>     based on listening socket's dscp value.
> 
>     - ovsdb-server creates passive connection and listens on it.
>     - ovsdb-server accepts connection by ovsdb_jsonrpc_server_run().
>       The accepted socket inherits from the listening sockets.
>       ovsdb_jsonrpc_server_run() creates json session, but leaves dscp of
>       struct jsonrpc_session zero.
>     - On calling reconfigure_from_db(), it resets dscp value to
>       all jsonrpc sessions. Eventually jsonrpc_session_set_dscp() is called.
>       Then jsonrpc_session_force_reconnect() closes the connection.
> 
>     With this patch,
>     - struct jsonrpc_session::dscp is correctly set based on
>       listening sockets dscp value.
>     - dscp of listening socket is changed dynamically by setsockopt.
>       This leaves a window where accepted socket may have old dscp.
>       But it is ignored for now because it would complicates codes
>       too much.
> 
>     The related change sets:
>     - 0442efd9b1a88d923b56eab6b72b6be8231a49f7
>       Reapplying the dscp changes: No need to restart DB/OVS on changing
>       dscp value.
>     - 59efa47adf3234ec51541405726d033173851285
>       Revert DSCP update changes.
>     - b2e18db292cd4962af3248f11e9f17e6eaf9c033
>       No need to restart DB / OVS on changing dscp value.
>     - f125905cdd3dc0339ad968c0a70128807884b400
>       Allow configuring DSCP on controller and manager connections.   5
> 
>     Signed-off-by: Isaku Yamahata <yamahata at valinux.co.jp>
>     ---
>     Changes v1 -> v2:
>     - addressed listening socket
>     ---
>      lib/jsonrpc.c          |   17 ++++++++++++++-
>      lib/jsonrpc.h          |    3 +-
>      ovsdb/jsonrpc-server.c |   20 ++++++++++++++++++-
>      tests/ovsdb-server.at  |   49
>     ++++++++++++++++++++++++++++++++++++++++++++++++
>      4 files changed, 85 insertions(+), 4 deletions(-)
> 
>     diff --git a/lib/jsonrpc.c b/lib/jsonrpc.c
>     index 0e1788c..d6e11c3 100644
>     --- a/lib/jsonrpc.c
>     +++ b/lib/jsonrpc.c
>     @@ -791,7 +791,7 @@ jsonrpc_session_open(const char *name)
>       * On the assumption that such connections are likely to be short-lived
>       * (e.g. from ovs-vsctl), informational logging for them is suppressed. */
>      struct jsonrpc_session *
>     -jsonrpc_session_open_unreliably(struct jsonrpc *jsonrpc)
>     +jsonrpc_session_open_unreliably(struct jsonrpc *jsonrpc, uint8_t dscp)
>      {
>          struct jsonrpc_session *s;
> 
>     @@ -801,7 +801,7 @@ jsonrpc_session_open_unreliably(struct jsonrpc
>     *jsonrpc)
>          reconnect_set_name(s->reconnect, jsonrpc_get_name(jsonrpc));
>          reconnect_set_max_tries(s->reconnect, 0);
>          reconnect_connected(s->reconnect, time_msec());
>     -    s->dscp = 0;
>     +    s->dscp = dscp;
>          s->rpc = jsonrpc;
>          s->stream = NULL;
>          s->pstream = NULL;
>     @@ -1093,6 +1093,19 @@ jsonrpc_session_set_dscp(struct jsonrpc_session *s,
>                               uint8_t dscp)
>      {
>          if (s->dscp != dscp) {
>     +        if (s->pstream) {
>     +            int error;
>     +            error = pstream_set_dscp(s->pstream, dscp);
>     +            if (error) {
>     +                VLOG_ERR("%s: failed set_dscp %s",
>     +                         reconnect_get_name(s->reconnect), strerror
>     (error));
>     +            }
>     +            /*
>     +             * TODO:XXX race window between setting dscp to listening
>     socket
>     +             * and accepting socket. accepted socket may have old dscp
>     value.
>     +             * Ignore this race window for now.
>     +             */
>     +        }
>              s->dscp = dscp;
>              jsonrpc_session_force_reconnect(s);
>          }
>     diff --git a/lib/jsonrpc.h b/lib/jsonrpc.h
>     index 44ae06f..b5acf89 100644
>     --- a/lib/jsonrpc.h
>     +++ b/lib/jsonrpc.h
>     @@ -99,7 +99,8 @@ struct json *jsonrpc_msg_to_json(struct jsonrpc_msg *);
>      /* A JSON-RPC session with reconnection. */
> 
>      struct jsonrpc_session *jsonrpc_session_open(const char *name);
>     -struct jsonrpc_session *jsonrpc_session_open_unreliably(struct jsonrpc *);
>     +struct jsonrpc_session *jsonrpc_session_open_unreliably(struct jsonrpc *,
>     +                                                        uint8_t);
>      void jsonrpc_session_close(struct jsonrpc_session *);
> 
>      void jsonrpc_session_run(struct jsonrpc_session *);
>     diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
>     index bc0c89e..6a67f2e 100644
>     --- a/ovsdb/jsonrpc-server.c
>     +++ b/ovsdb/jsonrpc-server.c
>     @@ -98,6 +98,7 @@ struct ovsdb_jsonrpc_remote {
>          struct ovsdb_jsonrpc_server *server;
>          struct pstream *listener;   /* Listener, if passive. */
>          struct list sessions;       /* List of "struct
>     ovsdb_jsonrpc_session"s. */
>     +    uint8_t dscp;
>      };
> 
>      static struct ovsdb_jsonrpc_remote *ovsdb_jsonrpc_server_add_remote(
>     @@ -193,6 +194,7 @@ ovsdb_jsonrpc_server_add_remote(struct
>     ovsdb_jsonrpc_server *svr,
>          remote->server = svr;
>          remote->listener = listener;
>          list_init(&remote->sessions);
>     +    remote->dscp = options->dscp;
>          shash_add(&svr->remotes, name, remote);
> 
>          if (!listener) {
>     @@ -269,7 +271,8 @@ ovsdb_jsonrpc_server_run(struct ovsdb_jsonrpc_server
>     *svr)
>                  error = pstream_accept(remote->listener, &stream);
>                  if (!error) {
>                      struct jsonrpc_session *js;
>     -                js = jsonrpc_session_open_unreliably(jsonrpc_open
>     (stream));
>     +                js = jsonrpc_session_open_unreliably(jsonrpc_open(stream),
>     +                                                     remote->dscp);
>                      ovsdb_jsonrpc_session_create(remote, js);
>                  } else if (error != EAGAIN) {
>                      VLOG_WARN_RL(&rl, "%s: accept failed: %s",
>     @@ -505,6 +508,21 @@ ovsdb_jsonrpc_session_set_all_options(
>      {
>          struct ovsdb_jsonrpc_session *s;
> 
>     +    if (remote->listener) {
>     +        int error;
>     +        error = pstream_set_dscp(remote->listener, options->dscp);
>     +        if (error) {
>     +            VLOG_ERR("%s: set_dscp failed %s",
>     +                     pstream_get_name(remote->listener), strerror(error));
>     +        } else {
>     +            remote->dscp = options->dscp;
>     +        }
>     +        /*
>     +         * TODO:XXX race window between setting dscp to listening socket
>     +         * and accepting socket. Accepted socket may have old dscp value.
>     +         * Ignore this race window for now.
>     +         */
>     +    }
>          LIST_FOR_EACH (s, node, &remote->sessions) {
>              ovsdb_jsonrpc_session_set_options(s, options);
>          }
>     diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at
>     index b0a3377..87046f6 100644
>     --- a/tests/ovsdb-server.at
>     +++ b/tests/ovsdb-server.at
>     @@ -415,6 +415,55 @@ cat stdout >> output
> 
>      EXECUTION_EXAMPLES
> 
>     +AT_BANNER([OVSDB -- ovsdb-server transactions (TCP sockets)])
>     +
>     +AT_SETUP([ovsdb-client get-schema-version - tcp socket])
>     +AT_KEYWORDS([ovsdb server positive tcp])
>     +ordinal_schema > schema
>     +AT_CHECK([perl $srcdir/choose-port.pl], [0], [stdout])
>     +TCP_PORT=`cat stdout`
>     +AT_CHECK([ovsdb-tool create db schema], [0], [ignore], [ignore])
>     +AT_CHECK([ovsdb-server --detach --no-chdir --pidfile="`pwd`"/pid --unixctl
>     ="`pwd`"/unixctl --remote=ptcp:$TCP_PORT:127.0.0.1 db], [0], [ignore],
>     [ignore])
>     +AT_CHECK([ovsdb-client get-schema-version tcp:127.0.0.1:$TCP_PORT
>     ordinals], [0], [5.1.3
>     +])
>     +OVSDB_SERVER_SHUTDOWN
>     +AT_CLEANUP])
>     +
>     +# OVSDB_CHECK_EXECUTION(TITLE, SCHEMA, TRANSACTIONS, OUTPUT, [KEYWORDS])
>     +#
>     +# Creates a database with the given SCHEMA, starts an ovsdb-server on
>     +# that database, and runs each of the TRANSACTIONS (which should be a
>     +# quoted list of quoted strings) against it with ovsdb-client one at a
>     +# time.
>     +#
>     +# Checks that the overall output is OUTPUT, but UUIDs in the output
>     +# are replaced by markers of the form <N> where N is a number.  The
>     +# first unique UUID is replaced by <0>, the next by <1>, and so on.
>     +# If a given UUID appears more than once it is always replaced by the
>     +# same marker.
>     +#
>     +# TITLE is provided to AT_SETUP and KEYWORDS to AT_KEYWORDS.
>     +m4_define([OVSDB_CHECK_EXECUTION],
>     +  [AT_SETUP([$1])
>     +   AT_KEYWORDS([ovsdb server positive tcp $5])
>     +   $2 > schema
>     +   AT_CHECK([perl $srcdir/choose-port.pl], [0], [stdout])
>     +   TCP_PORT=`cat stdout`
>     +   PKIDIR=$abs_top_builddir/tests
>     +   AT_CHECK([ovsdb-tool create db schema], [0], [stdout], [ignore])
>     +   AT_CHECK([ovsdb-server --detach --no-chdir --pidfile="`pwd`"/pid
>     --remote=ptcp:$TCP_PORT:127.0.0.1 --unixctl="`pwd`"/unixctl db], [0],
>     [ignore], [ignore])
>     +   m4_foreach([txn], [$3],
>     +     [AT_CHECK([ovsdb-client transact tcp:127.0.0.1:$TCP_PORT 'txn'], [0],
>     [stdout], [ignore],
>     +     [test ! -e pid || kill `cat pid`])
>     +cat stdout >> output
>     +])
>     +   AT_CHECK([perl $srcdir/uuidfilt.pl output], [0], [$4], [ignore],
>     +            [test ! -e pid || kill `cat pid`])
>     +   OVSDB_SERVER_SHUTDOWN
>     +   AT_CLEANUP])
>     +
>     +EXECUTION_EXAMPLES
>     +
>      AT_BANNER([OVSDB -- transactions on transient ovsdb-server])
> 
>      # OVSDB_CHECK_EXECUTION(TITLE, SCHEMA, TRANSACTIONS, OUTPUT, [KEYWORDS])
>     --
>     1.7.1.1
> 
> 
> 

-- 
yamahata



More information about the dev mailing list