From 5905404093985f08c023a724cc69e098497faddc Mon Sep 17 00:00:00 2001 From: anish-n Date: Sat, 21 Mar 2026 01:05:43 +0000 Subject: [PATCH 1/3] Add additional loopback consumption logic Signed-off-by: anish-n --- src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py | 55 ++++++++++++++++------- 1 file changed, 40 insertions(+), 15 deletions(-) diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py b/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py index b9d3f514a85..4718447ddfb 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py @@ -97,6 +97,8 @@ def __init__(self, common_objs, db_name, table_name, peer_type, check_neig_meta) self.constants = self.common_objs["constants"] self.fabric = common_objs['tf'] self.peer_type = peer_type + self.loopbacks = ["Loopback0"] + self.post_dependencies_init_complete = False base_template = "bgpd/templates/" + self.constants["bgp"]["peers"][peer_type]["template_dir"] + "/" self.templates = { @@ -175,23 +177,20 @@ def add_peer(self, vrf, nbr, data): :param data: associated data :return: True if this adding was successful, False otherwise """ - print_data = vrf, nbr, data - bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"] - # - lo0_ipv4 = self.get_lo_ipv4("Loopback0|") - if (lo0_ipv4 is None and "bgp_router_id" - not in self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]): - log_warn("Loopback0 ipv4 address is not presented yet and bgp_router_id not configured") - return False - # - if self.peer_type == 'internal': - lo4096_ipv4 = self.get_lo_ipv4("Loopback4096|") - if (lo4096_ipv4 is None and "bgp_router_id" + if not self.post_dependencies_init_complete: + self.post_dependencies_init() + + for loopback in self.loopbacks: + lo_ipv4 = self.get_lo_ipv4(loopback + "|") + if (lo_ipv4 is None and "bgp_router_id" not in self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]): - log_warn("Loopback4096 ipv4 address is not presented yet and bgp_router_id not configured") + log_warn(loopback + " ipv4 address is not presented yet and bgp_router_id not configured") return False + print_data = vrf, nbr, data + bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"] + if "local_addr" not in data: log_warn("Peer %s. Missing attribute 'local_addr'" % nbr) else: @@ -213,6 +212,8 @@ def add_peer(self, vrf, nbr, data): 'CONFIG_DB__LOOPBACK_INTERFACE':{ tuple(key.split('|')) : {} for key in self.directory.get_slot("CONFIG_DB", swsscommon.CFG_LOOPBACK_INTERFACE_TABLE_NAME) if '|' in key } } + + lo0_ipv4 = self.get_lo_ipv4("Loopback0|") if lo0_ipv4 is not None: kwargs['loopback0_ipv4'] = lo0_ipv4 if self.check_neig_meta: @@ -241,6 +242,30 @@ def add_peer(self, vrf, nbr, data): self.directory.put(self.db_name, self.table_name, vrf + '|' + nbr, data) return True + def post_dependencies_init(self): + base_template = "bgpd/templates/" + self.constants["bgp"]["peers"][self.peer_type]["template_dir"] + "/" + if (os.path.exists(self.fabric.env.loader.searchpath[0] + "/" + base_template + "additional_loopbacks.conf.j2")): + kwargs = { + 'CONFIG_DB__DEVICE_METADATA': self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME) + } + try: + rendered_loopbacks = self.fabric.from_file(base_template + "additional_loopbacks.conf.j2").render(**kwargs) + except jinja2.TemplateError as e: + msg = "Error in rendering the template additional_loopbnacks.conf.j2" + log_err("%s: %s" % (msg, str(e))) + return + + for loopback in rendered_loopbacks.splitlines(): + loopback_name = loopback.strip() + if loopback_name and loopback_name not in self.loopbacks: + self.loopbacks.append(loopback_name) + + log_info("Additional loopbacks acquired for peer %s, loopback list %s" % (self.peer_type, self.loopbacks)) + else: + log_info("No additional loopbacks acquired for peer %s, loopback list %s" %(self.peer_type, self.loopbacks)) + + self.post_dependencies_init_complete = True + def update_state_db(self, vrf, nbr, data, op): """ Update the database with the new data @@ -482,8 +507,8 @@ def apply_op(self, cmd, vrf): def get_lo_ipv4(self, loopback_str): """ - Extract Loopback0 ipv4 address from the Directory - :return: ipv4 address for Loopback0, None if nothing found + Extract LoopbackX ipv4 address from the Directory + :return: ipv4 address for LoopbackX, None if nothing found """ loopback0_ipv4 = None for loopback in self.directory.get_slot("CONFIG_DB", swsscommon.CFG_LOOPBACK_INTERFACE_TABLE_NAME).keys(): From 45604d9a4c21008b9f32a32cf49029a5f8fc50bc Mon Sep 17 00:00:00 2001 From: anish-n Date: Tue, 24 Mar 2026 01:25:44 +0000 Subject: [PATCH 2/3] Add UT and additional_loopback for internal peer Signed-off-by: anish-n --- .../templates/internal/additional_loopbacks.conf.j2 | 1 + src/sonic-bgpcfgd/tests/test_bgp.py | 10 ++++++++-- 2 files changed, 9 insertions(+), 2 deletions(-) create mode 100644 dockers/docker-fpm-frr/frr/bgpd/templates/internal/additional_loopbacks.conf.j2 diff --git a/dockers/docker-fpm-frr/frr/bgpd/templates/internal/additional_loopbacks.conf.j2 b/dockers/docker-fpm-frr/frr/bgpd/templates/internal/additional_loopbacks.conf.j2 new file mode 100644 index 00000000000..745178e4891 --- /dev/null +++ b/dockers/docker-fpm-frr/frr/bgpd/templates/internal/additional_loopbacks.conf.j2 @@ -0,0 +1 @@ +Loopback4096 diff --git a/src/sonic-bgpcfgd/tests/test_bgp.py b/src/sonic-bgpcfgd/tests/test_bgp.py index e66c5621c4b..28bda36ca4f 100644 --- a/src/sonic-bgpcfgd/tests/test_bgp.py +++ b/src/sonic-bgpcfgd/tests/test_bgp.py @@ -115,11 +115,13 @@ def test_add_peer_internal(): res = m.set_handler("30.30.30.1", {'asn': '65200', 'holdtime': '180', 'keepalive': '60', 'local_addr': '30.30.30.30', 'name': 'TOR', 'nhopself': '0', 'rrclient': '0'}) assert res, "Expect True return value" -def test_add_peer_internal_no_router_id_no_lo4096(): +@patch('bgpcfgd.managers_bgp.log_info') +def test_add_peer_internal_no_router_id_no_lo4096(mocked_log_info): for constant in load_constant_files(): m = constructor(constant, peer_type="internal") res = m.set_handler("30.30.30.1", {'asn': '65200', 'holdtime': '180', 'keepalive': '60', 'local_addr': '30.30.30.30', 'name': 'TOR', 'nhopself': '0', 'rrclient': '0'}) assert not res, "Expect False return value" + mocked_log_info.assert_called_with("Additional loopbacks acquired for peer internal, loopback list ['Loopback0', 'Loopback4096']") def test_add_peer_internal_router_id(): for constant in load_constant_files(): @@ -137,11 +139,15 @@ def test_add_peer_router_id(): res = m.set_handler("30.30.30.1", {'asn': '65200', 'holdtime': '180', 'keepalive': '60', 'local_addr': '30.30.30.30', 'name': 'TOR', 'nhopself': '0', 'rrclient': '0'}) assert res, "Expect True return value" -def test_add_peer_without_lo_ipv4(): +@patch('bgpcfgd.managers_bgp.log_info') +@patch('bgpcfgd.managers_bgp.log_warn') +def test_add_peer_without_lo_ipv4(mocked_log_warn, mocked_log_info): for constant in load_constant_files(): m = constructor(constant, with_lo0_ipv4=False) res = m.set_handler("30.30.30.1", {'asn': '65200', 'holdtime': '180', 'keepalive': '60', 'local_addr': '30.30.30.30', 'name': 'TOR', 'nhopself': '0', 'rrclient': '0'}) assert not res, "Expect False return value" + mocked_log_info.assert_called_with("No additional loopbacks acquired for peer general, loopback list ['Loopback0']") + mocked_log_warn.assert_called_with("Loopback0 ipv4 address is not presented yet and bgp_router_id not configured") def test_add_peer_without_lo_ipv4_router_id(): for constant in load_constant_files(): From 25bf0dc060321b181210dbc743b50c141c46a793 Mon Sep 17 00:00:00 2001 From: anish-n Date: Thu, 26 Mar 2026 02:00:24 +0000 Subject: [PATCH 3/3] Incorporate ai review comments and improve logging logic Signed-off-by: anish-n --- src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py b/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py index 4718447ddfb..b727400d97b 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py @@ -243,6 +243,8 @@ def add_peer(self, vrf, nbr, data): return True def post_dependencies_init(self): + self.post_dependencies_init_complete = True #Skip retrying template render failures to not impact existing workflow + orig_lo_list_len = len(self.loopbacks) base_template = "bgpd/templates/" + self.constants["bgp"]["peers"][self.peer_type]["template_dir"] + "/" if (os.path.exists(self.fabric.env.loader.searchpath[0] + "/" + base_template + "additional_loopbacks.conf.j2")): kwargs = { @@ -251,7 +253,7 @@ def post_dependencies_init(self): try: rendered_loopbacks = self.fabric.from_file(base_template + "additional_loopbacks.conf.j2").render(**kwargs) except jinja2.TemplateError as e: - msg = "Error in rendering the template additional_loopbnacks.conf.j2" + msg = "Error in rendering the template " + base_template + "additional_loopbacks.conf.j2" log_err("%s: %s" % (msg, str(e))) return @@ -260,11 +262,11 @@ def post_dependencies_init(self): if loopback_name and loopback_name not in self.loopbacks: self.loopbacks.append(loopback_name) + if len(self.loopbacks) > orig_lo_list_len: log_info("Additional loopbacks acquired for peer %s, loopback list %s" % (self.peer_type, self.loopbacks)) else: - log_info("No additional loopbacks acquired for peer %s, loopback list %s" %(self.peer_type, self.loopbacks)) + log_info("No additional loopbacks acquired for peer %s, loopback list %s" % (self.peer_type, self.loopbacks)) - self.post_dependencies_init_complete = True def update_state_db(self, vrf, nbr, data, op): """