northd: working I think! to-do: edit manpage

When preparing to build ECMP and static route flows, Reported-at: https://issues.redhat.com/browse/FDP-786 Signed-off-by: Rosemarie O'Riorden <[email protected]>
roseoriorden · Sep 16, 2024 · a4d3758 · a4d3758
1 parent a11cc56
commit a4d3758
Show file tree

Hide file tree

Showing 3 changed files with 48 additions and 12 deletions.
diff --git a/northd/northd.c b/northd/northd.c
@@ -11567,21 +11567,48 @@ 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, //here, CONTAINER_OF casts list
                   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, ");");
+    /*
+    ds_clear(&actions);
+    if (rp->n_valid_nexthops > 1) {
+        ds_put_format(&actions, "%s = %"PRIu16
+                      "; %s = select(", REG_ECMP_GROUP_ID, ecmp_group_id,
+                      REG_ECMP_MEMBER_ID);
+
+        for (size_t i = 0; i < rp->n_valid_nexthops; i++) {
+            if (i > 0) {
+                ds_put_cstr(&actions, ", ");
+            }
 
+            ds_put_format(&actions, "%"PRIuSIZE, i + 1);
+        }
+        ds_put_cstr(&actions, ");");
+    } else {
+        ds_put_format(&actions, "%s = %"PRIu16
+                      "; %s = 1; next;", REG_ECMP_GROUP_ID,
+                      ecmp_group_id, REG_ECMP_MEMBER_ID);
+    }
+    */
     ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING, priority,
                   ds_cstr(&route_match), ds_cstr(&actions),
                   lflow_ref);
@@ -13543,6 +13570,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,8 @@ next;
 ip.ttl--;
 flags.loopback = 1;
 reg8[0..15] = <var>GID</var>;
-select(reg8[16..31], <var>MID1</var>, <var>MID2</var>, ...);
+select(reg8[16..31], <var>MID1</var>, <var>MID2</var>, ...); <!-- fix order -->
+<!-- add that if there's only one route in ECMP group actions will be different -->
         </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;)
 ])