Skip to content

[neighorch]: Track remote next hops on inband RIF#4769

Open
Xichen96 wants to merge 3 commits into
sonic-net:masterfrom
Xichen96:dev/xichenlin/neighorch-remote-inband-rif
Open

[neighorch]: Track remote next hops on inband RIF#4769
Xichen96 wants to merge 3 commits into
sonic-net:masterfrom
Xichen96:dev/xichenlin/neighorch-remote-inband-rif

Conversation

@Xichen96

@Xichen96 Xichen96 commented Jul 20, 2026

Copy link
Copy Markdown

What I did

Remote-system-port next hops are represented through the local inband interface, but NeighOrch previously used two different interface identities for the same object.

Root cause

addNextHop() already rewrote the next-hop key from the remote system port to the inband interface:

remote system port alias
→ local inband alias
→ m_syncdNextHops key

The rest of the bookkeeping did not consistently follow that normalized key:

  • the SAI next hop used the remote system port's RIF;
  • the non-bulk path incremented the remote interface refcount;
  • the bulk completion path stored the original remote key and incremented the remote refcount;
  • removal searched with the inband key but decremented the original remote alias.

This produced three mismatches:

  1. The SAI object could reference a different RIF than the key used to track it.
  2. Normal add/remove updated different interface refcounts.
  3. Bulk add stored a key that the existing removal path could not find.

Fix

This PR makes the normalized inband next-hop key authoritative:

remote alias
→ resolve inband alias once
→ use inband RIF
→ store inband key
→ increment inband refcount
→ remove inband key
→ decrement inband refcount

The changes are limited to remote-system-port next-hop identity and bookkeeping:

  • addNextHop() obtains the RIF from nexthop.alias after remote-to-inband normalization.
  • Normal and bulk completion increment the refcount for nexthop.alias.
  • Bulk completion performs the same remote-to-inband key normalization as the non-bulk path.
  • Removal decrements the refcount for the normalized alias.

Why I did it

The next-hop key, SAI RIF, sync map entry, and interface refcount describe one logical dependency. Using different aliases across those surfaces can leak references, prevent deletion, or leave bulk-created next hops unreachable by the removal path.

This defect predates the interface deletion and VRF-rehome work. It is therefore isolated from PRs #4746 and #4764 rather than being included as incidental hardening.

Scope

This PR does not change boot-order handling when the inband port is not yet available. The existing assertion behavior is handled in a separate follow-up so identity/bookkeeping and retry semantics remain independently reviewable.

How I verified it

Focused mock coverage configures a remote system port and local inband interface, then verifies:

  • the SAI next hop receives the inband RIF ID;
  • the next hop is stored under the inband key;
  • the remote interface refcount is unchanged;
  • the inband interface refcount increments on normal add and balances on removal;
  • bulk completion stores the same inband key and balances the same refcount on removal.

The available single-ASIC hardware testbed cannot exercise remote system ports, so this path is covered with focused unit tests and exact-head CI.

Current exact-head master PR build

  • Head: df387c75cabd5ec325682a1304d1b6ac74882a1a
  • Azure PR build: 1171076
  • All architecture, ASAN, Docker, Trixie, Test vstest, and TestAsan vstest lanes passed.
  • CodeQL, Semgrep, DCO, EasyCLA, and the exact-head Copilot review are clean.
  • No local build was used; validation is PR-build only.

202605 PR-build validation

Details if related

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079
Signed-off-by: Xichen96 <lukelin0907@gmail.com>
@Xichen96
Xichen96 requested a review from prsunny as a code owner July 20, 2026 16:45
Copilot AI review requested due to automatic review settings July 20, 2026 16:45
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@azure-pipelines

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

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 fixes NeighOrch bookkeeping for remote-system-port next hops by making the inband-normalized next-hop identity (alias/key, SAI RIF selection, sync map entry, and interface refcount) consistent across normal add, bulk completion, and removal paths.

Changes:

  • Use the normalized nexthop.alias (inband) to select the RIF and to increment/decrement router-interface refcounts in next-hop add/remove paths.
  • Normalize the key in the bulk completion path so bulk-created next hops are tracked/removable via the same inband key.
  • Add focused mock test coverage validating inband RIF usage, inband-key storage, and correct refcount balancing for both non-bulk and bulk completion.

Reviewed changes

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

File Description
orchagent/neighorch.cpp Normalizes remote-system-port next-hop identity to inband across RIF selection, sync-map keys, and interface refcount updates (including bulk completion).
tests/mock_tests/neighorch_ut.cpp Adds unit test coverage for remote-system-port next-hop normalization (inband RIF, key, and refcount behavior).

Comment thread orchagent/neighorch.cpp
Comment thread orchagent/neighorch.cpp
Comment thread orchagent/neighorch.cpp
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079
Signed-off-by: Xichen96 <lukelin0907@gmail.com>
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

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

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

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

@Xichen96

Copy link
Copy Markdown
Author

/azpw run

@mssonicbld

Copy link
Copy Markdown
Collaborator

⚠️ Notice: /azpw run only runs failed jobs now. If you want to trigger a whole pipline run, please rebase your branch or close and reopen the PR.
💡 Tip: You can also use /azpw retry to retry failed jobs directly.

Retrying failed(or canceled) jobs...

@mssonicbld

Copy link
Copy Markdown
Collaborator

Retrying failed(or canceled) stages in build 1170069:

✅Stage TestAsan:

  • Job vstest: retried.

✅Stage Test:

  • Job vstest: retried.
  • Job vstest: retried.

@Xichen96 Xichen96 closed this Jul 21, 2026
@Xichen96 Xichen96 reopened this Jul 21, 2026
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@azure-pipelines

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

Remote system-port next hops remain on the global inband RIF when their neighbor moves between remote ports.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079
Signed-off-by: Xichen96 <lukelin0907@gmail.com>
Copilot AI review requested due to automatic review settings July 21, 2026 17:19
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

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

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@mssonicbld

Copy link
Copy Markdown
Collaborator

This PR has backport request label(s) for branch(es): 202605, but is missing required test information. Please make sure you tick the tested branch(es) in the Tested branch section and provide test evidence (e.g., 202605: <test result>) in the Test result section as well in your PR description.

---Powered by SONiC BuildBot

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants