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