diff --git a/src/sonic-bgpcfgd/bgpcfgd/config.py b/src/sonic-bgpcfgd/bgpcfgd/config.py index c642842a2c16..bba45133c9df 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/config.py +++ b/src/sonic-bgpcfgd/bgpcfgd/config.py @@ -1,31 +1,24 @@ -import os -import tempfile - -from .vars import g_debug -from .log import log_crit, log_err -from .utils import run_command - - class ConfigMgr(object): """ The class represents frr configuration """ - def __init__(self): + def __init__(self, frr): + self.frr = frr self.current_config = None self.current_config_raw = None + self.changes = "" + self.peer_groups_to_restart = [] def reset(self): """ Reset stored config """ self.current_config = None self.current_config_raw = None + self.changes = "" + self.peer_groups_to_restart = [] def update(self): """ Read current config from FRR """ self.current_config = None self.current_config_raw = None - ret_code, out, err = run_command(["vtysh", "-c", "show running-config"]) - if ret_code != 0: - # FIXME: should we throw exception here? - log_crit("can't update running config: rc=%d out='%s' err='%s'" % (ret_code, out, err)) - return + out = self.frr.get_config() text = [] for line in out.split('\n'): if line.lstrip().startswith('!'): @@ -33,40 +26,41 @@ def update(self): text.append(line) text += [" "] # Add empty line to have something to work on, if there is no text self.current_config_raw = text - self.current_config = self.to_canonical(out) # FIXME: use test as an input + self.current_config = self.to_canonical(out) # FIXME: use text as an input def push_list(self, cmdlist): - return self.push("\n".join(cmdlist)) + """ + Prepare new changes for FRR. The changes should be committed by self.commit() + :param cmdlist: configuration change for FRR. Type: List of Strings + """ + self.changes += "\n".join(cmdlist) + "\n" def push(self, cmd): """ - Push new changes to FRR + Prepare new changes for FRR. The changes should be committed by self.commit() :param cmd: configuration change for FRR. Type: String - :return: True if change was applied successfully, False otherwise """ - return self.write(cmd) + self.changes += cmd + "\n" + return True + + def restart_peer_groups(self, peer_groups): + """ + Schedule peer_groups for restart on commit + :param peer_groups: List of peer_groups + """ + self.peer_groups_to_restart.extend(peer_groups) - def write(self, cmd): + def commit(self): """ Write configuration change to FRR. - :param cmd: new configuration to write into FRR. Type: String :return: True if change was applied successfully, False otherwise """ - fd, tmp_filename = tempfile.mkstemp(dir='/tmp') - os.close(fd) - with open(tmp_filename, 'w') as fp: - fp.write("%s\n" % cmd) - command = ["vtysh", "-f", tmp_filename] - ret_code, out, err = run_command(command) - if not g_debug: - os.remove(tmp_filename) - if ret_code != 0: - err_tuple = str(cmd), ret_code, out, err - log_err("ConfigMgr::push(): can't push configuration '%s', rc='%d', stdout='%s', stderr='%s'" % err_tuple) - if ret_code == 0: - self.current_config = None # invalidate config - self.current_config_raw = None - return ret_code == 0 + if self.changes.strip() == "": + return True + rc_write = self.frr.write(self.changes) + rc_restart = self.frr.restart_peer_groups(self.peer_groups_to_restart) + self.reset() + return rc_write and rc_restart def get_text(self): return self.current_config_raw @@ -89,7 +83,6 @@ def to_canonical(raw_config): for line in lines: n_spaces = ConfigMgr.count_spaces(line) s_line = line.strip() -# assert(n_spaces == cur_offset or (n_spaces + 1) == cur_offset or (n_spaces - 1) == cur_offset) if n_spaces == cur_offset: cur_path[-1] = s_line elif n_spaces > cur_offset: diff --git a/src/sonic-bgpcfgd/bgpcfgd/frr.py b/src/sonic-bgpcfgd/bgpcfgd/frr.py new file mode 100644 index 000000000000..6b88e5ee47f5 --- /dev/null +++ b/src/sonic-bgpcfgd/bgpcfgd/frr.py @@ -0,0 +1,70 @@ +import os +import datetime +import time +import tempfile + +from bgpcfgd.log import log_err, log_info, log_warn, log_crit +from .vars import g_debug +from .utils import run_command + + +class FRR(object): + """Proxy object with FRR""" + def __init__(self, daemons): + self.daemons = daemons + + def wait_for_daemons(self, seconds): + """ + Wait until FRR daemons are ready for requests + :param seconds: number of seconds to wait, until raise an error + """ + stop_time = datetime.datetime.now() + datetime.timedelta(seconds=seconds) + log_info("Start waiting for FRR daemons: %s" % str(datetime.datetime.now())) + while datetime.datetime.now() < stop_time: + ret_code, out, err = run_command(["vtysh", "-c", "show daemons"], hide_errors=True) + if ret_code == 0 and all(daemon in out for daemon in self.daemons): + log_info("All required daemons have connected to vtysh: %s" % str(datetime.datetime.now())) + return + else: + log_warn("Can't read daemon status from FRR: %s" % str(err)) + time.sleep(0.1) # sleep 100 ms + raise RuntimeError("FRR daemons hasn't been started in %d seconds" % seconds) + + @staticmethod + def get_config(): + ret_code, out, err = run_command(["vtysh", "-c", "show running-config"]) + if ret_code != 0: + log_crit("can't update running config: rc=%d out='%s' err='%s'" % (ret_code, out, err)) + return "" + return out + + @staticmethod + def write(config_text): + fd, tmp_filename = tempfile.mkstemp(dir='/tmp') + os.close(fd) + with open(tmp_filename, 'w') as fp: + fp.write("%s\n" % config_text) + command = ["vtysh", "-f", tmp_filename] + ret_code, out, err = run_command(command) + if ret_code != 0: + err_tuple = tmp_filename, ret_code, out, err + log_err("ConfigMgr::commit(): can't push configuration from file='%s', rc='%d', stdout='%s', stderr='%s'" % err_tuple) + else: + if not g_debug: + os.remove(tmp_filename) + return ret_code == 0 + + @staticmethod + def restart_peer_groups(peer_groups): + """ Restart peer-groups which support BBR + :param peer_groups: List of peer_groups to restart + :return: True if restart of all peer-groups was successful, False otherwise + """ + res = True + for peer_group in sorted(peer_groups): + rc, out, err = run_command(["vtysh", "-c", "clear bgp peer-group %s soft in" % peer_group]) + if rc != 0: + log_value = peer_group, rc, out, err + log_crit("Can't restart bgp peer-group '%s'. rc='%d', out='%s', err='%s'" % log_value) + res = res and (rc == 0) + return res diff --git a/src/sonic-bgpcfgd/bgpcfgd/main.py b/src/sonic-bgpcfgd/bgpcfgd/main.py index 9330330a1b3e..360b54dc11aa 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/main.py +++ b/src/sonic-bgpcfgd/bgpcfgd/main.py @@ -17,17 +17,19 @@ from .managers_setsrc import ZebraSetSrc from .runner import Runner, signal_handler from .template import TemplateFabric -from .utils import wait_for_daemons, read_constants +from .utils import read_constants +from .frr import FRR from .vars import g_debug def do_work(): """ Main function """ - wait_for_daemons(["bgpd", "zebra", "staticd"], seconds=20) + frr = FRR(["bgpd", "zebra", "staticd"]) + frr.wait_for_daemons(seconds=20) # common_objs = { 'directory': Directory(), - 'cfg_mgr': ConfigMgr(), + 'cfg_mgr': ConfigMgr(frr), 'tf': TemplateFabric(), 'constants': read_constants(), } @@ -52,7 +54,7 @@ def do_work(): # BBR Manager BBRMgr(common_objs, "CONFIG_DB", "BGP_BBR"), ] - runner = Runner() + runner = Runner(common_objs['cfg_mgr']) for mgr in managers: runner.add_manager(mgr) runner.run() diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_allow_list.py b/src/sonic-bgpcfgd/bgpcfgd/managers_allow_list.py index 4d0df8a7622e..2ca89630786a 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_allow_list.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_allow_list.py @@ -6,7 +6,7 @@ from .log import log_debug, log_info, log_err, log_warn from .template import TemplateFabric from .manager import Manager -from .utils import run_command + class BGPAllowListMgr(Manager): """ This class initialize "AllowList" settings """ @@ -147,9 +147,10 @@ def __update_policy(self, deployment_id, community_value, prefixes_v4, prefixes_ cmds += self.__update_allow_route_map_entry(self.V4, names['pl_v4'], names['community'], names['rm_v4']) cmds += self.__update_allow_route_map_entry(self.V6, names['pl_v6'], names['community'], names['rm_v6']) if cmds: - rc = self.cfg_mgr.push_list(cmds) - rc = rc and self.__restart_peers(deployment_id) - log_debug("BGPAllowListMgr::__update_policy. The peers were updated: rc=%s" % rc) + self.cfg_mgr.push_list(cmds) + peer_groups = self.__find_peer_group_by_deployment_id(deployment_id) + self.cfg_mgr.restart_peers(peer_groups) + log_debug("BGPAllowListMgr::__update_policy. The peers configuration scheduled for updates") else: log_debug("BGPAllowListMgr::__update_policy. Nothing to update") log_info("BGPAllowListMgr::Done") @@ -176,9 +177,10 @@ def __remove_policy(self, deployment_id, community_value): cmds += self.__remove_prefix_list(self.V6, names['pl_v6']) cmds += self.__remove_community(names['community']) if cmds: - rc = self.cfg_mgr.push_list(cmds) - rc = rc and self.__restart_peers(deployment_id) - log_debug("BGPAllowListMgr::__remove_policy. 'Allow list' policy was removed. rc:%s" % rc) + self.cfg_mgr.push_list(cmds) + peer_groups = self.__find_peer_group_by_deployment_id(deployment_id) + self.cfg_mgr.restart_peers(peer_groups) + log_debug("BGPAllowListMgr::__remove_policy. 'Allow list' policy was scheduled for removal") else: log_debug("BGPAllowListMgr::__remove_policy. Nothing to remove") log_info('BGPAllowListMgr::Done') @@ -531,7 +533,8 @@ def __get_route_map_calls(self, rms): inside_name = result.group(1) return rm_2_call - def __get_peer_group_to_restart(self, deployment_id, pg_2_rm, rm_2_call): + @staticmethod + def __get_peer_group_to_restart(deployment_id, pg_2_rm, rm_2_call): """ Get peer_groups which are assigned to deployment_id :deployment_id: deployment_id number @@ -560,23 +563,6 @@ def __find_peer_group_by_deployment_id(self, deployment_id): ret = self.__get_peer_group_to_restart(deployment_id, pg_2_rm, rm_2_call) return list(ret) - def __restart_peers(self, deployment_id): - """ - Restart peer-groups with requested deployment_id - :param deployment_id: deployment_id number - """ - log_info("BGPAllowListMgr::Restart peers with deployment_id=%d" % deployment_id) - peer_groups = self.__find_peer_group_by_deployment_id(deployment_id) - rv = True - if peer_groups: - for peer_group in peer_groups: - no_error, _, _ = run_command(["vtysh", "-c", "clear bgp peer-group %s soft in" % peer_group]) - rv = no_error == 0 and rv - else: - no_error, _, _ = run_command(["vtysh", "-c", "clear bgp * soft in"]) - rv = no_error == 0 - return rv - def __get_enabled(self): """ Load enable/disabled property from constants diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_bbr.py b/src/sonic-bgpcfgd/bgpcfgd/managers_bbr.py index 7adfd203f455..27f3646ac421 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_bbr.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_bbr.py @@ -2,9 +2,8 @@ from swsscommon import swsscommon -from .log import log_err, log_info, log_crit +from .log import log_err, log_info from .manager import Manager -from .utils import run_command class BBRMgr(Manager): @@ -34,12 +33,10 @@ def set_handler(self, key, data): return True if not self.__set_validation(key, data): return True - cmds = self.__set_prepare_config(data['status']) - rv = self.cfg_mgr.push_list(cmds) - if not rv: - log_crit("BBRMgr::can't apply configuration") - return True - self.__restart_peers() + cmds, peer_groups_to_restart = self.__set_prepare_config(data['status']) + self.cfg_mgr.push_list(cmds) + self.cfg_mgr.restart_peer_groups(peer_groups_to_restart) + log_info("BBRMgr::Scheduled BBR update") return True def del_handler(self, key): @@ -112,12 +109,14 @@ def __set_prepare_config(self, status): available_peer_groups = self.__get_available_peer_groups() cmds = ["router bgp %s" % bgp_asn] prefix_of_commands = "" if status == "enabled" else "no " + peer_groups_to_restart = set() for af in ["ipv4", "ipv6"]: cmds.append(" address-family %s" % af) for pg_name in sorted(self.bbr_enabled_pgs.keys()): if pg_name in available_peer_groups and af in self.bbr_enabled_pgs[pg_name]: cmds.append(" %sneighbor %s allowas-in 1" % (prefix_of_commands, pg_name)) - return cmds + peer_groups_to_restart.add(pg_name) + return cmds, list(peer_groups_to_restart) def __get_available_peer_groups(self): """ @@ -132,11 +131,3 @@ def __get_available_peer_groups(self): if m: res.add(m.group(1)) return res - - def __restart_peers(self): - """ Restart peer-groups which support BBR """ - for peer_group in sorted(self.bbr_enabled_pgs.keys()): - rc, out, err = run_command(["vtysh", "-c", "clear bgp peer-group %s soft in" % peer_group]) - if rc != 0: - log_value = peer_group, rc, out, err - log_crit("BBRMgr::Can't restart bgp peer-group '%s'. rc='%d', out='%s', err='%s'" % log_value) diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py b/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py index b1545dae9288..81d78689c121 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py @@ -45,8 +45,8 @@ def update_policy(self, name, **kwargs): except jinja2.TemplateError as e: log_err("Can't render policy template name: '%s': %s" % (name, str(e))) return False - - return self.update_entity(policy, "Routing policy for peer '%s'" % name) + self.update_entity(policy, "Routing policy for peer '%s'" % name) + return True def update_pg(self, name, **kwargs): """ @@ -64,8 +64,8 @@ def update_pg(self, name, **kwargs): cmd = ('router bgp %s\n' % kwargs['bgp_asn']) + pg else: cmd = ('router bgp %s vrf %s\n' % (kwargs['bgp_asn'], kwargs['vrf'])) + pg - - return self.update_entity(cmd, "Peer-group for peer '%s'" % name) + self.update_entity(cmd, "Peer-group for peer '%s'" % name) + return True def update_entity(self, cmd, txt): """ @@ -74,12 +74,9 @@ def update_entity(self, cmd, txt): :param txt: text for the syslog output :return: """ - ret_code = self.cfg_mgr.push(cmd) - if ret_code: - log_info("%s was updated" % txt) - else: - log_err("Can't update %s" % txt) - return ret_code + self.cfg_mgr.push(cmd) + log_info("%s has been scheduled to be updated" % txt) + return True class BGPPeerMgrBase(Manager): @@ -212,13 +209,10 @@ def add_peer(self, vrf, nbr, data): log_err("%s: %s" % (msg, str(e))) return True if cmd is not None: - ret_code = self.apply_op(cmd, vrf) + self.apply_op(cmd, vrf) key = (vrf, nbr) - if ret_code: - self.peers.add(key) - log_info("Peer '(%s|%s)' added with attributes '%s'" % print_data) - else: - log_err("Peer '(%s|%s)' wasn't added." % (vrf, nbr)) + self.peers.add(key) + log_info("Peer '(%s|%s)' has been scheduled to be added with attributes '%s'" % print_data) return True @@ -300,7 +294,8 @@ def apply_op(self, cmd, vrf): cmd = ('router bgp %s\n' % bgp_asn) + cmd else: cmd = ('router bgp %s vrf %s\n' % (bgp_asn, vrf)) + cmd - return self.cfg_mgr.push(cmd) + self.cfg_mgr.push(cmd) + return True def get_lo0_ipv4(self): """ diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_setsrc.py b/src/sonic-bgpcfgd/bgpcfgd/managers_setsrc.py index 19b589cf0446..d1de585b0520 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_setsrc.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_setsrc.py @@ -57,10 +57,8 @@ def set_handler(self, key, data): except jinja2.TemplateError as e: log_err("Error while rendering 'set src' template: %s" % str(e)) return True - if self.cfg_mgr.push(txt): - log_info("The 'set src' configuration with Loopback0 ip '%s' was pushed" % ip_addr) - else: - log_err("The 'set src' configuration with Loopback0 ip '%s' wasn't pushed" % ip_addr) + self.cfg_mgr.push(txt) + log_info("The 'set src' configuration with Loopback0 ip '%s' has been scheduled to be added" % ip_addr) return True def del_handler(self, key): diff --git a/src/sonic-bgpcfgd/bgpcfgd/runner.py b/src/sonic-bgpcfgd/bgpcfgd/runner.py index 07799af5e7e4..4c160e5967cf 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/runner.py +++ b/src/sonic-bgpcfgd/bgpcfgd/runner.py @@ -1,7 +1,7 @@ from collections import defaultdict from swsscommon import swsscommon -from .log import log_debug +from .log import log_debug, log_crit g_run = True @@ -20,8 +20,9 @@ class Runner(object): """ SELECT_TIMEOUT = 1000 - def __init__(self): + def __init__(self, cfg_manager): """ Constructor """ + self.cfg_manager = cfg_manager self.db_connectors = {} self.selector = swsscommon.Select() self.callbacks = defaultdict(lambda: defaultdict(list)) # db -> table -> handlers[] @@ -57,9 +58,13 @@ def run(self): raise Exception("Received error from select") for subscriber in self.subscribers: - key, op, fvs = subscriber.pop() - if not key: - continue - log_debug("Received message : '%s'" % str((key, op, fvs))) - for callback in self.callbacks[subscriber.getDbConnector().getDbId()][subscriber.getTableName()]: - callback(key, op, dict(fvs)) \ No newline at end of file + while True: + key, op, fvs = subscriber.pop() + if not key: + break + log_debug("Received message : '%s'" % str((key, op, fvs))) + for callback in self.callbacks[subscriber.getDbConnector().getDbId()][subscriber.getTableName()]: + callback(key, op, dict(fvs)) + rc = self.cfg_manager.commit() + if not rc: + log_crit("Runner::commit was unsuccessful") diff --git a/src/sonic-bgpcfgd/bgpcfgd/utils.py b/src/sonic-bgpcfgd/bgpcfgd/utils.py index 63d53512d04c..189d875b83e9 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/utils.py +++ b/src/sonic-bgpcfgd/bgpcfgd/utils.py @@ -1,10 +1,7 @@ -import datetime import subprocess -import time - import yaml -from .log import log_debug, log_err, log_info, log_warn, log_crit +from .log import log_crit, log_debug, log_err def run_command(command, shell=False, hide_errors=False): @@ -26,25 +23,6 @@ def run_command(command, shell=False, hide_errors=False): return p.returncode, stdout, stderr -def wait_for_daemons(daemons, seconds): - """ - Wait until FRR daemons are ready for requests - :param daemons: list of FRR daemons to wait - :param seconds: number of seconds to wait, until raise an error - """ - stop_time = datetime.datetime.now() + datetime.timedelta(seconds=seconds) - log_info("Start waiting for FRR daemons: %s" % str(datetime.datetime.now())) - while datetime.datetime.now() < stop_time: - ret_code, out, err = run_command(["vtysh", "-c", "show daemons"], hide_errors=True) - if ret_code == 0 and all(daemon in out for daemon in daemons): - log_info("All required daemons have connected to vtysh: %s" % str(datetime.datetime.now())) - return - else: - log_warn("Can't read daemon status from FRR: %s" % str(err)) - time.sleep(0.1) # sleep 100 ms - raise RuntimeError("FRR daemons hasn't been started in %d seconds" % seconds) - - def read_constants(): """ Read file with constants values from /etc/sonic/constants.yml """ with open('/etc/sonic/constants.yml') as fp: diff --git a/src/sonic-bgpcfgd/tests/test_allow_list.py b/src/sonic-bgpcfgd/tests/test_allow_list.py index 3288702b5814..aa3038e35b99 100644 --- a/src/sonic-bgpcfgd/tests/test_allow_list.py +++ b/src/sonic-bgpcfgd/tests/test_allow_list.py @@ -1,3 +1,4 @@ +import bgpcfgd.frr from bgpcfgd.directory import Directory from bgpcfgd.template import TemplateFabric import bgpcfgd @@ -30,7 +31,7 @@ def push_list(args): assert args == expected_config return True # - bgpcfgd.managers_allow_list.run_command = lambda cmd: (0, "", "") + bgpcfgd.frr.run_command = lambda cmd: (0, "", "") # cfg_mgr = MagicMock() cfg_mgr.update.return_value = None @@ -429,6 +430,7 @@ def test___find_peer_group_by_deployment_id(): } mgr = BGPAllowListMgr(common_objs, "CONFIG_DB", "BGP_ALLOWED_PREFIXES") values = mgr._BGPAllowListMgr__find_peer_group_by_deployment_id(0) +<<<<<<< HEAD assert values == ['PEER_V4_INT', 'PEER_V6_INT', 'PEER_V6', 'PEER_V4'] @patch.dict("sys.modules", swsscommon=swsscommon_module_mock) @@ -479,6 +481,9 @@ def run_command(cmd): bgpcfgd.managers_allow_list.run_command = run_command rc = mgr._BGPAllowListMgr__restart_peers(5) assert rc +======= + assert set(values) == {'PEER_V4_INT', 'PEER_V6_INT', 'PEER_V6', 'PEER_V4'} +>>>>>>> 148436d4... [bgpcfg]: Batch bgp updates (#6006) @patch.dict("sys.modules", swsscommon=swsscommon_module_mock) def test___to_prefix_list(): diff --git a/src/sonic-bgpcfgd/tests/test_bbr.py b/src/sonic-bgpcfgd/tests/test_bbr.py index 86ca2ee20ee6..61ef30daa422 100644 --- a/src/sonic-bgpcfgd/tests/test_bbr.py +++ b/src/sonic-bgpcfgd/tests/test_bbr.py @@ -3,7 +3,6 @@ from mock import MagicMock, patch from copy import deepcopy import swsscommon_test -import bgpcfgd with patch.dict("sys.modules", swsscommon=swsscommon_test): from bgpcfgd.managers_bbr import BBRMgr @@ -37,10 +36,9 @@ def test_constructor():#m1, m2, m3): assert m.directory.get("CONFIG_DB", "BGP_BBR", "status") == "disabled" @patch('bgpcfgd.managers_bbr.log_info') -@patch('bgpcfgd.managers_bbr.log_crit') def set_handler_common(key, value, - is_enabled, is_valid, has_no_push_cmd_errors, - mocked_log_crit, mocked_log_info): + is_enabled, is_valid, + mocked_log_info): cfg_mgr = MagicMock() common_objs = { 'directory': Directory(), @@ -50,13 +48,19 @@ def set_handler_common(key, value, } m = BBRMgr(common_objs, "CONFIG_DB", "BGP_BBR") m.enabled = is_enabled - prepare_config_return_value = [ - ["vtysh", "-c", "clear bgp peer-group PEER_V4 soft in"], - ["vtysh", "-c", "clear bgp peer-group PEER_V6 soft in"] - ] + prepare_config_return_value = ( + [ + ["vtysh", "-c", "clear bgp peer-group PEER_V4 soft in"], + ["vtysh", "-c", "clear bgp peer-group PEER_V6 soft in"] + ], + [ + "PEER_V4", + "PEER_V6" + ] + ) m._BBRMgr__set_prepare_config = MagicMock(return_value = prepare_config_return_value) - m.cfg_mgr.push_list = MagicMock(return_value = has_no_push_cmd_errors) - m._BBRMgr__restart_peers = MagicMock() + m.cfg_mgr.push_list = MagicMock(return_value = None) + m.cfg_mgr.restart_peer_groups = MagicMock(return_value = None) # FIXME: check for input res = m.set_handler(key, value) assert res, "Returns always True" if not is_enabled: @@ -64,28 +68,21 @@ def set_handler_common(key, value, else: if is_valid: m._BBRMgr__set_prepare_config.assert_called_once_with(value["status"]) - m.cfg_mgr.push_list.assert_called_once_with(prepare_config_return_value) - if has_no_push_cmd_errors: - m._BBRMgr__restart_peers.assert_called_once() - else: - mocked_log_crit.assert_called_with("BBRMgr::can't apply configuration") - m._BBRMgr__restart_peers.assert_not_called() + m.cfg_mgr.push_list.assert_called_once_with(prepare_config_return_value[0]) + m.cfg_mgr.restart_peer_groups.assert_called_once_with(prepare_config_return_value[1]) else: m._BBRMgr__set_prepare_config.assert_not_called() m.cfg_mgr.push_list.assert_not_called() - m._BBRMgr__restart_peers.assert_not_called() + m.cfg_mgr.restart_peer_groups.assert_not_called() -def test_set_handler_1(): - set_handler_common("anything", {}, False, False, True) +def test_set_handler_not_enabled_not_valid(): + set_handler_common("anything", {}, False, False) -def test_set_handler_2(): - set_handler_common("anything", {}, True, False, True) +def test_set_handler_enabled_not_valid(): + set_handler_common("anything", {}, True, False) -def test_set_handler_3(): - set_handler_common("all", {"status": "enabled"}, True, True, True) - -def test_set_handler_4(): - set_handler_common("all", {"status": "enabled"}, True, True, False) +def test_set_handler_enabled_valid(): + set_handler_common("all", {"status": "enabled"}, True, True) @patch('bgpcfgd.managers_bbr.log_err') def test_del_handler(mocked_log_err): @@ -271,8 +268,9 @@ def __set_prepare_config_common(status, bbr_enabled_pgs, available_pgs, expected } m.bbr_enabled_pgs = bbr_enabled_pgs m._BBRMgr__get_available_peer_groups = MagicMock(return_value = available_pgs) - cmds = m._BBRMgr__set_prepare_config(status) + cmds, peer_groups = m._BBRMgr__set_prepare_config(status) assert cmds == expected_cmds + assert set(peer_groups) == available_pgs def test___set_prepare_config_enabled(): __set_prepare_config_common("enabled", { @@ -285,7 +283,7 @@ def test___set_prepare_config_enabled(): ' address-family ipv6', ' neighbor PEER_V4 allowas-in 1', ' neighbor PEER_V6 allowas-in 1', - ]) + ]) def test___set_prepare_config_disabled(): __set_prepare_config_common("disabled", { @@ -357,45 +355,3 @@ def test__get_available_peer_groups(): ]) res = m._BBRMgr__get_available_peer_groups() assert res == {"PEER_V4", "PEER_V6"} - -@patch('bgpcfgd.managers_bbr.log_crit') -def __restart_peers_common(run_command_results, run_command_expects, last_log_crit_message, mocked_log_crit): - cfg_mgr = MagicMock() - common_objs = { - 'directory': Directory(), - 'cfg_mgr': cfg_mgr, - 'tf': TemplateFabric(), - 'constants': global_constants, - } - m = BBRMgr(common_objs, "CONFIG_DB", "BGP_BBR") - m.bbr_enabled_pgs = { - "PEER_V4": ["ipv4", "ipv6"], - "PEER_V6": ["ipv6"], - } - def run_command_mock(cmd): - assert cmd == run_command_expects[run_command_mock.run] - res = run_command_results[run_command_mock.run] - run_command_mock.run += 1 - return res - run_command_mock.run = 0 - bgpcfgd.managers_bbr.run_command = run_command_mock - #lambda cmd: (0, "", "") - m._BBRMgr__restart_peers() - if last_log_crit_message is not None: - mocked_log_crit.assert_called_with(last_log_crit_message) - -def test___restart_peers_1(): - __restart_peers_common([(0, "", ""), (0, "", "")], - [ - ["vtysh", "-c", "clear bgp peer-group PEER_V4 soft in"], - ["vtysh", "-c", "clear bgp peer-group PEER_V6 soft in"] - ], - None) - -def test___restart_peers_2(): - __restart_peers_common([(1, "out1", "err1"), (0, "", "")], - [ - ["vtysh", "-c", "clear bgp peer-group PEER_V4 soft in"], - ["vtysh", "-c", "clear bgp peer-group PEER_V6 soft in"] - ], - "BBRMgr::Can't restart bgp peer-group 'PEER_V4'. rc='1', out='out1', err='err1'") diff --git a/src/sonic-bgpcfgd/tests/test_config.py b/src/sonic-bgpcfgd/tests/test_config.py new file mode 100644 index 000000000000..c8bd0c9ca2be --- /dev/null +++ b/src/sonic-bgpcfgd/tests/test_config.py @@ -0,0 +1,207 @@ +from unittest.mock import MagicMock + +from bgpcfgd.config import ConfigMgr + + +def test_constructor(): + frr = MagicMock() + c = ConfigMgr(frr) + assert c.frr == frr + assert c.current_config is None + assert c.current_config_raw is None + assert c.changes == "" + assert c.peer_groups_to_restart == [] + +def test_reset(): + frr = MagicMock() + c = ConfigMgr(frr) + c.reset() + assert c.frr == frr + assert c.current_config is None + assert c.current_config_raw is None + assert c.changes == "" + assert c.peer_groups_to_restart == [] + +def test_update(): + frr = MagicMock() + frr.get_config = MagicMock(return_value = """! + text1 + ! comment + text2 + text3 + ! comment + text4 + """) + c = ConfigMgr(frr) + c.update() + assert c.current_config_raw == [' text1', ' text2', ' text3', ' text4', ' ', ' '] + assert c.current_config == [['text1'], ['text2'], ['text3'], ['text4']] + +def test_push_list(): + frr = MagicMock() + c = ConfigMgr(frr) + c.push_list(["change1", "change2"]) + assert c.changes == "change1\nchange2\n" + c.push_list(["change3", "change4"]) + assert c.changes == "change1\nchange2\nchange3\nchange4\n" + +def test_push(): + frr = MagicMock() + c = ConfigMgr(frr) + c.push("update1\nupdate2\n") + assert c.changes == "update1\nupdate2\n\n" + c.push("update3\nupdate4\n") + assert c.changes == "update1\nupdate2\n\nupdate3\nupdate4\n\n" + +def test_push_and_push_list(): + frr = MagicMock() + c = ConfigMgr(frr) + c.push("update1\nupdate2\n") + c.push_list(["change1", "change2"]) + assert c.changes == "update1\nupdate2\n\nchange1\nchange2\n" + +def test_restart_peer_groups(): + frr = MagicMock() + c = ConfigMgr(frr) + c.restart_peer_groups(["pg_1", "pg_2"]) + assert c.peer_groups_to_restart == ["pg_1", "pg_2"] + c.restart_peer_groups(["pg_3", "pg_4"]) + assert c.peer_groups_to_restart == ["pg_1", "pg_2", "pg_3", "pg_4"] + +def test_commit_empty_changes(): + frr = MagicMock() + c = ConfigMgr(frr) + res = c.commit() + assert res + assert not frr.write.called + +def commit_changes_common(write_error, restart_error, result): + frr = MagicMock() + frr.write = MagicMock(return_value = write_error) + frr.restart_peer_groups = MagicMock(return_value = restart_error) + c = ConfigMgr(frr) + c.reset = MagicMock() + c.push_list(["change1", "change2"]) + c.restart_peer_groups(["pg1", "pg2"]) + res = c.commit() + assert res == result + assert c.reset.called + frr.write.assert_called_with('change1\nchange2\n') + frr.restart_peer_groups.assert_called_with(["pg1", "pg2"]) + +def test_commit_changes_no_errors(): + commit_changes_common(True, True, True) + +def test_commit_changes_write_error(): + commit_changes_common(False, True, False) + +def test_commit_changes_restart_error(): + commit_changes_common(True, False, False) + +def test_commit_changes_both_errors(): + commit_changes_common(False, False, False) + +def test_restart_get_text(): + frr = MagicMock() + frr.get_config = MagicMock(return_value = """! + text1 + ! comment + text2 + text3 + ! comment + text4 + """) + c = ConfigMgr(frr) + c.update() + assert c.get_text() == [' text1', ' text2', ' text3', ' text4', ' ', ' '] + +def to_canonical_common(raw_text, expected_canonical): + frr = MagicMock() + c = ConfigMgr(frr) + assert c.to_canonical(raw_text) == expected_canonical + +def test_to_canonical_empty(): + raw_config = """ +! + ! + ! + ! + + ! + +""" + to_canonical_common(raw_config, []) + +def test_to_canonical_(): + raw_config = """ +! +router bgp 12345 + bgp router-id 1020 + address-family ipv4 + neighbor PEER_V4 peer-group + neighbor PEER_V4 route-map A10 in + exit-address-family + address-family ipv6 + neighbor PEER_V6 peer-group + neighbor PEER_V6 route-map A20 in + exit-address-family +route-map A10 permit 10 +! +route-map A20 permit 10 +! + +""" + expected = [ + ['router bgp 12345'], + ['router bgp 12345', 'bgp router-id 1020'], + ['router bgp 12345', 'address-family ipv4'], + ['router bgp 12345', 'address-family ipv4', 'neighbor PEER_V4 peer-group'], + ['router bgp 12345', 'address-family ipv4', 'neighbor PEER_V4 route-map A10 in'], + ['router bgp 12345', 'exit-address-family'], + ['router bgp 12345', 'address-family ipv6'], + ['router bgp 12345', 'address-family ipv6', 'neighbor PEER_V6 peer-group'], + ['router bgp 12345', 'address-family ipv6', 'neighbor PEER_V6 route-map A20 in'], + ['router bgp 12345', 'exit-address-family'], + ['route-map A10 permit 10'], + ['route-map A20 permit 10'] + ] + to_canonical_common(raw_config, expected) + +def test_count_spaces(): + frr = MagicMock() + c = ConfigMgr(frr) + assert c.count_spaces(" !") == 2 + assert c.count_spaces("!") == 0 + assert c.count_spaces("") == 0 + +def test_from_canonical(): + canonical = [ + ['router bgp 12345'], + ['router bgp 12345', 'bgp router-id 1020'], + ['router bgp 12345', 'address-family ipv4'], + ['router bgp 12345', 'address-family ipv4', 'neighbor PEER_V4 peer-group'], + ['router bgp 12345', 'address-family ipv4', 'neighbor PEER_V4 route-map A10 in'], + ['router bgp 12345', 'exit-address-family'], + ['router bgp 12345', 'address-family ipv6'], + ['router bgp 12345', 'address-family ipv6', 'neighbor PEER_V6 peer-group'], + ['router bgp 12345', 'address-family ipv6', 'neighbor PEER_V6 route-map A20 in'], + ['router bgp 12345', 'exit-address-family'], + ['route-map A10 permit 10'], + ['route-map A20 permit 10'] + ] + expected = 'router bgp 12345\n' \ + ' bgp router-id 1020\n' \ + ' address-family ipv4\n' \ + ' neighbor PEER_V4 peer-group\n' \ + ' neighbor PEER_V4 route-map A10 in\n' \ + ' exit-address-family\n' \ + ' address-family ipv6\n' \ + ' neighbor PEER_V6 peer-group\n' \ + ' neighbor PEER_V6 route-map A20 in\n' \ + ' exit-address-family\n' \ + 'route-map A10 permit 10\n' \ + 'route-map A20 permit 10\n' + frr = MagicMock() + c = ConfigMgr(frr) + raw = c.from_canonical(canonical) + assert raw == expected diff --git a/src/sonic-bgpcfgd/tests/test_frr.py b/src/sonic-bgpcfgd/tests/test_frr.py new file mode 100644 index 000000000000..47bdcaa82f10 --- /dev/null +++ b/src/sonic-bgpcfgd/tests/test_frr.py @@ -0,0 +1,6 @@ +from bgpcfgd.frr import FRR + + +def test_constructor(): + f = FRR(["abc", "cde"]) + assert f.daemons == ["abc", "cde"]