northd: working! docs updated

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, the ECMP route flow would be built out of the groups, and the static route flow would be built out of the unique routes. However, this included unique routes that used the --ecmp-symmetric-reply flag, meaning that they would not be added to an ECMP group, and thus ECMP symmetric replies would not be used for those flows. This could cause problems if a node goes down and then back up again, because Edited documentation to support this change. Also updated incorrect actions in documentation. Fixes: Reported-at: https://issues.redhat.com/browse/FDP-786 Signed-off-by: Rosemarie O'Riorden <[email protected]>
roseoriorden · Sep 16, 2024 · 6dbc7b6 · 6dbc7b6
1 parent a11cc56
commit 6dbc7b6
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 12 deletions.
diff --git a/northd/northd.c b/northd/northd.c
@@ -11567,21 +11567,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,
+                  "; %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);
@@ -13543,6 +13549,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
@@ -4328,7 +4328,17 @@ next;
 ip.ttl--;
 flags.loopback = 1;
 reg8[0..15] = <var>GID</var>;
-select(reg8[16..31], <var>MID1</var>, <var>MID2</var>, ...);
+reg8[16..31] = select(<var>MID1</var>, <var>MID2</var>, ...);
+        </pre>
+        <p>
+          However when there is only one route in ECMP, group actions will be:
+        </p>
+
+        <pre>
+ip.ttl--;
+flags.loopback = 1;
+reg8[0..15] = <var>GID</var>;
+reg8[16..31] = <var>MID1</var>);
         </pre>
       </li>
 

diff --git 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;)
 ])