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

vivekrnv · 2022-05-04T08:39:09Z

What I did

Why I did it

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

How I verified it

Checked this config and verified the remove vlan passed though

1. config vlan add 40
2. config vlan member add 40 Ethernet0
On the other side. 
3. sudo ip link add link enp4s0f0 name enp4s0f0.40 type vlan id 40
4. sudo ip link set enp4s0f0.40 up
5. Make sure a fdb entry is learnt. 

root@sonic:/home/admin# fdbshow
  No.    Vlan  MacAddress         Port       Type
-----  ------  -----------------  ---------  -------
    1      40  7C:FE:90:12:22:EC  Ethernet0  Dynamic

dump state fdb all --db STATE_DB
{
    "Vlan40:7c:fe:90:12:22:ec": {
        "STATE_DB": {
            "keys": [
                {
                    "FDB_TABLE|Vlan40:7c:fe:90:12:22:ec": {
                        "port": "Ethernet0",
                        "type": "dynamic"
                    }
                }
            ],
            "tables_not_found": []
        }
    }
}

7. config vlan member del 40 Ethernet0
8. config vlan del 40
9. dump state fdb all --db STATE_DB
{}

Unit Tests:

vkarri@4897dcba242c:/sonic/src/sonic-swss/tests/mock_tests$ ./tests --gtest_filter=FdbOrchTest*
Running main() from /usr/src/gtest/src/gtest_main.cc
Note: Google Test filter = FdbOrchTest*
[==========] Running 6 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 6 tests from FdbOrchTest
[ RUN      ] FdbOrchTest.ConsolidatedFlushVlanandPort
[       OK ] FdbOrchTest.ConsolidatedFlushVlanandPort (14 ms)
[ RUN      ] FdbOrchTest.ConsolidatedFlushAll
[       OK ] FdbOrchTest.ConsolidatedFlushAll (15 ms)
[ RUN      ] FdbOrchTest.ConsolidatedFlushVlan
[       OK ] FdbOrchTest.ConsolidatedFlushVlan (14 ms)
[ RUN      ] FdbOrchTest.ConsolidatedFlushPort
[       OK ] FdbOrchTest.ConsolidatedFlushPort (15 ms)
[ RUN      ] FdbOrchTest.ConsolidatedFlushVlanandPortBridgeportDeleted
[       OK ] FdbOrchTest.ConsolidatedFlushVlanandPortBridgeportDeleted (14 ms)
[ RUN      ] FdbOrchTest.NonConsolidatedFlushVlanandPort
[       OK ] FdbOrchTest.NonConsolidatedFlushVlanandPort (15 ms)
[----------] 6 tests from FdbOrchTest (87 ms total)

[----------] Global test environment tear-down
[==========] 6 tests from 1 test case ran. (87 ms total)
[  PASSED  ] 6 tests.

Details if related

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

…ic-swss into fdb_remove_vlan

liat-grozovik · 2022-05-04T14:04:14Z

@vivekreddynv on which branch this fix should go? master only?

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

…ic-swss into fdb_remove_vlan

vivekrnv · 2022-05-07T02:52:42Z

/azpw run Azure.sonic-swss

mssonicbld · 2022-05-07T02:52:43Z

/AzurePipelines run Azure.sonic-swss

azure-pipelines · 2022-05-07T02:52:52Z

Azure Pipelines successfully started running 1 pipeline(s).

vivekrnv · 2022-05-10T00:20:19Z

/azpw run Azure.sonic-swss

mssonicbld · 2022-05-10T00:20:21Z

/AzurePipelines run Azure.sonic-swss

azure-pipelines · 2022-05-10T00:20:30Z

Azure Pipelines successfully started running 1 pipeline(s).

prsunny · 2022-05-16T17:26:20Z

orchagent/fdborch.cpp

