-
Notifications
You must be signed in to change notification settings - Fork 480
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
Support for static FDB Entries to allow MAC Move #1024
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Can you elaborate on the use-case where we want to allow MAC_MOVE of a static MAC address?
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 discussed in the SAI community meeting on Dec 19 the use case is for EVPN wherein the MACs donot age out as they are created and deleted by the EVPN control plane (BGP) . In that respect they are static MACs. However these MACs must be subjected to a MAC Move. For e.g. a MAC installed by BGP moves from the remote to a local port. This should trigger a hardware learn event. The SAI attribute is to allow for the hardware to learn the MAC even though the MAC is programmed as static.
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.
can you add some clarification on the TRAP attributed introduced in #696 ? What is the expectation of that notification when MAC_MOVE is enabled or disabled on this attribute.
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.
The trap introduced in #696 is a generic trap, where as this PR is about a specific MAC entry. So the specific entry would take precedence over a generic trap. I hope this clarifies.
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.
(Additional clarification) When MAC_MOVE is explicitly disabled for a static mac-entry, the trap (introduced in #696) would also not be generated.
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.
Is this also expected to work with MLAG?
The behavior of this entry is closer to being "dynamic but not ageable" than "static but movable" because static FDB is not supposed to be changed by anything besides the application that created it, whereas dynamic is expected to change, and we just say that it does not expire. Let me know if you considered this way of definition.
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 this is expected to work with MCLAG as well. Please refer to the MC Lag enhancements HLD floated in sonic forum. Static vs dynamic refers to a MAC being ageable and not whether they are movable. Here we are adding an attribute to static mac specifying if this is moveable or not.
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.
@bandaru-viswanath, thanks for the clarification. can you make the clarification in the header?
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.
@lguohan Updated the header file.