Skip to content

Commit

Permalink
[bgpcfgd]: Fixes for BBR (sonic-net#5956)
Browse files Browse the repository at this point in the history 
* 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>
  • Loading branch information
@pavel-shirshov @santhosh-kt
2 people authored and santhosh-kt committed Feb 25, 2021
1 parent 3686577 commit 2a0e9ef
Show file tree
Hide file tree
Showing 3 changed files with 124 additions and 19 deletions.
1 change: 1 addition & 0 deletions files/image_config/constants/constants.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ constants:
- "deny 0::/0 ge 65"
bbr:
enabled: true
default_state: "disabled"
peers:
general: # peer_type
db_table: "BGP_NEIGHBOR"
Expand Down
36 changes: 29 additions & 7 deletions src/sonic-bgpcfgd/bgpcfgd/managers_bbr.py
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import re

from swsscommon import swsscommon

from .log import log_err, log_info, log_crit
Expand Down Expand Up @@ -49,18 +51,23 @@ def __init(self):
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):
"""
Expand Down Expand Up @@ -102,15 +109,30 @@ def __set_prepare_config(self, status):
: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()):
Expand Down
106 changes: 94 additions & 12 deletions src/sonic-bgpcfgd/tests/test_bbr.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,6 @@
}
}

#@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 = {
Expand Down Expand Up @@ -123,8 +120,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:
Expand All @@ -135,17 +130,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)
Expand All @@ -164,7 +159,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):
Expand Down Expand Up @@ -232,7 +255,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(),
Expand All @@ -249,14 +272,15 @@ 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

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',
Expand All @@ -269,7 +293,35 @@ 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',
' address-family ipv6',
' no neighbor PEER_V4 allowas-in 1',
' 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',
Expand All @@ -278,6 +330,36 @@ def test___set_prepare_config_disabled():
' 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()
Expand Down

0 comments on commit 2a0e9ef

Please sign in to comment.