[bgpcfg]: Batch bgp updates (#6006)

* [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.

* Use correct file to import run_command

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"])
+        out = self.frr.get_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
         text = []
         for line in out.split('\n'):
             if line.lstrip().startswith('!'):
@@ -33,40 +26,41 @@ class ConfigMgr(object):
             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 write(self, cmd):
+    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 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')
+        if self.changes.strip() == "":
-        os.close(fd)
+            return True
-        with open(tmp_filename, 'w') as fp:
+        rc_write = self.frr.write(self.changes)
-            fp.write("%s\n" % cmd)
+        rc_restart = self.frr.restart_peer_groups(self.peer_groups_to_restart)
-        command = ["vtysh", "-f", tmp_filename]
+        self.reset()
-        ret_code, out, err = run_command(command)
+        return rc_write and rc_restart
-        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
 
     def get_text(self):
         return self.current_config_raw
@@ -89,7 +83,6 @@ class ConfigMgr(object):
         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:

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

src/sonic-bgpcfgd/bgpcfgd/main.py

@@ -17,17 +17,19 @@ from .managers_intf import InterfaceMgr
 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()

src/sonic-bgpcfgd/bgpcfgd/managers_allow_list.py

@@ -6,7 +6,7 @@ import re
 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 @@ class BGPAllowListMgr(Manager):
         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)
+            self.cfg_mgr.push_list(cmds)
-            rc = rc and self.__restart_peers(deployment_id)
+            peer_groups = self.__find_peer_group_by_deployment_id(deployment_id)
-            log_debug("BGPAllowListMgr::__update_policy. The peers were updated: rc=%s" % rc)
+            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 @@ class BGPAllowListMgr(Manager):
         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)
+            self.cfg_mgr.push_list(cmds)
-            rc = rc and self.__restart_peers(deployment_id)
+            peer_groups = self.__find_peer_group_by_deployment_id(deployment_id)
-            log_debug("BGPAllowListMgr::__remove_policy. 'Allow list' policy was removed. rc:%s" % rc)
+            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 @@ class BGPAllowListMgr(Manager):
             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 @@ class BGPAllowListMgr(Manager):
         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

src/sonic-bgpcfgd/bgpcfgd/managers_bbr.py

@@ -2,9 +2,8 @@ import re
 
 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 @@ class BBRMgr(Manager):
             return True
         if not self.__set_validation(key, data):
             return True
-        cmds = self.__set_prepare_config(data['status'])
+        cmds, peer_groups_to_restart = self.__set_prepare_config(data['status'])
-        rv = self.cfg_mgr.push_list(cmds)
+        self.cfg_mgr.push_list(cmds)
-        if not rv:
+        self.cfg_mgr.restart_peer_groups(peer_groups_to_restart)
-            log_crit("BBRMgr::can't apply configuration")
+        log_info("BBRMgr::Scheduled BBR update")
-            return True
-        self.__restart_peers()
         return True
 
     def del_handler(self, key):
@@ -112,12 +109,14 @@ class BBRMgr(Manager):
         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 @@ class BBRMgr(Manager):
             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)

src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py

@@ -45,8 +45,8 @@ class BGPPeerGroupMgr(object):
         except jinja2.TemplateError as e:
             log_err("Can't render policy template name: '%s': %s" % (name, str(e)))
             return False
-
+        self.update_entity(policy, "Routing policy for peer '%s'" % name)
-        return self.update_entity(policy, "Routing policy for peer '%s'" % name)
+        return True
 
     def update_pg(self, name, **kwargs):
         """
@@ -64,8 +64,8 @@ class BGPPeerGroupMgr(object):
             cmd = ('router bgp %s\n' % kwargs['bgp_asn']) + pg
         else:
             cmd = ('router bgp %s vrf %s\n' % (kwargs['bgp_asn'], kwargs['vrf'])) + pg
-
+        self.update_entity(cmd, "Peer-group for peer '%s'" % name)
-        return self.update_entity(cmd, "Peer-group for peer '%s'" % name)
+        return True
 
     def update_entity(self, cmd, txt):
         """
