Skip to content

Commit

Permalink
Prioritize configuration from config_db over constants.yml during BBR…
Browse files Browse the repository at this point in the history 
… status initialization (sonic-net#19590)

Why I did it
There are two places which we can configure the BBR disabled/enabled, one is constants.yml, the other is by config_db. During run time, config_db win. But at the init stage, constants.yml win. This is a wrong logic, constants.yml only win when there is no such config in the config_db.

Work item tracking
Microsoft ADO (number only): 28302790
How I did it
Initialize BBR status from constants.yml only when config_db doesn't have BBR entry.

How to verify it
Build image and run the following:

# bbr status in constants.yml is set to disabled
# change the bbr status in config_db to enabled and reload config
admin@str2-7050cx3-acs-01:~$ redis-cli -n 4 hset "BGP_BBR|all" "status" "enabled"
(integer) 0
admin@str2-7050cx3-acs-01:~$ redis-cli -n 4 hget "BGP_BBR|all" "status"
"enabled"
admin@str2-7050cx3-acs-01:~$ vtysh -c 'show run' | grep 'allowas'
  neighbor PEER_V4 allowas-in 1
  neighbor PEER_V6 allowas-in 1
admin@str2-7050cx3-acs-01:~$ sudo config save -y
Running command: /usr/local/bin/sonic-cfggen -d --print-data > /etc/sonic/config_db.json
admin@str2-7050cx3-acs-01:~$ sudo config reload -y

# check bgpcfgd log, bbr default status is enabled
2024 Aug 14 05:42:47.653216 str2-7050cx3-acs-01 INFO bgp#bgpcfgd: BBRMgr::Initialized and enabled from config_db. Default state: 'enabled'
  • Loading branch information
@Gfrom2016
Gfrom2016 authored Aug 16, 2024
1 parent 7480022 commit 084ba1a
Show file tree
Hide file tree
Showing 2 changed files with 85 additions and 18 deletions.
71 changes: 56 additions & 15 deletions src/sonic-bgpcfgd/bgpcfgd/managers_bbr.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,26 +45,38 @@ def del_handler(self, key):

def __init(self):
""" Initialize BBRMgr. Extracted from constructor """
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']:
# Check BGP_BBR table from config_db first
bbr_status_from_config_db = self.get_bbr_status_from_config_db()

if bbr_status_from_config_db is None:
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']:
self.bbr_enabled_pgs = self.__read_pgs()
if self.bbr_enabled_pgs:
self.enabled = True
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 from constants. Default state: '%s'" % default_status)
else:
log_info("BBRMgr::Disabled: no BBR enabled peers")
else:
log_info("BBRMgr::Disabled: no bgp.bbr.enabled in the constants")
else:
self.bbr_enabled_pgs = self.__read_pgs()
if self.bbr_enabled_pgs:
self.enabled = True
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)
self.directory.put(self.db_name, self.table_name, 'status', bbr_status_from_config_db)
log_info("BBRMgr::Initialized and enabled from config_db. Default state: '%s'" % bbr_status_from_config_db)
else:
log_info("BBRMgr::Disabled: no BBR enabled peers")
else:
log_info("BBRMgr::Disabled: no bgp.bbr.enabled in the constants")

def __read_pgs(self):
"""
Expand All @@ -82,6 +94,35 @@ def __read_pgs(self):
res[pg_name] = pg_afs
return res

def get_bbr_status_from_config_db(self):
"""
Read BBR status from CONFIG_DB
:return: BBR status from CONFIG_DB or None if not found
"""
try:
config_db = swsscommon.ConfigDBConnector()
if config_db is None:
log_info("BBRMgr::Failed to connect to CONFIG_DB, get BBR default state from constants.yml")
return None
config_db.connect()
except Exception as e:
log_info("BBRMgr::Failed to connect to CONFIG_DB with exception %s, get BBR default state from constants.yml" % str(e))
return None

try:
bbr_table_data = config_db.get_table(self.table_name)
if bbr_table_data and 'all' in bbr_table_data and 'status' in bbr_table_data["all"]:
if bbr_table_data["all"]["status"] == "enabled":
return "enabled"
else:
return "disabled"
else:
log_info("BBRMgr::BBR status is not found in CONFIG_DB, get BBR default state from constants.yml")
return None
except Exception as e:
log_info("BBRMgr::Failed to read BBR status from CONFIG_DB with exception %s, get BBR default state from constants.yml" % str(e))
return None

def __set_validation(self, key, data):
""" Validate set-command arguments
:param key: key of 'set' command
Expand Down
32 changes: 29 additions & 3 deletions src/sonic-bgpcfgd/tests/test_bbr.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ def __init_common(constants,
'tf': TemplateFabric(),
'constants': constants,
}

m = BBRMgr(common_objs, "CONFIG_DB", "BGP_BBR")
m._BBRMgr__init()
assert m.bbr_enabled_pgs == expected_bbr_enabled_pgs
Expand Down Expand Up @@ -157,7 +158,7 @@ def test___init_6():
"bbr": expected_bbr_entries,
}
}
__init_common(constants, "BBRMgr::Initialized and enabled. Default state: 'disabled'", None, expected_bbr_entries, "disabled")
__init_common(constants, "BBRMgr::Initialized and enabled from constants. Default state: 'disabled'", None, expected_bbr_entries, "disabled")

def test___init_7():
expected_bbr_entries = {
Expand All @@ -171,7 +172,7 @@ def test___init_7():
"bbr": expected_bbr_entries,
}
}
__init_common(constants, "BBRMgr::Initialized and enabled. Default state: 'disabled'", None, expected_bbr_entries, "disabled")
__init_common(constants, "BBRMgr::Initialized and enabled from constants. Default state: 'disabled'", None, expected_bbr_entries, "disabled")

def test___init_8():
expected_bbr_entries = {
Expand All @@ -185,7 +186,32 @@ def test___init_8():
"bbr": expected_bbr_entries,
}
}
__init_common(constants, "BBRMgr::Initialized and enabled. Default state: 'enabled'", None, expected_bbr_entries, "enabled")
__init_common(constants, "BBRMgr::Initialized and enabled from constants. Default state: 'enabled'", None, expected_bbr_entries, "enabled")

@patch('bgpcfgd.managers_bbr.BBRMgr.get_bbr_status_from_config_db', return_value='disabled')
def test___init_with_config_db_overwirte_constants(mocked_get_bbr_status_from_config_db):
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,
}
}

# BBR status from config_db should be prioritized over constants
__init_common(constants, "BBRMgr::Initialized and enabled from config_db. Default state: 'disabled'", None, expected_bbr_entries, "disabled")

@patch('bgpcfgd.managers_bbr.BBRMgr.get_bbr_status_from_config_db', return_value='enabled')
def test___init_with_config_db_no_peers(mocked_get_bbr_status_from_config_db):

constants = deepcopy(global_constants)
constants["bgp"]["bbr"] = {"enabled": True}

__init_common(constants, "BBRMgr::Disabled: no BBR enabled peers", None, {}, "disabled")

@patch('bgpcfgd.managers_bbr.log_info')
def read_pgs_common(constants, expected_log_info, expected_bbr_enabled_pgs, mocked_log_info):
Expand Down

0 comments on commit 084ba1a

Please sign in to comment.