From 7863b59531686ccadcbf20c18518d60d33b0063e Mon Sep 17 00:00:00 2001 From: Renuka Manavalan Date: Fri, 1 Oct 2021 23:47:21 +0000 Subject: [PATCH 01/16] ChangeApplier and unit test code. --- generic_config_updater/change_applier.py | 52 +++ generic_config_updater/generic_updater.py | 7 +- .../change_applier_test.py | 164 ++++++++++ .../files/change_applier_test.data.json | 301 ++++++++++++++++++ 4 files changed, 520 insertions(+), 4 deletions(-) create mode 100644 generic_config_updater/change_applier.py create mode 100644 tests/generic_config_updater/change_applier_test.py create mode 100644 tests/generic_config_updater/files/change_applier_test.data.json diff --git a/generic_config_updater/change_applier.py b/generic_config_updater/change_applier.py new file mode 100644 index 0000000000..e85e3fc950 --- /dev/null +++ b/generic_config_updater/change_applier.py @@ -0,0 +1,52 @@ +import copy +import json +import os +import tempfile +from swsscommon.swsscommon import ConfigDBConnector +from .gu_common import JsonChange + + +def get_running_config(): + (_, fname) = tempfile.mkstemp(suffix="_changeApplier") + os.system("sonic-cfggen -d --print-data > {}".format(fname)) + run_data = {} + with open(fname, "r") as s: + run_data = json.load(s) + if os.path.isfile(fname): + os.remove(fname) + return run_data + + +def get_config_db(): + config_db = ConfigDBConnector() + config_db.connect() + return config_db + + +def set_config(config_db, tbl, key, data): + config_db.set_entry(tbl, key, data) + + +class ChangeApplier: + def __init__(self): + self.config_db = get_config_db() + + def _upd_data(self, tbl, run_tbl, upd_tbl): + for key in set(run_tbl.keys()).union(set(upd_tbl.keys())): + run_data = run_tbl.get(key, None) + upd_data = upd_tbl.get(key, None) + + if run_data != upd_data: + set_config(self.config_db, tbl, key, upd_data) + + + def apply(self, change): + run_data = get_running_config() + upd_data = change.apply(copy.deepcopy(run_data)) + + for tbl in set(run_data.keys()).union(set(upd_data.keys())): + self._upd_data(tbl, run_data.get(tbl, {}), upd_data.get(tbl, {})) + + + + diff --git a/generic_config_updater/generic_updater.py b/generic_config_updater/generic_updater.py index 8fd36ced91..adfcc1e79f 100644 --- a/generic_config_updater/generic_updater.py +++ b/generic_config_updater/generic_updater.py @@ -4,6 +4,8 @@ from .gu_common import GenericConfigUpdaterError, ConfigWrapper, \ DryRunConfigWrapper, PatchWrapper from .patch_sorter import PatchSorter +from .change_applier import ChangeApplier + CHECKPOINTS_DIR = "/etc/sonic/checkpoints" CHECKPOINT_EXT = ".cp.json" @@ -17,15 +19,12 @@ def release_lock(self): # TODO: Implement ConfigLock pass -class ChangeApplier: - def apply(self, change): - # TODO: Implement change applier - raise NotImplementedError("ChangeApplier.apply(change) is not implemented yet") class ConfigFormat(Enum): CONFIGDB = 1 SONICYANG = 2 + class PatchApplier: def __init__(self, patchsorter=None, diff --git a/tests/generic_config_updater/change_applier_test.py b/tests/generic_config_updater/change_applier_test.py new file mode 100644 index 0000000000..216e9e95e2 --- /dev/null +++ b/tests/generic_config_updater/change_applier_test.py @@ -0,0 +1,164 @@ +import copy +import json +import os +import unittest +from unittest.mock import patch + +import generic_config_updater.change_applier +import generic_config_updater.gu_common + +SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) +DATA_FILE = os.path.join(SCRIPT_DIR, "files", "change_applier_test.data.json") + +read_data = {} +running_config = {} +json_changes = {} +json_change_index = 0 + +DB_HANDLE = "config_db" + +in_debug = False + +def debug_print(msg): + if in_debug: + print(msg) + + +# Mimics os.system call for sonic-cfggen -d --print-data > filename +# +def os_system_cfggen(cmd): + global running_config + + fname = cmd.split(">")[-1].strip() + with open(fname, "w") as s: + s.write(json.dumps(running_config, indent=4)) + debug_print("File created {} type={} cfg={}".format(fname, + type(running_config), json.dumps(running_config)[1:40])) + return 0 + + +# mimics config_db.set_entry +# +def set_entry(config_db, tbl, key, data): + global running_config, json_changes, json_change_index + + assert config_db == DB_HANDLE + debug_print("set_entry: {} {} {}".format(tbl, key, str(data))) + + json_change = json_changes[json_change_index] + change_data = json_change["update"] if data != None else json_change["remove"] + + assert tbl in change_data + assert key in change_data[tbl] + + if data != None: + if tbl not in running_config: + running_config[tbl] = {} + running_config[tbl][key] = data + else: + assert tbl in running_config + assert key in running_config[tbl] + running_config[tbl].pop(key) + if not running_config[tbl]: + running_config.pop(tbl) + + change_data[tbl].pop(key) + if not change_data[tbl]: + change_data.pop(tbl) + + +# mimics JsonChange.apply +# +class mock_obj: + def apply(self, config): + json_change = json_changes[json_change_index] + + update = copy.deepcopy(json_change["update"]) + for tbl in update: + if tbl not in config: + config[tbl] = {} + for key in update[tbl]: + debug_print("apply: tbl={} key={} ".format(tbl, key)) + if key in config[tbl]: + config[tbl][key].update(update[tbl][key]) + else: + config[tbl][key] = update[tbl][key] + + remove = json_change["remove"] + for tbl in remove: + if tbl in config: + for key in remove[tbl]: + config[tbl].pop(key, None) + debug_print("apply: popped tbl={} key={}".format(tbl, key)) + return config + +def print_diff(expect, ct): + for tbl in set(expect.keys()).union(set(ct.keys())): + if tbl not in expect: + debug_print("Unexpected table in current: {}".format(tbl)) + elif tbl not in ct: + debug_print("Missing table in current: {}".format(tbl)) + else: + ex_tbl = expect[tbl] + ct_tbl = ct[tbl] + for key in set(ex_tbl.keys()).union(set(ct_tbl.keys())): + if key not in ex_tbl: + debug_print("Unexpected key in current: {}/{}".format(tbl, key)) + elif key not in ct_tbl: + debug_print("Missing key in current: {}/{}".format(tbl, key)) + else: + ex_val = ex_tbl[key] + ct_val = ct_tbl[key] + if ex_val != ct_val: + debug_print("Val mismatch {}/{} expect:{} ct: {}".format( + tbl, key, ex_val, ct_val)) + debug_print("diff is complete") + + + +class TestChangeApplier(unittest.TestCase): + + @patch("generic_config_updater.change_applier.os.system") + @patch("generic_config_updater.change_applier.get_config_db") + @patch("generic_config_updater.change_applier.set_config") + def test_application(self, mock_set, mock_db, mock_os_sys): + global read_data, running_config, json_changes, json_change_index + + mock_os_sys.side_effect = os_system_cfggen + mock_db.return_value = DB_HANDLE + mock_set.side_effect = set_entry + + with open(DATA_FILE, "r") as s: + read_data = json.load(s) + + running_config = copy.deepcopy(read_data["running_data"]) + json_changes = copy.deepcopy(read_data["json_changes"]) + + applier = generic_config_updater.change_applier.ChangeApplier() + debug_print("invoked applier") + + for i in range(len(json_changes)): + json_change_index = i + debug_print("main: json_change_index={}".format(json_change_index)) + applier.apply(mock_obj()) + + # All changes are consumed + for i in range(len(json_changes)): + debug_print("Checking: index={} update:{} remove:{}".format(i, + json.dumps(json_changes[i]["update"])[0:20], + json.dumps(json_changes[i]["remove"])[0:20])) + assert not json_changes[i]["update"] + assert not json_changes[i]["remove"] + + # Test data is set up in such a way the multiple changes + # finally brings it back to original config. + # + if read_data["running_data"] != running_config: + print_diff(read_data["running_data"], running_config) + + assert read_data["running_data"] == running_config + + debug_print("all good for applier") + + + diff --git a/tests/generic_config_updater/files/change_applier_test.data.json b/tests/generic_config_updater/files/change_applier_test.data.json new file mode 100644 index 0000000000..e74972daa0 --- /dev/null +++ b/tests/generic_config_updater/files/change_applier_test.data.json @@ -0,0 +1,301 @@ +{ + "running_data": { + "ACL_TABLE": { + "DATAACL": { + "policy_desc": "DATAACL", + "ports": [ + "PortChannel0001", + "PortChannel0002", + "PortChannel0003", + "PortChannel0007" + ], + "stage": "ingress", + "type": "L3" + }, + "EVERFLOW": { + "policy_desc": "EVERFLOW", + "ports": [ + "PortChannel0001", + "PortChannel0002", + "PortChannel0003", + "PortChannel0004", + "Ethernet96" + ], + "stage": "ingress", + "type": "MIRROR" + }, + "EVERFLOWV6": { + "policy_desc": "EVERFLOWV6", + "ports": [ + "PortChannel0001", + "PortChannel0002", + "PortChannel0003", + "PortChannel0004", + "Ethernet96" + ], + "stage": "ingress", + "type": "MIRRORV6" + } + }, + "BGP_NEIGHBOR": { + "10.0.0.57": { + "asn": "64600", + "holdtime": "10", + "keepalive": "3", + "local_addr": "10.0.0.56", + "name": "ARISTA01T1", + "nhopself": "0", + "rrclient": "0" + }, + "10.0.0.59": { + "asn": "64600", + "holdtime": "10", + "keepalive": "3", + "local_addr": "10.0.0.58", + "name": "ARISTA02T1", + "nhopself": "0", + "rrclient": "0" + } + }, + "BGP_PEER_RANGE": { + "BGPSLBPassive": { + "ip_range": [ + "10.255.0.0/25" + ], + "name": "BGPSLBPassive", + "src_address": "10.1.0.32" + }, + "BGPVac": { + "ip_range": [ + "192.168.0.0/21" + ], + "name": "BGPVac", + "src_address": "10.1.0.32" + } + }, + "BUFFER_PG": { + "Ethernet0|3-4": { + "profile": "[BUFFER_PROFILE|pg_lossless_40000_300m_profile]" + }, + "Ethernet100|3-4": { + "profile": "[BUFFER_PROFILE|pg_lossless_40000_300m_profile]" + }, + "Ethernet104|3-4": { + "profile": "[BUFFER_PROFILE|pg_lossless_40000_300m_profile]" + }, + "Ethernet108|3-4": { + "profile": "[BUFFER_PROFILE|pg_lossless_40000_300m_profile]" + }, + "Ethernet112|0": { + "profile": "[BUFFER_PROFILE|ingress_lossy_profile]" + }, + "Ethernet112|3-4": { + "profile": "[BUFFER_PROFILE|pg_lossless_40000_300m_profile]" + } + }, + "DEVICE_METADATA": { + "localhost": { + "bgp_asn": "65100", + "buffer_model": "traditional", + "cloudtype": "None", + "default_bgp_status": "down", + "default_pfcwd_status": "enable", + "deployment_id": "1", + "docker_routing_config_mode": "separated", + "type": "ToRRouter" + } + }, + "DEVICE_NEIGHBOR_METADATA": { + "ARISTA01T1": { + "hwsku": "Arista-VM", + "lo_addr": "None", + "mgmt_addr": "10.64.246.220", + "type": "LeafRouter" + }, + "ARISTA02T1": { + "hwsku": "Arista-VM", + "lo_addr": "None", + "mgmt_addr": "10.64.246.221", + "type": "LeafRouter" + } + }, + "PORT": { + "Ethernet0": { + "alias": "fortyGigE0/0", + "description": "fortyGigE0/0", + "index": "0", + "lanes": "29,30,31,32", + "mtu": "9100", + "pfc_asym": "off", + "speed": "40000" + }, + "Ethernet100": { + "alias": "fortyGigE0/100", + "description": "fortyGigE0/100", + "index": "25", + "lanes": "125,126,127,128", + "mtu": "9100", + "pfc_asym": "off", + "speed": "40000" + }, + "Ethernet104": { + "alias": "fortyGigE0/104", + "description": "fortyGigE0/104", + "index": "26", + "lanes": "85,86,87,88", + "mtu": "9100", + "pfc_asym": "off", + "speed": "40000" + }, + "Ethernet108": { + "alias": "fortyGigE0/108", + "description": "fortyGigE0/108", + "index": "27", + "lanes": "81,82,83,84", + "mtu": "9100", + "pfc_asym": "off", + "speed": "40000" + } + }, + "PORTCHANNEL": { + "PortChannel0001": { + "admin_status": "up", + "members": [ + "Ethernet112" + ], + "min_links": "1", + "mtu": "9100" + }, + "PortChannel0002": { + "admin_status": "up", + "members": [ + "Ethernet116" + ], + "min_links": "1", + "mtu": "9100" + } + }, + "PORTCHANNEL_INTERFACE": { + "PortChannel0001": {}, + "PortChannel0002": {}, + "PortChannel0001|10.0.0.56/31": {}, + "PortChannel0001|FC00::71/126": {}, + "PortChannel0002|10.0.0.58/31": {}, + "PortChannel0002|FC00::75/126": {} + }, + "PORTCHANNEL_MEMBER": { + "PortChannel0001|Ethernet112": {}, + "PortChannel0002|Ethernet116": {}, + "PortChannel0003|Ethernet120": {}, + "PortChannel0004|Ethernet124": {} + }, + "QUEUE": { + "Ethernet112|0": { + "scheduler": "[SCHEDULER|scheduler.0]" + }, + "Ethernet112|1": { + "scheduler": "[SCHEDULER|scheduler.0]" + } + }, + "VLAN": { + "Vlan1000": { + "dhcp_servers": [ + "192.0.0.1", + "192.0.0.2", + "192.0.0.48" + ], + "members": [ + "Ethernet4", + "Ethernet8", + "Ethernet96" + ], + "vlanid": "1000" + } + }, + "VLAN_INTERFACE": { + "Vlan1000": {}, + "Vlan1000|192.168.0.1/21": {}, + "Vlan1000|2603:10b0:b13:c70::1/64": {} + }, + "VLAN_MEMBER": { + "Vlan1000|Ethernet12": { + "tagging_mode": "untagged" + }, + "Vlan1000|Ethernet16": { + "tagging_mode": "untagged" + }, + "Vlan1000|Ethernet28": { + } + }, + "WRED_PROFILE": { + "AZURE_LOSSLESS": { + "ecn": "ecn_all", + "green_drop_probability": "5", + "yellow_drop_probability": "5", + "yellow_max_threshold": "2097152", + "yellow_min_threshold": "1048576" + } + } + }, + "json_changes": [ + { + "name": "change_1", + "update": { + "VLAN_INTERFACE": { "Vlan2000": {}, "Vlan2000|192.168.0.2/21": {} }, + "WRED_PROFILE": { "AZURE_LOSSLESS": { "green_drop_probability": "99", "ecn": "88" }}, + "ACL_TABLE": {"DATAACL": {"type": "test_data" }} + }, + "remove": { + "BGP_NEIGHBOR": { "10.0.0.57": {} }, + "BGP_PEER_RANGE": { "BGPSLBPassive": {} } + } + }, + { + "name": "change_2", + "update": { + "BGP_NEIGHBOR": { "10.0.0.57": { + "asn": "64600", + "holdtime": "10", + "keepalive": "3", + "local_addr": "10.0.0.56", + "name": "ARISTA01T1", + "nhopself": "0", + "rrclient": "0" }}, + "WRED_PROFILE": { "AZURE_LOSSLESS": { "ecn": "ecn_all" } }, + "ACL_TABLE": {"DATAACL": {"type": "test_data11" }}, + "TEST_ONLY" : { "TEST_SUB" : {"foo": "88" } } + }, + "remove": { + "VLAN_INTERFACE": { "Vlan2000": {} } + } + }, + { + "name": "change_3", + "update": { + "WRED_PROFILE": { "AZURE_LOSSLESS": { "green_drop_probability": "5" } }, + "ACL_TABLE": {"DATAACL": { + "policy_desc": "DATAACL", + "ports": [ + "PortChannel0001", + "PortChannel0002", + "PortChannel0003", + "PortChannel0007" + ], + "stage": "ingress", + "type": "L3" } + }, + "BGP_PEER_RANGE": { + "BGPSLBPassive": { + "ip_range": ["10.255.0.0/25"], + "name": "BGPSLBPassive", + "src_address": "10.1.0.32" + } + } + }, + "remove": { + "VLAN_INTERFACE": { "Vlan2000|192.168.0.2/21": {} }, + "TEST_ONLY": { "TEST_SUB": {} } + } + } + ] +} From 84f493bdb280cf3bb358f70c9082f15e6faadfee Mon Sep 17 00:00:00 2001 From: Renuka Manavalan Date: Mon, 4 Oct 2021 21:54:05 +0000 Subject: [PATCH 02/16] minor update --- generic_config_updater/change_applier.py | 25 ++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/generic_config_updater/change_applier.py b/generic_config_updater/change_applier.py index e85e3fc950..5f2f94c85c 100644 --- a/generic_config_updater/change_applier.py +++ b/generic_config_updater/change_applier.py @@ -6,17 +6,6 @@ from .gu_common import JsonChange -def get_running_config(): - (_, fname) = tempfile.mkstemp(suffix="_changeApplier") - os.system("sonic-cfggen -d --print-data > {}".format(fname)) - run_data = {} - with open(fname, "r") as s: - run_data = json.load(s) - if os.path.isfile(fname): - os.remove(fname) - return run_data - - def get_config_db(): config_db = ConfigDBConnector() config_db.connect() @@ -41,12 +30,24 @@ def _upd_data(self, tbl, run_tbl, upd_tbl): def apply(self, change): - run_data = get_running_config() + run_data = self._get_running_config() upd_data = change.apply(copy.deepcopy(run_data)) for tbl in set(run_data.keys()).union(set(upd_data.keys())): self._upd_data(tbl, run_data.get(tbl, {}), upd_data.get(tbl, {})) + + def _get_running_config(self): + (_, fname) = tempfile.mkstemp(suffix="_changeApplier") + os.system("sonic-cfggen -d --print-data > {}".format(fname)) + run_data = {} + with open(fname, "r") as s: + run_data = json.load(s) + if os.path.isfile(fname): + os.remove(fname) + return run_data + + From dd337eb50c3d9a6136c80564667d86644db03867 Mon Sep 17 00:00:00 2001 From: Renuka Manavalan Date: Mon, 4 Oct 2021 23:47:23 +0000 Subject: [PATCH 03/16] fix unused import --- generic_config_updater/change_applier.py | 1 - 1 file changed, 1 deletion(-) diff --git a/generic_config_updater/change_applier.py b/generic_config_updater/change_applier.py index 5f2f94c85c..a1ea593e39 100644 --- a/generic_config_updater/change_applier.py +++ b/generic_config_updater/change_applier.py @@ -3,7 +3,6 @@ import os import tempfile from swsscommon.swsscommon import ConfigDBConnector -from .gu_common import JsonChange def get_config_db(): From 6e0bba0e50327ca1614accc7e4b90a3dcc8a5dfc Mon Sep 17 00:00:00 2001 From: Renuka Manavalan Date: Mon, 11 Oct 2021 18:27:25 +0000 Subject: [PATCH 04/16] Added service validation --- generic_config_updater/change_applier.py | 55 ++++++++- generic_config_updater/generic_updater.py | 6 +- .../generic_updater_config.conf.json | 44 +++++++ generic_config_updater/gu_common.py | 30 +++++ .../change_applier_test.py | 108 +++++++++++++++++- .../files/change_applier_test.conf.json | 24 ++++ .../files/change_applier_test.data.json | 5 + 7 files changed, 265 insertions(+), 7 deletions(-) create mode 100644 generic_config_updater/generic_updater_config.conf.json create mode 100644 tests/generic_config_updater/files/change_applier_test.conf.json diff --git a/generic_config_updater/change_applier.py b/generic_config_updater/change_applier.py index a1ea593e39..b9011845d5 100644 --- a/generic_config_updater/change_applier.py +++ b/generic_config_updater/change_applier.py @@ -1,8 +1,12 @@ import copy import json +import importlib import os +import syslog import tempfile +from collections import defaultdict from swsscommon.swsscommon import ConfigDBConnector +from .gu_common import log_error, log_debug, log_info def get_config_db(): @@ -15,25 +19,72 @@ def set_config(config_db, tbl, key, data): config_db.set_entry(tbl, key, data) +UPDATER_CONF_FILE = "/etc/sonic/generic_config_updater.conf" +updater_data = None + class ChangeApplier: def __init__(self): + global updater_data, log_level + self.config_db = get_config_db() + if updater_data == None: + with open(UPDATER_CONF_FILE, "r") as s: + updater_data = json.load(s) + + + def _invoke_cmd(cmd, old_cfg, upd_cfg, keys): + method_name = cmd.split(".")[-1] + module_name = ".".join(cmd.split(".")[0:-1]) + + module = importlib.import_module(module_name, package=None) + method_to_call = getattr(module, method_name) + + return method_to_call(old_cfg, upd_cfg, keys) + + + def _services_validate(old_cfg, upd_cfg, keys): + lst_svcs = set() + lst_cmds = set() + if not keys: + keys[""] = {} + for tbl in keys: + lst_svcs.update(updater_data.get(tbl, {}).get("services_to_validate", [])) + for svc in lst_svcs: + lst_cmds.update(updater_data.get(svc, {}).get("validate_commands", [])) + + for cmd in lst_cmds: + ret = _invoke_cmd(cmd, old_cfg, upd_cfg, keys) + if ret: + return ret + return 0 + - def _upd_data(self, tbl, run_tbl, upd_tbl): + def _upd_data(self, tbl, run_tbl, upd_tbl, upd_keys): for key in set(run_tbl.keys()).union(set(upd_tbl.keys())): run_data = run_tbl.get(key, None) upd_data = upd_tbl.get(key, None) if run_data != upd_data: set_config(self.config_db, tbl, key, upd_data) + upd_keys[tbl][key] = {} def apply(self, change): run_data = self._get_running_config() upd_data = change.apply(copy.deepcopy(run_data)) + upd_keys = defaultdict(dict) for tbl in set(run_data.keys()).union(set(upd_data.keys())): - self._upd_data(tbl, run_data.get(tbl, {}), upd_data.get(tbl, {})) + self._upd_data(tbl, run_data.get(tbl, {}), + upd_data.get(tbl, {}), upd_keys) + + ret = _services_validate(run_data, upd_data, upd_keys) + if not ret: + run_data = self._get_running_config() + if upd_data != run_data: + report_mismatch(run_data, upd_data) + ret = -1 + return ret def _get_running_config(self): diff --git a/generic_config_updater/generic_updater.py b/generic_config_updater/generic_updater.py index adfcc1e79f..da517729e2 100644 --- a/generic_config_updater/generic_updater.py +++ b/generic_config_updater/generic_updater.py @@ -2,7 +2,9 @@ import os from enum import Enum from .gu_common import GenericConfigUpdaterError, ConfigWrapper, \ - DryRunConfigWrapper, PatchWrapper + DryRunConfigWrapper, PatchWrapper, \ + set_log_level + from .patch_sorter import PatchSorter from .change_applier import ChangeApplier @@ -296,7 +298,7 @@ def init_verbose_logging(self, verbose): # Usually logs have levels such as: error, warning, info, debug. # By default all log levels should show up to the user, except debug. # By allowing verbose logging, debug msgs will also be shown to the user. - pass + set_log_level(syslog.LOG_ERR if not verbose else syslog.LOG_DEBUG) def get_config_wrapper(self, dry_run): if dry_run: diff --git a/generic_config_updater/generic_updater_config.conf.json b/generic_config_updater/generic_updater_config.conf.json new file mode 100644 index 0000000000..89365e64e1 --- /dev/null +++ b/generic_config_updater/generic_updater_config.conf.json @@ -0,0 +1,44 @@ +{ + "tables": { + "": { + "services_to_validate": [ "system_health" ] + }, + "PORT": { + "services_to_validate": [ "port_service" ] + } + }, + "README": [ + 'Validate_commands provides, module & method name as ', + ' .', + 'NOTE: module name could have "."', + ' ', + 'The last element separated by "." is considered as ', + 'method name', + '', + 'e.g. "show.acl.test_acl"', + '', + 'Here we load "show.acl" and call "test_acl" method on it.', + '', + 'called as:', + ' .>(, ', + ' , )', + ' config is in JSON format as in config_db.json', + ' affected_keys in same format, but w/o value', + ' { "ACL_TABLE": { "SNMP_ACL": {} ... }, ...}', + ' The affected keys has "added", "updated" & "deleted"', + '', + 'Multiple validate commands may be provided.',', + '', + 'Note: The commands may be called in any order', + '' + ], + "services": { + "system_health": { + "validate_commands": [ ] + }, + "port_service": { + "validate_commands": [ ] + } + } +} + diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index 66d9b0d7d9..4125634d32 100644 --- a/generic_config_updater/gu_common.py +++ b/generic_config_updater/gu_common.py @@ -13,6 +13,36 @@ class GenericConfigUpdaterError(Exception): pass +log_level = syslog.LOG_ERR + +def _log_msg(lvl, m): + if lvl <= log_level: + syslog.syslog(lvl, m) + if log_level == syslog.LOG_DEBUG: + print(m) + +def log_error(m): + _log_msg(syslog.LOG_ERR, m) + + +def log_info(m): + _log_msg(syslog.LOG_INFO, m) + + +def log_debug(m): + _log_msg(syslog.LOG_DEBUG, m) + + +def run_cmd(cmd): + proc = subprocess.run(cmd, shell=True, capture_output=True) + if proc.returncode: + log_error("Failed to run: ret={} cmd: {}".format( + proc.returncode, proc.args)) + log_error(f"Failed to run: stdout: {proc.stdout}") + log_error(f"Failed to run: stderr: {proc.stderr}") + return proc.returncode + + class JsonChange: """ A class that describes a partial change to a JSON object. diff --git a/tests/generic_config_updater/change_applier_test.py b/tests/generic_config_updater/change_applier_test.py index 216e9e95e2..e171e634e7 100644 --- a/tests/generic_config_updater/change_applier_test.py +++ b/tests/generic_config_updater/change_applier_test.py @@ -2,6 +2,7 @@ import json import os import unittest +from collections import defaultdict from unittest.mock import patch import generic_config_updater.change_applier @@ -9,10 +10,52 @@ SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) DATA_FILE = os.path.join(SCRIPT_DIR, "files", "change_applier_test.data.json") +CONF_FILE = os.path.join(SCRIPT_DIR, "files", "change_applier_test.conf.json") +# +# Datafile is structured as +# "running_config": {....} +# "json_changes": [ {"name": ..., "update": { : {: {}, ...}...}, +# "remove": { : { : {}, ..}, ...} }, ...] +# +# The json_changes is read into global json_changes +# The applier is called with each change +# The mocked JsonChange.apply applies this diff on given config +# The applier calls set_entry to update redis +# But we mock set_entry, and that instead: +# remove the corresponding changes from json_changes. +# Updates the global running_config +# +# At the end of application of all changes, expect global json-changes to +# be empty, which assures that set_entry is called for all expected keys. +# The global running config would reflect the final config +# +# The changes are written in such a way, upon the last change, the config +# will be same as the original config that we started with or as read from +# data file. +# +# So compare global running_config with read_data for running config +# from the file. +# This compares the integrity of final o/p +# Data read from file read_data = {} + +# Keep a copy of running_config before calling apply +# This is used by service_validate call to verify the args +# Args from change applier: ( +# +start_running_config = {} + +# The mock_set_entry (otherwise redis update) reflects the final config +# service_validate calls will verify against this +# running_config = {} + +# Copy of changes read. Used by mock JsonChange.apply +# Cleared by mocked set_entry json_changes = {} + +# The index into list of i/p json changes for mock code to use json_change_index = 0 DB_HANDLE = "config_db" @@ -115,13 +158,64 @@ def print_diff(expect, ct): debug_print("diff is complete") +# Test validators +# +def system_health(old_cfg, new_cfg, keys): + svc_name = "system_health" + if old_cfg != new_cfg: + print_diff(old_cfg, new_cfg) + assert False, "No change expected" + svcs = json_changes[json_change_index].get("services_validated", None) + if svcs != None: + assert svc_name not in svcs + svcs.remove(svc_name) + + +def _validate_keys(keys): + change = copy.deepcopy(read_data["json_changes"][json_change_index]) + change.update(read_data["json_changes"][json_change_index]) + + for tbl in set(change.keys()).union(set(keys.keys())): + assert tbl in change + assert tbl in keys + chg_tbl = change[tbl] + keys_tbl = keys[tbl] + for key in set(chg_tbl.keys()).union(set(keys_tbl.keys())): + assert key not in chg_tbl + assert key not in keys_tbl + + +def _validate_svc(svc_name, old_cfg, new_cfg, keys): + if old_cfg != start_running_config: + print_diff(old_cfg, start_running_config) + assert False + + if new_cfg != running_config: + print_diff(old_cfg, running_config) + assert False + + _validate_keys(keys) + + svcs = json_changes[json_change_index].get("services_validated", None) + if svcs != None: + assert svc_name not in svcs + svcs.remove(svc_name) + + +def acl_validate(old_cfg, new_cfg, keys): + _validate_svc("acl_validate", old_cfg, new_cfg, keys) + + +def vlan_validate(old_cfg, new_cfg, keys): + _validate_svc("vlan_validate", old_cfg, new_cfg, keys) + class TestChangeApplier(unittest.TestCase): + @patch("generic_config_updater.gu_common.subprocess.run") @patch("generic_config_updater.change_applier.os.system") @patch("generic_config_updater.change_applier.get_config_db") @patch("generic_config_updater.change_applier.set_config") - def test_application(self, mock_set, mock_db, mock_os_sys): global read_data, running_config, json_changes, json_change_index mock_os_sys.side_effect = os_system_cfggen @@ -134,21 +228,29 @@ def test_application(self, mock_set, mock_db, mock_os_sys): running_config = copy.deepcopy(read_data["running_data"]) json_changes = copy.deepcopy(read_data["json_changes"]) + generic_config_updater.change_applier.UPDATER_CONF_FILE = CONF_FILE + applier = generic_config_updater.change_applier.ChangeApplier() debug_print("invoked applier") for i in range(len(json_changes)): json_change_index = i + + # Take copy for comparison + start_running_config = copy.deepcopy(running_config) + debug_print("main: json_change_index={}".format(json_change_index)) applier.apply(mock_obj()) # All changes are consumed for i in range(len(json_changes)): - debug_print("Checking: index={} update:{} remove:{}".format(i, + debug_print("Checking: index={} update:{} remove:{} svcs:{}".format(i, json.dumps(json_changes[i]["update"])[0:20], - json.dumps(json_changes[i]["remove"])[0:20])) + json.dumps(json_changes[i]["remove"])[0:20], + json.dumps(json_changes[i].get("services_validated", []))[0:20])) assert not json_changes[i]["update"] assert not json_changes[i]["remove"] + assert not json_changes[i].get("services_validated", []) # Test data is set up in such a way the multiple changes # finally brings it back to original config. diff --git a/tests/generic_config_updater/files/change_applier_test.conf.json b/tests/generic_config_updater/files/change_applier_test.conf.json new file mode 100644 index 0000000000..4b4a31aad5 --- /dev/null +++ b/tests/generic_config_updater/files/change_applier_test.conf.json @@ -0,0 +1,24 @@ +{ + "tables": { + "": { + "services_to_validate": [ "system_health" ] + }, + "PORT": { + "services_to_validate": [ "port_service" ] + }, + "VLAN_INTERFACE": { + "services_to_validate": [ "port_service", "vlan_service" ] + } + }, + "services": { + "system_health": { + "validate_commands": [ "..tests.generic_config_updater.change_applier_test.system_health" ] + } + "ACL_TABLE": { + "validate_commands": [ "..tests.generic_config_updater.change_applier_test.acl_validate" ] + }, + "VLAN_INTERFACE": { + "validate_commands": [ "..tests.generic_config_updater.change_applier_test.vlan_validate" ] + } + } +} diff --git a/tests/generic_config_updater/files/change_applier_test.data.json b/tests/generic_config_updater/files/change_applier_test.data.json index e74972daa0..bb36101920 100644 --- a/tests/generic_config_updater/files/change_applier_test.data.json +++ b/tests/generic_config_updater/files/change_applier_test.data.json @@ -238,6 +238,11 @@ } }, "json_changes": [ + { + "name": "change_0", + "update": {}, + "remove": {} + }, { "name": "change_1", "update": { From cabcf23bf69623a56e77d2bd73a5ef7707e4575d Mon Sep 17 00:00:00 2001 From: Renuka Manavalan Date: Tue, 19 Oct 2021 16:30:32 +0000 Subject: [PATCH 05/16] Take off added code for logging --- generic_config_updater/generic_updater.py | 3 +-- generic_config_updater/gu_common.py | 30 ----------------------- 2 files changed, 1 insertion(+), 32 deletions(-) diff --git a/generic_config_updater/generic_updater.py b/generic_config_updater/generic_updater.py index da517729e2..a678ae7c4d 100644 --- a/generic_config_updater/generic_updater.py +++ b/generic_config_updater/generic_updater.py @@ -26,7 +26,6 @@ class ConfigFormat(Enum): CONFIGDB = 1 SONICYANG = 2 - class PatchApplier: def __init__(self, patchsorter=None, @@ -298,7 +297,7 @@ def init_verbose_logging(self, verbose): # Usually logs have levels such as: error, warning, info, debug. # By default all log levels should show up to the user, except debug. # By allowing verbose logging, debug msgs will also be shown to the user. - set_log_level(syslog.LOG_ERR if not verbose else syslog.LOG_DEBUG) + pass def get_config_wrapper(self, dry_run): if dry_run: diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index 4125634d32..66d9b0d7d9 100644 --- a/generic_config_updater/gu_common.py +++ b/generic_config_updater/gu_common.py @@ -13,36 +13,6 @@ class GenericConfigUpdaterError(Exception): pass -log_level = syslog.LOG_ERR - -def _log_msg(lvl, m): - if lvl <= log_level: - syslog.syslog(lvl, m) - if log_level == syslog.LOG_DEBUG: - print(m) - -def log_error(m): - _log_msg(syslog.LOG_ERR, m) - - -def log_info(m): - _log_msg(syslog.LOG_INFO, m) - - -def log_debug(m): - _log_msg(syslog.LOG_DEBUG, m) - - -def run_cmd(cmd): - proc = subprocess.run(cmd, shell=True, capture_output=True) - if proc.returncode: - log_error("Failed to run: ret={} cmd: {}".format( - proc.returncode, proc.args)) - log_error(f"Failed to run: stdout: {proc.stdout}") - log_error(f"Failed to run: stderr: {proc.stderr}") - return proc.returncode - - class JsonChange: """ A class that describes a partial change to a JSON object. From bd7781c8defafcb2534d5a9092ee2f2dcd8bd796 Mon Sep 17 00:00:00 2001 From: Renuka Manavalan Date: Tue, 19 Oct 2021 16:40:14 +0000 Subject: [PATCH 06/16] fix merge --- generic_config_updater/generic_updater.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/generic_config_updater/generic_updater.py b/generic_config_updater/generic_updater.py index 65d14177e2..91c248aeed 100644 --- a/generic_config_updater/generic_updater.py +++ b/generic_config_updater/generic_updater.py @@ -2,13 +2,11 @@ import os from enum import Enum from .gu_common import GenericConfigUpdaterError, ConfigWrapper, \ - DryRunConfigWrapper, PatchWrapper, \ - set_log_level + DryRunConfigWrapper, PatchWrapper, genericUpdaterLogging from .patch_sorter import PatchSorter from .change_applier import ChangeApplier - CHECKPOINTS_DIR = "/etc/sonic/checkpoints" CHECKPOINT_EXT = ".cp.json" From f839e53f85c5ef52e763591a2dc18a7fa24d2f6b Mon Sep 17 00:00:00 2001 From: Renuka Manavalan Date: Wed, 20 Oct 2021 17:20:31 +0000 Subject: [PATCH 07/16] change applier & test updated for service validation --- generic_config_updater/change_applier.py | 77 +++++++++---- generic_config_updater/generic_updater.py | 1 - .../generic_updater_config.conf.json | 46 ++++---- .../change_applier_test.py | 106 +++++++++--------- .../files/change_applier_test.conf.json | 18 +-- .../files/change_applier_test.data.json | 6 +- 6 files changed, 149 insertions(+), 105 deletions(-) diff --git a/generic_config_updater/change_applier.py b/generic_config_updater/change_applier.py index b9011845d5..3e12a5bc98 100644 --- a/generic_config_updater/change_applier.py +++ b/generic_config_updater/change_applier.py @@ -1,12 +1,40 @@ import copy import json +import jsondiff import importlib import os import syslog import tempfile from collections import defaultdict from swsscommon.swsscommon import ConfigDBConnector -from .gu_common import log_error, log_debug, log_info +from .gu_common import genericUpdaterLogging + + +UPDATER_CONF_FILE = "/etc/sonic/generic_config_updater.conf" +updater_data = None + +logger = genericUpdaterLogging.get_logger(title="Change Applier") + +print_to_console = False +print_to_stdout = False + +def set_print_options(to_console=False, to_stdout=False): + global print_to_console, print_to_stdout + + print_to_console = to_console + print_to_stdout = to_stdout + + +def log_debug(m): + logger.log_debug(m, print_to_console) + if print_to_stdout: + print(m) + + +def log_error(m): + logger.log_error(m, print_to_console) + if print_to_stdout: + print(m) def get_config_db(): @@ -19,20 +47,19 @@ def set_config(config_db, tbl, key, data): config_db.set_entry(tbl, key, data) -UPDATER_CONF_FILE = "/etc/sonic/generic_config_updater.conf" -updater_data = None - class ChangeApplier: def __init__(self): - global updater_data, log_level + global updater_data self.config_db = get_config_db() - if updater_data == None: + if not updater_data: with open(UPDATER_CONF_FILE, "r") as s: updater_data = json.load(s) - def _invoke_cmd(cmd, old_cfg, upd_cfg, keys): + def _invoke_cmd(self, cmd, old_cfg, upd_cfg, keys): + # cmd is in the format as . + # method_name = cmd.split(".")[-1] module_name = ".".join(cmd.split(".")[0:-1]) @@ -42,20 +69,29 @@ def _invoke_cmd(cmd, old_cfg, upd_cfg, keys): return method_to_call(old_cfg, upd_cfg, keys) - def _services_validate(old_cfg, upd_cfg, keys): + def _services_validate(self, old_cfg, upd_cfg, keys): lst_svcs = set() lst_cmds = set() if not keys: + # calling apply with no config would invoke + # default validation, if any + # keys[""] = {} + + tables = updater_data["tables"] for tbl in keys: - lst_svcs.update(updater_data.get(tbl, {}).get("services_to_validate", [])) + lst_svcs.update(tables.get(tbl, {}).get("services_to_validate", [])) + + services = updater_data["services"] for svc in lst_svcs: - lst_cmds.update(updater_data.get(svc, {}).get("validate_commands", [])) + lst_cmds.update(services.get(svc, {}).get("validate_commands", [])) for cmd in lst_cmds: - ret = _invoke_cmd(cmd, old_cfg, upd_cfg, keys) + ret = self._invoke_cmd(cmd, old_cfg, upd_cfg, keys) if ret: + log_error("service invoked: {} failed with ret={}".format(cmd, ret)) return ret + log_debug("service invoked: {}".format(cmd)) return 0 @@ -67,6 +103,12 @@ def _upd_data(self, tbl, run_tbl, upd_tbl, upd_keys): if run_data != upd_data: set_config(self.config_db, tbl, key, upd_data) upd_keys[tbl][key] = {} + log_debug("Patch affected tbl={} key={}".format(tbl, key)) + + + def _report_mismatch(self, run_data, upd_data): + log_error("run_data vs expected_data: {}".format( + str(jsondiff.diff(run_data, upd_data))[0:40])) def apply(self, change): @@ -74,16 +116,18 @@ def apply(self, change): upd_data = change.apply(copy.deepcopy(run_data)) upd_keys = defaultdict(dict) - for tbl in set(run_data.keys()).union(set(upd_data.keys())): + for tbl in sorted(set(run_data.keys()).union(set(upd_data.keys()))): self._upd_data(tbl, run_data.get(tbl, {}), upd_data.get(tbl, {}), upd_keys) - ret = _services_validate(run_data, upd_data, upd_keys) + ret = self._services_validate(run_data, upd_data, upd_keys) if not ret: run_data = self._get_running_config() if upd_data != run_data: - report_mismatch(run_data, upd_data) + self._report_mismatch(run_data, upd_data) ret = -1 + if ret: + log_error("Failed to apply Json change") return ret @@ -96,8 +140,3 @@ def _get_running_config(self): if os.path.isfile(fname): os.remove(fname) return run_data - - - - - diff --git a/generic_config_updater/generic_updater.py b/generic_config_updater/generic_updater.py index 91c248aeed..5a6c7172a1 100644 --- a/generic_config_updater/generic_updater.py +++ b/generic_config_updater/generic_updater.py @@ -3,7 +3,6 @@ from enum import Enum from .gu_common import GenericConfigUpdaterError, ConfigWrapper, \ DryRunConfigWrapper, PatchWrapper, genericUpdaterLogging - from .patch_sorter import PatchSorter from .change_applier import ChangeApplier diff --git a/generic_config_updater/generic_updater_config.conf.json b/generic_config_updater/generic_updater_config.conf.json index 89365e64e1..a0a60add04 100644 --- a/generic_config_updater/generic_updater_config.conf.json +++ b/generic_config_updater/generic_updater_config.conf.json @@ -8,29 +8,29 @@ } }, "README": [ - 'Validate_commands provides, module & method name as ', - ' .', - 'NOTE: module name could have "."', - ' ', - 'The last element separated by "." is considered as ', - 'method name', - '', - 'e.g. "show.acl.test_acl"', - '', - 'Here we load "show.acl" and call "test_acl" method on it.', - '', - 'called as:', - ' .>(, ', - ' , )', - ' config is in JSON format as in config_db.json', - ' affected_keys in same format, but w/o value', - ' { "ACL_TABLE": { "SNMP_ACL": {} ... }, ...}', - ' The affected keys has "added", "updated" & "deleted"', - '', - 'Multiple validate commands may be provided.',', - '', - 'Note: The commands may be called in any order', - '' + "Validate_commands provides, module & method name as ", + " .", + "NOTE: module name could have '.'", + " ", + "The last element separated by '.' is considered as ", + "method name", + "", + "e.g. 'show.acl.test_acl'", + "", + "Here we load 'show.acl' and call 'test_acl' method on it.", + "", + "called as:", + " .>(, ", + " , )", + " config is in JSON format as in config_db.json", + " affected_keys in same format, but w/o value", + " { 'ACL_TABLE': { 'SNMP_ACL': {} ... }, ...}", + " The affected keys has 'added', 'updated' & 'deleted'", + "", + "Multiple validate commands may be provided.", + "", + "Note: The commands may be called in any order", + "" ], "services": { "system_health": { diff --git a/tests/generic_config_updater/change_applier_test.py b/tests/generic_config_updater/change_applier_test.py index e171e634e7..5d5b0908dc 100644 --- a/tests/generic_config_updater/change_applier_test.py +++ b/tests/generic_config_updater/change_applier_test.py @@ -1,5 +1,6 @@ import copy import json +import jsondiff import os import unittest from collections import defaultdict @@ -14,14 +15,21 @@ # # Datafile is structured as # "running_config": {....} -# "json_changes": [ {"name": ..., "update": { : {: {}, ...}...}, -# "remove": { : { : {}, ..}, ...} }, ...] +# "json_changes": [ +# { +# "notes": , +# "update": { : {: {}, ...}...}, +# "remove": { : { : {}, ..}, ...}, +# "services_to_validate": [ , ...] +# }, +# ... +# ] # # The json_changes is read into global json_changes -# The applier is called with each change -# The mocked JsonChange.apply applies this diff on given config -# The applier calls set_entry to update redis -# But we mock set_entry, and that instead: +# The ChangeApplier.apply is called with each change +# The mocked JsonChange.apply applies this diff on given config +# The ChangeApplier.apply calls set_entry to update redis +# But we mock set_entry, and that instead: # remove the corresponding changes from json_changes. # Updates the global running_config # @@ -60,11 +68,8 @@ DB_HANDLE = "config_db" -in_debug = False - def debug_print(msg): - if in_debug: - print(msg) + print(msg) # Mimics os.system call for sonic-cfggen -d --print-data > filename @@ -133,90 +138,83 @@ def apply(self, config): for key in remove[tbl]: config[tbl].pop(key, None) debug_print("apply: popped tbl={} key={}".format(tbl, key)) + if not config[tbl]: + config.pop(tbl, None) + debug_print("apply: popped EMPTY tbl={}".format(tbl)) return config -def print_diff(expect, ct): - for tbl in set(expect.keys()).union(set(ct.keys())): - if tbl not in expect: - debug_print("Unexpected table in current: {}".format(tbl)) - elif tbl not in ct: - debug_print("Missing table in current: {}".format(tbl)) - else: - ex_tbl = expect[tbl] - ct_tbl = ct[tbl] - for key in set(ex_tbl.keys()).union(set(ct_tbl.keys())): - if key not in ex_tbl: - debug_print("Unexpected key in current: {}/{}".format(tbl, key)) - elif key not in ct_tbl: - debug_print("Missing key in current: {}/{}".format(tbl, key)) - else: - ex_val = ex_tbl[key] - ct_val = ct_tbl[key] - if ex_val != ct_val: - debug_print("Val mismatch {}/{} expect:{} ct: {}".format( - tbl, key, ex_val, ct_val)) - debug_print("diff is complete") - # Test validators # def system_health(old_cfg, new_cfg, keys): + debug_print("system_health called") svc_name = "system_health" if old_cfg != new_cfg: - print_diff(old_cfg, new_cfg) + debug_print("system_health: diff={}".format(str( + jsondiff.diff(old_cfg, new_cfg)))) assert False, "No change expected" svcs = json_changes[json_change_index].get("services_validated", None) if svcs != None: - assert svc_name not in svcs + assert svc_name in svcs svcs.remove(svc_name) def _validate_keys(keys): - change = copy.deepcopy(read_data["json_changes"][json_change_index]) - change.update(read_data["json_changes"][json_change_index]) - - for tbl in set(change.keys()).union(set(keys.keys())): - assert tbl in change + # validate keys against original change as read from data file + # + change = read_data["json_changes"][json_change_index] + change_data = copy.deepcopy(change["update"]) + change_data.update(change["remove"]) + + for tbl in set(change_data.keys()).union(set(keys.keys())): + assert tbl in change_data assert tbl in keys - chg_tbl = change[tbl] + chg_tbl = change_data[tbl] keys_tbl = keys[tbl] for key in set(chg_tbl.keys()).union(set(keys_tbl.keys())): - assert key not in chg_tbl - assert key not in keys_tbl + assert key in chg_tbl + assert key in keys_tbl def _validate_svc(svc_name, old_cfg, new_cfg, keys): if old_cfg != start_running_config: - print_diff(old_cfg, start_running_config) - assert False + debug_print("validate svc {}: old diff={}".format(svc_name, str( + jsondiff.diff(old_cfg, start_running_config)))) + assert False, "_validate_svc: old config mismatch" if new_cfg != running_config: - print_diff(old_cfg, running_config) - assert False + debug_print("validate svc {}: new diff={}".format(svc_name, str( + jsondiff.diff(new_cfg, running_config)))) + assert False, "_validate_svc: running config mismatch" _validate_keys(keys) + # None provides a chance for test data to skip services_validated + # verification svcs = json_changes[json_change_index].get("services_validated", None) if svcs != None: - assert svc_name not in svcs + assert svc_name in svcs svcs.remove(svc_name) def acl_validate(old_cfg, new_cfg, keys): + debug_print("acl_validate called") _validate_svc("acl_validate", old_cfg, new_cfg, keys) def vlan_validate(old_cfg, new_cfg, keys): + debug_print("vlan_validate called") _validate_svc("vlan_validate", old_cfg, new_cfg, keys) class TestChangeApplier(unittest.TestCase): - @patch("generic_config_updater.gu_common.subprocess.run") @patch("generic_config_updater.change_applier.os.system") @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_os_sys): global read_data, running_config, json_changes, json_change_index + global start_running_config mock_os_sys.side_effect = os_system_cfggen mock_db.return_value = DB_HANDLE @@ -229,6 +227,7 @@ class TestChangeApplier(unittest.TestCase): json_changes = copy.deepcopy(read_data["json_changes"]) generic_config_updater.change_applier.UPDATER_CONF_FILE = CONF_FILE + generic_config_updater.change_applier.set_print_options(to_stdout=True) applier = generic_config_updater.change_applier.ChangeApplier() debug_print("invoked applier") @@ -240,10 +239,11 @@ class TestChangeApplier(unittest.TestCase): start_running_config = copy.deepcopy(running_config) debug_print("main: json_change_index={}".format(json_change_index)) + applier.apply(mock_obj()) - # All changes are consumed - for i in range(len(json_changes)): + debug_print(f"Testing json_change {json_change_index}") + debug_print("Checking: index={} update:{} remove:{} svcs:{}".format(i, json.dumps(json_changes[i]["update"])[0:20], json.dumps(json_changes[i]["remove"])[0:20], @@ -251,12 +251,16 @@ class TestChangeApplier(unittest.TestCase): assert not json_changes[i]["update"] assert not json_changes[i]["remove"] assert not json_changes[i].get("services_validated", []) + debug_print(f"----------------------------- DONE {i} ---------------------------------") + + debug_print("All changes applied & tested") # Test data is set up in such a way the multiple changes # finally brings it back to original config. # if read_data["running_data"] != running_config: - print_diff(read_data["running_data"], running_config) + debug_print("final config mismatch: {}".format(str( + jsondiff.diff(read_data["running_data"], running_config)))) assert read_data["running_data"] == running_config diff --git a/tests/generic_config_updater/files/change_applier_test.conf.json b/tests/generic_config_updater/files/change_applier_test.conf.json index 4b4a31aad5..9b0f552a43 100644 --- a/tests/generic_config_updater/files/change_applier_test.conf.json +++ b/tests/generic_config_updater/files/change_applier_test.conf.json @@ -3,22 +3,22 @@ "": { "services_to_validate": [ "system_health" ] }, - "PORT": { - "services_to_validate": [ "port_service" ] + "ACL_TABLE": { + "services_to_validate": [ "acl_service" ] }, "VLAN_INTERFACE": { - "services_to_validate": [ "port_service", "vlan_service" ] + "services_to_validate": [ "acl_service", "vlan_service" ] } }, "services": { "system_health": { - "validate_commands": [ "..tests.generic_config_updater.change_applier_test.system_health" ] - } - "ACL_TABLE": { - "validate_commands": [ "..tests.generic_config_updater.change_applier_test.acl_validate" ] + "validate_commands": [ "tests.generic_config_updater.change_applier_test.system_health" ] }, - "VLAN_INTERFACE": { - "validate_commands": [ "..tests.generic_config_updater.change_applier_test.vlan_validate" ] + "acl_service": { + "validate_commands": [ "tests.generic_config_updater.change_applier_test.acl_validate" ] + }, + "vlan_service": { + "validate_commands": [ "tests.generic_config_updater.change_applier_test.vlan_validate" ] } } } diff --git a/tests/generic_config_updater/files/change_applier_test.data.json b/tests/generic_config_updater/files/change_applier_test.data.json index bb36101920..d75541a5de 100644 --- a/tests/generic_config_updater/files/change_applier_test.data.json +++ b/tests/generic_config_updater/files/change_applier_test.data.json @@ -241,7 +241,8 @@ { "name": "change_0", "update": {}, - "remove": {} + "remove": {}, + "services_validated": ["system_health"] }, { "name": "change_1", @@ -253,7 +254,8 @@ "remove": { "BGP_NEIGHBOR": { "10.0.0.57": {} }, "BGP_PEER_RANGE": { "BGPSLBPassive": {} } - } + }, + "services_validated": [ "vlan_validate", "acl_validate" ] }, { "name": "change_2", From beb1ea7203e119660a4f56713d8131d7ecacba68 Mon Sep 17 00:00:00 2001 From: Renuka Manavalan Date: Wed, 20 Oct 2021 17:34:08 +0000 Subject: [PATCH 08/16] removed unused import --- generic_config_updater/change_applier.py | 1 - 1 file changed, 1 deletion(-) diff --git a/generic_config_updater/change_applier.py b/generic_config_updater/change_applier.py index 3e12a5bc98..efd0e90acd 100644 --- a/generic_config_updater/change_applier.py +++ b/generic_config_updater/change_applier.py @@ -3,7 +3,6 @@ import jsondiff import importlib import os -import syslog import tempfile from collections import defaultdict from swsscommon.swsscommon import ConfigDBConnector From fe25d2eedc0e63fd705f448a2d98f46c02944ce0 Mon Sep 17 00:00:00 2001 From: Renuka Manavalan Date: Wed, 20 Oct 2021 22:23:14 +0000 Subject: [PATCH 09/16] Added two service validators --- generic_config_updater/change_applier.py | 2 +- .../generic_updater_config.conf.json | 15 +++++++++++++++ generic_config_updater/services_validator.py | 17 +++++++++++++++++ 3 files changed, 33 insertions(+), 1 deletion(-) create mode 100644 generic_config_updater/services_validator.py diff --git a/generic_config_updater/change_applier.py b/generic_config_updater/change_applier.py index efd0e90acd..59fdcccf65 100644 --- a/generic_config_updater/change_applier.py +++ b/generic_config_updater/change_applier.py @@ -51,7 +51,7 @@ def __init__(self): global updater_data self.config_db = get_config_db() - if not updater_data: + if (not updater_data) and os.path.exists(UPDATER_CONF_FILE): with open(UPDATER_CONF_FILE, "r") as s: updater_data = json.load(s) diff --git a/generic_config_updater/generic_updater_config.conf.json b/generic_config_updater/generic_updater_config.conf.json index a0a60add04..f86844cca5 100644 --- a/generic_config_updater/generic_updater_config.conf.json +++ b/generic_config_updater/generic_updater_config.conf.json @@ -5,6 +5,15 @@ }, "PORT": { "services_to_validate": [ "port_service" ] + }, + "SYSLOG_SERVER":{ + "services_to_validate": [ "rsyslog" ] + }, + "DHCP_RELAY": { + "services_to_validate": [ "dhcp-relay" ] + }, + "DHCP_SERVER": { + "services_to_validate": [ "dhcp-relay" ] } }, "README": [ @@ -38,6 +47,12 @@ }, "port_service": { "validate_commands": [ ] + }, + "rsyslog": { + "validate_commands": [ "services_validator.ryslog_validator" ] + }, + "dhcp-relay": { + "validate_commands": [ "services_validator.dhcp_validator" ] } } } diff --git a/generic_config_updater/services_validator.py b/generic_config_updater/services_validator.py new file mode 100644 index 0000000000..525afe971b --- /dev/null +++ b/generic_config_updater/services_validator.py @@ -0,0 +1,17 @@ +import os +from .gu_common import genericUpdaterLogging + +logger = genericUpdaterLogging.get_logger(title="Service Validator") + +def _service_restart(svc_name): + os.system(f"systemctl restart {svc_name}") + logger.log_notice(f"Restarted {svc_name}") + + +def ryslog_validator(old_config, upd_config, keys): + _service_restart("rsyslog-config") + + +def dhcp_validator(old_config, upd_config, keys): + _service_restart("dhcp_relay") + From fec8f8f874fdc4a0992f7360870a607fffa058a2 Mon Sep 17 00:00:00 2001 From: Renuka Manavalan Date: Sun, 24 Oct 2021 23:50:22 +0000 Subject: [PATCH 10/16] move global to class; No logical code changes --- generic_config_updater/change_applier.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/generic_config_updater/change_applier.py b/generic_config_updater/change_applier.py index 59fdcccf65..b5ed4a637b 100644 --- a/generic_config_updater/change_applier.py +++ b/generic_config_updater/change_applier.py @@ -10,8 +10,6 @@ UPDATER_CONF_FILE = "/etc/sonic/generic_config_updater.conf" -updater_data = None - logger = genericUpdaterLogging.get_logger(title="Change Applier") print_to_console = False @@ -47,13 +45,14 @@ def set_config(config_db, tbl, key, data): class ChangeApplier: - def __init__(self): - global updater_data + updater_conf = None + + def __init__(self): self.config_db = get_config_db() - if (not updater_data) and os.path.exists(UPDATER_CONF_FILE): + if (not ChangeApplier.updater_conf) and os.path.exists(UPDATER_CONF_FILE): with open(UPDATER_CONF_FILE, "r") as s: - updater_data = json.load(s) + ChangeApplier.updater_conf = json.load(s) def _invoke_cmd(self, cmd, old_cfg, upd_cfg, keys): @@ -77,11 +76,11 @@ def _services_validate(self, old_cfg, upd_cfg, keys): # keys[""] = {} - tables = updater_data["tables"] + tables = ChangeApplier.updater_conf["tables"] for tbl in keys: lst_svcs.update(tables.get(tbl, {}).get("services_to_validate", [])) - services = updater_data["services"] + services = ChangeApplier.updater_conf["services"] for svc in lst_svcs: lst_cmds.update(services.get(svc, {}).get("validate_commands", [])) From 7d902665daae68f1bdad277d358684bdbd1f010e Mon Sep 17 00:00:00 2001 From: Renuka Manavalan Date: Wed, 27 Oct 2021 16:53:45 +0000 Subject: [PATCH 11/16] A fix in file path --- generic_config_updater/change_applier.py | 2 +- .../generic_updater_config.conf.json | 34 +++++++++---------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/generic_config_updater/change_applier.py b/generic_config_updater/change_applier.py index b5ed4a637b..79855c672f 100644 --- a/generic_config_updater/change_applier.py +++ b/generic_config_updater/change_applier.py @@ -9,7 +9,7 @@ from .gu_common import genericUpdaterLogging -UPDATER_CONF_FILE = "/etc/sonic/generic_config_updater.conf" +UPDATER_CONF_FILE = "/etc/sonic/generic_updater_config.conf.json" logger = genericUpdaterLogging.get_logger(title="Change Applier") print_to_console = False diff --git a/generic_config_updater/generic_updater_config.conf.json b/generic_config_updater/generic_updater_config.conf.json index f86844cca5..9d5dd62591 100644 --- a/generic_config_updater/generic_updater_config.conf.json +++ b/generic_config_updater/generic_updater_config.conf.json @@ -1,21 +1,4 @@ { - "tables": { - "": { - "services_to_validate": [ "system_health" ] - }, - "PORT": { - "services_to_validate": [ "port_service" ] - }, - "SYSLOG_SERVER":{ - "services_to_validate": [ "rsyslog" ] - }, - "DHCP_RELAY": { - "services_to_validate": [ "dhcp-relay" ] - }, - "DHCP_SERVER": { - "services_to_validate": [ "dhcp-relay" ] - } - }, "README": [ "Validate_commands provides, module & method name as ", " .", @@ -41,6 +24,23 @@ "Note: The commands may be called in any order", "" ], + "tables": { + "": { + "services_to_validate": [ "system_health" ] + }, + "PORT": { + "services_to_validate": [ "port_service" ] + }, + "SYSLOG_SERVER":{ + "services_to_validate": [ "rsyslog" ] + }, + "DHCP_RELAY": { + "services_to_validate": [ "dhcp-relay" ] + }, + "DHCP_SERVER": { + "services_to_validate": [ "dhcp-relay" ] + } + }, "services": { "system_health": { "validate_commands": [ ] From 6cd9dd8b70e25eb9ffc469b6497ca04d5fb513c6 Mon Sep 17 00:00:00 2001 From: Renuka Manavalan Date: Tue, 2 Nov 2021 16:54:34 +0000 Subject: [PATCH 12/16] 1) Copy generic_updater_config.conf.json as part of install 2) Read conf file from install dir 3) Drop empty keys & tables upon jsonpatch.JsonPatch.apply to be in sync with redis update 4) Prefix service_validator module path with "generic_updater" --- generic_config_updater/change_applier.py | 8 +++---- .../generic_updater_config.conf.json | 4 ++-- generic_config_updater/gu_common.py | 23 +++++++++++++++++++ setup.py | 1 + 4 files changed, 30 insertions(+), 6 deletions(-) diff --git a/generic_config_updater/change_applier.py b/generic_config_updater/change_applier.py index 79855c672f..8145f64c67 100644 --- a/generic_config_updater/change_applier.py +++ b/generic_config_updater/change_applier.py @@ -6,10 +6,10 @@ import tempfile from collections import defaultdict from swsscommon.swsscommon import ConfigDBConnector -from .gu_common import genericUpdaterLogging +from .gu_common import genericUpdaterLogging, prune_empty_entries - -UPDATER_CONF_FILE = "/etc/sonic/generic_updater_config.conf.json" +SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) +UPDATER_CONF_FILE = f"{SCRIPT_DIR}/generic_updater_config.conf.json" logger = genericUpdaterLogging.get_logger(title="Change Applier") print_to_console = False @@ -111,7 +111,7 @@ def _report_mismatch(self, run_data, upd_data): def apply(self, change): run_data = self._get_running_config() - upd_data = change.apply(copy.deepcopy(run_data)) + upd_data = prune_empty_entries(change.apply(copy.deepcopy(run_data))) upd_keys = defaultdict(dict) for tbl in sorted(set(run_data.keys()).union(set(upd_data.keys()))): diff --git a/generic_config_updater/generic_updater_config.conf.json b/generic_config_updater/generic_updater_config.conf.json index 9d5dd62591..647e62342e 100644 --- a/generic_config_updater/generic_updater_config.conf.json +++ b/generic_config_updater/generic_updater_config.conf.json @@ -49,10 +49,10 @@ "validate_commands": [ ] }, "rsyslog": { - "validate_commands": [ "services_validator.ryslog_validator" ] + "validate_commands": [ "generic_config_updater.services_validator.ryslog_validator" ] }, "dhcp-relay": { - "validate_commands": [ "services_validator.dhcp_validator" ] + "validate_commands": [ "generic_config_updater.services_validator.dhcp_validator" ] } } } diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index be1f6e5db7..ea989bb885 100644 --- a/generic_config_updater/gu_common.py +++ b/generic_config_updater/gu_common.py @@ -716,3 +716,26 @@ def get_logger(self, title): return TitledLogger(SYSLOG_IDENTIFIER, title, self._verbose) genericUpdaterLogging = GenericUpdaterLogging() + +def prune_empty_entries(data): + # For JSON Patch empty entries are valid + # With redis, when last attribute/field is removed, the key gets + # removed. + # + # Hence where required, prune keys with no field and tables with no keys. + # + # Take a deep copy as data will be modified in loop + # Use copy for walking and input data for update + # + tmp_data = copy.deepcopy(data) + for tbl, tmp_tbl_data in tmp_data.items(): + tbl_data = data[tbl] + for key, tmp_key_data in tmp_tbl_data.items(): + if not tmp_key_data: + # Pop it from input data + tbl_data.pop(key) + if not tbl_data: + data.pop(tbl) + return data + + diff --git a/setup.py b/setup.py index c28af558ab..63b6c86919 100644 --- a/setup.py +++ b/setup.py @@ -58,6 +58,7 @@ 'watchdogutil', ], package_data={ + 'generic_config_updater': ['generic_updater_config.conf.json'], 'show': ['aliases.ini'], 'sonic_installer': ['aliases.ini'], 'tests': ['acl_input/*', From 30feec2555ca2611565fd0dea18046af397dfdb9 Mon Sep 17 00:00:00 2001 From: Renuka Manavalan Date: Wed, 3 Nov 2021 21:49:35 +0000 Subject: [PATCH 13/16] Prune only empty tables --- generic_config_updater/change_applier.py | 17 +++++++++++++++-- generic_config_updater/gu_common.py | 23 ----------------------- 2 files changed, 15 insertions(+), 25 deletions(-) diff --git a/generic_config_updater/change_applier.py b/generic_config_updater/change_applier.py index 8145f64c67..f01c4edc70 100644 --- a/generic_config_updater/change_applier.py +++ b/generic_config_updater/change_applier.py @@ -6,7 +6,7 @@ import tempfile from collections import defaultdict from swsscommon.swsscommon import ConfigDBConnector -from .gu_common import genericUpdaterLogging, prune_empty_entries +from .gu_common import genericUpdaterLogging SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) UPDATER_CONF_FILE = f"{SCRIPT_DIR}/generic_updater_config.conf.json" @@ -44,6 +44,19 @@ def set_config(config_db, tbl, key, data): config_db.set_entry(tbl, key, data) +def prune_empty_table(data): + # For JSON Patch empty entries are valid + # With redis, when last key is removed, the table gets removed too. + # + # Hence where required, prune tables with no keys. + # + tables = list(data.keys()) + for tbl in tables: + if not data[tbl]: + data.pop(tbl) + return data + + class ChangeApplier: updater_conf = None @@ -111,7 +124,7 @@ def _report_mismatch(self, run_data, upd_data): def apply(self, change): run_data = self._get_running_config() - upd_data = prune_empty_entries(change.apply(copy.deepcopy(run_data))) + upd_data = prune_empty_table(change.apply(copy.deepcopy(run_data))) upd_keys = defaultdict(dict) for tbl in sorted(set(run_data.keys()).union(set(upd_data.keys()))): diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index ea989bb885..be1f6e5db7 100644 --- a/generic_config_updater/gu_common.py +++ b/generic_config_updater/gu_common.py @@ -716,26 +716,3 @@ def get_logger(self, title): return TitledLogger(SYSLOG_IDENTIFIER, title, self._verbose) genericUpdaterLogging = GenericUpdaterLogging() - -def prune_empty_entries(data): - # For JSON Patch empty entries are valid - # With redis, when last attribute/field is removed, the key gets - # removed. - # - # Hence where required, prune keys with no field and tables with no keys. - # - # Take a deep copy as data will be modified in loop - # Use copy for walking and input data for update - # - tmp_data = copy.deepcopy(data) - for tbl, tmp_tbl_data in tmp_data.items(): - tbl_data = data[tbl] - for key, tmp_key_data in tmp_tbl_data.items(): - if not tmp_key_data: - # Pop it from input data - tbl_data.pop(key) - if not tbl_data: - data.pop(tbl) - return data - - From 8cd17fd54fc497a74b343d3943d36c9e380c5309 Mon Sep 17 00:00:00 2001 From: Renuka Manavalan Date: Wed, 10 Nov 2021 00:38:45 +0000 Subject: [PATCH 14/16] file rename --- generic_config_updater/change_applier.py | 2 +- ...pdater_config.conf.json => generic_config_updater.conf.json} | 0 setup.py | 2 +- 3 files changed, 2 insertions(+), 2 deletions(-) rename generic_config_updater/{generic_updater_config.conf.json => generic_config_updater.conf.json} (100%) diff --git a/generic_config_updater/change_applier.py b/generic_config_updater/change_applier.py index f01c4edc70..e7abe47c17 100644 --- a/generic_config_updater/change_applier.py +++ b/generic_config_updater/change_applier.py @@ -9,7 +9,7 @@ from .gu_common import genericUpdaterLogging SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) -UPDATER_CONF_FILE = f"{SCRIPT_DIR}/generic_updater_config.conf.json" +UPDATER_CONF_FILE = f"{SCRIPT_DIR}/generic_config_updater.conf.json" logger = genericUpdaterLogging.get_logger(title="Change Applier") print_to_console = False diff --git a/generic_config_updater/generic_updater_config.conf.json b/generic_config_updater/generic_config_updater.conf.json similarity index 100% rename from generic_config_updater/generic_updater_config.conf.json rename to generic_config_updater/generic_config_updater.conf.json diff --git a/setup.py b/setup.py index 1dba5e6bf7..95a7043359 100644 --- a/setup.py +++ b/setup.py @@ -58,7 +58,7 @@ 'watchdogutil', ], package_data={ - 'generic_config_updater': ['generic_updater_config.conf.json'], + 'generic_config_updater': ['generic_config_updater.conf.json'], 'show': ['aliases.ini'], 'sonic_installer': ['aliases.ini'], 'tests': ['acl_input/*', From 9abb300a413c698d10cd869067bf2baf1bb4a075 Mon Sep 17 00:00:00 2001 From: Renuka Manavalan Date: Tue, 16 Nov 2021 00:15:58 +0000 Subject: [PATCH 15/16] 1) Drop print to stdout from change_applier 2) Added vlan validator 3) Added test code for vlan validator --- generic_config_updater/change_applier.py | 34 +++++---- .../generic_config_updater.conf.json | 6 ++ generic_config_updater/services_validator.py | 34 +++++++-- .../change_applier_test.py | 4 +- .../service_validator_test.py | 71 +++++++++++++++++++ 5 files changed, 126 insertions(+), 23 deletions(-) create mode 100644 tests/generic_config_updater/service_validator_test.py diff --git a/generic_config_updater/change_applier.py b/generic_config_updater/change_applier.py index e7abe47c17..bff1e1d66e 100644 --- a/generic_config_updater/change_applier.py +++ b/generic_config_updater/change_applier.py @@ -13,25 +13,23 @@ logger = genericUpdaterLogging.get_logger(title="Change Applier") print_to_console = False -print_to_stdout = False -def set_print_options(to_console=False, to_stdout=False): - global print_to_console, print_to_stdout +def set_verbose(verbose=False): + global print_to_console, logger - print_to_console = to_console - print_to_stdout = to_stdout + print_to_console = verbose + if verbose: + logger.set_min_log_priority_debug() + else: + logger.set_min_log_priority_notice() -def log_debug(m): - logger.log_debug(m, print_to_console) - if print_to_stdout: - print(m) +def _log_debug(m): + logger.log(logger.LOG_PRIORITY_DEBUG, m, print_to_console) -def log_error(m): - logger.log_error(m, print_to_console) - if print_to_stdout: - print(m) +def _log_error(m): + logger.log(logger.LOG_PRIORITY_ERROR, m, print_to_console) def get_config_db(): @@ -100,9 +98,9 @@ def _services_validate(self, old_cfg, upd_cfg, keys): for cmd in lst_cmds: ret = self._invoke_cmd(cmd, old_cfg, upd_cfg, keys) if ret: - log_error("service invoked: {} failed with ret={}".format(cmd, ret)) + _log_error("service invoked: {} failed with ret={}".format(cmd, ret)) return ret - log_debug("service invoked: {}".format(cmd)) + _log_debug("service invoked: {}".format(cmd)) return 0 @@ -114,11 +112,11 @@ def _upd_data(self, tbl, run_tbl, upd_tbl, upd_keys): if run_data != upd_data: set_config(self.config_db, tbl, key, upd_data) upd_keys[tbl][key] = {} - log_debug("Patch affected tbl={} key={}".format(tbl, key)) + _log_debug("Patch affected tbl={} key={}".format(tbl, key)) def _report_mismatch(self, run_data, upd_data): - log_error("run_data vs expected_data: {}".format( + _log_error("run_data vs expected_data: {}".format( str(jsondiff.diff(run_data, upd_data))[0:40])) @@ -138,7 +136,7 @@ def apply(self, change): self._report_mismatch(run_data, upd_data) ret = -1 if ret: - log_error("Failed to apply Json change") + _log_error("Failed to apply Json change") return ret diff --git a/generic_config_updater/generic_config_updater.conf.json b/generic_config_updater/generic_config_updater.conf.json index 647e62342e..c2c00d344a 100644 --- a/generic_config_updater/generic_config_updater.conf.json +++ b/generic_config_updater/generic_config_updater.conf.json @@ -39,6 +39,9 @@ }, "DHCP_SERVER": { "services_to_validate": [ "dhcp-relay" ] + }, + "VLAN": { + "services_to_validate": [ "vlan-service" ] } }, "services": { @@ -53,6 +56,9 @@ }, "dhcp-relay": { "validate_commands": [ "generic_config_updater.services_validator.dhcp_validator" ] + }, + "vlan-service": { + "validate_commands": [ "generic_config_updater.services_validator.vlan_validator" ] } } } diff --git a/generic_config_updater/services_validator.py b/generic_config_updater/services_validator.py index 525afe971b..6ad6310ec1 100644 --- a/generic_config_updater/services_validator.py +++ b/generic_config_updater/services_validator.py @@ -3,15 +3,41 @@ logger = genericUpdaterLogging.get_logger(title="Service Validator") +print_to_console = False + +def set_verbose(verbose=False): + global print_to_console, logger + + print_to_console = verbose + if verbose: + logger.set_min_log_priority_debug() + else: + logger.set_min_log_priority_notice() + + def _service_restart(svc_name): - os.system(f"systemctl restart {svc_name}") - logger.log_notice(f"Restarted {svc_name}") + rc = os.system(f"systemctl restart {svc_name}") + logger.log(logger.LOG_PRIORITY_NOTICE, + f"Restarted {svc_name}", print_to_console) + return rc == 0 def ryslog_validator(old_config, upd_config, keys): - _service_restart("rsyslog-config") + return _service_restart("rsyslog-config") def dhcp_validator(old_config, upd_config, keys): - _service_restart("dhcp_relay") + return _service_restart("dhcp_relay") + + +def vlan_validator(old_config, upd_config, keys): + old_vlan = old_config.get("VLAN", {}) + upd_vlan = upd_config.get("VLAN", {}) + + for key in set(old_vlan.keys()).union(set(upd_vlan.keys())): + if (old_vlan.get(key, {}).get("dhcp_servers", []) != + upd_vlan.get(key, {}).get("dhcp_servers", [])): + return _service_restart("dhcp_relay") + # No update to DHCP servers. + return True diff --git a/tests/generic_config_updater/change_applier_test.py b/tests/generic_config_updater/change_applier_test.py index 5d5b0908dc..b734485ffd 100644 --- a/tests/generic_config_updater/change_applier_test.py +++ b/tests/generic_config_updater/change_applier_test.py @@ -7,6 +7,7 @@ from unittest.mock import patch import generic_config_updater.change_applier +import generic_config_updater.services_validator import generic_config_updater.gu_common SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) @@ -227,7 +228,8 @@ def test_change_apply(self, mock_set, mock_db, mock_os_sys): json_changes = copy.deepcopy(read_data["json_changes"]) generic_config_updater.change_applier.UPDATER_CONF_FILE = CONF_FILE - generic_config_updater.change_applier.set_print_options(to_stdout=True) + generic_config_updater.change_applier.set_verbose(True) + generic_config_updater.services_validator.set_verbose(True) applier = generic_config_updater.change_applier.ChangeApplier() debug_print("invoked applier") diff --git a/tests/generic_config_updater/service_validator_test.py b/tests/generic_config_updater/service_validator_test.py new file mode 100644 index 0000000000..749afaab2f --- /dev/null +++ b/tests/generic_config_updater/service_validator_test.py @@ -0,0 +1,71 @@ +import copy +import json +import jsondiff +import os +import unittest +from collections import defaultdict +from unittest.mock import patch + +from generic_config_updater.services_validator import vlan_validator +import generic_config_updater.gu_common + + +# Mimics os.system call +# +os_system_expected_cmd = "" +msg = "" + +def os_system_cfggen(cmd): + assert cmd == os_system_expected_cmd, msg + return 0 + + +test_data = [ + { "old": {}, "upd": {}, "cmd": "" }, + { + "old": { "VLAN": { "XXX": { "dhcp_servers": [ "10.10.10.10" ] } } }, + "upd": { "VLAN": { "XXX": { "dhcp_servers": [ "10.10.10.10" ] } } }, + "cmd": "" + }, + { + "old": { "VLAN": { "XXX": { "dhcp_servers": [ "10.10.10.10" ] } } }, + "upd": { "VLAN": { "XXX": { "dhcp_servers": [ "10.10.10.11" ] } } }, + "cmd": "systemctl restart dhcp_relay" + }, + { + "old": { "VLAN": { "XXX": { "dhcp_servers": [ "10.10.10.10" ] } } }, + "upd": { "VLAN": { + "XXX": { "dhcp_servers": [ "10.10.10.10" ] }, + "YYY": { "dhcp_servers": [ ] } } }, + "cmd": "" + }, + { + "old": { "VLAN": { "XXX": { "dhcp_servers": [ "10.10.10.10" ] } } }, + "upd": { "VLAN": { + "XXX": { "dhcp_servers": [ "10.10.10.10" ] }, + "YYY": { "dhcp_servers": [ "10.12.12.1" ] } } }, + "cmd": "systemctl restart dhcp_relay" + }, + { + "old": { "VLAN": { "XXX": { "dhcp_servers": [ "10.10.10.10" ] } } }, + "upd": {}, + "cmd": "systemctl restart dhcp_relay" + } + ] + +class TestServiceValidator(unittest.TestCase): + + @patch("generic_config_updater.change_applier.os.system") + def test_change_apply(self, mock_os_sys): + global os_system_expected_cmd + + mock_os_sys.side_effect = os_system_cfggen + + i = 0 + for entry in test_data: + os_system_expected_cmd = entry["cmd"] + msg = "case failed: {}".format(str(entry)) + + vlan_validator(entry["old"], entry["upd"], None) + + From a41052b7c6de266d6779989c6e5b742a519d9759 Mon Sep 17 00:00:00 2001 From: Renuka Manavalan Date: Mon, 22 Nov 2021 18:47:06 +0000 Subject: [PATCH 16/16] No logical code changes; Name changes only, per review comments --- generic_config_updater/change_applier.py | 14 +++++++------- .../generic_config_updater.conf.json | 2 +- generic_config_updater/services_validator.py | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/generic_config_updater/change_applier.py b/generic_config_updater/change_applier.py index bff1e1d66e..23b9383814 100644 --- a/generic_config_updater/change_applier.py +++ b/generic_config_updater/change_applier.py @@ -24,11 +24,11 @@ def set_verbose(verbose=False): logger.set_min_log_priority_notice() -def _log_debug(m): +def log_debug(m): logger.log(logger.LOG_PRIORITY_DEBUG, m, print_to_console) -def _log_error(m): +def log_error(m): logger.log(logger.LOG_PRIORITY_ERROR, m, print_to_console) @@ -98,9 +98,9 @@ def _services_validate(self, old_cfg, upd_cfg, keys): for cmd in lst_cmds: ret = self._invoke_cmd(cmd, old_cfg, upd_cfg, keys) if ret: - _log_error("service invoked: {} failed with ret={}".format(cmd, ret)) + log_error("service invoked: {} failed with ret={}".format(cmd, ret)) return ret - _log_debug("service invoked: {}".format(cmd)) + log_debug("service invoked: {}".format(cmd)) return 0 @@ -112,11 +112,11 @@ def _upd_data(self, tbl, run_tbl, upd_tbl, upd_keys): if run_data != upd_data: set_config(self.config_db, tbl, key, upd_data) upd_keys[tbl][key] = {} - _log_debug("Patch affected tbl={} key={}".format(tbl, key)) + log_debug("Patch affected tbl={} key={}".format(tbl, key)) def _report_mismatch(self, run_data, upd_data): - _log_error("run_data vs expected_data: {}".format( + log_error("run_data vs expected_data: {}".format( str(jsondiff.diff(run_data, upd_data))[0:40])) @@ -136,7 +136,7 @@ def apply(self, change): self._report_mismatch(run_data, upd_data) ret = -1 if ret: - _log_error("Failed to apply Json change") + log_error("Failed to apply Json change") return ret diff --git a/generic_config_updater/generic_config_updater.conf.json b/generic_config_updater/generic_config_updater.conf.json index c2c00d344a..0951e31be0 100644 --- a/generic_config_updater/generic_config_updater.conf.json +++ b/generic_config_updater/generic_config_updater.conf.json @@ -52,7 +52,7 @@ "validate_commands": [ ] }, "rsyslog": { - "validate_commands": [ "generic_config_updater.services_validator.ryslog_validator" ] + "validate_commands": [ "generic_config_updater.services_validator.rsyslog_validator" ] }, "dhcp-relay": { "validate_commands": [ "generic_config_updater.services_validator.dhcp_validator" ] diff --git a/generic_config_updater/services_validator.py b/generic_config_updater/services_validator.py index 6ad6310ec1..89323df004 100644 --- a/generic_config_updater/services_validator.py +++ b/generic_config_updater/services_validator.py @@ -22,7 +22,7 @@ def _service_restart(svc_name): return rc == 0 -def ryslog_validator(old_config, upd_config, keys): +def rsyslog_validator(old_config, upd_config, keys): return _service_restart("rsyslog-config")