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 <echau...@redhat.com>
> ---
> 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.
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.
> +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.
> +
> +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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
