Re: [ovs-dev] [PATCH v2 4/4] tests: system-traffic: add coverage for drop action

2023-07-07 Thread Ilya Maximets 
On 7/7/23 16:50, Eelco Chaudron wrote:
> 
> 
> On 7 Jul 2023, at 16:48, Ilya Maximets wrote:
> 
>> On 7/7/23 16:42, Eelco Chaudron wrote:
>>>
>>>
>>> On 7 Jul 2023, at 16:06, Ilya Maximets wrote:
>>>
 On 7/7/23 15:51, Eric Garver wrote:
> On Fri, Jul 07, 2023 at 02:09:49PM +0200, Eelco Chaudron wrote:
>>
>>
>> On 7 Jul 2023, at 13:08, Ilya Maximets wrote:
>>
>>> On 7/5/23 21:47, Eelco Chaudron wrote:


 On Wed, Jul 5, 2023 at 6:46 PM Eric Garver  wrote:

 On Wed, Jul 05, 2023 at 04:58:47PM +0200, Eelco Chaudron wrote:
 >
 >
 > On 30 Jun 2023, at 21:05, Eric Garver wrote:
 > Hi Eric,
 >
 > I started reviewing the series, and this test was failing every 
 other run for me on ‘check-system-userspace’. I ended up making the 
 following additional change:
 >
 > diff --git a/tests/ofproto-macros.at  
 b/tests/ofproto-macros.at 
 > index d2e6ac768..573ecdd0f 100644
 > --- a/tests/ofproto-macros.at 
 > +++ b/tests/ofproto-macros.at 
 > @@ -120,7 +120,7 @@ strip_xids () {
 >
 >  # Changes all 'used:...' to say 'used:0.0', to make output 
 easier to compare.
 >  strip_used () {
 > -    sed 's/used:[[0-9]]\.[[0-9]]*/used:0.0/'
 > +    sed 's/used:\([[0-9]]\.[[0-9]]*s\|never\)/used:0.0s/'
 >  }

 In this test case I expect the flow to be used. My local test runs 
 do
 not yield "never".

 Maybe we need a "ovs-appctl time/warp 5000" before dumping the 
 flow?
 This is used in tests/drop-stats.at  .

 Tried that but did not work, if you look at the actual number of 
 packets received by the flow entry, it varies quite a lot from 0 (only 
 learning) to 5 when I run the test multiple times.

 I tried changing the following to generate more traffic:

 -dnl generate some traffic
 -NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2], [1], 
 [ignore])
 +dnl Generate some traffic.
 +NS_CHECK_EXEC([at_ns0], [ping -q -c 100 -i 0.01 -w 1 10.1.1.2], [1], 
 [ignore])

 And then run it 100 times, and the earlier problem did not happen:

   sudo bash -c 'for i in {1..100}; do make check-system-userspace 
 TESTSUITEFLAGS="-k drop -v" || break; echo "EC_DONE: $i"; done'

 However, I have some times it fails with no traffic:


 --- - 2023-07-05 21:46:59.860127196 +0200
 +++ 
 /home/echaudron/Documents/review/ovs_eric_DROP/tests/system-userspace-testsuite.dir/at-groups/45/stdout
  2023-07-05 21:46:59.857641231 +0200
 @@ -1,2 +1 @@
 -recirc_id(),in_port(2),eth_type(0x0800),ipv4(frag=no), 
 packets:0, bytes:0, used:0.0s, actions:drop
  
 Maybe this test needs some more love ;)
