[fpmsyncd]: Program ECMP routes with multiple SRv6 nexthops#4026
[fpmsyncd]: Program ECMP routes with multiple SRv6 nexthops#4026cscarpitta wants to merge 29 commits into
Conversation
There is a bug in fpmsyncd related to the handling of ECMP routes with multiple SRv6 nexthops. When FRR attempts to program an ECMP route with multiple SRv6 nexthops, the FPM module encodes all nexthops in the Netlink message. Upon receiving this Netlink message, fpmsyncd processes it but returns the error "Multipath SRv6 routes aren't supported" and subsequently drops the route. As a result, the ECMP route is not installed in the ASIC. This pull request extends fpmsyncd to properly handle and install ECMP routes with multiple SRv6 nexthops. Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
|
/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). |
| nl_connect(m_nl_sock, NETLINK_ROUTE); | ||
| rtnl_link_alloc_cache(m_nl_sock, AF_UNSPEC, &m_link_cache); | ||
|
|
||
| m_appDb = make_shared<DBConnector>("APPL_DB", 0); |
There was a problem hiding this comment.
The getSrv6SidListsFromRoute() function reads back the segment field from ROUTE_TABLE in APPL_DB to find the SID list to clean up on delete.
But delWithWarmRestart() is called before this read-back, which means the route entry may already be gone from Redis by the time the read happens (especially over the pipeline). This silently results in vpn_sid_str being empty, SID list refcounts never being decremented, and a persistent SID list leak in SRV6_SID_LIST_TABLE.
The cleanest way would be to add a std::map<std::string, std::string> m_srv6_route_to_sidlists (mapping routeKey → sidlist_csv) populated at RTM_NEWROUTE time and consumed at RTM_DELROUTE time — exactly the same pattern as the existing m_srv6_sidlist_refcnt. This eliminates both the new DBConnector and the race condition.
There was a problem hiding this comment.
@selva-nexthop : this map can have high memory footprint if the routescale is large.
|
|
||
| auto it = m_srv6_sidlist_refcnt.find(srv6SidListTableKey); | ||
| if (it != m_srv6_sidlist_refcnt.end()) | ||
| getSrv6SidListsFromRoute(routeTableKeyStr, vpn_sid_str); |
There was a problem hiding this comment.
the route delete must happen after reading the SID list, not before.
// Read FIRST, then delete
getSrv6SidListsFromRoute(routeTableKeyStr, vpn_sid_str);
delWithWarmRestart(...);
// Now decrement refcounts
|
|
||
| /* Delete SID lists from SRV6_SID_LIST_TABLE */ | ||
| vector<string> sidlists = tokenize(vpn_sid_str, ','); | ||
| for (auto sidlist : sidlists) |
There was a problem hiding this comment.
SID list key design:
Using the SID value (e.g., fc00:0:1:e000::) as the SRV6_SID_LIST_TABLE key works for sharing, but it diverges from the SONiC SRv6 HLD which uses user-defined segment names.
It also means a multi-segment SID list (e.g., fc00::1→fc00::2) has no natural single-SID key. Consider using FPM_ROUTE_ENCAP_SRV6_ENCAP_SIDLIST_NAME (already defined in dplane_fpm_sonic) to carry a stable name from FRR through to SONiC so the key is operator-visible and stable.
| routeTableKeyStr.c_str(), vpn_sid_str.c_str()); | ||
|
|
||
| /* Delete SID lists from SRV6_SID_LIST_TABLE */ | ||
| vector<string> sidlists = tokenize(vpn_sid_str, ','); |
There was a problem hiding this comment.
Changing FPM_ROUTE_ENCAP_SRV6_VPN_SID to carry a variable-length SID blob — this changes the wire semantics of an existing attribute and breaks older consumers.
FPM_ROUTE_ENCAP_SRV6_ENCAP_SIDLIST (already in the enum at value 7) was designed for exactly this purpose and carries structured segment entries.
Keeping VPN_SID as the primary/backward-compat SID and adding ENCAP_SIDLIST as an optional extension for multi-SID routes is both safer and more aligned with the SONiC SRv6 data model.
|
Thanks @cscarpitta for the work on multipath SRv6 steer routes — this is a much-needed feature. I have a few design-level observations that I think are worth discussing before this merges, because there are some issues that could lead to correctness problems in production. |
There is a bug in fpmsyncd related to the processing of SRv6 SID lists containing multiple SIDs. When FRR attempts to program a SID list with multiple SRv6 SIDs, the FPM module correctly encodes all SIDs in the Netlink message. However, upon receiving this message, fpmsyncd processes only the first SID and installs only that SID into the ASIC. As a result, the ASIC is programmed with an incomplete and incorrect SID list. This pull request updates fpmsyncd to correctly handle and install SID lists containing multiple SRv6 SIDs. Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| srv6SidListTableKey.c_str(), it->second); | ||
| } | ||
| else | ||
| /* Write SID lists to SRV6_SID_LIST_TABLE */ |
There was a problem hiding this comment.
When a multipath SRv6 steering route loses one nexthop, FRR sends RTM_NEWROUTE with the updated (smaller) nexthop set. The existing RTM_NEWROUTE handler only incremented refcounts for the new SID lists, but never decremented refcounts for the SID lists that belonged to the removed nexthop. This left stale entries in SRV6_SID_LIST_TABLE.
Specifically for test_AddRemoveSrv6SteeringRouteIpv4:
Route 192.0.3.0/24 is multipath: sids fc00:0:1:e000:: (refcnt=2, shared with 192.0.2.0/24) + fc00:0:3:e000:: (refcnt=1)
Removing 192.0.2.0/24 → RTM_DELROUTE → refcntfc00:0:1:e000:: = 1 ✓
Removing Ethernet104 from 192.0.3.0/24 → FRR sends RTM_NEWROUTE with only fc00:0:3:e000:: → refcntfc00:0:3:e000:: bumped to 2 (wrong!), fc00:0:1:e000:: stays at 1 (wrong!)
Final delete → refcntfc00:0:3:e000:: = 1 → both SID lists leaked, never deleted
wait_for_deleted_entry times out → test fails without teardown → sr0/Vrf13 persist → cascading failures into TestSrv6StaticSidFlavors
Fix: Before processing new SID lists on RTM_NEWROUTE, call the already-defined getSrv6SidListsFromRoute() to read the route's previous SID lists from APP_DB and decrement their refcounts. getSrv6SidListsFromRoute() was added in this commit precisely for this purpose but was never wired up.
we need to have below fix in this function.
/* If this route already exists in APP_DB, it is an update rather than a new
* route (e.g. a multipath route losing one of its nexthops). Decrement the
* refcounts for the old SID lists so that SID lists no longer referenced by
* any route are removed from SRV6_SID_LIST_TABLE.
*/
string old_sidlists_str;
if (getSrv6SidListsFromRoute(routeTableKeyStr, old_sidlists_str))
{
vector<string> old_sidlists = tokenize(old_sidlists_str, ',');
for (auto& old_sidlist : old_sidlists)
{
auto it = m_srv6_sidlist_refcnt.find(old_sidlist);
if (it != m_srv6_sidlist_refcnt.end())
{
assert(it->second > 0);
(it->second)--;
SWSS_LOG_INFO("Route update: refcount for old SID list '%s' decreased to %u",
old_sidlist.c_str(), it->second);
if (it->second == 0)
{
m_srv6SidListTable.del(old_sidlist);
SWSS_LOG_INFO("Route update: SID list '%s' removed from ApplDB (refcount zero)",
old_sidlist.c_str());
m_srv6_sidlist_refcnt.erase(it);
}
}
else
{
SWSS_LOG_WARN("Route update: old SID list '%s' not found in refcount map",
old_sidlist.c_str());
}
}
}
|
@deepak-singhal0408 could you add this PR for routing dashboard for PR tracking. |
Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
@GaladrielZhao could you relook at the change and sign-off if looks ok? |
9 participants
What I did
There is a bug in fpmsyncd related to the handling of ECMP routes with multiple SRv6 nexthops.
When FRR attempts to program an ECMP route with multiple SRv6 nexthops, the FPM module encodes all nexthops in the Netlink message.
Upon receiving this Netlink message, fpmsyncd processes it but returns the error "Multipath SRv6 routes aren't supported" and subsequently drops the route.
As a result, the ECMP route is not installed in the ASIC.
This pull request extends fpmsyncd to properly handle and install ECMP routes with multiple SRv6 nexthops.
Why I did it
ECMP routes with multiple SRv6 nexthops were not being processed by fpmsyncd and, as a result, were not installed in the ASIC.
How I verified it
The SONiC VS test cases have been extended to include a scenario with an ECMP route containing multiple SRv6 nexthops. The extended tests demonstrate that fpmsyncd now processes the ECMP route correctly and that the route is successfully installed in the ASIC.