Skip to content

Protection Nexthop group support in nhgorch - #4390

Open
manamand2020 wants to merge 19 commits into
sonic-net:masterfrom
manamand2020:protection_nhg
Open

Protection Nexthop group support in nhgorch#4390
manamand2020 wants to merge 19 commits into
sonic-net:masterfrom
manamand2020:protection_nhg

Conversation

@manamand2020

@manamand2020 manamand2020 commented Mar 25, 2026

Copy link
Copy Markdown
Contributor

What I did
Extended support for protection nexthop group in nhgorch.
Added APIs to create hardware protection and protection nexthop group.
This creates either SAI_NEXT_HOP_GROUP_TYPE_PROTECTION or SAI_NEXT_HOP_GROUP_TYPE_HW_PROTECTION type nexthop group which support only two members primary and a secondary in the NexthopGroup.

ProtNhg (protnhg.h/cpp)
ProtNhg models a SAI protection next hop group -- a group with primary and standby members where traffic is switched between them based on link health monitoring or administrative control.

ProtNhgMember wraps each member with:

A configured role (primary or standby).
An optional monitored object (e.g., BFD/ICMP session OID) that drives hardware failover.
Dynamic NH resolution -- individual NHs resolve via NeighOrch, recursive NHG members resolve via NhgOrch. This is determined by isRecursive() (whether m_nhg_key is set).
Observed role query to read back the hardware's active/inactive decision.

ProtNhg itself:

Supports two SAI types controlled by m_hw_protection: HW_PROTECTION (hardware-driven failover with admin role override) and PROTECTION (software-triggered switchover).
Supports two member forms: individual NextHopKeys, or entire NextHopGroupKeys (recursive/nested NHGs where each protection member points to an ECMP group).
sync() creates the SAI NHG object, then bulk-creates members via syncMembers(). Members whose NHs aren't yet resolved are skipped (can be synced later).
remove() tears down members (decrementing ref counts) then removes the SAI object.

NhgOrch Protection APIs (nhgorch.h/cpp)
NhgOrch owns the lifecycle of protection NHGs in m_protNhgs and exposes a CRUD + control API:

Create: createProtNhg() -- four overloads covering explicit-key vs auto-key, and individual-NH vs recursive-NHG members. Validates capacity, duplicates, and member existence before constructing a ProtNhg and calling sync().

Remove: removeProtNhg() -- refuses if ref_count > 0.
Key generation: buildProtNhgKey() produces a deterministic key from the members with a prot:hw: or prot:sw: prefix to distinguish group types.
Auto-Key examples:
Individual NH members (HW protection):
prot:hw:10.0.0.1@Ethernet0,10.0.0.2@Ethernet4|tunnel:MuxTunnel0@10.1.0.32

Recursive NHG members (SW protection):
prot:sw:10.0.0.1@Ethernet0,10.0.0.2@Ethernet4|10.0.0.3@Ethernet8,10.0.0.4@Ethernet12

Query: hasProtNhg(), getProtNhg(), getProtNhgId().
Control: setProtNhgAdminRole() (HW_PROTECTION only), setProtNhgSwitchover() (PROTECTION only), setProtNhgMonitoredObject().
Observability: getProtNhgMemberObservedRole(), getProtNhgAllObservedRoles().
Ref counting: incProtNhgRefCount() / decProtNhgRefCount() for route/consumer ownership.

SAI specs for protection NHG:
https://github.com/opencomputeproject/SAI/blob/master/doc/SAI-Proposal-FRR.md
https://github.com/opencomputeproject/SAI/blob/master/doc/SAI-Proposal-HW-FRR.md

Why I did it
This is needed to support hardware protection failover of mux port.
HLD:
sonic-net/SONiC#2251
sonic-net/SONiC#2346

How I verified it
Added mock Gtests
Details if related

@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Manas Kumar Mandal <manamand@cisco.com>
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@manamand2020
manamand2020 marked this pull request as ready for review March 25, 2026 20:46
@manamand2020
manamand2020 requested a review from prsunny as a code owner March 25, 2026 20:46
Comment thread orchagent/protnhg.cpp
Comment thread orchagent/protnhg.h
Signed-off-by: Manas Kumar Mandal <manamand@cisco.com>
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

Comment thread orchagent/nhgorch.cpp Outdated
Comment thread orchagent/nhgorch.cpp

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a new “protection next hop group” (SAI SAI_NEXT_HOP_GROUP_TYPE_HW_PROTECTION) implementation and exposes management APIs in NhgOrch, primarily to support dual-ToR/mux hardware failover workflows.

Changes:

  • Introduces ProtNhg / ProtNhgMember classes for creating/removing HW_PROTECTION NHGs, setting admin role, and tracking monitored objects / observed roles.
  • Extends NhgOrch with CRUD + query APIs and reference counting for protection NHGs.
  • Adds new mock unit tests and wires them into the mock test build.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/mock_tests/protnhg_ut.cpp Adds gtest coverage for the new protection NHG APIs (create/remove, refcount, admin role, monitored object, basic negative cases).
