diff mbox series

[ovs-dev] ovsdb-server: Use time based retention for the transaction history.

Message ID 20260930120738.3828621-1-tomasz@walaszek.net
State New
Delegated to: Ilya Maximets
Headers show
Series [ovs-dev] ovsdb-server: Use time based retention for the transaction history. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_FreeBSD_Build_and_Test success github build: passed

Commit Message

Tomasz Wałaszek Sept. 30, 2026, 12:07 p.m. UTC 
Currently, the transaction history keeps a fixed number of transactions,
hardcoded to 100.  On a busy system this may cover only a short period
of time, so a client that disconnects has a high chance of not finding
its last transaction in the history, which leads to a download of the
whole database.

This patch replaces the fixed number of transactions with a time based
limit.  Transactions are removed from the history once they are older
than the limit, regardless of how many of them are in the history. The
limit defaults to 60 seconds and can be changed with a new database
configuration option 'transaction-history-time-limit' for clustered and
relay databases.  The value is in seconds.

The limit on the number of atoms, which prevents the transaction history
from growing larger than the database itself, still applies.

Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2026-April/431593.html
Signed-off-by: Tomasz Wałaszek <tomasz@walaszek.net>
---
 ovsdb/ovsdb-server.c  | 44 +++++++++++++++++++++++++++-
 ovsdb/ovsdb.h         |  3 ++
 ovsdb/transaction.c   | 25 +++++++++++++---
 ovsdb/transaction.h   |  2 ++
 tests/ovsdb-server.at | 68 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 137 insertions(+), 5 deletions(-)

Comments

Aaron Conole Sept. 30, 2026, 5:50 p.m. UTC | #1 
Tomasz Wałaszek via dev <ovs-dev@openvswitch.org> writes:

> Currently, the transaction history keeps a fixed number of transactions,
> hardcoded to 100.  On a busy system this may cover only a short period
> of time, so a client that disconnects has a high chance of not finding
> its last transaction in the history, which leads to a download of the
> whole database.
>
> This patch replaces the fixed number of transactions with a time based
> limit.  Transactions are removed from the history once they are older
> than the limit, regardless of how many of them are in the history. The
> limit defaults to 60 seconds and can be changed with a new database
> configuration option 'transaction-history-time-limit' for clustered and
> relay databases.  The value is in seconds.
>
> The limit on the number of atoms, which prevents the transaction history
> from growing larger than the database itself, still applies.
>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2026-April/431593.html
> Signed-off-by: Tomasz Wałaszek <tomasz@walaszek.net>
> ---

[ ... ]

