diff --git a/cfgmgr/nbrmgr.cpp b/cfgmgr/nbrmgr.cpp index 3b919a7daa10..0b256cc3186d 100644 --- a/cfgmgr/nbrmgr.cpp +++ b/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 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()); + } +} diff --git a/cfgmgr/nbrmgr.h b/cfgmgr/nbrmgr.h index 6b0c664c8068..3a56c4b6cc9c 100644 --- a/cfgmgr/nbrmgr.h +++ b/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; diff --git a/orchagent/fdborch.cpp b/orchagent/fdborch.cpp index 0c75d0c8e4dc..d2edd0ba2026 100644 --- a/orchagent/fdborch.cpp +++ b/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,6 +543,34 @@ 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(); @@ -538,6 +578,21 @@ void FdbOrch::updatePortOperState(const PortOperStateUpdate& update) { 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}); diff --git a/orchagent/fdborch.h b/orchagent/fdborch.h index 415215d0c2e8..874bf0229f3e 100644 --- a/orchagent/fdborch.h +++ b/orchagent/fdborch.h @@ -24,6 +24,12 @@ struct FdbUpdate bool add; }; +struct FdbFlushUpdate +{ + vector 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; diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index 24141008268f..0c390c0cba38 100644 --- a/orchagent/neighorch.cpp +++ b/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 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(cntx); + processFDBFlushUpdate(*update); + break; + } + default: + break; + } + + return; +} bool NeighOrch::hasNextHop(const NextHopKey &nexthop) { return m_syncdNextHops.find(nexthop) != m_syncdNextHops.end(); diff --git a/orchagent/neighorch.h b/orchagent/neighorch.h index 60c40f9053d2..aa235bd55c71 100644 --- a/orchagent/neighorch.h +++ b/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); }; diff --git a/orchagent/observer.h b/orchagent/observer.h index 34b31e351c57..76f00f1bfd80 100644 --- a/orchagent/observer.h +++ b/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 diff --git a/orchagent/orchdaemon.cpp b/orchagent/orchdaemon.cpp index 89f28fb101ce..b6124b3dca1e 100644 --- a/orchagent/orchdaemon.cpp +++ b/orchagent/orchdaemon.cpp @@ -120,7 +120,7 @@ bool OrchDaemon::init() gDirectory.set(chassis_frontend_orch); gIntfsOrch = new IntfsOrch(m_applDb, APP_INTF_TABLE_NAME, vrf_orch); - gNeighOrch = new NeighOrch(m_applDb, APP_NEIGH_TABLE_NAME, gIntfsOrch); + gNeighOrch = new NeighOrch(m_applDb, APP_NEIGH_TABLE_NAME, gIntfsOrch, gFdbOrch, gPortsOrch); vector fgnhg_tables = { CFG_FG_NHG, diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 21318fa31958..60b3aca06327 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -1815,7 +1815,7 @@ bool PortsOrch::initPort(const string &alias, const int index, const set &l { Port p(alias, Port::PHY); - p.m_index = index; + p.m_index = index; p.m_port_id = id; /* Initialize the port and create corresponding host interface */ @@ -2843,8 +2843,8 @@ void PortsOrch::doLagTask(Consumer &consumer) Port lag; if (getPort(alias, lag)) { - operation_status_changed = - (string_oper_status.at(operation_status) != + operation_status_changed = + (string_oper_status.at(operation_status) != lag.m_oper_status); } } diff --git a/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am index 108cab6beba3..40cbbb4fcee1 100644 --- a/tests/mock_tests/Makefile.am +++ b/tests/mock_tests/Makefile.am @@ -64,7 +64,7 @@ tests_SOURCES = aclorch_ut.cpp \ $(top_srcdir)/orchagent/chassisorch.cpp \ $(top_srcdir)/orchagent/sfloworch.cpp \ $(top_srcdir)/orchagent/debugcounterorch.cpp \ - $(top_srcdir)/orchagent/natorch.cpp + $(top_srcdir)/orchagent/natorch.cpp tests_SOURCES += $(FLEX_CTR_DIR)/flex_counter_manager.cpp $(FLEX_CTR_DIR)/flex_counter_stat_manager.cpp tests_SOURCES += $(DEBUG_CTR_DIR)/debug_counter.cpp $(DEBUG_CTR_DIR)/drop_counter.cpp @@ -72,4 +72,4 @@ tests_SOURCES += $(DEBUG_CTR_DIR)/debug_counter.cpp $(DEBUG_CTR_DIR)/drop_counte tests_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI) tests_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI) -I$(top_srcdir)/orchagent tests_LDADD = $(LDADD_GTEST) $(LDADD_SAI) -lnl-genl-3 -lhiredis -lhiredis -lpthread \ - -lswsscommon -lswsscommon -lgtest -lgtest_main -lzmq + -lswsscommon -lswsscommon -lgtest -lgtest_main -lzmq -lnl-3 -lnl-route-3 diff --git a/tests/mock_tests/aclorch_ut.cpp b/tests/mock_tests/aclorch_ut.cpp index 7b279a6571fc..72f11dbb233b 100644 --- a/tests/mock_tests/aclorch_ut.cpp +++ b/tests/mock_tests/aclorch_ut.cpp @@ -313,8 +313,14 @@ namespace aclorch_test ASSERT_EQ(gIntfsOrch, nullptr); gIntfsOrch = new IntfsOrch(m_app_db.get(), APP_INTF_TABLE_NAME, gVrfOrch); + TableConnector applDbFdb(m_app_db.get(), APP_FDB_TABLE_NAME); + TableConnector stateDbFdb(m_state_db.get(), STATE_FDB_TABLE_NAME); + + ASSERT_EQ(gFdbOrch, nullptr); + gFdbOrch = new FdbOrch(applDbFdb, stateDbFdb, gPortsOrch); + ASSERT_EQ(gNeighOrch, nullptr); - gNeighOrch = new NeighOrch(m_app_db.get(), APP_NEIGH_TABLE_NAME, gIntfsOrch); + gNeighOrch = new NeighOrch(m_app_db.get(), APP_NEIGH_TABLE_NAME, gIntfsOrch, gFdbOrch, gPortsOrch); ASSERT_EQ(gFgNhgOrch, nullptr); vector fgnhg_tables = { @@ -327,12 +333,6 @@ namespace aclorch_test ASSERT_EQ(gRouteOrch, nullptr); gRouteOrch = new RouteOrch(m_app_db.get(), APP_ROUTE_TABLE_NAME, gSwitchOrch, gNeighOrch, gIntfsOrch, gVrfOrch, gFgNhgOrch); - TableConnector applDbFdb(m_app_db.get(), APP_FDB_TABLE_NAME); - TableConnector stateDbFdb(m_state_db.get(), STATE_FDB_TABLE_NAME); - - ASSERT_EQ(gFdbOrch, nullptr); - gFdbOrch = new FdbOrch(applDbFdb, stateDbFdb, gPortsOrch); - PolicerOrch *policer_orch = new PolicerOrch(m_config_db.get(), "POLICER"); TableConnector stateDbMirrorSession(m_state_db.get(), STATE_MIRROR_SESSION_TABLE_NAME); @@ -355,14 +355,14 @@ namespace aclorch_test delete gSwitchOrch; gSwitchOrch = nullptr; - delete gFdbOrch; - gFdbOrch = nullptr; delete gMirrorOrch; gMirrorOrch = nullptr; delete gRouteOrch; gRouteOrch = nullptr; delete gNeighOrch; gNeighOrch = nullptr; + delete gFdbOrch; + gFdbOrch = nullptr; delete gIntfsOrch; gIntfsOrch = nullptr; delete gVrfOrch; @@ -372,7 +372,7 @@ namespace aclorch_test delete gPortsOrch; gPortsOrch = nullptr; delete gFgNhgOrch; - gFgNhgOrch = nullptr; + gFgNhgOrch = nullptr; auto status = sai_switch_api->remove_switch(gSwitchId); ASSERT_EQ(status, SAI_STATUS_SUCCESS); @@ -621,7 +621,7 @@ namespace aclorch_test if (crmAclTableBindingCount != aclorchAclTableBindingCount) { ADD_FAILURE() << "ACL table binding count is not consistent between CrmOrch (" - << crmAclTableBindingCount << ") and AclOrch (" + << crmAclTableBindingCount << ") and AclOrch (" << aclorchAclTableBindingCount << ")"; return false; }