From e690505eac761ba84390f4b192fb044ddf99f904 Mon Sep 17 00:00:00 2001 From: Manas Kumar Mandal Date: Wed, 25 Mar 2026 10:43:16 -0700 Subject: [PATCH 01/20] Protection nexthop group support in nhgorch Signed-off-by: Manas Kumar Mandal --- orchagent/Makefile.am | 1 + orchagent/nhgorch.cpp | 244 ++++++++++++++++++ orchagent/nhgorch.h | 56 ++++ orchagent/protnhg.cpp | 439 ++++++++++++++++++++++++++++++++ orchagent/protnhg.h | 101 ++++++++ tests/mock_tests/Makefile.am | 2 + tests/mock_tests/protnhg_ut.cpp | 332 ++++++++++++++++++++++++ 7 files changed, 1175 insertions(+) create mode 100644 orchagent/protnhg.cpp create mode 100644 orchagent/protnhg.h create mode 100644 tests/mock_tests/protnhg_ut.cpp diff --git a/orchagent/Makefile.am b/orchagent/Makefile.am index a504f41ab2c..1804133e51c 100644 --- a/orchagent/Makefile.am +++ b/orchagent/Makefile.am @@ -58,6 +58,7 @@ orchagent_SOURCES = \ notifications.cpp \ nhgorch.cpp \ nhgbase.cpp \ + protnhg.cpp \ cbf/cbfnhgorch.cpp \ cbf/nhgmaporch.cpp \ routeorch.cpp \ diff --git a/orchagent/nhgorch.cpp b/orchagent/nhgorch.cpp index b457775afbc..be820fd04fe 100644 --- a/orchagent/nhgorch.cpp +++ b/orchagent/nhgorch.cpp @@ -1158,3 +1158,247 @@ bool NextHopGroup::invalidateNextHop(const NextHopKey& nh_key) return true; } + +/* ----------------------------------------------------------------------- */ +/* Protection NHG management APIs */ +/* ----------------------------------------------------------------------- */ + +bool NhgOrch::isHwProtectionSupported() +{ + static bool checked = false; + static bool supported = false; + + if (checked) + { + return supported; + } + + checked = 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) + { + SWSS_LOG_NOTICE("Cannot query NHG type enum metadata"); + return false; + } + + vector values_list(meta->enummetadata->valuescount); + sai_s32_list_t values; + values.count = static_cast(values_list.size()); + values.list = values_list.data(); + + sai_status_t 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) + { + SWSS_LOG_NOTICE("Failed to query NHG type capabilities, rv: %d", status); + return false; + } + + for (uint32_t i = 0; i < values.count; i++) + { + if (values.list[i] == SAI_NEXT_HOP_GROUP_TYPE_HW_PROTECTION) + { + supported = true; + break; + } + } + + SWSS_LOG_NOTICE("SAI_NEXT_HOP_GROUP_TYPE_HW_PROTECTION is %s", + supported ? "supported" : "not supported"); + return supported; +} + +bool NhgOrch::createProtNhg(const string &key, + const vector &primary_nhs, + const NextHopKey &standby_nh, + sai_object_id_t standby_nh_id) +{ + SWSS_LOG_ENTER(); + + if (primary_nhs.empty()) + { + SWSS_LOG_ERROR("Protection NHG %s requires at least one primary NH", key.c_str()); + return false; + } + + if (m_protNhgs.find(key) != m_protNhgs.end()) + { + SWSS_LOG_ERROR("Protection NHG %s already exists", key.c_str()); + return false; + } + + if (gRouteOrch->getNhgCount() + NhgBase::getSyncedCount() >= + gRouteOrch->getMaxNhgCount()) + { + SWSS_LOG_ERROR("NHG capacity exhausted, cannot create protection NHG %s", + key.c_str()); + return false; + } + + auto nhg = make_unique(key, primary_nhs, standby_nh, standby_nh_id); + + if (!nhg->sync()) + { + SWSS_LOG_ERROR("Failed to sync protection NHG %s", key.c_str()); + return false; + } + + m_protNhgs.emplace(key, NhgEntry(move(nhg))); + + string primary_str; + for (const auto &nh : primary_nhs) + { + if (!primary_str.empty()) + { + primary_str += ", "; + } + primary_str += nh.to_string(); + } + + SWSS_LOG_NOTICE("Created protection NHG %s (primaries: [%s], standby: %s)", + key.c_str(), + primary_str.c_str(), + standby_nh.to_string().c_str()); + + return true; +} + +bool NhgOrch::removeProtNhg(const string &key) +{ + SWSS_LOG_ENTER(); + + auto it = m_protNhgs.find(key); + if (it == m_protNhgs.end()) + { + SWSS_LOG_ERROR("Protection NHG %s does not exist", key.c_str()); + return false; + } + + if (it->second.ref_count > 0) + { + SWSS_LOG_ERROR("Protection NHG %s still referenced (ref_count=%u)", + key.c_str(), it->second.ref_count); + return false; + } + + if (!it->second.nhg->remove()) + { + SWSS_LOG_ERROR("Failed to remove protection NHG %s from SAI", key.c_str()); + return false; + } + + m_protNhgs.erase(it); + + SWSS_LOG_NOTICE("Removed protection NHG %s", key.c_str()); + + return true; +} + +bool NhgOrch::hasProtNhg(const string &key) const +{ + SWSS_LOG_ENTER(); + return m_protNhgs.find(key) != m_protNhgs.end(); +} + +const ProtNhg& NhgOrch::getProtNhg(const string &key) const +{ + SWSS_LOG_ENTER(); + return *m_protNhgs.at(key).nhg; +} + +sai_object_id_t NhgOrch::getProtNhgId(const string &key) const +{ + SWSS_LOG_ENTER(); + + auto it = m_protNhgs.find(key); + if (it == m_protNhgs.end()) + { + return SAI_NULL_OBJECT_ID; + } + + return it->second.nhg->getId(); +} + +bool NhgOrch::setProtNhgAdminRole(const string &key, sai_int32_t admin_role) +{ + SWSS_LOG_ENTER(); + + auto it = m_protNhgs.find(key); + if (it == m_protNhgs.end()) + { + SWSS_LOG_ERROR("Protection NHG %s does not exist", key.c_str()); + return false; + } + + return it->second.nhg->setAdminRole(admin_role); +} + +bool NhgOrch::setProtNhgMonitoredObject(const string &key, + const NextHopKey &nh_key, + sai_object_id_t monitored_oid) +{ + SWSS_LOG_ENTER(); + + auto it = m_protNhgs.find(key); + if (it == m_protNhgs.end()) + { + SWSS_LOG_ERROR("Protection NHG %s does not exist", key.c_str()); + return false; + } + + return it->second.nhg->updateMemberMonitoredObject(nh_key, monitored_oid); +} + +bool NhgOrch::getProtNhgMemberObservedRole( + const string &key, + const NextHopKey &nh_key, + sai_next_hop_group_member_observed_role_t &observed_role) const +{ + SWSS_LOG_ENTER(); + + auto it = m_protNhgs.find(key); + if (it == m_protNhgs.end()) + { + SWSS_LOG_ERROR("Protection NHG %s does not exist", key.c_str()); + return false; + } + + return it->second.nhg->getMemberObservedRole(nh_key, observed_role); +} + +bool NhgOrch::getProtNhgAllObservedRoles( + const string &key, + map &observed_roles) const +{ + SWSS_LOG_ENTER(); + + auto it = m_protNhgs.find(key); + if (it == m_protNhgs.end()) + { + SWSS_LOG_ERROR("Protection NHG %s does not exist", key.c_str()); + return false; + } + + return it->second.nhg->getAllMemberObservedRoles(observed_roles); +} + +void NhgOrch::incProtNhgRefCount(const string &key) +{ + SWSS_LOG_ENTER(); + ++m_protNhgs.at(key).ref_count; +} + +void NhgOrch::decProtNhgRefCount(const string &key) +{ + SWSS_LOG_ENTER(); + + auto &entry = m_protNhgs.at(key); + assert(entry.ref_count > 0); + --entry.ref_count; +} diff --git a/orchagent/nhgorch.h b/orchagent/nhgorch.h index d8a92e61310..69e86aa7d6f 100644 --- a/orchagent/nhgorch.h +++ b/orchagent/nhgorch.h @@ -1,6 +1,7 @@ #pragma once #include "cbf/cbfnhgorch.h" +#include "protnhg.h" #include "vector" #include "portsorch.h" #include "routeorch.h" @@ -129,6 +130,61 @@ class NhgOrch : public NhgOrchCommon bool validateNextHop(const NextHopKey& nh_key); bool invalidateNextHop(const NextHopKey& nh_key); + /* Check if hardware supports protection NHG type. */ + bool isHwProtectionSupported(); + + /* + * Protection NHG APIs. + * MuxOrch is the primary consumer of these for dual-ToR hardware + * protection switching. Capacity accounting is shared with ECMP NHGs. + */ + + /* Create a protection NHG with one or more primary and one standby next hop. + * standby_nh_id: optional pre-resolved SAI OID for the standby NH + * (e.g., tunnel NH not registered in NeighOrch). + */ + bool createProtNhg(const string &key, + const vector &primary_nhs, + const NextHopKey &standby_nh, + sai_object_id_t standby_nh_id = SAI_NULL_OBJECT_ID); + + /* Remove a protection NHG by key. */ + bool removeProtNhg(const string &key); + + /* Check if a protection NHG exists. */ + bool hasProtNhg(const string &key) const; + + /* Get a const reference to a protection NHG. */ + const ProtNhg& getProtNhg(const string &key) const; + + /* Get the SAI object ID of a protection NHG. */ + sai_object_id_t getProtNhgId(const string &key) const; + + /* Toggle admin role (auto / force primary / force standby). */ + bool setProtNhgAdminRole(const string &key, sai_int32_t admin_role); + + /* Update the monitored object on a protection NHG member. */ + bool setProtNhgMonitoredObject(const string &key, + const NextHopKey &nh_key, + sai_object_id_t monitored_oid); + + /* Query the hardware-observed role (active/inactive) of a protection NHG member. */ + bool getProtNhgMemberObservedRole(const string &key, + const NextHopKey &nh_key, + sai_next_hop_group_member_observed_role_t &observed_role) const; + + /* Query observed roles for all synced members of a protection NHG. */ + bool getProtNhgAllObservedRoles( + const string &key, + map &observed_roles) const; + + /* Ref counting for protection NHGs. */ + void incProtNhgRefCount(const string &key); + void decProtNhgRefCount(const string &key); + private: void doTask(Consumer& consumer) override; + + /* Storage for protection NHGs, keyed by a string identifier (e.g., port name). */ + unordered_map> m_protNhgs; }; diff --git a/orchagent/protnhg.cpp b/orchagent/protnhg.cpp new file mode 100644 index 00000000000..e133acf0139 --- /dev/null +++ b/orchagent/protnhg.cpp @@ -0,0 +1,439 @@ +#include "protnhg.h" +#include "neighorch.h" +#include "logger.h" +#include "sai_serialize.h" + +extern NeighOrch *gNeighOrch; + +ProtNhgMember::ProtNhgMember(const NextHopKey &key, ProtNhgRole role, + sai_object_id_t nh_id_override) : + NhgMember(key), + m_role(role), + m_monitored_oid(SAI_NULL_OBJECT_ID), + m_nh_id_override(nh_id_override) +{ + SWSS_LOG_ENTER(); +} + +ProtNhgMember::ProtNhgMember(ProtNhgMember &&nhgm) : + NhgMember(move(nhgm)), + m_role(nhgm.m_role), + m_monitored_oid(nhgm.m_monitored_oid), + m_nh_id_override(nhgm.m_nh_id_override) +{ + SWSS_LOG_ENTER(); + nhgm.m_monitored_oid = SAI_NULL_OBJECT_ID; + nhgm.m_nh_id_override = SAI_NULL_OBJECT_ID; +} + +ProtNhgMember::~ProtNhgMember() +{ + SWSS_LOG_ENTER(); +} + +void ProtNhgMember::sync(sai_object_id_t gm_id) +{ + SWSS_LOG_ENTER(); + NhgMember::sync(gm_id); + + if (m_nh_id_override == SAI_NULL_OBJECT_ID) + { + gNeighOrch->increaseNextHopRefCount(m_key); + } +} + +void ProtNhgMember::remove() +{ + SWSS_LOG_ENTER(); + + if (!isSynced()) + { + return; + } + + if (m_nh_id_override == SAI_NULL_OBJECT_ID) + { + gNeighOrch->decreaseNextHopRefCount(m_key); + } + NhgMember::remove(); +} + +sai_object_id_t ProtNhgMember::getNhId() const +{ + SWSS_LOG_ENTER(); + + if (m_nh_id_override != SAI_NULL_OBJECT_ID) + { + return m_nh_id_override; + } + + if (gNeighOrch->hasNextHop(m_key)) + { + return gNeighOrch->getNextHopId(m_key); + } + + return SAI_NULL_OBJECT_ID; +} + +bool ProtNhgMember::updateMonitoredObject(sai_object_id_t oid) +{ + SWSS_LOG_ENTER(); + + m_monitored_oid = oid; + + if (!isSynced()) + { + return true; + } + + sai_attribute_t attr; + attr.id = SAI_NEXT_HOP_GROUP_MEMBER_ATTR_MONITORED_OBJECT; + attr.value.oid = oid; + + sai_status_t status = + sai_next_hop_group_api->set_next_hop_group_member_attribute(m_gm_id, &attr); + + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to update monitored object for member %s, rv: %d", + to_string().c_str(), status); + return false; + } + + return true; +} + +bool ProtNhgMember::getObservedRole( + sai_next_hop_group_member_observed_role_t &observed_role) const +{ + SWSS_LOG_ENTER(); + + if (!isSynced()) + { + SWSS_LOG_WARN("Cannot query observed role on unsynced member %s", + m_key.to_string().c_str()); + return false; + } + + sai_attribute_t attr; + attr.id = SAI_NEXT_HOP_GROUP_MEMBER_ATTR_OBSERVED_ROLE; + + sai_status_t status = + sai_next_hop_group_api->get_next_hop_group_member_attribute(m_gm_id, 1, &attr); + + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to get observed role for member %s, rv: %d", + m_key.to_string().c_str(), status); + return false; + } + + observed_role = + static_cast(attr.value.s32); + + return true; +} + +string ProtNhgMember::to_string() const +{ + string role_str = (m_role == ProtNhgRole::PRIMARY) ? "primary" : "standby"; + return m_key.to_string() + " [" + role_str + "], SAI ID: " + std::to_string(m_gm_id); +} + +/* ----------------------------------------------------------------------- */ + +ProtNhg::ProtNhg(const string &key, + const vector &primary_nhs, + const NextHopKey &standby_nh, + sai_object_id_t standby_nh_id) : + NhgCommon(key) +{ + SWSS_LOG_ENTER(); + + for (const auto &nh : primary_nhs) + { + m_members.emplace(nh, ProtNhgMember(nh, ProtNhgRole::PRIMARY)); + } + m_members.emplace(standby_nh, + ProtNhgMember(standby_nh, ProtNhgRole::STANDBY, standby_nh_id)); +} + +ProtNhg::ProtNhg(ProtNhg &&nhg) : + NhgCommon(move(nhg)) +{ + SWSS_LOG_ENTER(); +} + +bool ProtNhg::sync() +{ + SWSS_LOG_ENTER(); + + if (isSynced()) + { + return true; + } + + if (m_members.size() < 2) + { + SWSS_LOG_ERROR("Protection NHG %s must have at least 2 members, has %zu", + m_key.c_str(), m_members.size()); + return false; + } + + sai_attribute_t nhg_attr; + vector nhg_attrs; + + nhg_attr.id = SAI_NEXT_HOP_GROUP_ATTR_TYPE; + nhg_attr.value.s32 = SAI_NEXT_HOP_GROUP_TYPE_HW_PROTECTION; + nhg_attrs.push_back(nhg_attr); + + sai_status_t status = sai_next_hop_group_api->create_next_hop_group( + &m_id, + gSwitchId, + static_cast(nhg_attrs.size()), + nhg_attrs.data()); + + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to create protection NHG %s, rv: %d", + m_key.c_str(), status); + return false; + } + + gCrmOrch->incCrmResUsedCounter(CrmResourceType::CRM_NEXTHOP_GROUP); + incSyncedCount(); + + set member_keys; + for (const auto &mbr : m_members) + { + member_keys.insert(mbr.first); + } + + if (!syncMembers(member_keys)) + { + SWSS_LOG_WARN("Failed to sync members of protection NHG %s", m_key.c_str()); + return false; + } + + return true; +} + +bool ProtNhg::remove() +{ + SWSS_LOG_ENTER(); + + if (!isSynced()) + { + return true; + } + + return NhgCommon::remove(); +} + +bool ProtNhg::setAdminRole(sai_int32_t admin_role) +{ + SWSS_LOG_ENTER(); + + if (!isSynced()) + { + SWSS_LOG_ERROR("Cannot set admin role on unsynced protection NHG %s", + m_key.c_str()); + return false; + } + + sai_attribute_t attr; + attr.id = SAI_NEXT_HOP_GROUP_ATTR_ADMIN_ROLE; + attr.value.s32 = admin_role; + + sai_status_t status = + sai_next_hop_group_api->set_next_hop_group_attribute(m_id, &attr); + + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to set admin role %d on protection NHG %s, rv: %d", + admin_role, m_key.c_str(), status); + return false; + } + + SWSS_LOG_NOTICE("Set admin role %d on protection NHG %s", + admin_role, m_key.c_str()); + + return true; +} + +bool ProtNhg::updateMemberMonitoredObject(const NextHopKey &nh_key, + sai_object_id_t monitored_oid) +{ + SWSS_LOG_ENTER(); + + auto it = m_members.find(nh_key); + if (it == m_members.end()) + { + SWSS_LOG_ERROR("Member %s not found in protection NHG %s", + nh_key.to_string().c_str(), m_key.c_str()); + return false; + } + + return it->second.updateMonitoredObject(monitored_oid); +} + +vector ProtNhg::getPrimaryMembers() const +{ + SWSS_LOG_ENTER(); + + vector primaries; + for (const auto &mbr : m_members) + { + if (mbr.second.getRole() == ProtNhgRole::PRIMARY) + { + primaries.push_back(&mbr.second); + } + } + + return primaries; +} + +const ProtNhgMember* ProtNhg::getStandbyMember() const +{ + SWSS_LOG_ENTER(); + + for (const auto &mbr : m_members) + { + if (mbr.second.getRole() == ProtNhgRole::STANDBY) + { + return &mbr.second; + } + } + + return nullptr; +} + +bool ProtNhg::getMemberObservedRole( + const NextHopKey &nh_key, + sai_next_hop_group_member_observed_role_t &observed_role) const +{ + SWSS_LOG_ENTER(); + + auto it = m_members.find(nh_key); + if (it == m_members.end()) + { + SWSS_LOG_ERROR("Member %s not found in protection NHG %s", + nh_key.to_string().c_str(), m_key.c_str()); + return false; + } + + return it->second.getObservedRole(observed_role); +} + +bool ProtNhg::getAllMemberObservedRoles( + map &observed_roles) const +{ + SWSS_LOG_ENTER(); + + observed_roles.clear(); + + bool success = true; + for (const auto &mbr : m_members) + { + if (!mbr.second.isSynced()) + { + continue; + } + + sai_next_hop_group_member_observed_role_t role; + if (mbr.second.getObservedRole(role)) + { + observed_roles[mbr.first] = role; + } + else + { + SWSS_LOG_WARN("Failed to get observed role for member %s in NHG %s", + mbr.first.to_string().c_str(), m_key.c_str()); + success = false; + } + } + + return success; +} + +bool ProtNhg::syncMembers(const set &member_keys) +{ + SWSS_LOG_ENTER(); + + ObjectBulker bulker(sai_next_hop_group_api, + gSwitchId, + gMaxBulkSize); + map syncing; + + for (const auto &nh_key : member_keys) + { + ProtNhgMember &nhgm = m_members.at(nh_key); + + if (nhgm.isSynced()) + { + continue; + } + + if (nhgm.getNhId() == SAI_NULL_OBJECT_ID) + { + SWSS_LOG_WARN("Next hop %s not resolved for protection NHG %s", + nh_key.to_string().c_str(), m_key.c_str()); + continue; + } + + vector attrs = createNhgmAttrs(nhgm); + bulker.create_entry(&syncing[nh_key], + static_cast(attrs.size()), + attrs.data()); + } + + bulker.flush(); + + bool success = true; + for (const auto &entry : syncing) + { + if (entry.second == SAI_NULL_OBJECT_ID) + { + SWSS_LOG_ERROR("Failed to create member %s of protection NHG %s", + entry.first.to_string().c_str(), m_key.c_str()); + success = false; + } + else + { + m_members.at(entry.first).sync(entry.second); + } + } + + return success; +} + +vector ProtNhg::createNhgmAttrs(const ProtNhgMember &member) const +{ + SWSS_LOG_ENTER(); + + vector attrs; + sai_attribute_t attr; + + attr.id = SAI_NEXT_HOP_GROUP_MEMBER_ATTR_NEXT_HOP_GROUP_ID; + attr.value.oid = m_id; + attrs.push_back(attr); + + attr.id = SAI_NEXT_HOP_GROUP_MEMBER_ATTR_NEXT_HOP_ID; + attr.value.oid = member.getNhId(); + attrs.push_back(attr); + + attr.id = SAI_NEXT_HOP_GROUP_MEMBER_ATTR_CONFIGURED_ROLE; + attr.value.s32 = (member.getRole() == ProtNhgRole::PRIMARY) + ? SAI_NEXT_HOP_GROUP_MEMBER_CONFIGURED_ROLE_PRIMARY + : SAI_NEXT_HOP_GROUP_MEMBER_CONFIGURED_ROLE_STANDBY; + attrs.push_back(attr); + + if (member.getMonitoredObject() != SAI_NULL_OBJECT_ID) + { + attr.id = SAI_NEXT_HOP_GROUP_MEMBER_ATTR_MONITORED_OBJECT; + attr.value.oid = member.getMonitoredObject(); + attrs.push_back(attr); + } + + return attrs; +} diff --git a/orchagent/protnhg.h b/orchagent/protnhg.h new file mode 100644 index 00000000000..33ca1b46e47 --- /dev/null +++ b/orchagent/protnhg.h @@ -0,0 +1,101 @@ +#pragma once + +#include "nhgbase.h" +#include "nexthopkey.h" +#include "vector" + +using namespace std; + +extern sai_object_id_t gSwitchId; +extern sai_next_hop_group_api_t* sai_next_hop_group_api; + +enum class ProtNhgRole +{ + PRIMARY, + STANDBY +}; + +/* + * ProtNhgMember represents a member of a hardware protection next hop group. + * Each member has a configured role (primary or standby) and an optional + * monitored object (e.g., ICMP echo session OID) for hardware-based failover. + */ +class ProtNhgMember : public NhgMember +{ +public: + ProtNhgMember(const NextHopKey &key, ProtNhgRole role, + sai_object_id_t nh_id_override = SAI_NULL_OBJECT_ID); + + ProtNhgMember(ProtNhgMember &&nhgm); + + ~ProtNhgMember(); + + void sync(sai_object_id_t gm_id) override; + void remove() override; + + inline ProtNhgRole getRole() const { return m_role; } + sai_object_id_t getNhId() const; + + inline sai_object_id_t getMonitoredObject() const { return m_monitored_oid; } + void setMonitoredObject(sai_object_id_t oid) { m_monitored_oid = oid; } + + bool updateMonitoredObject(sai_object_id_t oid); + + /* Query the hardware-observed role (active/inactive) from SAI. */ + bool getObservedRole(sai_next_hop_group_member_observed_role_t &observed_role) const; + + string to_string() const override; + +private: + ProtNhgRole m_role; + sai_object_id_t m_monitored_oid; + sai_object_id_t m_nh_id_override; +}; + +/* + * ProtNhg represents a SAI_NEXT_HOP_GROUP_TYPE_HW_PROTECTION group. + * It has one or more primary next hops and exactly one standby next hop. + * Hardware toggles traffic between the primary set and the standby based on + * the monitored object state. Administrative override is supported via + * SAI_NEXT_HOP_GROUP_ATTR_ADMIN_ROLE. + */ +class ProtNhg : public NhgCommon +{ +public: + ProtNhg(const string &key, + const vector &primary_nhs, + const NextHopKey &standby_nh, + sai_object_id_t standby_nh_id = SAI_NULL_OBJECT_ID); + + ProtNhg(ProtNhg &&nhg); + + ~ProtNhg() { SWSS_LOG_ENTER(); remove(); } + + bool sync() override; + bool remove() override; + + inline bool isTemp() const override { return false; } + inline NextHopGroupKey getNhgKey() const override { return {}; } + + bool setAdminRole(sai_int32_t admin_role); + + bool updateMemberMonitoredObject(const NextHopKey &nh_key, + sai_object_id_t monitored_oid); + + vector getPrimaryMembers() const; + const ProtNhgMember* getStandbyMember() const; + + /* Query a specific member's observed role from SAI. */ + bool getMemberObservedRole(const NextHopKey &nh_key, + sai_next_hop_group_member_observed_role_t &observed_role) const; + + /* Query observed roles for all synced members at once. */ + bool getAllMemberObservedRoles( + map &observed_roles) const; + + string to_string() const override { return m_key; } + +private: + bool syncMembers(const set &member_keys) override; + vector createNhgmAttrs(const ProtNhgMember &member) const override; +}; diff --git a/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am index eecf05a1948..289374c373b 100644 --- a/tests/mock_tests/Makefile.am +++ b/tests/mock_tests/Makefile.am @@ -62,6 +62,7 @@ tests_SOURCES = aclorch_ut.cpp \ orchdaemon_ut.cpp \ intfsorch_ut.cpp \ mux_rollback_ut.cpp \ + protnhg_ut.cpp \ warmrestartassist_ut.cpp \ test_failure_handling.cpp \ switchorch_ut.cpp \ @@ -97,6 +98,7 @@ tests_SOURCES = aclorch_ut.cpp \ $(top_srcdir)/orchagent/fgnhgorch.cpp \ $(top_srcdir)/orchagent/nhgbase.cpp \ $(top_srcdir)/orchagent/nhgorch.cpp \ + $(top_srcdir)/orchagent/protnhg.cpp \ $(top_srcdir)/orchagent/cbf/cbfnhgorch.cpp \ $(top_srcdir)/orchagent/cbf/nhgmaporch.cpp \ $(top_srcdir)/orchagent/neighorch.cpp \ diff --git a/tests/mock_tests/protnhg_ut.cpp b/tests/mock_tests/protnhg_ut.cpp new file mode 100644 index 00000000000..8a2e07024ef --- /dev/null +++ b/tests/mock_tests/protnhg_ut.cpp @@ -0,0 +1,332 @@ +#define private public +#include "directory.h" +#undef private +#define protected public +#include "orch.h" +#undef protected + +#include "ut_helper.h" +#include "mock_orchagent_main.h" +#include "mock_sai_api.h" +#include "mock_orch_test.h" +#include "nhgorch.h" +#include "protnhg.h" + +#include "gtest/gtest.h" + +#include +#include + +using namespace mock_orch_test; +using namespace std; + +using ::testing::_; +using ::testing::Return; + +DEFINE_SAI_GENERIC_APIS_MOCK(next_hop_group, next_hop_group, next_hop_group_member) + +EXTERN_MOCK_FNS + +namespace protnhg_test +{ + static uint64_t nhg_oid_counter = 0x5000000; + static uint64_t nhgm_oid_counter = 0x6000000; + + class ProtNhgTest : public MockOrchTest + { + protected: + void PostSetUp() override + { + INIT_SAI_API_MOCK(next_hop_group); + MockSaiApis(); + + ON_CALL(*mock_sai_next_hop_group_api, + create_next_hop_group(_, _, _, _)) + .WillByDefault( + [](sai_object_id_t *id, sai_object_id_t, + uint32_t, const sai_attribute_t *) { + *id = ++nhg_oid_counter; + return SAI_STATUS_SUCCESS; + }); + + ON_CALL(*mock_sai_next_hop_group_api, + remove_next_hop_group(_)) + .WillByDefault(Return(SAI_STATUS_SUCCESS)); + + ON_CALL(*mock_sai_next_hop_group_api, + set_next_hop_group_attribute(_, _)) + .WillByDefault(Return(SAI_STATUS_SUCCESS)); + + ON_CALL(*mock_sai_next_hop_group_api, + create_next_hop_group_member(_, _, _, _)) + .WillByDefault( + [](sai_object_id_t *id, sai_object_id_t, + uint32_t, const sai_attribute_t *) { + *id = ++nhgm_oid_counter; + return SAI_STATUS_SUCCESS; + }); + + ON_CALL(*mock_sai_next_hop_group_api, + remove_next_hop_group_member(_)) + .WillByDefault(Return(SAI_STATUS_SUCCESS)); + + ON_CALL(*mock_sai_next_hop_group_api, + set_next_hop_group_member_attribute(_, _)) + .WillByDefault(Return(SAI_STATUS_SUCCESS)); + + ON_CALL(*mock_sai_next_hop_group_api, + create_next_hop_group_members(_, _, _, _, _, _, _)) + .WillByDefault( + [](sai_object_id_t, uint32_t count, + const uint32_t *, const sai_attribute_t **, + sai_bulk_op_error_mode_t, + sai_object_id_t *ids, sai_status_t *statuses) { + for (uint32_t i = 0; i < count; i++) + { + ids[i] = ++nhgm_oid_counter; + statuses[i] = SAI_STATUS_SUCCESS; + } + return SAI_STATUS_SUCCESS; + }); + + ON_CALL(*mock_sai_next_hop_group_api, + remove_next_hop_group_members(_, _, _, _)) + .WillByDefault( + [](uint32_t count, const sai_object_id_t *, + sai_bulk_op_error_mode_t, sai_status_t *statuses) { + for (uint32_t i = 0; i < count; i++) + { + statuses[i] = SAI_STATUS_SUCCESS; + } + return SAI_STATUS_SUCCESS; + }); + } + + void PreTearDown() override + { + RestoreSaiApis(); + DEINIT_SAI_API_MOCK(next_hop_group); + } + }; + + TEST_F(ProtNhgTest, CreateAndRemoveProtNhg) + { + string key = "prot_nhg_1"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + sai_object_id_t standby_nh_id = 0x1234; + + vector primaries = {primary_nh}; + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, standby_nh_id)); + EXPECT_TRUE(gNhgOrch->hasProtNhg(key)); + EXPECT_NE(gNhgOrch->getProtNhgId(key), SAI_NULL_OBJECT_ID); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + EXPECT_FALSE(gNhgOrch->hasProtNhg(key)); + EXPECT_EQ(gNhgOrch->getProtNhgId(key), SAI_NULL_OBJECT_ID); + } + + TEST_F(ProtNhgTest, CreateDuplicateProtNhgFails) + { + string key = "prot_nhg_dup"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + sai_object_id_t standby_nh_id = 0x1234; + vector primaries = {primary_nh}; + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, standby_nh_id)); + EXPECT_FALSE(gNhgOrch->createProtNhg(key, primaries, standby_nh, standby_nh_id)); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + } + + TEST_F(ProtNhgTest, CreateProtNhgEmptyPrimariesFails) + { + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + vector primaries; + + EXPECT_FALSE(gNhgOrch->createProtNhg("empty", primaries, standby_nh, 0x1234)); + EXPECT_FALSE(gNhgOrch->hasProtNhg("empty")); + } + + TEST_F(ProtNhgTest, RemoveNonExistentProtNhgFails) + { + EXPECT_FALSE(gNhgOrch->removeProtNhg("does_not_exist")); + } + + TEST_F(ProtNhgTest, RemoveReferencedProtNhgFails) + { + string key = "prot_nhg_ref"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + vector primaries = {primary_nh}; + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + + gNhgOrch->incProtNhgRefCount(key); + EXPECT_FALSE(gNhgOrch->removeProtNhg(key)); + EXPECT_TRUE(gNhgOrch->hasProtNhg(key)); + + gNhgOrch->decProtNhgRefCount(key); + EXPECT_TRUE(gNhgOrch->removeProtNhg(key)); + EXPECT_FALSE(gNhgOrch->hasProtNhg(key)); + } + + TEST_F(ProtNhgTest, GetProtNhgMembers) + { + string key = "prot_nhg_members"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + sai_object_id_t standby_nh_id = 0x1234; + vector primaries = {primary_nh}; + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, standby_nh_id)); + + const ProtNhg &nhg = gNhgOrch->getProtNhg(key); + EXPECT_NE(nhg.getId(), SAI_NULL_OBJECT_ID); + + const ProtNhgMember *standby = nhg.getStandbyMember(); + ASSERT_NE(standby, nullptr); + EXPECT_EQ(standby->getRole(), ProtNhgRole::STANDBY); + + auto primary_out = nhg.getPrimaryMembers(); + ASSERT_EQ(primary_out.size(), 1u); + EXPECT_EQ(primary_out[0]->getRole(), ProtNhgRole::PRIMARY); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + } + + TEST_F(ProtNhgTest, MultiplePrimaryMembers) + { + string key = "prot_nhg_multi"; + NextHopKey primary1(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey primary2(IpAddress("10.0.0.2"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + vector primaries = {primary1, primary2}; + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + + const ProtNhg &nhg = gNhgOrch->getProtNhg(key); + auto primary_out = nhg.getPrimaryMembers(); + EXPECT_EQ(primary_out.size(), 2u); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + } + + TEST_F(ProtNhgTest, SetAdminRole) + { + string key = "prot_nhg_admin"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + vector primaries = {primary_nh}; + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + + EXPECT_CALL(*mock_sai_next_hop_group_api, + set_next_hop_group_attribute(_, _)) + .Times(1) + .WillOnce(Return(SAI_STATUS_SUCCESS)); + + EXPECT_TRUE(gNhgOrch->setProtNhgAdminRole( + key, SAI_NEXT_HOP_GROUP_MEMBER_CONFIGURED_ROLE_PRIMARY)); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + } + + TEST_F(ProtNhgTest, SetAdminRoleNonExistentFails) + { + EXPECT_FALSE(gNhgOrch->setProtNhgAdminRole( + "no_such_key", SAI_NEXT_HOP_GROUP_MEMBER_CONFIGURED_ROLE_PRIMARY)); + } + + TEST_F(ProtNhgTest, SetAdminRoleSaiFailure) + { + string key = "prot_nhg_admin_fail"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + vector primaries = {primary_nh}; + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + + EXPECT_CALL(*mock_sai_next_hop_group_api, + set_next_hop_group_attribute(_, _)) + .Times(1) + .WillOnce(Return(SAI_STATUS_FAILURE)); + + EXPECT_FALSE(gNhgOrch->setProtNhgAdminRole( + key, SAI_NEXT_HOP_GROUP_MEMBER_CONFIGURED_ROLE_PRIMARY)); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + } + + TEST_F(ProtNhgTest, SetMonitoredObjectOnStandbyMember) + { + string key = "prot_nhg_monitor"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + sai_object_id_t standby_nh_id = 0x1234; + sai_object_id_t session_oid = 0xABCD; + vector primaries = {primary_nh}; + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, standby_nh_id)); + + EXPECT_TRUE(gNhgOrch->setProtNhgMonitoredObject(key, standby_nh, session_oid)); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + } + + TEST_F(ProtNhgTest, SetMonitoredObjectNonExistentNhgFails) + { + NextHopKey nh(IpAddress("10.0.0.1"), string("Ethernet0")); + EXPECT_FALSE(gNhgOrch->setProtNhgMonitoredObject("no_such_key", nh, 0xABCD)); + } + + TEST_F(ProtNhgTest, SetMonitoredObjectNonExistentMemberFails) + { + string key = "prot_nhg_monitor_bad_mbr"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + NextHopKey unknown_nh(IpAddress("10.0.0.99"), string("Ethernet0")); + vector primaries = {primary_nh}; + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + EXPECT_FALSE(gNhgOrch->setProtNhgMonitoredObject(key, unknown_nh, 0xABCD)); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + } + + TEST_F(ProtNhgTest, ObservedRoleNonExistentNhgFails) + { + NextHopKey nh(IpAddress("10.0.0.1"), string("Ethernet0")); + sai_next_hop_group_member_observed_role_t role; + EXPECT_FALSE(gNhgOrch->getProtNhgMemberObservedRole("no_such_key", nh, role)); + } + + TEST_F(ProtNhgTest, AllObservedRolesNonExistentNhgFails) + { + map roles; + EXPECT_FALSE(gNhgOrch->getProtNhgAllObservedRoles("no_such_key", roles)); + } + + TEST_F(ProtNhgTest, CreateSaiFailure) + { + EXPECT_CALL(*mock_sai_next_hop_group_api, + create_next_hop_group(_, _, _, _)) + .WillOnce(Return(SAI_STATUS_FAILURE)); + + string key = "prot_nhg_sai_fail"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + vector primaries = {primary_nh}; + + EXPECT_FALSE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + EXPECT_FALSE(gNhgOrch->hasProtNhg(key)); + } + + TEST_F(ProtNhgTest, HasAndGetIdForNonExistentKey) + { + EXPECT_FALSE(gNhgOrch->hasProtNhg("ghost")); + EXPECT_EQ(gNhgOrch->getProtNhgId("ghost"), SAI_NULL_OBJECT_ID); + } +} From 58e09fbc0fbf154d6f589b04d99bc6583ec31c9c Mon Sep 17 00:00:00 2001 From: Manas Kumar Mandal Date: Wed, 1 Apr 2026 18:52:49 -0700 Subject: [PATCH 02/20] remove the protnhg when syncMembers() fail. Signed-off-by: Manas Kumar Mandal --- orchagent/protnhg.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/orchagent/protnhg.cpp b/orchagent/protnhg.cpp index e133acf0139..bd29abc8335 100644 --- a/orchagent/protnhg.cpp +++ b/orchagent/protnhg.cpp @@ -212,6 +212,7 @@ bool ProtNhg::sync() if (!syncMembers(member_keys)) { SWSS_LOG_WARN("Failed to sync members of protection NHG %s", m_key.c_str()); + remove(); return false; } From 41ef342cb54694535a6277104bc092d39780bff8 Mon Sep 17 00:00:00 2001 From: Manas Kumar Mandal Date: Sat, 4 Apr 2026 10:44:15 -0700 Subject: [PATCH 03/20] Added more test for coverage Signed-off-by: Manas Kumar Mandal --- tests/mock_tests/protnhg_ut.cpp | 320 ++++++++++++++++++++++++++++++++ 1 file changed, 320 insertions(+) diff --git a/tests/mock_tests/protnhg_ut.cpp b/tests/mock_tests/protnhg_ut.cpp index 8a2e07024ef..2d945fd087c 100644 --- a/tests/mock_tests/protnhg_ut.cpp +++ b/tests/mock_tests/protnhg_ut.cpp @@ -329,4 +329,324 @@ namespace protnhg_test EXPECT_FALSE(gNhgOrch->hasProtNhg("ghost")); EXPECT_EQ(gNhgOrch->getProtNhgId("ghost"), SAI_NULL_OBJECT_ID); } + + + TEST_F(ProtNhgTest, SyncAlreadySynced) + { + string key = "prot_nhg_double_sync"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + vector primaries = {primary_nh}; + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + + auto &nhg = const_cast(gNhgOrch->getProtNhg(key)); + EXPECT_TRUE(nhg.sync()); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + } + + TEST_F(ProtNhgTest, SyncMembersFailure) + { + EXPECT_CALL(*mock_sai_next_hop_group_api, + create_next_hop_group_members(_, _, _, _, _, _, _)) + .WillOnce( + [](sai_object_id_t, uint32_t count, + const uint32_t *, const sai_attribute_t **, + sai_bulk_op_error_mode_t, + sai_object_id_t *ids, sai_status_t *statuses) { + for (uint32_t i = 0; i < count; i++) + { + ids[i] = SAI_NULL_OBJECT_ID; + statuses[i] = SAI_STATUS_FAILURE; + } + return SAI_STATUS_FAILURE; + }); + + string key = "prot_nhg_sync_mbr_fail"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + vector primaries = {primary_nh}; + + EXPECT_FALSE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + EXPECT_FALSE(gNhgOrch->hasProtNhg(key)); + } + + TEST_F(ProtNhgTest, SetAdminRoleUnsyncedNhg) + { + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + vector primaries = {primary_nh}; + + ProtNhg nhg("unsynced_nhg", primaries, standby_nh, 0x1234); + EXPECT_FALSE(nhg.setAdminRole( + SAI_NEXT_HOP_GROUP_MEMBER_CONFIGURED_ROLE_PRIMARY)); + } + + TEST_F(ProtNhgTest, UpdateMonitoredObjectUnsynced) + { + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + vector primaries = {primary_nh}; + + ProtNhg nhg("unsynced_mon", primaries, standby_nh, 0x1234); + EXPECT_TRUE(nhg.updateMemberMonitoredObject(standby_nh, 0xABCD)); + } + + TEST_F(ProtNhgTest, UpdateMonitoredObjectSaiFailure) + { + string key = "prot_nhg_mon_sai_fail"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + vector primaries = {primary_nh}; + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + + EXPECT_CALL(*mock_sai_next_hop_group_api, + set_next_hop_group_member_attribute(_, _)) + .WillOnce(Return(SAI_STATUS_FAILURE)); + + EXPECT_FALSE(gNhgOrch->setProtNhgMonitoredObject(key, standby_nh, 0xABCD)); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + } + + TEST_F(ProtNhgTest, ObservedRoleUnsyncedMember) + { + string key = "prot_nhg_obs_unsynced"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + vector primaries = {primary_nh}; + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + + sai_next_hop_group_member_observed_role_t role; + EXPECT_FALSE(gNhgOrch->getProtNhgMemberObservedRole( + key, primary_nh, role)); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + } + + TEST_F(ProtNhgTest, ObservedRoleSuccess) + { + string key = "prot_nhg_obs_ok"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + vector primaries = {primary_nh}; + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + + auto old_get_fn = + ut_sai_next_hop_group_api.get_next_hop_group_member_attribute; + ut_sai_next_hop_group_api.get_next_hop_group_member_attribute = + [](sai_object_id_t, uint32_t, sai_attribute_t *attr_list) + -> sai_status_t { + attr_list[0].value.s32 = 0; + return SAI_STATUS_SUCCESS; + }; + + sai_next_hop_group_member_observed_role_t role; + EXPECT_TRUE(gNhgOrch->getProtNhgMemberObservedRole( + key, standby_nh, role)); + EXPECT_EQ(static_cast(role), 0); + + ut_sai_next_hop_group_api.get_next_hop_group_member_attribute = + old_get_fn; + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + } + + TEST_F(ProtNhgTest, ObservedRoleSaiFailure) + { + string key = "prot_nhg_obs_fail"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + vector primaries = {primary_nh}; + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + + auto old_get_fn = + ut_sai_next_hop_group_api.get_next_hop_group_member_attribute; + ut_sai_next_hop_group_api.get_next_hop_group_member_attribute = + [](sai_object_id_t, uint32_t, sai_attribute_t *) + -> sai_status_t { + return SAI_STATUS_FAILURE; + }; + + sai_next_hop_group_member_observed_role_t role; + EXPECT_FALSE(gNhgOrch->getProtNhgMemberObservedRole( + key, standby_nh, role)); + + ut_sai_next_hop_group_api.get_next_hop_group_member_attribute = + old_get_fn; + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + } + + TEST_F(ProtNhgTest, GetMemberObservedRoleNotFound) + { + string key = "prot_nhg_obs_notfound"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + NextHopKey unknown_nh(IpAddress("10.0.0.99"), string("Ethernet0")); + vector primaries = {primary_nh}; + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + + sai_next_hop_group_member_observed_role_t role; + EXPECT_FALSE(gNhgOrch->getProtNhgMemberObservedRole( + key, unknown_nh, role)); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + } + + TEST_F(ProtNhgTest, GetAllMemberObservedRolesSuccess) + { + string key = "prot_nhg_all_obs"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + vector primaries = {primary_nh}; + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + + auto old_get_fn = + ut_sai_next_hop_group_api.get_next_hop_group_member_attribute; + ut_sai_next_hop_group_api.get_next_hop_group_member_attribute = + [](sai_object_id_t, uint32_t, sai_attribute_t *attr_list) + -> sai_status_t { + attr_list[0].value.s32 = 0; + return SAI_STATUS_SUCCESS; + }; + + map roles; + EXPECT_TRUE(gNhgOrch->getProtNhgAllObservedRoles(key, roles)); + EXPECT_EQ(roles.size(), 1u); + + ut_sai_next_hop_group_api.get_next_hop_group_member_attribute = + old_get_fn; + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + } + + TEST_F(ProtNhgTest, GetAllMemberObservedRolesSaiFailure) + { + string key = "prot_nhg_all_obs_fail"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + vector primaries = {primary_nh}; + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + + auto old_get_fn = + ut_sai_next_hop_group_api.get_next_hop_group_member_attribute; + ut_sai_next_hop_group_api.get_next_hop_group_member_attribute = + [](sai_object_id_t, uint32_t, sai_attribute_t *) + -> sai_status_t { + return SAI_STATUS_FAILURE; + }; + + map roles; + EXPECT_FALSE(gNhgOrch->getProtNhgAllObservedRoles(key, roles)); + EXPECT_TRUE(roles.empty()); + + ut_sai_next_hop_group_api.get_next_hop_group_member_attribute = + old_get_fn; + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + } + + TEST_F(ProtNhgTest, RemoveProtNhgSaiFailure) + { + string key = "prot_nhg_remove_fail"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + vector primaries = {primary_nh}; + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + + EXPECT_CALL(*mock_sai_next_hop_group_api, + remove_next_hop_group(_)) + .WillOnce(Return(SAI_STATUS_FAILURE)) + .WillRepeatedly(Return(SAI_STATUS_SUCCESS)); + + EXPECT_FALSE(gNhgOrch->removeProtNhg(key)); + EXPECT_TRUE(gNhgOrch->hasProtNhg(key)); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + } + + TEST_F(ProtNhgTest, ProtNhgInlineMethods) + { + string key = "prot_nhg_inline"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + vector primaries = {primary_nh}; + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + + const ProtNhg &nhg = gNhgOrch->getProtNhg(key); + EXPECT_FALSE(nhg.isTemp()); + EXPECT_EQ(nhg.getNhgKey(), NextHopGroupKey()); + EXPECT_EQ(nhg.to_string(), key); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + } + + TEST_F(ProtNhgTest, MemberToString) + { + string key = "prot_nhg_mbr_str"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + vector primaries = {primary_nh}; + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + + const ProtNhg &nhg = gNhgOrch->getProtNhg(key); + const ProtNhgMember *standby = nhg.getStandbyMember(); + ASSERT_NE(standby, nullptr); + string sstr = standby->to_string(); + EXPECT_FALSE(sstr.empty()); + EXPECT_NE(sstr.find("standby"), string::npos); + + auto primary_out = nhg.getPrimaryMembers(); + ASSERT_GE(primary_out.size(), 1u); + string pstr = primary_out[0]->to_string(); + EXPECT_NE(pstr.find("primary"), string::npos); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + } + + TEST_F(ProtNhgTest, MemberRemoveUnsynced) + { + NextHopKey nh(IpAddress("10.0.0.1"), string("Ethernet0")); + ProtNhgMember member(nh, ProtNhgRole::PRIMARY); + + EXPECT_FALSE(member.isSynced()); + member.remove(); + EXPECT_FALSE(member.isSynced()); + } + + TEST_F(ProtNhgTest, MoveConstructor) + { + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + vector primaries = {primary_nh}; + + ProtNhg nhg1("nhg_move", primaries, standby_nh, 0x1234); + ProtNhg nhg2(std::move(nhg1)); + + EXPECT_NE(nhg2.getStandbyMember(), nullptr); + EXPECT_EQ(nhg2.to_string(), "nhg_move"); + } + + TEST_F(ProtNhgTest, SyncWithMonitoredObject) + { + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + sai_object_id_t standby_nh_id = 0x1234; + sai_object_id_t session_oid = 0xABCD; + vector primaries = {primary_nh}; + + ProtNhg nhg("nhg_mon_sync", primaries, standby_nh, standby_nh_id); + + EXPECT_TRUE(nhg.updateMemberMonitoredObject(standby_nh, session_oid)); + EXPECT_TRUE(nhg.sync()); + EXPECT_TRUE(nhg.isSynced()); + } } From 4b8d7340d44c60d700bf6da05caac31d8bcec8e6 Mon Sep 17 00:00:00 2001 From: Manas Kumar Mandal Date: Tue, 7 Apr 2026 19:14:37 -0700 Subject: [PATCH 04/20] Support NHGs as primary and secondary members Signed-off-by: Manas Kumar Mandal --- orchagent/nhgorch.cpp | 85 ++++++++++++++++++++++++++ orchagent/nhgorch.h | 7 +++ orchagent/protnhg.cpp | 18 ++++++ orchagent/protnhg.h | 10 ++++ tests/mock_tests/protnhg_ut.cpp | 103 +++++++++++++++++++++++++++++++- 5 files changed, 222 insertions(+), 1 deletion(-) diff --git a/orchagent/nhgorch.cpp b/orchagent/nhgorch.cpp index be820fd04fe..6f32715488e 100644 --- a/orchagent/nhgorch.cpp +++ b/orchagent/nhgorch.cpp @@ -1269,6 +1269,91 @@ bool NhgOrch::createProtNhg(const string &key, return true; } +bool NhgOrch::createProtNhg(const string &key, + const NextHopGroupKey &primary_nhg_key, + const NextHopGroupKey &standby_nhg_key) +{ + SWSS_LOG_ENTER(); + + if (primary_nhg_key.getSize() == 0) + { + SWSS_LOG_ERROR("Protection NHG %s primary group key is empty", key.c_str()); + return false; + } + + if (standby_nhg_key.getSize() == 0) + { + SWSS_LOG_ERROR("Protection NHG %s standby group key is empty", key.c_str()); + return false; + } + + string primary_key_str = primary_nhg_key.to_string(); + string standby_key_str = standby_nhg_key.to_string(); + + if (!hasNhg(primary_key_str)) + { + SWSS_LOG_ERROR("Protection NHG %s: primary NHG %s does not exist", + key.c_str(), primary_key_str.c_str()); + return false; + } + + if (!hasNhg(standby_key_str)) + { + SWSS_LOG_ERROR("Protection NHG %s: standby NHG %s does not exist", + key.c_str(), standby_key_str.c_str()); + return false; + } + + sai_object_id_t primary_nhg_id = getNhg(primary_key_str).getId(); + sai_object_id_t standby_nhg_id = getNhg(standby_key_str).getId(); + + if (primary_nhg_id == SAI_NULL_OBJECT_ID) + { + SWSS_LOG_ERROR("Protection NHG %s: primary NHG %s is not synced", + key.c_str(), primary_key_str.c_str()); + return false; + } + + if (standby_nhg_id == SAI_NULL_OBJECT_ID) + { + SWSS_LOG_ERROR("Protection NHG %s: standby NHG %s is not synced", + key.c_str(), standby_key_str.c_str()); + return false; + } + + if (m_protNhgs.find(key) != m_protNhgs.end()) + { + SWSS_LOG_ERROR("Protection NHG %s already exists", key.c_str()); + return false; + } + + if (gRouteOrch->getNhgCount() + NhgBase::getSyncedCount() >= + gRouteOrch->getMaxNhgCount()) + { + SWSS_LOG_ERROR("NHG capacity exhausted, cannot create protection NHG %s", + key.c_str()); + return false; + } + + auto nhg = make_unique(key, primary_nhg_key, primary_nhg_id, + standby_nhg_key, standby_nhg_id); + + if (!nhg->sync()) + { + SWSS_LOG_ERROR("Failed to sync protection NHG %s", key.c_str()); + return false; + } + + m_protNhgs.emplace(key, NhgEntry(move(nhg))); + + SWSS_LOG_NOTICE("Created protection NHG %s (primary NHG: %s, standby NHG: %s)", + key.c_str(), + primary_key_str.c_str(), + standby_key_str.c_str()); + + return true; +} + bool NhgOrch::removeProtNhg(const string &key) { SWSS_LOG_ENTER(); diff --git a/orchagent/nhgorch.h b/orchagent/nhgorch.h index 69e86aa7d6f..0e5ef99c142 100644 --- a/orchagent/nhgorch.h +++ b/orchagent/nhgorch.h @@ -148,6 +148,13 @@ class NhgOrch : public NhgOrchCommon const NextHopKey &standby_nh, sai_object_id_t standby_nh_id = SAI_NULL_OBJECT_ID); + /* Create a protection NHG where each role is an existing ECMP NHG. + * The group keys are resolved to their SAI OIDs via hasNhg/getNhg. + */ + bool createProtNhg(const string &key, + const NextHopGroupKey &primary_nhg_key, + const NextHopGroupKey &standby_nhg_key); + /* Remove a protection NHG by key. */ bool removeProtNhg(const string &key); diff --git a/orchagent/protnhg.cpp b/orchagent/protnhg.cpp index bd29abc8335..0999e082327 100644 --- a/orchagent/protnhg.cpp +++ b/orchagent/protnhg.cpp @@ -158,6 +158,24 @@ ProtNhg::ProtNhg(const string &key, ProtNhgMember(standby_nh, ProtNhgRole::STANDBY, standby_nh_id)); } +ProtNhg::ProtNhg(const string &key, + const NextHopGroupKey &primary_nhg_key, + sai_object_id_t primary_nhg_id, + const NextHopGroupKey &standby_nhg_key, + sai_object_id_t standby_nhg_id) : + NhgCommon(key) +{ + SWSS_LOG_ENTER(); + + const NextHopKey &primary_rep = *primary_nhg_key.getNextHops().begin(); + const NextHopKey &standby_rep = *standby_nhg_key.getNextHops().begin(); + + m_members.emplace(primary_rep, + ProtNhgMember(primary_rep, ProtNhgRole::PRIMARY, primary_nhg_id)); + m_members.emplace(standby_rep, + ProtNhgMember(standby_rep, ProtNhgRole::STANDBY, standby_nhg_id)); +} + ProtNhg::ProtNhg(ProtNhg &&nhg) : NhgCommon(move(nhg)) { diff --git a/orchagent/protnhg.h b/orchagent/protnhg.h index 33ca1b46e47..7901ca621e0 100644 --- a/orchagent/protnhg.h +++ b/orchagent/protnhg.h @@ -67,6 +67,16 @@ class ProtNhg : public NhgCommon const NextHopKey &standby_nh, sai_object_id_t standby_nh_id = SAI_NULL_OBJECT_ID); + /* Construct from NextHopGroupKey pairs with pre-resolved NHG OIDs. + * Each group becomes a single protection member whose SAI next-hop ID + * points to the resolved ECMP NHG OID (recursive/nested NHG). + */ + ProtNhg(const string &key, + const NextHopGroupKey &primary_nhg_key, + sai_object_id_t primary_nhg_id, + const NextHopGroupKey &standby_nhg_key, + sai_object_id_t standby_nhg_id); + ProtNhg(ProtNhg &&nhg); ~ProtNhg() { SWSS_LOG_ENTER(); remove(); } diff --git a/tests/mock_tests/protnhg_ut.cpp b/tests/mock_tests/protnhg_ut.cpp index 2d945fd087c..6581f94f474 100644 --- a/tests/mock_tests/protnhg_ut.cpp +++ b/tests/mock_tests/protnhg_ut.cpp @@ -1,11 +1,16 @@ #define private public -#include "directory.h" #undef private +#include "directory.h" #define protected public #include "orch.h" #undef protected #include "ut_helper.h" + +#define protected public +#include "nhgbase.h" +#undef protected + #include "mock_orchagent_main.h" #include "mock_sai_api.h" #include "mock_orch_test.h" @@ -649,4 +654,100 @@ namespace protnhg_test EXPECT_TRUE(nhg.sync()); EXPECT_TRUE(nhg.isSynced()); } + + + static uint64_t ecmp_nhg_oid_counter = 0x7000000; + + /* Helper: register a fake synced ECMP NHG in gNhgOrch. + * Directly assigns a SAI OID instead of calling sync(), which would + * try to resolve individual NHs through NeighOrch. + */ + static void addEcmpNhg(const NextHopGroupKey &nhg_key) + { + string key_str = nhg_key.to_string(); + auto nhg = make_unique(nhg_key, false); + nhg->m_id = ++ecmp_nhg_oid_counter; + gNhgOrch->m_syncdNextHopGroups.emplace( + key_str, NhgEntry(move(nhg))); + } + + static void removeEcmpNhg(const string &key_str) + { + auto it = gNhgOrch->m_syncdNextHopGroups.find(key_str); + if (it != gNhgOrch->m_syncdNextHopGroups.end()) + { + it->second.nhg->m_id = SAI_NULL_OBJECT_ID; + gNhgOrch->m_syncdNextHopGroups.erase(it); + } + } + + TEST_F(ProtNhgTest, CreateProtNhgWithNhgKeys) + { + NextHopGroupKey primary_nhg_key("10.0.0.1@Ethernet0,10.0.0.2@Ethernet0"); + NextHopGroupKey standby_nhg_key("10.0.0.100@Ethernet4"); + + addEcmpNhg(primary_nhg_key); + addEcmpNhg(standby_nhg_key); + + string key = "prot_nhg_keys"; + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nhg_key, standby_nhg_key)); + EXPECT_TRUE(gNhgOrch->hasProtNhg(key)); + EXPECT_NE(gNhgOrch->getProtNhgId(key), SAI_NULL_OBJECT_ID); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + EXPECT_FALSE(gNhgOrch->hasProtNhg(key)); + + removeEcmpNhg(primary_nhg_key.to_string()); + removeEcmpNhg(standby_nhg_key.to_string()); + } + + TEST_F(ProtNhgTest, CreateProtNhgWithNhgKeysPrimaryNotFound) + { + NextHopGroupKey primary_nhg_key("10.0.0.1@Ethernet0"); + NextHopGroupKey standby_nhg_key("10.0.0.100@Ethernet4"); + + addEcmpNhg(standby_nhg_key); + + EXPECT_FALSE(gNhgOrch->createProtNhg("prot_no_primary", + primary_nhg_key, + standby_nhg_key)); + EXPECT_FALSE(gNhgOrch->hasProtNhg("prot_no_primary")); + + removeEcmpNhg(standby_nhg_key.to_string()); + } + + TEST_F(ProtNhgTest, CreateProtNhgWithNhgKeysStandbyNotFound) + { + NextHopGroupKey primary_nhg_key("10.0.0.1@Ethernet0"); + NextHopGroupKey standby_nhg_key("10.0.0.100@Ethernet4"); + + addEcmpNhg(primary_nhg_key); + + EXPECT_FALSE(gNhgOrch->createProtNhg("prot_no_standby", + primary_nhg_key, + standby_nhg_key)); + EXPECT_FALSE(gNhgOrch->hasProtNhg("prot_no_standby")); + + removeEcmpNhg(primary_nhg_key.to_string()); + } + + TEST_F(ProtNhgTest, CreateProtNhgWithNhgKeysEmptyPrimary) + { + NextHopGroupKey empty_primary; + NextHopGroupKey standby_nhg_key("10.0.0.100@Ethernet4"); + + EXPECT_FALSE(gNhgOrch->createProtNhg("prot_empty_primary", + empty_primary, + standby_nhg_key)); + } + + TEST_F(ProtNhgTest, CreateProtNhgWithNhgKeysEmptyStandby) + { + NextHopGroupKey primary_nhg_key("10.0.0.1@Ethernet0"); + NextHopGroupKey empty_standby; + + EXPECT_FALSE(gNhgOrch->createProtNhg("prot_empty_standby", + primary_nhg_key, + empty_standby)); + } } From 92c815adaa8feb959c810482a103372c58528254 Mon Sep 17 00:00:00 2001 From: Manas Kumar Mandal Date: Fri, 10 Apr 2026 20:27:10 -0700 Subject: [PATCH 05/20] parameter to disguish with regular protection nhg Signed-off-by: Manas Kumar Mandal --- orchagent/nhgorch.cpp | 26 ++++++++-- orchagent/nhgorch.h | 11 ++-- orchagent/protnhg.cpp | 66 +++++++++++++++++++++--- orchagent/protnhg.h | 11 +++- tests/mock_tests/protnhg_ut.cpp | 89 +++++++++++++++++++++++++++++++++ 5 files changed, 188 insertions(+), 15 deletions(-) diff --git a/orchagent/nhgorch.cpp b/orchagent/nhgorch.cpp index 6f32715488e..ead7bfddcd4 100644 --- a/orchagent/nhgorch.cpp +++ b/orchagent/nhgorch.cpp @@ -1217,7 +1217,8 @@ bool NhgOrch::isHwProtectionSupported() bool NhgOrch::createProtNhg(const string &key, const vector &primary_nhs, const NextHopKey &standby_nh, - sai_object_id_t standby_nh_id) + sai_object_id_t standby_nh_id, + bool hw_protection) { SWSS_LOG_ENTER(); @@ -1241,7 +1242,8 @@ bool NhgOrch::createProtNhg(const string &key, return false; } - auto nhg = make_unique(key, primary_nhs, standby_nh, standby_nh_id); + auto nhg = make_unique(key, primary_nhs, standby_nh, standby_nh_id, + hw_protection); if (!nhg->sync()) { @@ -1271,7 +1273,8 @@ bool NhgOrch::createProtNhg(const string &key, bool NhgOrch::createProtNhg(const string &key, const NextHopGroupKey &primary_nhg_key, - const NextHopGroupKey &standby_nhg_key) + const NextHopGroupKey &standby_nhg_key, + bool hw_protection) { SWSS_LOG_ENTER(); @@ -1336,7 +1339,8 @@ bool NhgOrch::createProtNhg(const string &key, } auto nhg = make_unique(key, primary_nhg_key, primary_nhg_id, - standby_nhg_key, standby_nhg_id); + standby_nhg_key, standby_nhg_id, + hw_protection); if (!nhg->sync()) { @@ -1424,6 +1428,20 @@ bool NhgOrch::setProtNhgAdminRole(const string &key, sai_int32_t admin_role) return it->second.nhg->setAdminRole(admin_role); } +bool NhgOrch::setProtNhgSwitchover(const string &key, bool enable) +{ + SWSS_LOG_ENTER(); + + auto it = m_protNhgs.find(key); + if (it == m_protNhgs.end()) + { + SWSS_LOG_ERROR("Protection NHG %s does not exist", key.c_str()); + return false; + } + + return it->second.nhg->setSwitchover(enable); +} + bool NhgOrch::setProtNhgMonitoredObject(const string &key, const NextHopKey &nh_key, sai_object_id_t monitored_oid) diff --git a/orchagent/nhgorch.h b/orchagent/nhgorch.h index 0e5ef99c142..fe3396c2b42 100644 --- a/orchagent/nhgorch.h +++ b/orchagent/nhgorch.h @@ -146,14 +146,16 @@ class NhgOrch : public NhgOrchCommon bool createProtNhg(const string &key, const vector &primary_nhs, const NextHopKey &standby_nh, - sai_object_id_t standby_nh_id = SAI_NULL_OBJECT_ID); + sai_object_id_t standby_nh_id = SAI_NULL_OBJECT_ID, + bool hw_protection = true); /* Create a protection NHG where each role is an existing ECMP NHG. * The group keys are resolved to their SAI OIDs via hasNhg/getNhg. */ bool createProtNhg(const string &key, const NextHopGroupKey &primary_nhg_key, - const NextHopGroupKey &standby_nhg_key); + const NextHopGroupKey &standby_nhg_key, + bool hw_protection = true); /* Remove a protection NHG by key. */ bool removeProtNhg(const string &key); @@ -167,9 +169,12 @@ class NhgOrch : public NhgOrchCommon /* Get the SAI object ID of a protection NHG. */ sai_object_id_t getProtNhgId(const string &key) const; - /* Toggle admin role (auto / force primary / force standby). */ + /* Toggle admin role -- HW_PROTECTION groups only. */ bool setProtNhgAdminRole(const string &key, sai_int32_t admin_role); + /* Trigger switchover from primary to backup -- PROTECTION groups only. */ + bool setProtNhgSwitchover(const string &key, bool enable); + /* Update the monitored object on a protection NHG member. */ bool setProtNhgMonitoredObject(const string &key, const NextHopKey &nh_key, diff --git a/orchagent/protnhg.cpp b/orchagent/protnhg.cpp index 0999e082327..9403fc60eef 100644 --- a/orchagent/protnhg.cpp +++ b/orchagent/protnhg.cpp @@ -145,8 +145,10 @@ string ProtNhgMember::to_string() const ProtNhg::ProtNhg(const string &key, const vector &primary_nhs, const NextHopKey &standby_nh, - sai_object_id_t standby_nh_id) : - NhgCommon(key) + sai_object_id_t standby_nh_id, + bool hw_protection) : + NhgCommon(key), + m_hw_protection(hw_protection) { SWSS_LOG_ENTER(); @@ -162,8 +164,10 @@ ProtNhg::ProtNhg(const string &key, const NextHopGroupKey &primary_nhg_key, sai_object_id_t primary_nhg_id, const NextHopGroupKey &standby_nhg_key, - sai_object_id_t standby_nhg_id) : - NhgCommon(key) + sai_object_id_t standby_nhg_id, + bool hw_protection) : + NhgCommon(key), + m_hw_protection(hw_protection) { SWSS_LOG_ENTER(); @@ -177,7 +181,8 @@ ProtNhg::ProtNhg(const string &key, } ProtNhg::ProtNhg(ProtNhg &&nhg) : - NhgCommon(move(nhg)) + NhgCommon(move(nhg)), + m_hw_protection(nhg.m_hw_protection) { SWSS_LOG_ENTER(); } @@ -202,7 +207,9 @@ bool ProtNhg::sync() vector nhg_attrs; nhg_attr.id = SAI_NEXT_HOP_GROUP_ATTR_TYPE; - nhg_attr.value.s32 = SAI_NEXT_HOP_GROUP_TYPE_HW_PROTECTION; + nhg_attr.value.s32 = m_hw_protection + ? SAI_NEXT_HOP_GROUP_TYPE_HW_PROTECTION + : SAI_NEXT_HOP_GROUP_TYPE_PROTECTION; nhg_attrs.push_back(nhg_attr); sai_status_t status = sai_next_hop_group_api->create_next_hop_group( @@ -253,6 +260,14 @@ bool ProtNhg::setAdminRole(sai_int32_t admin_role) { SWSS_LOG_ENTER(); + if (!m_hw_protection) + { + SWSS_LOG_ERROR("Admin role is only supported on HW_PROTECTION NHGs, " + "use setSwitchover() for PROTECTION NHG %s", + m_key.c_str()); + return false; + } + if (!isSynced()) { SWSS_LOG_ERROR("Cannot set admin role on unsynced protection NHG %s", @@ -280,6 +295,45 @@ bool ProtNhg::setAdminRole(sai_int32_t admin_role) return true; } +bool ProtNhg::setSwitchover(bool enable) +{ + SWSS_LOG_ENTER(); + + if (m_hw_protection) + { + SWSS_LOG_ERROR("Switchover is only supported on PROTECTION NHGs, " + "use setAdminRole() for HW_PROTECTION NHG %s", + m_key.c_str()); + return false; + } + + if (!isSynced()) + { + SWSS_LOG_ERROR("Cannot set switchover on unsynced protection NHG %s", + m_key.c_str()); + return false; + } + + sai_attribute_t attr; + attr.id = SAI_NEXT_HOP_GROUP_ATTR_SET_SWITCHOVER; + attr.value.booldata = enable; + + sai_status_t status = + sai_next_hop_group_api->set_next_hop_group_attribute(m_id, &attr); + + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to set switchover %s on protection NHG %s, rv: %d", + enable ? "true" : "false", m_key.c_str(), status); + return false; + } + + SWSS_LOG_NOTICE("Set switchover %s on protection NHG %s", + enable ? "true" : "false", m_key.c_str()); + + return true; +} + bool ProtNhg::updateMemberMonitoredObject(const NextHopKey &nh_key, sai_object_id_t monitored_oid) { diff --git a/orchagent/protnhg.h b/orchagent/protnhg.h index 7901ca621e0..b13dd1864ba 100644 --- a/orchagent/protnhg.h +++ b/orchagent/protnhg.h @@ -65,7 +65,8 @@ class ProtNhg : public NhgCommon ProtNhg(const string &key, const vector &primary_nhs, const NextHopKey &standby_nh, - sai_object_id_t standby_nh_id = SAI_NULL_OBJECT_ID); + sai_object_id_t standby_nh_id = SAI_NULL_OBJECT_ID, + bool hw_protection = true); /* Construct from NextHopGroupKey pairs with pre-resolved NHG OIDs. * Each group becomes a single protection member whose SAI next-hop ID @@ -75,7 +76,8 @@ class ProtNhg : public NhgCommon const NextHopGroupKey &primary_nhg_key, sai_object_id_t primary_nhg_id, const NextHopGroupKey &standby_nhg_key, - sai_object_id_t standby_nhg_id); + sai_object_id_t standby_nhg_id, + bool hw_protection = true); ProtNhg(ProtNhg &&nhg); @@ -89,6 +91,9 @@ class ProtNhg : public NhgCommon bool setAdminRole(sai_int32_t admin_role); + /* Trigger switchover from primary to backup (PROTECTION type only). */ + bool setSwitchover(bool enable); + bool updateMemberMonitoredObject(const NextHopKey &nh_key, sai_object_id_t monitored_oid); @@ -106,6 +111,8 @@ class ProtNhg : public NhgCommon string to_string() const override { return m_key; } private: + bool m_hw_protection; + bool syncMembers(const set &member_keys) override; vector createNhgmAttrs(const ProtNhgMember &member) const override; }; diff --git a/tests/mock_tests/protnhg_ut.cpp b/tests/mock_tests/protnhg_ut.cpp index 6581f94f474..649d8d3d82a 100644 --- a/tests/mock_tests/protnhg_ut.cpp +++ b/tests/mock_tests/protnhg_ut.cpp @@ -750,4 +750,93 @@ namespace protnhg_test primary_nhg_key, empty_standby)); } + + TEST_F(ProtNhgTest, CreateNonHwProtectionNhg) + { + string key = "prot_nhg_sw"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + sai_object_id_t standby_nh_id = 0x1234; + vector primaries = {primary_nh}; + + EXPECT_CALL(*mock_sai_next_hop_group_api, + create_next_hop_group(_, _, _, _)) + .WillOnce( + [](sai_object_id_t *id, sai_object_id_t, + uint32_t attr_count, const sai_attribute_t *attrs) { + for (uint32_t i = 0; i < attr_count; i++) + { + if (attrs[i].id == SAI_NEXT_HOP_GROUP_ATTR_TYPE) + { + EXPECT_EQ(attrs[i].value.s32, + SAI_NEXT_HOP_GROUP_TYPE_PROTECTION); + } + } + *id = 0xAA00; + return SAI_STATUS_SUCCESS; + }); + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, + standby_nh_id, + false)); + EXPECT_TRUE(gNhgOrch->hasProtNhg(key)); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + } + + TEST_F(ProtNhgTest, SetAdminRoleOnProtectionNhgFails) + { + string key = "prot_nhg_admin_prot"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + vector primaries = {primary_nh}; + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, + 0x1234, false)); + + EXPECT_FALSE(gNhgOrch->setProtNhgAdminRole( + key, SAI_NEXT_HOP_GROUP_MEMBER_CONFIGURED_ROLE_PRIMARY)); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + } + + TEST_F(ProtNhgTest, SetSwitchoverSuccess) + { + string key = "prot_nhg_switchover"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + vector primaries = {primary_nh}; + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, + 0x1234, false)); + + EXPECT_CALL(*mock_sai_next_hop_group_api, + set_next_hop_group_attribute(_, _)) + .Times(1) + .WillOnce(Return(SAI_STATUS_SUCCESS)); + + EXPECT_TRUE(gNhgOrch->setProtNhgSwitchover(key, true)); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + } + + TEST_F(ProtNhgTest, SetSwitchoverOnHwProtectionFails) + { + string key = "prot_nhg_switchover_hw"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + vector primaries = {primary_nh}; + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, + 0x1234, true)); + + EXPECT_FALSE(gNhgOrch->setProtNhgSwitchover(key, true)); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + } + + TEST_F(ProtNhgTest, SetSwitchoverNonExistentNhgFails) + { + EXPECT_FALSE(gNhgOrch->setProtNhgSwitchover("no_such_key", true)); + } } From 6b73e8c9cc44ce3dd009f6a4b8308ca371e3be0a Mon Sep 17 00:00:00 2001 From: Manas Kumar Mandal Date: Thu, 16 Apr 2026 16:20:54 -0700 Subject: [PATCH 06/20] Added key based ProtNhg creation Signed-off-by: Manas Kumar Mandal --- orchagent/muxorch.cpp | 11 + orchagent/neighorch.cpp | 57 ++++ orchagent/neighorch.h | 3 + orchagent/nexthopkey.cpp | 1 + orchagent/nexthopkey.h | 38 ++- orchagent/nhgorch.cpp | 68 +++-- orchagent/nhgorch.h | 24 +- orchagent/protnhg.cpp | 47 ++-- orchagent/protnhg.h | 15 +- orchagent/tunneldecaporch.cpp | 14 + tests/mock_tests/protnhg_ut.cpp | 464 +++++++++++++++++++++++++++++--- 11 files changed, 647 insertions(+), 95 deletions(-) diff --git a/orchagent/muxorch.cpp b/orchagent/muxorch.cpp index baf173c0f81..6aa249c89f3 100644 --- a/orchagent/muxorch.cpp +++ b/orchagent/muxorch.cpp @@ -1521,6 +1521,11 @@ sai_object_id_t MuxOrch::createNextHopTunnel(std::string tunnelKey, swss::IpAddr if (SAI_NULL_OBJECT_ID != nh) { mux_tunnel_nh_[ipAddr] = { nh, 1 }; + NextHopKey nhKey(ipAddr, tunnelKey, true /*tunnel_nh*/, 0 /*tag*/); + if (gNeighOrch) + { + gNeighOrch->addIpinipTunnelNextHop(nhKey, nh); + } } return nh; @@ -1545,6 +1550,12 @@ bool MuxOrch::removeNextHopTunnel(std::string tunnelKey, swss::IpAddress& ipAddr tunnelKey.c_str(), ipAddr.to_string().c_str()); } mux_tunnel_nh_.erase(ipAddr); + + NextHopKey nhKey(ipAddr, tunnelKey, true /*tunnel_nh*/, 0 /*tag*/); + if (gNeighOrch) + { + gNeighOrch->removeIpinipTunnelNextHop(nhKey); + } } SWSS_LOG_INFO("NH tunnel removed %s, ip %s or decremented to ref count %d", diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index 0b1c3982223..46b8413c705 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -2045,6 +2045,63 @@ bool NeighOrch::removeTunnelNextHop(const NextHopKey& nh) return true; } +bool NeighOrch::addIpinipTunnelNextHop(const NextHopKey& nh, sai_object_id_t nh_id) +{ + SWSS_LOG_ENTER(); + + if (!nh.isTunnelNextHop()) + { + SWSS_LOG_ERROR("NextHopKey is not a tunnel NH: %s", nh.to_string().c_str()); + return false; + } + + if (nh_id == SAI_NULL_OBJECT_ID) + { + SWSS_LOG_ERROR("Invalid SAI OID for tunnel NH %s", nh.to_string().c_str()); + return false; + } + + if (m_syncdNextHops.find(nh) != m_syncdNextHops.end()) + { + SWSS_LOG_INFO("IPinIP tunnel NH already registered: %s", nh.to_string().c_str()); + return true; + } + + NextHopEntry next_hop_entry; + next_hop_entry.next_hop_id = nh_id; + next_hop_entry.ref_count = 0; + next_hop_entry.nh_flags = 0; + m_syncdNextHops[nh] = next_hop_entry; + + SWSS_LOG_NOTICE("Registered IPinIP tunnel NH %s (OID 0x%" PRIx64 ")", + nh.to_string().c_str(), nh_id); + return true; +} + +bool NeighOrch::removeIpinipTunnelNextHop(const NextHopKey& nh) +{ + SWSS_LOG_ENTER(); + + auto it = m_syncdNextHops.find(nh); + if (it == m_syncdNextHops.end()) + { + SWSS_LOG_ERROR("IPinIP tunnel NH not found: %s", nh.to_string().c_str()); + return false; + } + + if (it->second.ref_count > 0) + { + SWSS_LOG_ERROR("Cannot remove still-referenced IPinIP tunnel NH %s (ref_count=%d)", + nh.to_string().c_str(), it->second.ref_count); + return false; + } + + m_syncdNextHops.erase(it); + + SWSS_LOG_NOTICE("Unregistered IPinIP tunnel NH %s", nh.to_string().c_str()); + return true; +} + void NeighOrch::doVoqSystemNeighTask(Consumer &consumer) { SWSS_LOG_ENTER(); diff --git a/orchagent/neighorch.h b/orchagent/neighorch.h index 80ef011eff4..7d41e9da582 100644 --- a/orchagent/neighorch.h +++ b/orchagent/neighorch.h @@ -100,6 +100,9 @@ class NeighOrch : public Orch, public Subject, public Observer sai_object_id_t addTunnelNextHop(const NextHopKey&); bool removeTunnelNextHop(const NextHopKey&); + bool addIpinipTunnelNextHop(const NextHopKey& nh, sai_object_id_t nh_id); + bool removeIpinipTunnelNextHop(const NextHopKey& nh); + bool ifChangeInformNextHop(const string &, bool); bool isNextHopFlagSet(const NextHopKey &, const uint32_t); diff --git a/orchagent/nexthopkey.cpp b/orchagent/nexthopkey.cpp index 68707ca37be..5eec31369d0 100644 --- a/orchagent/nexthopkey.cpp +++ b/orchagent/nexthopkey.cpp @@ -12,6 +12,7 @@ std::size_t hash_value(const NextHopKey& obj) { boost::hash_combine(nh_hash, obj.srv6_segment); boost::hash_combine(nh_hash, obj.srv6_source); boost::hash_combine(nh_hash, obj.srv6_vpn_sid); + boost::hash_combine(nh_hash, obj.tunnel_name); return nh_hash; } diff --git a/orchagent/nexthopkey.h b/orchagent/nexthopkey.h index 21bc81f741c..1d45695029f 100644 --- a/orchagent/nexthopkey.h +++ b/orchagent/nexthopkey.h @@ -18,6 +18,7 @@ extern "C" #define NH_DELIMITER '@' #define NHG_DELIMITER ',' #define VRF_PREFIX "Vrf" +#define TUNNEL_PREFIX "tunnel:" extern IntfsOrch *gIntfsOrch; struct NextHopKey @@ -33,6 +34,7 @@ struct NextHopKey string srv6_segment; // SRV6 segment string string srv6_source; // SRV6 source address string srv6_vpn_sid; // SRV6 vpn sid + string tunnel_name; // IPinIP tunnel name NextHopKey() : weight(0) {} NextHopKey(const std::string &str, const std::string &alias) : @@ -51,6 +53,20 @@ struct NextHopKey std::string err = "Error converting " + str + " to NextHop"; throw std::invalid_argument(err); } + if (str.compare(0, strlen(TUNNEL_PREFIX), TUNNEL_PREFIX) == 0) + { + std::string body = str.substr(strlen(TUNNEL_PREFIX)); + auto keys = tokenize(body, NH_DELIMITER); + if (keys.size() != 2) + { + std::string err = "Error converting " + str + " to tunnel NextHop"; + throw std::invalid_argument(err); + } + tunnel_name = keys[0]; + ip_address = keys[1]; + weight = 0; + return; + } std::string ip_str = parseMplsNextHop(str); auto keys = tokenize(ip_str, NH_DELIMITER); if (keys.size() == 1) @@ -116,9 +132,18 @@ struct NextHopKey NextHopKey(const IpAddress &ip, const MacAddress &mac, const uint32_t &vni, bool overlay_nh) : ip_address(ip), alias(""), vni(vni), mac_address(mac), weight(0){} NextHopKey(const IpAddress &ip, const std::string &alias, const MacAddress &mac, const uint32_t &vni, bool overlay_nh) : ip_address(ip), alias(alias), vni(vni), mac_address(mac), weight(0){} + /* IPinIP tunnel next hop: identified by tunnel name + destination IP. */ + NextHopKey(const IpAddress &ip, const std::string &tnl_name, bool /*tunnel_nh*/, int /*tag*/) : + ip_address(ip), vni(0), mac_address(), weight(0), tunnel_name(tnl_name) {} + const std::string to_string() const { + if (isTunnelNextHop()) + { + return std::string(TUNNEL_PREFIX) + tunnel_name + + NH_DELIMITER + ip_address.to_string(); + } std::string str = formatMplsNextHop(); str += ip_address.to_string() + NH_DELIMITER + alias; return str; @@ -139,8 +164,8 @@ struct NextHopKey bool operator<(const NextHopKey &o) const { - return std::tie(ip_address, alias, label_stack, vni, mac_address, srv6_segment, srv6_source, srv6_vpn_sid) < - std::tie(o.ip_address, o.alias, o.label_stack, o.vni, o.mac_address, o.srv6_segment, o.srv6_source, o.srv6_vpn_sid); + return std::tie(ip_address, alias, label_stack, vni, mac_address, srv6_segment, srv6_source, srv6_vpn_sid, tunnel_name) < + std::tie(o.ip_address, o.alias, o.label_stack, o.vni, o.mac_address, o.srv6_segment, o.srv6_source, o.srv6_vpn_sid, o.tunnel_name); } bool operator==(const NextHopKey &o) const @@ -149,7 +174,8 @@ struct NextHopKey (label_stack == o.label_stack) && (vni == o.vni) && (mac_address == o.mac_address) && (srv6_segment == o.srv6_segment) && (srv6_source == o.srv6_source) && - (srv6_vpn_sid == o.srv6_vpn_sid); + (srv6_vpn_sid == o.srv6_vpn_sid) && + (tunnel_name == o.tunnel_name); } bool operator!=(const NextHopKey &o) const @@ -177,6 +203,12 @@ struct NextHopKey return (srv6_vpn_sid != ""); } + bool isTunnelNextHop() const + { + return (!tunnel_name.empty()); + } + + std::string parseMplsNextHop(const std::string& str) { // parseMplsNextHop initializes MPLS-related member data of the NextHopKey diff --git a/orchagent/nhgorch.cpp b/orchagent/nhgorch.cpp index ead7bfddcd4..e7d04645cff 100644 --- a/orchagent/nhgorch.cpp +++ b/orchagent/nhgorch.cpp @@ -1217,7 +1217,6 @@ bool NhgOrch::isHwProtectionSupported() bool NhgOrch::createProtNhg(const string &key, const vector &primary_nhs, const NextHopKey &standby_nh, - sai_object_id_t standby_nh_id, bool hw_protection) { SWSS_LOG_ENTER(); @@ -1242,8 +1241,7 @@ bool NhgOrch::createProtNhg(const string &key, return false; } - auto nhg = make_unique(key, primary_nhs, standby_nh, standby_nh_id, - hw_protection); + auto nhg = make_unique(key, primary_nhs, standby_nh, hw_protection); if (!nhg->sync()) { @@ -1271,6 +1269,50 @@ bool NhgOrch::createProtNhg(const string &key, return true; } +string NhgOrch::buildProtNhgKey(const vector &primary_nhs, + const NextHopKey &standby_nh, + bool hw_protection) +{ + string prefix = hw_protection ? "prot:hw:" : "prot:sw:"; + + set sorted(primary_nhs.begin(), primary_nhs.end()); + string primary_str; + for (auto it = sorted.begin(); it != sorted.end(); ++it) + { + if (it != sorted.begin()) + { + primary_str += NHG_DELIMITER; + } + primary_str += it->to_string(); + } + + return prefix + primary_str + "|" + standby_nh.to_string(); +} + +string NhgOrch::buildProtNhgKey(const NextHopGroupKey &primary_nhg_key, + const NextHopGroupKey &standby_nhg_key, + bool hw_protection) +{ + string prefix = hw_protection ? "prot:hw:" : "prot:sw:"; + return prefix + primary_nhg_key.to_string() + "|" + standby_nhg_key.to_string(); +} + +bool NhgOrch::createProtNhg(const vector &primary_nhs, + const NextHopKey &standby_nh, + bool hw_protection) +{ + return createProtNhg(buildProtNhgKey(primary_nhs, standby_nh, hw_protection), + primary_nhs, standby_nh, hw_protection); +} + +bool NhgOrch::createProtNhg(const NextHopGroupKey &primary_nhg_key, + const NextHopGroupKey &standby_nhg_key, + bool hw_protection) +{ + return createProtNhg(buildProtNhgKey(primary_nhg_key, standby_nhg_key, hw_protection), + primary_nhg_key, standby_nhg_key, hw_protection); +} + bool NhgOrch::createProtNhg(const string &key, const NextHopGroupKey &primary_nhg_key, const NextHopGroupKey &standby_nhg_key, @@ -1307,23 +1349,6 @@ bool NhgOrch::createProtNhg(const string &key, return false; } - sai_object_id_t primary_nhg_id = getNhg(primary_key_str).getId(); - sai_object_id_t standby_nhg_id = getNhg(standby_key_str).getId(); - - if (primary_nhg_id == SAI_NULL_OBJECT_ID) - { - SWSS_LOG_ERROR("Protection NHG %s: primary NHG %s is not synced", - key.c_str(), primary_key_str.c_str()); - return false; - } - - if (standby_nhg_id == SAI_NULL_OBJECT_ID) - { - SWSS_LOG_ERROR("Protection NHG %s: standby NHG %s is not synced", - key.c_str(), standby_key_str.c_str()); - return false; - } - if (m_protNhgs.find(key) != m_protNhgs.end()) { SWSS_LOG_ERROR("Protection NHG %s already exists", key.c_str()); @@ -1338,8 +1363,7 @@ bool NhgOrch::createProtNhg(const string &key, return false; } - auto nhg = make_unique(key, primary_nhg_key, primary_nhg_id, - standby_nhg_key, standby_nhg_id, + auto nhg = make_unique(key, primary_nhg_key, standby_nhg_key, hw_protection); if (!nhg->sync()) diff --git a/orchagent/nhgorch.h b/orchagent/nhgorch.h index fe3396c2b42..a72218c19c9 100644 --- a/orchagent/nhgorch.h +++ b/orchagent/nhgorch.h @@ -140,23 +140,39 @@ class NhgOrch : public NhgOrchCommon */ /* Create a protection NHG with one or more primary and one standby next hop. - * standby_nh_id: optional pre-resolved SAI OID for the standby NH - * (e.g., tunnel NH not registered in NeighOrch). + * Individual NHs are resolved via NeighOrch at sync time. */ bool createProtNhg(const string &key, const vector &primary_nhs, const NextHopKey &standby_nh, - sai_object_id_t standby_nh_id = SAI_NULL_OBJECT_ID, + bool hw_protection = true); + + /* Auto-keyed convenience overload -- key is derived from the members. */ + bool createProtNhg(const vector &primary_nhs, + const NextHopKey &standby_nh, bool hw_protection = true); /* Create a protection NHG where each role is an existing ECMP NHG. - * The group keys are resolved to their SAI OIDs via hasNhg/getNhg. + * NHG OIDs are dynamically resolved via NhgOrch at sync time. */ bool createProtNhg(const string &key, const NextHopGroupKey &primary_nhg_key, const NextHopGroupKey &standby_nhg_key, bool hw_protection = true); + /* Auto-keyed convenience overload -- key is derived from the group keys. */ + bool createProtNhg(const NextHopGroupKey &primary_nhg_key, + const NextHopGroupKey &standby_nhg_key, + bool hw_protection = true); + + /* Build the deterministic key for a protection NHG from its members. */ + static string buildProtNhgKey(const vector &primary_nhs, + const NextHopKey &standby_nh, + bool hw_protection = true); + static string buildProtNhgKey(const NextHopGroupKey &primary_nhg_key, + const NextHopGroupKey &standby_nhg_key, + bool hw_protection = true); + /* Remove a protection NHG by key. */ bool removeProtNhg(const string &key); diff --git a/orchagent/protnhg.cpp b/orchagent/protnhg.cpp index 9403fc60eef..a5d11700861 100644 --- a/orchagent/protnhg.cpp +++ b/orchagent/protnhg.cpp @@ -1,16 +1,18 @@ #include "protnhg.h" #include "neighorch.h" +#include "nhgorch.h" #include "logger.h" #include "sai_serialize.h" extern NeighOrch *gNeighOrch; +extern NhgOrch *gNhgOrch; ProtNhgMember::ProtNhgMember(const NextHopKey &key, ProtNhgRole role, - sai_object_id_t nh_id_override) : + const string &nhg_key) : NhgMember(key), m_role(role), m_monitored_oid(SAI_NULL_OBJECT_ID), - m_nh_id_override(nh_id_override) + m_nhg_key(nhg_key) { SWSS_LOG_ENTER(); } @@ -19,11 +21,10 @@ ProtNhgMember::ProtNhgMember(ProtNhgMember &&nhgm) : NhgMember(move(nhgm)), m_role(nhgm.m_role), m_monitored_oid(nhgm.m_monitored_oid), - m_nh_id_override(nhgm.m_nh_id_override) + m_nhg_key(move(nhgm.m_nhg_key)) { SWSS_LOG_ENTER(); nhgm.m_monitored_oid = SAI_NULL_OBJECT_ID; - nhgm.m_nh_id_override = SAI_NULL_OBJECT_ID; } ProtNhgMember::~ProtNhgMember() @@ -36,7 +37,11 @@ void ProtNhgMember::sync(sai_object_id_t gm_id) SWSS_LOG_ENTER(); NhgMember::sync(gm_id); - if (m_nh_id_override == SAI_NULL_OBJECT_ID) + if (isRecursive()) + { + gNhgOrch->incNhgRefCount(m_nhg_key); + } + else { gNeighOrch->increaseNextHopRefCount(m_key); } @@ -51,7 +56,11 @@ void ProtNhgMember::remove() return; } - if (m_nh_id_override == SAI_NULL_OBJECT_ID) + if (isRecursive()) + { + gNhgOrch->decNhgRefCount(m_nhg_key); + } + else { gNeighOrch->decreaseNextHopRefCount(m_key); } @@ -62,9 +71,15 @@ sai_object_id_t ProtNhgMember::getNhId() const { SWSS_LOG_ENTER(); - if (m_nh_id_override != SAI_NULL_OBJECT_ID) + if (isRecursive()) { - return m_nh_id_override; + if (gNhgOrch->hasNhg(m_nhg_key)) + { + return gNhgOrch->getNhg(m_nhg_key).getId(); + } + SWSS_LOG_WARN("Recursive NHG %s not found for member %s", + m_nhg_key.c_str(), m_key.to_string().c_str()); + return SAI_NULL_OBJECT_ID; } if (gNeighOrch->hasNextHop(m_key)) @@ -79,10 +94,9 @@ bool ProtNhgMember::updateMonitoredObject(sai_object_id_t oid) { SWSS_LOG_ENTER(); - m_monitored_oid = oid; - if (!isSynced()) { + m_monitored_oid = oid; return true; } @@ -100,6 +114,7 @@ bool ProtNhgMember::updateMonitoredObject(sai_object_id_t oid) return false; } + m_monitored_oid = oid; return true; } @@ -145,7 +160,6 @@ string ProtNhgMember::to_string() const ProtNhg::ProtNhg(const string &key, const vector &primary_nhs, const NextHopKey &standby_nh, - sai_object_id_t standby_nh_id, bool hw_protection) : NhgCommon(key), m_hw_protection(hw_protection) @@ -157,14 +171,12 @@ ProtNhg::ProtNhg(const string &key, m_members.emplace(nh, ProtNhgMember(nh, ProtNhgRole::PRIMARY)); } m_members.emplace(standby_nh, - ProtNhgMember(standby_nh, ProtNhgRole::STANDBY, standby_nh_id)); + ProtNhgMember(standby_nh, ProtNhgRole::STANDBY)); } ProtNhg::ProtNhg(const string &key, const NextHopGroupKey &primary_nhg_key, - sai_object_id_t primary_nhg_id, const NextHopGroupKey &standby_nhg_key, - sai_object_id_t standby_nhg_id, bool hw_protection) : NhgCommon(key), m_hw_protection(hw_protection) @@ -175,9 +187,11 @@ ProtNhg::ProtNhg(const string &key, const NextHopKey &standby_rep = *standby_nhg_key.getNextHops().begin(); m_members.emplace(primary_rep, - ProtNhgMember(primary_rep, ProtNhgRole::PRIMARY, primary_nhg_id)); + ProtNhgMember(primary_rep, ProtNhgRole::PRIMARY, + primary_nhg_key.to_string())); m_members.emplace(standby_rep, - ProtNhgMember(standby_rep, ProtNhgRole::STANDBY, standby_nhg_id)); + ProtNhgMember(standby_rep, ProtNhgRole::STANDBY, + standby_nhg_key.to_string())); } ProtNhg::ProtNhg(ProtNhg &&nhg) : @@ -237,7 +251,6 @@ bool ProtNhg::sync() if (!syncMembers(member_keys)) { SWSS_LOG_WARN("Failed to sync members of protection NHG %s", m_key.c_str()); - remove(); return false; } diff --git a/orchagent/protnhg.h b/orchagent/protnhg.h index b13dd1864ba..3403014deb3 100644 --- a/orchagent/protnhg.h +++ b/orchagent/protnhg.h @@ -24,7 +24,7 @@ class ProtNhgMember : public NhgMember { public: ProtNhgMember(const NextHopKey &key, ProtNhgRole role, - sai_object_id_t nh_id_override = SAI_NULL_OBJECT_ID); + const string &nhg_key = ""); ProtNhgMember(ProtNhgMember &&nhgm); @@ -36,6 +36,9 @@ class ProtNhgMember : public NhgMember inline ProtNhgRole getRole() const { return m_role; } sai_object_id_t getNhId() const; + /* True when this member represents a recursive NHG rather than an individual NH. */ + inline bool isRecursive() const { return !m_nhg_key.empty(); } + inline sai_object_id_t getMonitoredObject() const { return m_monitored_oid; } void setMonitoredObject(sai_object_id_t oid) { m_monitored_oid = oid; } @@ -49,7 +52,7 @@ class ProtNhgMember : public NhgMember private: ProtNhgRole m_role; sai_object_id_t m_monitored_oid; - sai_object_id_t m_nh_id_override; + string m_nhg_key; /* Non-empty when member is a recursive NHG (resolved via NhgOrch). */ }; /* @@ -65,18 +68,16 @@ class ProtNhg : public NhgCommon ProtNhg(const string &key, const vector &primary_nhs, const NextHopKey &standby_nh, - sai_object_id_t standby_nh_id = SAI_NULL_OBJECT_ID, bool hw_protection = true); - /* Construct from NextHopGroupKey pairs with pre-resolved NHG OIDs. + + /* Construct from NextHopGroupKey pairs (recursive/nested NHG). * Each group becomes a single protection member whose SAI next-hop ID - * points to the resolved ECMP NHG OID (recursive/nested NHG). + * is dynamically resolved via NhgOrch at sync time. */ ProtNhg(const string &key, const NextHopGroupKey &primary_nhg_key, - sai_object_id_t primary_nhg_id, const NextHopGroupKey &standby_nhg_key, - sai_object_id_t standby_nhg_id, bool hw_protection = true); ProtNhg(ProtNhg &&nhg); diff --git a/orchagent/tunneldecaporch.cpp b/orchagent/tunneldecaporch.cpp index 2d7796b906d..3e4b8c77ec2 100644 --- a/orchagent/tunneldecaporch.cpp +++ b/orchagent/tunneldecaporch.cpp @@ -3,6 +3,7 @@ #include "tunneldecaporch.h" #include "portsorch.h" #include "crmorch.h" +#include "neighorch.h" #include "logger.h" #include "swssnet.h" #include "qosorch.h" @@ -26,6 +27,7 @@ extern sai_object_id_t gSwitchId; extern PortsOrch* gPortsOrch; extern CrmOrch* gCrmOrch; extern QosOrch* gQosOrch; +extern NeighOrch* gNeighOrch; TunnelDecapOrch::TunnelDecapOrch( DBConnector *appDb, DBConnector *stateDb, @@ -1351,6 +1353,12 @@ sai_object_id_t TunnelDecapOrch::createNextHopTunnel(std::string tunnelKey, IpAd } tunnelNhs[tunnelKey][ipAddr] = { next_hop_id, 1 }; + + NextHopKey nhKey(ipAddr, tunnelKey, true /*tunnel_nh*/, 0 /*tag*/); + if (gNeighOrch) + { + gNeighOrch->addIpinipTunnelNextHop(nhKey, next_hop_id); + } } return next_hop_id; @@ -1414,6 +1422,12 @@ bool TunnelDecapOrch::removeNextHopTunnel(std::string tunnelKey, IpAddress& ipAd tunnelNhs[tunnelKey].erase(ipAddr); + NextHopKey nhKey(ipAddr, tunnelKey, true /*tunnel_nh*/, 0 /*tag*/); + if (gNeighOrch) + { + gNeighOrch->removeIpinipTunnelNextHop(nhKey); + } + return true; } diff --git a/tests/mock_tests/protnhg_ut.cpp b/tests/mock_tests/protnhg_ut.cpp index 649d8d3d82a..06c8d23ef2c 100644 --- a/tests/mock_tests/protnhg_ut.cpp +++ b/tests/mock_tests/protnhg_ut.cpp @@ -11,6 +11,11 @@ #include "nhgbase.h" #undef protected +#define private public +#include "neighorch.h" +#undef private + + #include "mock_orchagent_main.h" #include "mock_sai_api.h" #include "mock_orch_test.h" @@ -36,6 +41,22 @@ namespace protnhg_test { static uint64_t nhg_oid_counter = 0x5000000; static uint64_t nhgm_oid_counter = 0x6000000; + static uint64_t nh_oid_counter = 0x8000000; + + static void registerNextHop(const NextHopKey &nh, + sai_object_id_t nh_id = SAI_NULL_OBJECT_ID) + { + if (nh_id == SAI_NULL_OBJECT_ID) + { + nh_id = ++nh_oid_counter; + } + gNeighOrch->m_syncdNextHops[nh] = { nh_id, 0, 0 }; + } + + static void unregisterNextHop(const NextHopKey &nh) + { + gNeighOrch->m_syncdNextHops.erase(nh); + } class ProtNhgTest : public MockOrchTest { @@ -119,17 +140,22 @@ namespace protnhg_test string key = "prot_nhg_1"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - sai_object_id_t standby_nh_id = 0x1234; + + registerNextHop(primary_nh); + registerNextHop(standby_nh); vector primaries = {primary_nh}; - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, standby_nh_id)); + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); EXPECT_TRUE(gNhgOrch->hasProtNhg(key)); EXPECT_NE(gNhgOrch->getProtNhgId(key), SAI_NULL_OBJECT_ID); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); EXPECT_FALSE(gNhgOrch->hasProtNhg(key)); EXPECT_EQ(gNhgOrch->getProtNhgId(key), SAI_NULL_OBJECT_ID); + + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, CreateDuplicateProtNhgFails) @@ -137,13 +163,17 @@ namespace protnhg_test string key = "prot_nhg_dup"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - sai_object_id_t standby_nh_id = 0x1234; vector primaries = {primary_nh}; - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, standby_nh_id)); - EXPECT_FALSE(gNhgOrch->createProtNhg(key, primaries, standby_nh, standby_nh_id)); + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + EXPECT_FALSE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, CreateProtNhgEmptyPrimariesFails) @@ -151,7 +181,7 @@ namespace protnhg_test NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); vector primaries; - EXPECT_FALSE(gNhgOrch->createProtNhg("empty", primaries, standby_nh, 0x1234)); + EXPECT_FALSE(gNhgOrch->createProtNhg("empty", primaries, standby_nh)); EXPECT_FALSE(gNhgOrch->hasProtNhg("empty")); } @@ -167,7 +197,10 @@ namespace protnhg_test NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); vector primaries = {primary_nh}; - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); gNhgOrch->incProtNhgRefCount(key); EXPECT_FALSE(gNhgOrch->removeProtNhg(key)); @@ -176,6 +209,9 @@ namespace protnhg_test gNhgOrch->decProtNhgRefCount(key); EXPECT_TRUE(gNhgOrch->removeProtNhg(key)); EXPECT_FALSE(gNhgOrch->hasProtNhg(key)); + + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, GetProtNhgMembers) @@ -183,10 +219,12 @@ namespace protnhg_test string key = "prot_nhg_members"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - sai_object_id_t standby_nh_id = 0x1234; vector primaries = {primary_nh}; - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, standby_nh_id)); + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); const ProtNhg &nhg = gNhgOrch->getProtNhg(key); EXPECT_NE(nhg.getId(), SAI_NULL_OBJECT_ID); @@ -200,6 +238,8 @@ namespace protnhg_test EXPECT_EQ(primary_out[0]->getRole(), ProtNhgRole::PRIMARY); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, MultiplePrimaryMembers) @@ -210,13 +250,20 @@ namespace protnhg_test NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); vector primaries = {primary1, primary2}; - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + registerNextHop(primary1); + registerNextHop(primary2); + registerNextHop(standby_nh); + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); const ProtNhg &nhg = gNhgOrch->getProtNhg(key); auto primary_out = nhg.getPrimaryMembers(); EXPECT_EQ(primary_out.size(), 2u); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(primary1); + unregisterNextHop(primary2); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, SetAdminRole) @@ -226,7 +273,10 @@ namespace protnhg_test NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); vector primaries = {primary_nh}; - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); EXPECT_CALL(*mock_sai_next_hop_group_api, set_next_hop_group_attribute(_, _)) @@ -237,6 +287,8 @@ namespace protnhg_test key, SAI_NEXT_HOP_GROUP_MEMBER_CONFIGURED_ROLE_PRIMARY)); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, SetAdminRoleNonExistentFails) @@ -252,7 +304,10 @@ namespace protnhg_test NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); vector primaries = {primary_nh}; - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); EXPECT_CALL(*mock_sai_next_hop_group_api, set_next_hop_group_attribute(_, _)) @@ -263,6 +318,8 @@ namespace protnhg_test key, SAI_NEXT_HOP_GROUP_MEMBER_CONFIGURED_ROLE_PRIMARY)); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, SetMonitoredObjectOnStandbyMember) @@ -270,15 +327,19 @@ namespace protnhg_test string key = "prot_nhg_monitor"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - sai_object_id_t standby_nh_id = 0x1234; sai_object_id_t session_oid = 0xABCD; vector primaries = {primary_nh}; - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, standby_nh_id)); + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); EXPECT_TRUE(gNhgOrch->setProtNhgMonitoredObject(key, standby_nh, session_oid)); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, SetMonitoredObjectNonExistentNhgFails) @@ -295,10 +356,15 @@ namespace protnhg_test NextHopKey unknown_nh(IpAddress("10.0.0.99"), string("Ethernet0")); vector primaries = {primary_nh}; - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); EXPECT_FALSE(gNhgOrch->setProtNhgMonitoredObject(key, unknown_nh, 0xABCD)); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, ObservedRoleNonExistentNhgFails) @@ -325,8 +391,14 @@ namespace protnhg_test NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); vector primaries = {primary_nh}; - EXPECT_FALSE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + EXPECT_FALSE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); EXPECT_FALSE(gNhgOrch->hasProtNhg(key)); + + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, HasAndGetIdForNonExistentKey) @@ -335,7 +407,6 @@ namespace protnhg_test EXPECT_EQ(gNhgOrch->getProtNhgId("ghost"), SAI_NULL_OBJECT_ID); } - TEST_F(ProtNhgTest, SyncAlreadySynced) { string key = "prot_nhg_double_sync"; @@ -343,12 +414,17 @@ namespace protnhg_test NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); vector primaries = {primary_nh}; - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); auto &nhg = const_cast(gNhgOrch->getProtNhg(key)); EXPECT_TRUE(nhg.sync()); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, SyncMembersFailure) @@ -373,8 +449,14 @@ namespace protnhg_test NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); vector primaries = {primary_nh}; - EXPECT_FALSE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + EXPECT_FALSE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); EXPECT_FALSE(gNhgOrch->hasProtNhg(key)); + + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, SetAdminRoleUnsyncedNhg) @@ -383,7 +465,7 @@ namespace protnhg_test NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); vector primaries = {primary_nh}; - ProtNhg nhg("unsynced_nhg", primaries, standby_nh, 0x1234); + ProtNhg nhg("unsynced_nhg", primaries, standby_nh); EXPECT_FALSE(nhg.setAdminRole( SAI_NEXT_HOP_GROUP_MEMBER_CONFIGURED_ROLE_PRIMARY)); } @@ -394,7 +476,7 @@ namespace protnhg_test NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); vector primaries = {primary_nh}; - ProtNhg nhg("unsynced_mon", primaries, standby_nh, 0x1234); + ProtNhg nhg("unsynced_mon", primaries, standby_nh); EXPECT_TRUE(nhg.updateMemberMonitoredObject(standby_nh, 0xABCD)); } @@ -405,7 +487,10 @@ namespace protnhg_test NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); vector primaries = {primary_nh}; - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); EXPECT_CALL(*mock_sai_next_hop_group_api, set_next_hop_group_member_attribute(_, _)) @@ -414,6 +499,8 @@ namespace protnhg_test EXPECT_FALSE(gNhgOrch->setProtNhgMonitoredObject(key, standby_nh, 0xABCD)); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, ObservedRoleUnsyncedMember) @@ -423,13 +510,16 @@ namespace protnhg_test NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); vector primaries = {primary_nh}; - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + registerNextHop(standby_nh); + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); sai_next_hop_group_member_observed_role_t role; EXPECT_FALSE(gNhgOrch->getProtNhgMemberObservedRole( key, primary_nh, role)); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, ObservedRoleSuccess) @@ -439,7 +529,10 @@ namespace protnhg_test NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); vector primaries = {primary_nh}; - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); auto old_get_fn = ut_sai_next_hop_group_api.get_next_hop_group_member_attribute; @@ -458,6 +551,8 @@ namespace protnhg_test ut_sai_next_hop_group_api.get_next_hop_group_member_attribute = old_get_fn; ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, ObservedRoleSaiFailure) @@ -467,7 +562,10 @@ namespace protnhg_test NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); vector primaries = {primary_nh}; - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); auto old_get_fn = ut_sai_next_hop_group_api.get_next_hop_group_member_attribute; @@ -484,6 +582,8 @@ namespace protnhg_test ut_sai_next_hop_group_api.get_next_hop_group_member_attribute = old_get_fn; ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, GetMemberObservedRoleNotFound) @@ -494,13 +594,18 @@ namespace protnhg_test NextHopKey unknown_nh(IpAddress("10.0.0.99"), string("Ethernet0")); vector primaries = {primary_nh}; - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); sai_next_hop_group_member_observed_role_t role; EXPECT_FALSE(gNhgOrch->getProtNhgMemberObservedRole( key, unknown_nh, role)); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, GetAllMemberObservedRolesSuccess) @@ -510,7 +615,10 @@ namespace protnhg_test NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); vector primaries = {primary_nh}; - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); auto old_get_fn = ut_sai_next_hop_group_api.get_next_hop_group_member_attribute; @@ -523,11 +631,13 @@ namespace protnhg_test map roles; EXPECT_TRUE(gNhgOrch->getProtNhgAllObservedRoles(key, roles)); - EXPECT_EQ(roles.size(), 1u); + EXPECT_EQ(roles.size(), 2u); ut_sai_next_hop_group_api.get_next_hop_group_member_attribute = old_get_fn; ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, GetAllMemberObservedRolesSaiFailure) @@ -537,7 +647,10 @@ namespace protnhg_test NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); vector primaries = {primary_nh}; - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); auto old_get_fn = ut_sai_next_hop_group_api.get_next_hop_group_member_attribute; @@ -554,6 +667,8 @@ namespace protnhg_test ut_sai_next_hop_group_api.get_next_hop_group_member_attribute = old_get_fn; ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, RemoveProtNhgSaiFailure) @@ -563,7 +678,10 @@ namespace protnhg_test NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); vector primaries = {primary_nh}; - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); EXPECT_CALL(*mock_sai_next_hop_group_api, remove_next_hop_group(_)) @@ -574,6 +692,8 @@ namespace protnhg_test EXPECT_TRUE(gNhgOrch->hasProtNhg(key)); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, ProtNhgInlineMethods) @@ -583,7 +703,10 @@ namespace protnhg_test NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); vector primaries = {primary_nh}; - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); const ProtNhg &nhg = gNhgOrch->getProtNhg(key); EXPECT_FALSE(nhg.isTemp()); @@ -591,6 +714,8 @@ namespace protnhg_test EXPECT_EQ(nhg.to_string(), key); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, MemberToString) @@ -600,7 +725,10 @@ namespace protnhg_test NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); vector primaries = {primary_nh}; - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, 0x1234)); + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); const ProtNhg &nhg = gNhgOrch->getProtNhg(key); const ProtNhgMember *standby = nhg.getStandbyMember(); @@ -615,6 +743,8 @@ namespace protnhg_test EXPECT_NE(pstr.find("primary"), string::npos); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, MemberRemoveUnsynced) @@ -633,7 +763,7 @@ namespace protnhg_test NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); vector primaries = {primary_nh}; - ProtNhg nhg1("nhg_move", primaries, standby_nh, 0x1234); + ProtNhg nhg1("nhg_move", primaries, standby_nh); ProtNhg nhg2(std::move(nhg1)); EXPECT_NE(nhg2.getStandbyMember(), nullptr); @@ -644,17 +774,21 @@ namespace protnhg_test { NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - sai_object_id_t standby_nh_id = 0x1234; sai_object_id_t session_oid = 0xABCD; vector primaries = {primary_nh}; - ProtNhg nhg("nhg_mon_sync", primaries, standby_nh, standby_nh_id); + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + ProtNhg nhg("nhg_mon_sync", primaries, standby_nh); EXPECT_TRUE(nhg.updateMemberMonitoredObject(standby_nh, session_oid)); EXPECT_TRUE(nhg.sync()); EXPECT_TRUE(nhg.isSynced()); - } + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); + } static uint64_t ecmp_nhg_oid_counter = 0x7000000; @@ -756,9 +890,11 @@ namespace protnhg_test string key = "prot_nhg_sw"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - sai_object_id_t standby_nh_id = 0x1234; vector primaries = {primary_nh}; + registerNextHop(primary_nh); + registerNextHop(standby_nh); + EXPECT_CALL(*mock_sai_next_hop_group_api, create_next_hop_group(_, _, _, _)) .WillOnce( @@ -777,11 +913,12 @@ namespace protnhg_test }); ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, - standby_nh_id, false)); EXPECT_TRUE(gNhgOrch->hasProtNhg(key)); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, SetAdminRoleOnProtectionNhgFails) @@ -791,13 +928,18 @@ namespace protnhg_test NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); vector primaries = {primary_nh}; + registerNextHop(primary_nh); + registerNextHop(standby_nh); + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, - 0x1234, false)); + false)); EXPECT_FALSE(gNhgOrch->setProtNhgAdminRole( key, SAI_NEXT_HOP_GROUP_MEMBER_CONFIGURED_ROLE_PRIMARY)); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, SetSwitchoverSuccess) @@ -807,8 +949,11 @@ namespace protnhg_test NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); vector primaries = {primary_nh}; + registerNextHop(primary_nh); + registerNextHop(standby_nh); + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, - 0x1234, false)); + false)); EXPECT_CALL(*mock_sai_next_hop_group_api, set_next_hop_group_attribute(_, _)) @@ -818,6 +963,8 @@ namespace protnhg_test EXPECT_TRUE(gNhgOrch->setProtNhgSwitchover(key, true)); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, SetSwitchoverOnHwProtectionFails) @@ -827,16 +974,249 @@ namespace protnhg_test NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); vector primaries = {primary_nh}; + registerNextHop(primary_nh); + registerNextHop(standby_nh); + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, - 0x1234, true)); + true)); EXPECT_FALSE(gNhgOrch->setProtNhgSwitchover(key, true)); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, SetSwitchoverNonExistentNhgFails) { EXPECT_FALSE(gNhgOrch->setProtNhgSwitchover("no_such_key", true)); } -} + + TEST_F(ProtNhgTest, CreateProtNhgAutoKey) + { + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + vector primaries = {primary_nh}; + + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + string expected_key = NhgOrch::buildProtNhgKey(primaries, standby_nh); + EXPECT_FALSE(expected_key.empty()); + + ASSERT_TRUE(gNhgOrch->createProtNhg(primaries, standby_nh)); + EXPECT_TRUE(gNhgOrch->hasProtNhg(expected_key)); + + EXPECT_FALSE(gNhgOrch->createProtNhg(primaries, standby_nh)); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(expected_key)); + EXPECT_FALSE(gNhgOrch->hasProtNhg(expected_key)); + + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); + } + + TEST_F(ProtNhgTest, CreateProtNhgAutoKeyWithNhgKeys) + { + NextHopGroupKey primary_nhg_key("10.0.0.1@Ethernet0,10.0.0.2@Ethernet0"); + NextHopGroupKey standby_nhg_key("10.0.0.100@Ethernet4"); + + addEcmpNhg(primary_nhg_key); + addEcmpNhg(standby_nhg_key); + + string expected_key = NhgOrch::buildProtNhgKey(primary_nhg_key, + standby_nhg_key); + EXPECT_FALSE(expected_key.empty()); + + ASSERT_TRUE(gNhgOrch->createProtNhg(primary_nhg_key, standby_nhg_key)); + EXPECT_TRUE(gNhgOrch->hasProtNhg(expected_key)); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(expected_key)); + + removeEcmpNhg(primary_nhg_key.to_string()); + removeEcmpNhg(standby_nhg_key.to_string()); + } + + TEST_F(ProtNhgTest, BuildProtNhgKeySortsPrimaries) + { + NextHopKey nh_a(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey nh_b(IpAddress("10.0.0.2"), string("Ethernet0")); + NextHopKey standby(IpAddress("10.0.0.100"), string("Ethernet4")); + + string key_ab = NhgOrch::buildProtNhgKey({nh_a, nh_b}, standby); + string key_ba = NhgOrch::buildProtNhgKey({nh_b, nh_a}, standby); + + EXPECT_EQ(key_ab, key_ba); + } + + /* --- IPinIP tunnel NextHopKey tests --- */ + + TEST_F(ProtNhgTest, TunnelNextHopKeyConstructor) + { + IpAddress ip("10.1.0.32"); + NextHopKey nh(ip, string("MuxTunnel0"), true /*tunnel_nh*/, 0 /*tag*/); + + EXPECT_TRUE(nh.isTunnelNextHop()); + EXPECT_EQ(nh.ip_address, ip); + EXPECT_EQ(nh.tunnel_name, "MuxTunnel0"); + EXPECT_EQ(nh.alias, ""); + EXPECT_EQ(nh.vni, 0u); + EXPECT_FALSE(nh.isSrv6NextHop()); + EXPECT_FALSE(nh.isMplsNextHop()); + } + + TEST_F(ProtNhgTest, TunnelNextHopKeyToStringRoundtrip) + { + IpAddress ip("192.168.1.1"); + NextHopKey original(ip, string("IPINIP_TUNNEL"), true /*tunnel_nh*/, 0 /*tag*/); + + string str = original.to_string(); + EXPECT_EQ(str, "tunnel:IPINIP_TUNNEL@192.168.1.1"); + + NextHopKey parsed(str); + EXPECT_TRUE(parsed.isTunnelNextHop()); + EXPECT_EQ(parsed.tunnel_name, "IPINIP_TUNNEL"); + EXPECT_EQ(parsed.ip_address, ip); + EXPECT_EQ(original, parsed); + } + + TEST_F(ProtNhgTest, TunnelNextHopKeyComparison) + { + NextHopKey nh_a(IpAddress("10.0.0.1"), string("TunA"), true, 0); + NextHopKey nh_b(IpAddress("10.0.0.1"), string("TunB"), true, 0); + NextHopKey nh_same(IpAddress("10.0.0.1"), string("TunA"), true, 0); + + EXPECT_EQ(nh_a, nh_same); + EXPECT_NE(nh_a, nh_b); + + NextHopKey regular_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + EXPECT_NE(nh_a, regular_nh); + } + + TEST_F(ProtNhgTest, TunnelNextHopKeyInvalidParseFails) + { + EXPECT_THROW(NextHopKey("tunnel:@10.0.0.1@extra"), std::invalid_argument); + EXPECT_THROW(NextHopKey("tunnel:OnlyName"), std::invalid_argument); + } + + /* --- Protection NHG key prefix tests --- */ + + TEST_F(ProtNhgTest, BuildProtNhgKeyHwPrefix) + { + NextHopKey primary(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby(IpAddress("10.0.0.100"), string("Ethernet4")); + + string key = NhgOrch::buildProtNhgKey({primary}, standby, true); + EXPECT_EQ(key.substr(0, 8), "prot:hw:"); + EXPECT_NE(key.find("10.0.0.1@Ethernet0"), string::npos); + } + + TEST_F(ProtNhgTest, BuildProtNhgKeySwPrefix) + { + NextHopKey primary(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby(IpAddress("10.0.0.100"), string("Ethernet4")); + + string key = NhgOrch::buildProtNhgKey({primary}, standby, false); + EXPECT_EQ(key.substr(0, 8), "prot:sw:"); + } + + TEST_F(ProtNhgTest, BuildProtNhgKeyNhgHwPrefix) + { + NextHopGroupKey primary("10.0.0.1@Ethernet0,10.0.0.2@Ethernet0"); + NextHopGroupKey standby("10.0.0.100@Ethernet4"); + + string key = NhgOrch::buildProtNhgKey(primary, standby, true); + EXPECT_EQ(key.substr(0, 8), "prot:hw:"); + } + + TEST_F(ProtNhgTest, BuildProtNhgKeyNhgSwPrefix) + { + NextHopGroupKey primary("10.0.0.1@Ethernet0,10.0.0.2@Ethernet0"); + NextHopGroupKey standby("10.0.0.100@Ethernet4"); + + string key = NhgOrch::buildProtNhgKey(primary, standby, false); + EXPECT_EQ(key.substr(0, 8), "prot:sw:"); + } + + TEST_F(ProtNhgTest, HwAndSwKeysAreDifferent) + { + NextHopKey primary(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby(IpAddress("10.0.0.100"), string("Ethernet4")); + + string hw_key = NhgOrch::buildProtNhgKey({primary}, standby, true); + string sw_key = NhgOrch::buildProtNhgKey({primary}, standby, false); + EXPECT_NE(hw_key, sw_key); + } + + /* --- Tunnel NH in a protection NHG --- */ + + TEST_F(ProtNhgTest, CreateProtNhgWithTunnelStandby) + { + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.1.0.32"), string("MuxTunnel0"), + true /*tunnel_nh*/, 0 /*tag*/); + vector primaries = {primary_nh}; + + registerNextHop(primary_nh); + registerNextHop(standby_nh, 0xBEEF); + + string key = "prot_tunnel_standby"; + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + EXPECT_TRUE(gNhgOrch->hasProtNhg(key)); + EXPECT_NE(gNhgOrch->getProtNhgId(key), SAI_NULL_OBJECT_ID); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + EXPECT_FALSE(gNhgOrch->hasProtNhg(key)); + + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); + } + + TEST_F(ProtNhgTest, AutoKeyWithTunnelNextHop) + { + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.1.0.32"), string("MuxTunnel0"), + true /*tunnel_nh*/, 0 /*tag*/); + vector primaries = {primary_nh}; + + registerNextHop(primary_nh); + registerNextHop(standby_nh, 0xBEEF); + + string expected_key = NhgOrch::buildProtNhgKey(primaries, standby_nh, true); + EXPECT_NE(expected_key.find("tunnel:MuxTunnel0"), string::npos); + + ASSERT_TRUE(gNhgOrch->createProtNhg(primaries, standby_nh)); + EXPECT_TRUE(gNhgOrch->hasProtNhg(expected_key)); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(expected_key)); + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); + } + + TEST_F(ProtNhgTest, RecursiveMemberResolvesViaNhgOrch) + { + NextHopGroupKey primary_nhg_key("10.0.0.1@Ethernet0,10.0.0.2@Ethernet0"); + NextHopGroupKey standby_nhg_key("10.0.0.100@Ethernet4"); + + addEcmpNhg(primary_nhg_key); + addEcmpNhg(standby_nhg_key); + + string key = "prot_recursive_resolve"; + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nhg_key, standby_nhg_key)); + + const ProtNhg &nhg = gNhgOrch->getProtNhg(key); + auto primaries = nhg.getPrimaryMembers(); + ASSERT_EQ(primaries.size(), 1u); + EXPECT_TRUE(primaries[0]->isRecursive()); + EXPECT_NE(primaries[0]->getNhId(), SAI_NULL_OBJECT_ID); + + const ProtNhgMember *standby = nhg.getStandbyMember(); + ASSERT_NE(standby, nullptr); + EXPECT_TRUE(standby->isRecursive()); + EXPECT_NE(standby->getNhId(), SAI_NULL_OBJECT_ID); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + removeEcmpNhg(primary_nhg_key.to_string()); + removeEcmpNhg(standby_nhg_key.to_string()); + } + } From 5ccf44d384f0ff138b29359f7e0564b647e3b3ee Mon Sep 17 00:00:00 2001 From: Manas Kumar Mandal Date: Fri, 29 May 2026 12:25:36 -0700 Subject: [PATCH 07/20] Make createProtNhg idempotent Signed-off-by: Manas Kumar Mandal --- orchagent/nhgorch.cpp | 19 +++++++++++-------- orchagent/nhgorch.h | 5 +++++ 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/orchagent/nhgorch.cpp b/orchagent/nhgorch.cpp index e7d04645cff..fc85e9ec3b4 100644 --- a/orchagent/nhgorch.cpp +++ b/orchagent/nhgorch.cpp @@ -1229,8 +1229,8 @@ bool NhgOrch::createProtNhg(const string &key, if (m_protNhgs.find(key) != m_protNhgs.end()) { - SWSS_LOG_ERROR("Protection NHG %s already exists", key.c_str()); - return false; + SWSS_LOG_INFO("Protection NHG %s already exists", key.c_str()); + return true; } if (gRouteOrch->getNhgCount() + NhgBase::getSyncedCount() >= @@ -1332,6 +1332,15 @@ bool NhgOrch::createProtNhg(const string &key, return false; } + /* Idempotent re-create: short-circuit before reference / capacity checks + * so a repeat call after a transient member-NHG removal still returns + * success, matching the individual-NH overload's contract. */ + if (m_protNhgs.find(key) != m_protNhgs.end()) + { + SWSS_LOG_INFO("Protection NHG %s already exists", key.c_str()); + return true; + } + string primary_key_str = primary_nhg_key.to_string(); string standby_key_str = standby_nhg_key.to_string(); @@ -1349,12 +1358,6 @@ bool NhgOrch::createProtNhg(const string &key, return false; } - if (m_protNhgs.find(key) != m_protNhgs.end()) - { - SWSS_LOG_ERROR("Protection NHG %s already exists", key.c_str()); - return false; - } - if (gRouteOrch->getNhgCount() + NhgBase::getSyncedCount() >= gRouteOrch->getMaxNhgCount()) { diff --git a/orchagent/nhgorch.h b/orchagent/nhgorch.h index a72218c19c9..61159ede7e5 100644 --- a/orchagent/nhgorch.h +++ b/orchagent/nhgorch.h @@ -137,6 +137,11 @@ class NhgOrch : public NhgOrchCommon * Protection NHG APIs. * MuxOrch is the primary consumer of these for dual-ToR hardware * protection switching. Capacity accounting is shared with ECMP NHGs. + * + * All createProtNhg overloads are idempotent: re-creating with an + * existing canonical key is a no-op that returns true. Membership + * is immutable once created -- callers wishing to change membership + * must removeProtNhg() first. */ /* Create a protection NHG with one or more primary and one standby next hop. From b62e0fff18b6c7f1bd42ab2e4d5f77b7edfd6c23 Mon Sep 17 00:00:00 2001 From: Manas Kumar Mandal Date: Wed, 3 Jun 2026 22:27:23 -0700 Subject: [PATCH 08/20] Remove IPinIP tunnel NH registration from Protection NHG PR Signed-off-by: Manas Kumar Mandal --- orchagent/muxorch.cpp | 11 ------- orchagent/neighorch.cpp | 57 --------------------------------- orchagent/neighorch.h | 3 -- orchagent/nexthopkey.cpp | 1 - orchagent/nexthopkey.h | 38 ++-------------------- orchagent/tunneldecaporch.cpp | 14 -------- tests/mock_tests/protnhg_ut.cpp | 50 ----------------------------- 7 files changed, 3 insertions(+), 171 deletions(-) diff --git a/orchagent/muxorch.cpp b/orchagent/muxorch.cpp index 6aa249c89f3..baf173c0f81 100644 --- a/orchagent/muxorch.cpp +++ b/orchagent/muxorch.cpp @@ -1521,11 +1521,6 @@ sai_object_id_t MuxOrch::createNextHopTunnel(std::string tunnelKey, swss::IpAddr if (SAI_NULL_OBJECT_ID != nh) { mux_tunnel_nh_[ipAddr] = { nh, 1 }; - NextHopKey nhKey(ipAddr, tunnelKey, true /*tunnel_nh*/, 0 /*tag*/); - if (gNeighOrch) - { - gNeighOrch->addIpinipTunnelNextHop(nhKey, nh); - } } return nh; @@ -1550,12 +1545,6 @@ bool MuxOrch::removeNextHopTunnel(std::string tunnelKey, swss::IpAddress& ipAddr tunnelKey.c_str(), ipAddr.to_string().c_str()); } mux_tunnel_nh_.erase(ipAddr); - - NextHopKey nhKey(ipAddr, tunnelKey, true /*tunnel_nh*/, 0 /*tag*/); - if (gNeighOrch) - { - gNeighOrch->removeIpinipTunnelNextHop(nhKey); - } } SWSS_LOG_INFO("NH tunnel removed %s, ip %s or decremented to ref count %d", diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index 46b8413c705..0b1c3982223 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -2045,63 +2045,6 @@ bool NeighOrch::removeTunnelNextHop(const NextHopKey& nh) return true; } -bool NeighOrch::addIpinipTunnelNextHop(const NextHopKey& nh, sai_object_id_t nh_id) -{ - SWSS_LOG_ENTER(); - - if (!nh.isTunnelNextHop()) - { - SWSS_LOG_ERROR("NextHopKey is not a tunnel NH: %s", nh.to_string().c_str()); - return false; - } - - if (nh_id == SAI_NULL_OBJECT_ID) - { - SWSS_LOG_ERROR("Invalid SAI OID for tunnel NH %s", nh.to_string().c_str()); - return false; - } - - if (m_syncdNextHops.find(nh) != m_syncdNextHops.end()) - { - SWSS_LOG_INFO("IPinIP tunnel NH already registered: %s", nh.to_string().c_str()); - return true; - } - - NextHopEntry next_hop_entry; - next_hop_entry.next_hop_id = nh_id; - next_hop_entry.ref_count = 0; - next_hop_entry.nh_flags = 0; - m_syncdNextHops[nh] = next_hop_entry; - - SWSS_LOG_NOTICE("Registered IPinIP tunnel NH %s (OID 0x%" PRIx64 ")", - nh.to_string().c_str(), nh_id); - return true; -} - -bool NeighOrch::removeIpinipTunnelNextHop(const NextHopKey& nh) -{ - SWSS_LOG_ENTER(); - - auto it = m_syncdNextHops.find(nh); - if (it == m_syncdNextHops.end()) - { - SWSS_LOG_ERROR("IPinIP tunnel NH not found: %s", nh.to_string().c_str()); - return false; - } - - if (it->second.ref_count > 0) - { - SWSS_LOG_ERROR("Cannot remove still-referenced IPinIP tunnel NH %s (ref_count=%d)", - nh.to_string().c_str(), it->second.ref_count); - return false; - } - - m_syncdNextHops.erase(it); - - SWSS_LOG_NOTICE("Unregistered IPinIP tunnel NH %s", nh.to_string().c_str()); - return true; -} - void NeighOrch::doVoqSystemNeighTask(Consumer &consumer) { SWSS_LOG_ENTER(); diff --git a/orchagent/neighorch.h b/orchagent/neighorch.h index 7d41e9da582..80ef011eff4 100644 --- a/orchagent/neighorch.h +++ b/orchagent/neighorch.h @@ -100,9 +100,6 @@ class NeighOrch : public Orch, public Subject, public Observer sai_object_id_t addTunnelNextHop(const NextHopKey&); bool removeTunnelNextHop(const NextHopKey&); - bool addIpinipTunnelNextHop(const NextHopKey& nh, sai_object_id_t nh_id); - bool removeIpinipTunnelNextHop(const NextHopKey& nh); - bool ifChangeInformNextHop(const string &, bool); bool isNextHopFlagSet(const NextHopKey &, const uint32_t); diff --git a/orchagent/nexthopkey.cpp b/orchagent/nexthopkey.cpp index 5eec31369d0..68707ca37be 100644 --- a/orchagent/nexthopkey.cpp +++ b/orchagent/nexthopkey.cpp @@ -12,7 +12,6 @@ std::size_t hash_value(const NextHopKey& obj) { boost::hash_combine(nh_hash, obj.srv6_segment); boost::hash_combine(nh_hash, obj.srv6_source); boost::hash_combine(nh_hash, obj.srv6_vpn_sid); - boost::hash_combine(nh_hash, obj.tunnel_name); return nh_hash; } diff --git a/orchagent/nexthopkey.h b/orchagent/nexthopkey.h index 1d45695029f..21bc81f741c 100644 --- a/orchagent/nexthopkey.h +++ b/orchagent/nexthopkey.h @@ -18,7 +18,6 @@ extern "C" #define NH_DELIMITER '@' #define NHG_DELIMITER ',' #define VRF_PREFIX "Vrf" -#define TUNNEL_PREFIX "tunnel:" extern IntfsOrch *gIntfsOrch; struct NextHopKey @@ -34,7 +33,6 @@ struct NextHopKey string srv6_segment; // SRV6 segment string string srv6_source; // SRV6 source address string srv6_vpn_sid; // SRV6 vpn sid - string tunnel_name; // IPinIP tunnel name NextHopKey() : weight(0) {} NextHopKey(const std::string &str, const std::string &alias) : @@ -53,20 +51,6 @@ struct NextHopKey std::string err = "Error converting " + str + " to NextHop"; throw std::invalid_argument(err); } - if (str.compare(0, strlen(TUNNEL_PREFIX), TUNNEL_PREFIX) == 0) - { - std::string body = str.substr(strlen(TUNNEL_PREFIX)); - auto keys = tokenize(body, NH_DELIMITER); - if (keys.size() != 2) - { - std::string err = "Error converting " + str + " to tunnel NextHop"; - throw std::invalid_argument(err); - } - tunnel_name = keys[0]; - ip_address = keys[1]; - weight = 0; - return; - } std::string ip_str = parseMplsNextHop(str); auto keys = tokenize(ip_str, NH_DELIMITER); if (keys.size() == 1) @@ -132,18 +116,9 @@ struct NextHopKey NextHopKey(const IpAddress &ip, const MacAddress &mac, const uint32_t &vni, bool overlay_nh) : ip_address(ip), alias(""), vni(vni), mac_address(mac), weight(0){} NextHopKey(const IpAddress &ip, const std::string &alias, const MacAddress &mac, const uint32_t &vni, bool overlay_nh) : ip_address(ip), alias(alias), vni(vni), mac_address(mac), weight(0){} - /* IPinIP tunnel next hop: identified by tunnel name + destination IP. */ - NextHopKey(const IpAddress &ip, const std::string &tnl_name, bool /*tunnel_nh*/, int /*tag*/) : - ip_address(ip), vni(0), mac_address(), weight(0), tunnel_name(tnl_name) {} - const std::string to_string() const { - if (isTunnelNextHop()) - { - return std::string(TUNNEL_PREFIX) + tunnel_name + - NH_DELIMITER + ip_address.to_string(); - } std::string str = formatMplsNextHop(); str += ip_address.to_string() + NH_DELIMITER + alias; return str; @@ -164,8 +139,8 @@ struct NextHopKey bool operator<(const NextHopKey &o) const { - return std::tie(ip_address, alias, label_stack, vni, mac_address, srv6_segment, srv6_source, srv6_vpn_sid, tunnel_name) < - std::tie(o.ip_address, o.alias, o.label_stack, o.vni, o.mac_address, o.srv6_segment, o.srv6_source, o.srv6_vpn_sid, o.tunnel_name); + return std::tie(ip_address, alias, label_stack, vni, mac_address, srv6_segment, srv6_source, srv6_vpn_sid) < + std::tie(o.ip_address, o.alias, o.label_stack, o.vni, o.mac_address, o.srv6_segment, o.srv6_source, o.srv6_vpn_sid); } bool operator==(const NextHopKey &o) const @@ -174,8 +149,7 @@ struct NextHopKey (label_stack == o.label_stack) && (vni == o.vni) && (mac_address == o.mac_address) && (srv6_segment == o.srv6_segment) && (srv6_source == o.srv6_source) && - (srv6_vpn_sid == o.srv6_vpn_sid) && - (tunnel_name == o.tunnel_name); + (srv6_vpn_sid == o.srv6_vpn_sid); } bool operator!=(const NextHopKey &o) const @@ -203,12 +177,6 @@ struct NextHopKey return (srv6_vpn_sid != ""); } - bool isTunnelNextHop() const - { - return (!tunnel_name.empty()); - } - - std::string parseMplsNextHop(const std::string& str) { // parseMplsNextHop initializes MPLS-related member data of the NextHopKey diff --git a/orchagent/tunneldecaporch.cpp b/orchagent/tunneldecaporch.cpp index 3e4b8c77ec2..2d7796b906d 100644 --- a/orchagent/tunneldecaporch.cpp +++ b/orchagent/tunneldecaporch.cpp @@ -3,7 +3,6 @@ #include "tunneldecaporch.h" #include "portsorch.h" #include "crmorch.h" -#include "neighorch.h" #include "logger.h" #include "swssnet.h" #include "qosorch.h" @@ -27,7 +26,6 @@ extern sai_object_id_t gSwitchId; extern PortsOrch* gPortsOrch; extern CrmOrch* gCrmOrch; extern QosOrch* gQosOrch; -extern NeighOrch* gNeighOrch; TunnelDecapOrch::TunnelDecapOrch( DBConnector *appDb, DBConnector *stateDb, @@ -1353,12 +1351,6 @@ sai_object_id_t TunnelDecapOrch::createNextHopTunnel(std::string tunnelKey, IpAd } tunnelNhs[tunnelKey][ipAddr] = { next_hop_id, 1 }; - - NextHopKey nhKey(ipAddr, tunnelKey, true /*tunnel_nh*/, 0 /*tag*/); - if (gNeighOrch) - { - gNeighOrch->addIpinipTunnelNextHop(nhKey, next_hop_id); - } } return next_hop_id; @@ -1422,12 +1414,6 @@ bool TunnelDecapOrch::removeNextHopTunnel(std::string tunnelKey, IpAddress& ipAd tunnelNhs[tunnelKey].erase(ipAddr); - NextHopKey nhKey(ipAddr, tunnelKey, true /*tunnel_nh*/, 0 /*tag*/); - if (gNeighOrch) - { - gNeighOrch->removeIpinipTunnelNextHop(nhKey); - } - return true; } diff --git a/tests/mock_tests/protnhg_ut.cpp b/tests/mock_tests/protnhg_ut.cpp index 06c8d23ef2c..1eff439b99e 100644 --- a/tests/mock_tests/protnhg_ut.cpp +++ b/tests/mock_tests/protnhg_ut.cpp @@ -1049,56 +1049,6 @@ namespace protnhg_test EXPECT_EQ(key_ab, key_ba); } - /* --- IPinIP tunnel NextHopKey tests --- */ - - TEST_F(ProtNhgTest, TunnelNextHopKeyConstructor) - { - IpAddress ip("10.1.0.32"); - NextHopKey nh(ip, string("MuxTunnel0"), true /*tunnel_nh*/, 0 /*tag*/); - - EXPECT_TRUE(nh.isTunnelNextHop()); - EXPECT_EQ(nh.ip_address, ip); - EXPECT_EQ(nh.tunnel_name, "MuxTunnel0"); - EXPECT_EQ(nh.alias, ""); - EXPECT_EQ(nh.vni, 0u); - EXPECT_FALSE(nh.isSrv6NextHop()); - EXPECT_FALSE(nh.isMplsNextHop()); - } - - TEST_F(ProtNhgTest, TunnelNextHopKeyToStringRoundtrip) - { - IpAddress ip("192.168.1.1"); - NextHopKey original(ip, string("IPINIP_TUNNEL"), true /*tunnel_nh*/, 0 /*tag*/); - - string str = original.to_string(); - EXPECT_EQ(str, "tunnel:IPINIP_TUNNEL@192.168.1.1"); - - NextHopKey parsed(str); - EXPECT_TRUE(parsed.isTunnelNextHop()); - EXPECT_EQ(parsed.tunnel_name, "IPINIP_TUNNEL"); - EXPECT_EQ(parsed.ip_address, ip); - EXPECT_EQ(original, parsed); - } - - TEST_F(ProtNhgTest, TunnelNextHopKeyComparison) - { - NextHopKey nh_a(IpAddress("10.0.0.1"), string("TunA"), true, 0); - NextHopKey nh_b(IpAddress("10.0.0.1"), string("TunB"), true, 0); - NextHopKey nh_same(IpAddress("10.0.0.1"), string("TunA"), true, 0); - - EXPECT_EQ(nh_a, nh_same); - EXPECT_NE(nh_a, nh_b); - - NextHopKey regular_nh(IpAddress("10.0.0.1"), string("Ethernet0")); - EXPECT_NE(nh_a, regular_nh); - } - - TEST_F(ProtNhgTest, TunnelNextHopKeyInvalidParseFails) - { - EXPECT_THROW(NextHopKey("tunnel:@10.0.0.1@extra"), std::invalid_argument); - EXPECT_THROW(NextHopKey("tunnel:OnlyName"), std::invalid_argument); - } - /* --- Protection NHG key prefix tests --- */ TEST_F(ProtNhgTest, BuildProtNhgKeyHwPrefix) From f8f822ca8c0f29666260a9f826cb9fb848700ffa Mon Sep 17 00:00:00 2001 From: Manas Kumar Mandal Date: Wed, 3 Jun 2026 22:58:55 -0700 Subject: [PATCH 09/20] publish capability Signed-off-by: Manas Kumar Mandal --- orchagent/nhgorch.cpp | 90 +++++++++++++++++++++------------ orchagent/nhgorch.h | 16 +++++- orchagent/protnhg.h | 12 +++-- orchagent/switchorch.h | 2 + tests/mock_tests/protnhg_ut.cpp | 68 +++++++++---------------- 5 files changed, 107 insertions(+), 81 deletions(-) diff --git a/orchagent/nhgorch.cpp b/orchagent/nhgorch.cpp index fc85e9ec3b4..5dea58853a5 100644 --- a/orchagent/nhgorch.cpp +++ b/orchagent/nhgorch.cpp @@ -3,6 +3,7 @@ #include "crmorch.h" #include "routeorch.h" #include "srv6orch.h" +#include "switchorch.h" #include "bulker.h" #include "logger.h" #include "swssnet.h" @@ -14,6 +15,7 @@ extern NeighOrch *gNeighOrch; extern RouteOrch *gRouteOrch; extern NhgOrch *gNhgOrch; extern Srv6Orch *gSrv6Orch; +extern SwitchOrch *gSwitchOrch; extern size_t gMaxBulkSize; @@ -1163,17 +1165,14 @@ bool NextHopGroup::invalidateNextHop(const NextHopKey& nh_key) /* Protection NHG management APIs */ /* ----------------------------------------------------------------------- */ -bool NhgOrch::isHwProtectionSupported() +void NhgOrch::probeProtectionCapabilities() { - static bool checked = false; - static bool supported = false; - - if (checked) + if (m_protCapChecked) { - return supported; + return; } - checked = true; + m_protCapChecked = true; const auto *meta = sai_metadata_get_attr_metadata( SAI_OBJECT_TYPE_NEXT_HOP_GROUP, @@ -1181,37 +1180,66 @@ bool NhgOrch::isHwProtectionSupported() if (!meta || !meta->isenum) { SWSS_LOG_NOTICE("Cannot query NHG type enum metadata"); - return false; } - - vector values_list(meta->enummetadata->valuescount); - sai_s32_list_t values; - values.count = static_cast(values_list.size()); - values.list = values_list.data(); - - sai_status_t 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) + else { - SWSS_LOG_NOTICE("Failed to query NHG type capabilities, rv: %d", status); - return false; + vector values_list(meta->enummetadata->valuescount); + sai_s32_list_t values; + values.count = static_cast(values_list.size()); + values.list = values_list.data(); + + sai_status_t 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) + { + SWSS_LOG_NOTICE("Failed to query NHG type capabilities, rv: %d", status); + } + else + { + for (uint32_t i = 0; i < values.count; i++) + { + if (values.list[i] == SAI_NEXT_HOP_GROUP_TYPE_PROTECTION) + { + m_swProtectionSupported = true; + } + else if (values.list[i] == SAI_NEXT_HOP_GROUP_TYPE_HW_PROTECTION) + { + m_hwProtectionSupported = true; + } + } + } } - for (uint32_t i = 0; i < values.count; i++) + SWSS_LOG_NOTICE("SAI_NEXT_HOP_GROUP_TYPE_PROTECTION is %s; " + "SAI_NEXT_HOP_GROUP_TYPE_HW_PROTECTION is %s", + m_swProtectionSupported ? "supported" : "not supported", + m_hwProtectionSupported ? "supported" : "not supported"); + + /* Publish both capabilities to STATE_DB|SWITCH_CAPABILITY so consumers and + * CLI can discover which protection NHG types are available. */ + if (gSwitchOrch) { - if (values.list[i] == SAI_NEXT_HOP_GROUP_TYPE_HW_PROTECTION) - { - supported = true; - break; - } + gSwitchOrch->set_switch_capability( + { swss::FieldValueTuple(SWITCH_CAPABILITY_TABLE_SW_NHG_PROTECTION_CAPABLE, + m_swProtectionSupported ? "true" : "false"), + swss::FieldValueTuple(SWITCH_CAPABILITY_TABLE_HW_NHG_PROTECTION_CAPABLE, + m_hwProtectionSupported ? "true" : "false") }); } +} - SWSS_LOG_NOTICE("SAI_NEXT_HOP_GROUP_TYPE_HW_PROTECTION is %s", - supported ? "supported" : "not supported"); - return supported; +bool NhgOrch::isSwProtectionSupported() +{ + probeProtectionCapabilities(); + return m_swProtectionSupported; +} + +bool NhgOrch::isHwProtectionSupported() +{ + probeProtectionCapabilities(); + return m_hwProtectionSupported; } bool NhgOrch::createProtNhg(const string &key, diff --git a/orchagent/nhgorch.h b/orchagent/nhgorch.h index 61159ede7e5..6dd04712c6f 100644 --- a/orchagent/nhgorch.h +++ b/orchagent/nhgorch.h @@ -130,7 +130,12 @@ class NhgOrch : public NhgOrchCommon bool validateNextHop(const NextHopKey& nh_key); bool invalidateNextHop(const NextHopKey& nh_key); - /* Check if hardware supports protection NHG type. */ + /* Check if the ASIC supports the SW protection NHG type + * (SAI_NEXT_HOP_GROUP_TYPE_PROTECTION). */ + bool isSwProtectionSupported(); + + /* Check if the ASIC supports the HW protection NHG type + * (SAI_NEXT_HOP_GROUP_TYPE_HW_PROTECTION). */ bool isHwProtectionSupported(); /* @@ -218,6 +223,15 @@ class NhgOrch : public NhgOrchCommon private: void doTask(Consumer& consumer) override; + /* Probe the ASIC once for SW/HW protection NHG support and publish the + * result to STATE_DB|SWITCH_CAPABILITY. Subsequent calls are no-ops. */ + void probeProtectionCapabilities(); + + /* Cached protection-capability probe results. */ + bool m_protCapChecked = false; + bool m_swProtectionSupported = false; + bool m_hwProtectionSupported = false; + /* Storage for protection NHGs, keyed by a string identifier (e.g., port name). */ unordered_map> m_protNhgs; }; diff --git a/orchagent/protnhg.h b/orchagent/protnhg.h index 3403014deb3..da18098e2ca 100644 --- a/orchagent/protnhg.h +++ b/orchagent/protnhg.h @@ -56,11 +56,15 @@ class ProtNhgMember : public NhgMember }; /* - * ProtNhg represents a SAI_NEXT_HOP_GROUP_TYPE_HW_PROTECTION group. + * ProtNhg represents a SAI protection next hop group of either type, selected + * at construction via the hw_protection flag: + * - SAI_NEXT_HOP_GROUP_TYPE_PROTECTION (software protection), or + * - SAI_NEXT_HOP_GROUP_TYPE_HW_PROTECTION (hardware protection). * It has one or more primary next hops and exactly one standby next hop. - * Hardware toggles traffic between the primary set and the standby based on - * the monitored object state. Administrative override is supported via - * SAI_NEXT_HOP_GROUP_ATTR_ADMIN_ROLE. + * For HW protection, the hardware toggles traffic between the primary set and + * the standby based on the monitored object state, with software override via + * SAI_NEXT_HOP_GROUP_ATTR_ADMIN_ROLE. For SW protection, the switchover is + * driven by software via SAI_NEXT_HOP_GROUP_ATTR_SET_SWITCHOVER. */ class ProtNhg : public NhgCommon { diff --git a/orchagent/switchorch.h b/orchagent/switchorch.h index d55bd72f8b1..3dafc07df66 100644 --- a/orchagent/switchorch.h +++ b/orchagent/switchorch.h @@ -20,6 +20,8 @@ #define SWITCH_CAPABILITY_TABLE_PORT_EGRESS_SAMPLE_CAPABLE "PORT_EGRESS_SAMPLE_CAPABLE" #define SWITCH_CAPABILITY_TABLE_PATH_TRACING_CAPABLE "PATH_TRACING_CAPABLE" #define SWITCH_CAPABILITY_TABLE_ICMP_OFFLOAD_CAPABLE "ICMP_OFFLOAD_CAPABLE" +#define SWITCH_CAPABILITY_TABLE_SW_NHG_PROTECTION_CAPABLE "SW_NHG_PROTECTION_CAPABLE" +#define SWITCH_CAPABILITY_TABLE_HW_NHG_PROTECTION_CAPABLE "HW_NHG_PROTECTION_CAPABLE" #define ASIC_SDK_HEALTH_EVENT_ELIMINATE_INTERVAL 3600 #define SWITCH_CAPABILITY_TABLE_ASIC_SDK_HEALTH_EVENT_CAPABLE "ASIC_SDK_HEALTH_EVENT" diff --git a/tests/mock_tests/protnhg_ut.cpp b/tests/mock_tests/protnhg_ut.cpp index 1eff439b99e..cbb6d7498af 100644 --- a/tests/mock_tests/protnhg_ut.cpp +++ b/tests/mock_tests/protnhg_ut.cpp @@ -1098,51 +1098,6 @@ namespace protnhg_test EXPECT_NE(hw_key, sw_key); } - /* --- Tunnel NH in a protection NHG --- */ - - TEST_F(ProtNhgTest, CreateProtNhgWithTunnelStandby) - { - NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); - NextHopKey standby_nh(IpAddress("10.1.0.32"), string("MuxTunnel0"), - true /*tunnel_nh*/, 0 /*tag*/); - vector primaries = {primary_nh}; - - registerNextHop(primary_nh); - registerNextHop(standby_nh, 0xBEEF); - - string key = "prot_tunnel_standby"; - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); - EXPECT_TRUE(gNhgOrch->hasProtNhg(key)); - EXPECT_NE(gNhgOrch->getProtNhgId(key), SAI_NULL_OBJECT_ID); - - ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); - EXPECT_FALSE(gNhgOrch->hasProtNhg(key)); - - unregisterNextHop(primary_nh); - unregisterNextHop(standby_nh); - } - - TEST_F(ProtNhgTest, AutoKeyWithTunnelNextHop) - { - NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); - NextHopKey standby_nh(IpAddress("10.1.0.32"), string("MuxTunnel0"), - true /*tunnel_nh*/, 0 /*tag*/); - vector primaries = {primary_nh}; - - registerNextHop(primary_nh); - registerNextHop(standby_nh, 0xBEEF); - - string expected_key = NhgOrch::buildProtNhgKey(primaries, standby_nh, true); - EXPECT_NE(expected_key.find("tunnel:MuxTunnel0"), string::npos); - - ASSERT_TRUE(gNhgOrch->createProtNhg(primaries, standby_nh)); - EXPECT_TRUE(gNhgOrch->hasProtNhg(expected_key)); - - ASSERT_TRUE(gNhgOrch->removeProtNhg(expected_key)); - unregisterNextHop(primary_nh); - unregisterNextHop(standby_nh); - } - TEST_F(ProtNhgTest, RecursiveMemberResolvesViaNhgOrch) { NextHopGroupKey primary_nhg_key("10.0.0.1@Ethernet0,10.0.0.2@Ethernet0"); @@ -1169,4 +1124,27 @@ namespace protnhg_test removeEcmpNhg(primary_nhg_key.to_string()); removeEcmpNhg(standby_nhg_key.to_string()); } + + /* --- Protection capabilities are published to STATE_DB --- */ + + TEST_F(ProtNhgTest, ProtectionCapabilitiesPublishedToStateDb) + { + /* Probing either capability must publish both SW and HW protection + * capability fields to the standard switch capability row, + * regardless of whether the platform supports them. */ + bool hw_supported = gNhgOrch->isHwProtectionSupported(); + bool sw_supported = gNhgOrch->isSwProtectionSupported(); + + string hw_val; + gSwitchOrch->get_switch_capability( + SWITCH_CAPABILITY_TABLE_HW_NHG_PROTECTION_CAPABLE, hw_val); + EXPECT_FALSE(hw_val.empty()); + EXPECT_EQ(hw_val, hw_supported ? "true" : "false"); + + string sw_val; + gSwitchOrch->get_switch_capability( + SWITCH_CAPABILITY_TABLE_SW_NHG_PROTECTION_CAPABLE, sw_val); + EXPECT_FALSE(sw_val.empty()); + EXPECT_EQ(sw_val, sw_supported ? "true" : "false"); + } } From 713de426655aa89a15a35521bdf866b48fdeb868 Mon Sep 17 00:00:00 2001 From: Manas Kumar Mandal Date: Wed, 3 Jun 2026 23:14:11 -0700 Subject: [PATCH 10/20] CRM NHG members Signed-off-by: Manas Kumar Mandal --- orchagent/protnhg.cpp | 6 +++++ tests/mock_tests/protnhg_ut.cpp | 45 +++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/orchagent/protnhg.cpp b/orchagent/protnhg.cpp index a5d11700861..6fbf70e1911 100644 --- a/orchagent/protnhg.cpp +++ b/orchagent/protnhg.cpp @@ -475,6 +475,12 @@ bool ProtNhg::syncMembers(const set &member_keys) bulker.flush(); + /* + * Go through the synced members and, for the successful ones, call sync() + * which records the SAI member ID and increments the CRM_NEXTHOP_GROUP_MEMBER + * ref count (via NhgMember::sync()). The matching decrement happens in + * NhgMember::remove() through ProtNhg::remove() -> NhgCommon::removeMembers(). + */ bool success = true; for (const auto &entry : syncing) { diff --git a/tests/mock_tests/protnhg_ut.cpp b/tests/mock_tests/protnhg_ut.cpp index cbb6d7498af..63934c2dbf3 100644 --- a/tests/mock_tests/protnhg_ut.cpp +++ b/tests/mock_tests/protnhg_ut.cpp @@ -21,6 +21,7 @@ #include "mock_orch_test.h" #include "nhgorch.h" #include "protnhg.h" +#include "portal.h" #include "gtest/gtest.h" @@ -58,6 +59,18 @@ namespace protnhg_test gNeighOrch->m_syncdNextHops.erase(nh); } + /* Sum the CRM "used" counter across all keys for a resource type. */ + static uint32_t crmUsed(CrmResourceType type) + { + uint32_t count = 0; + const auto &resourceMap = Portal::CrmOrchInternal::getResourceMap(gCrmOrch); + for (const auto &kv : resourceMap.at(type).countersMap) + { + count += kv.second.usedCounter; + } + return count; + } + class ProtNhgTest : public MockOrchTest { protected: @@ -1147,4 +1160,36 @@ namespace protnhg_test EXPECT_FALSE(sw_val.empty()); EXPECT_EQ(sw_val, sw_supported ? "true" : "false"); } + + /* --- CRM resource accounting --- */ + + TEST_F(ProtNhgTest, CrmAccountingOnCreateAndRemove) + { + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + vector primaries = {primary_nh}; + + uint32_t grp_before = crmUsed(CrmResourceType::CRM_NEXTHOP_GROUP); + uint32_t mbr_before = crmUsed(CrmResourceType::CRM_NEXTHOP_GROUP_MEMBER); + + string key = "prot_crm"; + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + + /* One protection group with two synced members (1 primary + 1 standby). */ + EXPECT_EQ(crmUsed(CrmResourceType::CRM_NEXTHOP_GROUP), grp_before + 1); + EXPECT_EQ(crmUsed(CrmResourceType::CRM_NEXTHOP_GROUP_MEMBER), mbr_before + 2); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + + /* Removal must release both the group and the member counters. */ + EXPECT_EQ(crmUsed(CrmResourceType::CRM_NEXTHOP_GROUP), grp_before); + EXPECT_EQ(crmUsed(CrmResourceType::CRM_NEXTHOP_GROUP_MEMBER), mbr_before); + + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); + } } From d52fb49137eca0a947224df01a71dee8325b4a5e Mon Sep 17 00:00:00 2001 From: Manas Kumar Mandal Date: Thu, 4 Jun 2026 00:09:20 -0700 Subject: [PATCH 11/20] Removed vector of primary NHs. Signed-off-by: Manas Kumar Mandal --- orchagent/nhgorch.cpp | 46 ++------- orchagent/nhgorch.h | 11 ++- orchagent/protnhg.cpp | 8 +- orchagent/protnhg.h | 13 ++- tests/mock_tests/protnhg_ut.cpp | 163 +++++++------------------------- 5 files changed, 61 insertions(+), 180 deletions(-) diff --git a/orchagent/nhgorch.cpp b/orchagent/nhgorch.cpp index 5dea58853a5..c683956e782 100644 --- a/orchagent/nhgorch.cpp +++ b/orchagent/nhgorch.cpp @@ -1243,18 +1243,12 @@ bool NhgOrch::isHwProtectionSupported() } bool NhgOrch::createProtNhg(const string &key, - const vector &primary_nhs, + const NextHopKey &primary_nh, const NextHopKey &standby_nh, bool hw_protection) { SWSS_LOG_ENTER(); - if (primary_nhs.empty()) - { - SWSS_LOG_ERROR("Protection NHG %s requires at least one primary NH", key.c_str()); - return false; - } - if (m_protNhgs.find(key) != m_protNhgs.end()) { SWSS_LOG_INFO("Protection NHG %s already exists", key.c_str()); @@ -1269,7 +1263,7 @@ bool NhgOrch::createProtNhg(const string &key, return false; } - auto nhg = make_unique(key, primary_nhs, standby_nh, hw_protection); + auto nhg = make_unique(key, primary_nh, standby_nh, hw_protection); if (!nhg->sync()) { @@ -1279,42 +1273,20 @@ bool NhgOrch::createProtNhg(const string &key, m_protNhgs.emplace(key, NhgEntry(move(nhg))); - string primary_str; - for (const auto &nh : primary_nhs) - { - if (!primary_str.empty()) - { - primary_str += ", "; - } - primary_str += nh.to_string(); - } - - SWSS_LOG_NOTICE("Created protection NHG %s (primaries: [%s], standby: %s)", + SWSS_LOG_NOTICE("Created protection NHG %s (primary: %s, standby: %s)", key.c_str(), - primary_str.c_str(), + primary_nh.to_string().c_str(), standby_nh.to_string().c_str()); return true; } -string NhgOrch::buildProtNhgKey(const vector &primary_nhs, +string NhgOrch::buildProtNhgKey(const NextHopKey &primary_nh, const NextHopKey &standby_nh, bool hw_protection) { string prefix = hw_protection ? "prot:hw:" : "prot:sw:"; - - set sorted(primary_nhs.begin(), primary_nhs.end()); - string primary_str; - for (auto it = sorted.begin(); it != sorted.end(); ++it) - { - if (it != sorted.begin()) - { - primary_str += NHG_DELIMITER; - } - primary_str += it->to_string(); - } - - return prefix + primary_str + "|" + standby_nh.to_string(); + return prefix + primary_nh.to_string() + "|" + standby_nh.to_string(); } string NhgOrch::buildProtNhgKey(const NextHopGroupKey &primary_nhg_key, @@ -1325,12 +1297,12 @@ string NhgOrch::buildProtNhgKey(const NextHopGroupKey &primary_nhg_key, return prefix + primary_nhg_key.to_string() + "|" + standby_nhg_key.to_string(); } -bool NhgOrch::createProtNhg(const vector &primary_nhs, +bool NhgOrch::createProtNhg(const NextHopKey &primary_nh, const NextHopKey &standby_nh, bool hw_protection) { - return createProtNhg(buildProtNhgKey(primary_nhs, standby_nh, hw_protection), - primary_nhs, standby_nh, hw_protection); + return createProtNhg(buildProtNhgKey(primary_nh, standby_nh, hw_protection), + primary_nh, standby_nh, hw_protection); } bool NhgOrch::createProtNhg(const NextHopGroupKey &primary_nhg_key, diff --git a/orchagent/nhgorch.h b/orchagent/nhgorch.h index 6dd04712c6f..4a88c9f69e5 100644 --- a/orchagent/nhgorch.h +++ b/orchagent/nhgorch.h @@ -149,16 +149,17 @@ class NhgOrch : public NhgOrchCommon * must removeProtNhg() first. */ - /* Create a protection NHG with one or more primary and one standby next hop. - * Individual NHs are resolved via NeighOrch at sync time. + /* Create a protection NHG as a strict pair: one primary and one standby + * next hop. Individual NHs are resolved via NeighOrch at sync time. + * For N:M, use the recursive NextHopGroupKey-pair overloads below. */ bool createProtNhg(const string &key, - const vector &primary_nhs, + const NextHopKey &primary_nh, const NextHopKey &standby_nh, bool hw_protection = true); /* Auto-keyed convenience overload -- key is derived from the members. */ - bool createProtNhg(const vector &primary_nhs, + bool createProtNhg(const NextHopKey &primary_nh, const NextHopKey &standby_nh, bool hw_protection = true); @@ -176,7 +177,7 @@ class NhgOrch : public NhgOrchCommon bool hw_protection = true); /* Build the deterministic key for a protection NHG from its members. */ - static string buildProtNhgKey(const vector &primary_nhs, + static string buildProtNhgKey(const NextHopKey &primary_nh, const NextHopKey &standby_nh, bool hw_protection = true); static string buildProtNhgKey(const NextHopGroupKey &primary_nhg_key, diff --git a/orchagent/protnhg.cpp b/orchagent/protnhg.cpp index 6fbf70e1911..b92e7ffdc96 100644 --- a/orchagent/protnhg.cpp +++ b/orchagent/protnhg.cpp @@ -158,7 +158,7 @@ string ProtNhgMember::to_string() const /* ----------------------------------------------------------------------- */ ProtNhg::ProtNhg(const string &key, - const vector &primary_nhs, + const NextHopKey &primary_nh, const NextHopKey &standby_nh, bool hw_protection) : NhgCommon(key), @@ -166,10 +166,8 @@ ProtNhg::ProtNhg(const string &key, { SWSS_LOG_ENTER(); - for (const auto &nh : primary_nhs) - { - m_members.emplace(nh, ProtNhgMember(nh, ProtNhgRole::PRIMARY)); - } + m_members.emplace(primary_nh, + ProtNhgMember(primary_nh, ProtNhgRole::PRIMARY)); m_members.emplace(standby_nh, ProtNhgMember(standby_nh, ProtNhgRole::STANDBY)); } diff --git a/orchagent/protnhg.h b/orchagent/protnhg.h index da18098e2ca..bef4dd01c5e 100644 --- a/orchagent/protnhg.h +++ b/orchagent/protnhg.h @@ -60,17 +60,22 @@ class ProtNhgMember : public NhgMember * at construction via the hw_protection flag: * - SAI_NEXT_HOP_GROUP_TYPE_PROTECTION (software protection), or * - SAI_NEXT_HOP_GROUP_TYPE_HW_PROTECTION (hardware protection). - * It has one or more primary next hops and exactly one standby next hop. - * For HW protection, the hardware toggles traffic between the primary set and - * the standby based on the monitored object state, with software override via + * It is a strict pair: exactly one primary next hop and exactly one standby + * next hop, matching the SAI protection-group model (a primary-backup pair). + * For HW protection, the hardware toggles traffic between the primary and the + * standby based on the monitored object state, with software override via * SAI_NEXT_HOP_GROUP_ATTR_ADMIN_ROLE. For SW protection, the switchover is * driven by software via SAI_NEXT_HOP_GROUP_ATTR_SET_SWITCHOVER. + * + * Multi-primary (N:M) is expressed via the recursive NextHopGroupKey-pair + * constructor, where the primary/standby members each point to an ECMP (or + * fine-grained) NHG resolved through NhgOrch. */ class ProtNhg : public NhgCommon { public: ProtNhg(const string &key, - const vector &primary_nhs, + const NextHopKey &primary_nh, const NextHopKey &standby_nh, bool hw_protection = true); diff --git a/tests/mock_tests/protnhg_ut.cpp b/tests/mock_tests/protnhg_ut.cpp index 63934c2dbf3..30537e51f03 100644 --- a/tests/mock_tests/protnhg_ut.cpp +++ b/tests/mock_tests/protnhg_ut.cpp @@ -157,9 +157,7 @@ namespace protnhg_test registerNextHop(primary_nh); registerNextHop(standby_nh); - vector primaries = {primary_nh}; - - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); EXPECT_TRUE(gNhgOrch->hasProtNhg(key)); EXPECT_NE(gNhgOrch->getProtNhgId(key), SAI_NULL_OBJECT_ID); @@ -176,28 +174,17 @@ namespace protnhg_test string key = "prot_nhg_dup"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); - EXPECT_FALSE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); + EXPECT_FALSE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); unregisterNextHop(primary_nh); unregisterNextHop(standby_nh); } - TEST_F(ProtNhgTest, CreateProtNhgEmptyPrimariesFails) - { - NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries; - - EXPECT_FALSE(gNhgOrch->createProtNhg("empty", primaries, standby_nh)); - EXPECT_FALSE(gNhgOrch->hasProtNhg("empty")); - } - TEST_F(ProtNhgTest, RemoveNonExistentProtNhgFails) { EXPECT_FALSE(gNhgOrch->removeProtNhg("does_not_exist")); @@ -208,12 +195,10 @@ namespace protnhg_test string key = "prot_nhg_ref"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); gNhgOrch->incProtNhgRefCount(key); EXPECT_FALSE(gNhgOrch->removeProtNhg(key)); @@ -232,12 +217,10 @@ namespace protnhg_test string key = "prot_nhg_members"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); const ProtNhg &nhg = gNhgOrch->getProtNhg(key); EXPECT_NE(nhg.getId(), SAI_NULL_OBJECT_ID); @@ -255,41 +238,15 @@ namespace protnhg_test unregisterNextHop(standby_nh); } - TEST_F(ProtNhgTest, MultiplePrimaryMembers) - { - string key = "prot_nhg_multi"; - NextHopKey primary1(IpAddress("10.0.0.1"), string("Ethernet0")); - NextHopKey primary2(IpAddress("10.0.0.2"), string("Ethernet0")); - NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary1, primary2}; - - registerNextHop(primary1); - registerNextHop(primary2); - registerNextHop(standby_nh); - - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); - - const ProtNhg &nhg = gNhgOrch->getProtNhg(key); - auto primary_out = nhg.getPrimaryMembers(); - EXPECT_EQ(primary_out.size(), 2u); - - ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); - unregisterNextHop(primary1); - unregisterNextHop(primary2); - unregisterNextHop(standby_nh); - } - TEST_F(ProtNhgTest, SetAdminRole) { string key = "prot_nhg_admin"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); EXPECT_CALL(*mock_sai_next_hop_group_api, set_next_hop_group_attribute(_, _)) @@ -315,12 +272,10 @@ namespace protnhg_test string key = "prot_nhg_admin_fail"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); EXPECT_CALL(*mock_sai_next_hop_group_api, set_next_hop_group_attribute(_, _)) @@ -341,13 +296,11 @@ namespace protnhg_test NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); sai_object_id_t session_oid = 0xABCD; - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); EXPECT_TRUE(gNhgOrch->setProtNhgMonitoredObject(key, standby_nh, session_oid)); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); @@ -367,12 +320,10 @@ namespace protnhg_test NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); NextHopKey unknown_nh(IpAddress("10.0.0.99"), string("Ethernet0")); - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); EXPECT_FALSE(gNhgOrch->setProtNhgMonitoredObject(key, unknown_nh, 0xABCD)); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); @@ -402,12 +353,10 @@ namespace protnhg_test string key = "prot_nhg_sai_fail"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); - EXPECT_FALSE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + EXPECT_FALSE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); EXPECT_FALSE(gNhgOrch->hasProtNhg(key)); unregisterNextHop(primary_nh); @@ -425,12 +374,10 @@ namespace protnhg_test string key = "prot_nhg_double_sync"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); auto &nhg = const_cast(gNhgOrch->getProtNhg(key)); EXPECT_TRUE(nhg.sync()); @@ -460,12 +407,10 @@ namespace protnhg_test string key = "prot_nhg_sync_mbr_fail"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); - EXPECT_FALSE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + EXPECT_FALSE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); EXPECT_FALSE(gNhgOrch->hasProtNhg(key)); unregisterNextHop(primary_nh); @@ -476,9 +421,7 @@ namespace protnhg_test { NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - - ProtNhg nhg("unsynced_nhg", primaries, standby_nh); + ProtNhg nhg("unsynced_nhg", primary_nh, standby_nh); EXPECT_FALSE(nhg.setAdminRole( SAI_NEXT_HOP_GROUP_MEMBER_CONFIGURED_ROLE_PRIMARY)); } @@ -487,9 +430,7 @@ namespace protnhg_test { NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - - ProtNhg nhg("unsynced_mon", primaries, standby_nh); + ProtNhg nhg("unsynced_mon", primary_nh, standby_nh); EXPECT_TRUE(nhg.updateMemberMonitoredObject(standby_nh, 0xABCD)); } @@ -498,12 +439,10 @@ namespace protnhg_test string key = "prot_nhg_mon_sai_fail"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); EXPECT_CALL(*mock_sai_next_hop_group_api, set_next_hop_group_member_attribute(_, _)) @@ -521,11 +460,9 @@ namespace protnhg_test string key = "prot_nhg_obs_unsynced"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - registerNextHop(standby_nh); - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); sai_next_hop_group_member_observed_role_t role; EXPECT_FALSE(gNhgOrch->getProtNhgMemberObservedRole( @@ -540,12 +477,10 @@ namespace protnhg_test string key = "prot_nhg_obs_ok"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); auto old_get_fn = ut_sai_next_hop_group_api.get_next_hop_group_member_attribute; @@ -573,12 +508,10 @@ namespace protnhg_test string key = "prot_nhg_obs_fail"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); auto old_get_fn = ut_sai_next_hop_group_api.get_next_hop_group_member_attribute; @@ -605,12 +538,10 @@ namespace protnhg_test NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); NextHopKey unknown_nh(IpAddress("10.0.0.99"), string("Ethernet0")); - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); sai_next_hop_group_member_observed_role_t role; EXPECT_FALSE(gNhgOrch->getProtNhgMemberObservedRole( @@ -626,12 +557,10 @@ namespace protnhg_test string key = "prot_nhg_all_obs"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); auto old_get_fn = ut_sai_next_hop_group_api.get_next_hop_group_member_attribute; @@ -658,12 +587,10 @@ namespace protnhg_test string key = "prot_nhg_all_obs_fail"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); auto old_get_fn = ut_sai_next_hop_group_api.get_next_hop_group_member_attribute; @@ -689,12 +616,10 @@ namespace protnhg_test string key = "prot_nhg_remove_fail"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); EXPECT_CALL(*mock_sai_next_hop_group_api, remove_next_hop_group(_)) @@ -714,12 +639,10 @@ namespace protnhg_test string key = "prot_nhg_inline"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); const ProtNhg &nhg = gNhgOrch->getProtNhg(key); EXPECT_FALSE(nhg.isTemp()); @@ -736,12 +659,10 @@ namespace protnhg_test string key = "prot_nhg_mbr_str"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); const ProtNhg &nhg = gNhgOrch->getProtNhg(key); const ProtNhgMember *standby = nhg.getStandbyMember(); @@ -774,9 +695,7 @@ namespace protnhg_test { NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - - ProtNhg nhg1("nhg_move", primaries, standby_nh); + ProtNhg nhg1("nhg_move", primary_nh, standby_nh); ProtNhg nhg2(std::move(nhg1)); EXPECT_NE(nhg2.getStandbyMember(), nullptr); @@ -788,12 +707,10 @@ namespace protnhg_test NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); sai_object_id_t session_oid = 0xABCD; - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); - ProtNhg nhg("nhg_mon_sync", primaries, standby_nh); + ProtNhg nhg("nhg_mon_sync", primary_nh, standby_nh); EXPECT_TRUE(nhg.updateMemberMonitoredObject(standby_nh, session_oid)); EXPECT_TRUE(nhg.sync()); @@ -903,8 +820,6 @@ namespace protnhg_test string key = "prot_nhg_sw"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); @@ -925,7 +840,7 @@ namespace protnhg_test return SAI_STATUS_SUCCESS; }); - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh, false)); EXPECT_TRUE(gNhgOrch->hasProtNhg(key)); @@ -939,12 +854,10 @@ namespace protnhg_test string key = "prot_nhg_admin_prot"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh, false)); EXPECT_FALSE(gNhgOrch->setProtNhgAdminRole( @@ -960,12 +873,10 @@ namespace protnhg_test string key = "prot_nhg_switchover"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh, false)); EXPECT_CALL(*mock_sai_next_hop_group_api, @@ -985,12 +896,10 @@ namespace protnhg_test string key = "prot_nhg_switchover_hw"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh, + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh, true)); EXPECT_FALSE(gNhgOrch->setProtNhgSwitchover(key, true)); @@ -1009,18 +918,16 @@ namespace protnhg_test { NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); - vector primaries = {primary_nh}; - registerNextHop(primary_nh); registerNextHop(standby_nh); - string expected_key = NhgOrch::buildProtNhgKey(primaries, standby_nh); + string expected_key = NhgOrch::buildProtNhgKey(primary_nh, standby_nh); EXPECT_FALSE(expected_key.empty()); - ASSERT_TRUE(gNhgOrch->createProtNhg(primaries, standby_nh)); + ASSERT_TRUE(gNhgOrch->createProtNhg(primary_nh, standby_nh)); EXPECT_TRUE(gNhgOrch->hasProtNhg(expected_key)); - EXPECT_FALSE(gNhgOrch->createProtNhg(primaries, standby_nh)); + EXPECT_FALSE(gNhgOrch->createProtNhg(primary_nh, standby_nh)); ASSERT_TRUE(gNhgOrch->removeProtNhg(expected_key)); EXPECT_FALSE(gNhgOrch->hasProtNhg(expected_key)); @@ -1171,13 +1078,11 @@ namespace protnhg_test registerNextHop(primary_nh); registerNextHop(standby_nh); - vector primaries = {primary_nh}; - uint32_t grp_before = crmUsed(CrmResourceType::CRM_NEXTHOP_GROUP); uint32_t mbr_before = crmUsed(CrmResourceType::CRM_NEXTHOP_GROUP_MEMBER); string key = "prot_crm"; - ASSERT_TRUE(gNhgOrch->createProtNhg(key, primaries, standby_nh)); + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); /* One protection group with two synced members (1 primary + 1 standby). */ EXPECT_EQ(crmUsed(CrmResourceType::CRM_NEXTHOP_GROUP), grp_before + 1); From 80f224a82cc050fad89c5c011659a7f43e78f8f6 Mon Sep 17 00:00:00 2001 From: Manas Kumar Mandal Date: Tue, 16 Jun 2026 14:54:41 -0700 Subject: [PATCH 12/20] Triggering CI pipeline Signed-off-by: Manas Kumar Mandal From 3060ca4a42ab044cacde072f99d577c33a43f482 Mon Sep 17 00:00:00 2001 From: Manas Kumar Mandal Date: Tue, 16 Jun 2026 16:34:21 -0700 Subject: [PATCH 13/20] fix the ut compilation errors Signed-off-by: Manas Kumar Mandal --- tests/mock_tests/protnhg_ut.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/mock_tests/protnhg_ut.cpp b/tests/mock_tests/protnhg_ut.cpp index 30537e51f03..a285fad8bd2 100644 --- a/tests/mock_tests/protnhg_ut.cpp +++ b/tests/mock_tests/protnhg_ut.cpp @@ -957,16 +957,16 @@ namespace protnhg_test removeEcmpNhg(standby_nhg_key.to_string()); } - TEST_F(ProtNhgTest, BuildProtNhgKeySortsPrimaries) + TEST_F(ProtNhgTest, BuildProtNhgKeyDiffersByPrimary) { NextHopKey nh_a(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey nh_b(IpAddress("10.0.0.2"), string("Ethernet0")); NextHopKey standby(IpAddress("10.0.0.100"), string("Ethernet4")); - string key_ab = NhgOrch::buildProtNhgKey({nh_a, nh_b}, standby); - string key_ba = NhgOrch::buildProtNhgKey({nh_b, nh_a}, standby); + string key_ab = NhgOrch::buildProtNhgKey(nh_a, standby); + string key_ba = NhgOrch::buildProtNhgKey(nh_b, standby); - EXPECT_EQ(key_ab, key_ba); + EXPECT_NE(key_ab, key_ba); } /* --- Protection NHG key prefix tests --- */ @@ -976,7 +976,7 @@ namespace protnhg_test NextHopKey primary(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby(IpAddress("10.0.0.100"), string("Ethernet4")); - string key = NhgOrch::buildProtNhgKey({primary}, standby, true); + string key = NhgOrch::buildProtNhgKey(primary, standby, true); EXPECT_EQ(key.substr(0, 8), "prot:hw:"); EXPECT_NE(key.find("10.0.0.1@Ethernet0"), string::npos); } @@ -986,7 +986,7 @@ namespace protnhg_test NextHopKey primary(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby(IpAddress("10.0.0.100"), string("Ethernet4")); - string key = NhgOrch::buildProtNhgKey({primary}, standby, false); + string key = NhgOrch::buildProtNhgKey(primary, standby, false); EXPECT_EQ(key.substr(0, 8), "prot:sw:"); } @@ -1013,8 +1013,8 @@ namespace protnhg_test NextHopKey primary(IpAddress("10.0.0.1"), string("Ethernet0")); NextHopKey standby(IpAddress("10.0.0.100"), string("Ethernet4")); - string hw_key = NhgOrch::buildProtNhgKey({primary}, standby, true); - string sw_key = NhgOrch::buildProtNhgKey({primary}, standby, false); + string hw_key = NhgOrch::buildProtNhgKey(primary, standby, true); + string sw_key = NhgOrch::buildProtNhgKey(primary, standby, false); EXPECT_NE(hw_key, sw_key); } From 7ab87bc5edeed0b359fa48f3c03d23b65ce57920 Mon Sep 17 00:00:00 2001 From: Manas Kumar Mandal Date: Wed, 17 Jun 2026 15:19:11 -0700 Subject: [PATCH 14/20] Triggering AZP run Signed-off-by: Manas Kumar Mandal From c672ac9a878e5e15f53c64380e5b762dc8f918fa Mon Sep 17 00:00:00 2001 From: Manas Kumar Mandal Date: Thu, 18 Jun 2026 11:24:06 -0700 Subject: [PATCH 15/20] Fix mock test expectations for idempotent create Signed-off-by: Manas Kumar Mandal --- tests/mock_tests/protnhg_ut.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/mock_tests/protnhg_ut.cpp b/tests/mock_tests/protnhg_ut.cpp index a285fad8bd2..90a468ac78e 100644 --- a/tests/mock_tests/protnhg_ut.cpp +++ b/tests/mock_tests/protnhg_ut.cpp @@ -169,7 +169,7 @@ namespace protnhg_test unregisterNextHop(standby_nh); } - TEST_F(ProtNhgTest, CreateDuplicateProtNhgFails) + TEST_F(ProtNhgTest, CreateDuplicateProtNhgIsIdempotent) { string key = "prot_nhg_dup"; NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); @@ -178,7 +178,7 @@ namespace protnhg_test registerNextHop(standby_nh); ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); - EXPECT_FALSE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); + EXPECT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); unregisterNextHop(primary_nh); @@ -927,7 +927,7 @@ namespace protnhg_test ASSERT_TRUE(gNhgOrch->createProtNhg(primary_nh, standby_nh)); EXPECT_TRUE(gNhgOrch->hasProtNhg(expected_key)); - EXPECT_FALSE(gNhgOrch->createProtNhg(primary_nh, standby_nh)); + EXPECT_TRUE(gNhgOrch->createProtNhg(primary_nh, standby_nh)); ASSERT_TRUE(gNhgOrch->removeProtNhg(expected_key)); EXPECT_FALSE(gNhgOrch->hasProtNhg(expected_key)); From 9c7af18aca735e29fc09408d9463acb0d7c58380 Mon Sep 17 00:00:00 2001 From: Manas Kumar Mandal Date: Wed, 5 Aug 2026 18:47:38 -0700 Subject: [PATCH 16/20] Triggering AZP run Signed-off-by: Manas Kumar Mandal From 9397237abdc1bfaefe2620d286737080083670ac Mon Sep 17 00:00:00 2001 From: Manas Kumar Mandal Date: Wed, 5 Aug 2026 20:33:59 -0700 Subject: [PATCH 17/20] Triggering AZP run Signed-off-by: Manas Kumar Mandal From 4a1d0908d79d2c9dc7061f0cd176e9cb17fbac9b Mon Sep 17 00:00:00 2001 From: Manas Kumar Mandal Date: Mon, 10 Aug 2026 15:56:44 -0700 Subject: [PATCH 18/20] Addressed Tamer's comments Signed-off-by: Manas Kumar Mandal --- orchagent/neighorch.cpp | 14 +++ orchagent/nhgorch.cpp | 76 ++++++++++- orchagent/nhgorch.h | 9 ++ orchagent/protnhg.cpp | 65 ++++++++-- orchagent/protnhg.h | 9 +- tests/mock_tests/protnhg_ut.cpp | 215 ++++++++++++++++++++++++++++++-- 6 files changed, 364 insertions(+), 24 deletions(-) diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index 7ee60a95803..7e3342e68ba 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -477,6 +477,13 @@ bool NeighOrch::addNextHop(NeighborContext& ctx) gFgNhgOrch->validNextHopInNextHopGroup(nexthop); + /* Sync any group member(s) that were skipped for lack of this next hop. */ + if (!gNhgOrch->validateNextHop(nexthop)) + { + SWSS_LOG_WARN("Failed to validate next hop %s in next hop group(s)", + nexthop.to_string().c_str()); + } + // For nexthop with incoming port which has down oper status, NHFLAGS_IFDOWN // flag should be set on it. // This scenario may happen under race condition where buffered neighbor event @@ -569,6 +576,13 @@ bool NeighOrch::processBulkAddNextHop(NeighborContext& ctx) gFgNhgOrch->validNextHopInNextHopGroup(nexthop); + /* Sync any group member(s) that were skipped for lack of this next hop. */ + if (!gNhgOrch->validateNextHop(nexthop)) + { + SWSS_LOG_WARN("Failed to validate next hop %s in next hop group(s)", + nexthop.to_string().c_str()); + } + // For nexthop with incoming port which has down oper status, NHFLAGS_IFDOWN // flag should be set on it. // This scenario may happen under race condition where buffered neighbor event diff --git a/orchagent/nhgorch.cpp b/orchagent/nhgorch.cpp index 6f55b6eaa40..adaa9b41730 100644 --- a/orchagent/nhgorch.cpp +++ b/orchagent/nhgorch.cpp @@ -487,6 +487,23 @@ bool NhgOrch::validateNextHop(const NextHopKey& nh_key) } } + /* Also validate the next hop in any protection groups containing it. */ + for (auto& it : m_protNhgs) + { + auto& nhg = it.second.nhg; + + if (nhg->hasMember(nh_key)) + { + if (!nhg->validateNextHop(nh_key)) + { + SWSS_LOG_ERROR("Failed to validate next hop %s in protection group %s", + nh_key.to_string().c_str(), + it.first.c_str()); + return false; + } + } + } + return true; } @@ -524,6 +541,23 @@ bool NhgOrch::invalidateNextHop(const NextHopKey& nh_key) } } + /* Also invalidate the next hop in any protection groups containing it. */ + for (auto& it : m_protNhgs) + { + auto& nhg = it.second.nhg; + + if (nhg->hasMember(nh_key)) + { + if (!nhg->invalidateNextHop(nh_key)) + { + SWSS_LOG_WARN("Failed to invalidate next hop %s from protection group %s", + nh_key.to_string().c_str(), + it.first.c_str()); + return false; + } + } + } + return true; } @@ -1258,6 +1292,14 @@ bool NhgOrch::createProtNhg(const string &key, return true; } + if (!(hw_protection ? isHwProtectionSupported() : isSwProtectionSupported())) + { + SWSS_LOG_ERROR("%s NHG protection not supported by ASIC, cannot create " + "protection NHG %s", + hw_protection ? "Hardware" : "Software", key.c_str()); + return false; + } + if (gRouteOrch->getNhgCount() + NhgBase::getSyncedCount() >= gRouteOrch->getMaxNhgCount()) { @@ -1268,12 +1310,24 @@ bool NhgOrch::createProtNhg(const string &key, auto nhg = make_unique(key, primary_nh, standby_nh, hw_protection); - if (!nhg->sync()) + bool synced = nhg->sync(); + + /* The SAI group itself is what makes the NHG usable/registerable; a + * member left unresolved doesn't fail creation, it self-heals later + * via validateNextHop(). */ + if (!nhg->isSynced()) { SWSS_LOG_ERROR("Failed to sync protection NHG %s", key.c_str()); return false; } + if (!synced) + { + SWSS_LOG_WARN("Protection NHG %s created with unresolved member(s); " + "will complete once the next hop(s) are validated", + key.c_str()); + } + m_protNhgs.emplace(key, NhgEntry(move(nhg))); SWSS_LOG_NOTICE("Created protection NHG %s (primary: %s, standby: %s)", @@ -1344,6 +1398,14 @@ bool NhgOrch::createProtNhg(const string &key, return true; } + if (!(hw_protection ? isHwProtectionSupported() : isSwProtectionSupported())) + { + SWSS_LOG_ERROR("%s NHG protection not supported by ASIC, cannot create " + "protection NHG %s", + hw_protection ? "Hardware" : "Software", key.c_str()); + return false; + } + string primary_key_str = primary_nhg_key.to_string(); string standby_key_str = standby_nhg_key.to_string(); @@ -1372,12 +1434,22 @@ bool NhgOrch::createProtNhg(const string &key, auto nhg = make_unique(key, primary_nhg_key, standby_nhg_key, hw_protection); - if (!nhg->sync()) + bool synced = nhg->sync(); + + /* See the individual-NH overload above. */ + if (!nhg->isSynced()) { SWSS_LOG_ERROR("Failed to sync protection NHG %s", key.c_str()); return false; } + if (!synced) + { + SWSS_LOG_WARN("Protection NHG %s created with unresolved member(s); " + "will complete once the next hop(s) are validated", + key.c_str()); + } + m_protNhgs.emplace(key, NhgEntry(move(nhg))); SWSS_LOG_NOTICE("Created protection NHG %s (primary NHG: %s, standby NHG: %s)", diff --git a/orchagent/nhgorch.h b/orchagent/nhgorch.h index 4a88c9f69e5..635ed8b3af9 100644 --- a/orchagent/nhgorch.h +++ b/orchagent/nhgorch.h @@ -147,6 +147,15 @@ class NhgOrch : public NhgOrchCommon * existing canonical key is a no-op that returns true. Membership * is immutable once created -- callers wishing to change membership * must removeProtNhg() first. + * + * Return value reflects registration, not full member sync: a group + * with an unresolved next hop still returns true and self-heals via + * validateNextHop(); query member state directly (e.g. getProtNhg()) + * if that distinction matters to the caller. + * + * Each overload rejects creation up front if the ASIC doesn't support + * the requested hw_protection type (see isHwProtectionSupported() / + * isSwProtectionSupported()). */ /* Create a protection NHG as a strict pair: one primary and one standby diff --git a/orchagent/protnhg.cpp b/orchagent/protnhg.cpp index b92e7ffdc96..f97799fedd0 100644 --- a/orchagent/protnhg.cpp +++ b/orchagent/protnhg.cpp @@ -208,10 +208,30 @@ bool ProtNhg::sync() return true; } - if (m_members.size() < 2) + /* Count by role: a primary/standby NextHopKey collision would silently + * collapse m_members to a single entry (emplace() no-ops on duplicate + * keys), which size() alone wouldn't catch. */ + size_t primary_count = 0; + size_t standby_count = 0; + + for (const auto &mbr : m_members) { - SWSS_LOG_ERROR("Protection NHG %s must have at least 2 members, has %zu", - m_key.c_str(), m_members.size()); + if (mbr.second.getRole() == ProtNhgRole::PRIMARY) + { + ++primary_count; + } + else + { + ++standby_count; + } + } + + if (primary_count != 1 || standby_count != 1) + { + SWSS_LOG_ERROR("Protection NHG %s must have exactly one primary and one " + "standby member, has %zu primary and %zu standby " + "(primary and standby next hop may be identical)", + m_key.c_str(), primary_count, standby_count); return false; } @@ -267,6 +287,32 @@ bool ProtNhg::remove() return NhgCommon::remove(); } +/* Sync the member for nh_key once its next hop is resolved. */ +bool ProtNhg::validateNextHop(const NextHopKey &nh_key) +{ + SWSS_LOG_ENTER(); + + if (!isSynced()) + { + return true; + } + + return syncMembers({nh_key}); +} + +/* Remove the member for nh_key once its next hop is no longer valid. */ +bool ProtNhg::invalidateNextHop(const NextHopKey &nh_key) +{ + SWSS_LOG_ENTER(); + + if (!isSynced()) + { + return true; + } + + return removeMembers({nh_key}); +} + bool ProtNhg::setAdminRole(sai_int32_t admin_role) { SWSS_LOG_ENTER(); @@ -361,20 +407,19 @@ bool ProtNhg::updateMemberMonitoredObject(const NextHopKey &nh_key, return it->second.updateMonitoredObject(monitored_oid); } -vector ProtNhg::getPrimaryMembers() const +const ProtNhgMember* ProtNhg::getPrimaryMember() const { SWSS_LOG_ENTER(); - vector primaries; for (const auto &mbr : m_members) { if (mbr.second.getRole() == ProtNhgRole::PRIMARY) { - primaries.push_back(&mbr.second); + return &mbr.second; } } - return primaries; + return nullptr; } const ProtNhgMember* ProtNhg::getStandbyMember() const @@ -449,6 +494,10 @@ bool ProtNhg::syncMembers(const set &member_keys) gMaxBulkSize); map syncing; + /* Unresolved members are skipped but still fail this call, so the + * caller knows to retry them later via validateNextHop(). */ + bool success = true; + for (const auto &nh_key : member_keys) { ProtNhgMember &nhgm = m_members.at(nh_key); @@ -462,6 +511,7 @@ bool ProtNhg::syncMembers(const set &member_keys) { SWSS_LOG_WARN("Next hop %s not resolved for protection NHG %s", nh_key.to_string().c_str(), m_key.c_str()); + success = false; continue; } @@ -479,7 +529,6 @@ bool ProtNhg::syncMembers(const set &member_keys) * ref count (via NhgMember::sync()). The matching decrement happens in * NhgMember::remove() through ProtNhg::remove() -> NhgCommon::removeMembers(). */ - bool success = true; for (const auto &entry : syncing) { if (entry.second == SAI_NULL_OBJECT_ID) diff --git a/orchagent/protnhg.h b/orchagent/protnhg.h index bef4dd01c5e..518152e23d8 100644 --- a/orchagent/protnhg.h +++ b/orchagent/protnhg.h @@ -62,6 +62,7 @@ class ProtNhgMember : public NhgMember * - SAI_NEXT_HOP_GROUP_TYPE_HW_PROTECTION (hardware protection). * It is a strict pair: exactly one primary next hop and exactly one standby * next hop, matching the SAI protection-group model (a primary-backup pair). + * Enforced in sync(), not just by convention. * For HW protection, the hardware toggles traffic between the primary and the * standby based on the monitored object state, with software override via * SAI_NEXT_HOP_GROUP_ATTR_ADMIN_ROLE. For SW protection, the switchover is @@ -96,6 +97,12 @@ class ProtNhg : public NhgCommon bool sync() override; bool remove() override; + /* Sync a member once its next hop becomes valid. */ + bool validateNextHop(const NextHopKey &nh_key); + + /* Remove a member once its next hop becomes invalid. */ + bool invalidateNextHop(const NextHopKey &nh_key); + inline bool isTemp() const override { return false; } inline NextHopGroupKey getNhgKey() const override { return {}; } @@ -107,7 +114,7 @@ class ProtNhg : public NhgCommon bool updateMemberMonitoredObject(const NextHopKey &nh_key, sai_object_id_t monitored_oid); - vector getPrimaryMembers() const; + const ProtNhgMember* getPrimaryMember() const; const ProtNhgMember* getStandbyMember() const; /* Query a specific member's observed role from SAI. */ diff --git a/tests/mock_tests/protnhg_ut.cpp b/tests/mock_tests/protnhg_ut.cpp index 90a468ac78e..75013d524f7 100644 --- a/tests/mock_tests/protnhg_ut.cpp +++ b/tests/mock_tests/protnhg_ut.cpp @@ -15,11 +15,15 @@ #include "neighorch.h" #undef private +/* Must precede mock_orchagent_main.h, which includes nhgorch.h unguarded; + * #pragma once would otherwise discard this override. */ +#define private public +#include "nhgorch.h" +#undef private #include "mock_orchagent_main.h" #include "mock_sai_api.h" #include "mock_orch_test.h" -#include "nhgorch.h" #include "protnhg.h" #include "portal.h" @@ -139,6 +143,14 @@ namespace protnhg_test } return SAI_STATUS_SUCCESS; }); + + /* Force protection-capability support by default so tests don't + * depend on the real SAI metadata capability query, which isn't + * meaningful against a mock switch. Tests exercising the probe + * or the unsupported-type path override this explicitly. */ + gNhgOrch->m_protCapChecked = true; + gNhgOrch->m_hwProtectionSupported = true; + gNhgOrch->m_swProtectionSupported = true; } void PreTearDown() override @@ -229,9 +241,9 @@ namespace protnhg_test ASSERT_NE(standby, nullptr); EXPECT_EQ(standby->getRole(), ProtNhgRole::STANDBY); - auto primary_out = nhg.getPrimaryMembers(); - ASSERT_EQ(primary_out.size(), 1u); - EXPECT_EQ(primary_out[0]->getRole(), ProtNhgRole::PRIMARY); + const ProtNhgMember *primary = nhg.getPrimaryMember(); + ASSERT_NE(primary, nullptr); + EXPECT_EQ(primary->getRole(), ProtNhgRole::PRIMARY); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); unregisterNextHop(primary_nh); @@ -387,6 +399,25 @@ namespace protnhg_test unregisterNextHop(standby_nh); } + TEST_F(ProtNhgTest, SyncFailsWhenPrimaryAndStandbyAreIdentical) + { + NextHopKey nh(IpAddress("10.0.0.1"), string("Ethernet0")); + registerNextHop(nh); + + /* + * Primary and standby resolve to the same NextHopKey, so the + * constructor's second m_members.emplace() is a no-op (map keys + * must be unique) and the group ends up with a single member. + * sync() must reject this rather than silently creating a + * degenerate 1-member "protection" group. + */ + ProtNhg nhg("prot_nhg_dup_nh", nh, nh); + EXPECT_FALSE(nhg.sync()); + EXPECT_FALSE(nhg.isSynced()); + + unregisterNextHop(nh); + } + TEST_F(ProtNhgTest, SyncMembersFailure) { EXPECT_CALL(*mock_sai_next_hop_group_api, @@ -410,9 +441,94 @@ namespace protnhg_test registerNextHop(primary_nh); registerNextHop(standby_nh); - EXPECT_FALSE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); - EXPECT_FALSE(gNhgOrch->hasProtNhg(key)); + /* + * The group's SAI object was created successfully, only the members + * failed to sync. createProtNhg() still reports success since the + * group is registered; it isn't stuck forever, as a later + * validateNextHop() call can complete it. + */ + EXPECT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); + EXPECT_TRUE(gNhgOrch->hasProtNhg(key)); + EXPECT_NE(gNhgOrch->getProtNhgId(key), SAI_NULL_OBJECT_ID); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); + } + + TEST_F(ProtNhgTest, UnresolvedMemberSyncedLaterViaValidateNextHop) + { + string key = "prot_nhg_unresolved"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + + /* Only the primary's next hop is resolved; the standby's next hop + * hasn't been learned by NeighOrch yet (e.g. ARP still pending). */ + registerNextHop(primary_nh); + + EXPECT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); + ASSERT_TRUE(gNhgOrch->hasProtNhg(key)); + + const ProtNhg &nhg = gNhgOrch->getProtNhg(key); + EXPECT_NE(nhg.getId(), SAI_NULL_OBJECT_ID); + + /* Both members must still be tracked -- the unresolved standby must + * not have been dropped from the group (Tamer's "zero members" + * concern from the original review). */ + EXPECT_EQ(nhg.getSize(), 2u); + + const ProtNhgMember *primary = nhg.getPrimaryMember(); + ASSERT_NE(primary, nullptr); + EXPECT_TRUE(primary->isSynced()); + + const ProtNhgMember *standby = nhg.getStandbyMember(); + ASSERT_NE(standby, nullptr); + EXPECT_FALSE(standby->isSynced()); + + /* The standby's next hop resolves; NeighOrch::addNextHop() would + * call gNhgOrch->validateNextHop() at this point. */ + registerNextHop(standby_nh); + EXPECT_TRUE(gNhgOrch->validateNextHop(standby_nh)); + + standby = nhg.getStandbyMember(); + ASSERT_NE(standby, nullptr); + EXPECT_TRUE(standby->isSynced()); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); + } + + TEST_F(ProtNhgTest, ValidateNextHopNoOpWhenMemberNotFound) + { + NextHopKey unrelated_nh(IpAddress("10.0.0.200"), string("Ethernet8")); + EXPECT_TRUE(gNhgOrch->validateNextHop(unrelated_nh)); + EXPECT_TRUE(gNhgOrch->invalidateNextHop(unrelated_nh)); + } + + TEST_F(ProtNhgTest, InvalidateNextHopRemovesMemberAndValidateRestoresIt) + { + string key = "prot_nhg_invalidate"; + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); + + const ProtNhg &nhg = gNhgOrch->getProtNhg(key); + ASSERT_NE(nhg.getStandbyMember(), nullptr); + ASSERT_TRUE(nhg.getStandbyMember()->isSynced()); + + /* e.g. the standby's interface goes down. */ + EXPECT_TRUE(gNhgOrch->invalidateNextHop(standby_nh)); + EXPECT_FALSE(nhg.getStandbyMember()->isSynced()); + /* e.g. the standby's interface comes back up. */ + EXPECT_TRUE(gNhgOrch->validateNextHop(standby_nh)); + EXPECT_TRUE(nhg.getStandbyMember()->isSynced()); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); unregisterNextHop(primary_nh); unregisterNextHop(standby_nh); } @@ -671,9 +787,9 @@ namespace protnhg_test EXPECT_FALSE(sstr.empty()); EXPECT_NE(sstr.find("standby"), string::npos); - auto primary_out = nhg.getPrimaryMembers(); - ASSERT_GE(primary_out.size(), 1u); - string pstr = primary_out[0]->to_string(); + const ProtNhgMember *primary = nhg.getPrimaryMember(); + ASSERT_NE(primary, nullptr); + string pstr = primary->to_string(); EXPECT_NE(pstr.find("primary"), string::npos); ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); @@ -815,6 +931,33 @@ namespace protnhg_test empty_standby)); } + TEST_F(ProtNhgTest, CreateProtNhgWithNhgKeysFailsWhenRepresentativesCollide) + { + /* + * The ProtNhg(NextHopGroupKey, NextHopGroupKey) constructor keys each + * member on *nhg_key.getNextHops().begin(), not the full group key. + * Here the primary and standby groups both contain 10.0.0.1@Ethernet0, + * which sorts first in both, so the second m_members.emplace() is a + * no-op and the group would collapse to a single member if sync() + * didn't reject it (see SyncFailsWhenPrimaryAndStandbyAreIdentical for + * the equivalent direct-NextHopKey-overload case). + */ + NextHopGroupKey primary_nhg_key("10.0.0.1@Ethernet0,10.0.0.2@Ethernet0"); + NextHopGroupKey standby_nhg_key("10.0.0.1@Ethernet0,10.0.0.100@Ethernet4"); + + addEcmpNhg(primary_nhg_key); + addEcmpNhg(standby_nhg_key); + + EXPECT_CALL(*mock_sai_next_hop_group_api, create_next_hop_group(_, _, _, _)).Times(0); + + string key = "prot_nhg_keys_rep_collision"; + EXPECT_FALSE(gNhgOrch->createProtNhg(key, primary_nhg_key, standby_nhg_key)); + EXPECT_FALSE(gNhgOrch->hasProtNhg(key)); + + removeEcmpNhg(primary_nhg_key.to_string()); + removeEcmpNhg(standby_nhg_key.to_string()); + } + TEST_F(ProtNhgTest, CreateNonHwProtectionNhg) { string key = "prot_nhg_sw"; @@ -1030,10 +1173,10 @@ namespace protnhg_test ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nhg_key, standby_nhg_key)); const ProtNhg &nhg = gNhgOrch->getProtNhg(key); - auto primaries = nhg.getPrimaryMembers(); - ASSERT_EQ(primaries.size(), 1u); - EXPECT_TRUE(primaries[0]->isRecursive()); - EXPECT_NE(primaries[0]->getNhId(), SAI_NULL_OBJECT_ID); + const ProtNhgMember *primary = nhg.getPrimaryMember(); + ASSERT_NE(primary, nullptr); + EXPECT_TRUE(primary->isRecursive()); + EXPECT_NE(primary->getNhId(), SAI_NULL_OBJECT_ID); const ProtNhgMember *standby = nhg.getStandbyMember(); ASSERT_NE(standby, nullptr); @@ -1049,6 +1192,11 @@ namespace protnhg_test TEST_F(ProtNhgTest, ProtectionCapabilitiesPublishedToStateDb) { + /* Undo PostSetUp()'s forced capability state so this test exercises + * the real probe; it only cares about publish behavior, not the + * specific values a mock switch reports. */ + gNhgOrch->m_protCapChecked = false; + /* Probing either capability must publish both SW and HW protection * capability fields to the standard switch capability row, * regardless of whether the platform supports them. */ @@ -1068,6 +1216,47 @@ namespace protnhg_test EXPECT_EQ(sw_val, sw_supported ? "true" : "false"); } + TEST_F(ProtNhgTest, CreateFailsWhenHwProtectionUnsupported) + { + /* Simulate an ASIC that doesn't support SAI_NEXT_HOP_GROUP_TYPE_HW_PROTECTION. */ + gNhgOrch->m_hwProtectionSupported = false; + + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + /* createProtNhg() must reject the request before ever touching SAI. */ + EXPECT_CALL(*mock_sai_next_hop_group_api, create_next_hop_group(_, _, _, _)).Times(0); + + string key = "prot_nhg_hw_unsupported"; + EXPECT_FALSE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh, /*hw_protection=*/true)); + EXPECT_FALSE(gNhgOrch->hasProtNhg(key)); + + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); + } + + TEST_F(ProtNhgTest, CreateFailsWhenSwProtectionUnsupported) + { + /* Simulate an ASIC that doesn't support SAI_NEXT_HOP_GROUP_TYPE_PROTECTION. */ + gNhgOrch->m_swProtectionSupported = false; + + NextHopKey primary_nh(IpAddress("10.0.0.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.0.100"), string("Ethernet4")); + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + EXPECT_CALL(*mock_sai_next_hop_group_api, create_next_hop_group(_, _, _, _)).Times(0); + + string key = "prot_nhg_sw_unsupported"; + EXPECT_FALSE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh, /*hw_protection=*/false)); + EXPECT_FALSE(gNhgOrch->hasProtNhg(key)); + + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); + } + /* --- CRM resource accounting --- */ TEST_F(ProtNhgTest, CrmAccountingOnCreateAndRemove) From ac192bc48222a8a2e2555f4e32e2d4c8e1329539 Mon Sep 17 00:00:00 2001 From: Manas Kumar Mandal Date: Tue, 11 Aug 2026 22:05:05 -0700 Subject: [PATCH 19/20] Addressed more comments Signed-off-by: Manas Kumar Mandal --- orchagent/neighorch.cpp | 8 ++++---- orchagent/protnhg.cpp | 12 ++++++++++++ tests/mock_tests/protnhg_ut.cpp | 20 ++++++++++++++++++++ 3 files changed, 36 insertions(+), 4 deletions(-) diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index 7e3342e68ba..26f9ad598a6 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -477,8 +477,8 @@ bool NeighOrch::addNextHop(NeighborContext& ctx) gFgNhgOrch->validNextHopInNextHopGroup(nexthop); - /* Sync any group member(s) that were skipped for lack of this next hop. */ - if (!gNhgOrch->validateNextHop(nexthop)) + /* Sync skipped group member(s); gNhgOrch may be null in unit tests. */ + if (gNhgOrch && !gNhgOrch->validateNextHop(nexthop)) { SWSS_LOG_WARN("Failed to validate next hop %s in next hop group(s)", nexthop.to_string().c_str()); @@ -576,8 +576,8 @@ bool NeighOrch::processBulkAddNextHop(NeighborContext& ctx) gFgNhgOrch->validNextHopInNextHopGroup(nexthop); - /* Sync any group member(s) that were skipped for lack of this next hop. */ - if (!gNhgOrch->validateNextHop(nexthop)) + /* Sync skipped group member(s); gNhgOrch may be null in unit tests. */ + if (gNhgOrch && !gNhgOrch->validateNextHop(nexthop)) { SWSS_LOG_WARN("Failed to validate next hop %s in next hop group(s)", nexthop.to_string().c_str()); diff --git a/orchagent/protnhg.cpp b/orchagent/protnhg.cpp index f97799fedd0..67fb178e8f5 100644 --- a/orchagent/protnhg.cpp +++ b/orchagent/protnhg.cpp @@ -297,6 +297,12 @@ bool ProtNhg::validateNextHop(const NextHopKey &nh_key) return true; } + /* syncMembers() assumes nh_key is already in m_members. */ + if (!hasMember(nh_key)) + { + return true; + } + return syncMembers({nh_key}); } @@ -310,6 +316,12 @@ bool ProtNhg::invalidateNextHop(const NextHopKey &nh_key) return true; } + /* removeMembers() (NhgCommon) assumes nh_key is already in m_members. */ + if (!hasMember(nh_key)) + { + return true; + } + return removeMembers({nh_key}); } diff --git a/tests/mock_tests/protnhg_ut.cpp b/tests/mock_tests/protnhg_ut.cpp index 75013d524f7..a716f4204b3 100644 --- a/tests/mock_tests/protnhg_ut.cpp +++ b/tests/mock_tests/protnhg_ut.cpp @@ -502,8 +502,28 @@ namespace protnhg_test TEST_F(ProtNhgTest, ValidateNextHopNoOpWhenMemberNotFound) { NextHopKey unrelated_nh(IpAddress("10.0.0.200"), string("Ethernet8")); + + /* No group exists yet, so this doesn't exercise ProtNhg's own guard. */ EXPECT_TRUE(gNhgOrch->validateNextHop(unrelated_nh)); EXPECT_TRUE(gNhgOrch->invalidateNextHop(unrelated_nh)); + + /* Call directly, bypassing NhgOrch's guard, to test ProtNhg's own. */ + string key = "prot_nhg_unrelated_member"; + NextHopKey primary_nh(IpAddress("10.0.1.1"), string("Ethernet0")); + NextHopKey standby_nh(IpAddress("10.0.1.100"), string("Ethernet4")); + registerNextHop(primary_nh); + registerNextHop(standby_nh); + + ASSERT_TRUE(gNhgOrch->createProtNhg(key, primary_nh, standby_nh)); + ProtNhg &nhg = *gNhgOrch->m_protNhgs.at(key).nhg; + + ASSERT_FALSE(nhg.hasMember(unrelated_nh)); + EXPECT_TRUE(nhg.validateNextHop(unrelated_nh)); + EXPECT_TRUE(nhg.invalidateNextHop(unrelated_nh)); + + ASSERT_TRUE(gNhgOrch->removeProtNhg(key)); + unregisterNextHop(primary_nh); + unregisterNextHop(standby_nh); } TEST_F(ProtNhgTest, InvalidateNextHopRemovesMemberAndValidateRestoresIt) From 897686b33592762ed578ce99cf15f56843972bd0 Mon Sep 17 00:00:00 2001 From: Manas Kumar Mandal Date: Wed, 12 Aug 2026 10:21:02 -0700 Subject: [PATCH 20/20] Triggering AZP run Signed-off-by: Manas Kumar Mandal