From 7b88a427b183e3897c2d1ab75218fd26b83c771f Mon Sep 17 00:00:00 2001 From: zitingguo-ms Date: Fri, 16 Aug 2024 08:54:04 +0800 Subject: [PATCH] Prioritize configuration from config_db over constants.yml during BBR status initialization (#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' --- src/sonic-bgpcfgd/bgpcfgd/managers_bbr.py | 71 ++++++++++++++++++----- src/sonic-bgpcfgd/tests/test_bbr.py | 32 +++++++++- 2 files changed, 85 insertions(+), 18 deletions(-) diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_bbr.py b/src/sonic-bgpcfgd/bgpcfgd/managers_bbr.py index 0e82d0a4b..6e1a33e89 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_bbr.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_bbr.py @@ -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): """ @@ -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 diff --git a/src/sonic-bgpcfgd/tests/test_bbr.py b/src/sonic-bgpcfgd/tests/test_bbr.py index b11277bae..8bb67e998 100644 --- a/src/sonic-bgpcfgd/tests/test_bbr.py +++ b/src/sonic-bgpcfgd/tests/test_bbr.py @@ -111,6 +111,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 @@ -156,7 +157,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 = { @@ -170,7 +171,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 = { @@ -184,7 +185,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):