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.
>
> 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?
> 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
> 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
