New P4Orch development. (sonic-net#2425)

Upstream new development on p4orch:

*Reference count bug fix in wcmp manager.
*New update in gre tunnel manager.
*Bulk support in wcmp group member.

orchagent/p4orch/gre_tunnel_manager.cpp

@@ -89,9 +89,10 @@ std::vector<sai_attribute_t> getSaiAttrs(const P4GreTunnelEntry &gre_tunnel_entr
 } // namespace
 
 P4GreTunnelEntry::P4GreTunnelEntry(const std::string &tunnel_id, const std::string &router_interface_id,
-                                   const swss::IpAddress &encap_src_ip, const swss::IpAddress &encap_dst_ip)
+                                   const swss::IpAddress &encap_src_ip, const swss::IpAddress &encap_dst_ip,
+                                   const swss::IpAddress &neighbor_id)
     : tunnel_id(tunnel_id), router_interface_id(router_interface_id), encap_src_ip(encap_src_ip),
-      encap_dst_ip(encap_dst_ip)
+      encap_dst_ip(encap_dst_ip), neighbor_id(neighbor_id)
 {
     SWSS_LOG_ENTER();
     tunnel_key = KeyGenerator::generateTunnelKey(tunnel_id);
@@ -188,7 +189,7 @@ P4GreTunnelEntry *GreTunnelManager::getGreTunnelEntry(const std::string &tunnel_
     }
 };
 
-ReturnCodeOr<const std::string> GreTunnelManager::getUnderlayIfFromGreTunnelEntry(const std::string &tunnel_key)
+ReturnCodeOr<const P4GreTunnelEntry> GreTunnelManager::getConstGreTunnelEntry(const std::string &tunnel_key)
 {
     SWSS_LOG_ENTER();
 
@@ -200,7 +201,7 @@ ReturnCodeOr<const std::string> GreTunnelManager::getUnderlayIfFromGreTunnelEntr
     }
     else
     {
-        return tunnel->router_interface_id;
+        return *tunnel;
     }
 }
 
@@ -274,7 +275,7 @@ ReturnCode GreTunnelManager::processAddRequest(const P4GreTunnelAppDbEntry &app_
     SWSS_LOG_ENTER();
 
     P4GreTunnelEntry gre_tunnel_entry(app_db_entry.tunnel_id, app_db_entry.router_interface_id,
-                                      app_db_entry.encap_src_ip, app_db_entry.encap_dst_ip);
+                                      app_db_entry.encap_src_ip, app_db_entry.encap_dst_ip, app_db_entry.encap_dst_ip);
     auto status = createGreTunnel(gre_tunnel_entry);
     if (!status.ok())
     {
@@ -570,6 +571,15 @@ std::string GreTunnelManager::verifyStateCache(const P4GreTunnelAppDbEntry &app_
         return msg.str();
     }
 
+    if (gre_tunnel_entry->neighbor_id.to_string() != app_db_entry.encap_dst_ip.to_string())
+    {
+        std::stringstream msg;
+        msg << "GreTunnel " << QuotedVar(app_db_entry.tunnel_id) << " with destination IP "
+            << QuotedVar(app_db_entry.encap_dst_ip.to_string()) << " does not match internal cache "
+            << QuotedVar(gre_tunnel_entry->neighbor_id.to_string()) << " fo neighbor_id in GreTunnel manager.";
+        return msg.str();
+    }
+
     return m_p4OidMapper->verifyOIDMapping(SAI_OBJECT_TYPE_TUNNEL, gre_tunnel_entry->tunnel_key,
                                            gre_tunnel_entry->tunnel_oid);
 }
@@ -616,4 +626,4 @@ std::string GreTunnelManager::verifyStateAsicDb(const P4GreTunnelEntry *gre_tunn
 
     return verifyAttrs(values, exp, std::vector<swss::FieldValueTuple>{},
                        /*allow_unknown=*/false);
-}
+}

orchagent/p4orch/gre_tunnel_manager.h

