[FDB] [202012] Fix fbdorch to properly handle syncd FDB FLUSH Notif (sonic-net#2401)

vivekrnv · vivekrnv · commit 26b12bf0a266 · 2022-08-30T00:13:47.000Z
* [FDB] Fix fbdorch to properly handle syncd FDB FLUSH Notif (sonic-net#2254) Vlan delete couldn't be handled by OA when there is fdb learnt on the member and when the member is deleted This inability of handling APPL_DB notif is affecting warm-restart. FDB Entry from State DB is not removed. OA doesn't have the logic to handle consolidate flush notif coming from syncd FdbOrch doesn't have logic to clear internal cache and decrement corresponding fdb counters during a flush notification Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>
diff --git a/orchagent/fdborch.cpp b/orchagent/fdborch.cpp
@@ -17,7 +17,6 @@
 extern sai_fdb_api_t    *sai_fdb_api;
 
 extern sai_object_id_t  gSwitchId;
-extern PortsOrch*       gPortsOrch;
 extern CrmOrch *        gCrmOrch;
 extern Directory<Orch*> gDirectory;
 
@@ -151,6 +150,104 @@ bool FdbOrch::storeFdbEntryState(const FdbUpdate& update)
     }
 }
 
+/*
+clears stateDb and decrements corresponding internal fdb counters
+*/
+void FdbOrch::clearFdbEntry(const FdbEntry& entry)
+{
+    FdbUpdate update;
+    update.entry = entry;
+    update.add = false;
+
+    /* Fetch Vlan and decrement the counter */
+    Port temp_vlan;
+    if (m_portsOrch->getPort(entry.bv_id, temp_vlan))
+    {
+        m_portsOrch->decrFdbCount(temp_vlan.m_alias, 1);
+    }
+
+    /* Decrement port fdb_counter */
+    m_portsOrch->decrFdbCount(entry.port_name, 1);
+
+    /* Remove the FdbEntry from the internal cache, update state DB and CRM counter */
+    storeFdbEntryState(update);
+    notify(SUBJECT_TYPE_FDB_CHANGE, &update);
+
+    SWSS_LOG_INFO("FdbEntry removed from internal cache, MAC: %s , port: %s, BVID: 0x%" PRIx64,
+                   update.entry.mac.to_string().c_str(), update.entry.port_name.c_str(), update.entry.bv_id);
+}
+
+/*
+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)
+{
+    // 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))
+            {
+                clearFdbEntry(curr->first);
+            }
+        }
+    }
+    else if (bv_id == SAI_NULL_OBJECT_ID)
+    {
+        /* FLUSH based on PORT */
+        for (auto itr = m_entries.begin(); itr != m_entries.end();)
+        {
+            auto curr = itr++;
+            if (curr->second.bridge_port_id == bridge_port_id)
+            {
+                if (curr->second.type != "static" && (curr->first.mac == mac || mac == flush_mac))
+                {
+                    clearFdbEntry(curr->first);
+                }
+            }
+        }
+    }
+    else if (bridge_port_id == SAI_NULL_OBJECT_ID)
+    {
+        /* FLUSH based on BV_ID */
+        for (auto itr = m_entries.begin(); itr != m_entries.end();)
+        {
+            auto curr = itr++;
+            if (curr->first.bv_id == bv_id)
+            {
+                if (curr->second.type != "static" && (curr->first.mac == mac || mac == flush_mac))
+                {
+                    clearFdbEntry(curr->first);
+                }
+            }
+        }
+    }
+    else
+    {
+        /* FLUSH based on port and VLAN */
+        for (auto itr = m_entries.begin(); itr != m_entries.end();)
+        {
+            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))
+                {
+                    clearFdbEntry(curr->first);
+                }
+            }
+        }
+    }
+}
+
 void FdbOrch::update(sai_fdb_event_t        type,
                      const sai_fdb_entry_t* entry,
                      sai_object_id_t        bridge_port_id)
@@ -168,24 +265,29 @@ void FdbOrch::update(sai_fdb_event_t        type,
                    type, update.entry.mac.to_string().c_str(),
                    entry->bv_id, bridge_port_id);
 
