On Sun, Dec 19, 2021 at 6:09 AM Ilya Maximets <i.maxim...@ovn.org> 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); > + } 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 > +
Thanks Ilya! Shall we ensure that the last_id is not zero when the server1 is cluster mode? Otherwise it looks good to me. Acked-by: Han Zhou <hz...@ovn.org> > 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]) > -- > 2.31.1 > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev