From 5f5ec04ddac11ad469deb65f962b3dc6d5601a18 Mon Sep 17 00:00:00 2001 From: pavel-shirshov Date: Thu, 19 Nov 2020 00:07:58 -0800 Subject: [PATCH] [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 --- files/image_config/constants/constants.yml | 1 + src/sonic-bgpcfgd/bgpcfgd/managers_bbr.py | 36 +++++-- src/sonic-bgpcfgd/tests/test_bbr.py | 106 ++++++++++++++++++--- 3 files changed, 124 insertions(+), 19 deletions(-) diff --git a/files/image_config/constants/constants.yml b/files/image_config/constants/constants.yml index ac4b521865..86179aee43 100644 --- a/files/image_config/constants/constants.yml +++ b/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" diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_bbr.py b/src/sonic-bgpcfgd/bgpcfgd/managers_bbr.py index 5c03eabf4b..7adfd203f4 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_bbr.py +++ b/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 \ - 'enabled' in self.constants['bgp']['bbr'] and \ - self.constants['bgp']['bbr']['enabled']: + if 'bbr' in self.constants['bgp'] \ + and 'enabled' in self.constants['bgp']['bbr'] \ + 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") - log_info("BBRMgr::Initialized and enabled") + if 'default_state' in self.constants['bgp']['bbr'] \ + 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()): diff --git a/src/sonic-bgpcfgd/tests/test_bbr.py b/src/sonic-bgpcfgd/tests/test_bbr.py index 5f95d12acf..86ca2ee20e 100644 --- a/src/sonic-bgpcfgd/tests/test_bbr.py +++ b/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()