Skip to content
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

Jump to bottom

Layer 2 Forwarding Enhancements #529

Closed
wants to merge 59 commits into from
Closed

Layer 2 Forwarding Enhancements #529

wants to merge 59 commits into from

Conversation

anilkpandey
Copy link
Contributor

@anilkpandey anilkpandey commented May 10, 2019 •
edited
Loading

- What I did

  1. Added code changes related to Layer 2 Forwarding Enhancements:
    Added the following commands:

VLAN range add/del
VLAN member range add/del
Static FDB add/del
FDB aging time configuration
show fdb aging time
show mac count
show vlan count
added port and vlan option in 'sonic-clear fdb'

anilkpandey added 4 commits May 9, 2019 16:22
@anilkpandey
Update main.py
eee827e
@anilkpandey
Update main.py
d4e5346
@anilkpandey
Update main.py
b63e795
@anilkpandey
added configuration validations
a4beefc 
- validate vlan id for vlan creation
- verify that port is not part of vlan when assigning IP address and vice versa
- verify that portchannel has no member and portchannel is not member of vlan when deleting the portchannel
- verify that port is not already untagged member of vlan when adding the port as untagged member of a vlan
-
@prsunny
Copy link
Contributor

prsunny commented May 10, 2019

I'm not sure if these validations must be here. There is an design discussion for overall Config validator and I don't think we need to scatter these checks in multiple places.

@jleveque jleveque requested review from stcheng and prsunny May 31, 2019 19:53
@anilkpandey
'show mac' performance improvement
1564302 
'show mac' was taking a lot of time to display the fdb entries when a large number of entries are present.
Using redis pipeline, reduced the time significantly.
Earlier it used to take about 10 minutes to show 32K MAC. After the fix it takes about 17-20 seconds.
@anilkpandey
revert changes
f567fbb 
revert changes
@anilkpandey anilkpandey changed the title added validation checks for VLAN, IP and Portchannel configurations 'show mac' performance improvements Jun 5, 2019
@anilkpandey
Copy link
Contributor Author

Hi @stcheng and @prsunny,

I just reverted the previous changes for validations and added a new file fdbshow for 'show mac' performance enhancements. Please review the changes.

In case you want to consider the validations changes, let me know, I will readd the changes in that case.

Thanks.

stcheng
stcheng suggested changes Jun 5, 2019
View reviewed changes
Copy link
Contributor

@stcheng stcheng left a comment

Choose a reason for hiding this comment

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

could you resolve the conflicts? and also remove the .DS_Store file in the pull request?

stcheng
stcheng reviewed Jun 6, 2019
View reviewed changes
config/main.py Outdated Show resolved Hide resolved
@anilkpandey
revert validation related changes
d544203 
reverted changes made as part of validations
prsunny
prsunny reviewed Jun 11, 2019
View reviewed changes
scripts/fdbshow Outdated Show resolved Hide resolved
scripts/fdbshow Outdated Show resolved Hide resolved
scripts/fdbshow Outdated Show resolved Hide resolved
@prsunny prsunny mentioned this pull request Jun 13, 2019
Open
@prsunny
Copy link
Contributor

prsunny commented Jul 9, 2019

retest this please

prsunny
prsunny previously approved these changes Jul 9, 2019
View reviewed changes
stcheng
stcheng previously approved these changes Jul 10, 2019
View reviewed changes
@jleveque
Copy link
Contributor

Retest this please

1 similar comment
@jleveque
Copy link
Contributor

Retest this please

@anilkpandey anilkpandey changed the title 'show mac' performance improvements Layer 2 Forwarding Enhancements Sep 8, 2019
@anilkpandey
Copy link
Contributor Author

Added code changes related to Layer 2 Forwarding Enhancements

@prsunny prsunny self-requested a review September 9, 2019 19:13
@lgtm-com
Copy link

lgtm-com bot commented Dec 9, 2020

This pull request introduces 1 alert when merging 2a9ab88 into d414970 - view on LGTM.com

new alerts:

  • 1 for Unnecessary pass

@anilkpandey
added more tests for vlan range and fdb config commands
a8c651e 
added more tests for vlan range and fdb config commands
@lgtm-com
Copy link

lgtm-com bot commented Dec 18, 2020

This pull request introduces 2 alerts when merging a8c651e into 9419627 - view on LGTM.com

new alerts:

  • 1 for Unnecessary pass
  • 1 for Unused local variable

@anilkpan
Copy link
Contributor

retest this please

1 similar comment
@anilkpan
Copy link
Contributor

retest this please

lguohan
lguohan reviewed May 12, 2021
View reviewed changes
scripts/fdbshow Outdated
@@ -31,8 +31,8 @@ import json
import sys

from natsort import natsorted
from swsssdk import port_util
from swsscommon.swsscommon import SonicV2Connector
from swsssdk import SonicV2Connector, port_util
Copy link
Contributor

Choose a reason for hiding this comment

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

we should not use swsssdk, it is deprecating.

@anilkpandey anilkpandey mentioned this pull request Oct 22, 2021
Merged
@lgtm-com
Copy link

lgtm-com bot commented Nov 3, 2021

This pull request introduces 2 alerts when merging 2fcd83d into 474bdbf - view on LGTM.com

new alerts:

  • 1 for Unused local variable
  • 1 for Wrong number of arguments in a call

@lgtm-com
Copy link

lgtm-com bot commented Nov 9, 2021

This pull request introduces 2 alerts when merging b037b7d into 8a8577b - view on LGTM.com

new alerts:

  • 1 for Unreachable code
  • 1 for Wrong number of arguments in a call

@anilkpandey
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 529 in repo Azure/sonic-utilities

@anilkpandey
Copy link
Contributor Author

retest this please

@anilkpandey
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@anilkpandey
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@raphaelt-nvidia raphaelt-nvidia mentioned this pull request Jan 20, 2022
Open
@ljyfree
Copy link

ljyfree commented Apr 12, 2022

Hi all
I notice that "A Static FDB entry should be configurable in hardware from CLI" was mentioned in SONiC Layer 2 Forwarding Enhancements HLD.md.
Wonder why this pull was not be merged which contain SONiC-CLI to config static FDB? Without this pull,how could I add a static FDB from CLI?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tahmed-dev tahmed-dev tahmed-dev left review comments

@jainp1979 jainp1979 jainp1979 left review comments

@prsunny prsunny prsunny left review comments

@lguohan lguohan lguohan left review comments

@stcheng stcheng stcheng left review comments

Assignees
No one assigned
Labels
brcm-upstream Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.