From 8f2e60d03b76b4949aecabe73ecd779a8b3c4937 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 19 Jul 2026 17:49:47 +0000 Subject: [PATCH 1/2] [202605][intfsorch]: Validate VRF race and loopback fixes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079 Signed-off-by: Xichen96 --- orchagent/intfsorch.cpp | 168 +++++++++++++++--- orchagent/intfsorch.h | 6 +- orchagent/neighorch.cpp | 48 +++-- orchagent/routeorch.cpp | 12 +- tests/mock_tests/intfsorch_ut.cpp | 282 +++++++++++++++++++++++++++++- tests/mock_tests/neighorch_ut.cpp | 100 +++++++++++ 6 files changed, 580 insertions(+), 36 deletions(-) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index 22956c00b65..475f92c9aea 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -116,6 +116,17 @@ 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() || + m_pendingVrfUpdates.find(alias) != m_pendingVrfUpdates.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()) @@ -477,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(); @@ -487,6 +499,11 @@ bool IntfsOrch::setIntf(const string& alias, sai_object_id_t vrf_id, const IpPre return false; } + if (ip_prefix && m_pendingVrfUpdates.find(alias) != m_pendingVrfUpdates.end()) + { + return false; + } + Port port; gPortsOrch->getPort(alias, port); @@ -500,8 +517,18 @@ 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; + 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 { @@ -510,30 +537,101 @@ 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; + Port rehome_port = port; + rehome_port.m_mac = intfs_entry.mac; + if (rehome_port.m_type == Port::SUBPORT) + { + rehome_port.m_admin_state_up = adminUp; + if (mtu) + { + rehome_port.m_mtu = mtu; + } + } + if (!removeRouterIntfs(port)) + { + return false; } - if (port.m_admin_state_up != adminUp) + 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 { - port.m_admin_state_up = adminUp; - attrChanged = true; + 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; + } + + 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; - setRouterIntfsAdminStatus(port); + 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); + } } } } @@ -638,6 +736,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) { @@ -705,6 +804,8 @@ void IntfsOrch::doTask(Consumer &consumer) const vector& data = kfvFieldsValues(t); string vrf_name = "", vnet_name = "", nat_zone = ""; + bool vrfNameIsExplicit = false; + bool mplsIsExplicit = false; MacAddress mac; uint32_t mtu = 0; @@ -724,6 +825,7 @@ void IntfsOrch::doTask(Consumer &consumer) if (field == "vrf_name") { vrf_name = value; + vrfNameIsExplicit = true; } else if (field == "vnet_name") { @@ -744,6 +846,7 @@ void IntfsOrch::doTask(Consumer &consumer) else if (field == "mpls") { mpls = (value == "enable" ? true : false); + mplsIsExplicit = true; } else if (field == "nat_zone") { @@ -834,6 +937,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) @@ -962,7 +1074,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; @@ -987,7 +1100,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; @@ -998,7 +1111,12 @@ void IntfsOrch::doTask(Consumer &consumer) /* Set loopback action */ if (!loopbackAction.empty()) { - setIntfLoopbackAction(port, loopbackAction); + if (!setIntfLoopbackAction(port, loopbackAction)) + { + it++; + continue; + } + m_syncdIntfses[alias].loopback_action = loopbackAction; } } } @@ -1028,6 +1146,9 @@ void IntfsOrch::doTask(Consumer &consumer) { SWSS_LOG_NOTICE("Set router interface mac %s for port %s success", mac.to_string().c_str(), port.m_alias.c_str()); + m_syncdIntfses[alias].mac = mac; + port.m_mac = mac; + gPortsOrch->setPort(alias, port); } } else @@ -1129,12 +1250,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; } @@ -1779,4 +1906,3 @@ void IntfsOrch::voqSyncIntfState(string &alias, bool isUp) } } - diff --git a/orchagent/intfsorch.h b/orchagent/intfsorch.h index aa5129bef45..10940fd883b 100644 --- a/orchagent/intfsorch.h +++ b/orchagent/intfsorch.h @@ -26,6 +26,8 @@ struct IntfsEntry int ref_count; sai_object_id_t vrf_id; bool proxy_arp; + MacAddress mac; + std::string loopback_action; }; typedef map IntfsTable; @@ -36,6 +38,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 = ""); @@ -57,7 +60,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); @@ -92,6 +95,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 9c7899ef953..ee17fa03766 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -261,14 +261,22 @@ 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; } assert(!hasNextHop(nexthop)); - sai_object_id_t rif_id = m_intfsOrch->getRouterIntfsId(nh.alias); + sai_object_id_t rif_id = m_intfsOrch->getRouterIntfsIdForNewDependency(nexthop.alias); + if (rif_id == SAI_NULL_OBJECT_ID) + { + SWSS_LOG_INFO("Failed to get rif_id for %s", nexthop.alias.c_str()); + return false; + } vector next_hop_attrs; @@ -346,7 +354,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()) { @@ -405,6 +413,18 @@ bool NeighOrch::processBulkAddNextHop(NeighborContext& ctx) } NextHopKey nexthop(nh); + if (m_intfsOrch->isRemoteSystemPortIntf(nh.alias)) + { + Port inbp; + 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; + } + if (ctx.next_hop_id == SAI_NULL_OBJECT_ID) { sai_status_t bulker_status = gNextHopBulker.create_status(ctx.next_hop_id); @@ -438,7 +458,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()) { @@ -655,8 +675,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; } @@ -673,7 +696,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; } @@ -1201,7 +1224,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()); @@ -1323,10 +1346,14 @@ bool NeighOrch::addNeighbor(NeighborContext& ctx) // prefix-route neighbors do not use bulk_op if (bulk_op) { + if (!addNextHop(ctx)) + { + return false; + } + SWSS_LOG_INFO("Adding neighbor entry %s on %s to bulker.", ip_address.to_string().c_str(), alias.c_str()); object_statuses.emplace_back(); gNeighBulker.create_entry(&object_statuses.back(), &neighbor_entry, (uint32_t)neighbor_attrs.size(), neighbor_attrs.data()); - addNextHop(ctx); return true; } @@ -1902,6 +1929,7 @@ bool NeighOrch::enableNeighbors(std::list& bulk_ctx_list) if(!addNeighbor(*ctx)) { SWSS_LOG_ERROR("Neighbor %s create entry failed.", neighborEntry.ip_address.to_string().c_str()); + ret = false; continue; } } 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 e3a7d2eccd5..098ed83f619 100644 --- a/tests/mock_tests/intfsorch_ut.cpp +++ b/tests/mock_tests/intfsorch_ut.cpp @@ -16,6 +16,10 @@ namespace intfsorch_test int create_rif_count = 0; int remove_rif_count = 0; + bool saw_loopback_action = false; + bool fail_next_rif_create = 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; @@ -26,6 +30,20 @@ namespace intfsorch_test _In_ const sai_attribute_t *attr_list) { ++create_rif_count; + if (fail_next_rif_create) + { + fail_next_rif_create = false; + return SAI_STATUS_INSUFFICIENT_RESOURCES; + } + *router_interface_id = 0x100000 + create_rif_count; + 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 SAI_STATUS_SUCCESS; } @@ -36,6 +54,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 +96,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; + saw_loopback_action = false; + last_loopback_action = SAI_PACKET_ACTION_FORWARD; + fail_next_rif_create = false; + fail_next_rif_set = false; m_app_db = make_shared("APPL_DB", 0); m_config_db = make_shared("CONFIG_DB", 0); @@ -308,17 +348,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(); @@ -395,4 +448,231 @@ namespace intfsorch_test m_syncdIntfses = gIntfsOrch->getSyncdIntfses(); ASSERT_EQ(m_syncdIntfses["Loopback3"].vrf_id, gVirtualRouterId); } + + 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)); + port.m_rif_id = 0x123456; + port.m_vr_id = gVirtualRouterId; + gPortsOrch->setPort("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_EQ(gIntfsOrch->getRouterIntfsIdForNewDependency("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_EQ(gIntfsOrch->getRouterIntfsIdForNewDependency("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")); + } + + 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(); + + 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); + ASSERT_NE(gIntfsOrch->getSyncdIntfses().at("Ethernet0").loopback_action, "drop"); + + static_cast(gIntfsOrch)->doTask(); + + ASSERT_TRUE(consumer->m_toSync.empty()); + ASSERT_TRUE(saw_loopback_action); + ASSERT_EQ(last_loopback_action, SAI_PACKET_ACTION_DROP); + ASSERT_EQ(gIntfsOrch->getSyncdIntfses().at("Ethernet0").loopback_action, "drop"); + } + + TEST_F(IntfsOrchTest, IntfsOrchIgnoresInvalidLoopbackActionField) + { + 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(); + + 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); + } } \ No newline at end of file diff --git a/tests/mock_tests/neighorch_ut.cpp b/tests/mock_tests/neighorch_ut.cpp index 28c3f53adf9..db827766854 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); } }; @@ -208,6 +234,80 @@ namespace neighorch_test ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN2000_NEIGH), 0); } + TEST_F(NeighOrchTest, RemoteSystemPortNextHopUsesInbandRif) + { + ConfigureRemoteSystemPort(ETHERNET0, ETHERNET4); + + auto inband_rif = gIntfsOrch->getRouterIntfsIdForNewDependency(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, BulkRemoteSystemPortNeighborRetriesWhenInbandRifIsFenced) + { + ConfigureRemoteSystemPort(ETHERNET0, ETHERNET4); + gIntfsOrch->m_removingIntfses.insert(ETHERNET4); + + NeighborContext ctx(NeighborEntry(TEST_IP, ETHERNET0), true); + ctx.mac = MacAddress(MAC1); + + EXPECT_FALSE(gNeighOrch->addNeighbor(ctx)); + EXPECT_TRUE(ctx.object_statuses.empty()); + + gIntfsOrch->m_removingIntfses.erase(ETHERNET4); + } + + 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, MultiVlanUnableToRemoveNeighbor) { EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry); From 3cdb247f22e94d5512b8ed6041fa211aa32c8243 Mon Sep 17 00:00:00 2001 From: Xichen96 Date: Sun, 19 Jul 2026 18:09:25 +0000 Subject: [PATCH 2/2] 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 | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/mock_tests/intfsorch_ut.cpp b/tests/mock_tests/intfsorch_ut.cpp index 098ed83f619..f0c876fd2a8 100644 --- a/tests/mock_tests/intfsorch_ut.cpp +++ b/tests/mock_tests/intfsorch_ut.cpp @@ -35,6 +35,7 @@ namespace intfsorch_test fail_next_rif_create = false; return SAI_STATUS_INSUFFICIENT_RESOURCES; } + *router_interface_id = 0x100000 + create_rif_count; for (uint32_t i = 0; i < attr_count; ++i) { @@ -662,7 +663,7 @@ namespace intfsorch_test entries = { {"Ethernet0", "SET", { {"loopback_action", "invalid"}, - {"mtu", "1500"} + {"nat_zone", "7"} }} }; consumer->addToSync(entries); @@ -673,6 +674,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); } -} \ No newline at end of file +}