What I did:

Implemented recursive nexthop group enhancement in Orchagent (NhgOrch). They have been proposed in: sonic-net/SONiC#1636 Why I did it: These changes are required to handle a new field - "nexthop_group" in the App DB NEXT_HOP_GROUP_TABLE. Such nexthop groups are called recursive nexthop groups. This field contains the list of member (singleton) nexthop groups. Such recursive nexthop groups are represented by NEXT_HOP_GROUP objects.
utpalkantpintoo · May 14, 2024 · 43e939f · 43e939f
1 parent 32c2552
commit 43e939f
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 12 deletions.
diff --git a/orchagent/nhgorch.cpp b/orchagent/nhgorch.cpp
@@ -637,8 +637,8 @@ bool NextHopGroup::sync()
         return true;
     }
 
-    /* If the group is non-recursive, the group ID will be the only member's NH ID */
-    if (!isRecursive())
+    /* If the group is non-recursive with single member, the group ID will be the only member's NH ID */
+    if (!isRecursive() && (m_members.size() == 1))
     {
         const NextHopGroupMember& nhgm = m_members.begin()->second;
         sai_object_id_t nhid = nhgm.getNhId();
@@ -771,8 +771,13 @@ bool NextHopGroup::remove()
 {
     SWSS_LOG_ENTER();
 
+    if (!isSynced())
+    {
+        return true;
+    }
     //  If the group is temporary or non-recursive, update the neigh or rif ref-count and reset the ID.
-    if (m_is_temp || !isRecursive())
+    if (m_is_temp ||
+        (!isRecursive() && m_members.size() == 1))
     {
         const NextHopGroupMember& nhgm = m_members.begin()->second;
         auto nh_key = nhgm.getKey();
@@ -802,8 +807,8 @@ bool NextHopGroup::syncMembers(const std::set<NextHopKey>& nh_keys)
 {
     SWSS_LOG_ENTER();
 
-    /* This method should not be called for non-recursive nexthop groups */
-    assert(isRecursive());
+    /* This method should not be called for single-membered non-recursive nexthop groups */
+    assert(isRecursive() || (m_members.size() > 1));
 
     ObjectBulker<sai_next_hop_group_api_t> nextHopGroupMemberBulker(sai_next_hop_group_api, gSwitchId, gMaxBulkSize);
 
@@ -894,7 +899,8 @@ bool NextHopGroup::update(const NextHopGroupKey& nhg_key)
 {
     SWSS_LOG_ENTER();
 
-    if (!(isRecursive() && isSynced()))
+    if (!isSynced() ||
+        (!isRecursive() && (m_members.size() == 1 || nhg_key.getSize() == 1)))
     {
         bool was_synced = isSynced();
         bool was_temp = isTemp();
@@ -1025,7 +1031,7 @@ bool NextHopGroup::validateNextHop(const NextHopKey& nh_key)
 {
     SWSS_LOG_ENTER();
 
-    if (isRecursive())
+    if (isRecursive() || (m_members.size() > 1))
     {
         return syncMembers({nh_key});
     }
@@ -1044,7 +1050,7 @@ bool NextHopGroup::invalidateNextHop(const NextHopKey& nh_key)
 {
     SWSS_LOG_ENTER();
 
-    if (isRecursive())
+    if (isRecursive() || (m_members.size() > 1))
     {
         return removeMembers({nh_key});
     }

diff --git a/tests/test_nhg.py b/tests/test_nhg.py
@@ -84,6 +84,8 @@ def get_nhg_map_id(self, nhg_map_index):
 
         # Create a NHG
         fvs = swsscommon.FieldValuePairs([('nexthop', '10.0.0.1'), ('ifname', 'Ethernet0')])
+        nhg_ps.set('_testnhg', fvs)
+        fvs = swsscommon.FieldValuePairs([('nexthop_group', '_testnhg')])
         nhg_ps.set('testnhg', fvs)
 
         # Add a CBF NHG pointing to the given map
@@ -98,6 +100,7 @@ def get_nhg_map_id(self, nhg_map_index):
             # Remove the added NHGs
             cbf_nhg_ps._del('testcbfnhg')
             nhg_ps._del('testnhg')
+            nhg_ps._del('_testnhg')
             self.asic_db.wait_for_n_keys(self.ASIC_NHG_STR, asic_nhgs_count)
             return None
 
@@ -111,6 +114,7 @@ def get_nhg_map_id(self, nhg_map_index):
         # Remove the added NHGs
         cbf_nhg_ps._del('testcbfnhg')
         nhg_ps._del('testnhg')
+        nhg_ps._del('_testnhg')
         self.asic_db.wait_for_n_keys(self.ASIC_NHG_STR, asic_nhgs_count)
 
         return nhg_map_id
@@ -1705,8 +1709,8 @@ def update_nhgm_count_test():
             # Update the group to one NH only
             fvs = swsscommon.FieldValuePairs([('nexthop', '10.0.0.1'), ("ifname", "Ethernet0")])
             self.nhg_ps.set("group1", fvs)
-            self.asic_db.wait_for_n_keys(self.ASIC_NHGM_STR, self.asic_nhgms_count + 1)
-            assert len(self.get_nhgm_ids('group1')) == 1
+            self.asic_db.wait_for_n_keys(self.ASIC_NHGM_STR, self.asic_nhgms_count)
+            assert len(self.get_nhgm_ids('group1')) == 0
 
             # Update the group to 2 NHs
             fvs = swsscommon.FieldValuePairs([('nexthop', '10.0.0.1,10.0.0.3'), ("ifname", "Ethernet0,Ethernet4")])
@@ -1838,9 +1842,11 @@ def data_validation_test():
         # - update the CBF NHG reordering the members and assert the new details match
         def update_cbf_nhg_members_test():
             # Create a NHG with a single next hop
-            fvs = swsscommon.FieldValuePairs([('nexthop', '10.0.0.1'),
-                                            ("ifname", "Ethernet0")])
+            fvs = swsscommon.FieldValuePairs([('nexthop', '10.0.0.1'), ('ifname', 'Ethernet0')])
+            self.nhg_ps.set("_group3", fvs)
+            fvs = swsscommon.FieldValuePairs([('nexthop_group','_group3')])
             self.nhg_ps.set("group3", fvs)
+
             self.asic_db.wait_for_n_keys(self.ASIC_NHG_STR, self.asic_nhgs_count + 3)
 
             # Create a CBF NHG
@@ -2035,6 +2041,8 @@ def create_cbf_invalid_nhg_map_test():
         self.cbf_nhg_ps._del('cbfgroup1')
         self.nhg_ps._del('group2')
         self.nhg_ps._del('group3')
+        self.nhg_ps._del('_group3')
+
         self.asic_db.wait_for_n_keys(self.ASIC_NHG_STR, self.asic_nhgs_count)
         self.asic_db.wait_for_n_keys(self.ASIC_NHGM_STR, self.asic_nhgms_count)