Skip to content

[neighorch]: Retry until inband port is available#4770

Open
Xichen96 wants to merge 7 commits into
sonic-net:masterfrom
Xichen96:dev/xichenlin/neighorch-inband-retry
Open

[neighorch]: Retry until inband port is available#4770
Xichen96 wants to merge 7 commits into
sonic-net:masterfrom
Xichen96:dev/xichenlin/neighorch-inband-retry

Conversation

@Xichen96

@Xichen96 Xichen96 commented Jul 20, 2026

Copy link
Copy Markdown

What I did

NeighOrch previously asserted that an inband port always existed whenever it processed a remote-system-port next hop. This PR converts that boot-order assumption into the normal SWSS retain-and-retry behavior.

Root cause

Remote-system-port next hops must be normalized to the local inband interface. The original implementation called getInbandPort() and then asserted that the returned alias was non-empty.

That assumption has existed since the remote-system-port path was introduced in 2021. It is not guaranteed during initialization: a remote next-hop update can be processed before the local inband port has been configured.

The old behavior was:

remote next-hop event
→ inband port not ready
→ assert
→ terminate orchagent

An unavailable prerequisite is transient state, not an invariant violation.

Fix

Each remote next-hop path now checks the boolean result from getInbandPort():

remote next-hop event
→ inband port not ready
→ return false
→ retain work
→ retry after inband configuration is available

The check covers:

  • normal next-hop creation;
  • bulk next-hop completion;
  • IP next-hop removal;
  • MPLS next-hop removal.

No fallback alias or synthetic success is used. The operation reports retry and leaves the existing state machine responsible for reprocessing it.

Why I did it

SWSS orchagents routinely return false when a required object is not available yet. Treating inband-port boot order the same way avoids a process abort without hiding the missing dependency or programming an object against the wrong interface.

This defect predates the interface deletion and VRF-rehome work. It is kept separate from the inband RIF identity/bookkeeping fix because the two changes address different failure modes:

  • identity/bookkeeping determines which RIF, key, and refcount represent a remote next hop;
  • this PR determines what happens when that required inband interface does not exist yet.

Dependency

This branch is stacked on PR #4769 because that PR adds the bulk normalization path containing the third inband lookup. Until #4769 merges, GitHub's Files changed tab includes the prerequisite commit. The final upstream merge order is #4769 first, then this retry change.

How I verified it

Focused mock coverage configures a remote system port, removes the inband-port designation, and verifies that next-hop operations return false rather than asserting:

  • addNextHop();
  • processBulkAddNextHop();
  • removeNextHop();
  • removeMplsNextHop().

Route and label-route removal coverage also verifies that missing-inband MPLS dependencies are detected before any bulk SAI operation is queued. NhgOrch performs the same preflight before changing group or member state.

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: b7c61bb6e2a9ef0b7af97a003b850b061439fa94
  • Azure PR build: 1171077
  • All architecture, ASAN, Docker, Trixie, Test vstest, and TestAsan vstest lanes passed.
  • CodeQL, Semgrep, DCO, EasyCLA, all review threads, 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

Review follow-up

  • Bulk neighbor creation and both RouteOrch MPLS helper paths now propagate addNextHop() retry before advancing dependent work.
  • Non-bulk and bulk neighbor deletion propagate removeNextHop() retry before refcount/cache progression.
  • MPLS next-hop removal now returns retry instead of asserting when the inband port is unavailable.
  • Route, label-route, and NhgOrch deletion preflight remote MPLS dependencies before submitting SAI operations or changing refcounts.
  • Focused coverage verifies missing-inband add and deletion paths preserve retry state.

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 20, 2026 16:46
@Xichen96
Xichen96 requested a review from prsunny as a code owner July 20, 2026 16:46
@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 updates NeighOrch remote-system-port next-hop handling to follow SWSS’s retain-and-retry model when the local inband port is not yet available during initialization, avoiding an orchagent abort caused by assertions.

Changes:

  • Replace assert(getInbandPort()) with a boolean check and return false (retry) in remote-system-port next-hop add, bulk completion, and remove paths.
  • Normalize remote-system-port next hops to use the inband alias consistently for RIF lookup and interface refcounting.
  • Add focused mock tests to validate inband RIF usage and retry behavior when the inband port is absent.

Reviewed changes

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

File Description
orchagent/neighorch.cpp Converts remote-system-port inband-port dependency from assert to retry semantics; normalizes alias use for RIF/refcounting.
tests/mock_tests/neighorch_ut.cpp Adds unit tests and SAI next-hop mocking to validate inband normalization and retry behavior.

Comment thread orchagent/neighorch.cpp
Comment thread orchagent/neighorch.cpp
Xichen96 added 2 commits July 20, 2026 17:07
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079
Signed-off-by: Xichen96 <lukelin0907@gmail.com>
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 force-pushed the dev/xichenlin/neighorch-inband-retry branch from 3093e9c to 6af04e5 Compare July 20, 2026 17:08
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

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

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

1 similar comment
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

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

1 similar comment
@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 1 comment.

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>
Copilot AI review requested due to automatic review settings July 21, 2026 11: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 6 out of 6 changed files in this pull request and generated 3 comments.

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

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 6 out of 6 changed files in this pull request and generated no new comments.

Check the global inband dependency once per route or next-hop group before beginning removal.

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:13
@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 6 out of 6 changed files in this pull request and generated no new comments.

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:20
@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 7 out of 7 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