Flush ARP/neighbor entry on FDB flush when port L2-L3 (sonic-net#1506)

Flush ARP entries when FDB entries are flushed.

*Neighbor orchagent attaches to FDB orchagent and observes.
*When FDB orchagent removes/flushes a FDB entry, neighbor orchagent will be notified.
*Neighbor orchagent will notifies nbrmgr to send a arp refresh. 

Co-authored-by: Vasant <vapatil@linkedin.com>

cfgmgr/nbrmgr.cpp

@@ -59,6 +59,11 @@ NbrMgr::NbrMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, con
     {
         SWSS_LOG_ERROR("Netlink socket connect failed, error '%s'", nl_geterror(err));
     }
+
+    auto consumerStateTable = new swss::ConsumerStateTable(appDb, APP_NEIGH_RESOLVE_TABLE_NAME,
+                              TableConsumable::DEFAULT_POP_BATCH_SIZE, default_orch_pri);
+    auto consumer = new Consumer(consumerStateTable, this, APP_NEIGH_RESOLVE_TABLE_NAME);
+    Orch::addExecutor(consumer);
 }
 
 bool NbrMgr::isIntfStateOk(const string &alias)
@@ -185,7 +190,28 @@ bool NbrMgr::setNeighbor(const string& alias, const IpAddress& ip, const MacAddr
     return send_message(m_nl_sock, msg);
 }
 
-void NbrMgr::doTask(Consumer &consumer)
+void NbrMgr::doResolveNeighTask(Consumer &consumer)
+{
+    SWSS_LOG_ENTER();
+
+    auto it = consumer.m_toSync.begin();
+    while (it != consumer.m_toSync.end())
+    {
+        KeyOpFieldsValuesTuple    t = it->second;
+        vector<string>            keys = tokenize(kfvKey(t),delimiter);
+        MacAddress                mac;
+        IpAddress                 ip(keys[1]);
+        string                    alias(keys[0]);
+
+        if (!setNeighbor(alias, ip, mac))
+        {
+            SWSS_LOG_ERROR("Neigh entry resolve failed for '%s'", kfvKey(t).c_str());
+        }
+        it = consumer.m_toSync.erase(it);
+    }
+}
+
+void NbrMgr::doSetNeighTask(Consumer &consumer)
 {
     SWSS_LOG_ENTER();
 
@@ -257,3 +283,19 @@ void NbrMgr::doTask(Consumer &consumer)
         it = consumer.m_toSync.erase(it);
     }
 }
+
+void NbrMgr::doTask(Consumer &consumer)
+{
+    string table_name = consumer.getTableName();
+
+    if (table_name == CFG_NEIGH_TABLE_NAME)
+    {
+        doSetNeighTask(consumer);
+    } else if (table_name == APP_NEIGH_RESOLVE_TABLE_NAME)
+    {
+        doResolveNeighTask(consumer);
+    } else
+    {
+        SWSS_LOG_ERROR("Unknown REDIS table %s ", table_name.c_str());
+    }
+}

cfgmgr/nbrmgr.h

@@ -26,6 +26,8 @@ class NbrMgr : public Orch
     bool isIntfStateOk(const std::string &alias);
     bool setNeighbor(const std::string& alias, const IpAddress& ip, const MacAddress& mac);
 
+    void doResolveNeighTask(Consumer &consumer);
+    void doSetNeighTask(Consumer &consumer);
     void doTask(Consumer &consumer);
 
     Table m_statePortTable, m_stateLagTable, m_stateVlanTable, m_stateIntfTable, m_stateNeighRestoreTable;

orchagent/fdborch.cpp

@@ -113,8 +113,8 @@ bool FdbOrch::storeFdbEntryState(const FdbUpdate& update)
     }
 }
 
