Thanks Han for detailed review. On Fri, Aug 23, 2019 at 8:17 AM Han Zhou <[email protected]> wrote:
> On Thu, Aug 22, 2019 at 10:29 PM aginwala aginwala <[email protected]> > wrote: > > > Thanks for the review Han. > > > > On Thu, Aug 22, 2019 at 7:27 PM Han Zhou <[email protected]> wrote: > > > >> Ali, thanks for the patch. Please see my comments below. > >> > >> On Thu, Aug 22, 2019 at 5:53 PM <[email protected]> wrote: > >> > > >> > From: Aliasgar Ginwala <[email protected]> > >> > > >> > 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 <[email protected]> > >> > --- > >> > 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 > >> > [email protected] > >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
