Skip to content

Commit

Permalink
[Orchagent] Recursive nexthop group enhancement (sonic-net#3105)
Browse files Browse the repository at this point in the history 
* 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.

* 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.

* 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.

* 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.

* Addressed review comments
  • Loading branch information
@utpalkantpintoo
utpalkantpintoo authored May 18, 2024
1 parent 84ce1a7 commit 536f43a
Show file tree
Hide file tree
Showing 3 changed files with 253 additions and 24 deletions.
190 changes: 171 additions & 19 deletions orchagent/nhgorch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

extern sai_object_id_t gSwitchId;

extern IntfsOrch *gIntfsOrch;
extern NeighOrch *gNeighOrch;
extern RouteOrch *gRouteOrch;
extern NhgOrch *gNhgOrch;
Expand Down Expand Up @@ -58,6 +59,8 @@ void NhgOrch::doTask(Consumer& consumer)
string aliases;
string weights;
string mpls_nhs;
string nhgs;
bool is_recursive = false;

/* Get group's next hop IPs and aliases */
for (auto i : kfvFieldsValues(t))
Expand All @@ -73,31 +76,111 @@ void NhgOrch::doTask(Consumer& consumer)

if (fvField(i) == "mpls_nh")
mpls_nhs = fvValue(i);
}

/* Split ips and alaises strings into vectors of tokens. */
if (fvField(i) == "nexthop_group")
{
nhgs = fvValue(i);
if (!nhgs.empty())
is_recursive = true;
}
}
/* A NHG should not have both regular(ip/alias) and recursive fields */
if (is_recursive && (!ips.empty() || !aliases.empty()))
{
SWSS_LOG_ERROR("Nexthop group %s has both regular(ip/alias) and recursive fields", index.c_str());
it = consumer.m_toSync.erase(it);
continue;
}
/* Split ips and aliases strings into vectors of tokens. */
vector<string> ipv = tokenize(ips, ',');
vector<string> alsv = tokenize(aliases, ',');
vector<string> mpls_nhv = tokenize(mpls_nhs, ',');
vector<string> nhgv = tokenize(nhgs, NHG_DELIMITER);

/* Create the next hop group key. */
string nhg_str;

for (uint32_t i = 0; i < ipv.size(); i++)
/* Keeps track of any non-existing member of a recursive nexthop group */
bool non_existent_member = false;

if (is_recursive)
{
if (i) nhg_str += NHG_DELIMITER;
if (!mpls_nhv.empty() && mpls_nhv[i] != "na")
SWSS_LOG_INFO("Adding recursive nexthop group %s with %s", index.c_str(), nhgs.c_str());

/* Reset the "nexthop_group" field and update it with only the existing members */
nhgs = "";

/* Check if any of the members are a recursive or temporary nexthop group */
bool invalid_member = false;

for (auto& nhgm : nhgv)
{
const auto& nhgm_it = m_syncdNextHopGroups.find(nhgm);
if (nhgm_it == m_syncdNextHopGroups.end())
{
SWSS_LOG_INFO("Member nexthop group %s in parent nhg %s not ready",
nhgm.c_str(), index.c_str());

non_existent_member = true;
continue;
}
if ((nhgm_it->second.nhg) &&
(nhgm_it->second.nhg->isRecursive() || nhgm_it->second.nhg->isTemp()))
{
SWSS_LOG_ERROR("Invalid member nexthop group %s in parent nhg %s",
nhgm.c_str(), index.c_str());

invalid_member = true;
break;
}
/* Keep only the members which exist in the local cache */
if (nhgs.empty())
nhgs = nhgm;
else
nhgs += NHG_DELIMITER + nhgm;
}
if (invalid_member)
{
nhg_str += mpls_nhv[i] + LABELSTACK_DELIMITER;
it = consumer.m_toSync.erase(it);
continue;
}
/* If no members are present */
if (nhgs.empty())
{
it++;
continue;
}

/* Form nexthopgroup key with the nexthopgroup keys of available members */
nhgv = tokenize(nhgs, NHG_DELIMITER);

for (uint32_t i = 0; i < nhgv.size(); i++)
{
if (i) nhg_str += NHG_DELIMITER;

nhg_str += m_syncdNextHopGroups.at(nhgv[i]).nhg->getKey().to_string();
}
}
else
{
for (uint32_t i = 0; i < ipv.size(); i++)
{
if (i) nhg_str += NHG_DELIMITER;
if (!mpls_nhv.empty() && mpls_nhv[i] != "na")
{
nhg_str += mpls_nhv[i] + LABELSTACK_DELIMITER;
}
nhg_str += ipv[i] + NH_DELIMITER + alsv[i];
}
nhg_str += ipv[i] + NH_DELIMITER + alsv[i];
}

NextHopGroupKey nhg_key = NextHopGroupKey(nhg_str, weights);

/* If the group does not exist, create one. */
if (nhg_it == m_syncdNextHopGroups.end())
{
SWSS_LOG_INFO("Create nexthop group %s with %s", index.c_str(), nhg_str.c_str());

/*
* If we've reached the NHG limit, we're going to create a temporary
* group, represented by one of it's NH only until we have
Expand Down Expand Up @@ -133,17 +216,30 @@ void NhgOrch::doTask(Consumer& consumer)
else
{
auto nhg = std::make_unique<NextHopGroup>(nhg_key, false);
success = nhg->sync();

/*
* Mark the nexthop group as recursive so as to create a
* nexthop group object even if it has just one available path
*/
nhg->setRecursive(is_recursive);

success = nhg->sync();
if (success)
{
/* Keep the msg in loop if any member path is not available yet */
if (is_recursive && non_existent_member)
{
success = false;
}
m_syncdNextHopGroups.emplace(index, NhgEntry<NextHopGroup>(std::move(nhg)));
}
}
}
/* If the group exists, update it. */
else
{
SWSS_LOG_INFO("Update nexthop group %s with %s", index.c_str(), nhg_str.c_str());

const auto& nhg_ptr = nhg_it->second.nhg;

/*
Expand Down Expand Up @@ -216,6 +312,12 @@ void NhgOrch::doTask(Consumer& consumer)
else
{
success = nhg_ptr->update(nhg_key);

/* Keep the msg in loop if any member path is not available yet */
if (is_recursive && non_existent_member)
{
success = false;
}
}
}
}
Expand Down Expand Up @@ -367,7 +469,11 @@ sai_object_id_t NextHopGroupMember::getNhId() const

sai_object_id_t nh_id = SAI_NULL_OBJECT_ID;

if (gNeighOrch->hasNextHop(m_key))
if (m_key.isIntfNextHop())
{
nh_id = gIntfsOrch->getRouterIntfsId(m_key.alias);
}
else if (gNeighOrch->hasNextHop(m_key))
{
nh_id = gNeighOrch->getNextHopId(m_key);
}
Expand Down Expand Up @@ -484,7 +590,7 @@ NextHopGroupMember::~NextHopGroupMember()
* Params: IN key - The next hop group's key.
* Returns: Nothing.
*/
NextHopGroup::NextHopGroup(const NextHopGroupKey& key, bool is_temp) : NhgCommon(key), m_is_temp(is_temp)
NextHopGroup::NextHopGroup(const NextHopGroupKey& key, bool is_temp) : NhgCommon(key), m_is_temp(is_temp), m_is_recursive(false)
{
SWSS_LOG_ENTER();

Expand All @@ -506,6 +612,7 @@ NextHopGroup& NextHopGroup::operator=(NextHopGroup&& nhg)
SWSS_LOG_ENTER();

m_is_temp = nhg.m_is_temp;
m_is_recursive = nhg.m_is_recursive;

NhgCommon::operator=(std::move(nhg));

Expand All @@ -532,11 +639,8 @@ bool NextHopGroup::sync()
return true;
}

/*
* If the group is temporary, the group ID will be the only member's NH
* ID.
*/
if (m_is_temp)
/* 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();
Expand All @@ -549,6 +653,12 @@ bool NextHopGroup::sync()
else
{
m_id = nhid;

auto nh_key = nhgm.getKey();
if (nh_key.isIntfNextHop())
gIntfsOrch->increaseRouterIntfsRefCount(nh_key.alias);
else
gNeighOrch->increaseNextHopRefCount(nh_key);
}
}
else
Expand Down Expand Up @@ -663,9 +773,21 @@ bool NextHopGroup::remove()
{
SWSS_LOG_ENTER();

// If the group is temporary, there is nothing to be done - just reset the ID.
if (m_is_temp)
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() && m_members.size() == 1))
{
const NextHopGroupMember& nhgm = m_members.begin()->second;
auto nh_key = nhgm.getKey();
if (nh_key.isIntfNextHop())
gIntfsOrch->decreaseRouterIntfsRefCount(nh_key.alias);
else
gNeighOrch->decreaseNextHopRefCount(nh_key);

m_id = SAI_NULL_OBJECT_ID;
return true;
}
Expand All @@ -687,6 +809,9 @@ bool NextHopGroup::syncMembers(const std::set<NextHopKey>& nh_keys)
{
SWSS_LOG_ENTER();

/* 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);

/*
Expand Down Expand Up @@ -776,6 +901,23 @@ bool NextHopGroup::update(const NextHopGroupKey& nhg_key)
{
SWSS_LOG_ENTER();

if (!isSynced() ||
(!isRecursive() && (m_members.size() == 1 || nhg_key.getSize() == 1)))
{
bool was_synced = isSynced();
bool was_temp = isTemp();
*this = NextHopGroup(nhg_key, false);

/*
* For temporary nexthop group being updated, set the recursive flag
* as it is expected to get promoted to multiple NHG
*/
setRecursive(was_temp);

/* Sync the group only if it was synced before. */
return (was_synced ? sync() : true);
}

/* Update the key. */
m_key = nhg_key;

Expand Down Expand Up @@ -891,7 +1033,12 @@ bool NextHopGroup::validateNextHop(const NextHopKey& nh_key)
{
SWSS_LOG_ENTER();

return syncMembers({nh_key});
if (isRecursive() || (m_members.size() > 1))
{
return syncMembers({nh_key});
}

return true;
}

/*
Expand All @@ -905,5 +1052,10 @@ bool NextHopGroup::invalidateNextHop(const NextHopKey& nh_key)
{
SWSS_LOG_ENTER();

return removeMembers({nh_key});
if (isRecursive() || (m_members.size() > 1))
{
return removeMembers({nh_key});
}

return true;
}
9 changes: 8 additions & 1 deletion orchagent/nhgorch.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class NextHopGroup : public NhgCommon<NextHopGroupKey, NextHopKey, NextHopGroupM
explicit NextHopGroup(const NextHopGroupKey& key, bool is_temp);

NextHopGroup(NextHopGroup&& nhg) :
NhgCommon(move(nhg)), m_is_temp(nhg.m_is_temp)
NhgCommon(move(nhg)), m_is_temp(nhg.m_is_temp), m_is_recursive(nhg.m_is_recursive)
{ SWSS_LOG_ENTER(); }

NextHopGroup& operator=(NextHopGroup&& nhg);
Expand Down Expand Up @@ -83,6 +83,10 @@ class NextHopGroup : public NhgCommon<NextHopGroupKey, NextHopKey, NextHopGroupM
/* Getters / Setters. */
inline bool isTemp() const override { return m_is_temp; }

inline bool isRecursive() const { return m_is_recursive; }

inline void setRecursive(bool is_recursive) { m_is_recursive = is_recursive; }

NextHopGroupKey getNhgKey() const override { return m_key; }

/* Convert NHG's details to a string. */
Expand All @@ -95,6 +99,9 @@ class NextHopGroup : public NhgCommon<NextHopGroupKey, NextHopKey, NextHopGroupM
/* Whether the group is temporary or not. */
bool m_is_temp;

/* Whether the group is recursive i.e. having other nexthop group(s) as members */
bool m_is_recursive;

/* Add group's members over the SAI API for the given keys. */
bool syncMembers(const set<NextHopKey>& nh_keys) override;

Expand Down
Loading

0 comments on commit 536f43a

Please sign in to comment.