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

MikeHcChen · 2020-05-15T02:13:23Z

What I did

Add a method to inform fdborch to flush FDB while port operation status changed.
Implement codes for processing events of the flushed FDB on specified port or vlan notifications of SAI

Why I did it
Fix issue: [FDB] Doesn't flush fdb entries on port link down #1294

How I verified it
Please refer test steps in [FDB] Doesn't flush fdb entries on port link down #1294

Test on broadcom platform 'x86_64-delta_ag9032v1-r0 with SONiC Software Version: 201911 branch commit 2218ed4 on May 1, 2020.

Details if related

Add a method to inform fdborch to flush FDB while port operation status changed. Implement processing events of the flushed FDB on specified port or vlan notifications of SAI

msftclas · 2020-05-15T02:13:36Z

All CLA requirements met.

prsunny · 2020-05-18T18:55:48Z

There is a similar PR that handles this use-case differently - #909. It could be incomplete based on the changes you provided in this PR. One question is when you do "flush" do you also get FDB delete notifications back from SAI for each of the entry that's removed?

MikeHcChen · 2020-05-20T03:45:20Z

There is a similar PR that handles this use-case differently - #909. It could be incomplete based on the changes you provided in this PR. One question is when you do "flush" do you also get FDB delete notifications back from SAI for each of the entry that's removed?

Thanks for your comment.
This PR is focused on flushing FDB after normal port's removing or shutdown, and it is not complicated as PR#909 which contains LAG concerns.

If we do 'flush'on a port, the ASIC will finally notify SONiC with sai_fdb_event_t
event type 'SAI_FDB_EVENT_AGED' or 'SAI_FDB_EVENT_FLUSHED' depending on chip venders' SAI implementation .
The flow chart will be like: https://github.com/Azure/SONiC/blob/master/doc/layer2-forwarding-enhancements/images/portDownFlush.jpg

For case SAI_FDB_EVENT_AGED, original (FdbOrch::update() in fdborch.cpp) codes can handle well.
For case SAI_FDB_EVENT_FLUSHED, I added codes to handle
the notification of flushed a port in case SAI_FDB_EVENT_FLUSHED of FdbOrch::update() in fdborch.cpp.

prsunny · 2020-05-28T18:34:40Z

orchagent/fdborch.cpp

@@ -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)


I think the name looks better as flushFdbEntries

prsunny · 2020-05-28T18:35:44Z

Looks like there is also another PR that handles this - https://github.com/Azure/sonic-swss/pull/1242/files. @vasant17 , can you review this PR?

vasant17 · 2020-05-28T20:00:46Z

orchagent/fdborch.cpp

@@ -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))


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

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)

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.

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

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)

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

vasant17 · 2020-05-28T20:09:15Z

orchagent/portsorch.cpp

@@ -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))


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

vasant17 · 2020-05-29T05:00:15Z

orchagent/fdborch.h

@@ -9,6 +9,7 @@ struct FdbEntry
 {
    MacAddress mac;
    sai_object_id_t bv_id;
+    std::string port_name;


I liked this. This was much needed!

vasant17 · 2020-05-29T06:56:07Z

orchagent/fdborch.cpp

+    /* Update entry's port name if needed */
+    if (entry.port_name.empty())
+    {
+        entry.port_name = port_name;


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

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?

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).

vasant17 · 2020-06-01T05:15:07Z

orchagent/fdborch.cpp

@@ -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)


I think we can avoid making the mutable.

vasant17 · 2020-06-01T05:27:42Z

orchagent/fdborch.cpp

@@ -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)


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().

prsunny · 2020-07-08T16:13:06Z

@MikeHcChen , PR #1242 is merged which handles the same functionality. Can you close this PR?

MikeHcChen · 2020-07-09T01:10:30Z

@MikeHcChen , PR #1242 is merged which handles the same functionality. Can you close this PR?

OK! Thanks.

…#1295) * Fix fast-reboot when NDP present The IPv6 address delimiter ':' caused an exception Signed-off-by: Shlomi Bitton <shlomibi@nvidia.com> * Add a comment

[fdborch] Flush fdb entries on port link down (sonic-net#1294)

5662e1c

Add a method to inform fdborch to flush FDB while port operation status changed. Implement processing events of the flushed FDB on specified port or vlan notifications of SAI

MikeHcChen changed the title ~~[fdborch]: Flush fdb entries on port link down (#1294)~~ [fdborch]: Flush fdb entries on port link down (Fix issue #1294) May 15, 2020

MikeHcChen changed the title ~~[fdborch]: Flush fdb entries on port link down (Fix issue #1294)~~ [fdborch]: Flush fdb entries on port link down (Fixes #1294) May 15, 2020

MikeHcChen changed the title ~~[fdborch]: Flush fdb entries on port link down (Fixes #1294)~~ [fdborch]: Flush fdb entries on port link down (Resolves #1294, resolves Azure/sonic-buildimage#4184) May 15, 2020

MikeHcChen mentioned this pull request May 28, 2020

[fdb]fdborch: fdb entries are cleared according to vlan or port or vlan&&port #1063

Open

MikeHcChen changed the title ~~[fdborch]: Flush fdb entries on port link down (Resolves #1294, resolves Azure/sonic-buildimage#4184)~~ [fdborch]: Flush fdb entries on port link down (Resolves #1063, #1294, resolves Azure/sonic-buildimage#4184) May 28, 2020

prsunny reviewed May 28, 2020

View reviewed changes

vasant17 reviewed May 28, 2020

View reviewed changes

vasant17 reviewed May 29, 2020

View reviewed changes

vasant17 reviewed Jun 1, 2020

View reviewed changes

MikeHcChen closed this Jul 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

MikeHcChen commented May 15, 2020 •

edited

Loading

msftclas commented May 15, 2020 •

edited

Loading

prsunny commented May 18, 2020

MikeHcChen commented May 20, 2020 •

edited

Loading

prsunny May 28, 2020

prsunny commented May 28, 2020

vasant17 May 28, 2020

vasant17 May 29, 2020

vasant17 May 28, 2020

vasant17 May 29, 2020

vasant17 May 29, 2020 •

edited

Loading

vasant17 May 29, 2020

MikeHcChen Jun 1, 2020

vasant17 Jun 1, 2020

vasant17 Jun 1, 2020

prsunny commented Jul 8, 2020 •

edited

Loading

MikeHcChen commented Jul 9, 2020

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

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

Conversation

MikeHcChen commented May 15, 2020 • edited Loading

msftclas commented May 15, 2020 • edited Loading

prsunny commented May 18, 2020

MikeHcChen commented May 20, 2020 • edited Loading

Choose a reason for hiding this comment

prsunny commented May 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasant17 May 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prsunny commented Jul 8, 2020 • edited Loading

MikeHcChen commented Jul 9, 2020

MikeHcChen commented May 15, 2020 •

edited

Loading

msftclas commented May 15, 2020 •

edited

Loading

MikeHcChen commented May 20, 2020 •

edited

Loading

vasant17 May 29, 2020 •

edited

Loading

prsunny commented Jul 8, 2020 •

edited

Loading