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

aginwala aginwala at asu.edu
Fri Aug 23 18:11:08 UTC 2019


Thanks Han for detailed review.

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

> 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().
>
> 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 at openvswitch.org
> >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>
> >
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list