On Sun, 2021-12-19 at 15:09 +0100, Ilya Maximets wrote:
> Even though relays can be scaled to the big number of servers to
> handle a lot more clients, lack of transaction history may cause
> significant load if clients are re-connecting.
> E.g. in case of the upgrade of a large-scale OVN deployment, relays
> can be taken down one by one forcing all the clients of one relay to
> jump to other ones. And all these clients will download the database
> from scratch from a new relay.
>
> Since relay itself supports monitor_cond_since connection to the
> main cluster, it receives the last transaction id along with each
> update. Since these transaction ids are 'eid's of actual transactions,
> they can be used by relay for a transaction history.
>
> Relay may not receive all the transaction ids, because the main cluster
> may combine several changes into a single monitor update. However,
> all relays will, likely, receive same updates with the same transaction
> ids, so the case where transaction id can not be found after
> re-connection between relays should not be very common. If some id
> is missing on the relay (i.e. this update was merged with some other
> update and newer id was used) the client will just re-download the
> database as if there was a normal transaction history miss.
>
> OVSDB client synchronization module updated to provide the last
> transaction id along with the update. Relay module updated to use
> these ids as a transaction id. If ids are zero, relay decides that
> the main server doesn't support transaction ids and disables the
> transaction history accordingly.
>
> Using ovsdb_txn_replay_commit() instead of ovsdb_txn_propose_commit_block(),
> so transactions are added to the history. This can be done, because
> relays has no file storage, so there is no need to write anything.
>
> Relay tests modified to test both standalone and clustered database
> as a main server. Checks added to ensure that all servers receive the
> same transaction ids in monitor updates.
>
> Signed-off-by: Ilya Maximets <i.maxim...@ovn.org>
> ---
> NEWS | 3 +++
> lib/ovsdb-cs.c | 1 +
> lib/ovsdb-cs.h | 1 +
> ovsdb/ovsdb-server.c | 8 +++++---
> ovsdb/relay.c | 28 ++++++++++++++++++++++-----
> ovsdb/transaction.c | 9 +++++----
> tests/ovsdb-server.at | 44 +++++++++++++++++++++++++++++++------------
> 7 files changed, 70 insertions(+), 24 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index bc4a1cfac..e37ed97de 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -28,6 +28,9 @@ Post-v2.16.0
> * Default selection method for select groups with up to 256 buckets is
> now dp_hash. Previously this was limited to 64 buckets. This change
> is mainly for the benefit of OVN load balancing configurations.
> + - OVSDB:
> + * 'relay' service model now supports transaction history, i.e. honors
> the
> + 'last-txn-id' field in 'monitor_cond_since' requests from clients.
>
>
> v2.16.0 - 16 Aug 2021
> diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c
> index 2d2b77026..f9acda419 100644
> --- a/lib/ovsdb-cs.c
> +++ b/lib/ovsdb-cs.c
> @@ -1529,6 +1529,7 @@ ovsdb_cs_db_add_update(struct ovsdb_cs_db *db,
> .clear = clear,
> .monitor_reply = monitor_reply,
> .version = version,
> + .last_id = db->last_id,
> };
> }
>
> diff --git a/lib/ovsdb-cs.h b/lib/ovsdb-cs.h
> index 03bbd7ee1..5d5b58f0a 100644
> --- a/lib/ovsdb-cs.h
> +++ b/lib/ovsdb-cs.h
> @@ -99,6 +99,7 @@ struct ovsdb_cs_event {
> bool monitor_reply;
> struct json *table_updates;
> int version;
> + struct uuid last_id;
> } update;
>
> /* The "result" member from a transaction reply. The transaction is
> diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
> index 9fe90592e..b975c17fc 100644
> --- a/ovsdb/ovsdb-server.c
> +++ b/ovsdb/ovsdb-server.c
> @@ -729,9 +729,11 @@ open_db(struct server_config *config, const char
> *filename)
> db->db = ovsdb_create(schema, storage);
> ovsdb_jsonrpc_server_add_db(config->jsonrpc, db->db);
>
> - /* Enable txn history for clustered mode. It is not enabled for other
> mode
> - * for now, since txn id is available for clustered mode only. */
> - ovsdb_txn_history_init(db->db, ovsdb_storage_is_clustered(storage));
> + /* Enable txn history for clustered and relay modes. It is not enabled
> for
> + * other modes for now, since txn id is available for clustered and relay
> + * modes only. */
> + ovsdb_txn_history_init(db->db,
> + is_relay || ovsdb_storage_is_clustered(storage));
>
> read_db(config, db);
>
> diff --git a/ovsdb/relay.c b/ovsdb/relay.c
> index ef0e44d34..2df393403 100644
> --- a/ovsdb/relay.c
> +++ b/ovsdb/relay.c
> @@ -222,7 +222,8 @@ ovsdb_relay_process_row_update(struct ovsdb_table *table,
>
> static struct ovsdb_error *
> ovsdb_relay_parse_update__(struct ovsdb *db,
> - const struct ovsdb_cs_db_update *du)
> + const struct ovsdb_cs_db_update *du,
> + const struct uuid *last_id)
> {
> struct ovsdb_error *error = NULL;
> struct ovsdb_txn *txn;
> @@ -254,8 +255,17 @@ exit:
> ovsdb_txn_abort(txn);
> return error;
> } else {
> - /* Commit transaction. */
> - error = ovsdb_txn_propose_commit_block(txn, false);
> + if (uuid_is_zero(last_id)) {
> + /* The relay source doesn't support unique transaction ids,
> + * disabling transaction history for relay. */
> + ovsdb_txn_history_destroy(db);
> + ovsdb_txn_history_init(db, false);
Nit: In the case that the relay doesn't support unique transaction ids
wouldn't this cause us to have to call destroy/init on every update,
even if it were already turned off?
-M
> + } else {
> + ovsdb_txn_set_txnid(last_id, txn);
> + }
> + /* Commit transaction.
> + * There is no storage, so ovsdb_txn_replay_commit() can be used. */
> + error = ovsdb_txn_replay_commit(txn);
> }
>
> return error;
> @@ -266,6 +276,7 @@ ovsdb_relay_clear(struct ovsdb *db)
> {
> struct ovsdb_txn *txn = ovsdb_txn_create(db);
> struct shash_node *table_node;
> + struct ovsdb_error *error;
>
> SHASH_FOR_EACH (table_node, &db->tables) {
> struct ovsdb_table *table = table_node->data;
> @@ -276,7 +287,14 @@ ovsdb_relay_clear(struct ovsdb *db)
> }
> }
>
> - return ovsdb_txn_propose_commit_block(txn, false);
> + /* There is no storage, so ovsdb_txn_replay_commit() can be used. */
> + error = ovsdb_txn_replay_commit(txn);
> +
> + /* Clearing the transaction history, and re-enabling it. */
> + ovsdb_txn_history_destroy(db);
> + ovsdb_txn_history_init(db, true);
> +
> + return error;
> }
>
> static void
> @@ -304,7 +322,7 @@ ovsdb_relay_parse_update(struct relay_ctx *ctx,
> error = ovsdb_relay_clear(ctx->db);
> }
> if (!error) {
> - error = ovsdb_relay_parse_update__(ctx->db, du);
> + error = ovsdb_relay_parse_update__(ctx->db, du,
> &update->last_id);
> }
> }
> ovsdb_cs_db_update_destroy(du);
> diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c
> index db86d847c..090068603 100644
> --- a/ovsdb/transaction.c
> +++ b/ovsdb/transaction.c
> @@ -40,7 +40,7 @@ struct ovsdb_txn {
> struct ovsdb *db;
> struct ovs_list txn_tables; /* Contains "struct ovsdb_txn_table"s. */
> struct ds comment;
> - struct uuid txnid; /* For clustered mode only. It is the eid. */
> + struct uuid txnid; /* For clustered and relay modes. It is the eid. */
> size_t n_atoms; /* Number of atoms in all transaction rows. */
> ssize_t n_atoms_diff; /* Difference between number of added and
> * removed atoms. */
> @@ -1143,9 +1143,10 @@ ovsdb_txn_complete(struct ovsdb_txn *txn)
>
> /* Applies 'txn' to the internal representation of the database. This is for
> * transactions that don't need to be written to storage; probably, they came
> - * from storage. These transactions shouldn't ordinarily fail because
> storage
> - * should contain only consistent transactions. (One exception is for
> database
> - * conversion in ovsdb_convert().) */
> + * from storage or from relay. These transactions shouldn't ordinarily fail
> + * because storage should contain only consistent transactions. (One
> exception
> + * is for database conversion in ovsdb_convert().) Transactions from relay
> + * should also be consistent, since relay source should have verified them.
> */
> struct ovsdb_error * OVS_WARN_UNUSED_RESULT
> ovsdb_txn_replay_commit(struct ovsdb_txn *txn)
> {
> diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at
> index 876cb836c..a3b4051b2 100644
> --- a/tests/ovsdb-server.at
> +++ b/tests/ovsdb-server.at
> @@ -1482,11 +1482,13 @@ EXECUTION_EXAMPLES
>
> AT_BANNER([OVSDB -- ovsdb-server relay])
>
> -# OVSDB_CHECK_EXECUTION(TITLE, SCHEMA, TRANSACTIONS, OUTPUT, [KEYWORDS])
> +# OVSDB_CHECK_EXECUTION_RELAY(MODEL, TITLE, SCHEMA, TRANSACTIONS,
> +# OUTPUT, [KEYWORDS])
> #
> -# Creates a database with the given SCHEMA and starts an ovsdb-server on
> -# it. Also starts a daisy chain of ovsdb-servers in relay mode where the
> -# first relay server is connected to the main non-relay ovsdb-server.
> +# Creates a clustered or standalone (MODEL) database with the given SCHEMA
> +# and starts an ovsdb-server on it. Also starts a daisy chain of
> +# ovsdb-servers in relay mode where the first relay server is connected to
> +# the main non-relay ovsdb-server.
> #
> # Runs each of the TRANSACTIONS (which should be a quoted list of
> # quoted strings) against one of relay servers in the middle with
> @@ -1508,17 +1510,21 @@ AT_BANNER([OVSDB -- ovsdb-server relay])
> # If a given UUID appears more than once it is always replaced by the
> # same marker.
> #
> -# Checks that the dump of all databases is the same.
> +# Checks that the dump of all databases and transaction ids are the same.
> #
> # TITLE is provided to AT_SETUP and KEYWORDS to AT_KEYWORDS.
> -m4_define([OVSDB_CHECK_EXECUTION],
> - [AT_SETUP([$1])
> - AT_KEYWORDS([ovsdb server tcp relay $5])
> +m4_define([OVSDB_CHECK_EXECUTION_RELAY],
> + [AT_SETUP([$2 - relay - $1])
> + AT_KEYWORDS([ovsdb server tcp relay $6 $1])
> n_servers=6
> target=4
> - $2 > schema
> + $3 > schema
> schema_name=`ovsdb-tool schema-name schema`
> - AT_CHECK([ovsdb-tool create db1 schema], [0], [stdout], [ignore])
> + AT_CHECK([if test $1 = standalone; then
> + ovsdb-tool create db1 schema
> + else
> + ovsdb-tool create-cluster db1 schema unix:s1.raft
> + fi], [0], [stdout], [ignore])
>
> on_exit 'kill `cat *.pid`'
> AT_CHECK([ovsdb-server --detach --no-chdir --log-file=ovsdb-server1.log
> dnl
> @@ -1534,13 +1540,13 @@ m4_define([OVSDB_CHECK_EXECUTION],
> ], [0], [ignore], [ignore])
> done
>
> - m4_foreach([txn], [$3],
> + m4_foreach([txn], [$4],
> [AT_CHECK([ovsdb-client transact unix:db${target}.sock 'txn'], [0],
> [stdout], [ignore])
> cat stdout >> output
> ])
>
> - AT_CHECK([uuidfilt output], [0], [$4], [ignore])
> + AT_CHECK([uuidfilt output], [0], [$5], [ignore])
>
> AT_CHECK([ovsdb-client dump unix:db1.sock], [0], [stdout], [ignore])
> for i in $(seq 2 ${n_servers}); do
> @@ -1548,12 +1554,26 @@ m4_define([OVSDB_CHECK_EXECUTION],
> diff stdout dump${i}])
> done
>
> + dnl Check that transaction ids in notifications are the same on all
> relays.
> + last_id_pattern='s/\(.*"monid","[[a-z]]*".,"\)\(.*\)\(",{".*\)/\2/'
> + AT_CHECK([grep 'received notification, method="update3"'
> ovsdb-server2.log dnl
> + | sed $last_id_pattern > txn_ids2])
> + for i in $(seq 3 ${n_servers}); do
> + AT_CHECK([grep 'received notification, method="update3"'
> ovsdb-server$i.log dnl
> + | sed $last_id_pattern > txn_ids$i])
> + AT_CHECK([diff txn_ids2 txn_ids$i])
> + done
> +
> OVSDB_SERVER_SHUTDOWN
> for i in $(seq 2 ${n_servers}); do
> OVSDB_SERVER_SHUTDOWN_N([$i])
> done
> AT_CLEANUP])
>
> +m4_define([OVSDB_CHECK_EXECUTION],
> + [OVSDB_CHECK_EXECUTION_RELAY(standalone, $@)
> + OVSDB_CHECK_EXECUTION_RELAY(clustered, $@)])
> +
> EXECUTION_EXAMPLES
>
> AT_BANNER([OVSDB -- ovsdb-server replication])
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