-
     if (bridge_port_id &&
         !m_portsOrch->getPortByBridgePortId(bridge_port_id, update.port))
     {
         if (type == SAI_FDB_EVENT_FLUSHED)
         {
-            /* In case of flush - can be ignored due to a race.
-               There are notifications about FDB FLUSH (syncd/sai_redis) on port,
-               which was already removed by orchagent as a result of
-               removeVlanMember action (removeBridgePort) */
+            /* There are notifications about FDB FLUSH (syncd/sai_redis) on port,
+               which was already removed by orchagent as a result of removeVlanMember
+               action (removeBridgePort). But the internal cleanup of statedb and
+               internal counters is yet to be performed, thus continue
+            */
             SWSS_LOG_INFO("Flush event: Failed to get port by bridge port ID 0x%" PRIx64 ".",
                         bridge_port_id);
-
         } else {
             SWSS_LOG_ERROR("Failed to get port by bridge port ID 0x%" PRIx64 ".",
                         bridge_port_id);
-
+            return;
         }
+    }
+
+    if (entry->bv_id &&
+        !m_portsOrch->getPort(entry->bv_id, vlan))
+    {
+        SWSS_LOG_NOTICE("FdbOrch notification type %d: Failed to locate vlan port from bv_id 0x%" PRIx64, type, entry->bv_id);
         return;
     }
 
