Skip to content

Commit

Permalink
New P4Orch development. (sonic-net#2425)
Browse files Browse the repository at this point in the history
Upstream new development on p4orch:

*Reference count bug fix in wcmp manager.
*New update in gre tunnel manager.
*Bulk support in wcmp group member.
  • Loading branch information
mint570 authored Nov 9, 2022
1 parent ab0e474 commit 6e288dc
Show file tree
Hide file tree
Showing 14 changed files with 1,484 additions and 752 deletions.
22 changes: 16 additions & 6 deletions orchagent/p4orch/gre_tunnel_manager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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();

Expand All @@ -200,7 +201,7 @@ ReturnCodeOr<const std::string> GreTunnelManager::getUnderlayIfFromGreTunnelEntr
}
else
{
return tunnel->router_interface_id;
return *tunnel;
}
}

Expand Down Expand Up @@ -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())
{
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -616,4 +626,4 @@ std::string GreTunnelManager::verifyStateAsicDb(const P4GreTunnelEntry *gre_tunn

return verifyAttrs(values, exp, std::vector<swss::FieldValueTuple>{},
/*allow_unknown=*/false);
}
}
8 changes: 6 additions & 2 deletions orchagent/p4orch/gre_tunnel_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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.
Expand Down
65 changes: 53 additions & 12 deletions orchagent/p4orch/next_hop_manager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 "
Expand All @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions orchagent/p4orch/p4orch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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());
Expand Down
4 changes: 2 additions & 2 deletions orchagent/p4orch/p4orch_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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.
Expand Down
11 changes: 9 additions & 2 deletions orchagent/p4orch/tests/gre_tunnel_manager_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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";
Expand Down
26 changes: 26 additions & 0 deletions orchagent/p4orch/tests/mock_sai_next_hop_group.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
extern "C"
{
#include "sai.h"
#include "sainexthopgroup.h"
}

// Mock class including mock functions mapping to SAI next hop group's
Expand All @@ -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
Expand Down Expand Up @@ -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);
}
Loading

0 comments on commit 6e288dc

Please sign in to comment.