On 20 Dec 2022, at 13:42, Ilya Maximets wrote:
> On 12/7/22 17:17, Eelco Chaudron wrote:
>> The dpif_execute_helper_cb() function is supposed to add the
>> OVS_ACTION_ATTR_SET(OVS_KEY_ATTR_TUNNEL()) action to the
>> list of actions when passing it down to the kernel.
>>
>> This function was only checking if the IPv4 destination
>> address was set, not both. This patch fixes this, including
>> a datapath testcase.
>>
>> Fixes: 076caa2fb077 ("ofproto: Meter translation.")
>> Signed-off-by: Eelco Chaudron <[email protected]>
>> ---
>> lib/dpif.c | 2 +-
>> tests/system-traffic.at | 44 ++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 45 insertions(+), 1 deletion(-)
>
> Hi, Eelco. Good catch!
>
> I wonder if we can have a unit test instead of a system test here.
> The issue doesn't seem to depend on the datapath implementation.
Actually, it does, it’s only the kernel datapath that needs the additional
attribute.
Userspace works fine without it, even the source code mentions this (see diff
below).
> Maybe something similar to what we have in tests/tunnel-push-pop.at ?
> We can set IPs and capture packets in pcap files on dummy ports
> as well. Probably, the 'tunnel_push_pop - packet_out debug_slow'
> test can be used as a reference.
>
> A couple of small comments inline.
>
> Best regards, Ilya Maximets.
>
>>
>> diff --git a/lib/dpif.c b/lib/dpif.c
>> index 40f5fe446..fe4db83fb 100644
>> --- a/lib/dpif.c
>> +++ b/lib/dpif.c
>> @@ -1213,7 +1213,7 @@ dpif_execute_helper_cb(void *aux_, struct
>> dp_packet_batch *packets_,
>> /* The Linux kernel datapath throws away the tunnel information
>> * that we supply as metadata. We have to use a "set" action to
>> * supply it. */
>> - if (md->tunnel.ip_dst) {
>> + if (flow_tnl_dst_is_set(&md->tunnel)) {
>> odp_put_tunnel_action(&md->tunnel, &execute_actions, NULL);
>> }
>> ofpbuf_put(&execute_actions, action,
>> NLA_ALIGN(action->nla_len));
>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
>> index e5403519f..91e15ddef 100644
>> --- a/tests/system-traffic.at
>> +++ b/tests/system-traffic.at
>> @@ -855,6 +855,50 @@ NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 -w
>> 2 10.1.1.100 | FORMAT_PI
>> OVS_TRAFFIC_VSWITCHD_STOP
>> AT_CLEANUP
>>
>> +AT_SETUP([datapath - slow_action on geneve6 tunnel])
>> +AT_SKIP_IF([test $HAVE_TCPDUMP = no])
>> +OVS_CHECK_TUNNEL_TSO()
>> +OVS_CHECK_GENEVE_UDP6ZEROCSUM()
>> +
>> +OVS_TRAFFIC_VSWITCHD_START()
>> +ADD_BR([br-underlay])
>> +
>> +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
>> +AT_CHECK([ovs-ofctl add-flow br-underlay "actions=normal"])
>> +
>> +ADD_NAMESPACES(at_ns0)
>> +
>> +dnl Set up underlay link from host into the namespace using veth pair.
>> +ADD_VETH(p0, at_ns0, br-underlay, "fc00::1/64", [], [], "nodad")
>> +AT_CHECK([ip addr add dev br-underlay "fc00::100/64" nodad])
>> +AT_CHECK([ip link set dev br-underlay up])
>> +
>> +dnl Set up tunnel endpoints on OVS outside the namespace and with a native
>> +dnl linux device inside the namespace.
>> +ADD_OVS_TUNNEL6([geneve], [br0], [at_gnv0], [fc00::1], [10.1.1.100/24])
>> +ADD_NATIVE_TUNNEL6([geneve], [ns_gnv0], [at_ns0], [fc00::100],
>> [10.1.1.1/24],
>> + [vni 0 udp6zerocsumtx udp6zerocsumrx])
>> +AT_CHECK([ovs-ofctl add-flow br0 "table=37,actions=at_gnv0"])
>> +
>> +OVS_WAIT_UNTIL([ip netns exec at_ns0 ping6 -c 1 fc00::100])
>> +
>> +dnl First, check the underlay.
>> +NS_CHECK_EXEC([at_ns0], [ping6 -q -c 3 -i 0.3 -w 2 fc00::100 |
>> FORMAT_PING], [0], [dnl
>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms
>> +])
>> +
>> +dnl Start tcpdump to capture the encapsulated packets.
>> +NETNS_DAEMONIZE([at_ns0], [tcpdump -l -n -xx -U -i p0 > p0.pcap],
>> [tcpdump.pid])
>
> This doesn't generate a pcap file AFAICT, so the name p0.pcap is a bit
> misleading.
You are right will change the name in v2.
>> +sleep 1
>> +
>> +dnl Generate a single packet trough the controler that needs an ARP
>> modification
>> +AT_CHECK([ovs-ofctl -O OpenFlow15 packet-out br0 "in_port=controller
>> packet=fffffffffffffa163e949d8008060001080006040001fa163e949d80c0a820300000000000000a0000fe
>>
>> actions=set_field:0xa0000f4->reg1,move:NXM_NX_XXREG0[[64..95]]->NXM_OF_ARP_SPA[[]],resubmit(,37)"])
>
>
> As an alternative, we may use 'actions=debug_slow,<...>' to force the
> slow action execution in userspace. This should ensure that we're
> testing what we want to test.
This action does not seem to work with packet-out, so I’ll keep it as is for
now.
>> +
>> +dnl Stop OVS and tcpdump and verify the results.
>> +OVS_TRAFFIC_VSWITCHD_STOP
>> +AT_CHECK([grep -Eq "IP6 fc00::100\..*> fc00::1.geneve: Geneve, Flags
>> \[[none\]], vni 0x0: ARP, Request who-has 10\.0\.0\.254 tell 10\.0\.0\.244,
>> length 28" p0.pcap])
>> +AT_CLEANUP
>> +
>> AT_SETUP([datapath - ping over gre tunnel by simulated packets])
>> OVS_CHECK_TUNNEL_TSO()
>> OVS_CHECK_MIN_KERNEL(3, 10)
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
