-
Notifications
You must be signed in to change notification settings - Fork 156
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
Improve parsing of media-settings.json for non-CMIS and breakout ports #533
Improve parsing of media-settings.json for non-CMIS and breakout ports #533
Conversation
@prgeor @mihirpat1 Could you please review? |
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.
Approved with minor comments
@liat-grozovik can you assign reviewer ? |
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.
@longhuan-cisco @AnoopKamath could you please attach the test result on Cisco 8111 ?
@prgeor : DPB feature is currently supported on 8102 only and will be extended to all other platforms soon. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
@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
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@prgeor
|
tested on 8101-O32 with 400G AOC:
|
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 |
#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
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
Description
Enhance/fix media_settings infra in below aspects:
Support/fix for 100G QSFP28 transceivers:
media_key
parsed asQSFP28-Unknown-...
due to its compliance code defined inExtended Specification Compliance
field rather than10/40G Ethernet Compliance Code
lane_speed_key
parsed asNone
due to QSFP28 having noApplication Advertisement
(which is CMIS specific field and containshost_electrical_interface_id
used 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|25G
Add
lane_speed_key
support underDefault
vendor/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)