[ovs-dev] [PATCH 11/15] ovsdb: Add support for online schema conversion.

[Ben Pfaff]

On Mon, Feb 05, 2018 at 02:27:51PM -0800, Justin Pettit wrote:
> 
> > On Dec 31, 2017, at 9:16 PM, Ben Pfaff <blp at ovn.org> wrote:
> > 
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -5,6 +5,8 @@ Post-v2.8.0
> >      * New high-level documentation in ovsdb(7).
> >      * New file format documentation for developers in ovsdb(5).
> >      * Protocol documentation moved from ovsdb-server(1) to ovsdb-server(7).
> > +     * ovsdb-server now supports online schema conversion via
> > +       "ovsdb-client convert".
> 
> Do you think it's worth mentioning the "needs-conversion" and "convert" commands?

I don't think "needs-conversion" really needs to stand alone here.  When
a user investigates "convert", (s)he will find out about
"needs-conversion".

I believe I mentioned "convert".

> > diff --git a/ovsdb/file.c b/ovsdb/file.c
> > index 4aafb3be8ab4..dadb988d3088 100644
> > --- a/ovsdb/file.c
> > +++ b/ovsdb/file.c
> > @@ -566,22 +566,31 @@ ovsdb_file_txn_annotate(struct json *json, const char *comment)
> >     return json;
> > }
> > 
> > -struct ovsdb_error *
> > -ovsdb_file_commit(struct ovsdb_file *file,
> > -                  const struct ovsdb_txn *txn, bool durable)
> > +/* Returns 'txn' transformed into the JSON format that is used in OVSDB files.
> > + * (But the caller must use ovsdb_file_txn_annotate() to add the _comment the
> > + * _date members.)  If 'txn' doesn't actually change anything, returns NULL */
> 
> I think that should be "and _date members".

Thanks, fixed.

> > diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
> > index 3e58c3fbd274..97706932614c 100644
> > --- a/ovsdb/monitor.c
> > +++ b/ovsdb/monitor.c
> > ...
> > +void
> > +ovsdb_monitor_prereplace_db(struct ovsdb *db)
> > +{
> > +    struct ovsdb_monitor *m, *next_m;
> > +
> > +    LIST_FOR_EACH_SAFE (m, next_m, list_node, &db->monitors) {
> > +        struct jsonrpc_monitor_node *jm, *next_jm;
> > +
> > +        /* Delete all front end monitors. Removing the last front
> > +         * end monitor will also destroy the corresponding 'ovsdb_monitor'.
> > +         * ovsdb monitor will also be destroied.  */
> 
> I found the last sentence confusing in relation with the sentence
> before it.  If that sentence should stay, it should be "destroyed".

Thanks, fixed.

> > diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c
> > index 600c5070db78..a7cab600c98b 100644
> > --- a/ovsdb/ovsdb-client.c
> > +++ b/ovsdb/ovsdb-client.c
> > ...
> > @@ -303,6 +308,8 @@ usage(void)
> >            "    DATABASE on SERVER.\n"
> >            "    COLUMNs may include !initial, !insert, !delete, !modify\n"
> >            "    to avoid seeing the specified kinds of changes.\n"
> > +           "\n  convert [SERVER] SCHEMA\n"
> > +           "    convert database on SERVER named in SCHEMA to SCHEMA.\n"
> 
> The man page discusses "need-conversion", but it's not mentioned in
> the usage.  Is this intentional?

No.  I added a mention.

> > +static void
> > +do_convert(struct jsonrpc *rpc, const char *database OVS_UNUSED,
> > +           int argc OVS_UNUSED, char *argv[])
> > +{
> > +    struct ovsdb_schema *new_schema;
> > +    check_ovsdb_error(ovsdb_schema_from_file(argv[0], &new_schema));
> > +
> > +    struct jsonrpc_msg *request, *reply;
> > +    request = jsonrpc_create_request(
> > +        "convert",
> > +        json_array_create_2(json_string_create(new_schema->name),
> > +                            ovsdb_schema_to_json(new_schema)), NULL);
> 
> I"m probably misunderstanding something, but I thought the only
> parameter to "convert" was "<database-schema>" based on the
> documentation in "ovsdb-server.7.rst", but this seems to be an array
> that consists of the name and the schema.  My reading of the RFC is
> that "<ovsdb-schema>" only contains the schema.

It looks like that was my original design but that I changed it later
without updating the RFC.  I've now updated the RFC.

> > diff --git a/ovsdb/trigger.h b/ovsdb/trigger.h
> > index 90246a4a42bd..d9df97f31222 100644
> > --- a/ovsdb/trigger.h
> > +++ b/ovsdb/trigger.h
> > @@ -25,8 +25,8 @@ struct ovsdb_trigger {
> >     struct ovsdb *db;           /* Database on which trigger acts. */
> >     struct ovs_list node;       /* !result: in db->triggers;
> >                                  * result: in session->completions. */
> > -    struct json *request;       /* Database request. */
> > -    struct json *result;        /* Result (null if none yet). */
> > +    struct jsonrpc_msg *request; /* Database request. */
> > +    struct jsonrpc_msg *reply;   /* Result (null if none yet).. */
> 
> There appears to be an extra period at the end of this comment.

Thanks, removed.

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

Thanks!