diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index b5361602..ae423b32 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -38,7 +38,7 @@ extern NeighOrch *gNeighOrch; extern string gMySwitchType; extern int32_t gVoqMySwitchId; extern bool gTraditionalFlexCounter; -extern bool isChassisDbInUse(); +extern bool isVoqChassisDbInUse(); const int intfsorch_pri = 35; @@ -99,7 +99,7 @@ IntfsOrch::IntfsOrch(DBConnector *db, string tableName, VRFOrch *vrf_orch, DBCon RIF_PLUGIN_FIELD, rifRateSha); - if(isChassisDbInUse()) + if(isVoqChassisDbInUse()) { //Add subscriber to process VOQ system interface tableName = CHASSIS_APP_SYSTEM_INTERFACE_TABLE_NAME; @@ -109,6 +109,7 @@ IntfsOrch::IntfsOrch(DBConnector *db, string tableName, VRFOrch *vrf_orch, DBCon } + sai_object_id_t IntfsOrch::getRouterIntfsId(const string &alias) { Port port; @@ -1311,7 +1312,7 @@ bool IntfsOrch::addRouterIntfs(sai_object_id_t vrf_id, Port &port, string loopba SWSS_LOG_NOTICE("Create router interface %s MTU %u", port.m_alias.c_str(), port.m_mtu); - if(isChassisDbInUse()) + if(isVoqChassisDbInUse()) { // Sync the interface of local port/LAG to the SYSTEM_INTERFACE table of CHASSIS_APP_DB voqSyncAddIntf(port.m_alias); @@ -1364,7 +1365,7 @@ bool IntfsOrch::removeRouterIntfs(Port &port) SWSS_LOG_NOTICE("Remove router interface for port %s", port.m_alias.c_str()); - if(isChassisDbInUse()) + if(isVoqChassisDbInUse()) { // Sync the removal of interface of local port/LAG to the SYSTEM_INTERFACE table of CHASSIS_APP_DB voqSyncDelIntf(port.m_alias); diff --git a/orchagent/main.cpp b/orchagent/main.cpp index 57c0eccf..607f3784 100644 --- a/orchagent/main.cpp +++ b/orchagent/main.cpp @@ -83,7 +83,7 @@ bool gTraditionalFlexCounter = false; uint32_t create_switch_timeout = 0; bool gMultiAsicVoq = false; -bool isChassisDbInUse() +bool isVoqChassisDbInUse() { return gMultiAsicVoq; } diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index 1109642e..c0696972 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -9,6 +9,8 @@ #include "subscriberstatetable.h" #include "nhgorch.h" +#include + extern sai_neighbor_api_t* sai_neighbor_api; extern sai_next_hop_api_t* sai_next_hop_api; @@ -24,8 +26,9 @@ extern int32_t gVoqMySwitchId; extern BfdOrch *gBfdOrch; extern size_t gMaxBulkSize; extern string gMyHostName; +extern string gMyAsicName; -extern bool isChassisDbInUse(); +extern bool isVoqChassisDbInUse(); const int neighorch_pri = 30; @@ -48,7 +51,7 @@ NeighOrch::NeighOrch(DBConnector *appDb, string tableName, IntfsOrch *intfsOrch, gBfdOrch->attach(this); } - if(isChassisDbInUse()) + if(isVoqChassisDbInUse()) { //Add subscriber to process VOQ system neigh tableName = CHASSIS_APP_SYSTEM_NEIGH_TABLE_NAME; @@ -1233,7 +1236,7 @@ bool NeighOrch::addNeighbor(NeighborContext& ctx) NeighborUpdate update = { neighborEntry, macAddress, true }; notify(SUBJECT_TYPE_NEIGH_CHANGE, static_cast(&update)); - if(isChassisDbInUse()) + if(isVoqChassisDbInUse()) { //Sync the neighbor to add to the CHASSIS_APP_DB voqSyncAddNeigh(alias, ip_address, macAddress, neighbor_entry); @@ -1382,7 +1385,7 @@ bool NeighOrch::removeNeighbor(NeighborContext& ctx, bool disable) NeighborUpdate update = { neighborEntry, MacAddress(), false }; notify(SUBJECT_TYPE_NEIGH_CHANGE, static_cast(&update)); - if(isChassisDbInUse()) + if(isVoqChassisDbInUse()) { //Sync the neighbor to delete from the CHASSIS_APP_DB voqSyncDelNeigh(alias, ip_address); @@ -1867,11 +1870,26 @@ void NeighOrch::doVoqSystemNeighTask(Consumer &consumer) string alias = key.substr(0, found); - size_t pos = alias.find('|'); - std::string port_hostname = (pos != std::string::npos) ? alias.substr(0, pos) : alias; - if(gIntfsOrch->isLocalSystemPortIntf(alias)) - { - //Synced local neighbor. Skip + // VoQ aliases can include || even without chassis DB. + const auto alias_tokens = tokenize(alias, '|'); + std::string port_hostname = alias_tokens.empty() ? alias : alias_tokens[0]; + bool is_local_by_host_asic = false; + if (isVoqChassisDbInUse() && gMyHostName == port_hostname) + { + std::string port_asic = alias_tokens.size() > 1 ? alias_tokens[1] : ""; + std::string lower_port_asic = port_asic; + std::string lower_my_asic = gMyAsicName; + boost::algorithm::to_lower(lower_port_asic); + boost::algorithm::to_lower(lower_my_asic); + SWSS_LOG_DEBUG("doVoqSystemNeighTask: alias=%s hostname=%s asic=%s local_asic=%s", + alias.c_str(), port_hostname.c_str(), port_asic.c_str(), gMyAsicName.c_str()); + is_local_by_host_asic = (lower_port_asic == lower_my_asic); + } + bool is_local_intf = gIntfsOrch->isLocalSystemPortIntf(alias); + if(is_local_intf || is_local_by_host_asic) + { + SWSS_LOG_DEBUG("doVoqSystemNeighTask: skipping local neighbor %s (isLocalIntf=%d isLocalByHostAsic=%d)", + alias.c_str(), is_local_intf, is_local_by_host_asic); it = consumer.m_toSync.erase(it); continue; } diff --git a/orchagent/p4orch/tests/test_main.cpp b/orchagent/p4orch/tests/test_main.cpp index 852240d8..60574874 100644 --- a/orchagent/p4orch/tests/test_main.cpp +++ b/orchagent/p4orch/tests/test_main.cpp @@ -40,7 +40,7 @@ string gMySwitchType = "switch"; event_handle_t g_events_handle; bool gMultiAsicVoq = false; -bool isChassisDbInUse() +bool isVoqChassisDbInUse() { return gMultiAsicVoq; } diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index c69119e6..b6d741c7 100644 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -23,6 +23,8 @@ #include #include +#include + #include #include "net/if.h" @@ -69,7 +71,7 @@ extern int32_t gVoqMySwitchId; extern string gMyHostName; extern string gMyAsicName; extern event_handle_t g_events_handle; -extern bool isChassisDbInUse(); +extern bool isVoqChassisDbInUse(); extern bool gMultiAsicVoq; // defines ------------------------------------------------------------------------------------------------------------ @@ -1070,7 +1072,7 @@ PortsOrch::PortsOrch(DBConnector *db, DBConnector *stateDb, vector|| even without chassis DB. + const auto alias_tokens = tokenize(lag_alias, '|'); + std::string port_hostname = alias_tokens.empty() ? lag_alias : alias_tokens[0]; if (gMyHostName == port_hostname) { - it = consumer.m_toSync.erase(it); - continue; + if (isVoqChassisDbInUse()) + { + std::string port_asic = alias_tokens.size() > 1 ? alias_tokens[1] : ""; + std::string lower_port_asic = port_asic; + std::string lower_my_asic = gMyAsicName; + boost::algorithm::to_lower(lower_port_asic); + boost::algorithm::to_lower(lower_my_asic); + SWSS_LOG_DEBUG("doLagMemberTask: lag_alias=%s hostname=%s asic=%s local_asic=%s", + lag_alias.c_str(), port_hostname.c_str(), port_asic.c_str(), gMyAsicName.c_str()); + if (lower_port_asic == lower_my_asic) + { + SWSS_LOG_DEBUG("doLagMemberTask: erasing local entry %s (same host and asic)", lag_alias.c_str()); + it = consumer.m_toSync.erase(it); + continue; + } + } + else + { + SWSS_LOG_DEBUG("doLagMemberTask: erasing local entry %s (single-asic voq)", lag_alias.c_str()); + it = consumer.m_toSync.erase(it); + continue; + } } } SWSS_LOG_INFO("Failed to locate LAG %s", lag_alias.c_str()); @@ -6289,7 +6312,7 @@ void PortsOrch::doLagMemberTask(Consumer &consumer) } } - if (isChassisDbInUse() && (port.m_type != Port::SYSTEM)) + if (isVoqChassisDbInUse() && (port.m_type != Port::SYSTEM)) { //Sync to SYSTEM_LAG_MEMBER_TABLE of CHASSIS_APP_DB voqSyncAddLagMember(lag, port, status); @@ -8028,7 +8051,7 @@ bool PortsOrch::removeLag(Port lag) m_counterLagTable->hdel("", lag.m_alias); - if (isChassisDbInUse()) + if (isVoqChassisDbInUse()) { // Free the lag id, if this is local LAG @@ -8141,7 +8164,7 @@ bool PortsOrch::addLagMember(Port &lag, Port &port, string member_status) LagMemberUpdate update = { lag, port, true }; notify(SUBJECT_TYPE_LAG_MEMBER_CHANGE, static_cast(&update)); - if (isChassisDbInUse()) + if (isVoqChassisDbInUse()) { //Sync to SYSTEM_LAG_MEMBER_TABLE of CHASSIS_APP_DB voqSyncAddLagMember(lag, port, member_status); @@ -8189,7 +8212,7 @@ bool PortsOrch::removeLagMember(Port &lag, Port &port) LagMemberUpdate update = { lag, port, false }; notify(SUBJECT_TYPE_LAG_MEMBER_CHANGE, static_cast(&update)); - if (isChassisDbInUse()) + if (isVoqChassisDbInUse()) { //Sync to SYSTEM_LAG_MEMBER_TABLE of CHASSIS_APP_DB voqSyncDelLagMember(lag, port); @@ -9812,7 +9835,7 @@ void PortsOrch::updatePortOperStatus(Port &port, sai_port_oper_status_t status) } } - if(isChassisDbInUse()) + if(isVoqChassisDbInUse()) { if (gIntfsOrch->isLocalSystemPortIntf(port.m_alias)) { diff --git a/tests/mock_tests/mock_orchagent_main.cpp b/tests/mock_tests/mock_orchagent_main.cpp index 1e44533f..b44e5356 100644 --- a/tests/mock_tests/mock_orchagent_main.cpp +++ b/tests/mock_tests/mock_orchagent_main.cpp @@ -28,7 +28,7 @@ VRFOrch *gVrfOrch; void syncd_apply_view() {} bool gMultiAsicVoq = false; -bool isChassisDbInUse() +bool isVoqChassisDbInUse() { return gMultiAsicVoq; } diff --git a/tests/mock_tests/neighorch_ut.cpp b/tests/mock_tests/neighorch_ut.cpp index 7b08de08..a3a1dd78 100644 --- a/tests/mock_tests/neighorch_ut.cpp +++ b/tests/mock_tests/neighorch_ut.cpp @@ -11,9 +11,15 @@ #include "mock_orchagent_main.h" #include "mock_sai_api.h" #include "mock_orch_test.h" +#include "subscriberstatetable.h" EXTERN_MOCK_FNS +extern std::string gMySwitchType; +extern std::string gMyHostName; +extern std::string gMyAsicName; +extern bool gMultiAsicVoq; + namespace neighorch_test { DEFINE_SAI_API_MOCK(neighbor); @@ -29,6 +35,51 @@ namespace neighorch_test static const NeighborEntry VLAN3000_NEIGH = NeighborEntry(TEST_IP, VLAN_3000); static const NeighborEntry VLAN4000_NEIGH = NeighborEntry(TEST_IP, VLAN_4000); + struct VoqGlobalsGuard + { + string switch_type = gMySwitchType; + string host_name = gMyHostName; + string asic_name = gMyAsicName; + bool multi_asic_voq = gMultiAsicVoq; + + ~VoqGlobalsGuard() + { + gMySwitchType = switch_type; + gMyHostName = host_name; + gMyAsicName = asic_name; + gMultiAsicVoq = multi_asic_voq; + } + }; + + struct PortListGuard + { + string alias; + bool port_exists; + Port port; + + explicit PortListGuard(const string &alias) : alias(alias) + { + auto port_it = gPortsOrch->m_portList.find(alias); + port_exists = (port_it != gPortsOrch->m_portList.end()); + if (port_exists) + { + port = port_it->second; + } + } + + ~PortListGuard() + { + if (port_exists) + { + gPortsOrch->m_portList[alias] = port; + } + else + { + gPortsOrch->m_portList.erase(alias); + } + } + }; + class NeighOrchTest : public MockOrchTest { protected: @@ -49,6 +100,45 @@ namespace neighorch_test neigh_table.del(key); } + std::unique_ptr CreateVoqSystemNeighConsumer() + { + return std::unique_ptr(new Consumer( + new swss::SubscriberStateTable( + m_chassis_app_db.get(), + CHASSIS_APP_SYSTEM_NEIGH_TABLE_NAME, + swss::TableConsumable::DEFAULT_POP_BATCH_SIZE, + 0), + gNeighOrch, + CHASSIS_APP_SYSTEM_NEIGH_TABLE_NAME)); + } + + void AddVoqSystemNeighTask(Consumer &consumer, const string &alias) + { + string key = alias + consumer.getConsumerTable()->getTableNameSeparator() + TEST_IP; + consumer.addToSync({ key, SET_COMMAND, { { "encap_index", "1" }, { "neigh", MAC1 } } }); + } + + void SetVoqInbandPortReady() + { + string inband_alias = "Vlan4094"; + Port inband_port; + inband_port.m_alias = inband_alias; + inband_port.m_type = Port::VLAN; + gPortsOrch->m_portList[inband_alias] = inband_port; + gPortsOrch->m_inbandPortName = inband_alias; + } + + void AddRemoteSystemPort(const string &alias) + { + Port remote_system_port; + remote_system_port.m_alias = alias; + remote_system_port.m_type = Port::SYSTEM; + remote_system_port.m_rif_id = SAI_NULL_OBJECT_ID; + remote_system_port.m_system_port_info.alias = alias; + remote_system_port.m_system_port_info.type = SAI_SYSTEM_PORT_TYPE_REMOTE; + gPortsOrch->m_portList[alias] = remote_system_port; + } + void ApplyInitialConfigs() { Table port_table = Table(m_app_db.get(), APP_PORT_TABLE_NAME); @@ -189,6 +279,46 @@ namespace neighorch_test } }; + TEST_F(NeighOrchTest, SystemNeighFromDifferentAsicOnSameHost) + { + VoqGlobalsGuard guard; + gMySwitchType = "voq"; + gMyHostName = "Linecard1"; + gMyAsicName = "Asic0"; + gMultiAsicVoq = true; + SetVoqInbandPortReady(); + + auto consumer = CreateVoqSystemNeighConsumer(); + string remote_asic_alias = gMyHostName + "|Asic1|Ethernet999"; + PortListGuard port_guard(remote_asic_alias); + AddRemoteSystemPort(remote_asic_alias); + AddVoqSystemNeighTask(*consumer, remote_asic_alias); + + gNeighOrch->doVoqSystemNeighTask(*consumer); + + ASSERT_EQ(consumer->m_toSync.size(), 1u); + EXPECT_EQ(kfvKey(consumer->m_toSync.begin()->second), + remote_asic_alias + consumer->getConsumerTable()->getTableNameSeparator() + TEST_IP); + } + + TEST_F(NeighOrchTest, SystemNeighFromSameAsicOnSameHost) + { + VoqGlobalsGuard guard; + gMySwitchType = "voq"; + gMyHostName = "Linecard1"; + gMyAsicName = "Asic0"; + gMultiAsicVoq = true; + SetVoqInbandPortReady(); + + auto consumer = CreateVoqSystemNeighConsumer(); + string local_asic_alias = gMyHostName + "|asic0|Ethernet999"; + AddVoqSystemNeighTask(*consumer, local_asic_alias); + + gNeighOrch->doVoqSystemNeighTask(*consumer); + + ASSERT_TRUE(consumer->m_toSync.empty()); + } + TEST_F(NeighOrchTest, MultiVlanDuplicateNeighbor) { EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry); diff --git a/tests/mock_tests/portsorch_ut.cpp b/tests/mock_tests/portsorch_ut.cpp index 7ca069d1..69534edb 100644 --- a/tests/mock_tests/portsorch_ut.cpp +++ b/tests/mock_tests/portsorch_ut.cpp @@ -22,6 +22,10 @@ extern redisReply *mockReply; extern sai_redis_communication_mode_t gRedisCommunicationMode; +extern string gMySwitchType; +extern string gMyHostName; +extern string gMyAsicName; +extern bool gMultiAsicVoq; using ::testing::_; using ::testing::StrictMock; @@ -34,6 +38,20 @@ namespace portsorch_test // SAI default ports std::map> defaultPortList; + struct VoqGlobalsGuard + { + string switch_type = gMySwitchType; + string asic_name = gMyAsicName; + bool multi_asic_voq = gMultiAsicVoq; + + ~VoqGlobalsGuard() + { + gMySwitchType = switch_type; + gMyAsicName = asic_name; + gMultiAsicVoq = multi_asic_voq; + } + }; + sai_port_api_t ut_sai_port_api; sai_port_api_t *pold_sai_port_api; sai_switch_api_t ut_sai_switch_api; @@ -4413,6 +4431,93 @@ namespace portsorch_test ASSERT_FALSE(ts.empty()); } + /* Test that a LAG member entry from a different ASIC on the same host is NOT + * erased from the consumer table in VoQ mode. The entry has the same hostname + * but a different ASIC name, so it must remain pending. + */ + TEST_F(PortsOrchTest, LagMemberFromDifferentAsicOnSameHost) + { + VoqGlobalsGuard guard; + gMySwitchType = "voq"; + gMyAsicName = "Asic0"; + gMultiAsicVoq = true; + + Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME); + Table lagMemberTable = Table(m_app_db.get(), APP_LAG_MEMBER_TABLE_NAME); + + auto ports = ut_helper::getInitialSaiPorts(); + + for (const auto &it : ports) + { + portTable.set(it.first, it.second); + } + + portTable.set("PortConfigDone", { { "count", to_string(ports.size()) } }); + portTable.set("PortInitDone", { { } }); + + // LAG member from same hostname but different ASIC (Asic1 vs local Asic0) + string remoteAsicLag = gMyHostName + "|Asic1|PortChannel999"; + lagMemberTable.set( + remoteAsicLag + lagMemberTable.getTableNameSeparator() + ports.begin()->first, + { {"status", "enabled"} }); + + gPortsOrch->addExistingData(&portTable); + gPortsOrch->addExistingData(&lagMemberTable); + + static_cast(gPortsOrch)->doTask(); + + // Entry must NOT be erased — it's for a different ASIC on the same host + vector ts; + auto exec = gPortsOrch->getExecutor(APP_LAG_MEMBER_TABLE_NAME); + auto consumer = static_cast(exec); + consumer->dumpPendingTasks(ts); + ASSERT_FALSE(ts.empty()); + } + + /* Test that a LAG member entry from the same ASIC on the same host IS erased + * from the consumer table in VoQ mode. Both hostname and ASIC name match the + * local instance, so the entry is a duplicate local reference and should be + * silently dropped. + */ + TEST_F(PortsOrchTest, LagMemberFromSameAsicOnSameHost) + { + VoqGlobalsGuard guard; + gMySwitchType = "voq"; + gMyAsicName = "Asic0"; + gMultiAsicVoq = true; + + Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME); + Table lagMemberTable = Table(m_app_db.get(), APP_LAG_MEMBER_TABLE_NAME); + + auto ports = ut_helper::getInitialSaiPorts(); + + for (const auto &it : ports) + { + portTable.set(it.first, it.second); + } + + portTable.set("PortConfigDone", { { "count", to_string(ports.size()) } }); + portTable.set("PortInitDone", { { } }); + + // LAG member from same hostname AND same ASIC (Asic0 matches local gMyAsicName) + string localAsicLag = gMyHostName + "|asic0|PortChannel999"; + lagMemberTable.set( + localAsicLag + lagMemberTable.getTableNameSeparator() + ports.begin()->first, + { {"status", "enabled"} }); + + gPortsOrch->addExistingData(&portTable); + gPortsOrch->addExistingData(&lagMemberTable); + + static_cast(gPortsOrch)->doTask(); + + // Entry MUST be erased — same hostname and same ASIC means local duplicate + vector ts; + auto exec = gPortsOrch->getExecutor(APP_LAG_MEMBER_TABLE_NAME); + auto consumer = static_cast(exec); + consumer->dumpPendingTasks(ts); + ASSERT_TRUE(ts.empty()); + } + /* This test checks that a LAG member validation happens on orchagent level * and no SAI call is executed in case a port requested to be a LAG member * is already a LAG member.