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.

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 2 commits into from
Jun 1, 2020
Merged

Support for static FDB Entries to allow MAC Move #1024

merged 2 commits into from
Jun 1, 2020

Conversation

bandaru-viswanath
Copy link
Contributor

No description provided.

* @default false
* @validonly SAI_FDB_ENTRY_ATTR_TYPE == SAI_FDB_ENTRY_TYPE_STATIC
*/
SAI_FDB_ENTRY_ATTR_ALLOW_MAC_MOVE,
Copy link
Contributor

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?

Copy link

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link

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.

Copy link
Collaborator

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?

Copy link
Contributor Author

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.

@dgsudharsan
Copy link
Contributor

Have you checked this? #696
In this case when a static move is detected the application will be notified and it can re-program the static MAC on the port.

@srj102
Copy link

srj102 commented Jan 22, 2020

Have you checked this? #696
In this case when a static move is detected the application will be notified and it can re-program the static MAC on the port.

696 is after a static move is detected. Here the setting is to allow for a static move as described in the previous reply.

@srj102
Copy link

srj102 commented Mar 19, 2020

Have you checked this? #696
In this case when a static move is detected the application will be notified and it can re-program the static MAC on the port.

The #696 has a trap type added for static fdb mac move.
However I do not see the SAI_SWITCH_ATTR_STATIC_FDB_MOVE_PACKET_ACTION merged to saiswitch.h though it is mentioned in the PR.

I am not sure if this was intentional. Without this the added trap does not seem to make sense.

The above packet action even if added is a switch wide attribute which is applicable to locally configured static MACs as well. We may not want this and would like to
differentiate from an EVPN MAC. In other words we would like a drop if there is a mac move experienced for a local static but at the same time would like to
allow EVPN MACs to be subjected to MAC Move.

The EVPN remote macs though programmed as static are not the same as locally configured static MACs. These are dynamically learnt MACs on a remote
switch.

@lguohan
Copy link
Collaborator

lguohan commented Apr 14, 2020

@marian-pritsak and @itaibaz to check the behavior.

@rlhui
Copy link
Collaborator

rlhui commented May 20, 2020

@kcudnik , would you please review/approve also? thanks

Added additional clarification for the attribute based on community feedback
@rlhui rlhui merged commit 744ff7f into opencomputeproject:master Jun 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants