On Mon, Feb 5, 2024 at 10:47 AM Ales Musil <[email protected]> wrote:
> 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]> > I forgot the dot at the end (again...) and also: Fixes: 506f7d4bcfbc ("northd: rely on new actions for ecmp-symmetric routing") > --- > 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 > > -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
