Added change not to create ECMP Group in SAI and program the route if none of ECMP members are active/link-up (#3394)

What I did:
Added change not to create ECMP Group in SAI and program the route if none of the ECMP members are active/link-up.
Also do not program the Temp Route if Neigh is not active (Link Down)

Also as part of this change if Route is not programmed and if we remove that route than decrement VRF Reference count in removeRoute as removeRoutePost will not be called in this case.

Why I did:
In scale setup of T2 it's possible all links can go down simultaneously which case we can get Route messages with all nexthops being in down state. In such case we might create empty Nexthop Group in SAI for the given route which causes not needed SAI call for Nexthop Group creation and also create traffic blackhole for the route where that route can still forward traffic via default route if eligible/applicable.

Also in this case no point to add Temp Route if neighbor is link down.

Author: abdosi

orchagent/routeorch.cpp

@@ -1331,7 +1331,11 @@ bool RouteOrch::addNextHopGroup(const NextHopGroupKey &nexthops)
             nhopgroup_shared_set[next_hop_id].insert(it);
         }
     }
-
+    if (!next_hop_ids.size())
+    {
+        SWSS_LOG_INFO("Skipping creation of nexthop group as none of nexthop are active");
+        return false;
+    }
     sai_attribute_t nhg_attr;
     vector<sai_attribute_t> nhg_attrs;
 
@@ -1716,9 +1720,15 @@ void RouteOrch::addTempRoute(RouteBulkContext& ctx, const NextHopGroupKey &nextH
             SWSS_LOG_INFO("Failed to get next hop %s for %s",
                    (*it).to_string().c_str(), ipPrefix.to_string().c_str());
             it = next_hop_set.erase(it);
+            continue;
         }
-        else
-            it++;
+        if(m_neighOrch->isNextHopFlagSet(*it, NHFLAGS_IFDOWN))
+        {
+            SWSS_LOG_INFO("Interface down for NH %s, skip this NH", (*it).to_string().c_str());
+            it = next_hop_set.erase(it);
+            continue;
+        }
+        it++;
     }
 
     /* Return if next_hop_set is empty */
@@ -2421,8 +2431,14 @@ bool RouteOrch::removeRoute(RouteBulkContext& ctx)
     size_t creating = gRouteBulker.creating_entries_count(route_entry);
     if (it_route == it_route_table->second.end() && creating == 0)
     {
-        SWSS_LOG_INFO("Failed to find route entry, vrf_id 0x%" PRIx64 ", prefix %s\n", vrf_id,
-                ipPrefix.to_string().c_str());
+       if (it_route_table->second.size() == 0)
+       {
+            m_syncdRoutes.erase(vrf_id);
+            m_vrfOrch->decreaseVrfRefCount(vrf_id);
+       }
+       SWSS_LOG_INFO("Failed to find route entry, vrf_id 0x%" PRIx64 ", prefix %s\n", vrf_id,
+                     ipPrefix.to_string().c_str());
+ 
         return true;
     }
 

tests/mock_tests/routeorch_ut.cpp

@@ -281,6 +281,7 @@ namespace routeorch_test
             for (const auto &it : ports)
             {
                 portTable.set(it.first, it.second);
+                portTable.set(it.first, {{ "oper_status", "up" }});
             }
 
             // Set PortConfigDone

tests/test_sub_port_intf.py

@@ -393,7 +393,7 @@ def get_default_vrf_oid(self):
         assert len(oids) == 1, "Wrong # of default vrfs: %d, expected #: 1." % (len(oids))
         return oids[0]
 
-    def get_ip_prefix_nhg_oid(self, ip_prefix, vrf_oid=None):
+    def get_ip_prefix_nhg_oid(self, ip_prefix, vrf_oid=None, prefix_present=True):
         if vrf_oid is None:
             vrf_oid = self.default_vrf_oid
 
@@ -407,18 +407,24 @@ def _access_function():
                     route_entry_found = True
                     assert route_entry_key["vr"] == vrf_oid
                     break
