| Message ID | 20230327194301.3773536-4-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Commit | 4d6cdd8e0d86d2b3b6866aaacf327d8c5e7092df |
| Headers | show |
| Series | ovsdb: conversion: Bug fixes & Optimizations. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Mon, Mar 27, 2023 at 09:42:58PM +0200, Ilya Maximets wrote: > If the schema with no data was read from the clustered storage, it > should mean a database conversion request. In general, we can get: > > 1. Just data --> Transaction record. > 2. Schema + Data --> Database conversion or raft snapshot install. > 3. Just schema --> New. Database conversion request. > > We cannot distinguish between conversion and snapshot installation > request in the current implementation, so we will keep handling > conversion with data in the same way as before, i.e. if data is > provided, we should use it. > > ovsdb-tool is updated to handle this record type as well while > converting cluster to standalone. > > This change doesn't introduce a way for such records to appear in > the database. That will be added in the future commits targeting > conversion speed increase. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> Reviewed-by: Simon Horman <simon.horman@corigine.com>
On 3/27/23 21:42, Ilya Maximets wrote: > If the schema with no data was read from the clustered storage, it > should mean a database conversion request. In general, we can get: > > 1. Just data --> Transaction record. > 2. Schema + Data --> Database conversion or raft snapshot install. > 3. Just schema --> New. Database conversion request. > > We cannot distinguish between conversion and snapshot installation > request in the current implementation, so we will keep handling > conversion with data in the same way as before, i.e. if data is > provided, we should use it. > > ovsdb-tool is updated to handle this record type as well while > converting cluster to standalone. > > This change doesn't introduce a way for such records to appear in > the database. That will be added in the future commits targeting > conversion speed increase. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > ovsdb/ovsdb-server.c | 65 ++++++++++++++++++++++++++++++-------------- > ovsdb/ovsdb-tool.c | 35 ++++++++++++++++++------ > ovsdb/relay.c | 20 +++++++++++--- > ovsdb/relay.h | 7 +++-- > 4 files changed, 91 insertions(+), 36 deletions(-) > > diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c > index 4fea2dbda..91c284e99 100644 > --- a/ovsdb/ovsdb-server.c > +++ b/ovsdb/ovsdb-server.c > @@ -573,8 +573,9 @@ close_db(struct server_config *config, struct db *db, char *comment) > } > } > > -static void > -update_schema(struct ovsdb *db, const struct ovsdb_schema *schema, void *aux) > +static struct ovsdb_error * OVS_WARN_UNUSED_RESULT > +update_schema(struct ovsdb *db, const struct ovsdb_schema *schema, > + bool conversion_with_no_data, void *aux) > { > struct server_config *config = aux; > > @@ -586,13 +587,27 @@ update_schema(struct ovsdb *db, const struct ovsdb_schema *schema, void *aux) > : xasprintf("database %s connected to storage", db->name))); > } > > - ovsdb_replace(db, ovsdb_create(ovsdb_schema_clone(schema), NULL)); > + if (db->schema && conversion_with_no_data) { > + struct ovsdb *new_db = NULL; > + struct ovsdb_error *error; > + > + error = ovsdb_convert(db, schema, &new_db); > + if (error) { > + /* Should never happen, because conversion should have been > + * checked before writing the schema to the storage. */ > + return error; > + } > + ovsdb_replace(db, new_db); > + } else { > + ovsdb_replace(db, ovsdb_create(ovsdb_schema_clone(schema), NULL)); > + } > > /* Force update to schema in _Server database. */ > struct db *dbp = shash_find_data(config->all_dbs, db->name); > if (dbp) { > dbp->row_uuid = UUID_ZERO; > } > + return NULL; > } > > static struct ovsdb_error * OVS_WARN_UNUSED_RESULT > @@ -600,23 +615,30 @@ parse_txn(struct server_config *config, struct db *db, > const struct ovsdb_schema *schema, const struct json *txn_json, > const struct uuid *txnid) > { > + struct ovsdb_error *error = NULL; > + struct ovsdb_txn *txn = NULL; > + > if (schema) { > - /* We're replacing the schema (and the data). Destroy the database > - * (first grabbing its storage), then replace it with the new schema. > - * The transaction must also include the replacement data. > + /* We're replacing the schema (and the data). If transaction includes > + * replacement data, destroy the database (first grabbing its storage), > + * then replace it with the new schema. If not, it's a conversion > + * without data specified. In this case, convert the current database > + * to a new schema instead. > * > * Only clustered database schema changes and snapshot installs > * go through this path. > */ > - ovs_assert(txn_json); > ovs_assert(ovsdb_storage_is_clustered(db->db->storage)); > > - struct ovsdb_error *error = ovsdb_schema_check_for_ephemeral_columns( > - schema); > + error = ovsdb_schema_check_for_ephemeral_columns(schema); > + if (error) { > + return error; > + } > + > + error = update_schema(db->db, schema, txn_json == NULL, config); > if (error) { > return error; > } > - update_schema(db->db, schema, config); > } > > if (txn_json) { > @@ -624,24 +646,25 @@ parse_txn(struct server_config *config, struct db *db, > return ovsdb_error(NULL, "%s: data without schema", db->filename); > } > > - struct ovsdb_txn *txn; > - struct ovsdb_error *error; > - > error = ovsdb_file_txn_from_json(db->db, txn_json, false, &txn); > - if (!error) { > - ovsdb_txn_set_txnid(txnid, txn); > - log_and_free_error(ovsdb_txn_replay_commit(txn)); > - } > - if (!error && !uuid_is_zero(txnid)) { > - db->db->prereq = *txnid; > - } > if (error) { > ovsdb_storage_unread(db->db->storage); > return error; > } > + } else if (schema) { > + /* We just performed conversion without data. Transaction history > + * was destroyed. Commit a dummy transaction to set the txnid. */ > + txn = ovsdb_txn_create(db->db); > } > > - return NULL; > + if (txn) { > + ovsdb_txn_set_txnid(txnid, txn); > + error = ovsdb_txn_replay_commit(txn); > + if (!error && !uuid_is_zero(txnid)) { > + db->db->prereq = *txnid; > + } > + } > + return error; > } > > static void > diff --git a/ovsdb/ovsdb-tool.c b/ovsdb/ovsdb-tool.c > index ea2b75b46..e26536532 100644 > --- a/ovsdb/ovsdb-tool.c > +++ b/ovsdb/ovsdb-tool.c > @@ -1006,7 +1006,8 @@ raft_header_to_standalone_log(const struct raft_header *h, > } > > static void > -raft_record_to_standalone_log(const struct raft_record *r, > +raft_record_to_standalone_log(const char *db_file_name, > + const struct raft_record *r, > struct ovsdb_log *db_log_data) > { > if (r->type == RAFT_REC_ENTRY) { > @@ -1024,15 +1025,30 @@ raft_record_to_standalone_log(const struct raft_record *r, > > if (schema_json->type != JSON_NULL) { > /* This is a database conversion record. Reset the log and > - * write the new schema. Data JSON should also be part of > - * the conversion. */ > + * write the new schema. */ > struct ovsdb_schema *schema; > > + check_ovsdb_error(ovsdb_schema_from_json(schema_json, &schema)); > + > if (data_json->type == JSON_NULL) { > - ovs_fatal( > - 0, "Invalid database conversion in the log: no data"); > + /* We have a conversion request with no data. There is no > + * other way as to read back what we have and convert. */ > + struct ovsdb *old_db, *new_db; > + > + check_ovsdb_error(ovsdb_log_commit_block(db_log_data)); > + > + old_db = ovsdb_file_read(db_file_name, false); > + check_ovsdb_error(ovsdb_convert(old_db, schema, &new_db)); > + ovsdb_destroy(old_db); > + > + pa->elems[1] = ovsdb_to_txn_json( > + new_db, "converted by ovsdb-tool", true); > + ovsdb_destroy(new_db); > + > + json_destroy(data_json); > + data_json = pa->elems[1]; > } > - check_ovsdb_error(ovsdb_schema_from_json(schema_json, &schema)); > + > ovsdb_schema_destroy(schema); > check_ovsdb_error(ovsdb_log_reset(db_log_data)); > check_ovsdb_error(ovsdb_log_write(db_log_data, schema_json)); > @@ -1654,7 +1670,8 @@ do_compare_versions(struct ovs_cmdl_context *ctx) > } > > static void > -do_convert_to_standalone(struct ovsdb_log *log, struct ovsdb_log *db_log_data) > +do_convert_to_standalone(const char *db_file_name, > + struct ovsdb_log *log, struct ovsdb_log *db_log_data) > { > for (unsigned int i = 0; ; i++) { > struct json *json; > @@ -1671,7 +1688,7 @@ do_convert_to_standalone(struct ovsdb_log *log, struct ovsdb_log *db_log_data) > } else { > struct raft_record r; > check_ovsdb_error(raft_record_from_json(&r, json)); > - raft_record_to_standalone_log(&r, db_log_data); > + raft_record_to_standalone_log(db_file_name, &r, db_log_data); > raft_record_uninit(&r); > } > json_destroy(json); > @@ -1694,7 +1711,7 @@ do_cluster_standalone(struct ovs_cmdl_context *ctx) > if (strcmp(ovsdb_log_get_magic(log), RAFT_MAGIC) != 0) { > ovs_fatal(0, "Database is not clustered db.\n"); > } > - do_convert_to_standalone(log, db_log_data); > + do_convert_to_standalone(db_file_name, log, db_log_data); > check_ovsdb_error(ovsdb_log_commit_block(db_log_data)); > ovsdb_log_close(db_log_data); > ovsdb_log_close(log); > diff --git a/ovsdb/relay.c b/ovsdb/relay.c > index 9ff6ed8f3..4a00f0a0a 100644 > --- a/ovsdb/relay.c > +++ b/ovsdb/relay.c > @@ -301,6 +301,8 @@ static void > ovsdb_relay_parse_update(struct relay_ctx *ctx, > const struct ovsdb_cs_update_event *update) > { > + struct ovsdb_error *error = NULL; > + > if (!ctx->db) { > return; > } > @@ -308,15 +310,25 @@ ovsdb_relay_parse_update(struct relay_ctx *ctx, > if (update->monitor_reply && ctx->new_schema) { > /* There was a schema change. Updating a database with a new schema > * before processing monitor reply with the new data. */ > - ctx->schema_change_cb(ctx->db, ctx->new_schema, > - ctx->schema_change_aux); > + error = ctx->schema_change_cb(ctx->db, ctx->new_schema, false, > + ctx->schema_change_aux); > + if (error) { > + /* Should never happen, but handle this case anyway. */ > + char *s = ovsdb_error_to_string_free(error); > + VLOG_ERR("%s", s); Should this be VLOG_ERR_RL? Regardless: Acked-by: Dumitru Ceara <dceara@redhat.com> Thanks!
diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c index 4fea2dbda..91c284e99 100644 --- a/ovsdb/ovsdb-server.c +++ b/ovsdb/ovsdb-server.c @@ -573,8 +573,9 @@ close_db(struct server_config *config, struct db *db, char *comment) } } -static void -update_schema(struct ovsdb *db, const struct ovsdb_schema *schema, void *aux) +static struct ovsdb_error * OVS_WARN_UNUSED_RESULT +update_schema(struct ovsdb *db, const struct ovsdb_schema *schema, + bool conversion_with_no_data, void *aux) { struct server_config *config = aux; @@ -586,13 +587,27 @@ update_schema(struct ovsdb *db, const struct ovsdb_schema *schema, void *aux) : xasprintf("database %s connected to storage", db->name))); } - ovsdb_replace(db, ovsdb_create(ovsdb_schema_clone(schema), NULL)); + if (db->schema && conversion_with_no_data) { + struct ovsdb *new_db = NULL; + struct ovsdb_error *error; + + error = ovsdb_convert(db, schema, &new_db); + if (error) { + /* Should never happen, because conversion should have been + * checked before writing the schema to the storage. */ + return error; + } + ovsdb_replace(db, new_db); + } else { + ovsdb_replace(db, ovsdb_create(ovsdb_schema_clone(schema), NULL)); + } /* Force update to schema in _Server database. */ struct db *dbp = shash_find_data(config->all_dbs, db->name); if (dbp) { dbp->row_uuid = UUID_ZERO; } + return NULL; } static struct ovsdb_error * OVS_WARN_UNUSED_RESULT @@ -600,23 +615,30 @@ parse_txn(struct server_config *config, struct db *db, const struct ovsdb_schema *schema, const struct json *txn_json, const struct uuid *txnid) { + struct ovsdb_error *error = NULL; + struct ovsdb_txn *txn = NULL; + if (schema) { - /* We're replacing the schema (and the data). Destroy the database - * (first grabbing its storage), then replace it with the new schema. - * The transaction must also include the replacement data. + /* We're replacing the schema (and the data). If transaction includes + * replacement data, destroy the database (first grabbing its storage), + * then replace it with the new schema. If not, it's a conversion + * without data specified. In this case, convert the current database + * to a new schema instead. * * Only clustered database schema changes and snapshot installs * go through this path. */ - ovs_assert(txn_json); ovs_assert(ovsdb_storage_is_clustered(db->db->storage)); - struct ovsdb_error *error = ovsdb_schema_check_for_ephemeral_columns( - schema); + error = ovsdb_schema_check_for_ephemeral_columns(schema); + if (error) { + return error; + } + + error = update_schema(db->db, schema, txn_json == NULL, config); if (error) { return error; } - update_schema(db->db, schema, config); } if (txn_json) { @@ -624,24 +646,25 @@ parse_txn(struct server_config *config, struct db *db, return ovsdb_error(NULL, "%s: data without schema", db->filename); } - struct ovsdb_txn *txn; - struct ovsdb_error *error; - error = ovsdb_file_txn_from_json(db->db, txn_json, false, &txn); - if (!error) { - ovsdb_txn_set_txnid(txnid, txn); - log_and_free_error(ovsdb_txn_replay_commit(txn)); - } - if (!error && !uuid_is_zero(txnid)) { - db->db->prereq = *txnid; - } if (error) { ovsdb_storage_unread(db->db->storage); return error; } + } else if (schema) { + /* We just performed conversion without data. Transaction history + * was destroyed. Commit a dummy transaction to set the txnid. */ + txn = ovsdb_txn_create(db->db); } - return NULL; + if (txn) { + ovsdb_txn_set_txnid(txnid, txn); + error = ovsdb_txn_replay_commit(txn); + if (!error && !uuid_is_zero(txnid)) { + db->db->prereq = *txnid; + } + } + return error; } static void diff --git a/ovsdb/ovsdb-tool.c b/ovsdb/ovsdb-tool.c index ea2b75b46..e26536532 100644 --- a/ovsdb/ovsdb-tool.c +++ b/ovsdb/ovsdb-tool.c @@ -1006,7 +1006,8 @@ raft_header_to_standalone_log(const struct raft_header *h, } static void -raft_record_to_standalone_log(const struct raft_record *r, +raft_record_to_standalone_log(const char *db_file_name, + const struct raft_record *r, struct ovsdb_log *db_log_data) { if (r->type == RAFT_REC_ENTRY) { @@ -1024,15 +1025,30 @@ raft_record_to_standalone_log(const struct raft_record *r, if (schema_json->type != JSON_NULL) { /* This is a database conversion record. Reset the log and - * write the new schema. Data JSON should also be part of - * the conversion. */ + * write the new schema. */ struct ovsdb_schema *schema; + check_ovsdb_error(ovsdb_schema_from_json(schema_json, &schema)); + if (data_json->type == JSON_NULL) { - ovs_fatal( - 0, "Invalid database conversion in the log: no data"); + /* We have a conversion request with no data. There is no + * other way as to read back what we have and convert. */ + struct ovsdb *old_db, *new_db; + + check_ovsdb_error(ovsdb_log_commit_block(db_log_data)); + + old_db = ovsdb_file_read(db_file_name, false); + check_ovsdb_error(ovsdb_convert(old_db, schema, &new_db)); + ovsdb_destroy(old_db); + + pa->elems[1] = ovsdb_to_txn_json( + new_db, "converted by ovsdb-tool", true); + ovsdb_destroy(new_db); + + json_destroy(data_json); + data_json = pa->elems[1]; } - check_ovsdb_error(ovsdb_schema_from_json(schema_json, &schema)); + ovsdb_schema_destroy(schema); check_ovsdb_error(ovsdb_log_reset(db_log_data)); check_ovsdb_error(ovsdb_log_write(db_log_data, schema_json)); @@ -1654,7 +1670,8 @@ do_compare_versions(struct ovs_cmdl_context *ctx) } static void -do_convert_to_standalone(struct ovsdb_log *log, struct ovsdb_log *db_log_data) +do_convert_to_standalone(const char *db_file_name, + struct ovsdb_log *log, struct ovsdb_log *db_log_data) { for (unsigned int i = 0; ; i++) { struct json *json; @@ -1671,7 +1688,7 @@ do_convert_to_standalone(struct ovsdb_log *log, struct ovsdb_log *db_log_data) } else { struct raft_record r; check_ovsdb_error(raft_record_from_json(&r, json)); - raft_record_to_standalone_log(&r, db_log_data); + raft_record_to_standalone_log(db_file_name, &r, db_log_data); raft_record_uninit(&r); } json_destroy(json); @@ -1694,7 +1711,7 @@ do_cluster_standalone(struct ovs_cmdl_context *ctx) if (strcmp(ovsdb_log_get_magic(log), RAFT_MAGIC) != 0) { ovs_fatal(0, "Database is not clustered db.\n"); } - do_convert_to_standalone(log, db_log_data); + do_convert_to_standalone(db_file_name, log, db_log_data); check_ovsdb_error(ovsdb_log_commit_block(db_log_data)); ovsdb_log_close(db_log_data); ovsdb_log_close(log); diff --git a/ovsdb/relay.c b/ovsdb/relay.c index 9ff6ed8f3..4a00f0a0a 100644 --- a/ovsdb/relay.c +++ b/ovsdb/relay.c @@ -301,6 +301,8 @@ static void ovsdb_relay_parse_update(struct relay_ctx *ctx, const struct ovsdb_cs_update_event *update) { + struct ovsdb_error *error = NULL; + if (!ctx->db) { return; } @@ -308,15 +310,25 @@ ovsdb_relay_parse_update(struct relay_ctx *ctx, if (update->monitor_reply && ctx->new_schema) { /* There was a schema change. Updating a database with a new schema * before processing monitor reply with the new data. */ - ctx->schema_change_cb(ctx->db, ctx->new_schema, - ctx->schema_change_aux); + error = ctx->schema_change_cb(ctx->db, ctx->new_schema, false, + ctx->schema_change_aux); + if (error) { + /* Should never happen, but handle this case anyway. */ + char *s = ovsdb_error_to_string_free(error); + VLOG_ERR("%s", s); + free(s); + + ovsdb_cs_flag_inconsistency(ctx->cs); + return; + } ovsdb_schema_destroy(ctx->new_schema); ctx->new_schema = NULL; } struct ovsdb_cs_db_update *du; - struct ovsdb_error *error = ovsdb_cs_parse_db_update(update->table_updates, - update->version, &du); + + error = ovsdb_cs_parse_db_update(update->table_updates, + update->version, &du); if (!error) { if (update->clear) { error = ovsdb_relay_clear(ctx->db); diff --git a/ovsdb/relay.h b/ovsdb/relay.h index 390ea70c8..2d66b5e5f 100644 --- a/ovsdb/relay.h +++ b/ovsdb/relay.h @@ -23,8 +23,11 @@ struct json; struct ovsdb; struct ovsdb_schema; -typedef void (*schema_change_callback)(struct ovsdb *, - const struct ovsdb_schema *, void *aux); +typedef struct ovsdb_error *(*schema_change_callback)( + struct ovsdb *, + const struct ovsdb_schema *, + bool conversion_with_no_data, + void *aux); void ovsdb_relay_add_db(struct ovsdb *, const char *remote, schema_change_callback schema_change_cb,
If the schema with no data was read from the clustered storage, it should mean a database conversion request. In general, we can get: 1. Just data --> Transaction record. 2. Schema + Data --> Database conversion or raft snapshot install. 3. Just schema --> New. Database conversion request. We cannot distinguish between conversion and snapshot installation request in the current implementation, so we will keep handling conversion with data in the same way as before, i.e. if data is provided, we should use it. ovsdb-tool is updated to handle this record type as well while converting cluster to standalone. This change doesn't introduce a way for such records to appear in the database. That will be added in the future commits targeting conversion speed increase. Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- ovsdb/ovsdb-server.c | 65 ++++++++++++++++++++++++++++++-------------- ovsdb/ovsdb-tool.c | 35 ++++++++++++++++++------ ovsdb/relay.c | 20 +++++++++++--- ovsdb/relay.h | 7 +++-- 4 files changed, 91 insertions(+), 36 deletions(-)