From 00660c95a283fb79996f12f90667e2fdd5afb494 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 19 Jul 2026 16:56:07 +0000 Subject: [PATCH 01/10] [intfsorch]: Retry loopback action update failures Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079 Signed-off-by: Xichen96 --- orchagent/intfsorch.cpp | 17 ++++++- tests/mock_tests/intfsorch_ut.cpp | 74 +++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 2 deletions(-) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index 2921393d7c..e72335272b 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -76,6 +76,7 @@ IntfsOrch::IntfsOrch(DBConnector *db, vector tableNames, m_vidToRidTable = unique_ptr(new Table(m_asic_db.get(), "VIDTORID")); } + auto intervT = timespec { .tv_sec = UPDATE_MAPS_SEC , .tv_nsec = 0 }; m_updateMapsTimer = new SelectableTimer(intervT); auto executorT = new ExecutableTimer(m_updateMapsTimer, this, "UPDATE_MAPS_TIMER"); @@ -880,6 +881,15 @@ void IntfsOrch::doTask(Consumer &consumer) string op = kfvOp(t); if (op == SET_COMMAND) { + if (!loopbackAction.empty()) + { + sai_packet_action_t action; + if (!getSaiLoopbackAction(loopbackAction, action)) + { + loopbackAction.clear(); + } + } + if (is_lo) { if (!ip_prefix_in_key) @@ -1066,7 +1076,11 @@ void IntfsOrch::doTask(Consumer &consumer) /* Set loopback action */ if (!loopbackAction.empty()) { - setIntfLoopbackAction(port, loopbackAction); + if (!setIntfLoopbackAction(port, loopbackAction)) + { + it++; + continue; + } } } } @@ -1975,4 +1989,3 @@ void IntfsOrch::voqSyncIntfState(string &alias, bool isUp) } } - diff --git a/tests/mock_tests/intfsorch_ut.cpp b/tests/mock_tests/intfsorch_ut.cpp index 5e50e73183..c0e856ad30 100644 --- a/tests/mock_tests/intfsorch_ut.cpp +++ b/tests/mock_tests/intfsorch_ut.cpp @@ -16,6 +16,9 @@ namespace intfsorch_test int create_rif_count = 0; int remove_rif_count = 0; + bool saw_loopback_action = false; + 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; @@ -36,6 +39,23 @@ namespace intfsorch_test return SAI_STATUS_SUCCESS; } + sai_status_t _ut_set_router_interface_attribute( + _In_ sai_object_id_t router_interface_id, + _In_ const sai_attribute_t *attr) + { + if (fail_next_rif_set) + { + fail_next_rif_set = false; + return SAI_STATUS_INSUFFICIENT_RESOURCES; + } + if (attr->id == SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION) + { + saw_loopback_action = true; + last_loopback_action = static_cast(attr->value.s32); + } + return SAI_STATUS_SUCCESS; + } + struct IntfsOrchTest : public ::testing::Test { shared_ptr m_app_db; @@ -61,6 +81,10 @@ namespace intfsorch_test sai_router_intfs_api->create_router_interface = _ut_create_router_interface; sai_router_intfs_api->remove_router_interface = _ut_remove_router_interface; + sai_router_intfs_api->set_router_interface_attribute = _ut_set_router_interface_attribute; + saw_loopback_action = false; + fail_next_rif_set = false; + last_loopback_action = SAI_PACKET_ACTION_FORWARD; m_app_db = make_shared("APPL_DB", 0); m_config_db = make_shared("CONFIG_DB", 0); @@ -488,5 +512,55 @@ namespace intfsorch_test syncd = gIntfsOrch->getSyncdIntfses(); ASSERT_EQ(syncd["Loopback6"].vrf_id, gVrfOrch->getVRFid("Vrf-Blue")); ASSERT_EQ(gVrfOrch->getVrfRefCount("Vrf-Blue"), base_vrf_ref + 1); + TEST_F(IntfsOrchTest, IntfsOrchRetriesLoopbackActionSetFailure) + { + std::deque entries{ + {"Ethernet0", "SET", {{"mtu", "9100"}}} + }; + auto consumer = dynamic_cast(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME)); + consumer->addToSync(entries); + static_cast(gIntfsOrch)->doTask(); + + fail_next_rif_set = true; + entries = { + {"Ethernet0", "SET", {{"loopback_action", "drop"}}} + }; + consumer->addToSync(entries); + static_cast(gIntfsOrch)->doTask(); + + ASSERT_EQ(consumer->m_toSync.size(), 1u); + ASSERT_FALSE(saw_loopback_action); + + static_cast(gIntfsOrch)->doTask(); + + ASSERT_TRUE(consumer->m_toSync.empty()); + ASSERT_TRUE(saw_loopback_action); + ASSERT_EQ(last_loopback_action, SAI_PACKET_ACTION_DROP); + } + + TEST_F(IntfsOrchTest, IntfsOrchIgnoresInvalidLoopbackActionField) + { + std::deque entries{ + {"Ethernet0", "SET", {{"mtu", "9100"}}} + }; + auto consumer = dynamic_cast(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME)); + consumer->addToSync(entries); + static_cast(gIntfsOrch)->doTask(); + + entries = { + {"Ethernet0", "SET", { + {"loopback_action", "invalid"}, + {"mtu", "1500"} + }} + }; + consumer->addToSync(entries); + static_cast(gIntfsOrch)->doTask(); + + ASSERT_TRUE(consumer->m_toSync.empty()); + ASSERT_FALSE(saw_loopback_action); + + Port port; + ASSERT_TRUE(gPortsOrch->getPort("Ethernet0", port)); + ASSERT_EQ(port.m_mtu, 1500u); } } From 0be58a25f96930a152bb0fb810cc6fe273b29a27 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 19 Jul 2026 17:12:54 +0000 Subject: [PATCH 02/10] [tests]: Validate loopback test consumer Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079 Signed-off-by: Xichen96 --- tests/mock_tests/intfsorch_ut.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/mock_tests/intfsorch_ut.cpp b/tests/mock_tests/intfsorch_ut.cpp index c0e856ad30..2c135f4cc0 100644 --- a/tests/mock_tests/intfsorch_ut.cpp +++ b/tests/mock_tests/intfsorch_ut.cpp @@ -518,6 +518,7 @@ namespace intfsorch_test {"Ethernet0", "SET", {{"mtu", "9100"}}} }; auto consumer = dynamic_cast(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME)); + ASSERT_NE(consumer, nullptr); consumer->addToSync(entries); static_cast(gIntfsOrch)->doTask(); @@ -544,6 +545,7 @@ namespace intfsorch_test {"Ethernet0", "SET", {{"mtu", "9100"}}} }; auto consumer = dynamic_cast(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME)); + ASSERT_NE(consumer, nullptr); consumer->addToSync(entries); static_cast(gIntfsOrch)->doTask(); From 367d4ac92043d7077d039c26098faf93bf9bbba4 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 19 Jul 2026 18:09:22 +0000 Subject: [PATCH 03/10] tests: Fix invalid loopback action regression check Use NAT zone as the bundled valid update because physical interface MTU updates are not handled by IntfsOrch after RIF creation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079 Signed-off-by: Xichen96 --- tests/mock_tests/intfsorch_ut.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/mock_tests/intfsorch_ut.cpp b/tests/mock_tests/intfsorch_ut.cpp index 2c135f4cc0..67a2ba8b66 100644 --- a/tests/mock_tests/intfsorch_ut.cpp +++ b/tests/mock_tests/intfsorch_ut.cpp @@ -552,7 +552,7 @@ namespace intfsorch_test entries = { {"Ethernet0", "SET", { {"loopback_action", "invalid"}, - {"mtu", "1500"} + {"nat_zone", "7"} }} }; consumer->addToSync(entries); @@ -563,6 +563,6 @@ namespace intfsorch_test Port port; ASSERT_TRUE(gPortsOrch->getPort("Ethernet0", port)); - ASSERT_EQ(port.m_mtu, 1500u); + ASSERT_EQ(port.m_nat_zone_id, 7u); } } From faf1dee64b09c6bc17b70d3b8e9343e369aab4ef Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Wed, 22 Jul 2026 07:10:23 +0000 Subject: [PATCH 04/10] [tests]: Preserve VRF bind regression boundary Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079 Signed-off-by: Xichen96 --- tests/mock_tests/intfsorch_ut.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/mock_tests/intfsorch_ut.cpp b/tests/mock_tests/intfsorch_ut.cpp index 67a2ba8b66..ae9769ba68 100644 --- a/tests/mock_tests/intfsorch_ut.cpp +++ b/tests/mock_tests/intfsorch_ut.cpp @@ -512,13 +512,14 @@ namespace intfsorch_test syncd = gIntfsOrch->getSyncdIntfses(); ASSERT_EQ(syncd["Loopback6"].vrf_id, gVrfOrch->getVRFid("Vrf-Blue")); ASSERT_EQ(gVrfOrch->getVrfRefCount("Vrf-Blue"), base_vrf_ref + 1); + } + TEST_F(IntfsOrchTest, IntfsOrchRetriesLoopbackActionSetFailure) { std::deque entries{ {"Ethernet0", "SET", {{"mtu", "9100"}}} }; auto consumer = dynamic_cast(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME)); - ASSERT_NE(consumer, nullptr); consumer->addToSync(entries); static_cast(gIntfsOrch)->doTask(); @@ -545,14 +546,13 @@ namespace intfsorch_test {"Ethernet0", "SET", {{"mtu", "9100"}}} }; auto consumer = dynamic_cast(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME)); - ASSERT_NE(consumer, nullptr); consumer->addToSync(entries); static_cast(gIntfsOrch)->doTask(); entries = { {"Ethernet0", "SET", { {"loopback_action", "invalid"}, - {"nat_zone", "7"} + {"mtu", "1500"} }} }; consumer->addToSync(entries); @@ -563,6 +563,6 @@ namespace intfsorch_test Port port; ASSERT_TRUE(gPortsOrch->getPort("Ethernet0", port)); - ASSERT_EQ(port.m_nat_zone_id, 7u); + ASSERT_EQ(port.m_mtu, 1500u); } } From 76aaa566fac92bbb2909b4d92126ca35fd8efb76 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Wed, 22 Jul 2026 07:12:02 +0000 Subject: [PATCH 05/10] [tests]: Restore final loopback regression checks Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079 Signed-off-by: Xichen96 --- tests/mock_tests/intfsorch_ut.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/mock_tests/intfsorch_ut.cpp b/tests/mock_tests/intfsorch_ut.cpp index ae9769ba68..a38c0652de 100644 --- a/tests/mock_tests/intfsorch_ut.cpp +++ b/tests/mock_tests/intfsorch_ut.cpp @@ -520,6 +520,7 @@ namespace intfsorch_test {"Ethernet0", "SET", {{"mtu", "9100"}}} }; auto consumer = dynamic_cast(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME)); + ASSERT_NE(consumer, nullptr); consumer->addToSync(entries); static_cast(gIntfsOrch)->doTask(); @@ -546,13 +547,14 @@ namespace intfsorch_test {"Ethernet0", "SET", {{"mtu", "9100"}}} }; auto consumer = dynamic_cast(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME)); + ASSERT_NE(consumer, nullptr); consumer->addToSync(entries); static_cast(gIntfsOrch)->doTask(); entries = { {"Ethernet0", "SET", { {"loopback_action", "invalid"}, - {"mtu", "1500"} + {"nat_zone", "7"} }} }; consumer->addToSync(entries); @@ -563,6 +565,6 @@ namespace intfsorch_test Port port; ASSERT_TRUE(gPortsOrch->getPort("Ethernet0", port)); - ASSERT_EQ(port.m_mtu, 1500u); + ASSERT_EQ(port.m_nat_zone_id, 7u); } } From a6d671f713779bb42824c99425af6d1a7f616cc6 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Wed, 22 Jul 2026 07:26:25 +0000 Subject: [PATCH 06/10] [tests]: Scope loopback action failure injection Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079 Signed-off-by: Xichen96 --- tests/mock_tests/intfsorch_ut.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/mock_tests/intfsorch_ut.cpp b/tests/mock_tests/intfsorch_ut.cpp index a38c0652de..56dd06999f 100644 --- a/tests/mock_tests/intfsorch_ut.cpp +++ b/tests/mock_tests/intfsorch_ut.cpp @@ -43,13 +43,13 @@ namespace intfsorch_test _In_ sai_object_id_t router_interface_id, _In_ const sai_attribute_t *attr) { - if (fail_next_rif_set) - { - fail_next_rif_set = false; - return SAI_STATUS_INSUFFICIENT_RESOURCES; - } if (attr->id == SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION) { + if (fail_next_rif_set) + { + fail_next_rif_set = false; + return SAI_STATUS_INSUFFICIENT_RESOURCES; + } saw_loopback_action = true; last_loopback_action = static_cast(attr->value.s32); } From 4905c25ef16496ef3b7dbba506a24747e671211b Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Wed, 22 Jul 2026 08:08:16 +0000 Subject: [PATCH 07/10] [tests]: Preserve unrelated RIF setter behavior Delegate non-loopback router-interface attribute updates to the original SAI implementation so the retry hook does not mask other unit-test behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079 Signed-off-by: Xichen96 --- tests/mock_tests/intfsorch_ut.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/mock_tests/intfsorch_ut.cpp b/tests/mock_tests/intfsorch_ut.cpp index 56dd06999f..3ead83c2a2 100644 --- a/tests/mock_tests/intfsorch_ut.cpp +++ b/tests/mock_tests/intfsorch_ut.cpp @@ -53,7 +53,8 @@ namespace intfsorch_test saw_loopback_action = true; last_loopback_action = static_cast(attr->value.s32); } - return SAI_STATUS_SUCCESS; + return pold_sai_rif_api->set_router_interface_attribute( + router_interface_id, attr); } struct IntfsOrchTest : public ::testing::Test From 5481ae09e1f4f2b2832cb517e0f8a26f2a53c255 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Fri, 31 Jul 2026 04:53:14 +1000 Subject: [PATCH 08/10] Treat invalid loopback action as terminal 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 --- orchagent/intfsorch.cpp | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index e72335272b..54bdf45521 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -437,7 +437,7 @@ bool IntfsOrch::setIntfLoopbackAction(const Port &port, string actionStr) if (!getSaiLoopbackAction(actionStr, action)) { - return false; + return true; } attr.id = SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION; @@ -881,15 +881,6 @@ void IntfsOrch::doTask(Consumer &consumer) string op = kfvOp(t); if (op == SET_COMMAND) { - if (!loopbackAction.empty()) - { - sai_packet_action_t action; - if (!getSaiLoopbackAction(loopbackAction, action)) - { - loopbackAction.clear(); - } - } - if (is_lo) { if (!ip_prefix_in_key) From 4e4347d7981c6100dd24df56474a9573489cdeaf Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Fri, 31 Jul 2026 15:45:32 +1000 Subject: [PATCH 09/10] Complete intercepted loopback RIF sets 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 --- tests/mock_tests/intfsorch_ut.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/mock_tests/intfsorch_ut.cpp b/tests/mock_tests/intfsorch_ut.cpp index 3ead83c2a2..fa4e066ee2 100644 --- a/tests/mock_tests/intfsorch_ut.cpp +++ b/tests/mock_tests/intfsorch_ut.cpp @@ -52,6 +52,7 @@ namespace intfsorch_test } saw_loopback_action = true; last_loopback_action = static_cast(attr->value.s32); + return SAI_STATUS_SUCCESS; } return pold_sai_rif_api->set_router_interface_attribute( router_interface_id, attr); From d1ef1b1adba352db3a6f93ef152d72916c14f81e Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 2 Aug 2026 22:07:19 +1000 Subject: [PATCH 10/10] Preserve RIF mock fidelity 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 --- tests/mock_tests/intfsorch_ut.cpp | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/tests/mock_tests/intfsorch_ut.cpp b/tests/mock_tests/intfsorch_ut.cpp index fa4e066ee2..93970e4fb2 100644 --- a/tests/mock_tests/intfsorch_ut.cpp +++ b/tests/mock_tests/intfsorch_ut.cpp @@ -18,6 +18,7 @@ namespace intfsorch_test int remove_rif_count = 0; bool saw_loopback_action = false; bool fail_next_rif_set = false; + int loopback_action_set_count = 0; 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; @@ -29,14 +30,15 @@ namespace intfsorch_test _In_ const sai_attribute_t *attr_list) { ++create_rif_count; - return SAI_STATUS_SUCCESS; + return pold_sai_rif_api->create_router_interface( + router_interface_id, switch_id, attr_count, attr_list); } sai_status_t _ut_remove_router_interface( _In_ sai_object_id_t router_interface_id) { ++remove_rif_count; - return SAI_STATUS_SUCCESS; + return pold_sai_rif_api->remove_router_interface(router_interface_id); } sai_status_t _ut_set_router_interface_attribute( @@ -45,13 +47,14 @@ namespace intfsorch_test { if (attr->id == SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION) { + ++loopback_action_set_count; + last_loopback_action = static_cast(attr->value.s32); if (fail_next_rif_set) { fail_next_rif_set = false; return SAI_STATUS_INSUFFICIENT_RESOURCES; } saw_loopback_action = true; - last_loopback_action = static_cast(attr->value.s32); return SAI_STATUS_SUCCESS; } return pold_sai_rif_api->set_router_interface_attribute( @@ -86,6 +89,7 @@ namespace intfsorch_test sai_router_intfs_api->set_router_interface_attribute = _ut_set_router_interface_attribute; saw_loopback_action = false; fail_next_rif_set = false; + loopback_action_set_count = 0; last_loopback_action = SAI_PACKET_ACTION_FORWARD; m_app_db = make_shared("APPL_DB", 0); @@ -534,11 +538,13 @@ namespace intfsorch_test static_cast(gIntfsOrch)->doTask(); ASSERT_EQ(consumer->m_toSync.size(), 1u); + ASSERT_EQ(loopback_action_set_count, 1); ASSERT_FALSE(saw_loopback_action); static_cast(gIntfsOrch)->doTask(); ASSERT_TRUE(consumer->m_toSync.empty()); + ASSERT_EQ(loopback_action_set_count, 2); ASSERT_TRUE(saw_loopback_action); ASSERT_EQ(last_loopback_action, SAI_PACKET_ACTION_DROP); } @@ -563,6 +569,7 @@ namespace intfsorch_test static_cast(gIntfsOrch)->doTask(); ASSERT_TRUE(consumer->m_toSync.empty()); + ASSERT_EQ(loopback_action_set_count, 0); ASSERT_FALSE(saw_loopback_action); Port port;