[intfsorch]: Retry explicit router interface VRF rehome

[Xichen96]

How to read this two-PR fix

Start here for the externally visible failure: an explicit VRF-changing interface SET could be consumed while old prefixes still existed, so the RIF and its IP2Me routes remained in the old VRF.

Then read PR #4746 for the prerequisite: retaining the SET cannot complete reliably if RouteOrch and NeighOrch can keep attaching new dependencies to the old RIF during the drain.

The required implementation and merge order remains PR #4746 first, then this PR, because this PR extends its explicit addition-side removal checks with a separate pending-VRF predicate.

Until #4746 merges, GitHub's Files changed tab includes that prerequisite commit. This PR's own changes implement the rehome state machine in IntfsOrch and extend the standard local route, neighbor, and next-hop admission checks to the pending-VRF state.

What I did

This PR fixes the lost VRF-transition request by making an explicit vrf_name update use the same retain-and-retry model that whole-interface deletion already uses.

Observed failure

The DHCP relay test configured Vlan1000 in Vrf01, but hardware tracing showed:

CONFIG_DB / intended interface VRF:  Vrf01
ASIC RIF virtual router:             default VRF
192.168.0.1/32 IP2Me route VR:       default VRF
fc02:1000::1/128 IP2Me route VR:     default VRF

The returning DHCP OFFER was addressed to the relay interface in Vrf01. Because the corresponding IP2Me trap route existed in the default virtual router instead, the packet did not reach the relay process.

Root cause 1: a VRF-changing SET was consumed instead of retried

intfmgrd intends a VRF move to occur as a delete/recreate sequence:

remove old prefixes
→ unbind the Linux interface
→ bind it to Vrf01
→ recreate the interface and prefixes

However, its Linux-side checks do not acknowledge orchagent or ASIC completion. getIntfIpCount() == 0 means only that Linux no longer has addresses; APP_DB prefix deletions may still be waiting for IntfsOrch.

There are two additional ordering effects:

1. ProducerStateTable/ConsumerStateTable communicates the latest state of a key. If interface DEL and replacement interface SET occur before the consumer pops that key, the intermediate interface DEL can be represented to orchagent as only:

   Vlan1000 SET vrf_name=Vrf01
   

2. The interface row and prefix rows are different APP_DB keys. When they overlap in IntfsOrch's pending multimap, the bare key sorts first:

   Vlan1000                         SET vrf_name=Vrf01
   Vlan1000:192.168.0.1/21         DEL
   Vlan1000:fc02:1000::1/64        DEL
   

The trace confirmed that the explicit interface SET was processed before the final IPv6 prefix DEL.

At that point the old RIF still had prefixes and could not safely be replaced. The critical bug was an asymmetry between DEL and SET:

Operation	Dependencies remain	Previous behavior
Whole-interface DEL	Yes	Return false; keep the command queued and retry
Explicit VRF-changing interface SET	Yes	Return true; erase the command without moving the RIF

After the later prefix deletions drained, the explicit VRF request was gone. Nothing remained in m_toSync to retry the move, so the RIF and later IP2Me routes stayed in the old virtual router.

This is the central root cause:

Interface deletion was designed to tolerate dependency reordering; explicit RIF VRF rehome was not.

Root cause 2: the old RIF could refill while the move waited

Preserving the VRF SET is necessary but not sufficient. The RIF cannot be removed while routes, neighbors, or next hops still reference it.

The hardware trace observed new references admitted after the VRF transition had already been seen:

New dependency on the old RIF	Count
Standard routes	108
Neighbors	60
Neighbor next hops	60
Total	228

Without a transition fence, the rehome could repeatedly wait for ref_count == 0 while other orchagents continued attaching work to the old RIF.

PR #4746 introduces an explicit removal-state predicate checked only by addition paths. This PR adds a separate pending-VRF predicate and makes those same addition paths check both states.

Fix

The fix turns VRF rehome into an explicit per-interface state machine:

OLD/STABLE
    old RIF is authoritative
        |
        | explicit vrf_name change observed
        v
TRANSITION
    retain VRF SET
    allow old prefix/route/neighbor DELs
    block and retain new prefix/route/neighbor SETs
        |
        | prefixes and RIF references reach zero
        v
RECREATE
    remove old RIF
    create RIF in requested VRF
        |
        v
NEW/STABLE
    release retained additions against new RIF

1. Distinguish explicit VRF intent

APP_DB SET is a partial update. These are not equivalent:

SET vrf_name=Vrf01
SET admin_status=down

Without remembering whether vrf_name was actually present, a later MTU or admin-only update could be mistaken for a request to move the interface back to the default VRF.

The consumer therefore records vrfNameIsExplicit and enters the rehome path only when:

this is the interface key
AND vrf_name was explicitly present
AND requested VRF differs from the current RIF VRF

2. Retain the unresolved VRF SET

When prefixes or references remain:

mark Vlan1000 as pending VRF rehome
→ return false
→ leave the original SET, including its target vrf_name, in m_toSync

m_pendingVrfUpdates is only the transition marker; it does not duplicate the requested VRF. The retained APP_DB tuple remains authoritative.

3. Drain old work and retain new work

During the pending transition:

• Old prefix, route, and neighbor deletions continue using the old RIF.
• New prefix additions return false.
• Standard local route, neighbor, and neighbor-next-hop additions explicitly check isIntfVrfUpdatePending() and return false before obtaining the old RIF ID.
• Those original additions remain queued; no new producer notification is required.

For the processing order discussed during review:

Vlan1000                         SET vrf_name=Vrf01
Vlan1000:192.168.0.1/21         DEL
Vlan1000:192.168.0.1/21         SET   # add replacement prefix

the effective execution becomes:

first pass:
    interface SET → retained; transition fence established
    prefix DEL    → old prefix removed
    prefix SET    → retained because transition is pending

next pass:
    interface SET → old RIF removed; new RIF created in Vrf01
    prefix SET    → added using the new RIF and Vrf01

The multimap may visit SET → DEL → ADD, but dependency checks produce the required semantic order:

delete old state → replace RIF → add new state

4. Recreate safely

The replacement preserves the existing interface state used to create the RIF, including:

• custom RIF MAC;
• loopback action;
• MTU and admin state;
• MPLS state;
• NAT zone;
• proxy ARP and other IntfsEntry state.

Simultaneous subinterface MTU/admin changes are applied during the rehome. A later partial SET without vrf_name does not trigger another move.

Logical VRF/refcount state changes only after the replacement RIF is created successfully. If creation fails:

recreate the old RIF
→ keep the VRF SET queued
→ retry later without aborting the rest of the consumer drain

If SAG is enabled, the task remains pending rather than attempting to migrate shared SAG link-local accounting as part of this targeted fix.

Why I did it

Why not require APP_DB to deliver DEL → SET → ADD?

APP_DB StateTable is a latest desired-state synchronization mechanism, not a durable transactional event log. Preserving one producer's call order is insufficient because:

• same-key updates can be coalesced before a consumer sees them;
• differently keyed interface and prefix rows are sorted by the consumer;
• routes and neighbors are handled by different tables and orchagents;
• retained operations retry in later drain cycles;
• multiple producers run independently;
• restart recovery reconstructs current state rather than replaying the original command sequence.

A FIFO or priority queue inside IntfsOrch would address only one table and could cause head-of-line blocking. It would not order IntfsOrch against RouteOrch or NeighOrch.

A producer-side barrier or Redis transaction would also be insufficient by itself. Atomic publication does not guarantee ordered execution across consumers, retries, dependencies, or restarts.

The fully general solution would require an interface generation or transaction protocol, for example:

interface_generation=42
transition_state=draining

with every dependent route and neighbor carrying the same generation. That would allow orchagent to classify an addition intended for a future interface even if it arrived before the transition signal. Such a design requires coordinated schema and behavior changes across intfmgrd, IntfsOrch, RouteOrch, NeighOrch, producers, and recovery logic.

This PR instead follows the existing SWSS model:

recognize missing prerequisites
→ retain work by returning false
→ allow dependencies to drain
→ retry when local state permits

Boundary of this fix

Once the interface transition is visible:

later additions are retained
old deletions are allowed
retained additions retry on the new RIF

Before any transition signal is visible, an operation naming Vlan1000 must be interpreted against the currently active old interface. Without generation metadata, orchagent cannot know that an early addition belongs to a future Vlan1000.

That means a true ADD(new) → SET transition → DEL(old) inversion cannot be classified perfectly. The old dependency will safely block the move until it receives a deletion; the code does not remove a still-referenced RIF.

The traced hardware failure is within the solved boundary: the explicit VRF SET had already been processed, but the old code did not retain it or establish a transition fence, so 228 later additions were admitted to the old RIF.

This PR also does not reconstruct a prefix DEL that ProducerStateTable may have coalesced with a same-key replacement prefix SET before either operation reached IntfsOrch. A priority queue cannot restore an operation that is no longer present.

How I verified it

Focused mock coverage verifies:

• the explicit VRF SET remains queued across prefix and reference drain;
• same-prefix DEL followed by replacement SET is applied to the new RIF;
• new prefix, local neighbor, and local neighbor-next-hop dependencies are blocked while the move is pending;
• loopback action, MPLS state, custom MAC, and other RIF creation attributes survive replacement;
• simultaneous subinterface VRF/MTU/admin changes are applied;
• later partial updates without vrf_name preserve the current VRF;
• replacement creation failure restores the old RIF and retains the move without aborting the remaining drain pass.

