add exception handling in get_chassis #652

LinJin23 · 2025-07-31T15:02:26Z

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:

TestThermalControlDaemon.test_get_chassis_exception()

Added tests for None chassis handling:

TestFanUpdater.test_update_with_none_chassis()
TestTemperatureUpdater.test_init_with_none_chassis()
TestTemperatureUpdater.test_update_with_none_chassis()

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)

mssonicbld · 2025-07-31T15:02:33Z

/azp run

azure-pipelines · 2025-07-31T15:02:43Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2025-08-11T06:48:26Z

Commenter does not have sufficient privileges for PR 652 in repo sonic-net/sonic-platform-daemons

LinJin23 · 2025-08-11T06:49:01Z

/Azpw run

mssonicbld · 2025-08-11T06:49:03Z

/AzurePipelines run

azure-pipelines · 2025-08-11T06:49:11Z

Azure Pipelines successfully started running 1 pipeline(s).

vvolam · 2025-08-21T04:23:09Z

sonic-thermalctld/scripts/thermalctld

+        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)))


Use log_error instead here.

vvolam · 2025-08-21T04:23:58Z

sonic-thermalctld/scripts/thermalctld


-        self.chassis = sonic_platform.platform.Platform().get_chassis()
+        try:
+            self.chassis = sonic_platform.platform.Platform().get_chassis()


Also, please check if these lines have code coverage in sonic-thermalctld/tests/test_thermalctld.py

Added code coverage.

vvolam · 2025-08-21T04:28:14Z

@LinJin23 could you please test this exception path is hit by testing on any platform and simulating the exception

mssonicbld · 2025-08-26T14:38:28Z

/azp run

azure-pipelines · 2025-08-26T14:38:38Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-09-02T19:18:56Z

/azp run

azure-pipelines · 2025-09-02T19:19:04Z

Azure Pipelines successfully started running 1 pipeline(s).

…lControlDaemon Signed-off-by: Lin Jin <[email protected]>

mssonicbld · 2025-09-26T10:52:30Z

/azp run

azure-pipelines · 2025-09-26T10:52:39Z

Azure Pipelines successfully started running 1 pipeline(s).

Copilot

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.}

Copilot · 2025-09-30T01:07:50Z

sonic-thermalctld/scripts/thermalctld

+        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)))


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.

Suggested change

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

With self.chassis initialized, this comment could be ignored.

Copilot · 2025-09-30T01:07:51Z

sonic-thermalctld/tests/mocked_libs/swsscommon/swsscommon.py

+# 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



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.

Suggested change

# 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

vvolam · 2025-09-30T01:09:48Z

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?

Additional Information (Optional)

Please updated "How Has This Been Tested?" step as well

vvolam · 2025-09-30T04:49:37Z

sonic-thermalctld/scripts/thermalctld

        self.stop_event = threading.Event()

        self.wait_time = self.INTERVAL



Initialize self.chassis to None

Signed-off-by: Lin Jin <[email protected]>

mssonicbld · 2025-10-08T15:16:35Z

/azp run

azure-pipelines · 2025-10-08T15:16:44Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Lin Jin <[email protected]>

mssonicbld · 2025-10-10T00:57:18Z

/azp run

azure-pipelines · 2025-10-10T00:57:27Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Lin Jin <[email protected]>

mssonicbld · 2025-10-10T05:24:49Z

/azp run

azure-pipelines · 2025-10-10T05:24:58Z

Azure Pipelines successfully started running 1 pipeline(s).

LinJin23 · 2025-10-10T14:52:32Z

@LinJin23 could you please test this exception path is hit by testing on any platform and simulating the exception

Already tested on a physical platform to verify that when an exception occurs in get_chassis, the thermalctld daemon doesn’t crash.

Copilot

Pull Request Overview

Copilot reviewed 3 out of 3 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.}

Copilot · 2025-10-10T14:53:32Z

sonic-thermalctld/scripts/thermalctld

+        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)))


The chassis initialization should set self.chassis to None in the exception handler to ensure it's explicitly None when chassis loading fails, making the error state more obvious.

Suggested change

self.log_error("Failed to get chassis due to {}".format(repr(e)))

self.log_error("Failed to get chassis due to {}".format(repr(e)))

self.chassis = None

Copilot · 2025-10-10T14:53:33Z

sonic-thermalctld/scripts/thermalctld

+            self.is_smartswitch_dpu = chassis.is_smartswitch() and chassis.is_dpu()
+            self.is_chassis_upd_required = self.is_chassis_system or self.is_smartswitch_dpu
+
+        if self.is_chassis_upd_required and chassis is not None:


The condition chassis is not None in line 555 is redundant since it's already inside the else block that handles the case when chassis is not None.

Suggested change

if self.is_chassis_upd_required and chassis is not None:

if self.is_chassis_upd_required:

Signed-off-by: Lin Jin <[email protected]>

mssonicbld · 2025-10-10T15:00:16Z

/azp run

azure-pipelines · 2025-10-10T15:00:25Z

Azure Pipelines successfully started running 1 pipeline(s).

Copilot

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-10T15:13:45Z

