[swss]:Skip APPL_STATE_DB and pub/sub writes when FIB suppression is disabled#4333
[swss]:Skip APPL_STATE_DB and pub/sub writes when FIB suppression is disabled#4333mike-dubrovsky wants to merge 1 commit into
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Hi, thank you for the work on adding FIB‑suppression support across orchagent and fpmsyncd.
This is required for the RouteOrch tests to build and appears to be another file that the PR may have unintentionally missed? After defining gEnableFibSuppress, the following failing RouteOrch tests all passed:
Question: Did the upstream CI pipeline succeed or did I miss something from the PR? |
3120ae9 to
6a7694d
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@mike-dubrovsky , please follow the description template "why i did", "how tested" etc. Also update results from sonic-mgmt tests. @dgsudharsan for viz. |
| s.removeSelectable(&eoiuCheckTimer); | ||
| } | ||
| } | ||
| else if (temps == &deviceMetadataTableSubscriber) |
There was a problem hiding this comment.
Why is this removed? seems unrelated.
There was a problem hiding this comment.
This fpmsyncd code was subscribing to CONFIG_DB changes to dynamically toggle FIB suppression at runtime. After this fix, the suppress-fib-pending configuration becomes operational only on swss container restart (cold/fast reboot or systemctl restart swss), so there is no longer a need to listen for CONFIG_DB changes.
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce route programming overhead by avoiding APPL_STATE_DB route-state writes/notifications when BGP FIB suppression is disabled, and by making FIB suppression a startup-time decision coordinated between orchagent and fpmsyncd.
Changes:
- Add an
orchagentstartup flag (-F) to enable FIB suppression and guardRouteOrch::publishRouteState()behind it. - When suppression is disabled, clear stale route entries from
APPL_STATE_DBduringRouteOrchconstruction. - Update
fpmsyncdto read suppression enablement at startup (no longer dynamically via CONFIG_DB subscription), and adjust VS/mock tests accordingly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
orchagent/main.cpp |
Adds -F CLI parsing and global flag to control suppression behavior. |
orchagent/routeorch.cpp |
Skips publishing route state when suppression is off; clears stale APPL_STATE_DB route entries when suppression is off. |
fpmsyncd/fpmsyncd.cpp |
Removes CONFIG_DB subscription logic and reads suppression state at startup from STATE_DB. |
tests/test_route.py |
Restarts swss/fpmsyncd in the route offload test so suppression state changes take effect. |
tests/mock_tests/routeorch_ut.cpp |
Forces suppression enabled for publish-related unit tests. |
You can also share your feedback on Copilot code review. Take the survey.
| std::string suppressionEnabledStr; | ||
| deviceMetadataTable.hget("localhost", "suppress-fib-pending", suppressionEnabledStr); | ||
| Table fibSuppressTable(&stateDb, STATE_FIB_SUPPRESS_TABLE_NAME); | ||
| fibSuppressTable.hget("system", "oper_state", suppressionEnabledStr); | ||
| if (suppressionEnabledStr == "enabled") | ||
| { | ||
| routeResponseChannel = std::make_unique<NotificationConsumer>(&applStateDb, routeResponseChannelName); | ||
| sync.setSuppressionEnabled(true); | ||
| } | ||
| SWSS_LOG_NOTICE("FIB suppression oper_state: %s", suppressionEnabledStr.c_str()); |
There was a problem hiding this comment.
The FIB_SUPPRESS_TABLE|system oper_state is written to STATE_DB by swss.sh before the swss container starts, as part of the companion PR sonic-buildimage #26151. Both orchagent and fpmsyncd read this value at startup.
|
@vpandian-nokia, I’m sorry — I initially posted an incomplete diff. I think this version is better and tested. I will address review comments in a day or two. |
6a7694d to
a4f788f
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
a4f788f to
2f62db4
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@dgsudharsan Could you please help me with a code review? |
|
|
||
| std::string suppressionEnabledStr; | ||
| deviceMetadataTable.hget("localhost", "suppress-fib-pending", suppressionEnabledStr); | ||
| Table fibSuppressTable(&stateDb, STATE_FIB_SUPPRESS_TABLE_NAME); |
There was a problem hiding this comment.
I recommend having the device-metadata table and if someone configures during runtime, lets throw a message in CLI saying please perform config save and config reload to take effect.
There was a problem hiding this comment.
@dgsudharsan
Thank you for the review; sorry for the delay with the reply.
I do keep the suppress-fib-pending config state in the DEVICE_METADATA table. The CLI (config suppress-fib-pending) writes to DEVICE_METADATA and prints a message telling the user to perform config save and config reload (or systemctl restart swss) for the change to take effect. This is in the sonic-utilities PR (#4361).
The reason I also introduced an oper state in STATE_DB is the following: between the time the user changes CONFIG_DB and actually restarts swss, if the BGP container restarts independently, fpmsyncd would read the new CONFIG_DB value while orchagent is still running with the old flag. If fpmsyncd thinks suppression is enabled but orchagent isn't sending route confirmations, BGP route propagation stalls (traffic black hole).
To solve this, swss.sh reads CONFIG_DB and writes the current operational FIB suppression state to STATE_DB (FIB_SUPPRESS_TABLE|system / oper_state) before orchagent and fpmsyncd start. All processes (orchagent, fpmsyncd, route_check) read the oper state from STATE_DB, so they always stay in sync regardless of CONFIG_DB changes.
| void usage() | ||
| { | ||
| cout << "usage: orchagent [-h] [-r record_type] [-d record_location] [-f swss_rec_filename] [-j sairedis_rec_filename] [-b batch_size] [-m MAC] [-i INST_ID] [-s] [-z mode] [-k bulk_size] [-q zmq_server_address] [-c mode] [-t create_switch_timeout] [-v VRF] [-I heart_beat_interval] [-R] [-M]" << endl; | ||
| cout << "usage: orchagent [-h] [-r record_type] [-d record_location] [-f swss_rec_filename] [-j sairedis_rec_filename] [-b batch_size] [-m MAC] [-i INST_ID] [-s] [-z mode] [-k bulk_size] [-q zmq_server_address] [-c mode] [-t create_switch_timeout] [-v VRF] [-I heart_beat_interval] [-R] [-M] [-F]" << endl; |
There was a problem hiding this comment.
Lets continue to use the config_db knob but make sure we add relevant message in CLI and documentation that they need to config save and reboot to take effect.
There was a problem hiding this comment.
@dgsudharsan
Same as my reply above: the proposed fix does continue to use CONFIG_DB, and we do print a message in the CLI.
However, if the user does not restart swss (or perform config save / config reload) right away, there is a vulnerability: if BGP crashes or the BGP container restarts, fpmsyncd and orchagent can get out of sync. So I introduced an oper state in STATE_DB to keep them in sync.
|
There is a PR #4437 that does async update of APPL_STATE_DB, notification and recording instead of completely skipping the update of APPL_STATE_DB. With PR #4437 we observed almost same performance with both suppress-fib-pending enabled and disabled while keeping APPL_STATE_DB contents and recordings intact. |
|
Hi @mike-dubrovsky , This PR changes FIB suppression from a runtime dynamic toggle to a startup-time flag ( |
|
Also @mike-dubrovsky, I don't think swss is the right backend for BGP config — it should not own the STATE_DB population. Here are two potential options: Design Proposal: FIB Suppress Config Backend & Conditional FRR Config Currently bgpd.main.conf.j2 always includes bgp suppress-fib-pending — even when the feature is disabled. The current code works as a workaround to support runtime config changes, but if we get rid of that requirement, we should make this conditional as well. Proposal: Make the FRR config conditional on CONFIG_DB, and ensure the right component owns the config lifecycle. Common to both options: Conditional FRR template {% if DEVICE_METADATA['localhost']['suppress-fib-pending'] == 'enabled' %} This eliminates the FRR ↔ feedback loop mismatch. config reload regenerates the template — this is why config reload (not just swss restart) should be the only option given to the user. Option A: Direct CONFIG_DB read (Recommended — simplest) Each consumer reads CONFIG_DB at startup. No new STATE_DB table, no intermediary. config reload Pros: Minimal code, no new tables, CONFIG_DB is populated before containers start (no race). Option B: bgpcfgd → STATE_DB intermediary bgpcfgd owns the config lifecycle, writes operational state to STATE_DB. config reload Pros: Follows config manager pattern, STATE_DB reflects operational truth. Cons: bgpcfgd/fpmsyncd startup race (same container, parallel supervisord), extra indirection. let me know what you think.. cc @stepanblyschak @StormLiangMS @prsunny @dgsudharsan @vganesan-nokia |
|
This set of PRs blocks both pub/sub and APPL_STATE_DB population when FIB suppression is disabled. P.S. Plus the current Sonic memory consumption is about 4,852 bytes per route ... |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
agree.. we need not populate some DB for which there is no consumer.. That extra thread processing capacity populating APPL_STATE_DB(in async update case), could be used for other tasks. |
|
Hi @deepak-singhal0408, Regarding @stepanblyschak sign-off : Why STATE_DB instead of Option A (direct CONFIG_DB read) The problem is the window between the CLI command and the actual reload. When the user runs
One alternative would be to restart swss container immediately when the CLI command is entered -- then we could use Option A because there would be no vulnerability window. But an immediate orchagent restart would be disruptive (data plane impact), so I hesitated to take that approach. STATE_DB avoids this entire class of problems: swss.sh writes the oper_state when swss restarts I am happy to do immediate orchagent restart if you / Sudarshan okay with it. It would simplify things. Why not Option B (bgpcfgd → STATE_DB) bgpcfgd is not the only FRR config manager — if frr_mgmt_framework_config is set, the container runs frrcfgd instead. So bgpcfgd is not a universal source of truth. Additionally, bgpcfgd and fpmsyncd both run inside the BGP container and are started in parallel by supervisord, creating a startup race for who writes/reads STATE_DB first. |
|
@mike-dubrovsky Lets discuss this in routing subgroup to understand feedback from others whether if restarting orchagent is preferred or we can stay with the current approach. |
|
@deepak-singhal0408 We discussed this briefly in AM routing subgroup on 5th May. The reason for this complexity is to handle a rare scenario where config is saved by services have not restarted and FRR docker crashes and restarts (or restarted by the user). This is a highly unlikely scenario and we can add this is not supported in the HLD. This would simply the design to just using the existing field in the config_db and we can avoid the new state_db table altogether. @mike-dubrovsky is fine with the approach. Please let us know if you have any concerns. |
|
@dgsudharsan In the PM meeting, Deepak asked: what's the reason we implemented the SONiC FIB-suppression CLI so that it takes effect instantly, instead of waiting for the next config reload? Do you happen to know the history? |
@prsunny @StormLiangMS Are you aware of the reason for the CLI. I believe it should be as part of the initial requirements for the suppress fib pending feature. |
Route download speed degraded after BGP prefix suppression was introduced. RouteOrch::publishRouteState() unconditionally writes to APPL_STATE_DB and sends a pub/sub notification on every route add/remove, even when FIB suppression is disabled and no consumer needs the data. Changes: Config changes take effect after a full config reload or device reboot, not dynamically at runtime. This eliminates multi-process race conditions between orchagent, fpmsyncd, and FRR. - orchagent: add -F command-line flag (gEnableFibSuppress) to control FIB suppression at startup; guard publishRouteState() to return early when suppression is disabled - fpmsyncd: read suppress-fib-pending from CONFIG_DB at startup; remove dynamic CONFIG_DB subscription (SubscriberStateTable) since config changes now require a config reload or device reboot - fpmsyncd: conditionally create routeResponseChannel and add it to the select loop only when suppression is enabled - tests: update mock tests to set gEnableFibSuppress before exercising publishRouteState; update integration test to restart swss/fpmsyncd after toggling suppress-fib-pending
2f62db4 to
48d0f60
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
i need to check, @StormLiangMS , do you recollect? |
|
@deepak-singhal0408 @dgsudharsan Could you please take a look when you have time? I re-did the PR as per our discussion. |
Why I did it
Route download speed degraded after BGP prefix suppression was introduced. RouteOrch::publishRouteState() unconditionally writes to APPL_STATE_DB and sends a pub/sub notification on every route add/remove, even when FIB suppression is disabled and no consumer needs the data.
What I did
Made publishRouteState() a no-op until FIB suppression is enabled.
Made "suppress-fib-pending" config changes take effect only after swss restart or config reload,
not dynamically at runtime. This eliminates multi-process race conditions between orchagent, fpmsyncd, and FRR.
Detailed description:
The CLI (config suppress-fib-pending) writes to DEVICE_METADATA and prints a message telling the user to perform config save and config reload for the change to take effect. This is in the sonic-utilities PR (#4361).
Detailed Changes in this PR:
How I verified it
UT and sonic-mgmt script.
Details if related
This change needs to go together with
sonic-net/sonic-buildimage#26151
sonic-net/sonic-utilities#4361
sonic-net/sonic-mgmt#22916
sonic-net/SONiC#2335