From c5dcc46b660df9d5aeabb95487ffc92d4409b82b Mon Sep 17 00:00:00 2001 From: Vasant Date: Mon, 16 Nov 2020 03:31:03 +0000 Subject: [PATCH 1/6] Flush ARP/neighbor entry on FDB flush when port L2-L3 Signed-off-by: Vasant --- orchagent/fdborch.cpp | 72 ++++++++-- orchagent/fdborch.h | 7 + orchagent/neighorch.cpp | 227 +++++++++++++++++++++++++++++++- orchagent/neighorch.h | 15 ++- orchagent/observer.h | 1 + orchagent/orchdaemon.cpp | 2 +- orchagent/portsorch.cpp | 6 +- tests/mock_tests/Makefile.am | 4 +- tests/mock_tests/aclorch_ut.cpp | 22 ++-- 9 files changed, 327 insertions(+), 29 deletions(-) diff --git a/orchagent/fdborch.cpp b/orchagent/fdborch.cpp index 0c75d0c8e4..d2edd0ba20 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 415215d0c2..874bf0229f 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 2414100826..ddae3268cc 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -1,4 +1,7 @@ #include +#include +#include + #include "neighorch.h" #include "logger.h" #include "swssnet.h" @@ -16,12 +19,232 @@ extern FgNhgOrch *gFgNhgOrch; const int neighorch_pri = 30; -NeighOrch::NeighOrch(DBConnector *db, string tableName, IntfsOrch *intfsOrch) : - Orch(db, tableName, neighorch_pri), m_intfsOrch(intfsOrch) +static bool send_message(struct nl_sock *socket_p, struct nl_msg *msg_p) +{ + int err = 0; + + if (!socket_p) + { + SWSS_LOG_ERROR("Netlink socket null pointer"); + return false; + } + + if ((err = nl_send_auto(socket_p, msg_p)) < 0) + { + SWSS_LOG_ERROR("Netlink send message failed, error '%s'", nl_geterror(err)); + return false; + } + + nlmsg_free(msg_p); + return true; +} + +NeighOrch::NeighOrch(DBConnector *db, string tableName, IntfsOrch *intfsOrch, FdbOrch *fdbOrch, PortsOrch *portsOrch) : + Orch(db, tableName, neighorch_pri), m_intfsOrch(intfsOrch), m_fdbOrch(fdbOrch), m_portsOrch(portsOrch) { + int err = 0; + SWSS_LOG_ENTER(); + + m_fdbOrch->attach(this); + gPortsOrch->attach(this); + + m_nl_sock = nl_socket_alloc(); + if (!m_nl_sock) + { + SWSS_LOG_ERROR("Netlink socket is NOT allocacted"); + } + else if ((err = nl_connect(m_nl_sock, NETLINK_ROUTE)) < 0) + { + SWSS_LOG_ERROR("Netlink socket connect failed, error '%s'", nl_geterror(err)); + nl_socket_free(m_nl_sock); + m_nl_sock = NULL; + } +} + +NeighOrch::~NeighOrch() +{ + if (m_fdbOrch) + { + m_fdbOrch->detach(this); + } + + if (m_nl_sock) + { + nl_close(m_nl_sock); + nl_socket_free(m_nl_sock); + } +} + +bool NeighOrch::flushNeighborEntry(const NeighborEntry &entry, const MacAddress &mac) +{ + SWSS_LOG_ENTER(); + + IpAddress ip = entry.ip_address; + string alias = entry.alias; + + SWSS_LOG_INFO("Flushing ARP entry '%s' as FDB entry '%s' is flushed", + ip.to_string().c_str(), mac.to_string().c_str()); + + if (!m_nl_sock) + { + SWSS_LOG_ERROR("Netlink socket is NOT allocated"); + return false; + } + + struct nl_msg *msg_p = nlmsg_alloc(); + if (!msg_p) + { + SWSS_LOG_ERROR("Netlink message alloc failed for '%s'", ip.to_string().c_str()); + return false; + } + + auto flags = (NLM_F_REQUEST | NLM_F_ACK); + struct nlmsghdr *hdr = nlmsg_put(msg_p, NL_AUTO_PORT, NL_AUTO_SEQ, RTM_DELNEIGH, 0, flags); + + if (!hdr) + { + SWSS_LOG_ERROR("Netlink message header alloc failed for '%s'", ip.to_string().c_str()); + nlmsg_free(msg_p); + return false; + } + + struct ndmsg *nd_msg_p = static_cast + (nlmsg_reserve(msg_p, sizeof(struct ndmsg), NLMSG_ALIGNTO)); + if (!nd_msg_p) + { + SWSS_LOG_ERROR("Netlink ndmsg reserve failed for '%s'", ip.to_string().c_str()); + nlmsg_free(msg_p); + return false; + } + + memset(nd_msg_p, 0, sizeof(struct ndmsg)); + + nd_msg_p->ndm_ifindex = if_nametoindex(alias.c_str()); + + // Fill in the IPV4/IPV6 address + auto addr_len = ip.isV4()? sizeof(struct in_addr) : sizeof(struct in6_addr); + + struct rtattr *rta_p = static_cast + (nlmsg_reserve(msg_p, sizeof(struct rtattr) + addr_len, NLMSG_ALIGNTO)); + if (!rta_p) + { + SWSS_LOG_ERROR("Netlink rtattr (IP) failed for '%s'", ip.to_string().c_str()); + nlmsg_free(msg_p); + return false; + } + + rta_p->rta_type = NDA_DST; + rta_p->rta_len = static_cast(RTA_LENGTH(addr_len)); + + nd_msg_p->ndm_type = RTN_UNICAST; + auto ip_addr = ip.getIp(); + + if (ip.isV4()) + { + nd_msg_p->ndm_family = AF_INET; + memcpy(RTA_DATA(rta_p), &ip_addr.ip_addr.ipv4_addr, addr_len); + } + else + { + nd_msg_p->ndm_family = AF_INET6; + memcpy(RTA_DATA(rta_p), &ip_addr.ip_addr.ipv6_addr, addr_len); + } + + // Fill in the MAC address + auto mac_len = ETHER_ADDR_LEN; + auto mac_addr = mac.getMac(); + + rta_p = static_cast + (nlmsg_reserve(msg_p, sizeof(struct rtattr) + mac_len, NLMSG_ALIGNTO)); + if (!rta_p) + { + SWSS_LOG_ERROR("Netlink rtattr (MAC) failed for '%s'", ip.to_string().c_str()); + nlmsg_free(msg_p); + return false; + } + + rta_p->rta_type = NDA_LLADDR; + rta_p->rta_len = static_cast(RTA_LENGTH(mac_len)); + memcpy(RTA_DATA(rta_p), mac_addr, mac_len); + + return send_message(m_nl_sock, msg_p); } +/* + * 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. Also this ensures that underlying physical port operation + * status or admin state is DOWN. + */ +void NeighOrch::processFDBFlushUpdate(const FdbFlushUpdate& update) +{ + SWSS_LOG_NOTICE("processFDBFlushUpdate port: %s", + update.port.m_alias.c_str()); + /* + if (update.port.m_admin_state_up == true && + update.port.m_oper_status == SAI_PORT_OPER_STATUS_UP) + { + return; + } */ + + 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) + { + SWSS_LOG_INFO("Flushing ARP [ %s , %s ] = { port: %s }", + vlan.m_alias.c_str(), + entry.mac.to_string().c_str(), + update.port.m_alias.c_str()); + flushNeighborEntry(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 60c40f9053..64bc477640 100644 --- a/orchagent/neighorch.h +++ b/orchagent/neighorch.h @@ -5,9 +5,11 @@ #include "observer.h" #include "portsorch.h" #include "intfsorch.h" +#include "fdborch.h" #include "ipaddress.h" #include "nexthopkey.h" +#include "netmsg.h" #define NHFLAGS_IFDOWN 0x1 // nexthop's outbound i/f is down @@ -32,10 +34,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 +54,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; + struct nl_sock *m_nl_sock; NeighborTable m_syncdNeighbors; NextHopTable m_syncdNextHops; @@ -66,6 +74,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 flushNeighborEntry(const NeighborEntry &, const MacAddress &); + void doTask(Consumer &consumer); }; diff --git a/orchagent/observer.h b/orchagent/observer.h index 34b31e351c..76f00f1bfd 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 89f28fb101..b6124b3dca 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 21318fa319..60b3aca063 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 108cab6beb..40cbbb4fce 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 7b279a6571..72f11dbb23 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; } From dbc0d9d686537ef6ef52019a8d19f823ccec41c8 Mon Sep 17 00:00:00 2001 From: Vasant Date: Sat, 21 Nov 2020 05:18:51 +0000 Subject: [PATCH 2/6] Code-review comments addressed Signed-off-by: Vasant --- cfgmgr/nbrmgr.cpp | 240 ++++++++++++++++++++++++++++++++-------- cfgmgr/nbrmgr.h | 5 + orchagent/neighorch.cpp | 163 ++++----------------------- orchagent/neighorch.h | 6 +- 4 files changed, 221 insertions(+), 193 deletions(-) diff --git a/cfgmgr/nbrmgr.cpp b/cfgmgr/nbrmgr.cpp index 3b919a7daa..a670b5a86d 100644 --- a/cfgmgr/nbrmgr.cpp +++ b/cfgmgr/nbrmgr.cpp @@ -11,6 +11,7 @@ #include "nbrmgr.h" #include "exec.h" #include "shellcmd.h" +#include "subscriberstatetable.h" using namespace swss; @@ -59,6 +60,10 @@ NbrMgr::NbrMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, con { SWSS_LOG_ERROR("Netlink socket connect failed, error '%s'", nl_geterror(err)); } + + auto subscriberStateTable = new swss::SubscriberStateTable(appDb, APP_NEIGH_RESOLVE_TABLE_NAME, TableConsumable::DEFAULT_POP_BATCH_SIZE, default_orch_pri); + auto subscriberStateConsumer = new Consumer(subscriberStateTable, this, APP_NEIGH_RESOLVE_TABLE_NAME); + Orch::addExecutor(subscriberStateConsumer); } bool NbrMgr::isIntfStateOk(const string &alias) @@ -87,75 +92,157 @@ bool NbrMgr::isNeighRestoreDone() return false; } -bool NbrMgr::setNeighbor(const string& alias, const IpAddress& ip, const MacAddress& mac) + +bool NbrMgr::addIp(struct nl_msg* msg_p, const IpAddress& ip) +{ + auto ip_addr = ip.getIp(); + auto addr_len = ip.isV4()? sizeof(struct in_addr) : sizeof(struct in6_addr); + + struct rtattr *rta_p = static_cast + (nlmsg_reserve(msg_p, sizeof(struct rtattr) + addr_len, NLMSG_ALIGNTO)); + if (!rta_p) + { + SWSS_LOG_ERROR("Netlink rtattr (IP) failed for '%s'", ip.to_string().c_str()); + return false; + } + + rta_p->rta_type = NDA_DST; + rta_p->rta_len = static_cast(RTA_LENGTH(addr_len)); + ip.isV4() ? memcpy(RTA_DATA(rta_p), &ip_addr.ip_addr.ipv4_addr, addr_len) : + memcpy(RTA_DATA(rta_p), &ip_addr.ip_addr.ipv6_addr, addr_len); + + return true; +} + +bool NbrMgr::addMac(struct nl_msg* msg_p, const MacAddress& mac) +{ + auto mac_len = ETHER_ADDR_LEN; + auto mac_addr = mac.getMac(); + + struct rtattr *rta_p = static_cast + (nlmsg_reserve(msg_p, sizeof(struct rtattr) + mac_len, NLMSG_ALIGNTO)); + if (!rta_p) + { + SWSS_LOG_ERROR("Netlink rtattr (MAC) failed for '%s'", mac.to_string().c_str()); + return false; + } + + rta_p->rta_type = NDA_LLADDR; + rta_p->rta_len = static_cast(RTA_LENGTH(mac_len)); + memcpy(RTA_DATA(rta_p), mac_addr, mac_len); + + return true; +} + +bool NbrMgr::deleteNeighbor(const string& alias, const IpAddress& ip, const MacAddress& mac) { SWSS_LOG_ENTER(); - struct nl_msg *msg = nlmsg_alloc(); - if (!msg) + SWSS_LOG_NOTICE("Remove ARP entry '%s:%s -> %s'", + alias.c_str(), ip.to_string().c_str(), mac.to_string().c_str()); + + struct nl_msg *msg_p = nlmsg_alloc(); + if (!msg_p) { SWSS_LOG_ERROR("Netlink message alloc failed for '%s'", ip.to_string().c_str()); return false; } - auto flags = (NLM_F_REQUEST | NLM_F_ACK | NLM_F_CREATE | NLM_F_REPLACE); - - struct nlmsghdr *hdr = nlmsg_put(msg, NL_AUTO_PORT, NL_AUTO_SEQ, RTM_NEWNEIGH, 0, flags); - if (!hdr) + auto flags = (NLM_F_REQUEST | NLM_F_ACK); + struct nlmsghdr *hdr_p = nlmsg_put(msg_p, NL_AUTO_PORT, NL_AUTO_SEQ, RTM_DELNEIGH, 0, flags); + if (!hdr_p) { SWSS_LOG_ERROR("Netlink message header alloc failed for '%s'", ip.to_string().c_str()); - nlmsg_free(msg); + nlmsg_free(msg_p); return false; } - struct ndmsg *nd_msg = static_cast - (nlmsg_reserve(msg, sizeof(struct ndmsg), NLMSG_ALIGNTO)); - if (!nd_msg) + // Prepare ndmsg to remove an entry from neighbor table + struct ndmsg *nd_msg_p = static_cast + (nlmsg_reserve(msg_p, sizeof(struct ndmsg), NLMSG_ALIGNTO)); + if (!nd_msg_p) { SWSS_LOG_ERROR("Netlink ndmsg reserve failed for '%s'", ip.to_string().c_str()); - nlmsg_free(msg); + nlmsg_free(msg_p); return false; } + memset(nd_msg_p, 0, sizeof(struct ndmsg)); - memset(nd_msg, 0, sizeof(struct ndmsg)); + // Fill in the interface + nd_msg_p->ndm_ifindex = if_nametoindex(alias.c_str()); - nd_msg->ndm_ifindex = if_nametoindex(alias.c_str()); + // Fill in the IPv4/IPv6 address + if (!addIp(msg_p, ip)) + { + nlmsg_free(msg_p); + return false; + } + nd_msg_p->ndm_type = RTN_UNICAST; + nd_msg_p->ndm_family = ip.isV4() ? AF_INET : AF_INET6; - auto addr_len = ip.isV4()? sizeof(struct in_addr) : sizeof(struct in6_addr); + // Fill in the MAC address + if (!addMac(msg_p, mac)) + { + nlmsg_free(msg_p); + return false; + } + + // Send ndmsg + return send_message(m_nl_sock, msg_p); +} - struct rtattr *rta = static_cast - (nlmsg_reserve(msg, sizeof(struct rtattr) + addr_len, NLMSG_ALIGNTO)); - if (!rta) +bool NbrMgr::setNeighbor(const string& alias, const IpAddress& ip, const MacAddress& mac) +{ + SWSS_LOG_ENTER(); + + struct nl_msg *msg_p = nlmsg_alloc(); + if (!msg_p) { - SWSS_LOG_ERROR("Netlink rtattr (IP) failed for '%s'", ip.to_string().c_str()); - nlmsg_free(msg); + SWSS_LOG_ERROR("Netlink message alloc failed for '%s'", ip.to_string().c_str()); return false; } - rta->rta_type = NDA_DST; - rta->rta_len = static_cast(RTA_LENGTH(addr_len)); + auto flags = (NLM_F_REQUEST | NLM_F_ACK | NLM_F_CREATE | NLM_F_REPLACE); - nd_msg->ndm_type = RTN_UNICAST; - auto ip_addr = ip.getIp(); + struct nlmsghdr *hdr_p = nlmsg_put(msg_p, NL_AUTO_PORT, NL_AUTO_SEQ, RTM_NEWNEIGH, 0, flags); + if (!hdr_p) + { + SWSS_LOG_ERROR("Netlink message header alloc failed for '%s'", ip.to_string().c_str()); + nlmsg_free(msg_p); + return false; + } - if (ip.isV4()) + // Prepare ndmsg to add an entry to neighbor table + struct ndmsg *nd_msg_p = static_cast + (nlmsg_reserve(msg_p, sizeof(struct ndmsg), NLMSG_ALIGNTO)); + if (!nd_msg_p) { - nd_msg->ndm_family = AF_INET; - memcpy(RTA_DATA(rta), &ip_addr.ip_addr.ipv4_addr, addr_len); + SWSS_LOG_ERROR("Netlink ndmsg reserve failed for '%s'", ip.to_string().c_str()); + nlmsg_free(msg_p); + return false; } - else + memset(nd_msg_p, 0, sizeof(struct ndmsg)); + + // Populate interface + nd_msg_p->ndm_ifindex = if_nametoindex(alias.c_str()); + + // Populate IP + if (!addIp(msg_p, ip)) { - nd_msg->ndm_family = AF_INET6; - memcpy(RTA_DATA(rta), &ip_addr.ip_addr.ipv6_addr, addr_len); + nlmsg_free(msg_p); + return false; } + nd_msg_p->ndm_type = RTN_UNICAST; + nd_msg_p->ndm_family = ip.isV4() ? AF_INET : AF_INET6; + // Populate MAC is provided if (!mac) { /* * If mac is not provided, expected to resolve the MAC */ - nd_msg->ndm_state = NUD_DELAY; - nd_msg->ndm_flags = NTF_USE; + nd_msg_p->ndm_state = NUD_DELAY; + nd_msg_p->ndm_flags = NTF_USE; SWSS_LOG_INFO("Resolve request for '%s'", ip.to_string().c_str()); } @@ -163,29 +250,71 @@ bool NbrMgr::setNeighbor(const string& alias, const IpAddress& ip, const MacAddr { SWSS_LOG_INFO("Set mac address '%s'", mac.to_string().c_str()); - nd_msg->ndm_state = NUD_PERMANENT; - - auto mac_len = ETHER_ADDR_LEN; - auto mac_addr = mac.getMac(); - - struct rtattr *rta = static_cast - (nlmsg_reserve(msg, sizeof(struct rtattr) + mac_len, NLMSG_ALIGNTO)); - if (!rta) + if (!addMac(msg_p, mac)) { - SWSS_LOG_ERROR("Netlink rtattr (MAC) failed for '%s'", ip.to_string().c_str()); - nlmsg_free(msg); + nlmsg_free(msg_p); return false; } - rta->rta_type = NDA_LLADDR; - rta->rta_len = static_cast(RTA_LENGTH(mac_len)); - memcpy(RTA_DATA(rta), mac_addr, mac_len); + nd_msg_p->ndm_state = NUD_PERMANENT; } - return send_message(m_nl_sock, msg); + // Send ndmsg + return send_message(m_nl_sock, msg_p); } -void NbrMgr::doTask(Consumer &consumer) +void NbrMgr::doDeleteNeighTask(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); + const vector& data = kfvFieldsValues(t); + + string alias(keys[0]); + IpAddress ip(keys[1]); + string op = kfvOp(t); + MacAddress mac; + bool invalid_mac = false; + + for (auto idx : data) + { + const auto &field = fvField(idx); + const auto &value = fvValue(idx); + if (field == "mac") + { + try + { + mac = value; + } + catch (const std::invalid_argument& e) + { + SWSS_LOG_ERROR("Invalid Mac addr '%s' for '%s'", value.c_str(), kfvKey(t).c_str()); + invalid_mac = true; + break; + } + } + } + + + if (invalid_mac) + { + it = consumer.m_toSync.erase(it); + continue; + } + + if (!deleteNeighbor(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 +386,20 @@ 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); + } + + if (table_name == APP_NEIGH_RESOLVE_TABLE_NAME) + { + doDeleteNeighTask(consumer); + } + + SWSS_LOG_ERROR("Unknown REDIS table %s ", table_name.c_str()); +} diff --git a/cfgmgr/nbrmgr.h b/cfgmgr/nbrmgr.h index 6b0c664c80..999e040353 100644 --- a/cfgmgr/nbrmgr.h +++ b/cfgmgr/nbrmgr.h @@ -24,8 +24,13 @@ class NbrMgr : public Orch private: bool isIntfStateOk(const std::string &alias); + bool addMac(struct nl_msg* msg_p, const MacAddress& mac); + bool addIp(struct nl_msg* msg_p, const IpAddress& ip); bool setNeighbor(const std::string& alias, const IpAddress& ip, const MacAddress& mac); + bool deleteNeighbor(const std::string& alias, const IpAddress& ip, const MacAddress& mac); + void doDeleteNeighTask(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/neighorch.cpp b/orchagent/neighorch.cpp index ddae3268cc..b74677a714 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -19,47 +19,17 @@ extern FgNhgOrch *gFgNhgOrch; const int neighorch_pri = 30; -static bool send_message(struct nl_sock *socket_p, struct nl_msg *msg_p) +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) { - int err = 0; - - if (!socket_p) - { - SWSS_LOG_ERROR("Netlink socket null pointer"); - return false; - } - - if ((err = nl_send_auto(socket_p, msg_p)) < 0) - { - SWSS_LOG_ERROR("Netlink send message failed, error '%s'", nl_geterror(err)); - return false; - } - - nlmsg_free(msg_p); - return true; -} - -NeighOrch::NeighOrch(DBConnector *db, string tableName, IntfsOrch *intfsOrch, FdbOrch *fdbOrch, PortsOrch *portsOrch) : - Orch(db, tableName, neighorch_pri), m_intfsOrch(intfsOrch), m_fdbOrch(fdbOrch), m_portsOrch(portsOrch) -{ - int err = 0; - SWSS_LOG_ENTER(); m_fdbOrch->attach(this); gPortsOrch->attach(this); - - m_nl_sock = nl_socket_alloc(); - if (!m_nl_sock) - { - SWSS_LOG_ERROR("Netlink socket is NOT allocacted"); - } - else if ((err = nl_connect(m_nl_sock, NETLINK_ROUTE)) < 0) - { - SWSS_LOG_ERROR("Netlink socket connect failed, error '%s'", nl_geterror(err)); - nl_socket_free(m_nl_sock); - m_nl_sock = NULL; - } } NeighOrch::~NeighOrch() @@ -68,108 +38,24 @@ NeighOrch::~NeighOrch() { m_fdbOrch->detach(this); } - - if (m_nl_sock) - { - nl_close(m_nl_sock); - nl_socket_free(m_nl_sock); - } } -bool NeighOrch::flushNeighborEntry(const NeighborEntry &entry, const MacAddress &mac) +bool NeighOrch::resolveNeighborEntry(const NeighborEntry &entry, const MacAddress &mac) { - SWSS_LOG_ENTER(); - - IpAddress ip = entry.ip_address; - string alias = entry.alias; + vector data; + IpAddress ip = entry.ip_address; + string key, alias = entry.alias; - SWSS_LOG_INFO("Flushing ARP entry '%s' as FDB entry '%s' is flushed", - ip.to_string().c_str(), mac.to_string().c_str()); + key = alias + ":" + entry.ip_address.to_string(); + FieldValueTuple fvTuple("mac", mac.to_string().c_str()); + data.push_back(fvTuple); - if (!m_nl_sock) - { - SWSS_LOG_ERROR("Netlink socket is NOT allocated"); - return false; - } - - struct nl_msg *msg_p = nlmsg_alloc(); - if (!msg_p) - { - SWSS_LOG_ERROR("Netlink message alloc failed for '%s'", ip.to_string().c_str()); - return false; - } - - auto flags = (NLM_F_REQUEST | NLM_F_ACK); - struct nlmsghdr *hdr = nlmsg_put(msg_p, NL_AUTO_PORT, NL_AUTO_SEQ, RTM_DELNEIGH, 0, flags); - - if (!hdr) - { - SWSS_LOG_ERROR("Netlink message header alloc failed for '%s'", ip.to_string().c_str()); - nlmsg_free(msg_p); - return false; - } - - struct ndmsg *nd_msg_p = static_cast - (nlmsg_reserve(msg_p, sizeof(struct ndmsg), NLMSG_ALIGNTO)); - if (!nd_msg_p) - { - SWSS_LOG_ERROR("Netlink ndmsg reserve failed for '%s'", ip.to_string().c_str()); - nlmsg_free(msg_p); - return false; - } - - memset(nd_msg_p, 0, sizeof(struct ndmsg)); - - nd_msg_p->ndm_ifindex = if_nametoindex(alias.c_str()); - - // Fill in the IPV4/IPV6 address - auto addr_len = ip.isV4()? sizeof(struct in_addr) : sizeof(struct in6_addr); - - struct rtattr *rta_p = static_cast - (nlmsg_reserve(msg_p, sizeof(struct rtattr) + addr_len, NLMSG_ALIGNTO)); - if (!rta_p) - { - SWSS_LOG_ERROR("Netlink rtattr (IP) failed for '%s'", ip.to_string().c_str()); - nlmsg_free(msg_p); - return false; - } - - rta_p->rta_type = NDA_DST; - rta_p->rta_len = static_cast(RTA_LENGTH(addr_len)); - - nd_msg_p->ndm_type = RTN_UNICAST; - auto ip_addr = ip.getIp(); - - if (ip.isV4()) - { - nd_msg_p->ndm_family = AF_INET; - memcpy(RTA_DATA(rta_p), &ip_addr.ip_addr.ipv4_addr, addr_len); - } - else - { - nd_msg_p->ndm_family = AF_INET6; - memcpy(RTA_DATA(rta_p), &ip_addr.ip_addr.ipv6_addr, addr_len); - } - - // Fill in the MAC address - auto mac_len = ETHER_ADDR_LEN; - auto mac_addr = mac.getMac(); - - rta_p = static_cast - (nlmsg_reserve(msg_p, sizeof(struct rtattr) + mac_len, NLMSG_ALIGNTO)); - if (!rta_p) - { - SWSS_LOG_ERROR("Netlink rtattr (MAC) failed for '%s'", ip.to_string().c_str()); - nlmsg_free(msg_p); - return false; - } - - rta_p->rta_type = NDA_LLADDR; - rta_p->rta_len = static_cast(RTA_LENGTH(mac_len)); - memcpy(RTA_DATA(rta_p), mac_addr, mac_len); - - return send_message(m_nl_sock, msg_p); + SWSS_LOG_NOTICE("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: @@ -187,13 +73,6 @@ void NeighOrch::processFDBFlushUpdate(const FdbFlushUpdate& update) SWSS_LOG_NOTICE("processFDBFlushUpdate port: %s", update.port.m_alias.c_str()); - /* - if (update.port.m_admin_state_up == true && - update.port.m_oper_status == SAI_PORT_OPER_STATUS_UP) - { - return; - } */ - for (auto entry : update.entries) { // Get Vlan object @@ -215,11 +94,7 @@ void NeighOrch::processFDBFlushUpdate(const FdbFlushUpdate& update) if (neighborEntry.first.alias == vlan.m_alias && neighborEntry.second == entry.mac) { - SWSS_LOG_INFO("Flushing ARP [ %s , %s ] = { port: %s }", - vlan.m_alias.c_str(), - entry.mac.to_string().c_str(), - update.port.m_alias.c_str()); - flushNeighborEntry(neighborEntry.first, neighborEntry.second); + resolveNeighborEntry(neighborEntry.first, neighborEntry.second); } } } diff --git a/orchagent/neighorch.h b/orchagent/neighorch.h index 64bc477640..fb989ad447 100644 --- a/orchagent/neighorch.h +++ b/orchagent/neighorch.h @@ -10,6 +10,8 @@ #include "ipaddress.h" #include "nexthopkey.h" #include "netmsg.h" +#include "producerstatetable.h" +#include "schema.h" #define NHFLAGS_IFDOWN 0x1 // nexthop's outbound i/f is down @@ -60,7 +62,7 @@ class NeighOrch : public Orch, public Subject, public Observer PortsOrch *m_portsOrch; IntfsOrch *m_intfsOrch; FdbOrch *m_fdbOrch; - struct nl_sock *m_nl_sock; + ProducerStateTable m_appNeighResolveProducer; NeighborTable m_syncdNeighbors; NextHopTable m_syncdNextHops; @@ -75,7 +77,7 @@ class NeighOrch : public Orch, public Subject, public Observer bool clearNextHopFlag(const NextHopKey &, const uint32_t); void processFDBFlushUpdate(const FdbFlushUpdate &); - bool flushNeighborEntry(const NeighborEntry &, const MacAddress &); + bool resolveNeighborEntry(const NeighborEntry &, const MacAddress &); void doTask(Consumer &consumer); }; From dd114693976f1c334e27117a0299ffaede4bd0b4 Mon Sep 17 00:00:00 2001 From: Vasant Date: Mon, 23 Nov 2020 20:52:09 +0000 Subject: [PATCH 3/6] Code-review comments addressed --- cfgmgr/nbrmgr.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cfgmgr/nbrmgr.cpp b/cfgmgr/nbrmgr.cpp index a670b5a86d..c9d059f093 100644 --- a/cfgmgr/nbrmgr.cpp +++ b/cfgmgr/nbrmgr.cpp @@ -11,7 +11,6 @@ #include "nbrmgr.h" #include "exec.h" #include "shellcmd.h" -#include "subscriberstatetable.h" using namespace swss; @@ -61,9 +60,10 @@ NbrMgr::NbrMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, con SWSS_LOG_ERROR("Netlink socket connect failed, error '%s'", nl_geterror(err)); } - auto subscriberStateTable = new swss::SubscriberStateTable(appDb, APP_NEIGH_RESOLVE_TABLE_NAME, TableConsumable::DEFAULT_POP_BATCH_SIZE, default_orch_pri); - auto subscriberStateConsumer = new Consumer(subscriberStateTable, this, APP_NEIGH_RESOLVE_TABLE_NAME); - Orch::addExecutor(subscriberStateConsumer); + 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) From 4ca0179940851d74491c858f709906e9a0d152e7 Mon Sep 17 00:00:00 2001 From: Vasant Date: Mon, 23 Nov 2020 23:23:34 +0000 Subject: [PATCH 4/6] Addressed code-review comments --- cfgmgr/nbrmgr.cpp | 104 ++++------------------------------------ cfgmgr/nbrmgr.h | 4 +- orchagent/neighorch.cpp | 12 ++--- 3 files changed, 17 insertions(+), 103 deletions(-) diff --git a/cfgmgr/nbrmgr.cpp b/cfgmgr/nbrmgr.cpp index c9d059f093..a02eeadf0b 100644 --- a/cfgmgr/nbrmgr.cpp +++ b/cfgmgr/nbrmgr.cpp @@ -134,63 +134,6 @@ bool NbrMgr::addMac(struct nl_msg* msg_p, const MacAddress& mac) return true; } -bool NbrMgr::deleteNeighbor(const string& alias, const IpAddress& ip, const MacAddress& mac) -{ - SWSS_LOG_ENTER(); - - SWSS_LOG_NOTICE("Remove ARP entry '%s:%s -> %s'", - alias.c_str(), ip.to_string().c_str(), mac.to_string().c_str()); - - struct nl_msg *msg_p = nlmsg_alloc(); - if (!msg_p) - { - SWSS_LOG_ERROR("Netlink message alloc failed for '%s'", ip.to_string().c_str()); - return false; - } - - auto flags = (NLM_F_REQUEST | NLM_F_ACK); - struct nlmsghdr *hdr_p = nlmsg_put(msg_p, NL_AUTO_PORT, NL_AUTO_SEQ, RTM_DELNEIGH, 0, flags); - if (!hdr_p) - { - SWSS_LOG_ERROR("Netlink message header alloc failed for '%s'", ip.to_string().c_str()); - nlmsg_free(msg_p); - return false; - } - - // Prepare ndmsg to remove an entry from neighbor table - struct ndmsg *nd_msg_p = static_cast - (nlmsg_reserve(msg_p, sizeof(struct ndmsg), NLMSG_ALIGNTO)); - if (!nd_msg_p) - { - SWSS_LOG_ERROR("Netlink ndmsg reserve failed for '%s'", ip.to_string().c_str()); - nlmsg_free(msg_p); - return false; - } - memset(nd_msg_p, 0, sizeof(struct ndmsg)); - - // Fill in the interface - nd_msg_p->ndm_ifindex = if_nametoindex(alias.c_str()); - - // Fill in the IPv4/IPv6 address - if (!addIp(msg_p, ip)) - { - nlmsg_free(msg_p); - return false; - } - nd_msg_p->ndm_type = RTN_UNICAST; - nd_msg_p->ndm_family = ip.isV4() ? AF_INET : AF_INET6; - - // Fill in the MAC address - if (!addMac(msg_p, mac)) - { - nlmsg_free(msg_p); - return false; - } - - // Send ndmsg - return send_message(m_nl_sock, msg_p); -} - bool NbrMgr::setNeighbor(const string& alias, const IpAddress& ip, const MacAddress& mac) { SWSS_LOG_ENTER(); @@ -263,50 +206,20 @@ bool NbrMgr::setNeighbor(const string& alias, const IpAddress& ip, const MacAddr return send_message(m_nl_sock, msg_p); } -void NbrMgr::doDeleteNeighTask(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); - const vector& data = kfvFieldsValues(t); - - string alias(keys[0]); - IpAddress ip(keys[1]); - string op = kfvOp(t); - MacAddress mac; - bool invalid_mac = false; - - for (auto idx : data) - { - const auto &field = fvField(idx); - const auto &value = fvValue(idx); - if (field == "mac") - { - try - { - mac = value; - } - catch (const std::invalid_argument& e) - { - SWSS_LOG_ERROR("Invalid Mac addr '%s' for '%s'", value.c_str(), kfvKey(t).c_str()); - invalid_mac = true; - break; - } - } - } - - - if (invalid_mac) - { - it = consumer.m_toSync.erase(it); - continue; - } + KeyOpFieldsValuesTuple t = it->second; + vector keys = tokenize(kfvKey(t),delimiter); + MacAddress mac; + IpAddress ip(keys[1]); + string alias(keys[0]); - if (!deleteNeighbor(alias, ip, mac)) + if (!setNeighbor(alias, ip, mac)) { SWSS_LOG_ERROR("Neigh entry resolve failed for '%s'", kfvKey(t).c_str()); } @@ -318,6 +231,7 @@ void NbrMgr::doSetNeighTask(Consumer &consumer) { SWSS_LOG_ENTER(); + auto it = consumer.m_toSync.begin(); while (it != consumer.m_toSync.end()) { @@ -398,7 +312,7 @@ void NbrMgr::doTask(Consumer &consumer) if (table_name == APP_NEIGH_RESOLVE_TABLE_NAME) { - doDeleteNeighTask(consumer); + doResolveNeighTask(consumer); } SWSS_LOG_ERROR("Unknown REDIS table %s ", table_name.c_str()); diff --git a/cfgmgr/nbrmgr.h b/cfgmgr/nbrmgr.h index 999e040353..2fc4d799cf 100644 --- a/cfgmgr/nbrmgr.h +++ b/cfgmgr/nbrmgr.h @@ -27,9 +27,9 @@ class NbrMgr : public Orch bool addMac(struct nl_msg* msg_p, const MacAddress& mac); bool addIp(struct nl_msg* msg_p, const IpAddress& ip); bool setNeighbor(const std::string& alias, const IpAddress& ip, const MacAddress& mac); - bool deleteNeighbor(const std::string& alias, const IpAddress& ip, const MacAddress& mac); + //bool deleteNeighbor(const std::string& alias, const IpAddress& ip, const MacAddress& mac); - void doDeleteNeighTask(Consumer &consumer); + void doResolveNeighTask(Consumer &consumer); void doSetNeighTask(Consumer &consumer); void doTask(Consumer &consumer); diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index b74677a714..6648e5a812 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -29,7 +29,6 @@ NeighOrch::NeighOrch(DBConnector *appDb, string tableName, IntfsOrch *intfsOrch, SWSS_LOG_ENTER(); m_fdbOrch->attach(this); - gPortsOrch->attach(this); } NeighOrch::~NeighOrch() @@ -47,11 +46,13 @@ bool NeighOrch::resolveNeighborEntry(const NeighborEntry &entry, const MacAddres 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_NOTICE("Flushing ARP entry '%s:%s --> %s'", - alias.c_str(), ip.to_string().c_str(), mac.to_string().c_str()); + 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; } @@ -65,12 +66,11 @@ bool NeighOrch::resolveNeighborEntry(const NeighborEntry &entry, const MacAddres * 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. Also this ensures that underlying physical port operation - * status or admin state is DOWN. + * respectively. */ void NeighOrch::processFDBFlushUpdate(const FdbFlushUpdate& update) { - SWSS_LOG_NOTICE("processFDBFlushUpdate port: %s", + SWSS_LOG_INFO("processFDBFlushUpdate port: %s", update.port.m_alias.c_str()); for (auto entry : update.entries) From a6a29c8ed525d0e2a9f92b6bd024a090e1e11937 Mon Sep 17 00:00:00 2001 From: Vasant Date: Tue, 24 Nov 2020 01:03:47 +0000 Subject: [PATCH 5/6] Addressed code-review comments --- cfgmgr/nbrmgr.cpp | 120 ++++++++++++++++++++-------------------------- cfgmgr/nbrmgr.h | 3 -- 2 files changed, 52 insertions(+), 71 deletions(-) diff --git a/cfgmgr/nbrmgr.cpp b/cfgmgr/nbrmgr.cpp index a02eeadf0b..e7d714a2f1 100644 --- a/cfgmgr/nbrmgr.cpp +++ b/cfgmgr/nbrmgr.cpp @@ -92,54 +92,12 @@ bool NbrMgr::isNeighRestoreDone() return false; } - -bool NbrMgr::addIp(struct nl_msg* msg_p, const IpAddress& ip) -{ - auto ip_addr = ip.getIp(); - auto addr_len = ip.isV4()? sizeof(struct in_addr) : sizeof(struct in6_addr); - - struct rtattr *rta_p = static_cast - (nlmsg_reserve(msg_p, sizeof(struct rtattr) + addr_len, NLMSG_ALIGNTO)); - if (!rta_p) - { - SWSS_LOG_ERROR("Netlink rtattr (IP) failed for '%s'", ip.to_string().c_str()); - return false; - } - - rta_p->rta_type = NDA_DST; - rta_p->rta_len = static_cast(RTA_LENGTH(addr_len)); - ip.isV4() ? memcpy(RTA_DATA(rta_p), &ip_addr.ip_addr.ipv4_addr, addr_len) : - memcpy(RTA_DATA(rta_p), &ip_addr.ip_addr.ipv6_addr, addr_len); - - return true; -} - -bool NbrMgr::addMac(struct nl_msg* msg_p, const MacAddress& mac) -{ - auto mac_len = ETHER_ADDR_LEN; - auto mac_addr = mac.getMac(); - - struct rtattr *rta_p = static_cast - (nlmsg_reserve(msg_p, sizeof(struct rtattr) + mac_len, NLMSG_ALIGNTO)); - if (!rta_p) - { - SWSS_LOG_ERROR("Netlink rtattr (MAC) failed for '%s'", mac.to_string().c_str()); - return false; - } - - rta_p->rta_type = NDA_LLADDR; - rta_p->rta_len = static_cast(RTA_LENGTH(mac_len)); - memcpy(RTA_DATA(rta_p), mac_addr, mac_len); - - return true; -} - bool NbrMgr::setNeighbor(const string& alias, const IpAddress& ip, const MacAddress& mac) { SWSS_LOG_ENTER(); - struct nl_msg *msg_p = nlmsg_alloc(); - if (!msg_p) + struct nl_msg *msg = nlmsg_alloc(); + if (!msg) { SWSS_LOG_ERROR("Netlink message alloc failed for '%s'", ip.to_string().c_str()); return false; @@ -147,45 +105,62 @@ bool NbrMgr::setNeighbor(const string& alias, const IpAddress& ip, const MacAddr auto flags = (NLM_F_REQUEST | NLM_F_ACK | NLM_F_CREATE | NLM_F_REPLACE); - struct nlmsghdr *hdr_p = nlmsg_put(msg_p, NL_AUTO_PORT, NL_AUTO_SEQ, RTM_NEWNEIGH, 0, flags); - if (!hdr_p) + struct nlmsghdr *hdr = nlmsg_put(msg, NL_AUTO_PORT, NL_AUTO_SEQ, RTM_NEWNEIGH, 0, flags); + if (!hdr) { SWSS_LOG_ERROR("Netlink message header alloc failed for '%s'", ip.to_string().c_str()); - nlmsg_free(msg_p); + nlmsg_free(msg); return false; } - // Prepare ndmsg to add an entry to neighbor table - struct ndmsg *nd_msg_p = static_cast - (nlmsg_reserve(msg_p, sizeof(struct ndmsg), NLMSG_ALIGNTO)); - if (!nd_msg_p) + struct ndmsg *nd_msg = static_cast + (nlmsg_reserve(msg, sizeof(struct ndmsg), NLMSG_ALIGNTO)); + if (!nd_msg) { SWSS_LOG_ERROR("Netlink ndmsg reserve failed for '%s'", ip.to_string().c_str()); - nlmsg_free(msg_p); + nlmsg_free(msg); return false; } - memset(nd_msg_p, 0, sizeof(struct ndmsg)); - // Populate interface - nd_msg_p->ndm_ifindex = if_nametoindex(alias.c_str()); + memset(nd_msg, 0, sizeof(struct ndmsg)); - // Populate IP - if (!addIp(msg_p, ip)) + nd_msg->ndm_ifindex = if_nametoindex(alias.c_str()); + + auto addr_len = ip.isV4()? sizeof(struct in_addr) : sizeof(struct in6_addr); + + struct rtattr *rta = static_cast + (nlmsg_reserve(msg, sizeof(struct rtattr) + addr_len, NLMSG_ALIGNTO)); + if (!rta) { - nlmsg_free(msg_p); + SWSS_LOG_ERROR("Netlink rtattr (IP) failed for '%s'", ip.to_string().c_str()); + nlmsg_free(msg); return false; } - nd_msg_p->ndm_type = RTN_UNICAST; - nd_msg_p->ndm_family = ip.isV4() ? AF_INET : AF_INET6; - // Populate MAC is provided + rta->rta_type = NDA_DST; + rta->rta_len = static_cast(RTA_LENGTH(addr_len)); + + nd_msg->ndm_type = RTN_UNICAST; + auto ip_addr = ip.getIp(); + + if (ip.isV4()) + { + nd_msg->ndm_family = AF_INET; + memcpy(RTA_DATA(rta), &ip_addr.ip_addr.ipv4_addr, addr_len); + } + else + { + nd_msg->ndm_family = AF_INET6; + memcpy(RTA_DATA(rta), &ip_addr.ip_addr.ipv6_addr, addr_len); + } + if (!mac) { /* * If mac is not provided, expected to resolve the MAC */ - nd_msg_p->ndm_state = NUD_DELAY; - nd_msg_p->ndm_flags = NTF_USE; + nd_msg->ndm_state = NUD_DELAY; + nd_msg->ndm_flags = NTF_USE; SWSS_LOG_INFO("Resolve request for '%s'", ip.to_string().c_str()); } @@ -193,17 +168,26 @@ bool NbrMgr::setNeighbor(const string& alias, const IpAddress& ip, const MacAddr { SWSS_LOG_INFO("Set mac address '%s'", mac.to_string().c_str()); - if (!addMac(msg_p, mac)) + nd_msg->ndm_state = NUD_PERMANENT; + + auto mac_len = ETHER_ADDR_LEN; + auto mac_addr = mac.getMac(); + + struct rtattr *rta = static_cast + (nlmsg_reserve(msg, sizeof(struct rtattr) + mac_len, NLMSG_ALIGNTO)); + if (!rta) { - nlmsg_free(msg_p); + SWSS_LOG_ERROR("Netlink rtattr (MAC) failed for '%s'", ip.to_string().c_str()); + nlmsg_free(msg); return false; } - nd_msg_p->ndm_state = NUD_PERMANENT; + rta->rta_type = NDA_LLADDR; + rta->rta_len = static_cast(RTA_LENGTH(mac_len)); + memcpy(RTA_DATA(rta), mac_addr, mac_len); } - // Send ndmsg - return send_message(m_nl_sock, msg_p); + return send_message(m_nl_sock, msg); } void NbrMgr::doResolveNeighTask(Consumer &consumer) diff --git a/cfgmgr/nbrmgr.h b/cfgmgr/nbrmgr.h index 2fc4d799cf..3a56c4b6cc 100644 --- a/cfgmgr/nbrmgr.h +++ b/cfgmgr/nbrmgr.h @@ -24,10 +24,7 @@ class NbrMgr : public Orch private: bool isIntfStateOk(const std::string &alias); - bool addMac(struct nl_msg* msg_p, const MacAddress& mac); - bool addIp(struct nl_msg* msg_p, const IpAddress& ip); bool setNeighbor(const std::string& alias, const IpAddress& ip, const MacAddress& mac); - //bool deleteNeighbor(const std::string& alias, const IpAddress& ip, const MacAddress& mac); void doResolveNeighTask(Consumer &consumer); void doSetNeighTask(Consumer &consumer); From 622ad2acc02d5ce698309e3f0a15b57279cb336c Mon Sep 17 00:00:00 2001 From: Vasant Date: Tue, 24 Nov 2020 02:17:52 +0000 Subject: [PATCH 6/6] Cleanup --- cfgmgr/nbrmgr.cpp | 10 ++++------ orchagent/neighorch.cpp | 3 --- orchagent/neighorch.h | 1 - 3 files changed, 4 insertions(+), 10 deletions(-) diff --git a/cfgmgr/nbrmgr.cpp b/cfgmgr/nbrmgr.cpp index e7d714a2f1..0b256cc318 100644 --- a/cfgmgr/nbrmgr.cpp +++ b/cfgmgr/nbrmgr.cpp @@ -215,7 +215,6 @@ void NbrMgr::doSetNeighTask(Consumer &consumer) { SWSS_LOG_ENTER(); - auto it = consumer.m_toSync.begin(); while (it != consumer.m_toSync.end()) { @@ -292,12 +291,11 @@ void NbrMgr::doTask(Consumer &consumer) if (table_name == CFG_NEIGH_TABLE_NAME) { doSetNeighTask(consumer); - } - - if (table_name == APP_NEIGH_RESOLVE_TABLE_NAME) + } else if (table_name == APP_NEIGH_RESOLVE_TABLE_NAME) { doResolveNeighTask(consumer); + } else + { + SWSS_LOG_ERROR("Unknown REDIS table %s ", table_name.c_str()); } - - SWSS_LOG_ERROR("Unknown REDIS table %s ", table_name.c_str()); } diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index 6648e5a812..0c390c0cba 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -1,7 +1,4 @@ #include -#include -#include - #include "neighorch.h" #include "logger.h" #include "swssnet.h" diff --git a/orchagent/neighorch.h b/orchagent/neighorch.h index fb989ad447..aa235bd55c 100644 --- a/orchagent/neighorch.h +++ b/orchagent/neighorch.h @@ -9,7 +9,6 @@ #include "ipaddress.h" #include "nexthopkey.h" -#include "netmsg.h" #include "producerstatetable.h" #include "schema.h"