-
Notifications
You must be signed in to change notification settings - Fork 237
[xcvr]: Detect coherent modules via CoherentPagesSupported bit #720
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -965,13 +965,103 @@ def test_get_module_media_interface(self, mock_response1, mock_response2, expect | |
| @pytest.mark.parametrize("mock_response, expected", [ | ||
| ('Copper cable', False), | ||
| ('400ZR', True), | ||
| # FOIC-named media interfaces (e.g. 800G-ZR+ FOIC, no 'ZR' substring) | ||
| # are coherent too; only reachable when CoherentPagesSupported is | ||
| # unavailable (mocked read() below defaults to None). | ||
| ('FOIC1.4-DO (G.709.3/Y.1331.3)', True), | ||
| ]) | ||
| def test_is_coherent_module(self, mock_response, expected): | ||
| self.clear_cache('is_coherent_module') | ||
| # CoherentPagesSupported unavailable: force the string-matching | ||
| # fallback path regardless of what earlier tests left behind on the | ||
| # shared self.api.xcvr_eeprom mock. | ||
| self.api.xcvr_eeprom.read = MagicMock(return_value=None) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On a live system, I think the read returns
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed, and that's exactly the regression this exposed — XcvrEeprom.read() only returns None on an actual EEPROM/I2C failure, not based on CMIS revision, so a real CMIS 5.2-or-earlier module would read the reserved bit as a genuine 0 and short-circuit straight to False, skipping the fallback and breaking detection for currently-supported coherent modules. Fixed by requiring CMIS 5.3+ before trusting the bit (see reply on the other comment). Added test cases: CMIS 5.2 module with the reserved bit read as 0 and as 1 (both now correctly fall through to string matching), plus unreadable CMIS revision fields. |
||
| self.api.get_module_media_interface = MagicMock() | ||
| self.api.get_module_media_interface.return_value = mock_response | ||
| result = self.api.is_coherent_module() | ||
| assert result == expected | ||
|
|
||
| @pytest.mark.parametrize("mock_response, expected", [ | ||
| (1, True), | ||
| (0, False), | ||
| ]) | ||
| def test_is_coherent_module_coherent_pages_bit(self, mock_response, expected): | ||
| # On CMIS 5.3+, CoherentPagesSupported is advertised and takes | ||
| # precedence over the media interface name (which is deliberately | ||
| # left un-mocked / not matching 'ZR' or 'FOIC', to prove the bit | ||
| # alone decides this). | ||
| self.clear_cache('is_coherent_module') | ||
| self.api.get_module_media_interface = MagicMock(return_value='Copper cable') | ||
| def mock_read(field): | ||
| if field == consts.CMIS_MAJOR_REVISION: | ||
| return 5 | ||
| if field == consts.CMIS_MINOR_REVISION: | ||
| return 3 | ||
| if field == consts.COHERENT_PAGES_SUPPORTED: | ||
| return mock_response | ||
| return None | ||
| self.api.xcvr_eeprom.read = MagicMock(side_effect=mock_read) | ||
| result = self.api.is_coherent_module() | ||
| assert result == expected | ||
|
|
||
| @pytest.mark.parametrize("cmis_minor, mintf, expected", [ | ||
| (2, '400ZR', True), | ||
| (2, 'Copper cable', False), | ||
| ]) | ||
| def test_is_coherent_module_ignores_reserved_bit_pre_5_3(self, cmis_minor, mintf, expected): | ||
| # Byte 142 bit 4 is Reserved prior to CMIS 5.3 (OIF-CMIS-05.2 Table | ||
| # 8-41). A pre-5.3 module may report this bit as 0 (the spec's | ||
| # convention for reserved bits) or 1 (not spec-guaranteed, but not | ||
| # excluded either) while still being a real coherent module - the | ||
| # bit must never override the media interface name check for | ||
| # modules that predate the bit's definition. | ||
| self.clear_cache('is_coherent_module') | ||
| self.api.get_module_media_interface = MagicMock(return_value=mintf) | ||
| def mock_read(field): | ||
| if field == consts.CMIS_MAJOR_REVISION: | ||
| return 5 | ||
| if field == consts.CMIS_MINOR_REVISION: | ||
| return cmis_minor | ||
| if field == consts.COHERENT_PAGES_SUPPORTED: | ||
| return 0 | ||
| return None | ||
| self.api.xcvr_eeprom.read = MagicMock(side_effect=mock_read) | ||
| result = self.api.is_coherent_module() | ||
| assert result == expected | ||
|
|
||
| def test_is_coherent_module_unknown_cmis_revision_falls_back(self): | ||
| # CmisMajorRevision/CmisMinorRevision reads failing (None) must not | ||
| # crash the (major, minor) >= (5, 3) comparison; behave as if the | ||
| # bit is untrustworthy and use the string-matching fallback. | ||
| self.clear_cache('is_coherent_module') | ||
| self.api.get_module_media_interface = MagicMock(return_value='400ZR') | ||
| self.api.xcvr_eeprom.read = MagicMock(return_value=None) | ||
| result = self.api.is_coherent_module() | ||
| assert result is True | ||
|
|
||
| @pytest.mark.parametrize("mintf", [ | ||
| '400ZR', | ||
| 'FOIC1.4-DO (G.709.3/Y.1331.3)', | ||
| ]) | ||
| def test_is_coherent_module_name_match_overrides_cleared_bit(self, mintf): | ||
| # A CMIS 5.3+ coherent module that mis-advertises | ||
| # CoherentPagesSupported as 0 while still naming a coherent media | ||
| # interface must stay coherent: the bit can only add detection, it | ||
| # must never drop a module the name match already covers. This | ||
| # guarantees no regression vs. the pre-bit ('ZR'/'FOIC') behavior. | ||
| self.clear_cache('is_coherent_module') | ||
| self.api.get_module_media_interface = MagicMock(return_value=mintf) | ||
| def mock_read(field): | ||
| if field == consts.CMIS_MAJOR_REVISION: | ||
| return 5 | ||
| if field == consts.CMIS_MINOR_REVISION: | ||
| return 3 | ||
| if field == consts.COHERENT_PAGES_SUPPORTED: | ||
| return 0 | ||
| return None | ||
| self.api.xcvr_eeprom.read = MagicMock(side_effect=mock_read) | ||
| assert self.api.is_coherent_module() is True | ||
|
|
||
| @pytest.mark.parametrize("mock_response1, mock_response2, expected", [ | ||
| (True, '1', 0 ), | ||
| (False, None, 0), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can just use
get_cmis_revhere?Also, being pedantic here, but if a current SONiC release runs a CMIS 5.3+ coherent module that does not correctly advertise
COHERENT_PAGES_SUPPORTEDbit, such a module would be detected as coherent before, but will not be detected as coherent after this change.We could claim though, that in that case, that the module is not CMIS-compliant due to the mis-advertisement. It is a change of behavior though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right that
returning
bool(CoherentPagesSupported)directly on 5.3+ was a regression: acoherent module that reports the bit as
0while still naming aZR/FOICmedia interface would flip from coherent to non-coherent.
Reworked
is_coherent_module()so detection is the union of the two signalsrather than the bit overriding the name. The name match returns
Trueon its own (same as before the bit existed), and the bit now only adds coherent modules whose name contains neither keyword.It can no longer drop a module the name match already covered, so the behavior
you flagged is gone. Added a test for a 5.3+ module reporting the bit as
0with a coherent media interface name.
get_cmis_rev. I looked at reusing it, but it returns a formatted string(
"5.3", or"None.None"when the read fails), so it can't feed the numeric(major, minor) >= (5, 3)comparison without parsing it back — and alexicographic version compare would be unsafe (
"5.10" < "5.3"). I kept the twoexplicit int reads for that reason. Happy to switch to a small numeric helper
if you'd prefer that instead.