-
Notifications
You must be signed in to change notification settings - Fork 580
Allow IPv4 link-local nexthops #1903
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
Conversation
Signed-off-by: Utpal Kant Pintoo [email protected]
continue; | ||
} | ||
} | ||
|
||
NeighborEntry neighbor_entry = { ip_address, alias }; |
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.
root@sonic:/home/admin# redis-cli hgetall "NEIGH_TABLE:Ethernet0:169.254.0.1"
- "family"
- "IPv4"
- "neigh"
- "00:10:94:00:00:01"
Is this static or dynamic neighbor?
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.
For testing, the neighbour/route entries were manually added to APP DB. In typical use-case, this neighbour entry will be 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.
Okay, which module would push this static ARP and MAC into the kernel? I believe, we should push static nbr entry add/del code as well to community to support BGP unnumbered use-case, right?
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.
FRR(Zebra) is already adding static(PERMANENT) IPv4 LL neighbours in kernel. Please check the APP DB log in the issue. This change is only to allow such entries in NeighOrch as well.
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.
@utpalkantpintoo Can you attach the testing logs of BGP unnumbered use-case with the community image, just to make sure this code is working fine for that use-case?
/azpw run |
/AzurePipelines run |
Azure Pipelines successfully started running 1 pipeline(s). |
@venkatmahalingam , @dgsudharsan , could you please review and signoff? |
Sure Prince, I left one last comment now for the BGP unnumbered use-case testing, since there are multiple PRs being raised, it's always better to attach the integration testing logs for the benefit of the community. |
@utpalkantpintoo @prsunny @venkatmahalingam |
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 UT to route module to cover link local next hop cases. Can you please add them?
@utpalkantpintoo Can you please provide an ETA when UT will be provided? |
@utpalkantpintoo @adyeung Can you please provide an ETA for addressing the review comments? |
Hi I have verified the changes with IPv4/v6 traffic in 2-node topology with BGP un-numbered config.
|
@utpalkantpintoo, @prsunny do we need this on 2021111 as well? |
*Orchagent (NeighOrch) currently ignores IPv4 link-local neighbours. Any route pointing to such a nexthop is not added to the ASIC DB. Such routes are expected and should be supported. Signed-off-by: Utpal Kant Pintoo [email protected]
*Orchagent (NeighOrch) currently ignores IPv4 link-local neighbours. Any route pointing to such a nexthop is not added to the ASIC DB. Such routes are expected and should be supported. Signed-off-by: Utpal Kant Pintoo [email protected]
What I did
Fix for issue: sonic-net/sonic-buildimage#8606
Why I did it
Orchagent (NeighOrch) currently ignores IPv4 link-local neighbours. Any route pointing to such a nexthop is not added to the ASIC DB. Such routes are expected and should be supported.
How I verified it
Details if related