[syncd] Add support for a clear fdb cli by keboliu · Pull Request #279 · sonic-net/sonic-sairedis

keboliu · 2018-01-12T11:44:35Z

To implement a clear fdb cli, added code in syncd.cpp and syncd_notifications.cpp.

for syncd_notifications.cpp
- revise funtion redisPutFdbEntryToAsicView to flush the fdb entries in ASIC_DB.

design doc see https://github.com/Azure/SONiC/wiki/SONiC-Clear-FDB-CLI-Design

merge from Azure/sonic-sairedis master

kcudnik · 2018-01-12T19:39:50Z

syncd/syncd.cpp

    return false;
 }

+bool getSwitchRid(sai_object_id_t* switch_rid)


switch rid can be obtained in much easier way: global object "switches" contains all the switches where key is VID of the switch, take a look syncd.cpp at line 50, usually there will be 1 or 0 switches

this is not needed

optimized as suggested.

kcudnik · 2018-01-12T19:41:03Z

syncd/syncd.cpp


    std::shared_ptr<swss::ConsumerTable> asicState = std::make_shared<swss::ConsumerTable>(dbAsic.get(), ASIC_STATE_TABLE);
    std::shared_ptr<swss::NotificationConsumer> restartQuery = std::make_shared<swss::NotificationConsumer>(dbAsic.get(), "RESTARTQUERY");
+    std::shared_ptr<swss::NotificationConsumer> flushfdbRequest = std::make_shared<swss::NotificationConsumer>(dbAsic.get(), "FLUSHFDBREQUEST");


flush fdb request should be initiated via SAI api, and IMO it should be synchronous call similar to GET, and called as GET api, no need for another handler

When started the design my idea is that since "clear fdb" is a one-shot action, not like a normal configuration, should not change configurations in the DB, so implement it via publish/subscribe mechanism is a quite straightforward way. Make it synchronous is a good suggestion, I am thinking next phase when adding "flush port" and "flush vlan" support, also make cli utilities to wait for a response from syncd or somewhere.

I agree with @kcudnik , the request should be initiated via SAI API. Besides, the sonic-clear command should invoke APP DB and let orchagent to issue corresponding SAI API.

@keboliu, you probably did not understand "synchronous call similiar to GET api". You probably need to look at how GET api are implemented in syncd and sairedis.

@lguohan yes, can you give some example for GET api if you have now? And for your previous comments "regarding invoke APP DB and let orchagent to call SAI API", do you mean we need to add some new configuration in the APP DB and trigger orchagent with manipulating this new configuration?

kcudnik · 2018-01-12T19:42:20Z

syncd/syncd_notifications.cpp

+        sai_vlan_id_t vlan_id = fdb->fdb_entry.vlan_id;
+        sai_object_id_t port_oid = 0;
+        int port_oid_found = 0;
+        int attr_num = fdb->attr_count;


extra variable not needed, use direct attr_count in line 109

optimized as suggested.

kcudnik · 2018-01-12T19:42:37Z

syncd/syncd_notifications.cpp

+    {
+        sai_vlan_id_t vlan_id = fdb->fdb_entry.vlan_id;
+        sai_object_id_t port_oid = 0;
+        int port_oid_found = 0;


changed to bool.

kcudnik · 2018-01-12T19:50:14Z

syncd/syncd_notifications.cpp

+                ]
+            }]
+            */
+            SWSS_LOG_NOTICE("received a flush all fdb event");


this behaviour is not described in SAI headers that when no port and no vlan id then 1 notification is send to notify that all have been flushed from the device, please put that description (and consult with SAI community?) whether that behavior is good
for example,
-all entries were flushed if "mac == all 0, vlanid == 0, bridge_id == 0"

this is fine by me, but please update SAI headers

I will talk to SAI guys see whether they already have something similar if not may ask them to add.

kcudnik · 2018-01-12T19:50:48Z

syncd/syncd_notifications.cpp

+        else
+        {
+            SWSS_LOG_ERROR("received a unknown flush fdb event, unsupported");
+        }


those 2 erros can be combined in 1 error message and print port_oid and vlan_id

kcudnik · 2018-01-23T20:13:52Z

take a look at #288 i added support for sairedis to clear fdb entries, which request can come from orchagent directly

prsunny · 2018-01-25T21:35:35Z

syncd/syncd_notifications.cpp

+            if(fdb->attr[i].id == SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID)
+            {
+                port_oid = fdb->attr[i].value.oid;
+                port_oid_found = true;


Just a note, you can break from here once port_oid_found is set to true

* add code to support clear fdb cli * remove redundant code and revise the error log * remove the fdb flush trigger from syncd

keboliu added 2 commits January 12, 2018 11:44

Merge pull request #1 from Azure/master

2ece9d4

merge from Azure/sonic-sairedis master

add code to support clear fdb cli

10d4011

keboliu changed the title ~~Add support for a clear fdb cli~~ [syncd] Add support for a clear fdb cli Jan 12, 2018

kcudnik reviewed Jan 12, 2018

View reviewed changes

remove redundant code and revise the error log

b50306c

remove the fdb flush trigger from syncd

4609487

kcudnik approved these changes Jan 24, 2018

View reviewed changes

prsunny approved these changes Jan 25, 2018

View reviewed changes

lguohan merged commit a6b5be8 into sonic-net:master Jan 26, 2018

keboliu deleted the clear-fdb-cli branch January 27, 2018 05:12

keboliu mentioned this pull request Jan 30, 2018

update the pointers to pick up clear fdb cli support sonic-net/sonic-buildimage#1354

Merged

Conversation

keboliu commented Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lguohan Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kcudnik commented Jan 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

keboliu commented Jan 12, 2018 •

edited

Loading

lguohan Jan 16, 2018 •

edited

Loading