-
Notifications
You must be signed in to change notification settings - Fork 148
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
Handling exceptions in CMIS SM to prevent xcvrd crash #483
Conversation
Signed-off-by: Mihir Patel <patelmi@microsoft.com>
@@ -1376,6 +1381,11 @@ def task_worker(self): | |||
# Skip if these essential routines are not available | |||
self.update_port_transceiver_status_table_sw_cmis_state(lport, CMIS_STATE_READY) | |||
continue | |||
except Exception as e: |
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.
@mihirpat1 why we made distinction between AttributeError vs others?
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.
@prgeor The AttributeError relevant handling here seems to be a day 1 code. However, I am not sure on the original reason for this so decided to leave the existing behavior as is since with AttributeError, we are moving CMIS SM to CMIS_STATE_READY
.
Hence, I am now handling all other exceptions in a different block and moving the CMIS SM to CMIS_STATE_FAILED
state.
@@ -1376,6 +1381,11 @@ def task_worker(self): | |||
# Skip if these essential routines are not available |
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.
It will be good to understand when the Attribute error exception happens. Ideally the below line "except Exception as e:" is a superset and should cover all exceptions.
Also what is the exact error/exception when the eeprom data is corrupted/bad -- sue to bad optics
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.
@judyjoseph Yes - the below line "except Exception as e:" covers all exceptions apart from AttributeError. However, for AttributeError, the CMIS SM is currently being transitioned to CMIS_STATE_READY
and not CMIS_STATE_FAILED
.
In case of bad optics, I have seen KeyError and TypeError so far.
202305 cherry-pick for #483
@mihirpat1 , This PR has conflict for 202205. Can you please raise a separate PR under 202205 branch and link the PR# here. |
Description
Currently, the CmisManagerTask thread crashes upon encountering an exception which causes the entire XCVRD process to restart. The CmisManagerTask thread crash scenarios are more often seen during instances of failure to read EEPROM of the transceivers.
Crash snippet
Motivation and Context
In order to avoid restarting of XCVRD triggered due to CmisManagerTask thread crash, this PR will ensure to move the CMIS SM to
CMIS_STATE_FAILED
state for the corresponding ports which have generated an exception. This will also help in ensuring that if module EEPROM access fails for 1 or more ports, the corresponding port will transition toCMIS_STATE_FAILED
instead.How Has This Been Tested?
An exception was manually generated while CMIS SM was in
CMIS_STATE_INSERTED
and it was ensured that XCVRD did not crash.Also, CMIS initialization was successful on the same port after the exception was not seen any more.
Additional Information (Optional)
MSFT ADO - 27441561