From b9b6ee40a6d242fa81688b89ef2e4f5352a80c3a Mon Sep 17 00:00:00 2001 From: gpunathilell Date: Mon, 6 Oct 2025 16:09:13 +0000 Subject: [PATCH 1/4] Host container differentiation --- sonic_platform_base/module_base.py | 38 ++++++-- tests/module_base_test.py | 137 ++++++++++++++++++++++------- 2 files changed, 135 insertions(+), 40 deletions(-) diff --git a/sonic_platform_base/module_base.py b/sonic_platform_base/module_base.py index bd333571a..4590072d7 100644 --- a/sonic_platform_base/module_base.py +++ b/sonic_platform_base/module_base.py @@ -13,6 +13,7 @@ import threading import contextlib import shutil +import subprocess # PCI state database constants PCIE_DETACH_INFO_TABLE = "PCIE_DETACH_INFO" @@ -95,6 +96,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_container = not self.__is_host() @contextlib.contextmanager def _file_operation_lock(self, lock_file_path): @@ -614,6 +618,9 @@ def get_voltage_sensor(self, index): return voltage_sensor + def __is_host(self): + return subprocess.call(["docker"]) == 0 + ############################################## # Current sensor methods ############################################## @@ -798,16 +805,26 @@ 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_container: + 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) != 0: return True - shutil.copy2(source_file, target_file) + subprocess.call(copy_command) # Restart sensord with lock with self._sensord_operation_lock(): - os.system("service sensord restart") + subprocess.call(restart_command) return True except Exception as e: @@ -825,17 +842,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_container: + 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) != 0: return True # Remove the file - os.remove(target_file) + subprocess.call(remove_command) # Restart sensord with lock with self._sensord_operation_lock(): - os.system("service sensord restart") + subprocess.call(restart_command) return True except Exception as e: diff --git a/tests/module_base_test.py b/tests/module_base_test.py index 025849e9f..cf7e83054 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_container_detection(self): + # Test when docker command succeeds (return code 0) - running on host + with patch('subprocess.call', return_value=0): + module = ModuleBase() + assert module._is_container is False + + # Test when docker command fails (return code != 0) - running in container + with patch('subprocess.call', return_value=1): + module = ModuleBase() + assert module._is_container is True + def test_sensors(self): module = ModuleBase() assert(module.get_num_voltage_sensors() == 0) @@ -173,76 +184,134 @@ def test_handle_pci_rescan(self): def test_handle_sensor_removal(self): module = ModuleBase() + # Test successful case on host - file exists and operations succeed + 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_container = 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 + assert mock_call.call_count == 3 + # Verify file existence check + mock_call.assert_any_call(['test', '-f', '/usr/share/sonic/platform/module_sensors_ignore_conf/ignore_sensors_DPU0.conf']) + # Verify copy command + 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']) + # Verify restart command + mock_call.assert_any_call(['service', 'sensord', 'restart']) + mock_lock.assert_called_once() + + # Test successful case in container - commands should be prefixed with 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_container = 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 - 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 + # Verify file existence check with docker exec prefix + mock_call.assert_any_call(['docker', 'exec', 'pmon', 'test', '-f', '/usr/share/sonic/platform/module_sensors_ignore_conf/ignore_sensors_DPU0.conf']) + # Verify copy command with docker exec prefix + 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']) + # Verify restart command with docker exec prefix + mock_call.assert_any_call(['docker', 'exec', 'pmon', 'service', 'sensord', 'restart']) 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_container = False + # 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 + mock_call.assert_called_once_with(['test', '-f', '/usr/share/sonic/platform/module_sensors_ignore_conf/ignore_sensors_DPU0.conf']) 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_container = False assert module.handle_sensor_removal() is False def test_handle_sensor_addition(self): module = ModuleBase() + # Test successful case on host - file exists and operations succeed + 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_container = 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 + assert mock_call.call_count == 3 + # Verify file existence check + mock_call.assert_any_call(['test', '-f', '/etc/sensors.d/ignore_sensors_DPU0.conf']) + # Verify remove command + mock_call.assert_any_call(['rm', '/etc/sensors.d/ignore_sensors_DPU0.conf']) + # Verify restart command + mock_call.assert_any_call(['service', 'sensord', 'restart']) + mock_lock.assert_called_once() + + # Test successful case in container - commands should be prefixed with 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_container = 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 - 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 + # Verify file existence check with docker exec prefix + mock_call.assert_any_call(['docker', 'exec', 'pmon', 'test', '-f', '/etc/sensors.d/ignore_sensors_DPU0.conf']) + # Verify remove command with docker exec prefix + mock_call.assert_any_call(['docker', 'exec', 'pmon', 'rm', '/etc/sensors.d/ignore_sensors_DPU0.conf']) + # Verify restart command with docker exec prefix + mock_call.assert_any_call(['docker', 'exec', 'pmon', 'service', 'sensord', 'restart']) 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_container = False + # 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 + mock_call.assert_called_once_with(['test', '-f', '/etc/sensors.d/ignore_sensors_DPU0.conf']) 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_container = False 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): From 3156205ea9699f7204eb567adfbf69fab29827be Mon Sep 17 00:00:00 2001 From: gpunathilell Date: Mon, 6 Oct 2025 16:46:40 +0000 Subject: [PATCH 2/4] Fix test --- sonic_platform_base/module_base.py | 24 +++---- tests/module_base_test.py | 100 ++++++++++++++++------------- 2 files changed, 70 insertions(+), 54 deletions(-) diff --git a/sonic_platform_base/module_base.py b/sonic_platform_base/module_base.py index 4590072d7..7b342d890 100644 --- a/sonic_platform_base/module_base.py +++ b/sonic_platform_base/module_base.py @@ -14,6 +14,7 @@ import contextlib import shutil import subprocess +import os # PCI state database constants PCIE_DETACH_INFO_TABLE = "PCIE_DETACH_INFO" @@ -98,7 +99,7 @@ def __init__(self): self._asic_list = [] # Flag to indicate if the module is running on the host/container - self._is_container = not self.__is_host() + self.is_host = self._is_host() @contextlib.contextmanager def _file_operation_lock(self, lock_file_path): @@ -618,8 +619,9 @@ def get_voltage_sensor(self, index): return voltage_sensor - def __is_host(self): - return subprocess.call(["docker"]) == 0 + def _is_host(self): + docker_env_file = '/.dockerenv' + return not os.path.exists(docker_env_file) ############################################## # Current sensor methods @@ -811,20 +813,20 @@ def handle_sensor_removal(self): restart_command = ['service', 'sensord', 'restart'] - if self._is_container: + 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 subprocess.call(file_exists_command) != 0: + if subprocess.call(file_exists_command, stdout=subprocess.DEVNULL) != 0: return True - subprocess.call(copy_command) + subprocess.call(copy_command, stdout=subprocess.DEVNULL) # Restart sensord with lock with self._sensord_operation_lock(): - subprocess.call(restart_command) + subprocess.call(restart_command, stdout=subprocess.DEVNULL) return True except Exception as e: @@ -847,21 +849,21 @@ def handle_sensor_addition(self): container_command = ['docker', 'exec', 'pmon'] file_exists_command = ['test', '-f', target_file] - if self._is_container: + 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 subprocess.call(file_exists_command) != 0: + if subprocess.call(file_exists_command, stdout=subprocess.DEVNULL) != 0: return True # Remove the file - subprocess.call(remove_command) + subprocess.call(remove_command, stdout=subprocess.DEVNULL) # Restart sensord with lock with self._sensord_operation_lock(): - subprocess.call(restart_command) + 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 cf7e83054..f618ff11d 100644 --- a/tests/module_base_test.py +++ b/tests/module_base_test.py @@ -55,16 +55,16 @@ def test_module_base(self): assert exception_raised - def test_is_container_detection(self): - # Test when docker command succeeds (return code 0) - running on host - with patch('subprocess.call', return_value=0): + 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_container is False + assert module.is_host is True - # Test when docker command fails (return code != 0) - running in container - with patch('subprocess.call', return_value=1): + # Test when /.dockerenv exists - running in container + with patch('os.path.exists', return_value=True): module = ModuleBase() - assert module._is_container is True + assert module.is_host is False def test_sensors(self): module = ModuleBase() @@ -188,54 +188,61 @@ def test_handle_sensor_removal(self): 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_container = False + 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 - # Verify file existence check - mock_call.assert_any_call(['test', '-f', '/usr/share/sonic/platform/module_sensors_ignore_conf/ignore_sensors_DPU0.conf']) - # Verify copy command + # Verify file existence check with stdout suppression + mock_call.assert_any_call(['test', '-f', '/usr/share/sonic/platform/module_sensors_ignore_conf/ignore_sensors_DPU0.conf'], + stdout=subprocess.DEVNULL) + # Verify copy command with stdout suppression 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']) - # Verify restart command - mock_call.assert_any_call(['service', 'sensord', 'restart']) + '/etc/sensors.d/ignore_sensors_DPU0.conf'], + stdout=subprocess.DEVNULL) + # Verify restart command with stdout suppression + mock_call.assert_any_call(['service', 'sensord', 'restart'], + stdout=subprocess.DEVNULL) mock_lock.assert_called_once() # Test successful case in container - commands should be prefixed with docker exec 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_container = True + 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 assert mock_call.call_count == 3 - # Verify file existence check with docker exec prefix - mock_call.assert_any_call(['docker', 'exec', 'pmon', 'test', '-f', '/usr/share/sonic/platform/module_sensors_ignore_conf/ignore_sensors_DPU0.conf']) - # Verify copy command with docker exec prefix + # Verify file existence check with docker exec prefix and stdout suppression + 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) + # Verify copy command with docker exec prefix and stdout suppression 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']) - # Verify restart command with docker exec prefix - mock_call.assert_any_call(['docker', 'exec', 'pmon', 'service', 'sensord', 'restart']) + '/etc/sensors.d/ignore_sensors_DPU0.conf'], + stdout=subprocess.DEVNULL) + # Verify restart command with docker exec prefix and stdout suppression + mock_call.assert_any_call(['docker', 'exec', 'pmon', '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('subprocess.call') as mock_call, \ patch.object(module, '_sensord_operation_lock') as mock_lock: - module._is_container = False + module.is_host = True # Return 1 to indicate file doesn't exist mock_call.return_value = 1 assert module.handle_sensor_removal() is True - # Only the file existence check should be called - mock_call.assert_called_once_with(['test', '-f', '/usr/share/sonic/platform/module_sensors_ignore_conf/ignore_sensors_DPU0.conf']) + # Only the file existence check should be called with stdout suppression + mock_call.assert_called_once_with(['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('subprocess.call', side_effect=Exception("Copy failed")): - module._is_container = False + module.is_host = True assert module.handle_sensor_removal() is False def test_handle_sensor_addition(self): @@ -245,52 +252,59 @@ def test_handle_sensor_addition(self): 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_container = False + 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 - # Verify file existence check - mock_call.assert_any_call(['test', '-f', '/etc/sensors.d/ignore_sensors_DPU0.conf']) - # Verify remove command - mock_call.assert_any_call(['rm', '/etc/sensors.d/ignore_sensors_DPU0.conf']) - # Verify restart command - mock_call.assert_any_call(['service', 'sensord', 'restart']) + # Verify file existence check with stdout suppression + mock_call.assert_any_call(['test', '-f', '/etc/sensors.d/ignore_sensors_DPU0.conf'], + stdout=subprocess.DEVNULL) + # Verify remove command with stdout suppression + mock_call.assert_any_call(['rm', '/etc/sensors.d/ignore_sensors_DPU0.conf'], + stdout=subprocess.DEVNULL) + # Verify restart command with stdout suppression + mock_call.assert_any_call(['service', 'sensord', 'restart'], + stdout=subprocess.DEVNULL) mock_lock.assert_called_once() # Test successful case in container - commands should be prefixed with docker exec 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_container = True + 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 assert mock_call.call_count == 3 - # Verify file existence check with docker exec prefix - mock_call.assert_any_call(['docker', 'exec', 'pmon', 'test', '-f', '/etc/sensors.d/ignore_sensors_DPU0.conf']) - # Verify remove command with docker exec prefix - mock_call.assert_any_call(['docker', 'exec', 'pmon', 'rm', '/etc/sensors.d/ignore_sensors_DPU0.conf']) - # Verify restart command with docker exec prefix - mock_call.assert_any_call(['docker', 'exec', 'pmon', 'service', 'sensord', 'restart']) + # Verify file existence check with docker exec prefix and stdout suppression + mock_call.assert_any_call(['docker', 'exec', 'pmon', 'test', '-f', '/etc/sensors.d/ignore_sensors_DPU0.conf'], + stdout=subprocess.DEVNULL) + # Verify remove command with docker exec prefix and stdout suppression + mock_call.assert_any_call(['docker', 'exec', 'pmon', 'rm', '/etc/sensors.d/ignore_sensors_DPU0.conf'], + stdout=subprocess.DEVNULL) + # Verify restart command with docker exec prefix and stdout suppression + mock_call.assert_any_call(['docker', 'exec', 'pmon', '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('subprocess.call') as mock_call, \ patch.object(module, '_sensord_operation_lock') as mock_lock: - module._is_container = False + module.is_host = True # Return 1 to indicate file doesn't exist mock_call.return_value = 1 assert module.handle_sensor_addition() is True - # Only the file existence check should be called - mock_call.assert_called_once_with(['test', '-f', '/etc/sensors.d/ignore_sensors_DPU0.conf']) + # Only the file existence check should be called with stdout suppression + mock_call.assert_called_once_with(['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('subprocess.call', side_effect=Exception("Remove failed")): - module._is_container = False + module.is_host = True assert module.handle_sensor_addition() is False def test_module_pre_shutdown(self): From e38a379237fac920f4e811353b0d913356f6c0c3 Mon Sep 17 00:00:00 2001 From: gpunathilell Date: Mon, 6 Oct 2025 17:13:22 +0000 Subject: [PATCH 3/4] Fix test --- tests/module_base_test.py | 58 +++++++++++++++++---------------------- 1 file changed, 25 insertions(+), 33 deletions(-) diff --git a/tests/module_base_test.py b/tests/module_base_test.py index f618ff11d..ec8d8421a 100644 --- a/tests/module_base_test.py +++ b/tests/module_base_test.py @@ -61,7 +61,7 @@ def test_is_host_detection(self): module = ModuleBase() assert module.is_host is True - # Test when /.dockerenv exists - running in container + # 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 @@ -184,7 +184,7 @@ def test_handle_pci_rescan(self): def test_handle_sensor_removal(self): module = ModuleBase() - # Test successful case on host - file exists and operations succeed + # 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: @@ -193,19 +193,17 @@ def test_handle_sensor_removal(self): mock_call.side_effect = [0, 0, 0] assert module.handle_sensor_removal() is True assert mock_call.call_count == 3 - # Verify file existence check with stdout suppression - mock_call.assert_any_call(['test', '-f', '/usr/share/sonic/platform/module_sensors_ignore_conf/ignore_sensors_DPU0.conf'], + # 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) - # Verify copy command with stdout suppression - mock_call.assert_any_call(['cp', '/usr/share/sonic/platform/module_sensors_ignore_conf/ignore_sensors_DPU0.conf', + 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) - # Verify restart command with stdout suppression - mock_call.assert_any_call(['service', 'sensord', 'restart'], + mock_call.assert_any_call(['docker', 'exec', 'pmon', 'service', 'sensord', 'restart'], stdout=subprocess.DEVNULL) mock_lock.assert_called_once() - # Test successful case in container - commands should be prefixed with docker exec + # Test successful case inside container - commands run directly without docker exec with patch.object(module, 'get_name', return_value="DPU0"), \ patch('subprocess.call') as mock_call, \ patch.object(module, '_sensord_operation_lock') as mock_lock: @@ -214,15 +212,13 @@ def test_handle_sensor_removal(self): mock_call.side_effect = [0, 0, 0] assert module.handle_sensor_removal() is True assert mock_call.call_count == 3 - # Verify file existence check with docker exec prefix and stdout suppression - mock_call.assert_any_call(['docker', 'exec', 'pmon', 'test', '-f', '/usr/share/sonic/platform/module_sensors_ignore_conf/ignore_sensors_DPU0.conf'], + # 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) - # Verify copy command with docker exec prefix and stdout suppression - mock_call.assert_any_call(['docker', 'exec', 'pmon', 'cp', '/usr/share/sonic/platform/module_sensors_ignore_conf/ignore_sensors_DPU0.conf', + 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) - # Verify restart command with docker exec prefix and stdout suppression - mock_call.assert_any_call(['docker', 'exec', 'pmon', 'service', 'sensord', 'restart'], + mock_call.assert_any_call(['service', 'sensord', 'restart'], stdout=subprocess.DEVNULL) mock_lock.assert_called_once() @@ -234,8 +230,8 @@ def test_handle_sensor_removal(self): # Return 1 to indicate file doesn't exist mock_call.return_value = 1 assert module.handle_sensor_removal() is True - # Only the file existence check should be called with stdout suppression - mock_call.assert_called_once_with(['test', '-f', '/usr/share/sonic/platform/module_sensors_ignore_conf/ignore_sensors_DPU0.conf'], + # 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() @@ -248,7 +244,7 @@ def test_handle_sensor_removal(self): def test_handle_sensor_addition(self): module = ModuleBase() - # Test successful case on host - file exists and operations succeed + # 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: @@ -257,18 +253,16 @@ def test_handle_sensor_addition(self): mock_call.side_effect = [0, 0, 0] assert module.handle_sensor_addition() is True assert mock_call.call_count == 3 - # Verify file existence check with stdout suppression - mock_call.assert_any_call(['test', '-f', '/etc/sensors.d/ignore_sensors_DPU0.conf'], + # 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) - # Verify remove command with stdout suppression - mock_call.assert_any_call(['rm', '/etc/sensors.d/ignore_sensors_DPU0.conf'], + mock_call.assert_any_call(['docker', 'exec', 'pmon', 'rm', '/etc/sensors.d/ignore_sensors_DPU0.conf'], stdout=subprocess.DEVNULL) - # Verify restart command with stdout suppression - mock_call.assert_any_call(['service', 'sensord', 'restart'], + mock_call.assert_any_call(['docker', 'exec', 'pmon', 'service', 'sensord', 'restart'], stdout=subprocess.DEVNULL) mock_lock.assert_called_once() - # Test successful case in container - commands should be prefixed with docker exec + # Test successful case inside container - commands run directly without docker exec with patch.object(module, 'get_name', return_value="DPU0"), \ patch('subprocess.call') as mock_call, \ patch.object(module, '_sensord_operation_lock') as mock_lock: @@ -277,14 +271,12 @@ def test_handle_sensor_addition(self): mock_call.side_effect = [0, 0, 0] assert module.handle_sensor_addition() is True assert mock_call.call_count == 3 - # Verify file existence check with docker exec prefix and stdout suppression - mock_call.assert_any_call(['docker', 'exec', 'pmon', 'test', '-f', '/etc/sensors.d/ignore_sensors_DPU0.conf'], + # 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) - # Verify remove command with docker exec prefix and stdout suppression - mock_call.assert_any_call(['docker', 'exec', 'pmon', 'rm', '/etc/sensors.d/ignore_sensors_DPU0.conf'], + mock_call.assert_any_call(['rm', '/etc/sensors.d/ignore_sensors_DPU0.conf'], stdout=subprocess.DEVNULL) - # Verify restart command with docker exec prefix and stdout suppression - mock_call.assert_any_call(['docker', 'exec', 'pmon', 'service', 'sensord', 'restart'], + mock_call.assert_any_call(['service', 'sensord', 'restart'], stdout=subprocess.DEVNULL) mock_lock.assert_called_once() @@ -296,8 +288,8 @@ def test_handle_sensor_addition(self): # Return 1 to indicate file doesn't exist mock_call.return_value = 1 assert module.handle_sensor_addition() is True - # Only the file existence check should be called with stdout suppression - mock_call.assert_called_once_with(['test', '-f', '/etc/sensors.d/ignore_sensors_DPU0.conf'], + # 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() From 8ebdfb6b9fbdd0dc28da1560ce64b57293e7c2f8 Mon Sep 17 00:00:00 2001 From: Gagan Punathil Ellath Date: Tue, 21 Oct 2025 09:01:39 -0700 Subject: [PATCH 4/4] Update sonic_platform_base/module_base.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- sonic_platform_base/module_base.py | 1 - 1 file changed, 1 deletion(-) diff --git a/sonic_platform_base/module_base.py b/sonic_platform_base/module_base.py index 7b342d890..e8fb6af8d 100644 --- a/sonic_platform_base/module_base.py +++ b/sonic_platform_base/module_base.py @@ -812,7 +812,6 @@ def handle_sensor_removal(self): 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