@@ -36,6 +36,9 @@ struct P4GreTunnelEntry
     std::string router_interface_id;
     swss::IpAddress encap_src_ip;
     swss::IpAddress encap_dst_ip;
+    // neighbor_id is required to be equal to encap_dst_ip by BRCM. And the
+    // neighbor entry needs to be created before GRE tunnel object
+    swss::IpAddress neighbor_id;
 
     // SAI OID associated with this entry.
     sai_object_id_t tunnel_oid = SAI_NULL_OBJECT_ID;
@@ -45,7 +48,8 @@ struct P4GreTunnelEntry
     sai_object_id_t underlay_if_oid = SAI_NULL_OBJECT_ID;
 
     P4GreTunnelEntry(const std::string &tunnel_id, const std::string &router_interface_id,
-                     const swss::IpAddress &encap_src_ip, const swss::IpAddress &encap_dst_ip);
+                     const swss::IpAddress &encap_src_ip, const swss::IpAddress &encap_dst_ip,
+                     const swss::IpAddress &neighbor_id);
 };
 
 // GreTunnelManager listens to changes in table APP_P4RT_TUNNEL_TABLE_NAME and
@@ -69,7 +73,7 @@ class GreTunnelManager : public ObjectManagerInterface
     void drain() override;
     std::string verifyState(const std::string &key, const std::vector<swss::FieldValueTuple> &tuple) override;
 
-    ReturnCodeOr<const std::string> getUnderlayIfFromGreTunnelEntry(const std::string &gre_tunnel_key);
+    ReturnCodeOr<const P4GreTunnelEntry> getConstGreTunnelEntry(const std::string &gre_tunnel_key);
 
   private:
     // Gets the internal cached GRE tunnel entry by its key.

orchagent/p4orch/next_hop_manager.cpp

@@ -41,18 +41,22 @@ namespace
 
 ReturnCode validateAppDbEntry(const P4NextHopAppDbEntry &app_db_entry)
 {
+    // TODO(b/225242372): remove kSetNexthop action after P4RT and Orion update
+    // naming
     if (app_db_entry.action_str != p4orch::kSetIpNexthop && app_db_entry.action_str != p4orch::kSetNexthop &&
         app_db_entry.action_str != p4orch::kSetTunnelNexthop)
     {
         return ReturnCode(StatusCode::SWSS_RC_INVALID_PARAM)
                << "Invalid action " << QuotedVar(app_db_entry.action_str) << " of Nexthop App DB entry";
     }
-    if (app_db_entry.neighbor_id.isZero())
+    if (app_db_entry.action_str == p4orch::kSetIpNexthop && app_db_entry.neighbor_id.isZero())
     {
         return ReturnCode(StatusCode::SWSS_RC_INVALID_PARAM)
                << "Missing field " << QuotedVar(prependParamField(p4orch::kNeighborId)) << " for action "
                << QuotedVar(p4orch::kSetIpNexthop) << " in table entry";
     }
+    // TODO(b/225242372): remove kSetNexthop action after P4RT and Orion update
+    // naming
     if (app_db_entry.action_str == p4orch::kSetIpNexthop || app_db_entry.action_str == p4orch::kSetNexthop)
     {
         if (!app_db_entry.gre_tunnel_id.empty())
@@ -321,23 +325,27 @@ ReturnCode NextHopManager::createNextHop(P4NextHopEntry &next_hop_entry)
                                                                       << " already exists in centralized mapper");
     }
 
