Hi Ales Thank you for the review and the comments.
On Tue, Jan 14, 2025 at 8:12 AM Ales Musil <[email protected]> wrote: > > > On Tue, Dec 10, 2024 at 3:22 PM Xavier Simonart <[email protected]> > wrote: > >> The tests was sometimes (rarely) failing as looking for flows with e.g. >> cookie c5d057f >> but also catching flows with cookie bec5d057. >> >> Signed-off-by: Xavier Simonart <[email protected]> >> --- >> > > Hi Xavier, > > thank you for the patch, I have one question down below. > > tests/ovn.at | 77 +++++++++++++++++++++++----------------------------- >> 1 file changed, 34 insertions(+), 43 deletions(-) >> >> diff --git a/tests/ovn.at b/tests/ovn.at >> index e2fbab247..30617ea9c 100644 >> --- a/tests/ovn.at >> +++ b/tests/ovn.at >> @@ -35404,6 +35404,22 @@ m4_define([MULTIPLE_OVS_INT], >> sed -e 's/n_bytes=[[0-9]]*, //g' >> } >> >> + WAIT_UNTIL_FLOWS() >> + { >> + cookie=${1} >> + interface=${2} >> + OVS_WAIT_UNTIL([ >> + ofport=$(ovs-vsctl --bare --columns ofport find Interface >> name=$interface) >> + ovs-ofctl dump-flows br-int | grep "cookie=0x$cookie" | grep >> "actions=output:$ofport" >> + ]) >> + } >> + >> + get_nb_flows() >> + { >> + cookie=${1} >> + ovs-ofctl dump-flows br-int | grep "cookie=0x$cookie" | wc -l >> + } >> + >> check ovn-nbctl ls-add ls >> check ovn-nbctl lsp-add ls lp >> if test X$1 != X; then >> @@ -35429,11 +35445,9 @@ m4_define([MULTIPLE_OVS_INT], >> echo ====================================================== >> COOKIE=$(ovn-sbctl find port_binding logical_port=lp|grep uuid|cut >> -d: -f2| cut -c1-8 | sed 's/^\s*0\{0,8\}//') >> > > Since this changes most of the test, the cookie extraction could be > replaced with the following: > > "ovn-debug uuid-to-cookie $(fetch_column port_binding _uuid > logical_port=lp)" > > WDYT? > I think that, in that case, we might still be (very) unlucky: we have for instance some actions such as "actions=set_field:0x1000000000000/0x1000000000000 in some flows", which would succeed a grep for a uuid 10000000. Hence I think that, to be safe, we should grep for "cookie=0x$cookie" (or cookie=$cookie if $cookie is already hex based). There could be fewer changes in the proposed patch, but I had the impression that this patch was the opportunity to avoid some code duplication. > > >> >> - OVS_WAIT_UNTIL([ >> - ofport=$(ovs-vsctl --bare --columns ofport find Interface >> name=lpold) >> - ovs-ofctl dump-flows br-int | grep $COOKIE | grep >> "actions=output:$ofport" >> - ]) >> - nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l` >> + WAIT_UNTIL_FLOWS $COOKIE lpold >> + >> + nb_flows=$(get_nb_flows $COOKIE ) >> echo $nb_flows "flows after iface-id set for old interface" >> >> echo ====================================================== >> @@ -35442,11 +35456,8 @@ m4_define([MULTIPLE_OVS_INT], >> # Set external_ids:iface-id within same transaction as adding the >> port. >> # This will generally cause ovn-controller to get initially notified >> of ovs interface changes with ofport == 0. >> check ovs-vsctl add-port br-int lpnew -- set interface lpnew >> type=internal -- set interface lpnew external_ids:iface-id=lp >> - OVS_WAIT_UNTIL([ >> - ofport=$(ovs-vsctl --bare --columns ofport find Interface >> name=lpnew) >> - ovs-ofctl dump-flows br-int | grep $COOKIE | grep >> "actions=output:$ofport" >> - ]) >> - check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE >> | wc -l) >> + WAIT_UNTIL_FLOWS $COOKIE lpnew >> + check test "$nb_flows" = $(get_nb_flows $COOKIE) >> flows_lpnew=$(get_flows $COOKIE) >> >> echo ====================================================== >> @@ -35455,7 +35466,7 @@ m4_define([MULTIPLE_OVS_INT], >> check ovs-vsctl del-port br-int lpold >> # We do not expect changes, so let's wait for controller to get time >> to process any update >> check ovn-nbctl --wait=hv sync >> - check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE >> | wc -l) >> + check test "$nb_flows" = $(get_nb_flows $COOKIE) >> flows_after_deletion=$(get_flows $COOKIE) >> check test "$flows_lpnew" = "$flows_after_deletion" >> >> @@ -35466,23 +35477,15 @@ m4_define([MULTIPLE_OVS_INT], >> # This will generally cause ovn-controller to get notified of ovs >> interface changes with a proper ofport. >> check ovs-vsctl add-port br-int lptemp -- set Interface lptemp >> type=internal >> check ovs-vsctl set Interface lptemp external_ids:iface-id=lp >> - OVS_WAIT_UNTIL([ >> - ofport=$(ovs-vsctl --bare --columns ofport find Interface >> name=lptemp) >> - ovs-ofctl dump-flows br-int | grep $COOKIE | grep >> "actions=output:$ofport" >> - ]) >> - check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE >> | wc -l) >> + WAIT_UNTIL_FLOWS $COOKIE lptemp >> + check test "$nb_flows" = $(get_nb_flows $COOKIE) >> >> echo ====================================================== >> echo ======= Flows after lptemp interface is deleted ====== >> echo ====================================================== >> check ovs-vsctl del-port br-int lptemp >> - OVS_WAIT_UNTIL([ >> - ofport=$(ovs-vsctl --bare --columns ofport find Interface >> name=lpnew) >> - echo $ofport >> - ovs-ofctl dump-flows br-int | grep $COOKIE >> - ovs-ofctl dump-flows br-int | grep $COOKIE | grep >> "actions=output:$ofport" >> - ]) >> - check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE >> | wc -l) >> + WAIT_UNTIL_FLOWS $COOKIE lpnew >> + check test "$nb_flows" = $(get_nb_flows $COOKIE) >> flows_after_deletion=$(get_flows $COOKIE) >> check test "$flows_lpnew" = "$flows_after_deletion" >> >> @@ -35491,7 +35494,7 @@ m4_define([MULTIPLE_OVS_INT], >> echo ====================================================== >> check ovs-vsctl del-port br-int lpnew >> OVS_WAIT_UNTIL([ >> - nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l` >> + nb_flows=`get_nb_flows $COOKIE` >> test "${nb_flows}" = $nb_flows_ref >> ]) >> >> @@ -35504,21 +35507,15 @@ m4_define([MULTIPLE_OVS_INT], >> check ovs-vsctl set interface lpnew external_ids:iface-id=lp >> >> # Wait for lpnew flows to be installed >> - OVS_WAIT_UNTIL([ >> - ofport=$(ovs-vsctl --bare --columns ofport find Interface >> name=lpnew) >> - ovs-ofctl dump-flows br-int | grep $COOKIE | grep >> "actions=output:$ofport" >> - ]) >> + WAIT_UNTIL_FLOWS $COOKIE lpnew >> flows_lpnew=$(get_flows $COOKIE) >> - nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l` >> + nb_flows=`get_nb_flows $COOKIE` >> >> check ovs-vsctl add-port br-int lptemp -- set Interface lptemp >> type=internal >> check ovs-vsctl set Interface lptemp external_ids:iface-id=lp >> >> # Wait for lptemp flows to be installed >> - OVS_WAIT_UNTIL([ >> - ofport=$(ovs-vsctl --bare --columns ofport find Interface >> name=lptemp) >> - ovs-ofctl dump-flows br-int | grep $COOKIE | grep >> "actions=output:$ofport" >> - ]) >> + WAIT_UNTIL_FLOWS $COOKIE lptemp >> >> # Delete both lpold and lptemp to go to a stable situation >> check ovs-vsctl del-port br-int lptemp >> @@ -35529,21 +35526,15 @@ m4_define([MULTIPLE_OVS_INT], >> ]) >> >> # Wait for correct/lpnew flows to be installed >> - OVS_WAIT_UNTIL([ >> - ofport=$(ovs-vsctl --bare --columns ofport find Interface >> name=lpnew) >> - ovs-ofctl dump-flows br-int | grep $COOKIE | grep >> "actions=output:$ofport" >> - ]) >> - check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE >> | wc -l) >> + WAIT_UNTIL_FLOWS $COOKIE lpnew >> + check test "$nb_flows" = $(get_nb_flows $COOKIE) >> flows_after_deletion=$(get_flows $COOKIE) >> check test "$flows_lpnew" = "$flows_after_deletion" >> >> # Check that recompute still works >> check ovn-appctl -t ovn-controller recompute >> - OVS_WAIT_UNTIL([ >> - ofport=$(ovs-vsctl --bare --columns ofport find Interface >> name=lpnew) >> - ovs-ofctl dump-flows br-int | grep $COOKIE | grep >> "actions=output:$ofport" >> - ]) >> - check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE >> | wc -l) >> + WAIT_UNTIL_FLOWS $COOKIE lpnew >> + check test "$nb_flows" = $(get_nb_flows $COOKIE) >> flows_after_deletion=$(get_flows $COOKIE) >> check test "$flows_lpnew" = "$flows_after_deletion" >> >> -- >> 2.47.1 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > Thanks, > Ales > Thanks Xavier _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