@@ -74,12 +74,9 @@ class BGPPeerGroupMgr(object):
         :param txt: text for the syslog output
         :return:
         """
-        ret_code = self.cfg_mgr.push(cmd)
+        self.cfg_mgr.push(cmd)
-        if ret_code:
+        log_info("%s has been scheduled to be updated" % txt)
-            log_info("%s was updated" % txt)
+        return True
-        else:
-            log_err("Can't update %s" % txt)
-        return ret_code
 
 
 class BGPPeerMgrBase(Manager):
@@ -212,13 +209,10 @@ class BGPPeerMgrBase(Manager):
             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)
-                self.peers.add(key)
+            log_info("Peer '(%s|%s)' has been scheduled to be added with attributes '%s'" % print_data)
-                log_info("Peer '(%s|%s)' added with attributes '%s'" % print_data)
-            else:
-                log_err("Peer '(%s|%s)' wasn't added." % (vrf, nbr))
 
         return True
 
@@ -300,7 +294,8 @@ class BGPPeerMgrBase(Manager):
             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):
         """

src/sonic-bgpcfgd/bgpcfgd/managers_setsrc.py

@@ -57,10 +57,8 @@ class ZebraSetSrc(Manager):
             except jinja2.TemplateError as e:
                 log_err("Error while rendering 'set src' template: %s" % str(e))
                 return True
-            if self.cfg_mgr.push(txt):
+            self.cfg_mgr.push(txt)
-                log_info("The 'set src' configuration with Loopback0 ip '%s' was pushed" % ip_addr)
+            log_info("The 'set src' configuration with Loopback0 ip '%s' has been scheduled to be added" % ip_addr)
-            else:
-                log_err("The 'set src' configuration with Loopback0 ip '%s' wasn't pushed" % ip_addr)
         return True
 
     def del_handler(self, key):

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 @@ class Runner(object):
                 raise Exception("Received error from select")
 
             for subscriber in self.subscribers:
-                key, op, fvs = subscriber.pop()
+                while True:
-                if not key:
+                    key, op, fvs = subscriber.pop()
-                    continue
+                    if not key:
-                log_debug("Received message : '%s'" % str((key, op, fvs)))
+                        break
-                for callback in self.callbacks[subscriber.getDbConnector().getDbId()][subscriber.getTableName()]:
+                    log_debug("Received message : '%s'" % str((key, op, fvs)))
-                    callback(key, op, dict(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")

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:

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 set_del_test(op, args, currect_config, expected_config):
         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'])
+    assert set(values) == {'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
 
 @patch.dict("sys.modules", swsscommon=swsscommon_module_mock)
 def test___to_prefix_list():

src/sonic-bgpcfgd/tests/test_bbr.py

@@ -4,7 +4,6 @@ from bgpcfgd.directory import Directory
 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,
+                       is_enabled, is_valid,
-                       mocked_log_crit, mocked_log_info):
+                       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 = [
+    prepare_config_return_value = (
-                               ["vtysh", "-c", "clear bgp peer-group PEER_V4 soft in"],
+        [
-                               ["vtysh", "-c", "clear bgp peer-group PEER_V6 soft in"]
+           ["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.cfg_mgr.push_list = MagicMock(return_value = None)
-    m._BBRMgr__restart_peers = MagicMock()
+    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)
+            m.cfg_mgr.push_list.assert_called_once_with(prepare_config_return_value[0])
-            if has_no_push_cmd_errors:
+            m.cfg_mgr.restart_peer_groups.assert_called_once_with(prepare_config_return_value[1])
-                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()
         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():
+def test_set_handler_not_enabled_not_valid():
-    set_handler_common("anything", {}, False, False, True)
+    set_handler_common("anything", {}, False, False)
 
-def test_set_handler_2():
+def test_set_handler_enabled_not_valid():
-    set_handler_common("anything", {}, True, False, True)
+    set_handler_common("anything", {}, True, False)
 
-def test_set_handler_3():
+def test_set_handler_enabled_valid():
-    set_handler_common("all", {"status": "enabled"}, True, True, True)
+    set_handler_common("all", {"status": "enabled"}, True, True)
-
-def test_set_handler_4():
-    set_handler_common("all", {"status": "enabled"}, True, True, False)
 
 @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'")

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

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"]