[ovs-dev,v2] ovsdb-tool: Convert clustered db to standalone db.
diff mbox series

Message ID 20190823005310.99329-1-amginwal@gmail.com
State New
Headers show
Series
  • [ovs-dev,v2] ovsdb-tool: Convert clustered db to standalone db.
Related show

Commit Message

aginwala aginwala Aug. 23, 2019, 12:53 a.m. UTC
From: Aliasgar Ginwala <aginwala@ebay.com>

Add support in ovsdb-tool for migrating clustered dbs to standalone dbs.
E.g. usage to migrate nb/sb db to standalone db from raft:
ovsdb-tool migrate-cluster-db ovnnb_db.db ovnnb_db_cluster.db

Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com>
---
 ovsdb/ovsdb-tool.c | 154 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 152 insertions(+), 2 deletions(-)

Comments

Han Zhou Aug. 23, 2019, 2:27 a.m. UTC | #1
Ali, thanks for the patch. Please see my comments below.

On Thu, Aug 22, 2019 at 5:53 PM <amginwal@gmail.com> wrote:
>
> From: Aliasgar Ginwala <aginwala@ebay.com>
>
> Add support in ovsdb-tool for migrating clustered dbs to standalone dbs.
> E.g. usage to migrate nb/sb db to standalone db from raft:
> ovsdb-tool migrate-cluster-db ovnnb_db.db ovnnb_db_cluster.db

The name "migrate-cluster-db" is a little confusing. It would be better to
tell the direction from the name. I suggest "cluster-to-standalone", if
"convert-from-cluster-to-standalone" is too long.

>
> Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com>
> ---
>  ovsdb/ovsdb-tool.c | 154 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 152 insertions(+), 2 deletions(-)
>
> diff --git a/ovsdb/ovsdb-tool.c b/ovsdb/ovsdb-tool.c
> index 438f97590..4aa1d4b3f 100644
> --- a/ovsdb/ovsdb-tool.c
> +++ b/ovsdb/ovsdb-tool.c
> @@ -173,6 +173,8 @@ usage(void)
>             "  compare-versions A OP B  compare OVSDB schema version
numbers\n"
>             "  query [DB] TRNS         execute read-only transaction on
DB\n"
>             "  transact [DB] TRNS      execute read/write transaction on
DB\n"
> +           "  migrate-cluster-db [DB [DB]]    Migrate clustered DB to\n"
> +                   "standalone DB\n "
>             "  [-m]... show-log [DB]   print DB's log entries\n"
>             "The default DB is %s.\n"
>             "The default SCHEMA is %s.\n",
> @@ -206,7 +208,7 @@ default_schema(void)
>      }
>      return schema;
>  }
> -
> +

Any special character change here?

>  static struct json *
>  parse_json(const char *s)
>  {
> @@ -244,7 +246,7 @@ read_standalone_schema(const char *filename)
>      ovsdb_storage_close(storage);
>      return schema;
>  }
> -
> +
>  static void
>  do_create(struct ovs_cmdl_context *ctx)
>  {
> @@ -942,6 +944,94 @@ print_raft_record(const struct raft_record *r,
>      }
>  }
>
> +static struct ovsdb_log *
> +write_raft_header_to_file(const struct json *data, const char
*db_file_name)
> +{
> +    if (!data) {
> +        return NULL;
> +    }
> +
> +    if (json_array(data)->n != 2) {
> +        printf(" ***invalid data***\n");

Better to use ovs_fatal() so that the process exit with an error.

> +        return NULL;
> +    }
> +
> +    struct ovsdb_log *log;
> +    struct json *schema_json = json_array(data)->elems[0];
> +    if (schema_json->type != JSON_NULL) {
> +        struct ovsdb_schema *schema;
> +        check_ovsdb_error(ovsdb_schema_from_json(schema_json, &schema));
> +        check_ovsdb_error(ovsdb_log_open(db_file_name, OVSDB_MAGIC,
> +                                     OVSDB_LOG_CREATE_EXCL, -1, &log));

It seems not the right place to open the standalone DB file. It is better
to be opened in the same place where the clustered DB is opened. The open()
and close() are better to be paired at same level of call stack.

> +        check_ovsdb_error(ovsdb_log_write_and_free(log, schema_json));
> +        check_ovsdb_error(ovsdb_log_commit_block(log));
> +    }
> +
> +    struct json *data_json = json_array(data)->elems[1];
> +    if (!data_json || data_json->type != JSON_OBJECT) {
> +        return NULL;
> +    }
> +    if (data_json->type != JSON_NULL) {
> +        check_ovsdb_error(ovsdb_log_write_and_free(log, data_json));
> +        check_ovsdb_error(ovsdb_log_commit_block(log));
> +    }
> +    return log;
> +}
> +
> +static struct ovsdb_log *
> +write_raft_header(const struct raft_header *h, const char *db_file_name)
> +{
> +    if (h->snap_index) {
> +        return write_raft_header_to_file(h->snap.data, db_file_name);
> +    }
> +    return NULL;
> +}
> +
> +static void
> +write_raft_records_to_file(const struct json *data, struct ovsdb_log
*log_data)
> +{
> +    if (json_array(data)->n != 2) {
> +        printf(" ***invalid data***\n");

Better to use ovs_fatal() so that the process exit with an error.

> +        return;
> +    }
> +
> +    struct json *data_json = json_array(data)->elems[1];
> +    if (data_json->type != JSON_NULL) {
> +        check_ovsdb_error(ovsdb_log_write_and_free(log_data, data_json));
> +        check_ovsdb_error(ovsdb_log_commit_block(log_data));
> +    }
> +}
> +
> +static void
> +write_raft_records(const struct raft_record *r, struct ovsdb_log
*log_data)

The function name is confusing. It is actually writing a single raft record
to a standalone format file.

> +{
> +    switch (r->type) {
> +    case RAFT_REC_ENTRY:

No need to use switch-case since there is only one case to be handled.
Better to use if/else instead.

> +        if (!r->entry.data) {
> +            return;
> +        }
> +        write_raft_records_to_file(r->entry.data, log_data);
> +        break;
> +
> +    case RAFT_REC_TERM:
> +        break;
> +
> +    case RAFT_REC_VOTE:
> +        break;
> +
> +    case RAFT_REC_NOTE:
> +        break;
> +
> +    case RAFT_REC_COMMIT_INDEX:
> +        break;
> +
> +    case RAFT_REC_LEADER:
> +        break;
> +    default:
> +        OVS_NOT_REACHED();
> +    }
> +}
> +
>  static void
>  do_show_log_cluster(struct ovsdb_log *log)
>  {
> @@ -1511,6 +1601,65 @@ do_compare_versions(struct ovs_cmdl_context *ctx)
>      exit(result ? 0 : 2);
>  }
>
> +static void
> +do_migrate_cluster(struct ovsdb_log *log, const char *db_file_name)
> +{
> +    struct ovsdb_log *log_data = NULL;
> +    for (unsigned int i = 0; ; i++) {
> +        struct json *json;
> +        check_ovsdb_error(ovsdb_log_read(log, &json));
> +        if (!json) {
> +            break;
> +        }
> +
> +        printf("record %u:\n", i);

I think it would be better not printing each record. If it is needed, it
would be better to add an option -v to support it.

> +        struct ovsdb_error *error;
> +        if (i == 0) {
> +            struct raft_header h;
> +            error = raft_header_from_json(&h, json);
> +            if (!error) {
> +                log_data = write_raft_header(&h, db_file_name);
> +                raft_header_uninit(&h);
> +                if (!log_data) {
> +                    return;
> +                }
> +            }
> +        } else {
> +            struct raft_record r;
> +            error = raft_record_from_json(&r, json);
> +            if (!error) {
> +                write_raft_records(&r, log_data);
> +                raft_record_uninit(&r);
> +            }
> +        }
> +        if (error) {

Why not checking error using check_ovsdb_error()? Is any error expected so
that it doesn't want to error out?

> +            char *s = ovsdb_error_to_string_free(error);
> +            puts(s);
> +            free(s);
> +        }
> +
> +        putchar('\n');
> +    }
> +    ovsdb_log_close(log_data);

It seems the code seems will never reach here.

> +}
> +
> +static void
> +do_migrate(struct ovs_cmdl_context *ctx)
> +{
> +    const char *db_file_name = ctx->argv[1];
> +    const char *cluster_db_file_name = ctx->argv[2];
> +    struct ovsdb_log *log;
> +
> +    check_ovsdb_error(ovsdb_log_open(cluster_db_file_name,
> +                                     OVSDB_MAGIC"|"RAFT_MAGIC,
> +                                     OVSDB_LOG_READ_ONLY, -1, &log));
> +    if (!strcmp(ovsdb_log_get_magic(log), OVSDB_MAGIC)) {

It would be more straightforward to check if the magic is not RAFT_MAGIC.
Otherwise, this check will need to be updated in the future if a 3rd magic
is supported.

> +       printf("Data base is not clustered db.");

Better to use ovs_fatal() instead of printf.

> +    } else {
> +        do_migrate_cluster(log, db_file_name);
> +    }
> +    ovsdb_log_close(log);
> +}
>
>  static void
>  do_help(struct ovs_cmdl_context *ctx OVS_UNUSED)
> @@ -1550,6 +1699,7 @@ static const struct ovs_cmdl_command all_commands[]
= {
>      { "compare-versions", "a op b", 3, 3, do_compare_versions, OVS_RO },
>      { "help", NULL, 0, INT_MAX, do_help, OVS_RO },
>      { "list-commands", NULL, 0, INT_MAX, do_list_commands, OVS_RO },
> +    { "migrate-cluster-db", "[db [clusterdb]]", 0, 2, do_migrate, OVS_RW
},

This command requires 2 args. So it should be 2, 2.
Besides, it would be better to follow the convention to use function name
do_xxx_yyy for command xxx-yyy.

>      { NULL, NULL, 0, 0, NULL, OVS_RO },
>  };
>
> --
> 2.20.1 (Apple Git-117)
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
aginwala aginwala Aug. 23, 2019, 5:28 a.m. UTC | #2
Thanks for the review Han.

