-
Notifications
You must be signed in to change notification settings - Fork 650
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.
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
Display large number of FDB entries fast from state_db and appl_db #968
Conversation
retest this please |
@anilkpandey , can you take a look? There was a performance optimization done as part of #529, and it is a different approach. |
I see the improvement from 10 minutes to 17-20 seconds in Layer 2 Forwarding Enhancements. What I do is to show 8K mac in 2 seconds. |
retest this please |
I have test again. |
ok, @qiluo-msft , can you take a look as well? improvement looks good to me |
scripts/fdbshow
Outdated
@@ -93,7 +93,24 @@ class FdbShow(object): | |||
self.bridge_mac_list.sort(key = lambda x: x[0]) | |||
return | |||
|
|||
|
|||
def fetch_state_fdb_data(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fetch_state_fdb_data [](start = 8, length = 20)
fetch_state_fdb_data [](start = 8, length = 20)
@kcudnik Please help check.
The StateDB FDB is learned from syncd notification. So is it the same as AsicDB, for example, how about static fdb entries programmed? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, asic db learns those from fdb notification, static entries are created by user explicitly, so those comes from OA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, It does not process static fdb tables. It is a problem. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you cannot find static FDB entries in StateDB, it will be inside AppDB. FYI if you want to further explore.
In reply to: 450189315 [](ancestors = 450189315)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, thats a good reason to have an option to display from state_db based on need. By default, and for small number of entries, it can use the existing option. But for large number, user can specify to display from state_db
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a good idea. I will add the option for the command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have add option "-f" to for displaying large number of FDB entries fastly from state_db and appl_db.
The static FDB entries are also displayed from appl_db in fast option.
root@sonic:/home/admin# fdbshow -h
usage: fdbshow [-h] [-p PORT] [-v VLAN] [-f]
Display ASIC FDB entries
optional arguments:
-h, --help show this help message and exit
-p PORT, --port PORT FDB learned on specific port: Ethernet0
-v VLAN, --vlan VLAN FDB learned on specific Vlan: 1001
-f, --fast Display large number of FDB entries fastly
root@sonic:/home/admin#
root@sonic:/home/admin# fdbshow -f
No. Vlan MacAddress Port Type
1 100 00:00:00:00:00:01 Ethernet1 dynamic
2 100 11:00:00:00:00:01 Ethernet1 static
3 100 11:00:00:00:00:02 Ethernet1 static
4 100 11:00:00:00:00:03 Ethernet1 static
Total number of entries 4
scripts/fdbshow
Outdated
|
||
def __init__(self): | ||
def __init__(self,fast): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
, [](start = 21, length = 1)
, [](start = 21, length = 1)
Add one space after ,
#Closed
scripts/fdbshow
Outdated
super(FdbShow,self).__init__() | ||
self.db = SonicV2Connector(host="127.0.0.1") | ||
self.if_name_map, \ | ||
self.if_oid_map = port_util.get_interface_oid_map(self.db) | ||
self.if_br_oid_map = port_util.get_bridge_port_map(self.db) | ||
self.fetch_fdb_data() | ||
self.fast=fast |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
= [](start = 17, length = 1)
= [](start = 17, length = 1)
Add one blank before and after =
#Closed
scripts/fdbshow
Outdated
super(FdbShow,self).__init__() | ||
self.db = SonicV2Connector(host="127.0.0.1") | ||
self.if_name_map, \ | ||
self.if_oid_map = port_util.get_interface_oid_map(self.db) | ||
self.if_br_oid_map = port_util.get_bridge_port_map(self.db) | ||
self.fetch_fdb_data() | ||
self.fast=fast | ||
if (self.fast is True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (self.fast is True): | |
if self.fast: | |
``` #Closed |
scripts/fdbshow
Outdated
@@ -139,10 +175,11 @@ def main(): | |||
formatter_class=argparse.RawTextHelpFormatter) | |||
parser.add_argument('-p', '--port', type=str, help='FDB learned on specific port: Ethernet0', default=None) | |||
parser.add_argument('-v', '--vlan', type=str, help='FDB learned on specific Vlan: 1001', default=None) | |||
parser.add_argument('-f', '--fast', action='store_true', help='Display large number of FDB entries fastly') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fastly [](start = 103, length = 6)
fastly [](start = 103, length = 6)
fast #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I have changed codes as reviewed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As comments
Hi, Do you know what's wrong with the check result "default — Build finished. No test results found." ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please keep old commits in future if you iterate in one PR. We have some internal tools to compare between commits. If you force push, the tool will confuse and lose the context of existing comments. Thanks!
Retest this please |
retest this please |
Hi qiluo, |
retest this please |
This artifacts are wrong with old code, we already fixed in sonic-net/sonic-buildimage@d4fc8e5 |
@JiangboHe will this PR will be further handled and taken to 201911? |
retest this please |
|
I am confused by the default failure after "retest this please". I have tried several times. Do you known what caused it? |
@JiangboHe , is this fix already in master? Just wondering why this is raised to 201911? |
|
ok, in that case, lets get the master PR first and cherry-pick to 201911. We can use this PR, if the cherry-pick has conflicts. For now, you can close this. |
Close and favor the PR to master branch. |
- What I did
I change the mothod by showing mac table from state FDB_TABLE to display large number of fdb tables faster.
The time cost to display change from 50 seconds to 2 seconds for 8000 fdb tables or 20 seconds to 1 seconds for 5000 fdb tables.
- How I did it
The key search and judge of fdb tables in asic table is too complex. It will cost 20 seconds for 5000 fdb tables or 50 seconds for 8000 fdb tables. It is not good for users or script running.
- How to verify it
Learn thounds of fdb tables and show them with fdbshow
- Previous command output (if the output of a command-line utility has changed)
- New command output (if the output of a command-line utility has changed)