[xcvrd] Handle platfom_chassis None scenario to protect xcvrd from crash #411
Conversation
keboliu
force-pushed
the
master_media_settings_parser_missing_platform_chassis_fix
branch
from
4014a6e to
7719fb8
Compare
|
Hi @prgeor , this is a bug fix PR for the previous merged CMIS mgmt code. would you please help to review? |
| 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") |
There was a problem hiding this comment.
@tshalvi when can this happen? How does it recover later?
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
@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()
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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
Description
Motivation and Context
How Has This Been Tested?