| Message ID | 20240326115651.3433227-1-xsimonar@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Dumitru Ceara |
| Headers | show |
| Series | [ovs-dev] tests: Add macro for checking flows after recompute. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Tue, Mar 26, 2024 at 12:57 PM Xavier Simonart <xsimonar@redhat.com> wrote: > The macro CHECK_FLOWS_AFTER_RECOMPUTE dumps the Openflows, then > recomputes, then dumps again the Openflows, and finally compares > both sets of flows. The test fails if flows are different. > As of now, the macro cannot be used in all tests: many tests would fail > as I+P does not properly remove flows when the last logical port of > a datapath is deleted. > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > --- > tests/ovn-macros.at | 44 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at > index ed93764d3..11377f616 100644 > --- a/tests/ovn-macros.at > +++ b/tests/ovn-macros.at > @@ -10,6 +10,50 @@ m4_define([OVN_CLEANUP_VSWITCH],[ > OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > ]) > > +# DUMP_FLOWS(sbox, output_file) > +# Dump openflows to output_file for sbox > +m4_define([DUMP_FLOWS], [ > + sbox=$1 > + output_file=$2 > + as $sbox > + ovs-ofctl dump-flows br-int | > + sed 's/cookie=0x[[^,]]*/cookie=xx/g' | > + sed 's/duration=[[^,]]*/duration=xx/g' | > + sed 's/idle_age=[[^,]]*/idle_age=xx/g' | > + sed 's/, hard_age=[[^,]]*//g' | > + sed 's/n_bytes=[[^,]]*/n_bytes=xx/g' | > + sed 's/n_packets=[[^,]]*/n_packets=xx/g' | > + sed 's/conjunction([[^,]]*/conjunction(xx/g' | > + sort > $output_file > +]) > + > +m4_define([CHECK_FLOWS_AFTER_RECOMPUTE], [ > + hv=$1 > + sbox=$2 > + # Make sure I+P has finalized his job before getting flows and > comparing them after recompte. > + # Some tests have northd and ovn-nb ovsdb stopped, so avoid ovn-nbctl > for those. > + if [[ -e ovn-nb/ovn-nb.sock ]] && [[ -e northd/ovn-northd.pid ]]; then > + # Do wait twice to handle some potential race conditions > + check ovn-nbctl --wait=hv sync > + check ovn-nbctl --wait=hv sync > + fi > + > + as $sbox > + if test "$hv" != "vtep"; then > + # Get flows before and after recompute > + DUMP_FLOWS([$sbox], [flows-$hv-1]) > + > + check ovn-appctl -t ovn-controller recompute > + # The recompute might cause some sb changes. Let controller catch > up. > + if [[ -e ovn-nb/ovn-nb.sock ]] && [[ -e northd/ovn-northd.pid ]]; > then > + check ovn-nbctl --wait=hv sync > + fi > + DUMP_FLOWS([$sbox], [flows-$hv-2]) > + diff flows-$hv-1 flows-$hv-2 > flow-diff > + AT_CHECK([test $(diff flows-$hv-1 flows-$hv-2 | wc -l) == 0]) > + fi > +]) > + > # OVN_CLEANUP_CONTROLLER(sbox) > # > # Gracefully terminate ovn-controller in the specified > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks. Acked-by: Ales Musil <amusil@redhat.com>
Hi Xavier, the patch looks good, but I have one question down below. On 3/26/24 07:56, Xavier Simonart wrote: > The macro CHECK_FLOWS_AFTER_RECOMPUTE dumps the Openflows, then > recomputes, then dumps again the Openflows, and finally compares > both sets of flows. The test fails if flows are different. > As of now, the macro cannot be used in all tests: many tests would fail > as I+P does not properly remove flows when the last logical port of > a datapath is deleted. > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > --- > tests/ovn-macros.at | 44 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at > index ed93764d3..11377f616 100644 > --- a/tests/ovn-macros.at > +++ b/tests/ovn-macros.at > @@ -10,6 +10,50 @@ m4_define([OVN_CLEANUP_VSWITCH],[ > OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > ]) > > +# DUMP_FLOWS(sbox, output_file) > +# Dump openflows to output_file for sbox > +m4_define([DUMP_FLOWS], [ > + sbox=$1 > + output_file=$2 > + as $sbox > + ovs-ofctl dump-flows br-int | > + sed 's/cookie=0x[[^,]]*/cookie=xx/g' | > + sed 's/duration=[[^,]]*/duration=xx/g' | > + sed 's/idle_age=[[^,]]*/idle_age=xx/g' | > + sed 's/, hard_age=[[^,]]*//g' | > + sed 's/n_bytes=[[^,]]*/n_bytes=xx/g' | > + sed 's/n_packets=[[^,]]*/n_packets=xx/g' | > + sed 's/conjunction([[^,]]*/conjunction(xx/g' | > + sort > $output_file Should we mask group IDs here? Can group IDs change because of a recompute? > +]) > + > +m4_define([CHECK_FLOWS_AFTER_RECOMPUTE], [ > + hv=$1 > + sbox=$2 > + # Make sure I+P has finalized his job before getting flows and comparing them after recompte. > + # Some tests have northd and ovn-nb ovsdb stopped, so avoid ovn-nbctl for those. > + if [[ -e ovn-nb/ovn-nb.sock ]] && [[ -e northd/ovn-northd.pid ]]; then > + # Do wait twice to handle some potential race conditions > + check ovn-nbctl --wait=hv sync > + check ovn-nbctl --wait=hv sync > + fi > + > + as $sbox > + if test "$hv" != "vtep"; then > + # Get flows before and after recompute > + DUMP_FLOWS([$sbox], [flows-$hv-1]) > + > + check ovn-appctl -t ovn-controller recompute > + # The recompute might cause some sb changes. Let controller catch up. > + if [[ -e ovn-nb/ovn-nb.sock ]] && [[ -e northd/ovn-northd.pid ]]; then > + check ovn-nbctl --wait=hv sync > + fi > + DUMP_FLOWS([$sbox], [flows-$hv-2]) > + diff flows-$hv-1 flows-$hv-2 > flow-diff > + AT_CHECK([test $(diff flows-$hv-1 flows-$hv-2 | wc -l) == 0]) > + fi > +]) > + > # OVN_CLEANUP_CONTROLLER(sbox) > # > # Gracefully terminate ovn-controller in the specified
Hi Mark Thanks for the review. On Thu, Apr 4, 2024 at 10:25 PM Mark Michelson <mmichels@redhat.com> wrote: > Hi Xavier, the patch looks good, but I have one question down below. > > On 3/26/24 07:56, Xavier Simonart wrote: > > The macro CHECK_FLOWS_AFTER_RECOMPUTE dumps the Openflows, then > > recomputes, then dumps again the Openflows, and finally compares > > both sets of flows. The test fails if flows are different. > > As of now, the macro cannot be used in all tests: many tests would fail > > as I+P does not properly remove flows when the last logical port of > > a datapath is deleted. > > > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > > --- > > tests/ovn-macros.at | 44 ++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 44 insertions(+) > > > > diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at > > index ed93764d3..11377f616 100644 > > --- a/tests/ovn-macros.at > > +++ b/tests/ovn-macros.at > > @@ -10,6 +10,50 @@ m4_define([OVN_CLEANUP_VSWITCH],[ > > OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > > ]) > > > > +# DUMP_FLOWS(sbox, output_file) > > +# Dump openflows to output_file for sbox > > +m4_define([DUMP_FLOWS], [ > > + sbox=$1 > > + output_file=$2 > > + as $sbox > > + ovs-ofctl dump-flows br-int | > > + sed 's/cookie=0x[[^,]]*/cookie=xx/g' | > > + sed 's/duration=[[^,]]*/duration=xx/g' | > > + sed 's/idle_age=[[^,]]*/idle_age=xx/g' | > > + sed 's/, hard_age=[[^,]]*//g' | > > + sed 's/n_bytes=[[^,]]*/n_bytes=xx/g' | > > + sed 's/n_packets=[[^,]]*/n_packets=xx/g' | > > + sed 's/conjunction([[^,]]*/conjunction(xx/g' | > > + sort > $output_file > > Should we mask group IDs here? Can group IDs change because of a recompute? > > Good question. I do not think so. When assigning the group_id a check is done to see if a group with the same content already exists, so the same group_id is reused in recompute. > > +]) > > + > > +m4_define([CHECK_FLOWS_AFTER_RECOMPUTE], [ > > + hv=$1 > > + sbox=$2 > > + # Make sure I+P has finalized his job before getting flows and > comparing them after recompte. > > + # Some tests have northd and ovn-nb ovsdb stopped, so avoid > ovn-nbctl for those. > > + if [[ -e ovn-nb/ovn-nb.sock ]] && [[ -e northd/ovn-northd.pid ]]; > then > > + # Do wait twice to handle some potential race conditions > > + check ovn-nbctl --wait=hv sync > > + check ovn-nbctl --wait=hv sync > > + fi > > + > > + as $sbox > > + if test "$hv" != "vtep"; then > > + # Get flows before and after recompute > > + DUMP_FLOWS([$sbox], [flows-$hv-1]) > > + > > + check ovn-appctl -t ovn-controller recompute > > + # The recompute might cause some sb changes. Let controller catch > up. > > + if [[ -e ovn-nb/ovn-nb.sock ]] && [[ -e northd/ovn-northd.pid ]]; > then > > + check ovn-nbctl --wait=hv sync > > + fi > > + DUMP_FLOWS([$sbox], [flows-$hv-2]) > > + diff flows-$hv-1 flows-$hv-2 > flow-diff > > + AT_CHECK([test $(diff flows-$hv-1 flows-$hv-2 | wc -l) == 0]) > > + fi > > +]) > > + > > # OVN_CLEANUP_CONTROLLER(sbox) > > # > > # Gracefully terminate ovn-controller in the specified > Thanks Xavier
On 4/5/24 11:55, Xavier Simonart wrote: > Hi Mark > > Thanks for the review. > > On Thu, Apr 4, 2024 at 10:25 PM Mark Michelson <mmichels@redhat.com> wrote: > >> Hi Xavier, the patch looks good, but I have one question down below. >> >> On 3/26/24 07:56, Xavier Simonart wrote: >>> The macro CHECK_FLOWS_AFTER_RECOMPUTE dumps the Openflows, then >>> recomputes, then dumps again the Openflows, and finally compares >>> both sets of flows. The test fails if flows are different. >>> As of now, the macro cannot be used in all tests: many tests would fail >>> as I+P does not properly remove flows when the last logical port of >>> a datapath is deleted. >>> >>> Signed-off-by: Xavier Simonart <xsimonar@redhat.com> >>> --- >>> tests/ovn-macros.at | 44 ++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 44 insertions(+) >>> >>> diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at >>> index ed93764d3..11377f616 100644 >>> --- a/tests/ovn-macros.at >>> +++ b/tests/ovn-macros.at >>> @@ -10,6 +10,50 @@ m4_define([OVN_CLEANUP_VSWITCH],[ >>> OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >>> ]) >>> >>> +# DUMP_FLOWS(sbox, output_file) >>> +# Dump openflows to output_file for sbox >>> +m4_define([DUMP_FLOWS], [ >>> + sbox=$1 >>> + output_file=$2 >>> + as $sbox >>> + ovs-ofctl dump-flows br-int | >>> + sed 's/cookie=0x[[^,]]*/cookie=xx/g' | >>> + sed 's/duration=[[^,]]*/duration=xx/g' | >>> + sed 's/idle_age=[[^,]]*/idle_age=xx/g' | >>> + sed 's/, hard_age=[[^,]]*//g' | >>> + sed 's/n_bytes=[[^,]]*/n_bytes=xx/g' | >>> + sed 's/n_packets=[[^,]]*/n_packets=xx/g' | >>> + sed 's/conjunction([[^,]]*/conjunction(xx/g' | >>> + sort > $output_file >> >> Should we mask group IDs here? Can group IDs change because of a recompute? >> >> Good question. I do not think so. When assigning the group_id a check is > done to see if a group with the same content already exists, so the same > group_id is reused in recompute. > Thanks Xavier, Ales, Mark! It seems that we're fine with this as is. If this behavior changes (group id change during recompute) we can revisit the macro. However, I think it's desirable that the group ID doesn't change. In any case I applied this to main. Looking forward to the patches that actually use this in tests. Regards, Dumitru
diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at index ed93764d3..11377f616 100644 --- a/tests/ovn-macros.at +++ b/tests/ovn-macros.at @@ -10,6 +10,50 @@ m4_define([OVN_CLEANUP_VSWITCH],[ OVS_APP_EXIT_AND_WAIT([ovsdb-server]) ]) +# DUMP_FLOWS(sbox, output_file) +# Dump openflows to output_file for sbox +m4_define([DUMP_FLOWS], [ + sbox=$1 + output_file=$2 + as $sbox + ovs-ofctl dump-flows br-int | + sed 's/cookie=0x[[^,]]*/cookie=xx/g' | + sed 's/duration=[[^,]]*/duration=xx/g' | + sed 's/idle_age=[[^,]]*/idle_age=xx/g' | + sed 's/, hard_age=[[^,]]*//g' | + sed 's/n_bytes=[[^,]]*/n_bytes=xx/g' | + sed 's/n_packets=[[^,]]*/n_packets=xx/g' | + sed 's/conjunction([[^,]]*/conjunction(xx/g' | + sort > $output_file +]) + +m4_define([CHECK_FLOWS_AFTER_RECOMPUTE], [ + hv=$1 + sbox=$2 + # Make sure I+P has finalized his job before getting flows and comparing them after recompte. + # Some tests have northd and ovn-nb ovsdb stopped, so avoid ovn-nbctl for those. + if [[ -e ovn-nb/ovn-nb.sock ]] && [[ -e northd/ovn-northd.pid ]]; then + # Do wait twice to handle some potential race conditions + check ovn-nbctl --wait=hv sync + check ovn-nbctl --wait=hv sync + fi + + as $sbox + if test "$hv" != "vtep"; then + # Get flows before and after recompute + DUMP_FLOWS([$sbox], [flows-$hv-1]) + + check ovn-appctl -t ovn-controller recompute + # The recompute might cause some sb changes. Let controller catch up. + if [[ -e ovn-nb/ovn-nb.sock ]] && [[ -e northd/ovn-northd.pid ]]; then + check ovn-nbctl --wait=hv sync + fi + DUMP_FLOWS([$sbox], [flows-$hv-2]) + diff flows-$hv-1 flows-$hv-2 > flow-diff + AT_CHECK([test $(diff flows-$hv-1 flows-$hv-2 | wc -l) == 0]) + fi +]) + # OVN_CLEANUP_CONTROLLER(sbox) # # Gracefully terminate ovn-controller in the specified
The macro CHECK_FLOWS_AFTER_RECOMPUTE dumps the Openflows, then recomputes, then dumps again the Openflows, and finally compares both sets of flows. The test fails if flows are different. As of now, the macro cannot be used in all tests: many tests would fail as I+P does not properly remove flows when the last logical port of a datapath is deleted. Signed-off-by: Xavier Simonart <xsimonar@redhat.com> --- tests/ovn-macros.at | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)