[fpmsyncd]: Fix SRv6 SID List sharing among prefixes #3860
+78
−37
Conversation
This file contains hidden or 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
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
cscarpitta
force-pushed
the
fix/fix_srv6_sidlist_sharing_v3
branch
from
a76f716 to
679c865
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
cscarpitta
force-pushed
the
fix/fix_srv6_sidlist_sharing_v3
branch
from
679c865 to
795980b
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Currently, there is a one-to-one mapping between prefixes and SRv6 SID List. When fpmsyncd receives a new route, it creates a SID List and associates it with the route. The key for the SID List is the Prefix of route. If fpmsyncd receives a second route pointing to the same SRv6 SID List, fpmsyncd creates another copy of the SID List for the second route. This approach results in unnecessary duplication of SID Lists for routes that could share the same SID List, and lead to significant scale issues. The number of SID Lists = the number of routes. This commit fixes the issue in fpmsyncd by enabling it to reuse existing SID Lists. Signed-off-by: Carmine Scarpitta <[email protected]>
The previous commit modified the SRv6 SID List to use the SIDs as the key. This commit updates the fpmsyncd mock test cases to align with this change, ensuring the test cases now expect the SIDs to serve as the key for the SID List. Signed-off-by: Carmine Scarpitta <[email protected]>
The previous commit modified the SRv6 SID List to use the SIDs as the key. This commit updates the SONiC VS test cases to align with this change, ensuring the test cases now expect the SIDs to serve as the key for the SID List. Signed-off-by: Carmine Scarpitta <[email protected]>
Signed-off-by: Carmine Scarpitta <[email protected]>
cscarpitta
force-pushed
the
fix/fix_srv6_sidlist_sharing_v3
branch
from
795980b to
93e1856
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ahsalam
approved these changes
Sep 12, 2025
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
dgsudharsan
previously approved these changes
Sep 15, 2025
dgsudharsan
reviewed
Sep 15, 2025
| { | ||
| string routeTableKeyStr = string(routeTableKey); | ||
| string srv6SidListTableKey = routeTableKeyStr; | ||
| string srv6SidListTableKey = vpn_sid_str; |
There was a problem hiding this comment.
With this new approach, should we maintain some reference count so that we don't delete the table when there are references?
There was a problem hiding this comment.
@dgsudharsan Thanks for the review.
I added the refcount for the SID lists.
| /* Write SID list to SRV6_SID_LIST_TABLE */ | ||
|
|
||
| string srv6SidListTableKey = routeTableKeyStr; | ||
| string srv6SidListTableKey = vpn_sid_str; |
There was a problem hiding this comment.
Hi Carmine,
I'm a bit confused about the SID field in this table.Does it represent the policy's SID(sid list) or the VPN's SID? Also, does the current code support only one SID?
There was a problem hiding this comment.
This is the SID list.
The current implementation can handle only one SID, but this is a different problem. I'm already working on another PR to fix this issue and allow fpmsyncd to properly handle SID lists with multiple SID. I will open a separate PR once this PR gets merged.
Signed-off-by: Carmine Scarpitta <[email protected]>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Currently, there is a one-to-one mapping between prefixes and SRv6 SID List. When fpmsyncd receives a new route, it creates a SID List and associates it with the route. The key for the SID List is the Prefix of route. If fpmsyncd receives a second route pointing to the same SRv6 SID List, fpmsyncd creates another copy of the SID List for the second route.
This approach results in unnecessary duplication of SID Lists for routes that could share the same SID List, and lead to significant scale issues. The number of SID Lists = the number of routes.
This commit fixes the issue in fpmsyncd by enabling it to reuse existing SID Lists.