On Wed, Sep 19, 2012 at 11:09:39AM -0700, Mehak Mahajan wrote:
> Hey Isaku,
>
> I am not sure I understand this patch completely. Your patch seems to suggest
> that we are not populating the "options" values from the database ? Are you
> suggesting that in case of passive connections we do not
> call ovsdb_jsonrpc_session_set_options() ?
No and no.
I'm suggesting that
- let's set dscp option of listening socket correctly when dscp in
the database is dynamically changed
- let's set options::dscp of accepted socket correctly
ovsdb_jsonrpc_session_set_options() doesn't handle listening socket
because it doesn't have jsonrpc session. But they are managed by
struct ovsdb_jsonrpc_remote.
Are you aware of the issue of ovsdb-client with remote ovsdb?
Do we agree that it should be fixed?
ovs-vsctl has the same issue, but it hides the issue by reconnection.
Comments inlined below.
> On Fri, Sep 14, 2012 at 1:56 AM, Isaku Yamahata <yamah...@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.
>
> 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 <yamah...@valinux.co.jp>
> ---
> There still remains an issue of ovsdb-server and vswitchd. When the
> setting of dscp is dynamically changed, the setting of listening
> sockets isn't updated.
> What should we do with listening socket?
> - close all listening sockets and re-creates them.
> setsockopt(SO_REUSE) would be needed
> - setsockopt(new dscp) for all listening sockets
> This leaves a window where accepted socket may have old dscp.
> To address it,
> - ignore such window or
> - setsockopt(accepted socket, new dscp)
> - other way?
> ---
> lib/jsonrpc.c | 4 +-
> lib/jsonrpc.h | 3 +-
> ovsdb/jsonrpc-server.c | 7 +++++-
> ovsdb/ovsdb-server.c | 1 -
> tests/ovsdb-server.at | 49
> ++++++++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 59 insertions(+), 5 deletions(-)
>
> diff --git a/lib/jsonrpc.c b/lib/jsonrpc.c
> index 0e1788c..4ada023 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;
>
> 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 bb887d0..f6504e1 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(
> @@ -137,6 +138,7 @@ ovsdb_jsonrpc_default_options(const char *target)
> options->probe_interval = (stream_or_pstream_needs_probes(target)
> ? RECONNECT_DEFAULT_PROBE_INTERVAL
> : 0);
> + options->dscp = DSCP_DEFAULT;
> return options;
> }
>
> @@ -192,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);
>
>
>
> Why are you adding this here ? ovsdb_jsonrpc_session_set_all_options() (called
> unconditionally after ovsdb_jsonrpc_server_add_remote() ) already reads the
> values from options and populates in the remote.
This parameter will be used for jsonrpc_session_open_unreliably() in
ovsdb_jsonrpc_server_run().
If something like pstream_get_dscp() is available, the value can be
retrieved.
I hesitated to adding new method to pstream, I choice to keep it here.
If adding pstream_get_dscp() is acceptable, remote::dscp member can
be eliminated.
> if (!listener) {
> @@ -268,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",
> @@ -504,6 +508,7 @@ ovsdb_jsonrpc_session_set_all_options(
> {
> struct ovsdb_jsonrpc_session *s;
>
> + remote->dscp = options->dscp; /* TODO:XXX */
> LIST_FOR_EACH (s, node, &remote->sessions) {
> ovsdb_jsonrpc_session_set_options(s, options);
> }
>
>
> ovsdb_jsonrpc_session_set_options() should already set the dscp value from
> options.
The function doesn't handle pstream case. So we have to handle
pstream case.
> diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
> index 1bf10d9..68f212f 100644
> --- a/ovsdb/ovsdb-server.c
> +++ b/ovsdb/ovsdb-server.c
> @@ -469,7 +469,6 @@ add_manager_options(struct shash *remotes, const
> struct
> ovsdb_row *row)
> options->probe_interval = probe_interval;
> }
>
> - options->dscp = DSCP_DEFAULT;
>
>
> I am unsure why you are taking this out ?
I think options::dscp should be initialized by ovsdb_jsonrpc_default_options()
uniformly. And I change it so.
thanks,
> dscp_string = read_map_string_column(row, "other_config", "dscp");
> if (dscp_string) {
> int dscp = atoi(dscp_string);
> 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
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
>
--
yamahata
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
