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.
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
EVPN VxLAN update for platforms using P2MP tunnel based L2 forwarding #806
EVPN VxLAN update for platforms using P2MP tunnel based L2 forwarding #806
Changes from 14 commits
3220491
2470cea
ebd5c97
8611ebf
e33233f
9e11684
6739f66
3d35f88
dd2be56
ab13816
eeba0d4
9b177f5
1715455
54ab573
cc0e8c8
7107e00
17583e3
21d2ae4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Add some details on P2P vs P2MP here.
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.
Done
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 tunnel object can be deleted only after all the IMR, MAC, IP Prefix routes are removed by BGP.
The bridge port object can be deleted only after the fdb count referencing the bridgeport goes down to 0.
Once the map count becomes 0, the vxlanmgr holds off handling further Map creations till the number of P2P
tunnels goes down to 0. This is to handle quick delete and readd of mapping entries.
VxlanMgr gets to know of P2P tunnel count by looking at the number of state table entries.
However for P2MP the state table entries are not populated..
How is this scenario handled ?
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.
There are no changes to P2MP tunnel logic currently except int this case it will always have 0 dip tunnels. Rest of the logic is the same as in existing code. I did have a question on FDB reference. In current orchagent code i don't not see tunnel delete skipped based on fdb reference count. Only bridge port removal is skipped while the actual tunnel might be deleted even when FDB reference count is non zero. Is that by design? If that's the case isn't it an issue with bridge port still referencing the tunnel port? https://github.com/Azure/sonic-swss/blob/4f1d726d4cbf8a283b22cd5f612cf03ca21a27b3/orchagent/vxlanorch.cpp#L1499
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.
Few more questions and clarifications.
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.
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.
Regarding your query https://github.com/Azure/SONiC/pull/806/files#r669059056 , No the bridgeport is deleted first and then the tunnel object which is being referenced by the bridgeport.
As part of deleteDynamicDIPTunnel there is a call to getTunnelPort. If this exists then the Tunnel Object is not deleted.
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.
@dgsudharsan @srj102 Currently when tunnel nexthops are created by orchagent, it doesn't check for reachability of remote destination. Are we following the same approach or add the reachability condition while creating the the nexthops?
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 don't think we are enhancing that here. The existing approach still holds 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.
Are there any changes required for MAC route and MAC Move handling?
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.
In case of P2MP tunnel there will be only one bridge port and thus there are some minor changes in code where instead of checking bridgeport alone fur duplicate, endpoint IP is also checked.
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.
Why is this command not supported for P2MP case? this is important command to show the tunnel status.
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.
@venkatmahalingam I don't think the command's output signifies the actual tunnel status, meaning we don't perform any kind of tunnel status /monitoring or checking reachability at both ends. These tunnels are logically created within the device and status signifies just receiving the remote end point. I believe With P2MP model we have only one tunnel. What i can do is consider introducing an enhancement using which end points are printed when displaying the P2MP tunnel.
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.
ok, maybe, you can remove the lines you have added for P2MP, as nothing is going to be changed in the output for P2MP case.
Do we have to display P2P/P2MP in any cmd output?
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.
Venkat. I believe the HLD is not inline with the actual implementation. This command's output maps to the implementation of "show vxlan remotevtep" https://github.com/Azure/sonic-utilities/blob/c2ac2d2327acc90a35b33ddead070995034d457a/show/vxlan.py#L176
This indication of remote tunnels will not be support in P2MP based scenarios since internally we create only one tunnel and not multiple tunnels. I will remove the line here and will try to update the command "show vxlan remotevtep" for P2MP scenario.
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.
We need to add more details on how counters are designed for P2P (in the future ?) and P2MP case.
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.
Done