[hostcfgd] [202012] Enhance hostcfgd to check feature state and run less system calls (#8157)

Currently hostcfgd is implemented in a way each feature which is enabled/disabled triggering execution of systemctl enable/unmask commands which eventually trigger 'systemctl daemon-reload' command.
Each call like this cost 0.6s and overall add a overhead of ~12 seconds of CPU time.
This change will verify the desired state of a feature and the current state of this feature on systemd and trigger a system call only when must.
What is changed: Check each feature status on systemd before executing a system call to enable and reload the systemctl daemon.
How to verify: Build an image with this change and observe less system calls are executed.

src/sonic-host-services/scripts/hostcfgd

@@ -457,6 +457,13 @@ class HostConfigDaemon:
     def enable_feature(self, feature_names, feature_suffixes):
         start_cmds = []
         for feature_name in feature_names:
+            # Check if it is already enabled, if yes skip the system call
+            cmd = "sudo systemctl status {}.{} | grep Loaded".format(feature_name, feature_suffixes[-1])
+            proc = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+            (stdout, stderr) = proc.communicate()
+            if "enabled" in str(stdout):
+                continue
+
             for suffix in feature_suffixes:
                 start_cmds.append("sudo systemctl unmask {}.{}".format(feature_name, suffix))
             # If feature has timer associated with it, start/enable corresponding systemd .timer unit
@@ -477,6 +484,13 @@ class HostConfigDaemon:
     def disable_feature(self, feature_names, feature_suffixes):
         stop_cmds = []
         for feature_name in feature_names:
+            # Check if it is already disabled, if yes skip the system call
+            cmd = "sudo systemctl status {}.{} | grep Loaded".format(feature_name, feature_suffixes[-1])
+            proc = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+            (stdout, stderr) = proc.communicate()
+            if "disabled" in str(stdout) or "masked" in str(stdout):
+                continue
+
             for suffix in reversed(feature_suffixes):
                 stop_cmds.append("sudo systemctl stop {}.{}".format(feature_name, suffix))
                 stop_cmds.append("sudo systemctl disable {}.{}".format(feature_name, suffix))

src/sonic-host-services/tests/hostcfgd/hostcfgd_test.py

@@ -65,6 +65,11 @@ class TestHostcfgd(TestCase):
         """
         MockConfigDb.set_config_db(test_data["config_db"])
         with mock.patch("hostcfgd.subprocess") as mocked_subprocess:
+            popen_mock = mock.Mock()
+            attrs = test_data["popen_attributes"]
+            popen_mock.configure_mock(**attrs)
+            mocked_subprocess.Popen.return_value = popen_mock
+
             host_config_daemon = hostcfgd.HostConfigDaemon()
             host_config_daemon.update_all_feature_states()
             assert self.__verify_table(

src/sonic-host-services/tests/hostcfgd/test_vectors.py

@@ -96,6 +96,9 @@ HOSTCFGD_TEST_VECTOR = [
                 call("sudo systemctl enable telemetry.timer", shell=True),
                 call("sudo systemctl start telemetry.timer", shell=True),
             ],
+            "popen_attributes": {
+                'communicate.return_value': ('output', 'error')
+            },
         },
     ],
     [
@@ -189,6 +192,9 @@ HOSTCFGD_TEST_VECTOR = [
                 call("sudo systemctl enable telemetry.timer", shell=True),
                 call("sudo systemctl start telemetry.timer", shell=True),
             ],
+            "popen_attributes": {
+                'communicate.return_value': ('output', 'error')
+            },
         },
     ],
     [
@@ -282,6 +288,96 @@ HOSTCFGD_TEST_VECTOR = [
                 call("sudo systemctl enable telemetry.timer", shell=True),
                 call("sudo systemctl start telemetry.timer", shell=True),
             ],
+            "popen_attributes": {
+                'communicate.return_value': ('output', 'error')
+            },
         },
     ],
+    [
+        "DualTorCaseWithNoSystemCalls",
+        {
+            "config_db": {
+                "DEVICE_METADATA": {
+                    "localhost": {
+                        "subtype": "DualToR",
+                        "type": "ToRRouter",
+                    }
+                },
+                "KDUMP": {
+                    "config": {
+                        "enabled": "false",
+                        "num_dumps": "3",
+                        "memory": "0M-2G:256M,2G-4G:320M,4G-8G:384M,8G-:448M"
+                        }
+                },
+                "FEATURE": {
+                    "dhcp_relay": {
+                        "auto_restart": "enabled",
+                        "has_global_scope": "True",
+                        "has_per_asic_scope": "False",
+                        "has_timer": "False",
+                        "high_mem_alert": "disabled",
+                        "set_owner": "kube",
+                        "state": "enabled"
+                    },
+                    "mux": {
+                        "auto_restart": "enabled",
+                        "has_global_scope": "True",
+                        "has_per_asic_scope": "False",
+                        "has_timer": "False",
+                        "high_mem_alert": "disabled",
+                        "set_owner": "local",
+                        "state": "{% if 'subtype' in DEVICE_METADATA['localhost'] and DEVICE_METADATA['localhost']['subtype'] == 'DualToR' %}enabled{% else %}always_disabled{% endif %}"
+                    },
+                    "telemetry": {
+                        "auto_restart": "enabled",
+                        "has_global_scope": "True",
+                        "has_per_asic_scope": "False",
+                        "has_timer": "True",
+                        "high_mem_alert": "disabled",
+                        "set_owner": "kube",
+                        "state": "enabled",
+                        "status": "enabled"
+                    },
+                },
+            },
+            "expected_config_db": {
+                "FEATURE": {
+                    "dhcp_relay": {
+                        "auto_restart": "enabled",
+                        "has_global_scope": "True",
+                        "has_per_asic_scope": "False",
+                        "has_timer": "False",
+                        "high_mem_alert": "disabled",
+                        "set_owner": "kube",
+                        "state": "enabled"
+                    },
+                    "mux": {
+                        "auto_restart": "enabled",
+                        "has_global_scope": "True",
+                        "has_per_asic_scope": "False",
+                        "has_timer": "False",
+                        "high_mem_alert": "disabled",
+                        "set_owner": "local",
+                        "state": "enabled"
+                    },
+                    "telemetry": {
+                        "auto_restart": "enabled",
+                        "has_global_scope": "True",
+                        "has_per_asic_scope": "False",
+                        "has_timer": "True",
+                        "high_mem_alert": "disabled",
+                        "set_owner": "kube",
+                        "state": "enabled",
+                        "status": "enabled"
+                    },
+                },
+            },
+            "expected_subprocess_calls": [
+            ],
+            "popen_attributes": {
+                'communicate.return_value': ('enabled', 'error')
+            },
+        }
+    ]
 ]