Improve parsing of media-settings.json for non-CMIS and breakout ports #533
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
longhuan-cisco
changed the title
longhuan-cisco
changed the title
|
@prgeor @mihirpat1 Could you please review? |
AnoopKamath
reviewed
Aug 23, 2024
AnoopKamath
approved these changes
Aug 30, 2024
mihirpat1
reviewed
Sep 7, 2024
mihirpat1
approved these changes
Sep 8, 2024
|
@liat-grozovik can you assign reviewer ? |
prgeor
reviewed
Sep 12, 2024
prgeor
reviewed
Sep 12, 2024
prgeor
reviewed
Sep 12, 2024
There was a problem hiding this comment.
@longhuan-cisco @AnoopKamath could you please attach the test result on Cisco 8111 ?
prgeor
changed the title
@prgeor : DPB feature is currently supported on 8102 only and will be extended to all other platforms soon. |
prgeor
reviewed
Sep 18, 2024
| if app_id and app_id in appl_adv_dict: | ||
| host_electrical_interface_id = appl_adv_dict[app_id].get('host_electrical_interface_id') | ||
| if host_electrical_interface_id: | ||
| lane_speed_key = LANE_SPEED_KEY_PREFIX + host_electrical_interface_id.split()[0] | ||
|
|
||
| if not lane_speed_key: |
There was a problem hiding this comment.
@longhuan-cisco would suggest to use else of if xcvrd.is_cmis_api(api) so that we know this is a non-CMIS case
There was a problem hiding this comment.
Originally I used else of if xcvrd.is_cmis_api(api)
if not lane_speed_key:
lane_speed_key = '{}{}G'.format(LANE_SPEED_KEY_PREFIX, port_speed // lane_count // 1000)
But the above code can make sure lane_speed_key always gets generated even if some CMIS module somehow doesn't have host_electrical_interface_id (due to bad/corrupted eeprom, etc). It has better survivability/robustness
I'm ok to change it back to else of if xcvrd.is_cmis_api(api), and print a error message for the case of No host_electrical_interface_id found for this CMIS module to construct lane_speed_key
prgeor
reviewed
Sep 18, 2024
There was a problem hiding this comment.
@longhuan-cisco @AnoopKamath can you please update the HLD:- https://github.com/sonic-net/SONiC/blob/master/doc/media-settings/Media-based-Port-settings.md
prgeor
reviewed
Sep 18, 2024
| @@ -17,6 +18,9 @@ | |||
| VENDOR_KEY = 'vendor_key' | |||
| MEDIA_KEY = 'media_key' | |||
| LANE_SPEED_KEY = 'lane_speed_key' | |||
| DEFAULT_KEY = 'Default' | |||
| # This is useful if default value is desired when no match is found for lane speed key | |||
There was a problem hiding this comment.
@longhuan-cisco not able to understand. Can you please explain with an example. Can you capture these in the HLD:- https://github.com/sonic-net/SONiC/blob/master/doc/media-settings/Media-based-Port-settings.md
There was a problem hiding this comment.
Below is an example mentioned in PR description:
Example:
{
'GLOBAL_MEDIA_SETTINGS': {
'0-31': {
'Default': {
'speed:400GAUI-8': {'idriver': {'lane0': '0x1a', ...}, ...},
'speed:Default': {'idriver': {'lane0': '0x1c', ...}, ...},
}
},
}
}
In this example, the speed key entry 'speed:400GAUI-8') covers lane_speed 50G, the 'speed:Default' entry is handy to cover all the reset of lane_speeds (10G, 25G, 100G ...)
yea, will raise PR for the HLD update.
tshalvi
reviewed
Sep 19, 2024
| # For 100G transceivers, it's usually in extended specification compliance | ||
| if media_compliance_code in sup_compliance_extended_values and \ | ||
| extended_spec_compliance_str in media_compliance_dict: | ||
| media_compliance_code = media_compliance_dict[extended_spec_compliance_str] |
There was a problem hiding this comment.
Could you provide an example for the media_key generated for a module with the following entry (from the PR description):
Extended Specification Compliance: 100GBASE-CR4, 25GBASE-CR CA-25G-L or 50GBASE-CR2 with RS
Will the media_key include all three options listed here, or only the one relevant to the module's speed and lane count? If the entire string is included, the key might end up very long.
There was a problem hiding this comment.
example:
'media_key': 'QSFP28-100GBASE-CR4, 25GBASE-CR CA-25G-L or 50GBASE-CR2 with RS-1.0M',
entire string will be included, we just take the entire extended_spec_compliance as the first part of media_key, so that we can keep original information as much as possible in the key, and based on which platform has the max flexibility to define its matching pattern in media_settings.json. The pattern can be shorter (e.g. just prefix QSFP28-100GBASE-CR4) or in regular expression (e.g. QSFP28-100GBASE-CR4.*-1.0M)
Junchao-Mellanox
approved these changes
Sep 23, 2024
|
@prgeor |
|
tested on 8101-O32 with 400G AOC: |
prgeor
approved these changes
Oct 1, 2024
mssonicbld
pushed a commit
to mssonicbld/sonic-platform-daemons
that referenced
this pull request
Oct 1, 2024
sonic-net#533) * Enhance media_settings_parser for 100G xcvr and DPB etc * Revert space change * Cover corner cases * Change log message level * Fix docstring and update name of get_speed_lane_count_and_subport * Address comment * Change to re.fullmatch for lane_speed key
|
Cherry-pick PR to 202405: #543 |
mssonicbld
pushed a commit
that referenced
this pull request
Oct 1, 2024
#533) * Enhance media_settings_parser for 100G xcvr and DPB etc * Revert space change * Cover corner cases * Change log message level * Fix docstring and update name of get_speed_lane_count_and_subport * Address comment * Change to re.fullmatch for lane_speed key
mssonicbld
added
Included in 202405 Branch
and removed
Created PR to 202405 Branch
labels
longhuan-cisco
added a commit
to longhuan-cisco/sonic-platform-daemons
that referenced
this pull request
Oct 3, 2024
sonic-net#533) * Enhance media_settings_parser for 100G xcvr and DPB etc * Revert space change * Cover corner cases * Change log message level * Fix docstring and update name of get_speed_lane_count_and_subport * Address comment * Change to re.fullmatch for lane_speed key
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Enhance/fix media_settings infra in below aspects:
Support/fix for 100G QSFP28 transceivers:
media_keyparsed asQSFP28-Unknown-...due to its compliance code defined inExtended Specification Compliancefield rather than10/40G Ethernet Compliance Codelane_speed_keyparsed asNonedue to QSFP28 having noApplication Advertisement(which is CMIS specific field and containshost_electrical_interface_idused by today's logic as speed key )Support/fix for DPB situations:
Add regular expression support for
lane_speed_key, so that multiple lane speed keys can be grouped together if they share the same lane speed value or same serdes SI values, e.g.speed:200GAUI-8|100GAUI-4|50GAUI-2|25GAdd
lane_speed_keysupport underDefaultvendor/media key. Also add support forspeed:Default, which is useful if default serdes SI setting value is desired when no match is found for available lane speed keys.Improved code coverage of media_settings_parser.py to 97%
Motivation and Context
This PR is mainly to make sure media_settings infra can work properly for 100G QSFP28 and DPB cases/etc
How Has This Been Tested?
Verified proper settings got notified with different transceivers under both DPB and non-DPB cases
Verified compatibility with existing media_settings.json
Additional Information (Optional)