On Thu, Aug 22, 2019 at 7:27 PM Han Zhou <zhouhan@gmail.com> wrote:

> Ali, thanks for the patch. Please see my comments below.
>
> On Thu, Aug 22, 2019 at 5:53 PM <amginwal@gmail.com> wrote:
> >
> > From: Aliasgar Ginwala <aginwala@ebay.com>
> >
> > Add support in ovsdb-tool for migrating clustered dbs to standalone dbs.
> > E.g. usage to migrate nb/sb db to standalone db from raft:
> > ovsdb-tool migrate-cluster-db ovnnb_db.db ovnnb_db_cluster.db
>
> The name "migrate-cluster-db" is a little confusing. It would be better to
> tell the direction from the name. I suggest "cluster-to-standalone", if
> "convert-from-cluster-to-standalone" is too long.
>
> Sure. Can change that.

> >
> > Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com>
> > ---
> >  ovsdb/ovsdb-tool.c | 154 ++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 152 insertions(+), 2 deletions(-)
> >
> > diff --git a/ovsdb/ovsdb-tool.c b/ovsdb/ovsdb-tool.c
> > index 438f97590..4aa1d4b3f 100644
> > --- a/ovsdb/ovsdb-tool.c
> > +++ b/ovsdb/ovsdb-tool.c
> > @@ -173,6 +173,8 @@ usage(void)
> >             "  compare-versions A OP B  compare OVSDB schema version
> numbers\n"
> >             "  query [DB] TRNS         execute read-only transaction on
> DB\n"
> >             "  transact [DB] TRNS      execute read/write transaction on
> DB\n"
> > +           "  migrate-cluster-db [DB [DB]]    Migrate clustered DB to\n"
> > +                   "standalone DB\n "
> >             "  [-m]... show-log [DB]   print DB's log entries\n"
> >             "The default DB is %s.\n"
> >             "The default SCHEMA is %s.\n",
> > @@ -206,7 +208,7 @@ default_schema(void)
> >      }
> >      return schema;
> >  }
> > -
> > +
>
> Any special character change here?
>
> > Checkpatch didn't show me this. Will see why it is showing up.

> >  static struct json *
> >  parse_json(const char *s)
> >  {
> > @@ -244,7 +246,7 @@ read_standalone_schema(const char *filename)
> >      ovsdb_storage_close(storage);
> >      return schema;
> >  }
> > -
> > +
> >  static void
> >  do_create(struct ovs_cmdl_context *ctx)
> >  {
> > @@ -942,6 +944,94 @@ print_raft_record(const struct raft_record *r,
> >      }
> >  }
> >
> > +static struct ovsdb_log *
> > +write_raft_header_to_file(const struct json *data, const char
> *db_file_name)
> > +{
> > +    if (!data) {
> > +        return NULL;
> > +    }
> > +
> > +    if (json_array(data)->n != 2) {
> > +        printf(" ***invalid data***\n");
>
> Better to use ovs_fatal() so that the process exit with an error.
>
> > Actually it is ok to print since its not an error. It's just invalid
data since its tool. This is common usage in ovsdb-tool for iterating
invalid data in current code itself. Do you want me to refactor that too? I
can handle that in separate patch to be consistent.

> > +        return NULL;
> > +    }
> > +
> > +    struct ovsdb_log *log;
> > +    struct json *schema_json = json_array(data)->elems[0];
> > +    if (schema_json->type != JSON_NULL) {
> > +        struct ovsdb_schema *schema;
> > +        check_ovsdb_error(ovsdb_schema_from_json(schema_json, &schema));
> > +        check_ovsdb_error(ovsdb_log_open(db_file_name, OVSDB_MAGIC,
> > +                                     OVSDB_LOG_CREATE_EXCL, -1, &log));
>
> It seems not the right place to open the standalone DB file. It is better
> to be opened in the same place where the clustered DB is opened. The open()
> and close() are better to be paired at same level of call stack.
>
> Yes I considered it before. However, I felt it actually doesn't make
sense to open new standalone db in the very beginning even before parsing
raft header at the minimal if raft header has  invalid data. Hence, opened
here. Let me know if you still want me to move there. Agree, it will be
more neat to read in do_migrat() where we also open/close clustered db
files.

>
> > +        check_ovsdb_error(ovsdb_log_write_and_free(log, schema_json));
> > +        check_ovsdb_error(ovsdb_log_commit_block(log));
> > +    }
> > +
> > +    struct json *data_json = json_array(data)->elems[1];
> > +    if (!data_json || data_json->type != JSON_OBJECT) {
> > +        return NULL;
> > +    }
> > +    if (data_json->type != JSON_NULL) {
> > +        check_ovsdb_error(ovsdb_log_write_and_free(log, data_json));
> > +        check_ovsdb_error(ovsdb_log_commit_block(log));
> > +    }
> > +    return log;
> > +}
> > +
> > +static struct ovsdb_log *
> > +write_raft_header(const struct raft_header *h, const char *db_file_name)
> > +{
> > +    if (h->snap_index) {
> > +        return write_raft_header_to_file(h->snap.data, db_file_name);
> > +    }
> > +    return NULL;
> > +}
> > +
> > +static void
> > +write_raft_records_to_file(const struct json *data, struct ovsdb_log
> *log_data)
> > +{
> > +    if (json_array(data)->n != 2) {
> > +        printf(" ***invalid data***\n");
>
> Better to use ovs_fatal() so that the process exit with an error.
>
> Same as above.

>
> > +        return;
> > +    }
> > +
> > +    struct json *data_json = json_array(data)->elems[1];
> > +    if (data_json->type != JSON_NULL) {
> > +        check_ovsdb_error(ovsdb_log_write_and_free(log_data,
> data_json));
> > +        check_ovsdb_error(ovsdb_log_commit_block(log_data));
> > +    }
> > +}
> > +
> > +static void
> > +write_raft_records(const struct raft_record *r, struct ovsdb_log
> *log_data)
>
> The function name is confusing. It is actually writing a single raft
> record to a standalone format file.
>
> Sure will change.

>
> > +{
> > +    switch (r->type) {
> > +    case RAFT_REC_ENTRY:
>
> No need to use switch-case since there is only one case to be handled.
> Better to use if/else instead.
>
> Guess I got warnings back while compiling few days back  if all r-types
are not handled as per its interface. Can double confirm if thats still
complaining. Else will keep it.

>
> > +        if (!r->entry.data) {
> > +            return;
> > +        }
> > +        write_raft_records_to_file(r->entry.data, log_data);
> > +        break;
> > +
> > +    case RAFT_REC_TERM:
> > +        break;
> > +
> > +    case RAFT_REC_VOTE:
> > +        break;
> > +
> > +    case RAFT_REC_NOTE:
> > +        break;
> > +
> > +    case RAFT_REC_COMMIT_INDEX:
> > +        break;
> > +
> > +    case RAFT_REC_LEADER:
> > +        break;
> > +    default:
> > +        OVS_NOT_REACHED();
> > +    }
> > +}
> > +
> >  static void
> >  do_show_log_cluster(struct ovsdb_log *log)
> >  {
> > @@ -1511,6 +1601,65 @@ do_compare_versions(struct ovs_cmdl_context *ctx)
> >      exit(result ? 0 : 2);
> >  }
> >
> > +static void
> > +do_migrate_cluster(struct ovsdb_log *log, const char *db_file_name)
> > +{
> > +    struct ovsdb_log *log_data = NULL;
> > +    for (unsigned int i = 0; ; i++) {
> > +        struct json *json;
> > +        check_ovsdb_error(ovsdb_log_read(log, &json));
> > +        if (!json) {
> > +            break;
> > +        }
> > +
> > +        printf("record %u:\n", i);
>
> I think it would be better not printing each record. If it is needed, it
> would be better to add an option -v to support it.
>
> We are just printing record number so that if someone wants to still
validation of what was migrated, its easy. Let me know else I can delete it
and handle it in -v.

>
> > +        struct ovsdb_error *error;
> > +        if (i == 0) {
> > +            struct raft_header h;
> > +            error = raft_header_from_json(&h, json);
> > +            if (!error) {
> > +                log_data = write_raft_header(&h, db_file_name);
> > +                raft_header_uninit(&h);
> > +                if (!log_data) {
> > +                    return;
> > +                }
> > +            }
> > +        } else {
> > +            struct raft_record r;
> > +            error = raft_record_from_json(&r, json);
> > +            if (!error) {
> > +                write_raft_records(&r, log_data);
> > +                raft_record_uninit(&r);
> > +            }
> > +        }
> > +        if (error) {
>
> Why not checking error using check_ovsdb_error()? Is any error expected so
> that it doesn't want to error out?
>
>  This is just to check if error parsing raft header or record for
whatever reason. Not just validating db.

>
> > +            char *s = ovsdb_error_to_string_free(error);
> > +            puts(s);
> > +            free(s);
> > +        }
> > +
> > +        putchar('\n');
> > +    }
> > +    ovsdb_log_close(log_data);
>
> It seems the code seems will never reach here.
>
>Sorry, please let me know whats the confusion.

>
>
>
> > +}
> > +
> > +static void
> > +do_migrate(struct ovs_cmdl_context *ctx)
> > +{
> > +    const char *db_file_name = ctx->argv[1];
> > +    const char *cluster_db_file_name = ctx->argv[2];
> > +    struct ovsdb_log *log;
> > +
> > +    check_ovsdb_error(ovsdb_log_open(cluster_db_file_name,
> > +                                     OVSDB_MAGIC"|"RAFT_MAGIC,
> > +                                     OVSDB_LOG_READ_ONLY, -1, &log));
> > +    if (!strcmp(ovsdb_log_get_magic(log), OVSDB_MAGIC)) {
>
> It would be more straightforward to check if the magic is not RAFT_MAGIC.
> Otherwise, this check will need to be updated in the future if a 3rd magic
> is supported.
>
Yeah sure. Will do that.

>
> > +       printf("Data base is not clustered db.");
>
> Better to use ovs_fatal() instead of printf.
>
> Ok sure can do that.

>
> > +    } else {
> > +        do_migrate_cluster(log, db_file_name);
> > +    }
> > +    ovsdb_log_close(log);
> > +}
> >
> >  static void
> >  do_help(struct ovs_cmdl_context *ctx OVS_UNUSED)
> > @@ -1550,6 +1699,7 @@ static const struct ovs_cmdl_command
> all_commands[] = {
> >      { "compare-versions", "a op b", 3, 3, do_compare_versions, OVS_RO },
> >      { "help", NULL, 0, INT_MAX, do_help, OVS_RO },
> >      { "list-commands", NULL, 0, INT_MAX, do_list_commands, OVS_RO },
> > +    { "migrate-cluster-db", "[db [clusterdb]]", 0, 2, do_migrate,
> OVS_RW },
>
> This command requires 2 args. So it should be 2, 2.
> Besides, it would be better to follow the convention to use function name
> do_xxx_yyy for command xxx-yyy.
>
> Ack! Will handle.

>
> >      { NULL, NULL, 0, 0, NULL, OVS_RO },
> >  };
> >
> > --
> > 2.20.1 (Apple Git-117)
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Han Zhou Aug. 23, 2019, 3:17 p.m. UTC | #3
On Thu, Aug 22, 2019 at 10:29 PM aginwala aginwala <amginwal@gmail.com>
wrote:

> Thanks for the review Han.
>
> On Thu, Aug 22, 2019 at 7:27 PM Han Zhou <zhouhan@gmail.com> wrote:
>
>> Ali, thanks for the patch. Please see my comments below.
>>
>> On Thu, Aug 22, 2019 at 5:53 PM <amginwal@gmail.com> wrote:
>> >
>> > From: Aliasgar Ginwala <aginwala@ebay.com>
>> >
>> > Add support in ovsdb-tool for migrating clustered dbs to standalone dbs.
>> > E.g. usage to migrate nb/sb db to standalone db from raft:
>> > ovsdb-tool migrate-cluster-db ovnnb_db.db ovnnb_db_cluster.db
>>
>> The name "migrate-cluster-db" is a little confusing. It would be better
>> to tell the direction from the name. I suggest "cluster-to-standalone", if
>> "convert-from-cluster-to-standalone" is too long.
>>
>> Sure. Can change that.
>
>> >
>> > Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com>
>> > ---
>> >  ovsdb/ovsdb-tool.c | 154 ++++++++++++++++++++++++++++++++++++++++++++-
>> >  1 file changed, 152 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/ovsdb/ovsdb-tool.c b/ovsdb/ovsdb-tool.c
>> > index 438f97590..4aa1d4b3f 100644
>> > --- a/ovsdb/ovsdb-tool.c
>> > +++ b/ovsdb/ovsdb-tool.c
>> > @@ -173,6 +173,8 @@ usage(void)
>> >             "  compare-versions A OP B  compare OVSDB schema version
>> numbers\n"
>> >             "  query [DB] TRNS         execute read-only transaction on
>> DB\n"
>> >             "  transact [DB] TRNS      execute read/write transaction
>> on DB\n"
>> > +           "  migrate-cluster-db [DB [DB]]    Migrate clustered DB
>> to\n"
>> > +                   "standalone DB\n "
>> >             "  [-m]... show-log [DB]   print DB's log entries\n"
>> >             "The default DB is %s.\n"
>> >             "The default SCHEMA is %s.\n",
>> > @@ -206,7 +208,7 @@ default_schema(void)
>> >      }
>> >      return schema;
>> >  }
>> > -
>> > +
>>
>> Any special character change here?
>>
>> > Checkpatch didn't show me this. Will see why it is showing up.
>
>  static struct json *
>> >  parse_json(const char *s)
>> >  {
>> > @@ -244,7 +246,7 @@ read_standalone_schema(const char *filename)
>> >      ovsdb_storage_close(storage);
>> >      return schema;
>> >  }
>> > -
>> > +
>> >  static void
>> >  do_create(struct ovs_cmdl_context *ctx)
>> >  {
>> > @@ -942,6 +944,94 @@ print_raft_record(const struct raft_record *r,
>> >      }
>> >  }
>> >
>> > +static struct ovsdb_log *
>> > +write_raft_header_to_file(const struct json *data, const char
>> *db_file_name)
>> > +{
>> > +    if (!data) {
>> > +        return NULL;
>> > +    }
>> > +
>> > +    if (json_array(data)->n != 2) {
>> > +        printf(" ***invalid data***\n");
>>
>> Better to use ovs_fatal() so that the process exit with an error.
>>
>> > Actually it is ok to print since its not an error. It's just invalid
> data since its tool. This is common usage in ovsdb-tool for iterating
> invalid data in current code itself. Do you want me to refactor that too? I
> can handle that in separate patch to be consistent.
>
The use case of the existing code is a little different. It is in function
print_data(), which is to print the data in readable format. So if there is
invalid data, it is reasonable to just print it out and continue. However,
here we are doing data conversion. I think it is better to report error and
at the same time treat the conversion as failed if the source data is
invalid. At least this is how ovsdb-tool create-cluster from standalone DB
behaves. Actually you are also doing this below when parsing the header
fails, so I think it is better to make this behavior consistent across the
process of conversion, and also consistent between converting to and from
clustered DB. It is not necessary to refactor existing code in print_data().


> > +        return NULL;
>> > +    }
>> > +
>> > +    struct ovsdb_log *log;
>> > +    struct json *schema_json = json_array(data)->elems[0];
>> > +    if (schema_json->type != JSON_NULL) {
>> > +        struct ovsdb_schema *schema;
>> > +        check_ovsdb_error(ovsdb_schema_from_json(schema_json,
>> &schema));
>> > +        check_ovsdb_error(ovsdb_log_open(db_file_name, OVSDB_MAGIC,
>> > +                                     OVSDB_LOG_CREATE_EXCL, -1, &log));
>>
>> It seems not the right place to open the standalone DB file. It is better
>> to be opened in the same place where the clustered DB is opened. The open()
>> and close() are better to be paired at same level of call stack.
>>
> > Yes I considered it before. However, I felt it actually doesn't make
> sense to open new standalone db in the very beginning even before parsing
> raft header at the minimal if raft header has  invalid data. Hence, opened
> here. Let me know if you still want me to move there. Agree, it will be
> more neat to read in do_migrat() where we also open/close clustered db
> files.
>
It makes sense to do validation before openning the standalone DB file.
However, I suggest the validation logic (together with openning/closing the
file) be done at a higher layer. I guess the intent of putting it here is
to make sure there is no file created if parsing the header failed, but
even if parsing header succeeded here, the conversion can fail anywhere
later. It can just delete the new file if the conversion fails after the
new log file is openned (whether it counters error in header parsing or
body parsing).


>> > +        check_ovsdb_error(ovsdb_log_write_and_free(log, schema_json));
>> > +        check_ovsdb_error(ovsdb_log_commit_block(log));
>> > +    }
>> > +
>> > +    struct json *data_json = json_array(data)->elems[1];
>> > +    if (!data_json || data_json->type != JSON_OBJECT) {
>> > +        return NULL;
>> > +    }
>> > +    if (data_json->type != JSON_NULL) {
>> > +        check_ovsdb_error(ovsdb_log_write_and_free(log, data_json));
>> > +        check_ovsdb_error(ovsdb_log_commit_block(log));
>> > +    }
>> > +    return log;
>> > +}
>> > +
>> > +static struct ovsdb_log *
>> > +write_raft_header(const struct raft_header *h, const char
>> *db_file_name)
>> > +{
>> > +    if (h->snap_index) {
>> > +        return write_raft_header_to_file(h->snap.data, db_file_name);
>> > +    }
>> > +    return NULL;
>> > +}
>> > +
>> > +static void
>> > +write_raft_records_to_file(const struct json *data, struct ovsdb_log
>> *log_data)
>> > +{
>> > +    if (json_array(data)->n != 2) {
>> > +        printf(" ***invalid data***\n");
>>
>> Better to use ovs_fatal() so that the process exit with an error.
>>
> > Same as above.
>
>>
>> > +        return;
>> > +    }
>> > +
>> > +    struct json *data_json = json_array(data)->elems[1];
>> > +    if (data_json->type != JSON_NULL) {
>> > +        check_ovsdb_error(ovsdb_log_write_and_free(log_data,
>> data_json));
>> > +        check_ovsdb_error(ovsdb_log_commit_block(log_data));
>>
>
I forgot to comment here that it may be inefficient to do commit_block for
every record writing.


> > +    }
>> > +}
>> > +
>> > +static void
>> > +write_raft_records(const struct raft_record *r, struct ovsdb_log
>> *log_data)
>>
>> The function name is confusing. It is actually writing a single raft
>> record to a standalone format file.
>>
> > Sure will change.
>
>>
>> > +{
>> > +    switch (r->type) {
>> > +    case RAFT_REC_ENTRY:
>>
>> No need to use switch-case since there is only one case to be handled.
>> Better to use if/else instead.
>>
> > Guess I got warnings back while compiling few days back  if all r-types
> are not handled as per its interface. Can double confirm if thats still
> complaining. Else will keep it.
>

This swtich case is equivalent to:

if (r->type == RAFT_REC_ENTRY) {
    ...
}

Would there be any warnings?


>> > +        if (!r->entry.data) {
>> > +            return;
>> > +        }
>> > +        write_raft_records_to_file(r->entry.data, log_data);
>> > +        break;
>> > +
>> > +    case RAFT_REC_TERM:
>> > +        break;
>> > +
>> > +    case RAFT_REC_VOTE:
>> > +        break;
>> > +
>> > +    case RAFT_REC_NOTE:
>> > +        break;
>> > +
>> > +    case RAFT_REC_COMMIT_INDEX:
>> > +        break;
>> > +
>> > +    case RAFT_REC_LEADER:
>> > +        break;
>> > +    default:
>> > +        OVS_NOT_REACHED();
>> > +    }
>> > +}
>> > +
>> >  static void
>> >  do_show_log_cluster(struct ovsdb_log *log)
>> >  {
>> > @@ -1511,6 +1601,65 @@ do_compare_versions(struct ovs_cmdl_context *ctx)
>> >      exit(result ? 0 : 2);
>> >  }
>> >
>> > +static void
>> > +do_migrate_cluster(struct ovsdb_log *log, const char *db_file_name)
>> > +{
>> > +    struct ovsdb_log *log_data = NULL;
>> > +    for (unsigned int i = 0; ; i++) {
>> > +        struct json *json;
>> > +        check_ovsdb_error(ovsdb_log_read(log, &json));
>> > +        if (!json) {
>> > +            break;
>> > +        }
>> > +
>> > +        printf("record %u:\n", i);
>>
>> I think it would be better not printing each record. If it is needed, it
>> would be better to add an option -v to support it.
>>
> > We are just printing record number so that if someone wants to still
> validation of what was migrated, its easy. Let me know else I can delete it
> and handle it in -v.
>

Could you explain why would it be useful for validation by printing each
record number? Or did you want to just print the error record number? I'd
prefer just don't print it otherwise.


>> > +        struct ovsdb_error *error;
>> > +        if (i == 0) {
>> > +            struct raft_header h;
>> > +            error = raft_header_from_json(&h, json);
>> > +            if (!error) {
>> > +                log_data = write_raft_header(&h, db_file_name);
>> > +                raft_header_uninit(&h);
>> > +                if (!log_data) {
>> > +                    return;
>>
>
The file is not closed in this situation.

> +                }
>> > +            }
>> > +        } else {
>> > +            struct raft_record r;
>> > +            error = raft_record_from_json(&r, json);
>> > +            if (!error) {
>> > +                write_raft_records(&r, log_data);
>> > +                raft_record_uninit(&r);
>> > +            }
>> > +        }
>> > +        if (error) {
>>
>> Why not checking error using check_ovsdb_error()? Is any error expected
>> so that it doesn't want to error out?
>>
> >  This is just to check if error parsing raft header or record for
> whatever reason. Not just validating db.
>

I meant, why not error out (exit the process with error) when error
encourntered during conversion?


>
>> > +            char *s = ovsdb_error_to_string_free(error);
>> > +            puts(s);
>> > +            free(s);
>> > +        }
>> > +
>> > +        putchar('\n');
>> > +    }
>> > +    ovsdb_log_close(log_data);
>>
>> It seems the code seems will never reach here.
>>
> >Sorry, please let me know whats the confusion.
>
Sorry, I misread it.

>
>>
> >
>> > +}
>> > +
>> > +static void
>> > +do_migrate(struct ovs_cmdl_context *ctx)
>> > +{
>> > +    const char *db_file_name = ctx->argv[1];
>> > +    const char *cluster_db_file_name = ctx->argv[2];
>> > +    struct ovsdb_log *log;
>> > +
>> > +    check_ovsdb_error(ovsdb_log_open(cluster_db_file_name,
>> > +                                     OVSDB_MAGIC"|"RAFT_MAGIC,
>> > +                                     OVSDB_LOG_READ_ONLY, -1, &log));
>> > +    if (!strcmp(ovsdb_log_get_magic(log), OVSDB_MAGIC)) {
>>
>> It would be more straightforward to check if the magic is not RAFT_MAGIC.
>> Otherwise, this check will need to be updated in the future if a 3rd magic
>> is supported.
>>
> Yeah sure. Will do that.
>
>>
>> > +       printf("Data base is not clustered db.");
>>
>> Better to use ovs_fatal() instead of printf.
>>
> > Ok sure can do that.
>
>>
>> > +    } else {
>> > +        do_migrate_cluster(log, db_file_name);
>> > +    }
>> > +    ovsdb_log_close(log);
>> > +}
>> >
>> >  static void
>> >  do_help(struct ovs_cmdl_context *ctx OVS_UNUSED)
>> > @@ -1550,6 +1699,7 @@ static const struct ovs_cmdl_command
>> all_commands[] = {
>> >      { "compare-versions", "a op b", 3, 3, do_compare_versions, OVS_RO
>> },
>> >      { "help", NULL, 0, INT_MAX, do_help, OVS_RO },
>> >      { "list-commands", NULL, 0, INT_MAX, do_list_commands, OVS_RO },
>> > +    { "migrate-cluster-db", "[db [clusterdb]]", 0, 2, do_migrate,
>> OVS_RW },
>>
>> This command requires 2 args. So it should be 2, 2.
>> Besides, it would be better to follow the convention to use function name
>> do_xxx_yyy for command xxx-yyy.
>>
> > Ack! Will handle.
>
>>
>> >      { NULL, NULL, 0, 0, NULL, OVS_RO },
>> >  };
>> >
>> > --
>> > 2.20.1 (Apple Git-117)
>> >
>> > _______________________________________________
>> > dev mailing list
>> > dev@openvswitch.org
>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
Ben Pfaff Aug. 23, 2019, 5:40 p.m. UTC | #4
On Thu, Aug 22, 2019 at 05:53:10PM -0700, amginwal@gmail.com wrote:
> From: Aliasgar Ginwala <aginwala@ebay.com>
> 
> Add support in ovsdb-tool for migrating clustered dbs to standalone dbs.
> E.g. usage to migrate nb/sb db to standalone db from raft:
> ovsdb-tool migrate-cluster-db ovnnb_db.db ovnnb_db_cluster.db
> 
> Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com>

