OA changes to support Ordered ECMP and DVS test for same. (sonic-net#2092)

abdosi · web-flow · commit c78aa1b81a3a · 2022-01-12T20:01:56.000-08:00
What I did:-

Orchagent changes to support Ordered ECMP as per design Order ECMP HLD. SONiC#896
While testing Order ECMP for Overlay ECMP use case found that for single nexthop based tunnel we create route without checking BFD state for endpoint. This results in not needed transaction of add_route followed by del_route.

Signed-off-by: Abhishek Dosi &lt;abdosi@microsoft.com&gt;
diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp
@@ -376,6 +376,13 @@ bool RouteOrch::validnexthopinNextHopGroup(const NextHopKey &nexthop, uint32_t&
             nhgm_attrs.push_back(nhgm_attr);
         }
 
+        if (m_switchOrch->checkOrderedEcmpEnable())
+        {
+            nhgm_attr.id = SAI_NEXT_HOP_GROUP_MEMBER_ATTR_SEQUENCE_ID;
+            nhgm_attr.value.u32 = nhopgroup->second.nhopgroup_members[nexthop].seq_id;
+            nhgm_attrs.push_back(nhgm_attr);
+        }
+
         status = sai_next_hop_group_api->create_next_hop_group_member(&nexthop_id, gSwitchId,
                                                                       (uint32_t)nhgm_attrs.size(),
                                                                       nhgm_attrs.data());
@@ -393,7 +400,7 @@ bool RouteOrch::validnexthopinNextHopGroup(const NextHopKey &nexthop, uint32_t&
 
         ++count;
         gCrmOrch->incCrmResUsedCounter(CrmResourceType::CRM_NEXTHOP_GROUP_MEMBER);
-        nhopgroup->second.nhopgroup_members[nexthop] = nexthop_id;
+        nhopgroup->second.nhopgroup_members[nexthop].next_hop_id = nexthop_id;
     }
 
     if (!m_fgNhgOrch->validNextHopInNextHopGroup(nexthop))
@@ -421,7 +428,7 @@ bool RouteOrch::invalidnexthopinNextHopGroup(const NextHopKey &nexthop, uint32_t
             continue;
         }
 
-        nexthop_id = nhopgroup->second.nhopgroup_members[nexthop];
+        nexthop_id = nhopgroup->second.nhopgroup_members[nexthop].next_hop_id;
         status = sai_next_hop_group_api->remove_next_hop_group_member(nexthop_id);
 
         if (status != SAI_STATUS_SUCCESS)
@@ -1241,7 +1248,7 @@ bool RouteOrch::addNextHopGroup(const NextHopGroupKey &nexthops)
     vector<sai_attribute_t> nhg_attrs;
 
     nhg_attr.id = SAI_NEXT_HOP_GROUP_ATTR_TYPE;
-    nhg_attr.value.s32 = SAI_NEXT_HOP_GROUP_TYPE_ECMP;
+    nhg_attr.value.s32 = m_switchOrch->checkOrderedEcmpEnable() ? SAI_NEXT_HOP_GROUP_TYPE_DYNAMIC_ORDERED_ECMP : SAI_NEXT_HOP_GROUP_TYPE_ECMP;
     nhg_attrs.push_back(nhg_attr);
 
     sai_object_id_t next_hop_group_id;
@@ -1295,6 +1302,13 @@ bool RouteOrch::addNextHopGroup(const NextHopGroupKey &nexthops)
             nhgm_attrs.push_back(nhgm_attr);
         }
 
+        if (m_switchOrch->checkOrderedEcmpEnable())
+        {
+            nhgm_attr.id = SAI_NEXT_HOP_GROUP_MEMBER_ATTR_SEQUENCE_ID;
+            nhgm_attr.value.u32 = ((uint32_t)i) + 1; // To make non-zero sequence id
+            nhgm_attrs.push_back(nhgm_attr);
+        }
+
         gNextHopGroupMemberBulker.create_entry(&nhgm_ids[i],
                                                  (uint32_t)nhgm_attrs.size(),
                                                  nhgm_attrs.data());
@@ -1319,7 +1333,8 @@ bool RouteOrch::addNextHopGroup(const NextHopGroupKey &nexthops)
         if (nhopgroup_shared_set.find(nhid) != nhopgroup_shared_set.end())
         {
             auto it = nhopgroup_shared_set[nhid].begin();
-            next_hop_group_entry.nhopgroup_members[*it] = nhgm_id;
+            next_hop_group_entry.nhopgroup_members[*it].next_hop_id = nhgm_id;
+            next_hop_group_entry.nhopgroup_members[*it].seq_id = (uint32_t)i + 1;
             nhopgroup_shared_set[nhid].erase(it);
             if (nhopgroup_shared_set[nhid].empty())
             {
@@ -1328,7 +1343,8 @@ bool RouteOrch::addNextHopGroup(const NextHopGroupKey &nexthops)
         }
         else
         {
-            next_hop_group_entry.nhopgroup_members[nhopgroup_members_set.find(nhid)->second] = nhgm_id;
+            next_hop_group_entry.nhopgroup_members[nhopgroup_members_set.find(nhid)->second].next_hop_id = nhgm_id;
+            next_hop_group_entry.nhopgroup_members[nhopgroup_members_set.find(nhid)->second].seq_id = ((uint32_t)i) + 1;
         }
     }
 
@@ -1373,12 +1389,12 @@ bool RouteOrch::removeNextHopGroup(const NextHopGroupKey &nexthops)
         if (m_neighOrch->isNextHopFlagSet(nhop->first, NHFLAGS_IFDOWN))
         {
             SWSS_LOG_WARN("NHFLAGS_IFDOWN set for next hop group member %s with next_hop_id %" PRIx64,
-                           nhop->first.to_string().c_str(), nhop->second);
+                           nhop->first.to_string().c_str(), nhop->second.next_hop_id);
             nhop = nhgm.erase(nhop);
             continue;
         }
 
-        next_hop_ids.push_back(nhop->second);
+        next_hop_ids.push_back(nhop->second.next_hop_id);
         nhop = nhgm.erase(nhop);
     }
 
diff --git a/orchagent/routeorch.h b/orchagent/routeorch.h
@@ -24,7 +24,13 @@
 
 #define LOOPBACK_PREFIX     "Loopback"
 
-typedef std::map<NextHopKey, sai_object_id_t> NextHopGroupMembers;
+struct NextHopGroupMemberEntry
+{
+    sai_object_id_t  next_hop_id; // next hop sai oid
+    uint32_t         seq_id; // Sequence Id of nexthop in the group
+};
+
+typedef std::map<NextHopKey, NextHopGroupMemberEntry> NextHopGroupMembers;
 
 struct NhgBase;
 
diff --git a/orchagent/switchorch.cpp b/orchagent/switchorch.cpp
@@ -1,4 +1,5 @@
 #include <map>
+#include <set>
 #include <inttypes.h>
 
 #include "switchorch.h"
@@ -44,6 +45,9 @@ const map<string, sai_packet_action_t> packet_action_map =
     {"trap",    SAI_PACKET_ACTION_TRAP}
 };
 
+
+const std::set<std::string> switch_non_sai_attribute_set = {"ordered_ecmp"};
+
 SwitchOrch::SwitchOrch(DBConnector *db, vector<TableConnector>& connectors, TableConnector switchTable):
         Orch(connectors),
         m_switchTable(switchTable.first, switchTable.second),
@@ -224,7 +228,51 @@ void SwitchOrch::doCfgSensorsTableTask(Consumer &consumer)
     }
 }
 
