Skip to content

Commit

Permalink
[FDB] Fix fbdorch to properly handle syncd FDB FLUSH Notif (sonic-net…
Browse files Browse the repository at this point in the history 
…#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 <[email protected]>
  • Loading branch information
@vivekrnv
vivekrnv authored May 18, 2022
1 parent d16f8f1 commit bbbd5f4
Show file tree
Hide file tree
Showing 7 changed files with 577 additions and 110 deletions.
223 changes: 123 additions & 100 deletions orchagent/fdborch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
extern sai_fdb_api_t *sai_fdb_api;

extern sai_object_id_t gSwitchId;
extern PortsOrch* gPortsOrch;
extern CrmOrch * gCrmOrch;
extern MlagOrch* gMlagOrch;
extern Directory<Orch*> gDirectory;
Expand Down Expand Up @@ -175,6 +174,107 @@ bool FdbOrch::storeFdbEntryState(const FdbUpdate& update)
}
}

/*
clears stateDb and decrements corresponding internal fdb counters
*/
void FdbOrch::clearFdbEntry(const MacAddress& mac,
const sai_object_id_t& bv_id,
const string& port_alias)
{
FdbUpdate update;
update.entry.mac = mac;
update.entry.bv_id = bv_id;
update.add = false;

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

/* Decrement port fdb_counter */
m_portsOrch->decrFdbCount(port_alias, 1);

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

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

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

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

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


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

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

return;
}
}

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

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

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

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

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

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

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

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

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

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

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

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

storeFdbEntryState(update);

notify(SUBJECT_TYPE_FDB_CHANGE, &update);

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

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

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

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

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

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

if (!gPortsOrch->allPortsReady())
if (!m_portsOrch->allPortsReady())
{
return;
}
Expand Down Expand Up @@ -892,7 +915,7 @@ void FdbOrch::doTask(NotificationConsumer& consumer)
SWSS_LOG_ERROR("Receive wrong port to flush fdb!");
return;
}
if (!gPortsOrch->getPort(alias, port))
if (!m_portsOrch->getPort(alias, port))
{
SWSS_LOG_ERROR("Get Port from port(%s) failed!", alias.c_str());
return;
Expand All @@ -913,7 +936,7 @@ void FdbOrch::doTask(NotificationConsumer& consumer)
SWSS_LOG_ERROR("Receive wrong vlan to flush fdb!");
return;
}
if (!gPortsOrch->getPort(vlan, vlanPort))
if (!m_portsOrch->getPort(vlan, vlanPort))
{
SWSS_LOG_ERROR("Get Port from vlan(%s) failed!", vlan.c_str());
return;
Expand All @@ -939,12 +962,12 @@ void FdbOrch::doTask(NotificationConsumer& consumer)
SWSS_LOG_ERROR("Receive wrong port or vlan to flush fdb!");
return;
}
if (!gPortsOrch->getPort(alias, port))
if (!m_portsOrch->getPort(alias, port))
{
SWSS_LOG_ERROR("Get Port from port(%s) failed!", alias.c_str());
return;
}
if (!gPortsOrch->getPort(vlan, vlanPort))
if (!m_portsOrch->getPort(vlan, vlanPort))
{
SWSS_LOG_ERROR("Get Port from vlan(%s) failed!", vlan.c_str());
return;
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 @@ -122,6 +122,9 @@ class FdbOrch: public Orch, public Subject, public Observer

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

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

#endif /* SWSS_FDBORCH_H */
17 changes: 16 additions & 1 deletion orchagent/portsorch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4573,9 +4573,10 @@ bool PortsOrch::removeVlan(Port vlan)
return false for retry */
if (vlan.m_fdb_count > 0)
{
SWSS_LOG_NOTICE("VLAN %s still has assiciated FDB entries", vlan.m_alias.c_str());
SWSS_LOG_NOTICE("VLAN %s still has %d FDB entries", vlan.m_alias.c_str(), vlan.m_fdb_count);
return false;
}

if (m_port_ref_count[vlan.m_alias] > 0)
{
SWSS_LOG_ERROR("Failed to remove ref count %d VLAN %s",
Expand Down Expand Up @@ -6928,3 +6929,17 @@ std::unordered_set<std::string> PortsOrch::generateCounterStats(const string& ty
}
return counter_stats;
}

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

bool getPortOperStatus(const Port& port, sai_port_oper_status_t& status) const;

bool decrFdbCount(const string& alias, int count);
private:
unique_ptr<Table> m_counterTable;
unique_ptr<Table> m_counterLagTable;
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 @@ -26,6 +26,7 @@ tests_SOURCES = aclorch_ut.cpp \
routeorch_ut.cpp \
qosorch_ut.cpp \
bufferorch_ut.cpp \
fdborch/flush_syncd_notif_ut.cpp \
copporch_ut.cpp \
saispy_ut.cpp \
consumer_ut.cpp \
Expand Down
Loading

0 comments on commit bbbd5f4

Please sign in to comment.