>>>
>>> The test likely needs a revalidator/wait call, since you're
>>> checking datapath flows.  Otherwise, it'll be time sensitive.
>>> Stats are gathered during revalidation.
>>
>> That was might thought initially, and I tried it (no luck), but it’s dp 
>> flows, not openflow.
>
> revalidator/wait still fails.
>
> The only thing I've found that makes this stable is the sleep below.
>
> --->8---
>
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 50d6c2d05e5d..ab893c5c696b 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -2068,6 +2068,9 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>
>  dnl Generate some traffic.
>  NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2], [1], 
> [ignore])
> +dnl sleep to let the flow hit the datapath
> +sleep 5
> +NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2], [1], 
> [ignore])
>
>  AT_CHECK([ovs-appctl dpctl/dump-flows | grep "eth_type(0x0800)" | dnl
>sed 
> 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/;s/,eth(),/,/;' | dnl
>

 Yeah, revalidator doesn't matter if we're talking about datapath flows, 
 true.
 The only thing that matters is how fast the kernel makes counter visible to
 userpsace.  It may not happen on very short time slots.

 However, waiting for 5 seconds is not a good solution.  If waiting is the 
 only
 option, we should use the OVS_WAIT_UNTIL_EQUAL instead.
>>>
>>> With the following ping command:
>>>
>>>   NS_CHECK_EXEC([at_ns0], [ping -q -c 100 -i 0.01 -w 1 10.1.1.2], [1],
>>>
>>> I never saw the

Re: [ovs-dev] [PATCH v2 4/4] tests: system-traffic: add coverage for drop action

2023-07-07 Thread Eelco Chaudron 


On 7 Jul 2023, at 16:48, Ilya Maximets wrote:

> On 7/7/23 16:42, Eelco Chaudron wrote:
>>
>>
>> On 7 Jul 2023, at 16:06, Ilya Maximets wrote:
>>
>>> On 7/7/23 15:51, Eric Garver wrote:
 On Fri, Jul 07, 2023 at 02:09:49PM +0200, Eelco Chaudron wrote:
>
>
> On 7 Jul 2023, at 13:08, Ilya Maximets wrote:
>
>> On 7/5/23 21:47, Eelco Chaudron wrote:
>>>
>>>
>>> On Wed, Jul 5, 2023 at 6:46 PM Eric Garver  wrote:
>>>
>>> On Wed, Jul 05, 2023 at 04:58:47PM +0200, Eelco Chaudron wrote:
>>> >
>>> >
>>> > On 30 Jun 2023, at 21:05, Eric Garver wrote:
>>> > Hi Eric,
>>> >
>>> > I started reviewing the series, and this test was failing every 
>>> other run for me on ‘check-system-userspace’. I ended up making the 
>>> following additional change:
>>> >
>>> > diff --git a/tests/ofproto-macros.at  
>>> b/tests/ofproto-macros.at 
>>> > index d2e6ac768..573ecdd0f 100644
>>> > --- a/tests/ofproto-macros.at 
>>> > +++ b/tests/ofproto-macros.at 
>>> > @@ -120,7 +120,7 @@ strip_xids () {
>>> >
>>> >  # Changes all 'used:...' to say 'used:0.0', to make output 
>>> easier to compare.
>>> >  strip_used () {
>>> > -    sed 's/used:[[0-9]]\.[[0-9]]*/used:0.0/'
>>> > +    sed 's/used:\([[0-9]]\.[[0-9]]*s\|never\)/used:0.0s/'
>>> >  }
>>>
>>> In this test case I expect the flow to be used. My local test runs 
>>> do
>>> not yield "never".
>>>
>>> Maybe we need a "ovs-appctl time/warp 5000" before dumping the flow?
>>> This is used in tests/drop-stats.at  .
>>>
>>> Tried that but did not work, if you look at the actual number of 
>>> packets received by the flow entry, it varies quite a lot from 0 (only 
>>> learning) to 5 when I run the test multiple times.
>>>
>>> I tried changing the following to generate more traffic:
>>>
>>> -dnl generate some traffic
>>> -NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2], [1], 
>>> [ignore])
>>> +dnl Generate some traffic.
>>> +NS_CHECK_EXEC([at_ns0], [ping -q -c 100 -i 0.01 -w 1 10.1.1.2], [1], 
>>> [ignore])
>>>
>>> And then run it 100 times, and the earlier problem did not happen:
>>>
>>>   sudo bash -c 'for i in {1..100}; do make check-system-userspace 
>>> TESTSUITEFLAGS="-k drop -v" || break; echo "EC_DONE: $i"; done'
>>>
>>> However, I have some times it fails with no traffic:
>>>
>>>
>>> --- - 2023-07-05 21:46:59.860127196 +0200
>>> +++ 
>>> /home/echaudron/Documents/review/ovs_eric_DROP/tests/system-userspace-testsuite.dir/at-groups/45/stdout
>>>  2023-07-05 21:46:59.857641231 +0200
>>> @@ -1,2 +1 @@
>>> -recirc_id(),in_port(2),eth_type(0x0800),ipv4(frag=no), 
>>> packets:0, bytes:0, used:0.0s, actions:drop
>>>  
>>> Maybe this test needs some more love ;)
>>
>> The test likely needs a revalidator/wait call, since you're
>> checking datapath flows.  Otherwise, it'll be time sensitive.
>> Stats are gathered during revalidation.
>
> That was might thought initially, and I tried it (no luck), but it’s dp 
> flows, not openflow.

 revalidator/wait still fails.

 The only thing I've found that makes this stable is the sleep below.

 --->8---

 diff --git a/tests/system-traffic.at b/tests/system-traffic.at
 index 50d6c2d05e5d..ab893c5c696b 100644
 --- a/tests/system-traffic.at
 +++ b/tests/system-traffic.at
 @@ -2068,6 +2068,9 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])

  dnl Generate some traffic.
  NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2], [1], 
 [ignore])
 +dnl sleep to let the flow hit the datapath
 +sleep 5
 +NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2], [1], 
 [ignore])

  AT_CHECK([ovs-appctl dpctl/dump-flows | grep "eth_type(0x0800)" | dnl
sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/;s/,eth(),/,/;' 
 | dnl

>>>
>>> Yeah, revalidator doesn't matter if we're talking about datapath flows, 
>>> true.
>>> The only thing that matters is how fast the kernel makes counter visible to
>>> userpsace.  It may not happen on very short time slots.
>>>
>>> However, waiting for 5 seconds is not a good solution.  If waiting is the 
>>> only
>>> option, we should use the OVS_WAIT_UNTIL_EQUAL instead.
>>
>> With the following ping command:
>>
>>   NS_CHECK_EXEC([at_ns0], [ping -q -c 100 -i 0.01 -w 1 10.1.1.2], [1],
>>
>> I never saw the unused issue, only the one that showed no flow entry at all. 
>> Maybe with this ping instead and the OVS_WAIT_UNTIL_EQUAL() it might work?
>
> There is no point sending so many

Re: [ovs-dev] [PATCH v2 4/4] tests: system-traffic: add coverage for drop action

2023-07-07 Thread Ilya Maximets 
On 7/7/23 16:42, Eelco Chaudron wrote:
> 
> 
> On 7 Jul 2023, at 16:06, Ilya Maximets wrote:
> 
>> On 7/7/23 15:51, Eric Garver wrote:
>>> On Fri, Jul 07, 2023 at 02:09:49PM +0200, Eelco Chaudron wrote:


 On 7 Jul 2023, at 13:08, Ilya Maximets wrote:

> On 7/5/23 21:47, Eelco Chaudron wrote:
>>
>>
>> On Wed, Jul 5, 2023 at 6:46 PM Eric Garver  wrote:
>>
>> On Wed, Jul 05, 2023 at 04:58:47PM +0200, Eelco Chaudron wrote:
>> >
>> >
>> > On 30 Jun 2023, at 21:05, Eric Garver wrote:
>> > Hi Eric,
>> >
>> > I started reviewing the series, and this test was failing every 
>> other run for me on ‘check-system-userspace’. I ended up making the 
>> following additional change:
>> >
>> > diff --git a/tests/ofproto-macros.at  
>> b/tests/ofproto-macros.at 
>> > index d2e6ac768..573ecdd0f 100644
>> > --- a/tests/ofproto-macros.at 
>> > +++ b/tests/ofproto-macros.at 
>> > @@ -120,7 +120,7 @@ strip_xids () {
>> >
>> >  # Changes all 'used:...' to say 'used:0.0', to make output easier 
>> to compare.
>> >  strip_used () {
>> > -    sed 's/used:[[0-9]]\.[[0-9]]*/used:0.0/'
>> > +    sed 's/used:\([[0-9]]\.[[0-9]]*s\|never\)/used:0.0s/'
>> >  }
>>
>> In this test case I expect the flow to be used. My local test runs do
>> not yield "never".
>>
>> Maybe we need a "ovs-appctl time/warp 5000" before dumping the flow?
>> This is used in tests/drop-stats.at  .
>>
>> Tried that but did not work, if you look at the actual number of packets 
>> received by the flow entry, it varies quite a lot from 0 (only learning) 
>> to 5 when I run the test multiple times.
>>
>> I tried changing the following to generate more traffic:
>>
>> -dnl generate some traffic
>> -NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2], [1], 
>> [ignore])
>> +dnl Generate some traffic.
>> +NS_CHECK_EXEC([at_ns0], [ping -q -c 100 -i 0.01 -w 1 10.1.1.2], [1], 
>> [ignore])
>>
>> And then run it 100 times, and the earlier problem did not happen:
>>
>>   sudo bash -c 'for i in {1..100}; do make check-system-userspace 
>> TESTSUITEFLAGS="-k drop -v" || break; echo "EC_DONE: $i"; done'
>>
>> However, I have some times it fails with no traffic:
>>
>>
>> --- - 2023-07-05 21:46:59.860127196 +0200
>> +++ 
>> /home/echaudron/Documents/review/ovs_eric_DROP/tests/system-userspace-testsuite.dir/at-groups/45/stdout
>>  2023-07-05 21:46:59.857641231 +0200
>> @@ -1,2 +1 @@
>> -recirc_id(),in_port(2),eth_type(0x0800),ipv4(frag=no), 
>> packets:0, bytes:0, used:0.0s, actions:drop
>>  
>> Maybe this test needs some more love ;)
>
> The test likely needs a revalidator/wait call, since you're
> checking datapath flows.  Otherwise, it'll be time sensitive.
> Stats are gathered during revalidation.

 That was might thought initially, and I tried it (no luck), but it’s dp 
 flows, not openflow.
>>>
>>> revalidator/wait still fails.
>>>
>>> The only thing I've found that makes this stable is the sleep below.
>>>
>>> --->8---
>>>
>>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
>>> index 50d6c2d05e5d..ab893c5c696b 100644
>>> --- a/tests/system-traffic.at
>>> +++ b/tests/system-traffic.at
>>> @@ -2068,6 +2068,9 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>>>
>>>  dnl Generate some traffic.
>>>  NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2], [1], 
>>> [ignore])
>>> +dnl sleep to let the flow hit the datapath
>>> +sleep 5
>>> +NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2], [1], 
>>> [ignore])
>>>
>>>  AT_CHECK([ovs-appctl dpctl/dump-flows | grep "eth_type(0x0800)" | dnl
>>>sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/;s/,eth(),/,/;' 
>>> | dnl
>>>
>>
>> Yeah, revalidator doesn't matter if we're talking about datapath flows, true.
>> The only thing that matters is how fast the kernel makes counter visible to
>> userpsace.  It may not happen on very short time slots.
>>
>> However, waiting for 5 seconds is not a good solution.  If waiting is the 
>> only
>> option, we should use the OVS_WAIT_UNTIL_EQUAL instead.
> 
> With the following ping command:
> 
>   NS_CHECK_EXEC([at_ns0], [ping -q -c 100 -i 0.01 -w 1 10.1.1.2], [1],
> 
> I never saw the unused issue, only the one that showed no flow entry at all. 
> Maybe with this ping instead and the OVS_WAIT_UNTIL_EQUAL() it might work?

There is no point sending so many packets.  Just send 3 with 0.3 interval
as other tests do and wait for result with OVS_WAIT_UNTIL_EQUAL.
That should always work.  OVS_WAIT_UNTIL_EQUAL will check multiple times
with short

Re: [ovs-dev] [PATCH v2 4/4] tests: system-traffic: add coverage for drop action

2023-07-07 Thread Eelco Chaudron 


On 7 Jul 2023, at 16:06, Ilya Maximets wrote:

> On 7/7/23 15:51, Eric Garver wrote:
>> On Fri, Jul 07, 2023 at 02:09:49PM +0200, Eelco Chaudron wrote:
>>>
>>>
>>> On 7 Jul 2023, at 13:08, Ilya Maximets wrote:
>>>
 On 7/5/23 21:47, Eelco Chaudron wrote:
>
>
> On Wed, Jul 5, 2023 at 6:46 PM Eric Garver  wrote:
>
> On Wed, Jul 05, 2023 at 04:58:47PM +0200, Eelco Chaudron wrote:
> >
> >
> > On 30 Jun 2023, at 21:05, Eric Garver wrote:
> > Hi Eric,
> >
> > I started reviewing the series, and this test was failing every 
> other run for me on ‘check-system-userspace’. I ended up making the 
> following additional change:
> >
> > diff --git a/tests/ofproto-macros.at  
> b/tests/ofproto-macros.at 
> > index d2e6ac768..573ecdd0f 100644
> > --- a/tests/ofproto-macros.at 
> > +++ b/tests/ofproto-macros.at 
> > @@ -120,7 +120,7 @@ strip_xids () {
> >
> >  # Changes all 'used:...' to say 'used:0.0', to make output easier 
> to compare.
> >  strip_used () {
> > -    sed 's/used:[[0-9]]\.[[0-9]]*/used:0.0/'
> > +    sed 's/used:\([[0-9]]\.[[0-9]]*s\|never\)/used:0.0s/'
> >  }
>
> In this test case I expect the flow to be used. My local test runs do
> not yield "never".
>
> Maybe we need a "ovs-appctl time/warp 5000" before dumping the flow?
> This is used in tests/drop-stats.at  .
>
> Tried that but did not work, if you look at the actual number of packets 
> received by the flow entry, it varies quite a lot from 0 (only learning) 
> to 5 when I run the test multiple times.
>
> I tried changing the following to generate more traffic:
>
> -dnl generate some traffic
> -NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2], [1], 
> [ignore])
> +dnl Generate some traffic.
> +NS_CHECK_EXEC([at_ns0], [ping -q -c 100 -i 0.01 -w 1 10.1.1.2], [1], 
> [ignore])
>
> And then run it 100 times, and the earlier problem did not happen:
>
>   sudo bash -c 'for i in {1..100}; do make check-system-userspace 
> TESTSUITEFLAGS="-k drop -v" || break; echo "EC_DONE: $i"; done'
>
> However, I have some times it fails with no traffic:
>
>
> --- - 2023-07-05 21:46:59.860127196 +0200
> +++ 
> /home/echaudron/Documents/review/ovs_eric_DROP/tests/system-userspace-testsuite.dir/at-groups/45/stdout
>  2023-07-05 21:46:59.857641231 +0200
> @@ -1,2 +1 @@
> -recirc_id(),in_port(2),eth_type(0x0800),ipv4(frag=no), 
> packets:0, bytes:0, used:0.0s, actions:drop
>  
> Maybe this test needs some more love ;)

 The test likely needs a revalidator/wait call, since you're
 checking datapath flows.  Otherwise, it'll be time sensitive.
 Stats are gathered during revalidation.
>>>
>>> That was might thought initially, and I tried it (no luck), but it’s dp 
>>> flows, not openflow.
>>
>> revalidator/wait still fails.
>>
>> The only thing I've found that makes this stable is the sleep below.
>>
>> --->8---
>>
>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
>> index 50d6c2d05e5d..ab893c5c696b 100644
>> --- a/tests/system-traffic.at
>> +++ b/tests/system-traffic.at
>> @@ -2068,6 +2068,9 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>>
>>  dnl Generate some traffic.
>>  NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2], [1], [ignore])
>> +dnl sleep to let the flow hit the datapath
>> +sleep 5
>> +NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2], [1], [ignore])
>>
>>  AT_CHECK([ovs-appctl dpctl/dump-flows | grep "eth_type(0x0800)" | dnl
>>sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/;s/,eth(),/,/;' | 
>> dnl
>>
>
> Yeah, revalidator doesn't matter if we're talking about datapath flows, true.
> The only thing that matters is how fast the kernel makes counter visible to
> userpsace.  It may not happen on very short time slots.
>
> However, waiting for 5 seconds is not a good solution.  If waiting is the only
> option, we should use the OVS_WAIT_UNTIL_EQUAL instead.

With the following ping command:

  NS_CHECK_EXEC([at_ns0], [ping -q -c 100 -i 0.01 -w 1 10.1.1.2], [1],

I never saw the unused issue, only the one that showed no flow entry at all. 
Maybe with this ping instead and the OVS_WAIT_UNTIL_EQUAL() it might work?

>
> Best regards, Ilya Maximets.

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

Re: [ovs-dev] [PATCH v2 4/4] tests: system-traffic: add coverage for drop action

2023-07-07 Thread Ilya Maximets 
On 7/7/23 15:51, Eric Garver wrote:
> On Fri, Jul 07, 2023 at 02:09:49PM +0200, Eelco Chaudron wrote:
>>
>>
>> On 7 Jul 2023, at 13:08, Ilya Maximets wrote:
>>
>>> On 7/5/23 21:47, Eelco Chaudron wrote:


 On Wed, Jul 5, 2023 at 6:46 PM Eric Garver  wrote:

 On Wed, Jul 05, 2023 at 04:58:47PM +0200, Eelco Chaudron wrote:
 >
 >
 > On 30 Jun 2023, at 21:05, Eric Garver wrote:
 > Hi Eric,
 >
 > I started reviewing the series, and this test was failing every 
 other run for me on ‘check-system-userspace’. I ended up making the 
 following additional change:
 >
 > diff --git a/tests/ofproto-macros.at  
 b/tests/ofproto-macros.at 
 > index d2e6ac768..573ecdd0f 100644
 > --- a/tests/ofproto-macros.at 
 > +++ b/tests/ofproto-macros.at 
 > @@ -120,7 +120,7 @@ strip_xids () {
 >
 >  # Changes all 'used:...' to say 'used:0.0', to make output easier 
 to compare.
 >  strip_used () {
 > -    sed 's/used:[[0-9]]\.[[0-9]]*/used:0.0/'
 > +    sed 's/used:\([[0-9]]\.[[0-9]]*s\|never\)/used:0.0s/'
 >  }

 In this test case I expect the flow to be used. My local test runs do
 not yield "never".

 Maybe we need a "ovs-appctl time/warp 5000" before dumping the flow?
 This is used in tests/drop-stats.at  .

 Tried that but did not work, if you look at the actual number of packets 
 received by the flow entry, it varies quite a lot from 0 (only learning) 
 to 5 when I run the test multiple times.

 I tried changing the following to generate more traffic:

 -dnl generate some traffic
 -NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2], [1], 
 [ignore])
 +dnl Generate some traffic.
 +NS_CHECK_EXEC([at_ns0], [ping -q -c 100 -i 0.01 -w 1 10.1.1.2], [1], 
 [ignore])

 And then run it 100 times, and the earlier problem did not happen:

   sudo bash -c 'for i in {1..100}; do make check-system-userspace 
 TESTSUITEFLAGS="-k drop -v" || break; echo "EC_DONE: $i"; done'

 However, I have some times it fails with no traffic:


 --- - 2023-07-05 21:46:59.860127196 +0200
 +++ 
 /home/echaudron/Documents/review/ovs_eric_DROP/tests/system-userspace-testsuite.dir/at-groups/45/stdout
  2023-07-05 21:46:59.857641231 +0200
 @@ -1,2 +1 @@
 -recirc_id(),in_port(2),eth_type(0x0800),ipv4(frag=no), packets:0, 
 bytes:0, used:0.0s, actions:drop
  
 Maybe this test needs some more love ;)
>>>
>>> The test likely needs a revalidator/wait call, since you're
>>> checking datapath flows.  Otherwise, it'll be time sensitive.
>>> Stats are gathered during revalidation.
>>
>> That was might thought initially, and I tried it (no luck), but it’s dp 
>> flows, not openflow.
> 
> revalidator/wait still fails.
> 
> The only thing I've found that makes this stable is the sleep below.
> 
> --->8---
> 
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 50d6c2d05e5d..ab893c5c696b 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -2068,6 +2068,9 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>  
>  dnl Generate some traffic.
>  NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2], [1], [ignore])
> +dnl sleep to let the flow hit the datapath
> +sleep 5
> +NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2], [1], [ignore])
>  
>  AT_CHECK([ovs-appctl dpctl/dump-flows | grep "eth_type(0x0800)" | dnl
>sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/;s/,eth(),/,/;' | 
> dnl
> 

Yeah, revalidator doesn't matter if we're talking about datapath flows, true.
The only thing that matters is how fast the kernel makes counter visible to
userpsace.  It may not happen on very short time slots.

However, waiting for 5 seconds is not a good solution.  If waiting is the only
option, we should use the OVS_WAIT_UNTIL_EQUAL instead.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 4/4] tests: system-traffic: add coverage for drop action

2023-07-07 Thread Eric Garver 
On Fri, Jul 07, 2023 at 02:09:49PM +0200, Eelco Chaudron wrote:
> 
> 
> On 7 Jul 2023, at 13:08, Ilya Maximets wrote:
> 
> > On 7/5/23 21:47, Eelco Chaudron wrote:
> >>
> >>
> >> On Wed, Jul 5, 2023 at 6:46 PM Eric Garver  wrote:
> >>
> >> On Wed, Jul 05, 2023 at 04:58:47PM +0200, Eelco Chaudron wrote:
> >> >
> >> >
> >> > On 30 Jun 2023, at 21:05, Eric Garver wrote:
> >> > Hi Eric,
> >> >
> >> > I started reviewing the series, and this test was failing every 
> >> other run for me on ‘check-system-userspace’. I ended up making the 
> >> following additional change:
> >> >
> >> > diff --git a/tests/ofproto-macros.at  
> >> b/tests/ofproto-macros.at 
> >> > index d2e6ac768..573ecdd0f 100644
> >> > --- a/tests/ofproto-macros.at 
> >> > +++ b/tests/ofproto-macros.at 
> >> > @@ -120,7 +120,7 @@ strip_xids () {
> >> >
> >> >  # Changes all 'used:...' to say 'used:0.0', to make output easier 
> >> to compare.
> >> >  strip_used () {
> >> > -    sed 's/used:[[0-9]]\.[[0-9]]*/used:0.0/'
> >> > +    sed 's/used:\([[0-9]]\.[[0-9]]*s\|never\)/used:0.0s/'
> >> >  }
> >>
> >> In this test case I expect the flow to be used. My local test runs do
> >> not yield "never".
> >>
> >> Maybe we need a "ovs-appctl time/warp 5000" before dumping the flow?
> >> This is used in tests/drop-stats.at  .
> >>
> >> Tried that but did not work, if you look at the actual number of packets 
> >> received by the flow entry, it varies quite a lot from 0 (only learning) 
> >> to 5 when I run the test multiple times.
> >>
> >> I tried changing the following to generate more traffic:
> >>
> >> -dnl generate some traffic
> >> -NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2], [1], 
> >> [ignore])
> >> +dnl Generate some traffic.
> >> +NS_CHECK_EXEC([at_ns0], [ping -q -c 100 -i 0.01 -w 1 10.1.1.2], [1], 
> >> [ignore])
> >>
> >> And then run it 100 times, and the earlier problem did not happen:
> >>
> >>   sudo bash -c 'for i in {1..100}; do make check-system-userspace 
> >> TESTSUITEFLAGS="-k drop -v" || break; echo "EC_DONE: $i"; done'
> >>
> >> However, I have some times it fails with no traffic:
> >>
> >>
> >> --- - 2023-07-05 21:46:59.860127196 +0200
> >> +++ 
> >> /home/echaudron/Documents/review/ovs_eric_DROP/tests/system-userspace-testsuite.dir/at-groups/45/stdout
> >>  2023-07-05 21:46:59.857641231 +0200
> >> @@ -1,2 +1 @@
> >> -recirc_id(),in_port(2),eth_type(0x0800),ipv4(frag=no), packets:0, 
> >> bytes:0, used:0.0s, actions:drop
> >>  
> >> Maybe this test needs some more love ;)
> >
> > The test likely needs a revalidator/wait call, since you're
> > checking datapath flows.  Otherwise, it'll be time sensitive.
> > Stats are gathered during revalidation.
> 
> That was might thought initially, and I tried it (no luck), but it’s dp 
> flows, not openflow.