sonic-thermalctld/scripts/thermalctld

+            self.is_chassis_system = chassis.is_modular_chassis()
+            self.is_smartswitch_dpu = chassis.is_smartswitch() and chassis.is_dpu()
+            self.is_chassis_upd_required = self.is_chassis_system or self.is_smartswitch_dpu


The else block should be indented at the same level as the if statement. Currently, the else is incorrectly aligned with the inner statements.

Suggested change

self.is_chassis_system = chassis.is_modular_chassis()

self.is_smartswitch_dpu = chassis.is_smartswitch() and chassis.is_dpu()

self.is_chassis_upd_required = self.is_chassis_system or self.is_smartswitch_dpu

self.is_chassis_system = chassis.is_modular_chassis()

self.is_smartswitch_dpu = chassis.is_smartswitch() and chassis.is_dpu()

self.is_chassis_upd_required = self.is_chassis_system or self.is_smartswitch_dpu

Not applicable

Copilot · 2025-10-10T15:13:45Z

sonic-thermalctld/tests/test_thermalctld.py

+        with (mock.patch('thermalctld.sonic_platform.platform.Platform') as mock_platform_class,
+              mock.patch.object(thermalctld.ThermalControlDaemon, 'log_error') as mock_log_error):


Using parentheses for multi-line with statements is a Python 3.9+ feature. Consider using backslash continuation for better compatibility: with mock.patch(...) as mock_platform_class, \\\n mock.patch.object(...) as mock_log_error:

Copilot · 2025-10-10T15:13:45Z

sonic-thermalctld/tests/test_thermalctld.py

+        with (mock.patch('thermalctld.sonic_platform.platform.Platform') as mock_platform_class,
+              mock.patch.object(thermalctld.ThermalControlDaemon, 'log_error') as mock_log_error):


Using parentheses for multi-line with statements is a Python 3.9+ feature. Consider using backslash continuation for better compatibility: with mock.patch(...) as mock_platform_class, \\\n mock.patch.object(...) as mock_log_error:

Signed-off-by: Lin Jin <[email protected]>

mssonicbld · 2025-10-10T15:31:23Z

/azp run

azure-pipelines · 2025-10-10T15:31:32Z

Azure Pipelines successfully started running 1 pipeline(s).

Copilot

Pull Request Overview

Copilot reviewed 3 out of 3 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.}

Copilot · 2025-10-10T15:32:31Z

sonic-thermalctld/tests/mocked_libs/swsscommon/swsscommon.py

+            pass
+    class SonicV2Connector:
+        def __init__(self):
+            pass
+    class SonicDBConfig:
+        def __init__(self):
+            pass


The fallback mock classes are empty and may not provide the interface expected by test code. Consider adding basic method stubs that return appropriate default values to prevent AttributeError exceptions during testing.

Suggested change

pass

class SonicV2Connector:

def __init__(self):

pass

class SonicDBConfig:

def __init__(self):

pass

self.db = {}

def connect(self):

pass

def get(self, table, key):

return self.db.get(table, {}).get(key, None)

def set(self, table, key, value):

if table not in self.db:

self.db[table] = {}

self.db[table][key] = value

def get_table(self, table):

return self.db.get(table, {})

class SonicV2Connector:

def __init__(self):

self.db = {}

def connect(self, db_name):

pass

def get(self, db_name, table, key):

return self.db.get(db_name, {}).get(table, {}).get(key, None)

def set(self, db_name, table, key, value):

if db_name not in self.db:

self.db[db_name] = {}

if table not in self.db[db_name]:

self.db[db_name][table] = {}

self.db[db_name][table][key] = value

def get_table(self, db_name, table):

return self.db.get(db_name, {}).get(table, {})

class SonicDBConfig:

def __init__(self):

self.config = {}

def get_db_config(self, db_name):

return self.config.get(db_name, {})

Copilot · 2025-10-10T15:32:31Z

sonic-thermalctld/scripts/thermalctld

+            self.is_smartswitch_dpu = chassis.is_smartswitch() and chassis.is_dpu()
+            self.is_chassis_upd_required = self.is_chassis_system or self.is_smartswitch_dpu
+
        if self.is_chassis_upd_required:


The chassis None check logic should be consistently applied to the chassis table initialization code below (lines 555-563). Currently, if chassis is None but somehow the conditions pass, there could be AttributeError exceptions when calling chassis methods.

Suggested change

if self.is_chassis_upd_required:

if self.is_chassis_upd_required and chassis is not None:

done. BTW this is the opposite of the previous suggestion, which recommended that I remove it.

Signed-off-by: Lin Jin <[email protected]>

mssonicbld · 2025-10-10T15:38:50Z

/azp run

azure-pipelines · 2025-10-10T15:39:00Z

Azure Pipelines successfully started running 1 pipeline(s).

add exception in get chassis

2898503

LinJin23 changed the title ~~add exception in get chassis~~ add exception handling in get_chassis Aug 4, 2025

LinJin23 requested a review from vvolam August 13, 2025 01:08

vvolam reviewed Aug 21, 2025

