From ff2112ef11b13486ea2b02acf28d924f4f851c7e Mon Sep 17 00:00:00 2001 From: Utpal Pintoo <utpal.pintoo@broadcom.com> Date: Sun, 7 Apr 2024 02:04:48 -0700 Subject: [PATCH] What I did: Implemented recursive nexthop group enhancement in Orchagent (NhgOrch). They have been proposed in: https://github.com/sonic-net/SONiC/pull/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. --- orchagent/nhgorch.cpp | 180 +++++++++++++++++++++++++++++++++++++----- orchagent/nhgorch.h | 9 ++- 2 files changed, 170 insertions(+), 19 deletions(-) diff --git a/orchagent/nhgorch.cpp b/orchagent/nhgorch.cpp index cefc2efbb1..fd757d6b97 100644 --- a/orchagent/nhgorch.cpp +++ b/orchagent/nhgorch.cpp @@ -8,6 +8,7 @@ extern sai_object_id_t gSwitchId; +extern IntfsOrch *gIntfsOrch; extern NeighOrch *gNeighOrch; extern RouteOrch *gRouteOrch; extern NhgOrch *gNhgOrch; @@ -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)) @@ -73,24 +76,96 @@ void NhgOrch::doTask(Consumer& consumer) if (fvField(i) == "mpls_nh") mpls_nhs = fvValue(i); + + if (fvField(i) == "nexthop_group") + { + nhgs = fvValue(i); + if (!nhgs.empty()) + is_recursive = true; + } } - /* Split ips and alaises strings into vectors of tokens. */ + /* 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, ','); /* 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 += ',' + nhgm; + } + if (invalid_member) + { + it = consumer.m_toSync.erase(it); + continue; + } + /* If no members are present */ + if (nhgs.empty()) { - nhg_str += mpls_nhv[i] + LABELSTACK_DELIMITER; + it++; + continue; + } + + /* Form nexthopgroup key with the nexthopgroup keys of available members */ + nhgv = tokenize(nhgs, ','); + + 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); @@ -133,10 +208,21 @@ 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))); } } @@ -216,6 +302,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; + } } } } @@ -367,7 +459,19 @@ 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); + + if ((nh_id == SAI_NULL_OBJECT_ID) && + !m_key.alias.compare(0, strlen("Loopback"), "Loopback")) + { + Port cpu_port; + gPortsOrch->getCpuPort(cpu_port); + nh_id = cpu_port.m_port_id; + } + } + else if (gNeighOrch->hasNextHop(m_key)) { nh_id = gNeighOrch->getNextHopId(m_key); } @@ -484,7 +588,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(); @@ -506,6 +610,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)); @@ -532,11 +637,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, the group ID will be the only member's NH ID */ + if (!isRecursive()) { const NextHopGroupMember& nhgm = m_members.begin()->second; sai_object_id_t nhid = nhgm.getNhId(); @@ -549,6 +651,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 @@ -663,9 +771,16 @@ 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 the group is temporary or non-recursive, update the neigh or rif ref-count and reset the ID. + if (m_is_temp || !isRecursive()) { + 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; } @@ -687,6 +802,9 @@ 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()); + ObjectBulker<sai_next_hop_group_api_t> nextHopGroupMemberBulker(sai_next_hop_group_api, gSwitchId, gMaxBulkSize); /* @@ -776,6 +894,22 @@ bool NextHopGroup::update(const NextHopGroupKey& nhg_key) { SWSS_LOG_ENTER(); + if (!(isRecursive() && isSynced())) + { + 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; @@ -891,7 +1025,12 @@ bool NextHopGroup::validateNextHop(const NextHopKey& nh_key) { SWSS_LOG_ENTER(); - return syncMembers({nh_key}); + if (isRecursive()) + { + return syncMembers({nh_key}); + } + + return true; } /* @@ -905,5 +1044,10 @@ bool NextHopGroup::invalidateNextHop(const NextHopKey& nh_key) { SWSS_LOG_ENTER(); - return removeMembers({nh_key}); + if (isRecursive()) + { + return removeMembers({nh_key}); + } + + return true; } diff --git a/orchagent/nhgorch.h b/orchagent/nhgorch.h index 225d3ffaf2..d8a92e6131 100644 --- a/orchagent/nhgorch.h +++ b/orchagent/nhgorch.h @@ -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); @@ -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. */ @@ -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;