On 1/6/23 07:35, Han Zhou wrote: > > > On Tue, Jan 3, 2023 at 8:22 AM Ilya Maximets <[email protected] > <mailto:[email protected]>> wrote: >> >> The counter for the number of atoms has to be re-set to the number from >> the new database, otherwise the value will be incorrect. For example, >> this is causing the atom counter doubling after online conversion of >> a clustered database. >> >> Miscounting may also lead to increased memory consumption by the >> transaction history or otherwise too aggressive transaction history >> sweep. >> >> Fixes: 317b1bfd7dd3 ("ovsdb: Don't let transaction history grow larger than >> the database.") >> Signed-off-by: Ilya Maximets <[email protected] <mailto:[email protected]>> >> --- >> ovsdb/ovsdb.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/ovsdb/ovsdb.c b/ovsdb/ovsdb.c >> index 11786f376..afec96264 100644 >> --- a/ovsdb/ovsdb.c >> +++ b/ovsdb/ovsdb.c >> @@ -715,5 +715,8 @@ ovsdb_replace(struct ovsdb *dst, struct ovsdb *src) >> >> dst->rbac_role = ovsdb_get_table(dst, "RBAC_Role"); >> >> + /* Get statistics from the new database. */ >> + dst->n_atoms = src->n_atoms; >> + >> ovsdb_destroy(src); >> } >> -- >> 2.38.1 >> > > Thanks Ilya. The fix looks good to me. It would be even better to update a > related test case to verify both n_atoms and n_txn_history_atoms are correct > after DB conversion. > > Acked-by: Han Zhou <[email protected] <mailto:[email protected]>>
Makes sense. I can add the following change: diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at index 0828e6d04..bf539b6e5 100644 --- a/tests/ovsdb-server.at +++ b/tests/ovsdb-server.at @@ -1308,6 +1308,24 @@ dnl After removing all the bridges, the number of atoms in the database dnl should return to its initial value. AT_CHECK([test $(get_memory_value atoms) -eq $initial_db_atoms]) +dnl Add a few more resources. +for i in $(seq 1 10); do + cmd=$(add_ports $i $(($i / 4 + 1))) + AT_CHECK([ovs-vsctl --no-wait add-br br$i $cmd]) +done +check_atoms + +db_atoms_before_conversion=$(get_memory_value atoms) + +dnl Trigger online conversion. +AT_CHECK([ovsdb-client convert $abs_top_srcdir/vswitchd/vswitch.ovsschema], + [0], [ignore], [ignore]) + +dnl Check that conversion didn't change the number of atoms and the history +dnl still has a reasonable size. +check_atoms +AT_CHECK([test $(get_memory_value atoms) -eq $db_atoms_before_conversion]) + OVS_APP_EXIT_AND_WAIT([ovsdb-server]) AT_CLEANUP ^L --- What do you think? Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