-
-            return (route_entry_found, raw_route_entry_key)
+            if prefix_present:
+                return (route_entry_found, raw_route_entry_key)
+            else:
+                return (not route_entry_found, None)
 
         (route_entry_found, raw_route_entry_key) = wait_for_result(_access_function)
 
-        fvs = self.asic_db.get_entry(ASIC_ROUTE_ENTRY_TABLE, raw_route_entry_key)
+        if not prefix_present:
+            assert raw_route_entry_key == None
+            return None
+        else:
+            fvs = self.asic_db.get_entry(ASIC_ROUTE_ENTRY_TABLE, raw_route_entry_key)
 
-        nhg_oid = fvs.get("SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID", "")
-        assert nhg_oid != ""
-        assert nhg_oid != "oid:0x0"
+            nhg_oid = fvs.get("SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID", "")
+            assert nhg_oid != ""
+            assert nhg_oid != "oid:0x0"
 
-        return nhg_oid
+            return nhg_oid
 
     def check_sub_port_intf_key_existence(self, db, table_name, key):
         db.wait_for_matching_keys(table_name, [key])
@@ -1543,21 +1549,26 @@ def _test_sub_port_intf_oper_down_with_pending_neigh_route_tasks(self, dvs, sub_
             self.add_route_appl_db(ip_prefix, nhop_ips, ifnames, vrf_name)
 
             # Verify route entry created in ASIC_DB and get next hop group oid
-            nhg_oid = self.get_ip_prefix_nhg_oid(ip_prefix, vrf_oid)
-
-            # Verify next hop group of the specified oid created in ASIC_DB
-            self.check_sub_port_intf_key_existence(self.asic_db, ASIC_NEXT_HOP_GROUP_TABLE, nhg_oid)
+            nhg_oid = self.get_ip_prefix_nhg_oid(ip_prefix, vrf_oid, prefix_present = i < (nhop_num - 1))
 
-            # Verify next hop group member # created in ASIC_DB
-            nhg_member_oids = self.asic_db.wait_for_n_keys(ASIC_NEXT_HOP_GROUP_MEMBER_TABLE,
-                                                           (nhop_num - 1) - i if create_intf_on_parent_port == False else ((nhop_num - 1) - i) * 2)
+            if i < (nhop_num - 1):
+                # Verify next hop group of the specified oid created in ASIC_DB
+                self.check_sub_port_intf_key_existence(self.asic_db, ASIC_NEXT_HOP_GROUP_TABLE, nhg_oid)
 
-            # Verify that next hop group members all belong to the next hop group of the specified oid
-            fv_dict = {
-                "SAI_NEXT_HOP_GROUP_MEMBER_ATTR_NEXT_HOP_GROUP_ID": nhg_oid,
-            }
-            for nhg_member_oid in nhg_member_oids:
-                self.check_sub_port_intf_fvs(self.asic_db, ASIC_NEXT_HOP_GROUP_MEMBER_TABLE, nhg_member_oid, fv_dict)
+                # Verify next hop group member # created in ASIC_DB
+                nhg_member_oids = self.asic_db.wait_for_n_keys(ASIC_NEXT_HOP_GROUP_MEMBER_TABLE,
+                                                               (nhop_num - 1) - i if create_intf_on_parent_port == False \
+                                                               else ((nhop_num - 1) - i) * 2)
+
+                # Verify that next hop group members all belong to the next hop group of the specified oid
+                fv_dict = {
+                    "SAI_NEXT_HOP_GROUP_MEMBER_ATTR_NEXT_HOP_GROUP_ID": nhg_oid,
+                }
+                for nhg_member_oid in nhg_member_oids:
+                    self.check_sub_port_intf_fvs(self.asic_db, ASIC_NEXT_HOP_GROUP_MEMBER_TABLE, nhg_member_oid, fv_dict)
+            else:
+                assert nhg_oid == None
+                self.asic_db.wait_for_n_keys(ASIC_NEXT_HOP_GROUP_MEMBER_TABLE, 0)
 
             nhop_cnt = len(self.asic_db.get_keys(ASIC_NEXT_HOP_TABLE))
             # Remove next hop objects on sub port interfaces