+void SwitchOrch::setSwitchNonSaiAttributes(swss::FieldValueTuple &val)
+{
+    auto attribute = fvField(val);
+    auto value = fvValue(val);
 
+    if (attribute == "ordered_ecmp")
+    {
+        vector<FieldValueTuple> fvVector;
+        if (value == "true")
+        {
+            const auto* meta = sai_metadata_get_attr_metadata(SAI_OBJECT_TYPE_NEXT_HOP_GROUP, SAI_NEXT_HOP_GROUP_ATTR_TYPE);
+            if (meta && meta->isenum)
+            {
+                vector<int32_t> values_list(meta->enummetadata->valuescount);
+                sai_s32_list_t values;
+                values.count = static_cast<uint32_t>(values_list.size());
+                values.list = values_list.data();
+
+                auto status = sai_query_attribute_enum_values_capability(gSwitchId,
+                                                                         SAI_OBJECT_TYPE_NEXT_HOP_GROUP,
+                                                                         SAI_NEXT_HOP_GROUP_ATTR_TYPE,
+                                                                         &values);
+                if (status == SAI_STATUS_SUCCESS)
+                {
+                    for (size_t i = 0; i < values.count; i++)
+                    {
+                        if (values.list[i] == SAI_NEXT_HOP_GROUP_TYPE_DYNAMIC_ORDERED_ECMP)
+                        {
+                            m_orderedEcmpEnable = true;
+                            fvVector.emplace_back(SWITCH_CAPABILITY_TABLE_ORDERED_ECMP_CAPABLE, "true");
+                            set_switch_capability(fvVector);
+                            SWSS_LOG_NOTICE("Ordered ECMP/Nexthop-Group is configured");
+                            return;
+                        }
+                    }
+                }
+            }
+        }
+        m_orderedEcmpEnable = false;
+        fvVector.emplace_back(SWITCH_CAPABILITY_TABLE_ORDERED_ECMP_CAPABLE, "false");
+        set_switch_capability(fvVector);
+        SWSS_LOG_NOTICE("Ordered ECMP/Nexthop-Group is not configured");
+        return;
+    }
+}
 sai_status_t SwitchOrch::setSwitchTunnelVxlanParams(swss::FieldValueTuple &val)
 {
     auto attribute = fvField(val);
@@ -296,7 +344,12 @@ void SwitchOrch::doAppSwitchTableTask(Consumer &consumer)
             {
                 auto attribute = fvField(i);
 
-                if (switch_attribute_map.find(attribute) == switch_attribute_map.end())
+                if (switch_non_sai_attribute_set.find(attribute) != switch_non_sai_attribute_set.end())
+                {
+                    setSwitchNonSaiAttributes(i);
+                    continue;
+                }
+                else if (switch_attribute_map.find(attribute) == switch_attribute_map.end())
                 {
                     // Check additionally 'switch_tunnel_attribute_map' for Switch Tunnel
                     if (switch_tunnel_attribute_map.find(attribute) == switch_tunnel_attribute_map.end())
diff --git a/orchagent/switchorch.h b/orchagent/switchorch.h
@@ -10,6 +10,7 @@
 
 #define SWITCH_CAPABILITY_TABLE_PORT_TPID_CAPABLE                      "PORT_TPID_CAPABLE"
 #define SWITCH_CAPABILITY_TABLE_LAG_TPID_CAPABLE                       "LAG_TPID_CAPABLE"
+#define SWITCH_CAPABILITY_TABLE_ORDERED_ECMP_CAPABLE                   "ORDERED_ECMP_CAPABLE"
 
 struct WarmRestartCheck
 {
@@ -37,6 +38,8 @@ class SwitchOrch : public Orch
     // Initialize the ACL groups bind to Switch
     void initAclGroupsBindToSwitch();
 
+    bool checkOrderedEcmpEnable() { return m_orderedEcmpEnable; }
+
 private:
     void doTask(Consumer &consumer);
     void doTask(swss::SelectableTimer &timer);
@@ -45,6 +48,8 @@ class SwitchOrch : public Orch
     void initSensorsTable();
     void querySwitchTpidCapability();
     sai_status_t setSwitchTunnelVxlanParams(swss::FieldValueTuple &val);
+    void setSwitchNonSaiAttributes(swss::FieldValueTuple &val);
+
 
     // Create the default ACL group for the given stage, bind point is
     // SAI_ACL_BIND_POINT_TYPE_SWITCH and group type is
@@ -74,6 +79,7 @@ class SwitchOrch : public Orch
     bool m_sensorsMaxTempSupported = true;
     bool m_sensorsAvgTempSupported = true;
     bool m_vxlanSportUserModeEnabled = false;
+    bool m_orderedEcmpEnable = false;
 
     // Information contained in the request from
     // external program for orchagent pre-shutdown state check
diff --git a/orchagent/vnetorch.cpp b/orchagent/vnetorch.cpp
@@ -40,7 +40,7 @@ extern CrmOrch *gCrmOrch;
 extern RouteOrch *gRouteOrch;
 extern MacAddress gVxlanMacAddress;
 extern BfdOrch *gBfdOrch;
-
+extern SwitchOrch *gSwitchOrch;
 /*
  * VRF Modeling and VNetVrf class definitions
  */
@@ -680,9 +680,12 @@ bool VNetRouteOrch::addNextHopGroup(const string& vnet, const NextHopGroupKey &n
     vector<sai_object_id_t> next_hop_ids;
     set<NextHopKey> next_hop_set = nexthops.getNextHops();
     std::map<sai_object_id_t, NextHopKey> nhopgroup_members_set;
+    std::map<NextHopKey, uint32_t> nh_seq_id_in_nhgrp;
+    uint32_t seq_id = 0;
 
     for (auto it : next_hop_set)
     {
+        nh_seq_id_in_nhgrp[it] = ++seq_id;
         if (nexthop_info_[vnet].find(it.ip_address) != nexthop_info_[vnet].end() && nexthop_info_[vnet][it.ip_address].bfd_state != SAI_BFD_SESSION_STATE_UP)
         {
             continue;
@@ -696,7 +699,7 @@ bool VNetRouteOrch::addNextHopGroup(const string& vnet, const NextHopGroupKey &n
     vector<sai_attribute_t> nhg_attrs;
 
     nhg_attr.id = SAI_NEXT_HOP_GROUP_ATTR_TYPE;
-    nhg_attr.value.s32 = SAI_NEXT_HOP_GROUP_TYPE_ECMP;
+    nhg_attr.value.s32 = gSwitchOrch->checkOrderedEcmpEnable() ? SAI_NEXT_HOP_GROUP_TYPE_DYNAMIC_ORDERED_ECMP : SAI_NEXT_HOP_GROUP_TYPE_ECMP;
     nhg_attrs.push_back(nhg_attr);
 
     sai_object_id_t next_hop_group_id;
@@ -733,6 +736,13 @@ bool VNetRouteOrch::addNextHopGroup(const string& vnet, const NextHopGroupKey &n
         nhgm_attr.value.oid = nhid;
         nhgm_attrs.push_back(nhgm_attr);
 
+        if (gSwitchOrch->checkOrderedEcmpEnable())
+        {
+            nhgm_attr.id = SAI_NEXT_HOP_GROUP_MEMBER_ATTR_SEQUENCE_ID;
+            nhgm_attr.value.u32 = nh_seq_id_in_nhgrp[nhopgroup_members_set.find(nhid)->second];
+            nhgm_attrs.push_back(nhgm_attr);
+        }
+
         sai_object_id_t next_hop_group_member_id;
         status = sai_next_hop_group_api->create_next_hop_group_member(&next_hop_group_member_id,
                                                                     gSwitchId,
@@ -865,7 +875,10 @@ bool VNetRouteOrch::doRouteTask<VNetVrfObject>(const string& vnet, IpPrefix& ipP
                 NextHopGroupInfo next_hop_group_entry;
                 next_hop_group_entry.next_hop_group_id = vrf_obj->getTunnelNextHop(nexthop);
                 next_hop_group_entry.ref_count = 0;
-                next_hop_group_entry.active_members[nexthop] = SAI_NULL_OBJECT_ID;
+                if (nexthop_info_[vnet].find(nexthop.ip_address) == nexthop_info_[vnet].end() || nexthop_info_[vnet][nexthop.ip_address].bfd_state == SAI_BFD_SESSION_STATE_UP)
+                {
+                    next_hop_group_entry.active_members[nexthop] = SAI_NULL_OBJECT_ID;
+                }
                 syncd_nexthop_groups_[vnet][nexthops] = next_hop_group_entry;
             }
             else
@@ -1680,7 +1693,20 @@ void VNetRouteOrch::updateVnetTunnel(const BfdUpdate& update)
         NextHopGroupKey nexthops = nhg_info_pair.first;
         NextHopGroupInfo& nhg_info = nhg_info_pair.second;
 
-        if (!(nexthops.contains(endpoint)))
+        std::set<NextHopKey> next_hop_set = nexthops.getNextHops();
+        uint32_t seq_id = 0;
+        uint32_t nh_seq_id = 0;
+        for (auto nh: next_hop_set)
+        {
+            seq_id++;
+            if (nh == endpoint)
+            {
+                nh_seq_id = seq_id;
+                break;
+            }
+        }
+
+        if (!nh_seq_id)
         {
             continue;
         }
@@ -1702,6 +1728,13 @@ void VNetRouteOrch::updateVnetTunnel(const BfdUpdate& update)
                 nhgm_attr.value.oid = vrf_obj->getTunnelNextHop(endpoint);
                 nhgm_attrs.push_back(nhgm_attr);
 
+                if (gSwitchOrch->checkOrderedEcmpEnable())
+                {
+                    nhgm_attr.id = SAI_NEXT_HOP_GROUP_MEMBER_ATTR_SEQUENCE_ID;
+                    nhgm_attr.value.u32 = nh_seq_id;
+                    nhgm_attrs.push_back(nhgm_attr);
+                }
+
                 sai_status_t status = sai_next_hop_group_api->create_next_hop_group_member(&next_hop_group_member_id,
                                                                                 gSwitchId,
                                                                                 (uint32_t)nhgm_attrs.size(),
diff --git a/tests/test_nhg.py b/tests/test_nhg.py
diff --git a/tests/test_vnet.py b/tests/test_vnet.py