+
+/*
+Handles the SAI_FDB_EVENT_FLUSHED notification recieved from syncd
+Ref: https://github.com/opencomputeproject/SAI/blob/master/inc/saifdb.h#L223


Pleaser remove reference to code. This will not be useful when the header file gets changed.

prsunny · 2022-05-16T17:30:24Z

orchagent/fdborch.cpp

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


This can be a NOTICE since the flow is in notification. The sonic-mgmt tests is now strict on what scenarios the ERR logs are being generated. There are currently some cases in fdb notif flow which is generating ERR which we are planning to revisit.

prsunny · 2022-05-16T17:33:04Z

orchagent/fdborch.cpp

@@ -493,86 +594,21 @@ void FdbOrch::update(sai_fdb_event_t        type,
    }
    case SAI_FDB_EVENT_FLUSHED:

-        SWSS_LOG_INFO("FDB Flush event received: [ %s , 0x%" PRIx64 " ], \
-                       bridge port ID: 0x%" PRIx64 ".",
+        SWSS_LOG_NOTICE("FDB Flush event received: [ %s , 0x%" PRIx64 " ], \


Please change it to INFO.

prsunny · 2022-05-16T17:35:18Z

orchagent/fdborch.cpp

+                                    const string& fdb_type,
+                                    const MacAddress& mac)
+{
+    MacAddress flush_mac(CONSOLIDATED_FLUSH_MAC);


Default assignment should be 0. I don't think we should define a new variable. You can give a comment that flush event shall have zero mac

prsunny · 2022-05-16T17:39:43Z

orchagent/fdborch.cpp

@@ -973,17 +1009,22 @@ void FdbOrch::doTask(NotificationConsumer& consumer)
        for (uint32_t i = 0; i < count; ++i)
        {
            sai_object_id_t oid = SAI_NULL_OBJECT_ID;
+            sai_int32_t fdb_entry_type = -1;


Suggest assign to 0.

prsunny · 2022-05-16T18:06:54Z

orchagent/fdborch.cpp

+        for (auto itr = m_entries.begin(); itr != m_entries.end();)
+        {
+            auto curr = itr++;
+            if (curr->second.type.find(fdb_type) != std::string::npos)


I think this check is redundant. afaik, the fdb_type is always populated.

These entries might be static as well, so i think it's required to check the fdb_type before clearing them

so you are not checking for static vs dynamic here. my point is, one or other is always true and never be npos, right?

"find" returns the index of first occurence, so this logic holds true, npos indicates a mismatch. I've used specifically thus because the type stored in internal cache can be of static, dynamic or dynamic_local. whereas the type received from the flush will either be static or dynamic.

Thus i've used this instead of a simple equality check.

May be i'm missing something. what do we miss with the following?

if (curr->second.type.find(fdb_type) != std::string::npos) { if (curr->first.mac == mac || mac == flush_mac) { clearFdbEntry(curr->first.mac, curr->first.bv_id, fdb_type, curr->first.port_name); } }

to

if (curr->first.mac == mac || mac == flush_mac) { clearFdbEntry(curr->first.mac, curr->first.bv_id, fdb_type, curr->first.port_name); }

Got it. In the flush, can user specify fdb_type? If not, is it correct that flush notification would be for all types?

Link User can specify the flush type. Looks like there can also be a SAI_FDB_FLUSH_ENTRY_TYPE_ALL along with SAI_FDB_FLUSH_ENTRY_TYPE_STATIC/SAI_FDB_FLUSH_ENTRY_TYPE_DYNAMIC

I'll update to handle the SAI_FDB_FLUSH_ENTRY_TYPE_ALL case as well.

Sry, the flush notif will have either STATIC or DYNAMIC, so "all" case need not be handled.

EDIT: Updated, remove reading the SAI_FDB_ENTRY_ATTR_TYPE attribute and cleared only non-static entries

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

vivekrnv · 2022-05-17T18:13:47Z

/azpw run Azure.sonic-swss

mssonicbld · 2022-05-17T18:13:49Z

/AzurePipelines run Azure.sonic-swss

azure-pipelines · 2022-05-17T18:13:59Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

…ic-swss into fdb_remove_vlan

judyjoseph · 2022-05-25T23:24:11Z

@vivekreddynv Please raise a separate PR for 202111 as there are conflicts

…#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>

…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>

…onic-net#2401) * [FDB] Fix fbdorch to properly handle syncd FDB FLUSH Notif (sonic-net#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>

vivekrnv and others added 15 commits April 19, 2022 21:30

temp fix for remove vlanop to work

34899a9

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

Moved to original place

d5bc119

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

Add back the retry mechanism based on fdb_count

3bee64f

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

Compilation suceeded

9da8456

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

minor comments

197b82d

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

UT skeleton Added

1ea475c

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

Testcase not crashing

da282ff

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

First test passed

d04d3cd

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

Unit tests finished

d9b9c35

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

minor fixes

1e92c07

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

handled bridge_port_id deleted case

548e4de

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

Update fdborch.cpp

2b511da

Update fdborch.h

8a2adf0

comments addressed

77819f6

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

Merge branch 'fdb_remove_vlan' of https://github.com/vivekreddynv/son…

c944d9a

…ic-swss into fdb_remove_vlan

vivekrnv requested a review from prsunny as a code owner May 4, 2022 08:39

Merge branch 'Azure:master' into fdb_remove_vlan

f5d2aa6

dgsudharsan previously approved these changes May 4, 2022

View reviewed changes

dgsudharsan added the Request for 202111 Branch label May 4, 2022

vivekrnv and others added 4 commits May 4, 2022 13:59

Merge branch 'Azure:master' into fdb_remove_vlan

5f3f0fe

Merge branch 'Azure:master' into fdb_remove_vlan

b3ae5f4

Missing condition added

77d9bb9

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

Merge branch 'fdb_remove_vlan' of https://github.com/vivekreddynv/son…

1e20eee

…ic-swss into fdb_remove_vlan

vivekrnv dismissed dgsudharsan’s stale review via 1e20eee May 6, 2022 08:12

dgsudharsan previously approved these changes May 7, 2022

View reviewed changes

prsunny reviewed May 16, 2022

View reviewed changes

Handled Comments

f6feafc

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

vivekrnv dismissed dgsudharsan’s stale review via f6feafc May 17, 2022 03:01

vivekrnv and others added 2 commits May 16, 2022 20:07

Merge branch 'master' into fdb_remove_vlan

fd70c78

Minor comment

fa68eb4

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

Merge branch 'Azure:master' into fdb_remove_vlan

cd2e4fa

dgsudharsan previously approved these changes May 17, 2022

View reviewed changes

vivekrnv added 2 commits May 18, 2022 01:01

Only flush non-static entries

e1669d4

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

Merge branch 'fdb_remove_vlan' of https://github.com/vivekreddynv/son…

e32df7c

…ic-swss into fdb_remove_vlan

vivekrnv dismissed dgsudharsan’s stale review via e32df7c May 18, 2022 01:01

vivekrnv requested review from prsunny and dgsudharsan May 18, 2022 16:42

prsunny approved these changes May 18, 2022

View reviewed changes

prsunny merged commit bbbd5f4 into sonic-net:master May 18, 2022

dprital mentioned this pull request May 25, 2022

[submodule] Advanved sonic-swss pointer sonic-net/sonic-buildimage#10926

Merged

6 tasks

judyjoseph removed the Request for 202111 Branch label May 25, 2022

vivekrnv mentioned this pull request May 26, 2022

[202111] [FDB] Fix fbdorch to properly handle syncd FDB FLUSH Notif #2291

Merged

volodymyrsamotiy mentioned this pull request Aug 1, 2022

[FDB] [202012] Fix fbdorch to properly handle syncd FDB FLUSH Notif #2401

Merged

prasanna-cls mentioned this pull request Jan 18, 2023

When Bridge port is deleted treat L2 call back event AGED as FLUSH #2623

Open

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vivekrnv commented May 4, 2022

liat-grozovik commented May 4, 2022

vivekrnv commented May 7, 2022

mssonicbld commented May 7, 2022

azure-pipelines bot commented May 7, 2022

vivekrnv commented May 10, 2022

mssonicbld commented May 10, 2022

azure-pipelines bot commented May 10, 2022

prsunny May 16, 2022

vivekrnv May 17, 2022

prsunny May 16, 2022

vivekrnv May 17, 2022

prsunny May 16, 2022

vivekrnv May 17, 2022

prsunny May 16, 2022

vivekrnv May 17, 2022

prsunny May 16, 2022

prsunny May 16, 2022

vivekrnv May 17, 2022

prsunny May 17, 2022

vivekrnv May 17, 2022

prsunny May 17, 2022

prsunny May 17, 2022

vivekrnv May 17, 2022

vivekrnv May 17, 2022

vivekrnv May 17, 2022

vivekrnv May 18, 2022

vivekrnv commented May 17, 2022

mssonicbld commented May 17, 2022

azure-pipelines bot commented May 17, 2022

judyjoseph commented May 25, 2022

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

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

Conversation

vivekrnv commented May 4, 2022

liat-grozovik commented May 4, 2022

vivekrnv commented May 7, 2022

mssonicbld commented May 7, 2022

azure-pipelines bot commented May 7, 2022

vivekrnv commented May 10, 2022

mssonicbld commented May 10, 2022

azure-pipelines bot commented May 10, 2022

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

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

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

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

vivekrnv commented May 17, 2022

mssonicbld commented May 17, 2022

azure-pipelines bot commented May 17, 2022

judyjoseph commented May 25, 2022