-void FdbOrch::update(sai_fdb_event_t        type, 
-                     const sai_fdb_entry_t* entry, 
+void FdbOrch::update(sai_fdb_event_t        type,
+                     const sai_fdb_entry_t* entry,
                      sai_object_id_t        bridge_port_id)
 {
     SWSS_LOG_ENTER();
@@ -124,11 +124,11 @@ void FdbOrch::update(sai_fdb_event_t        type,
     update.entry.bv_id = entry->bv_id;
 
     SWSS_LOG_INFO("FDB event:%d, MAC: %s , BVID: 0x%" PRIx64 " , \
-                   bridge port ID: 0x%" PRIx64 ".", 
-                   type, update.entry.mac.to_string().c_str(), 
+                   bridge port ID: 0x%" PRIx64 ".",
+                   type, update.entry.mac.to_string().c_str(),
                    entry->bv_id, bridge_port_id);
 
-    if (bridge_port_id && 
+    if (bridge_port_id &&
         !m_portsOrch->getPortByBridgePortId(bridge_port_id, update.port))
     {
         SWSS_LOG_ERROR("Failed to get port by bridge port ID 0x%" PRIx64 ".",
@@ -199,7 +199,7 @@ void FdbOrch::update(sai_fdb_event_t        type,
         }
 
 
-        if (bridge_port_id == SAI_NULL_OBJECT_ID && 
+        if (bridge_port_id == SAI_NULL_OBJECT_ID &&
             entry->bv_id == SAI_NULL_OBJECT_ID)
         {
             SWSS_LOG_INFO("FDB Flush: [ %s , %s ] = { port: - }",
@@ -491,6 +491,18 @@ void FdbOrch::doTask(NotificationConsumer& consumer)
     }
 }
 
+/*
+ * Name: flushFDBEntries
+ * Params:
+ *     bridge_port_oid - SAI object ID of bridge port associated with the port
+ *     vlan_oid - SAI object ID of the VLAN
+ * Description:
+ *     Flushes FDB entries based on bridge_port_oid, or vlan_oid or both.
+ *     This function is called in three cases.
+ *     1. Port is reoved from VLAN (via SUBJECT_TYPE_VLAN_MEMBER_CHANGE)
+ *     2. Bridge port OID is removed (Direct call)
+ *     3. Port is shut down (via SUBJECT_TYPE_
+ */
 void FdbOrch::flushFDBEntries(sai_object_id_t bridge_port_oid,
                               sai_object_id_t vlan_oid)
 {
@@ -531,13 +543,56 @@ void FdbOrch::flushFDBEntries(sai_object_id_t bridge_port_oid,
     }
 }
 
+void FdbOrch::notifyObserversFDBFlush(Port &port, sai_object_id_t& bvid)
+{
+    FdbFlushUpdate flushUpdate;
+    flushUpdate.port = port;
+
+    for (auto itr = m_entries.begin(); itr != m_entries.end(); ++itr)
+    {
+        if ((itr->port_name == port.m_alias) &&
+            (itr->bv_id == bvid))
+        {
+            SWSS_LOG_INFO("Adding MAC learnt on [ port:%s , bvid:0x%" PRIx64 "]\
+                           to ARP flush", port.m_alias.c_str(), bvid);
+            FdbEntry entry;
+            entry.mac = itr->mac;
+            entry.bv_id = itr->bv_id;
+            flushUpdate.entries.push_back(entry);
+        }
+    }
+
+    if (!flushUpdate.entries.empty())
+    {
+        for (auto observer: m_observers)
+        {
+            observer->update(SUBJECT_TYPE_FDB_FLUSH_CHANGE, &flushUpdate);
+        }
+    }
+}
+
 void FdbOrch::updatePortOperState(const PortOperStateUpdate& update)
 {
     SWSS_LOG_ENTER();
     if (update.operStatus == SAI_PORT_OPER_STATUS_DOWN)
     {
         swss::Port p = update.port;
         flushFDBEntries(p.m_bridge_port_id, SAI_NULL_OBJECT_ID);
+
+        // Get BVID of each VLAN that this port is a member of
+        // and call notifyObserversFDBFlush
+        for (const auto& vlan_member: p.m_vlan_members)
+        {
+            swss::Port vlan;
+            string vlan_alias = VLAN_PREFIX + to_string(vlan_member.first);
+            if (!m_portsOrch->getPort(vlan_alias, vlan))
+            {
+                SWSS_LOG_INFO("Failed to locate VLAN %s", vlan_alias.c_str());
+                continue;
+            }
+            notifyObserversFDBFlush(p, vlan.m_vlan_info.vlan_oid);
+        }
+
     }
     return;
 }
@@ -551,6 +606,7 @@ void FdbOrch::updateVlanMember(const VlanMemberUpdate& update)
         swss::Port vlan = update.vlan;
         swss::Port port = update.member;
         flushFDBEntries(port.m_bridge_port_id, vlan.m_vlan_info.vlan_oid);
+        notifyObserversFDBFlush(port, vlan.m_vlan_info.vlan_oid);
         return;
     }
 
@@ -580,7 +636,7 @@ bool FdbOrch::addFdbEntry(const FdbEntry& entry, const string& type)
     if (!m_portsOrch->getPort(entry.port_name, port))
     {
         SWSS_LOG_DEBUG("Saving a fdb entry until port %s becomes active",
-                        entry. port_name.c_str());
+                        entry.port_name.c_str());
         saved_fdb_entries[entry.port_name].push_back({entry, type});
 
         return true;
@@ -589,7 +645,7 @@ bool FdbOrch::addFdbEntry(const FdbEntry& entry, const string& type)
     /* Retry until port is added to the VLAN */
     if (!port.m_bridge_port_id)
     {
-        SWSS_LOG_DEBUG("Saving a fdb entry until port %s has got a bridge port ID", 
+        SWSS_LOG_DEBUG("Saving a fdb entry until port %s has got a bridge port ID",
                         entry.port_name.c_str());
         saved_fdb_entries[entry.port_name].push_back({entry, type});
 

orchagent/fdborch.h

@@ -24,6 +24,12 @@ struct FdbUpdate
     bool add;
 };
 
+struct FdbFlushUpdate
+{
+    vector<FdbEntry> entries;
+    Port port;
+};
+
 struct SavedFdbEntry
 {
     FdbEntry entry;
@@ -49,6 +55,7 @@ class FdbOrch: public Orch, public Subject, public Observer
     bool getPort(const MacAddress&, uint16_t, Port&);
     void flushFDBEntries(sai_object_id_t bridge_port_oid,
                          sai_object_id_t vlan_oid);
+    void notifyObserversFDBFlush(Port &p, sai_object_id_t&);
 
 private:
     PortsOrch *m_portsOrch;

orchagent/neighorch.cpp

@@ -16,12 +16,107 @@ extern FgNhgOrch *gFgNhgOrch;
 
 const int neighorch_pri = 30;
 
-NeighOrch::NeighOrch(DBConnector *db, string tableName, IntfsOrch *intfsOrch) :
-        Orch(db, tableName, neighorch_pri), m_intfsOrch(intfsOrch)
+NeighOrch::NeighOrch(DBConnector *appDb, string tableName, IntfsOrch *intfsOrch, FdbOrch *fdbOrch, PortsOrch *portsOrch) :
+        Orch(appDb, tableName, neighorch_pri),
+        m_intfsOrch(intfsOrch),
+        m_fdbOrch(fdbOrch),
+        m_portsOrch(portsOrch),
+        m_appNeighResolveProducer(appDb, APP_NEIGH_RESOLVE_TABLE_NAME)
 {
     SWSS_LOG_ENTER();
+
+    m_fdbOrch->attach(this);
+}
+
+NeighOrch::~NeighOrch()
+{
+    if (m_fdbOrch)
+    {
+        m_fdbOrch->detach(this);
+    }
+}
+
+bool NeighOrch::resolveNeighborEntry(const NeighborEntry &entry, const MacAddress &mac)
+{
+    vector<FieldValueTuple>    data;
+    IpAddress                  ip = entry.ip_address;
+    string                     key, alias = entry.alias;
+
+    key = alias + ":" + entry.ip_address.to_string();
+    // We do NOT need to populate mac field as its NOT
+    // even used in nbrmgr during ARP resolve. But just keeping here.
+    FieldValueTuple fvTuple("mac", mac.to_string().c_str());
+    data.push_back(fvTuple);
+
+    SWSS_LOG_INFO("Flushing ARP entry '%s:%s --> %s'",
+                  alias.c_str(), ip.to_string().c_str(), mac.to_string().c_str());
+    m_appNeighResolveProducer.set(key, data);
+    return true;
+}
+
+/*
+ * Function Name: processFDBFlushUpdate
+ * Description:
+ *     Goal of this function is to delete neighbor/ARP entries
+ * when a port belonging to a VLAN gets removed.
+ * This function is called whenever neighbor orchagent receives
+ * SUBJECT_TYPE_FDB_FLUSH_CHANGE notification. Currently we only care for
+ * deleted FDB entries. We flush neighbor entry that matches its
+ * in-coming interface and MAC with FDB entry's VLAN name and MAC
+ * respectively.
+ */
+void NeighOrch::processFDBFlushUpdate(const FdbFlushUpdate& update)
+{
+    SWSS_LOG_INFO("processFDBFlushUpdate port: %s",
+                    update.port.m_alias.c_str());
+
+    for (auto entry : update.entries)
+    {
+        // Get Vlan object
+        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);
+            continue;
+        }
+        SWSS_LOG_INFO("Flushing ARP for port: %s, VLAN: %s",
+                      vlan.m_alias.c_str(), update.port.m_alias.c_str());
+
+        // If the FDB entry MAC matches with neighbor/ARP entry MAC,
+        // and ARP entry incoming interface matches with VLAN name,
+        // flush neighbor/arp entry.
+        for (const auto &neighborEntry : m_syncdNeighbors)
+        {
+            if (neighborEntry.first.alias == vlan.m_alias &&
+                neighborEntry.second == entry.mac)
+            {
+                resolveNeighborEntry(neighborEntry.first, neighborEntry.second);
+            }
+        }
+    }
+    return;
 }
 
+void NeighOrch::update(SubjectType type, void *cntx)
+{
+    SWSS_LOG_ENTER();
+
+    assert(cntx);
+
+    switch(type) {
+        case SUBJECT_TYPE_FDB_FLUSH_CHANGE:
+        {
+            FdbFlushUpdate *update = reinterpret_cast<FdbFlushUpdate *>(cntx);
+            processFDBFlushUpdate(*update);
+            break;
+        }
+        default:
+            break;
+    }
+
+    return;
+}
 bool NeighOrch::hasNextHop(const NextHopKey &nexthop)
 {
     return m_syncdNextHops.find(nexthop) != m_syncdNextHops.end();

orchagent/neighorch.h

@@ -5,9 +5,12 @@
 #include "observer.h"
 #include "portsorch.h"
 #include "intfsorch.h"
+#include "fdborch.h"
 
 #include "ipaddress.h"
 #include "nexthopkey.h"
+#include "producerstatetable.h"
+#include "schema.h"
 
 #define NHFLAGS_IFDOWN                  0x1 // nexthop's outbound i/f is down
 
@@ -32,10 +35,11 @@ struct NeighborUpdate
     bool add;
 };
 
-class NeighOrch : public Orch, public Subject
+class NeighOrch : public Orch, public Subject, public Observer
 {
 public:
-    NeighOrch(DBConnector *db, string tableName, IntfsOrch *intfsOrch);
+    NeighOrch(DBConnector *db, string tableName, IntfsOrch *intfsOrch, FdbOrch *fdbOrch, PortsOrch *portsOrch);
+    ~NeighOrch();
 
     bool hasNextHop(const NextHopKey&);
 
@@ -51,8 +55,13 @@ class NeighOrch : public Orch, public Subject
     bool ifChangeInformNextHop(const string &, bool);
     bool isNextHopFlagSet(const NextHopKey &, const uint32_t);
 
+    void update(SubjectType, void *);
+
 private:
+    PortsOrch *m_portsOrch;
     IntfsOrch *m_intfsOrch;
+    FdbOrch *m_fdbOrch;
+    ProducerStateTable m_appNeighResolveProducer;
 
     NeighborTable m_syncdNeighbors;
     NextHopTable m_syncdNextHops;
@@ -66,6 +75,9 @@ class NeighOrch : public Orch, public Subject
     bool setNextHopFlag(const NextHopKey &, const uint32_t);
     bool clearNextHopFlag(const NextHopKey &, const uint32_t);
 
+    void processFDBFlushUpdate(const FdbFlushUpdate &);
+    bool resolveNeighborEntry(const NeighborEntry &, const MacAddress &);
+
     void doTask(Consumer &consumer);
 };
 

orchagent/observer.h

@@ -17,6 +17,7 @@ enum SubjectType
     SUBJECT_TYPE_INT_SESSION_CHANGE,
     SUBJECT_TYPE_PORT_CHANGE,
     SUBJECT_TYPE_PORT_OPER_STATE_CHANGE,
+    SUBJECT_TYPE_FDB_FLUSH_CHANGE,
 };
 
 class Observer