[ovs-dev] [PATCH 10/15] ovsdb-server: Add new RPC "set_db_change_aware".

[Ben Pfaff]

Thanks for the review and for your valuable corrections!

On Wed, Jan 31, 2018 at 06:24:14PM -0800, Justin Pettit wrote:
> > On Dec 31, 2017, at 9:16 PM, Ben Pfaff <blp at ovn.org> wrote:
> > 
> > +      Traditionally, <code>ovsdb-server</code> disconnects all of its clients
> > +      when a significant configuration change occurs, because this prompts a
> > +      well-written client to reassess what is available from the server when it
> > +      reconnects.  Because this table provides an alternative and more
> > +      efficient way to find out about those changes, OVS 2.9 also introduces
> > +      the <code>set_db_change_aware</code> RPC, documented in
> > +      <code>ovsdb-server</code>(1), to allow clients to suppress this
> > +      disconnection behavior.
> 
> Is that in section 1 or 7 of the ovsdb-server man pages?

7.  Fixed.

> > +      When a database is removed from the server, in addition to
> > +      <code>Database</code> table updates, the server sends <code>cancel</code>
> > +      messages, as described in RFC 7047 section 4.1.4, in reply to outstanding
> > +      transactions for the removed database.  The server also cancels any
> > +      outstanding monitoring initiated by <code>monitor</code> or
> > +      <code>monitor_cond</code> requested on the removed database, sending the
> > +      <code>monitor_canceled</code> RPC described in
> > +      <code>ovsdb-server</code>(5).  Only clients that disable disconnection
> > +      with <code>set_db_change_aware</code> receive these messages.
> 
> I believe this file is section 5 of the ovsdb-server man page.  Should
> it also be section 7?

Yes.  Fixed.

> > @@ -333,17 +331,20 @@ ovsdb_jsonrpc_server_free_remote_status(
> >     free(status->locks_lost);
> > }
> > 
> > -/* Forces all of the JSON-RPC sessions managed by 'svr' to disconnect and
> > - * reconnect. */
> > +/* Makes all of the JSON-RPC sessions managed by 'svr' to disconnect.  (They
> > + * will then generally reconnect.).
> 
> I think you should drop "to".

Done, thanks.

> > @@ -432,6 +433,20 @@ struct ovsdb_jsonrpc_session {
> >     struct ovsdb_session up;
> >     struct ovsdb_jsonrpc_remote *remote;
> > 
> > +    /* RFC 7047 does not contemplate how to alert clients to changes to the set
> > +     * of databases, e.g. databases that are added or removed while the
> > +     * database server is running.  Traditionally, ovsdb-server disconnects all
> > +     * of its clients when this happens; a well-written client will reassess
> > +     * what is available from the server upon reconnection.
> > +     *
> > +     * OVS 2.9 introduces a way for clients to monitor changes to the databases
> > +     * being served, through the Database table in the _Server database that
> > +     * OVSDB adds in this version.  ovsdb-server suppresses the connection
> > +     * close for clients that identify themselves as taking advantage of this
> > +     * mechanism.
> > +     */
> > +    bool db_change_aware;
> 
> I think it might be worth being explicit that setting this flag to
> true indicates that the client has requested this suppression to
> occur.

Thanks, I added a sentence.

> > +/* Database 'db' is about to be removed from the database server.  To prepare,
> > + * this function removes all references to 'db' from session 's'. */
> > +static void
> > +ovsdb_jsonrpc_session_preremove_db(struct ovsdb_jsonrpc_remote *remote,
> > +                                   struct ovsdb *db)
> 
> The argument looks to be 'db', not 's', which may contain more than
> one session.

Thanks, fixed.

> > +/* Makes all of the JSON-RPC sessions managed by 'remove' to disconnect.  (They
> > + * will then generally reconnect.).
> > + *
> > + * If 'force' is true, disconnects all sessions.  Otherwise, disconnects only
> > + * sesions that aren't database change aware. */
> > static void
> > +ovsdb_jsonrpc_session_reconnect_all(struct ovsdb_jsonrpc_remote *remote,
> > +                                    bool force)
> 
> The comment should be 'remote' instead of 'remove'.

Thanks, fixed.

> Acked-by: Justin Pettit <jpettit at ovn.org>

Thanks for the review!