Consolidate the get running config way. (#3585)

#### What I did Addressing the [issue 20508](sonic-net/sonic-buildimage#20508). ADO: 29979987 #### How I did it Remove temp file as intermediate steps. #### How to verify it ``` admin@str2-7250-lc1-2:~$ cat test.json [ { "op": "add", "path": "/asic0/BGP_DEVICE_GLOBAL/STATE/idf_isolation_state", "value": "unisolated" } ] admin@str2-7250-lc1-2:~$ sudo config apply-patch test.json sonic_yang(6):Note: Below table(s) have no YANG models: DHCP_SERVER sonic_yang(6):Note: Below table(s) have no YANG models: LOGGER sonic_yang(6):Note: Below table(s) have no YANG models: LOGGER Patch Applier: asic0: Patch application starting. Patch Applier: asic0: Patch: [{"op": "add", "path": "/BGP_DEVICE_GLOBAL/STATE/idf_isolation_state", "value": "unisolated"}] Patch Applier: asic0 getting current config db. Patch Applier: asic0: simulating the target full config after applying the patch. Patch Applier: asic0: validating all JsonPatch operations are permitted on the specified fields Patch Applier: asic0: validating target config does not have empty tables, since they do not show up in ConfigDb. Patch Applier: asic0: sorting patch updates. Patch Applier: The asic0 patch was converted into 1 change: Patch Applier: asic0: applying 1 change in order: Patch Applier: * [{"op": "replace", "path": "/BGP_DEVICE_GLOBAL/STATE/idf_isolation_state", "value": "unisolated"}] Patch Applier: asic0: verifying patch updates are reflected on ConfigDB. Patch Applier: asic0 patch application completed. Patch applied successfully. admin@str2-7250-lc1-2:~$ show ver SONiC Software Version: SONiC.20220532.72 SONiC OS Version: 11 Distribution: Debian 11.9 Kernel: 5.10.0-23-2-amd64 Build commit: 7766169087 Build date: Fri Oct 4 00:15:40 UTC 2024 Built by: azureuser@98b2318ac000000 Platform: x86_64-nokia_ixr7250e_36x400g-r0 HwSKU: Nokia-IXR7250E-36x100G ASIC: broadcom ASIC Count: 2 Serial Number: NS220304200 Model Number: 3HE12578AARA01 Hardware Revision: 56 Uptime: 05:08:45 up 2 days, 10:16, 1 user, load average: 1.64, 1.82, 1.74 Date: Fri 25 Oct 2024 05:08:45 Docker images: REPOSITORY TAG IMAGE ID SIZE docker-mux 20220532.72 a27de04f0900 375MB docker-mux latest a27de04f0900 375MB docker-macsec latest 9cad4ac054db 372MB docker-sonic-restapi 20220532.72 2dc9b6c42cdb 345MB docker-sonic-restapi latest 2dc9b6c42cdb 345MB docker-orchagent 20220532.72 560867c70e69 360MB docker-orchagent latest 560867c70e69 360MB docker-fpm-frr 20220532.72 525aad3b1670 367MB docker-fpm-frr latest 525aad3b1670 367MB docker-teamd 20220532.72 9bc2875ba21c 343MB docker-teamd latest 9bc2875ba21c 343MB docker-syncd-brcm-dnx 20220532.72 58ee35f9df5b 674MB docker-syncd-brcm-dnx latest 58ee35f9df5b 674MB docker-gbsyncd-credo 20220532.72 5084ec39b3fc 346MB docker-gbsyncd-credo latest 5084ec39b3fc 346MB docker-gbsyncd-broncos 20220532.72 f1011e5ed75c 374MB docker-gbsyncd-broncos latest f1011e5ed75c 374MB docker-dhcp-relay latest 137faf5f4038 337MB docker-platform-monitor 20220532.72 41d6954ab85a 452MB docker-platform-monitor latest 41d6954ab85a 452MB docker-snmp 20220532.72 916f66a40a77 376MB docker-snmp latest 916f66a40a77 376MB docker-sonic-telemetry 20220532.72 e8037e0fd00c 407MB docker-sonic-telemetry latest e8037e0fd00c 407MB docker-router-advertiser 20220532.72 a5afbccec5da 327MB docker-router-advertiser latest a5afbccec5da 327MB docker-lldp 20220532.72 01386dd544cf 369MB docker-lldp latest 01386dd544cf 369MB docker-database 20220532.72 2da62f2abd04 327MB docker-database latest 2da62f2abd04 327MB docker-acms 20220532.72 264a51a7a259 374MB docker-acms latest 264a51a7a259 374MB k8s.gcr.io/pause 3.5 ed210e3e4a5b 683kB ```
sonic-net · Nov 30, 2024 · 18938a2 · 18938a2
1 parent 3cc74d3
commit 18938a2
Show file tree

Hide file tree

Showing 5 changed files with 88 additions and 118 deletions.
diff --git a/generic_config_updater/change_applier.py b/generic_config_updater/change_applier.py
@@ -9,6 +9,7 @@
 from swsscommon.swsscommon import ConfigDBConnector
 from sonic_py_common import multi_asic
 from .gu_common import GenericConfigUpdaterError, genericUpdaterLogging
+from .gu_common import get_config_db_as_json
 
 SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__))
 UPDATER_CONF_FILE = f"{SCRIPT_DIR}/gcu_services_validator.conf.json"
@@ -137,7 +138,7 @@ def _report_mismatch(self, run_data, upd_data):
             str(jsondiff.diff(run_data, upd_data))[0:40]))
 
     def apply(self, change):
-        run_data = self._get_running_config()
+        run_data = get_config_db_as_json(self.scope)
         upd_data = prune_empty_table(change.apply(copy.deepcopy(run_data)))
         upd_keys = defaultdict(dict)
 
@@ -146,7 +147,7 @@ def apply(self, change):
 
         ret = self._services_validate(run_data, upd_data, upd_keys)
         if not ret:
-            run_data = self._get_running_config()
+            run_data = get_config_db_as_json(self.scope)
             self.remove_backend_tables_from_config(upd_data)
             self.remove_backend_tables_from_config(run_data)
             if upd_data != run_data:
@@ -159,31 +160,3 @@ def apply(self, change):
     def remove_backend_tables_from_config(self, data):
         for key in self.backend_tables:
             data.pop(key, None)
-
-    def _get_running_config(self):
-        _, fname = tempfile.mkstemp(suffix="_changeApplier")
-
-        if self.scope:
-            cmd = ['sonic-cfggen', '-d', '--print-data', '-n', self.scope]
-        else:
-            cmd = ['sonic-cfggen', '-d', '--print-data']
-
-        with open(fname, "w") as file:
-            result = subprocess.Popen(cmd, stdout=file, stderr=subprocess.PIPE, text=True)
-            _, err = result.communicate()
-
-        return_code = result.returncode
-        if return_code:
-            os.remove(fname)
-            raise GenericConfigUpdaterError(
-                f"Failed to get running config for scope: {self.scope}," +
-                f"Return code: {return_code}, Error: {err}")
-
-        run_data = {}
-        try:
-            with open(fname, "r") as file:
-                run_data = json.load(file)
-        finally:
-            if os.path.isfile(fname):
-                os.remove(fname)
-        return run_data
diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py
@@ -53,31 +53,39 @@ def __eq__(self, other):
             return self.patch == other.patch
         return False
 
+
+def get_config_db_as_json(scope=None):
+    text = get_config_db_as_text(scope=scope)
+    config_db_json = json.loads(text)
+    config_db_json.pop("bgpraw", None)
+    return config_db_json
+
+
+def get_config_db_as_text(scope=None):
+    if scope is not None and scope != multi_asic.DEFAULT_NAMESPACE:
+        cmd = ['sonic-cfggen', '-d', '--print-data', '-n', scope]
+    else:
+        cmd = ['sonic-cfggen', '-d', '--print-data']
+    result = subprocess.Popen(cmd, shell=False, text=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    text, err = result.communicate()
+    return_code = result.returncode
+    if return_code:
+        raise GenericConfigUpdaterError(f"Failed to get running config for namespace: {scope},"
+                                        f" Return code: {return_code}, Error: {err}")
+    return text
+
+
 class ConfigWrapper:
     def __init__(self, yang_dir=YANG_DIR, scope=multi_asic.DEFAULT_NAMESPACE):
         self.scope = scope
         self.yang_dir = YANG_DIR
         self.sonic_yang_with_loaded_models = None
 
     def get_config_db_as_json(self):
-        text = self._get_config_db_as_text()
-        config_db_json = json.loads(text)
-        config_db_json.pop("bgpraw", None)
-        return config_db_json
+        return get_config_db_as_json(self.scope)
 
     def _get_config_db_as_text(self):
-        if self.scope is not None and self.scope != multi_asic.DEFAULT_NAMESPACE:
-            cmd = ['sonic-cfggen', '-d', '--print-data', '-n', self.scope]
-        else:
-            cmd = ['sonic-cfggen', '-d', '--print-data']
-
-        result = subprocess.Popen(cmd, shell=False, text=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        text, err = result.communicate()
-        return_code = result.returncode
-        if return_code: # non-zero means failure
-            raise GenericConfigUpdaterError(f"Failed to get running config for namespace: {self.scope},"
-                                            f" Return code: {return_code}, Error: {err}")
-        return text
+        return get_config_db_as_text(self.scope)
 
     def get_sonic_yang_as_json(self):
         config_db_json = self.get_config_db_as_json()

diff --git a/tests/generic_config_updater/change_applier_test.py b/tests/generic_config_updater/change_applier_test.py
@@ -72,28 +72,25 @@
 def debug_print(msg):
     print(msg)
 
-
-# Mimics os.system call for sonic-cfggen -d --print-data > filename
+# Mimics os.system call for `sonic-cfggen -d --print-data` output
 def subprocess_Popen_cfggen(cmd, *args, **kwargs):
     global running_config
 
-    # Extract file name from kwargs if 'stdout' is a file object
-    stdout = kwargs.get('stdout')
-    if hasattr(stdout, 'name'):
-        fname = stdout.name
+    stdout = kwargs.get('stdout', None)
+
+    if stdout is None:
+        output = json.dumps(running_config, indent=4)
+    elif isinstance(stdout, int) and stdout == -1:
+        output = json.dumps(running_config, indent=4)
     else:
-        raise ValueError("stdout is not a file")
+        raise ValueError("stdout must be set to subprocess.PIPE or omitted for capturing output")
 
-    # Write the running configuration to the file specified in stdout
-    with open(fname, "w") as s:
-        json.dump(running_config, s, indent=4)
-
     class MockPopen:
         def __init__(self):
-            self.returncode = 0  # Simulate successful command execution
+            self.returncode = 0
 
         def communicate(self):
-            return "", ""  # Simulate empty stdout and stderr
+            return output.encode(), "".encode()
 
     return MockPopen()
 
@@ -225,7 +222,7 @@ def vlan_validate(old_cfg, new_cfg, keys):
 
 class TestChangeApplier(unittest.TestCase):
 
-    @patch("generic_config_updater.change_applier.subprocess.Popen")
+    @patch("generic_config_updater.gu_common.subprocess.Popen")
     @patch("generic_config_updater.change_applier.get_config_db")
     @patch("generic_config_updater.change_applier.set_config")
     def test_change_apply(self, mock_set, mock_db, mock_subprocess_Popen):

diff --git a/tests/generic_config_updater/gcu_feature_patch_application_test.py b/tests/generic_config_updater/gcu_feature_patch_application_test.py
@@ -6,13 +6,15 @@
 from mock import patch
 
 import generic_config_updater.change_applier
+import generic_config_updater.gu_common
 import generic_config_updater.patch_sorter as ps
 import generic_config_updater.generic_updater as gu
 from .gutest_helpers import Files
 from generic_config_updater.gu_common import ConfigWrapper, PatchWrapper
 
 running_config = {}
-
+
+
 def set_entry(config_db, tbl, key, data):
     global running_config
     if data != None:
@@ -26,9 +28,11 @@ def set_entry(config_db, tbl, key, data):
         if not running_config[tbl]:
             running_config.pop(tbl)
 
-def get_running_config():
+
+def get_running_config(scope="localhost"):
     return running_config
 
+
 class TestFeaturePatchApplication(unittest.TestCase):
     def setUp(self):
         self.config_wrapper = ConfigWrapper()
@@ -87,13 +91,13 @@ def create_patch_applier(self, config):
         config_wrapper = self.config_wrapper
         config_wrapper.get_config_db_as_json = MagicMock(side_effect=get_running_config)
         change_applier = generic_config_updater.change_applier.ChangeApplier()
-        change_applier._get_running_config = MagicMock(side_effect=get_running_config)
         patch_wrapper = PatchWrapper(config_wrapper)
         return gu.PatchApplier(config_wrapper=config_wrapper, patch_wrapper=patch_wrapper, changeapplier=change_applier)
 
+    @patch('generic_config_updater.change_applier.get_config_db_as_json', side_effect=get_running_config)
     @patch("generic_config_updater.change_applier.get_config_db")
     @patch("generic_config_updater.change_applier.set_config")
-    def run_single_success_case_applier(self, data, mock_set, mock_db):
+    def run_single_success_case_applier(self, data, mock_set, mock_db, mock_get_config_db_as_json):
         current_config = data["current_config"]
         expected_config = data["expected_config"]
         patch = jsonpatch.JsonPatch(data["patch"])
@@ -121,7 +125,8 @@ def run_single_success_case_applier(self, data, mock_set, mock_db):
         self.assertEqual(simulated_config, expected_config)
 
     @patch("generic_config_updater.change_applier.get_config_db")
-    def run_single_failure_case_applier(self, data, mock_db):
+    @patch('generic_config_updater.change_applier.get_config_db_as_json', side_effect=get_running_config)
+    def run_single_failure_case_applier(self, data, mock_db, mock_get_config_db_as_json):
         current_config = data["current_config"]
         patch = jsonpatch.JsonPatch(data["patch"])
         expected_error_substrings = data["expected_error_substrings"]

diff --git a/tests/generic_config_updater/multiasic_change_applier_test.py b/tests/generic_config_updater/multiasic_change_applier_test.py
@@ -7,6 +7,30 @@
 import generic_config_updater.gu_common
 
 
+def mock_get_running_config_side_effect(scope):
+    print(f"mocked_value_for_{scope}")
+    return {
+        "tables": {
+            "ACL_TABLE": {
+                "services_to_validate": ["aclservice"],
+                "validate_commands": ["acl_loader show table"]
+            },
+            "PORT": {
+                "services_to_validate": ["portservice"],
+                "validate_commands": ["show interfaces status"]
+            }
+        },
+        "services": {
+            "aclservice": {
+                "validate_commands": ["acl_loader show table"]
+            },
+            "portservice": {
+                "validate_commands": ["show interfaces status"]
+            }
+        }
+    }
+
+
 class TestMultiAsicChangeApplier(unittest.TestCase):
 
     def test_extract_scope(self):
@@ -38,34 +62,15 @@ def test_extract_scope(self):
             except Exception as e:
                 assert(result == False)
 
-    @patch('generic_config_updater.change_applier.ChangeApplier._get_running_config', autospec=True)
+    @patch('generic_config_updater.change_applier.get_config_db_as_json', autospec=True)
     @patch('generic_config_updater.change_applier.ConfigDBConnector', autospec=True)
     def test_apply_change_default_scope(self, mock_ConfigDBConnector, mock_get_running_config):
         # Setup mock for ConfigDBConnector
         mock_db = MagicMock()
         mock_ConfigDBConnector.return_value = mock_db
 
         # Setup mock for json.load to return some running configuration
-        mock_get_running_config.return_value = {
-            "tables": {
-                "ACL_TABLE": {
-                    "services_to_validate": ["aclservice"],
-                    "validate_commands": ["acl_loader show table"]
-                },
-                "PORT": {
-                    "services_to_validate": ["portservice"],
-                    "validate_commands": ["show interfaces status"]
-                }
-            },
-            "services": {
-                "aclservice": {
-                    "validate_commands": ["acl_loader show table"]
-                },
-                "portservice": {
-                    "validate_commands": ["show interfaces status"]
-                }
-            }
-        }
+        mock_get_running_config.side_effect = mock_get_running_config_side_effect
 
         # Instantiate ChangeApplier with the default scope
         applier = generic_config_updater.change_applier.ChangeApplier()
@@ -79,34 +84,13 @@ def test_apply_change_default_scope(self, mock_ConfigDBConnector, mock_get_runni
         # Assert ConfigDBConnector called with the correct namespace
         mock_ConfigDBConnector.assert_called_once_with(use_unix_socket_path=True, namespace="")
 
-    @patch('generic_config_updater.change_applier.ChangeApplier._get_running_config', autospec=True)
+    @patch('generic_config_updater.change_applier.get_config_db_as_json', autospec=True)
     @patch('generic_config_updater.change_applier.ConfigDBConnector', autospec=True)
     def test_apply_change_given_scope(self, mock_ConfigDBConnector, mock_get_running_config):
         # Setup mock for ConfigDBConnector
         mock_db = MagicMock()
         mock_ConfigDBConnector.return_value = mock_db
-
-        # Setup mock for json.load to return some running configuration
-        mock_get_running_config.return_value = {
-            "tables": {
-                "ACL_TABLE": {
-                    "services_to_validate": ["aclservice"],
-                    "validate_commands": ["acl_loader show table"]
-                },
-                "PORT": {
-                    "services_to_validate": ["portservice"],
-                    "validate_commands": ["show interfaces status"]
-                }
-            },
-            "services": {
-                "aclservice": {
-                    "validate_commands": ["acl_loader show table"]
-                },
-                "portservice": {
-                    "validate_commands": ["show interfaces status"]
-                }
-            }
-        }
+        mock_get_running_config.side_effect = mock_get_running_config_side_effect
 
         # Instantiate ChangeApplier with the default scope
         applier = generic_config_updater.change_applier.ChangeApplier(scope="asic0")
@@ -120,7 +104,7 @@ def test_apply_change_given_scope(self, mock_ConfigDBConnector, mock_get_running
         # Assert ConfigDBConnector called with the correct scope
         mock_ConfigDBConnector.assert_called_once_with(use_unix_socket_path=True, namespace="asic0")
 
-    @patch('generic_config_updater.change_applier.ChangeApplier._get_running_config', autospec=True)
+    @patch('generic_config_updater.change_applier.get_config_db_as_json', autospec=True)
     @patch('generic_config_updater.change_applier.ConfigDBConnector', autospec=True)
     def test_apply_change_failure(self, mock_ConfigDBConnector, mock_get_running_config):
         # Setup mock for ConfigDBConnector
@@ -142,22 +126,25 @@ def test_apply_change_failure(self, mock_ConfigDBConnector, mock_get_running_con
 
         self.assertTrue('Failed to get running config' in str(context.exception))
 
-    @patch('generic_config_updater.change_applier.ChangeApplier._get_running_config', autospec=True)
+    @patch('generic_config_updater.change_applier.get_config_db_as_json', autospec=True)
     @patch('generic_config_updater.change_applier.ConfigDBConnector', autospec=True)
     def test_apply_patch_with_empty_tables_failure(self, mock_ConfigDBConnector, mock_get_running_config):
         # Setup mock for ConfigDBConnector
         mock_db = MagicMock()
         mock_ConfigDBConnector.return_value = mock_db
 
         # Setup mock for json.load to simulate configuration where crucial tables are unexpectedly empty
-        mock_get_running_config.return_value = {
-            "tables": {
-                # Simulate empty tables or missing crucial configuration
-            },
-            "services": {
-                # Normally, services would be listed here
+        def mock_get_empty_running_config_side_effect():
+            return {
+                "tables": {
+                    # Simulate empty tables or missing crucial configuration
+                },
+                "services": {
+                    # Normally, services would be listed here
+                }
             }
-        }
+
+        mock_get_running_config.side_effect = mock_get_empty_running_config_side_effect
 
         # Instantiate ChangeApplier with a specific scope to simulate applying changes in a multi-asic environment
         applier = generic_config_updater.change_applier.ChangeApplier(scope="asic0")