update hostcfgd code

Signed-off-by: Arham-Nasir <[email protected]>

scripts/hostcfgd

@@ -11,20 +11,17 @@ import re
 import jinja2
 import time
 import json
-import importlib
 from shutil import copy2
 from datetime import datetime
 from sonic_py_common import device_info
 from sonic_py_common.general import check_output_pipe
 from swsscommon.swsscommon import ConfigDBConnector, DBConnector, Table
 from swsscommon import swsscommon
-from swsscommon import RestartWaiter
 from sonic_installer import bootloader
 hostcfg_file_path = os.path.abspath(__file__)
 hostcfg_dir_path = os.path.dirname(hostcfg_file_path)
 sys.path.append(hostcfg_dir_path)
 import ldap
-importlib.reload(swsscommon)
 
 # FILE
 PAM_AUTH_CONF = "/etc/pam.d/common-auth-sonic"
@@ -1722,97 +1719,117 @@ class FipsCfg(object):
 
 print(swsscommon.__file__)
 
-class Memory_StatisticsCfg:
+class MemoryStatisticsCfg:
     """
-    Memory_StatisticsCfg class handles the configuration updates for the MemoryStatisticsDaemon.
+    MemoryStatisticsCfg class handles the configuration updates for the MemoryStatisticsDaemon.
     It listens to ConfigDB changes and applies them by restarting, shutting down, or reloading
     the MemoryStatisticsDaemon.
     """
-
+
+    VALID_KEYS = ["enabled", "sampling_interval", "retention_period"]  # Valid keys for configuration
+
     def __init__(self, config_db):
+        """Initialize MemoryStatisticsCfg with a configuration database."""
         self.cache = {
             "enabled": "false",
-            "retention": "15",
-            "sampling": "5"
+            "sampling_interval": "5",
+            "retention_period": "15"
         }
         self.config_db = config_db  # Store config_db instance for further use
- 
+
     def load(self, memory_statistics_config: dict):
         """Load initial memory statistics configuration."""
-        syslog.syslog(syslog.LOG_INFO, 'Memory_StatisticsCfg: Load initial configuration')
-
-        if not memory_statistics_config:
-            memory_statistics_config = {}
-
-        # Call memory_statistics_message to handle the initial config load for each key
-        self.memory_statistics_message("enabled", memory_statistics_config.get("enabled", "false"))
-        self.memory_statistics_message("retention", memory_statistics_config.get("retention", "15"))
-        self.memory_statistics_message("sampling", memory_statistics_config.get("sampling", "5"))
-
+        syslog.syslog(syslog.LOG_INFO, 'MemoryStatisticsCfg: Load initial configuration')
+
+        # Use default values if no config provided
+        memory_statistics_config = memory_statistics_config or {}
+
+        # Load configuration for each valid key
+        for key in self.VALID_KEYS:
+            self.memory_statistics_update(key, memory_statistics_config.get(key, self.cache[key]))
+
     def memory_statistics_update(self, key, data):
         """
         Apply memory statistics settings handler.
+        
         Args:
             key: DB table's key that triggered the change.
             data: New table data to process.
         """
-        # Ensure data is a string or convertible to the required value
-        if not isinstance(data, str):
-            data = str(data)
-
+        if key not in self.VALID_KEYS:
+            syslog.syslog(syslog.LOG_ERR, f"MemoryStatisticsCfg: Invalid key '{key}' received.")
+            return
+
+        # Convert data to string if it's not already
+        data = str(data)
+
+        # Validate numeric keys (retention_period and sampling_interval)
+        if key in ["retention_period", "sampling_interval"] and (not data.isdigit() or int(data) <= 0):
+            syslog.syslog(syslog.LOG_ERR, f"MemoryStatisticsCfg: Invalid value '{data}' for key '{key}'. Must be a positive integer.")
+            return
+
         # Check if any value has changed
         if data != self.cache.get(key):
-            syslog.syslog(syslog.LOG_INFO, f"Memory_StatisticsCfg: Detected change in '{key}'")
- 
+            syslog.syslog(syslog.LOG_INFO, f"MemoryStatisticsCfg: Detected change in '{key}' to '{data}'")
+
             try:
-                if key == "enabled":
-                    enabled = data.lower() == "true"
-                    if enabled:
-                        self.restart_memory_statistics()  # Start or restart the daemon
-                    else:
-                        self.shutdown_memory_statistics()  # Stop the daemon if disabled
-                else:
-                    # If other keys (like sampling/retention) are changed, just reload the daemon config
-                    self.reload_memory_statistics()
-
-                # Update cache with the new value
-                self.cache[key] = data
+                self.apply_setting(key, data)
+                self.cache[key] = data  # Update cache with the new value
             except Exception as e:
-                syslog.syslog(syslog.LOG_ERR, f'Memory_StatisticsCfg: Failed to manage MemoryStatisticsDaemon: {e}')
-
+                syslog.syslog(syslog.LOG_ERR, f'MemoryStatisticsCfg: Failed to manage MemoryStatisticsDaemon: {e}')
+
+    def apply_setting(self, key, data):
+        """Apply the setting based on the key."""
+        if key == "enabled":
+            if data.lower() == "true":
+                self.restart_memory_statistics()
+            else:
+                self.shutdown_memory_statistics()
+        else:
+            self.reload_memory_statistics()
+
     def restart_memory_statistics(self):
         """Restart the memory statistics daemon."""
-        self.shutdown_memory_statistics()  # Ensure the daemon is stopped before restarting
-        time.sleep(1)  # Brief delay to allow shutdown
-        syslog.syslog(syslog.LOG_INFO, "Memory_StatisticsCfg: Starting MemoryStatisticsDaemon")
+        self.shutdown_memory_statistics()  # Stop the daemon before restarting
+        time.sleep(1)  # Optional delay to ensure proper restart
+        syslog.syslog(syslog.LOG_INFO, "MemoryStatisticsCfg: Starting MemoryStatisticsDaemon")
+
         try:
-            subprocess.Popen(['/usr/bin/memorystatsd'])  
+            subprocess.Popen(['/usr/bin/memorystatsd'])  # Replace with the correct daemon path
         except Exception as e:
-            syslog.syslog(syslog.LOG_ERR, f"Memory_StatisticsCfg: Failed to start MemoryStatisticsDaemon: {e}")
- 
+            syslog.syslog(syslog.LOG_ERR, f"MemoryStatisticsCfg: Failed to start MemoryStatisticsDaemon: {e}")
+
     def reload_memory_statistics(self):
         """Send SIGHUP to the MemoryStatisticsDaemon to reload its configuration."""
         pid = self.get_memory_statistics_pid()
         if pid:
-            os.kill(pid, signal.SIGHUP)  # Notify daemon to reload its configuration
-            syslog.syslog(syslog.LOG_INFO, "Memory_StatisticsCfg: Sent SIGHUP to reload daemon configuration")
-
+            try:
+                os.kill(pid, signal.SIGHUP)  # Notify daemon to reload its configuration
+                syslog.syslog(syslog.LOG_INFO, "MemoryStatisticsCfg: Sent SIGHUP to reload daemon configuration")
+            except Exception as e:
+                syslog.syslog(syslog.LOG_ERR, f"MemoryStatisticsCfg: Failed to reload MemoryStatisticsDaemon: {e}")
+
     def shutdown_memory_statistics(self):
         """Send SIGTERM to stop the MemoryStatisticsDaemon gracefully."""
         pid = self.get_memory_statistics_pid()
         if pid:
-            os.kill(pid, signal.SIGTERM)  # Graceful shutdown
-            syslog.syslog(syslog.LOG_INFO, "Memory_StatisticsCfg: Sent SIGTERM to stop MemoryStatisticsDaemon")
-
+            try:
+                os.kill(pid, signal.SIGTERM)  # Graceful shutdown
+                syslog.syslog(syslog.LOG_INFO, "MemoryStatisticsCfg: Sent SIGTERM to stop MemoryStatisticsDaemon")
+            except Exception as e:
+                syslog.syslog(syslog.LOG_ERR, f"MemoryStatisticsCfg: Failed to shutdown MemoryStatisticsDaemon: {e}")
+
     def get_memory_statistics_pid(self):
         """Retrieve the PID of the running MemoryStatisticsDaemon."""
         try:
             with open('/var/run/memorystatsd.pid', 'r') as pid_file:
                 pid = int(pid_file.read().strip())
             return pid
+        except FileNotFoundError:
+            syslog.syslog(syslog.LOG_WARNING, "MemoryStatisticsCfg: PID file not found.")
         except Exception as e:
-            syslog.syslog(syslog.LOG_ERR, f"Memory_StatisticsCfg: Failed to retrieve MemoryStatisticsDaemon PID: {e}")
-            return None
+            syslog.syslog(syslog.LOG_ERR, f"MemoryStatisticsCfg: Failed to retrieve MemoryStatisticsDaemon PID: {e}")
+        return None
 
 class SerialConsoleCfg:
 
@@ -1937,7 +1954,7 @@ class HostConfigDaemon:
         # Initialize KDump Config and set the config to default if nothing is provided
         self.kdumpCfg = KdumpCfg(self.config_db)
 
-        self.memory_statisticsCfg = Memory_StatisticsCfg(self.config_db)
+        self.memorystatisticscfg = MemoryStatisticsCfg(self.config_db)
 
         # Initialize IpTables
         self.iptables = Iptables()
@@ -2010,7 +2027,7 @@ class HostConfigDaemon:
         self.aaacfg.load(aaa, tacacs_global, tacacs_server, radius_global, radius_server, ldap_global, ldap_server)
         self.iptables.load(lpbk_table)
         self.kdumpCfg.load(kdump)
-        self.memory_statisticsCfg.load(memory_statistics)
+        self.memorystatisticscfg.load(memory_statistics)
         self.passwcfg.load(passwh)
         self.sshscfg.load(ssh_server)
         self.devmetacfg.load(dev_meta)
@@ -2144,7 +2161,7 @@ class HostConfigDaemon:
 
     def memory_statistics_handler (self, key, op, data):
         syslog.syslog(syslog.LOG_INFO, 'Memory_Statistics handler...')
-        self.memory_statisticsCfg.memory_statistics_update(key, data)
+        self.memorystatisticscfg.memory_statistics_update(key, data)
 
     def device_metadata_handler(self, key, op, data):
         syslog.syslog(syslog.LOG_INFO, 'DeviceMeta handler...')

tests/hostcfgd/hostcfgd_test.py

@@ -7,10 +7,8 @@
 from parameterized import parameterized
 from sonic_py_common.general import load_module_from_source
 from unittest import TestCase, mock
-
 from .test_vectors import HOSTCFG_DAEMON_INIT_CFG_DB, HOSTCFG_DAEMON_CFG_DB
 from tests.common.mock_configdb import MockConfigDb, MockDBConnector
-from unittest.mock import patch
 from pyfakefs.fake_filesystem_unittest import patchfs
 from deepdiff import DeepDiff
 from unittest.mock import call
@@ -324,40 +322,6 @@ def test_mgmtiface_event(self):
                 ]
                 mocked_check_output.assert_has_calls(expected)
 
-    @patch('sonic_py_common.ConfigDBConnector', autospec=True)
-    def test_memory_statistics_event(self, mock_config_db_connector):
-        # Mock the ConfigDBConnector instance methods
-        mock_instance = mock_config_db_connector.return_value
-        # Ensure get_table returns the correct nested structur
-        mock_instance.get_table.return_value = HOSTCFG_DAEMON_CFG_DB['MEMORY_STATISTICS']['memory_statistics']
-
-        # Patch subprocess.Popen and check_call
-        with mock.patch('hostcfgd.subprocess.Popen') as mocked_popen, \
-            mock.patch('hostcfgd.subprocess.check_call') as mocked_check_call:
-
-            # Create the daemon instance
-            daemon = hostcfgd.HostConfigDaemon()
-            # Load config using the correct nested dictionary
-            daemon.memory_statisticsCfg.load(HOSTCFG_DAEMON_CFG_DB['MEMORY_STATISTICS']['memory_statistics'])
-
-            # Mock subprocess.Popen behavior
-            popen_mock = mock.Mock()
-            attrs = {'communicate.return_value': ('output', 'error')}
-            popen_mock.configure_mock(**attrs)
-            mocked_popen.return_value = popen_mock
-
-            # Trigger the event handler via event queue
-            daemon.event_queue.append(('MEMORY_STATISTICS', 'memory_statistics'))
-            daemon.memory_statistics_handler('enabled', 'SET', 'true')
-
-            # Define expected subprocess calls
-            expected_calls = [
-                mock.call(['/usr/bin/memorystatsd']),
-            ]
-
-            # Check if subprocess Popen was called with correct arguments
-            mocked_popen.assert_has_calls(expected_calls, any_order=True)
-
     def test_dns_events(self):
         MockConfigDb.set_config_db(HOSTCFG_DAEMON_CFG_DB)
         MockConfigDb.event_queue = [('DNS_NAMESERVER', '1.1.1.1')]
@@ -390,9 +354,6 @@ def test_load(self):
 
         dns_cfg.dns_update.assert_called()
 
-        dns_cfg.dns_update.assert_called()
-
-
 class TestBannerCfg:
     def test_load(self):
         banner_cfg = hostcfgd.BannerCfg()
@@ -409,3 +370,64 @@ def test_banner_message(self, mock_run_cmd):
 
         mock_run_cmd.assert_has_calls([call(['systemctl', 'restart', 'banner-config'], True, True)])
 
+
+class TestMemoryStatisticsCfgd(TestCase):
+    """
+    Test MemoryStatisticsCfg functionalities.
+    """
+
+    def setUp(self):
+        # Initial configuration for Memory Statistics
+        MockConfigDb.CONFIG_DB['MEMORY_STATISTICS'] = {
+            'enabled': 'false',
+            'sampling_interval': '5',
+            'retention_period': '15'
+        }
+        self.mem_stat_cfg = hostcfgd.MemoryStatisticsCfg(MockConfigDb.CONFIG_DB)
+
+    def tearDown(self):
+        MockConfigDb.CONFIG_DB = {}
+
+    def test_memory_statistics_load(self):
+        with mock.patch('hostcfgd.subprocess') as mocked_subprocess:
+            self.mem_stat_cfg.load(MockConfigDb.CONFIG_DB['MEMORY_STATISTICS'])
+            mocked_subprocess.Popen.assert_called_once_with(['/usr/bin/memorystatsd'])
+            self.assertEqual(self.mem_stat_cfg.cache['enabled'], 'false')
+            self.assertEqual(self.mem_stat_cfg.cache['sampling_interval'], '5')
+            self.assertEqual(self.mem_stat_cfg.cache['retention_period'], '15')
+
+    def test_memory_statistics_update_enabled(self):
+        with mock.patch('hostcfgd.subprocess') as mocked_subprocess, \
+             mock.patch('hostcfgd.os.kill') as mocked_kill:
+            self.mem_stat_cfg.memory_statistics_update('enabled', 'true')
+            mocked_kill.assert_called_once()
+            mocked_subprocess.Popen.assert_called_once_with(['/usr/bin/memorystatsd'])
+
+    def test_memory_statistics_is_caching_config(self):
+        self.mem_stat_cfg.cache['enabled'] = 'true'
+        with mock.patch('hostcfgd.subprocess') as mocked_subprocess:
+            self.mem_stat_cfg.memory_statistics_update('enabled', 'true')
+            mocked_subprocess.Popen.assert_not_called()
+            self.assertEqual(self.mem_stat_cfg.cache['enabled'], 'true')  # Confirm no unnecessary cache update
+
+    def test_memory_statistics_update_sampling_interval(self):
+        with mock.patch('hostcfgd.subprocess') as mocked_subprocess:
+            self.mem_stat_cfg.memory_statistics_update('sampling_interval', '3')
+            mocked_subprocess.Popen.assert_not_called()
+            self.assertEqual(self.mem_stat_cfg.cache['sampling_interval'], '3')
+
+    def test_memory_statistics_update_retention_period(self):
+        with mock.patch('hostcfgd.subprocess') as mocked_subprocess:
+            self.mem_stat_cfg.memory_statistics_update('retention_period', '30')
+            mocked_subprocess.Popen.assert_not_called()
+            self.assertEqual(self.mem_stat_cfg.cache['retention_period'], '30')
+
+    def test_memory_statistics_update_invalid_sampling_interval(self):
+        with mock.patch('hostcfgd.syslog') as mocked_syslog:
+            self.mem_stat_cfg.memory_statistics_update('sampling_interval', '-10')
+            mocked_syslog.syslog.assert_called_with(syslog.LOG_ERR, "Memory_StatisticsCfg: Invalid value '-10' for key 'sampling_interval'. Must be a positive integer.")
+
+    def test_memory_statistics_update_invalid_retention_period(self):
+        with mock.patch('hostcfgd.syslog') as mocked_syslog:
+            self.mem_stat_cfg.memory_statistics_update('retention_period', 'not_a_number')
+            mocked_syslog.syslog.assert_called_with(syslog.LOG_ERR, "Memory_StatisticsCfg: Invalid value 'not_a_number' for key 'retention_period'. Must be a positive integer.")

tests/hostcfgd/test_vectors.py

@@ -82,10 +82,10 @@
     },
     "MEMORY_STATISTICS": {
         "memory_statistics": {
-            "enabled": "true",
-            "retention_time": "15",
-            "sampling_interval": "5"
-    }
+            "enabled": "false",
+            "sampling_interval": "5",
+            "retention_period": "15"  
+        }
     },
     "MGMT_INTERFACE": {
         "eth0|1.2.3.4/24": {}