From 7b00627433f3ee066cf6a5b727fec614c5e6eb77 Mon Sep 17 00:00:00 2001 From: Rosemarie O'Riorden Date: Fri, 20 Sep 2024 11:36:35 -0400 Subject: [PATCH] northd: Respect --ecmp-symmetric-reply for single routes. When preparing to build ECMP and static route flows, routes are sorted into unique routes (meaning they are not part of a group) or they are added to EMCP groups. Then, ECMP route flows are built out of the groups, and static route flows are built out of the unique routes. However, 'unique routes' include ones that use the --ecmp-symmetric-reply flag, meaning that they may not be added to an ECMP group, and thus ECMP symmetric reply would not be used for those flows. For example, if one route is added and traffic is started, and then later another route is added, the already flowing traffic might be rerouted since it wasn't conntracked initially. This could break symmetric reply with traffic using a different next-hop than before. This change makes it so that when the --ecmp-symmetric-reply flag is used, even for unique routes, an ECMP group is created which they are added to. Thus they are added to the ECMP route flow, rather than static. This allows ECMP groups to persist even when there is only one route. Edited documentation to support this change. Also updated incorrect actions in documentation. Fixes: 4fdca656857d ("Add ECMP symmetric replies.") Reported-at: https://issues.redhat.com/browse/FDP-786 Signed-off-by: Rosemarie O'Riorden Signed-off-by: Dumitru Ceara (cherry picked from commit b93e9a5e6f3aa3cb3e2065bd8e0aa0b6fc1fd19a) --- northd/northd.c | 33 ++++++++++++++++++++++----------- northd/ovn-northd.8.xml | 13 ++++++++++++- tests/ovn-northd.at | 5 ++++- tests/system-ovn.at | 32 ++++++++++++++++++++++++++++++++ 4 files changed, 70 insertions(+), 13 deletions(-) diff --git a/northd/northd.c b/northd/northd.c index a267cd5f8e..ddfe0171f6 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -11565,21 +11565,27 @@ build_ecmp_route_flow(struct lflow_table *lflows, struct ovn_datapath *od, struct ds actions = DS_EMPTY_INITIALIZER; ds_put_format(&actions, "ip.ttl--; flags.loopback = 1; %s = %"PRIu16 - "; %s = select(", REG_ECMP_GROUP_ID, eg->id, - REG_ECMP_MEMBER_ID); + "; %s = ", REG_ECMP_GROUP_ID, eg->id, REG_ECMP_MEMBER_ID); - bool is_first = true; - LIST_FOR_EACH (er, list_node, &eg->route_list) { - if (is_first) { - is_first = false; - } else { - ds_put_cstr(&actions, ", "); + if (!ovs_list_is_singleton(&eg->route_list)) { + bool is_first = true; + + ds_put_cstr(&actions, "select("); + LIST_FOR_EACH (er, list_node, &eg->route_list) { + if (is_first) { + is_first = false; + } else { + ds_put_cstr(&actions, ", "); + } + ds_put_format(&actions, "%"PRIu16, er->id); } - ds_put_format(&actions, "%"PRIu16, er->id); + ds_put_cstr(&actions, ");"); + } else { + er = CONTAINER_OF(ovs_list_front(&eg->route_list), + struct ecmp_route_list_node, list_node); + ds_put_format(&actions, "%"PRIu16"; next;", er->id); } - ds_put_cstr(&actions, ");"); - ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING, priority, ds_cstr(&route_match), ds_cstr(&actions), lflow_ref); @@ -13541,6 +13547,11 @@ build_static_route_flows_for_lrouter( if (group) { ecmp_groups_add_route(group, route); } + } else if (route->ecmp_symmetric_reply) { + /* Traffic for symmetric reply routes has to be conntracked + * even if there is only one next-hop, in case another next-hop + * is added later. */ + ecmp_groups_add(&ecmp_groups, route); } else { unique_routes_add(&unique_routes, route); } diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml index ede38882a4..ef5cd0c8cb 100644 --- a/northd/ovn-northd.8.xml +++ b/northd/ovn-northd.8.xml @@ -4328,7 +4328,18 @@ next; ip.ttl--; flags.loopback = 1; reg8[0..15] = GID; -select(reg8[16..31], MID1, MID2, ...); +reg8[16..31] = select(MID1, MID2, ...); + +

+ However, when there is only one route in an ECMP group, group actions + will be: +