tests/mock_tests/Makefile.am Includes the new unit test and links orchagent/protnhg.cpp into the mock test binary.
orchagent/protnhg.h Declares ProtNhgRole, ProtNhgMember, and ProtNhg public interfaces.
orchagent/protnhg.cpp Implements SAI programming for HW_PROTECTION NHGs and member operations (monitored object, observed role queries).
orchagent/nhgorch.h Exposes new public NhgOrch APIs for protection NHG lifecycle and queries.
orchagent/nhgorch.cpp Implements NhgOrch protection NHG management and adds an HW_PROTECTION capability probe helper.
orchagent/Makefile.am Adds protnhg.cpp to orchagent build sources.

Comment thread orchagent/protnhg.cpp Outdated
Comment thread orchagent/protnhg.cpp
Signed-off-by: Manas Kumar Mandal <manamand@cisco.com>
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@tahmed-dev

Copy link
Copy Markdown
Contributor

Bug Fix: ProtNhg /32 route orphaning from neighbor churn

File: orchagent/evpnmhorch.cpp

Three bugs cause ASIC transit forwarding failures:

  1. Observer tears down ProtNhg on every neighbor DEL. EVPN extern_learn neighbors from zebra are periodically reinstalled (NUD_NOARP), causing a create/destroy cycle every ~2 minutes. ProtNhg lifecycle should be tied to ES config, not individual neighbor events.

  2. create_route_entry returns ITEM_ALREADY_EXISTS treated as error. When re-creating the /32 route after a churn cycle, the route still exists from neighorch. The ProtNhg is created but the route still points to the old NH OID. Fix: fall back to set_route_entry_attribute to update the NH.

  3. remove_route_entry return value unchecked. If the ASIC fails to remove the /32 route, it remains with a stale NHG OID while the ProtNhg is destroyed.

+    if (status == SAI_STATUS_ITEM_ALREADY_EXISTS)
+    {
+        status = sai_route_api->set_route_entry_attribute(&route_entry, &route_attr);
+        // Update existing route NH → new ProtNhg
+    }

The orphaned /32 routes (more specific than /24 connected) capture all server-destined transit traffic and forward to invalid NHG OIDs → 100% packet loss on return path.

Signed-off-by: Manas Kumar Mandal <manamand@cisco.com>
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

zjswhhh
zjswhhh previously approved these changes Apr 8, 2026
@prsunny
prsunny requested a review from dgsudharsan April 10, 2026 20:36
@prsunny

prsunny commented Apr 10, 2026

Copy link
Copy Markdown
Collaborator

Bug Fix: ProtNhg /32 route orphaning from neighbor churn

File: orchagent/evpnmhorch.cpp

Three bugs cause ASIC transit forwarding failures:

  1. Observer tears down ProtNhg on every neighbor DEL. EVPN extern_learn neighbors from zebra are periodically reinstalled (NUD_NOARP), causing a create/destroy cycle every ~2 minutes. ProtNhg lifecycle should be tied to ES config, not individual neighbor events.
  2. create_route_entry returns ITEM_ALREADY_EXISTS treated as error. When re-creating the /32 route after a churn cycle, the route still exists from neighorch. The ProtNhg is created but the route still points to the old NH OID. Fix: fall back to set_route_entry_attribute to update the NH.
  3. remove_route_entry return value unchecked. If the ASIC fails to remove the /32 route, it remains with a stale NHG OID while the ProtNhg is destroyed.
+    if (status == SAI_STATUS_ITEM_ALREADY_EXISTS)
+    {
+        status = sai_route_api->set_route_entry_attribute(&route_entry, &route_attr);
+        // Update existing route NH → new ProtNhg
+    }

The orphaned /32 routes (more specific than /24 connected) capture all server-destined transit traffic and forward to invalid NHG OIDs → 100% packet loss on return path.

@tahmed-dev , I see a reference to evpnmhorch.cpp. Could you clarify what needs to be addressed here?

@prsunny

prsunny commented Apr 10, 2026

Copy link
Copy Markdown
Collaborator

@manamand2020 , please provide all details in the description.

@prsunny

prsunny commented Apr 10, 2026

Copy link
Copy Markdown
Collaborator

@prashanth-nexthop , could you re-review/signoff?

@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

Signed-off-by: Manas Kumar Mandal <manamand@cisco.com>
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

@abdosi

abdosi commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

@prashanth-nexthop : please review the comments so that we can do sign-off again

@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Manas Kumar Mandal <manamand@cisco.com>
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Manas Kumar Mandal <manamand@cisco.com>
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Manas Kumar Mandal <manamand@cisco.com>
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Manas Kumar Mandal <manamand@cisco.com>
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Manas Kumar Mandal <manamand@cisco.com>
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Signed-off-by: Manas Kumar Mandal <manamand@cisco.com>
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@manamand2020

Copy link
Copy Markdown
Contributor Author

@prsunny @dgsudharsan do you have any comments on this? You approvals will help merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants