[bgpcfgd]: Fixes for BBR (#5956)

* Add explicit default state into the constants.yml
* Enable/disable only peer-groups, available in the config
* Retrieve updates from frr before using configuration

Co-authored-by: Pavel Shirshov <pavel.contrib@gmail.com>

files/image_config/constants/constants.yml

@@ -31,6 +31,7 @@ constants:
           - "deny 0::/0 ge 65"
     bbr:
       enabled: true
+      default_state: "disabled"
     peers:
       general: # peer_type
         db_table: "BGP_NEIGHBOR"

src/sonic-bgpcfgd/bgpcfgd/managers_bbr.py

@@ -1,3 +1,5 @@
+import re
+
 from swsscommon import swsscommon
 
 from .log import log_err, log_info, log_crit
@@ -49,18 +51,23 @@ class BBRMgr(Manager):
         if not 'bgp' in self.constants:
             log_err("BBRMgr::Disabled: 'bgp' key is not found in constants")
             return
-        if 'bbr' in self.constants['bgp'] and \
+        if 'bbr' in self.constants['bgp'] \
-                'enabled' in self.constants['bgp']['bbr'] and \
+                and 'enabled' in self.constants['bgp']['bbr'] \
-                self.constants['bgp']['bbr']['enabled']:
+                and self.constants['bgp']['bbr']['enabled']:
             self.bbr_enabled_pgs = self.__read_pgs()
             if self.bbr_enabled_pgs:
                 self.enabled = True
-                self.directory.put(self.db_name, self.table_name, 'status', "enabled")
+                if 'default_state' in self.constants['bgp']['bbr'] \
-                log_info("BBRMgr::Initialized and enabled")
+                        and self.constants['bgp']['bbr']['default_state'] == 'enabled':
+                    default_status = "enabled"
+                else:
+                    default_status = "disabled"
+                self.directory.put(self.db_name, self.table_name, 'status', default_status)
+                log_info("BBRMgr::Initialized and enabled. Default state: '%s'" % default_status)
             else:
                 log_info("BBRMgr::Disabled: no BBR enabled peers")
         else:
-            log_info("BBRMgr::Disabled: not enabled in the constants")
+            log_info("BBRMgr::Disabled: no bgp.bbr.enabled in the constants")
 
     def __read_pgs(self):
         """
@@ -102,15 +109,30 @@ class BBRMgr(Manager):
         :return: list of commands prepared for FRR
         """
         bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"]
+        available_peer_groups = self.__get_available_peer_groups()
         cmds = ["router bgp %s" % bgp_asn]
         prefix_of_commands = "" if status == "enabled" else "no "
         for af in ["ipv4", "ipv6"]:
             cmds.append(" address-family %s" % af)
             for pg_name in sorted(self.bbr_enabled_pgs.keys()):
-                if af in self.bbr_enabled_pgs[pg_name]:
+                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
 
+    def __get_available_peer_groups(self):
+        """
+        Extract configured peer-groups from the config
+        :return: set of available peer-groups
+        """
+        re_pg = re.compile(r'^\s*neighbor\s+(\S+)\s+peer-group\s*$')
+        res = set()
+        self.cfg_mgr.update()
+        for line in self.cfg_mgr.get_text():
+            m = re_pg.match(line)
+            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()):

src/sonic-bgpcfgd/tests/test_bbr.py

@@ -23,9 +23,6 @@ global_constants = {
     }
 }
 
-#@patch('bgpcfgd.managers_bbr.log_info')
-#@patch('bgpcfgd.managers_bbr.log_err')
-#@patch('bgpcfgd.managers_bbr.log_crit')
 def test_constructor():#m1, m2, m3):
     cfg_mgr = MagicMock()
     common_objs = {
@@ -121,8 +118,6 @@ def __init_common(constants,
     assert m.directory.get("CONFIG_DB", "BGP_BBR", "status") == expected_status
     if expected_status == "enabled":
         assert m.enabled
-    else:
-        assert not m.enabled
     if expected_log_err is not None:
         mocked_log_err.assert_called_with(expected_log_err)
     if expected_log_info is not None:
@@ -133,17 +128,17 @@ def test___init_1():
 
 def test___init_2():
     constants = deepcopy(global_constants)
-    __init_common(constants, "BBRMgr::Disabled: not enabled in the constants", None, {}, "disabled")
+    __init_common(constants, "BBRMgr::Disabled: no bgp.bbr.enabled in the constants", None, {}, "disabled")
 
 def test___init_3():
     constants = deepcopy(global_constants)
     constants["bgp"]["bbr"] = { "123" : False }
-    __init_common(constants, "BBRMgr::Disabled: not enabled in the constants", None, {}, "disabled")
+    __init_common(constants, "BBRMgr::Disabled: no bgp.bbr.enabled in the constants", None, {}, "disabled")
 
 def test___init_4():
     constants = deepcopy(global_constants)
     constants["bgp"]["bbr"] = { "enabled" : False }
-    __init_common(constants, "BBRMgr::Disabled: not enabled in the constants", None, {}, "disabled")
+    __init_common(constants, "BBRMgr::Disabled: no bgp.bbr.enabled in the constants", None, {}, "disabled")
 
 def test___init_5():
     constants = deepcopy(global_constants)
@@ -162,7 +157,35 @@ def test___init_6():
             "bbr": expected_bbr_entries,
         }
     }
