[xcvrd] Bootstrap the CMIS state machine for ports that miss their PORT_SET - #862
[xcvrd] Bootstrap the CMIS state machine for ports that miss their PORT_SET#862tahmed-dev wants to merge 2 commits into
Conversation
…RT_SET A port only enters the CMIS state machine once a PORT_SET event assigns it an initial state through force_cmis_reinit(). get_cmis_state_from_state_db() reports an absent cmis_state as UNKNOWN, and process_single_lport() returns immediately on UNKNOWN, so a port whose PORT_SET was never delivered is skipped on every iteration. Such a port is left completely unmanaged: no CMIS state is ever written, the transceiver is never queried and nothing is logged. This was observed on a port created by a dynamic port breakout, which had no cmis_state at all while every other port on the switch reported READY. Only restarting xcvrd recovered it. Bootstrap the state machine when a port has no CMIS state but a complete configuration, so a missed event no longer leaves the port unmanaged. Ports without a usable configuration are still skipped, and the validity check is shared with the existing one through is_port_config_complete(). Also always record a physical index in port_dict. on_port_update_event() only stored it when it was valid, while port_event_helper defaults a missing index to -1, so a port could be admitted without one and is_decomm_pending() and is_decomm_lead_lport() would raise KeyError when indexing into it. Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Bootstraps the CMIS state machine in sonic-xcvrd for logical ports that never received a PORT_SET, preventing them from remaining permanently unmanaged (no CMIS state written, no transceiver probing) until xcvrd restarts.
Changes:
- Add
is_port_config_complete()and use it to consistently gate CMIS processing and (when needed) bootstrap ports with missing CMIS state toINSERTED. - Ensure
port_dict[lport]['index']is always present (default-1) to avoidKeyErrorin decommission helpers. - Add unit tests covering config completeness, always-record-index behavior, bootstrapping behavior, and skip-on-incomplete-config behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py | Adds config completeness helper, bootstraps missing CMIS state, and always initializes index in port_dict. |
| sonic-xcvrd/tests/test_xcvrd.py | Adds targeted unit tests to validate the new bootstrap and config completeness behavior. |
Comments suppressed due to low confidence (1)
sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py:1308
process_single_lport()callsint()onindex/speed(and readslanes) before checkingis_port_config_complete(info). If any of these fields are malformed (e.g.'index': 'N/A'), this will raise and crash the polling loop instead of safely skipping the port.
pport = int(info.get('index', "-1"))
speed = int(info.get('speed', "0"))
lanes = info.get('lanes', "").strip()
subport = info.get('subport', 0)
if not self.is_port_config_complete(info):
| Boolean, true if the physical index, speed, lanes and subport are all | ||
| present and valid. |
| # UNKNOWN, and UNKNOWN is skipped below, the port would stay unmanaged until | ||
| # xcvrd is restarted. Bootstrap the state machine instead. | ||
| self.log_notice("{}: no CMIS state found, starting the CMIS state machine".format(lport)) | ||
| self.update_port_transceiver_status_table_sw_cmis_state(lport, CMIS_STATE_INSERTED) |
There was a problem hiding this comment.
@tahmed-dev are you saying the port update event was missed by Xcvrd during port breakout? There are important port attributes like lanes, speed, subport values that are obtained when new port is created(that how self.port_dict[]) without that even if CMIS_STATE_INSERTED is forced, the port cannot be reliably initialized(its working in one case doesn't invalidates the requirement). If Xcvrd is missing the event, how do we know that process_single_lport() is now operating on old port info versus newly created breakout port information?
update_port_transceiver_status_table_sw_cmis_state() passes the result of
get_asic_id() straight into get_status_sw_tbl(). get_asic_id() reports -1 for a
port that is not tracked in port_dict, and the per-ASIC tables are dicts keyed by
asic_id, so -1 is never a valid key and the lookup raises. The existing
"if status_table is None" guard below it is therefore unreachable for that case:
the exception is raised inside the getter, before the guard can run.
The exception propagates out of the CmisManagerTask thread, which terminates the
whole daemon. This was seen after a dynamic port breakout, where events for the
removed subports still referenced ports that had already left port_dict:
on_port_update_event -> force_cmis_reinit
-> update_port_transceiver_status_table_sw_cmis_state
-> get_status_tbl(asic_index)
KeyError
Exception occured at CmisManagerTask thread
Exiting main loop as child thread raised exception!
gave up: xcvrd entered FATAL state, too many start retries too quickly
Resolve the ASIC id before indexing so the existing guard becomes reachable and
an event for an untracked port is logged and skipped instead of taking xcvrd
down. Behaviour for tracked ports is unchanged.
Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py:1312
- process_single_lport() converts index/speed to int (and strips lanes) before calling is_port_config_complete(). That defeats the purpose of centralizing the validity check: if any of those fields are missing/invalid (e.g., non-numeric index/speed), ValueError/AttributeError can be raised before the helper gets a chance to return False and skip the port.
pport = int(info.get('index', "-1"))
speed = int(info.get('speed', "0"))
lanes = info.get('lanes', "").strip()
subport = info.get('subport', 0)
if not self.is_port_config_complete(info):
| asic_id = self.get_asic_id(lport) | ||
| status_table = self.xcvr_table_helper.get_status_sw_tbl(asic_id) if asic_id >= 0 else None | ||
| if status_table is None: | ||
| helper_logger.log_error("status_table is None while updating " | ||
| "sw CMIS state for lport {}".format(lport)) |
Why I did it
A port only enters the CMIS state machine once a
PORT_SETevent assigns it an initial state throughforce_cmis_reinit().get_cmis_state_from_state_db()reports an absentcmis_stateasUNKNOWN, andprocess_single_lport()returns immediately onUNKNOWN:So
UNKNOWNis treated as "already finished" rather than "not yet started". A port whosePORT_SETwas never delivered is skipped on every iteration and left completely unmanaged: no CMIS state is ever written, the transceiver is never queried, and nothing is logged. There is no recovery path short of restarting xcvrd.This was observed on a port created by a dynamic port breakout. It had no
cmis_statefield at all, while every other port on the switch reportedREADY. The transceiver stayed on the application it powered up with, and the link never came up. Restarting xcvrd fixed it, since the port was then present when the port map was built.Reproduced against the current code with a fully configured port that never received a
PORT_SET:cmis_statePORT_SETmissedPORT_SETdelivered (control)INSERTEDPORT_SETwithoutindexINSERTED, stuckKeyError('index')How I did it
Bootstrap the state machine in
process_single_lport()when a port has no CMIS state but a complete configuration, instead of skipping it. The port then follows the normalINSERTEDpath. This closes the window regardless of why the event was lost, and is a no-op for ports that already have a state.The completeness check is factored into
is_port_config_complete(), which also replaces the existing inline validity check so both use the same rule. Ports without a usable configuration, for example a port whose physical index is not known yet, are still skipped exactly as before.Also always record a physical index in
port_dict.on_port_update_event()only stored it whenpport >= 0, whileport_event_helperdefaults a missingindexfield to-1. A port could therefore be admitted toport_dictwithout anindexkey, andis_decomm_pending()andis_decomm_lead_lport()index into it directly, outside thetryblock inprocess_cmis_state_machine(). This part is defensive, as I did not establish a reachable path to thatKeyErrorin the current flow.How to verify it
Added:
test_CmisManagerTask_is_port_config_complete, a matrix over missing, invalid and non numericindex,speed,lanesandsubport.test_CmisManagerTask_on_port_update_event_always_records_index, covering aPORT_SETwith no index followed by one with a valid index.test_CmisManagerTask_process_single_lport_bootstraps_missing_cmis_state, which reproduces the reported case: a fully configured port with no CMIS state is now bootstrapped and its transceiver queried.test_CmisManagerTask_process_single_lport_skips_incomplete_config, which confirms a port without a usable configuration is still skipped and its transceiver never touched.On hardware, create a port through a dynamic port breakout and confirm
cmis_stateappears inTRANSCEIVER_STATUSwithout restarting xcvrd.Description for the changelog
Bootstrap the CMIS state machine for ports that never received a PORT_SET event, so they are not left unmanaged until xcvrd restarts.