[Smartswitch] Host container differentiation for sensor commands

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:

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