The earlier combined branches containing this implementation completed the amd64, ARM, ASAN, Docker, and Trixie compile stages in Azure builds 1168368 (master) and 1168367 (202605). Their vstest jobs stopped before executing tests because those workers lacked pip3.

An earlier combined candidate, hardware build 1167872, passed traced and production 1x/5x runs and all ten production 10x test bodies. This smaller stacked PR adds state-preservation and rollback hardening after that run, so exact-commit validation is provided by this PR's CI.

Current exact-head master PR build

• Head: ae4666cadab9015ca2533f5f7ce2c2ef380fd490
• Azure PR build: 1171658
• This head is the current-master restack that preserves merged PR [intfsorch]Fix race condition between VRF bind and ip address removal #4758 and its regression coverage.
• The fresh exact-head PR build is in progress. The previous build 1171071 is superseded because its retry could not merge current master after PR [intfsorch]Fix race condition between VRF bind and ip address removal #4758 landed.
• DCO, EasyCLA, and Semgrep are already green on the restacked head; final build, test, and exact-head review results will be recorded when complete.
• No local build was used; validation is PR-build only.

202605 PR-build validation

• Validation-only PR: #4747
• The complete current-202605 stack includes merged PR [intfsorch]Fix race condition between VRF bind and ip address removal #4758 and all six user PRs in final dependency order: [intfsorch]: Fence new RIF dependencies during interface removal #4746, [intfsorch]: Retry loopback action update failures #4766, [neighorch]: Track remote next hops on inband RIF #4769, [neighorch]: Avoid inserting missing bulk neighbors #4772, this PR, then [neighorch]: Retry until inband port is available #4770.
• The target-branch conflict resolution preserves both the [intfsorch]: Retry loopback action update failures #4766 loopback-action retry and this PR's RIF state tracking.
• Exact validation head: d4b0502ecdac76d56fdf063a633045c0fb0de977
• Azure PR build: 1171672 — in progress.
• The Tested for 202605 branch label must remain absent until that full-stack result is complete.

Details if related

• Required prerequisite: PR #4746
• Microsoft ADO: 38514061
• Merge batch 2: merge PR [intfsorch]: Fence new RIF dependencies during interface removal #4746 first, merge/recheck batch-1 PR [intfsorch]: Retry loopback action update failures #4766, then merge this PR.

[mssonicbld]

/azp run

[azure-pipelines]

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

[azure-pipelines]

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

[Copilot]

Pull request overview

This PR extends IntfsOrch’s retry/retention model to explicit physical/subinterface vrf_name updates by fencing new dependencies during a VRF rehome, waiting for local quiescence (prefixes + refcount drain), and then replacing the RIF in the requested VRF. It also updates Route/Neighbor programming paths to honor the new “guarded” RIF lookup and adds mock tests covering the VRF-rehome retry/rollback behavior and attribute preservation.

Changes:

• Add VRF-rehome fencing via m_pendingVrfUpdates and a new getRouterIntfsIdForNewDependency() API to block new dependencies while removal/rehome is pending.
• Update RouteOrch/NeighOrch to treat dependency creation as retryable when the guarded RIF lookup returns SAI_NULL_OBJECT_ID.
• Add focused IntfsOrch mock tests for VRF rehome drain/retry, partial updates, attribute preservation, and rollback on create failure.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/mock_tests/intfsorch_ut.cpp	Adds mock coverage for VRF rehome retry/drain, attribute preservation, partial updates, and rollback behavior.
orchagent/routeorch.cpp	Makes route/NHG programming retry when neighbor NH creation is deferred and uses guarded RIF lookup for interface NHs.
orchagent/neighorch.cpp	Uses guarded RIF lookup for new neighbor/next-hop creation to defer work during interface remove/rehome fences.
orchagent/intfsorch.h	Adds loopback_action to IntfsEntry, declares guarded RIF lookup, and extends setIntf() with an “explicit VRF” flag.
orchagent/intfsorch.cpp	Implements pending VRF-update fencing, VRF-rehome RIF replacement logic, and explicit-field guarding for MPLS/MAC updates.

[mssonicbld]

/azp run

[azure-pipelines]

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

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

[mssonicbld]

/azp run

[azure-pipelines]

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

[Copilot]

Pull request overview

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

[mssonicbld]

/azp run

[azure-pipelines]

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

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

[Xichen96]

/azp run

[azure-pipelines]

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.

[mssonicbld]

/azp run

[azure-pipelines]

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (3)

orchagent/neighorch.cpp:386

• In addNextHop(), after potentially rewriting a remote-system-port next hop to the inband alias (via nexthop.alias = inbp.m_alias), the new pending-removal/VRF-update fence still checks nh.alias and also looks up the RIF using nh.alias. This can fetch/fence the wrong interface (and will be wrong for remote-system-port next hops, which should operate on the rewritten inband alias).

    if (!m_intfsOrch->isRemoteSystemPortIntf(nh.alias) &&
        (m_intfsOrch->isIntfRemovalPending(nh.alias) ||
         m_intfsOrch->isIntfVrfUpdatePending(nh.alias)))
    {
        SWSS_LOG_INFO("Interface %s is pending removal or VRF update", nh.alias.c_str());

orchagent/intfsorch.cpp:1173

• If setIntfLoopbackAction() fails (including retryable SAI failures via handleSaiSetStatus()), the code currently consumes the APP_DB entry anyway, which can permanently drop the intended loopback-action update. Please keep the entry queued for retry when the set fails (consistent with the MAC update retry handling below).

                    /* Set loopback action */
                    if (!loopbackAction.empty())
                    {
                        if (setIntfLoopbackAction(port, loopbackAction))
                        {

tests/mock_tests/intfsorch_ut.cpp:84

• The test fixture resets several globals in SetUp(), but last_loopback_action is not reset. Since tests update last_loopback_action based on the last RIF create attributes, leaving it unchanged can make assertions order-dependent if another test observes the previous value.

            create_rif_count = 0;
            remove_rif_count = 0;
            saw_loopback_action = false;
            fail_next_rif_create = false;

[mssonicbld]

/azp run

[azure-pipelines]

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (3)

orchagent/neighorch.cpp:385

• In NeighOrch::addNextHop(), the interface pending-removal/VRF-update fence and the subsequent RIF lookup are done using nh.alias (the original kernel alias). For remote system-port next hops, nexthop.alias is rewritten to the inband interface, so using nh.alias here can select the wrong RIF and bypass the intended inband-interface fencing. Use nexthop.alias consistently for the fence and getRouterIntfsId() after the potential rewrite, and log the same alias you actually gate on.

    assert(!hasNextHop(nexthop));
    if (!m_intfsOrch->isRemoteSystemPortIntf(nh.alias) &&
        (m_intfsOrch->isIntfRemovalPending(nh.alias) ||
         m_intfsOrch->isIntfVrfUpdatePending(nh.alias)))
    {

orchagent/intfsorch.cpp:1173

• Loopback-action updates are still consumed even if setIntfLoopbackAction() fails (including retryable SAI failures). Since this is a SET path, returning false here should keep the entry in m_toSync for retry; otherwise the desired loopback_action can be lost permanently while m_syncdIntfses may remain stale.

                    /* Set loopback action */
                    if (!loopbackAction.empty())
                    {
                        if (setIntfLoopbackAction(port, loopbackAction))
                        {

tests/mock_tests/intfsorch_ut.cpp:84

• The test fixture resets saw_loopback_action and other globals in SetUp(), but last_loopback_action is not reset. This can make assertions depend on test execution order if a prior test set a different loopback action. Reset last_loopback_action in SetUp() to keep tests hermetic.

            sai_router_intfs_api->create_router_interface = _ut_create_router_interface;
            sai_router_intfs_api->remove_router_interface = _ut_remove_router_interface;
            create_rif_count = 0;
            remove_rif_count = 0;
            saw_loopback_action = false;
            fail_next_rif_create = false;

[mssonicbld]

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}

[mssonicbld]

/azp run

[azure-pipelines]

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

[azure-pipelines]

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

[mssonicbld]

/azp run

[azure-pipelines]

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

orchagent/intfsorch.cpp:1173

• Loopback-action updates are still consumed when setIntfLoopbackAction() indicates a retryable SAI failure (it returns false on task_need_retry). This can permanently drop the desired loopback action and also leaves m_syncdIntfses[alias].loopback_action stale, which can break VRF-rehome state preservation. Please keep the APP_DB entry queued for retry on retryable failures, while still ignoring unsupported values.

                    /* Set loopback action */
                    if (!loopbackAction.empty())
                    {
                        if (setIntfLoopbackAction(port, loopbackAction))
                        {

tests/mock_tests/intfsorch_ut.cpp:84

• Test fixture state is not fully reset in SetUp(): last_loopback_action is a global and can carry over from previous tests, making assertions order-dependent. Reset it alongside the other globals to keep tests hermetic.

            create_rif_count = 0;
            remove_rif_count = 0;
            saw_loopback_action = false;
            fail_next_rif_create = false;

[Xichen96]

Retrying CI once: both amd64 compile jobs in build 1173110 were canceled by the 60-minute agent limit after extended container startup; no code failure was reported.

[azure-pipelines]

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

[mssonicbld]

/azp run

[azure-pipelines]

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