@@ -195,12 +297,6 @@ void FdbOrch::update(sai_fdb_event_t        type,
     {
         SWSS_LOG_INFO("Received LEARN event for bvid=0x%" PRIx64 "mac=%s port=0x%" PRIx64, entry->bv_id, update.entry.mac.to_string().c_str(), bridge_port_id);
 
-        if (!m_portsOrch->getPort(entry->bv_id, vlan))
-        {
-            SWSS_LOG_ERROR("FdbOrch LEARN notification: Failed to locate vlan port from bv_id 0x%" PRIx64, entry->bv_id);
-            return;
-        }
-
         // we already have such entries
         auto existing_entry = m_entries.find(update.entry);
         if (existing_entry != m_entries.end())
@@ -229,11 +325,6 @@ void FdbOrch::update(sai_fdb_event_t        type,
         SWSS_LOG_INFO("Received AGE event for bvid=0x%" PRIx64 " mac=%s port=0x%" PRIx64,
                        entry->bv_id, update.entry.mac.to_string().c_str(), bridge_port_id);
 
-        if (!m_portsOrch->getPort(entry->bv_id, vlan))
-        {
-            SWSS_LOG_NOTICE("FdbOrch AGE notification: Failed to locate vlan port from bv_id 0x%" PRIx64, entry->bv_id);
-        }
-
         auto existing_entry = m_entries.find(update.entry);
         // we don't have such entries
         if (existing_entry == m_entries.end())
@@ -326,12 +417,6 @@ void FdbOrch::update(sai_fdb_event_t        type,
         SWSS_LOG_INFO("Received MOVE event for bvid=0x%" PRIx64 " mac=%s port=0x%" PRIx64,
                        entry->bv_id, update.entry.mac.to_string().c_str(), bridge_port_id);
 
-        if (!m_portsOrch->getPort(entry->bv_id, vlan))
-        {
-            SWSS_LOG_ERROR("FdbOrch MOVE notification: Failed to locate vlan port from bv_id 0x%" PRIx64, entry->bv_id);
-            return;
-        }
-
         // We should already have such entry
         if (existing_entry == m_entries.end())
         {
@@ -369,86 +454,15 @@ void FdbOrch::update(sai_fdb_event_t        type,
                        bridge_port_id);
 
         string vlanName = "-";
-        if (entry->bv_id) {
-            Port vlan;
-
-            if (!m_portsOrch->getPort(entry->bv_id, vlan))
-            {
-                SWSS_LOG_NOTICE("FdbOrch notification: Failed to locate vlan\
-                                port from bv_id 0x%" PRIx64, entry->bv_id);
-                return;
-            }
+        if (!vlan.m_alias.empty()) {
             vlanName = "Vlan" + to_string(vlan.m_vlan_info.vlan_id);
         }
 
+        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());
 
-        if (bridge_port_id == SAI_NULL_OBJECT_ID &&
-            entry->bv_id == SAI_NULL_OBJECT_ID)
-        {
-            SWSS_LOG_INFO("FDB Flush: [ %s , %s ] = { port: - }",
-                           update.entry.mac.to_string().c_str(), vlanName.c_str());
-            for (auto itr = m_entries.begin(); itr != m_entries.end();)
-            {
-                /*
-                   TODO: here should only delete the dynamic fdb entries,
-                   but unfortunately in structure FdbEntry currently have
-                   no member to indicate the fdb entry type,
-                   if there is static mac added, here will have issue.
-                */
-                update.entry.mac = itr->first.mac;
-                update.entry.bv_id = itr->first.bv_id;
-                update.add = false;
-                itr++;
-
-                storeFdbEntryState(update);
-
-                for (auto observer: m_observers)
-                {
-                    observer->update(SUBJECT_TYPE_FDB_CHANGE, &update);
-                }
-            }
-        }
-        else if (entry->bv_id == SAI_NULL_OBJECT_ID)
-        {
-            /* FLUSH based on port */
-            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());
-
-            for (auto itr = m_entries.begin(); itr != m_entries.end();)
-            {
-                auto next_item = std::next(itr);
-                if (itr->first.port_name == update.port.m_alias)
-                {
-                    update.entry.mac = itr->first.mac;
-                    update.entry.bv_id = itr->first.bv_id;
-                    update.add = false;
-
-                    storeFdbEntryState(update);
+        handleSyncdFlushNotif(entry->bv_id, bridge_port_id, update.entry.mac);
 
-                    for (auto observer: m_observers)
-                    {
-                        observer->update(SUBJECT_TYPE_FDB_CHANGE, &update);
-                    }
-                }
-                itr = next_item;
-            }
-        }
-        else if (bridge_port_id == SAI_NULL_OBJECT_ID)
-        {
-            /* FLUSH based on VLAN - unsupported */
-            SWSS_LOG_ERROR("Unsupported FDB Flush: [ %s , %s ] = { port: - }",
-                           update.entry.mac.to_string().c_str(),
-                           vlanName.c_str());
-
-        }
-        else
-        {
-            /* FLUSH based on port and VLAN - unsupported */
-            SWSS_LOG_ERROR("Unsupported FDB Flush: [ %s , %s ] = { port: %s }",
-                           update.entry.mac.to_string().c_str(),
-                           vlanName.c_str(), update.port.m_alias.c_str());
-        }
         break;
     }
 
@@ -524,7 +538,7 @@ void FdbOrch::doTask(Consumer& consumer)
 {
     SWSS_LOG_ENTER();
 
-    if (!gPortsOrch->allPortsReady())
+    if (!m_portsOrch->allPortsReady())
     {
         return;
     }
@@ -681,7 +695,7 @@ void FdbOrch::doTask(NotificationConsumer& consumer)
 {
     SWSS_LOG_ENTER();
 
-    if (!gPortsOrch->allPortsReady())
+    if (!m_portsOrch->allPortsReady())
     {
         return;
     }
diff --git a/orchagent/fdborch.h b/orchagent/fdborch.h
@@ -119,6 +119,9 @@ class FdbOrch: public Orch, public Subject, public Observer
 
     bool storeFdbEntryState(const FdbUpdate& update);
     void notifyTunnelOrch(Port& port);
+
+    void clearFdbEntry(const FdbEntry&);
+    void handleSyncdFlushNotif(const sai_object_id_t&, const sai_object_id_t&, const MacAddress& );
 };
 
 #endif /* SWSS_FDBORCH_H */
diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp
@@ -5355,3 +5355,17 @@ std::unordered_set<std::string> PortsOrch::generateCounterStats(const string& ty
     }
     return counter_stats;
 }
+
+bool PortsOrch::decrFdbCount(const std::string& alias, int count)
+{
+    auto itr = m_portList.find(alias);
+    if (itr == m_portList.end())
+    {
+        return false;
+    }
+    else
+    {
+        itr->second.m_fdb_count -= count;
+    }
+    return true;
+}
diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h
@@ -147,6 +147,7 @@ class PortsOrch : public Orch, public Subject
     bool removeVlanMember(Port &vlan, Port &port);
     bool isVlanMember(Port &vlan, Port &port);
 
+    bool decrFdbCount(const string& alias, int count);
 private:
     unique_ptr<Table> m_counterTable;
     unique_ptr<Table> m_counterLagTable;
diff --git a/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am
@@ -22,6 +22,7 @@ LDADD_GTEST = -L/usr/src/gtest
 
 tests_SOURCES = aclorch_ut.cpp \
                 portsorch_ut.cpp \
+                fdborch/flush_syncd_notif_ut.cpp \
                 saispy_ut.cpp \
                 consumer_ut.cpp \
                 ut_saihelper.cpp \
diff --git a/tests/mock_tests/fdborch/flush_syncd_notif_ut.cpp b/tests/mock_tests/fdborch/flush_syncd_notif_ut.cpp
diff --git a/tests/mock_tests/mock_table.cpp b/tests/mock_tests/mock_table.cpp
