Skip to content

[fdborch]: Flush fdb entries on port link down (Resolves #1063, #1294, resolves Azure/sonic-buildimage#4184) #1295

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
MikeHcChen wants to merge 1 commit into from
Closed

[fdborch]: Flush fdb entries on port link down (Resolves #1063, #1294, resolves Azure/sonic-buildimage#4184) #1295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 127 additions & 7 deletions orchagent/fdborch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,17 +186,95 @@ void FdbOrch::update(sai_fdb_event_t type, const sai_fdb_entry_t* entry, sai_obj
}
else if (bridge_port_id && entry->bv_id == SAI_NULL_OBJECT_ID)
{
/*this is a placeholder for flush port fdb case, not supported yet.*/
SWSS_LOG_ERROR("FdbOrch notification: not supported flush port fdb action, port_id = 0x%" PRIx64 ", bv_id = 0x%" PRIx64 ".", bridge_port_id, entry->bv_id);
if (!m_portsOrch->getPortByBridgePortId(bridge_port_id, update.port))
Copy link
Copy Markdown
Contributor

@vasant17 vasant17 May 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we call SAI flush API, with bridge_port_id, we get SAI_FDB_EVENT_AGED event but NOT SAI_FDB_EVENT_FLUSHED. I did verify it by looking at logs. So, here is my understanding

  1. If we call SAI flush API with bvid and bridge_port_id both set to SAI_NULL_OBJECT_ID, we get the notification SAI_FDB_EVENT_FLUSHED. So the new code you added wont be exericised as it will enter the first if case
if (bridge_port_id == SAI_NULL_OBJECT_ID && entry->bv_id == SAI_NULL_OBJECT_ID)
  1. When we call SAI FDB flush API with either bvid or bridge_port_id or both valid, we would end up receiving SAI_FDB_EVENT_AGED. Hence. Again the new code you added here wont be exercised.

Copy link
Copy Markdown
Contributor

@vasant17 vasant17 May 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we call SAI flush API, with bridge_port_id, we get SAI_FDB_EVENT_AGED event but NOT SAI_FDB_EVENT_FLUSHED. I did verify it by looking at logs. So, here is my understanding

  1. If we call SAI flush API with bvid and bridge_port_id both set to SAI_NULL_OBJECT_ID, we get the notification SAI_FDB_EVENT_FLUSHED. So the new code you added wont be exericised as it will enter the first if case
if (bridge_port_id == SAI_NULL_OBJECT_ID && entry->bv_id == SAI_NULL_OBJECT_ID)
  1. When we call SAI FDB flush API with either bvid or bridge_port_id or both valid, we would end up receiving SAI_FDB_EVENT_AGED. Hence. Again the new code you added here wont be exercised.

Sorry, I take back my comment on receiving SAI_FDB_EVENT_AGED instead of instead of SAI_FDB_EVENT_FLUSHED. My observation was on one platform. But it was NOT the same on VS container

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

update.entry.port_name = update.port.m_alias;

/* Remove all FDB entries of specified port by going through m_entries */
for (auto itr = m_entries.begin(); itr != m_entries.end();)
{
if (itr->port_name == update.port.m_alias)
{
update.entry.mac = itr->mac;
update.entry.bv_id = itr->bv_id;
update.add = false;
itr++;

storeFdbEntryState(update);

SWSS_LOG_DEBUG("FdbOrch notification: mac %s , bv_id 0x%" PRIx64 " on port ID 0x%" PRIx64 " was removed", update.entry.mac.to_string().c_str(), update.entry.bv_id, bridge_port_id);

for (auto observer: m_observers)
{
observer->update(SUBJECT_TYPE_FDB_CHANGE, &update);
}
continue;
}
itr++;
}
}
else if (bridge_port_id == SAI_NULL_OBJECT_ID && entry->bv_id != SAI_NULL_OBJECT_ID)
{
/*this is a placeholder for flush vlan fdb case, not supported yet.*/
SWSS_LOG_ERROR("FdbOrch notification: not supported flush vlan fdb action, port_id = 0x%" PRIx64 ", bv_id = 0x%" PRIx64 ".", bridge_port_id, entry->bv_id);
/* Remove all FDB entries of specified vlan by going through m_entries */
for (auto itr = m_entries.begin(); itr != m_entries.end();)
{
if (itr->bv_id == entry->bv_id)
{
update.entry.mac = itr->mac;
update.entry.bv_id = itr->bv_id;
update.add = false;
itr++;

storeFdbEntryState(update);

SWSS_LOG_DEBUG("FdbOrch notification: mac %s , bv_id 0x%" PRIx64 " was removed", update.entry.mac.to_string().c_str(), update.entry.bv_id);

for (auto observer: m_observers)
{
observer->update(SUBJECT_TYPE_FDB_CHANGE, &update);
}
continue;
}
itr++;
}
}
else
{
SWSS_LOG_ERROR("FdbOrch notification: not supported flush fdb action, port_id = 0x%" PRIx64 ", bv_id = 0x%" PRIx64 ".", bridge_port_id, entry->bv_id);
if (!m_portsOrch->getPortByBridgePortId(bridge_port_id, update.port))
{
SWSS_LOG_ERROR("Failed to get port by bridge port ID 0x%" PRIx64, bridge_port_id);
return;
}

update.entry.port_name = update.port.m_alias;

/* Remove all FDB entries of specified port and vlan by going through m_entries */
for (auto itr = m_entries.begin(); itr != m_entries.end();)
{
if ((itr->port_name == update.port.m_alias)
&& (itr->bv_id == entry->bv_id))
{
update.entry.mac = itr->mac;
update.entry.bv_id = itr->bv_id;
update.add = false;
itr++;

storeFdbEntryState(update);

SWSS_LOG_DEBUG("FdbOrch notification: mac %s , bv_id 0x%" PRIx64 " on port ID 0x%" PRIx64 " was removed", update.entry.mac.to_string().c_str(), update.entry.bv_id, bridge_port_id);

for (auto observer: m_observers)
{
observer->update(SUBJECT_TYPE_FDB_CHANGE, &update);
}
continue;
}
itr++;
}
}
break;
}
Expand Down Expand Up @@ -259,6 +337,40 @@ bool FdbOrch::getPort(const MacAddress& mac, uint16_t vlan, Port& port)
return true;
}