> @@ -1725,3 +1734,11 @@ 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, long long int txn_history_time_max)
> +{
> +    ovs_assert(txn_history_time_max >= 0);
> +    db->txn_history_time_max = txn_history_time_max;
> +    ovsdb_txn_history_run(db);

An AI generated this comment, and it looked relevant to a human reviewer
so it is forwarded here.

Concern (defensive): `ovsdb_txn_history_init()` does not initialize
`txn_history_time_max`; it relies on `xzalloc` in `ovsdb_create()`
leaving it 0 and on callers calling `ovsdb_txn_history_update()`. Today
every path that enables history goes through ovsdb-server's
`open_db()`/`database_update_config()` (verified: relay.c:310 re-enable
keeps the already-set value, `ovsdb_replace()` only destroys nodes), so
there is no live bug. But a 0 value means "expire everything older than
this millisecond", i.e., history collapses to one entry — a future
caller of `ovsdb_txn_history_init(db, true)` that forgets the update
call would get silently degenerate behavior. Setting a sane default in
`init()` (or documenting the dependency) would be safer. Low severity.
Tomasz Wałaszek Oct. 1, 2026, 11:03 p.m. UTC | #2 
Thanks Aaron, that is a valid point.

As the AI review suggested, currently there is no path that would lead to silently
dropping the history. 


The problem might occur in the future if someone would call ovsdb_txn_history_init
with history enabled (need_txn_history sets to true) and forgot to call 

ovsdb_txn_history_update. That would leave txn_history_time_max at 0 and shrink 

the history to a single entry.

To solve this potential problem we could - as the AI review suggested - add this
to the documentation or change the ovsdb_txn_history_init function to

void
ovsdb_txn_history_init(struct ovsdb *db, bool need_txn_history,
                       long long int txn_history_time_max)

The downside of this solution is that a caller must provide a history time limit 

even if the need_txn_history is set to false.

I have the v2 patch with changed ovsdb_txn_history_init but I will wait for the general
review first.

Thanks,
Tomasz

środa, 30 września 2026 19:50, Aaron Conole <aconole@ovn.org> napisał(a):

> Tomasz Wałaszek via dev <ovs-dev@openvswitch.org> writes:
>
> > Currently, the transaction history keeps a fixed number of transactions,
> > hardcoded to 100.  On a busy system this may cover only a short period
> > of time, so a client that disconnects has a high chance of not finding
> > its last transaction in the history, which leads to a download of the
> > whole database.
> >
> > This patch replaces the fixed number of transactions with a time based
> > limit.  Transactions are removed from the history once they are older
> > than the limit, regardless of how many of them are in the history. The
> > limit defaults to 60 seconds and can be changed with a new database
> > configuration option 'transaction-history-time-limit' for clustered and
> > relay databases.  The value is in seconds.
> >
> > The limit on the number of atoms, which prevents the transaction history
> > from growing larger than the database itself, still applies.
> >
> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2026-April/431593.html
> > Signed-off-by: Tomasz Wałaszek <tomasz@walaszek.net>
> > ---
>
> [ ... ]
>
> > @@ -1725,3 +1734,11 @@ 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, long long int txn_history_time_max)
> > +{
> > +    ovs_assert(txn_history_time_max >= 0);
> > +    db->txn_history_time_max = txn_history_time_max;
> > +    ovsdb_txn_history_run(db);
>
> An AI generated this comment, and it looked relevant to a human reviewer
> so it is forwarded here.
>
> Concern (defensive): `ovsdb_txn_history_init()` does not initialize
> `txn_history_time_max`; it relies on `xzalloc` in `ovsdb_create()`
> leaving it 0 and on callers calling `ovsdb_txn_history_update()`. Today
> every path that enables history goes through ovsdb-server's
> `open_db()`/`database_update_config()` (verified: relay.c:310 re-enable
> keeps the already-set value, `ovsdb_replace()` only destroys nodes), so
> there is no live bug. But a 0 value means "expire everything older than
> this millisecond", i.e., history collapses to one entry — a future
> caller of `ovsdb_txn_history_init(db, true)` that forgets the update
> call would get silently degenerate behavior. Setting a sane default in
> `init()` (or documenting the dependency) would be safer. Low severity.
>
diff mbox series

Patch

diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index af217476f..2b765686a 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -156,8 +156,15 @@  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. */
+    long long int txn_history_time_max; /* Keep history entries younger than
+                                         * this many seconds. */
 };
 
+/* Default value for 'txn_history_time_max', in seconds. */
+#define TXN_HISTORY_TIME_MAX_DEFAULT 60
+
 struct db {
     struct ovsdb *db;
     char *filename;
@@ -459,6 +466,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->txn_history_time_max = c->txn_history_time_max;
 
     return conf;
 }
@@ -486,6 +494,8 @@  add_database_config(struct shash *db_conf, const char *opt,
     struct db_config *conf = xzalloc(sizeof *conf);
     char *filename = NULL;
 
+    conf->txn_history_time_max = TXN_HISTORY_TIME_MAX_DEFAULT;
+
     if (parse_relay_args(opt, &filename, &conf->source)) {
         conf->model = SM_RELAY;
         conf->options = get_jsonrpc_options(conf->source, conf->model);
@@ -569,6 +579,11 @@  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) {
+        ovsdb_txn_history_update(db->db,
+                                 conf->txn_history_time_max * 1000);
+    }
 }
 
 static bool
@@ -1175,6 +1190,10 @@  open_db(struct server_config *server_config,
      * modes only. */
     ovsdb_txn_history_init(db->db, model == SM_RELAY || model == SM_CLUSTERED);
 
+    if (model == SM_RELAY || model == SM_CLUSTERED) {
+        ovsdb_txn_history_update(db->db, conf->txn_history_time_max * 1000);
+    }
+
     read_db(server_config, db);
 
     error = (db->db->name[0] == '_'
@@ -2901,6 +2920,12 @@  db_config_to_json(const struct db_config *conf)
         }
         json_object_put(json, "backup", json_boolean_create(conf->ab.backup));
     }
