Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 9 additions & 1 deletion sonic-thermalctld/scripts/thermalctld
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,14 @@ class FanUpdater(logger.Logger):
self.drawer_table = swsscommon.Table(state_db, FanUpdater.FAN_DRAWER_INFO_TABLE_NAME)
self.phy_entity_table = swsscommon.Table(state_db, PHYSICAL_ENTITY_INFO_TABLE)

def get_psu_key(self, psu_index):
if self.chassis is not None:
Comment on lines +204 to +205

Copilot AI Mar 10, 2026

Copy link

Choose a reason for hiding this comment

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

FanUpdater.get_psu_key() is a new public helper but it currently has no docstring, while the surrounding public methods in this file are documented. Please add a short docstring describing expected indexing (0-based vs 1-based) and fallback behavior.

Copilot uses AI. Check for mistakes.
try:
return self.chassis.get_psu(psu_index).get_name()
except (NotImplementedError, AttributeError):
pass
return 'PSU {}'.format(psu_index + 1)
Comment on lines +206 to +210

Copilot AI Mar 10, 2026

Copy link

Choose a reason for hiding this comment

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

get_psu_key() may raise IndexError/other exceptions from chassis.get_psu() or psu.get_name(), which will cause PSU fan updates to be skipped (caught by update() and logged) instead of cleanly falling back to the legacy key. Consider catching broader exceptions (e.g., IndexError/Exception) and also falling back when get_name() returns None/empty, so this helper always returns a usable string key.

Copilot uses AI. Check for mistakes.

def __del__(self):
if self.table:
table_keys = self.table.getKeys()
Expand Down Expand Up @@ -308,7 +316,7 @@ class FanUpdater(logger.Logger):
"""
drawer_name = NOT_AVAILABLE if fan_type != FanType.DRAWER else str(try_get(parent.get_name))
if fan_type == FanType.PSU:
parent_name = try_get(parent.get_name, default='PSU {}'.format(parent_index + 1))
parent_name = self.get_psu_key(parent_index)
elif fan_type == FanType.MODULE:
parent_name = try_get(parent.get_name, default='Module {}'.format(parent_index + 1))
else:
Expand Down
31 changes: 31 additions & 0 deletions sonic-thermalctld/tests/test_thermalctld.py
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,37 @@ def test_update_module_fans(self):
else:
fan_updater.log_warning.assert_called_with("Failed to update module fan status - Exception('Test message',)")

def test_get_psu_key_returns_psu_name(self):
"""Test get_psu_key returns PSU name from platform API"""
chassis = MockChassis()
psu = MockPsu()
psu._name = 'PSU0'
chassis._psu_list.append(psu)
fan_updater = thermalctld.FanUpdater(chassis, threading.Event())
assert fan_updater.get_psu_key(0) == 'PSU0'

def test_get_psu_key_attribute_error(self):
"""Test get_psu_key falls back when get_psu returns None (AttributeError on .get_name())"""
chassis = MockChassis()
# No PSUs added, so get_psu(index) returns None and .get_name() raises AttributeError
fan_updater = thermalctld.FanUpdater(chassis, threading.Event())
assert fan_updater.get_psu_key(0) == 'PSU 1'
assert fan_updater.get_psu_key(1) == 'PSU 2'

def test_get_psu_key_not_implemented_error(self):
"""Test get_psu_key falls back to 1-based 'PSU <index+1>' when get_name raises NotImplementedError"""
chassis = MockChassis()
psu = MockPsu()
psu.get_name = mock.MagicMock(side_effect=NotImplementedError)
chassis._psu_list.append(psu)
fan_updater = thermalctld.FanUpdater(chassis, threading.Event())
assert fan_updater.get_psu_key(0) == 'PSU 1'

def test_get_psu_key_chassis_none(self):
"""Test get_psu_key falls back to 1-based 'PSU <index+1>' when chassis is None"""
fan_updater = thermalctld.FanUpdater(None, threading.Event())
assert fan_updater.get_psu_key(0) == 'PSU 1'

Comment on lines +292 to +322

Copilot AI Mar 10, 2026

Copy link

Choose a reason for hiding this comment

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

The new get_psu_key() tests cover AttributeError/NotImplementedError and chassis=None, but they don't cover cases where chassis.get_psu() (or get_name()) raises IndexError/Exception or where get_name() returns None. Adding tests for these paths will help ensure thermalctld remains resilient across platform implementations (similar to psud's get_psu_key coverage).

Copilot generated this review using guidance from repository custom instructions.
class TestLiquidCoolingUpdater(object):
def test_update(self):
mock_chassis = MockChassis()
Expand Down
Loading