[ovs-dev] [PATCH] system-offloads-traffic.at: Add sFlow offload test cases

Fri Oct 8 06:55:25 UTC 2021

On 10/8/2021 2:47 PM, Eelco Chaudron wrote:
> Hi Chris,
>
>   I hope you enjoyed the holiday. And don't worry about the delay, as I try to only spend time reviewing stuff on Fridays.
>
> Thanks for fixing the comments, and I will re-review once it's integrated into the sflow patchset.
Sure.

Thanks,
Chris
>
> //Eelco
>
> On 8 Oct 2021, at 8:23, Chris Mi wrote:
>
>> Hi Eelco,
>>
>> Sorry for the late reply due to the long National Day holiday in China.
>>
>> On 10/1/2021 10:34 PM, Eelco Chaudron wrote:
>>> Thanks for working on the test cases. I have two small requests below to make the tests also verify the datapath actions.
>>>
>>> And as you already discussed with Simon this should be part of the generic TC sFlow patchset.
>>>
>>> //Eelco
>>>
>>> On 16 Sep 2021, at 10:38, Chris Mi wrote:
>>>
>>>> Add two sFlow offload test caes:
>>>>
>>>>     3: sflow offloads with sampling=1 - ping between two ports - offloads enabled ok
>>>>     4: sflow offloads with sampling=2 - ping between two ports - offloads enabled ok
>>>>
>>>> Signed-off-by: Chris Mi <cmi at nvidia.com>
>>>> ---
>>>>    tests/system-offloads-traffic.at | 84 ++++++++++++++++++++++++++++++++
>>>>    1 file changed, 84 insertions(+)
>>>>
>>>> diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at
>>>> index be63057bb..f263c9183 100644
>>>> --- a/tests/system-offloads-traffic.at
>>>> +++ b/tests/system-offloads-traffic.at
>>>> @@ -5,6 +5,7 @@ AT_BANNER([datapath offloads])
>>>>    # Normilizes output ports, recirc_id, packets and macs.
>>>>    #
>>>>    m4_define([DUMP_CLEAN_SORTED], [sed -e 's/used:[[0-9]].[[0-9]]*s/used:0.001s/;s/eth(src=[[a-z0-9:]]*,dst=[[a-z0-9:]]*)/eth(macs)/;s/actions:[[0-9,]]*/actions:output/;s/recirc_id(0),//' | sort])
>>>> +m4_define([DUMP_CLEAN_SORTED_SFLOW], [sed -e 's/used:[[0-9]].[[0-9]]*s/used:0.001s/;s/eth(src=[[a-z0-9:]]*,dst=[[a-z0-9:]]*)/eth(macs)/;s/actions:.*$/actions:output/;s/recirc_id(0),//' | sort])
>>>>
>>>>    AT_SETUP([offloads - ping between two ports - offloads disabled])
>>>>    OVS_TRAFFIC_VSWITCHD_START()
>>>> @@ -71,6 +72,89 @@ AT_CHECK([ovs-appctl upcall/show | grep -E "offloaded flows : [[1-9]]"], [0], [i
>>>>    OVS_TRAFFIC_VSWITCHD_STOP
>>>>    AT_CLEANUP
>>>>
>>>> +AT_SETUP([sflow offloads with sampling=1 - ping between two ports - offloads enabled])
>>>> +OVS_TRAFFIC_VSWITCHD_START()
>>>> +
>>>> +on_exit 'kill `cat test-sflow.pid`'
>>>> +AT_CHECK([ovstest test-sflow --log-file --detach --no-chdir --pidfile 0:127.0.0.1 > sflow.log], [0], [], [ignore])
>>>> +AT_CAPTURE_FILE([sflow.log])
>>>> +PARSE_LISTENING_PORT([test-sflow.log], [SFLOW_PORT])
>>>> +
>>>> +AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true])
>>>> +
>>>> +AT_CHECK([ovs-appctl -t ovsdb-server exit])
>>>> +AT_CHECK([ovs-appctl -t ovs-vswitchd exit])
>>>> +AT_CHECK([rm -f ovsdb-server.pid])
>>>> +AT_CHECK([rm -f ovs-vswitchd.pid])
>>>> +AT_CHECK([ovsdb-server --detach --no-chdir --pidfile --log-file --remote=punix:$OVS_RUNDIR/db.sock], [0], [], [stderr])
>>>> +AT_CHECK([ovs-vswitchd --detach --no-chdir --pidfile --log-file -vvconn -vofproto_dpif -vunixctl], [0], [], [stderr])
>>>> +on_exit "kill `cat ovsdb-server.pid`"
>>>> +on_exit "kill_ovs_vswitchd `cat ovs-vswitchd.pid`"
>>>> +
>>>> +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
>>>> +
>>>> +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_CHECK([ovs-appctl dpctl/dump-flows], [0], [ignore])
>>>> +
>>>> +NS_CHECK_EXEC([at_ns0], [ping -c 10 -w 12 10.1.1.2 | FORMAT_PING], [0], [dnl
>>>> +10 packets transmitted, 10 received, 0% packet loss, time 0ms
>>>> +])
>>>> +AT_CHECK([ovs-vsctl -- --id=@sflow create sflow agent=lo target=\"127.0.0.1:$SFLOW_PORT\" header=128 sampling=1 polling=100 -- set bridge br0 sflow=@sflow], [0], [ignore])
>>>> +NS_CHECK_EXEC([at_ns0], [ping -c 10 -w 12 10.1.1.2 | FORMAT_PING], [0], [dnl
>>>> +10 packets transmitted, 10 received, 0% packet loss, time 0ms
>>>> +])
>>>> +
>>>> +AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep "actions:userspace" |grep "sFlow" | grep "eth_type(0x0800)" | DUMP_CLEAN_SORTED_SFLOW], [0], [dnl
>>>> +in_port(2),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:10, bytes:840, used:0.001s, actions:output
>>>> +in_port(3),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:10, bytes:840, used:0.001s, actions:output
>>>> +])
>>> I think your filter to much in this case, as we would like to verify the correct datapath format for actions, so I think it should be something like:
>>>
>>> actions:userspace(pid=XXX,sFlow(vid=0,pcp=0,output=341),actions),3
>>>
>>> Where you can ignore/replace the actual pid value, but you need to find a way to map the correct if_index.
>> Done.
>>>> +
>>>> +OVS_TRAFFIC_VSWITCHD_STOP
>>>> +OVS_APP_EXIT_AND_WAIT([test-sflow])
>>>> +count=`cat sflow.log | wc -l`
>>>> +AT_CHECK([[[[ $count -le 22 && $count -ge 18 ]]]])
>>>> +AT_CLEANUP
>>>> +
>>>> +AT_SETUP([sflow offloads with sampling=2 - ping between two ports - offloads enabled])
>>>> +OVS_TRAFFIC_VSWITCHD_START()
>>>> +
>>>> +on_exit 'kill `cat test-sflow.pid`'
>>>> +AT_CHECK([ovstest test-sflow --log-file --detach --no-chdir --pidfile 0:127.0.0.1 > sflow.log], [0], [], [ignore])
>>>> +AT_CAPTURE_FILE([sflow.log])
>>>> +PARSE_LISTENING_PORT([test-sflow.log], [SFLOW_PORT])
>>>> +
>>>> +AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true])
>>>> +
>>>> +AT_CHECK([ovs-appctl -t ovsdb-server exit])
>>>> +AT_CHECK([ovs-appctl -t ovs-vswitchd exit])
>>>> +AT_CHECK([rm -f ovsdb-server.pid])
>>>> +AT_CHECK([rm -f ovs-vswitchd.pid])
>>>> +AT_CHECK([ovsdb-server --detach --no-chdir --pidfile --log-file --remote=punix:$OVS_RUNDIR/db.sock], [0], [], [stderr])
>>>> +AT_CHECK([ovs-vswitchd --detach --no-chdir --pidfile --log-file -vvconn -vofproto_dpif -vunixctl], [0], [], [stderr])
>>>> +on_exit "kill `cat ovsdb-server.pid`"
>>>> +on_exit "kill_ovs_vswitchd `cat ovs-vswitchd.pid`"
>>>> +
>>>> +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
>>>> +
>>>> +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_CHECK([ovs-appctl dpctl/dump-flows], [0], [ignore])
>>>> +
>>>> +AT_CHECK([ovs-vsctl -- --id=@sflow create sflow agent=lo target=\"127.0.0.1:$SFLOW_PORT\" header=128 sampling=2 polling=100 -- set bridge br0 sflow=@sflow], [0], [ignore])
>>>> +NS_CHECK_EXEC([at_ns0], [ping -c 1000 -i 0.01 -w 12 10.1.1.2], [0], [ignore])
>>>> +AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep "actions:sample" | grep "sFlow" | grep "eth_type(0x0800)"], [0], [ignore])
>>> Why is this ignore? It should match flow/action,see also above:
>>>
>>>     sample(sample=50.0%,actions(userspace(pid=XXX,sFlow(vid=0,pcp=0,output=361),actions))),2
>> Because I think we have enough grep, so I ignored it. Anyway, it's done now.
>> I will address your other comments in generic TC sFlow patchset and put this patch at the end of it.
>>
>> Thanks,
>> Chris
>>>> +
>>>> +OVS_TRAFFIC_VSWITCHD_STOP
>>>> +OVS_APP_EXIT_AND_WAIT([test-sflow])
>>>> +count=`cat sflow.log | wc -l`
>>>> +AT_CHECK([[[[ $count -le 1100 && $count -ge 900 ]]]])
>>>> +AT_CLEANUP
>>>> +
>>>>    AT_SETUP([offloads - set ingress_policing_rate and ingress_policing_burst - offloads disabled])
>>>>    AT_KEYWORDS([ingress_policing])
>>>>    AT_SKIP_IF([test $HAVE_TC = "no"])
>>>> -- 
>>>> 2.27.0