On 1/17/23 19:34, Han Zhou wrote: > > > On Tue, Jan 17, 2023 at 10:13 AM Ilya Maximets <[email protected] > <mailto:[email protected]>> wrote: >> >> 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]> <mailto:[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]> <mailto:[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]> >> > <mailto:[email protected] <mailto:[email protected]>>> >> >> Makes sense. I can add the following change: >> >> diff --git a/tests/ovsdb-server.at <http://ovsdb-server.at> >> b/tests/ovsdb-server.at <http://ovsdb-server.at> >> index 0828e6d04..bf539b6e5 100644 >> --- a/tests/ovsdb-server.at <http://ovsdb-server.at> >> +++ b/tests/ovsdb-server.at <http://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? >> > LGTM. Thanks Ilya.
Thanks! Applied and backported down to 2.17. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
