Re: [ovs-dev] [PATCH ovn] tests: Add macro for checking flows after recompute.

[Dumitru Ceara]

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  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 
>>> ---
>>>   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

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] tests: Add macro for checking flows after recompute.

[Xavier Simonart]

Hi Mark

Thanks for the review.

On Thu, Apr 4, 2024 at 10:25 PM Mark Michelson  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 
> > ---
> >   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
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] tests: Add macro for checking flows after recompute.

[Mark Michelson]

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 
---
  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


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] tests: Add macro for checking flows after recompute.

[Ales Musil]

On Tue, Mar 26, 2024 at 12:57 PM 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 
> ---
>  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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Looks good to me, thanks.

Acked-by: Ales Musil 

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.com

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn] tests: Add macro for checking flows after recompute.

[Xavier Simonart]

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 
---
 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev