From 011b96bc78d4948fa3c42403427005c4d8943bee Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Wed, 24 Feb 2021 21:07:41 +0000 Subject: [PATCH 01/43] [thermalctld] Refactor to allow for greater unit test coverage --- sonic-thermalctld/scripts/thermalctld | 226 +++++++++++++++----------- 1 file changed, 129 insertions(+), 97 deletions(-) diff --git a/sonic-thermalctld/scripts/thermalctld b/sonic-thermalctld/scripts/thermalctld index edadb9008..509c71025 100644 --- a/sonic-thermalctld/scripts/thermalctld +++ b/sonic-thermalctld/scripts/thermalctld @@ -1,39 +1,47 @@ -#!/usr/bin/env python2 +#!/usr/bin/env python3 """ thermalctld Thermal control daemon for SONiC """ -try: - import os - import signal - import threading - import time - from datetime import datetime +import os +import signal +import threading +import time +from datetime import datetime - from sonic_py_common import daemon_base, logger - from sonic_py_common.task_base import ProcessTaskBase +import sonic_platform +from sonic_py_common import daemon_base, logger +from sonic_py_common.task_base import ProcessTaskBase - # If unit testing is occurring, mock swsscommon - if os.getenv("THERMALCTLD_UNIT_TESTING") == "1": - from tests import mock_swsscommon as swsscommon - else: - from swsscommon import swsscommon -except ImportError as e: - raise ImportError(repr(e) + " - required module not found") +# If unit testing is occurring, mock swsscommon +if os.getenv("THERMALCTLD_UNIT_TESTING") == "1": + from tests import mock_swsscommon as swsscommon +else: + from swsscommon import swsscommon +# TODO: Once we no longer support Python 2, we can eliminate this and get the +# name using the 'name' field (e.g., `signal.SIGINT.name`) starting with Python 3.5 +SIGNALS_TO_NAMES_DICT = dict((getattr(signal, n), n) + for n in dir(signal) if n.startswith('SIG') and '_' not in n) + SYSLOG_IDENTIFIER = 'thermalctld' NOT_AVAILABLE = 'N/A' CHASSIS_INFO_KEY = 'chassis 1' PHYSICAL_ENTITY_INFO_TABLE = 'PHYSICAL_ENTITY_INFO' INVALID_SLOT = -1 -# utility functions +ERR_UNKNOWN = 1 +ERR_INIT_FAILED = 2 -# try get information from platform API and return a default value if caught NotImplementedError +# Thermal control daemon is designed to never exit, it must always +# return non-zero exit code when exiting and so that supervisord will +# restart it automatically. +exit_code = ERR_UNKNOWN +# utility functions def try_get(callback, default=NOT_AVAILABLE): """ @@ -63,11 +71,11 @@ class FanStatus(logger.Logger): absence_fan_count = 0 fault_fan_count = 0 - def __init__(self, log_identifier, fan=None, is_psu_fan=False): + def __init__(self, fan=None, is_psu_fan=False): """ Constructor of FanStatus """ - super(FanStatus, self).__init__(log_identifier) + super(FanStatus, self).__init__(SYSLOG_IDENTIFIER) self.fan = fan self.is_psu_fan = is_psu_fan @@ -188,12 +196,12 @@ class FanUpdater(logger.Logger): FAN_INFO_TABLE_NAME = 'FAN_INFO' FAN_DRAWER_INFO_TABLE_NAME = 'FAN_DRAWER_INFO' - def __init__(self, log_identifier, chassis): + def __init__(self, chassis): """ Constructor for FanUpdater :param chassis: Object representing a platform chassis """ - super(FanUpdater, self).__init__(log_identifier) + super(FanUpdater, self).__init__(SYSLOG_IDENTIFIER) self.chassis = chassis self.fan_status_dict = {} @@ -294,7 +302,7 @@ class FanUpdater(logger.Logger): fan_name = try_get(fan.get_name, '{} FAN {}'.format(parent_name, fan_index + 1)) update_entity_info(self.phy_entity_table, parent_name, fan_name, fan, fan_index + 1) if fan_name not in self.fan_status_dict: - self.fan_status_dict[fan_name] = FanStatus(SYSLOG_IDENTIFIER, fan, is_psu_fan) + self.fan_status_dict[fan_name] = FanStatus(fan, is_psu_fan) fan_status = self.fan_status_dict[fan_name] @@ -423,8 +431,8 @@ class FanUpdater(logger.Logger): class TemperatureStatus(logger.Logger): TEMPERATURE_DIFF_THRESHOLD = 10 - def __init__(self, log_identifier): - super(TemperatureStatus, self).__init__(log_identifier) + def __init__(self): + super(TemperatureStatus, self).__init__(SYSLOG_IDENTIFIER) self.temperature = None self.over_temperature = False @@ -507,12 +515,12 @@ class TemperatureUpdater(logger.Logger): # Temperature information table name in database TEMPER_INFO_TABLE_NAME = 'TEMPERATURE_INFO' - def __init__(self, log_identifier, chassis): + def __init__(self, chassis): """ Constructor of TemperatureUpdater :param chassis: Object representing a platform chassis """ - super(TemperatureUpdater, self).__init__(log_identifier) + super(TemperatureUpdater, self).__init__(SYSLOG_IDENTIFIER) self.chassis = chassis self.temperature_status_dict = {} @@ -600,7 +608,7 @@ class TemperatureUpdater(logger.Logger): update_entity_info(self.phy_entity_table, parent_name, name, thermal, thermal_index + 1) if name not in self.temperature_status_dict: - self.temperature_status_dict[name] = TemperatureStatus(SYSLOG_IDENTIFIER) + self.temperature_status_dict[name] = TemperatureStatus() temperature_status = self.temperature_status_dict[name] @@ -661,10 +669,12 @@ class TemperatureUpdater(logger.Logger): class ThermalMonitor(ProcessTaskBase): # Initial update interval INITIAL_INTERVAL = 5 + # Update interval value UPDATE_INTERVAL = 60 + # Update elapse threshold. If update used time is larger than the value, generate a warning log. - UPDATE_ELAPSE_THRESHOLD = 30 + UPDATE_ELAPSED_THRESHOLD = 30 def __init__(self, chassis): """ @@ -673,10 +683,16 @@ class ThermalMonitor(ProcessTaskBase): """ ProcessTaskBase.__init__(self) + self.wait_time = self.INITIAL_INTERVAL + # TODO: Refactor to eliminate the need for this Logger instance self.logger = logger.Logger(SYSLOG_IDENTIFIER) - self.fan_updater = FanUpdater(SYSLOG_IDENTIFIER, chassis) - self.temperature_updater = TemperatureUpdater(SYSLOG_IDENTIFIER, chassis) + + # Set minimum logging level to INFO + self.logger.set_min_log_priority_info() + + self.fan_updater = FanUpdater(chassis) + self.temperature_updater = TemperatureUpdater(chassis) def task_worker(self): """ @@ -686,20 +702,19 @@ class ThermalMonitor(ProcessTaskBase): self.logger.log_info("Start thermal monitoring loop") # Start loop to update fan, temperature info in DB periodically - wait_time = ThermalMonitor.INITIAL_INTERVAL - while not self.task_stopping_event.wait(wait_time): + while not self.task_stopping_event.wait(self.wait_time): begin = time.time() self.fan_updater.update() self.temperature_updater.update() - elapse = time.time() - begin - if elapse < ThermalMonitor.UPDATE_INTERVAL: - wait_time = ThermalMonitor.UPDATE_INTERVAL - elapse + elapsed = time.time() - begin + if elapsed < self.UPDATE_INTERVAL: + self.wait_time = self.UPDATE_INTERVAL - elapsed else: - wait_time = ThermalMonitor.INITIAL_INTERVAL + self.wait_time = self.INITIAL_INTERVAL - if elapse > ThermalMonitor.UPDATE_ELAPSE_THRESHOLD: + if elapsed > self.UPDATE_ELAPSED_THRESHOLD: self.logger.log_warning('Update fan and temperature status takes {} seconds, ' - 'there might be performance risk'.format(elapse)) + 'there might be performance risk'.format(elapsed)) self.fan_updater.deinit() self.temperature_updater.deinit() @@ -718,19 +733,48 @@ class ThermalControlDaemon(daemon_base.DaemonBase): POLICY_FILE = '/usr/share/sonic/platform/thermal_policy.json' - def __init__(self, log_identifier): + def __init__(self): """ Constructor of ThermalControlDaemon """ - super(ThermalControlDaemon, self).__init__(log_identifier) + super(ThermalControlDaemon, self).__init__(SYSLOG_IDENTIFIER) + + # Set minimum logging level to INFO + self.set_min_log_priority_info() + self.stop_event = threading.Event() - # Thermal control daemon is designed to never exit, it must always - # return non zero exit code when exiting and so that supervisord will - # restart it automatically. - self.exit_code = 1 + self.wait_time = self.INTERVAL + + chassis = sonic_platform.platform.Platform().get_chassis() + + self.thermal_monitor = ThermalMonitor(chassis) + self.thermal_monitor.task_run() + + self.thermal_manager = None + try: + self.thermal_manager = chassis.get_thermal_manager() + if self.thermal_manager: + self.thermal_manager.initialize() + self.thermal_manager.load(ThermalControlDaemon.POLICY_FILE) + self.thermal_manager.init_thermal_algorithm(chassis) + except NotImplementedError: + self.log_warning('Thermal manager is not supported on this platform.') + except Exception as e: + self.log_error('Caught exception while initializing thermal manager - {}'.format(repr(e))) + sys.exit(ERR_INIT_FAILED) + + + def __del__(self): + try: + if self.thermal_manager: + self.thermal_manager.deinitialize() + except Exception as e: + self.log_error('Caught exception while destroying thermal manager - {}'.format(repr(e))) - # Signal handler + self.thermal_monitor.task_stop() + + # Override signal handler from DaemonBase def signal_handler(self, sig, frame): """ Signal handler @@ -738,77 +782,65 @@ class ThermalControlDaemon(daemon_base.DaemonBase): :param frame: not used :return: """ - if sig == signal.SIGHUP: - self.log_info("Caught SIGHUP - ignoring...") - elif sig == signal.SIGINT or sig == signal.SIGTERM: - self.log_info("Caught signal {} - exiting...".format(sig)) - self.exit_code = sig + 128 + FATAL_SIGNALS = [signal.SIGINT, signal.SIGTERM] + NONFATAL_SIGNALS = [signal.SIGHUP] + + global exit_code + + if sig in FATAL_SIGNALS: + self.log_info("Caught signal '{}' - exiting...".format(SIGNALS_TO_NAMES_DICT[sig])) + exit_code = 128 + sig # Make sure we exit with a non-zero code so that supervisor will try to restart us self.stop_event.set() + elif sig in NONFATAL_SIGNALS: + self.log_info("Caught signal '{}' - ignoring...".format(SIGNALS_TO_NAMES_DICT[sig])) else: - self.log_warning("Caught unhandled signal '" + sig + "'") + self.log_warning("Caught unhandled signal '{}' - ignoring...".format(SIGNALS_TO_NAMES_DICT[sig])) + # Main daemon logic def run(self): """ Run main logical of this daemon :return: """ - self.log_info("Starting up...") - - import sonic_platform.platform - chassis = sonic_platform.platform.Platform().get_chassis() - - thermal_monitor = ThermalMonitor(chassis) - thermal_monitor.task_run() + if self.stop_event.wait(self.wait_time): + # We received a fatal signal + return False - thermal_manager = None + begin = time.time() try: - thermal_manager = chassis.get_thermal_manager() - if thermal_manager: - thermal_manager.initialize() - thermal_manager.load(ThermalControlDaemon.POLICY_FILE) - thermal_manager.init_thermal_algorithm(chassis) - except NotImplementedError: - self.log_warning('Thermal manager is not supported on this platform.') + if self.thermal_manager: + self.thermal_manager.run_policy(chassis) except Exception as e: - self.log_error('Caught exception while initializing thermal manager - {}'.format(repr(e))) + self.log_error('Caught exception while running thermal policy - {}'.format(repr(e))) - wait_time = ThermalControlDaemon.INTERVAL - while not self.stop_event.wait(wait_time): - begin = time.time() - try: - if thermal_manager: - thermal_manager.run_policy(chassis) - except Exception as e: - self.log_error('Caught exception while running thermal policy - {}'.format(repr(e))) - elapsed = time.time() - begin - if elapsed < ThermalControlDaemon.INTERVAL: - wait_time = ThermalControlDaemon.INTERVAL - elapsed - else: - wait_time = ThermalControlDaemon.FAST_START_INTERVAL - - if elapsed > ThermalControlDaemon.RUN_POLICY_WARN_THRESHOLD_SECS: - self.log_warning('Thermal policy execution takes {} seconds, ' - 'there might be performance risk'.format(elapsed)) - - try: - if thermal_manager: - thermal_manager.deinitialize() - except Exception as e: - self.log_error('Caught exception while destroy thermal manager - {}'.format(repr(e))) + elapsed = time.time() - begin + if elapsed < self.INTERVAL: + self.wait_time = self.INTERVAL - elapsed + else: + self.wait_time = self.FAST_START_INTERVAL - thermal_monitor.task_stop() + if elapsed > self.RUN_POLICY_WARN_THRESHOLD_SECS: + self.log_warning('Thermal policy execution takes {} seconds, ' + 'there might be performance risk'.format(elapsed)) - self.log_info("Shutdown with exit code {}...".format(self.exit_code)) - exit(self.exit_code) + return True # # Main ========================================================================= # def main(): - thermal_control = ThermalControlDaemon(SYSLOG_IDENTIFIER) - thermal_control.run() + thermal_control = ThermalControlDaemon() + + thermal_control.log_info("Starting up...") + + while thermal_control.run(): + pass + + thermal_control.log_info("Shutting down with exit code {}...".format(exit_code)) + + return exit_code if __name__ == '__main__': - main() + sys.exit(main()) From b814e1687b859236f8fff35695aab775ef95964e Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Thu, 25 Feb 2021 02:52:59 +0000 Subject: [PATCH 02/43] Increase pytest verbosity --- sonic-thermalctld/pytest.ini | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sonic-thermalctld/pytest.ini b/sonic-thermalctld/pytest.ini index aa4fe636e..d90ee9ed9 100644 --- a/sonic-thermalctld/pytest.ini +++ b/sonic-thermalctld/pytest.ini @@ -1,2 +1,2 @@ [pytest] -addopts = --cov=scripts --cov-report html --cov-report term --cov-report xml --junitxml=test-results.xml -v +addopts = --cov=scripts --cov-report html --cov-report term --cov-report xml --junitxml=test-results.xml -vv From 126263c823b50236daab8a08d18f7e584a831ec9 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Thu, 25 Feb 2021 03:20:26 +0000 Subject: [PATCH 03/43] Refactor unit tests to align with source --- azure-pipelines.yml | 4 + sonic-thermalctld/setup.py | 3 +- .../mocked_libs/sonic_platform/__init__.py | 6 + .../mocked_libs/sonic_platform/chassis.py | 25 ++++ .../mocked_libs/sonic_platform/platform.py | 11 ++ sonic-thermalctld/tests/test_thermalctld.py | 127 +++++++++--------- 6 files changed, 113 insertions(+), 63 deletions(-) create mode 100644 sonic-thermalctld/tests/mocked_libs/sonic_platform/__init__.py create mode 100644 sonic-thermalctld/tests/mocked_libs/sonic_platform/chassis.py create mode 100644 sonic-thermalctld/tests/mocked_libs/sonic_platform/platform.py diff --git a/azure-pipelines.yml b/azure-pipelines.yml index 05bb0b21d..cd7d1f3cd 100644 --- a/azure-pipelines.yml +++ b/azure-pipelines.yml @@ -81,8 +81,12 @@ jobs: set -xe sudo pip2 install swsssdk-2.0.1-py2-none-any.whl sudo pip2 install sonic_py_common-1.0-py2-none-any.whl + sudo pip2 install sonic_config_engine-1.0-py2-none-any.whl + sudo pip2 install sonic_platform_common-1.0-py2-none-any.whl sudo pip3 install swsssdk-2.0.1-py3-none-any.whl sudo pip3 install sonic_py_common-1.0-py3-none-any.whl + sudo pip3 install sonic_config_engine-1.0-py3-none-any.whl + sudo pip3 install sonic_platform_common-1.0-py3-none-any.whl workingDirectory: $(Pipeline.Workspace)/target/python-wheels/ displayName: 'Install Python dependencies' diff --git a/sonic-thermalctld/setup.py b/sonic-thermalctld/setup.py index 6955ecdd8..5e9bc083f 100644 --- a/sonic-thermalctld/setup.py +++ b/sonic-thermalctld/setup.py @@ -23,7 +23,8 @@ tests_require=[ 'mock>=2.0.0; python_version < "3.3"', 'pytest', - 'pytest-cov' + 'pytest-cov', + 'sonic-platform-common' ], classifiers=[ 'Development Status :: 4 - Beta', diff --git a/sonic-thermalctld/tests/mocked_libs/sonic_platform/__init__.py b/sonic-thermalctld/tests/mocked_libs/sonic_platform/__init__.py new file mode 100644 index 000000000..e491d5b52 --- /dev/null +++ b/sonic-thermalctld/tests/mocked_libs/sonic_platform/__init__.py @@ -0,0 +1,6 @@ +""" + Mock implementation of sonic_platform package for unit testing +""" + +from . import chassis +from . import platform diff --git a/sonic-thermalctld/tests/mocked_libs/sonic_platform/chassis.py b/sonic-thermalctld/tests/mocked_libs/sonic_platform/chassis.py new file mode 100644 index 000000000..49a939987 --- /dev/null +++ b/sonic-thermalctld/tests/mocked_libs/sonic_platform/chassis.py @@ -0,0 +1,25 @@ +""" + Mock implementation of sonic_platform package for unit testing +""" + +# TODO: Clean this up once we no longer need to support Python 2 +import sys +if sys.version_info.major == 3: + from unittest import mock +else: + import mock + +from sonic_platform_base.chassis_base import ChassisBase + + +class Chassis(ChassisBase): + def __init__(self): + ChassisBase.__init__(self) + self._eeprom = mock.MagicMock() + self._thermal_manager = mock.MagicMock() + + def get_eeprom(self): + return self._eeprom + + def get_thermal_manager(self): + return self._thermal_manager diff --git a/sonic-thermalctld/tests/mocked_libs/sonic_platform/platform.py b/sonic-thermalctld/tests/mocked_libs/sonic_platform/platform.py new file mode 100644 index 000000000..e1e7735f3 --- /dev/null +++ b/sonic-thermalctld/tests/mocked_libs/sonic_platform/platform.py @@ -0,0 +1,11 @@ +""" + Mock implementation of sonic_platform package for unit testing +""" + +from sonic_platform_base.platform_base import PlatformBase +from sonic_platform.chassis import Chassis + +class Platform(PlatformBase): + def __init__(self): + PlatformBase.__init__(self) + self._chassis = Chassis() diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index 3f203ab87..201b1eda5 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -1,58 +1,61 @@ import os import sys -from imp import load_source +from imp import load_source # TODO: Replace with importlib once we no longer need to support Python 2 # TODO: Clean this up once we no longer need to support Python 2 if sys.version_info.major == 3: - from unittest.mock import Mock, MagicMock, patch + from unittest import mock else: - from mock import Mock, MagicMock, patch + import mock + from sonic_py_common import daemon_base from .mock_platform import MockChassis, MockFan, MockThermal -SYSLOG_IDENTIFIER = 'thermalctld_test' -NOT_AVAILABLE = 'N/A' +daemon_base.db_connect = mock.MagicMock() + +tests_path = os.path.dirname(os.path.abspath(__file__)) -daemon_base.db_connect = MagicMock() +# Add mocked_libs path so that the file under test can load mocked modules from there +mocked_libs_path = os.path.join(tests_path, 'mocked_libs') +sys.path.insert(0, mocked_libs_path) -test_path = os.path.dirname(os.path.abspath(__file__)) -modules_path = os.path.dirname(test_path) -scripts_path = os.path.join(modules_path, "scripts") +# Add path to the file under test so that we can load it +modules_path = os.path.dirname(tests_path) +scripts_path = os.path.join(modules_path, 'scripts') sys.path.insert(0, modules_path) -os.environ["THERMALCTLD_UNIT_TESTING"] = "1" -load_source('thermalctld', scripts_path + '/thermalctld') -from thermalctld import * +load_source('thermalctld', os.path.join(scripts_path, 'thermalctld')) +import thermalctld TEMPER_INFO_TABLE_NAME = 'TEMPERATURE_INFO' def setup_function(): - FanStatus.log_notice = MagicMock() - FanStatus.log_warning = MagicMock() - FanUpdater.log_notice = MagicMock() - FanUpdater.log_warning = MagicMock() - TemperatureStatus.log_notice = MagicMock() - TemperatureStatus.log_warning = MagicMock() - TemperatureUpdater.log_notice = MagicMock() - TemperatureUpdater.log_warning = MagicMock() + thermalctld.FanStatus.log_notice = mock.MagicMock() + thermalctld.FanStatus.log_warning = mock.MagicMock() + thermalctld.FanUpdater.log_notice = mock.MagicMock() + thermalctld.FanUpdater.log_warning = mock.MagicMock() + thermalctld.TemperatureStatus.log_notice = mock.MagicMock() + thermalctld.TemperatureStatus.log_warning = mock.MagicMock() + thermalctld.TemperatureUpdater.log_notice = mock.MagicMock() + thermalctld.TemperatureUpdater.log_warning = mock.MagicMock() def teardown_function(): - FanStatus.log_notice.reset() - FanStatus.log_warning.reset() - FanUpdater.log_notice.reset() - FanUpdater.log_notice.reset() - TemperatureStatus.log_notice.reset() - TemperatureStatus.log_warning.reset() - TemperatureUpdater.log_warning.reset() - TemperatureUpdater.log_warning.reset() + thermalctld.FanStatus.log_notice.reset() + thermalctld.FanStatus.log_warning.reset() + thermalctld.FanUpdater.log_notice.reset() + thermalctld.FanUpdater.log_notice.reset() + thermalctld.TemperatureStatus.log_notice.reset() + thermalctld.TemperatureStatus.log_warning.reset() + thermalctld.TemperatureUpdater.log_warning.reset() + thermalctld.TemperatureUpdater.log_warning.reset() def test_fanstatus_set_presence(): - fan_status = FanStatus(SYSLOG_IDENTIFIER) + fan_status = thermalctld.FanStatus() ret = fan_status.set_presence(True) assert fan_status.presence assert not ret @@ -63,14 +66,14 @@ def test_fanstatus_set_presence(): def test_fanstatus_set_under_speed(): - fan_status = FanStatus(SYSLOG_IDENTIFIER) - ret = fan_status.set_under_speed(NOT_AVAILABLE, NOT_AVAILABLE, NOT_AVAILABLE) + fan_status = thermalctld.FanStatus() + ret = fan_status.set_under_speed(thermalctld.NOT_AVAILABLE, thermalctld.NOT_AVAILABLE, thermalctld.NOT_AVAILABLE) assert not ret - ret = fan_status.set_under_speed(NOT_AVAILABLE, NOT_AVAILABLE, 0) + ret = fan_status.set_under_speed(thermalctld.NOT_AVAILABLE, thermalctld.NOT_AVAILABLE, 0) assert not ret - ret = fan_status.set_under_speed(NOT_AVAILABLE, 0, 0) + ret = fan_status.set_under_speed(thermalctld.NOT_AVAILABLE, 0, 0) assert not ret ret = fan_status.set_under_speed(0, 0, 0) @@ -88,14 +91,14 @@ def test_fanstatus_set_under_speed(): def test_fanstatus_set_over_speed(): - fan_status = FanStatus(SYSLOG_IDENTIFIER) - ret = fan_status.set_over_speed(NOT_AVAILABLE, NOT_AVAILABLE, NOT_AVAILABLE) + fan_status = thermalctld.FanStatus() + ret = fan_status.set_over_speed(thermalctld.NOT_AVAILABLE, thermalctld.NOT_AVAILABLE, thermalctld.NOT_AVAILABLE) assert not ret - ret = fan_status.set_over_speed(NOT_AVAILABLE, NOT_AVAILABLE, 0) + ret = fan_status.set_over_speed(thermalctld.NOT_AVAILABLE, thermalctld.NOT_AVAILABLE, 0) assert not ret - ret = fan_status.set_over_speed(NOT_AVAILABLE, 0, 0) + ret = fan_status.set_over_speed(thermalctld.NOT_AVAILABLE, 0, 0) assert not ret ret = fan_status.set_over_speed(0, 0, 0) @@ -115,7 +118,7 @@ def test_fanstatus_set_over_speed(): def test_fanupdater_fan_absence(): chassis = MockChassis() chassis.make_absence_fan() - fan_updater = FanUpdater(SYSLOG_IDENTIFIER, chassis) + fan_updater = thermalctld.FanUpdater(chassis) fan_updater.update() fan_list = chassis.get_all_fans() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED @@ -130,7 +133,7 @@ def test_fanupdater_fan_absence(): def test_fanupdater_fan_fault(): chassis = MockChassis() chassis.make_fault_fan() - fan_updater = FanUpdater(SYSLOG_IDENTIFIER, chassis) + fan_updater = thermalctld.FanUpdater(chassis) fan_updater.update() fan_list = chassis.get_all_fans() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED @@ -145,7 +148,7 @@ def test_fanupdater_fan_fault(): def test_fanupdater_fan_under_speed(): chassis = MockChassis() chassis.make_under_speed_fan() - fan_updater = FanUpdater(SYSLOG_IDENTIFIER, chassis) + fan_updater = thermalctld.FanUpdater(chassis) fan_updater.update() fan_list = chassis.get_all_fans() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED @@ -160,7 +163,7 @@ def test_fanupdater_fan_under_speed(): def test_fanupdater_fan_over_speed(): chassis = MockChassis() chassis.make_over_speed_fan() - fan_updater = FanUpdater(SYSLOG_IDENTIFIER, chassis) + fan_updater = thermalctld.FanUpdater(chassis) fan_updater.update() fan_list = chassis.get_all_fans() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED @@ -173,8 +176,8 @@ def test_fanupdater_fan_over_speed(): def test_insufficient_fan_number(): - fan_status1 = FanStatus(SYSLOG_IDENTIFIER) - fan_status2 = FanStatus(SYSLOG_IDENTIFIER) + fan_status1 = thermalctld.FanStatus() + fan_status2 = thermalctld.FanStatus() fan_status1.set_presence(False) fan_status2.set_fault_status(False) assert FanStatus.get_bad_fan_count() == 2 @@ -184,7 +187,7 @@ def test_insufficient_fan_number(): chassis = MockChassis() chassis.make_absence_fan() chassis.make_fault_fan() - fan_updater = FanUpdater(SYSLOG_IDENTIFIER, chassis) + fan_updater = thermalctld.FanUpdater(chassis) fan_updater.update() assert fan_updater.log_warning.call_count == 3 fan_updater.log_warning.assert_called_with('Insufficient number of working fans warning: 2 fans are not working.') @@ -203,14 +206,14 @@ def test_insufficient_fan_number(): def test_temperature_status_set_over_temper(): - temperatue_status = TemperatureStatus(SYSLOG_IDENTIFIER) - ret = temperatue_status.set_over_temperature(NOT_AVAILABLE, NOT_AVAILABLE) + temperatue_status = thermalctld.TemperatureStatus() + ret = temperatue_status.set_over_temperature(thermalctld.NOT_AVAILABLE, thermalctld.NOT_AVAILABLE) assert not ret - ret = temperatue_status.set_over_temperature(NOT_AVAILABLE, 0) + ret = temperatue_status.set_over_temperature(thermalctld.NOT_AVAILABLE, 0) assert not ret - ret = temperatue_status.set_over_temperature(0, NOT_AVAILABLE) + ret = temperatue_status.set_over_temperature(0, thermalctld.NOT_AVAILABLE) assert not ret ret = temperatue_status.set_over_temperature(2, 1) @@ -223,14 +226,14 @@ def test_temperature_status_set_over_temper(): def test_temperstatus_set_under_temper(): - temperature_status = TemperatureStatus(SYSLOG_IDENTIFIER) - ret = temperature_status.set_under_temperature(NOT_AVAILABLE, NOT_AVAILABLE) + temperature_status = thermalctld.TemperatureStatus() + ret = temperature_status.set_under_temperature(thermalctld.NOT_AVAILABLE, thermalctld.NOT_AVAILABLE) assert not ret - ret = temperature_status.set_under_temperature(NOT_AVAILABLE, 0) + ret = temperature_status.set_under_temperature(thermalctld.NOT_AVAILABLE, 0) assert not ret - ret = temperature_status.set_under_temperature(0, NOT_AVAILABLE) + ret = temperature_status.set_under_temperature(0, thermalctld.NOT_AVAILABLE) assert not ret ret = temperature_status.set_under_temperature(1, 2) @@ -245,7 +248,7 @@ def test_temperstatus_set_under_temper(): def test_temperupdater_over_temper(): chassis = MockChassis() chassis.make_over_temper_thermal() - temperature_updater = TemperatureUpdater(SYSLOG_IDENTIFIER, chassis) + temperature_updater = thermalctld.TemperatureUpdater(chassis) temperature_updater.update() thermal_list = chassis.get_all_thermals() temperature_updater.log_warning.assert_called_once() @@ -258,7 +261,7 @@ def test_temperupdater_over_temper(): def test_temperupdater_under_temper(): chassis = MockChassis() chassis.make_under_temper_thermal() - temperature_updater = TemperatureUpdater(SYSLOG_IDENTIFIER, chassis) + temperature_updater = thermalctld.TemperatureUpdater(chassis) temperature_updater.update() thermal_list = chassis.get_all_thermals() temperature_updater.log_warning.assert_called_once() @@ -275,7 +278,7 @@ def test_update_fan_with_exception(): fan.make_over_speed() chassis.get_all_fans().append(fan) - fan_updater = FanUpdater(SYSLOG_IDENTIFIER, chassis) + fan_updater = thermalctld.FanUpdater(chassis) fan_updater.update() assert fan.get_status_led() == MockFan.STATUS_LED_COLOR_RED fan_updater.log_warning.assert_called() @@ -288,7 +291,7 @@ def test_update_thermal_with_exception(): thermal.make_over_temper() chassis.get_all_thermals().append(thermal) - temperature_updater = TemperatureUpdater(SYSLOG_IDENTIFIER, chassis) + temperature_updater = thermalctld.TemperatureUpdater(chassis) temperature_updater.update() temperature_updater.log_warning.assert_called() @@ -299,19 +302,19 @@ def test_updater_thermal_check_modular_chassis(): chassis = MockChassis() assert chassis.is_modular_chassis() == False - temperature_updater = TemperatureUpdater(SYSLOG_IDENTIFIER, chassis) + temperature_updater = thermalctld.TemperatureUpdater(chassis) assert temperature_updater.chassis_table == None chassis.set_modular_chassis(True) chassis.set_my_slot(-1) - temperature_updater = TemperatureUpdater(SYSLOG_IDENTIFIER, chassis) + temperature_updater = thermalctld.TemperatureUpdater(chassis) assert temperature_updater.chassis_table == None my_slot = 1 chassis.set_my_slot(my_slot) - temperature_updater = TemperatureUpdater(SYSLOG_IDENTIFIER, chassis) + temperature_updater = thermalctld.TemperatureUpdater(chassis) assert temperature_updater.chassis_table != None - assert temperature_updater.chassis_table.table_name == TEMPER_INFO_TABLE_NAME+'_'+str(my_slot) + assert temperature_updater.chassis_table.table_name == '{}_{}'.format(TEMPER_INFO_TABLE_NAME, str(my_slot)) def test_updater_thermal_check_chassis_table(): @@ -322,7 +325,7 @@ def test_updater_thermal_check_chassis_table(): chassis.set_modular_chassis(True) chassis.set_my_slot(1) - temperature_updater = TemperatureUpdater(SYSLOG_IDENTIFIER, chassis) + temperature_updater = thermalctld.TemperatureUpdater(chassis) temperature_updater.update() assert temperature_updater.chassis_table.get_size() == chassis.get_num_thermals() @@ -344,7 +347,7 @@ def test_updater_thermal_check_min_max(): chassis.set_modular_chassis(True) chassis.set_my_slot(1) - temperature_updater = TemperatureUpdater(SYSLOG_IDENTIFIER, chassis) + temperature_updater = thermalctld.TemperatureUpdater(chassis) temperature_updater.update() slot_dict = temperature_updater.chassis_table.get(thermal.get_name()) From 2156279c4ee2a8fcc8d09cd5cdd55bea643cb48b Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Thu, 25 Feb 2021 04:19:03 +0000 Subject: [PATCH 04/43] Add mocked swsscommon lib --- sonic-thermalctld/tests/mock_swsscommon.py | 4 ++ .../tests/mocked_libs/swsscommon/__init__.py | 5 ++ .../mocked_libs/swsscommon/swsscommon.py | 54 +++++++++++++++++++ 3 files changed, 63 insertions(+) create mode 100644 sonic-thermalctld/tests/mocked_libs/swsscommon/__init__.py create mode 100644 sonic-thermalctld/tests/mocked_libs/swsscommon/swsscommon.py diff --git a/sonic-thermalctld/tests/mock_swsscommon.py b/sonic-thermalctld/tests/mock_swsscommon.py index c46c8a70a..ade0d3541 100644 --- a/sonic-thermalctld/tests/mock_swsscommon.py +++ b/sonic-thermalctld/tests/mock_swsscommon.py @@ -1,3 +1,7 @@ +''' + Mock implementation of swsscommon package for unit testing +''' + STATE_DB = '' CHASSIS_STATE_DB = '' diff --git a/sonic-thermalctld/tests/mocked_libs/swsscommon/__init__.py b/sonic-thermalctld/tests/mocked_libs/swsscommon/__init__.py new file mode 100644 index 000000000..012af621e --- /dev/null +++ b/sonic-thermalctld/tests/mocked_libs/swsscommon/__init__.py @@ -0,0 +1,5 @@ +''' + Mock implementation of swsscommon package for unit testing +''' + +from . import swsscommon diff --git a/sonic-thermalctld/tests/mocked_libs/swsscommon/swsscommon.py b/sonic-thermalctld/tests/mocked_libs/swsscommon/swsscommon.py new file mode 100644 index 000000000..6947a8601 --- /dev/null +++ b/sonic-thermalctld/tests/mocked_libs/swsscommon/swsscommon.py @@ -0,0 +1,54 @@ +''' + Mock implementation of swsscommon package for unit testing +''' + +STATE_DB = '' + + +class Table: + def __init__(self, db, table_name): + self.table_name = table_name + self.mock_dict = {} + + def _del(self, key): + del self.mock_dict[key] + pass + + def set(self, key, fvs): + self.mock_dict[key] = fvs.fv_dict + pass + + def get(self, key): + if key in self.mock_dict: + return self.mock_dict[key] + return None + + def get_size(self): + return (len(self.mock_dict)) + + +class FieldValuePairs: + fv_dict = {} + + def __init__(self, tuple_list): + if isinstance(tuple_list, list) and isinstance(tuple_list[0], tuple): + self.fv_dict = dict(tuple_list) + + def __setitem__(self, key, kv_tuple): + self.fv_dict[kv_tuple[0]] = kv_tuple[1] + + def __getitem__(self, key): + return self.fv_dict[key] + + def __eq__(self, other): + if not isinstance(other, FieldValuePairs): + # don't attempt to compare against unrelated types + return NotImplemented + + return self.fv_dict == other.fv_dict + + def __repr__(self): + return repr(self.fv_dict) + + def __str__(self): + return repr(self.fv_dict) From 51986928dffe9c1f2af4b52118f4e5b105bbd9e3 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Thu, 25 Feb 2021 04:20:10 +0000 Subject: [PATCH 05/43] Remove THERMALCTLD_UNIT_TESTING env var --- sonic-thermalctld/scripts/thermalctld | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/sonic-thermalctld/scripts/thermalctld b/sonic-thermalctld/scripts/thermalctld index 509c71025..2cecbc859 100644 --- a/sonic-thermalctld/scripts/thermalctld +++ b/sonic-thermalctld/scripts/thermalctld @@ -5,7 +5,6 @@ Thermal control daemon for SONiC """ -import os import signal import threading import time @@ -14,12 +13,7 @@ from datetime import datetime import sonic_platform from sonic_py_common import daemon_base, logger from sonic_py_common.task_base import ProcessTaskBase - -# If unit testing is occurring, mock swsscommon -if os.getenv("THERMALCTLD_UNIT_TESTING") == "1": - from tests import mock_swsscommon as swsscommon -else: - from swsscommon import swsscommon +from swsscommon import swsscommon # TODO: Once we no longer support Python 2, we can eliminate this and get the From 7663fa040daefe18d819609b1f65477927c0e1c4 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Thu, 25 Feb 2021 04:20:40 +0000 Subject: [PATCH 06/43] Use pytest fixture for setup/teardown --- sonic-thermalctld/tests/test_thermalctld.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index 201b1eda5..392e14deb 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -8,6 +8,7 @@ else: import mock +import pytest from sonic_py_common import daemon_base from .mock_platform import MockChassis, MockFan, MockThermal @@ -32,7 +33,8 @@ TEMPER_INFO_TABLE_NAME = 'TEMPERATURE_INFO' -def setup_function(): +@pytest.fixture(scope='function', autouse=True) +def configure_mocks(): thermalctld.FanStatus.log_notice = mock.MagicMock() thermalctld.FanStatus.log_warning = mock.MagicMock() thermalctld.FanUpdater.log_notice = mock.MagicMock() @@ -42,8 +44,8 @@ def setup_function(): thermalctld.TemperatureUpdater.log_notice = mock.MagicMock() thermalctld.TemperatureUpdater.log_warning = mock.MagicMock() + yield -def teardown_function(): thermalctld.FanStatus.log_notice.reset() thermalctld.FanStatus.log_warning.reset() thermalctld.FanUpdater.log_notice.reset() From 082f3c099051469ad738a40cc2d662255bdb0ce6 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Thu, 25 Feb 2021 04:38:33 +0000 Subject: [PATCH 07/43] Use call_count in favor of assert_called[_once] --- sonic-thermalctld/tests/test_thermalctld.py | 28 ++++++++++----------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index 392e14deb..2b57dd774 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -124,12 +124,12 @@ def test_fanupdater_fan_absence(): fan_updater.update() fan_list = chassis.get_all_fans() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED - fan_updater.log_warning.assert_called() + assert fan_updater.log_warning.call_count == 1 fan_list[0].presence = True fan_updater.update() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN - fan_updater.log_notice.assert_called() + assert fan_updater.log_notice.call_count == 1 def test_fanupdater_fan_fault(): @@ -139,12 +139,12 @@ def test_fanupdater_fan_fault(): fan_updater.update() fan_list = chassis.get_all_fans() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED - fan_updater.log_warning.assert_called() + assert fan_updater.log_warning.call_count == 1 fan_list[0].status = True fan_updater.update() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN - fan_updater.log_notice.assert_called() + assert fan_updater.log_notice.call_count == 1 def test_fanupdater_fan_under_speed(): @@ -154,12 +154,12 @@ def test_fanupdater_fan_under_speed(): fan_updater.update() fan_list = chassis.get_all_fans() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED - fan_updater.log_warning.assert_called_once() + assert fan_updater.log_warning.call_count == 1 fan_list[0].make_normal_speed() fan_updater.update() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN - fan_updater.log_notice.assert_called_once() + assert fan_updater.log_notice.call_count == 1 def test_fanupdater_fan_over_speed(): @@ -169,12 +169,12 @@ def test_fanupdater_fan_over_speed(): fan_updater.update() fan_list = chassis.get_all_fans() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED - fan_updater.log_warning.assert_called_once() + assert fan_updater.log_warning.call_count == 1 fan_list[0].make_normal_speed() fan_updater.update() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN - fan_updater.log_notice.assert_called_once() + assert fan_updater.log_notice.call_count == 1 def test_insufficient_fan_number(): @@ -253,11 +253,11 @@ def test_temperupdater_over_temper(): temperature_updater = thermalctld.TemperatureUpdater(chassis) temperature_updater.update() thermal_list = chassis.get_all_thermals() - temperature_updater.log_warning.assert_called_once() + assert temperature_updater.log_warning.call_count == 1 thermal_list[0].make_normal_temper() temperature_updater.update() - temperature_updater.log_notice.assert_called_once() + assert temperature_updater.log_notice.call_count == 1 def test_temperupdater_under_temper(): @@ -266,11 +266,11 @@ def test_temperupdater_under_temper(): temperature_updater = thermalctld.TemperatureUpdater(chassis) temperature_updater.update() thermal_list = chassis.get_all_thermals() - temperature_updater.log_warning.assert_called_once() + assert temperature_updater.log_warning.call_count == 1 thermal_list[0].make_normal_temper() temperature_updater.update() - temperature_updater.log_notice.assert_called_once() + assert temperature_updater.log_notice.call_count == 1 def test_update_fan_with_exception(): @@ -283,7 +283,7 @@ def test_update_fan_with_exception(): fan_updater = thermalctld.FanUpdater(chassis) fan_updater.update() assert fan.get_status_led() == MockFan.STATUS_LED_COLOR_RED - fan_updater.log_warning.assert_called() + assert fan_updater.log_warning.call_count == 1 def test_update_thermal_with_exception(): @@ -295,7 +295,7 @@ def test_update_thermal_with_exception(): temperature_updater = thermalctld.TemperatureUpdater(chassis) temperature_updater.update() - temperature_updater.log_warning.assert_called() + assert temperature_updater.log_warning.call_count == 1 # Modular chassis related tests From 53fea67b551188bd0da2e8e8c544c84f5102716f Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Thu, 25 Feb 2021 05:08:25 +0000 Subject: [PATCH 08/43] Fix test bug and check both instances to ensure class var is correct --- sonic-thermalctld/tests/test_thermalctld.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index 2b57dd774..cfd0581c5 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -180,11 +180,17 @@ def test_fanupdater_fan_over_speed(): def test_insufficient_fan_number(): fan_status1 = thermalctld.FanStatus() fan_status2 = thermalctld.FanStatus() + fan_status1.set_presence(False) fan_status2.set_fault_status(False) - assert FanStatus.get_bad_fan_count() == 2 - FanStatus.reset_fan_counter() - assert FanStatus.get_bad_fan_count() == 0 + assert thermalctld.FanStatus.get_bad_fan_count() == 2 + assert fan_status1.get_bad_fan_count() == 2 + assert fan_status2.get_bad_fan_count() == 2 + + thermalctld.FanStatus.reset_fan_counter() + assert thermalctld.FanStatus.get_bad_fan_count() == 0 + assert fan_status1.get_bad_fan_count() == 0 + assert fan_status2.get_bad_fan_count() == 0 chassis = MockChassis() chassis.make_absence_fan() From a2df880257522ecac19db267a5844a6e3df3e30c Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Thu, 25 Feb 2021 07:54:56 +0000 Subject: [PATCH 09/43] [mock_platform] inherit from sonic_platform_base --- sonic-thermalctld/tests/mock_platform.py | 80 +++++++++------------ sonic-thermalctld/tests/test_thermalctld.py | 6 +- 2 files changed, 36 insertions(+), 50 deletions(-) diff --git a/sonic-thermalctld/tests/mock_platform.py b/sonic-thermalctld/tests/mock_platform.py index 168de9754..7277b9b21 100644 --- a/sonic-thermalctld/tests/mock_platform.py +++ b/sonic-thermalctld/tests/mock_platform.py @@ -1,3 +1,9 @@ +from sonic_platform_base import chassis_base +from sonic_platform_base import fan_base +from sonic_platform_base import fan_drawer_base +from sonic_platform_base import module_base +from sonic_platform_base import psu_base + class MockDevice: def __init__(self): self.name = None @@ -169,94 +175,74 @@ def get_temperature(self): raise Exception('Fail to get temperature') -class MockChassis: +class MockChassis(chassis_base.ChassisBase): def __init__(self): - self.fan_list = [] - self.psu_list = [] - self.thermal_list = [] - self.fan_drawer_list = [] - self.sfp_list = [] - self.is_chassis_system = False - - def get_all_fans(self): - return self.fan_list - - def get_all_psus(self): - return self.psu_list - - def get_all_thermals(self): - return self.thermal_list - - def get_all_fan_drawers(self): - return self.fan_drawer_list - - def get_all_sfps(self): - return self.sfp_list + super(MockChassis, self).__init__() - def get_num_thermals(self): - return len(self.thermal_list) + self._is_chassis_system = False + self._my_slot = module_base.ModuleBase.MODULE_INVALID_SLOT - def make_absence_fan(self): + def make_absent_fan(self): fan = MockFan() fan.presence = False - fan_drawer = MockFanDrawer(len(self.fan_drawer_list)) + fan_drawer = MockFanDrawer(len(self._fan_drawer_list)) fan_drawer.fan_list.append(fan) - self.fan_list.append(fan) - self.fan_drawer_list.append(fan_drawer) + self._fan_list.append(fan) + self._fan_drawer_list.append(fan_drawer) def make_fault_fan(self): fan = MockFan() fan.status = False - fan_drawer = MockFanDrawer(len(self.fan_drawer_list)) + fan_drawer = MockFanDrawer(len(self._fan_drawer_list)) fan_drawer.fan_list.append(fan) - self.fan_list.append(fan) - self.fan_drawer_list.append(fan_drawer) + self._fan_list.append(fan) + self._fan_drawer_list.append(fan_drawer) def make_under_speed_fan(self): fan = MockFan() fan.make_under_speed() - fan_drawer = MockFanDrawer(len(self.fan_drawer_list)) + fan_drawer = MockFanDrawer(len(self._fan_drawer_list)) fan_drawer.fan_list.append(fan) - self.fan_list.append(fan) - self.fan_drawer_list.append(fan_drawer) + self._fan_list.append(fan) + self._fan_drawer_list.append(fan_drawer) def make_over_speed_fan(self): fan = MockFan() fan.make_over_speed() - fan_drawer = MockFanDrawer(len(self.fan_drawer_list)) + fan_drawer = MockFanDrawer(len(self._fan_drawer_list)) fan_drawer.fan_list.append(fan) - self.fan_list.append(fan) - self.fan_drawer_list.append(fan_drawer) + self._fan_list.append(fan) + self._fan_drawer_list.append(fan_drawer) def make_error_fan(self): fan = MockErrorFan() - fan_drawer = MockFanDrawer(len(self.fan_drawer_list)) + fan_drawer = MockFanDrawer(len(self._fan_drawer_list)) fan_drawer.fan_list.append(fan) - self.fan_list.append(fan) - self.fan_drawer_list.append(fan_drawer) + self._fan_list.append(fan) + self._fan_drawer_list.append(fan_drawer) def make_over_temper_thermal(self): thermal = MockThermal() thermal.make_over_temper() - self.thermal_list.append(thermal) + self._thermal_list.append(thermal) def make_under_temper_thermal(self): thermal = MockThermal() thermal.make_under_temper() - self.thermal_list.append(thermal) + self._thermal_list.append(thermal) def make_error_thermal(self): thermal = MockErrorThermal() - self.thermal_list.append(thermal) + self._thermal_list.append(thermal) def is_modular_chassis(self): - return self.is_chassis_system + return self._is_chassis_system def set_modular_chassis(self, is_true): - self.is_chassis_system = is_true + self._is_chassis_system = is_true def set_my_slot(self, my_slot): - self.my_slot = my_slot + self._my_slot = my_slot def get_my_slot(self): - return self.my_slot + return self._my_slot diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index cfd0581c5..583d61797 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -117,9 +117,9 @@ def test_fanstatus_set_over_speed(): assert fan_status.is_ok() -def test_fanupdater_fan_absence(): +def test_fanupdater_fan_absent(): chassis = MockChassis() - chassis.make_absence_fan() + chassis.make_absent_fan() fan_updater = thermalctld.FanUpdater(chassis) fan_updater.update() fan_list = chassis.get_all_fans() @@ -193,7 +193,7 @@ def test_insufficient_fan_number(): assert fan_status2.get_bad_fan_count() == 0 chassis = MockChassis() - chassis.make_absence_fan() + chassis.make_absent_fan() chassis.make_fault_fan() fan_updater = thermalctld.FanUpdater(chassis) fan_updater.update() From aca158071e0481beb43b8999ebc296ec9dc59648 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Thu, 25 Feb 2021 19:05:27 +0000 Subject: [PATCH 10/43] Update mock_platform.py --- sonic-thermalctld/tests/mock_platform.py | 354 ++++++++++++++++------- 1 file changed, 245 insertions(+), 109 deletions(-) diff --git a/sonic-thermalctld/tests/mock_platform.py b/sonic-thermalctld/tests/mock_platform.py index 7277b9b21..d74f7bd14 100644 --- a/sonic-thermalctld/tests/mock_platform.py +++ b/sonic-thermalctld/tests/mock_platform.py @@ -3,198 +3,306 @@ from sonic_platform_base import fan_drawer_base from sonic_platform_base import module_base from sonic_platform_base import psu_base +from sonic_platform_base import thermal_base -class MockDevice: + +class MockFan(fan_base.FanBase): def __init__(self): - self.name = None - self.presence = True - self.model = 'FAN Model' - self.serial = 'Fan Serial' + super(MockFan, self).__init__() + self._name = None + self._presence = True + self._model = 'Fan Model' + self._serial = 'Fan Serial' + self._status = True + self._position_in_parent = 1 + self._replaceable = True + + self._speed = 20 + self._speed_tolerance = 20 + self._target_speed = 20 + self._direction = self.FAN_DIRECTION_INTAKE + self._status_led = self.STATUS_LED_COLOR_RED + + def get_speed(self): + return self._speed + + def get_speed_tolerance(self): + return self._speed_tolerance + + def get_target_speed(self): + return self._target_speed + + def get_direction(self): + return self._direction + + def get_status_led(self): + return self._status_led + + def set_status_led(self, value): + self._status_led = value + def make_under_speed(self): + self._speed = 1 + self._target_speed = 2 + self._speed_tolerance = 0 + + def make_over_speed(self): + self._speed = 2 + self._target_speed = 1 + self._speed_tolerance = 0 + + def make_normal_speed(self): + self._speed = 1 + self._target_speed = 1 + self._speed_tolerance = 0 + + # Methods inherited from DeviceBase class and related setters def get_name(self): - return self.name + return self._name def get_presence(self): - return self.presence + return self._presence + + def set_presence(self, presence): + self._presence = presence def get_model(self): - return self.model + return self._model def get_serial(self): - return self.serial + return self._serial + + def get_status(self): + return self._status + + def set_status(self, status): + self._status = status def get_position_in_parent(self): - return 1 + return self._position_in_parent def is_replaceable(self): - return True + return self._replaceable - def get_status(self): - return True +class MockErrorFan(MockFan): + def get_speed(self): + raise Exception('Failed to get speed') -class MockFan(MockDevice): - STATUS_LED_COLOR_RED = 'red' - STATUS_LED_COLOR_GREEN = 'green' +class MockPsu(psu_base.PsuBase): def __init__(self): - MockDevice.__init__(self) - self.speed = 20 - self.speed_tolerance = 20 - self.target_speed = 20 - self.status = True - self.direction = 'intake' - self.led_status = 'red' - - def get_speed(self): - return self.speed + super(MockPsu, self).__init__() + self._name = None + self._presence = True + self._model = 'PSU Model' + self._serial = 'PSU Serial' + self._status = True + self._position_in_parent = 1 + self._replaceable = True - def get_speed_tolerance(self): - return self.speed_tolerance + self._fan_list = [] - def get_target_speed(self): - return self.target_speed + def get_all_fans(self): + return self._fan_list - def get_status(self): - return self.status + # Methods inherited from DeviceBase class and related setters + def get_name(self): + return self._name - def get_direction(self): - return self.direction + def get_presence(self): + return self._presence - def get_status_led(self): - return self.led_status + def set_presence(self, presence): + self._presence = presence - def set_status_led(self, value): - self.led_status = value + def get_model(self): + return self._model - def make_under_speed(self): - self.speed = 1 - self.target_speed = 2 - self.speed_tolerance = 0 + def get_serial(self): + return self._serial - def make_over_speed(self): - self.speed = 2 - self.target_speed = 1 - self.speed_tolerance = 0 + def get_status(self): + return self._status - def make_normal_speed(self): - self.speed = 1 - self.target_speed = 1 - self.speed_tolerance = 0 + def set_status(self, status): + self._status = status + def get_position_in_parent(self): + return self._position_in_parent -class MockErrorFan(MockFan): - def get_speed(self): - raise Exception('Fail to get speed') + def is_replaceable(self): + return self._replaceable -class MockPsu(MockDevice): - def __init__(self): - MockDevice.__init__(self) - self.fan_list = [] +class MockFanDrawer(fan_drawer_base.FanDrawerBase): + def __init__(self, index): + super(MockFanDrawer, self).__init__() + self._name = 'FanDrawer {}'.format(index) + self._presence = True + self._model = 'Fan Drawer Model' + self._serial = 'Fan Drawer Serial' + self._status = True + self._position_in_parent = 1 + self._replaceable = True + + self._fan_list = [] + self._status_led = self.STATUS_LED_COLOR_RED def get_all_fans(self): - return self.fan_list + return self._fan_list + def get_status_led(self): + return self._status_led -class MockFanDrawer(MockDevice): - def __init__(self, index): - MockDevice.__init__(self) - self.name = 'FanDrawer {}'.format(index) - self.fan_list = [] - self.led_status = 'red' + def set_status_led(self, value): + self._status_led = value + # Methods inherited from DeviceBase class and related setters def get_name(self): - return self.name + return self._name - def get_all_fans(self): - return self.fan_list + def get_presence(self): + return self._presence - def get_status_led(self): - return self.led_status + def set_presence(self, presence): + self._presence = presence - def set_status_led(self, value): - self.led_status = value + def get_model(self): + return self._model + def get_serial(self): + return self._serial -class MockThermal(MockDevice): - def __init__(self, index=None): - MockDevice.__init__(self) - self.name = None - self.name = 'Thermal {}'.format(index) if index != None else None - self.temperature = 2 - self.minimum_temperature = 1 - self.maximum_temperature = 5 - self.high_threshold = 3 - self.low_threshold = 1 - self.high_critical_threshold = 4 - self.low_critical_threshold = 0 + def get_status(self): + return self._status - def get_name(self): - return self.name + def set_status(self, status): + self._status = status + + def get_position_in_parent(self): + return self._position_in_parent + + def is_replaceable(self): + return self._replaceable + + +class MockThermal(thermal_base.ThermalBase): + def __init__(self, index=None): + super(MockThermal, self).__init__() + self._name = 'Thermal {}'.format(index) if index != None else None + self._presence = True + self._model = 'Thermal Model' + self._serial = 'Thermal Serial' + self._status = True + self._position_in_parent = 1 + self._replaceable = False + + self._temperature = 2 + self._minimum_temperature = 1 + self._maximum_temperature = 5 + self._high_threshold = 3 + self._low_threshold = 1 + self._high_critical_threshold = 4 + self._low_critical_threshold = 0 def get_temperature(self): - return self.temperature + return self._temperature def get_minimum_recorded(self): - return self.minimum_temperature + return self._minimum_temperature def get_maximum_recorded(self): - return self.maximum_temperature + return self._maximum_temperature def get_high_threshold(self): - return self.high_threshold + return self._high_threshold def get_low_threshold(self): - return self.low_threshold + return self._low_threshold def get_high_critical_threshold(self): - return self.high_critical_threshold + return self._high_critical_threshold def get_low_critical_threshold(self): - return self.low_critical_threshold + return self._low_critical_threshold def make_over_temper(self): - self.high_threshold = 2 - self.temperature = 3 - self.low_threshold = 1 + self._high_threshold = 2 + self._temperature = 3 + self._low_threshold = 1 def make_under_temper(self): - self.high_threshold = 3 - self.temperature = 1 - self.low_threshold = 2 + self._high_threshold = 3 + self._temperature = 1 + self._low_threshold = 2 def make_normal_temper(self): - self.high_threshold = 3 - self.temperature = 2 - self.low_threshold = 1 + self._high_threshold = 3 + self._temperature = 2 + self._low_threshold = 1 + + # Methods inherited from DeviceBase class and related setters + def get_name(self): + return self._name + + def get_presence(self): + return self._presence + + def set_presence(self, presence): + self._presence = presence + + def get_model(self): + return self._model + + def get_serial(self): + return self._serial + + def get_status(self): + return self._status + + def set_status(self, status): + self._status = status + + def get_position_in_parent(self): + return self._position_in_parent + + def is_replaceable(self): + return self._replaceable class MockErrorThermal(MockThermal): def get_temperature(self): - raise Exception('Fail to get temperature') + raise Exception('Failed to get temperature') class MockChassis(chassis_base.ChassisBase): def __init__(self): super(MockChassis, self).__init__() + self._name = None + self._presence = True + self._model = 'Chassis Model' + self._serial = 'Chassis Serial' + self._status = True + self._position_in_parent = 1 + self._replaceable = False self._is_chassis_system = False self._my_slot = module_base.ModuleBase.MODULE_INVALID_SLOT def make_absent_fan(self): fan = MockFan() - fan.presence = False + fan.set_presence(False) fan_drawer = MockFanDrawer(len(self._fan_drawer_list)) - fan_drawer.fan_list.append(fan) + fan_drawer._fan_list.append(fan) self._fan_list.append(fan) self._fan_drawer_list.append(fan_drawer) def make_fault_fan(self): fan = MockFan() - fan.status = False + fan.set_status(False) fan_drawer = MockFanDrawer(len(self._fan_drawer_list)) - fan_drawer.fan_list.append(fan) + fan_drawer._fan_list.append(fan) self._fan_list.append(fan) self._fan_drawer_list.append(fan_drawer) @@ -202,7 +310,7 @@ def make_under_speed_fan(self): fan = MockFan() fan.make_under_speed() fan_drawer = MockFanDrawer(len(self._fan_drawer_list)) - fan_drawer.fan_list.append(fan) + fan_drawer._fan_list.append(fan) self._fan_list.append(fan) self._fan_drawer_list.append(fan_drawer) @@ -210,14 +318,14 @@ def make_over_speed_fan(self): fan = MockFan() fan.make_over_speed() fan_drawer = MockFanDrawer(len(self._fan_drawer_list)) - fan_drawer.fan_list.append(fan) + fan_drawer._fan_list.append(fan) self._fan_list.append(fan) self._fan_drawer_list.append(fan_drawer) def make_error_fan(self): fan = MockErrorFan() fan_drawer = MockFanDrawer(len(self._fan_drawer_list)) - fan_drawer.fan_list.append(fan) + fan_drawer._fan_list.append(fan) self._fan_list.append(fan) self._fan_drawer_list.append(fan_drawer) @@ -246,3 +354,31 @@ def set_my_slot(self, my_slot): def get_my_slot(self): return self._my_slot + + # Methods inherited from DeviceBase class and related setters + def get_name(self): + return self._name + + def get_presence(self): + return self._presence + + def set_presence(self, presence): + self._presence = presence + + def get_model(self): + return self._model + + def get_serial(self): + return self._serial + + def get_status(self): + return self._status + + def set_status(self, status): + self._status = status + + def get_position_in_parent(self): + return self._position_in_parent + + def is_replaceable(self): + return self._replaceable From e25363c6497303ce9214e8a30ca7e0b6a07bf6ed Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Thu, 25 Feb 2021 19:39:58 +0000 Subject: [PATCH 11/43] Tests use new setters --- sonic-thermalctld/tests/test_thermalctld.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index 583d61797..1f88d700a 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -126,7 +126,7 @@ def test_fanupdater_fan_absent(): assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED assert fan_updater.log_warning.call_count == 1 - fan_list[0].presence = True + fan_list[0].set_presence(True) fan_updater.update() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN assert fan_updater.log_notice.call_count == 1 @@ -141,7 +141,7 @@ def test_fanupdater_fan_fault(): assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED assert fan_updater.log_warning.call_count == 1 - fan_list[0].status = True + fan_list[0].set_status(True) fan_updater.update() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN assert fan_updater.log_notice.call_count == 1 @@ -201,12 +201,12 @@ def test_insufficient_fan_number(): fan_updater.log_warning.assert_called_with('Insufficient number of working fans warning: 2 fans are not working.') fan_list = chassis.get_all_fans() - fan_list[0].presence = True + fan_list[0].set_presence(True) fan_updater.update() assert fan_updater.log_notice.call_count == 1 fan_updater.log_warning.assert_called_with('Insufficient number of working fans warning: 1 fans are not working.') - fan_list[1].status = True + fan_list[1].set_status(True) fan_updater.update() assert fan_updater.log_notice.call_count == 3 fan_updater.log_notice.assert_called_with( From 02bc7ab55a5b5ff70e7d111dec4a006072c69c95 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Thu, 25 Feb 2021 22:49:49 +0000 Subject: [PATCH 12/43] Use parens instead of escaping newlines --- sonic-thermalctld/scripts/thermalctld | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/sonic-thermalctld/scripts/thermalctld b/sonic-thermalctld/scripts/thermalctld index 2cecbc859..898df51ff 100644 --- a/sonic-thermalctld/scripts/thermalctld +++ b/sonic-thermalctld/scripts/thermalctld @@ -175,11 +175,11 @@ class FanStatus(logger.Logger): Indicate the Fan works as expect :return: True if Fan works normal else False """ - return self.presence and \ - self.status and \ - not self.under_speed and \ - not self.over_speed and \ - not self.invalid_direction + return (self.presence and + self.status and + not self.under_speed and + not self.over_speed and + not self.invalid_direction) # From b118e0e3eca9f7786a3c9da19a52c5c26e4adaff Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Thu, 25 Feb 2021 23:29:23 +0000 Subject: [PATCH 13/43] Change 'absence' to 'absent', 'fault' to 'faulty' where applicable --- sonic-thermalctld/scripts/thermalctld | 14 +++++++------- sonic-thermalctld/tests/mock_platform.py | 2 +- sonic-thermalctld/tests/test_thermalctld.py | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/sonic-thermalctld/scripts/thermalctld b/sonic-thermalctld/scripts/thermalctld index 898df51ff..da56894e1 100644 --- a/sonic-thermalctld/scripts/thermalctld +++ b/sonic-thermalctld/scripts/thermalctld @@ -62,8 +62,8 @@ def update_entity_info(table, parent_name, key, device, device_index): class FanStatus(logger.Logger): - absence_fan_count = 0 - fault_fan_count = 0 + absent_fan_count = 0 + faulty_fan_count = 0 def __init__(self, fan=None, is_psu_fan=False): """ @@ -81,12 +81,12 @@ class FanStatus(logger.Logger): @classmethod def get_bad_fan_count(cls): - return cls.absence_fan_count + cls.fault_fan_count + return cls.absent_fan_count + cls.faulty_fan_count @classmethod def reset_fan_counter(cls): - cls.absence_fan_count = 0 - cls.fault_fan_count = 0 + cls.absent_fan_count = 0 + cls.faulty_fan_count = 0 def set_presence(self, presence): """ @@ -95,7 +95,7 @@ class FanStatus(logger.Logger): :return: True if status changed else False """ if not presence and not self.is_psu_fan: - FanStatus.absence_fan_count += 1 + FanStatus.absent_fan_count += 1 if presence == self.presence: return False @@ -110,7 +110,7 @@ class FanStatus(logger.Logger): :return: True if status changed else False """ if not status: - FanStatus.fault_fan_count += 1 + FanStatus.faulty_fan_count += 1 if status == self.status: return False diff --git a/sonic-thermalctld/tests/mock_platform.py b/sonic-thermalctld/tests/mock_platform.py index d74f7bd14..423805066 100644 --- a/sonic-thermalctld/tests/mock_platform.py +++ b/sonic-thermalctld/tests/mock_platform.py @@ -298,7 +298,7 @@ def make_absent_fan(self): self._fan_list.append(fan) self._fan_drawer_list.append(fan_drawer) - def make_fault_fan(self): + def make_faulty_fan(self): fan = MockFan() fan.set_status(False) fan_drawer = MockFanDrawer(len(self._fan_drawer_list)) diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index 1f88d700a..428a09b56 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -134,7 +134,7 @@ def test_fanupdater_fan_absent(): def test_fanupdater_fan_fault(): chassis = MockChassis() - chassis.make_fault_fan() + chassis.make_faulty_fan() fan_updater = thermalctld.FanUpdater(chassis) fan_updater.update() fan_list = chassis.get_all_fans() @@ -194,7 +194,7 @@ def test_insufficient_fan_number(): chassis = MockChassis() chassis.make_absent_fan() - chassis.make_fault_fan() + chassis.make_faulty_fan() fan_updater = thermalctld.FanUpdater(chassis) fan_updater.update() assert fan_updater.log_warning.call_count == 3 From 13ddb52160cab1b49ee23e683c60dd8b69559ab5 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Fri, 26 Feb 2021 07:27:33 +0000 Subject: [PATCH 14/43] Add tests for signal handler --- sonic-thermalctld/tests/test_thermalctld.py | 57 +++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index 428a09b56..ad5e3246e 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -361,3 +361,60 @@ def test_updater_thermal_check_min_max(): slot_dict = temperature_updater.chassis_table.get(thermal.get_name()) assert slot_dict['minimum_temperature'] == str(thermal.get_minimum_recorded()) assert slot_dict['maximum_temperature'] == str(thermal.get_maximum_recorded()) + + +def test_signal_handler(): + daemon_thermalctld = thermalctld.ThermalControlDaemon() + daemon_thermalctld.stop_event.set = mock.MagicMock() + daemon_thermalctld.log_info = mock.MagicMock() + daemon_thermalctld.log_warning = mock.MagicMock() + + # Test SIGHUP + daemon_thermalctld.signal_handler(thermalctld.signal.SIGHUP, None) + assert daemon_thermalctld.log_info.call_count == 1 + daemon_thermalctld.log_info.assert_called_with("Caught signal 'SIGHUP' - ignoring...") + assert daemon_thermalctld.log_warning.call_count == 0 + assert daemon_thermalctld.stop_event.set.call_count == 0 + assert thermalctld.exit_code == thermalctld.ERR_UNKNOWN + + # Reset + daemon_thermalctld.log_info.reset_mock() + daemon_thermalctld.log_warning.reset_mock() + daemon_thermalctld.stop_event.set.reset_mock() + + # Test SIGINT + test_signal = thermalctld.signal.SIGINT + daemon_thermalctld.signal_handler(test_signal, None) + assert daemon_thermalctld.log_info.call_count == 1 + daemon_thermalctld.log_info.assert_called_with("Caught signal 'SIGINT' - exiting...") + assert daemon_thermalctld.log_warning.call_count == 0 + assert daemon_thermalctld.stop_event.set.call_count == 1 + assert thermalctld.exit_code == (128 + test_signal) + + # Reset + daemon_thermalctld.log_info.reset_mock() + daemon_thermalctld.log_warning.reset_mock() + daemon_thermalctld.stop_event.set.reset_mock() + + # Test SIGTERM + test_signal = thermalctld.signal.SIGTERM + daemon_thermalctld.signal_handler(test_signal, None) + assert daemon_thermalctld.log_info.call_count == 1 + daemon_thermalctld.log_info.assert_called_with("Caught signal 'SIGTERM' - exiting...") + assert daemon_thermalctld.log_warning.call_count == 0 + assert daemon_thermalctld.stop_event.set.call_count == 1 + assert thermalctld.exit_code == (128 + test_signal) + + # Reset + daemon_thermalctld.log_info.reset_mock() + daemon_thermalctld.log_warning.reset_mock() + daemon_thermalctld.stop_event.set.reset_mock() + thermalctld.exit_code = thermalctld.ERR_UNKNOWN + + # Test an unhandled signal + daemon_thermalctld.signal_handler(thermalctld.signal.SIGUSR1, None) + assert daemon_thermalctld.log_warning.call_count == 1 + daemon_thermalctld.log_warning.assert_called_with("Caught unhandled signal 'SIGUSR1' - ignoring...") + assert daemon_thermalctld.log_info.call_count == 0 + assert daemon_thermalctld.stop_event.set.call_count == 0 + assert thermalctld.exit_code == thermalctld.ERR_UNKNOWN From c72e31e9b332e6fa0fadc2101a1b1ed369b98824 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Fri, 26 Feb 2021 07:41:22 +0000 Subject: [PATCH 15/43] import sys --- sonic-thermalctld/scripts/thermalctld | 1 + 1 file changed, 1 insertion(+) diff --git a/sonic-thermalctld/scripts/thermalctld b/sonic-thermalctld/scripts/thermalctld index da56894e1..d30e42936 100644 --- a/sonic-thermalctld/scripts/thermalctld +++ b/sonic-thermalctld/scripts/thermalctld @@ -6,6 +6,7 @@ """ import signal +import sys import threading import time from datetime import datetime From 04eb5efcb0399556f5226b6cf861b31277ed01d2 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Fri, 26 Feb 2021 17:55:42 +0000 Subject: [PATCH 16/43] Add MockThermalManager class to mock_platform.py --- sonic-thermalctld/tests/mock_platform.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/sonic-thermalctld/tests/mock_platform.py b/sonic-thermalctld/tests/mock_platform.py index 423805066..7326d28e5 100644 --- a/sonic-thermalctld/tests/mock_platform.py +++ b/sonic-thermalctld/tests/mock_platform.py @@ -4,6 +4,7 @@ from sonic_platform_base import module_base from sonic_platform_base import psu_base from sonic_platform_base import thermal_base +from sonic_platform_base.sonic_thermal_control import thermal_manager_base class MockFan(fan_base.FanBase): @@ -276,6 +277,11 @@ def get_temperature(self): raise Exception('Failed to get temperature') +class MockThermalManager(thermal_manager_base.ThermalManagerBase): + def __init__(self): + super(MockThermalManager, self).__init__() + + class MockChassis(chassis_base.ChassisBase): def __init__(self): super(MockChassis, self).__init__() @@ -289,6 +295,7 @@ def __init__(self): self._is_chassis_system = False self._my_slot = module_base.ModuleBase.MODULE_INVALID_SLOT + self._thermal_manager = MockThermalManager() def make_absent_fan(self): fan = MockFan() @@ -355,6 +362,9 @@ def set_my_slot(self, my_slot): def get_my_slot(self): return self._my_slot + def get_thermal_manager(self): + return self._thermal_manager + # Methods inherited from DeviceBase class and related setters def get_name(self): return self._name From f4a036a499cafbcd68e5ce87807d674245c81e31 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Fri, 26 Feb 2021 19:20:05 +0000 Subject: [PATCH 17/43] Replace 'deinit()' methods with actual destructors --- sonic-thermalctld/scripts/thermalctld | 26 +++++++++++---------- sonic-thermalctld/tests/test_thermalctld.py | 3 --- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/sonic-thermalctld/scripts/thermalctld b/sonic-thermalctld/scripts/thermalctld index d30e42936..6ead9d3d3 100644 --- a/sonic-thermalctld/scripts/thermalctld +++ b/sonic-thermalctld/scripts/thermalctld @@ -68,7 +68,7 @@ class FanStatus(logger.Logger): def __init__(self, fan=None, is_psu_fan=False): """ - Constructor of FanStatus + Initializer of FanStatus """ super(FanStatus, self).__init__(SYSLOG_IDENTIFIER) @@ -193,7 +193,7 @@ class FanUpdater(logger.Logger): def __init__(self, chassis): """ - Constructor for FanUpdater + Initializer for FanUpdater :param chassis: Object representing a platform chassis """ super(FanUpdater, self).__init__(SYSLOG_IDENTIFIER) @@ -205,7 +205,7 @@ 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 deinit(self): + def __del__(self): """ Destructor of FanUpdater :return: @@ -213,6 +213,8 @@ class FanUpdater(logger.Logger): for name in self.fan_status_dict.keys(): self.table._del(name) + super(FanUpdater, self).__del__() + def _log_on_status_changed(self, normal_status, normal_log, abnormal_log): """ Log when any status changed @@ -512,7 +514,7 @@ class TemperatureUpdater(logger.Logger): def __init__(self, chassis): """ - Constructor of TemperatureUpdater + Initializer of TemperatureUpdater :param chassis: Object representing a platform chassis """ super(TemperatureUpdater, self).__init__(SYSLOG_IDENTIFIER) @@ -532,9 +534,9 @@ class TemperatureUpdater(logger.Logger): chassis_state_db = daemon_base.db_connect("CHASSIS_STATE_DB") self.chassis_table = swsscommon.Table(chassis_state_db, table_name) - def deinit(self): + def __del__(self): """ - Destructor of TemperatureUpdater + Destructor of TemperatureUpdater :return: """ for name in self.temperature_status_dict.keys(): @@ -542,6 +544,8 @@ class TemperatureUpdater(logger.Logger): if self.is_chassis_system and self.chassis_table is not None: self.chassis_table._del(name) + super(TemperatureUpdater, self).__del__() + def _log_on_status_changed(self, normal_status, normal_log, abnormal_log): """ Log when any status changed @@ -673,7 +677,7 @@ class ThermalMonitor(ProcessTaskBase): def __init__(self, chassis): """ - Constructor for ThermalMonitor + Initializer for ThermalMonitor :param chassis: Object representing a platform chassis """ ProcessTaskBase.__init__(self) @@ -711,9 +715,6 @@ class ThermalMonitor(ProcessTaskBase): self.logger.log_warning('Update fan and temperature status takes {} seconds, ' 'there might be performance risk'.format(elapsed)) - self.fan_updater.deinit() - self.temperature_updater.deinit() - self.logger.log_info("Stop thermal monitoring loop") @@ -730,7 +731,7 @@ class ThermalControlDaemon(daemon_base.DaemonBase): def __init__(self): """ - Constructor of ThermalControlDaemon + Initializer of ThermalControlDaemon """ super(ThermalControlDaemon, self).__init__(SYSLOG_IDENTIFIER) @@ -759,7 +760,6 @@ class ThermalControlDaemon(daemon_base.DaemonBase): self.log_error('Caught exception while initializing thermal manager - {}'.format(repr(e))) sys.exit(ERR_INIT_FAILED) - def __del__(self): try: if self.thermal_manager: @@ -769,6 +769,8 @@ class ThermalControlDaemon(daemon_base.DaemonBase): self.thermal_monitor.task_stop() + super(ThermalControlDaemon, self).__del__() + # Override signal handler from DaemonBase def signal_handler(self, sig, frame): """ diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index ad5e3246e..d5075515a 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -343,9 +343,6 @@ def test_updater_thermal_check_chassis_table(): temperature_updater.update() assert temperature_updater.chassis_table.get_size() == chassis.get_num_thermals() - temperature_updater.deinit() - assert temperature_updater.chassis_table.get_size() == 0 - def test_updater_thermal_check_min_max(): chassis = MockChassis() From ff163a735af78ecfed27cb8b2c927efa8c628b6e Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Fri, 26 Feb 2021 20:09:41 +0000 Subject: [PATCH 18/43] Replace 'takes' with 'took' --- sonic-thermalctld/scripts/thermalctld | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sonic-thermalctld/scripts/thermalctld b/sonic-thermalctld/scripts/thermalctld index 6ead9d3d3..9dd070b26 100644 --- a/sonic-thermalctld/scripts/thermalctld +++ b/sonic-thermalctld/scripts/thermalctld @@ -712,7 +712,7 @@ class ThermalMonitor(ProcessTaskBase): self.wait_time = self.INITIAL_INTERVAL if elapsed > self.UPDATE_ELAPSED_THRESHOLD: - self.logger.log_warning('Update fan and temperature status takes {} seconds, ' + self.logger.log_warning('Update fan and temperature status took {} seconds, ' 'there might be performance risk'.format(elapsed)) self.logger.log_info("Stop thermal monitoring loop") @@ -817,7 +817,7 @@ class ThermalControlDaemon(daemon_base.DaemonBase): self.wait_time = self.FAST_START_INTERVAL if elapsed > self.RUN_POLICY_WARN_THRESHOLD_SECS: - self.log_warning('Thermal policy execution takes {} seconds, ' + self.log_warning('Thermal policy execution took {} seconds, ' 'there might be performance risk'.format(elapsed)) return True From ba8069ef738074727b22565d2ea931bfed94cea3 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Fri, 26 Feb 2021 20:12:18 +0000 Subject: [PATCH 19/43] Use more modern method of initializing base class --- sonic-thermalctld/scripts/thermalctld | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sonic-thermalctld/scripts/thermalctld b/sonic-thermalctld/scripts/thermalctld index 9dd070b26..29e085397 100644 --- a/sonic-thermalctld/scripts/thermalctld +++ b/sonic-thermalctld/scripts/thermalctld @@ -680,7 +680,7 @@ class ThermalMonitor(ProcessTaskBase): Initializer for ThermalMonitor :param chassis: Object representing a platform chassis """ - ProcessTaskBase.__init__(self) + super(ThermalMonitor, self).__init__() self.wait_time = self.INITIAL_INTERVAL From 8b480d599077cf42002eb8cba49545496c56cc7b Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Sat, 27 Feb 2021 00:56:07 +0000 Subject: [PATCH 20/43] Go back to deinitializer methods There appears to be a circular dependency which prevents the ThermalControlDaemon object in main from being garbage collected when we add a custom destructor --- sonic-thermalctld/scripts/thermalctld | 24 +++++++++++---------- sonic-thermalctld/tests/test_thermalctld.py | 2 ++ 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/sonic-thermalctld/scripts/thermalctld b/sonic-thermalctld/scripts/thermalctld index 29e085397..ce03a67b5 100644 --- a/sonic-thermalctld/scripts/thermalctld +++ b/sonic-thermalctld/scripts/thermalctld @@ -205,16 +205,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 __del__(self): + def deinit(self): """ - Destructor of FanUpdater + Deinitializer of FanUpdater :return: """ for name in self.fan_status_dict.keys(): self.table._del(name) - super(FanUpdater, self).__del__() - def _log_on_status_changed(self, normal_status, normal_log, abnormal_log): """ Log when any status changed @@ -534,9 +532,9 @@ class TemperatureUpdater(logger.Logger): chassis_state_db = daemon_base.db_connect("CHASSIS_STATE_DB") self.chassis_table = swsscommon.Table(chassis_state_db, table_name) - def __del__(self): + def deinit(self): """ - Destructor of TemperatureUpdater + Deinitializer of TemperatureUpdater :return: """ for name in self.temperature_status_dict.keys(): @@ -544,8 +542,6 @@ class TemperatureUpdater(logger.Logger): if self.is_chassis_system and self.chassis_table is not None: self.chassis_table._del(name) - super(TemperatureUpdater, self).__del__() - def _log_on_status_changed(self, normal_status, normal_log, abnormal_log): """ Log when any status changed @@ -715,6 +711,9 @@ class ThermalMonitor(ProcessTaskBase): self.logger.log_warning('Update fan and temperature status took {} seconds, ' 'there might be performance risk'.format(elapsed)) + self.fan_updater.deinit() + self.temperature_updater.deinit() + self.logger.log_info("Stop thermal monitoring loop") @@ -760,7 +759,10 @@ class ThermalControlDaemon(daemon_base.DaemonBase): self.log_error('Caught exception while initializing thermal manager - {}'.format(repr(e))) sys.exit(ERR_INIT_FAILED) - def __del__(self): + def deinit(self): + """ + Deinitializer of ThermalControlDaemon + """ try: if self.thermal_manager: self.thermal_manager.deinitialize() @@ -769,8 +771,6 @@ class ThermalControlDaemon(daemon_base.DaemonBase): self.thermal_monitor.task_stop() - super(ThermalControlDaemon, self).__del__() - # Override signal handler from DaemonBase def signal_handler(self, sig, frame): """ @@ -836,6 +836,8 @@ def main(): thermal_control.log_info("Shutting down with exit code {}...".format(exit_code)) + thermal_control.deinit() + return exit_code diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index d5075515a..56b567251 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -415,3 +415,5 @@ def test_signal_handler(): assert daemon_thermalctld.log_info.call_count == 0 assert daemon_thermalctld.stop_event.set.call_count == 0 assert thermalctld.exit_code == thermalctld.ERR_UNKNOWN + + daemon_thermalctld.deinit() From 487278f1df12d4b3c5a8dae8370b5b2ce605c3c3 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Sat, 27 Feb 2021 02:11:45 +0000 Subject: [PATCH 21/43] become -> became --- sonic-thermalctld/scripts/thermalctld | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sonic-thermalctld/scripts/thermalctld b/sonic-thermalctld/scripts/thermalctld index ce03a67b5..3fe0b0b60 100644 --- a/sonic-thermalctld/scripts/thermalctld +++ b/sonic-thermalctld/scripts/thermalctld @@ -126,7 +126,7 @@ class FanStatus(logger.Logger): return False if current_status is True: - self.log_warning('Fan speed or target_speed or tolerance become unavailable, ' + self.log_warning('Fan speed or target_speed or tolerance became unavailable, ' 'speed={}, target_speed={}, tolerance={}'.format(speed, target_speed, tolerance)) return False return True @@ -442,7 +442,7 @@ class TemperatureStatus(logger.Logger): """ if temperature == NOT_AVAILABLE: if self.temperature is not None: - self.log_warning('Temperature of {} become unavailable'.format(name)) + self.log_warning('Temperature of {} became unavailable'.format(name)) self.temperature = None return @@ -459,7 +459,7 @@ class TemperatureStatus(logger.Logger): def _check_temperature_value_available(self, temperature, threshold, current_status): if temperature == NOT_AVAILABLE or threshold == NOT_AVAILABLE: if current_status is True: - self.log_warning('Thermal temperature or threshold become unavailable, ' + self.log_warning('Thermal temperature or threshold became unavailable, ' 'temperature={}, threshold={}'.format(temperature, threshold)) return False return True From 873827ec0055726ad645774336cecf633f44b2b1 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Sat, 27 Feb 2021 02:20:23 +0000 Subject: [PATCH 22/43] Add tests for ThermalControlDaemon.run() and main() --- sonic-thermalctld/tests/test_thermalctld.py | 61 ++++++++++++++------- 1 file changed, 42 insertions(+), 19 deletions(-) diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index 56b567251..31737b210 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -361,59 +361,82 @@ def test_updater_thermal_check_min_max(): def test_signal_handler(): + + # Test SIGHUP daemon_thermalctld = thermalctld.ThermalControlDaemon() daemon_thermalctld.stop_event.set = mock.MagicMock() daemon_thermalctld.log_info = mock.MagicMock() daemon_thermalctld.log_warning = mock.MagicMock() - - # Test SIGHUP daemon_thermalctld.signal_handler(thermalctld.signal.SIGHUP, None) + daemon_thermalctld.deinit() # Deinit becuase the test will hang if we assert assert daemon_thermalctld.log_info.call_count == 1 daemon_thermalctld.log_info.assert_called_with("Caught signal 'SIGHUP' - ignoring...") assert daemon_thermalctld.log_warning.call_count == 0 assert daemon_thermalctld.stop_event.set.call_count == 0 assert thermalctld.exit_code == thermalctld.ERR_UNKNOWN - # Reset - daemon_thermalctld.log_info.reset_mock() - daemon_thermalctld.log_warning.reset_mock() - daemon_thermalctld.stop_event.set.reset_mock() - # Test SIGINT + daemon_thermalctld = thermalctld.ThermalControlDaemon() + daemon_thermalctld.stop_event.set = mock.MagicMock() + daemon_thermalctld.log_info = mock.MagicMock() + daemon_thermalctld.log_warning = mock.MagicMock() test_signal = thermalctld.signal.SIGINT daemon_thermalctld.signal_handler(test_signal, None) + daemon_thermalctld.deinit() # Deinit becuase the test will hang if we assert assert daemon_thermalctld.log_info.call_count == 1 daemon_thermalctld.log_info.assert_called_with("Caught signal 'SIGINT' - exiting...") assert daemon_thermalctld.log_warning.call_count == 0 assert daemon_thermalctld.stop_event.set.call_count == 1 assert thermalctld.exit_code == (128 + test_signal) - # Reset - daemon_thermalctld.log_info.reset_mock() - daemon_thermalctld.log_warning.reset_mock() - daemon_thermalctld.stop_event.set.reset_mock() - # Test SIGTERM + thermalctld.exit_code = thermalctld.ERR_UNKNOWN + daemon_thermalctld = thermalctld.ThermalControlDaemon() + daemon_thermalctld.stop_event.set = mock.MagicMock() + daemon_thermalctld.log_info = mock.MagicMock() + daemon_thermalctld.log_warning = mock.MagicMock() test_signal = thermalctld.signal.SIGTERM daemon_thermalctld.signal_handler(test_signal, None) + daemon_thermalctld.deinit() # Deinit becuase the test will hang if we assert assert daemon_thermalctld.log_info.call_count == 1 daemon_thermalctld.log_info.assert_called_with("Caught signal 'SIGTERM' - exiting...") assert daemon_thermalctld.log_warning.call_count == 0 assert daemon_thermalctld.stop_event.set.call_count == 1 assert thermalctld.exit_code == (128 + test_signal) - # Reset - daemon_thermalctld.log_info.reset_mock() - daemon_thermalctld.log_warning.reset_mock() - daemon_thermalctld.stop_event.set.reset_mock() - thermalctld.exit_code = thermalctld.ERR_UNKNOWN - # Test an unhandled signal + thermalctld.exit_code = thermalctld.ERR_UNKNOWN + daemon_thermalctld = thermalctld.ThermalControlDaemon() + daemon_thermalctld.stop_event.set = mock.MagicMock() + daemon_thermalctld.log_info = mock.MagicMock() + daemon_thermalctld.log_warning = mock.MagicMock() daemon_thermalctld.signal_handler(thermalctld.signal.SIGUSR1, None) + daemon_thermalctld.deinit() # Deinit becuase the test will hang if we assert assert daemon_thermalctld.log_warning.call_count == 1 daemon_thermalctld.log_warning.assert_called_with("Caught unhandled signal 'SIGUSR1' - ignoring...") assert daemon_thermalctld.log_info.call_count == 0 assert daemon_thermalctld.stop_event.set.call_count == 0 assert thermalctld.exit_code == thermalctld.ERR_UNKNOWN - daemon_thermalctld.deinit() + +def test_daemon_run(): + daemon_thermalctld = thermalctld.ThermalControlDaemon() + daemon_thermalctld.stop_event.wait = mock.MagicMock(return_value=True) + ret = daemon_thermalctld.run() + daemon_thermalctld.deinit() # Deinit becuase the test will hang if we assert + assert ret is False + + daemon_thermalctld = thermalctld.ThermalControlDaemon() + daemon_thermalctld.stop_event.wait = mock.MagicMock(return_value=False) + ret = daemon_thermalctld.run() + daemon_thermalctld.deinit() # Deinit becuase the test will hang if we assert + assert ret is True + + +@mock.patch('thermalctld.ThermalControlDaemon.run') +def test_main(mock_run): + mock_run.return_value = False + + ret = thermalctld.main() + assert mock_run.call_count == 1 + assert ret != 0 From 79151bfa98162acdaf84dcdd5a9bce4360cf6bb0 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Sat, 27 Feb 2021 05:14:52 +0000 Subject: [PATCH 23/43] Add test_temperupdater_deinit --- sonic-thermalctld/tests/test_thermalctld.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index 31737b210..a8c4c3501 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -252,6 +252,14 @@ def test_temperstatus_set_under_temper(): assert ret assert not temperature_status.under_temperature +def test_temperupdater_deinit(): + chassis = MockChassis() + temp_updater = thermalctld.TemperatureUpdater(chassis) + temp_updater.temperature_status_dict = { 'key1': 'value1', 'key2': 'value2' } + temp_updater.table._del = mock.MagicMock() + + temp_updater.deinit() + assert temp_updater.table._del.call_count == 2 def test_temperupdater_over_temper(): chassis = MockChassis() From 4d5f6ea5d2d111e405737daa8645b801ea27be58 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Sat, 27 Feb 2021 09:39:46 +0000 Subject: [PATCH 24/43] Grammar tweaks, pluralize message dynamically --- sonic-thermalctld/scripts/thermalctld | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sonic-thermalctld/scripts/thermalctld b/sonic-thermalctld/scripts/thermalctld index 3fe0b0b60..9372c2306 100644 --- a/sonic-thermalctld/scripts/thermalctld +++ b/sonic-thermalctld/scripts/thermalctld @@ -254,8 +254,8 @@ class FanUpdater(logger.Logger): bad_fan_count = FanStatus.get_bad_fan_count() if bad_fan_count > 0 and old_bad_fan_count != bad_fan_count: - self.log_warning("Insufficient number of working fans warning: {} fans are not working.".format( - bad_fan_count + self.log_warning("Insufficient number of working fans warning: {} fan{} not working.".format( + bad_fan_count, " is" if bad_fan_count == 1 else "s are" )) elif old_bad_fan_count > 0 and bad_fan_count == 0: self.log_notice("Insufficient number of working fans warning cleared: all fans are back to normal.") @@ -627,7 +627,7 @@ class TemperatureUpdater(logger.Logger): warning = False if temperature != NOT_AVAILABLE and temperature_status.set_over_temperature(temperature, high_threshold): self._log_on_status_changed(not temperature_status.over_temperature, - 'High temperature warning cleared: {} temperature restore to {}C, high threshold {}C.'. + 'High temperature warning cleared: {} temperature restored to {}C, high threshold {}C.'. format(name, temperature, high_threshold), 'High temperature warning: {} current temperature {}C, high threshold {}C'. format(name, temperature, high_threshold) @@ -636,7 +636,7 @@ class TemperatureUpdater(logger.Logger): if temperature != NOT_AVAILABLE and temperature_status.set_under_temperature(temperature, low_threshold): self._log_on_status_changed(not temperature_status.under_temperature, - 'Low temperature warning cleared: {} temperature restore to {}C, low threshold {}C.'. + 'Low temperature warning cleared: {} temperature restored to {}C, low threshold {}C.'. format(name, temperature, low_threshold), 'Low temperature warning: {} current temperature {}C, low threshold {}C'. format(name, temperature, low_threshold) From 0321f2a0feb3bd3f520ad2e1933ac2f29cafcc52 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Sat, 27 Feb 2021 07:46:09 +0000 Subject: [PATCH 25/43] Temp test --- sonic-thermalctld/tests/test_thermalctld.py | 69 ++++++++++++++++++--- 1 file changed, 61 insertions(+), 8 deletions(-) diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index a8c4c3501..811cebc90 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -125,11 +125,17 @@ def test_fanupdater_fan_absent(): fan_list = chassis.get_all_fans() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED assert fan_updater.log_warning.call_count == 1 + fan_updater.log_warning.assert_called_with('Fan removed warning: FanDrawer 0 FAN 1 was removed from the system, potential overheat hazard') fan_list[0].set_presence(True) fan_updater.update() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN - assert fan_updater.log_notice.call_count == 1 + assert fan_updater.log_notice.call_count == 2 + expected_calls = [ + mock.call('Fan removed warning cleared: FanDrawer 0 FAN 1 was inserted.'), + mock.call('Insufficient number of working fans warning cleared: all fans are back to normal.') + ] + assert fan_updater.log_notice.mock_calls == expected_calls def test_fanupdater_fan_fault(): @@ -139,12 +145,22 @@ def test_fanupdater_fan_fault(): fan_updater.update() fan_list = chassis.get_all_fans() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED - assert fan_updater.log_warning.call_count == 1 + assert fan_updater.log_warning.call_count == 2 + expected_calls = [ + mock.call('Fan fault warning: FanDrawer 0 FAN 1 is broken.'), + mock.call('Insufficient number of working fans warning: 1 fan is not working.') + ] + assert fan_updater.log_warning.mock_calls == expected_calls fan_list[0].set_status(True) fan_updater.update() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN - assert fan_updater.log_notice.call_count == 1 + assert fan_updater.log_notice.call_count == 2 + expected_calls = [ + mock.call('Fan fault warning cleared: FanDrawer 0 FAN 1 is back to normal.'), + mock.call('Insufficient number of working fans warning cleared: all fans are back to normal.') + ] + assert fan_updater.log_notice.mock_calls == expected_calls def test_fanupdater_fan_under_speed(): @@ -155,11 +171,13 @@ def test_fanupdater_fan_under_speed(): fan_list = chassis.get_all_fans() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED assert fan_updater.log_warning.call_count == 1 + fan_updater.log_warning.assert_called_with('Fan low speed warning: FanDrawer 0 FAN 1 current speed=1, target speed=2, tolerance=0.') fan_list[0].make_normal_speed() fan_updater.update() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN assert fan_updater.log_notice.call_count == 1 + fan_updater.log_notice.assert_called_with('Fan low speed warning cleared: FanDrawer 0 FAN 1 speed is back to normal.') def test_fanupdater_fan_over_speed(): @@ -198,19 +216,28 @@ def test_insufficient_fan_number(): fan_updater = thermalctld.FanUpdater(chassis) fan_updater.update() assert fan_updater.log_warning.call_count == 3 - fan_updater.log_warning.assert_called_with('Insufficient number of working fans warning: 2 fans are not working.') + expected_calls = [ + mock.call('Fan removed warning: FanDrawer 0 FAN 1 was removed from the system, potential overheat hazard'), + mock.call('Fan fault warning: FanDrawer 1 FAN 1 is broken.'), + mock.call('Insufficient number of working fans warning: 2 fans are not working.') + ] + assert fan_updater.log_warning.mock_calls == expected_calls fan_list = chassis.get_all_fans() fan_list[0].set_presence(True) fan_updater.update() assert fan_updater.log_notice.call_count == 1 - fan_updater.log_warning.assert_called_with('Insufficient number of working fans warning: 1 fans are not working.') + fan_updater.log_warning.assert_called_with('Insufficient number of working fans warning: 1 fan is not working.') fan_list[1].set_status(True) fan_updater.update() assert fan_updater.log_notice.call_count == 3 - fan_updater.log_notice.assert_called_with( - 'Insufficient number of working fans warning cleared: all fans are back to normal.') + expected_calls = [ + mock.call('Fan removed warning cleared: FanDrawer 0 FAN 1 was inserted.'), + mock.call('Fan fault warning cleared: FanDrawer 1 FAN 1 is back to normal.'), + mock.call('Insufficient number of working fans warning cleared: all fans are back to normal.') + ] + assert fan_updater.log_notice.mock_calls == expected_calls def test_temperature_status_set_over_temper(): @@ -260,6 +287,8 @@ def test_temperupdater_deinit(): temp_updater.deinit() assert temp_updater.table._del.call_count == 2 + expected_calls = [mock.call('key1'), mock.call('key2')] + temp_updater.table._del.assert_has_calls(expected_calls, any_order=True) def test_temperupdater_over_temper(): chassis = MockChassis() @@ -268,10 +297,12 @@ def test_temperupdater_over_temper(): temperature_updater.update() thermal_list = chassis.get_all_thermals() assert temperature_updater.log_warning.call_count == 1 + temperature_updater.log_warning.assert_called_with('High temperature warning: chassis 1 Thermal 1 current temperature 3C, high threshold 2C') thermal_list[0].make_normal_temper() temperature_updater.update() assert temperature_updater.log_notice.call_count == 1 + temperature_updater.log_notice.assert_called_with('High temperature warning cleared: chassis 1 Thermal 1 temperature restored to 2C, high threshold 3C.') def test_temperupdater_under_temper(): @@ -281,10 +312,13 @@ def test_temperupdater_under_temper(): temperature_updater.update() thermal_list = chassis.get_all_thermals() assert temperature_updater.log_warning.call_count == 1 + temperature_updater.log_warning.assert_called_with('Low temperature warning: chassis 1 Thermal 1 current temperature 1C, low threshold 2C') thermal_list[0].make_normal_temper() temperature_updater.update() assert temperature_updater.log_notice.call_count == 1 + temperature_updater.log_notice.assert_called_with('Low temperature warning cleared: chassis 1 Thermal 1 temperature restored to 2C, low threshold 1C.') + def test_update_fan_with_exception(): @@ -299,6 +333,12 @@ def test_update_fan_with_exception(): assert fan.get_status_led() == MockFan.STATUS_LED_COLOR_RED assert fan_updater.log_warning.call_count == 1 + # TODO: Clean this up once we no longer need to support Python 2 + if sys.version_info.major == 3: + fan_updater.log_warning.assert_called_with("Failed to update FAN status - Exception('Failed to get speed')") + else: + fan_updater.log_warning.assert_called_with("Failed to update FAN status - Exception('Failed to get speed',)") # Python 2 adds a trailing comma + def test_update_thermal_with_exception(): chassis = MockChassis() @@ -309,7 +349,20 @@ def test_update_thermal_with_exception(): temperature_updater = thermalctld.TemperatureUpdater(chassis) temperature_updater.update() - assert temperature_updater.log_warning.call_count == 1 + assert temperature_updater.log_warning.call_count == 2 + + # TODO: Clean this up once we no longer need to support Python 2 + if sys.version_info.major == 3: + expected_calls = [ + mock.call("Failed to update thermal status - Exception('Failed to get temperature')"), + mock.call('High temperature warning: chassis 1 Thermal 2 current temperature 3C, high threshold 2C') + ] + else: + expected_calls = [ + mock.call("Failed to update thermal status - Exception('Failed to get temperature',)"), # Python 2 adds a trailing comma + mock.call('High temperature warning: chassis 1 Thermal 2 current temperature 3C, high threshold 2C') + ] + assert temperature_updater.log_warning.mock_calls == expected_calls # Modular chassis related tests From b9ae0230aa02f420446d8340f215d9ae48c970c1 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Sat, 27 Feb 2021 21:22:00 +0000 Subject: [PATCH 26/43] Remove unnecessary periods from log messages to align all --- sonic-thermalctld/scripts/thermalctld | 24 ++++++++-------- sonic-thermalctld/tests/test_thermalctld.py | 32 ++++++++++----------- 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/sonic-thermalctld/scripts/thermalctld b/sonic-thermalctld/scripts/thermalctld index 9372c2306..3a1983a10 100644 --- a/sonic-thermalctld/scripts/thermalctld +++ b/sonic-thermalctld/scripts/thermalctld @@ -254,11 +254,11 @@ class FanUpdater(logger.Logger): bad_fan_count = FanStatus.get_bad_fan_count() if bad_fan_count > 0 and old_bad_fan_count != bad_fan_count: - self.log_warning("Insufficient number of working fans warning: {} fan{} not working.".format( + self.log_warning("Insufficient number of working fans warning: {} fan{} not working".format( bad_fan_count, " is" if bad_fan_count == 1 else "s are" )) elif old_bad_fan_count > 0 and bad_fan_count == 0: - self.log_notice("Insufficient number of working fans warning cleared: all fans are back to normal.") + self.log_notice("Insufficient number of working fans warning cleared: all fans are back to normal") self.log_debug("End fan updating") @@ -319,7 +319,7 @@ class FanUpdater(logger.Logger): if fan_status.set_presence(presence): set_led = True self._log_on_status_changed(fan_status.presence, - 'Fan removed warning cleared: {} was inserted.'.format(fan_name), + 'Fan removed warning cleared: {} was inserted'.format(fan_name), 'Fan removed warning: {} was removed from ' 'the system, potential overheat hazard'.format(fan_name) ) @@ -327,23 +327,23 @@ class FanUpdater(logger.Logger): if presence and fan_status.set_fault_status(fan_fault_status): set_led = True self._log_on_status_changed(fan_status.status, - 'Fan fault warning cleared: {} is back to normal.'.format(fan_name), - 'Fan fault warning: {} is broken.'.format(fan_name) + 'Fan fault warning cleared: {} is back to normal'.format(fan_name), + 'Fan fault warning: {} is broken'.format(fan_name) ) if presence and fan_status.set_under_speed(speed, speed_target, speed_tolerance): set_led = True self._log_on_status_changed(not fan_status.under_speed, - 'Fan low speed warning cleared: {} speed is back to normal.'.format(fan_name), - 'Fan low speed warning: {} current speed={}, target speed={}, tolerance={}.'. + 'Fan low speed warning cleared: {} speed is back to normal'.format(fan_name), + 'Fan low speed warning: {} current speed={}, target speed={}, tolerance={}'. format(fan_name, speed, speed_target, speed_tolerance) ) if presence and fan_status.set_over_speed(speed, speed_target, speed_tolerance): set_led = True self._log_on_status_changed(not fan_status.over_speed, - 'Fan high speed warning cleared: {} speed is back to normal.'.format(fan_name), - 'Fan high speed warning: {} target speed={}, current speed={}, tolerance={}.'. + 'Fan high speed warning cleared: {} speed is back to normal'.format(fan_name), + 'Fan high speed warning: {} target speed={}, current speed={}, tolerance={}'. format(fan_name, speed_target, speed, speed_tolerance) ) @@ -627,7 +627,7 @@ class TemperatureUpdater(logger.Logger): warning = False if temperature != NOT_AVAILABLE and temperature_status.set_over_temperature(temperature, high_threshold): self._log_on_status_changed(not temperature_status.over_temperature, - 'High temperature warning cleared: {} temperature restored to {}C, high threshold {}C.'. + 'High temperature warning cleared: {} temperature restored to {}C, high threshold {}C'. format(name, temperature, high_threshold), 'High temperature warning: {} current temperature {}C, high threshold {}C'. format(name, temperature, high_threshold) @@ -636,7 +636,7 @@ class TemperatureUpdater(logger.Logger): if temperature != NOT_AVAILABLE and temperature_status.set_under_temperature(temperature, low_threshold): self._log_on_status_changed(not temperature_status.under_temperature, - 'Low temperature warning cleared: {} temperature restored to {}C, low threshold {}C.'. + 'Low temperature warning cleared: {} temperature restored to {}C, low threshold {}C'. format(name, temperature, low_threshold), 'Low temperature warning: {} current temperature {}C, low threshold {}C'. format(name, temperature, low_threshold) @@ -754,7 +754,7 @@ class ThermalControlDaemon(daemon_base.DaemonBase): self.thermal_manager.load(ThermalControlDaemon.POLICY_FILE) self.thermal_manager.init_thermal_algorithm(chassis) except NotImplementedError: - self.log_warning('Thermal manager is not supported on this platform.') + self.log_warning('Thermal manager is not supported on this platform') except Exception as e: self.log_error('Caught exception while initializing thermal manager - {}'.format(repr(e))) sys.exit(ERR_INIT_FAILED) diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index 811cebc90..31efb11b8 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -132,8 +132,8 @@ def test_fanupdater_fan_absent(): assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN assert fan_updater.log_notice.call_count == 2 expected_calls = [ - mock.call('Fan removed warning cleared: FanDrawer 0 FAN 1 was inserted.'), - mock.call('Insufficient number of working fans warning cleared: all fans are back to normal.') + mock.call('Fan removed warning cleared: FanDrawer 0 FAN 1 was inserted'), + mock.call('Insufficient number of working fans warning cleared: all fans are back to normal') ] assert fan_updater.log_notice.mock_calls == expected_calls @@ -147,8 +147,8 @@ def test_fanupdater_fan_fault(): assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED assert fan_updater.log_warning.call_count == 2 expected_calls = [ - mock.call('Fan fault warning: FanDrawer 0 FAN 1 is broken.'), - mock.call('Insufficient number of working fans warning: 1 fan is not working.') + mock.call('Fan fault warning: FanDrawer 0 FAN 1 is broken'), + mock.call('Insufficient number of working fans warning: 1 fan is not working') ] assert fan_updater.log_warning.mock_calls == expected_calls @@ -157,8 +157,8 @@ def test_fanupdater_fan_fault(): assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN assert fan_updater.log_notice.call_count == 2 expected_calls = [ - mock.call('Fan fault warning cleared: FanDrawer 0 FAN 1 is back to normal.'), - mock.call('Insufficient number of working fans warning cleared: all fans are back to normal.') + mock.call('Fan fault warning cleared: FanDrawer 0 FAN 1 is back to normal'), + mock.call('Insufficient number of working fans warning cleared: all fans are back to normal') ] assert fan_updater.log_notice.mock_calls == expected_calls @@ -171,13 +171,13 @@ def test_fanupdater_fan_under_speed(): fan_list = chassis.get_all_fans() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED assert fan_updater.log_warning.call_count == 1 - fan_updater.log_warning.assert_called_with('Fan low speed warning: FanDrawer 0 FAN 1 current speed=1, target speed=2, tolerance=0.') + fan_updater.log_warning.assert_called_with('Fan low speed warning: FanDrawer 0 FAN 1 current speed=1, target speed=2, tolerance=0') fan_list[0].make_normal_speed() fan_updater.update() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN assert fan_updater.log_notice.call_count == 1 - fan_updater.log_notice.assert_called_with('Fan low speed warning cleared: FanDrawer 0 FAN 1 speed is back to normal.') + fan_updater.log_notice.assert_called_with('Fan low speed warning cleared: FanDrawer 0 FAN 1 speed is back to normal') def test_fanupdater_fan_over_speed(): @@ -218,8 +218,8 @@ def test_insufficient_fan_number(): assert fan_updater.log_warning.call_count == 3 expected_calls = [ mock.call('Fan removed warning: FanDrawer 0 FAN 1 was removed from the system, potential overheat hazard'), - mock.call('Fan fault warning: FanDrawer 1 FAN 1 is broken.'), - mock.call('Insufficient number of working fans warning: 2 fans are not working.') + mock.call('Fan fault warning: FanDrawer 1 FAN 1 is broken'), + mock.call('Insufficient number of working fans warning: 2 fans are not working') ] assert fan_updater.log_warning.mock_calls == expected_calls @@ -227,15 +227,15 @@ def test_insufficient_fan_number(): fan_list[0].set_presence(True) fan_updater.update() assert fan_updater.log_notice.call_count == 1 - fan_updater.log_warning.assert_called_with('Insufficient number of working fans warning: 1 fan is not working.') + fan_updater.log_warning.assert_called_with('Insufficient number of working fans warning: 1 fan is not working') fan_list[1].set_status(True) fan_updater.update() assert fan_updater.log_notice.call_count == 3 expected_calls = [ - mock.call('Fan removed warning cleared: FanDrawer 0 FAN 1 was inserted.'), - mock.call('Fan fault warning cleared: FanDrawer 1 FAN 1 is back to normal.'), - mock.call('Insufficient number of working fans warning cleared: all fans are back to normal.') + mock.call('Fan removed warning cleared: FanDrawer 0 FAN 1 was inserted'), + mock.call('Fan fault warning cleared: FanDrawer 1 FAN 1 is back to normal'), + mock.call('Insufficient number of working fans warning cleared: all fans are back to normal') ] assert fan_updater.log_notice.mock_calls == expected_calls @@ -302,7 +302,7 @@ def test_temperupdater_over_temper(): thermal_list[0].make_normal_temper() temperature_updater.update() assert temperature_updater.log_notice.call_count == 1 - temperature_updater.log_notice.assert_called_with('High temperature warning cleared: chassis 1 Thermal 1 temperature restored to 2C, high threshold 3C.') + temperature_updater.log_notice.assert_called_with('High temperature warning cleared: chassis 1 Thermal 1 temperature restored to 2C, high threshold 3C') def test_temperupdater_under_temper(): @@ -317,7 +317,7 @@ def test_temperupdater_under_temper(): thermal_list[0].make_normal_temper() temperature_updater.update() assert temperature_updater.log_notice.call_count == 1 - temperature_updater.log_notice.assert_called_with('Low temperature warning cleared: chassis 1 Thermal 1 temperature restored to 2C, low threshold 1C.') + temperature_updater.log_notice.assert_called_with('Low temperature warning cleared: chassis 1 Thermal 1 temperature restored to 2C, low threshold 1C') From 2e8409fd4b8c3bf18d81bd89c8deb92d51666b69 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Sat, 27 Feb 2021 21:35:48 +0000 Subject: [PATCH 27/43] Add test case for TemperatureStatus.set_temperature when temp is not available --- sonic-thermalctld/tests/test_thermalctld.py | 33 ++++++++++++++------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index 31efb11b8..f1774af11 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -241,23 +241,23 @@ def test_insufficient_fan_number(): def test_temperature_status_set_over_temper(): - temperatue_status = thermalctld.TemperatureStatus() - ret = temperatue_status.set_over_temperature(thermalctld.NOT_AVAILABLE, thermalctld.NOT_AVAILABLE) + temperature_status = thermalctld.TemperatureStatus() + ret = temperature_status.set_over_temperature(thermalctld.NOT_AVAILABLE, thermalctld.NOT_AVAILABLE) assert not ret - ret = temperatue_status.set_over_temperature(thermalctld.NOT_AVAILABLE, 0) + ret = temperature_status.set_over_temperature(thermalctld.NOT_AVAILABLE, 0) assert not ret - ret = temperatue_status.set_over_temperature(0, thermalctld.NOT_AVAILABLE) + ret = temperature_status.set_over_temperature(0, thermalctld.NOT_AVAILABLE) assert not ret - ret = temperatue_status.set_over_temperature(2, 1) + ret = temperature_status.set_over_temperature(2, 1) assert ret - assert temperatue_status.over_temperature + assert temperature_status.over_temperature - ret = temperatue_status.set_over_temperature(1, 2) + ret = temperature_status.set_over_temperature(1, 2) assert ret - assert not temperatue_status.over_temperature + assert not temperature_status.over_temperature def test_temperstatus_set_under_temper(): @@ -279,6 +279,18 @@ def test_temperstatus_set_under_temper(): assert ret assert not temperature_status.under_temperature + +def test_temperature_status_set_not_available(): + THERMAL_NAME = 'Chassis 1 Thermal 1' + temperature_status = thermalctld.TemperatureStatus() + temperature_status.temperature = 20.0 + + temperature_status.set_temperature(THERMAL_NAME, thermalctld.NOT_AVAILABLE) + assert temperature_status.temperature is None + assert temperature_status.log_warning.call_count == 1 + temperature_status.log_warning.assert_called_with('Temperature of {} became unavailable'.format(THERMAL_NAME)) + + def test_temperupdater_deinit(): chassis = MockChassis() temp_updater = thermalctld.TemperatureUpdater(chassis) @@ -290,6 +302,7 @@ def test_temperupdater_deinit(): expected_calls = [mock.call('key1'), mock.call('key2')] temp_updater.table._del.assert_has_calls(expected_calls, any_order=True) + def test_temperupdater_over_temper(): chassis = MockChassis() chassis.make_over_temper_thermal() @@ -320,7 +333,6 @@ def test_temperupdater_under_temper(): temperature_updater.log_notice.assert_called_with('Low temperature warning cleared: chassis 1 Thermal 1 temperature restored to 2C, low threshold 1C') - def test_update_fan_with_exception(): chassis = MockChassis() chassis.make_error_fan() @@ -364,7 +376,8 @@ def test_update_thermal_with_exception(): ] assert temperature_updater.log_warning.mock_calls == expected_calls -# Modular chassis related tests + +# Modular chassis-related tests def test_updater_thermal_check_modular_chassis(): From dd14af492abf05c182a6f41ef91d91f2ca656f16 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Sat, 27 Feb 2021 22:00:30 +0000 Subject: [PATCH 28/43] Add TestFanStatus class and test_check_speed_value_available method --- sonic-thermalctld/tests/test_thermalctld.py | 144 ++++++++++++-------- 1 file changed, 85 insertions(+), 59 deletions(-) diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index f1774af11..0d5fe357d 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -56,65 +56,91 @@ def configure_mocks(): thermalctld.TemperatureUpdater.log_warning.reset() -def test_fanstatus_set_presence(): - fan_status = thermalctld.FanStatus() - ret = fan_status.set_presence(True) - assert fan_status.presence - assert not ret - - ret = fan_status.set_presence(False) - assert not fan_status.presence - assert ret - - -def test_fanstatus_set_under_speed(): - fan_status = thermalctld.FanStatus() - ret = fan_status.set_under_speed(thermalctld.NOT_AVAILABLE, thermalctld.NOT_AVAILABLE, thermalctld.NOT_AVAILABLE) - assert not ret - - ret = fan_status.set_under_speed(thermalctld.NOT_AVAILABLE, thermalctld.NOT_AVAILABLE, 0) - assert not ret - - ret = fan_status.set_under_speed(thermalctld.NOT_AVAILABLE, 0, 0) - assert not ret - - ret = fan_status.set_under_speed(0, 0, 0) - assert not ret - - ret = fan_status.set_under_speed(80, 100, 19) - assert ret - assert fan_status.under_speed - assert not fan_status.is_ok() - - ret = fan_status.set_under_speed(81, 100, 19) - assert ret - assert not fan_status.under_speed - assert fan_status.is_ok() - - -def test_fanstatus_set_over_speed(): - fan_status = thermalctld.FanStatus() - ret = fan_status.set_over_speed(thermalctld.NOT_AVAILABLE, thermalctld.NOT_AVAILABLE, thermalctld.NOT_AVAILABLE) - assert not ret - - ret = fan_status.set_over_speed(thermalctld.NOT_AVAILABLE, thermalctld.NOT_AVAILABLE, 0) - assert not ret - - ret = fan_status.set_over_speed(thermalctld.NOT_AVAILABLE, 0, 0) - assert not ret - - ret = fan_status.set_over_speed(0, 0, 0) - assert not ret - - ret = fan_status.set_over_speed(120, 100, 19) - assert ret - assert fan_status.over_speed - assert not fan_status.is_ok() - - ret = fan_status.set_over_speed(120, 100, 21) - assert ret - assert not fan_status.over_speed - assert fan_status.is_ok() +class TestFanStatus(object): + """ + Test cases to cover functionality in FanStatus class + """ + def test_check_speed_value_available(self): + fan_status = thermalctld.FanStatus() + + ret = fan_status._check_speed_value_available(30, 32, 5, True) + assert ret == True + assert fan_status.log_warning.call_count == 0 + + ret = fan_status._check_speed_value_available(thermalctld.NOT_AVAILABLE, 32, 105, True) + assert ret == False + assert fan_status.log_warning.call_count == 1 + fan_status.log_warning.assert_called_with('Invalid tolerance value: 105') + + # Reset + fan_status.log_warning.reset_mock() + + ret = fan_status._check_speed_value_available(thermalctld.NOT_AVAILABLE, 32, 5, False) + assert ret == False + assert fan_status.log_warning.call_count == 0 + + ret = fan_status._check_speed_value_available(thermalctld.NOT_AVAILABLE, 32, 5, True) + assert ret == False + assert fan_status.log_warning.call_count == 1 + fan_status.log_warning.assert_called_with('Fan speed or target_speed or tolerance became unavailable, speed=N/A, target_speed=32, tolerance=5') + + def test_set_presence(self): + fan_status = thermalctld.FanStatus() + ret = fan_status.set_presence(True) + assert fan_status.presence + assert not ret + + ret = fan_status.set_presence(False) + assert not fan_status.presence + assert ret + + def test_set_under_speed(self): + fan_status = thermalctld.FanStatus() + ret = fan_status.set_under_speed(thermalctld.NOT_AVAILABLE, thermalctld.NOT_AVAILABLE, thermalctld.NOT_AVAILABLE) + assert not ret + + ret = fan_status.set_under_speed(thermalctld.NOT_AVAILABLE, thermalctld.NOT_AVAILABLE, 0) + assert not ret + + ret = fan_status.set_under_speed(thermalctld.NOT_AVAILABLE, 0, 0) + assert not ret + + ret = fan_status.set_under_speed(0, 0, 0) + assert not ret + + ret = fan_status.set_under_speed(80, 100, 19) + assert ret + assert fan_status.under_speed + assert not fan_status.is_ok() + + ret = fan_status.set_under_speed(81, 100, 19) + assert ret + assert not fan_status.under_speed + assert fan_status.is_ok() + + def test_set_over_speed(self): + fan_status = thermalctld.FanStatus() + ret = fan_status.set_over_speed(thermalctld.NOT_AVAILABLE, thermalctld.NOT_AVAILABLE, thermalctld.NOT_AVAILABLE) + assert not ret + + ret = fan_status.set_over_speed(thermalctld.NOT_AVAILABLE, thermalctld.NOT_AVAILABLE, 0) + assert not ret + + ret = fan_status.set_over_speed(thermalctld.NOT_AVAILABLE, 0, 0) + assert not ret + + ret = fan_status.set_over_speed(0, 0, 0) + assert not ret + + ret = fan_status.set_over_speed(120, 100, 19) + assert ret + assert fan_status.over_speed + assert not fan_status.is_ok() + + ret = fan_status.set_over_speed(120, 100, 21) + assert ret + assert not fan_status.over_speed + assert fan_status.is_ok() def test_fanupdater_fan_absent(): From c4de3fcbf8f224b2dde2c20fd7de5e10a87d55be Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Sat, 27 Feb 2021 22:16:53 +0000 Subject: [PATCH 29/43] Add test case for try_get() --- sonic-thermalctld/tests/test_thermalctld.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index 0d5fe357d..3e68a4dfc 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -533,6 +533,23 @@ def test_daemon_run(): assert ret is True +def test_try_get(): + def good_callback(): + return 'good result' + + def unimplemented_callback(): + raise NotImplementedError + + ret = thermalctld.try_get(good_callback) + assert ret == 'good result' + + ret = thermalctld.try_get(unimplemented_callback) + assert ret == thermalctld.NOT_AVAILABLE + + ret = thermalctld.try_get(unimplemented_callback, 'my default') + assert ret == 'my default' + + @mock.patch('thermalctld.ThermalControlDaemon.run') def test_main(mock_run): mock_run.return_value = False From 720b8b87aa8503e246600484c4d79966e6a5b9be Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Sat, 27 Feb 2021 22:30:24 +0000 Subject: [PATCH 30/43] Add TestFanUpdater class and test_deinit() method --- sonic-thermalctld/tests/test_thermalctld.py | 150 +++++++++++--------- 1 file changed, 82 insertions(+), 68 deletions(-) diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index 3e68a4dfc..70005c2d1 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -143,82 +143,96 @@ def test_set_over_speed(self): assert fan_status.is_ok() -def test_fanupdater_fan_absent(): - chassis = MockChassis() - chassis.make_absent_fan() - fan_updater = thermalctld.FanUpdater(chassis) - fan_updater.update() - fan_list = chassis.get_all_fans() - assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED - assert fan_updater.log_warning.call_count == 1 - fan_updater.log_warning.assert_called_with('Fan removed warning: FanDrawer 0 FAN 1 was removed from the system, potential overheat hazard') - - fan_list[0].set_presence(True) - fan_updater.update() - assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN - assert fan_updater.log_notice.call_count == 2 - expected_calls = [ - mock.call('Fan removed warning cleared: FanDrawer 0 FAN 1 was inserted'), - mock.call('Insufficient number of working fans warning cleared: all fans are back to normal') - ] - assert fan_updater.log_notice.mock_calls == expected_calls +class TestFanUpdater(object): + """ + Test cases to cover functionality in FanUpdater class + """ + def test_deinit(self): + fan_updater = thermalctld.FanUpdater(MockChassis()) + fan_updater.fan_status_dict = {'key1': 'value1', 'key2': 'value2'} + fan_updater.table._del = mock.MagicMock() + + fan_updater.deinit() + assert fan_updater.table._del.call_count == 2 + expected_calls = [mock.call('key1'), mock.call('key2')] + fan_updater.table._del.assert_has_calls(expected_calls, any_order=True) + + def test_fanupdater_fan_absent(self): + chassis = MockChassis() + chassis.make_absent_fan() + fan_updater = thermalctld.FanUpdater(chassis) + fan_updater.update() + fan_list = chassis.get_all_fans() + assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED + assert fan_updater.log_warning.call_count == 1 + fan_updater.log_warning.assert_called_with('Fan removed warning: FanDrawer 0 FAN 1 was removed from the system, potential overheat hazard') + + fan_list[0].set_presence(True) + fan_updater.update() + assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN + assert fan_updater.log_notice.call_count == 2 + expected_calls = [ + mock.call('Fan removed warning cleared: FanDrawer 0 FAN 1 was inserted'), + mock.call('Insufficient number of working fans warning cleared: all fans are back to normal') + ] + assert fan_updater.log_notice.mock_calls == expected_calls -def test_fanupdater_fan_fault(): - chassis = MockChassis() - chassis.make_faulty_fan() - fan_updater = thermalctld.FanUpdater(chassis) - fan_updater.update() - fan_list = chassis.get_all_fans() - assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED - assert fan_updater.log_warning.call_count == 2 - expected_calls = [ - mock.call('Fan fault warning: FanDrawer 0 FAN 1 is broken'), - mock.call('Insufficient number of working fans warning: 1 fan is not working') - ] - assert fan_updater.log_warning.mock_calls == expected_calls + def test_fanupdater_fan_fault(self): + chassis = MockChassis() + chassis.make_faulty_fan() + fan_updater = thermalctld.FanUpdater(chassis) + fan_updater.update() + fan_list = chassis.get_all_fans() + assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED + assert fan_updater.log_warning.call_count == 2 + expected_calls = [ + mock.call('Fan fault warning: FanDrawer 0 FAN 1 is broken'), + mock.call('Insufficient number of working fans warning: 1 fan is not working') + ] + assert fan_updater.log_warning.mock_calls == expected_calls - fan_list[0].set_status(True) - fan_updater.update() - assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN - assert fan_updater.log_notice.call_count == 2 - expected_calls = [ - mock.call('Fan fault warning cleared: FanDrawer 0 FAN 1 is back to normal'), - mock.call('Insufficient number of working fans warning cleared: all fans are back to normal') - ] - assert fan_updater.log_notice.mock_calls == expected_calls + fan_list[0].set_status(True) + fan_updater.update() + assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN + assert fan_updater.log_notice.call_count == 2 + expected_calls = [ + mock.call('Fan fault warning cleared: FanDrawer 0 FAN 1 is back to normal'), + mock.call('Insufficient number of working fans warning cleared: all fans are back to normal') + ] + assert fan_updater.log_notice.mock_calls == expected_calls -def test_fanupdater_fan_under_speed(): - chassis = MockChassis() - chassis.make_under_speed_fan() - fan_updater = thermalctld.FanUpdater(chassis) - fan_updater.update() - fan_list = chassis.get_all_fans() - assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED - assert fan_updater.log_warning.call_count == 1 - fan_updater.log_warning.assert_called_with('Fan low speed warning: FanDrawer 0 FAN 1 current speed=1, target speed=2, tolerance=0') + def test_fanupdater_fan_under_speed(self): + chassis = MockChassis() + chassis.make_under_speed_fan() + fan_updater = thermalctld.FanUpdater(chassis) + fan_updater.update() + fan_list = chassis.get_all_fans() + assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED + assert fan_updater.log_warning.call_count == 1 + fan_updater.log_warning.assert_called_with('Fan low speed warning: FanDrawer 0 FAN 1 current speed=1, target speed=2, tolerance=0') - fan_list[0].make_normal_speed() - fan_updater.update() - assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN - assert fan_updater.log_notice.call_count == 1 - fan_updater.log_notice.assert_called_with('Fan low speed warning cleared: FanDrawer 0 FAN 1 speed is back to normal') + fan_list[0].make_normal_speed() + fan_updater.update() + assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN + assert fan_updater.log_notice.call_count == 1 + fan_updater.log_notice.assert_called_with('Fan low speed warning cleared: FanDrawer 0 FAN 1 speed is back to normal') -def test_fanupdater_fan_over_speed(): - chassis = MockChassis() - chassis.make_over_speed_fan() - fan_updater = thermalctld.FanUpdater(chassis) - fan_updater.update() - fan_list = chassis.get_all_fans() - assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED - assert fan_updater.log_warning.call_count == 1 + def test_fanupdater_fan_over_speed(self): + chassis = MockChassis() + chassis.make_over_speed_fan() + fan_updater = thermalctld.FanUpdater(chassis) + fan_updater.update() + fan_list = chassis.get_all_fans() + assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED + assert fan_updater.log_warning.call_count == 1 - fan_list[0].make_normal_speed() - fan_updater.update() - assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN - assert fan_updater.log_notice.call_count == 1 + fan_list[0].make_normal_speed() + fan_updater.update() + assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN + assert fan_updater.log_notice.call_count == 1 def test_insufficient_fan_number(): @@ -320,7 +334,7 @@ def test_temperature_status_set_not_available(): def test_temperupdater_deinit(): chassis = MockChassis() temp_updater = thermalctld.TemperatureUpdater(chassis) - temp_updater.temperature_status_dict = { 'key1': 'value1', 'key2': 'value2' } + temp_updater.temperature_status_dict = {'key1': 'value1', 'key2': 'value2'} temp_updater.table._del = mock.MagicMock() temp_updater.deinit() From 53152058742f1f3c66e9f40642f8239d33019843 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Sat, 27 Feb 2021 22:46:37 +0000 Subject: [PATCH 31/43] Add test case for FanUpdater._refresh_fan_drawer_status --- sonic-thermalctld/tests/test_thermalctld.py | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index 70005c2d1..65bb33b8b 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -157,7 +157,18 @@ def test_deinit(self): expected_calls = [mock.call('key1'), mock.call('key2')] fan_updater.table._del.assert_has_calls(expected_calls, any_order=True) - def test_fanupdater_fan_absent(self): + @mock.patch('thermalctld.try_get', mock.MagicMock(return_value=thermalctld.NOT_AVAILABLE)) + def test_refresh_fan_drawer_status_fan_drawer_get_name_not_impl(self): + # Test case where fan_drawer.get_name is not implemented + fan_updater = thermalctld.FanUpdater(MockChassis()) + thermalctld.update_entity_info = mock.MagicMock() + mock_fan_drawer = mock.MagicMock() + fan_updater._refresh_fan_drawer_status(mock_fan_drawer, 1) + assert thermalctld.update_entity_info.call_count == 0 + + # TODO: Add a test case for _refresh_fan_drawer_status with a good fan drawer + + def test_fan_absent(self): chassis = MockChassis() chassis.make_absent_fan() fan_updater = thermalctld.FanUpdater(chassis) @@ -178,7 +189,7 @@ def test_fanupdater_fan_absent(self): assert fan_updater.log_notice.mock_calls == expected_calls - def test_fanupdater_fan_fault(self): + def test_fan_faulty(self): chassis = MockChassis() chassis.make_faulty_fan() fan_updater = thermalctld.FanUpdater(chassis) @@ -203,7 +214,7 @@ def test_fanupdater_fan_fault(self): assert fan_updater.log_notice.mock_calls == expected_calls - def test_fanupdater_fan_under_speed(self): + def test_fan_under_speed(self): chassis = MockChassis() chassis.make_under_speed_fan() fan_updater = thermalctld.FanUpdater(chassis) @@ -220,7 +231,7 @@ def test_fanupdater_fan_under_speed(self): fan_updater.log_notice.assert_called_with('Fan low speed warning cleared: FanDrawer 0 FAN 1 speed is back to normal') - def test_fanupdater_fan_over_speed(self): + def test_fan_over_speed(self): chassis = MockChassis() chassis.make_over_speed_fan() fan_updater = thermalctld.FanUpdater(chassis) From f019f503f047d59c9c4bda853eef6e5f4c88d073 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Sat, 27 Feb 2021 23:17:53 +0000 Subject: [PATCH 32/43] Fix grammar, add details to log messages --- sonic-thermalctld/scripts/thermalctld | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sonic-thermalctld/scripts/thermalctld b/sonic-thermalctld/scripts/thermalctld index 3a1983a10..5b65c3607 100644 --- a/sonic-thermalctld/scripts/thermalctld +++ b/sonic-thermalctld/scripts/thermalctld @@ -392,7 +392,7 @@ class FanUpdater(logger.Logger): fan.set_status_led(fan.STATUS_LED_COLOR_RED) fan_drawer.set_status_led(fan.STATUS_LED_COLOR_RED) except NotImplementedError as e: - self.log_warning('Failed to set led to fan, set_status_led not implemented') + self.log_warning('Failed to set status LED for fan {}, set_status_led not implemented'.format(fan_name)) def _update_led_color(self): for fan_name, fan_status in self.fan_status_dict.items(): @@ -401,7 +401,7 @@ class FanUpdater(logger.Logger): ('led_status', str(try_get(fan_status.fan.get_status_led))) ]) except Exception as e: - self.log_warning('Failed to get led status for fan - {}'.format(repr(e))) + self.log_warning('Failed to get status LED state for fan {} - {}'.format(fan_name, repr(e))) fvs = swsscommon.FieldValuePairs([ ('led_status', NOT_AVAILABLE) ]) @@ -416,7 +416,7 @@ class FanUpdater(logger.Logger): ('led_status', str(try_get(drawer.get_status_led))) ]) except Exception as e: - self.log_warning('Failed to get led status for fan drawer') + self.log_warning('Failed to get status LED state for fan drawer') fvs = swsscommon.FieldValuePairs([ ('led_status', NOT_AVAILABLE) ]) From 4f5db27baaff0e7b9cc5b564358256d5340e9563 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Sun, 28 Feb 2021 00:37:28 +0000 Subject: [PATCH 33/43] Add test case for update_entity_info() --- sonic-thermalctld/tests/test_thermalctld.py | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index 65bb33b8b..63d50a94b 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -158,10 +158,10 @@ def test_deinit(self): fan_updater.table._del.assert_has_calls(expected_calls, any_order=True) @mock.patch('thermalctld.try_get', mock.MagicMock(return_value=thermalctld.NOT_AVAILABLE)) + @mock.patch('thermalctld.update_entity_info', mock.MagicMock()) def test_refresh_fan_drawer_status_fan_drawer_get_name_not_impl(self): # Test case where fan_drawer.get_name is not implemented fan_updater = thermalctld.FanUpdater(MockChassis()) - thermalctld.update_entity_info = mock.MagicMock() mock_fan_drawer = mock.MagicMock() fan_updater._refresh_fan_drawer_status(mock_fan_drawer, 1) assert thermalctld.update_entity_info.call_count == 0 @@ -575,6 +575,19 @@ def unimplemented_callback(): assert ret == 'my default' +def test_update_entity_info(): + mock_table = mock.MagicMock() + mock_fan = MockFan() + expected_fvp = thermalctld.swsscommon.FieldValuePairs( + [('position_in_parent', '1'), + ('parent_name', 'Parent Name') + ]) + + thermalctld.update_entity_info(mock_table, 'Parent Name', 'Key Name', mock_fan, 1) + assert mock_table.set.call_count == 1 + mock_table.set.assert_called_with('Key Name', expected_fvp) + + @mock.patch('thermalctld.ThermalControlDaemon.run') def test_main(mock_run): mock_run.return_value = False From 006cbc96229fdbe64b1181f244a72837c3098f1c Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Sun, 28 Feb 2021 02:32:59 +0000 Subject: [PATCH 34/43] Add test_set_fan_led_exception --- sonic-thermalctld/tests/test_thermalctld.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index 63d50a94b..45220beeb 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -168,6 +168,17 @@ def test_refresh_fan_drawer_status_fan_drawer_get_name_not_impl(self): # TODO: Add a test case for _refresh_fan_drawer_status with a good fan drawer + def test_set_fan_led_exception(self): + fan_status = thermalctld.FanStatus() + mock_fan_drawer = mock.MagicMock() + mock_fan = MockFan() + mock_fan.set_status_led = mock.MagicMock(side_effect=NotImplementedError) + + fan_updater = thermalctld.FanUpdater(MockChassis()) + fan_updater._set_fan_led(mock_fan_drawer, mock_fan, 'Test Fan', fan_status) + assert fan_updater.log_warning.call_count == 1 + fan_updater.log_warning.assert_called_with('Failed to set status LED for fan Test Fan, set_status_led not implemented') + def test_fan_absent(self): chassis = MockChassis() chassis.make_absent_fan() From 637822bf81cb3ae1a94b4cf3854ce6f41b396dde Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Sun, 28 Feb 2021 03:59:35 +0000 Subject: [PATCH 35/43] [ThermalMonitor] Break loop logic out into separate function for unit testing purposes --- sonic-thermalctld/scripts/thermalctld | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/sonic-thermalctld/scripts/thermalctld b/sonic-thermalctld/scripts/thermalctld index 5b65c3607..0cb578ae1 100644 --- a/sonic-thermalctld/scripts/thermalctld +++ b/sonic-thermalctld/scripts/thermalctld @@ -689,6 +689,20 @@ class ThermalMonitor(ProcessTaskBase): self.fan_updater = FanUpdater(chassis) self.temperature_updater = TemperatureUpdater(chassis) + def main(self): + begin = time.time() + self.fan_updater.update() + self.temperature_updater.update() + elapsed = time.time() - begin + if elapsed < self.UPDATE_INTERVAL: + self.wait_time = self.UPDATE_INTERVAL - elapsed + else: + self.wait_time = self.INITIAL_INTERVAL + + if elapsed > self.UPDATE_ELAPSED_THRESHOLD: + self.logger.log_warning('Update fan and temperature status took {} seconds, ' + 'there might be performance risk'.format(elapsed)) + def task_worker(self): """ Thread function to handle Fan status update and temperature status update @@ -698,18 +712,7 @@ class ThermalMonitor(ProcessTaskBase): # Start loop to update fan, temperature info in DB periodically while not self.task_stopping_event.wait(self.wait_time): - begin = time.time() - self.fan_updater.update() - self.temperature_updater.update() - elapsed = time.time() - begin - if elapsed < self.UPDATE_INTERVAL: - self.wait_time = self.UPDATE_INTERVAL - elapsed - else: - self.wait_time = self.INITIAL_INTERVAL - - if elapsed > self.UPDATE_ELAPSED_THRESHOLD: - self.logger.log_warning('Update fan and temperature status took {} seconds, ' - 'there might be performance risk'.format(elapsed)) + self.main() self.fan_updater.deinit() self.temperature_updater.deinit() From 74447f75bf758c86266076f40c519edbd89e7a92 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Sun, 28 Feb 2021 04:05:01 +0000 Subject: [PATCH 36/43] Add TestThermalMonitor class and test_main() method --- sonic-thermalctld/tests/test_thermalctld.py | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index 45220beeb..6da08e025 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -199,7 +199,6 @@ def test_fan_absent(self): ] assert fan_updater.log_notice.mock_calls == expected_calls - def test_fan_faulty(self): chassis = MockChassis() chassis.make_faulty_fan() @@ -224,7 +223,6 @@ def test_fan_faulty(self): ] assert fan_updater.log_notice.mock_calls == expected_calls - def test_fan_under_speed(self): chassis = MockChassis() chassis.make_under_speed_fan() @@ -241,7 +239,6 @@ def test_fan_under_speed(self): assert fan_updater.log_notice.call_count == 1 fan_updater.log_notice.assert_called_with('Fan low speed warning cleared: FanDrawer 0 FAN 1 speed is back to normal') - def test_fan_over_speed(self): chassis = MockChassis() chassis.make_over_speed_fan() @@ -257,6 +254,21 @@ def test_fan_over_speed(self): assert fan_updater.log_notice.call_count == 1 +class TestThermalMonitor(object): + """ + Test cases to cover functionality in ThermalMonitor class + """ + def test_main(self): + mock_chassis = MockChassis() + thermal_monitor = thermalctld.ThermalMonitor(mock_chassis) + thermal_monitor.fan_updater.update = mock.MagicMock() + thermal_monitor.temperature_updater.update = mock.MagicMock() + + thermal_monitor.main() + assert thermal_monitor.fan_updater.update.call_count == 1 + assert thermal_monitor.temperature_updater.update.call_count == 1 + + def test_insufficient_fan_number(): fan_status1 = thermalctld.FanStatus() fan_status2 = thermalctld.FanStatus() From 04f08383bc8f4a1ce274fa86e8ff5317c8be69fc Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Sun, 28 Feb 2021 04:41:35 +0000 Subject: [PATCH 37/43] [TestFanUpdater] Add PSU fan test --- sonic-thermalctld/scripts/thermalctld | 6 +-- sonic-thermalctld/tests/test_thermalctld.py | 45 +++++++++++++++------ 2 files changed, 35 insertions(+), 16 deletions(-) diff --git a/sonic-thermalctld/scripts/thermalctld b/sonic-thermalctld/scripts/thermalctld index 0cb578ae1..516714912 100644 --- a/sonic-thermalctld/scripts/thermalctld +++ b/sonic-thermalctld/scripts/thermalctld @@ -241,14 +241,14 @@ class FanUpdater(logger.Logger): try: self._refresh_fan_status(drawer, drawer_index, fan, fan_index) except Exception as e: - self.log_warning('Failed to update FAN status - {}'.format(repr(e))) + self.log_warning('Failed to update fan status - {}'.format(repr(e))) for psu_index, psu in enumerate(self.chassis.get_all_psus()): for fan_index, fan in enumerate(psu.get_all_fans()): try: self._refresh_fan_status(psu, psu_index, fan, fan_index, True) except Exception as e: - self.log_warning('Failed to update PSU FAN status - {}'.format(repr(e))) + self.log_warning('Failed to update PSU fan status - {}'.format(repr(e))) self._update_led_color() @@ -294,7 +294,7 @@ class FanUpdater(logger.Logger): parent_name = 'PSU {}'.format(parent_index + 1) else: parent_name = drawer_name if drawer_name != NOT_AVAILABLE else CHASSIS_INFO_KEY - fan_name = try_get(fan.get_name, '{} FAN {}'.format(parent_name, fan_index + 1)) + fan_name = try_get(fan.get_name, '{} fan {}'.format(parent_name, fan_index + 1)) update_entity_info(self.phy_entity_table, parent_name, fan_name, fan, fan_index + 1) if fan_name not in self.fan_status_dict: self.fan_status_dict[fan_name] = FanStatus(fan, is_psu_fan) diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index 6da08e025..39643237a 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -11,7 +11,7 @@ import pytest from sonic_py_common import daemon_base -from .mock_platform import MockChassis, MockFan, MockThermal +from .mock_platform import MockChassis, MockFan, MockPsu, MockThermal daemon_base.db_connect = mock.MagicMock() @@ -187,14 +187,14 @@ def test_fan_absent(self): fan_list = chassis.get_all_fans() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED assert fan_updater.log_warning.call_count == 1 - fan_updater.log_warning.assert_called_with('Fan removed warning: FanDrawer 0 FAN 1 was removed from the system, potential overheat hazard') + fan_updater.log_warning.assert_called_with('Fan removed warning: FanDrawer 0 fan 1 was removed from the system, potential overheat hazard') fan_list[0].set_presence(True) fan_updater.update() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN assert fan_updater.log_notice.call_count == 2 expected_calls = [ - mock.call('Fan removed warning cleared: FanDrawer 0 FAN 1 was inserted'), + mock.call('Fan removed warning cleared: FanDrawer 0 fan 1 was inserted'), mock.call('Insufficient number of working fans warning cleared: all fans are back to normal') ] assert fan_updater.log_notice.mock_calls == expected_calls @@ -208,7 +208,7 @@ def test_fan_faulty(self): assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED assert fan_updater.log_warning.call_count == 2 expected_calls = [ - mock.call('Fan fault warning: FanDrawer 0 FAN 1 is broken'), + mock.call('Fan fault warning: FanDrawer 0 fan 1 is broken'), mock.call('Insufficient number of working fans warning: 1 fan is not working') ] assert fan_updater.log_warning.mock_calls == expected_calls @@ -218,7 +218,7 @@ def test_fan_faulty(self): assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN assert fan_updater.log_notice.call_count == 2 expected_calls = [ - mock.call('Fan fault warning cleared: FanDrawer 0 FAN 1 is back to normal'), + mock.call('Fan fault warning cleared: FanDrawer 0 fan 1 is back to normal'), mock.call('Insufficient number of working fans warning cleared: all fans are back to normal') ] assert fan_updater.log_notice.mock_calls == expected_calls @@ -231,13 +231,13 @@ def test_fan_under_speed(self): fan_list = chassis.get_all_fans() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED assert fan_updater.log_warning.call_count == 1 - fan_updater.log_warning.assert_called_with('Fan low speed warning: FanDrawer 0 FAN 1 current speed=1, target speed=2, tolerance=0') + fan_updater.log_warning.assert_called_with('Fan low speed warning: FanDrawer 0 fan 1 current speed=1, target speed=2, tolerance=0') fan_list[0].make_normal_speed() fan_updater.update() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN assert fan_updater.log_notice.call_count == 1 - fan_updater.log_notice.assert_called_with('Fan low speed warning cleared: FanDrawer 0 FAN 1 speed is back to normal') + fan_updater.log_notice.assert_called_with('Fan low speed warning cleared: FanDrawer 0 fan 1 speed is back to normal') def test_fan_over_speed(self): chassis = MockChassis() @@ -253,6 +253,25 @@ def test_fan_over_speed(self): assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN assert fan_updater.log_notice.call_count == 1 + def test_psu_fans(self): + chassis = MockChassis() + psu = MockPsu() + mock_fan = MockFan() + psu._fan_list.append(mock_fan) + chassis._psu_list.append(psu) + fan_updater = thermalctld.FanUpdater(chassis) + fan_updater.update() + assert fan_updater.log_warning.call_count == 0 + + fan_updater._refresh_fan_status = mock.MagicMock(side_effect=Exception("Test message")) + fan_updater.update() + assert fan_updater.log_warning.call_count == 1 + # TODO: Clean this up once we no longer need to support Python 2 + if sys.version_info.major == 3: + fan_updater.log_warning.assert_called_with("Failed to update PSU fan status - Exception('Test message')") + else: + fan_updater.log_warning.assert_called_with("Failed to update PSU fan status - Exception('Test message',)") # Python 2 adds a trailing comma + class TestThermalMonitor(object): """ @@ -291,8 +310,8 @@ def test_insufficient_fan_number(): fan_updater.update() assert fan_updater.log_warning.call_count == 3 expected_calls = [ - mock.call('Fan removed warning: FanDrawer 0 FAN 1 was removed from the system, potential overheat hazard'), - mock.call('Fan fault warning: FanDrawer 1 FAN 1 is broken'), + mock.call('Fan removed warning: FanDrawer 0 fan 1 was removed from the system, potential overheat hazard'), + mock.call('Fan fault warning: FanDrawer 1 fan 1 is broken'), mock.call('Insufficient number of working fans warning: 2 fans are not working') ] assert fan_updater.log_warning.mock_calls == expected_calls @@ -307,8 +326,8 @@ def test_insufficient_fan_number(): fan_updater.update() assert fan_updater.log_notice.call_count == 3 expected_calls = [ - mock.call('Fan removed warning cleared: FanDrawer 0 FAN 1 was inserted'), - mock.call('Fan fault warning cleared: FanDrawer 1 FAN 1 is back to normal'), + mock.call('Fan removed warning cleared: FanDrawer 0 fan 1 was inserted'), + mock.call('Fan fault warning cleared: FanDrawer 1 fan 1 is back to normal'), mock.call('Insufficient number of working fans warning cleared: all fans are back to normal') ] assert fan_updater.log_notice.mock_calls == expected_calls @@ -421,9 +440,9 @@ def test_update_fan_with_exception(): # TODO: Clean this up once we no longer need to support Python 2 if sys.version_info.major == 3: - fan_updater.log_warning.assert_called_with("Failed to update FAN status - Exception('Failed to get speed')") + fan_updater.log_warning.assert_called_with("Failed to update fan status - Exception('Failed to get speed')") else: - fan_updater.log_warning.assert_called_with("Failed to update FAN status - Exception('Failed to get speed',)") # Python 2 adds a trailing comma + fan_updater.log_warning.assert_called_with("Failed to update fan status - Exception('Failed to get speed',)") # Python 2 adds a trailing comma def test_update_thermal_with_exception(): From 41a3996f56790bd81bb037be6e5741a0d3bd28f7 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Sun, 28 Feb 2021 17:45:27 +0000 Subject: [PATCH 38/43] Add TestTemperatureUpdater class and test_update_psu_thermals() method; Reorganize --- sonic-thermalctld/tests/test_thermalctld.py | 178 +++++++++++--------- 1 file changed, 98 insertions(+), 80 deletions(-) diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index 39643237a..ca56ad08d 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -168,6 +168,24 @@ def test_refresh_fan_drawer_status_fan_drawer_get_name_not_impl(self): # TODO: Add a test case for _refresh_fan_drawer_status with a good fan drawer + def test_update_fan_with_exception(self): + chassis = MockChassis() + chassis.make_error_fan() + fan = MockFan() + fan.make_over_speed() + chassis.get_all_fans().append(fan) + + fan_updater = thermalctld.FanUpdater(chassis) + fan_updater.update() + assert fan.get_status_led() == MockFan.STATUS_LED_COLOR_RED + assert fan_updater.log_warning.call_count == 1 + + # TODO: Clean this up once we no longer need to support Python 2 + if sys.version_info.major == 3: + fan_updater.log_warning.assert_called_with("Failed to update fan status - Exception('Failed to get speed')") + else: + fan_updater.log_warning.assert_called_with("Failed to update fan status - Exception('Failed to get speed',)") # Python 2 adds a trailing comma + def test_set_fan_led_exception(self): fan_status = thermalctld.FanStatus() mock_fan_drawer = mock.MagicMock() @@ -253,7 +271,7 @@ def test_fan_over_speed(self): assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN assert fan_updater.log_notice.call_count == 1 - def test_psu_fans(self): + def test_update_psu_fans(self): chassis = MockChassis() psu = MockPsu() mock_fan = MockFan() @@ -384,90 +402,91 @@ def test_temperature_status_set_not_available(): temperature_status.log_warning.assert_called_with('Temperature of {} became unavailable'.format(THERMAL_NAME)) -def test_temperupdater_deinit(): - chassis = MockChassis() - temp_updater = thermalctld.TemperatureUpdater(chassis) - temp_updater.temperature_status_dict = {'key1': 'value1', 'key2': 'value2'} - temp_updater.table._del = mock.MagicMock() - - temp_updater.deinit() - assert temp_updater.table._del.call_count == 2 - expected_calls = [mock.call('key1'), mock.call('key2')] - temp_updater.table._del.assert_has_calls(expected_calls, any_order=True) - - -def test_temperupdater_over_temper(): - chassis = MockChassis() - chassis.make_over_temper_thermal() - temperature_updater = thermalctld.TemperatureUpdater(chassis) - temperature_updater.update() - thermal_list = chassis.get_all_thermals() - assert temperature_updater.log_warning.call_count == 1 - temperature_updater.log_warning.assert_called_with('High temperature warning: chassis 1 Thermal 1 current temperature 3C, high threshold 2C') - - thermal_list[0].make_normal_temper() - temperature_updater.update() - assert temperature_updater.log_notice.call_count == 1 - temperature_updater.log_notice.assert_called_with('High temperature warning cleared: chassis 1 Thermal 1 temperature restored to 2C, high threshold 3C') - - -def test_temperupdater_under_temper(): - chassis = MockChassis() - chassis.make_under_temper_thermal() - temperature_updater = thermalctld.TemperatureUpdater(chassis) - temperature_updater.update() - thermal_list = chassis.get_all_thermals() - assert temperature_updater.log_warning.call_count == 1 - temperature_updater.log_warning.assert_called_with('Low temperature warning: chassis 1 Thermal 1 current temperature 1C, low threshold 2C') - - thermal_list[0].make_normal_temper() - temperature_updater.update() - assert temperature_updater.log_notice.call_count == 1 - temperature_updater.log_notice.assert_called_with('Low temperature warning cleared: chassis 1 Thermal 1 temperature restored to 2C, low threshold 1C') - - -def test_update_fan_with_exception(): - chassis = MockChassis() - chassis.make_error_fan() - fan = MockFan() - fan.make_over_speed() - chassis.get_all_fans().append(fan) +class TestTemperatureUpdater(object): + """ + Test cases to cover functionality in TemperatureUpdater class + """ + def test_deinit(self): + chassis = MockChassis() + temp_updater = thermalctld.TemperatureUpdater(chassis) + temp_updater.temperature_status_dict = {'key1': 'value1', 'key2': 'value2'} + temp_updater.table._del = mock.MagicMock() - fan_updater = thermalctld.FanUpdater(chassis) - fan_updater.update() - assert fan.get_status_led() == MockFan.STATUS_LED_COLOR_RED - assert fan_updater.log_warning.call_count == 1 + temp_updater.deinit() + assert temp_updater.table._del.call_count == 2 + expected_calls = [mock.call('key1'), mock.call('key2')] + temp_updater.table._del.assert_has_calls(expected_calls, any_order=True) - # TODO: Clean this up once we no longer need to support Python 2 - if sys.version_info.major == 3: - fan_updater.log_warning.assert_called_with("Failed to update fan status - Exception('Failed to get speed')") - else: - fan_updater.log_warning.assert_called_with("Failed to update fan status - Exception('Failed to get speed',)") # Python 2 adds a trailing comma + def test_over_temper(self): + chassis = MockChassis() + chassis.make_over_temper_thermal() + temperature_updater = thermalctld.TemperatureUpdater(chassis) + temperature_updater.update() + thermal_list = chassis.get_all_thermals() + assert temperature_updater.log_warning.call_count == 1 + temperature_updater.log_warning.assert_called_with('High temperature warning: chassis 1 Thermal 1 current temperature 3C, high threshold 2C') + + thermal_list[0].make_normal_temper() + temperature_updater.update() + assert temperature_updater.log_notice.call_count == 1 + temperature_updater.log_notice.assert_called_with('High temperature warning cleared: chassis 1 Thermal 1 temperature restored to 2C, high threshold 3C') + + def test_under_temper(self): + chassis = MockChassis() + chassis.make_under_temper_thermal() + temperature_updater = thermalctld.TemperatureUpdater(chassis) + temperature_updater.update() + thermal_list = chassis.get_all_thermals() + assert temperature_updater.log_warning.call_count == 1 + temperature_updater.log_warning.assert_called_with('Low temperature warning: chassis 1 Thermal 1 current temperature 1C, low threshold 2C') + + thermal_list[0].make_normal_temper() + temperature_updater.update() + assert temperature_updater.log_notice.call_count == 1 + temperature_updater.log_notice.assert_called_with('Low temperature warning cleared: chassis 1 Thermal 1 temperature restored to 2C, low threshold 1C') + + def test_update_psu_thermals(self): + chassis = MockChassis() + psu = MockPsu() + mock_thermal = MockThermal() + psu._thermal_list.append(mock_thermal) + chassis._psu_list.append(psu) + temperature_updater = thermalctld.TemperatureUpdater(chassis) + temperature_updater.update() + assert temperature_updater.log_warning.call_count == 0 + temperature_updater._refresh_temperature_status = mock.MagicMock(side_effect=Exception("Test message")) + temperature_updater.update() + assert temperature_updater.log_warning.call_count == 1 + # TODO: Clean this up once we no longer need to support Python 2 + if sys.version_info.major == 3: + temperature_updater.log_warning.assert_called_with("Failed to update thermal status - Exception('Test message')") + else: + temperature_updater.log_warning.assert_called_with("Failed to update thermal status - Exception('Test message',)") # Python 2 adds a trailing comma -def test_update_thermal_with_exception(): - chassis = MockChassis() - chassis.make_error_thermal() - thermal = MockThermal() - thermal.make_over_temper() - chassis.get_all_thermals().append(thermal) + def test_update_thermal_with_exception(self): + chassis = MockChassis() + chassis.make_error_thermal() + thermal = MockThermal() + thermal.make_over_temper() + chassis.get_all_thermals().append(thermal) - temperature_updater = thermalctld.TemperatureUpdater(chassis) - temperature_updater.update() - assert temperature_updater.log_warning.call_count == 2 + temperature_updater = thermalctld.TemperatureUpdater(chassis) + temperature_updater.update() + assert temperature_updater.log_warning.call_count == 2 - # TODO: Clean this up once we no longer need to support Python 2 - if sys.version_info.major == 3: - expected_calls = [ - mock.call("Failed to update thermal status - Exception('Failed to get temperature')"), - mock.call('High temperature warning: chassis 1 Thermal 2 current temperature 3C, high threshold 2C') - ] - else: - expected_calls = [ - mock.call("Failed to update thermal status - Exception('Failed to get temperature',)"), # Python 2 adds a trailing comma - mock.call('High temperature warning: chassis 1 Thermal 2 current temperature 3C, high threshold 2C') - ] - assert temperature_updater.log_warning.mock_calls == expected_calls + # TODO: Clean this up once we no longer need to support Python 2 + if sys.version_info.major == 3: + expected_calls = [ + mock.call("Failed to update thermal status - Exception('Failed to get temperature')"), + mock.call('High temperature warning: chassis 1 Thermal 2 current temperature 3C, high threshold 2C') + ] + else: + expected_calls = [ + mock.call("Failed to update thermal status - Exception('Failed to get temperature',)"), # Python 2 adds a trailing comma + mock.call('High temperature warning: chassis 1 Thermal 2 current temperature 3C, high threshold 2C') + ] + assert temperature_updater.log_warning.mock_calls == expected_calls # Modular chassis-related tests @@ -528,7 +547,6 @@ def test_updater_thermal_check_min_max(): def test_signal_handler(): - # Test SIGHUP daemon_thermalctld = thermalctld.ThermalControlDaemon() daemon_thermalctld.stop_event.set = mock.MagicMock() From 89a797fdd38af75303b25492e033d20dcc26e7dc Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Sun, 28 Feb 2021 18:02:30 +0000 Subject: [PATCH 39/43] Remove 'repr()' around exception to make log message identical between Python 2 and 3 --- sonic-thermalctld/scripts/thermalctld | 14 ++++----- sonic-thermalctld/tests/test_thermalctld.py | 33 +++++---------------- 2 files changed, 14 insertions(+), 33 deletions(-) diff --git a/sonic-thermalctld/scripts/thermalctld b/sonic-thermalctld/scripts/thermalctld index 516714912..ea0cbc807 100644 --- a/sonic-thermalctld/scripts/thermalctld +++ b/sonic-thermalctld/scripts/thermalctld @@ -241,14 +241,14 @@ class FanUpdater(logger.Logger): try: self._refresh_fan_status(drawer, drawer_index, fan, fan_index) except Exception as e: - self.log_warning('Failed to update fan status - {}'.format(repr(e))) + self.log_warning('Failed to update fan status - {}'.format(e)) for psu_index, psu in enumerate(self.chassis.get_all_psus()): for fan_index, fan in enumerate(psu.get_all_fans()): try: self._refresh_fan_status(psu, psu_index, fan, fan_index, True) except Exception as e: - self.log_warning('Failed to update PSU fan status - {}'.format(repr(e))) + self.log_warning('Failed to update PSU fan status - {}'.format(e)) self._update_led_color() @@ -401,7 +401,7 @@ class FanUpdater(logger.Logger): ('led_status', str(try_get(fan_status.fan.get_status_led))) ]) except Exception as e: - self.log_warning('Failed to get status LED state for fan {} - {}'.format(fan_name, repr(e))) + self.log_warning('Failed to get status LED state for fan {} - {}'.format(fan_name, e)) fvs = swsscommon.FieldValuePairs([ ('led_status', NOT_AVAILABLE) ]) @@ -565,7 +565,7 @@ class TemperatureUpdater(logger.Logger): try: self._refresh_temperature_status(CHASSIS_INFO_KEY, thermal, index) except Exception as e: - self.log_warning('Failed to update thermal status - {}'.format(repr(e))) + self.log_warning('Failed to update thermal status - {}'.format(e)) for psu_index, psu in enumerate(self.chassis.get_all_psus()): parent_name = 'PSU {}'.format(psu_index + 1) @@ -759,7 +759,7 @@ class ThermalControlDaemon(daemon_base.DaemonBase): except NotImplementedError: self.log_warning('Thermal manager is not supported on this platform') except Exception as e: - self.log_error('Caught exception while initializing thermal manager - {}'.format(repr(e))) + self.log_error('Caught exception while initializing thermal manager - {}'.format(e)) sys.exit(ERR_INIT_FAILED) def deinit(self): @@ -770,7 +770,7 @@ class ThermalControlDaemon(daemon_base.DaemonBase): if self.thermal_manager: self.thermal_manager.deinitialize() except Exception as e: - self.log_error('Caught exception while destroying thermal manager - {}'.format(repr(e))) + self.log_error('Caught exception while destroying thermal manager - {}'.format(e)) self.thermal_monitor.task_stop() @@ -811,7 +811,7 @@ class ThermalControlDaemon(daemon_base.DaemonBase): if self.thermal_manager: self.thermal_manager.run_policy(chassis) except Exception as e: - self.log_error('Caught exception while running thermal policy - {}'.format(repr(e))) + self.log_error('Caught exception while running thermal policy - {}'.format(e)) elapsed = time.time() - begin if elapsed < self.INTERVAL: diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index ca56ad08d..728b23a55 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -180,11 +180,7 @@ def test_update_fan_with_exception(self): assert fan.get_status_led() == MockFan.STATUS_LED_COLOR_RED assert fan_updater.log_warning.call_count == 1 - # TODO: Clean this up once we no longer need to support Python 2 - if sys.version_info.major == 3: - fan_updater.log_warning.assert_called_with("Failed to update fan status - Exception('Failed to get speed')") - else: - fan_updater.log_warning.assert_called_with("Failed to update fan status - Exception('Failed to get speed',)") # Python 2 adds a trailing comma + fan_updater.log_warning.assert_called_with("Failed to update fan status - Failed to get speed") def test_set_fan_led_exception(self): fan_status = thermalctld.FanStatus() @@ -284,11 +280,7 @@ def test_update_psu_fans(self): fan_updater._refresh_fan_status = mock.MagicMock(side_effect=Exception("Test message")) fan_updater.update() assert fan_updater.log_warning.call_count == 1 - # TODO: Clean this up once we no longer need to support Python 2 - if sys.version_info.major == 3: - fan_updater.log_warning.assert_called_with("Failed to update PSU fan status - Exception('Test message')") - else: - fan_updater.log_warning.assert_called_with("Failed to update PSU fan status - Exception('Test message',)") # Python 2 adds a trailing comma + fan_updater.log_warning.assert_called_with("Failed to update PSU fan status - Test message") class TestThermalMonitor(object): @@ -458,11 +450,7 @@ def test_update_psu_thermals(self): temperature_updater._refresh_temperature_status = mock.MagicMock(side_effect=Exception("Test message")) temperature_updater.update() assert temperature_updater.log_warning.call_count == 1 - # TODO: Clean this up once we no longer need to support Python 2 - if sys.version_info.major == 3: - temperature_updater.log_warning.assert_called_with("Failed to update thermal status - Exception('Test message')") - else: - temperature_updater.log_warning.assert_called_with("Failed to update thermal status - Exception('Test message',)") # Python 2 adds a trailing comma + temperature_updater.log_warning.assert_called_with("Failed to update thermal status - Test message") def test_update_thermal_with_exception(self): chassis = MockChassis() @@ -475,17 +463,10 @@ def test_update_thermal_with_exception(self): temperature_updater.update() assert temperature_updater.log_warning.call_count == 2 - # TODO: Clean this up once we no longer need to support Python 2 - if sys.version_info.major == 3: - expected_calls = [ - mock.call("Failed to update thermal status - Exception('Failed to get temperature')"), - mock.call('High temperature warning: chassis 1 Thermal 2 current temperature 3C, high threshold 2C') - ] - else: - expected_calls = [ - mock.call("Failed to update thermal status - Exception('Failed to get temperature',)"), # Python 2 adds a trailing comma - mock.call('High temperature warning: chassis 1 Thermal 2 current temperature 3C, high threshold 2C') - ] + expected_calls = [ + mock.call("Failed to update thermal status - Failed to get temperature"), + mock.call('High temperature warning: chassis 1 Thermal 2 current temperature 3C, high threshold 2C') + ] assert temperature_updater.log_warning.mock_calls == expected_calls From 1c2c6312606ff228901d9ebc8aaa334de5948a82 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Sun, 28 Feb 2021 18:22:53 +0000 Subject: [PATCH 40/43] Check for more log messages --- sonic-thermalctld/tests/test_thermalctld.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index 728b23a55..3be2fd5e5 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -200,8 +200,12 @@ def test_fan_absent(self): fan_updater.update() fan_list = chassis.get_all_fans() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED - assert fan_updater.log_warning.call_count == 1 - fan_updater.log_warning.assert_called_with('Fan removed warning: FanDrawer 0 fan 1 was removed from the system, potential overheat hazard') + assert fan_updater.log_warning.call_count == 2 + expected_calls = [ + mock.call('Fan removed warning: FanDrawer 0 fan 1 was removed from the system, potential overheat hazard'), + mock.call('Insufficient number of working fans warning: 1 fan is not working') + ] + assert fan_updater.log_warning.mock_calls == expected_calls fan_list[0].set_presence(True) fan_updater.update() @@ -261,11 +265,13 @@ def test_fan_over_speed(self): fan_list = chassis.get_all_fans() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_RED assert fan_updater.log_warning.call_count == 1 + fan_updater.log_warning.assert_called_with('Fan high speed warning: FanDrawer 0 fan 1 target speed=1, current speed=2, tolerance=0') fan_list[0].make_normal_speed() fan_updater.update() assert fan_list[0].get_status_led() == MockFan.STATUS_LED_COLOR_GREEN assert fan_updater.log_notice.call_count == 1 + fan_updater.log_notice.assert_called_with('Fan high speed warning cleared: FanDrawer 0 fan 1 speed is back to normal') def test_update_psu_fans(self): chassis = MockChassis() From a5af74949a3c436fee517582fac6e2fac55f63bd Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Sun, 28 Feb 2021 19:49:16 +0000 Subject: [PATCH 41/43] Add MockSfpBase class to mock_platform.py --- sonic-thermalctld/tests/mock_platform.py | 78 ++++++++++++++++++------ 1 file changed, 58 insertions(+), 20 deletions(-) diff --git a/sonic-thermalctld/tests/mock_platform.py b/sonic-thermalctld/tests/mock_platform.py index 7326d28e5..117dcafcf 100644 --- a/sonic-thermalctld/tests/mock_platform.py +++ b/sonic-thermalctld/tests/mock_platform.py @@ -3,6 +3,7 @@ from sonic_platform_base import fan_drawer_base from sonic_platform_base import module_base from sonic_platform_base import psu_base +from sonic_platform_base import sfp_base from sonic_platform_base import thermal_base from sonic_platform_base.sonic_thermal_control import thermal_manager_base @@ -91,22 +92,28 @@ def get_speed(self): raise Exception('Failed to get speed') -class MockPsu(psu_base.PsuBase): - def __init__(self): - super(MockPsu, self).__init__() - self._name = None +class MockFanDrawer(fan_drawer_base.FanDrawerBase): + def __init__(self, index): + super(MockFanDrawer, self).__init__() + self._name = 'FanDrawer {}'.format(index) self._presence = True - self._model = 'PSU Model' - self._serial = 'PSU Serial' + self._model = 'Fan Drawer Model' + self._serial = 'Fan Drawer Serial' self._status = True self._position_in_parent = 1 self._replaceable = True - self._fan_list = [] + self._status_led = self.STATUS_LED_COLOR_RED def get_all_fans(self): return self._fan_list + def get_status_led(self): + return self._status_led + + def set_status_led(self, value): + self._status_led = value + # Methods inherited from DeviceBase class and related setters def get_name(self): return self._name @@ -136,28 +143,59 @@ def is_replaceable(self): return self._replaceable -class MockFanDrawer(fan_drawer_base.FanDrawerBase): - def __init__(self, index): - super(MockFanDrawer, self).__init__() - self._name = 'FanDrawer {}'.format(index) +class MockPsu(psu_base.PsuBase): + def __init__(self): + super(MockPsu, self).__init__() + self._name = None self._presence = True - self._model = 'Fan Drawer Model' - self._serial = 'Fan Drawer Serial' + self._model = 'PSU Model' + self._serial = 'PSU Serial' self._status = True self._position_in_parent = 1 self._replaceable = True - self._fan_list = [] - self._status_led = self.STATUS_LED_COLOR_RED - def get_all_fans(self): return self._fan_list - def get_status_led(self): - return self._status_led + # Methods inherited from DeviceBase class and related setters + def get_name(self): + return self._name - def set_status_led(self, value): - self._status_led = value + def get_presence(self): + return self._presence + + def set_presence(self, presence): + self._presence = presence + + def get_model(self): + return self._model + + def get_serial(self): + return self._serial + + def get_status(self): + return self._status + + def set_status(self, status): + self._status = status + + def get_position_in_parent(self): + return self._position_in_parent + + def is_replaceable(self): + return self._replaceable + + +class MockSfp(sfp_base.SfpBase): + def __init__(self): + super(MockSfp, self).__init__() + self._name = None + self._presence = True + self._model = 'SFP Model' + self._serial = 'SFP Serial' + self._status = True + self._position_in_parent = 1 + self._replaceable = True # Methods inherited from DeviceBase class and related setters def get_name(self): From 8e280945317988edb8b7d42b1ba1c884d4d5637c Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Sun, 28 Feb 2021 20:10:38 +0000 Subject: [PATCH 42/43] Add test_update_sfp_thermals --- sonic-thermalctld/tests/test_thermalctld.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index 3be2fd5e5..bdde3e385 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -11,7 +11,7 @@ import pytest from sonic_py_common import daemon_base -from .mock_platform import MockChassis, MockFan, MockPsu, MockThermal +from .mock_platform import MockChassis, MockFan, MockPsu, MockSfp, MockThermal daemon_base.db_connect = mock.MagicMock() @@ -458,6 +458,21 @@ def test_update_psu_thermals(self): assert temperature_updater.log_warning.call_count == 1 temperature_updater.log_warning.assert_called_with("Failed to update thermal status - Test message") + def test_update_sfp_thermals(self): + chassis = MockChassis() + sfp = MockSfp() + mock_thermal = MockThermal() + sfp._thermal_list.append(mock_thermal) + chassis._sfp_list.append(sfp) + temperature_updater = thermalctld.TemperatureUpdater(chassis) + temperature_updater.update() + assert temperature_updater.log_warning.call_count == 0 + + temperature_updater._refresh_temperature_status = mock.MagicMock(side_effect=Exception("Test message")) + temperature_updater.update() + assert temperature_updater.log_warning.call_count == 1 + temperature_updater.log_warning.assert_called_with("Failed to update thermal status - Test message") + def test_update_thermal_with_exception(self): chassis = MockChassis() chassis.make_error_thermal() From 3104fdf8f92a738839721d8305ecf8213cacdb06 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Thu, 18 Mar 2021 01:52:08 +0000 Subject: [PATCH 43/43] Switch back to printing 'repr(e)' --- sonic-thermalctld/scripts/thermalctld | 16 ++++---- sonic-thermalctld/tests/test_thermalctld.py | 42 +++++++++++++++++---- 2 files changed, 42 insertions(+), 16 deletions(-) diff --git a/sonic-thermalctld/scripts/thermalctld b/sonic-thermalctld/scripts/thermalctld index ea0cbc807..1da7d4328 100644 --- a/sonic-thermalctld/scripts/thermalctld +++ b/sonic-thermalctld/scripts/thermalctld @@ -241,14 +241,14 @@ class FanUpdater(logger.Logger): try: self._refresh_fan_status(drawer, drawer_index, fan, fan_index) except Exception as e: - self.log_warning('Failed to update fan status - {}'.format(e)) + self.log_warning('Failed to update fan status - {}'.format(repr(e))) for psu_index, psu in enumerate(self.chassis.get_all_psus()): for fan_index, fan in enumerate(psu.get_all_fans()): try: self._refresh_fan_status(psu, psu_index, fan, fan_index, True) except Exception as e: - self.log_warning('Failed to update PSU fan status - {}'.format(e)) + self.log_warning('Failed to update PSU fan status - {}'.format(repr(e))) self._update_led_color() @@ -565,7 +565,7 @@ class TemperatureUpdater(logger.Logger): try: self._refresh_temperature_status(CHASSIS_INFO_KEY, thermal, index) except Exception as e: - self.log_warning('Failed to update thermal status - {}'.format(e)) + self.log_warning('Failed to update thermal status - {}'.format(repr(e))) for psu_index, psu in enumerate(self.chassis.get_all_psus()): parent_name = 'PSU {}'.format(psu_index + 1) @@ -573,7 +573,7 @@ class TemperatureUpdater(logger.Logger): try: self._refresh_temperature_status(parent_name, thermal, thermal_index) except Exception as e: - self.log_warning('Failed to update thermal status - {}'.format(e)) + self.log_warning('Failed to update thermal status - {}'.format(repr(e))) for sfp_index, sfp in enumerate(self.chassis.get_all_sfps()): parent_name = 'SFP {}'.format(sfp_index + 1) @@ -581,7 +581,7 @@ class TemperatureUpdater(logger.Logger): try: self._refresh_temperature_status(parent_name, thermal, thermal_index) except Exception as e: - self.log_warning('Failed to update thermal status - {}'.format(e)) + self.log_warning('Failed to update thermal status - {}'.format(repr(e))) self.log_debug("End temperature updating") @@ -759,7 +759,7 @@ class ThermalControlDaemon(daemon_base.DaemonBase): except NotImplementedError: self.log_warning('Thermal manager is not supported on this platform') except Exception as e: - self.log_error('Caught exception while initializing thermal manager - {}'.format(e)) + self.log_error('Caught exception while initializing thermal manager - {}'.format(repr(e))) sys.exit(ERR_INIT_FAILED) def deinit(self): @@ -770,7 +770,7 @@ class ThermalControlDaemon(daemon_base.DaemonBase): if self.thermal_manager: self.thermal_manager.deinitialize() except Exception as e: - self.log_error('Caught exception while destroying thermal manager - {}'.format(e)) + self.log_error('Caught exception while destroying thermal manager - {}'.format(repr(e))) self.thermal_monitor.task_stop() @@ -811,7 +811,7 @@ class ThermalControlDaemon(daemon_base.DaemonBase): if self.thermal_manager: self.thermal_manager.run_policy(chassis) except Exception as e: - self.log_error('Caught exception while running thermal policy - {}'.format(e)) + self.log_error('Caught exception while running thermal policy - {}'.format(repr(e))) elapsed = time.time() - begin if elapsed < self.INTERVAL: diff --git a/sonic-thermalctld/tests/test_thermalctld.py b/sonic-thermalctld/tests/test_thermalctld.py index bdde3e385..d2f647384 100644 --- a/sonic-thermalctld/tests/test_thermalctld.py +++ b/sonic-thermalctld/tests/test_thermalctld.py @@ -180,7 +180,11 @@ def test_update_fan_with_exception(self): assert fan.get_status_led() == MockFan.STATUS_LED_COLOR_RED assert fan_updater.log_warning.call_count == 1 - fan_updater.log_warning.assert_called_with("Failed to update fan status - Failed to get speed") + # TODO: Clean this up once we no longer need to support Python 2 + if sys.version_info.major == 3: + fan_updater.log_warning.assert_called_with("Failed to update fan status - Exception('Failed to get speed')") + else: + fan_updater.log_warning.assert_called_with("Failed to update fan status - Exception('Failed to get speed',)") def test_set_fan_led_exception(self): fan_status = thermalctld.FanStatus() @@ -286,7 +290,12 @@ def test_update_psu_fans(self): fan_updater._refresh_fan_status = mock.MagicMock(side_effect=Exception("Test message")) fan_updater.update() assert fan_updater.log_warning.call_count == 1 - fan_updater.log_warning.assert_called_with("Failed to update PSU fan status - Test message") + + # TODO: Clean this up once we no longer need to support Python 2 + if sys.version_info.major == 3: + fan_updater.log_warning.assert_called_with("Failed to update PSU fan status - Exception('Test message')") + else: + fan_updater.log_warning.assert_called_with("Failed to update PSU fan status - Exception('Test message',)") class TestThermalMonitor(object): @@ -456,7 +465,12 @@ def test_update_psu_thermals(self): temperature_updater._refresh_temperature_status = mock.MagicMock(side_effect=Exception("Test message")) temperature_updater.update() assert temperature_updater.log_warning.call_count == 1 - temperature_updater.log_warning.assert_called_with("Failed to update thermal status - Test message") + + # TODO: Clean this up once we no longer need to support Python 2 + if sys.version_info.major == 3: + temperature_updater.log_warning.assert_called_with("Failed to update thermal status - Exception('Test message')") + else: + temperature_updater.log_warning.assert_called_with("Failed to update thermal status - Exception('Test message',)") def test_update_sfp_thermals(self): chassis = MockChassis() @@ -471,7 +485,12 @@ def test_update_sfp_thermals(self): temperature_updater._refresh_temperature_status = mock.MagicMock(side_effect=Exception("Test message")) temperature_updater.update() assert temperature_updater.log_warning.call_count == 1 - temperature_updater.log_warning.assert_called_with("Failed to update thermal status - Test message") + + # TODO: Clean this up once we no longer need to support Python 2 + if sys.version_info.major == 3: + temperature_updater.log_warning.assert_called_with("Failed to update thermal status - Exception('Test message')") + else: + temperature_updater.log_warning.assert_called_with("Failed to update thermal status - Exception('Test message',)") def test_update_thermal_with_exception(self): chassis = MockChassis() @@ -484,10 +503,17 @@ def test_update_thermal_with_exception(self): temperature_updater.update() assert temperature_updater.log_warning.call_count == 2 - expected_calls = [ - mock.call("Failed to update thermal status - Failed to get temperature"), - mock.call('High temperature warning: chassis 1 Thermal 2 current temperature 3C, high threshold 2C') - ] + # TODO: Clean this up once we no longer need to support Python 2 + if sys.version_info.major == 3: + expected_calls = [ + mock.call("Failed to update thermal status - Exception('Failed to get temperature')"), + mock.call('High temperature warning: chassis 1 Thermal 2 current temperature 3C, high threshold 2C') + ] + else: + expected_calls = [ + mock.call("Failed to update thermal status - Exception('Failed to get temperature',)"), + mock.call('High temperature warning: chassis 1 Thermal 2 current temperature 3C, high threshold 2C') + ] assert temperature_updater.log_warning.mock_calls == expected_calls