Update Media-based-Port-settings.md to support custom SerDes attributes#2173
Conversation
|
/azp run |
|
No pipelines are associated with this pull request. |
Signed-off-by: Longyin Huang <longhuan@cisco.com>
b0f617c to
8cb1b04
Compare
|
/azp run |
|
No pipelines are associated with this pull request. |
|
@prgeor Please review, thanks |
|
|
||
| ``` | ||
| { | ||
| "GLOBAL_MEDIA_SETTINGS": { |
There was a problem hiding this comment.
@longhuan-cisco can you introduce "CUSTOM_MEDIA_SETTINGS" so that we can have a separate parser for custom attributes. I know this will mandate to do the port range and per port attribute parsing again for custom attribute but the decision to introduce custom attributes deviates from general sonic requirements so ok to introduce this new section. This will help to contain the changes to this feature only.
There was a problem hiding this comment.
@prgeor Thanks for the review and for raising the isolation / blast-radius concern — I agree we should keep the risk to existing platforms/vendors as low as possible, especially for those that don’t define any CUSTOM:XXX attributes in media_settings.json.
I considered the suggestion of introducing a dedicated CUSTOM_MEDIA_SETTINGS section. My main concern is that we’d either need to (a) replicate the existing parsing/matching behaviors (port ranges, DEFAULT_KEY, vendor+PN matching, regex key matching, media_lane_speed_key, etc.), which could become harder to maintain over time, or (b) do a larger refactor to share that logic, which may increase risk more than necessary for this feature.
As a small, low-impact compromise, I propose adding an early-return guard inside handle_custom_serdes_attrs():
- If the resolved
media_dicthas noCUSTOM:keys, return immediately (nodeepcopy, no conversion logic), so the legacy behavior stays unchanged for platforms/vendors that don’t opt in viaCUSTOM:keys. - If
CUSTOM:keys exist, proceed exactly as before for the opt-in case.
I updated sonic-net/sonic-platform-daemons#643 as the same, please refer:
https://github.com/longhuan-cisco/sonic-platform-daemons/blob/27dfe58d4668d678b2ad850b076194ca17891b76/sonic-xcvrd/xcvrd/xcvrd_utilities/media_settings_parser.py#L340-L358
def handle_custom_serdes_attrs(media_dict, lane_count, subport_num):
"""
Handle custom SerDes attributes in the media_dict and convert them to JSON.
Args:
media_dict: dictionary containing SerDes settings for all lanes of the port
lane_count: number of lanes for this subport
subport_num: subport number (1-based), 0 for non-breakout case
Returns:
media_dict: updated media_dict with custom SerDes attributes converted to JSON,
or the original media_dict unchanged if no custom attributes present
"""
# Keep the custom-SerDes-attributes feature fully opt-in.
# If there are no "CUSTOM:" keys in the resolved media_dict, do not run any of the
# new conversion logic (and avoid deepcopy). This minimizes blast radius for
# existing platforms/vendors that don't define custom SerDes attributes.
if not any(isinstance(k, str) and k.startswith(CUSTOM_SERDES_ATTR_PREFIX) for k in media_dict):
return media_dict
If you’d still prefer an even stricter opt-in boundary, one option would be to do a one-time scan of media_settings.json (e.g., look for the substring "CUSTOM:") and only enable/invoke the custom handling when that marker exists in the file at all.
There was a problem hiding this comment.
@prgeor Let me know if this early-return guard addresses the isolation requirement. I’m happy to adjust further if you’d prefer an even stricter opt-in mechanism (e.g., a one-time scan of media_settings.json to detect "CUSTOM:" before enabling the custom handling), or if you still feel a dedicated CUSTOM_MEDIA_SETTINGS section is the better direction.
There was a problem hiding this comment.
@longhuan-cisco there is hardly 24 lines to duplicate which is already happening (between the parsing of GLOBAL_MEDIA_SETTINGS_KEY and PORT_MEDIA_SETTINGS_KEY) but you can use oops principal if you desire to minimize the duplication.
There was a problem hiding this comment.
Sure, I've implemented the dedicated CUSTOM_MEDIA_SETTINGS section as requested at sonic-net/sonic-platform-daemons#643. Please let me know if it looks ok. Thanks.
One thing to flag from the dedicated-CUSTOM_MEDIA_SETTINGS approach: the dedicated section adds a new selector/lookup path (is_port_selected,get_custom_media_settings_value) and an extra merge path in notify_media_setting. The earlier approach only added a single post‑processing helper (handle_custom_serdes_attrs) and reused the existing GLOBAL/PORT selection path, so it touched less of the parser.
- old approach (no dedicated section) code reference
- Adds handle_custom_serdes_attrs()
- Single hook in notify_media_setting()
- Reuses existing GLOBAL/PORT lookup path; no new selector logic
- Current approach (dedicated CUSTOM_MEDIA_SETTINGS) code reference
- Adds new path: get_custom_media_settings_value()
- Adds selector parser: is_port_selected()
- Adds get_custom_serdes_attrs()
- Extra control flow in notify_media_setting() to merge custom JSON
- Extracts get_media_settings_for_key() to reuse matching logic
I’m totally good sticking with the dedicated section if separation is the priority, but wanted to make the added maintenance surface and more touching of the existing code explicit now that we have code to compare.
Signed-off-by: Longyin Huang <longhuan@cisco.com>
|
/azp run |
|
No pipelines are associated with this pull request. |
…es (#2173) * Add custom SerDes attributes support in media_settings.json Signed-off-by: Longyin Huang <longhuan@cisco.com> * docs: specify dedicated CUSTOM_MEDIA_SETTINGS for custom attrs Signed-off-by: Longyin Huang <longhuan@cisco.com> --------- Signed-off-by: Longyin Huang <longhuan@cisco.com> Signed-off-by: Eddie Ruan <eddie.ruan@alibaba-inc.com>
sonic-swss side change: sonic-net/sonic-swss#3764 (merged)
sonic-platform-daemons side change: sonic-net/sonic-platform-daemons#643 (under review)
Add custom SerDes attributes (SAI_PORT_SERDES_ATTR_CUSTOM_COLLECTION) support in media_settings.json