On 10/23/21 08:40, Han Zhou wrote: > > > On Thu, Oct 14, 2021 at 7:58 AM Ilya Maximets <[email protected] > <mailto:[email protected]>> wrote: >> >> If user frequently changes a lot of rows in a database, transaction >> history could grow way larger than the database itself. This wastes >> a lot of memory and also makes monitor_cond_since slower than >> usual monotor_cond if the transaction id is old enough, because >> re-construction of the changes from a history is slower than just >> creation of initial database snapshot. This is also the case if >> user deleted a lot of data, so transaction history still holds all of >> it while the database itself doesn't. >> >> In case of current lb-per-service model in ovn-kubernetes, each >> load-balancer is added to every logical switch/router. Such a >> transaction touches more than a half of a OVN_Northbound database. >> And each of these transactions is added to the transaction history. >> Since transaction history depth is 100, in worst case scenario, >> it will hold 100 copies of a database increasing memory consumption >> dramatically. In tests with 3000 LBs and 120 LSs, memory goes up >> to 3 GB, while holding at 30 MB if transaction history disabled in >> the code. >> >> Fixing that by keeping count of the number of ovsdb_atom's in the >> database and not allowing the total number of atoms in transaction >> history to grow larger than this value. Counting atoms is fairly >> cheap because we don't need to iterate over them, so it doesn't have >> significant performance impact. It would be ideal to measure the >> size of individual atoms, but that will hit the performance. >> Counting cells instead of atoms is not sufficient, because OVN >> users are adding hundreds or thousands of atoms to a single cell, >> so they are largely different in size. >> >> Signed-off-by: Ilya Maximets <[email protected] <mailto:[email protected]>> >> --- >> ovsdb/ovsdb.c | 5 ++++ >> ovsdb/ovsdb.h | 3 +++ >> ovsdb/transaction.c | 56 +++++++++++++++++++++++++++++++++++++++++++-- >> 3 files changed, 62 insertions(+), 2 deletions(-)
<snip> > > Thanks Ilya. This looks good to me. Although the atoms number is not equal to > the size of the data, it should be sufficient for the purpose of controlling > history size. > It would be good to have a test case to ensure this behavior, and also ensure > the atoms count is as expected, just to avoid messing up the counters by > mistake in the future. (if that happens it is not easily noticed because > there is no functional difference but possible performance impact of > fast-resync) > > Acked-by: Han Zhou <[email protected] <mailto:[email protected]>> Thanks for review! I prepared a test below for the number of atoms. If it looks good to you, I can fold it in before applying the patch. Han, Dumitru, What do you think? diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at index ac243d6a7..2b742f78b 100644 --- a/tests/ovsdb-server.at +++ b/tests/ovsdb-server.at @@ -1228,6 +1228,69 @@ AT_CHECK([test $logged_updates -lt $logged_nonblock_updates]) AT_CHECK_UNQUOTED([ovs-vsctl get open_vswitch . system_version], [0], [xyzzy$counter ]) +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) +AT_CLEANUP + +AT_SETUP([ovsdb-server transaction history size]) +on_exit 'kill `cat *.pid`' + +dnl Start an ovsdb-server with the clustered vswitchd schema. +AT_CHECK([ovsdb-tool create-cluster db dnl + $abs_top_srcdir/vswitchd/vswitch.ovsschema unix:s1.raft], + [0], [ignore], [ignore]) +AT_CHECK([ovsdb-server --detach --no-chdir --pidfile dnl + --log-file --remote=punix:db.sock db], + [0], [ignore], [ignore]) +AT_CHECK([ovs-vsctl --no-wait init]) + +dnl Create a bridge with N ports per transaction. Increase N every 4 +dnl iterations. And then remove the bridges. By increasing the size of +dnl transactions, ensuring that they take up a significant percentage of +dnl the total database size, so the transaction history will not be able +dnl to hold all of them. +dnl +dnl The test verifies that the number of atoms in the transaction history +dnl is always less than the number of atoms in the database. +get_n_atoms () { + n=$(ovs-appctl -t ovsdb-server memory/show dnl + | tr ' ' '\n' | grep atoms | grep "^$1:" | cut -d ':' -f 2) + if test X"$n" == "X"; then + n=0 + fi + echo $n +} + +check_atoms () { + n_db_atoms=$(get_n_atoms atoms) + n_txn_history_atoms=$(get_n_atoms txn-history-atoms) + echo "n_db_atoms: $n_db_atoms" + echo "n_txn_history_atoms: $n_txn_history_atoms" + AT_CHECK([test $n_txn_history_atoms -le $n_db_atoms]) +} + +add_ports () { + for j in $(seq 1 $2); do + printf " -- add-port br$1 p$1-%d" $j + done +} + +initial_db_atoms=$(get_n_atoms atoms) + +for i in $(seq 1 100); do + cmd=$(add_ports $i $(($i / 4 + 1))) + AT_CHECK([ovs-vsctl --no-wait add-br br$i $cmd]) + check_atoms +done + +for i in $(seq 1 100); do + AT_CHECK([ovs-vsctl --no-wait del-br br$i]) + check_atoms +done + +dnl After removing all the bridges, the number of atoms in the database +dnl should return to its initial value. +AT_CHECK([test $(get_n_atoms atoms) -eq $initial_db_atoms]) + OVS_APP_EXIT_AND_WAIT([ovsdb-server]) AT_CLEANUP --- _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