-    std::string router_interface_id = next_hop_entry.router_interface_id;
     if (!next_hop_entry.gre_tunnel_id.empty())
     {
-        auto underlay_if_or = gP4Orch->getGreTunnelManager()->getUnderlayIfFromGreTunnelEntry(
+        auto gre_tunnel_or = gP4Orch->getGreTunnelManager()->getConstGreTunnelEntry(
             KeyGenerator::generateTunnelKey(next_hop_entry.gre_tunnel_id));
-        if (!underlay_if_or.ok())
+        if (!gre_tunnel_or.ok())
         {
             LOG_ERROR_AND_RETURN(ReturnCode(StatusCode::SWSS_RC_NOT_FOUND)
                                  << "GRE Tunnel " << QuotedVar(next_hop_entry.gre_tunnel_id)
                                  << " does not exist in GRE Tunnel Manager");
         }
-        router_interface_id = *underlay_if_or;
+        next_hop_entry.router_interface_id = (*gre_tunnel_or).router_interface_id;
+        // BRCM requires neighbor object to be created before GRE tunnel, referring
+        // to the one in GRE tunnel object when creating next_hop_entry_with
+        // setTunnelAction
+        next_hop_entry.neighbor_id = (*gre_tunnel_or).neighbor_id;
     }
 
     // Neighbor doesn't have OID and the IP addr needed in next hop creation is
     // neighbor_id, so only check neighbor existence in centralized mapper.
-    const auto neighbor_key = KeyGenerator::generateNeighborKey(router_interface_id, next_hop_entry.neighbor_id);
+    const auto neighbor_key =
+        KeyGenerator::generateNeighborKey(next_hop_entry.router_interface_id, next_hop_entry.neighbor_id);
     if (!m_p4OidMapper->existsOID(SAI_OBJECT_TYPE_NEIGHBOR_ENTRY, neighbor_key))
     {
         LOG_ERROR_AND_RETURN(ReturnCode(StatusCode::SWSS_RC_NOT_FOUND)
@@ -456,15 +464,15 @@ ReturnCode NextHopManager::removeNextHop(const std::string &next_hop_key)
     std::string router_interface_id = next_hop_entry->router_interface_id;
     if (!next_hop_entry->gre_tunnel_id.empty())
     {
-        auto underlay_if_or = gP4Orch->getGreTunnelManager()->getUnderlayIfFromGreTunnelEntry(
+        auto gre_tunnel_or = gP4Orch->getGreTunnelManager()->getConstGreTunnelEntry(
             KeyGenerator::generateTunnelKey(next_hop_entry->gre_tunnel_id));
-        if (!underlay_if_or.ok())
+        if (!gre_tunnel_or.ok())
         {
             LOG_ERROR_AND_RETURN(ReturnCode(StatusCode::SWSS_RC_NOT_FOUND)
                                  << "GRE Tunnel " << QuotedVar(next_hop_entry->gre_tunnel_id)
                                  << " does not exist in GRE Tunnel Manager");
         }
-        router_interface_id = *underlay_if_or;
+        router_interface_id = (*gre_tunnel_or).router_interface_id;
     }
     m_p4OidMapper->decreaseRefCount(
         SAI_OBJECT_TYPE_NEIGHBOR_ENTRY,
@@ -560,15 +568,17 @@ std::string NextHopManager::verifyStateCache(const P4NextHopAppDbEntry &app_db_e
             << QuotedVar(next_hop_entry->next_hop_id) << " in nexthop manager.";
         return msg.str();
     }
-    if (next_hop_entry->router_interface_id != app_db_entry.router_interface_id)
+    if (app_db_entry.action_str == p4orch::kSetIpNexthop &&
+        next_hop_entry->router_interface_id != app_db_entry.router_interface_id)
     {
         std::stringstream msg;
         msg << "Nexthop " << QuotedVar(app_db_entry.next_hop_id) << " with ritf ID "
             << QuotedVar(app_db_entry.router_interface_id) << " does not match internal cache "
             << QuotedVar(next_hop_entry->router_interface_id) << " in nexthop manager.";
         return msg.str();
     }
-    if (next_hop_entry->neighbor_id.to_string() != app_db_entry.neighbor_id.to_string())
+    if (app_db_entry.action_str == p4orch::kSetIpNexthop &&
+        next_hop_entry->neighbor_id.to_string() != app_db_entry.neighbor_id.to_string())
     {
         std::stringstream msg;
         msg << "Nexthop " << QuotedVar(app_db_entry.next_hop_id) << " with neighbor ID "
@@ -577,14 +587,45 @@ std::string NextHopManager::verifyStateCache(const P4NextHopAppDbEntry &app_db_e
         return msg.str();
     }
 
-    if (next_hop_entry->gre_tunnel_id != app_db_entry.gre_tunnel_id)
+    if (app_db_entry.action_str == p4orch::kSetTunnelNexthop &&
+        next_hop_entry->gre_tunnel_id != app_db_entry.gre_tunnel_id)
     {
         std::stringstream msg;
         msg << "Nexthop " << QuotedVar(app_db_entry.next_hop_id) << " with GRE tunnel ID "
             << QuotedVar(app_db_entry.gre_tunnel_id) << " does not match internal cache "
             << QuotedVar(next_hop_entry->gre_tunnel_id) << " in nexthop manager.";
         return msg.str();
     }
+    if (!next_hop_entry->gre_tunnel_id.empty())
+    {
+        auto gre_tunnel_or = gP4Orch->getGreTunnelManager()->getConstGreTunnelEntry(
+            KeyGenerator::generateTunnelKey(next_hop_entry->gre_tunnel_id));
+        if (!gre_tunnel_or.ok())
+        {
+            std::stringstream msg;
+            msg << "GRE Tunnel " << QuotedVar(next_hop_entry->gre_tunnel_id) << " does not exist in GRE Tunnel Manager";
+            return msg.str();
+        }
+        P4GreTunnelEntry gre_tunnel = *gre_tunnel_or;
+        if (gre_tunnel.neighbor_id.to_string() != next_hop_entry->neighbor_id.to_string())
+        {
+            std::stringstream msg;
+            msg << "Nexthop " << QuotedVar(next_hop_entry->next_hop_id) << " with neighbor ID "
+                << QuotedVar(next_hop_entry->neighbor_id.to_string())
+                << " in nexthop manager does not match internal cache " << QuotedVar(gre_tunnel.neighbor_id.to_string())
+                << " with tunnel ID " << QuotedVar(gre_tunnel.tunnel_id) << " in GRE tunnel manager.";
+            return msg.str();
+        }
+        if (gre_tunnel.router_interface_id != next_hop_entry->router_interface_id)
+        {
+            std::stringstream msg;
+            msg << "Nexthop " << QuotedVar(next_hop_entry->next_hop_id) << " with rif ID "
+                << QuotedVar(next_hop_entry->router_interface_id)
+                << " in nexthop manager does not match internal cache " << QuotedVar(gre_tunnel.router_interface_id)
+                << " with tunnel ID " << QuotedVar(gre_tunnel.tunnel_id) << " in GRE tunnel manager.";
+            return msg.str();
+        }
+    }
 
     return m_p4OidMapper->verifyOIDMapping(SAI_OBJECT_TYPE_NEXT_HOP, next_hop_entry->next_hop_key,
                                            next_hop_entry->next_hop_oid);

orchagent/p4orch/p4orch.cpp

@@ -30,8 +30,8 @@ P4Orch::P4Orch(swss::DBConnector *db, std::vector<std::string> tableNames, VRFOr
     SWSS_LOG_ENTER();
 
     m_routerIntfManager = std::make_unique<RouterInterfaceManager>(&m_p4OidMapper, &m_publisher);
-    m_greTunnelManager = std::make_unique<GreTunnelManager>(&m_p4OidMapper, &m_publisher);
     m_neighborManager = std::make_unique<NeighborManager>(&m_p4OidMapper, &m_publisher);
+    m_greTunnelManager = std::make_unique<GreTunnelManager>(&m_p4OidMapper, &m_publisher);
     m_nextHopManager = std::make_unique<NextHopManager>(&m_p4OidMapper, &m_publisher);
     m_routeManager = std::make_unique<RouteManager>(&m_p4OidMapper, vrfOrch, &m_publisher);
     m_mirrorSessionManager = std::make_unique<p4orch::MirrorSessionManager>(&m_p4OidMapper, &m_publisher);
@@ -52,8 +52,8 @@ P4Orch::P4Orch(swss::DBConnector *db, std::vector<std::string> tableNames, VRFOr
     m_p4TableToManagerMap[APP_P4RT_L3_ADMIT_TABLE_NAME] = m_l3AdmitManager.get();
 
     m_p4ManagerPrecedence.push_back(m_routerIntfManager.get());
-    m_p4ManagerPrecedence.push_back(m_greTunnelManager.get());
     m_p4ManagerPrecedence.push_back(m_neighborManager.get());
+    m_p4ManagerPrecedence.push_back(m_greTunnelManager.get());
     m_p4ManagerPrecedence.push_back(m_nextHopManager.get());
     m_p4ManagerPrecedence.push_back(m_wcmpManager.get());
     m_p4ManagerPrecedence.push_back(m_routeManager.get());

orchagent/p4orch/p4orch_util.h

@@ -40,7 +40,7 @@ constexpr char *kSetWcmpGroupIdAndMetadata = "set_wcmp_group_id_and_metadata";
 constexpr char *kSetMetadataAndDrop = "set_metadata_and_drop";
 constexpr char *kSetNexthop = "set_nexthop";
 constexpr char *kSetIpNexthop = "set_ip_nexthop";
-constexpr char *kSetTunnelNexthop = "set_tunnel_encap_nexthop";
+constexpr char *kSetTunnelNexthop = "set_p2p_tunnel_encap_nexthop";
 constexpr char *kDrop = "drop";
 constexpr char *kTrap = "trap";
 constexpr char *kStage = "stage";
@@ -79,7 +79,7 @@ constexpr char *kTtl = "ttl";
 constexpr char *kTos = "tos";
 constexpr char *kMirrorAsIpv4Erspan = "mirror_as_ipv4_erspan";
 constexpr char *kL3AdmitAction = "admit_to_l3";
-constexpr char *kTunnelAction = "mark_for_tunnel_encap";
+constexpr char *kTunnelAction = "mark_for_p2p_tunnel_encap";
 } // namespace p4orch
 
 // Prepends "match/" to the input string str to construct a new string.

orchagent/p4orch/tests/gre_tunnel_manager_test.cpp

@@ -52,7 +52,7 @@ const P4GreTunnelAppDbEntry kP4GreTunnelAppDbEntry1{/*tunnel_id=*/"tunnel-1",
                                                     /*router_interface_id=*/"intf-eth-1/2/3",
                                                     /*encap_src_ip=*/swss::IpAddress("2607:f8b0:8096:3110::1"),
                                                     /*encap_dst_ip=*/swss::IpAddress("2607:f8b0:8096:311a::2"),
-                                                    /*action_str=*/"mark_for_tunnel_encap"};
+                                                    /*action_str=*/"mark_for_p2p_tunnel_encap"};
 
 std::unordered_map<sai_attr_id_t, sai_attribute_value_t> CreateAttributeListForGreTunnelObject(
     const P4GreTunnelAppDbEntry &app_entry, const sai_object_id_t &rif_oid)
@@ -304,6 +304,7 @@ bool GreTunnelManagerTest::ValidateGreTunnelEntryAdd(const P4GreTunnelAppDbEntry
     const auto *p4_gre_tunnel_entry = GetGreTunnelEntry(KeyGenerator::generateTunnelKey(app_db_entry.tunnel_id));
     if (p4_gre_tunnel_entry == nullptr || p4_gre_tunnel_entry->encap_src_ip != app_db_entry.encap_src_ip ||
         p4_gre_tunnel_entry->encap_dst_ip != app_db_entry.encap_dst_ip ||
+        p4_gre_tunnel_entry->neighbor_id != app_db_entry.encap_dst_ip ||
         p4_gre_tunnel_entry->router_interface_id != app_db_entry.router_interface_id ||
         p4_gre_tunnel_entry->tunnel_id != app_db_entry.tunnel_id)
     {
@@ -334,7 +335,7 @@ TEST_F(GreTunnelManagerTest, ProcessAddRequestShouldFailWhenDependingPortIsNotPr
                                             /*router_interface_id=*/"intf-eth-1/2/3",
                                             /*encap_src_ip=*/swss::IpAddress("2607:f8b0:8096:3110::1"),
                                             /*encap_dst_ip=*/swss::IpAddress("2607:f8b0:8096:311a::2"),
-                                            /*action_str=*/"mark_for_tunnel_encap"};
+                                            /*action_str=*/"mark_for_p2p_tunnel_encap"};
     const auto gre_tunnel_key = KeyGenerator::generateTunnelKey(kAppDbEntry.tunnel_id);
 
     EXPECT_EQ(StatusCode::SWSS_RC_NOT_FOUND, ProcessAddRequest(kAppDbEntry));
@@ -817,6 +818,12 @@ TEST_F(GreTunnelManagerTest, VerifyStateTest)
     EXPECT_FALSE(VerifyState(db_key, attributes).empty());
     p4_tunnel_entry->encap_dst_ip = saved_DST_IP;
 
+    // Verification should fail if IP mask mismatches.
+    auto saved_NEIGHBOR_ID = p4_tunnel_entry->neighbor_id;
+    p4_tunnel_entry->neighbor_id = swss::IpAddress("2.2.2.2");
+    EXPECT_FALSE(VerifyState(db_key, attributes).empty());
+    p4_tunnel_entry->neighbor_id = saved_NEIGHBOR_ID;
+
     // Verification should fail if tunnel_id mismatches.
     auto saved_tunnel_id = p4_tunnel_entry->tunnel_id;
     p4_tunnel_entry->tunnel_id = "invalid";

orchagent/p4orch/tests/mock_sai_next_hop_group.h

@@ -6,6 +6,7 @@
 extern "C"
 {
 #include "sai.h"
+#include "sainexthopgroup.h"
 }
 
 // Mock class including mock functions mapping to SAI next hop group's
@@ -27,6 +28,16 @@ class MockSaiNextHopGroup
 
     MOCK_METHOD2(set_next_hop_group_member_attribute,
                  sai_status_t(_In_ sai_object_id_t next_hop_group_member_id, _In_ const sai_attribute_t *attr));
+
+    MOCK_METHOD7(create_next_hop_group_members,
+                 sai_status_t(_In_ sai_object_id_t switch_id, _In_ uint32_t object_count,
+                              _In_ const uint32_t *attr_count, _In_ const sai_attribute_t **attr_list,
+                              _In_ sai_bulk_op_error_mode_t mode, _Out_ sai_object_id_t *object_id,
+                              _Out_ sai_status_t *object_statuses));
+
+    MOCK_METHOD4(remove_next_hop_group_members,
+                 sai_status_t(_In_ uint32_t object_count, _In_ const sai_object_id_t *object_id,
+                              _In_ sai_bulk_op_error_mode_t mode, _Out_ sai_status_t *object_statuses));
 };
 
 // Note that before mock functions below are used, mock_sai_next_hop_group must
@@ -62,3 +73,18 @@ sai_status_t set_next_hop_group_member_attribute(_In_ sai_object_id_t next_hop_g
 {
     return mock_sai_next_hop_group->set_next_hop_group_member_attribute(next_hop_group_member_id, attr);
 }
+
+sai_status_t create_next_hop_group_members(_In_ sai_object_id_t switch_id, _In_ uint32_t object_count,
+                                           _In_ const uint32_t *attr_count, _In_ const sai_attribute_t **attr_list,
+                                           _In_ sai_bulk_op_error_mode_t mode, _Out_ sai_object_id_t *object_id,
+                                           _Out_ sai_status_t *object_statuses)
+{
+    return mock_sai_next_hop_group->create_next_hop_group_members(switch_id, object_count, attr_count, attr_list, mode,
+                                                                  object_id, object_statuses);
+}
+
+sai_status_t remove_next_hop_group_members(_In_ uint32_t object_count, _In_ const sai_object_id_t *object_id,
+                                           _In_ sai_bulk_op_error_mode_t mode, _Out_ sai_status_t *object_statuses)
+{
+    return mock_sai_next_hop_group->remove_next_hop_group_members(object_count, object_id, mode, object_statuses);
+}