-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Srv6 hld #795
Srv6 hld #795
Conversation
doc/srv6/srv6-hld-v19.md
Outdated
|
||
![draw-configdb](images/drawing-configdb-frr3.png) | ||
|
||
Before FRR is ready, we will use static configuration to set SIDs and apply policy for TE. It enables basic SRv6 operation and populates SRv6 into ASIC, allows SRv6 data plan forwarding. More complicated SRv6 policy can be enabled when SRv6 is fully supported in FRR and passed from FRR to fpmsyncd. |
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.
data plane - typo
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.
fixed.
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.
@goldenrye , its not addressed
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.
@goldenrye , its not addressed
Hi @prsunny, the changes shown in this conversation are outdated. Can you please check the latest commit here
we have fixed the typo. thanks~
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 explain more on controller role in the picture? Do we have separation of the route management between controller and FRR? If both can operate on same route entry and controller is doing update. We need more explanation on how to coordination is handled between these two. e.g. removing route entries shared by controller and FRR..
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.
@caizhenghui-juniper Thanks for reviewing the HLD. For phase 1, the controller is supposed to avoid route conflicts with FRR. This can be achieved by using different VRFs or prefixes between routes from controller or FRR. For phase 2, all routes update need to go through FRR, who will take care of the route conflicts.
doc/srv6/srv6-hld-v19.md
Outdated
|
||
|
||
|
||
NextHopKey |
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.
Please capture this as a separate section for NH Key changes. Similar to heading SRV6Orchagent
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
doc/srv6/srv6-hld-v19.md
Outdated
seg_src = address ; New optional field. Source addrs for sid encap | ||
``` | ||
|
||
## 2.3 Orchestration Agent Changes |
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.
Please have a section to capture examples of kernel routes and kernel interaction
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.
I don't see this addressed. Did you miss any commit?
doc/srv6/srv6-hld-v19.md
Outdated
|
||
|
||
|
||
# 2 Feature Design |
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 details of srv6mgr modifying the ROUTE_TABLE is not clear. Please have it captured in detail and with flow diagram.
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.
update in the HLD
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 our previous discussion, it was decided to have controller to push routes to APP_DB directly thereby avoiding srv6mgr altogether. Also the config_db entries can be directly subscribed by srv6orch similar to fgnhgorch. Please address this
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.
@prsunny Hi Prince, yes we are avoiding srv6mgr as this was stop gap until FRR integration which is planned for next release. Agree that hence srv6 mgr can be avoided and have controller push the routes to APP_DB. We have these changes reflected in the HLD now and also in the diagram. Thanks.
doc/srv6/srv6-hld-v19.md
Outdated
; New table | ||
; holds local SID to behavior mapping, allow 1:1 or n:1 mapping | ||
|
||
key = SRV6_LOCAL_SID:ipv6address |
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.
config-DB table should use '|' key seperator, please change it in all config-DB tables.
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.
fixed
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 do not see this addressed. Please fix for all tables in this section
doc/srv6/srv6-hld-v19.md
Outdated
|
||
Schema: | ||
|
||
``` |
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.
Please create SONiC YANG PR for review if not done already.
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.
provided in the latest HLD
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 @prsunny Could you please take a look to review the SRv6 HLD. Thanks.
Updated NextHopKey section
Address review comments
Minor updates
File name update
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.
Approving from my side. I see that orchagent (routeorch<->srv6orch) interactions are not captured with flow diagrams. Will review the code for further clarity.
@venkatmahalingam , @caizhenghui-juniper , please sign-off |
Please plan for sonic-mgmt test PR and update the description with link |
Rename LOCAL_SID_TABLE to MY_SID_TABLE.
f3c16e2
Thanks @prsunny, will update the PR with sonic-mgmt tests PR. We will plan for the next Sonic release. |
@prsunny @venkatmahalingam @caizhenghui-juniper Updated the PR with name change of LOCAL_SID to MY_SID. Please approve and merge the HLD. |
@@ -0,0 +1,598 @@ | |||
# Segment Routing over IPv6 (SRv6) HLD | |||
|
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.
Please check LOCAL_SID to MY_SID replacements in the diagram as well, I see LOCAL_SID usage in the diagrams, please correct them.
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.
Please replace LOCAL_SID to MY_SID in all the diagrams as well.
Thanks @venkatmahalingam. Modified one diagram. Other one will be updated by @hzheng5 |
@venkatmahalingam @prsunny All images are updated with the correct MY_SID name. Please approve and merge the HLD. |
Why I did it
This document describes the Functionality and High-level design of the SRv6 feature.
Description for the changelog