In addition to Han's comments, I suggest updating the documentation,
both for ovsdb-tool itself and Documentation/ref/ovsdb.7.rst.  This kind
of tool can't be sure to get an up-to-date snapshot of the database,
since it only looks at one member of the raft cluster and therefore
can't be sure about the current consensus, so it's more risky to use it
than an online operation like "ovsdb-client backup".  That's probably OK
in some circumstances (notably, if the cluster is already down and can't
be easily revived), but I think that the documentation should be clear
that there is a trade-off.
aginwala Aug. 23, 2019, 6:11 p.m. UTC | #5
Thanks Han for detailed review.

On Fri, Aug 23, 2019 at 8:17 AM Han Zhou <zhouhan@gmail.com> wrote:

> On Thu, Aug 22, 2019 at 10:29 PM aginwala aginwala <amginwal@gmail.com>
> wrote:
>
> > Thanks for the review Han.
> >
> > On Thu, Aug 22, 2019 at 7:27 PM Han Zhou <zhouhan@gmail.com> wrote:
> >
> >> Ali, thanks for the patch. Please see my comments below.
> >>
> >> On Thu, Aug 22, 2019 at 5:53 PM <amginwal@gmail.com> wrote:
> >> >
> >> > From: Aliasgar Ginwala <aginwala@ebay.com>
> >> >
> >> > Add support in ovsdb-tool for migrating clustered dbs to standalone
> dbs.
> >> > E.g. usage to migrate nb/sb db to standalone db from raft:
> >> > ovsdb-tool migrate-cluster-db ovnnb_db.db ovnnb_db_cluster.db
> >>
> >> The name "migrate-cluster-db" is a little confusing. It would be better
> >> to tell the direction from the name. I suggest "cluster-to-standalone",
> if
> >> "convert-from-cluster-to-standalone" is too long.
> >>
> >> Sure. Can change that.
> >
> >> >
> >> > Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com>
> >> > ---
> >> >  ovsdb/ovsdb-tool.c | 154
> ++++++++++++++++++++++++++++++++++++++++++++-
> >> >  1 file changed, 152 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/ovsdb/ovsdb-tool.c b/ovsdb/ovsdb-tool.c
> >> > index 438f97590..4aa1d4b3f 100644
> >> > --- a/ovsdb/ovsdb-tool.c
> >> > +++ b/ovsdb/ovsdb-tool.c
> >> > @@ -173,6 +173,8 @@ usage(void)
> >> >             "  compare-versions A OP B  compare OVSDB schema version
> >> numbers\n"
> >> >             "  query [DB] TRNS         execute read-only transaction
> on
> >> DB\n"
> >> >             "  transact [DB] TRNS      execute read/write transaction
> >> on DB\n"
> >> > +           "  migrate-cluster-db [DB [DB]]    Migrate clustered DB
> >> to\n"
> >> > +                   "standalone DB\n "
> >> >             "  [-m]... show-log [DB]   print DB's log entries\n"
> >> >             "The default DB is %s.\n"
> >> >             "The default SCHEMA is %s.\n",
> >> > @@ -206,7 +208,7 @@ default_schema(void)
> >> >      }
> >> >      return schema;
> >> >  }
> >> > -
> >> > +
> >>
> >> Any special character change here?
> >>
> >> > Checkpatch didn't show me this. Will see why it is showing up.
> >
> >  static struct json *
> >> >  parse_json(const char *s)
> >> >  {
> >> > @@ -244,7 +246,7 @@ read_standalone_schema(const char *filename)
> >> >      ovsdb_storage_close(storage);
> >> >      return schema;
> >> >  }
> >> > -
> >> > +
> >> >  static void
> >> >  do_create(struct ovs_cmdl_context *ctx)
> >> >  {
> >> > @@ -942,6 +944,94 @@ print_raft_record(const struct raft_record *r,
> >> >      }
> >> >  }
> >> >
> >> > +static struct ovsdb_log *
> >> > +write_raft_header_to_file(const struct json *data, const char
> >> *db_file_name)
> >> > +{
> >> > +    if (!data) {
> >> > +        return NULL;
> >> > +    }
> >> > +
> >> > +    if (json_array(data)->n != 2) {
> >> > +        printf(" ***invalid data***\n");
> >>
> >> Better to use ovs_fatal() so that the process exit with an error.
> >>
> >> > Actually it is ok to print since its not an error. It's just invalid
> > data since its tool. This is common usage in ovsdb-tool for iterating
> > invalid data in current code itself. Do you want me to refactor that
> too? I
> > can handle that in separate patch to be consistent.
> >
> The use case of the existing code is a little different. It is in function
> print_data(), which is to print the data in readable format. So if there is
> invalid data, it is reasonable to just print it out and continue. However,
> here we are doing data conversion. I think it is better to report error and
> at the same time treat the conversion as failed if the source data is
> invalid. At least this is how ovsdb-tool create-cluster from standalone DB
> behaves. Actually you are also doing this below when parsing the header
> fails, so I think it is better to make this behavior consistent across the
> process of conversion, and also consistent between converting to and from
> clustered DB. It is not necessary to refactor existing code in
> print_data().
>
> ok got it. Will report error.

>
>
> > > +        return NULL;
> >> > +    }
> >> > +
> >> > +    struct ovsdb_log *log;
> >> > +    struct json *schema_json = json_array(data)->elems[0];
> >> > +    if (schema_json->type != JSON_NULL) {
> >> > +        struct ovsdb_schema *schema;
> >> > +        check_ovsdb_error(ovsdb_schema_from_json(schema_json,
> >> &schema));
> >> > +        check_ovsdb_error(ovsdb_log_open(db_file_name, OVSDB_MAGIC,
> >> > +                                     OVSDB_LOG_CREATE_EXCL, -1,
> &log));
> >>
> >> It seems not the right place to open the standalone DB file. It is
> better
> >> to be opened in the same place where the clustered DB is opened. The
> open()
> >> and close() are better to be paired at same level of call stack.
> >>
> > > Yes I considered it before. However, I felt it actually doesn't make
> > sense to open new standalone db in the very beginning even before parsing
> > raft header at the minimal if raft header has  invalid data. Hence,
> opened
> > here. Let me know if you still want me to move there. Agree, it will be
> > more neat to read in do_migrat() where we also open/close clustered db
> > files.
> >
> It makes sense to do validation before openning the standalone DB file.
> However, I suggest the validation logic (together with openning/closing the
> file) be done at a higher layer. I guess the intent of putting it here is
> to make sure there is no file created if parsing the header failed, but
> even if parsing header succeeded here, the conversion can fail anywhere
> later. It can just delete the new file if the conversion fails after the
> new log file is openned (whether it counters error in header parsing or
> body parsing).
>
> Ok no problem. I will align it with clustered db open/close call stack!

>
> >> > +        check_ovsdb_error(ovsdb_log_write_and_free(log,
> schema_json));
> >> > +        check_ovsdb_error(ovsdb_log_commit_block(log));
> >> > +    }
> >> > +
> >> > +    struct json *data_json = json_array(data)->elems[1];
> >> > +    if (!data_json || data_json->type != JSON_OBJECT) {
> >> > +        return NULL;
> >> > +    }
> >> > +    if (data_json->type != JSON_NULL) {
> >> > +        check_ovsdb_error(ovsdb_log_write_and_free(log, data_json));
> >> > +        check_ovsdb_error(ovsdb_log_commit_block(log));
> >> > +    }
> >> > +    return log;
> >> > +}
> >> > +
> >> > +static struct ovsdb_log *
> >> > +write_raft_header(const struct raft_header *h, const char
> >> *db_file_name)
> >> > +{
> >> > +    if (h->snap_index) {
> >> > +        return write_raft_header_to_file(h->snap.data, db_file_name);
> >> > +    }
> >> > +    return NULL;
> >> > +}
> >> > +
> >> > +static void
> >> > +write_raft_records_to_file(const struct json *data, struct ovsdb_log
> >> *log_data)
> >> > +{
> >> > +    if (json_array(data)->n != 2) {
> >> > +        printf(" ***invalid data***\n");
> >>
> >> Better to use ovs_fatal() so that the process exit with an error.
> >>
> > > Same as above.
> >
> >>
> >> > +        return;
> >> > +    }
> >> > +
> >> > +    struct json *data_json = json_array(data)->elems[1];
> >> > +    if (data_json->type != JSON_NULL) {
> >> > +        check_ovsdb_error(ovsdb_log_write_and_free(log_data,
> >> data_json));
> >> > +        check_ovsdb_error(ovsdb_log_commit_block(log_data));
> >>
> >
> I forgot to comment here that it may be inefficient to do commit_block for
> every record writing.
>
> Sure I will do final commit before closing the file all in one shot.

>
> > > +    }
> >> > +}
> >> > +
> >> > +static void
> >> > +write_raft_records(const struct raft_record *r, struct ovsdb_log
> >> *log_data)
> >>
> >> The function name is confusing. It is actually writing a single raft
> >> record to a standalone format file.
> >>
> > > Sure will change.
> >
> >>
> >> > +{
> >> > +    switch (r->type) {
> >> > +    case RAFT_REC_ENTRY:
> >>
> >> No need to use switch-case since there is only one case to be handled.
> >> Better to use if/else instead.
> >>
> > > Guess I got warnings back while compiling few days back  if all r-types
> > are not handled as per its interface. Can double confirm if thats still
> > complaining. Else will keep it.
> >
>
> This swtich case is equivalent to:
>
> if (r->type == RAFT_REC_ENTRY) {
>     ...
> }
>
> Would there be any warnings?
>
> Yes it enum warning as below when I use switch case:
warning: enumeration value ‘RAFT_REC_TERM’ not handled in switch
[-Wswitch-enum]
     switch (r->type) {
     ^
warning: enumeration value ‘RAFT_REC_VOTE’ not handled in switch
[-Wswitch-enum]
warning: enumeration value ‘RAFT_REC_NOTE’ not handled in switch
[-Wswitch-enum]
warning: enumeration value ‘RAFT_REC_COMMIT_INDEX’ not handled in switch
[-Wswitch-enum]
 warning: enumeration value ‘RAFT_REC_LEADER’ not handled in switch
[-Wswitch-enum]
Hence, kept it. I will use if and update accordingly.

>
>
> >> > +        if (!r->entry.data) {
> >> > +            return;
> >> > +        }
> >> > +        write_raft_records_to_file(r->entry.data, log_data);
> >> > +        break;
> >> > +
> >> > +    case RAFT_REC_TERM:
> >> > +        break;
> >> > +
> >> > +    case RAFT_REC_VOTE:
> >> > +        break;
> >> > +
> >> > +    case RAFT_REC_NOTE:
> >> > +        break;
> >> > +
> >> > +    case RAFT_REC_COMMIT_INDEX:
> >> > +        break;
> >> > +
> >> > +    case RAFT_REC_LEADER:
> >> > +        break;
> >> > +    default:
> >> > +        OVS_NOT_REACHED();
> >> > +    }
> >> > +}
> >> > +
> >> >  static void
> >> >  do_show_log_cluster(struct ovsdb_log *log)
> >> >  {
> >> > @@ -1511,6 +1601,65 @@ do_compare_versions(struct ovs_cmdl_context
> *ctx)
> >> >      exit(result ? 0 : 2);
> >> >  }
> >> >
> >> > +static void
> >> > +do_migrate_cluster(struct ovsdb_log *log, const char *db_file_name)
> >> > +{
> >> > +    struct ovsdb_log *log_data = NULL;
> >> > +    for (unsigned int i = 0; ; i++) {
> >> > +        struct json *json;
> >> > +        check_ovsdb_error(ovsdb_log_read(log, &json));
> >> > +        if (!json) {
> >> > +            break;
> >> > +        }
> >> > +
> >> > +        printf("record %u:\n", i);
> >>
> >> I think it would be better not printing each record. If it is needed, it
> >> would be better to add an option -v to support it.
> >>
> > > We are just printing record number so that if someone wants to still
> > validation of what was migrated, its easy. Let me know else I can delete
> it
> > and handle it in -v.
> >
>
> Could you explain why would it be useful for validation by printing each
> record number? Or did you want to just print the error record number? I'd
> prefer just don't print it otherwise.
>
Just to track in case of failure as to how many records got parsed
correctly and which one failed.
I will skip it.

>
>
> >> > +        struct ovsdb_error *error;
> >> > +        if (i == 0) {
> >> > +            struct raft_header h;
> >> > +            error = raft_header_from_json(&h, json);
> >> > +            if (!error) {
> >> > +                log_data = write_raft_header(&h, db_file_name);
> >> > +                raft_header_uninit(&h);
> >> > +                if (!log_data) {
> >> > +                    return;
> >>
> >
> The file is not closed in this situation.
>
Yup for sure.

>
> > +                }
> >> > +            }
> >> > +        } else {
> >> > +            struct raft_record r;
> >> > +            error = raft_record_from_json(&r, json);
> >> > +            if (!error) {
> >> > +                write_raft_records(&r, log_data);
> >> > +                raft_record_uninit(&r);
> >> > +            }
> >> > +        }
> >> > +        if (error) {
> >>
> >> Why not checking error using check_ovsdb_error()? Is any error expected
> >> so that it doesn't want to error out?
> >>
> > >  This is just to check if error parsing raft header or record for
> > whatever reason. Not just validating db.
> >
>
> I meant, why not error out (exit the process with error) when error
> encourntered during conversion?
>
Ok will error out with ovs_fatal()

>
>
> >
> >> > +            char *s = ovsdb_error_to_string_free(error);
> >> > +            puts(s);
> >> > +            free(s);
> >> > +        }
> >> > +
> >> > +        putchar('\n');
> >> > +    }
> >> > +    ovsdb_log_close(log_data);
> >>
> >> It seems the code seems will never reach here.
> >>
> > >Sorry, please let me know whats the confusion.
> >
> Sorry, I misread it.
>
> >
> >>
> > >
> >> > +}
> >> > +
> >> > +static void
> >> > +do_migrate(struct ovs_cmdl_context *ctx)
> >> > +{
> >> > +    const char *db_file_name = ctx->argv[1];
> >> > +    const char *cluster_db_file_name = ctx->argv[2];
> >> > +    struct ovsdb_log *log;
> >> > +
> >> > +    check_ovsdb_error(ovsdb_log_open(cluster_db_file_name,
> >> > +                                     OVSDB_MAGIC"|"RAFT_MAGIC,
> >> > +                                     OVSDB_LOG_READ_ONLY, -1, &log));
> >> > +    if (!strcmp(ovsdb_log_get_magic(log), OVSDB_MAGIC)) {
> >>
> >> It would be more straightforward to check if the magic is not
> RAFT_MAGIC.
> >> Otherwise, this check will need to be updated in the future if a 3rd
> magic
> >> is supported.
> >>
> > Yeah sure. Will do that.
> >
> >>
> >> > +       printf("Data base is not clustered db.");
> >>
> >> Better to use ovs_fatal() instead of printf.
> >>
> > > Ok sure can do that.
> >
> >>
> >> > +    } else {
> >> > +        do_migrate_cluster(log, db_file_name);
> >> > +    }
> >> > +    ovsdb_log_close(log);
> >> > +}
> >> >
> >> >  static void
> >> >  do_help(struct ovs_cmdl_context *ctx OVS_UNUSED)
> >> > @@ -1550,6 +1699,7 @@ static const struct ovs_cmdl_command
> >> all_commands[] = {
> >> >      { "compare-versions", "a op b", 3, 3, do_compare_versions, OVS_RO
> >> },
> >> >      { "help", NULL, 0, INT_MAX, do_help, OVS_RO },
> >> >      { "list-commands", NULL, 0, INT_MAX, do_list_commands, OVS_RO },
> >> > +    { "migrate-cluster-db", "[db [clusterdb]]", 0, 2, do_migrate,
> >> OVS_RW },
> >>
> >> This command requires 2 args. So it should be 2, 2.
> >> Besides, it would be better to follow the convention to use function
> name
> >> do_xxx_yyy for command xxx-yyy.
> >>
> > > Ack! Will handle.
> >
> >>
> >> >      { NULL, NULL, 0, 0, NULL, OVS_RO },
> >> >  };
> >> >
> >> > --
> >> > 2.20.1 (Apple Git-117)
> >> >
> >> > _______________________________________________
> >> > dev mailing list
> >> > dev@openvswitch.org
> >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>
> >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
aginwala Aug. 23, 2019, 6:15 p.m. UTC | #6
Thanks Ben for review!

On Fri, Aug 23, 2019 at 10:42 AM Ben Pfaff <blp@ovn.org> wrote:

> On Thu, Aug 22, 2019 at 05:53:10PM -0700, amginwal@gmail.com wrote:
> > From: Aliasgar Ginwala <aginwala@ebay.com>
> >
> > Add support in ovsdb-tool for migrating clustered dbs to standalone dbs.
> > E.g. usage to migrate nb/sb db to standalone db from raft:
> > ovsdb-tool migrate-cluster-db ovnnb_db.db ovnnb_db_cluster.db
> >
> > Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com>
>
> In addition to Han's comments, I suggest updating the documentation,
> both for ovsdb-tool itself and Documentation/ref/ovsdb.7.rst.  This kind
> of tool can't be sure to get an up-to-date snapshot of the database,
> since it only looks at one member of the raft cluster and therefore
> can't be sure about the current consensus, so it's more risky to use it
> than an online operation like "ovsdb-client backup".  That's probably OK
> in some circumstances (notably, if the cluster is already down and can't
> be easily revived), but I think that the documentation should be clear
> that there is a trade-off.
>
Yup sure. Actually, for testing I use leader's db to avoid major drift. But
overall agree as its little risky but can help in
 in crisis like situation if  cluster is down and we want to revert to
standalone mode. Will update document likewise
as per your suggestion,

> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Vishal Deep Ajmera via dev Aug. 23, 2019, 9:29 p.m. UTC | #7
Hi Ben:

It seems ovsdb-client backup does conversion for openvswitch databases and not OVN dbs:
$ovsdb-client backup > standalone_db.db

When trying for ovnnb dbs, it keeps complaining about syntax error. May be I miss something
ovsdb-client backup  /var/run/openvswitch/ovnnb_db.sock /etc/openvswitch/ovnnb_db.db > test1.db
ovsdb-client: invalid syntax for 'backup' (use --help for help)

However, I checked code and it doesn’t accept args as per code below in ovsdb_client_command
{ "backup",             NEED_DATABASE, 0, 0,       do_backup },


Also, for ovn control plane nodes, we don’t have/need to run ovs on the control plane nodes.
As I didn’t have ovs running on my control plane nodes, it complained about the same.
ovsdb-client: failed to connect to "unix:/var/run/openvswitch/db.sock" (No such file or directory)
Only works if I start ovs and then run the command.

So I think current doc itself needs update if I hope I didn’t miss anything trivial.
Let me know and I can send formal patch to correct existing doc.





From: aginwala <aginwala@asu.edu>
Date: Friday, August 23, 2019 at 11:15 AM
To: Ben Pfaff <blp@ovn.org>
Cc: aginwala <amginwal@gmail.com>, ovs dev <dev@openvswitch.org>, "Ginwala, Aliasgar" <aginwala@ebay.com>
Subject: Re: [ovs-dev] [PATCH v2] ovsdb-tool: Convert clustered db to standalone db.

Thanks Ben for review!

On Fri, Aug 23, 2019 at 10:42 AM Ben Pfaff <blp@ovn.org<mailto:blp@ovn.org>> wrote:
On Thu, Aug 22, 2019 at 05:53:10PM -0700, amginwal@gmail.com<mailto:amginwal@gmail.com> wrote:
> From: Aliasgar Ginwala <aginwala@ebay.com<mailto:aginwala@ebay.com>>
>
> Add support in ovsdb-tool for migrating clustered dbs to standalone dbs.
> E.g. usage to migrate nb/sb db to standalone db from raft:
> ovsdb-tool migrate-cluster-db ovnnb_db.db ovnnb_db_cluster.db
>
> Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com<mailto:aginwala@ebay.com>>

In addition to Han's comments, I suggest updating the documentation,
both for ovsdb-tool itself and Documentation/ref/ovsdb.7.rst.  This kind
of tool can't be sure to get an up-to-date snapshot of the database,
since it only looks at one member of the raft cluster and therefore
can't be sure about the current consensus, so it's more risky to use it
than an online operation like "ovsdb-client backup".  That's probably OK
in some circumstances (notably, if the cluster is already down and can't
be easily revived), but I think that the documentation should be clear
that there is a trade-off.
Yup sure. Actually, for testing I use leader's db to avoid major drift. But overall agree as its little risky but can help in
 in crisis like situation if  cluster is down and we want to revert to standalone mode. Will update document likewise
as per your suggestion,
Ben Pfaff Aug. 23, 2019, 9:50 p.m. UTC | #8
On Fri, Aug 23, 2019 at 09:29:00PM +0000, Ginwala, Aliasgar wrote:
> Hi Ben:
> 
> It seems ovsdb-client backup does conversion for openvswitch databases and not OVN dbs:
> $ovsdb-client backup > standalone_db.db
> 
> When trying for ovnnb dbs, it keeps complaining about syntax error. May be I miss something
> ovsdb-client backup  /var/run/openvswitch/ovnnb_db.sock /etc/openvswitch/ovnnb_db.db > test1.db
> ovsdb-client: invalid syntax for 'backup' (use --help for help)

ovsdb-client is definitely biased in favor of the Open_vSwitch database.
It recognizes server names based on the syntax for connections, which
always includes ":", so you would need to say
"unix:/var/run/openvswitch/ovnnb_db.sock" as the server name.

> However, I checked code and it doesn’t accept args as per code below in ovsdb_client_command
> { "backup",             NEED_DATABASE, 0, 0,       do_backup },

It doesn't accept args beyond an optional server name and database name,
which NEED_DATABASE indicates.

> Also, for ovn control plane nodes, we don’t have/need to run ovs on the control plane nodes.
> As I didn’t have ovs running on my control plane nodes, it complained about the same.
> ovsdb-client: failed to connect to "unix:/var/run/openvswitch/db.sock" (No such file or directory)
> Only works if I start ovs and then run the command.
> 
> So I think current doc itself needs update if I hope I didn’t miss anything trivial.
> Let me know and I can send formal patch to correct existing doc.

If the documentation isn't clear, let's improve it.
Vishal Deep Ajmera via dev Aug. 23, 2019, 10:37 p.m. UTC | #9
Thanks Ben:

On 8/23/19, 2:50 PM, "Ben Pfaff" <blp@ovn.org> wrote:

    On Fri, Aug 23, 2019 at 09:29:00PM +0000, Ginwala, Aliasgar wrote:
    > Hi Ben:
    > 
    > It seems ovsdb-client backup does conversion for openvswitch databases and not OVN dbs:
    > $ovsdb-client backup > standalone_db.db
    > 
    > When trying for ovnnb dbs, it keeps complaining about syntax error. May be I miss something
    > ovsdb-client backup  /var/run/openvswitch/ovnnb_db.sock /etc/openvswitch/ovnnb_db.db > test1.db
    > ovsdb-client: invalid syntax for 'backup' (use --help for help)
    
    ovsdb-client is definitely biased in favor of the Open_vSwitch database.
    It recognizes server names based on the syntax for connections, which
    always includes ":", so you would need to say
    "unix:/var/run/openvswitch/ovnnb_db.sock" as the server name.
Ah! Sure. Thanks for the pointer. Description had detail default db.sock with unix as prefix. Should have read that. 
    
    > However, I checked code and it doesn’t accept args as per code below in ovsdb_client_command
    > { "backup",             NEED_DATABASE, 0, 0,       do_backup },
    
    It doesn't accept args beyond an optional server name and database name,
    which NEED_DATABASE indicates.
    
    > Also, for ovn control plane nodes, we don’t have/need to run ovs on the control plane nodes.
    > As I didn’t have ovs running on my control plane nodes, it complained about the same.
    > ovsdb-client: failed to connect to "unix:/var/run/openvswitch/db.sock" (No such file or directory)
    > Only works if I start ovs and then run the command.
    > 
    > So I think current doc itself needs update if I hope I didn’t miss anything trivial.
    > Let me know and I can send formal patch to correct existing doc.
    
    If the documentation isn't clear, let's improve it.
Cool. That makes sense. And yes since we can pass ovn db socket, we don’t really need to have ovsdb running since ovsdb-client utility connects directly to db . So clear now.
Just sent the patch https://patchwork.ozlabs.org/patch/1152462/ to correct the usage details and correction in typo in existing doc.
aginwala Aug. 26, 2019, 9:04 p.m. UTC | #10
Hi Han and Ben:
Addressed review comments from both of you in v3
https://patchwork.ozlabs.org/patch/1153456/ . PTAL




On Fri, Aug 23, 2019 at 3:37 PM Ginwala, Aliasgar <aginwala@ebay.com> wrote:

> Thanks Ben:
>
> On 8/23/19, 2:50 PM, "Ben Pfaff" <blp@ovn.org> wrote:
>
>     On Fri, Aug 23, 2019 at 09:29:00PM +0000, Ginwala, Aliasgar wrote:
>     > Hi Ben:
>     >
>     > It seems ovsdb-client backup does conversion for openvswitch
> databases and not OVN dbs:
>     > $ovsdb-client backup > standalone_db.db
>     >
>     > When trying for ovnnb dbs, it keeps complaining about syntax error.
> May be I miss something
>     > ovsdb-client backup  /var/run/openvswitch/ovnnb_db.sock
> /etc/openvswitch/ovnnb_db.db > test1.db
>     > ovsdb-client: invalid syntax for 'backup' (use --help for help)
>
>     ovsdb-client is definitely biased in favor of the Open_vSwitch
> database.
>     It recognizes server names based on the syntax for connections, which
>     always includes ":", so you would need to say
>     "unix:/var/run/openvswitch/ovnnb_db.sock" as the server name.
> Ah! Sure. Thanks for the pointer. Description had detail default db.sock
> with unix as prefix. Should have read that.
>
>     > However, I checked code and it doesn’t accept args as per code below
> in ovsdb_client_command
>     > { "backup",             NEED_DATABASE, 0, 0,       do_backup },
>
>     It doesn't accept args beyond an optional server name and database
> name,
>     which NEED_DATABASE indicates.
>
>     > Also, for ovn control plane nodes, we don’t have/need to run ovs on
> the control plane nodes.
>     > As I didn’t have ovs running on my control plane nodes, it
> complained about the same.
>     > ovsdb-client: failed to connect to
> "unix:/var/run/openvswitch/db.sock" (No such file or directory)
>     > Only works if I start ovs and then run the command.
>     >
>     > So I think current doc itself needs update if I hope I didn’t miss
> anything trivial.
>     > Let me know and I can send formal patch to correct existing doc.
>
>     If the documentation isn't clear, let's improve it.
> Cool. That makes sense. And yes since we can pass ovn db socket, we don’t
> really need to have ovsdb running since ovsdb-client utility connects
> directly to db . So clear now.
> Just sent the patch https://patchwork.ozlabs.org/patch/1152462/ to
> correct the usage details and correction in typo in existing doc.
>
>
>

Patch
diff mbox series

diff --git a/ovsdb/ovsdb-tool.c b/ovsdb/ovsdb-tool.c
index 438f97590..4aa1d4b3f 100644
--- a/ovsdb/ovsdb-tool.c
+++ b/ovsdb/ovsdb-tool.c
@@ -173,6 +173,8 @@  usage(void)
            "  compare-versions A OP B  compare OVSDB schema version numbers\n"
            "  query [DB] TRNS         execute read-only transaction on DB\n"
            "  transact [DB] TRNS      execute read/write transaction on DB\n"
+           "  migrate-cluster-db [DB [DB]]    Migrate clustered DB to\n"
+                   "standalone DB\n "
            "  [-m]... show-log [DB]   print DB's log entries\n"
            "The default DB is %s.\n"
            "The default SCHEMA is %s.\n",
@@ -206,7 +208,7 @@  default_schema(void)
     }
     return schema;
 }
