[xcvrd] SI settings sync protocol between xcvrd and orchagent - #799
[xcvrd] SI settings sync protocol between xcvrd and orchagent#799arpit-nexthop wants to merge 10 commits into
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
arpit-nexthop
changed the title
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR updates sonic-xcvrd to support a redesigned SI settings synchronization protocol with orchagent, shifting from the legacy STATE_DB single-field status to an APPL_DB counter-based notification flow and adding a CMIS state-machine wait point for SI application acknowledgment.
Changes:
- Add an APPL_DB read path (cached
Table) and a helper to compute the next SI notification counter forsi_settings_notification. - Update SI settings publishing to include
si_settings_notification=SI_SETTINGS_NOTIFIED:<N>in APPL_DB, and reset toSI_SETTINGS_DEFAULT:<N>on SFP removal. - Extend the CMIS state machine with
CMIS_STATE_SI_SETTINGS_WAITand logic to wait for a correspondingSI_SYNC_DONE:<N>ack in STATE_DB.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sonic-xcvrd/xcvrd/xcvrd.py | Removes prior direct SI notify calls and switches SFP-removal reset to APPL_DB si_settings_notification counter format; adjusts SFF manager construction. |
| sonic-xcvrd/xcvrd/xcvrd_utilities/xcvr_table_helper.py | Adds cached APPL_DB PORT_TABLE reader and helper to compute the next notification number. |
| sonic-xcvrd/xcvrd/xcvrd_utilities/media_settings_parser.py | Adds si_settings_notification=SI_SETTINGS_NOTIFIED:<N> to the APPL_DB write and returns the notification number. |
| sonic-xcvrd/xcvrd/xcvrd_utilities/common.py | Adds CMIS_STATE_SI_SETTINGS_WAIT constant. |
| sonic-xcvrd/xcvrd/sff_mgr.py | Triggers SI settings notification on TRANSCEIVER_INFO updates (for non-CMIS paths). |
| sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py | Adds SI settings wait state and notification number tracking for CMIS ports. |
| sonic-xcvrd/tests/test_xcvrd.py | Updates/extends unit tests for the new counter-based protocol and new CMIS SI-wait state. |
|
|
||
| if rc != SFP_EEPROM_NOT_READY: | ||
| self.dom_db_utils.post_port_dom_thresholds_to_db(logical_port) | ||
| self.vdm_db_utils.post_port_vdm_thresholds_to_db(logical_port) |
There was a problem hiding this comment.
Added notify_media_setting_on_insert() at all insert sites; publishes for non-CMIS independent of --enable_sff_mgr, no-ops for CMIS/synced.
| # Check if SI settings notification number was cached during INSERTED state | ||
| notification_number = self.port_dict[lport].get('si_notification_number') | ||
|
|
||
| if notification_number is not None: | ||
| self.log_notice("{}: SI settings notified to OA with number {}, waiting for ASIC to apply".format( |
There was a problem hiding this comment.
The cached si_notification_number is now authoritative at AP_CONF because it's recovered earlier: a restart always re-enters CMIS_STATE_INSERTED (via force_cmis_reinit), and AP_CONF is strictly downstream of INSERTED.
| self.log_notice("{}: timeout waiting for SI settings to be applied (status: {}, expected: {})".format( | ||
| lport, si_settings_status, expected_notification_number)) | ||
| self.port_dict[lport].pop('si_notification_number', None) | ||
| self.port_dict[lport]['notify_si_settings'] = True | ||
| self.force_cmis_reinit(lport, retries + 1) |
There was a problem hiding this comment.
Will fix the PR description
| state_port_table = self.xcvr_table_helper.get_state_port_tbl(port_change_event.asic_id) | ||
| found, state_port_table_fvs = state_port_table.get(port_change_event.port_name) | ||
| if not found: | ||
| helper_logger.log_notice("Add logical port: Creating STATE_DB PORT_TABLE as unable to find for lport {}".format(port_change_event.port_name)) | ||
| state_port_table_fvs = [] |
|
|
||
| CMIS_MAX_RETRIES = 3 | ||
| CMIS_DEF_EXPIRED = 60 # seconds, default expiration time | ||
| CMIS_SI_SETTINGS_WAIT_TIMEOUT = 10 # seconds, timeout for waiting SI settings to be applied on ASIC |
There was a problem hiding this comment.
get_next_si_notification_number can return the same N twice due to producer/consumer lag
Reads and writes go through different swsscommon objects on different tables:
- Writes use
app_port_tbl(ProducerStateTable) - entries land in_PORT_TABLEand are only moved intoPORT_TABLEafter orchagent'sConsumerStateTabledrains them. get_next_si_notification_numberreadsapp_port_read_tbl(plainTableonPORT_TABLE) - i.e. the post-consumer view.
So two back-to-back calls (e.g. an initial notify followed shortly by a force_cmis_reinit on SI_SETTINGS_WAIT timeout) can both observe the same pre-write value and return the same N:
notify_media_setting() -> reads N=5 from PORT_TABLE -> writes :6 to _PORT_TABLE
SI_SETTINGS_WAIT timeout -> force_cmis_reinit
notify_media_setting() -> PORT_TABLE still shows :5 (OA hasn't drained) -> returns 6 again
This breaks the "monotonically increasing counter" invariant the HLD relies on, and can_skip_cmis_init_after_restart may then match a stale SI_SYNC_DONE:N from the first notification while the second is still pending. The window widens exactly when it matters most - high port-count startup or right after a SI_SETTINGS_WAIT timeout, since OA being slow is what caused the timeout in the first place.
|
@arpit-nexthop Please fix the build failure |
| helper_logger.log_notice("Notify media setting: Published SI setting " | ||
| "for lport {} in APP_DB".format(logical_port_name)) | ||
| "for lport {} in APP_DB with notification number {}".format(logical_port_name, notification_number)) | ||
| last_notification_number = notification_number |
There was a problem hiding this comment.
We are not consuming return value from notify_media_settings anywhere. Is it necessary for us to return last_notification_number here? If not, we can remove the use of this variable?
If it's necessary can't we just do return notification_number in the previous block itself?
There was a problem hiding this comment.
Return value is consumed by handle_cmis_inserted_state, keeping this form so early returns still give None
| return port_speed, lane_count, subport_num | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Nit: Remove this newline
@snider-nokia |
| lport, si_settings_status, e)) | ||
| return False | ||
|
|
||
| def can_skip_cmis_init_after_restart(self, lport, api, appl, host_lanes_mask): |
There was a problem hiding this comment.
This should already be solved, check it
| self.log_notice("{}: timeout waiting for SI settings to be applied (status: {}, expected: {})".format( | ||
| lport, si_settings_status, expected_notification_number)) | ||
| self.port_dict[lport].pop('si_notification_number', None) | ||
| self.port_dict[lport]['notify_si_settings'] = True | ||
| self.force_cmis_reinit(lport, retries + 1) |
There was a problem hiding this comment.
Will fix the PR description
| # Check if SI settings notification number was cached during INSERTED state | ||
| notification_number = self.port_dict[lport].get('si_notification_number') | ||
|
|
||
| if notification_number is not None: | ||
| self.log_notice("{}: SI settings notified to OA with number {}, waiting for ASIC to apply".format( |
There was a problem hiding this comment.
The cached si_notification_number is now authoritative at AP_CONF because it's recovered earlier: a restart always re-enters CMIS_STATE_INSERTED (via force_cmis_reinit), and AP_CONF is strictly downstream of INSERTED.
|
|
||
| if self.XCVR_TYPE in port_change_event.port_dict: | ||
| self.port_dict[lport][self.XCVR_TYPE] = port_change_event.port_dict[self.XCVR_TYPE] | ||
| self.port_dict[lport]['notify_si_settings'] = True |
There was a problem hiding this comment.
SFF publish guarded by is_si_settings_synced(); replayed TRANSCEIVER_INFO won't re-notify a synced module.
| self.log_notice("{}: timeout waiting for SI settings to be applied (status: {}, expected: {})".format( | ||
| lport, si_settings_status, expected_notification_number)) | ||
| self.port_dict[lport].pop('si_notification_number', None) | ||
| self.port_dict[lport]['notify_si_settings'] = True |
There was a problem hiding this comment.
Added the DEFAULT:<N+1> write via shared reset_si_settings_notification_to_default().
| return port_speed, lane_count, subport_num | ||
|
|
||
|
|
||
|
|
|
|
||
| if rc != SFP_EEPROM_NOT_READY: | ||
| self.dom_db_utils.post_port_dom_thresholds_to_db(logical_port) | ||
| self.vdm_db_utils.post_port_vdm_thresholds_to_db(logical_port) |
There was a problem hiding this comment.
Added notify_media_setting_on_insert() at all insert sites; publishes for non-CMIS independent of --enable_sff_mgr, no-ops for CMIS/synced.
…ync_master Signed-off-by: arpit-nexthop <arpit@nexthop.ai> # Conflicts: # sonic-xcvrd/tests/test_xcvrd.py # sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py # sonic-xcvrd/xcvrd/xcvrd.py
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
@mihirpat1 - please re-review if all comments are addressed? |
The CPO refactor (sonic-net#843) changed CmisManagerTask.__init__ to take port_obj_dict positionally and dropped the platform_chassis kwarg. Update the two SI-sync tests (notify-arming, SI_SETTINGS_WAIT) that still used the old signature. Signed-off-by: arpit-nexthop <arpit@nexthop.ai>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Cover the SI-sync helpers/paths previously only exercised through mocks: - check_si_sync_done_match: all branches - check_si_settings_ack_status: ack present / absent - notify_si_settings_unavailable early-return guards - is_si_settings_synced malformed notification/ack branches - notify_media_setting_on_insert CMIS / SFF-enabled / xcvr_info-None / api-error branches - handle_cmis_inserted_state restart recovery (already-synced skip, adopt-outstanding) Signed-off-by: arpit-nexthop <arpit@nexthop.ai>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
| prefix, count = ack.split(':', 1) | ||
| # OA acks either SI_SYNC_DONE:<N> (real SI applied) or SI_SETTINGS_DEFAULT:<N> | ||
| # (defaults applied); both mean notification <N> has been fully processed. | ||
| return prefix in ('SI_SYNC_DONE', 'SI_SETTINGS_DEFAULT') and int(count) == notified |
There was a problem hiding this comment.
High: SI_SETTINGS_DEFAULT:<N> is not proof that SI settings for the current module were applied
get_current_si_notification_number() discards the APPL_DB notification type and returns only N; this function then accepts either SI_SYNC_DONE:N or SI_SETTINGS_DEFAULT:N as synchronized. Those states have different meanings. DONE:N means OA applied a NOTIFIED:N request. DEFAULT:N is the removal/reset handshake; OA's DEFAULT path does not program new-module SerDes settings, it only echoes the DEFAULT ack.
A normal OIR sequence can therefore produce a false positive:
old module removed
APPL_DB = SI_SETTINGS_DEFAULT:8
STATE_DB = SI_SETTINGS_DEFAULT:8
new module inserted
is_si_settings_synced() returns True from matching number 8
SFF/direct path skips notification, or CMIS marks SI synced
new module's SI settings are never published/applied
There is a second ordering failure: if insertion observes DEFAULT:8 before OA has acknowledged it, CMIS treats the bare number as an outstanding SI notification and waits for SI_SYNC_DONE:8; OA will only answer SI_SETTINGS_DEFAULT:8, causing timeout/reinit instead of a fresh notification.
Shall we preserve and validate both type and counter? Restart-skip should require the exact pair APPL_DB = SI_SETTINGS_NOTIFIED:N and STATE_DB = SI_SYNC_DONE:N. Only NOTIFIED:N should be considered outstanding. DEFAULT:N must force a fresh SI decision on the next insertion, while UNAVAIL remains a separate terminal no-settings result. The tests should reject DEFAULT:N + DEFAULT:N as applied SI and cover removal followed immediately by insertion.
| api = None | ||
| # CMIS modules are handled by CmisManagerTask, and SFF modules by SffManagerTask | ||
| # when it is enabled; only publish here for a non-CMIS module with the SFF manager off. | ||
| if common.is_cmis_api(api) or self.enable_sff_mgr: |
There was a problem hiding this comment.
High regression: the new SI publisher split leaves valid CMIS and retry paths without a terminal notification
This is broader than the non-CMIS ownership issue discussed in #799 (comment). The added fallback covers non-CMIS modules when SFF manager is disabled, but the implementation still has paths for which no component owns SI publication.
Required invariant
For every present external port whose admin-up can be gated by OA, the insertion flow needs one responsible owner that eventually publishes one of these terminal results:
SI_SETTINGS_NOTIFIED:Nwhen settings must be applied.SI_SETTINGS_UNAVAILwhen there are no applicable/matching settings.
The current ownership split violates that invariant in several distinct cases:
- Flat-memory/non-paged CMIS: the direct
SfpStateUpdateTaskfallback returns for everycommon.is_cmis_api(api);SffManagerTaskalso skips every CMIS API; andCmisManagerTasksets flat-memory CMIS to READY and returns. Therefore none of the three paths publishesNOTIFIED:NorUNAVAIL. Flat-memory CMIS, including passive-copper CMIS, can still require platform-specific NPU SI based on cable length, lane speed, board trace, and ASIC. - CMIS with
--skip_cmis_mgr: the CMIS task does not exist, but the always-on fallback still skips all CMIS APIs. This is a supported configuration, not an error path, and it has no SI publisher. - Transient publisher-side information-read failure: if the second
get_transceiver_info()call below returnsNone, this path returns without adding the port back to the EEPROM retry set. The SFF and CMIS paths can similarly consume/clear their one-shotnotify_si_settingsflag before the publisher read succeeds. The initial EEPROM-ready retry does not guarantee a retry of this later read, so a temporary failure can become a permanent missing notification.
This is a behavioral regression. Before this PR, the always-on SfpStateUpdateTask directly called media_settings_parser.notify_media_setting() after successful normal EEPROM reads, including for flat-memory CMIS. The HLD defines a CMIS-state-machine-driven module as CMIS and not flat-memory; using is_cmis_api() as the ownership boundary is therefore too broad.
The resulting failure is:
external port is configured admin-up
-> OA defers NPU admin-up while waiting for SI
-> selected xcvrd path publishes neither NOTIFIED:N nor UNAVAIL
-> OA never releases the gate
-> the configured-up port remains down indefinitely
Shall we assign an explicit owner to every path: paged CMIS with the manager enabled can remain owned by CmisManagerTask; flat-memory CMIS and CMIS when the manager is intentionally skipped need the always-on/direct publisher. A successful decision must produce NOTIFIED:N or UNAVAIL, while transient read failures must rearm/retry rather than consume the event. Please also ensure paged CMIS has only one publisher.
Tests should cover flat-memory CMIS copper, flat-memory CMIS with no matching media entry, --skip_cmis_mgr, transient information-read failure followed by recovery, and no double-publication for paged CMIS.
| self.app_port_read_tbl = {} | ||
| # (asic_id, port_name) -> last SI notification number issued this process | ||
| # (keeps get_next_si_notification_number monotonic despite the PORT_TABLE read lag). | ||
| self.last_si_notification_number = {} |
There was a problem hiding this comment.
Medium: SI notification-number allocation is not serialized across xcvrd task owners
N is effectively a transaction ID. It lets OA say, "I processed exactly notification N," rather than acknowledging an older optic's settings.
Several threads can allocate it:
| Task | Notification |
|---|---|
SfpStateUpdateTask |
DEFAULT:N on removal |
CmisManagerTask |
NOTIFIED:N on insertion; DEFAULT:N on timeout |
SffManagerTask |
NOTIFIED:N on insertion |
Each task constructs a separate XcvrTableHelper, with its own last_si_notification_number cache. Allocation performs:
read current N from APPL_DB
next = max(database N, this helper's local N) + 1
update this helper's local cache
later publish the notification
Normal sequential processing is safe because the APPL_DB write is synchronous:
SfpStateUpdateTask processes removal and publishes DEFAULT:10
|
v
CmisManagerTask processes a later insertion and reads 10
|
v
publishes NOTIFIED:11
The problem occurs when processing for the same physical module overlaps. Here module A is current when CMIS processing begins and becomes the old/removed optic during the race:
1. Module A is inserted.
2. SfpStateUpdateTask processes SFP_STATUS_INSERTED and publishes A's TRANSCEIVER_INFO.
3. CmisManagerTask observes that update and starts processing CMIS_STATE_INSERTED for A.
4. CmisManagerTask reads APPL_DB N=10, allocates N=11, and prepares A's SI payload.
5. Module A is quickly removed before that payload is published.
6. SfpStateUpdateTask processes SFP_STATUS_REMOVED, also reads N=10 through its
separate helper, allocates N=11, and publishes DEFAULT:11.
7. CmisManagerTask's unfinished CMIS_STATE_INSERTED work for A resumes and publishes
A's stale SI payload as NOTIFIED:11.
8. Module B may subsequently be inserted into the same port.
The overlapping allocation looks like this:
APPL_DB currently has N=10
CmisManagerTask processing A's SfpStateUpdateTask processing A's
CMIS_STATE_INSERTED later SFP_STATUS_REMOVED event
-------------------------------- ----------------------------------
reads N=10
allocates N=11
prepares A's SI payload
reads N=10
allocates N=11
publishes DEFAULT:11
publishes stale NOTIFIED:11
|
v
Final APPL_DB = NOTIFIED:11
OA can then apply tuning captured for removed module A and acknowledge DONE:11. On module B's insertion or an xcvrd restart, retained APPL_DB = NOTIFIED:11 plus STATE_DB = DONE:11 looks perfectly synchronized, even though it belongs to the wrong module generation. This remains possible even after fixing the separate DEFAULT:N matching issue.
Could SI publication be made single-owner per port, or protected by a shared per-port lock/generation mechanism covering presence revalidation, number allocation, and the APPL_DB write? Atomic N allocation alone would guarantee unique numbers but would not stop stale work for module A from publishing after its removal. A barrier-based test that forces CMIS_STATE_INSERTED notification to overlap SFP_STATUS_REMOVED would make this behavior deterministic.
rebuild-source: sonic-net#799 @ nexthop-ai/sonic-platform-daemons d80bfce [case: upstream:open]
rebuild-source: sonic-net#799 @ nexthop-ai/sonic-platform-daemons d80bfce [case: upstream:open]
rebuild-source: sonic-net#799 @ nexthop-ai/sonic-platform-daemons d80bfce [case: upstream:open]
…sitive) is_si_settings_synced accepted SI_SETTINGS_DEFAULT:<N> as applied SI, so a remove->insert (OIR) with matching counters falsely reported synced and the new module's SI was never published. Make the notification type significant: - Synced requires APPL_DB SI_SETTINGS_NOTIFIED:<N> AND STATE_DB SI_SYNC_DONE:<N>. - Only SI_SETTINGS_NOTIFIED:<N> is 'outstanding' (get_current -> get_outstanding); DEFAULT:<N>/UNAVAIL force a fresh SI decision on the next insertion. - Add _parse_si_value helper; tests reject DEFAULT+DEFAULT and cover OIR. Addresses review comment on is_si_settings_synced (DEFAULT:N false positive). Signed-off-by: arpit-nexthop <arpit@nexthop.ai>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
notify_media_setting_on_insert used is_cmis_api() as the ownership boundary, which is too broad: flat-memory CMIS (incl. passive copper) and CMIS under --skip_cmis_mgr had no publisher (CmisManagerTask skips flat memory / does not run), so orchagent held admin-up forever waiting for an SI notification. Own here everything CmisManagerTask/SffManagerTask do not: publish unless the module is paged CMIS with the manager running, or non-CMIS with --enable_sff_mgr. Also re-arm the EEPROM retry on a transient get_transceiver_info() failure. Rework the test to cover paged/flat/skip_cmis_mgr ownership and the transient re-arm. Addresses review comment on the SI-publisher ownership split. Signed-off-by: arpit-nexthop <arpit@nexthop.ai>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
@anamehra @snider-nokia @mihirpat1 to review again + @arpit-nexthop can you get the sonic-mgmt testrun and share results PR description |
rebuild-source: sonic-net#799 @ nexthop-ai/sonic-platform-daemons e1989c0 [case: upstream:open]
rebuild-source: sonic-net#799 @ nexthop-ai/sonic-platform-daemons e1989c0 [case: upstream:open]
rebuild-source: sonic-net#799 @ nexthop-ai/sonic-platform-daemons e1989c0 [case: upstream:open]
rebuild-source: sonic-net#799 @ nexthop-ai/sonic-platform-daemons e1989c0 [case: upstream:open]
rebuild-source: sonic-net#799 @ nexthop-ai/sonic-platform-daemons e1989c0 [case: upstream:open]
rebuild-source: sonic-net#799 @ nexthop-ai/sonic-platform-daemons e1989c0 [case: upstream:open]
What I did
Implements sonic-net/SONiC#2301
Description
Implements the xcvrd side of a redesigned SI settings synchronization protocol between xcvrd and orchagent. The previous design used a single NPU_SI_SETTINGS_SYNC_STATUS field in STATE_DB with values NPU_SI_SETTINGS_DEFAULT / _NOTIFIED / _DONE, with xcvrd writing directly to STATE_DB. This is replaced by a two-field, counter-based protocol.
Changes across files:
xcvr_table_helper.py
xcvrd.py
media_settings_parser.py
cmis_manager_task.py
common.py
Motivation and Context
The old design had two problems:
How Has This Been Tested?
Unit tests added/updated in test_xcvrd.py:
Tests done on device:
Additional Information (Optional)