+
+    if (conf->txn_history_time_max != TXN_HISTORY_TIME_MAX_DEFAULT) {
+        json_object_put(json, "transaction-history-time-limit",
+                        json_integer_create(conf->txn_history_time_max));
+    }
+
     return json;
 }
 
@@ -3024,12 +3049,13 @@  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_time_limit;
     struct db_config *conf = xzalloc(sizeof *conf);
     struct ovsdb_parser parser;
     struct ovsdb_error *error;
 
     conf->model = SM_UNDEFINED;
+    conf->txn_history_time_max = TXN_HISTORY_TIME_MAX_DEFAULT;
 
     ovs_assert(json);
     if (json->type == JSON_NULL) {
@@ -3105,6 +3131,22 @@  db_config_from_json(const char *name, const struct json *json)
         }
     }
 
+    txn_time_limit = ovsdb_parser_member(&parser,
+                                         "transaction-history-time-limit",
+                                         OP_INTEGER | OP_OPTIONAL);
+    if (txn_time_limit) {
+        if (json_integer(txn_time_limit) < 0) {
+            ovsdb_parser_raise_error(&parser,
+                "transaction-history-time-limit must not be negative");
+        } else if (json_integer(txn_time_limit) > INT_MAX) {
+            ovsdb_parser_raise_error(&parser,
+                "transaction-history-time-limit must not be greater than %d",
+                INT_MAX);
+        } else {
+            conf->txn_history_time_max = json_integer(txn_time_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..3b027fe64 100644
--- a/ovsdb/ovsdb.h
+++ b/ovsdb/ovsdb.h
@@ -71,6 +71,7 @@  bool ovsdb_is_valid_version(const char *);
 struct ovsdb_txn_history_node {
     struct ovs_list node; /* Element in struct ovsdb's txn_history list */
     struct ovsdb_txn *txn;
+    long long int timestamp; /* Time of addition to the history, in msec. */
 };
 
 struct ovsdb_compaction_state {
@@ -108,6 +109,8 @@  struct ovsdb {
 
     /* History trasanctions for incremental monitor transfer. */
     bool need_txn_history;     /* Need to maintain history of transactions. */
+    long long int txn_history_time_max; /* Keep entries younger than this
+                                         * many msec. */
     unsigned int n_txn_history; /* Current number of history transactions. */
     unsigned int n_txn_history_atoms; /* Total number of atoms in history. */
     struct ovs_list txn_history; /* Contains "struct ovsdb_txn_history_node. */
diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c
index 0d0d27b61..ef0c668a2 100644
--- a/ovsdb/transaction.c
+++ b/ovsdb/transaction.c
@@ -33,6 +33,7 @@ 
 #include "row.h"
 #include "storage.h"
 #include "table.h"
+#include "timeval.h"
 #include "uuid.h"
 #include "util.h"
 
@@ -1189,6 +1190,7 @@  ovsdb_txn_add_to_history(struct ovsdb_txn *txn)
     if (txn->db->need_txn_history) {
         struct ovsdb_txn_history_node *node = xzalloc(sizeof *node);
         node->txn = ovsdb_txn_clone_for_history(txn);
+        node->timestamp = time_msec();
         ovs_list_push_back(&txn->db->txn_history, &node->node);
         txn->db->n_txn_history++;
         txn->db->n_txn_history_atoms += txn->n_atoms;
@@ -1683,15 +1685,22 @@  ovsdb_txn_history_run(struct ovsdb *db)
      * the number of ovsdb atoms in history becomes less than the number of
      * atoms in the database, because it will be faster to just get a database
      * snapshot than re-constructing changes from the history that big.
+     * Entries older than 'txn_history_time_max' are removed as well.
      * 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_atoms > db->n_atoms)) {
+    long long int now = time_msec();
+
+    while (db->n_txn_history > 1) {
         struct ovsdb_txn_history_node *txn_h_node = CONTAINER_OF(
-                ovs_list_pop_front(&db->txn_history),
+                ovs_list_front(&db->txn_history),
                 struct ovsdb_txn_history_node, node);
+        bool expired = now - txn_h_node->timestamp > db->txn_history_time_max;
+
+        if (!expired && db->n_txn_history_atoms <= db->n_atoms) {
+            break;
+        }
 
+        ovs_list_remove(&txn_h_node->node);
         db->n_txn_history_atoms -= txn_h_node->txn->n_atoms;
         ovsdb_txn_destroy_cloned(txn_h_node->txn);
         free(txn_h_node);
@@ -1725,3 +1734,11 @@  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, long long int txn_history_time_max)
+{
+    ovs_assert(txn_history_time_max >= 0);
+    db->txn_history_time_max = txn_history_time_max;
+    ovsdb_txn_history_run(db);
+}
diff --git a/ovsdb/transaction.h b/ovsdb/transaction.h
index d94205414..914cfdac5 100644
--- a/ovsdb/transaction.h
+++ b/ovsdb/transaction.h
@@ -77,5 +77,7 @@  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_destroy(struct ovsdb *);
+void ovsdb_txn_history_update(struct ovsdb *,
+                              long long int txn_history_time_max);
 
 #endif /* ovsdb/transaction.h */
diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at
index 7042b1fd3..509cf953c 100644
--- a/tests/ovsdb-server.at
+++ b/tests/ovsdb-server.at
@@ -1640,6 +1640,46 @@  dnl still has a reasonable size.
 check_atoms
 AT_CHECK([test $(get_memory_value atoms) -eq $db_atoms_before_conversion])
 
+OVSDB_SERVER_SHUTDOWN
+AT_CLEANUP
+
+AT_SETUP([ovsdb-server transaction history time limit])
+AT_KEYWORDS([ovsdb server transaction])
+on_exit 'kill `cat *.pid`'
+AT_CHECK([ovsdb-tool create-cluster db dnl
+            $abs_top_srcdir/vswitchd/vswitch.ovsschema unix:s1.raft],
+         [0], [ignore], [ignore])
+AT_DATA([config.json], [
+{"remotes": {"punix:db.sock": {}},
+ "databases": {"db": {"service-model": "clustered"}}}
+])
+AT_CHECK([ovsdb-server --detach --no-chdir --pidfile --log-file dnl
+            --config-file=config.json], [0], [ignore], [ignore])
+
+dnl Make the database large enough to not limit the history by atoms.
+AT_CHECK([ovs-vsctl --no-wait init -- add-br br0 dnl
+            $(for i in $(seq 250); do echo -- add-port br0 p$i; done)])
+AT_CHECK([for i in $(seq 110); do
+              ovs-vsctl --no-wait set port p1 external_ids:k=$i || exit 1
+          done])
+
+n_txns () {
+    ovs-appctl -t ovsdb-server memory/show | tr ' ' '\n' | sed -n 's/^txn-history://p'
+}
+
+dnl All transactions are younger than the default limit, so all are kept.
+AT_CHECK([test $(n_txns) -ge 110])
+
+dnl Set a short limit, so all the entries expire.  The last one is always kept.
+sleep 2
+AT_DATA([config.json], [
+{"remotes": {"punix:db.sock": {}},
+ "databases": {"db": {"service-model": "clustered",
+                      "transaction-history-time-limit": 1}}}
+])
+AT_CHECK([ovs-appctl -t ovsdb-server ovsdb-server/reload])
+AT_CHECK([test $(n_txns) -eq 1])
+
 OVSDB_SERVER_SHUTDOWN
 AT_CLEANUP
 
@@ -3052,6 +3092,34 @@  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([negative transaction-history-time-limit], [
+{
+    "remotes": { "punix:db.sock": {} },
+    "databases": {
+        "db": { "service-model": "clustered",
+                "transaction-history-time-limit": -1 }
+    }
+}
+], [1], [dnl
+WARN|syntax "{"service-model":"clustered","transaction-history-time-limit":-1}": dnl
+syntax error: Parsing database db failed: dnl
+transaction-history-time-limit must not be negative
+WARN|config: failed to parse 'databases'])
+
+TEST_CONFIG_FILE([too large transaction-history-time-limit], [
+{
+    "remotes": { "punix:db.sock": {} },
+    "databases": {
+        "db": { "service-model": "clustered",
+                "transaction-history-time-limit": 2147483648 }
+    }
+}
+], [1], [dnl
+WARN|syntax "{"service-model":"clustered","transaction-history-time-limit":2147483648}": dnl
+syntax error: Parsing database db failed: dnl
+transaction-history-time-limit must not be greater than 2147483647
+WARN|config: failed to parse 'databases'])
+
 TEST_CONFIG_FILE([unknown config], [
 {
     "remotes": { "punix:db.sock": {} },