add exception handling in get_chassis#652
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Commenter does not have sufficient privileges for PR 652 in repo sonic-net/sonic-platform-daemons |
|
/Azpw run |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| try: | ||
| self.chassis = sonic_platform.platform.Platform().get_chassis() | ||
| except Exception as e: | ||
| self.log_warning("Failed to get chassis due to {}".format(repr(e))) |
There was a problem hiding this comment.
Use log_error instead here.
|
|
||
| self.chassis = sonic_platform.platform.Platform().get_chassis() | ||
| try: | ||
| self.chassis = sonic_platform.platform.Platform().get_chassis() |
There was a problem hiding this comment.
Also, please check if these lines have code coverage in sonic-thermalctld/tests/test_thermalctld.py
There was a problem hiding this comment.
Added code coverage.
|
@LinJin23 could you please test this exception path is hit by testing on any platform and simulating the exception |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…lControlDaemon Signed-off-by: Lin Jin <linjin@microsoft.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
Adds exception handling to the get_chassis() method call in the ThermalControlDaemon initialization to prevent crashes when chassis loading fails.
- Wraps the platform chassis initialization in a try-except block to catch and log exceptions
- Adds comprehensive test coverage for both successful and failed chassis initialization scenarios
- Updates mock library to include missing methods and classes for improved test compatibility
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| sonic-thermalctld/scripts/thermalctld | Adds try-except wrapper around get_chassis() call with error logging |
| sonic-thermalctld/tests/test_thermalctld.py | Adds test class with comprehensive test cases for chassis initialization scenarios |
| sonic-thermalctld/tests/mocked_libs/swsscommon/swsscommon.py | Updates mock implementation with missing classes and methods for test compatibility |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| try: | ||
| self.chassis = sonic_platform.platform.Platform().get_chassis() | ||
| except Exception as e: | ||
| self.log_error("Failed to get chassis due to {}".format(repr(e))) |
There was a problem hiding this comment.
The exception handling catches the error but doesn't prevent the subsequent code from trying to use self.chassis. Line 829 will fail with AttributeError because self.chassis is never assigned when an exception occurs. Consider either setting self.chassis = None in the except block or adding a return/raise to prevent further initialization.
| self.log_error("Failed to get chassis due to {}".format(repr(e))) | |
| self.log_error("Failed to get chassis due to {}".format(repr(e))) | |
| raise |
There was a problem hiding this comment.
With self.chassis initialized, this comment could be ignored.
| # Mock classes that are imported by sonic_py_common | ||
| class ConfigDBConnector: | ||
| def __init__(self): | ||
| pass | ||
|
|
||
| class SonicV2Connector: | ||
| def __init__(self): | ||
| pass | ||
|
|
||
| class SonicDBConfig: | ||
| def __init__(self): | ||
| pass | ||
|
|
There was a problem hiding this comment.
The removal of the import statement from swsssdk import ConfigDBConnector, SonicDBConfig, SonicV2Connector and replacement with local mock classes creates potential issues. If other parts of the test suite depend on the imported classes from swsssdk, this change could break existing functionality. Consider keeping both approaches or ensuring all dependencies are properly mocked.
| # Mock classes that are imported by sonic_py_common | |
| class ConfigDBConnector: | |
| def __init__(self): | |
| pass | |
| class SonicV2Connector: | |
| def __init__(self): | |
| pass | |
| class SonicDBConfig: | |
| def __init__(self): | |
| pass | |
| # Try to import real swsssdk classes, fallback to mocks if not available | |
| try: | |
| from swsssdk import ConfigDBConnector, SonicDBConfig, SonicV2Connector | |
| except ImportError: | |
| class ConfigDBConnector: | |
| def __init__(self): | |
| pass | |
| class SonicV2Connector: | |
| def __init__(self): | |
| pass | |
| class SonicDBConfig: | |
| def __init__(self): | |
| pass |
Please updated "How Has This Been Tested?" step as well |
| @@ -821,7 +821,10 @@ class ThermalControlDaemon(daemon_base.DaemonBase): | |||
|
|
|||
| self.wait_time = self.INTERVAL | |||
|
|
|||
There was a problem hiding this comment.
Initialize self.chassis to None
Signed-off-by: Lin Jin <linjin@microsoft.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Lin Jin <linjin@microsoft.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| assert any( | ||
| "Failed to get chassis due to" in call_args[0] and "Failed to initialize chassis" in call_args[0] |
There was a problem hiding this comment.
The assertion logic checks if both substrings appear in the same call, but the actual log message format is 'Failed to get chassis due to {repr(e)}'. The check for 'Failed to initialize chassis' should verify it appears within the exception representation, not as a separate substring. Consider using a single contains check or verifying the full expected message format.
| assert any( | |
| "Failed to get chassis due to" in call_args[0] and "Failed to initialize chassis" in call_args[0] | |
| expected_msg = "Failed to get chassis due to Exception('Failed to initialize chassis')" | |
| assert any( | |
| expected_msg in call_args[0] |
Signed-off-by: Lin Jin <linjin@microsoft.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| mock_platform_instance.get_chassis.side_effect = Exception("Failed to initialize chassis") | ||
| mock_platform_class.return_value = mock_platform_instance | ||
|
|
||
| # ThermalControlDaemon should exit with error code when chassis initialization fails |
There was a problem hiding this comment.
[nitpick] The comment states 'should exit with error code' but doesn't mention the SystemExit behavior. Consider clarifying: 'ThermalControlDaemon should raise SystemExit with CHASSIS_GET_ERROR code when chassis initialization fails'.
| # ThermalControlDaemon should exit with error code when chassis initialization fails | |
| # ThermalControlDaemon should raise SystemExit with CHASSIS_GET_ERROR code when chassis initialization fails |
| if mock_log_error.called: | ||
| for call_args in mock_log_error.call_args_list: | ||
| args, _ = call_args | ||
| assert "Failed to get chassis due to" not in args[0] |
There was a problem hiding this comment.
This conditional check for mock_log_error.called before iterating is unnecessary. If the mock wasn't called, call_args_list would be empty and the loop wouldn't execute. Consider simplifying by removing the conditional and directly iterating over mock_log_error.call_args_list.
| if mock_log_error.called: | |
| for call_args in mock_log_error.call_args_list: | |
| args, _ = call_args | |
| assert "Failed to get chassis due to" not in args[0] | |
| for call_args in mock_log_error.call_args_list: | |
| args, _ = call_args | |
| assert "Failed to get chassis due to" not in args[0] |
Signed-off-by: Lin Jin <linjin@microsoft.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| expected_msg in call_args[0] | ||
| for call_args, _ in mock_log_error.call_args_list |
There was a problem hiding this comment.
[nitpick] The assertion iterates through call_args_list but extracts call_args[0] assuming positional arguments. This could be fragile if the logging call format changes. Consider using mock_log_error.assert_any_call(mock.ANY) with a custom matcher or assert_called_with() to make the test more maintainable.
| expected_msg in call_args[0] | |
| for call_args, _ in mock_log_error.call_args_list | |
| expected_msg in call.args[0] | |
| for call in mock_log_error.call_args_list |
There was a problem hiding this comment.
Not changing it for now.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| with mock.patch('thermalctld.sonic_platform.platform.Platform') as mock_platform_class, \ | ||
| mock.patch.object(thermalctld.ThermalControlDaemon, 'log_error') as mock_log_error: |
There was a problem hiding this comment.
[nitpick] The test patches 'log_error' but doesn't verify the call arguments precisely. Consider using mock_log_error.assert_called_with() or mock_log_error.assert_called_once_with() instead of the current approach with any() to make the assertion more explicit and maintainable.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Description
add exception handling in get_chassis()
Motivation and Context
Improve exception handling during chassis loading to prevent thermalctld crashes (e.g., ValueError: invalid literal for int() with base 16: 'read error').
How Has This Been Tested?
Unit tests were extended to test the chassis exception handling functionality:
Added test for chassis initialization failure:
Added tests for None chassis handling:
Verified:
Daemon continues running and logs appropriate warnings when chassis initialization fails.
Additionally, tested on a physical platform to verify that when an exception occurs in get_chassis, the thermalctld daemon doesn’t crash.
Additional Information (Optional)