View reviewed changes

address review comments

5307466

add code coverage

6ccb072

LinJin23 marked this pull request as draft September 23, 2025 16:22

Refactor swsscommon mock classes and enhance test coverage for Therma…

3461e4b

…lControlDaemon Signed-off-by: Lin Jin <[email protected]>

LinJin23 marked this pull request as ready for review September 29, 2025 16:04

LinJin23 requested a review from vvolam September 29, 2025 16:05

vvolam requested review from Copilot and judyjoseph September 30, 2025 01:07

Copilot AI reviewed Sep 30, 2025

View reviewed changes

vvolam reviewed Sep 30, 2025

View reviewed changes

Initialize self.chassis to None

00c3b11

Signed-off-by: Lin Jin <[email protected]>

LinJin23 marked this pull request as draft October 8, 2025 22:39

Fix test failure when self.chassis is None

1d12556

Signed-off-by: Lin Jin <[email protected]>

Add tests for handling None chassis in FanUpdater and TemperatureUpdater

23cfccd

Signed-off-by: Lin Jin <[email protected]>

LinJin23 requested a review from Copilot October 10, 2025 14:52

LinJin23 marked this pull request as ready for review October 10, 2025 14:52

Copilot AI reviewed Oct 10, 2025

View reviewed changes

address copilot comments

19aa9d2

Signed-off-by: Lin Jin <[email protected]>

LinJin23 requested a review from Copilot October 10, 2025 15:12

Copilot AI reviewed Oct 10, 2025

View reviewed changes

address copilot comments

7308c3d

Signed-off-by: Lin Jin <[email protected]>

LinJin23 requested a review from Copilot October 10, 2025 15:31

Copilot AI reviewed Oct 10, 2025

View reviewed changes

address copilot comment

3304a88

Signed-off-by: Lin Jin <[email protected]>

	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

		self.stop_event = threading.Event()

		self.wait_time = self.INTERVAL

	self.log_error("Failed to get chassis due to {}".format(repr(e)))
	self.log_error("Failed to get chassis due to {}".format(repr(e)))
	self.chassis = None

	if self.is_chassis_upd_required and chassis is not None:
	if self.is_chassis_upd_required:

		with (mock.patch('thermalctld.sonic_platform.platform.Platform') as mock_platform_class,
		mock.patch.object(thermalctld.ThermalControlDaemon, 'log_error') as mock_log_error):

-            pass
-    class SonicV2Connector:
-        def __init__(self):
-            pass
-    class SonicDBConfig:
-        def __init__(self):
-            pass
+            self.db = {}
+        def connect(self):
+            pass
+        def get(self, table, key):
+            return self.db.get(table, {}).get(key, None)
+        def set(self, table, key, value):
+            if table not in self.db:
+                self.db[table] = {}
+            self.db[table][key] = value
+        def get_table(self, table):
+            return self.db.get(table, {})
+    class SonicV2Connector:
+        def __init__(self):
+            self.db = {}
+        def connect(self, db_name):
+            pass
+        def get(self, db_name, table, key):
+            return self.db.get(db_name, {}).get(table, {}).get(key, None)
+        def set(self, db_name, table, key, value):
+            if db_name not in self.db:
+                self.db[db_name] = {}
+            if table not in self.db[db_name]:
+                self.db[db_name][table] = {}
+            self.db[db_name][table][key] = value
+        def get_table(self, db_name, table):
+            return self.db.get(db_name, {}).get(table, {})
+    class SonicDBConfig:
+        def __init__(self):
+            self.config = {}
+        def get_db_config(self, db_name):
+            return self.config.get(db_name, {})

add exception handling in get_chassis #652

Are you sure you want to change the base?

add exception handling in get_chassis #652

Conversation

LinJin23 commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Additional Information (Optional)

Uh oh!

mssonicbld commented Jul 31, 2025

Uh oh!

azure-pipelines bot commented Jul 31, 2025

Uh oh!

azure-pipelines bot commented Aug 11, 2025

Uh oh!

LinJin23 commented Aug 11, 2025

Uh oh!

mssonicbld commented Aug 11, 2025

Uh oh!

azure-pipelines bot commented Aug 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvolam commented Aug 21, 2025

Uh oh!

mssonicbld commented Aug 26, 2025

Uh oh!

azure-pipelines bot commented Aug 26, 2025

Uh oh!

mssonicbld commented Sep 2, 2025

Uh oh!

azure-pipelines bot commented Sep 2, 2025

Uh oh!

mssonicbld commented Sep 26, 2025

Uh oh!

azure-pipelines bot commented Sep 26, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvolam commented Sep 30, 2025

Description

Motivation and Context

How Has This Been Tested?

Additional Information (Optional)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mssonicbld commented Oct 8, 2025

Uh oh!

azure-pipelines bot commented Oct 8, 2025

Uh oh!

mssonicbld commented Oct 10, 2025

Uh oh!

azure-pipelines bot commented Oct 10, 2025

Uh oh!

mssonicbld commented Oct 10, 2025

Uh oh!

LinJin23 commented Jul 31, 2025 •

edited

Loading