-    __init_common(constants, 'BBRMgr::Initialized and enabled', None, expected_bbr_entries, "enabled")
+    __init_common(constants, "BBRMgr::Initialized and enabled. Default state: 'disabled'", None, expected_bbr_entries, "disabled")
+
+def test___init_7():
+    expected_bbr_entries = {
+                "PEER_V4": ["ipv4"],
+                "PEER_V6": ["ipv6"],
+    }
+    constants = deepcopy(global_constants)
+    constants["bgp"]["bbr"] = { "enabled" : True, "default_state": "disabled" }
+    constants["bgp"]["peers"] = {
+        "general": {
+            "bbr": expected_bbr_entries,
+        }
+    }
+    __init_common(constants, "BBRMgr::Initialized and enabled. Default state: 'disabled'", None, expected_bbr_entries, "disabled")
+
+def test___init_8():
+    expected_bbr_entries = {
+                "PEER_V4": ["ipv4"],
+                "PEER_V6": ["ipv6"],
+    }
+    constants = deepcopy(global_constants)
+    constants["bgp"]["bbr"] = { "enabled" : True, "default_state": "enabled" }
+    constants["bgp"]["peers"] = {
+        "general": {
+            "bbr": expected_bbr_entries,
+        }
+    }
+    __init_common(constants, "BBRMgr::Initialized and enabled. Default state: 'enabled'", None, expected_bbr_entries, "enabled")
 
 @patch('bgpcfgd.managers_bbr.log_info')
 def read_pgs_common(constants, expected_log_info, expected_bbr_enabled_pgs, mocked_log_info):
@@ -230,7 +253,7 @@ def test___set_validation_4():
 def test___set_validation_5():
     __set_validation_common("all", {"status": "disabled"}, None, True)
 
-def __set_prepare_config_common(status, bbr_enabled_pgs, expected_cmds):
+def __set_prepare_config_common(status, bbr_enabled_pgs, available_pgs, expected_cmds):
     cfg_mgr = MagicMock()
     common_objs = {
         'directory': Directory(),
@@ -247,6 +270,7 @@ def __set_prepare_config_common(status, bbr_enabled_pgs, expected_cmds):
         }
     }
     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)
     assert cmds == expected_cmds
 
@@ -254,7 +278,7 @@ def test___set_prepare_config_enabled():
     __set_prepare_config_common("enabled", {
                 "PEER_V4": ["ipv4", "ipv6"],
                 "PEER_V6": ["ipv6"],
-        }, [
+        }, {"PEER_V4", "PEER_V6"}, [
         'router bgp 65500',
         ' address-family ipv4',
         '  neighbor PEER_V4 allowas-in 1',
@@ -267,7 +291,7 @@ def test___set_prepare_config_disabled():
     __set_prepare_config_common("disabled", {
                 "PEER_V4": ["ipv4", "ipv6"],
                 "PEER_V6": ["ipv6"],
-        }, [
+        }, {"PEER_V4", "PEER_V6"}, [
         'router bgp 65500',
         ' address-family ipv4',
         '  no neighbor PEER_V4 allowas-in 1',
@@ -276,6 +300,64 @@ def test___set_prepare_config_disabled():
         '  no neighbor PEER_V6 allowas-in 1',
     ])
 
+def test___set_prepare_config_enabled_part():
+    __set_prepare_config_common("enabled", {
+                "PEER_V4": ["ipv4", "ipv6"],
+                "PEER_V6": ["ipv6"],
+                "PEER_V8": ["ipv4"]
+        }, {"PEER_V4", "PEER_V6"}, [
+        'router bgp 65500',
+        ' address-family ipv4',
+        '  neighbor PEER_V4 allowas-in 1',
+        ' address-family ipv6',
+        '  neighbor PEER_V4 allowas-in 1',
+        '  neighbor PEER_V6 allowas-in 1',
+    ])
+
+def test___set_prepare_config_disabled_part():
+    __set_prepare_config_common("disabled", {
+                "PEER_V4": ["ipv4", "ipv6"],
+                "PEER_V6": ["ipv6"],
+                "PEER_v10": ["ipv4"],
+        }, {"PEER_V4", "PEER_V6"}, [
+        'router bgp 65500',
+        ' address-family ipv4',
+        '  no neighbor PEER_V4 allowas-in 1',
+        ' address-family ipv6',
+        '  no neighbor PEER_V4 allowas-in 1',
+        '  no neighbor PEER_V6 allowas-in 1',
+    ])
+
+
+def test__get_available_peer_groups():
+    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.cfg_mgr.get_text = MagicMock(return_value=[
+        '  neighbor PEER_V4 peer-group',
+        '  neighbor PEER_V6 peer-group',
+        '  address-family ipv4',
+        '    neighbor PEER_V4 allowas-in 1',
+        '    neighbor PEER_V4 soft-reconfiguration inbound',
+        '    neighbor PEER_V4 route-map FROM_BGP_PEER_V4 in',
+        '    neighbor PEER_V4 route-map TO_BGP_PEER_V4 out',
+        '  exit-address-family',
+        '  address-family ipv6',
+        '    neighbor PEER_V6 allowas-in 1',
+        '    neighbor PEER_V6 soft-reconfiguration inbound',
+        '    neighbor PEER_V6 route-map FROM_BGP_PEER_V6 in',
+        '    neighbor PEER_V6 route-map TO_BGP_PEER_V6 out',
+        '  exit-address-family',
+        '     ',
+    ])
+    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()