From ebe8de73e74cf3d9fd298b8b74b5da797c889c40 Mon Sep 17 00:00:00 2001 From: Sudharsan Dhamal Gopalarathnam Date: Wed, 1 Mar 2023 11:27:29 -0800 Subject: [PATCH] [FDB]Fixing FDB consolidated flush for Remote MACs (#2673) * [FDB]Fixing FDB consolidated flush for Remote MACs * Removing todo --- orchagent/fdborch.cpp | 36 ++++--- orchagent/fdborch.h | 7 +- .../fdborch/flush_syncd_notif_ut.cpp | 94 ++++++++++++++++++- tests/mock_tests/mock_orchagent_main.h | 1 + tests/mock_tests/ut_saihelper.cpp | 1 + 5 files changed, 122 insertions(+), 17 deletions(-) diff --git a/orchagent/fdborch.cpp b/orchagent/fdborch.cpp index 7963e074e17a..4c12ad363c49 100644 --- a/orchagent/fdborch.cpp +++ b/orchagent/fdborch.cpp @@ -109,6 +109,7 @@ bool FdbOrch::storeFdbEntryState(const FdbUpdate& update) fdbdata.bridge_port_id = update.port.m_bridge_port_id; fdbdata.type = update.type; + fdbdata.sai_fdb_type = update.sai_fdb_type; fdbdata.origin = FDB_ORIGIN_LEARN; fdbdata.remote_ip = ""; fdbdata.esi = ""; @@ -206,20 +207,19 @@ Handles the SAI_FDB_EVENT_FLUSHED notification recieved from syncd */ void FdbOrch::handleSyncdFlushNotif(const sai_object_id_t& bv_id, const sai_object_id_t& bridge_port_id, - const MacAddress& mac) + const MacAddress& mac, + const sai_fdb_entry_type_t& sai_fdb_type) { // Consolidated flush will have a zero mac MacAddress flush_mac("00:00:00:00:00:00"); - /* TODO: Read the SAI_FDB_FLUSH_ATTR_ENTRY_TYPE attr from the flush notif - and clear the entries accordingly, currently only non-static entries are flushed - */ if (bridge_port_id == SAI_NULL_OBJECT_ID && bv_id == SAI_NULL_OBJECT_ID) { for (auto itr = m_entries.begin(); itr != m_entries.end();) { auto curr = itr++; - if (curr->second.type != "static" && (curr->first.mac == mac || mac == flush_mac) && curr->second.is_flush_pending) + if (curr->second.sai_fdb_type == sai_fdb_type && + (curr->first.mac == mac || mac == flush_mac) && curr->second.is_flush_pending) { clearFdbEntry(curr->first); } @@ -233,7 +233,8 @@ void FdbOrch::handleSyncdFlushNotif(const sai_object_id_t& bv_id, auto curr = itr++; if (curr->second.bridge_port_id == bridge_port_id) { - if (curr->second.type != "static" && (curr->first.mac == mac || mac == flush_mac) && curr->second.is_flush_pending) + if (curr->second.sai_fdb_type == sai_fdb_type && + (curr->first.mac == mac || mac == flush_mac) && curr->second.is_flush_pending) { clearFdbEntry(curr->first); } @@ -248,7 +249,8 @@ void FdbOrch::handleSyncdFlushNotif(const sai_object_id_t& bv_id, auto curr = itr++; if (curr->first.bv_id == bv_id) { - if (curr->second.type != "static" && (curr->first.mac == mac || mac == flush_mac) && curr->second.is_flush_pending) + if (curr->second.sai_fdb_type == sai_fdb_type && + (curr->first.mac == mac || mac == flush_mac) && curr->second.is_flush_pending) { clearFdbEntry(curr->first); } @@ -263,7 +265,8 @@ void FdbOrch::handleSyncdFlushNotif(const sai_object_id_t& bv_id, auto curr = itr++; if (curr->first.bv_id == bv_id && curr->second.bridge_port_id == bridge_port_id) { - if (curr->second.type != "static" && (curr->first.mac == mac || mac == flush_mac) && curr->second.is_flush_pending) + if (curr->second.sai_fdb_type == sai_fdb_type && + (curr->first.mac == mac || mac == flush_mac) && curr->second.is_flush_pending) { clearFdbEntry(curr->first); } @@ -274,7 +277,8 @@ void FdbOrch::handleSyncdFlushNotif(const sai_object_id_t& bv_id, void FdbOrch::update(sai_fdb_event_t type, const sai_fdb_entry_t* entry, - sai_object_id_t bridge_port_id) + sai_object_id_t bridge_port_id, + const sai_fdb_entry_type_t &sai_fdb_type) { SWSS_LOG_ENTER(); @@ -365,6 +369,7 @@ void FdbOrch::update(sai_fdb_event_t type, attr.id = SAI_FDB_ENTRY_ATTR_TYPE; attr.value.s32 = SAI_FDB_ENTRY_TYPE_DYNAMIC; + update.sai_fdb_type = SAI_FDB_ENTRY_TYPE_DYNAMIC; attrs.push_back(attr); attr.id = SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID; @@ -399,6 +404,7 @@ void FdbOrch::update(sai_fdb_event_t type, update.add = true; update.entry.port_name = update.port.m_alias; + update.sai_fdb_type = SAI_FDB_ENTRY_TYPE_DYNAMIC; update.type = "dynamic"; update.port.m_fdb_count++; m_portsOrch->setPort(update.port.m_alias, update.port); @@ -569,6 +575,7 @@ void FdbOrch::update(sai_fdb_event_t type, } update.port.m_fdb_count++; m_portsOrch->setPort(update.port.m_alias, update.port); + update.sai_fdb_type = SAI_FDB_ENTRY_TYPE_DYNAMIC; storeFdbEntryState(update); notify(SUBJECT_TYPE_FDB_CHANGE, &update); @@ -592,7 +599,7 @@ void FdbOrch::update(sai_fdb_event_t type, SWSS_LOG_INFO("FDB Flush: [ %s , %s ] = { port: %s }", update.entry.mac.to_string().c_str(), vlanName.c_str(), update.port.m_alias.c_str()); - handleSyncdFlushNotif(entry->bv_id, bridge_port_id, update.entry.mac); + handleSyncdFlushNotif(entry->bv_id, bridge_port_id, update.entry.mac, sai_fdb_type); break; } @@ -996,6 +1003,7 @@ void FdbOrch::doTask(NotificationConsumer& consumer) { uint32_t count; sai_fdb_event_notification_data_t *fdbevent = nullptr; + sai_fdb_entry_type_t sai_fdb_type = SAI_FDB_ENTRY_TYPE_DYNAMIC; sai_deserialize_fdb_event_ntf(data, count, &fdbevent); @@ -1008,11 +1016,14 @@ void FdbOrch::doTask(NotificationConsumer& consumer) if (fdbevent[i].attr[j].id == SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID) { oid = fdbevent[i].attr[j].value.oid; - break; + } + else if (fdbevent[i].attr[j].id == SAI_FDB_ENTRY_ATTR_TYPE) + { + sai_fdb_type = (sai_fdb_entry_type_t)fdbevent[i].attr[j].value.s32; } } - this->update(fdbevent[i].event_type, &fdbevent[i].fdb_entry, oid); + this->update(fdbevent[i].event_type, &fdbevent[i].fdb_entry, oid, sai_fdb_type); } sai_deserialize_free_fdb_event_ntf(count, fdbevent); @@ -1340,6 +1351,7 @@ bool FdbOrch::addFdbEntry(const FdbEntry& entry, const string& port_name, { attr.value.s32 = (fdbData.type == "dynamic") ? SAI_FDB_ENTRY_TYPE_DYNAMIC : SAI_FDB_ENTRY_TYPE_STATIC; } + fdbData.sai_fdb_type = (sai_fdb_entry_type_t)attr.value.s32; attrs.push_back(attr); diff --git a/orchagent/fdborch.h b/orchagent/fdborch.h index d9f739823797..09bc6dcc69ed 100644 --- a/orchagent/fdborch.h +++ b/orchagent/fdborch.h @@ -36,6 +36,7 @@ struct FdbUpdate Port port; string type; bool add; + sai_fdb_entry_type_t sai_fdb_type; }; struct FdbFlushUpdate @@ -63,6 +64,7 @@ struct FdbData string remote_ip; string esi; unsigned int vni; + sai_fdb_entry_type_t sai_fdb_type; }; struct SavedFdbEntry @@ -91,7 +93,7 @@ class FdbOrch: public Orch, public Subject, public Observer } bool bake() override; - void update(sai_fdb_event_t, const sai_fdb_entry_t *, sai_object_id_t); + void update(sai_fdb_event_t, const sai_fdb_entry_t *, sai_object_id_t, const sai_fdb_entry_type_t &); void update(SubjectType type, void *cntx); bool getPort(const MacAddress&, uint16_t, Port&); @@ -125,7 +127,8 @@ class FdbOrch: public Orch, public Subject, public Observer void notifyTunnelOrch(Port& port); void clearFdbEntry(const FdbEntry&); - void handleSyncdFlushNotif(const sai_object_id_t&, const sai_object_id_t&, const MacAddress& ); + void handleSyncdFlushNotif(const sai_object_id_t&, const sai_object_id_t&, const MacAddress&, + const sai_fdb_entry_type_t&); }; #endif /* SWSS_FDBORCH_H */ diff --git a/tests/mock_tests/fdborch/flush_syncd_notif_ut.cpp b/tests/mock_tests/fdborch/flush_syncd_notif_ut.cpp index d0f8954fd86f..e6bd8bea1cde 100644 --- a/tests/mock_tests/fdborch/flush_syncd_notif_ut.cpp +++ b/tests/mock_tests/fdborch/flush_syncd_notif_ut.cpp @@ -10,6 +10,7 @@ #define ETH0 "Ethernet0" #define VLAN40 "Vlan40" +#define VXLAN_REMOTE "Vxlan_1.1.1.1" extern redisReply *mockReply; extern CrmOrch* gCrmOrch; @@ -19,6 +20,28 @@ Test Fixture */ namespace fdb_syncd_flush_test { + + sai_fdb_api_t ut_sai_fdb_api; + sai_fdb_api_t *pold_sai_fdb_api; + + sai_status_t _ut_stub_sai_create_fdb_entry ( + _In_ const sai_fdb_entry_t *fdb_entry, + _In_ uint32_t attr_count, + _In_ const sai_attribute_t *attr_list) + { + return SAI_STATUS_SUCCESS; + } + void _hook_sai_fdb_api() + { + ut_sai_fdb_api = *sai_fdb_api; + pold_sai_fdb_api = sai_fdb_api; + ut_sai_fdb_api.create_fdb_entry = _ut_stub_sai_create_fdb_entry; + sai_fdb_api = &ut_sai_fdb_api; + } + void _unhook_sai_fdb_api() + { + sai_fdb_api = pold_sai_fdb_api; + } struct FdbOrchTest : public ::testing::Test { std::shared_ptr m_config_db; @@ -40,7 +63,7 @@ namespace fdb_syncd_flush_test }; ut_helper::initSaiApi(profile); - + /* Create Switch */ sai_attribute_t attr; attr.id = SAI_SWITCH_ATTR_INIT_SWITCH; @@ -70,6 +93,8 @@ namespace fdb_syncd_flush_test // 2) Crmorch ASSERT_EQ(gCrmOrch, nullptr); gCrmOrch = new CrmOrch(m_config_db.get(), CFG_CRM_TABLE_NAME); + VxlanTunnelOrch *vxlan_tunnel_orch_1 = new VxlanTunnelOrch(m_state_db.get(), m_app_db.get(), APP_VXLAN_TUNNEL_TABLE_NAME); + gDirectory.set(vxlan_tunnel_orch_1); // Construct fdborch vector app_fdb_tables = { @@ -91,7 +116,7 @@ namespace fdb_syncd_flush_test virtual void TearDown() override { delete gCrmOrch; gCrmOrch = nullptr; - + gDirectory.m_values.clear(); ut_helper::uninitSaiApi(); } }; @@ -126,6 +151,17 @@ namespace fdb_syncd_flush_test m_portsOrch->saiOidToAlias[oid] = alias; } + void setUpVxlanPort(PortsOrch* m_portsOrch){ + /* Updates portsOrch internal cache for Ethernet0 */ + std::string alias = VXLAN_REMOTE; + sai_object_id_t oid = 0x10000000004a5; + + Port port(alias, Port::PHY); + m_portsOrch->m_portList[alias] = port; + m_portsOrch->saiOidToAlias[oid] = alias; + } + + void setUpVlanMember(PortsOrch* m_portsOrch){ /* Updates portsOrch internal cache for adding Ethernet0 into Vlan40 */ sai_object_id_t bridge_port_id = 0x3a000000002c33; @@ -136,6 +172,16 @@ namespace fdb_syncd_flush_test m_portsOrch->m_portList[VLAN40].m_members.insert(ETH0); } + void setUpVxlanMember(PortsOrch* m_portsOrch){ + /* Updates portsOrch internal cache for adding Ethernet0 into Vlan40 */ + sai_object_id_t bridge_port_id = 0x3a000000002c34; + + /* Add Bridge Port */ + m_portsOrch->m_portList[VXLAN_REMOTE].m_bridge_port_id = bridge_port_id; + m_portsOrch->saiOidToAlias[bridge_port_id] = VXLAN_REMOTE; + m_portsOrch->m_portList[VLAN40].m_members.insert(VXLAN_REMOTE); + } + void triggerUpdate(FdbOrch* m_fdborch, sai_fdb_event_t type, vector mac_addr, @@ -146,7 +192,7 @@ namespace fdb_syncd_flush_test *(entry.mac_address+i) = mac_addr[i]; } entry.bv_id = bv_id; - m_fdborch->update(type, &entry, bridge_port_id); + m_fdborch->update(type, &entry, bridge_port_id, SAI_FDB_ENTRY_TYPE_DYNAMIC); } } @@ -445,4 +491,46 @@ namespace fdb_syncd_flush_test ASSERT_EQ(m_fdborch->m_fdbStateTable.hget("Vlan40:7c:fe:90:12:22:ec", "port", port), false); ASSERT_EQ(m_fdborch->m_fdbStateTable.hget("Vlan40:7c:fe:90:12:22:ec", "type", entry_type), false); } + + /* Test Consolidated Flush with origin VXLAN */ + TEST_F(FdbOrchTest, ConsolidatedFlushAllVxLAN) + { + _hook_sai_fdb_api(); + ASSERT_NE(m_portsOrch, nullptr); + setUpVlan(m_portsOrch.get()); + setUpVxlanPort(m_portsOrch.get()); + ASSERT_NE(m_portsOrch->m_portList.find(VLAN40), m_portsOrch->m_portList.end()); + ASSERT_NE(m_portsOrch->m_portList.find(VXLAN_REMOTE), m_portsOrch->m_portList.end()); + setUpVxlanMember(m_portsOrch.get()); + + FdbData fdbData; + fdbData.bridge_port_id = SAI_NULL_OBJECT_ID; + fdbData.type = "dynamic"; + fdbData.origin = FDB_ORIGIN_VXLAN_ADVERTIZED; + fdbData.remote_ip = "1.1.1.1"; + fdbData.esi = ""; + fdbData.vni = 100; + FdbEntry entry; + + MacAddress mac1 = MacAddress("52:54:00:ac:3a:99"); + entry.mac = mac1; + entry.port_name = VXLAN_REMOTE; + + entry.bv_id = m_portsOrch->m_portList[VLAN40].m_vlan_info.vlan_oid; + m_fdborch->addFdbEntry(entry, VXLAN_REMOTE, fdbData); + + /* Make sure fdb_count is incremented as expected */ + ASSERT_EQ(m_portsOrch->m_portList[VLAN40].m_fdb_count, 1); + ASSERT_EQ(m_portsOrch->m_portList[VXLAN_REMOTE].m_fdb_count, 1); + + /* Event2: Send a Consolidated Flush response from syncd */ + vector flush_mac_addr = {0, 0, 0, 0, 0, 0}; + triggerUpdate(m_fdborch.get(), SAI_FDB_EVENT_FLUSHED, flush_mac_addr, SAI_NULL_OBJECT_ID, + SAI_NULL_OBJECT_ID); + + /* make sure fdb_counters are decremented */ + ASSERT_EQ(m_portsOrch->m_portList[VLAN40].m_fdb_count, 1); + ASSERT_EQ(m_portsOrch->m_portList[VXLAN_REMOTE].m_fdb_count, 1); + _unhook_sai_fdb_api(); + } } diff --git a/tests/mock_tests/mock_orchagent_main.h b/tests/mock_tests/mock_orchagent_main.h index f378a53de867..0d4865ac3dfd 100644 --- a/tests/mock_tests/mock_orchagent_main.h +++ b/tests/mock_tests/mock_orchagent_main.h @@ -82,3 +82,4 @@ extern sai_udf_api_t* sai_udf_api; extern sai_mpls_api_t* sai_mpls_api; extern sai_counter_api_t* sai_counter_api; extern sai_samplepacket_api_t *sai_samplepacket_api; +extern sai_fdb_api_t* sai_fdb_api; diff --git a/tests/mock_tests/ut_saihelper.cpp b/tests/mock_tests/ut_saihelper.cpp index 40594cc32cdf..b871a2c137c3 100644 --- a/tests/mock_tests/ut_saihelper.cpp +++ b/tests/mock_tests/ut_saihelper.cpp @@ -86,6 +86,7 @@ namespace ut_helper sai_api_query(SAI_API_QUEUE, (void **)&sai_queue_api); sai_api_query(SAI_API_MPLS, (void**)&sai_mpls_api); sai_api_query(SAI_API_COUNTER, (void**)&sai_counter_api); + sai_api_query(SAI_API_FDB, (void**)&sai_fdb_api); return SAI_STATUS_SUCCESS; }