[FDB] Fix fbdorch to properly handle syncd FDB FLUSH Notif

orchagent/fdborch.cpp

@@ -18,7 +18,6 @@
 extern sai_fdb_api_t    *sai_fdb_api;
 
 extern sai_object_id_t  gSwitchId;
-extern PortsOrch*       gPortsOrch;
 extern CrmOrch *        gCrmOrch;
 extern MlagOrch*        gMlagOrch;
 extern Directory<Orch*> gDirectory;
@@ -175,6 +174,107 @@ bool FdbOrch::storeFdbEntryState(const FdbUpdate& update)
     }
 }
 
+/*
+clears stateDb and decrements corresponding internal fdb counters
+*/
+void FdbOrch::clearFdbEntry(const MacAddress& mac,
+                   const sai_object_id_t& bv_id,
+                   const string& port_alias)
+{
+    FdbUpdate update;
+    update.entry.mac = mac;
+    update.entry.bv_id = bv_id;
+    update.add = false;
+
+    /* Fetch Vlan and decrement the counter */
+    Port temp_vlan;
+    if (m_portsOrch->getPort(bv_id, temp_vlan))
+    {
+        m_portsOrch->decrFdbCount(temp_vlan.m_alias, 1);
+    }
+
+    /* Decrement port fdb_counter */
+    m_portsOrch->decrFdbCount(port_alias, 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,
+                   mac.to_string().c_str(), port_alias.c_str(), 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.mac, curr->first.bv_id, curr->first.port_name);
+            }
+        }
+    }
+    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.mac, curr->first.bv_id, curr->first.port_name);
+                }
+            }
+        }
+    }
+    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.mac, curr->first.bv_id, curr->first.port_name);
+                }
+            }
+        }
+    }
+    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.mac, curr->first.bv_id, curr->first.port_name);
+                }
+            }
+        }
+    }
+}
+
 void FdbOrch::update(sai_fdb_event_t        type,
                      const sai_fdb_entry_t* entry,
                      sai_object_id_t        bridge_port_id)
@@ -192,24 +292,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;
     }
 
@@ -219,12 +324,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())
@@ -319,11 +418,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())
@@ -457,12 +551,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())
         {
@@ -500,80 +588,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);
-
-                notify(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;
+        handleSyncdFlushNotif(entry->bv_id, bridge_port_id, update.entry.mac);
 
-                    storeFdbEntryState(update);
-                    notify(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;
     }
 
@@ -649,7 +672,7 @@ void FdbOrch::doTask(Consumer& consumer)
 {
     SWSS_LOG_ENTER();
 
-    if (!gPortsOrch->allPortsReady())
+    if (!m_portsOrch->allPortsReady())
     {
         return;
     }
@@ -856,7 +879,7 @@ void FdbOrch::doTask(NotificationConsumer& consumer)
 {
     SWSS_LOG_ENTER();
 
-    if (!gPortsOrch->allPortsReady())
+    if (!m_portsOrch->allPortsReady())
     {
         return;
     }
@@ -892,7 +915,7 @@ void FdbOrch::doTask(NotificationConsumer& consumer)
                 SWSS_LOG_ERROR("Receive wrong port to flush fdb!");
                 return;
             }
-            if (!gPortsOrch->getPort(alias, port))
+            if (!m_portsOrch->getPort(alias, port))
             {
                 SWSS_LOG_ERROR("Get Port from port(%s) failed!", alias.c_str());
                 return;
@@ -913,7 +936,7 @@ void FdbOrch::doTask(NotificationConsumer& consumer)
                 SWSS_LOG_ERROR("Receive wrong vlan to flush fdb!");
                 return;
             }
-            if (!gPortsOrch->getPort(vlan, vlanPort))
+            if (!m_portsOrch->getPort(vlan, vlanPort))
             {
                 SWSS_LOG_ERROR("Get Port from vlan(%s) failed!", vlan.c_str());
                 return;
@@ -939,12 +962,12 @@ void FdbOrch::doTask(NotificationConsumer& consumer)
                 SWSS_LOG_ERROR("Receive wrong port or vlan to flush fdb!");
                 return;
             }
-            if (!gPortsOrch->getPort(alias, port))
+            if (!m_portsOrch->getPort(alias, port))
             {
                 SWSS_LOG_ERROR("Get Port from port(%s) failed!", alias.c_str());
                 return;
             }
-            if (!gPortsOrch->getPort(vlan, vlanPort))
+            if (!m_portsOrch->getPort(vlan, vlanPort))
             {
                 SWSS_LOG_ERROR("Get Port from vlan(%s) failed!", vlan.c_str());
                 return;

orchagent/fdborch.h

@@ -122,6 +122,9 @@ class FdbOrch: public Orch, public Subject, public Observer
 
     bool storeFdbEntryState(const FdbUpdate& update);
     void notifyTunnelOrch(Port& port);
+
+    void clearFdbEntry(const MacAddress&, const sai_object_id_t&, const string&);
+    void handleSyncdFlushNotif(const sai_object_id_t&, const sai_object_id_t&, const MacAddress& );
 };
 
 #endif /* SWSS_FDBORCH_H */

orchagent/portsorch.cpp

@@ -4573,9 +4573,10 @@ bool PortsOrch::removeVlan(Port vlan)
        return false for retry */
     if (vlan.m_fdb_count > 0)
     {
-        SWSS_LOG_NOTICE("VLAN %s still has assiciated FDB entries", vlan.m_alias.c_str());
+        SWSS_LOG_NOTICE("VLAN %s still has %d FDB entries", vlan.m_alias.c_str(), vlan.m_fdb_count);
         return false;
     }
+
     if (m_port_ref_count[vlan.m_alias] > 0)
     {
         SWSS_LOG_ERROR("Failed to remove ref count %d VLAN %s",
@@ -6928,3 +6929,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;
+}

orchagent/portsorch.h

@@ -168,6 +168,7 @@ class PortsOrch : public Orch, public Subject
 
     bool getPortOperStatus(const Port& port, sai_port_oper_status_t& status) const;
 
+    bool decrFdbCount(const string& alias, int count);
 private:
     unique_ptr<Table> m_counterTable;
     unique_ptr<Table> m_counterLagTable;

tests/mock_tests/Makefile.am

@@ -26,6 +26,7 @@ tests_SOURCES = aclorch_ut.cpp \
                 routeorch_ut.cpp \
                 qosorch_ut.cpp \
                 bufferorch_ut.cpp \
+                fdborch/flush_syncd_notif_ut.cpp \
                 copporch_ut.cpp \
                 saispy_ut.cpp \
                 consumer_ut.cpp \