| Message ID | 20190823005310.99329-1-amginwal@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v2] ovsdb-tool: Convert clustered db to standalone db. | expand |
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
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 >
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 >> >
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.
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 >
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 >
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,
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.
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.
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. > > >
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 }, };