Skip to content

Commit

Permalink
[FDB] [202012] Fix fbdorch to properly handle syncd FDB FLUSH Notif (#…
Browse files Browse the repository at this point in the history 
…2401)

* [FDB] Fix fbdorch to properly handle syncd FDB FLUSH Notif (#2254)

Vlan delete couldn't be handled by OA when there is fdb learnt on the member and when the member is deleted
This inability of handling APPL_DB notif is affecting warm-restart. FDB Entry from State DB is not removed.
OA doesn't have the logic to handle consolidate flush notif coming from syncd
FdbOrch doesn't have logic to clear internal cache and decrement corresponding fdb counters during a flush notification

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>
  • Loading branch information
@vivekrnv
vivekrnv authored Aug 16, 2022
1 parent 4190c13 commit aa7b546
Show file tree
Hide file tree
Showing 7 changed files with 565 additions and 102 deletions.
218 changes: 116 additions & 102 deletions orchagent/fdborch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
extern sai_fdb_api_t *sai_fdb_api;

extern sai_object_id_t gSwitchId;
extern PortsOrch* gPortsOrch;
extern CrmOrch * gCrmOrch;
extern Directory<Orch*> gDirectory;

Expand Down Expand Up @@ -151,6 +150,104 @@ bool FdbOrch::storeFdbEntryState(const FdbUpdate& update)
}
}

/*
clears stateDb and decrements corresponding internal fdb counters
*/
void FdbOrch::clearFdbEntry(const FdbEntry& entry)
{
FdbUpdate update;
update.entry = entry;
update.add = false;

/* Fetch Vlan and decrement the counter */
Port temp_vlan;
if (m_portsOrch->getPort(entry.bv_id, temp_vlan))
{
m_portsOrch->decrFdbCount(temp_vlan.m_alias, 1);
}

/* Decrement port fdb_counter */
m_portsOrch->decrFdbCount(entry.port_name, 1);

/* Remove the FdbEntry from the internal cache, update state DB and CRM counter */
storeFdbEntryState(update);
notify(SUBJECT_TYPE_FDB_CHANGE, &update);

SWSS_LOG_INFO("FdbEntry removed from internal cache, MAC: %s , port: %s, BVID: 0x%" PRIx64,
update.entry.mac.to_string().c_str(), update.entry.port_name.c_str(), update.entry.bv_id);
}

/*
Handles the SAI_FDB_EVENT_FLUSHED notification recieved from syncd
*/
void FdbOrch::handleSyncdFlushNotif(const sai_object_id_t& bv_id,
const sai_object_id_t& bridge_port_id,
const MacAddress& mac)
{
// Consolidated flush will have a zero mac
MacAddress flush_mac("00:00:00:00:00:00");

/* TODO: Read the SAI_FDB_FLUSH_ATTR_ENTRY_TYPE attr from the flush notif
and clear the entries accordingly, currently only non-static entries are flushed
*/
if (bridge_port_id == SAI_NULL_OBJECT_ID && bv_id == SAI_NULL_OBJECT_ID)
{
for (auto itr = m_entries.begin(); itr != m_entries.end();)
{
auto curr = itr++;
if (curr->second.type != "static" && (curr->first.mac == mac || mac == flush_mac))
{
clearFdbEntry(curr->first);
}
}
}
else if (bv_id == SAI_NULL_OBJECT_ID)
{
/* FLUSH based on PORT */
for (auto itr = m_entries.begin(); itr != m_entries.end();)
{
auto curr = itr++;
if (curr->second.bridge_port_id == bridge_port_id)
{
if (curr->second.type != "static" && (curr->first.mac == mac || mac == flush_mac))
{
clearFdbEntry(curr->first);
}
}
}
}
else if (bridge_port_id == SAI_NULL_OBJECT_ID)
{
/* FLUSH based on BV_ID */
for (auto itr = m_entries.begin(); itr != m_entries.end();)
{
auto curr = itr++;
if (curr->first.bv_id == bv_id)
{
if (curr->second.type != "static" && (curr->first.mac == mac || mac == flush_mac))
{
clearFdbEntry(curr->first);
}
}
}
}
else
{
/* FLUSH based on port and VLAN */
for (auto itr = m_entries.begin(); itr != m_entries.end();)
{
auto curr = itr++;
if (curr->first.bv_id == bv_id && curr->second.bridge_port_id == bridge_port_id)
{
if (curr->second.type != "static" && (curr->first.mac == mac || mac == flush_mac))
{
clearFdbEntry(curr->first);
}
}
}
}
}

