Hey Isaku,
I apologize. I seem to have misunderstood the intent earlier.
The patch looks good to me.
Regarding the issue of ovsdb-server and vswitchd, I am more inclined
towards your second option
"setsockopt(new dscp) for all listening sockets". As for the already
accepted connections, we can ignore them for now and address them later,
should the need arise. Does that sound reasonable ?
May you make the changes for the ovsdb-server and vswitchd as well and
resubmit the patch ?
thanx!
mehak
On Wed, Sep 19, 2012 at 6:53 PM, Isaku Yamahata <yamah...@valinux.co.jp>wrote:
> 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
