There are 3 flows for matching ECMP symmetric reply that are different in a single match part that is the protocol (udp/tcp/sctp) for ct.new traffic and additional 3 flows (udp/tcp/sctp) for ct.est && !ct.rpl.
Remove the protocol requirement from those flows and merge the remaining two onto single flow. This also reduces the logical flows per ECMP reply from 6 to 1. Reported-at: https://issues.redhat.com/browse/FDP-358 Suggested-by: Dumitru Ceara <[email protected]> Signed-off-by: Ales Musil <[email protected]> --- v2: Rebase on top of current main. Use the original idea to reduce the number of flows as fix for FDP-358. --- northd/northd.c | 79 ++------------------------------------------- tests/ovn.at | 4 +-- tests/system-ovn.at | 41 +++++++++++++++++------ 3 files changed, 36 insertions(+), 88 deletions(-) diff --git a/northd/northd.c b/northd/northd.c index 01eec64ca..99a582439 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -10518,7 +10518,6 @@ add_ecmp_symmetric_reply_flows(struct lflow_table *lflows, struct lflow_ref *lflow_ref) { const struct nbrec_logical_router_static_route *st_route = route->route; - struct ds base_match = DS_EMPTY_INITIALIZER; struct ds match = DS_EMPTY_INITIALIZER; struct ds actions = DS_EMPTY_INITIALIZER; struct ds ecmp_reply = DS_EMPTY_INITIALIZER; @@ -10530,14 +10529,14 @@ add_ecmp_symmetric_reply_flows(struct lflow_table *lflows, /* If symmetric ECMP replies are enabled, then packets that arrive over * an ECMP route need to go through conntrack. */ - ds_put_format(&base_match, "inport == %s && ip%s.%s == %s", + ds_put_format(&match, "inport == %s && ip%s.%s == %s", out_port->json_key, IN6_IS_ADDR_V4MAPPED(&route->prefix) ? "4" : "6", route->is_src_route ? "dst" : "src", cidr); free(cidr); ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DEFRAG, 100, - ds_cstr(&base_match), "ct_next;", + ds_cstr(&match), "ct_next;", &st_route->header_, lflow_ref); /* And packets that go out over an ECMP route need conntrack */ @@ -10551,78 +10550,7 @@ add_ecmp_symmetric_reply_flows(struct lflow_table *lflows, * NOTE: we purposely are not clearing match before this * ds_put_cstr() call. The previous contents are needed. */ - ds_put_format(&match, "%s && (ct.new && !ct.est) && tcp", - ds_cstr(&base_match)); - ds_put_format(&actions, - "ct_commit { ct_label.ecmp_reply_eth = eth.src; " - " %s = %" PRId64 ";}; " - "next;", - ct_ecmp_reply_port_match, out_port->sb->tunnel_key); - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_ECMP_STATEFUL, 100, - ds_cstr(&match), ds_cstr(&actions), - &st_route->header_, - lflow_ref); - ds_clear(&match); - ds_put_format(&match, "%s && (ct.new && !ct.est) && udp", - ds_cstr(&base_match)); - ds_clear(&actions); - ds_put_format(&actions, - "ct_commit { ct_label.ecmp_reply_eth = eth.src; " - " %s = %" PRId64 ";}; " - "next;", - ct_ecmp_reply_port_match, out_port->sb->tunnel_key); - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_ECMP_STATEFUL, 100, - ds_cstr(&match), ds_cstr(&actions), - &st_route->header_, - lflow_ref); - ds_clear(&match); - ds_put_format(&match, "%s && (ct.new && !ct.est) && sctp", - ds_cstr(&base_match)); - ds_clear(&actions); - ds_put_format(&actions, - "ct_commit { ct_label.ecmp_reply_eth = eth.src; " - " %s = %" PRId64 ";}; " - "next;", - ct_ecmp_reply_port_match, out_port->sb->tunnel_key); - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_ECMP_STATEFUL, 100, - ds_cstr(&match), ds_cstr(&actions), - &st_route->header_, - lflow_ref); - - ds_clear(&match); - ds_put_format(&match, - "%s && (!ct.rpl && ct.est) && tcp", - ds_cstr(&base_match)); - ds_clear(&actions); - ds_put_format(&actions, - "ct_commit { ct_label.ecmp_reply_eth = eth.src; " - " %s = %" PRId64 ";}; " - "next;", - ct_ecmp_reply_port_match, out_port->sb->tunnel_key); - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_ECMP_STATEFUL, 100, - ds_cstr(&match), ds_cstr(&actions), - &st_route->header_, - lflow_ref); - - ds_clear(&match); - ds_put_format(&match, - "%s && (!ct.rpl && ct.est) && udp", - ds_cstr(&base_match)); - ds_clear(&actions); - ds_put_format(&actions, - "ct_commit { ct_label.ecmp_reply_eth = eth.src; " - " %s = %" PRId64 ";}; " - "next;", - ct_ecmp_reply_port_match, out_port->sb->tunnel_key); - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_ECMP_STATEFUL, 100, - ds_cstr(&match), ds_cstr(&actions), - &st_route->header_, - lflow_ref); - ds_clear(&match); - ds_put_format(&match, - "%s && (!ct.rpl && ct.est) && sctp", - ds_cstr(&base_match)); - ds_clear(&actions); + ds_put_cstr(&match, " && !ct.rpl && (ct.new || ct.est)"); ds_put_format(&actions, "ct_commit { ct_label.ecmp_reply_eth = eth.src; " " %s = %" PRId64 ";}; " @@ -10676,7 +10604,6 @@ add_ecmp_symmetric_reply_flows(struct lflow_table *lflows, action, &st_route->header_, lflow_ref); - ds_destroy(&base_match); ds_destroy(&match); ds_destroy(&actions); ds_destroy(&ecmp_reply); diff --git a/tests/ovn.at b/tests/ovn.at index 3e83cb2c0..361a81c55 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -28987,7 +28987,7 @@ AT_CHECK([ grep "priority=200" | \ grep -c "move:NXM_NX_CT_LABEL\\[[\\]]->NXM_NX_XXREG1\\[[\\]],move:NXM_NX_XXREG1\\[[32..79\\]]->NXM_OF_ETH_DST" done; :], [0], [dnl -6 +2 1 0 0 @@ -29112,7 +29112,7 @@ AT_CHECK([ grep "priority=200" | \ grep -c "move:NXM_NX_CT_LABEL\\[[\\]]->NXM_NX_XXREG1\\[[\\]],move:NXM_NX_XXREG1\\[[32..79\\]]->NXM_OF_ETH_DST" done; :], [0], [dnl -6 +2 1 0 0 diff --git a/tests/system-ovn.at b/tests/system-ovn.at index 8e3acd57e..af79caffe 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -6111,6 +6111,10 @@ on_exit 'ovs-ofctl dump-flows br-int' NETNS_DAEMONIZE([alice1], [nc -l -k 80], [alice1.pid]) NS_CHECK_EXEC([bob1], [nc -z 10.0.0.2 80], [0]) +NS_CHECK_EXEC([bob1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.2 | FORMAT_PING], \ +[0], [dnl +3 packets transmitted, 3 received, 0% packet loss, time 0ms +]) # Ensure conntrack entry is present. We should not try to predict # the tunnel key for the output port, so we strip it from the labels @@ -6118,17 +6122,19 @@ NS_CHECK_EXEC([bob1], [nc -z 10.0.0.2 80], [0]) AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.0.1) | \ sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | sed -e 's/mark=[[0-9]]*/mark=<cleared>/'], [0], [dnl +icmp,orig=(src=172.16.0.1,dst=10.0.0.2,id=<cleared>,type=8,code=0),reply=(src=10.0.0.2,dst=172.16.0.1,id=<cleared>,type=0,code=0),zone=<cleared>,mark=<cleared>,labels=0x401020400000000 tcp,orig=(src=172.16.0.1,dst=10.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=10.0.0.2,dst=172.16.0.1,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=<cleared>,labels=0x401020400000000,protoinfo=(state=<cleared>) ]) # Ensure datapaths show conntrack states as expected # Like with conntrack entries, we shouldn't try to predict # port binding tunnel keys. So omit them from expected labels. +ovs-appctl dpctl/dump-flows | grep 'ct_state(+new-est-rpl+trk).*ct(.*label=0x401020400000000/.*)' AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(+new-est-rpl+trk).*ct(.*label=0x401020400000000/.*)' -c], [0], [dnl -1 +2 ]) AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(-new+est+rpl+trk).*ct_label(0x401020400000000)' -c], [0], [dnl -1 +2 ]) # Flush conntrack entries for easier output parsing of next test. @@ -6142,16 +6148,21 @@ ovn-nbctl set Logical_Switch_Port r2-ext \ ovn-nbctl --wait=hv sync NS_CHECK_EXEC([bob1], [nc -z 10.0.0.2 80], [0]) +NS_CHECK_EXEC([bob1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.2 | FORMAT_PING], \ +[0], [dnl +3 packets transmitted, 3 received, 0% packet loss, time 0ms +]) AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(+new-est-rpl+trk).*ct(.*label=0x1001020400000000/.*)' -c], [0], [dnl -1 +2 ]) AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(-new+est+rpl+trk).*ct_label(0x1001020400000000)' -c], [0], [dnl -1 +2 ]) AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep 0x1001020400000000 | FORMAT_CT(172.16.0.1) | \ sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | -sed -e 's/mark=[[0-9]]*/mark=<cleared>/'], [0], [dnl +sed -e 's/mark=[[0-9]]*/mark=<cleared>/' | sort], [0], [dnl +icmp,orig=(src=172.16.0.1,dst=10.0.0.2,id=<cleared>,type=8,code=0),reply=(src=10.0.0.2,dst=172.16.0.1,id=<cleared>,type=0,code=0),zone=<cleared>,mark=<cleared>,labels=0x1001020400000000 tcp,orig=(src=172.16.0.1,dst=10.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=10.0.0.2,dst=172.16.0.1,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=<cleared>,labels=0x1001020400000000,protoinfo=(state=<cleared>) ]) # Check entries in table 76 and 77 expires w/o traffic @@ -6303,16 +6314,20 @@ on_exit 'ovs-ofctl dump-flows br-int' NETNS_DAEMONIZE([alice1], [nc -6 -l -k 80], [alice1.pid]) NS_CHECK_EXEC([bob1], [nc -6 -z fd01::2 80], [0]) +NS_CHECK_EXEC([bob1], [ping -q -c 3 -i 0.3 -w 2 fd01::2 | FORMAT_PING], \ +[0], [dnl +3 packets transmitted, 3 received, 0% packet loss, time 0ms +]) # Ensure datapaths show conntrack states as expected # Like with conntrack entries, we shouldn't try to predict # port binding tunnel keys. So omit them from expected labels. AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(+new-est-rpl+trk).*ct(.*label=0x401020400000000/.*)' -c], [0], [dnl -1 +2 ]) AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(-new+est+rpl+trk).*ct_label(0x401020400000000)' -c], [0], [dnl -1 +2 ]) # Ensure conntrack entry is present. We should not try to predict @@ -6320,7 +6335,8 @@ AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(-new+est+rpl+trk).*ct_lab # and just ensure that the known ethernet address is present. AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(fd01::2) | \ sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | -sed -e 's/mark=[[0-9]]*/mark=<cleared>/'], [0], [dnl +sed -e 's/mark=[[0-9]]*/mark=<cleared>/' | sort], [0], [dnl +icmpv6,orig=(src=fd07::1,dst=fd01::2,id=<cleared>,type=128,code=0),reply=(src=fd01::2,dst=fd07::1,id=<cleared>,type=129,code=0),zone=<cleared>,mark=<cleared>,labels=0x401020400000000 tcp,orig=(src=fd07::1,dst=fd01::2,sport=<cleared>,dport=<cleared>),reply=(src=fd01::2,dst=fd07::1,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=<cleared>,labels=0x401020400000000,protoinfo=(state=<cleared>) ]) @@ -6333,17 +6349,22 @@ ovn-nbctl --wait=hv set Logical_Switch_Port r2-ext \ type=router options:router-port=R2_ext addresses='"00:00:10:01:02:04"' NS_CHECK_EXEC([bob1], [nc -6 -z fd01::2 80], [0]) +NS_CHECK_EXEC([bob1], [ping -q -c 3 -i 0.3 -w 2 fd01::2 | FORMAT_PING], \ +[0], [dnl +3 packets transmitted, 3 received, 0% packet loss, time 0ms +]) AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(+new-est-rpl+trk).*ct(.*label=0x1001020400000000/.*)' -c], [0], [dnl -1 +2 ]) AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(-new+est+rpl+trk).*ct_label(0x1001020400000000)' -c], [0], [dnl -1 +2 ]) AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep 0x1001020400000000 | FORMAT_CT(fd01::2) | \ sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | sed -e 's/mark=[[0-9]]*/mark=<cleared>/'], [0], [dnl +icmpv6,orig=(src=fd07::1,dst=fd01::2,id=<cleared>,type=128,code=0),reply=(src=fd01::2,dst=fd07::1,id=<cleared>,type=129,code=0),zone=<cleared>,mark=<cleared>,labels=0x1001020400000000 tcp,orig=(src=fd07::1,dst=fd01::2,sport=<cleared>,dport=<cleared>),reply=(src=fd01::2,dst=fd07::1,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=<cleared>,labels=0x1001020400000000,protoinfo=(state=<cleared>) ]) -- 2.43.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
