[intfsorch]: Retry loopback action update failures by Xichen96 · Pull Request #4766 · sonic-net/sonic-swss

Xichen96 · 2026-07-19T16:56:46Z

What I did

Fixed dynamic loopback_action updates on an existing router interface so retryable SAI failures retain the APP_DB task instead of silently losing the requested action.

Historically, loopback_action was normally supplied while creating a RIF. setIntf() passed it to addRouterIntfs(), which included SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION in the CREATE attributes.

The operation exposing this bug changes an existing RIF dynamically between DROP and FORWARD without recreating it. That path calls setIntfLoopbackAction() from IntfsOrch::doTask().

Before:

setIntfLoopbackAction()
    -> transient SAI SET failure
    -> helper returns false: retry needed
    -> caller ignores false
    -> final m_toSync.erase()
    -> requested action is lost

After:

setIntfLoopbackAction()
    -> transient SAI SET failure
    -> helper returns false
    -> caller skips the final erase
    -> original APP_DB row remains queued
    -> complete update is retried on the next drain

The helper follows the existing IntfsOrch boolean-setter convention: false means retry is required; true means processing is terminal and should not be retried. Therefore an invalid action logs a warning and returns true without issuing a SAI call, allowing unrelated fields in the same row to converge.

The change remains surgical: setIntf() still owns the RIF lifecycle, and setIntfLoopbackAction() remains the single-attribute SAI setter.

Why I did it

A transient SAI resource condition must not silently discard desired state. Dynamic loopback packet-action updates need the same retain-and-retry behavior used by other retryable SAI operations.

Invalid input is different: it cannot converge on a later drain and must not leave the whole row queued forever. For example:

SET Ethernet0
    loopback_action = invalid
    nat_zone        = 7

The invalid action is ignored, while nat_zone=7 is still applied and the task is consumed.

This issue is independent of the interface-removal and VRF-rehome state machines in PRs #4746 and #4764.

How I verified it

Focused mock coverage verifies:

IntfsOrchRetriesLoopbackActionSetFailure
- injects one SAI_STATUS_INSUFFICIENT_RESOURCES result for the loopback-action attribute;
- verifies the task remains in m_toSync;
- retries the drain;
- verifies the task is consumed and SAI_PACKET_ACTION_DROP is applied.
IntfsOrchIgnoresInvalidLoopbackActionField
- submits an invalid action with nat_zone=7;
- verifies no loopback action is sent to SAI;
- verifies the NAT-zone update is applied and the task is consumed.
The test hook creates and removes real RIF objects through the original SAI implementation, intercepts only the loopback-action attribute, and delegates every unrelated RIF SET. It also counts both the failed first SET attempt and the successful retry.

Current head: d1ef1b1

Exact-head Azure PR build: pending.
Exact-head Copilot review: requested.
Previous product head 5481ae09 passed all architecture, ASAN, Docker, Trixie, regular vstest, and ASAN vstest lanes in build 1179543:
https://dev.azure.com/mssonic/be1b070f-be15-4154-aade-b1d3bfb17054/_build/results?buildId=1179543
DCO, EasyCLA, CodeQL, and Semgrep passed on the previous product head.

Details if related

Backport request: 202605.
The current branch has an automated Cherry Pick Conflict_202605 label.
Prior combined 202605 validation passed in PR [202605][intfsorch]: Fence new RIF dependencies during interface removal #4747 / build 1175840, but exact current-head validation must be refreshed before restoring Tested for 202605 branch:
- [202605][intfsorch]: Fence new RIF dependencies during interface removal #4747
- https://dev.azure.com/mssonic/be1b070f-be15-4154-aade-b1d3bfb17054/_build/results?buildId=1175840
Microsoft ADO:
https://msazure.visualstudio.com/One/_workitems/edit/38514061
Merge batch 1.
Recheck PR [intfsorch]: Retry explicit router interface VRF rehome #4764 after this PR merges because both touch the same IntfsOrch paths.

mssonicbld · 2026-07-19T16:56:53Z

/azp run

azure-pipelines · 2026-07-19T16:56:54Z

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

azure-pipelines · 2026-07-19T16:57:03Z

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

Xichen96 · 2026-07-19T16:58:33Z

/azp run

azure-pipelines · 2026-07-19T16:58:41Z

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

Copilot

Pull request overview

This PR improves IntfsOrch’s handling of loopback_action updates from APP_INTF_TABLE: it preserves SET tasks in the consumer queue when a retryable SAI failure occurs while updating the RIF loopback packet action, and it treats invalid loopback_action values as a logged no-op so other fields in the same SET can still be applied.

Changes:

Validate loopback_action early in IntfsOrch::doTask() and ignore invalid values (via getSaiLoopbackAction() warning + skipping the field).
If setting the RIF loopback action fails in a retryable way, keep the SET in m_toSync so it will be retried on the next drain.
Add mock unit tests covering (1) retry behavior on SAI set failure and (2) ignoring invalid loopback_action while still applying other fields (e.g., MTU).

Reviewed changes

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

File	Description
`orchagent/intfsorch.cpp`	Adds `loopback_action` validation and retry behavior so requested loopback action updates aren’t lost on retryable SAI errors.
`tests/mock_tests/intfsorch_ut.cpp`	Adds unit tests to verify retry-on-failure and ignore-invalid-field behavior for `loopback_action`.

mssonicbld · 2026-07-19T17:13:04Z

/azp run

azure-pipelines · 2026-07-19T17:13:14Z

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

Xichen96 · 2026-07-19T17:13:42Z

/azp run

azure-pipelines · 2026-07-19T17:13:51Z

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

Copilot

Pull request overview

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

mssonicbld · 2026-07-19T18:10:27Z

/azp run

azure-pipelines · 2026-07-19T18:10:37Z

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

Copilot

Pull request overview

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

Xichen96 · 2026-07-21T16:56:35Z

/azpw run

mssonicbld · 2026-07-21T16:56:37Z

⚠️ 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 · 2026-07-21T16:56:41Z

Retrying failed(or canceled) stages in build 1169337:

✅Stage TestAsan:

Job vstest: retried.

✅Stage Test:

Job vstest: retried.
Job vstest: retried.

mssonicbld · 2026-07-21T17:15:59Z

/azp run

azure-pipelines · 2026-07-21T17:16:00Z

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

azure-pipelines · 2026-07-21T17:16:10Z

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

azure-pipelines · 2026-07-22T15:33:45Z

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

azure-pipelines · 2026-07-22T15:33:56Z

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

rookie-who · 2026-07-27T16:28:00Z

Hi — this PR has a cherry-pick conflict for the 202605 branch (Cherry Pick Conflict_202605). Could you please rebase or resolve the conflict so we can proceed with the cherry-pick?

If you need help identifying the conflicting changes, let us know. Thanks!

Comment by @vaibhavhd via automated tooling

azure-pipelines · 2026-07-30T17:58:31Z

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

mssonicbld · 2026-07-30T18:22:10Z

The Tested branch section has been ticked and Test result is provided for branch(es): 202605. Added label(s): Tested for 202605 Branch.

_{---Powered by SONiC BuildBot}

Keep false reserved for retryable SAI failures so the caller can retain only tasks that may converge on a later drain. Invalid loopback-action values remain terminal and do not block unrelated fields in the same interface update. 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 · 2026-07-30T18:54:13Z

/azp run

azure-pipelines · 2026-07-30T18:54:26Z

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

Copilot

Pull request overview

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

Comments suppressed due to low confidence (1)

tests/mock_tests/intfsorch_ut.cpp:24

The UT stubs for create_router_interface/remove_router_interface return SAI_STATUS_SUCCESS without setting *router_interface_id or calling the original SAI implementation. With the newly added set_router_interface_attribute hook forwarding to pold_sai_rif_api, port.m_rif_id can remain SAI_NULL_OBJECT_ID and the test may exercise SET behavior against an invalid RIF ID (passing/failing for the wrong reason). Forward create/remove to the original API (or at least initialize *router_interface_id) so loopback-action SETs operate on a valid object.

    bool fail_next_rif_set = false;
    sai_packet_action_t last_loopback_action = SAI_PACKET_ACTION_FORWARD;
    sai_router_interface_api_t *pold_sai_rif_api;
    sai_router_interface_api_t ut_sai_rif_api;

Return success directly for the loopback-action attribute handled by the unit-test hook. Continue delegating every unrelated RIF attribute to the original SAI implementation. 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 · 2026-07-31T05:46:39Z

/azp run

azure-pipelines · 2026-07-31T05:46:51Z

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

Copilot

Pull request overview

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

Suppressed comments (2)

tests/mock_tests/intfsorch_ut.cpp:540

After the first drain, this test asserts saw_loopback_action is false, but that currently just means the mock didn’t record the attempted SET (it only flips on success). Once the mock records attempts, update the assertions to ensure the loopback-action SET was attempted on the first drain (failed + retained) and then attempted again on the second drain (succeeded).

        ASSERT_EQ(consumer->m_toSync.size(), 1u);
        ASSERT_FALSE(saw_loopback_action);

        static_cast<Orch *>(gIntfsOrch)->doTask();

tests/mock_tests/intfsorch_ut.cpp:56

In the SAI set-attribute mock, saw_loopback_action is only set on success. When the first call returns SAI_STATUS_INSUFFICIENT_RESOURCES, the test can’t distinguish “setter wasn’t called” from “setter was called and failed”. Record the attempted action (and mark it as seen) before injecting the failure so the test can assert the retry path was actually exercised.

This issue also appears on line 536 of the same file.

            if (fail_next_rif_set)
            {
                fail_next_rif_set = false;
                return SAI_STATUS_INSUFFICIENT_RESOURCES;
            }

Copilot

Pull request overview

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

Suppressed comments (1)

tests/mock_tests/intfsorch_ut.cpp:42

_ut_set_router_interface_attribute() delegates non-loopback attributes to the original SAI RIF API, but _ut_create_router_interface() / _ut_remove_router_interface() currently just return success without calling through. That means delegated SETs can be executed on a non-existent RIF object (invalid OID), making the hook fragile if any test path triggers other RIF attribute updates.

To keep the “intercept only loopback_action, delegate everything else” model consistent, consider calling through to the original create/remove while still maintaining the counters.

        ++remove_rif_count;
        return SAI_STATUS_SUCCESS;
    }

    sai_status_t _ut_set_router_interface_attribute(

Delegate router-interface creation, removal, and unrelated attribute updates to the original SAI implementation. Count loopback-action SET attempts before failure injection so the retry test proves both attempts occurred. 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 · 2026-08-02T12:08:22Z

/azp run

azure-pipelines · 2026-08-02T12:08:33Z

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

Copilot

Pull request overview

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

Xichen96 mentioned this pull request Jul 19, 2026

[intfsorch]: Retry explicit router interface VRF rehome #4764

Draft

Xichen96 requested a review from Copilot July 19, 2026 16:58

Copilot started reviewing on behalf of Xichen96 July 19, 2026 16:59 View session

Copilot AI reviewed Jul 19, 2026

View reviewed changes

Comment thread tests/mock_tests/intfsorch_ut.cpp

Comment thread tests/mock_tests/intfsorch_ut.cpp

Xichen96 requested a review from Copilot July 19, 2026 17:13

Copilot started reviewing on behalf of Xichen96 July 19, 2026 17:14 View session

Copilot AI reviewed Jul 19, 2026

View reviewed changes

Xichen96 mentioned this pull request Jul 19, 2026

[202605][intfsorch]: Validate VRF race and loopback fixes #4767

Draft

Xichen96 requested a review from Copilot July 19, 2026 18:10

Copilot started reviewing on behalf of Xichen96 July 19, 2026 18:56 View session

Copilot AI reviewed Jul 19, 2026

View reviewed changes

Xichen96 closed this Jul 21, 2026

Xichen96 reopened this Jul 21, 2026

Xichen96 marked this pull request as ready for review July 30, 2026 17:58

Xichen96 requested a review from prsunny as a code owner July 30, 2026 17:58

mssonicbld added the Tested for 202605 branch Tested for 202605 branch label Jul 30, 2026

Copilot AI review requested due to automatic review settings July 30, 2026 18:53

Copilot started reviewing on behalf of Xichen96 July 30, 2026 18:54 View session

Copilot AI reviewed Jul 30, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 31, 2026 05:46

Copilot started reviewing on behalf of Xichen96 July 31, 2026 05:46 View session

Copilot AI reviewed Jul 31, 2026

View reviewed changes

Xichen96 removed the Tested for 202605 branch Tested for 202605 branch label Jul 31, 2026

Xichen96 requested a review from Copilot July 31, 2026 05:57

Copilot started reviewing on behalf of Xichen96 July 31, 2026 05:58 View session

Copilot AI reviewed Jul 31, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 2, 2026 12:08

Copilot started reviewing on behalf of Xichen96 August 2, 2026 12:08 View session

Copilot AI reviewed Aug 2, 2026

View reviewed changes

Uh oh!

Conversation

Xichen96 commented Jul 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mssonicbld commented Jul 19, 2026

Uh oh!

azure-pipelines Bot commented Jul 19, 2026

Uh oh!

azure-pipelines Bot commented Jul 19, 2026

Uh oh!

Xichen96 commented Jul 19, 2026

Uh oh!

azure-pipelines Bot commented Jul 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

mssonicbld commented Jul 19, 2026

Uh oh!

azure-pipelines Bot commented Jul 19, 2026

Uh oh!

Xichen96 commented Jul 19, 2026

Uh oh!

azure-pipelines Bot commented Jul 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mssonicbld commented Jul 19, 2026

Uh oh!

azure-pipelines Bot commented Jul 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Xichen96 commented Jul 21, 2026

Uh oh!

mssonicbld commented Jul 21, 2026

Uh oh!

mssonicbld commented Jul 21, 2026

Uh oh!

mssonicbld commented Jul 21, 2026

Uh oh!

azure-pipelines Bot commented Jul 21, 2026

Uh oh!

azure-pipelines Bot commented Jul 21, 2026

Uh oh!

azure-pipelines Bot commented Jul 22, 2026

Uh oh!

azure-pipelines Bot commented Jul 22, 2026

Uh oh!

rookie-who commented Jul 27, 2026

Uh oh!

azure-pipelines Bot commented Jul 30, 2026

Uh oh!

mssonicbld commented Jul 30, 2026

Uh oh!

mssonicbld commented Jul 30, 2026

Uh oh!

azure-pipelines Bot commented Jul 30, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mssonicbld commented Jul 31, 2026

Uh oh!

azure-pipelines Bot commented Jul 31, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Xichen96 commented Jul 19, 2026 •

edited

Loading