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 <e...@garver.life> 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 <http://ofproto-macros.at> 
>>>> b/tests/ofproto-macros.at <http://ofproto-macros.at>
>>>>     > index d2e6ac768..573ecdd0f 100644
>>>>     > --- a/tests/ofproto-macros.at <http://ofproto-macros.at>
>>>>     > +++ b/tests/ofproto-macros.at <http://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 <http://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(<recirc>),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

Reply via email to