From e46968aaa63b51fa6df613e2491d6d9d8fc52a6c Mon Sep 17 00:00:00 2001 From: dgsudharsan Date: Tue, 14 Jul 2026 20:58:28 +0000 Subject: [PATCH 01/20] [intfsorch]Fix race condition between VRF bind and ip address removal Signed-off-by: dgsudharsan --- orchagent/intfsorch.cpp | 5 ++- tests/mock_tests/intfsorch_ut.cpp | 54 ++++++++++++++++++++++++++++++- 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index 22956c00b65..aa66d795355 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -857,7 +857,10 @@ void IntfsOrch::doTask(Consumer &consumer) } else { - SWSS_LOG_ERROR("Failed to set interface '%s' to VRF ID '%d' because it has IP addresses associated with it.", alias.c_str(), vrf_id); + SWSS_LOG_INFO("Interface '%s' still has %zu IP address(es); deferring VRF '%s' bind until pending IP removals are processed.", + alias.c_str(), m_syncdIntfses[alias].ip_addresses.size(), vrf_name.c_str()); + it++; + continue; } } } diff --git a/tests/mock_tests/intfsorch_ut.cpp b/tests/mock_tests/intfsorch_ut.cpp index e3a7d2eccd5..fd836d5b8ae 100644 --- a/tests/mock_tests/intfsorch_ut.cpp +++ b/tests/mock_tests/intfsorch_ut.cpp @@ -395,4 +395,56 @@ namespace intfsorch_test m_syncdIntfses = gIntfsOrch->getSyncdIntfses(); ASSERT_EQ(m_syncdIntfses["Loopback3"].vrf_id, gVirtualRouterId); } -} \ No newline at end of file + // Regression test for the batched IP-removal + VRF-bind race. + // m_toSync is an ordered multimap, so within a single drain the bare interface + // key ("Loopback6") is processed before the per-IP key ("Loopback6:6.6.6.6/32"). + // When a config sequence removes the loopback IPs and rebinds the interface to a + // VRF back-to-back, both land in one batch and the VRF-change SET is evaluated + // while the IP is still present. The fix defers (retains) that SET instead of + // logging an error and dropping it, so the bind converges on a later drain. + TEST_F(IntfsOrchTest, IntfsOrchVrfBindDeferredUntilIpRemoved) + { + // create a new vrf + std::deque entries; + entries.push_back({"Vrf-Blue", "SET", { {"NULL", "NULL"}}}); + auto vrfConsumer = dynamic_cast(gVrfOrch->getExecutor(APP_VRF_TABLE_NAME)); + vrfConsumer->addToSync(entries); + static_cast(gVrfOrch)->doTask(); + ASSERT_TRUE(gVrfOrch->isVRFexists("Vrf-Blue")); + auto base_vrf_ref = gVrfOrch->getVrfRefCount("Vrf-Blue"); + + auto intfConsumer = dynamic_cast(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME)); + + // create a loopback in the default vrf and give it an IP address + entries.clear(); + entries.push_back({"Loopback6", "SET", {}}); + entries.push_back({"Loopback6:6.6.6.6/32", "SET", {{"scope", "global"}, {"family", "IPv4"}}}); + intfConsumer->addToSync(entries); + static_cast(gIntfsOrch)->doTask(); + auto syncd = gIntfsOrch->getSyncdIntfses(); + ASSERT_EQ(syncd["Loopback6"].vrf_id, gVirtualRouterId); + ASSERT_EQ(syncd["Loopback6"].ip_addresses.size(), static_cast(1)); + + // Single batch: remove the IP AND rebind the interface to Vrf-Blue. + // The bare "Loopback6" SET sorts before "Loopback6:6.6.6.6/32" DEL, so the + // bind is evaluated first (IP still present) and must be deferred, not dropped. + entries.clear(); + entries.push_back({"Loopback6", "SET", { {"vrf_name", "Vrf-Blue"}}}); + entries.push_back({"Loopback6:6.6.6.6/32", "DEL", {}}); + intfConsumer->addToSync(entries); + static_cast(gIntfsOrch)->doTask(); + + // After the first drain: IP removed, interface still present and still in the + // default vrf (the bind is pending, not lost). + syncd = gIntfsOrch->getSyncdIntfses(); + ASSERT_NE(syncd.find("Loopback6"), syncd.end()); + ASSERT_EQ(syncd["Loopback6"].ip_addresses.size(), static_cast(0)); + ASSERT_EQ(syncd["Loopback6"].vrf_id, gVirtualRouterId); + + // The deferred bind is retried on the next drain and now succeeds. + static_cast(gIntfsOrch)->doTask(); + syncd = gIntfsOrch->getSyncdIntfses(); + ASSERT_EQ(syncd["Loopback6"].vrf_id, gVrfOrch->getVRFid("Vrf-Blue")); + ASSERT_EQ(gVrfOrch->getVrfRefCount("Vrf-Blue"), base_vrf_ref + 1); + } +} From 61df448223b2a7c7cf3053ffb56405643fed9d70 Mon Sep 17 00:00:00 2001 From: dgsudharsan Date: Thu, 16 Jul 2026 20:01:01 +0000 Subject: [PATCH 02/20] Changing log level as per review feedback Signed-off-by: dgsudharsan --- orchagent/intfsorch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index aa66d795355..70febe1dbfa 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -857,7 +857,7 @@ void IntfsOrch::doTask(Consumer &consumer) } else { - SWSS_LOG_INFO("Interface '%s' still has %zu IP address(es); deferring VRF '%s' bind until pending IP removals are processed.", + SWSS_LOG_NOTICE("Interface '%s' still has %zu IP address(es); deferring VRF '%s' bind until pending IP removals are processed.", alias.c_str(), m_syncdIntfses[alias].ip_addresses.size(), vrf_name.c_str()); it++; continue; From cc6381ba2ef80ada531a8e889daf7000eb7395dc Mon Sep 17 00:00:00 2001 From: Xichen Lin Date: Sun, 19 Jul 2026 21:26:35 +1000 Subject: [PATCH 03/20] [202605][intfsorch]: Fence route and neighbor creation during interface removal Keep the existing whole-interface DEL retry fenced after prefix deletion. Standard route, neighbor, and neighbor-next-hop creation now retries instead of attaching to the RIF being removed, while teardown continues to use the raw RIF lookup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079 Signed-off-by: Xichen Lin --- orchagent/intfsorch.cpp | 21 ++++++++++++++++++--- orchagent/intfsorch.h | 1 + orchagent/neighorch.cpp | 9 +++++++-- orchagent/routeorch.cpp | 12 +++++++++--- tests/mock_tests/intfsorch_ut.cpp | 15 ++++++++++++++- 5 files changed, 49 insertions(+), 9 deletions(-) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index 70febe1dbfa..456b511f49e 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -116,6 +116,16 @@ sai_object_id_t IntfsOrch::getRouterIntfsId(const string &alias) return port.m_rif_id; } +sai_object_id_t IntfsOrch::getRouterIntfsIdForNewDependency(const string &alias) +{ + if (m_removingIntfses.find(alias) != m_removingIntfses.end()) + { + return SAI_NULL_OBJECT_ID; + } + + return getRouterIntfsId(alias); +} + bool IntfsOrch::isPrefixSubnet(const IpPrefix &ip_prefix, const string &alias) { if (m_syncdIntfses.find(alias) == m_syncdIntfses.end()) @@ -1132,12 +1142,18 @@ void IntfsOrch::doTask(Consumer &consumer) { if (removeIntf(alias, port.m_vr_id, ip_prefix_in_key ? &ip_prefix : nullptr)) { - m_removingIntfses.erase(alias); + if (!ip_prefix_in_key) + { + m_removingIntfses.erase(alias); + } it = consumer.m_toSync.erase(it); } else { - m_removingIntfses.insert(alias); + if (!ip_prefix_in_key) + { + m_removingIntfses.insert(alias); + } it++; continue; } @@ -1782,4 +1798,3 @@ void IntfsOrch::voqSyncIntfState(string &alias, bool isUp) } } - diff --git a/orchagent/intfsorch.h b/orchagent/intfsorch.h index aa5129bef45..fa1b9088cc6 100644 --- a/orchagent/intfsorch.h +++ b/orchagent/intfsorch.h @@ -36,6 +36,7 @@ class IntfsOrch : public Orch IntfsOrch(DBConnector *db, string tableName, VRFOrch *vrf_orch, DBConnector *chassisAppDb); sai_object_id_t getRouterIntfsId(const string&); + sai_object_id_t getRouterIntfsIdForNewDependency(const string&); bool isPrefixSubnet(const IpPrefix&, const string&); bool isInbandIntfInMgmtVrf(const string& alias); string getRouterIntfsAlias(const IpAddress &ip, const string &vrf_name = ""); diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index 9c7899ef953..e5364680000 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -268,7 +268,12 @@ bool NeighOrch::addNextHop(NeighborContext& ctx) } assert(!hasNextHop(nexthop)); - sai_object_id_t rif_id = m_intfsOrch->getRouterIntfsId(nh.alias); + sai_object_id_t rif_id = m_intfsOrch->getRouterIntfsIdForNewDependency(nh.alias); + if (rif_id == SAI_NULL_OBJECT_ID) + { + SWSS_LOG_INFO("Failed to get rif_id for %s", nh.alias.c_str()); + return false; + } vector next_hop_attrs; @@ -1201,7 +1206,7 @@ bool NeighOrch::addNeighbor(NeighborContext& ctx) string alias = neighborEntry.alias; bool bulk_op = ctx.bulk_op; - sai_object_id_t rif_id = m_intfsOrch->getRouterIntfsId(alias); + sai_object_id_t rif_id = m_intfsOrch->getRouterIntfsIdForNewDependency(alias); if (rif_id == SAI_NULL_OBJECT_ID) { SWSS_LOG_INFO("Failed to get rif_id for %s", alias.c_str()); diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 54448b88202..dc217d8c492 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -1515,7 +1515,10 @@ bool RouteOrch::addNextHopGroup(const NextHopGroupKey &nexthops) m_neighOrch->hasNextHop(NextHopKey(it.ip_address, it.alias))) { NeighborContext ctx = NeighborContext(it); - m_neighOrch->addNextHop(ctx); + if (!m_neighOrch->addNextHop(ctx)) + { + return false; + } next_hop_id = m_neighOrch->getNextHopId(it); } else @@ -2091,7 +2094,7 @@ bool RouteOrch::addRoute(RouteBulkContext& ctx, const NextHopGroupKey &nextHops) return true; } - next_hop_id = m_intfsOrch->getRouterIntfsId(nexthop.alias); + next_hop_id = m_intfsOrch->getRouterIntfsIdForNewDependency(nexthop.alias); /* rif is not created yet */ if (next_hop_id == SAI_NULL_OBJECT_ID) { @@ -2126,7 +2129,10 @@ bool RouteOrch::addRoute(RouteBulkContext& ctx, const NextHopGroupKey &nextHops) { /* since IP neighbor NH exists, neighbor is resolved, add MPLS NH */ NeighborContext ctx = NeighborContext(nexthop); - m_neighOrch->addNextHop(ctx); + if (!m_neighOrch->addNextHop(ctx)) + { + return false; + } next_hop_id = m_neighOrch->getNextHopId(nexthop); } /* IP neighbor is not yet resolved */ diff --git a/tests/mock_tests/intfsorch_ut.cpp b/tests/mock_tests/intfsorch_ut.cpp index fd836d5b8ae..77daeff55bd 100644 --- a/tests/mock_tests/intfsorch_ut.cpp +++ b/tests/mock_tests/intfsorch_ut.cpp @@ -308,17 +308,30 @@ namespace intfsorch_test static_cast(gIntfsOrch)->doTask(); ASSERT_EQ(current_create_count + 1, create_rif_count); + // Add a prefix so the whole-interface DEL is processed before its dependency DEL. + entries.clear(); + entries.push_back({"Ethernet0:10.0.0.1/24", "SET", { + {"scope", "global"}, + {"family", "IPv4"} + }}); + consumer->addToSync(entries); + static_cast(gIntfsOrch)->doTask(); + // create dependency to the interface gIntfsOrch->increaseRouterIntfsRefCount("Ethernet0"); - // delete the interface, expect retry because dependency exists + // Delete the interface and prefix in lexical order. The successful prefix + // DEL must not clear the whole-interface removal fence. entries.clear(); entries.push_back({"Ethernet0", "DEL", { {} }}); + entries.push_back({"Ethernet0:10.0.0.1/24", "DEL", { {} }}); consumer = dynamic_cast(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME)); consumer->addToSync(entries); auto current_remove_count = remove_rif_count; static_cast(gIntfsOrch)->doTask(); ASSERT_EQ(current_remove_count, remove_rif_count); + ASSERT_EQ(consumer->m_toSync.size(), 1u); + ASSERT_EQ(gIntfsOrch->getRouterIntfsIdForNewDependency("Ethernet0"), SAI_NULL_OBJECT_ID); // create the interface again, expect retry because interface is in removing entries.clear(); From 26aa630205cf0578c49b93e0c81526480fb4f3b5 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Tue, 21 Jul 2026 14:38:58 +0000 Subject: [PATCH 04/20] [202605][intfsorch]: Match final removal fence design 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 ++++++++++------- orchagent/intfsorch.h | 2 +- orchagent/neighorch.cpp | 18 ++++++++++++++++-- orchagent/routeorch.cpp | 9 ++++++++- tests/mock_tests/intfsorch_ut.cpp | 2 +- tests/mock_tests/neighorch_ut.cpp | 14 ++++++++++++++ 6 files changed, 50 insertions(+), 12 deletions(-) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index 456b511f49e..f7cb63486ce 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -116,14 +116,9 @@ sai_object_id_t IntfsOrch::getRouterIntfsId(const string &alias) return port.m_rif_id; } -sai_object_id_t IntfsOrch::getRouterIntfsIdForNewDependency(const string &alias) +bool IntfsOrch::isIntfRemovalPending(const string &alias) const { - if (m_removingIntfses.find(alias) != m_removingIntfses.end()) - { - return SAI_NULL_OBJECT_ID; - } - - return getRouterIntfsId(alias); + return m_removingIntfses.find(alias) != m_removingIntfses.end(); } bool IntfsOrch::isPrefixSubnet(const IpPrefix &ip_prefix, const string &alias) @@ -1129,11 +1124,19 @@ void IntfsOrch::doTask(Consumer &consumer) if (vnet_orch->delIntf(alias, vnet_name, ip_prefix_in_key ? &ip_prefix : nullptr)) { + if (!ip_prefix_in_key) + { + m_removingIntfses.erase(alias); + } m_vnetInfses.erase(alias); it = consumer.m_toSync.erase(it); } else { + if (!ip_prefix_in_key) + { + m_removingIntfses.insert(alias); + } it++; continue; } diff --git a/orchagent/intfsorch.h b/orchagent/intfsorch.h index fa1b9088cc6..58a8a83a9b1 100644 --- a/orchagent/intfsorch.h +++ b/orchagent/intfsorch.h @@ -36,7 +36,7 @@ class IntfsOrch : public Orch IntfsOrch(DBConnector *db, string tableName, VRFOrch *vrf_orch, DBConnector *chassisAppDb); sai_object_id_t getRouterIntfsId(const string&); - sai_object_id_t getRouterIntfsIdForNewDependency(const string&); + bool isIntfRemovalPending(const string&) const; bool isPrefixSubnet(const IpPrefix&, const string&); bool isInbandIntfInMgmtVrf(const string& alias); string getRouterIntfsAlias(const IpAddress &ip, const string &vrf_name = ""); diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index e5364680000..e6fdbd55dfb 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -268,7 +268,14 @@ bool NeighOrch::addNextHop(NeighborContext& ctx) } assert(!hasNextHop(nexthop)); - sai_object_id_t rif_id = m_intfsOrch->getRouterIntfsIdForNewDependency(nh.alias); + if (!m_intfsOrch->isRemoteSystemPortIntf(nh.alias) && + m_intfsOrch->isIntfRemovalPending(nh.alias)) + { + SWSS_LOG_INFO("Interface %s is pending removal", nh.alias.c_str()); + return false; + } + + sai_object_id_t rif_id = m_intfsOrch->getRouterIntfsId(nh.alias); if (rif_id == SAI_NULL_OBJECT_ID) { SWSS_LOG_INFO("Failed to get rif_id for %s", nh.alias.c_str()); @@ -1206,7 +1213,14 @@ bool NeighOrch::addNeighbor(NeighborContext& ctx) string alias = neighborEntry.alias; bool bulk_op = ctx.bulk_op; - sai_object_id_t rif_id = m_intfsOrch->getRouterIntfsIdForNewDependency(alias); + if (!m_intfsOrch->isRemoteSystemPortIntf(alias) && + m_intfsOrch->isIntfRemovalPending(alias)) + { + SWSS_LOG_INFO("Interface %s is pending removal", alias.c_str()); + return false; + } + + sai_object_id_t rif_id = m_intfsOrch->getRouterIntfsId(alias); if (rif_id == SAI_NULL_OBJECT_ID) { SWSS_LOG_INFO("Failed to get rif_id for %s", alias.c_str()); diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index dc217d8c492..6538cdd82b4 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -2094,7 +2094,14 @@ bool RouteOrch::addRoute(RouteBulkContext& ctx, const NextHopGroupKey &nextHops) return true; } - next_hop_id = m_intfsOrch->getRouterIntfsIdForNewDependency(nexthop.alias); + if (!m_intfsOrch->isRemoteSystemPortIntf(nexthop.alias) && + m_intfsOrch->isIntfRemovalPending(nexthop.alias)) + { + SWSS_LOG_INFO("Interface %s is pending removal", nexthop.alias.c_str()); + return false; + } + + next_hop_id = m_intfsOrch->getRouterIntfsId(nexthop.alias); /* rif is not created yet */ if (next_hop_id == SAI_NULL_OBJECT_ID) { diff --git a/tests/mock_tests/intfsorch_ut.cpp b/tests/mock_tests/intfsorch_ut.cpp index 77daeff55bd..38aab21d023 100644 --- a/tests/mock_tests/intfsorch_ut.cpp +++ b/tests/mock_tests/intfsorch_ut.cpp @@ -331,7 +331,7 @@ namespace intfsorch_test static_cast(gIntfsOrch)->doTask(); ASSERT_EQ(current_remove_count, remove_rif_count); ASSERT_EQ(consumer->m_toSync.size(), 1u); - ASSERT_EQ(gIntfsOrch->getRouterIntfsIdForNewDependency("Ethernet0"), SAI_NULL_OBJECT_ID); + ASSERT_TRUE(gIntfsOrch->isIntfRemovalPending("Ethernet0")); // create the interface again, expect retry because interface is in removing entries.clear(); diff --git a/tests/mock_tests/neighorch_ut.cpp b/tests/mock_tests/neighorch_ut.cpp index 28c3f53adf9..a3a139194e9 100644 --- a/tests/mock_tests/neighorch_ut.cpp +++ b/tests/mock_tests/neighorch_ut.cpp @@ -189,6 +189,20 @@ namespace neighorch_test } }; + TEST_F(NeighOrchTest, LocalNeighborDependenciesRetryWhileInterfaceRemovalPending) + { + gIntfsOrch->m_removingIntfses.insert(VLAN_1000); + + NeighborContext neighbor_ctx(VLAN1000_NEIGH); + neighbor_ctx.mac = MacAddress(MAC1); + EXPECT_FALSE(gNeighOrch->addNeighbor(neighbor_ctx)); + + NeighborContext next_hop_ctx(VLAN1000_NEIGH); + EXPECT_FALSE(gNeighOrch->addNextHop(next_hop_ctx)); + + gIntfsOrch->m_removingIntfses.erase(VLAN_1000); + } + TEST_F(NeighOrchTest, MultiVlanDuplicateNeighbor) { EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry); From 0635408cac9b7ef45e71174b020b80ff7c0da011 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 19 Jul 2026 16:56:07 +0000 Subject: [PATCH 05/20] [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 | 16 ++++++- tests/mock_tests/intfsorch_ut.cpp | 74 +++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 1 deletion(-) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index f7cb63486ce..f9d9aafe15b 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -75,6 +75,7 @@ IntfsOrch::IntfsOrch(DBConnector *db, string tableName, VRFOrch *vrf_orch, DBCon 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"); @@ -839,6 +840,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) @@ -1006,7 +1016,11 @@ void IntfsOrch::doTask(Consumer &consumer) /* Set loopback action */ if (!loopbackAction.empty()) { - setIntfLoopbackAction(port, loopbackAction); + if (!setIntfLoopbackAction(port, loopbackAction)) + { + it++; + continue; + } } } } diff --git a/tests/mock_tests/intfsorch_ut.cpp b/tests/mock_tests/intfsorch_ut.cpp index 38aab21d023..a3037759f70 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); @@ -459,5 +483,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 b3df565669909123760fb74adff32bd9c7e6be86 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 19 Jul 2026 17:12:54 +0000 Subject: [PATCH 06/20] [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 a3037759f70..4271a1bbad0 100644 --- a/tests/mock_tests/intfsorch_ut.cpp +++ b/tests/mock_tests/intfsorch_ut.cpp @@ -489,6 +489,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(); @@ -515,6 +516,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 0e26ce8edced5aa7002781163de0de8b90e78743 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 19 Jul 2026 18:09:22 +0000 Subject: [PATCH 07/20] 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 4271a1bbad0..1b5e33c906a 100644 --- a/tests/mock_tests/intfsorch_ut.cpp +++ b/tests/mock_tests/intfsorch_ut.cpp @@ -523,7 +523,7 @@ namespace intfsorch_test entries = { {"Ethernet0", "SET", { {"loopback_action", "invalid"}, - {"mtu", "1500"} + {"nat_zone", "7"} }} }; consumer->addToSync(entries); @@ -534,6 +534,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 c15c06fb88be426d2844c2418c925e87142b7ad6 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Mon, 20 Jul 2026 16:29:26 +0000 Subject: [PATCH 08/20] [neighorch]: Track remote next hops on inband RIF Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079 Signed-off-by: Xichen96 --- orchagent/neighorch.cpp | 19 +++++--- tests/mock_tests/neighorch_ut.cpp | 73 +++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 5 deletions(-) diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index e6fdbd55dfb..1dac5f084c7 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -275,10 +275,10 @@ bool NeighOrch::addNextHop(NeighborContext& ctx) return false; } - sai_object_id_t rif_id = m_intfsOrch->getRouterIntfsId(nh.alias); + sai_object_id_t rif_id = m_intfsOrch->getRouterIntfsId(nexthop.alias); if (rif_id == SAI_NULL_OBJECT_ID) { - SWSS_LOG_INFO("Failed to get rif_id for %s", nh.alias.c_str()); + SWSS_LOG_INFO("Failed to get rif_id for %s", nexthop.alias.c_str()); return false; } @@ -358,7 +358,7 @@ bool NeighOrch::addNextHop(NeighborContext& ctx) next_hop_entry.nh_flags = 0; m_syncdNextHops[nexthop] = next_hop_entry; - m_intfsOrch->increaseRouterIntfsRefCount(nh.alias); + m_intfsOrch->increaseRouterIntfsRefCount(nexthop.alias); if (nexthop.isMplsNextHop()) { @@ -417,6 +417,15 @@ bool NeighOrch::processBulkAddNextHop(NeighborContext& ctx) } NextHopKey nexthop(nh); + if (m_intfsOrch->isRemoteSystemPortIntf(nh.alias)) + { + Port inbp; + gPortsOrch->getInbandPort(inbp); + assert(inbp.m_alias.length()); + + nexthop.alias = inbp.m_alias; + } + if (ctx.next_hop_id == SAI_NULL_OBJECT_ID) { sai_status_t bulker_status = gNextHopBulker.create_status(ctx.next_hop_id); @@ -450,7 +459,7 @@ bool NeighOrch::processBulkAddNextHop(NeighborContext& ctx) next_hop_entry.nh_flags = 0; m_syncdNextHops[nexthop] = next_hop_entry; - m_intfsOrch->increaseRouterIntfsRefCount(nh.alias); + m_intfsOrch->increaseRouterIntfsRefCount(nexthop.alias); if (nexthop.isMplsNextHop()) { @@ -685,7 +694,7 @@ bool NeighOrch::removeNextHop(const IpAddress &ipAddress, const string &alias) } m_syncdNextHops.erase(nexthop); - m_intfsOrch->decreaseRouterIntfsRefCount(alias); + m_intfsOrch->decreaseRouterIntfsRefCount(nexthop.alias); return true; } diff --git a/tests/mock_tests/neighorch_ut.cpp b/tests/mock_tests/neighorch_ut.cpp index a3a139194e9..51e3304ac74 100644 --- a/tests/mock_tests/neighorch_ut.cpp +++ b/tests/mock_tests/neighorch_ut.cpp @@ -17,7 +17,9 @@ EXTERN_MOCK_FNS namespace neighorch_test { DEFINE_SAI_API_MOCK(neighbor); + DEFINE_SAI_GENERIC_API_MOCK(next_hop, next_hop); using namespace std; + using ::testing::Invoke; using namespace mock_orch_test; using ::testing::Return; using ::testing::Throw; @@ -49,6 +51,27 @@ namespace neighorch_test neigh_table.del(key); } + void ConfigureRemoteSystemPort(const string& remote_alias, const string& inband_alias) + { + Table intf_table = Table(m_app_db.get(), APP_INTF_TABLE_NAME); + intf_table.set(remote_alias, {{"NULL", "NULL"}}); + intf_table.set(inband_alias, {{"NULL", "NULL"}}); + gIntfsOrch->addExistingData(&intf_table); + static_cast(gIntfsOrch)->doTask(); + + Port remote_port; + ASSERT_TRUE(gPortsOrch->getPort(remote_alias, remote_port)); + remote_port.m_system_port_info.type = SAI_SYSTEM_PORT_TYPE_REMOTE; + remote_port.m_oper_status = SAI_PORT_OPER_STATUS_UP; + gPortsOrch->setPort(remote_alias, remote_port); + + Port inband_port; + ASSERT_TRUE(gPortsOrch->getPort(inband_alias, inband_port)); + inband_port.m_oper_status = SAI_PORT_OPER_STATUS_UP; + gPortsOrch->setPort(inband_alias, inband_port); + gPortsOrch->m_inbandPortName = inband_alias; + } + void ApplyInitialConfigs() { Table port_table = Table(m_app_db.get(), APP_PORT_TABLE_NAME); @@ -180,12 +203,15 @@ namespace neighorch_test void PostSetUp() override { INIT_SAI_API_MOCK(neighbor); + INIT_SAI_API_MOCK(next_hop); MockSaiApis(); } void PreTearDown() override { RestoreSaiApis(); + DEINIT_SAI_API_MOCK(next_hop); + DEINIT_SAI_API_MOCK(neighbor); } }; @@ -203,6 +229,53 @@ namespace neighorch_test gIntfsOrch->m_removingIntfses.erase(VLAN_1000); } + TEST_F(NeighOrchTest, RemoteSystemPortNextHopUsesInbandRif) + { + ConfigureRemoteSystemPort(ETHERNET0, ETHERNET4); + + auto inband_rif = gIntfsOrch->getRouterIntfsId(ETHERNET4); + ASSERT_NE(inband_rif, SAI_NULL_OBJECT_ID); + auto remote_ref_count = gIntfsOrch->getSyncdIntfses().at(ETHERNET0).ref_count; + auto inband_ref_count = gIntfsOrch->getSyncdIntfses().at(ETHERNET4).ref_count; + NextHopKey inband_nexthop(TEST_IP, ETHERNET4); + + bool saw_inband_rif = false; + EXPECT_CALL(*mock_sai_next_hop_api, create_next_hop) + .WillOnce(Invoke([&](sai_object_id_t *next_hop_id, sai_object_id_t, uint32_t attr_count, + const sai_attribute_t *attr_list) { + *next_hop_id = 0x200000; + for (uint32_t i = 0; i < attr_count; ++i) + { + if (attr_list[i].id == SAI_NEXT_HOP_ATTR_ROUTER_INTERFACE_ID) + { + EXPECT_EQ(attr_list[i].value.oid, inband_rif); + saw_inband_rif = true; + } + } + return SAI_STATUS_SUCCESS; + })); + + NeighborContext ctx(NeighborEntry(TEST_IP, ETHERNET0)); + ASSERT_TRUE(gNeighOrch->addNextHop(ctx)); + ASSERT_TRUE(saw_inband_rif); + ASSERT_EQ(gNeighOrch->m_syncdNextHops.count(inband_nexthop), 1u); + ASSERT_EQ(gIntfsOrch->getSyncdIntfses().at(ETHERNET0).ref_count, remote_ref_count); + ASSERT_EQ(gIntfsOrch->getSyncdIntfses().at(ETHERNET4).ref_count, inband_ref_count + 1); + + ASSERT_TRUE(gNeighOrch->removeNextHop(IpAddress(TEST_IP), ETHERNET0)); + ASSERT_EQ(gIntfsOrch->getSyncdIntfses().at(ETHERNET4).ref_count, inband_ref_count); + + NeighborContext bulk_ctx(NeighborEntry(TEST_IP, ETHERNET0), true); + bulk_ctx.next_hop_id = 0x200001; + ASSERT_TRUE(gNeighOrch->processBulkAddNextHop(bulk_ctx)); + ASSERT_EQ(gNeighOrch->m_syncdNextHops.count(inband_nexthop), 1u); + ASSERT_EQ(gIntfsOrch->getSyncdIntfses().at(ETHERNET0).ref_count, remote_ref_count); + ASSERT_EQ(gIntfsOrch->getSyncdIntfses().at(ETHERNET4).ref_count, inband_ref_count + 1); + + ASSERT_TRUE(gNeighOrch->removeNextHop(IpAddress(TEST_IP), ETHERNET0)); + ASSERT_EQ(gIntfsOrch->getSyncdIntfses().at(ETHERNET4).ref_count, inband_ref_count); + } + TEST_F(NeighOrchTest, MultiVlanDuplicateNeighbor) { EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry); From d4831bd21f39b760fad9d475d961fb2ae090e975 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Mon, 20 Jul 2026 17:07:15 +0000 Subject: [PATCH 09/20] [neighorch]: Clarify remote next-hop port semantics Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079 Signed-off-by: Xichen96 --- orchagent/neighorch.cpp | 4 ++-- tests/mock_tests/neighorch_ut.cpp | 11 +++++++++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index 1dac5f084c7..ffc3dced9de 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -688,8 +688,8 @@ bool NeighOrch::removeNextHop(const IpAddress &ipAddress, const string &alias) if (m_syncdNextHops[nexthop].ref_count > 0) { - SWSS_LOG_ERROR("Failed to remove still referenced next hop %s on %s", - ipAddress.to_string().c_str(), alias.c_str()); + SWSS_LOG_ERROR("Failed to remove still referenced next hop %s requested on %s, tracked on %s", + ipAddress.to_string().c_str(), alias.c_str(), nexthop.alias.c_str()); return false; } diff --git a/tests/mock_tests/neighorch_ut.cpp b/tests/mock_tests/neighorch_ut.cpp index 51e3304ac74..c7f68b2d136 100644 --- a/tests/mock_tests/neighorch_ut.cpp +++ b/tests/mock_tests/neighorch_ut.cpp @@ -233,6 +233,15 @@ namespace neighorch_test { ConfigureRemoteSystemPort(ETHERNET0, ETHERNET4); + Port remote_port; + ASSERT_TRUE(gPortsOrch->getPort(ETHERNET0, remote_port)); + remote_port.m_oper_status = SAI_PORT_OPER_STATUS_DOWN; + gPortsOrch->setPort(ETHERNET0, remote_port); + + Port inband_port; + ASSERT_TRUE(gPortsOrch->getPort(ETHERNET4, inband_port)); + ASSERT_EQ(inband_port.m_oper_status, SAI_PORT_OPER_STATUS_UP); + auto inband_rif = gIntfsOrch->getRouterIntfsId(ETHERNET4); ASSERT_NE(inband_rif, SAI_NULL_OBJECT_ID); auto remote_ref_count = gIntfsOrch->getSyncdIntfses().at(ETHERNET0).ref_count; @@ -259,6 +268,7 @@ namespace neighorch_test ASSERT_TRUE(gNeighOrch->addNextHop(ctx)); ASSERT_TRUE(saw_inband_rif); ASSERT_EQ(gNeighOrch->m_syncdNextHops.count(inband_nexthop), 1u); + ASSERT_TRUE(gNeighOrch->isNextHopFlagSet(inband_nexthop, NHFLAGS_IFDOWN)); ASSERT_EQ(gIntfsOrch->getSyncdIntfses().at(ETHERNET0).ref_count, remote_ref_count); ASSERT_EQ(gIntfsOrch->getSyncdIntfses().at(ETHERNET4).ref_count, inband_ref_count + 1); @@ -269,6 +279,7 @@ namespace neighorch_test bulk_ctx.next_hop_id = 0x200001; ASSERT_TRUE(gNeighOrch->processBulkAddNextHop(bulk_ctx)); ASSERT_EQ(gNeighOrch->m_syncdNextHops.count(inband_nexthop), 1u); + ASSERT_TRUE(gNeighOrch->isNextHopFlagSet(inband_nexthop, NHFLAGS_IFDOWN)); ASSERT_EQ(gIntfsOrch->getSyncdIntfses().at(ETHERNET0).ref_count, remote_ref_count); ASSERT_EQ(gIntfsOrch->getSyncdIntfses().at(ETHERNET4).ref_count, inband_ref_count + 1); From 964650651ec5750b85b4898cebcb78150d74d113 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Wed, 22 Jul 2026 03:19:33 +1000 Subject: [PATCH 10/20] [neighorch]: Update remote next-hop RIF expectation 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 --- tests/test_virtual_chassis.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_virtual_chassis.py b/tests/test_virtual_chassis.py index a284b91e06c..03a15b16192 100644 --- a/tests/test_virtual_chassis.py +++ b/tests/test_virtual_chassis.py @@ -1250,7 +1250,7 @@ def test_remote_neighbor_add(self, vct): nexthop_entry = asic_db.get_entry("ASIC_STATE:SAI_OBJECT_TYPE_NEXT_HOP", nexthop_keys[0]) print("3:nexthop_entrty:",nexthop_entry) rif3 = nexthop_entry.get("SAI_NEXT_HOP_ATTR_ROUTER_INTERFACE_ID") - assert rif1 != rif3, "Neighbor is not replaced with new rif" + assert rif1 == rif3, "Remote neighbor next hop moved off the inband rif" #del the neighbor self.configure_neighbor(local_lc_dvs, "del", test_neigh_ip_1, test_neigh_mac_1, test_neigh_dev_2) From 3612980d47fb6b76e90b142478ab40bbbac0c0b1 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Mon, 20 Jul 2026 17:27:12 +0000 Subject: [PATCH 11/20] [neighorch]: Avoid inserting missing bulk neighbors Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079 Signed-off-by: Xichen96 --- orchagent/neighorch.cpp | 6 +++--- tests/mock_tests/neighorch_ut.cpp | 10 ++++++++++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index ffc3dced9de..b9cf23e9235 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -1911,13 +1911,13 @@ bool NeighOrch::enableNeighbors(std::list& bulk_ctx_list) for (auto ctx = bulk_ctx_list.begin(); ctx != bulk_ctx_list.end(); ctx++) { const NeighborEntry& neighborEntry = ctx->neighborEntry; - ctx->mac = m_syncdNeighbors[neighborEntry].mac; - - if (m_syncdNeighbors.find(neighborEntry) == m_syncdNeighbors.end()) + auto neighborIt = m_syncdNeighbors.find(neighborEntry); + if (neighborIt == m_syncdNeighbors.end()) { SWSS_LOG_INFO("Neighbor %s not found", neighborEntry.ip_address.to_string().c_str()); continue; } + ctx->mac = neighborIt->second.mac; if (isHwConfigured(neighborEntry)) { diff --git a/tests/mock_tests/neighorch_ut.cpp b/tests/mock_tests/neighorch_ut.cpp index c7f68b2d136..f4e94926591 100644 --- a/tests/mock_tests/neighorch_ut.cpp +++ b/tests/mock_tests/neighorch_ut.cpp @@ -306,6 +306,16 @@ namespace neighorch_test ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN2000_NEIGH), 0); } + TEST_F(NeighOrchTest, EnableNeighborsDoesNotInsertMissingNeighbor) + { + list contexts; + contexts.emplace_back(VLAN1000_NEIGH, true); + + ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN1000_NEIGH), 0u); + EXPECT_TRUE(gNeighOrch->enableNeighbors(contexts)); + EXPECT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN1000_NEIGH), 0u); + } + TEST_F(NeighOrchTest, MultiVlanUnableToRemoveNeighbor) { EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry); From 0c3a96d3f7a0c89906dda5ca8490dd312b563d61 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Mon, 20 Jul 2026 16:26:01 +0000 Subject: [PATCH 12/20] [intfsorch]: Retry explicit router interface VRF rehome Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079 Signed-off-by: Xichen96 --- orchagent/intfsorch.cpp | 194 ++++++++++++++++++++++++------ orchagent/intfsorch.h | 7 +- orchagent/neighorch.cpp | 10 +- orchagent/routeorch.cpp | 5 +- tests/mock_tests/intfsorch_ut.cpp | 194 +++++++++++++++++++++++++++++- tests/mock_tests/neighorch_ut.cpp | 14 +++ 6 files changed, 376 insertions(+), 48 deletions(-) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index f9d9aafe15b..d10694b31c2 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -122,6 +122,11 @@ bool IntfsOrch::isIntfRemovalPending(const string &alias) const return m_removingIntfses.find(alias) != m_removingIntfses.end(); } +bool IntfsOrch::isIntfVrfUpdatePending(const string &alias) const +{ + return m_pendingVrfUpdates.find(alias) != m_pendingVrfUpdates.end(); +} + bool IntfsOrch::isPrefixSubnet(const IpPrefix &ip_prefix, const string &alias) { if (m_syncdIntfses.find(alias) == m_syncdIntfses.end()) @@ -483,7 +488,8 @@ set IntfsOrch:: getSubnetRoutes() } bool IntfsOrch::setIntf(const string& alias, sai_object_id_t vrf_id, const IpPrefix *ip_prefix, - const bool adminUp, const uint32_t mtu, string loopbackAction) + const bool adminUp, const uint32_t mtu, string loopbackAction, + const bool vrfIdIsExplicit) { SWSS_LOG_ENTER(); @@ -493,6 +499,11 @@ bool IntfsOrch::setIntf(const string& alias, sai_object_id_t vrf_id, const IpPre return false; } + if (ip_prefix && isIntfVrfUpdatePending(alias)) + { + return false; + } + Port port; gPortsOrch->getPort(alias, port); @@ -506,8 +517,21 @@ bool IntfsOrch::setIntf(const string& alias, sai_object_id_t vrf_id, const IpPre intfs_entry.ref_count = 0; intfs_entry.proxy_arp = false; intfs_entry.vrf_id = vrf_id; + intfs_entry.sag_enabled = false; + // use the configured MAC address for setting router interface's attribute via SAI api + // or use the system's MAC address instead + if (port.m_mac) + { + intfs_entry.mac = port.m_mac; + } + else + { + intfs_entry.mac = gMacAddress; + } + intfs_entry.loopback_action = loopbackAction; m_syncdIntfses[alias] = intfs_entry; m_vrfOrch->increaseVrfRefCount(vrf_id); + m_pendingVrfUpdates.erase(alias); } else { @@ -516,30 +540,107 @@ bool IntfsOrch::setIntf(const string& alias, sai_object_id_t vrf_id, const IpPre } else { - if (!ip_prefix && port.m_type == Port::SUBPORT) + // Treat an explicit VRF change as a RIF replacement and retain the SET + // until the old interface has no prefixes or dependent objects. + sai_object_id_t old_vrf_id = it_intfs->second.vrf_id; + if (!ip_prefix && vrfIdIsExplicit && old_vrf_id != vrf_id) { - // port represents a sub interface - // Change sub interface config at run time - bool attrChanged = false; - if (mtu && port.m_mtu != mtu) + m_pendingVrfUpdates.insert(alias); + if (!it_intfs->second.ip_addresses.empty()) { - port.m_mtu = mtu; - attrChanged = true; + return false; + } - setRouterIntfsMtu(port); + IntfsEntry intfs_entry = it_intfs->second; + if (intfs_entry.sag_enabled) + { + SWSS_LOG_WARN("Cannot move SAG-enabled interface %s between VRFs", alias.c_str()); + return false; } - if (port.m_admin_state_up != adminUp) + Port rehome_port = port; + rehome_port.m_mac = intfs_entry.mac; + if (rehome_port.m_type == Port::SUBPORT) { - port.m_admin_state_up = adminUp; - attrChanged = true; + rehome_port.m_admin_state_up = adminUp; + if (mtu) + { + rehome_port.m_mtu = mtu; + } + } + if (!removeRouterIntfs(port)) + { + return false; + } + + rehome_port.m_rif_id = SAI_NULL_OBJECT_ID; + rehome_port.m_vr_id = SAI_NULL_OBJECT_ID; + string rehome_loopback_action = loopbackAction.empty() ? + intfs_entry.loopback_action : + loopbackAction; + try + { + if (!addRouterIntfs(vrf_id, rehome_port, rehome_loopback_action)) + { + return false; + } + } + catch (const std::exception& e) + { + Port rollback_port = rehome_port; + rollback_port.m_rif_id = SAI_NULL_OBJECT_ID; + rollback_port.m_vr_id = SAI_NULL_OBJECT_ID; + if (!addRouterIntfs(old_vrf_id, rollback_port, intfs_entry.loopback_action)) + { + SWSS_LOG_ERROR("Failed to restore interface %s after VRF move failed: %s", + alias.c_str(), e.what()); + throw; + } + SWSS_LOG_ERROR("Failed to move interface %s to VRF %s; restored the old RIF and will retry: %s", + alias.c_str(), m_vrfOrch->getVRFname(vrf_id).c_str(), e.what()); + return false; + } - setRouterIntfsAdminStatus(port); + m_vrfOrch->decreaseVrfRefCount(old_vrf_id); + m_vrfOrch->increaseVrfRefCount(vrf_id); + intfs_entry.vrf_id = vrf_id; + intfs_entry.loopback_action = rehome_loopback_action; + m_syncdIntfses[alias] = intfs_entry; + + m_pendingVrfUpdates.erase(alias); + } + else if (!ip_prefix) + { + if (vrfIdIsExplicit) + { + m_pendingVrfUpdates.erase(alias); } - if (attrChanged) + if (port.m_type == Port::SUBPORT) { - gPortsOrch->setPort(alias, port); + // port represents a sub interface + // Change sub interface config at run time + bool attrChanged = false; + if (mtu && port.m_mtu != mtu) + { + port.m_mtu = mtu; + attrChanged = true; + + setRouterIntfsMtu(port); + } + + if (port.m_admin_state_up != adminUp) + { + port.m_admin_state_up = adminUp; + attrChanged = true; + + setRouterIntfsAdminStatus(port); + } + + if (attrChanged) + { + gPortsOrch->setPort(alias, port); + } } } } @@ -644,6 +745,7 @@ bool IntfsOrch::removeIntf(const string& alias, sai_object_id_t vrf_id, const Ip gPortsOrch->decreasePortRefCount(alias); m_syncdIntfses.erase(alias); m_vrfOrch->decreaseVrfRefCount(vrf_id); + m_pendingVrfUpdates.erase(alias); if (port.m_type == Port::SUBPORT) { @@ -711,6 +813,9 @@ void IntfsOrch::doTask(Consumer &consumer) const vector& data = kfvFieldsValues(t); string vrf_name = "", vnet_name = "", nat_zone = ""; + bool vrfNameIsExplicit = false; + bool macIsExplicit = false; + bool mplsIsExplicit = false; MacAddress mac; uint32_t mtu = 0; @@ -730,6 +835,7 @@ void IntfsOrch::doTask(Consumer &consumer) if (field == "vrf_name") { vrf_name = value; + vrfNameIsExplicit = true; } else if (field == "vnet_name") { @@ -740,6 +846,7 @@ void IntfsOrch::doTask(Consumer &consumer) try { mac = MacAddress(value); + macIsExplicit = true; } catch (const std::invalid_argument &e) { @@ -750,6 +857,7 @@ void IntfsOrch::doTask(Consumer &consumer) else if (field == "mpls") { mpls = (value == "enable" ? true : false); + mplsIsExplicit = true; } else if (field == "nat_zone") { @@ -980,7 +1088,8 @@ void IntfsOrch::doTask(Consumer &consumer) adminUp = port.m_admin_state_up; } - if (!setIntf(alias, vrf_id, ip_prefix_in_key ? &ip_prefix : nullptr, adminUp, mtu, loopbackAction)) + if (!setIntf(alias, vrf_id, ip_prefix_in_key ? &ip_prefix : nullptr, adminUp, mtu, + loopbackAction, vrfNameIsExplicit)) { it++; continue; @@ -1005,7 +1114,7 @@ void IntfsOrch::doTask(Consumer &consumer) gPortsOrch->setPort(alias, port); } /* Set MPLS */ - if ((!ip_prefix_in_key) && (port.m_mpls != mpls)) + if (mplsIsExplicit && !ip_prefix_in_key && port.m_mpls != mpls) { port.m_mpls = mpls; @@ -1021,42 +1130,49 @@ void IntfsOrch::doTask(Consumer &consumer) it++; continue; } + m_syncdIntfses[alias].loopback_action = loopbackAction; } } } - if (mac) + // update mac if it is changed + if (macIsExplicit && m_syncdIntfses.find(alias) != m_syncdIntfses.end()) { - /* Get mac information and update mac of the interface*/ - sai_attribute_t attr; - attr.id = SAI_ROUTER_INTERFACE_ATTR_SRC_MAC_ADDRESS; - memcpy(attr.value.mac, mac.getMac(), sizeof(sai_mac_t)); - - /*port.m_rif_id is set in setIntf(), need get port again*/ - if (gPortsOrch->getPort(alias, port)) + if ((!ip_prefix_in_key) && (m_syncdIntfses[alias].mac != mac)) { - sai_status_t status = sai_router_intfs_api->set_router_interface_attribute(port.m_rif_id, &attr); - if (status != SAI_STATUS_SUCCESS) + // port.m_rif_id is set in setIntf(), need to get port again + if (gPortsOrch->getPort(alias, port)) { - SWSS_LOG_ERROR("Failed to set router interface mac %s for port %s, rv:%d", - mac.to_string().c_str(), port.m_alias.c_str(), status); - if (handleSaiSetStatus(SAI_API_ROUTER_INTERFACE, status) == task_need_retry) + sai_attribute_t attr; + attr.id = SAI_ROUTER_INTERFACE_ATTR_SRC_MAC_ADDRESS; + memcpy(attr.value.mac, mac.getMac(), sizeof(sai_mac_t)); + sai_status_t status = sai_router_intfs_api->set_router_interface_attribute(port.m_rif_id, &attr); + + if (status != SAI_STATUS_SUCCESS) { - it++; - continue; + SWSS_LOG_ERROR("Failed to set router interface mac %s for port %s, rv:%d", + mac.to_string().c_str(), port.m_alias.c_str(), status); + if (handleSaiSetStatus(SAI_API_ROUTER_INTERFACE, status) == task_need_retry) + { + it++; + continue; + } + } + else + { + SWSS_LOG_NOTICE("Set router interface mac %s for port %s success", + mac.to_string().c_str(), alias.c_str()); + m_syncdIntfses[alias].mac = mac; + port.m_mac = mac; + gPortsOrch->setPort(alias, port); } } else { - SWSS_LOG_NOTICE("Set router interface mac %s for port %s success", - mac.to_string().c_str(), port.m_alias.c_str()); + SWSS_LOG_ERROR("Failed to set router interface mac %s for port %s, get port fail", + mac.to_string().c_str(), alias.c_str()); } } - else - { - SWSS_LOG_ERROR("Failed to set router interface mac %s for port %s, getPort fail", - mac.to_string().c_str(), alias.c_str()); - } } if (!proxy_arp.empty()) diff --git a/orchagent/intfsorch.h b/orchagent/intfsorch.h index 58a8a83a9b1..ecad52cee5a 100644 --- a/orchagent/intfsorch.h +++ b/orchagent/intfsorch.h @@ -26,6 +26,9 @@ struct IntfsEntry int ref_count; sai_object_id_t vrf_id; bool proxy_arp; + bool sag_enabled = false; + MacAddress mac; + std::string loopback_action; }; typedef map IntfsTable; @@ -37,6 +40,7 @@ class IntfsOrch : public Orch sai_object_id_t getRouterIntfsId(const string&); bool isIntfRemovalPending(const string&) const; + bool isIntfVrfUpdatePending(const string&) const; bool isPrefixSubnet(const IpPrefix&, const string&); bool isInbandIntfInMgmtVrf(const string& alias); string getRouterIntfsAlias(const IpAddress &ip, const string &vrf_name = ""); @@ -58,7 +62,7 @@ class IntfsOrch : public Orch bool setIntfLoopbackAction(const Port &port, string actionStr); bool getSaiLoopbackAction(const string &actionStr, sai_packet_action_t &action); - bool setIntf(const string& alias, sai_object_id_t vrf_id = gVirtualRouterId, const IpPrefix *ip_prefix = nullptr, const bool adminUp = true, const uint32_t mtu = 0, string loopbackAction = ""); + bool setIntf(const string& alias, sai_object_id_t vrf_id = gVirtualRouterId, const IpPrefix *ip_prefix = nullptr, const bool adminUp = true, const uint32_t mtu = 0, string loopbackAction = "", const bool vrfIdIsExplicit = false); bool removeIntf(const string& alias, sai_object_id_t vrf_id = gVirtualRouterId, const IpPrefix *ip_prefix = nullptr); void addIp2MeRoute(sai_object_id_t vrf_id, const IpPrefix &ip_prefix); @@ -93,6 +97,7 @@ class IntfsOrch : public Orch unique_ptr
m_vidToRidTable; std::set m_removingIntfses; + std::set m_pendingVrfUpdates; std::string getRifFlexCounterTableKey(std::string s); diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index b9cf23e9235..72c1c00c53e 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -269,9 +269,10 @@ bool NeighOrch::addNextHop(NeighborContext& ctx) assert(!hasNextHop(nexthop)); if (!m_intfsOrch->isRemoteSystemPortIntf(nh.alias) && - m_intfsOrch->isIntfRemovalPending(nh.alias)) + (m_intfsOrch->isIntfRemovalPending(nh.alias) || + m_intfsOrch->isIntfVrfUpdatePending(nh.alias))) { - SWSS_LOG_INFO("Interface %s is pending removal", nh.alias.c_str()); + SWSS_LOG_INFO("Interface %s is pending removal or VRF update", nh.alias.c_str()); return false; } @@ -1223,9 +1224,10 @@ bool NeighOrch::addNeighbor(NeighborContext& ctx) bool bulk_op = ctx.bulk_op; if (!m_intfsOrch->isRemoteSystemPortIntf(alias) && - m_intfsOrch->isIntfRemovalPending(alias)) + (m_intfsOrch->isIntfRemovalPending(alias) || + m_intfsOrch->isIntfVrfUpdatePending(alias))) { - SWSS_LOG_INFO("Interface %s is pending removal", alias.c_str()); + SWSS_LOG_INFO("Interface %s is pending removal or VRF update", alias.c_str()); return false; } diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 6538cdd82b4..42cd9f6560a 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -2095,9 +2095,10 @@ bool RouteOrch::addRoute(RouteBulkContext& ctx, const NextHopGroupKey &nextHops) } if (!m_intfsOrch->isRemoteSystemPortIntf(nexthop.alias) && - m_intfsOrch->isIntfRemovalPending(nexthop.alias)) + (m_intfsOrch->isIntfRemovalPending(nexthop.alias) || + m_intfsOrch->isIntfVrfUpdatePending(nexthop.alias))) { - SWSS_LOG_INFO("Interface %s is pending removal", nexthop.alias.c_str()); + SWSS_LOG_INFO("Interface %s is pending removal or VRF update", nexthop.alias.c_str()); return false; } diff --git a/tests/mock_tests/intfsorch_ut.cpp b/tests/mock_tests/intfsorch_ut.cpp index 1b5e33c906a..dfa602ad4f9 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; + bool fail_next_rif_create = 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; @@ -29,14 +30,28 @@ namespace intfsorch_test _In_ const sai_attribute_t *attr_list) { ++create_rif_count; - return SAI_STATUS_SUCCESS; + if (fail_next_rif_create) + { + fail_next_rif_create = false; + return SAI_STATUS_INSUFFICIENT_RESOURCES; + } + for (uint32_t i = 0; i < attr_count; ++i) + { + if (attr_list[i].id == SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION) + { + saw_loopback_action = true; + last_loopback_action = static_cast(attr_list[i].value.s32); + } + } + 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( @@ -82,8 +97,11 @@ 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; + create_rif_count = 0; + remove_rif_count = 0; saw_loopback_action = false; fail_next_rif_set = false; + fail_next_rif_create = false; last_loopback_action = SAI_PACKET_ACTION_FORWARD; m_app_db = make_shared("APPL_DB", 0); @@ -483,6 +501,8 @@ 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{ @@ -536,4 +556,174 @@ namespace intfsorch_test ASSERT_TRUE(gPortsOrch->getPort("Ethernet0", port)); ASSERT_EQ(port.m_nat_zone_id, 7u); } + + TEST_F(IntfsOrchTest, IntfsOrchVrfUpdateWaitsForDrain) + { + std::deque entries{ + {"Vrf-Blue", "SET", {{"NULL", "NULL"}}} + }; + auto consumer = dynamic_cast(gVrfOrch->getExecutor(APP_VRF_TABLE_NAME)); + consumer->addToSync(entries); + static_cast(gVrfOrch)->doTask(); + + entries = { + {"Ethernet0", "SET", { + {"mtu", "9100"}, + {"loopback_action", "drop"}, + {"mpls", "enable"}, + {"mac_addr", "00:11:22:33:44:55"} + }}, + {"Ethernet0:10.0.0.1/24", "SET", {{"scope", "global"}, {"family", "IPv4"}}} + }; + consumer = dynamic_cast(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME)); + consumer->addToSync(entries); + static_cast(gIntfsOrch)->doTask(); + + Port port; + ASSERT_TRUE(gPortsOrch->getPort("Ethernet0", port)); + gIntfsOrch->increaseRouterIntfsRefCount("Ethernet0"); + + entries = { + {"Ethernet0", "SET", {{"vrf_name", "Vrf-Blue"}}}, + {"Ethernet0:10.0.0.1/24", "DEL", {}}, + {"Ethernet0:10.0.0.1/24", "SET", {{"scope", "global"}, {"family", "IPv4"}}} + }; + saw_loopback_action = false; + consumer->addToSync(entries); + + auto create_count = create_rif_count; + auto remove_count = remove_rif_count; + static_cast(gIntfsOrch)->doTask(); + ASSERT_EQ(consumer->m_toSync.size(), 2u); + ASSERT_EQ(create_count, create_rif_count); + ASSERT_EQ(remove_count, remove_rif_count); + ASSERT_TRUE(gIntfsOrch->isIntfVrfUpdatePending("Ethernet0")); + ASSERT_NE(gIntfsOrch->getRouterIntfsId("Ethernet0"), SAI_NULL_OBJECT_ID); + + static_cast(gIntfsOrch)->doTask(); + ASSERT_EQ(consumer->m_toSync.size(), 2u); + ASSERT_EQ(remove_count, remove_rif_count); + + gIntfsOrch->decreaseRouterIntfsRefCount("Ethernet0"); + static_cast(gIntfsOrch)->doTask(); + + ASSERT_TRUE(consumer->m_toSync.empty()); + ASSERT_EQ(create_count + 1, create_rif_count); + ASSERT_EQ(remove_count + 1, remove_rif_count); + ASSERT_EQ(gIntfsOrch->getSyncdIntfses().at("Ethernet0").vrf_id, + gVrfOrch->getVRFid("Vrf-Blue")); + ASSERT_EQ(gIntfsOrch->getSyncdIntfses().at("Ethernet0").ip_addresses.count( + IpPrefix("10.0.0.1/24")), + 1u); + ASSERT_TRUE(saw_loopback_action); + ASSERT_EQ(last_loopback_action, SAI_PACKET_ACTION_DROP); + ASSERT_TRUE(gPortsOrch->getPort("Ethernet0", port)); + ASSERT_TRUE(port.m_mpls); + ASSERT_EQ(port.m_mac, MacAddress("00:11:22:33:44:55")); + ASSERT_EQ(gIntfsOrch->getSyncdIntfses().at("Ethernet0").mac, + MacAddress("00:11:22:33:44:55")); + } + + TEST_F(IntfsOrchTest, IntfsOrchPartialUpdatePreservesVrf) + { + std::deque entries{ + {"Vrf-Blue", "SET", {{"NULL", "NULL"}}} + }; + auto consumer = dynamic_cast(gVrfOrch->getExecutor(APP_VRF_TABLE_NAME)); + consumer->addToSync(entries); + static_cast(gVrfOrch)->doTask(); + + entries = { + {"Ethernet0.10", "SET", { + {"vlan", "10"}, + {"admin_status", "up"}, + {"mtu", "9100"} + }} + }; + consumer = dynamic_cast(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME)); + consumer->addToSync(entries); + static_cast(gIntfsOrch)->doTask(); + + auto create_count = create_rif_count; + auto remove_count = remove_rif_count; + entries = { + {"Ethernet0.10", "SET", { + {"vrf_name", "Vrf-Blue"}, + {"admin_status", "down"}, + {"mtu", "1500"} + }} + }; + consumer->addToSync(entries); + static_cast(gIntfsOrch)->doTask(); + + ASSERT_EQ(create_count + 1, create_rif_count); + ASSERT_EQ(remove_count + 1, remove_rif_count); + ASSERT_EQ(gIntfsOrch->getSyncdIntfses().at("Ethernet0.10").vrf_id, + gVrfOrch->getVRFid("Vrf-Blue")); + + Port port; + ASSERT_TRUE(gPortsOrch->getPort("Ethernet0.10", port)); + ASSERT_FALSE(port.m_admin_state_up); + ASSERT_EQ(port.m_mtu, 1500u); + + create_count = create_rif_count; + remove_count = remove_rif_count; + entries = { + {"Ethernet0.10", "SET", {{"admin_status", "up"}}} + }; + consumer->addToSync(entries); + static_cast(gIntfsOrch)->doTask(); + + ASSERT_EQ(create_count, create_rif_count); + ASSERT_EQ(remove_count, remove_rif_count); + ASSERT_EQ(gIntfsOrch->getSyncdIntfses().at("Ethernet0.10").vrf_id, + gVrfOrch->getVRFid("Vrf-Blue")); + ASSERT_TRUE(gPortsOrch->getPort("Ethernet0.10", port)); + ASSERT_TRUE(port.m_admin_state_up); + } + + TEST_F(IntfsOrchTest, IntfsOrchVrfUpdateRollsBackCreateFailure) + { + std::deque entries{ + {"Vrf-Blue", "SET", {{"NULL", "NULL"}}} + }; + auto consumer = dynamic_cast(gVrfOrch->getExecutor(APP_VRF_TABLE_NAME)); + consumer->addToSync(entries); + static_cast(gVrfOrch)->doTask(); + + entries = { + {"Ethernet0", "SET", {{"mtu", "9100"}, {"loopback_action", "drop"}}} + }; + consumer = dynamic_cast(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME)); + consumer->addToSync(entries); + static_cast(gIntfsOrch)->doTask(); + + auto create_count = create_rif_count; + auto remove_count = remove_rif_count; + fail_next_rif_create = true; + entries = { + {"Ethernet0", "SET", {{"vrf_name", "Vrf-Blue"}}}, + {"Ethernet4", "SET", {{"loopback_action", "drop"}}} + }; + consumer->addToSync(entries); + EXPECT_NO_THROW(gIntfsOrch->doTask(*consumer)); + + ASSERT_EQ(consumer->m_toSync.size(), 1u); + ASSERT_EQ(consumer->m_toSync.begin()->first, "Ethernet0"); + ASSERT_EQ(create_count + 3, create_rif_count); + ASSERT_EQ(remove_count + 1, remove_rif_count); + ASSERT_EQ(gIntfsOrch->getSyncdIntfses().at("Ethernet0").vrf_id, gVirtualRouterId); + ASSERT_TRUE(gIntfsOrch->isIntfVrfUpdatePending("Ethernet0")); + ASSERT_NE(gIntfsOrch->getRouterIntfsId("Ethernet0"), SAI_NULL_OBJECT_ID); + ASSERT_NE(gIntfsOrch->getSyncdIntfses().find("Ethernet4"), + gIntfsOrch->getSyncdIntfses().end()); + + static_cast(gIntfsOrch)->doTask(); + + ASSERT_TRUE(consumer->m_toSync.empty()); + ASSERT_EQ(create_count + 4, create_rif_count); + ASSERT_EQ(remove_count + 2, remove_rif_count); + ASSERT_EQ(gIntfsOrch->getSyncdIntfses().at("Ethernet0").vrf_id, + gVrfOrch->getVRFid("Vrf-Blue")); + } } diff --git a/tests/mock_tests/neighorch_ut.cpp b/tests/mock_tests/neighorch_ut.cpp index f4e94926591..17037e1616c 100644 --- a/tests/mock_tests/neighorch_ut.cpp +++ b/tests/mock_tests/neighorch_ut.cpp @@ -287,6 +287,20 @@ namespace neighorch_test ASSERT_EQ(gIntfsOrch->getSyncdIntfses().at(ETHERNET4).ref_count, inband_ref_count); } + TEST_F(NeighOrchTest, LocalNeighborDependenciesRetryWhileVrfUpdatePending) + { + gIntfsOrch->m_pendingVrfUpdates.insert(VLAN_1000); + + NeighborContext neighbor_ctx(VLAN1000_NEIGH); + neighbor_ctx.mac = MacAddress(MAC1); + EXPECT_FALSE(gNeighOrch->addNeighbor(neighbor_ctx)); + + NeighborContext next_hop_ctx(VLAN1000_NEIGH); + EXPECT_FALSE(gNeighOrch->addNextHop(next_hop_ctx)); + + gIntfsOrch->m_pendingVrfUpdates.erase(VLAN_1000); + } + TEST_F(NeighOrchTest, MultiVlanDuplicateNeighbor) { EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry); From 33a7e2fedfbadea498a35b94f62666f7d8777ee9 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Mon, 20 Jul 2026 16:31:40 +0000 Subject: [PATCH 13/20] [neighorch]: Retry until inband port is available Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079 Signed-off-by: Xichen96 --- orchagent/neighorch.cpp | 21 +++++++++++++++------ tests/mock_tests/neighorch_ut.cpp | 13 +++++++++++++ 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index 72c1c00c53e..5b759319e7b 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -261,8 +261,11 @@ bool NeighOrch::addNextHop(NeighborContext& ctx) { //For remote system ports kernel nexthops are always on inband. Change the key Port inbp; - gPortsOrch->getInbandPort(inbp); - assert(inbp.m_alias.length()); + if (!gPortsOrch->getInbandPort(inbp)) + { + SWSS_LOG_INFO("Inband port is not available for remote next hop %s", nh.to_string().c_str()); + return false; + } nexthop.alias = inbp.m_alias; } @@ -421,8 +424,11 @@ bool NeighOrch::processBulkAddNextHop(NeighborContext& ctx) if (m_intfsOrch->isRemoteSystemPortIntf(nh.alias)) { Port inbp; - gPortsOrch->getInbandPort(inbp); - assert(inbp.m_alias.length()); + if (!gPortsOrch->getInbandPort(inbp)) + { + SWSS_LOG_INFO("Inband port is not available for remote next hop %s", nh.to_string().c_str()); + return false; + } nexthop.alias = inbp.m_alias; } @@ -677,8 +683,11 @@ bool NeighOrch::removeNextHop(const IpAddress &ipAddress, const string &alias) { //For remote system ports kernel nexthops are always on inband. Change the key Port inbp; - gPortsOrch->getInbandPort(inbp); - assert(inbp.m_alias.length()); + if (!gPortsOrch->getInbandPort(inbp)) + { + SWSS_LOG_INFO("Inband port is not available for remote next hop %s", nexthop.to_string().c_str()); + return false; + } nexthop.alias = inbp.m_alias; } diff --git a/tests/mock_tests/neighorch_ut.cpp b/tests/mock_tests/neighorch_ut.cpp index 17037e1616c..3f2e64b2e95 100644 --- a/tests/mock_tests/neighorch_ut.cpp +++ b/tests/mock_tests/neighorch_ut.cpp @@ -301,6 +301,19 @@ namespace neighorch_test gIntfsOrch->m_pendingVrfUpdates.erase(VLAN_1000); } + TEST_F(NeighOrchTest, RemoteSystemPortNextHopRetriesWithoutInbandPort) + { + ConfigureRemoteSystemPort(ETHERNET0, ETHERNET4); + gPortsOrch->m_inbandPortName.clear(); + + NeighborContext ctx(NeighborEntry(TEST_IP, ETHERNET0), true); + ctx.next_hop_id = 0x200001; + + EXPECT_FALSE(gNeighOrch->addNextHop(ctx)); + EXPECT_FALSE(gNeighOrch->processBulkAddNextHop(ctx)); + EXPECT_FALSE(gNeighOrch->removeNextHop(IpAddress(TEST_IP), ETHERNET0)); + } + TEST_F(NeighOrchTest, MultiVlanDuplicateNeighbor) { EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry); From d236c2be7ad0908998841aa9634a5cd8340c8289 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Mon, 20 Jul 2026 17:23:53 +0000 Subject: [PATCH 14/20] [neighorch]: Propagate inband retry failures Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079 Signed-off-by: Xichen96 --- orchagent/neighorch.cpp | 44 +++++++++++++++++++++++++++---- tests/mock_tests/neighorch_ut.cpp | 9 +++++++ 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index 5b759319e7b..8cae040cf73 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -1363,9 +1363,12 @@ bool NeighOrch::addNeighbor(NeighborContext& ctx) if (bulk_op) { SWSS_LOG_INFO("Adding neighbor entry %s on %s to bulker.", ip_address.to_string().c_str(), alias.c_str()); + if (!addNextHop(ctx)) + { + return false; + } object_statuses.emplace_back(); gNeighBulker.create_entry(&object_statuses.back(), &neighbor_entry, (uint32_t)neighbor_attrs.size(), neighbor_attrs.data()); - addNextHop(ctx); return true; } @@ -1491,6 +1494,12 @@ bool NeighOrch::removeNeighbor(NeighborContext& ctx, bool disable) bool bulk_op = ctx.bulk_op; NextHopKey nexthop = { ip_address, alias }; + auto neighborIt = m_syncdNeighbors.find(neighborEntry); + if (neighborIt == m_syncdNeighbors.end()) + { + return true; + } + sai_object_id_t port_vrf_id; port_vrf_id = gVirtualRouterId; @@ -1498,8 +1507,11 @@ bool NeighOrch::removeNeighbor(NeighborContext& ctx, bool disable) { //For remote system ports kernel nexthops are always on inband. Change the key Port inbp; - gPortsOrch->getInbandPort(inbp); - assert(inbp.m_alias.length()); + if (!gPortsOrch->getInbandPort(inbp)) + { + SWSS_LOG_INFO("Inband port is not available for remote neighbor %s", nexthop.to_string().c_str()); + return false; + } nexthop.alias = inbp.m_alias; } @@ -1514,6 +1526,9 @@ bool NeighOrch::removeNeighbor(NeighborContext& ctx, bool disable) SWSS_LOG_ERROR("Port does not exist for %s!", alias.c_str()); } + SWSS_LOG_INFO("Try to remove neighbor %s on %s", + ip_address.to_string().c_str(), alias.c_str()); + if (m_syncdNeighbors.find(neighborEntry) == m_syncdNeighbors.end()) { return true; @@ -1618,7 +1633,10 @@ bool NeighOrch::removeNeighbor(NeighborContext& ctx, bool disable) gCrmOrch->decCrmResUsedCounter(CrmResourceType::CRM_IPV6_NEIGHBOR); } - removeNextHop(ip_address, alias); + if (!removeNextHop(ip_address, nexthop.alias)) + { + return false; + } m_intfsOrch->decreaseRouterIntfsRefCount(alias); SWSS_LOG_NOTICE("Removed neighbor %s on %s", m_syncdNeighbors[neighborEntry].mac.to_string().c_str(), alias.c_str()); @@ -1767,12 +1785,25 @@ bool NeighOrch::processBulkDisableNeighbor(NeighborContext& ctx) const NeighborEntry neighborEntry = ctx.neighborEntry; string alias = neighborEntry.alias; IpAddress ip_address = neighborEntry.ip_address; + NextHopKey nexthop = { ip_address, alias }; if (m_syncdNeighbors.find(neighborEntry) == m_syncdNeighbors.end()) { return true; } + if (m_intfsOrch->isRemoteSystemPortIntf(alias)) + { + Port inbp; + if (!gPortsOrch->getInbandPort(inbp)) + { + SWSS_LOG_INFO("Inband port is not available for remote neighbor %s", nexthop.to_string().c_str()); + return false; + } + + nexthop.alias = inbp.m_alias; + } + SWSS_LOG_INFO("Checking neighbor remove entry status %s on %s.", ip_address.to_string().c_str(), m_syncdNeighbors[neighborEntry].mac.to_string().c_str()); if (isHwConfigured(neighborEntry)) @@ -1848,7 +1879,10 @@ bool NeighOrch::processBulkDisableNeighbor(NeighborContext& ctx) gCrmOrch->decCrmResUsedCounter(CrmResourceType::CRM_IPV6_NEIGHBOR); } - removeNextHop(ip_address, alias); + if (!removeNextHop(ip_address, nexthop.alias)) + { + return false; + } m_intfsOrch->decreaseRouterIntfsRefCount(alias); SWSS_LOG_NOTICE("Removed neighbor %s on %s", m_syncdNeighbors[neighborEntry].mac.to_string().c_str(), alias.c_str()); diff --git a/tests/mock_tests/neighorch_ut.cpp b/tests/mock_tests/neighorch_ut.cpp index 3f2e64b2e95..b46d48920c1 100644 --- a/tests/mock_tests/neighorch_ut.cpp +++ b/tests/mock_tests/neighorch_ut.cpp @@ -308,10 +308,19 @@ namespace neighorch_test NeighborContext ctx(NeighborEntry(TEST_IP, ETHERNET0), true); ctx.next_hop_id = 0x200001; + ctx.mac = MacAddress(MAC1); EXPECT_FALSE(gNeighOrch->addNextHop(ctx)); EXPECT_FALSE(gNeighOrch->processBulkAddNextHop(ctx)); EXPECT_FALSE(gNeighOrch->removeNextHop(IpAddress(TEST_IP), ETHERNET0)); + EXPECT_FALSE(gNeighOrch->addNeighbor(ctx)); + EXPECT_TRUE(ctx.object_statuses.empty()); + + gNeighOrch->m_syncdNeighbors[ctx.neighborEntry] = {ctx.mac, true}; + EXPECT_FALSE(gNeighOrch->removeNeighbor(ctx)); + EXPECT_EQ(gNeighOrch->m_syncdNeighbors.count(ctx.neighborEntry), 1u); + EXPECT_FALSE(gNeighOrch->processBulkDisableNeighbor(ctx)); + EXPECT_EQ(gNeighOrch->m_syncdNeighbors.count(ctx.neighborEntry), 1u); } TEST_F(NeighOrchTest, MultiVlanDuplicateNeighbor) From 7bad778fb5bddaec861236e6036888330d4421a2 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Tue, 21 Jul 2026 11:19:17 +0000 Subject: [PATCH 15/20] [neighorch]: Retry remote MPLS next hop removal Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079 Signed-off-by: Xichen96 --- orchagent/mplsrouteorch.cpp | 19 ++++++++++++++++++ orchagent/neighorch.cpp | 8 ++++++-- orchagent/nhgorch.cpp | 17 ++++++++++++++++ orchagent/routeorch.cpp | 18 +++++++++++++++++ tests/mock_tests/neighorch_ut.cpp | 8 ++++++++ tests/mock_tests/routeorch_ut.cpp | 32 +++++++++++++++++++++++++++++++ 6 files changed, 100 insertions(+), 2 deletions(-) diff --git a/orchagent/mplsrouteorch.cpp b/orchagent/mplsrouteorch.cpp index cce3fe5ca2c..4877d8b475a 100644 --- a/orchagent/mplsrouteorch.cpp +++ b/orchagent/mplsrouteorch.cpp @@ -16,6 +16,7 @@ extern sai_object_id_t gSwitchId; extern CrmOrch *gCrmOrch; extern NhgOrch *gNhgOrch; extern CbfNhgOrch *gCbfNhgOrch; +extern PortsOrch *gPortsOrch; void RouteOrch::doLabelTask(ConsumerBase& consumer) { @@ -876,6 +877,24 @@ bool RouteOrch::removeLabelRoute(LabelRouteBulkContext& ctx) return true; } + if (it_route->second.nhg_index.empty()) + { + for (const auto& nexthop : it_route->second.nhg_key.getNextHops()) + { + if (nexthop.isMplsNextHop() && + m_intfsOrch->isRemoteSystemPortIntf(nexthop.alias)) + { + Port inbp; + if (!gPortsOrch->getInbandPort(inbp)) + { + SWSS_LOG_INFO("Inband port is not available for remote MPLS next hop %s", + nexthop.to_string().c_str()); + return false; + } + } + } + } + auto& object_statuses = ctx.object_statuses; object_statuses.emplace_back(); diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index 8cae040cf73..7762db22989 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -717,8 +717,12 @@ bool NeighOrch::removeMplsNextHop(const NextHopKey& nh) { //For remote system ports kernel nexthops are always on inband. Change the key Port inbp; - gPortsOrch->getInbandPort(inbp); - assert(inbp.m_alias.length()); + if (!gPortsOrch->getInbandPort(inbp)) + { + SWSS_LOG_INFO("Inband port is not available for remote MPLS next hop %s", + nexthop.to_string().c_str()); + return false; + } nexthop.alias = inbp.m_alias; } diff --git a/orchagent/nhgorch.cpp b/orchagent/nhgorch.cpp index 548072740cc..d51db5edcdf 100644 --- a/orchagent/nhgorch.cpp +++ b/orchagent/nhgorch.cpp @@ -878,6 +878,23 @@ bool NextHopGroup::remove() { return true; } + + for (const auto& member : m_members) + { + const auto& nh_key = member.first; + if (nh_key.isMplsNextHop() && + gIntfsOrch->isRemoteSystemPortIntf(nh_key.alias)) + { + Port inbp; + if (!gPortsOrch->getInbandPort(inbp)) + { + SWSS_LOG_INFO("Inband port is not available for remote MPLS next hop %s", + nh_key.to_string().c_str()); + return false; + } + } + } + // If the group is temporary or non-recursive, update the neigh or rif ref-count and reset the ID. if (m_is_temp || (!isRecursive() && m_members.size() == 1)) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 42cd9f6560a..b25338609e6 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -2814,6 +2814,24 @@ bool RouteOrch::removeRoute(RouteBulkContext& ctx) return true; } + if (it_route != it_route_table->second.end() && it_route->second.nhg_index.empty()) + { + for (const auto& nexthop : it_route->second.nhg_key.getNextHops()) + { + if (nexthop.isMplsNextHop() && + m_intfsOrch->isRemoteSystemPortIntf(nexthop.alias)) + { + Port inbp; + if (!gPortsOrch->getInbandPort(inbp)) + { + SWSS_LOG_INFO("Inband port is not available for remote MPLS next hop %s", + nexthop.to_string().c_str()); + return false; + } + } + } + } + auto& object_statuses = ctx.object_statuses; // set to blackhole for default route diff --git a/tests/mock_tests/neighorch_ut.cpp b/tests/mock_tests/neighorch_ut.cpp index b46d48920c1..dd941d5ece7 100644 --- a/tests/mock_tests/neighorch_ut.cpp +++ b/tests/mock_tests/neighorch_ut.cpp @@ -321,6 +321,14 @@ namespace neighorch_test EXPECT_EQ(gNeighOrch->m_syncdNeighbors.count(ctx.neighborEntry), 1u); EXPECT_FALSE(gNeighOrch->processBulkDisableNeighbor(ctx)); EXPECT_EQ(gNeighOrch->m_syncdNeighbors.count(ctx.neighborEntry), 1u); + + NextHopKey remote_mpls_nexthop("push100+" + TEST_IP + "@" + ETHERNET0); + NextHopKey inband_mpls_nexthop(remote_mpls_nexthop); + inband_mpls_nexthop.alias = ETHERNET4; + gNeighOrch->m_syncdNextHops[inband_mpls_nexthop] = {0x200002, 0, 0}; + + EXPECT_FALSE(gNeighOrch->removeMplsNextHop(remote_mpls_nexthop)); + EXPECT_EQ(gNeighOrch->m_syncdNextHops.count(inband_mpls_nexthop), 1u); } TEST_F(NeighOrchTest, MultiVlanDuplicateNeighbor) diff --git a/tests/mock_tests/routeorch_ut.cpp b/tests/mock_tests/routeorch_ut.cpp index edcffef2ace..736bb06e9a8 100644 --- a/tests/mock_tests/routeorch_ut.cpp +++ b/tests/mock_tests/routeorch_ut.cpp @@ -1704,4 +1704,36 @@ namespace routeorch_test // desired_nhg_key is empty: route now directly points to NHG (no longer a temp route) ASSERT_EQ(it->second.desired_nhg_key.getSize(), 0); } + + TEST_F(RouteOrchTest, RemoteMplsRouteRemovalRetriesWithoutInbandPort) + { + Port remote_port; + ASSERT_TRUE(gPortsOrch->getPort("Ethernet0", remote_port)); + remote_port.m_system_port_info.type = SAI_SYSTEM_PORT_TYPE_REMOTE; + gPortsOrch->setPort("Ethernet0", remote_port); + gPortsOrch->m_inbandPortName.clear(); + + NextHopGroupKey nhg( + "push100+10.0.0.2@Ethernet0,push200+10.0.0.3@Ethernet0"); + + IpPrefix prefix("8.8.8.0/24"); + gRouteOrch->m_syncdRoutes[gVirtualRouterId][prefix] = RouteNhg(nhg, ""); + + RouteBulkContext route_ctx(prefix.to_string(), false); + route_ctx.vrf_id = gVirtualRouterId; + route_ctx.ip_prefix = prefix; + + EXPECT_FALSE(gRouteOrch->removeRoute(route_ctx)); + EXPECT_TRUE(route_ctx.object_statuses.empty()); + + Label label = 1000; + gRouteOrch->m_syncdLabelRoutes[gVirtualRouterId][label] = RouteNhg(nhg, ""); + + LabelRouteBulkContext label_ctx; + label_ctx.vrf_id = gVirtualRouterId; + label_ctx.label = label; + + EXPECT_FALSE(gRouteOrch->removeLabelRoute(label_ctx)); + EXPECT_TRUE(label_ctx.object_statuses.empty()); + } } From 88bb1ae956b41eb5b55736caaa883ff62b1387bc Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Wed, 22 Jul 2026 03:13:20 +1000 Subject: [PATCH 16/20] [neighorch]: Hoist remote MPLS inband preflight 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 --- orchagent/mplsrouteorch.cpp | 24 ++++++++++++++---------- orchagent/nhgorch.cpp | 26 +++++++++++++++----------- orchagent/routeorch.cpp | 24 ++++++++++++++---------- 3 files changed, 43 insertions(+), 31 deletions(-) diff --git a/orchagent/mplsrouteorch.cpp b/orchagent/mplsrouteorch.cpp index 4877d8b475a..161f14d3046 100644 --- a/orchagent/mplsrouteorch.cpp +++ b/orchagent/mplsrouteorch.cpp @@ -879,18 +879,22 @@ bool RouteOrch::removeLabelRoute(LabelRouteBulkContext& ctx) if (it_route->second.nhg_index.empty()) { - for (const auto& nexthop : it_route->second.nhg_key.getNextHops()) + const auto& nexthops = it_route->second.nhg_key.getNextHops(); + const auto remote_mpls_nexthop = find_if(nexthops.begin(), nexthops.end(), + [this](const auto& nexthop) + { + return nexthop.isMplsNextHop() && + m_intfsOrch->isRemoteSystemPortIntf(nexthop.alias); + }); + + if (remote_mpls_nexthop != nexthops.end()) { - if (nexthop.isMplsNextHop() && - m_intfsOrch->isRemoteSystemPortIntf(nexthop.alias)) + Port inbp; + if (!gPortsOrch->getInbandPort(inbp)) { - Port inbp; - if (!gPortsOrch->getInbandPort(inbp)) - { - SWSS_LOG_INFO("Inband port is not available for remote MPLS next hop %s", - nexthop.to_string().c_str()); - return false; - } + SWSS_LOG_INFO("Inband port is not available for remote MPLS next hop %s", + remote_mpls_nexthop->to_string().c_str()); + return false; } } } diff --git a/orchagent/nhgorch.cpp b/orchagent/nhgorch.cpp index d51db5edcdf..1c0678ba95a 100644 --- a/orchagent/nhgorch.cpp +++ b/orchagent/nhgorch.cpp @@ -1,3 +1,4 @@ +#include #include #include "nhgorch.h" #include "neighorch.h" @@ -879,19 +880,22 @@ bool NextHopGroup::remove() return true; } - for (const auto& member : m_members) + const auto remote_mpls_member = find_if(m_members.begin(), m_members.end(), + [](const auto& member) + { + const auto& nh_key = member.first; + return nh_key.isMplsNextHop() && + gIntfsOrch->isRemoteSystemPortIntf(nh_key.alias); + }); + + if (remote_mpls_member != m_members.end()) { - const auto& nh_key = member.first; - if (nh_key.isMplsNextHop() && - gIntfsOrch->isRemoteSystemPortIntf(nh_key.alias)) + Port inbp; + if (!gPortsOrch->getInbandPort(inbp)) { - Port inbp; - if (!gPortsOrch->getInbandPort(inbp)) - { - SWSS_LOG_INFO("Inband port is not available for remote MPLS next hop %s", - nh_key.to_string().c_str()); - return false; - } + SWSS_LOG_INFO("Inband port is not available for remote MPLS next hop %s", + remote_mpls_member->first.to_string().c_str()); + return false; } } diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index b25338609e6..d64f33e33f6 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -2816,18 +2816,22 @@ bool RouteOrch::removeRoute(RouteBulkContext& ctx) if (it_route != it_route_table->second.end() && it_route->second.nhg_index.empty()) { - for (const auto& nexthop : it_route->second.nhg_key.getNextHops()) + const auto& nexthops = it_route->second.nhg_key.getNextHops(); + const auto remote_mpls_nexthop = find_if(nexthops.begin(), nexthops.end(), + [this](const auto& nexthop) + { + return nexthop.isMplsNextHop() && + m_intfsOrch->isRemoteSystemPortIntf(nexthop.alias); + }); + + if (remote_mpls_nexthop != nexthops.end()) { - if (nexthop.isMplsNextHop() && - m_intfsOrch->isRemoteSystemPortIntf(nexthop.alias)) + Port inbp; + if (!gPortsOrch->getInbandPort(inbp)) { - Port inbp; - if (!gPortsOrch->getInbandPort(inbp)) - { - SWSS_LOG_INFO("Inband port is not available for remote MPLS next hop %s", - nexthop.to_string().c_str()); - return false; - } + SWSS_LOG_INFO("Inband port is not available for remote MPLS next hop %s", + remote_mpls_nexthop->to_string().c_str()); + return false; } } } From 3bb34af2230373da9bd89e3fec475fcd49834eca Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Wed, 22 Jul 2026 07:26:25 +0000 Subject: [PATCH 17/20] [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 dfa602ad4f9..34f65e1dfb5 100644 --- a/tests/mock_tests/intfsorch_ut.cpp +++ b/tests/mock_tests/intfsorch_ut.cpp @@ -58,13 +58,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 d4b0502ecdac76d56fdf063a633045c0fb0de977 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Wed, 22 Jul 2026 07:36:48 +0000 Subject: [PATCH 18/20] [202605][intfsorch]: Omit unavailable SAG rehome state Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079 Signed-off-by: Xichen96 --- orchagent/intfsorch.cpp | 7 ------- orchagent/intfsorch.h | 1 - 2 files changed, 8 deletions(-) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index d10694b31c2..d282e89e55b 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -517,7 +517,6 @@ bool IntfsOrch::setIntf(const string& alias, sai_object_id_t vrf_id, const IpPre intfs_entry.ref_count = 0; intfs_entry.proxy_arp = false; intfs_entry.vrf_id = vrf_id; - intfs_entry.sag_enabled = false; // use the configured MAC address for setting router interface's attribute via SAI api // or use the system's MAC address instead if (port.m_mac) @@ -552,12 +551,6 @@ bool IntfsOrch::setIntf(const string& alias, sai_object_id_t vrf_id, const IpPre } IntfsEntry intfs_entry = it_intfs->second; - if (intfs_entry.sag_enabled) - { - SWSS_LOG_WARN("Cannot move SAG-enabled interface %s between VRFs", alias.c_str()); - return false; - } - Port rehome_port = port; rehome_port.m_mac = intfs_entry.mac; if (rehome_port.m_type == Port::SUBPORT) diff --git a/orchagent/intfsorch.h b/orchagent/intfsorch.h index ecad52cee5a..2fbf73e542f 100644 --- a/orchagent/intfsorch.h +++ b/orchagent/intfsorch.h @@ -26,7 +26,6 @@ struct IntfsEntry int ref_count; sai_object_id_t vrf_id; bool proxy_arp; - bool sag_enabled = false; MacAddress mac; std::string loopback_action; }; From 76377cc28c00e54977b9d9cf93efc858a0e937b9 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Wed, 22 Jul 2026 08:08:19 +0000 Subject: [PATCH 19/20] [tests]: Preserve unrelated RIF setter behavior Carry the scoped loopback-action retry hook into the 202605 validation stack without masking unrelated router-interface attribute updates. 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 34f65e1dfb5..8e68e46b722 100644 --- a/tests/mock_tests/intfsorch_ut.cpp +++ b/tests/mock_tests/intfsorch_ut.cpp @@ -68,7 +68,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 d9033e7019d52a422fd2e2bb04d19c35110c805c Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Thu, 23 Jul 2026 17:54:15 +0000 Subject: [PATCH 20/20] [intfsorch]: Restore default MAC for zero updates Treat an explicit all-zero MAC as a request to use the switch source MAC while leaving MAC-absent partial SETs unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079 Signed-off-by: Xichen96 --- orchagent/intfsorch.cpp | 5 +++++ tests/mock_tests/intfsorch_ut.cpp | 24 ++++++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index d282e89e55b..cc63cff949c 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -1128,6 +1128,11 @@ void IntfsOrch::doTask(Consumer &consumer) } } + if (macIsExplicit && !mac) + { + mac = gMacAddress; + } + // update mac if it is changed if (macIsExplicit && m_syncdIntfses.find(alias) != m_syncdIntfses.end()) { diff --git a/tests/mock_tests/intfsorch_ut.cpp b/tests/mock_tests/intfsorch_ut.cpp index 8e68e46b722..54da871d7c5 100644 --- a/tests/mock_tests/intfsorch_ut.cpp +++ b/tests/mock_tests/intfsorch_ut.cpp @@ -625,6 +625,30 @@ namespace intfsorch_test MacAddress("00:11:22:33:44:55")); } + TEST_F(IntfsOrchTest, IntfsOrchExplicitZeroMacRestoresSystemMac) + { + std::deque entries{ + {"Ethernet0", "SET", {{"mac_addr", "00:11:22:33:44:55"}}} + }; + auto consumer = dynamic_cast(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME)); + consumer->addToSync(entries); + static_cast(gIntfsOrch)->doTask(); + + ASSERT_EQ(gIntfsOrch->getSyncdIntfses().at("Ethernet0").mac, + MacAddress("00:11:22:33:44:55")); + + entries = { + {"Ethernet0", "SET", {{"mac_addr", "00:00:00:00:00:00"}}} + }; + consumer->addToSync(entries); + static_cast(gIntfsOrch)->doTask(); + + Port port; + ASSERT_TRUE(gPortsOrch->getPort("Ethernet0", port)); + ASSERT_EQ(port.m_mac, gMacAddress); + ASSERT_EQ(gIntfsOrch->getSyncdIntfses().at("Ethernet0").mac, gMacAddress); + } + TEST_F(IntfsOrchTest, IntfsOrchPartialUpdatePreservesVrf) { std::deque entries{