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 <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. >>> >>> 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. > > But I guess the -w 2 can change to -w 1 to save another second.
Since we do no expect any replies, there is no need to wait 2 seconds, sure. > >> That should always work. OVS_WAIT_UNTIL_EQUAL will check multiple times >> with short intervals. First time after 0.1 seconds and 1 second for >> subsequent checks. It will wait 30 seconds for the output to become equal, >> then it will fail. >> >> Best regards, Ilya Maximets. > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