revalidator/wait still fails.

The only thing I've found that makes this stable is the sleep below.

--->8---

diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 50d6c2d05e5d..ab893c5c696b 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -2068,6 +2068,9 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
 
 dnl Generate some traffic.
 NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2], [1], [ignore])
+dnl sleep to let the flow hit the datapath
+sleep 5
+NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2], [1], [ignore])
 
 AT_CHECK([ovs-appctl dpctl/dump-flows | grep "eth_type(0x0800)" | dnl
   sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/;s/,eth(),/,/;' | dnl

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

Re: [ovs-dev] [PATCH v2 4/4] tests: system-traffic: add coverage for drop action

2023-07-07 Thread Eelco Chaudron 


On 7 Jul 2023, at 13:08, Ilya Maximets wrote:

> On 7/5/23 21:47, Eelco Chaudron wrote:
>>
>>
>> On Wed, Jul 5, 2023 at 6:46 PM Eric Garver  wrote:
>>
>> On Wed, Jul 05, 2023 at 04:58:47PM +0200, Eelco Chaudron wrote:
>> >
>> >
>> > On 30 Jun 2023, at 21:05, Eric Garver wrote:
>> > Hi Eric,
>> >
>> > I started reviewing the series, and this test was failing every other 
>> run for me on ‘check-system-userspace’. I ended up making the following 
>> additional change:
>> >
>> > diff --git a/tests/ofproto-macros.at  
>> b/tests/ofproto-macros.at 
>> > index d2e6ac768..573ecdd0f 100644
>> > --- a/tests/ofproto-macros.at 
>> > +++ b/tests/ofproto-macros.at 
>> > @@ -120,7 +120,7 @@ strip_xids () {
>> >
>> >  # Changes all 'used:...' to say 'used:0.0', to make output easier to 
>> compare.
>> >  strip_used () {
>> > -    sed 's/used:[[0-9]]\.[[0-9]]*/used:0.0/'
>> > +    sed 's/used:\([[0-9]]\.[[0-9]]*s\|never\)/used:0.0s/'
>> >  }
>>
>> In this test case I expect the flow to be used. My local test runs do
>> not yield "never".
>>
>> Maybe we need a "ovs-appctl time/warp 5000" before dumping the flow?
>> This is used in tests/drop-stats.at  .
>>
>> Tried that but did not work, if you look at the actual number of packets 
>> received by the flow entry, it varies quite a lot from 0 (only learning) to 
>> 5 when I run the test multiple times.
>>
>> I tried changing the following to generate more traffic:
>>
>> -dnl generate some traffic
>> -NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2], [1], [ignore])
>> +dnl Generate some traffic.
>> +NS_CHECK_EXEC([at_ns0], [ping -q -c 100 -i 0.01 -w 1 10.1.1.2], [1], 
>> [ignore])
>>
>> And then run it 100 times, and the earlier problem did not happen:
>>
>>   sudo bash -c 'for i in {1..100}; do make check-system-userspace 
>> TESTSUITEFLAGS="-k drop -v" || break; echo "EC_DONE: $i"; done'
>>
>> However, I have some times it fails with no traffic:
>>
>>
>> --- - 2023-07-05 21:46:59.860127196 +0200
>> +++ 
>> /home/echaudron/Documents/review/ovs_eric_DROP/tests/system-userspace-testsuite.dir/at-groups/45/stdout
>>  2023-07-05 21:46:59.857641231 +0200
>> @@ -1,2 +1 @@
>> -recirc_id(),in_port(2),eth_type(0x0800),ipv4(frag=no), packets:0, 
>> bytes:0, used:0.0s, actions:drop
>>  
>> Maybe this test needs some more love ;)
>
> The test likely needs a revalidator/wait call, since you're
> checking datapath flows.  Otherwise, it'll be time sensitive.
> Stats are gathered during revalidation.

That was might thought initially, and I tried it (no luck), but it’s dp flows, 
not openflow.

>>
>> >
>> > This is also the reason why the intel tests are failing in patchwork; 
>> https://mail.openvswitch.org/pipermail/ovs-build/2023-June/032011.html 
>> 
>> >
>> > I still need to review the other patches in the series, but some small 
>> comments while testing the patch set:
>> >
>> > We are trying to align all commit messages to start with a Capital and 
>> end with a dot. So in your case:
>> >
>> > ‘tests: system-traffic: Add coverage for drop action.’
>>
>> ACK. I'll address this in v3.
>>

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

Re: [ovs-dev] [PATCH v2 4/4] tests: system-traffic: add coverage for drop action

2023-07-07 Thread Ilya Maximets 
On 7/5/23 21:47, Eelco Chaudron wrote:
> 
> 
> On Wed, Jul 5, 2023 at 6:46 PM Eric Garver  wrote:
> 
> On Wed, Jul 05, 2023 at 04:58:47PM +0200, Eelco Chaudron wrote:
> >
> >
> > On 30 Jun 2023, at 21:05, Eric Garver wrote:
> > Hi Eric,
> >
> > I started reviewing the series, and this test was failing every other 
> run for me on ‘check-system-userspace’. I ended up making the following 
> additional change:
> >
> > diff --git a/tests/ofproto-macros.at  
> b/tests/ofproto-macros.at 
> > index d2e6ac768..573ecdd0f 100644
> > --- a/tests/ofproto-macros.at 
> > +++ b/tests/ofproto-macros.at 
> > @@ -120,7 +120,7 @@ strip_xids () {
> >
> >  # Changes all 'used:...' to say 'used:0.0', to make output easier to 
> compare.
> >  strip_used () {
> > -    sed 's/used:[[0-9]]\.[[0-9]]*/used:0.0/'
> > +    sed 's/used:\([[0-9]]\.[[0-9]]*s\|never\)/used:0.0s/'
> >  }
> 
> In this test case I expect the flow to be used. My local test runs do
> not yield "never".
> 
> Maybe we need a "ovs-appctl time/warp 5000" before dumping the flow?
> This is used in tests/drop-stats.at .
> 
> Tried that but did not work, if you look at the actual number of packets 
> received by the flow entry, it varies quite a lot from 0 (only learning) to 5 
> when I run the test multiple times.
> 
> I tried changing the following to generate more traffic:
> 
> -dnl generate some traffic
> -NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2], [1], [ignore])
> +dnl Generate some traffic.
> +NS_CHECK_EXEC([at_ns0], [ping -q -c 100 -i 0.01 -w 1 10.1.1.2], [1], 
> [ignore])
> 
> And then run it 100 times, and the earlier problem did not happen:
> 
>   sudo bash -c 'for i in {1..100}; do make check-system-userspace 
> TESTSUITEFLAGS="-k drop -v" || break; echo "EC_DONE: $i"; done'
> 
> However, I have some times it fails with no traffic:
> 
> 
> --- - 2023-07-05 21:46:59.860127196 +0200
> +++ 
> /home/echaudron/Documents/review/ovs_eric_DROP/tests/system-userspace-testsuite.dir/at-groups/45/stdout
>  2023-07-05 21:46:59.857641231 +0200
> @@ -1,2 +1 @@
> -recirc_id(),in_port(2),eth_type(0x0800),ipv4(frag=no), packets:0, 
> bytes:0, used:0.0s, actions:drop
>  
> Maybe this test needs some more love ;)

The test likely needs a revalidator/wait call, since you're
checking datapath flows.  Otherwise, it'll be time sensitive.
Stats are gathered during revalidation.

> 
> >
> > This is also the reason why the intel tests are failing in patchwork; 
> https://mail.openvswitch.org/pipermail/ovs-build/2023-June/032011.html 
> 
> >
> > I still need to review the other patches in the series, but some small 
> comments while testing the patch set:
> >
> > We are trying to align all commit messages to start with a Capital and 
> end with a dot. So in your case:
> >
> > ‘tests: system-traffic: Add coverage for drop action.’
> 
> ACK. I'll address this in v3.
> 

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

Re: [ovs-dev] [PATCH v2 4/4] tests: system-traffic: add coverage for drop action

2023-07-05 Thread Eelco Chaudron 
On Wed, Jul 5, 2023 at 6:46 PM Eric Garver  wrote:

> On Wed, Jul 05, 2023 at 04:58:47PM +0200, Eelco Chaudron wrote:
> >
> >
> > On 30 Jun 2023, at 21:05, Eric Garver wrote:
> > Hi Eric,
> >
> > I started reviewing the series, and this test was failing every other
> run for me on ‘check-system-userspace’. I ended up making the following
> additional change:
> >
> > diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at
> > index d2e6ac768..573ecdd0f 100644
> > --- a/tests/ofproto-macros.at
> > +++ b/tests/ofproto-macros.at
> > @@ -120,7 +120,7 @@ strip_xids () {
> >
> >  # Changes all 'used:...' to say 'used:0.0', to make output easier to
> compare.
> >  strip_used () {
> > -sed 's/used:[[0-9]]\.[[0-9]]*/used:0.0/'
> > +sed 's/used:\([[0-9]]\.[[0-9]]*s\|never\)/used:0.0s/'
> >  }
>
> In this test case I expect the flow to be used. My local test runs do
> not yield "never".
>
> Maybe we need a "ovs-appctl time/warp 5000" before dumping the flow?
> This is used in tests/drop-stats.at.
>
> Tried that but did not work, if you look at the actual number of packets
received by the flow entry, it varies quite a lot from 0 (only learning) to
5 when I run the test multiple times.

I tried changing the following to generate more traffic:

-dnl generate some traffic
-NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2], [1],
[ignore])
+dnl Generate some traffic.
+NS_CHECK_EXEC([at_ns0], [ping -q -c 100 -i 0.01 -w 1 10.1.1.2], [1],
[ignore])

And then run it 100 times, and the earlier problem did not happen:

  sudo bash -c 'for i in {1..100}; do make check-system-userspace
TESTSUITEFLAGS="-k drop -v" || break; echo "EC_DONE: $i"; done'

However, I have some times it fails with no traffic:


--- - 2023-07-05 21:46:59.860127196 +0200
+++
/home/echaudron/Documents/review/ovs_eric_DROP/tests/system-userspace-testsuite.dir/at-groups/45/stdout
2023-07-05 21:46:59.857641231 +0200
@@ -1,2 +1 @@
-recirc_id(),in_port(2),eth_type(0x0800),ipv4(frag=no), packets:0,
bytes:0, used:0.0s, actions:drop

Maybe this test needs some more love ;)

>
> > This is also the reason why the intel tests are failing in patchwork;
> https://mail.openvswitch.org/pipermail/ovs-build/2023-June/032011.html
> >
> > I still need to review the other patches in the series, but some small
> comments while testing the patch set:
> >
> > We are trying to align all commit messages to start with a Capital and
> end with a dot. So in your case:
> >
> > ‘tests: system-traffic: Add coverage for drop action.’
>
> ACK. I'll address this in v3.
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 4/4] tests: system-traffic: add coverage for drop action

2023-07-05 Thread Eric Garver 
On Wed, Jul 05, 2023 at 04:58:47PM +0200, Eelco Chaudron wrote:
> 
> 
> On 30 Jun 2023, at 21:05, Eric Garver wrote:
> Hi Eric,
> 
> I started reviewing the series, and this test was failing every other run for 
> me on ‘check-system-userspace’. I ended up making the following additional 
> change:
> 
> diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at
> index d2e6ac768..573ecdd0f 100644
> --- a/tests/ofproto-macros.at
> +++ b/tests/ofproto-macros.at
> @@ -120,7 +120,7 @@ strip_xids () {
> 
>  # Changes all 'used:...' to say 'used:0.0', to make output easier to compare.
>  strip_used () {
> -sed 's/used:[[0-9]]\.[[0-9]]*/used:0.0/'
> +sed 's/used:\([[0-9]]\.[[0-9]]*s\|never\)/used:0.0s/'
>  }

In this test case I expect the flow to be used. My local test runs do
not yield "never".

Maybe we need a "ovs-appctl time/warp 5000" before dumping the flow?
This is used in tests/drop-stats.at.

> 
> This is also the reason why the intel tests are failing in patchwork; 
> https://mail.openvswitch.org/pipermail/ovs-build/2023-June/032011.html
> 
> I still need to review the other patches in the series, but some small 
> comments while testing the patch set:
> 
> We are trying to align all commit messages to start with a Capital and end 
> with a dot. So in your case:
> 
> ‘tests: system-traffic: Add coverage for drop action.’

ACK. I'll address this in v3.

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

Re: [ovs-dev] [PATCH v2 4/4] tests: system-traffic: add coverage for drop action

2023-07-05 Thread Eelco Chaudron 


On 30 Jun 2023, at 21:05, Eric Garver wrote:
Hi Eric,

I started reviewing the series, and this test was failing every other run for 
me on ‘check-system-userspace’. I ended up making the following additional 
change:

diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at
index d2e6ac768..573ecdd0f 100644
--- a/tests/ofproto-macros.at
+++ b/tests/ofproto-macros.at
@@ -120,7 +120,7 @@ strip_xids () {

 # Changes all 'used:...' to say 'used:0.0', to make output easier to compare.
 strip_used () {
-sed 's/used:[[0-9]]\.[[0-9]]*/used:0.0/'
+sed 's/used:\([[0-9]]\.[[0-9]]*s\|never\)/used:0.0s/'
 }

This is also the reason why the intel tests are failing in patchwork; 
https://mail.openvswitch.org/pipermail/ovs-build/2023-June/032011.html

I still need to review the other patches in the series, but some small comments 
while testing the patch set:

We are trying to align all commit messages to start with a Capital and end with 
a dot. So in your case:

‘tests: system-traffic: Add coverage for drop action.’

> Signed-off-by: Eric Garver 
> ---
>  tests/system-traffic.at | 30 ++
>  1 file changed, 30 insertions(+)
>
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 4c378e1d02b0..31c1ef46d561 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -2048,6 +2048,36 @@ masks-cache:size:256
>  OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
>
> +AT_SETUP([datapath - drop action])
> +OVS_TRAFFIC_VSWITCHD_START()
> +OVS_CHECK_DROP_ACTION()
> +AT_KEYWORDS(drop_action)
> +
> +ADD_NAMESPACES(at_ns0, at_ns1)
> +
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
> +
> +AT_DATA([flows.txt], [dnl
> +table=0, dl_type=0x806, actions=normal
> +table=0, in_port=ovs-p0, actions=goto_table:1
> +table=1, in_port=ovs-p0, actions=goto_table:2
> +table=2, in_port=ovs-p0, actions=resubmit(,1)
> +])
> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> +
> +dnl generate some traffic

‘Generate some traffic.’

> +NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2], [1], [ignore])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-flows | grep "eth_type(0x0800)" | dnl
> +  sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/;s/,eth(),/,/;' | 
> dnl
> +  strip_recirc | strip_stats | strip_used | sort], [0], [dnl
> +recirc_id(),in_port(2),eth_type(0x0800),ipv4(frag=no), packets:0, 
> bytes:0, used:0.0s, actions:drop
> +])
> +
> +OVS_VSWITCHD_STOP(["/|WARN|/d"])
> +AT_CLEANUP
> +
>  AT_SETUP([datapath - simulated flow action update])
>  OVS_TRAFFIC_VSWITCHD_START()
>
> -- 
> 2.39.0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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