[xcvrd] Verify that the module activated the requested CMIS application - #861
[xcvrd] Verify that the module activated the requested CMIS application#861tahmed-dev wants to merge 1 commit into
Conversation
is_cmis_application_update_required() compares the desired application against get_application(), which reads the staged control set. The staged value reflects what xcvrd requested rather than what the module is running, so a module that accepts an application, reports ConfigSuccess and activates its datapath while leaving the Active Control Set unchanged is treated as correctly configured. When that happens the port is declared READY while the module keeps running the previous application on the media side. The link never comes up and neither the logs nor TRANSCEIVER_STATUS indicate why, since every value xcvrd inspects looks healthy. Compare the active application select code against the desired application before skipping an update and before transitioning to READY. A mismatch now forces a datapath reinitialization and, once CMIS_MAX_RETRIES is exhausted, the port transitions to FAILED instead of silently running the wrong application. The check is skipped when the active application code is not readable, so modules that do not report it keep the previous behaviour. Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
|
/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). |
There was a problem hiding this comment.
Pull request overview
This PR strengthens xcvrd’s CMIS datapath state machine by verifying that a CMIS module actually activates the requested application (Active Control Set), not just stages it (staged control set). This prevents ports from being marked READY when the module silently remains on a different media-side application, which can otherwise lead to links that never come up with little diagnostic signal.
Changes:
- Add
is_active_apsel_matching_desired()to compare the desired application code againstget_active_apsel_hostlane()for masked host lanes. - Extend
is_cmis_application_update_required()to force an application update when staged state and datapath health look good but the active application does not match. - Gate the
CMIS_STATE_DP_ACTIVATE→CMIS_STATE_READYtransition on the active application matching the desired one, otherwise trigger the existing reinit/retry path. - Add unit tests covering matching/mismatching/partial mismatching active appsel and the “staged matches but active mismatches” scenario.
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 active-vs-desired CMIS app verification and uses it to prevent premature READY and to force updates when needed. |
| sonic-xcvrd/tests/test_xcvrd.py | Adds unit tests for the new active appsel verification and for forcing update when active appsel mismatches. |
| active_apsel = api.get_active_apsel_hostlane() | ||
| if not active_apsel: | ||
| return True | ||
|
|
||
| for lane in range(self.CMIS_MAX_HOST_LANES): | ||
| if ((1 << lane) & host_lanes_mask) == 0: | ||
| continue | ||
| active = active_apsel.get("ActiveAppSelLane{}".format(lane + 1)) | ||
| # The active application code is not always readable, e.g. on flat | ||
| # memory modules it is reported as 'N/A'. Skip the check instead of | ||
| # forcing a needless datapath reinitialization. | ||
| if not isinstance(active, int): | ||
| return True | ||
| if active != appl: | ||
| self.log_error("Active application {} on host lane {} does not match " | ||
| "the desired application {}".format(active, lane + 1, appl)) | ||
| return False | ||
|
|
||
| return True |
|
Closing this — the premise doesn't hold. I opened it after finding a module reporting active application 4 while xcvrd had staged application 12, and concluded the port could never come up. That was wrong. The link in question is now up and passing traffic at 100G with exactly that staged/active mismatch still present. The actual fault was unrelated to CMIS: the switch port was cabled to a different host interface that was administratively down. So a staged/active application mismatch is not on its own evidence of a broken datapath. Worse, this change would be actively harmful in that situation: it would force repeated datapath reinitialisation on a working port and drive it to If there is still an appetite for surfacing a staged/active divergence, it should be a log or a state field rather than something that triggers reconfiguration — but I don't have a case that justifies it today. Apologies for the noise. #862 is unaffected and stands on its own reproduction. |
Why I did it
is_cmis_application_update_required()compares the desired application againstget_application(), which reads the staged control set. The staged value reflects what xcvrd requested, not what the module is actually running.A module can accept a staged application, return success from
ApplyDataPathInit, reportConfigSuccesson every lane and activate its datapath, while leaving the Active Control Set unchanged. xcvrd then sees a staged application equal to the desired one plus a healthy datapath, logsno CMIS application update required...READY, and marks the port READY.The module keeps running its previous application on the media side, so the link never comes up. Nothing in the logs or in
TRANSCEIVER_STATUSindicates why, because every value xcvrd inspects looks correct. The only sign is thatactive_apsel_hostlane*inTRANSCEIVER_INFOdiffers from the application xcvrd asked for.This was hit on a 100G port where xcvrd correctly selected the
CAUI-4 C2M/100G PSM4application, but the module stayed on a400GBASE-DR4application. The optical power was healthy in both directions while the media side ran the wrong modulation, so the port reported READY and never linked.How I did it
Added
is_active_apsel_matching_desired(), which compares the desired application againstget_active_apsel_hostlane()(the Active Control Set) for every host lane in the mask.It is used in two places:
is_cmis_application_update_required(): when the staged application matches and the datapath looks healthy, the active application is checked before the update is skipped. This covers xcvrd restarts, where the CMIS state is preserved.CMIS_STATE_DP_ACTIVATEtoCMIS_STATE_READYtransition: the active application is verified before the port is declared READY.On mismatch the datapath is reinitialized through the existing retry path. Once
CMIS_MAX_RETRIESis exhausted the port transitions toCMIS_STATE_FAILED, which surfaces the problem instead of leaving the port silently on the wrong application.The check is skipped when the active application code is not readable, for example the
'N/A'reported for flat memory modules, so modules that do not expose it keep the current behaviour. On a compliant module the staged and active codes match after activation, so this is a no-op.How to verify it
Unit tests:
Added
test_CmisManagerTask_is_active_apsel_matching_desired, covering a matching active application, a full mismatch, a partial per-lane mismatch, lanes outside the host lane mask, an unreadable'N/A'active code, and an empty result. Addedtest_CmisManagerTask_is_cmis_application_update_required_active_apsel_mismatch, which reproduces the reported case: staged application matches,DataPathActivatedplusConfigSuccesson all lanes, active application different, update still required.On hardware, compare
active_apsel_hostlane*inTRANSCEIVER_INFOagainst the application xcvrd selected. A module that fails to activate it now retries and ends inCMIS_STATE_FAILEDwith an explicit log, instead of reporting READY.Description for the changelog
Detect CMIS modules that fail to activate the requested application instead of reporting the port as READY.