| Message ID | 20211219140941.2279071-4-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Commit | a3e97b1af1bdcaa802c6caa9e73087df7077d2b1 |
| Headers | show |
| Series | ovsdb: Bug fixes + Relay txn history and performance. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
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.maximets@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])
On 1/7/22 21:41, Mike Pattrick wrote: >> 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? Yes, it will. But that should not be a big problem here, because ovsdb_txn_history_destroy() is just one check in that case, and init should not be heavy either. It's only ~5 memory writes and that should be negligible in compare with the actual processing of the received database update. We could add a separate API call like this: bool ovsdb_need_txn_history(struct ovsdb *db) { return db->need_txn_history; } and check, but it's kind of seems unnecessary for this use case. (I would like to not use db->need_txn_history directly here, even though we technically have access to it.) What do you think? Best regards, Ilya Maximets.
On Fri, Jan 21, 2022 at 1:03 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 1/7/22 21:41, Mike Pattrick wrote: > >> 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? > > Yes, it will. But that should not be a big problem here, because > ovsdb_txn_history_destroy() is just one check in that case, and > init should not be heavy either. It's only ~5 memory writes and > that should be negligible in compare with the actual processing of > the received database update. I think you're right that this shouldn't really impact performance. Acked-by: Mike Pattrick <mkp@redhat.com> > > We could add a separate API call like this: > > bool > ovsdb_need_txn_history(struct ovsdb *db) > { > return db->need_txn_history; > } > > and check, but it's kind of seems unnecessary for this use case. > (I would like to not use db->need_txn_history directly here, even > though we technically have access to it.) > > What do you think? > > Best regards, Ilya Maximets. >
On Sun, Dec 19, 2021 at 6:09 AM Ilya Maximets <i.maximets@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.maximets@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 <hzhou@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 >
On 1/25/22 18:47, Han Zhou wrote: > > > On Sun, Dec 19, 2021 at 6:09 AM Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@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.maximets@ovn.org <mailto:i.maximets@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 <http://ovsdb-server.at> | 44 +++++++++++++++++++++++++++++++------------ >> 7 files changed, 70 insertions(+), 24 deletions(-) >> <snip> >> @@ -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 <hzhou@ovn.org <mailto:hzhou@ovn.org>> Thanks, Mike an Han! I added the check for the transaction ids to be non-zero in the clustered case and applied the patch. Best regards, Ilya Maximets.
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 + 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])
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.maximets@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(-)