| Message ID | a17ddfd19abea266f665c15dbdfb0b98a944787b.1773736007.git.felix.huettner@digits.schwarz |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev,v2,1/2] ovsdb: Support custom transaction history size. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 3/17/26 9:28 AM, Felix Huettner via dev wrote: > Previously the transaction history was hard limited to 100 entries. > However in fast changing environments (e.g. with 20 transactions/sec) > this means that transactions are only in the history for a few seconds. > If now a client reconnects and tries to use monitor_cond_since it has > only a short timeframe where this will reliably work. > > We make the history limit configurable here so that use can choose the *user > speed vs memory tradeoff as needed. > We still keep the limit based on size of the history, as syncing a > history larger than the actual database size does not make sense in any > way. > > Signed-off-by: Felix Huettner <felix.huettner@digits.schwarz> > --- Hi, Felix. Thanks or the patch! And sorry for delay (lots of random security stuff lately...). I was thinking about this problem for along time but didn't make a patch so far, but the problem definitely needs some solution. One other use case here beside the basic transaction rate is when you run a sync command and every ovn-controller sends a tiny transaction to update the sequence number. In a large cluster the entire history will be overwritten multiple times within a few seconds. However, what I was thinking is maybe we should just drop the static history limit and always rely on the dynamic one. Have you tried this approach? Or do you see the configured history limit being hit after you increase it? If so, is the memory consumption significantly different than just allowing it to grow up to the database size? Just trying to see if we can drop the limit and avoid extra config knobs. I saw people hacking the code to set the limit to 5000 in practice, but I'm not sure if this value is even reachable in the normal operation, or is it always capped by the database size in the end anyway. See some small comments for the current patch below. > v1->v2: handle setting if the config does not specify the service-model. > > ovsdb/ovsdb-server.c | 29 +++++++++++++++++++++++++++-- > ovsdb/ovsdb.h | 1 + > ovsdb/relay.c | 4 ++-- > ovsdb/transaction.c | 33 +++++++++++++++++++++++++++++++-- > ovsdb/transaction.h | 5 ++++- > tests/ovsdb-server.at | 22 +++++++++++++++++++++- > 6 files changed, 86 insertions(+), 8 deletions(-) > > diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c > index 7c3a5ef11..7d8562cb2 100644 > --- a/ovsdb/ovsdb-server.c > +++ b/ovsdb/ovsdb-server.c > @@ -156,6 +156,11 @@ struct db_config { > bool backup; /* If true, the database is read-only and receives > * updates from the 'source'. */ > } ab; > + > + /* Valid for SM_CLUSTERED or SM_RELAY. */ > + unsigned int n_txn_history_max; /* The maximum amount of history entries > + * to keep. 0 means default. */ > + nit: The empty line here is not needed. > }; > > struct db { > @@ -462,6 +467,7 @@ db_config_clone(const struct db_config *c) > conf->options = ovsdb_jsonrpc_options_clone(c->options); > } > conf->ab.sync_exclude = nullable_xstrdup(c->ab.sync_exclude); > + conf->n_txn_history_max = c->n_txn_history_max; > > return conf; > } > @@ -572,6 +578,12 @@ database_update_config(struct server_config *server_config, > replication_set_db(db->db, conf->source, conf->ab.sync_exclude, > server_uuid, &conf->options->rpc); > } > + > + if ((conf->model == SM_CLUSTERED || conf->model == SM_RELAY) && > + conf->n_txn_history_max) { > + ovsdb_txn_history_update(db->db, conf->n_txn_history_max); > + nit: Extra empty line. > + } > } > > static bool > @@ -1180,7 +1192,8 @@ open_db(struct server_config *server_config, > /* 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, model == SM_RELAY || model == SM_CLUSTERED); > + ovsdb_txn_history_init(db->db, model == SM_RELAY || model == SM_CLUSTERED, > + conf->n_txn_history_max); > > read_db(server_config, db); > > @@ -2889,6 +2902,12 @@ db_config_to_json(const struct db_config *conf) > } > json_object_put(json, "backup", json_boolean_create(conf->ab.backup)); > } > + > + if (conf->n_txn_history_max) { > + json_object_put(json, "transaction-history-limit", > + json_integer_create(conf->n_txn_history_max)); > + } > + > return json; > } > > @@ -3012,7 +3031,7 @@ remotes_from_json(struct shash *remotes, const struct json *json) > static struct db_config * > db_config_from_json(const char *name, const struct json *json) > { > - const struct json *model, *source, *sync_exclude, *backup; > + const struct json *model, *source, *sync_exclude, *backup, *txn_limit; > struct db_config *conf = xzalloc(sizeof *conf); > struct ovsdb_parser parser; > struct ovsdb_error *error; > @@ -3093,6 +3112,12 @@ db_config_from_json(const char *name, const struct json *json) > } > } > > + txn_limit = ovsdb_parser_member(&parser, "transaction-history-limit", > + OP_INTEGER | OP_OPTIONAL); > + if (txn_limit) { > + conf->n_txn_history_max = json_integer(txn_limit); > + } > + > error = ovsdb_parser_finish(&parser); > if (error) { > char *s = ovsdb_error_to_string_free(error); > diff --git a/ovsdb/ovsdb.h b/ovsdb/ovsdb.h > index 325900bc6..a6a65dc1b 100644 > --- a/ovsdb/ovsdb.h > +++ b/ovsdb/ovsdb.h > @@ -110,6 +110,7 @@ struct ovsdb { > bool need_txn_history; /* Need to maintain history of transactions. */ > unsigned int n_txn_history; /* Current number of history transactions. */ > unsigned int n_txn_history_atoms; /* Total number of atoms in history. */ > + unsigned int n_txn_history_max; /* The maximum history length. */ > struct ovs_list txn_history; /* Contains "struct ovsdb_txn_history_node. */ > > size_t n_atoms; /* Total number of ovsdb atoms in the database. */ > diff --git a/ovsdb/relay.c b/ovsdb/relay.c > index a5e1a5f3a..7691e0b73 100644 > --- a/ovsdb/relay.c > +++ b/ovsdb/relay.c > @@ -274,7 +274,7 @@ exit: > /* 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); > + ovsdb_txn_history_init(db, false, 0); > } else { > ovsdb_txn_set_txnid(last_id, txn); > } > @@ -307,7 +307,7 @@ ovsdb_relay_clear(struct ovsdb *db) > > /* Clearing the transaction history, and re-enabling it. */ > ovsdb_txn_history_destroy(db); > - ovsdb_txn_history_init(db, true); > + ovsdb_txn_history_init(db, true, db->n_txn_history_max); > > return error; > } > diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c > index 0d0d27b61..7ebb8245f 100644 > --- a/ovsdb/transaction.c > +++ b/ovsdb/transaction.c > @@ -18,6 +18,7 @@ > #include "transaction.h" > > #include "bitmap.h" > +#include "coverage.h" > #include "openvswitch/dynamic-string.h" > #include "file.h" > #include "hash.h" > @@ -37,6 +38,9 @@ > #include "util.h" > > VLOG_DEFINE_THIS_MODULE(transaction); nit: Would add an empty line here. > +COVERAGE_DEFINE(transaction_history_add); Maybe worth abbreviating into 'txn'? We use this in idl for example, and shorter names are rendered better. > + > +#define TRANSACTION_HISTORY_LIMIT_DEFAULT 100 > > struct ovsdb_txn { > struct ovsdb *db; > @@ -1192,6 +1196,7 @@ ovsdb_txn_add_to_history(struct ovsdb_txn *txn) > ovs_list_push_back(&txn->db->txn_history, &node->node); > txn->db->n_txn_history++; > txn->db->n_txn_history_atoms += txn->n_atoms; > + COVERAGE_INC(transaction_history_add); > } > } > > @@ -1686,7 +1691,7 @@ ovsdb_txn_history_run(struct ovsdb *db) > * Keeping at least one transaction to avoid sending UUID_ZERO as a last id > * if all entries got removed due to the size limit. */ > while (db->n_txn_history > 1 && > - (db->n_txn_history > 100 || > + (db->n_txn_history > db->n_txn_history_max || > db->n_txn_history_atoms > db->n_atoms)) { > struct ovsdb_txn_history_node *txn_h_node = CONTAINER_OF( > ovs_list_pop_front(&db->txn_history), > @@ -1700,11 +1705,20 @@ ovsdb_txn_history_run(struct ovsdb *db) > } > > void > -ovsdb_txn_history_init(struct ovsdb *db, bool need_txn_history) > +ovsdb_txn_history_init(struct ovsdb *db, bool need_txn_history, > + unsigned int n_txn_history_max) > { > db->need_txn_history = need_txn_history; I wonder if we need this variable. If we're setting the max according to the service model anyway, we should be able to just check if it's zero or not. Right? > db->n_txn_history = 0; > db->n_txn_history_atoms = 0; > + db->n_txn_history_max = n_txn_history_max != 0 ? n_txn_history_max : > + TRANSACTION_HISTORY_LIMIT_DEFAULT; > + if (db->need_txn_history && db->n_txn_history_max < 2) { I'd say the 100 should be the default and the minimum, then we could just take the max between the user-configured value and the 100. > + VLOG_WARN("transaction history needed, but limit too low. " > + "%u < 2. Setting it to 2.", > + n_txn_history_max); > + db->n_txn_history_max = 2; > + } > ovs_list_init(&db->txn_history); > } > > @@ -1725,3 +1739,18 @@ ovsdb_txn_history_destroy(struct ovsdb *db) > db->n_txn_history = 0; > db->n_txn_history_atoms = 0; > } > + > +void > +ovsdb_txn_history_update(struct ovsdb *db, unsigned int n_txn_history_max) > +{ > + db->n_txn_history_max = n_txn_history_max != 0 ? n_txn_history_max : > + TRANSACTION_HISTORY_LIMIT_DEFAULT; > + if (db->need_txn_history && db->n_txn_history_max < 2) { > + VLOG_WARN("transaction history needed, but limit too low. " > + "%u < 2. Setting it to 2.", > + n_txn_history_max); > + db->n_txn_history_max = 2; > + } This code block is repeated twice, maybe worth moving into a function. > + > + ovsdb_txn_history_run(db); > +} > diff --git a/ovsdb/transaction.h b/ovsdb/transaction.h > index d94205414..04f96f079 100644 > --- a/ovsdb/transaction.h > +++ b/ovsdb/transaction.h > @@ -75,7 +75,10 @@ struct ovsdb_row *ovsdb_index_search(struct hmap *index, > void ovsdb_txn_add_comment(struct ovsdb_txn *, const char *); > const char *ovsdb_txn_get_comment(const struct ovsdb_txn *); > void ovsdb_txn_history_run(struct ovsdb *); > -void ovsdb_txn_history_init(struct ovsdb *, bool need_txn_history); > +void ovsdb_txn_history_init(struct ovsdb *, bool need_txn_history, > + unsigned int n_txn_history_max); > void ovsdb_txn_history_destroy(struct ovsdb *); > +void ovsdb_txn_history_update(struct ovsdb *db, > + unsigned int n_txn_history_max); > > #endif /* ovsdb/transaction.h */ > diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at > index d9389e12f..921339dfa 100644 > --- a/tests/ovsdb-server.at > +++ b/tests/ovsdb-server.at > @@ -1937,7 +1937,8 @@ m4_define([OVSDB_CHECK_EXECUTION_RELAY], > \"databases\": { > \"${schema_name}\": { > \"service-model\": \"relay\", > - \"source\": { \"unix:db$((i-1)).sock\": {} } > + \"source\": { \"unix:db$((i-1)).sock\": {} }, > + \"transaction-history-limit\": 1234 Not sure if we need this. Seems better to test with the default here. We have the special history limit test, might be better to adjust it or have a separate one. > } > } > }" > config${i}.json > @@ -3108,6 +3109,25 @@ WARN|syntax "{"dscp":42,"inactivity-probe":10000,"max-backoff":8000,"role":"My-R > syntax error: Parsing JSON-RPC options failed: Member 'role' is present but not allowed here. > ]) > > +TEST_CONFIG_FILE([relay with transaction-history-limit], [ > +{ > + "remotes": { "punix:db.sock": {} }, > + "databases": { > + "RelaySchema": { > + "service-model": "relay", > + "source": { > + "punix:db2.sock": { > + "inactivity-probe": 10000, > + "max-backoff": 8000, > + "dscp": 42 > + } > + }, > + "transaction-history-limit": 1000 > + } > + } > +} > +], [0]) Should probably update the "complex config" test as well. We'll also need some documentation updates in Documentation/ref/ovsdb.7.rst and ovsdb/ovsdb-server.1.in. And a NEWS entry. Best regards, Ilya Maximets.
Am Tue, Apr 07, 2026 at 10:36:19PM +0200 schrieb Ilya Maximets: > On 3/17/26 9:28 AM, Felix Huettner via dev wrote: > > Previously the transaction history was hard limited to 100 entries. > > However in fast changing environments (e.g. with 20 transactions/sec) > > this means that transactions are only in the history for a few seconds. > > If now a client reconnects and tries to use monitor_cond_since it has > > only a short timeframe where this will reliably work. > > > > We make the history limit configurable here so that use can choose the > > *user > > > speed vs memory tradeoff as needed. > > We still keep the limit based on size of the history, as syncing a > > history larger than the actual database size does not make sense in any > > way. > > > > Signed-off-by: Felix Huettner <felix.huettner@digits.schwarz> > > --- > > Hi, Felix. Thanks or the patch! And sorry for delay (lots of random > security stuff lately...). > > I was thinking about this problem for along time but didn't make a patch > so far, but the problem definitely needs some solution. > > One other use case here beside the basic transaction rate is when you > run a sync command and every ovn-controller sends a tiny transaction > to update the sequence number. In a large cluster the entire history > will be overwritten multiple times within a few seconds. > > However, what I was thinking is maybe we should just drop the static > history limit and always rely on the dynamic one. Have you tried this > approach? Or do you see the configured history limit being hit after > you increase it? If so, is the memory consumption significantly > different than just allowing it to grow up to the database size? > > Just trying to see if we can drop the limit and avoid extra config knobs. > I saw people hacking the code to set the limit to 5000 in practice, but > I'm not sure if this value is even reachable in the normal operation, or > is it always capped by the database size in the end anyway. Hi Ilya, thanks for the feedback. So just to get some rough numbers from one of our production southbound clusters: * atoms: 23_255_133 * txn-history: 100 * txn-history-atoms: 100_196 So we are currently at roughly 1000 atoms per history entry. This would mean that we could get up to roughly 23_000 transation history entries. At our current rate of roughly 20 transactions/sec this would last us for around 19 minutes. I am not sure how well a "struct ovs_list" works with so many entries :) So for me this equals the question if it still makes sense to be able to reconnect after 15 minutes and then get all the updates that happened in the mean time. My feeling would be that reconnects generally should be in the few seconds to maybe a minute category. If something takes longer it most probably was broken in one way or another. My first idea of this change had actually been to allow the user to specify a duration for the transaction history. This would be more in line with the actual needs that i see. I forgot by now why i did not implement it that way, but that would be the other option i currently see. Removing the cap alltogether would definately be nicer, but i'm really concerned about the health of it :) What would be your opinion on that? Thanks a lof, Felix > > See some small comments for the current patch below. > > > v1->v2: handle setting if the config does not specify the service-model. > > > > ovsdb/ovsdb-server.c | 29 +++++++++++++++++++++++++++-- > > ovsdb/ovsdb.h | 1 + > > ovsdb/relay.c | 4 ++-- > > ovsdb/transaction.c | 33 +++++++++++++++++++++++++++++++-- > > ovsdb/transaction.h | 5 ++++- > > tests/ovsdb-server.at | 22 +++++++++++++++++++++- > > 6 files changed, 86 insertions(+), 8 deletions(-) > > > > diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c > > index 7c3a5ef11..7d8562cb2 100644 > > --- a/ovsdb/ovsdb-server.c > > +++ b/ovsdb/ovsdb-server.c > > @@ -156,6 +156,11 @@ struct db_config { > > bool backup; /* If true, the database is read-only and receives > > * updates from the 'source'. */ > > } ab; > > + > > + /* Valid for SM_CLUSTERED or SM_RELAY. */ > > + unsigned int n_txn_history_max; /* The maximum amount of history entries > > + * to keep. 0 means default. */ > > + > > nit: The empty line here is not needed. > > > }; > > > > struct db { > > @@ -462,6 +467,7 @@ db_config_clone(const struct db_config *c) > > conf->options = ovsdb_jsonrpc_options_clone(c->options); > > } > > conf->ab.sync_exclude = nullable_xstrdup(c->ab.sync_exclude); > > + conf->n_txn_history_max = c->n_txn_history_max; > > > > return conf; > > } > > @@ -572,6 +578,12 @@ database_update_config(struct server_config *server_config, > > replication_set_db(db->db, conf->source, conf->ab.sync_exclude, > > server_uuid, &conf->options->rpc); > > } > > + > > + if ((conf->model == SM_CLUSTERED || conf->model == SM_RELAY) && > > + conf->n_txn_history_max) { > > + ovsdb_txn_history_update(db->db, conf->n_txn_history_max); > > + > > nit: Extra empty line. > > > + } > > } > > > > static bool > > @@ -1180,7 +1192,8 @@ open_db(struct server_config *server_config, > > /* 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, model == SM_RELAY || model == SM_CLUSTERED); > > + ovsdb_txn_history_init(db->db, model == SM_RELAY || model == SM_CLUSTERED, > > + conf->n_txn_history_max); > > > > read_db(server_config, db); > > > > @@ -2889,6 +2902,12 @@ db_config_to_json(const struct db_config *conf) > > } > > json_object_put(json, "backup", json_boolean_create(conf->ab.backup)); > > } > > + > > + if (conf->n_txn_history_max) { > > + json_object_put(json, "transaction-history-limit", > > + json_integer_create(conf->n_txn_history_max)); > > + } > > + > > return json; > > } > > > > @@ -3012,7 +3031,7 @@ remotes_from_json(struct shash *remotes, const struct json *json) > > static struct db_config * > > db_config_from_json(const char *name, const struct json *json) > > { > > - const struct json *model, *source, *sync_exclude, *backup; > > + const struct json *model, *source, *sync_exclude, *backup, *txn_limit; > > struct db_config *conf = xzalloc(sizeof *conf); > > struct ovsdb_parser parser; > > struct ovsdb_error *error; > > @@ -3093,6 +3112,12 @@ db_config_from_json(const char *name, const struct json *json) > > } > > } > > > > + txn_limit = ovsdb_parser_member(&parser, "transaction-history-limit", > > + OP_INTEGER | OP_OPTIONAL); > > + if (txn_limit) { > > + conf->n_txn_history_max = json_integer(txn_limit); > > + } > > + > > error = ovsdb_parser_finish(&parser); > > if (error) { > > char *s = ovsdb_error_to_string_free(error); > > diff --git a/ovsdb/ovsdb.h b/ovsdb/ovsdb.h > > index 325900bc6..a6a65dc1b 100644 > > --- a/ovsdb/ovsdb.h > > +++ b/ovsdb/ovsdb.h > > @@ -110,6 +110,7 @@ struct ovsdb { > > bool need_txn_history; /* Need to maintain history of transactions. */ > > unsigned int n_txn_history; /* Current number of history transactions. */ > > unsigned int n_txn_history_atoms; /* Total number of atoms in history. */ > > + unsigned int n_txn_history_max; /* The maximum history length. */ > > struct ovs_list txn_history; /* Contains "struct ovsdb_txn_history_node. */ > > > > size_t n_atoms; /* Total number of ovsdb atoms in the database. */ > > diff --git a/ovsdb/relay.c b/ovsdb/relay.c > > index a5e1a5f3a..7691e0b73 100644 > > --- a/ovsdb/relay.c > > +++ b/ovsdb/relay.c > > @@ -274,7 +274,7 @@ exit: > > /* 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); > > + ovsdb_txn_history_init(db, false, 0); > > } else { > > ovsdb_txn_set_txnid(last_id, txn); > > } > > @@ -307,7 +307,7 @@ ovsdb_relay_clear(struct ovsdb *db) > > > > /* Clearing the transaction history, and re-enabling it. */ > > ovsdb_txn_history_destroy(db); > > - ovsdb_txn_history_init(db, true); > > + ovsdb_txn_history_init(db, true, db->n_txn_history_max); > > > > return error; > > } > > diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c > > index 0d0d27b61..7ebb8245f 100644 > > --- a/ovsdb/transaction.c > > +++ b/ovsdb/transaction.c > > @@ -18,6 +18,7 @@ > > #include "transaction.h" > > > > #include "bitmap.h" > > +#include "coverage.h" > > #include "openvswitch/dynamic-string.h" > > #include "file.h" > > #include "hash.h" > > @@ -37,6 +38,9 @@ > > #include "util.h" > > > > VLOG_DEFINE_THIS_MODULE(transaction); > > nit: Would add an empty line here. > > > +COVERAGE_DEFINE(transaction_history_add); > > Maybe worth abbreviating into 'txn'? We use this in idl for example, > and shorter names are rendered better. > > > + > > +#define TRANSACTION_HISTORY_LIMIT_DEFAULT 100 > > > > struct ovsdb_txn { > > struct ovsdb *db; > > @@ -1192,6 +1196,7 @@ ovsdb_txn_add_to_history(struct ovsdb_txn *txn) > > ovs_list_push_back(&txn->db->txn_history, &node->node); > > txn->db->n_txn_history++; > > txn->db->n_txn_history_atoms += txn->n_atoms; > > + COVERAGE_INC(transaction_history_add); > > } > > } > > > > @@ -1686,7 +1691,7 @@ ovsdb_txn_history_run(struct ovsdb *db) > > * Keeping at least one transaction to avoid sending UUID_ZERO as a last id > > * if all entries got removed due to the size limit. */ > > while (db->n_txn_history > 1 && > > - (db->n_txn_history > 100 || > > + (db->n_txn_history > db->n_txn_history_max || > > db->n_txn_history_atoms > db->n_atoms)) { > > struct ovsdb_txn_history_node *txn_h_node = CONTAINER_OF( > > ovs_list_pop_front(&db->txn_history), > > @@ -1700,11 +1705,20 @@ ovsdb_txn_history_run(struct ovsdb *db) > > } > > > > void > > -ovsdb_txn_history_init(struct ovsdb *db, bool need_txn_history) > > +ovsdb_txn_history_init(struct ovsdb *db, bool need_txn_history, > > + unsigned int n_txn_history_max) > > { > > db->need_txn_history = need_txn_history; > > I wonder if we need this variable. If we're setting the max according > to the service model anyway, we should be able to just check if it's > zero or not. Right? > > > db->n_txn_history = 0; > > db->n_txn_history_atoms = 0; > > + db->n_txn_history_max = n_txn_history_max != 0 ? n_txn_history_max : > > + TRANSACTION_HISTORY_LIMIT_DEFAULT; > > + if (db->need_txn_history && db->n_txn_history_max < 2) { > > I'd say the 100 should be the default and the minimum, then we could just > take the max between the user-configured value and the 100. > > > + VLOG_WARN("transaction history needed, but limit too low. " > > + "%u < 2. Setting it to 2.", > > + n_txn_history_max); > > + db->n_txn_history_max = 2; > > + } > > ovs_list_init(&db->txn_history); > > } > > > > @@ -1725,3 +1739,18 @@ ovsdb_txn_history_destroy(struct ovsdb *db) > > db->n_txn_history = 0; > > db->n_txn_history_atoms = 0; > > } > > + > > +void > > +ovsdb_txn_history_update(struct ovsdb *db, unsigned int n_txn_history_max) > > +{ > > + db->n_txn_history_max = n_txn_history_max != 0 ? n_txn_history_max : > > + TRANSACTION_HISTORY_LIMIT_DEFAULT; > > + if (db->need_txn_history && db->n_txn_history_max < 2) { > > + VLOG_WARN("transaction history needed, but limit too low. " > > + "%u < 2. Setting it to 2.", > > + n_txn_history_max); > > + db->n_txn_history_max = 2; > > + } > > This code block is repeated twice, maybe worth moving into a function. > > > + > > + ovsdb_txn_history_run(db); > > +} > > diff --git a/ovsdb/transaction.h b/ovsdb/transaction.h > > index d94205414..04f96f079 100644 > > --- a/ovsdb/transaction.h > > +++ b/ovsdb/transaction.h > > @@ -75,7 +75,10 @@ struct ovsdb_row *ovsdb_index_search(struct hmap *index, > > void ovsdb_txn_add_comment(struct ovsdb_txn *, const char *); > > const char *ovsdb_txn_get_comment(const struct ovsdb_txn *); > > void ovsdb_txn_history_run(struct ovsdb *); > > -void ovsdb_txn_history_init(struct ovsdb *, bool need_txn_history); > > +void ovsdb_txn_history_init(struct ovsdb *, bool need_txn_history, > > + unsigned int n_txn_history_max); > > void ovsdb_txn_history_destroy(struct ovsdb *); > > +void ovsdb_txn_history_update(struct ovsdb *db, > > + unsigned int n_txn_history_max); > > > > #endif /* ovsdb/transaction.h */ > > diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at > > index d9389e12f..921339dfa 100644 > > --- a/tests/ovsdb-server.at > > +++ b/tests/ovsdb-server.at > > @@ -1937,7 +1937,8 @@ m4_define([OVSDB_CHECK_EXECUTION_RELAY], > > \"databases\": { > > \"${schema_name}\": { > > \"service-model\": \"relay\", > > - \"source\": { \"unix:db$((i-1)).sock\": {} } > > + \"source\": { \"unix:db$((i-1)).sock\": {} }, > > + \"transaction-history-limit\": 1234 > > Not sure if we need this. Seems better to test with the default here. > We have the special history limit test, might be better to adjust it > or have a separate one. > > > } > > } > > }" > config${i}.json > > @@ -3108,6 +3109,25 @@ WARN|syntax "{"dscp":42,"inactivity-probe":10000,"max-backoff":8000,"role":"My-R > > syntax error: Parsing JSON-RPC options failed: Member 'role' is present but not allowed here. > > ]) > > > > +TEST_CONFIG_FILE([relay with transaction-history-limit], [ > > +{ > > + "remotes": { "punix:db.sock": {} }, > > + "databases": { > > + "RelaySchema": { > > + "service-model": "relay", > > + "source": { > > + "punix:db2.sock": { > > + "inactivity-probe": 10000, > > + "max-backoff": 8000, > > + "dscp": 42 > > + } > > + }, > > + "transaction-history-limit": 1000 > > + } > > + } > > +} > > +], [0]) > > Should probably update the "complex config" test as well. > > We'll also need some documentation updates in Documentation/ref/ovsdb.7.rst > and ovsdb/ovsdb-server.1.in. And a NEWS entry. > > Best regards, Ilya Maximets.
diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c index 7c3a5ef11..7d8562cb2 100644 --- a/ovsdb/ovsdb-server.c +++ b/ovsdb/ovsdb-server.c @@ -156,6 +156,11 @@ struct db_config { bool backup; /* If true, the database is read-only and receives * updates from the 'source'. */ } ab; + + /* Valid for SM_CLUSTERED or SM_RELAY. */ + unsigned int n_txn_history_max; /* The maximum amount of history entries + * to keep. 0 means default. */ + }; struct db { @@ -462,6 +467,7 @@ db_config_clone(const struct db_config *c) conf->options = ovsdb_jsonrpc_options_clone(c->options); } conf->ab.sync_exclude = nullable_xstrdup(c->ab.sync_exclude); + conf->n_txn_history_max = c->n_txn_history_max; return conf; } @@ -572,6 +578,12 @@ database_update_config(struct server_config *server_config, replication_set_db(db->db, conf->source, conf->ab.sync_exclude, server_uuid, &conf->options->rpc); } + + if ((conf->model == SM_CLUSTERED || conf->model == SM_RELAY) && + conf->n_txn_history_max) { + ovsdb_txn_history_update(db->db, conf->n_txn_history_max); + + } } static bool @@ -1180,7 +1192,8 @@ open_db(struct server_config *server_config, /* 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, model == SM_RELAY || model == SM_CLUSTERED); + ovsdb_txn_history_init(db->db, model == SM_RELAY || model == SM_CLUSTERED, + conf->n_txn_history_max); read_db(server_config, db); @@ -2889,6 +2902,12 @@ db_config_to_json(const struct db_config *conf) } json_object_put(json, "backup", json_boolean_create(conf->ab.backup)); } + + if (conf->n_txn_history_max) { + json_object_put(json, "transaction-history-limit", + json_integer_create(conf->n_txn_history_max)); + } + return json; } @@ -3012,7 +3031,7 @@ remotes_from_json(struct shash *remotes, const struct json *json) static struct db_config * db_config_from_json(const char *name, const struct json *json) { - const struct json *model, *source, *sync_exclude, *backup; + const struct json *model, *source, *sync_exclude, *backup, *txn_limit; struct db_config *conf = xzalloc(sizeof *conf); struct ovsdb_parser parser; struct ovsdb_error *error; @@ -3093,6 +3112,12 @@ db_config_from_json(const char *name, const struct json *json) } } + txn_limit = ovsdb_parser_member(&parser, "transaction-history-limit", + OP_INTEGER | OP_OPTIONAL); + if (txn_limit) { + conf->n_txn_history_max = json_integer(txn_limit); + } + error = ovsdb_parser_finish(&parser); if (error) { char *s = ovsdb_error_to_string_free(error); diff --git a/ovsdb/ovsdb.h b/ovsdb/ovsdb.h index 325900bc6..a6a65dc1b 100644 --- a/ovsdb/ovsdb.h +++ b/ovsdb/ovsdb.h @@ -110,6 +110,7 @@ struct ovsdb { bool need_txn_history; /* Need to maintain history of transactions. */ unsigned int n_txn_history; /* Current number of history transactions. */ unsigned int n_txn_history_atoms; /* Total number of atoms in history. */ + unsigned int n_txn_history_max; /* The maximum history length. */ struct ovs_list txn_history; /* Contains "struct ovsdb_txn_history_node. */ size_t n_atoms; /* Total number of ovsdb atoms in the database. */ diff --git a/ovsdb/relay.c b/ovsdb/relay.c index a5e1a5f3a..7691e0b73 100644 --- a/ovsdb/relay.c +++ b/ovsdb/relay.c @@ -274,7 +274,7 @@ exit: /* 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); + ovsdb_txn_history_init(db, false, 0); } else { ovsdb_txn_set_txnid(last_id, txn); } @@ -307,7 +307,7 @@ ovsdb_relay_clear(struct ovsdb *db) /* Clearing the transaction history, and re-enabling it. */ ovsdb_txn_history_destroy(db); - ovsdb_txn_history_init(db, true); + ovsdb_txn_history_init(db, true, db->n_txn_history_max); return error; } diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c index 0d0d27b61..7ebb8245f 100644 --- a/ovsdb/transaction.c +++ b/ovsdb/transaction.c @@ -18,6 +18,7 @@ #include "transaction.h" #include "bitmap.h" +#include "coverage.h" #include "openvswitch/dynamic-string.h" #include "file.h" #include "hash.h" @@ -37,6 +38,9 @@ #include "util.h" VLOG_DEFINE_THIS_MODULE(transaction); +COVERAGE_DEFINE(transaction_history_add); + +#define TRANSACTION_HISTORY_LIMIT_DEFAULT 100 struct ovsdb_txn { struct ovsdb *db; @@ -1192,6 +1196,7 @@ ovsdb_txn_add_to_history(struct ovsdb_txn *txn) ovs_list_push_back(&txn->db->txn_history, &node->node); txn->db->n_txn_history++; txn->db->n_txn_history_atoms += txn->n_atoms; + COVERAGE_INC(transaction_history_add); } } @@ -1686,7 +1691,7 @@ ovsdb_txn_history_run(struct ovsdb *db) * Keeping at least one transaction to avoid sending UUID_ZERO as a last id * if all entries got removed due to the size limit. */ while (db->n_txn_history > 1 && - (db->n_txn_history > 100 || + (db->n_txn_history > db->n_txn_history_max || db->n_txn_history_atoms > db->n_atoms)) { struct ovsdb_txn_history_node *txn_h_node = CONTAINER_OF( ovs_list_pop_front(&db->txn_history), @@ -1700,11 +1705,20 @@ ovsdb_txn_history_run(struct ovsdb *db) } void -ovsdb_txn_history_init(struct ovsdb *db, bool need_txn_history) +ovsdb_txn_history_init(struct ovsdb *db, bool need_txn_history, + unsigned int n_txn_history_max) { db->need_txn_history = need_txn_history; db->n_txn_history = 0; db->n_txn_history_atoms = 0; + db->n_txn_history_max = n_txn_history_max != 0 ? n_txn_history_max : + TRANSACTION_HISTORY_LIMIT_DEFAULT; + if (db->need_txn_history && db->n_txn_history_max < 2) { + VLOG_WARN("transaction history needed, but limit too low. " + "%u < 2. Setting it to 2.", + n_txn_history_max); + db->n_txn_history_max = 2; + } ovs_list_init(&db->txn_history); } @@ -1725,3 +1739,18 @@ ovsdb_txn_history_destroy(struct ovsdb *db) db->n_txn_history = 0; db->n_txn_history_atoms = 0; } + +void +ovsdb_txn_history_update(struct ovsdb *db, unsigned int n_txn_history_max) +{ + db->n_txn_history_max = n_txn_history_max != 0 ? n_txn_history_max : + TRANSACTION_HISTORY_LIMIT_DEFAULT; + if (db->need_txn_history && db->n_txn_history_max < 2) { + VLOG_WARN("transaction history needed, but limit too low. " + "%u < 2. Setting it to 2.", + n_txn_history_max); + db->n_txn_history_max = 2; + } + + ovsdb_txn_history_run(db); +} diff --git a/ovsdb/transaction.h b/ovsdb/transaction.h index d94205414..04f96f079 100644 --- a/ovsdb/transaction.h +++ b/ovsdb/transaction.h @@ -75,7 +75,10 @@ struct ovsdb_row *ovsdb_index_search(struct hmap *index, void ovsdb_txn_add_comment(struct ovsdb_txn *, const char *); const char *ovsdb_txn_get_comment(const struct ovsdb_txn *); void ovsdb_txn_history_run(struct ovsdb *); -void ovsdb_txn_history_init(struct ovsdb *, bool need_txn_history); +void ovsdb_txn_history_init(struct ovsdb *, bool need_txn_history, + unsigned int n_txn_history_max); void ovsdb_txn_history_destroy(struct ovsdb *); +void ovsdb_txn_history_update(struct ovsdb *db, + unsigned int n_txn_history_max); #endif /* ovsdb/transaction.h */ diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at index d9389e12f..921339dfa 100644 --- a/tests/ovsdb-server.at +++ b/tests/ovsdb-server.at @@ -1937,7 +1937,8 @@ m4_define([OVSDB_CHECK_EXECUTION_RELAY], \"databases\": { \"${schema_name}\": { \"service-model\": \"relay\", - \"source\": { \"unix:db$((i-1)).sock\": {} } + \"source\": { \"unix:db$((i-1)).sock\": {} }, + \"transaction-history-limit\": 1234 } } }" > config${i}.json @@ -3108,6 +3109,25 @@ WARN|syntax "{"dscp":42,"inactivity-probe":10000,"max-backoff":8000,"role":"My-R syntax error: Parsing JSON-RPC options failed: Member 'role' is present but not allowed here. ]) +TEST_CONFIG_FILE([relay with transaction-history-limit], [ +{ + "remotes": { "punix:db.sock": {} }, + "databases": { + "RelaySchema": { + "service-model": "relay", + "source": { + "punix:db2.sock": { + "inactivity-probe": 10000, + "max-backoff": 8000, + "dscp": 42 + } + }, + "transaction-history-limit": 1000 + } + } +} +], [0]) + TEST_CONFIG_FILE([unknown config], [ { "remotes": { "punix:db.sock": {} },
Previously the transaction history was hard limited to 100 entries. However in fast changing environments (e.g. with 20 transactions/sec) this means that transactions are only in the history for a few seconds. If now a client reconnects and tries to use monitor_cond_since it has only a short timeframe where this will reliably work. We make the history limit configurable here so that use can choose the speed vs memory tradeoff as needed. We still keep the limit based on size of the history, as syncing a history larger than the actual database size does not make sense in any way. Signed-off-by: Felix Huettner <felix.huettner@digits.schwarz> --- v1->v2: handle setting if the config does not specify the service-model. ovsdb/ovsdb-server.c | 29 +++++++++++++++++++++++++++-- ovsdb/ovsdb.h | 1 + ovsdb/relay.c | 4 ++-- ovsdb/transaction.c | 33 +++++++++++++++++++++++++++++++-- ovsdb/transaction.h | 5 ++++- tests/ovsdb-server.at | 22 +++++++++++++++++++++- 6 files changed, 86 insertions(+), 8 deletions(-) base-commit: a8b0e4cab94f57bc414b20b4af43c7c5a800cf6c