diff --git a/sonic_platform_base/module_base.py b/sonic_platform_base/module_base.py index bd33357..e8fb6af 100644 --- a/sonic_platform_base/module_base.py +++ b/sonic_platform_base/module_base.py @@ -13,6 +13,8 @@ import threading import contextlib import shutil +import subprocess +import os # PCI state database constants PCIE_DETACH_INFO_TABLE = "PCIE_DETACH_INFO" @@ -95,6 +97,9 @@ def __init__(self): # List of ASIC-derived objects representing all ASICs # visibile in PCI domain on the module self._asic_list = [] + + # Flag to indicate if the module is running on the host/container + self.is_host = self._is_host() @contextlib.contextmanager def _file_operation_lock(self, lock_file_path): @@ -614,6 +619,10 @@ def get_voltage_sensor(self, index): return voltage_sensor + def _is_host(self): + docker_env_file = '/.dockerenv' + return not os.path.exists(docker_env_file) + ############################################## # Current sensor methods ############################################## @@ -798,16 +807,25 @@ def handle_sensor_removal(self): module_name = self.get_name() source_file = f"/usr/share/sonic/platform/module_sensors_ignore_conf/ignore_sensors_{module_name}.conf" target_file = f"/etc/sensors.d/ignore_sensors_{module_name}.conf" + file_exists_command = ['test', '-f', source_file] + copy_command = ['cp', source_file, target_file] + container_command = ['docker', 'exec', 'pmon'] + restart_command = ['service', 'sensord', 'restart'] + + if self.is_host: + file_exists_command = container_command + file_exists_command + copy_command = container_command + copy_command + restart_command = container_command + restart_command # If source file does not exist, we dont need to copy it and restart sensord - if not os.path.exists(source_file): + if subprocess.call(file_exists_command, stdout=subprocess.DEVNULL) != 0: return True - shutil.copy2(source_file, target_file) + subprocess.call(copy_command, stdout=subprocess.DEVNULL) # Restart sensord with lock with self._sensord_operation_lock(): - os.system("service sensord restart") + subprocess.call(restart_command, stdout=subprocess.DEVNULL) return True except Exception as e: @@ -825,17 +843,26 @@ def handle_sensor_addition(self): try: module_name = self.get_name() target_file = f"/etc/sensors.d/ignore_sensors_{module_name}.conf" + remove_command = ['rm', target_file] + restart_command = ['service', 'sensord', 'restart'] + container_command = ['docker', 'exec', 'pmon'] + file_exists_command = ['test', '-f', target_file] + + if self.is_host: + file_exists_command = container_command + file_exists_command + remove_command = container_command + remove_command + restart_command = container_command + restart_command # If target file does not exist, we dont need to remove it and restart sensord - if not os.path.exists(target_file): + if subprocess.call(file_exists_command, stdout=subprocess.DEVNULL) != 0: return True # Remove the file - os.remove(target_file) + subprocess.call(remove_command, stdout=subprocess.DEVNULL) # Restart sensord with lock with self._sensord_operation_lock(): - os.system("service sensord restart") + subprocess.call(restart_command, stdout=subprocess.DEVNULL) return True except Exception as e: diff --git a/tests/module_base_test.py b/tests/module_base_test.py index 025849e..ec8d842 100644 --- a/tests/module_base_test.py +++ b/tests/module_base_test.py @@ -5,7 +5,7 @@ import fcntl from unittest.mock import patch, MagicMock, call from io import StringIO -import shutil +import subprocess class MockFile: def __init__(self, data=None): @@ -55,6 +55,17 @@ def test_module_base(self): assert exception_raised + def test_is_host_detection(self): + # Test when /.dockerenv does not exist - running on host + with patch('os.path.exists', return_value=False): + module = ModuleBase() + assert module.is_host is True + + # Test when /.dockerenv exists - running in container (inside pmon) + with patch('os.path.exists', return_value=True): + module = ModuleBase() + assert module.is_host is False + def test_sensors(self): module = ModuleBase() assert(module.get_num_voltage_sensors() == 0) @@ -173,76 +184,140 @@ def test_handle_pci_rescan(self): def test_handle_sensor_removal(self): module = ModuleBase() + # Test successful case on host - commands run via docker exec pmon to access container + with patch.object(module, 'get_name', return_value="DPU0"), \ + patch('subprocess.call') as mock_call, \ + patch.object(module, '_sensord_operation_lock') as mock_lock: + module.is_host = True + # First call to test -f (file exists) returns 0, second call is cp, third is service restart + mock_call.side_effect = [0, 0, 0] + assert module.handle_sensor_removal() is True + assert mock_call.call_count == 3 + # When on host, commands are prefixed with docker exec pmon to run inside container + mock_call.assert_any_call(['docker', 'exec', 'pmon', 'test', '-f', '/usr/share/sonic/platform/module_sensors_ignore_conf/ignore_sensors_DPU0.conf'], + stdout=subprocess.DEVNULL) + mock_call.assert_any_call(['docker', 'exec', 'pmon', 'cp', '/usr/share/sonic/platform/module_sensors_ignore_conf/ignore_sensors_DPU0.conf', + '/etc/sensors.d/ignore_sensors_DPU0.conf'], + stdout=subprocess.DEVNULL) + mock_call.assert_any_call(['docker', 'exec', 'pmon', 'service', 'sensord', 'restart'], + stdout=subprocess.DEVNULL) + mock_lock.assert_called_once() + + # Test successful case inside container - commands run directly without docker exec with patch.object(module, 'get_name', return_value="DPU0"), \ - patch('os.path.exists', return_value=True), \ - patch('shutil.copy2') as mock_copy, \ - patch('os.system') as mock_system, \ + patch('subprocess.call') as mock_call, \ patch.object(module, '_sensord_operation_lock') as mock_lock: + module.is_host = False + # First call to test -f (file exists) returns 0, second call is cp, third is service restart + mock_call.side_effect = [0, 0, 0] assert module.handle_sensor_removal() is True - mock_copy.assert_called_once_with("/usr/share/sonic/platform/module_sensors_ignore_conf/ignore_sensors_DPU0.conf", - "/etc/sensors.d/ignore_sensors_DPU0.conf") - mock_system.assert_called_once_with("service sensord restart") + assert mock_call.call_count == 3 + # When inside container, commands run directly without docker exec prefix + mock_call.assert_any_call(['test', '-f', '/usr/share/sonic/platform/module_sensors_ignore_conf/ignore_sensors_DPU0.conf'], + stdout=subprocess.DEVNULL) + mock_call.assert_any_call(['cp', '/usr/share/sonic/platform/module_sensors_ignore_conf/ignore_sensors_DPU0.conf', + '/etc/sensors.d/ignore_sensors_DPU0.conf'], + stdout=subprocess.DEVNULL) + mock_call.assert_any_call(['service', 'sensord', 'restart'], + stdout=subprocess.DEVNULL) mock_lock.assert_called_once() + # Test file does not exist - should return True but not call copy or restart with patch.object(module, 'get_name', return_value="DPU0"), \ - patch('os.path.exists', return_value=False), \ - patch('shutil.copy2') as mock_copy, \ - patch('os.system') as mock_system, \ + patch('subprocess.call') as mock_call, \ patch.object(module, '_sensord_operation_lock') as mock_lock: + module.is_host = True + # Return 1 to indicate file doesn't exist + mock_call.return_value = 1 assert module.handle_sensor_removal() is True - mock_copy.assert_not_called() - mock_system.assert_not_called() + # Only the file existence check should be called (with docker exec when on host) + mock_call.assert_called_once_with(['docker', 'exec', 'pmon', 'test', '-f', '/usr/share/sonic/platform/module_sensors_ignore_conf/ignore_sensors_DPU0.conf'], + stdout=subprocess.DEVNULL) mock_lock.assert_not_called() + # Test exception handling with patch.object(module, 'get_name', return_value="DPU0"), \ - patch('os.path.exists', return_value=True), \ - patch('shutil.copy2', side_effect=Exception("Copy failed")): + patch('subprocess.call', side_effect=Exception("Copy failed")): + module.is_host = True assert module.handle_sensor_removal() is False def test_handle_sensor_addition(self): module = ModuleBase() + # Test successful case on host - commands run via docker exec pmon to access container + with patch.object(module, 'get_name', return_value="DPU0"), \ + patch('subprocess.call') as mock_call, \ + patch.object(module, '_sensord_operation_lock') as mock_lock: + module.is_host = True + # First call to test -f (file exists) returns 0, second call is rm, third is service restart + mock_call.side_effect = [0, 0, 0] + assert module.handle_sensor_addition() is True + assert mock_call.call_count == 3 + # When on host, commands are prefixed with docker exec pmon to run inside container + mock_call.assert_any_call(['docker', 'exec', 'pmon', 'test', '-f', '/etc/sensors.d/ignore_sensors_DPU0.conf'], + stdout=subprocess.DEVNULL) + mock_call.assert_any_call(['docker', 'exec', 'pmon', 'rm', '/etc/sensors.d/ignore_sensors_DPU0.conf'], + stdout=subprocess.DEVNULL) + mock_call.assert_any_call(['docker', 'exec', 'pmon', 'service', 'sensord', 'restart'], + stdout=subprocess.DEVNULL) + mock_lock.assert_called_once() + + # Test successful case inside container - commands run directly without docker exec with patch.object(module, 'get_name', return_value="DPU0"), \ - patch('os.path.exists', return_value=True), \ - patch('os.remove') as mock_remove, \ - patch('os.system') as mock_system, \ + patch('subprocess.call') as mock_call, \ patch.object(module, '_sensord_operation_lock') as mock_lock: + module.is_host = False + # First call to test -f (file exists) returns 0, second call is rm, third is service restart + mock_call.side_effect = [0, 0, 0] assert module.handle_sensor_addition() is True - mock_remove.assert_called_once_with("/etc/sensors.d/ignore_sensors_DPU0.conf") - mock_system.assert_called_once_with("service sensord restart") + assert mock_call.call_count == 3 + # When inside container, commands run directly without docker exec prefix + mock_call.assert_any_call(['test', '-f', '/etc/sensors.d/ignore_sensors_DPU0.conf'], + stdout=subprocess.DEVNULL) + mock_call.assert_any_call(['rm', '/etc/sensors.d/ignore_sensors_DPU0.conf'], + stdout=subprocess.DEVNULL) + mock_call.assert_any_call(['service', 'sensord', 'restart'], + stdout=subprocess.DEVNULL) mock_lock.assert_called_once() + # Test file does not exist - should return True but not call remove or restart with patch.object(module, 'get_name', return_value="DPU0"), \ - patch('os.path.exists', return_value=False), \ - patch('os.remove') as mock_remove, \ - patch('os.system') as mock_system, \ + patch('subprocess.call') as mock_call, \ patch.object(module, '_sensord_operation_lock') as mock_lock: + module.is_host = True + # Return 1 to indicate file doesn't exist + mock_call.return_value = 1 assert module.handle_sensor_addition() is True - mock_remove.assert_not_called() - mock_system.assert_not_called() + # Only the file existence check should be called (with docker exec when on host) + mock_call.assert_called_once_with(['docker', 'exec', 'pmon', 'test', '-f', '/etc/sensors.d/ignore_sensors_DPU0.conf'], + stdout=subprocess.DEVNULL) mock_lock.assert_not_called() + # Test exception handling with patch.object(module, 'get_name', return_value="DPU0"), \ - patch('os.path.exists', return_value=True), \ - patch('os.remove', side_effect=Exception("Remove failed")): + patch('subprocess.call', side_effect=Exception("Remove failed")): + module.is_host = True assert module.handle_sensor_addition() is False def test_module_pre_shutdown(self): module = ModuleBase() # Test successful case - with patch.object(module, 'handle_pci_removal', return_value=True), \ - patch.object(module, 'handle_sensor_removal', return_value=True): + with patch.object(module, 'handle_sensor_removal', return_value=True) as mock_sensor, \ + patch.object(module, 'handle_pci_removal', return_value=True) as mock_pci: assert module.module_pre_shutdown() is True + # Verify sensor removal is called before PCI removal + mock_sensor.assert_called_once() + mock_pci.assert_called_once() - # Test PCI removal failure - with patch.object(module, 'handle_pci_removal', return_value=False), \ - patch.object(module, 'handle_sensor_removal', return_value=True): + # Test sensor removal failure + with patch.object(module, 'handle_sensor_removal', return_value=False), \ + patch.object(module, 'handle_pci_removal', return_value=True): assert module.module_pre_shutdown() is False - # Test sensor removal failure - with patch.object(module, 'handle_pci_removal', return_value=True), \ - patch.object(module, 'handle_sensor_removal', return_value=False): + # Test PCI removal failure + with patch.object(module, 'handle_sensor_removal', return_value=True), \ + patch.object(module, 'handle_pci_removal', return_value=False): assert module.module_pre_shutdown() is False def test_module_post_startup(self):