| Message ID | 20240205094723.288440-1-amusil@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] northd: Remove the protocol match from ECMP symmetric reply flows | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Mon, Feb 5, 2024 at 10:47 AM Ales Musil <amusil@redhat.com> 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 <dceara@redhat.com> > Signed-off-by: Ales Musil <amusil@redhat.com> > 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 > >
On 2/5/24 11:07, Ales Musil wrote: > > > On Mon, Feb 5, 2024 at 10:47 AM Ales Musil <amusil@redhat.com > <mailto:amusil@redhat.com>> 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 > <https://issues.redhat.com/browse/FDP-358> > Suggested-by: Dumitru Ceara <dceara@redhat.com > <mailto:dceara@redhat.com>> > Signed-off-by: Ales Musil <amusil@redhat.com <mailto:amusil@redhat.com>> > > > I forgot the dot at the end (again...) and also: > Fixes: 506f7d4bcfbc ("northd: rely on new actions for ecmp-symmetric > routing") > I fixed up the commit log and added the "fixes" tag. I pushed this to main and backported it to all branches down to 22.06. Regards, Dumitru
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>) ])
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 <dceara@redhat.com> Signed-off-by: Ales Musil <amusil@redhat.com> --- 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(-)