Skip to content
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.

Sign up for GitHub

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

[xcvrd] Handle platfom_chassis None scenario to protect xcvrd from crash #411

Closed
wants to merge 2 commits into from
Closed

[xcvrd] Handle platfom_chassis None scenario to protect xcvrd from crash #411

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ea9f2e
Added functionality to media_settings_parser to handle the case where…
tshalvi Dec 4, 2023
7719fb8
Added error log messages in case platform_chassis is None
tshalvi Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 32 additions & 24 deletions sonic-xcvrd/xcvrd/xcvrd_utilities/media_settings_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,17 +40,20 @@ def media_settings_present():


def get_lane_speed_key(physical_port, port_speed, lane_count):
sfp = xcvrd.platform_chassis.get_sfp(physical_port)
api = sfp.get_xcvr_api()

lane_speed_key = None
if xcvrd.is_cmis_api(api):
appl_adv_dict = api.get_application_advertisement()
app_id = xcvrd.get_cmis_application_desired(api, int(lane_count), int(port_speed))
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 xcvrd.platform_chassis is not None:
sfp = xcvrd.platform_chassis.get_sfp(physical_port)
api = sfp.get_xcvr_api()
if xcvrd.is_cmis_api(api):
appl_adv_dict = api.get_application_advertisement()
app_id = xcvrd.get_cmis_application_desired(api, int(lane_count), int(port_speed))
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]
else:
helper_logger.log_error("Unable to derive lane_speed_key because platform_chassis is None")

return lane_speed_key

Expand All @@ -73,14 +76,17 @@ def get_media_settings_key(physical_port, transceiver_dict, port_speed, lane_cou
media_compliance_dict = {}

try:
sfp = xcvrd.platform_chassis.get_sfp(physical_port)
api = sfp.get_xcvr_api()
if xcvrd.is_cmis_api(api):
media_compliance_code = media_compliance_dict_str
if xcvrd.platform_chassis is not None:
sfp = xcvrd.platform_chassis.get_sfp(physical_port)
api = sfp.get_xcvr_api()
if xcvrd.is_cmis_api(api):
media_compliance_code = media_compliance_dict_str
else:
media_compliance_dict = ast.literal_eval(media_compliance_dict_str)
if sup_compliance_str in media_compliance_dict:
media_compliance_code = media_compliance_dict[sup_compliance_str]
else:
media_compliance_dict = ast.literal_eval(media_compliance_dict_str)
if sup_compliance_str in media_compliance_dict:
media_compliance_code = media_compliance_dict[sup_compliance_str]
helper_logger.log_error("Unable to derive media_key.media_compliance_code because platform_chassis is None")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tshalvi when can this happen? How does it recover later?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This scenario was observed only once, when trying to delete a table as part of another PR, which has since been reverted and is no longer relevant. Nevertheless, I still recommend implementing this fix as a precautionary measure to mitigate any unforeseen issues.
Regarding the recovery process, the key for the lookup in media_settings.json will simply include an empty string for the specification_compliance component, resulting in empty data for the JSON lookup. Instead of experiencing a crash in such a scenario, xcvrd will just end up not posting anything to APP_DB in notify_media_settings().

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tshalvi i think we need to understand the issue better. i don't see platform_chassis to be None since platforms have transition to using chassis APIs
platform_chassis = sonic_platform.platform.Platform().get_chassis()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what can make platform_chassis to be None, and it did only reproduce once on our side with some irrelevant piece of code, so if you think we should ignore this fix, let me know. It's worth noting that there are many places in xcvrd code where this validation is performed, though:
https://github.com/sonic-net/sonic-platform-daemons/blob/b2b890540d291d76b8763c1f6c22b3899385ff76/sonic-xcvrd/xcvrd/xcvrd.py#L226C1-L226C37
https://github.com/sonic-net/sonic-platform-daemons/blob/b2b890540d291d76b8763c1f6c22b3899385ff76/sonic-xcvrd/xcvrd/xcvrd.py#L235C1-L235C37
https://github.com/sonic-net/sonic-platform-daemons/blob/b2b890540d291d76b8763c1f6c22b3899385ff76/sonic-xcvrd/xcvrd/xcvrd.py#L244C1-L244C37
https://github.com/sonic-net/sonic-platform-daemons/blob/b2b890540d291d76b8763c1f6c22b3899385ff76/sonic-xcvrd/xcvrd/xcvrd.py#L280C1-L280C37

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tshalvi All platforms are supposed to use API 2.0. The wrapper needs to be clean up. Xcvrd should not run if platform_chassis is None

except ValueError as e:
helper_logger.log_error("Invalid value for port {} 'specification_compliance': {}".format(physical_port, media_compliance_dict_str))

Expand All @@ -90,20 +96,22 @@ def get_media_settings_key(physical_port, transceiver_dict, port_speed, lane_cou
media_key += media_type
if len(media_compliance_code) != 0:
media_key += '-' + media_compliance_code
sfp = xcvrd.platform_chassis.get_sfp(physical_port)
api = sfp.get_xcvr_api()
if xcvrd.is_cmis_api(api):
if media_compliance_code == "passive_copper_media_interface":
if xcvrd.platform_chassis is not None:
sfp = xcvrd.platform_chassis.get_sfp(physical_port)
api = sfp.get_xcvr_api()
if xcvrd.is_cmis_api(api):
if media_compliance_code == "passive_copper_media_interface":
if media_len != 0:
media_key += '-' + str(media_len) + 'M'
else:
if media_len != 0:
media_key += '-' + str(media_len) + 'M'
else:
if media_len != 0:
media_key += '-' + str(media_len) + 'M'
helper_logger.log_error("Unable to derive media_key.media_len because platform_chassis is None")
else:
media_key += '-' + '*'

lane_speed_key = get_lane_speed_key(physical_port, port_speed, lane_count)
# return (vendor_key, media_key, lane_speed_key)
return {
VENDOR_KEY: vendor_key,
MEDIA_KEY: media_key,
Expand Down