bool FdbOrch::ifChangeInformFdb(Port &port, bool if_up)
Copy link
Copy Markdown
Collaborator

@prsunny prsunny May 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the name looks better as flushFdbEntries

{
SWSS_LOG_ENTER();

sai_attribute_t attr;
vector<sai_attribute_t> attrs;

if (if_up)
{
SWSS_LOG_DEBUG("Port %s is goning up, and no need updating FDB for this port.", port.m_alias.c_str());
return true;
}

attr.id = SAI_FDB_FLUSH_ATTR_ENTRY_TYPE;
attr.value.s32 = SAI_FDB_ENTRY_TYPE_DYNAMIC;
attrs.push_back(attr);

attr.id = SAI_FDB_FLUSH_ATTR_BRIDGE_PORT_ID;
attr.value.oid = port.m_bridge_port_id;
attrs.push_back(attr);

sai_status_t status = sai_fdb_api->flush_fdb_entries(gSwitchId, (uint32_t)attrs.size(), attrs.data());
if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_ERROR("Failed to flush dynamic FDB on %s, rv:%d",
port.m_alias.c_str(), status);
return false;
}

SWSS_LOG_NOTICE("Success to flush dynamic FDB on %s", port.m_alias.c_str());

return true;
}

void FdbOrch::doTask(Consumer& consumer)
{
SWSS_LOG_ENTER();
Expand Down Expand Up @@ -310,6 +422,8 @@ void FdbOrch::doTask(Consumer& consumer)
/* FDB type is either dynamic or static */
assert(type == "dynamic" || type == "static");

entry.port_name = port;

if (addFdbEntry(entry, port, type))
it = consumer.m_toSync.erase(it);
else
Expand Down Expand Up @@ -429,7 +543,7 @@ void FdbOrch::updateVlanMember(const VlanMemberUpdate& update)
auto fdb_list = std::move(saved_fdb_entries[port_name]);
if(!fdb_list.empty())
{
for (const auto& fdb: fdb_list)
for (auto& fdb: fdb_list)
Copy link
Copy Markdown
Contributor

@vasant17 vasant17 Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can avoid making the mutable.

{
// try to insert an FDB entry. If the FDB entry is not ready to be inserted yet,
// it would be added back to the saved_fdb_entries structure by addFDBEntry()
Expand All @@ -438,7 +552,7 @@ void FdbOrch::updateVlanMember(const VlanMemberUpdate& update)
}
}

bool FdbOrch::addFdbEntry(const FdbEntry& entry, const string& port_name, const string& type)
bool FdbOrch::addFdbEntry(FdbEntry& entry, const string& port_name, const string& type)
Copy link
Copy Markdown
Contributor

@vasant17 vasant17 Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, I think, we shouldn't make entry mutable. Here is my suggestion:

Change prototype of addFdbEntry() by removing port from parameter list, as it's already part of FDB entry structure. As far as I remember addFdbEntry is called from only two places and you can populate entry.port_name in both places. In fact you don't have to do anything when it's called from updateVlanMember().

Also note that you have to make some changes in storeFdbStateEntry().
It is called from only update() method in three cases. LEARN, AGED/MOVE, FLUSH.

LEARN is the only case where storeFdbStateEntry() ends up adding the entry to m_entries/saved_fdb_entries. So, populate entry.port_name in LEARN case before calling storeFdbStateEntry().

And in the other two cases(AGED/MOVE, FLUSH), anyways we are going to remove the entry from m_entries, hence no need to populate entry.port_name. Also note that you can go ahead and cleanup(delete) few lines of port related code in storeFdbStateEntry().

{
SWSS_LOG_ENTER();

Expand Down Expand Up @@ -467,6 +581,12 @@ bool FdbOrch::addFdbEntry(const FdbEntry& entry, const string& port_name, const
return true;
}

/* Update entry's port name if needed */
if (entry.port_name.empty())
{
entry.port_name = port_name;
Copy link
Copy Markdown
Contributor

@vasant17 vasant17 May 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which case, the entry is created without port name?

Copy link
Copy Markdown
Contributor

@vasant17 vasant17 May 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We add an entry to m_entries in two places. One is storeFdbEntryState() and other is addFdbEntry(), Could you please make sure in both places we set port_name in entry before adding to m_entries?

Copy link
Copy Markdown
Author

@MikeHcChen MikeHcChen Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, this part of codes is now redundant in my PR.
I write this part of codes just for maximized backward compatibility when someone adds/tests their own codes which call addFdbEntry() with parameter of FdbEntry& entry in old fashion contents (only mac and bv_id).

}

sai_attribute_t attr;
vector<sai_attribute_t> attrs;

Expand Down
4 changes: 3 additions & 1 deletion orchagent/fdborch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ struct FdbEntry
{
MacAddress mac;
sai_object_id_t bv_id;
std::string port_name;
Copy link
Copy Markdown
Contributor

@vasant17 vasant17 May 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I liked this. This was much needed!


bool operator<(const FdbEntry& other) const
{
Expand Down Expand Up @@ -46,6 +47,7 @@ class FdbOrch: public Orch, public Subject, public Observer
void update(sai_fdb_event_t, const sai_fdb_entry_t *, sai_object_id_t);
void update(SubjectType type, void *cntx);
bool getPort(const MacAddress&, uint16_t, Port&);
bool ifChangeInformFdb(Port&, bool);

private:
PortsOrch *m_portsOrch;
Expand All @@ -60,7 +62,7 @@ class FdbOrch: public Orch, public Subject, public Observer
void doTask(NotificationConsumer& consumer);

void updateVlanMember(const VlanMemberUpdate&);
bool addFdbEntry(const FdbEntry&, const string&, const string&);
bool addFdbEntry(FdbEntry&, const string&, const string&);
bool removeFdbEntry(const FdbEntry&);

bool storeFdbEntryState(const FdbUpdate& update);
Expand Down
7 changes: 7 additions & 0 deletions orchagent/portsorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#include "intfsorch.h"
#include "bufferorch.h"
#include "neighorch.h"
#include "fdborch.h"

#include <inttypes.h>
#include <cassert>
Expand Down Expand Up @@ -39,6 +40,7 @@ extern IntfsOrch *gIntfsOrch;
extern NeighOrch *gNeighOrch;
extern CrmOrch *gCrmOrch;
extern BufferOrch *gBufferOrch;
extern FdbOrch *gFdbOrch;

#define VLAN_PREFIX "Vlan"
#define DEFAULT_VLAN_ID 1
Expand Down Expand Up @@ -3830,6 +3832,11 @@ void PortsOrch::updatePortOperStatus(Port &port, sai_port_oper_status_t status)
{
SWSS_LOG_WARN("Inform nexthop operation failed for interface %s", port.m_alias.c_str());
}

if (!gFdbOrch->ifChangeInformFdb(port, isUp))
Copy link
Copy Markdown
Contributor

@vasant17 vasant17 May 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we are just following the way ifChangeInformNextHop is done and introducing ifChangeInformFdb. And I was coding up the same solution yesterday and thought of doing it a different way. That is, today at most places if NOT all, we interact between modules using observer pattern. For example, when port gets created or deleted, portsOrch notifies all the observers. I think we should leverage that or follow the pattern to notify observers about port state change. That way we dont have introduce a new function for every module that wants to port status change. With my new code change, we should be able to delete ifChangeInformNextHop and let neighborOrch handle port status change notification. Please refer PR #1242. I will push the code in couple of hours

{
SWSS_LOG_WARN("Inform fdb operation failed for interface %s", port.m_alias.c_str());
}
}

/*
Expand Down