From 5530bab204c2e738bdd55205c60b0ee13685758f Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Fri, 6 Nov 2020 15:12:15 -0800 Subject: [PATCH 1/5] [bgpcfgd]: Batch bgp updates. vtysh -f command is slow. It is sometimes takes about 3 seconds. When we need to run many vtysh -f commands that slows down the system. Batch vtysh -f updates. --- src/sonic-bgpcfgd/bgpcfgd/config.py | 67 +++--- src/sonic-bgpcfgd/bgpcfgd/frr.py | 70 ++++++ src/sonic-bgpcfgd/bgpcfgd/main.py | 10 +- .../bgpcfgd/managers_allow_list.py | 36 +-- src/sonic-bgpcfgd/bgpcfgd/managers_bbr.py | 25 +-- src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py | 31 ++- src/sonic-bgpcfgd/bgpcfgd/managers_setsrc.py | 6 +- src/sonic-bgpcfgd/bgpcfgd/runner.py | 21 +- src/sonic-bgpcfgd/bgpcfgd/utils.py | 24 +- src/sonic-bgpcfgd/tests/test_allow_list.py | 54 +---- src/sonic-bgpcfgd/tests/test_bbr.py | 96 +++----- src/sonic-bgpcfgd/tests/test_config.py | 207 ++++++++++++++++++ src/sonic-bgpcfgd/tests/test_frr.py | 6 + .../tests/test_ipv6_nexthop_global.py | 3 +- 14 files changed, 397 insertions(+), 259 deletions(-) create mode 100644 src/sonic-bgpcfgd/bgpcfgd/frr.py create mode 100644 src/sonic-bgpcfgd/tests/test_config.py create mode 100644 src/sonic-bgpcfgd/tests/test_frr.py 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 52f1ac3e536d..3db3f4cf053d 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py @@ -7,7 +7,7 @@ from .log import log_warn, log_err, log_info, log_debug, log_crit from .manager import Manager from .template import TemplateFabric -from .utils import run_command +from .frr import run_command class BGPPeerGroupMgr(object): @@ -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 ce235af9d8e4..01c21b0f4872 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 d419cb4f1530..904ba14f2ecf 100644 --- a/src/sonic-bgpcfgd/tests/test_allow_list.py +++ b/src/sonic-bgpcfgd/tests/test_allow_list.py @@ -1,5 +1,6 @@ from unittest.mock import MagicMock, patch +import bgpcfgd.frr from bgpcfgd.directory import Directory from bgpcfgd.template import TemplateFabric import bgpcfgd @@ -31,7 +32,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 @@ -430,56 +431,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) - assert set(values) == set(['PEER_V4_INT', 'PEER_V6_INT', 'PEER_V6', 'PEER_V4']) - -@patch.dict("sys.modules", swsscommon=swsscommon_module_mock) -def test___restart_peers_found_deployment_id(): - from bgpcfgd.managers_allow_list import BGPAllowListMgr - test___restart_peers_found_deployment_id.run_command_counter = 0 - def run_command(cmd): - output = [ - ['vtysh', '-c', 'clear bgp peer-group BGP_TEST_PEER_GROUP_1 soft in'], - ['vtysh', '-c', 'clear bgp peer-group BGP_TEST_PEER_GROUP_2 soft in'], - ] - desired_value = output[test___restart_peers_found_deployment_id.run_command_counter] - assert cmd == desired_value - test___restart_peers_found_deployment_id.run_command_counter += 1 - return 0, "", "" - cfg_mgr = MagicMock() - common_objs = { - 'directory': Directory(), - 'cfg_mgr': cfg_mgr, - 'tf': TemplateFabric(), - 'constants': global_constants, - } - mgr = BGPAllowListMgr(common_objs, "CONFIG_DB", "BGP_ALLOWED_PREFIXES") - mocked = MagicMock(name='_BGPAllowListMgr__find_peer_group_by_deployment_id') - mocked.return_value = ["BGP_TEST_PEER_GROUP_1", "BGP_TEST_PEER_GROUP_2"] - mgr._BGPAllowListMgr__find_peer_group_by_deployment_id = mocked - bgpcfgd.managers_allow_list.run_command = run_command - rc = mgr._BGPAllowListMgr__restart_peers(5) - assert rc - -@patch.dict("sys.modules", swsscommon=swsscommon_module_mock) -def test___restart_peers_not_found_deployment_id(): - from bgpcfgd.managers_allow_list import BGPAllowListMgr - def run_command(cmd): - assert cmd == ['vtysh', '-c', 'clear bgp * soft in'] - return 0, "", "" - cfg_mgr = MagicMock() - common_objs = { - 'directory': Directory(), - 'cfg_mgr': cfg_mgr, - 'tf': TemplateFabric(), - 'constants': global_constants, - } - mgr = BGPAllowListMgr(common_objs, "CONFIG_DB", "BGP_ALLOWED_PREFIXES") - mocked = MagicMock(name='_BGPAllowListMgr__find_peer_group_by_deployment_id') - mocked.return_value = [] - mgr._BGPAllowListMgr__find_peer_group_by_deployment_id = mocked - 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'} @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 cc0c83fad648..45b8aa7c8f07 100644 --- a/src/sonic-bgpcfgd/tests/test_bbr.py +++ b/src/sonic-bgpcfgd/tests/test_bbr.py @@ -4,7 +4,6 @@ from bgpcfgd.template import TemplateFabric from copy import deepcopy from . import swsscommon_test -import bgpcfgd with patch.dict("sys.modules", swsscommon=swsscommon_test): @@ -39,10 +38,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(), @@ -52,13 +50,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: @@ -66,28 +70,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): @@ -273,8 +270,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", { @@ -287,7 +285,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", { @@ -359,45 +357,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"] diff --git a/src/sonic-bgpcfgd/tests/test_ipv6_nexthop_global.py b/src/sonic-bgpcfgd/tests/test_ipv6_nexthop_global.py index d1bb67ee9cd4..389835c1b25a 100644 --- a/src/sonic-bgpcfgd/tests/test_ipv6_nexthop_global.py +++ b/src/sonic-bgpcfgd/tests/test_ipv6_nexthop_global.py @@ -90,14 +90,13 @@ def check_routemap_in_file(filename, route_map_name): found_entry = False found_seq_no = None if route_map_re.match(line): - found_seq_no = None seq_n_txt = route_map_re.match(line).group(1) assert seq_n_txt.isdigit(), "wrong sequence number for line '%s'" % line found_seq_no = int(seq_n_txt) assert found_seq_no not in route_map_entries, "Route-map has duplicate entries: %s - %d" % (route_map_name, found_seq_no) found_entry = True results = [route_map_entries[seq] for seq in sorted(route_map_entries.keys())] - if (len(results)): + if len(results): err_msg = "route-map %s doesn't have mandatory permit entry for 'set ipv6 next-hop prefer-global" % route_map_name assert results[0], err_msg return len(results) > 0 From 5b8a37642b579ec7a5380394c51049355347ba7b Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Mon, 23 Nov 2020 09:38:05 -0800 Subject: [PATCH 2/5] Use correct file to import run_command --- src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py b/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py index 3db3f4cf053d..f6108bfb61c4 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py @@ -7,7 +7,7 @@ from .log import log_warn, log_err, log_info, log_debug, log_crit from .manager import Manager from .template import TemplateFabric -from .frr import run_command +from .utils import run_command class BGPPeerGroupMgr(object): From 85e6ce29f87429a0a63d60a954b6f26f23db119e Mon Sep 17 00:00:00 2001 From: pavel-shirshov Date: Tue, 24 Nov 2020 08:45:50 -0800 Subject: [PATCH 3/5] Update test_ipv6_nexthop_global.py --- .../tests/test_ipv6_nexthop_global.py | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/sonic-bgpcfgd/tests/test_ipv6_nexthop_global.py b/src/sonic-bgpcfgd/tests/test_ipv6_nexthop_global.py index 389835c1b25a..398c8a19f3f7 100644 --- a/src/sonic-bgpcfgd/tests/test_ipv6_nexthop_global.py +++ b/src/sonic-bgpcfgd/tests/test_ipv6_nexthop_global.py @@ -79,6 +79,7 @@ def extract_rm_from_peer_group(path, peer_group_name): def check_routemap_in_file(filename, route_map_name): route_map_re = re.compile(r'^route-map\s+%s\s+permit\s+(\d+)' % route_map_name) set_re = re.compile(r'set ipv6 next-hop prefer-global') + next_re = re.compile(r'on-match next') with open(filename) as fp: lines = [line.strip() for line in fp if not line.strip().startswith('!') and line.strip() != ''] found_entry = False @@ -86,20 +87,26 @@ def check_routemap_in_file(filename, route_map_name): route_map_entries = {} for line in lines: if found_entry: - route_map_entries[found_seq_no] = set_re.match(line) is not None - found_entry = False - found_seq_no = None + if set_re.match(line): + route_map_entries[found_seq_no][0] = True + elif next_re.match(line): + route_map_entries[found_seq_no][1] = True + else: + found_entry = False + found_seq_no = None if route_map_re.match(line): + found_seq_no = None seq_n_txt = route_map_re.match(line).group(1) assert seq_n_txt.isdigit(), "wrong sequence number for line '%s'" % line found_seq_no = int(seq_n_txt) assert found_seq_no not in route_map_entries, "Route-map has duplicate entries: %s - %d" % (route_map_name, found_seq_no) found_entry = True + route_map_entries[found_seq_no] = [False, False] results = [route_map_entries[seq] for seq in sorted(route_map_entries.keys())] - if len(results): - err_msg = "route-map %s doesn't have mandatory permit entry for 'set ipv6 next-hop prefer-global" % route_map_name - assert results[0], err_msg - return len(results) > 0 + err_msg = "route-map %s doesn't have mandatory permit entry for 'set ipv6 next-hop prefer-global" % route_map_name + assert len(results), err_msg + assert all(results[0]), "first ipv6 route-map entry doesn't have set ipv6 nexthop" + return True def check_routemap(path, route_map_name): result_files = load_results(path, "policies.conf") From c9c243cf88f59035355245ade39ae6b2dfa3c42b Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Tue, 24 Nov 2020 08:52:06 -0800 Subject: [PATCH 4/5] Revert "Update test_ipv6_nexthop_global.py" This reverts commit 85e6ce29f87429a0a63d60a954b6f26f23db119e. --- .../tests/test_ipv6_nexthop_global.py | 21 +++++++------------ 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/src/sonic-bgpcfgd/tests/test_ipv6_nexthop_global.py b/src/sonic-bgpcfgd/tests/test_ipv6_nexthop_global.py index 398c8a19f3f7..389835c1b25a 100644 --- a/src/sonic-bgpcfgd/tests/test_ipv6_nexthop_global.py +++ b/src/sonic-bgpcfgd/tests/test_ipv6_nexthop_global.py @@ -79,7 +79,6 @@ def extract_rm_from_peer_group(path, peer_group_name): def check_routemap_in_file(filename, route_map_name): route_map_re = re.compile(r'^route-map\s+%s\s+permit\s+(\d+)' % route_map_name) set_re = re.compile(r'set ipv6 next-hop prefer-global') - next_re = re.compile(r'on-match next') with open(filename) as fp: lines = [line.strip() for line in fp if not line.strip().startswith('!') and line.strip() != ''] found_entry = False @@ -87,26 +86,20 @@ def check_routemap_in_file(filename, route_map_name): route_map_entries = {} for line in lines: if found_entry: - if set_re.match(line): - route_map_entries[found_seq_no][0] = True - elif next_re.match(line): - route_map_entries[found_seq_no][1] = True - else: - found_entry = False - found_seq_no = None - if route_map_re.match(line): + route_map_entries[found_seq_no] = set_re.match(line) is not None + found_entry = False found_seq_no = None + if route_map_re.match(line): seq_n_txt = route_map_re.match(line).group(1) assert seq_n_txt.isdigit(), "wrong sequence number for line '%s'" % line found_seq_no = int(seq_n_txt) assert found_seq_no not in route_map_entries, "Route-map has duplicate entries: %s - %d" % (route_map_name, found_seq_no) found_entry = True - route_map_entries[found_seq_no] = [False, False] results = [route_map_entries[seq] for seq in sorted(route_map_entries.keys())] - err_msg = "route-map %s doesn't have mandatory permit entry for 'set ipv6 next-hop prefer-global" % route_map_name - assert len(results), err_msg - assert all(results[0]), "first ipv6 route-map entry doesn't have set ipv6 nexthop" - return True + if len(results): + err_msg = "route-map %s doesn't have mandatory permit entry for 'set ipv6 next-hop prefer-global" % route_map_name + assert results[0], err_msg + return len(results) > 0 def check_routemap(path, route_map_name): result_files = load_results(path, "policies.conf") From 4aa96f1ad0684bfc8102f9d13d00861ba85d3ce9 Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Tue, 24 Nov 2020 08:53:42 -0800 Subject: [PATCH 5/5] Revert changes in the test --- src/sonic-bgpcfgd/tests/test_ipv6_nexthop_global.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/sonic-bgpcfgd/tests/test_ipv6_nexthop_global.py b/src/sonic-bgpcfgd/tests/test_ipv6_nexthop_global.py index 389835c1b25a..d1bb67ee9cd4 100644 --- a/src/sonic-bgpcfgd/tests/test_ipv6_nexthop_global.py +++ b/src/sonic-bgpcfgd/tests/test_ipv6_nexthop_global.py @@ -90,13 +90,14 @@ def check_routemap_in_file(filename, route_map_name): found_entry = False found_seq_no = None if route_map_re.match(line): + found_seq_no = None seq_n_txt = route_map_re.match(line).group(1) assert seq_n_txt.isdigit(), "wrong sequence number for line '%s'" % line found_seq_no = int(seq_n_txt) assert found_seq_no not in route_map_entries, "Route-map has duplicate entries: %s - %d" % (route_map_name, found_seq_no) found_entry = True results = [route_map_entries[seq] for seq in sorted(route_map_entries.keys())] - if len(results): + if (len(results)): err_msg = "route-map %s doesn't have mandatory permit entry for 'set ipv6 next-hop prefer-global" % route_map_name assert results[0], err_msg return len(results) > 0