-
+
 static struct json *
 parse_json(const char *s)
 {
@@ -244,7 +246,7 @@  read_standalone_schema(const char *filename)
     ovsdb_storage_close(storage);
     return schema;
 }
-
+
 static void
 do_create(struct ovs_cmdl_context *ctx)
 {
@@ -942,6 +944,94 @@  print_raft_record(const struct raft_record *r,
     }
 }
 
+static struct ovsdb_log *
+write_raft_header_to_file(const struct json *data, const char *db_file_name)
+{
+    if (!data) {
+        return NULL;
+    }
+
+    if (json_array(data)->n != 2) {
+        printf(" ***invalid data***\n");
+        return NULL;
+    }
+
+    struct ovsdb_log *log;
+    struct json *schema_json = json_array(data)->elems[0];
+    if (schema_json->type != JSON_NULL) {
+        struct ovsdb_schema *schema;
+        check_ovsdb_error(ovsdb_schema_from_json(schema_json, &schema));
+        check_ovsdb_error(ovsdb_log_open(db_file_name, OVSDB_MAGIC,
+                                     OVSDB_LOG_CREATE_EXCL, -1, &log));
+        check_ovsdb_error(ovsdb_log_write_and_free(log, schema_json));
+        check_ovsdb_error(ovsdb_log_commit_block(log));
+    }
+
+    struct json *data_json = json_array(data)->elems[1];
+    if (!data_json || data_json->type != JSON_OBJECT) {
+        return NULL;
+    }
+    if (data_json->type != JSON_NULL) {
+        check_ovsdb_error(ovsdb_log_write_and_free(log, data_json));
+        check_ovsdb_error(ovsdb_log_commit_block(log));
+    }
+    return log;
+}
+
+static struct ovsdb_log *
+write_raft_header(const struct raft_header *h, const char *db_file_name)
+{
+    if (h->snap_index) {
+        return write_raft_header_to_file(h->snap.data, db_file_name);
+    }
+    return NULL;
+}
+
+static void
+write_raft_records_to_file(const struct json *data, struct ovsdb_log *log_data)
+{
+    if (json_array(data)->n != 2) {
+        printf(" ***invalid data***\n");
+        return;
+    }
+
+    struct json *data_json = json_array(data)->elems[1];
+    if (data_json->type != JSON_NULL) {
+        check_ovsdb_error(ovsdb_log_write_and_free(log_data, data_json));
+        check_ovsdb_error(ovsdb_log_commit_block(log_data));
+    }
+}
+
+static void
+write_raft_records(const struct raft_record *r, struct ovsdb_log *log_data)
+{
+    switch (r->type) {
+    case RAFT_REC_ENTRY:
+        if (!r->entry.data) {
+            return;
+        }
+        write_raft_records_to_file(r->entry.data, log_data);
+        break;
+
+    case RAFT_REC_TERM:
+        break;
+
+    case RAFT_REC_VOTE:
+        break;
+
+    case RAFT_REC_NOTE:
+        break;
+
+    case RAFT_REC_COMMIT_INDEX:
+        break;
+
+    case RAFT_REC_LEADER:
+        break;
+    default:
+        OVS_NOT_REACHED();
+    }
+}
+
 static void
 do_show_log_cluster(struct ovsdb_log *log)
 {
@@ -1511,6 +1601,65 @@  do_compare_versions(struct ovs_cmdl_context *ctx)
     exit(result ? 0 : 2);
 }
 
+static void
+do_migrate_cluster(struct ovsdb_log *log, const char *db_file_name)
+{
+    struct ovsdb_log *log_data = NULL;
+    for (unsigned int i = 0; ; i++) {
+        struct json *json;
+        check_ovsdb_error(ovsdb_log_read(log, &json));
+        if (!json) {
+            break;
+        }
+
+        printf("record %u:\n", i);
+        struct ovsdb_error *error;
+        if (i == 0) {
+            struct raft_header h;
+            error = raft_header_from_json(&h, json);
+            if (!error) {
+                log_data = write_raft_header(&h, db_file_name);
+                raft_header_uninit(&h);
+                if (!log_data) {
+                    return;
+                }
+            }
+        } else {
+            struct raft_record r;
+            error = raft_record_from_json(&r, json);
+            if (!error) {
+                write_raft_records(&r, log_data);
+                raft_record_uninit(&r);
+            }
+        }
+        if (error) {
+            char *s = ovsdb_error_to_string_free(error);
+            puts(s);
+            free(s);
+        }
+
+        putchar('\n');
+    }
+    ovsdb_log_close(log_data);
+}
+
+static void
+do_migrate(struct ovs_cmdl_context *ctx)
+{
+    const char *db_file_name = ctx->argv[1];
+    const char *cluster_db_file_name = ctx->argv[2];
+    struct ovsdb_log *log;
+
+    check_ovsdb_error(ovsdb_log_open(cluster_db_file_name,
+                                     OVSDB_MAGIC"|"RAFT_MAGIC,
+                                     OVSDB_LOG_READ_ONLY, -1, &log));
+    if (!strcmp(ovsdb_log_get_magic(log), OVSDB_MAGIC)) {
+       printf("Data base is not clustered db.");
+    } else {
+        do_migrate_cluster(log, db_file_name);
+    }
+    ovsdb_log_close(log);
+}
 
 static void
 do_help(struct ovs_cmdl_context *ctx OVS_UNUSED)
@@ -1550,6 +1699,7 @@  static const struct ovs_cmdl_command all_commands[] = {
     { "compare-versions", "a op b", 3, 3, do_compare_versions, OVS_RO },
     { "help", NULL, 0, INT_MAX, do_help, OVS_RO },
     { "list-commands", NULL, 0, INT_MAX, do_list_commands, OVS_RO },
+    { "migrate-cluster-db", "[db [clusterdb]]", 0, 2, do_migrate, OVS_RW },
     { NULL, NULL, 0, 0, NULL, OVS_RO },
 };