[srv6]: Add static MySID warm boot support - #4814
Draft
BYGX-wcr wants to merge 2 commits into
Draft
Conversation
Reconcile route and MySID APP_DB state under one bgp warm-start FSM, make Srv6Orch replay dependency-safe and idempotent, and add focused mock and DVS coverage. Signed-off-by: BYGX-wcr <wcr@live.cn>
Collaborator
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends SWSS SRv6 MySID handling to be warm-restart safe by reconciling ROUTE_TABLE and SRV6_MY_SID_TABLE under a single warm-start flow, making MySID replay dependency-safe/idempotent in Srv6Orch, and adding targeted unit + DVS coverage.
Changes:
- Extend
WarmStartHelperto manage restoration/reconciliation across multiple APP_DB tables (primary + registered tables). - Refactor
Srv6OrchMySID programming to use retry-cache constraints for VRF/neighbor dependencies and make replay/update paths more idempotent. - Add mock tests and DVS warm-restart tests for static MySID replay and reconciliation behavior.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| warmrestart/warmRestartHelper.h | Add multi-table context support to WarmStartHelper (registered tables). |
| warmrestart/warmRestartHelper.cpp | Implement registration, multi-table restore/reconcile, and per-table refresh maps. |
| tests/test_warm_reboot.py | Add DVS coverage for static MySID warm restart and fpmsyncd timer replay. |
| tests/mock_tests/warmrestarthelper_ut.cpp | Add UT coverage for multi-table WarmStartHelper reconciliation semantics. |
| tests/mock_tests/srv6orch_ut.cpp | Add UT coverage for MySID replay idempotence and dependency retry behavior. |
| tests/mock_tests/mock_table.h | Expose producer operation counters for more precise UT assertions. |
| tests/mock_tests/mock_table.cpp | Track ProducerStateTable set/del counts in mock DB layer. |
| tests/mock_tests/fpmsyncd/receive_srv6_mysids_ut.cpp | Add UT coverage for warm-restart deferral/dedup behavior in MySID reception. |
| tests/mock_tests/fdbsyncd/fake_warmstarthelper.cpp | Update stub WarmStartHelper API to match new methods/signatures. |
| tests/mock_tests/fake_warmstarthelper.cpp | Extend mock warmstart helper to track refresh maps per table name. |
| orchagent/vrforch.cpp | Notify Srv6Orch when VRFs become available to unblock MySID retries. |
| orchagent/srv6orch.h | Expand MySidEntry state tracking; add retry/dependency plumbing and new helpers. |
| orchagent/srv6orch.cpp | Implement dependency-safe/idempotent MySID programming and stale tunnel cleanup. |
| orchagent/retrycache.h | Add MySID-specific constraint types (VRF / NEXTHOP). |
| fpmsyncd/routesync.h | Add table-name-aware warm-restart set/del helpers. |
| fpmsyncd/routesync.cpp | Warm-restart defer/dedup for SRV6_MY_SID_TABLE via WarmStartHelper multi-table support. |
| doc/swss-schema.md | Document SRV6_MY_SID_TABLE schema details and warm-restart reconciliation expectations. |
Comment on lines
171
to
+175
| const std::string key = kfvKey(kfv); | ||
| auto table = m_tableContexts.find(syncTableName); | ||
|
|
||
| m_refreshMap[key] = kfv; | ||
| assert(table != m_tableContexts.end()); | ||
| table->second->refreshMap[key] = kfv; |
Comment on lines
+1716
to
1734
| auto status = sai_srv6_api->create_my_sid_entry(&my_sid_entry, (uint32_t) attributes.size(), attributes.data()); | ||
| if (status != SAI_STATUS_SUCCESS) | ||
| { | ||
| SWSS_LOG_ERROR("Failed to create my_sid entry %s, rv %d", key_string.c_str(), status); | ||
| return false; | ||
| SWSS_LOG_ERROR("Failed to create my_sid entry %s, rv %d", key_string.c_str(), status); | ||
| auto handle_status = handleSaiCreateStatus(SAI_API_SRV6, status); | ||
| if (handle_status != task_success) | ||
| { | ||
| removeMySidCounter(my_sid_entry, counter_oid); | ||
| if (created_tunnel_term) | ||
| { | ||
| removeMySidIpInIpTunnelTermEntry(tunnel_term_entry); | ||
| } | ||
| if (acquired_tunnel) | ||
| { | ||
| removeMySidIpInIpTunnel(dscp_mode.get()); | ||
| } | ||
| return handle_status; | ||
| } | ||
| } |
Signed-off-by: BYGX-wcr <wcr@live.cn>
Collaborator
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (3)
orchagent/srv6orch.cpp:1720
- When
create_my_sid_entry()returnsSAI_STATUS_ITEM_ALREADY_EXISTS,handleSaiCreateStatus()maps that totask_success, so this code proceeds as if creation succeeded without ensuring the existing entry is updated to reference the newly-created tunnel/counter. That can leak the newly created resources and leave the in-memory cache claiming a counter/tunnel is attached when it isn’t. Handle ITEM_ALREADY_EXISTS explicitly by applying the desired attributes viaset_my_sid_entry_attribute()(or otherwise ensuring resources are attached) before caching/incrementing CRM.
auto status = sai_srv6_api->create_my_sid_entry(&my_sid_entry, (uint32_t) attributes.size(), attributes.data());
if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_ERROR("Failed to create my_sid entry %s, rv %d", key_string.c_str(), status);
auto handle_status = handleSaiCreateStatus(SAI_API_SRV6, status);
warmrestart/warmRestartHelper.cpp:175
insertRefreshMap(syncTableName, ...)relies onassert(table != m_tableContexts.end()). In release builds (NDEBUG), this becomes a no-op and dereferencingtable->secondis undefined behavior if a caller forgets to register the table (or passes the wrong name). It should fail deterministically with a clear error instead of potentially crashing later.
const std::string key = kfvKey(kfv);
auto table = m_tableContexts.find(syncTableName);
assert(table != m_tableContexts.end());
table->second->refreshMap[key] = kfv;
tests/mock_tests/fake_warmstarthelper.cpp:136
getMockWarmStartHelperRefreshMapSize()usesg_mockRefreshMaps[tableName], which silently inserts a new empty entry when the table name is unknown. That mutates global test state and can mask bugs where a table was never registered.
size_t getMockWarmStartHelperRefreshMapSize(const std::string &tableName)
{
return g_mockRefreshMaps[tableName].size();
}
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
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.
Reconcile route and MySID APP_DB state under one bgp warm-start FSM, make Srv6Orch replay dependency-safe and idempotent, and add focused mock and DVS coverage.
What I did
Why I did it
How I verified it
Details if related