[ovs-dev] [PATCH v2] ovsdb-tool: Convert clustered db to standalone db.

Han Zhou zhouhan at gmail.com
Fri Aug 23 15:17:26 UTC 2019


On Thu, Aug 22, 2019 at 10:29 PM aginwala aginwala <amginwal at gmail.com>
wrote:

> Thanks for the review Han.
>
> On Thu, Aug 22, 2019 at 7:27 PM Han Zhou <zhouhan at gmail.com> wrote:
>
>> Ali, thanks for the patch. Please see my comments below.
>>
>> On Thu, Aug 22, 2019 at 5:53 PM <amginwal at gmail.com> wrote:
>> >
>> > From: Aliasgar Ginwala <aginwala at 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 at 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 at openvswitch.org
>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>


More information about the dev mailing list