From 0d44beee5ceb61d9c557f4adbdac2c4e8f2c6fd4 Mon Sep 17 00:00:00 2001 From: Lawrence Lee Date: Fri, 24 May 2024 00:27:17 +0000 Subject: [PATCH 1/5] Handle learning duplicate IPs on different VRFs - If we try to learn an existing neighbor on a different VLAN in the same VRF, delete the old neighbor entry before creating the new one. - For all other scenarios, proceed with neighbor learning normally. Signed-off-by: Lawrence Lee --- orchagent/neighorch.cpp | 27 +++++- tests/mock_tests/mock_orch_test.h | 8 ++ tests/mock_tests/neighorch_ut.cpp | 133 +++++++++++++++++++++--------- 3 files changed, 125 insertions(+), 43 deletions(-) diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index a2bdebbc62..266bd6e09b 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -945,12 +945,33 @@ bool NeighOrch::addNeighbor(const NeighborEntry &neighborEntry, const MacAddress NeighborEntry temp_entry = { ip_address, vlan_port }; if (m_syncdNeighbors.find(temp_entry) != m_syncdNeighbors.end()) { - SWSS_LOG_NOTICE("Neighbor %s on %s already exists, removing before adding new neighbor", ip_address.to_string().c_str(), vlan_port.c_str()); - if (!removeNeighbor(temp_entry)) + // Neighbor already exists on another VLAN. If they belong to the same VRF, delete the old neighbor + Port existing_vlan, new_vlan; + if (!gPortsOrch->getPort(vlan_port, new_vlan)) { - SWSS_LOG_ERROR("Failed to remove neighbor %s on %s", ip_address.to_string().c_str(), vlan_port.c_str()); + SWSS_LOG_ERROR("Failed to get port for %s", vlan_port.c_str()); return false; } + if (!gPortsOrch->getPort(alias, existing_vlan)) + { + SWSS_LOG_ERROR("Failed to get port for %s", alias.c_str()); + return false; + } + if (existing_vlan.m_vr_id == new_vlan.m_vr_id) + { + if (existing_vlan.m_vr_id != 0) + { + std::string vrf_name = gDirectory.get()->getVRFname(existing_vlan.m_vr_id); + SWSS_LOG_NOTICE("Neighbor %s already learned on %s in VRF %s, removing before adding new neighbor", ip_address.to_string().c_str(), vlan_port.c_str(), vrf_name.c_str()); + } else { + SWSS_LOG_NOTICE("Neighbor %s already learned on %s, removing before adding new neighbor", ip_address.to_string().c_str(), vlan_port.c_str()); + } + if (!removeNeighbor(temp_entry)) + { + SWSS_LOG_ERROR("Failed to remove neighbor %s on %s", ip_address.to_string().c_str(), vlan_port.c_str()); + return false; + } + } } } diff --git a/tests/mock_tests/mock_orch_test.h b/tests/mock_tests/mock_orch_test.h index 0f6163523d..912eb4faeb 100644 --- a/tests/mock_tests/mock_orch_test.h +++ b/tests/mock_tests/mock_orch_test.h @@ -19,16 +19,24 @@ namespace mock_orch_test static const string PEER_IPV4_ADDRESS = "1.1.1.1"; static const string ACTIVE_INTERFACE = "Ethernet4"; static const string STANDBY_INTERFACE = "Ethernet8"; + static const string ETHERNET0 = "Ethernet0"; + static const string ETHERNET4 = "Ethernet4"; + static const string ETHERNET8 = "Ethernet8"; + static const string ETHERNET12 = "Ethernet12"; static const string ACTIVE_STATE = "active"; static const string STANDBY_STATE = "standby"; static const string STATE = "state"; static const string VLAN_1000 = "Vlan1000"; static const string VLAN_2000 = "Vlan2000"; + static const string VLAN_3000 = "Vlan3000"; + static const string VLAN_4000 = "Vlan4000"; static const string SERVER_IP1 = "192.168.0.2"; static const string SERVER_IP2 = "192.168.0.3"; static const string MAC1 = "62:f9:65:10:2f:01"; static const string MAC2 = "62:f9:65:10:2f:02"; static const string MAC3 = "62:f9:65:10:2f:03"; + static const string MAC4 = "62:f9:65:10:2f:04"; + static const string MAC5 = "62:f9:65:10:2f:05"; class MockOrchTest: public ::testing::Test { diff --git a/tests/mock_tests/neighorch_ut.cpp b/tests/mock_tests/neighorch_ut.cpp index 03957436a6..e6aaf46423 100644 --- a/tests/mock_tests/neighorch_ut.cpp +++ b/tests/mock_tests/neighorch_ut.cpp @@ -9,7 +9,6 @@ #include "mock_sai_api.h" #include "mock_orch_test.h" - EXTERN_MOCK_FNS namespace neighorch_test @@ -21,15 +20,18 @@ namespace neighorch_test using ::testing::Throw; static const string TEST_IP = "10.10.10.10"; - static const NeighborEntry VLAN1000_NEIGH = NeighborEntry(TEST_IP, VLAN_1000); + static const string VRF_3000 = "Vrf3000"; + static const NeighborEntry VLAN1000_NEIGH = NeighborEntry(TEST_IP, VLAN_1000); static const NeighborEntry VLAN2000_NEIGH = NeighborEntry(TEST_IP, VLAN_2000); + static const NeighborEntry VLAN3000_NEIGH = NeighborEntry(TEST_IP, VLAN_3000); + static const NeighborEntry VLAN4000_NEIGH = NeighborEntry(TEST_IP, VLAN_4000); - class NeighOrchTest: public MockOrchTest + class NeighOrchTest : public MockOrchTest { protected: void SetAndAssertMuxState(std::string interface, std::string state) { - MuxCable* muxCable = m_MuxOrch->getMuxCable(interface); + MuxCable *muxCable = m_MuxOrch->getMuxCable(interface); muxCable->setState(state); EXPECT_EQ(state, muxCable->getState()); } @@ -55,25 +57,43 @@ namespace neighorch_test Table neigh_table = Table(m_app_db.get(), APP_NEIGH_TABLE_NAME); Table intf_table = Table(m_app_db.get(), APP_INTF_TABLE_NAME); Table fdb_table = Table(m_app_db.get(), APP_FDB_TABLE_NAME); + Table vrf_table = Table(m_app_db.get(), APP_VRF_TABLE_NAME); auto ports = ut_helper::getInitialSaiPorts(); - port_table.set(ACTIVE_INTERFACE, ports[ACTIVE_INTERFACE]); - port_table.set(STANDBY_INTERFACE, ports[STANDBY_INTERFACE]); + port_table.set(ETHERNET0, ports[ETHERNET0]); + port_table.set(ETHERNET4, ports[ETHERNET4]); + port_table.set(ETHERNET8, ports[ETHERNET8]); port_table.set("PortConfigDone", { { "count", to_string(1) } }); port_table.set("PortInitDone", { {} }); + vrf_table.set(VRF_3000, { {"NULL", "NULL"} }); + vlan_table.set(VLAN_1000, { { "admin_status", "up" }, { "mtu", "9100" }, { "mac", "00:aa:bb:cc:dd:ee" } }); - vlan_table.set(VLAN_2000, { { "admin_status", "up"}, + vlan_table.set(VLAN_2000, { { "admin_status", "up" }, { "mtu", "9100" }, { "mac", "aa:11:bb:22:cc:33" } }); + vlan_table.set(VLAN_3000, { { "admin_status", "up" }, + { "mtu", "9100" }, + { "mac", "99:ff:88:ee:77:dd" } }); + vlan_table.set(VLAN_4000, { { "admin_status", "up" }, + { "mtu", "9100" }, + { "mac", "99:ff:88:ee:77:dd" } }); + vlan_member_table.set( + VLAN_1000 + vlan_member_table.getTableNameSeparator() + ETHERNET0, + { { "tagging_mode", "untagged" } }); + vlan_member_table.set( - VLAN_1000 + vlan_member_table.getTableNameSeparator() + ACTIVE_INTERFACE, + VLAN_2000 + vlan_member_table.getTableNameSeparator() + ETHERNET4, { { "tagging_mode", "untagged" } }); vlan_member_table.set( - VLAN_2000 + vlan_member_table.getTableNameSeparator() + STANDBY_INTERFACE, + VLAN_3000 + vlan_member_table.getTableNameSeparator() + ETHERNET8, + { { "tagging_mode", "untagged" } }); + + vlan_member_table.set( + VLAN_4000 + vlan_member_table.getTableNameSeparator() + ETHERNET12, { { "tagging_mode", "untagged" } }); intf_table.set(VLAN_1000, { { "grat_arp", "enabled" }, @@ -84,6 +104,16 @@ namespace neighorch_test { "proxy_arp", "enabled" }, { "mac_addr", "00:00:00:00:00:00" } }); + intf_table.set(VLAN_3000, { { "grat_arp", "enabled" }, + { "proxy_arp", "enabled" }, + { "vrf_name", VRF_3000 }, + { "mac_addr", "00:00:00:00:00:00" } }); + + intf_table.set(VLAN_4000, { { "grat_arp", "enabled" }, + { "proxy_arp", "enabled" }, + { "vrf_name", VRF_3000 }, + { "mac_addr", "00:00:00:00:00:00" } }); + intf_table.set( VLAN_1000 + neigh_table.getTableNameSeparator() + "192.168.0.1/24", { { "scope", "global" }, @@ -95,60 +125,56 @@ namespace neighorch_test { "scope", "global" }, { "family", "IPv4" }, }); - tunnel_table.set(MUX_TUNNEL, { { "dscp_mode", "uniform" }, - { "dst_ip", "2.2.2.2" }, - { "ecn_mode", "copy_from_outer" }, - { "encap_ecn_mode", "standard" }, - { "ttl_mode", "pipe" }, - { "tunnel_type", "IPINIP" } }); - - peer_switch_table.set(PEER_SWITCH_HOSTNAME, { { "address_ipv4", PEER_IPV4_ADDRESS } }); - - mux_cable_table.set(ACTIVE_INTERFACE, { { "server_ipv4", SERVER_IP1 + "/32" }, - { "server_ipv6", "a::a/128" }, - { "state", "auto" } }); + intf_table.set( + VLAN_3000 + neigh_table.getTableNameSeparator() + "192.168.3.1/24", { + { "scope", "global" }, + { "family", "IPv4" }, + }); - mux_cable_table.set(STANDBY_INTERFACE, { { "server_ipv4", SERVER_IP2+ "/32" }, - { "server_ipv6", "a::b/128" }, - { "state", "auto" } }); + intf_table.set( + VLAN_4000 + neigh_table.getTableNameSeparator() + "192.168.3.1/24", { + { "scope", "global" }, + { "family", "IPv4" }, + }); gPortsOrch->addExistingData(&port_table); gPortsOrch->addExistingData(&vlan_table); gPortsOrch->addExistingData(&vlan_member_table); static_cast(gPortsOrch)->doTask(); + gVrfOrch->addExistingData(&vrf_table); + static_cast(gVrfOrch)->doTask(); + gIntfsOrch->addExistingData(&intf_table); static_cast(gIntfsOrch)->doTask(); - m_TunnelDecapOrch->addExistingData(&tunnel_table); - static_cast(m_TunnelDecapOrch)->doTask(); - - m_MuxOrch->addExistingData(&peer_switch_table); - static_cast(m_MuxOrch)->doTask(); - - m_MuxOrch->addExistingData(&mux_cable_table); - static_cast(m_MuxOrch)->doTask(); - fdb_table.set( VLAN_1000 + fdb_table.getTableNameSeparator() + MAC1, { { "type", "dynamic" }, - { "port", ACTIVE_INTERFACE } }); + { "port", ETHERNET0 } }); fdb_table.set( VLAN_2000 + fdb_table.getTableNameSeparator() + MAC2, { { "type", "dynamic" }, - { "port", STANDBY_INTERFACE} }); + { "port", ETHERNET4 } }); fdb_table.set( VLAN_1000 + fdb_table.getTableNameSeparator() + MAC3, { { "type", "dynamic" }, - { "port", ACTIVE_INTERFACE} }); + { "port", ETHERNET0 } }); + + fdb_table.set( + VLAN_3000 + fdb_table.getTableNameSeparator() + MAC4, + { { "type", "dynamic" }, + { "port", ETHERNET8 } }); + + fdb_table.set( + VLAN_4000 + fdb_table.getTableNameSeparator() + MAC5, + { { "type", "dynamic" }, + { "port", ETHERNET12 } }); gFdbOrch->addExistingData(&fdb_table); static_cast(gFdbOrch)->doTask(); - - SetAndAssertMuxState(ACTIVE_INTERFACE, ACTIVE_STATE); - SetAndAssertMuxState(STANDBY_INTERFACE, STANDBY_STATE); } void PostSetUp() override @@ -165,16 +191,17 @@ namespace neighorch_test TEST_F(NeighOrchTest, MultiVlanIpLearning) { - EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry); LearnNeighbor(VLAN_1000, TEST_IP, MAC1); ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN1000_NEIGH), 1); EXPECT_CALL(*mock_sai_neighbor_api, remove_neighbor_entry); + EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry); LearnNeighbor(VLAN_2000, TEST_IP, MAC2); ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN1000_NEIGH), 0); ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN2000_NEIGH), 1); + EXPECT_CALL(*mock_sai_neighbor_api, remove_neighbor_entry); EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry); LearnNeighbor(VLAN_1000, TEST_IP, MAC3); ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN1000_NEIGH), 1); @@ -195,4 +222,30 @@ namespace neighorch_test ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN1000_NEIGH), 1); ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN2000_NEIGH), 0); } + + TEST_F(NeighOrchTest, MultiVlanDifferentVrfDuplicateNeighbor) + { + EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry); + LearnNeighbor(VLAN_1000, TEST_IP, MAC1); + ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN1000_NEIGH), 1); + + EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry); + EXPECT_CALL(*mock_sai_neighbor_api, remove_neighbor_entry).Times(0); + LearnNeighbor(VLAN_3000, TEST_IP, MAC4); + ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN1000_NEIGH), 1); + ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN3000_NEIGH), 1); + } + + TEST_F(NeighOrchTest, MultiVlanSameVrfDuplicateNeighbor) + { + EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry); + LearnNeighbor(VLAN_3000, TEST_IP, MAC4); + ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN3000_NEIGH), 1); + + EXPECT_CALL(*mock_sai_neighbor_api, remove_neighbor_entry); + EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry); + LearnNeighbor(VLAN_4000, TEST_IP, MAC5); + ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN3000_NEIGH), 0); + ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN4000_NEIGH), 1); + } } From c3e06249e2d61c90160aca5e363c373b2342f184 Mon Sep 17 00:00:00 2001 From: Lawrence Lee Date: Fri, 24 May 2024 21:43:15 +0000 Subject: [PATCH 2/5] remove unused branch Signed-off-by: Lawrence Lee --- orchagent/neighorch.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index 266bd6e09b..899959fe6d 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -963,8 +963,6 @@ bool NeighOrch::addNeighbor(const NeighborEntry &neighborEntry, const MacAddress { std::string vrf_name = gDirectory.get()->getVRFname(existing_vlan.m_vr_id); SWSS_LOG_NOTICE("Neighbor %s already learned on %s in VRF %s, removing before adding new neighbor", ip_address.to_string().c_str(), vlan_port.c_str(), vrf_name.c_str()); - } else { - SWSS_LOG_NOTICE("Neighbor %s already learned on %s, removing before adding new neighbor", ip_address.to_string().c_str(), vlan_port.c_str()); } if (!removeNeighbor(temp_entry)) { From 7f75606420fb54e90b0999e3500ee50dcfdebf65 Mon Sep 17 00:00:00 2001 From: Lawrence Lee Date: Fri, 24 May 2024 22:14:37 +0000 Subject: [PATCH 3/5] improve unit test coverage Signed-off-by: Lawrence Lee --- orchagent/neighorch.cpp | 7 +++++-- tests/mock_tests/neighorch_ut.cpp | 22 +++++++++++++++++++++- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index 899959fe6d..dcc6e0873e 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -959,9 +959,12 @@ bool NeighOrch::addNeighbor(const NeighborEntry &neighborEntry, const MacAddress } if (existing_vlan.m_vr_id == new_vlan.m_vr_id) { - if (existing_vlan.m_vr_id != 0) + std::string vrf_name = gDirectory.get()->getVRFname(existing_vlan.m_vr_id); + if (vrf_name.empty()) + { + SWSS_LOG_NOTICE("Neighbor %s already learned on %s, removing before adding new neighbor", ip_address.to_string().c_str(), vlan_port.c_str()); + } { - std::string vrf_name = gDirectory.get()->getVRFname(existing_vlan.m_vr_id); SWSS_LOG_NOTICE("Neighbor %s already learned on %s in VRF %s, removing before adding new neighbor", ip_address.to_string().c_str(), vlan_port.c_str(), vrf_name.c_str()); } if (!removeNeighbor(temp_entry)) diff --git a/tests/mock_tests/neighorch_ut.cpp b/tests/mock_tests/neighorch_ut.cpp index e6aaf46423..afa107e4c8 100644 --- a/tests/mock_tests/neighorch_ut.cpp +++ b/tests/mock_tests/neighorch_ut.cpp @@ -189,7 +189,7 @@ namespace neighorch_test } }; - TEST_F(NeighOrchTest, MultiVlanIpLearning) + TEST_F(NeighOrchTest, MultiVlanDuplicateNeighbor) { EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry); LearnNeighbor(VLAN_1000, TEST_IP, MAC1); @@ -248,4 +248,24 @@ namespace neighorch_test ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN3000_NEIGH), 0); ASSERT_EQ(gNeighOrch->m_syncdNeighbors.count(VLAN4000_NEIGH), 1); } + + TEST_F(NeighOrchTest, MultiVlanDuplicateNeighborMissingExistingVlanPort) + { + LearnNeighbor(VLAN_1000, TEST_IP, MAC1); + + EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry).Times(0); + EXPECT_CALL(*mock_sai_neighbor_api, remove_neighbor_entry).Times(0); + gPortsOrch->m_portList.erase(VLAN_1000); + LearnNeighbor(VLAN_2000, TEST_IP, MAC2); + } + + TEST_F(NeighOrchTest, MultiVlanDuplicateNeighborMissingNewVlanPort) + { + LearnNeighbor(VLAN_1000, TEST_IP, MAC1); + + EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry).Times(0); + EXPECT_CALL(*mock_sai_neighbor_api, remove_neighbor_entry).Times(0); + gPortsOrch->m_portList.erase(VLAN_2000); + LearnNeighbor(VLAN_2000, TEST_IP, MAC2); + } } From d6e63a024542d5630a752f29369580e881f2a604 Mon Sep 17 00:00:00 2001 From: Lawrence Lee Date: Tue, 28 May 2024 20:11:25 +0000 Subject: [PATCH 4/5] add missing else Signed-off-by: Lawrence Lee --- orchagent/neighorch.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index dcc6e0873e..f5e59914ba 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -963,10 +963,10 @@ bool NeighOrch::addNeighbor(const NeighborEntry &neighborEntry, const MacAddress if (vrf_name.empty()) { SWSS_LOG_NOTICE("Neighbor %s already learned on %s, removing before adding new neighbor", ip_address.to_string().c_str(), vlan_port.c_str()); - } - { + } else { SWSS_LOG_NOTICE("Neighbor %s already learned on %s in VRF %s, removing before adding new neighbor", ip_address.to_string().c_str(), vlan_port.c_str(), vrf_name.c_str()); } + if (!removeNeighbor(temp_entry)) { SWSS_LOG_ERROR("Failed to remove neighbor %s on %s", ip_address.to_string().c_str(), vlan_port.c_str()); From 3fe95c1cbd9ef834b1293277a5680732bbec50cf Mon Sep 17 00:00:00 2001 From: Lawrence Lee Date: Tue, 28 May 2024 22:12:03 +0000 Subject: [PATCH 5/5] formatting Signed-off-by: Lawrence Lee --- orchagent/neighorch.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index f5e59914ba..6e752ba09c 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -963,7 +963,9 @@ bool NeighOrch::addNeighbor(const NeighborEntry &neighborEntry, const MacAddress if (vrf_name.empty()) { SWSS_LOG_NOTICE("Neighbor %s already learned on %s, removing before adding new neighbor", ip_address.to_string().c_str(), vlan_port.c_str()); - } else { + } + else + { SWSS_LOG_NOTICE("Neighbor %s already learned on %s in VRF %s, removing before adding new neighbor", ip_address.to_string().c_str(), vlan_port.c_str(), vrf_name.c_str()); }