Hi Rosemarie Thanks for the review/feedback/questions.
On Wed, Oct 21, 2026 at 6:05 PM Rosemarie O'Riorden <[email protected]> wrote: > Hi Xavier, > > On 10/20/26 8:46 AM, Xavier Simonart via dev wrote: > > Avoid using AT_CHECK in construction such as 'while read ... done < > db.txt' > > as it prevents OVS_PAUSE_TEST=1 to pause for terminal input, as it > > consumes the next line from db.txt and bypasses the pause prompt > > instantly. > > > > Also add checks for vtep related db changes. We do this after stopping > > ovs-vtep because ovs-vtep is quite slow. > > > > Fixes: c0486bfa1074 ("tests: Detect unexpected db changes when test > completes.") > > Signed-off-by: Xavier Simonart <[email protected]> > > --- > > tests/ovn-controller-vtep.at | 1 + > > tests/ovn-macros.at | 18 +++++++++++------- > > 2 files changed, 12 insertions(+), 7 deletions(-) > > > > diff --git a/tests/ovn-controller-vtep.at b/tests/ovn-controller-vtep.at > > index aa8da62a6..5b2895676 100644 > > --- a/tests/ovn-controller-vtep.at > > +++ b/tests/ovn-controller-vtep.at > > @@ -72,6 +72,7 @@ m4_define([OVN_CONTROLLER_VTEP_STOP],[ > > test -n "$2" && as "$2" > > #AT_CHECK([check_logs "$1"]) > > OVS_APP_EXIT_AND_WAIT([ovs-vtep]) > > + CHECK_UNEXPECTED_DB_CHANGES > > I know you said it's ok to do this after stopping ovs-vtep because it's > slow, but why not put it before in case it happens to stop quicker > sometimes? In every other STOP macro, CHECK_UNEXPECTED_DB_CHANGES is run > before stopping the daemon, so I'm just curious as to why you'd want to > do it the other way around for this case. > Those are valid questions. The key here is that the macro checks the database state, expecting a stable system; i.e., it expects to see no - or very few - changes. But ovs-vtep is ** really ** very slow i.e. I saw it sometimes handling changes after several seconds. So, if we check the DB while ovs-vtep is still running, we would see db changes (done by ovs-vtep) and report those as unexpected changes. Tests are supposed to be stable once completed, which isn't the case when ovs-vtep is still making changes. Stopping ovs-vtep is the easiest way to make those tests stable before/while cleanup. Another way would be to modify each vtep related test and add code to ensure ovs-vtep completed handling the changes before the clean up. But I found this would not only be quite painful (compared to the added-value) but also would make writing new vtep related tests more difficult. WDYT? If ovs-vtep happens to be fast, then there is no issue: the test would be stable before cleanup, and stopping it would not cause any issue. > > > OVS_APP_EXIT_AND_WAIT([ovn-controller-vtep]) > > OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > > OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])]) > > diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at > > index e0f70bcde..aa2e4453b 100644 > > --- a/tests/ovn-macros.at > > +++ b/tests/ovn-macros.at > > @@ -189,14 +189,18 @@ m4_define([CHECK_UNEXPECTED_DB_CHANGES],[ > > # Second pass: if there is any db change, rerun check and ignore > small number of changes. > > if [[ "$diff" -ne 0 ]]; then > > sleep 1 > > - while read before_count db; do > > - after_count=$(wc -l < "$db") > > - diff=$((after_count - before_count)) > > - if [[ "$diff" -ge "50" ]]; then > > - printf 'Got %d changes in %s\n' "$diff" "$db" > > - AT_CHECK([test "$diff" -lt "50"]) > > + db_list=$(cut -d' ' -f2- db.txt) > > + for db in $db_list; do > > + before_count=$(grep " $db\$" db.txt | cut -d' ' -f1) > > + if test -n "$before_count"; then > > + after_count=$(wc -l < "$db") > > + diff=$((after_count - before_count)) > > + if [[ "$diff" -ge "50" ]]; then > > + printf 'Got %d changes in %s\n' "$diff" "$db" > > + AT_CHECK([test "$diff" -lt "50"]) > > + fi > > fi > > - done < db.txt > > + done > > fi > > ]) > > > > Thanks Xavier _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
