On Mon, Jun 7, 2021 at 1:31 PM Toms Atteka <[email protected]> wrote:
>
> This optimization caused FLOW_TNL_F_UDPIF flag not to be used in
> hash calculation for geneve tunnel when revalidating flows which
> resulted in different cache hash values and incorrect behaviour.
>
> Added test to prevent regression.
>
> CC: Jesse Gross <[email protected]>
> Fixes: 6728d578f64e ("dpif-netdev: Translate Geneve options per-flow, not
> per-packet.")
> Reported-at: https://github.com/vmware-tanzu/antrea/issues/897
> Signed-off-by: Toms Atteka <[email protected]>
> ---
> lib/tun-metadata.c | 2 +-
> tests/system-traffic.at | 54 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/lib/tun-metadata.c b/lib/tun-metadata.c
> index c0b0ae044..af0bcbde8 100644
> --- a/lib/tun-metadata.c
> +++ b/lib/tun-metadata.c
> @@ -828,7 +828,7 @@ tun_metadata_to_geneve_nlattr(const struct flow_tnl *tun,
> } else {
> tun_metadata_to_geneve_nlattr_mask(key, tun, flow, b);
> }
> - } else if (flow->metadata.present.len || is_mask) {
> + } else {
I reverted the line above, but the regression prevention test you
added below still seems to pass. So - does this regression prevention
test serve any purpose or am I doing something wrong? Here is proof:
18: datapath - n ok
19: datapath - flow resume with geneve tun_metadata^C
system-kmod-testsuite: WARNING: caught signal INT, bailing out
make: *** [Makefile:6871: check-kernel] Error 1
aatteka@laptop:/tmp/ovs$ git diff
diff --git a/lib/tun-metadata.c b/lib/tun-metadata.c
index af0bcbde8..c0b0ae044 100644
--- a/lib/tun-metadata.c
+++ b/lib/tun-metadata.c
@@ -828,7 +828,7 @@ tun_metadata_to_geneve_nlattr(const struct flow_tnl *tun,
} else {
tun_metadata_to_geneve_nlattr_mask(key, tun, flow, b);
}
- } else {
+ } else if (flow->metadata.present.len || is_mask) {
nl_msg_put_unspec(b, OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS,
tun->metadata.opts.gnv,
flow->metadata.present.len);
> nl_msg_put_unspec(b, OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS,
> tun->metadata.opts.gnv,
> flow->metadata.present.len);
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index fb5b9a36d..483cc7e83 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -574,6 +574,60 @@ 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 - ping over geneve tunnel, delete flow regression])
> +OVS_CHECK_GENEVE()
> +
> +OVS_TRAFFIC_VSWITCHD_START()
> +ADD_BR([br-underlay])
> +
> +AT_DATA([flows.txt], [dnl
> +priority=100,icmp actions=resubmit(,10)
> +priority=0 actions=NORMAL
> +table=10, priority=100, ip, actions=ct(table=20,zone=65520)
> +table=20, priority=200, ip, ct_state=-new+trk, actions=resubmit(,30)
> +table=20, priority=100, ip, ct_state=+new, actions=resubmit(,30)
> +table=20, priority=50, ip, actions=DROP
> +table=30, priority=100, ip, actions=ct(commit,table=40,zone=65520)
> +table=40, actions=normal
> +])
> +
> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> +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, "172.31.1.1/24")
> +AT_CHECK([ip addr add dev br-underlay "172.31.1.100/24"])
> +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_TUNNEL([geneve], [br0], [at_gnv0], [172.31.1.1], [10.1.1.100/24])
> +ADD_NATIVE_TUNNEL([geneve], [ns_gnv0], [at_ns0], [172.31.1.100],
> [10.1.1.1/24],
> + [vni 0])
> +
> +dnl First, check the underlay
> +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 172.31.1.100 |
> FORMAT_PING], [0], [dnl
> +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> +])
> +
> +dnl ping over tunnel should work
> +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.100 | FORMAT_PING],
> [0], [dnl
> +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> +])
> +
> +AT_CHECK([ovs-ofctl del-flows br0 "ct_state=+new"])
> +
> +dnl ping should not go through after removal of the flow
> +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.100 | FORMAT_PING],
> [0], [dnl
> +7 packets transmitted, 0 received, 100% packet loss, time 0ms
> +])
> +
> +OVS_TRAFFIC_VSWITCHD_STOP(["/|ERR|/d
> +/|WARN|/d"])
> +AT_CLEANUP
> +
> AT_SETUP([datapath - flow resume with geneve tun_metadata])
> OVS_CHECK_GENEVE()
>
> --
> 2.25.1
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