void FdbOrch::update(sai_fdb_event_t type,
const sai_fdb_entry_t* entry,
sai_object_id_t bridge_port_id)
Expand All @@ -168,24 +265,29 @@ void FdbOrch::update(sai_fdb_event_t type,
type, update.entry.mac.to_string().c_str(),
entry->bv_id, bridge_port_id);


if (bridge_port_id &&
!m_portsOrch->getPortByBridgePortId(bridge_port_id, update.port))
{
if (type == SAI_FDB_EVENT_FLUSHED)
{
/* In case of flush - can be ignored due to a race.
There are notifications about FDB FLUSH (syncd/sai_redis) on port,
which was already removed by orchagent as a result of
removeVlanMember action (removeBridgePort) */
/* There are notifications about FDB FLUSH (syncd/sai_redis) on port,
which was already removed by orchagent as a result of removeVlanMember
action (removeBridgePort). But the internal cleanup of statedb and
internal counters is yet to be performed, thus continue
*/
SWSS_LOG_INFO("Flush event: Failed to get port by bridge port ID 0x%" PRIx64 ".",
bridge_port_id);

} else {
SWSS_LOG_ERROR("Failed to get port by bridge port ID 0x%" PRIx64 ".",
bridge_port_id);

return;
}
}

if (entry->bv_id &&
!m_portsOrch->getPort(entry->bv_id, vlan))
{
SWSS_LOG_NOTICE("FdbOrch notification type %d: Failed to locate vlan port from bv_id 0x%" PRIx64, type, entry->bv_id);
return;
}

Expand All @@ -195,12 +297,6 @@ void FdbOrch::update(sai_fdb_event_t type,
{
SWSS_LOG_INFO("Received LEARN event for bvid=0x%" PRIx64 "mac=%s port=0x%" PRIx64, entry->bv_id, update.entry.mac.to_string().c_str(), bridge_port_id);

if (!m_portsOrch->getPort(entry->bv_id, vlan))
{
SWSS_LOG_ERROR("FdbOrch LEARN notification: Failed to locate vlan port from bv_id 0x%" PRIx64, entry->bv_id);
return;
}

// we already have such entries
auto existing_entry = m_entries.find(update.entry);
if (existing_entry != m_entries.end())
Expand Down Expand Up @@ -229,11 +325,6 @@ void FdbOrch::update(sai_fdb_event_t type,
SWSS_LOG_INFO("Received AGE event for bvid=0x%" PRIx64 " mac=%s port=0x%" PRIx64,
entry->bv_id, update.entry.mac.to_string().c_str(), bridge_port_id);

if (!m_portsOrch->getPort(entry->bv_id, vlan))
{
SWSS_LOG_NOTICE("FdbOrch AGE notification: Failed to locate vlan port from bv_id 0x%" PRIx64, entry->bv_id);
}

auto existing_entry = m_entries.find(update.entry);
// we don't have such entries
if (existing_entry == m_entries.end())
Expand Down Expand Up @@ -326,12 +417,6 @@ void FdbOrch::update(sai_fdb_event_t type,
SWSS_LOG_INFO("Received MOVE event for bvid=0x%" PRIx64 " mac=%s port=0x%" PRIx64,
entry->bv_id, update.entry.mac.to_string().c_str(), bridge_port_id);

if (!m_portsOrch->getPort(entry->bv_id, vlan))
{
SWSS_LOG_ERROR("FdbOrch MOVE notification: Failed to locate vlan port from bv_id 0x%" PRIx64, entry->bv_id);
return;
}

// We should already have such entry
if (existing_entry == m_entries.end())
{
Expand Down Expand Up @@ -369,86 +454,15 @@ void FdbOrch::update(sai_fdb_event_t type,
bridge_port_id);

string vlanName = "-";
if (entry->bv_id) {
Port vlan;

if (!m_portsOrch->getPort(entry->bv_id, vlan))
{
SWSS_LOG_NOTICE("FdbOrch notification: Failed to locate vlan\
port from bv_id 0x%" PRIx64, entry->bv_id);
return;
}
if (!vlan.m_alias.empty()) {
vlanName = "Vlan" + to_string(vlan.m_vlan_info.vlan_id);
}

SWSS_LOG_INFO("FDB Flush: [ %s , %s ] = { port: %s }", update.entry.mac.to_string().c_str(),
vlanName.c_str(), update.port.m_alias.c_str());

if (bridge_port_id == SAI_NULL_OBJECT_ID &&
entry->bv_id == SAI_NULL_OBJECT_ID)
{
SWSS_LOG_INFO("FDB Flush: [ %s , %s ] = { port: - }",
update.entry.mac.to_string().c_str(), vlanName.c_str());
for (auto itr = m_entries.begin(); itr != m_entries.end();)
{
/*
TODO: here should only delete the dynamic fdb entries,
but unfortunately in structure FdbEntry currently have
no member to indicate the fdb entry type,
if there is static mac added, here will have issue.
*/
update.entry.mac = itr->first.mac;
update.entry.bv_id = itr->first.bv_id;
update.add = false;
itr++;

storeFdbEntryState(update);

for (auto observer: m_observers)
{
observer->update(SUBJECT_TYPE_FDB_CHANGE, &update);
}
}
}
else if (entry->bv_id == SAI_NULL_OBJECT_ID)
{
/* FLUSH based on port */
SWSS_LOG_INFO("FDB Flush: [ %s , %s ] = { port: %s }",
update.entry.mac.to_string().c_str(),
vlanName.c_str(), update.port.m_alias.c_str());

for (auto itr = m_entries.begin(); itr != m_entries.end();)
{
auto next_item = std::next(itr);
if (itr->first.port_name == update.port.m_alias)
{
update.entry.mac = itr->first.mac;
update.entry.bv_id = itr->first.bv_id;
update.add = false;

storeFdbEntryState(update);
handleSyncdFlushNotif(entry->bv_id, bridge_port_id, update.entry.mac);

for (auto observer: m_observers)
{
observer->update(SUBJECT_TYPE_FDB_CHANGE, &update);
}
}
itr = next_item;
}
}
else if (bridge_port_id == SAI_NULL_OBJECT_ID)
{
/* FLUSH based on VLAN - unsupported */
SWSS_LOG_ERROR("Unsupported FDB Flush: [ %s , %s ] = { port: - }",
update.entry.mac.to_string().c_str(),
vlanName.c_str());

}
else
{
/* FLUSH based on port and VLAN - unsupported */
SWSS_LOG_ERROR("Unsupported FDB Flush: [ %s , %s ] = { port: %s }",
update.entry.mac.to_string().c_str(),
vlanName.c_str(), update.port.m_alias.c_str());
}
break;
}

Expand Down Expand Up @@ -524,7 +538,7 @@ void FdbOrch::doTask(Consumer& consumer)
{
SWSS_LOG_ENTER();

if (!gPortsOrch->allPortsReady())
if (!m_portsOrch->allPortsReady())
{
return;
}
Expand Down Expand Up @@ -681,7 +695,7 @@ void FdbOrch::doTask(NotificationConsumer& consumer)
{
SWSS_LOG_ENTER();

if (!gPortsOrch->allPortsReady())
if (!m_portsOrch->allPortsReady())
{
return;
}
Expand Down
3 changes: 3 additions & 0 deletions orchagent/fdborch.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,9 @@ class FdbOrch: public Orch, public Subject, public Observer

bool storeFdbEntryState(const FdbUpdate& update);
void notifyTunnelOrch(Port& port);

void clearFdbEntry(const FdbEntry&);
void handleSyncdFlushNotif(const sai_object_id_t&, const sai_object_id_t&, const MacAddress& );
};

#endif /* SWSS_FDBORCH_H */
14 changes: 14 additions & 0 deletions orchagent/portsorch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5355,3 +5355,17 @@ std::unordered_set<std::string> PortsOrch::generateCounterStats(const string& ty
}
return counter_stats;
}

bool PortsOrch::decrFdbCount(const std::string& alias, int count)
{
auto itr = m_portList.find(alias);
if (itr == m_portList.end())
{
return false;
}
else
{
itr->second.m_fdb_count -= count;
}
return true;
}
1 change: 1 addition & 0 deletions orchagent/portsorch.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ class PortsOrch : public Orch, public Subject
bool removeVlanMember(Port &vlan, Port &port);
bool isVlanMember(Port &vlan, Port &port);

bool decrFdbCount(const string& alias, int count);
private:
unique_ptr<Table> m_counterTable;
unique_ptr<Table> m_counterLagTable;
Expand Down
1 change: 1 addition & 0 deletions tests/mock_tests/Makefile.am
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ LDADD_GTEST = -L/usr/src/gtest

tests_SOURCES = aclorch_ut.cpp \
portsorch_ut.cpp \
fdborch/flush_syncd_notif_ut.cpp \
saispy_ut.cpp \
consumer_ut.cpp \
ut_saihelper.cpp \
Expand Down
Loading

0 comments on commit aa7b546

Please sign in to comment.