+ + 
+ip.ttl--;
+flags.loopback = 1;
+reg8[0..15] = GID;
+reg8[16..31] = MID1);
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index dcc3dbbc31..d459c23c05 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -6799,20 +6799,23 @@ check ovn-nbctl lsp-set-type public-lr0 router check ovn-nbctl lsp-set-addresses public-lr0 router check ovn-nbctl lsp-set-options public-lr0 router-port=lr0-public +# ECMP flows will be added even if there is only one next-hop. check ovn-nbctl --wait=sb --ecmp-symmetric-reply lr-route-add lr0 1.0.0.1 192.168.0.10 ovn-sbctl dump-flows lr0 > lr0flows AT_CHECK([grep -w "lr_in_ip_routing" lr0flows | ovn_strip_lflows], [0], [dnl table=??(lr_in_ip_routing ), priority=0 , match=(1), action=(drop;) + table=??(lr_in_ip_routing ), priority=10300, match=(ct_mark.ecmp_reply_port == 1 && reg7 == 0 && ip4.dst == 1.0.0.1/32), action=(ip.ttl--; flags.loopback = 1; eth.src = 00:00:20:20:12:13; reg1 = 192.168.0.1; outport = "lr0-public"; next;) table=??(lr_in_ip_routing ), priority=10550, match=(nd_rs || nd_ra), action=(drop;) table=??(lr_in_ip_routing ), priority=194 , match=(inport == "lr0-public" && ip6.dst == fe80::/64), action=(ip.ttl--; reg8[[0..15]] = 0; xxreg0 = ip6.dst; xxreg1 = fe80::200:20ff:fe20:1213; eth.src = 00:00:20:20:12:13; outport = "lr0-public"; flags.loopback = 1; next;) table=??(lr_in_ip_routing ), priority=74 , match=(ip4.dst == 192.168.0.0/24), action=(ip.ttl--; reg8[[0..15]] = 0; reg0 = ip4.dst; reg1 = 192.168.0.1; eth.src = 00:00:20:20:12:13; outport = "lr0-public"; flags.loopback = 1; next;) - table=??(lr_in_ip_routing ), priority=97 , match=(reg7 == 0 && ip4.dst == 1.0.0.1/32), action=(ip.ttl--; reg8[[0..15]] = 0; reg0 = 192.168.0.10; reg1 = 192.168.0.1; eth.src = 00:00:20:20:12:13; outport = "lr0-public"; flags.loopback = 1; next;) + table=??(lr_in_ip_routing ), priority=97 , match=(reg7 == 0 && ip4.dst == 1.0.0.1/32), action=(ip.ttl--; flags.loopback = 1; reg8[[0..15]] = 1; reg8[[16..31]] = 1; next;) ]) AT_CHECK([grep -e "lr_in_ip_routing_ecmp" lr0flows | ovn_strip_lflows], [0], [dnl table=??(lr_in_ip_routing_ecmp), priority=0 , match=(1), action=(drop;) + table=??(lr_in_ip_routing_ecmp), priority=100 , match=(reg8[[0..15]] == 1 && reg8[[16..31]] == 1), action=(reg0 = 192.168.0.10; reg1 = 192.168.0.1; eth.src = 00:00:20:20:12:13; outport = "lr0-public"; next;) table=??(lr_in_ip_routing_ecmp), priority=150 , match=(reg8[[0..15]] == 0), action=(next;) ]) diff --git a/tests/system-ovn.at b/tests/system-ovn.at index b585e10da3..76445876e4 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -6241,6 +6241,22 @@ sed -e 's/mark=[[0-9]]*/mark=/' | sort], [0], [dnl tcp,orig=(src=172.16.0.1,dst=10.0.0.2,sport=,dport=),reply=(src=10.0.0.2,dst=172.16.0.1,sport=,dport=),zone=,mark=,labels=0x401020500000000,protoinfo=(state=) ]) +# Now remove one ECMP route and check that traffic is still being conntracked. +check ovn-nbctl --policy="src-ip" lr-route-del R1 10.0.0.0/24 20.0.0.3 +check ovn-nbctl --wait=hv sync +AT_CHECK([ovs-appctl dpctl/flush-conntrack]) +NETNS_DAEMONIZE([bob1], [nc -l -k 8081], [bob2.pid]) +NS_CHECK_EXEC([alice1], [nc -z 172.16.0.1 8081], [0]) +NS_CHECK_EXEC([alice1], [ping -q -c 3 -i 0.3 -w 2 172.16.0.1 | FORMAT_PING], \ +[0], [dnl +3 packets transmitted, 3 received, 0% packet loss, time 0ms +]) +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep 0x401020500000000 | FORMAT_CT(172.16.0.1) | \ +sed -e 's/zone=[[0-9]]*/zone=/' | +sed -e 's/mark=[[0-9]]*/mark=/' | sort], [0], [dnl +tcp,orig=(src=172.16.0.1,dst=10.0.0.2,sport=,dport=),reply=(src=10.0.0.2,dst=172.16.0.1,sport=,dport=),zone=,mark=,labels=0x401020500000000,protoinfo=(state=) +]) + OVS_APP_EXIT_AND_WAIT([ovn-controller]) as ovn-sb @@ -6457,6 +6473,22 @@ sed -e 's/mark=[[0-9]]*/mark=/' | sort], [0], [dnl tcp,orig=(src=fd07::1,dst=fd01::2,sport=,dport=),reply=(src=fd01::2,dst=fd07::1,sport=,dport=),zone=,mark=,labels=0x1001020400000000,protoinfo=(state=) ]) +# Now remove one ECMP route and check that traffic is still being conntracked. +check ovn-nbctl --policy="src-ip" lr-route-del R1 fd01::/126 fd02::3 +check ovn-nbctl --wait=hv sync +AT_CHECK([ovs-appctl dpctl/flush-conntrack]) +NETNS_DAEMONIZE([bob1], [nc -6 -l -k 8081], [bob2.pid]) +NS_CHECK_EXEC([alice1], [nc -6 -z fd07::1 8081], [0]) +NS_CHECK_EXEC([alice1], [ping -q -c 3 -i 0.3 -w 2 fd07::1 | FORMAT_PING], \ +[0], [dnl +3 packets transmitted, 3 received, 0% packet loss, time 0ms +]) +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep 0x1001020400000000 | FORMAT_CT(fd07::1) | \ +sed -e 's/zone=[[0-9]]*/zone=/' | +sed -e 's/mark=[[0-9]]*/mark=/' | sort], [0], [dnl +tcp,orig=(src=fd07::1,dst=fd01::2,sport=,dport=),reply=(src=fd01::2,dst=fd07::1,sport=,dport=),zone=,mark=,labels=0x1001020400000000,protoinfo=(state=) +]) + OVS_APP_EXIT_AND_WAIT([ovn-controller]) as ovn-sb