From 1bf4dd82ace7d1f7285b1fb53205d0cbddd39e5a Mon Sep 17 00:00:00 2001 From: Baorong Liu Date: Tue, 30 May 2023 19:27:39 -0700 Subject: [PATCH 1/4] [static_route][staticroutebfd]fix an issue on deleting a non-bfd static route --- src/sonic-bgpcfgd/staticroutebfd/main.py | 28 ++++++++++++------- src/sonic-bgpcfgd/tests/test_static_rt_bfd.py | 23 ++++++++++++++- 2 files changed, 40 insertions(+), 11 deletions(-) diff --git a/src/sonic-bgpcfgd/staticroutebfd/main.py b/src/sonic-bgpcfgd/staticroutebfd/main.py index 4483d0589cca..e67a76bff7bd 100644 --- a/src/sonic-bgpcfgd/staticroutebfd/main.py +++ b/src/sonic-bgpcfgd/staticroutebfd/main.py @@ -502,18 +502,26 @@ def static_route_del_handler(self, key, redis_del): arg_list = lambda v: [x.strip() for x in v.split(',')] if len(v.strip()) != 0 else None nh_list = arg_list(data['nexthop']) if 'nexthop' in data else None nh_vrf_list = arg_list(data['nexthop-vrf']) if 'nexthop-vrf' in data else None - for index in range(len(nh_list)): - nh_ip = nh_list[index] - nh_vrf = nh_vrf_list[index] - nh_key = nh_vrf + "|" + nh_ip - self.remove_from_nh_table_entry(nh_key, route_cfg_key) + bfd_field = arg_list(data['bfd']) if 'bfd' in data else ["false"] + bfd_enabled = self.isFieldTrue(bfd_field) + + # for a bfd_enabled static route, the nh_vrf_list was processed, has same length with nh_list + if bfd_enabled and nh_list and nh_vrf_list and len(nh_list) == len(nh_vrf_list): + for index in range(len(nh_list)): + nh_ip = nh_list[index] + nh_vrf = nh_vrf_list[index] + nh_key = nh_vrf + "|" + nh_ip + self.remove_from_nh_table_entry(nh_key, route_cfg_key) + + if len(self.get_local_db(LOCAL_NEXTHOP_TABLE, nh_key)) == 0: + bfd_key = nh_vrf + ":default:" + nh_ip + self.remove_from_local_db(LOCAL_BFD_TABLE, bfd_key) + self.del_bfd_session_from_appl_db(bfd_key) - if len(self.get_local_db(LOCAL_NEXTHOP_TABLE, nh_key)) == 0: - bfd_key = nh_vrf + ":default:" + nh_ip - self.remove_from_local_db(LOCAL_BFD_TABLE, bfd_key) - self.del_bfd_session_from_appl_db(bfd_key) + # do not delete it from appl_db if the route is not bfd enabled + if bfd_enabled: + self.del_static_route_from_appl_db(route_cfg_key.replace("|", ":")) - self.del_static_route_from_appl_db(route_cfg_key.replace("|", ":")) self.remove_from_local_db(LOCAL_SRT_TABLE, route_cfg_key) if redis_del: diff --git a/src/sonic-bgpcfgd/tests/test_static_rt_bfd.py b/src/sonic-bgpcfgd/tests/test_static_rt_bfd.py index e2eb32cb85d5..748386089541 100644 --- a/src/sonic-bgpcfgd/tests/test_static_rt_bfd.py +++ b/src/sonic-bgpcfgd/tests/test_static_rt_bfd.py @@ -1,5 +1,4 @@ from unittest.mock import patch -#from unittest.mock import MagicMock, patch from staticroutebfd.main import * from swsscommon import swsscommon @@ -169,6 +168,28 @@ def test_set_del(): {'del_default:2.2.2.0/24': {}} ) + # test add a non-bfd static route + set_del_test(dut, "srt", + "SET", + ("3.3.3.0/24", { + "nexthop": "192.168.1.2 , 192.168.2.2", + "ifname": "if1, if2", + }), + {}, + {} + ) + + # test delete a non-bfd static route + set_del_test(dut, "srt", + "DEL", + ("3.3.3.0/24", { + "nexthop": "192.168.1.2 , 192.168.2.2", + "ifname": "if1, if2", + }), + {}, + {} + ) + def test_bfd_del(): dut = constructor() intf_setup(dut) From a3bc84c156da9d7d0317d96de28db4e3d13256ea Mon Sep 17 00:00:00 2001 From: Baorong Liu Date: Tue, 30 May 2023 23:18:31 -0700 Subject: [PATCH 2/4] remove staticroutebfd from critical_processes and make it auto restart --- dockers/docker-fpm-frr/frr/supervisord/critical_processes.j2 | 1 - dockers/docker-fpm-frr/frr/supervisord/supervisord.conf.j2 | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/dockers/docker-fpm-frr/frr/supervisord/critical_processes.j2 b/dockers/docker-fpm-frr/frr/supervisord/critical_processes.j2 index dad38888e883..69f4e8e6931e 100644 --- a/dockers/docker-fpm-frr/frr/supervisord/critical_processes.j2 +++ b/dockers/docker-fpm-frr/frr/supervisord/critical_processes.j2 @@ -9,5 +9,4 @@ program:pimd program:frrcfgd {%- else %} program:bgpcfgd -program:staticroutebfd {%- endif %} diff --git a/dockers/docker-fpm-frr/frr/supervisord/supervisord.conf.j2 b/dockers/docker-fpm-frr/frr/supervisord/supervisord.conf.j2 index 584e7fdc2e51..f6edc17a5529 100644 --- a/dockers/docker-fpm-frr/frr/supervisord/supervisord.conf.j2 +++ b/dockers/docker-fpm-frr/frr/supervisord/supervisord.conf.j2 @@ -147,7 +147,7 @@ dependent_startup_wait_for=bgpd:running command=/usr/local/bin/staticroutebfd priority=6 autostart=false -autorestart=false +autorestart=true startsecs=0 stdout_logfile=syslog stderr_logfile=syslog From 64739f4feff11fd9c0a8acc75d8497358509e33b Mon Sep 17 00:00:00 2001 From: Baorong Liu Date: Tue, 30 May 2023 23:18:31 -0700 Subject: [PATCH 3/4] remove staticroutebfd from critical_processes and make it auto restart --- dockers/docker-fpm-frr/frr/supervisord/critical_processes.j2 | 1 - dockers/docker-fpm-frr/frr/supervisord/supervisord.conf.j2 | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/dockers/docker-fpm-frr/frr/supervisord/critical_processes.j2 b/dockers/docker-fpm-frr/frr/supervisord/critical_processes.j2 index dad38888e883..69f4e8e6931e 100644 --- a/dockers/docker-fpm-frr/frr/supervisord/critical_processes.j2 +++ b/dockers/docker-fpm-frr/frr/supervisord/critical_processes.j2 @@ -9,5 +9,4 @@ program:pimd program:frrcfgd {%- else %} program:bgpcfgd -program:staticroutebfd {%- endif %} diff --git a/dockers/docker-fpm-frr/frr/supervisord/supervisord.conf.j2 b/dockers/docker-fpm-frr/frr/supervisord/supervisord.conf.j2 index 584e7fdc2e51..f6edc17a5529 100644 --- a/dockers/docker-fpm-frr/frr/supervisord/supervisord.conf.j2 +++ b/dockers/docker-fpm-frr/frr/supervisord/supervisord.conf.j2 @@ -147,7 +147,7 @@ dependent_startup_wait_for=bgpd:running command=/usr/local/bin/staticroutebfd priority=6 autostart=false -autorestart=false +autorestart=true startsecs=0 stdout_logfile=syslog stderr_logfile=syslog From 8a76f1f1ddffe334e3ef70aed7d62500436baaf8 Mon Sep 17 00:00:00 2001 From: Baorong Liu Date: Wed, 31 May 2023 15:22:39 -0700 Subject: [PATCH 4/4] [staticroute][bfd]add testcase for nexthop-vrf list --- src/sonic-bgpcfgd/staticroutebfd/main.py | 17 +++-- src/sonic-bgpcfgd/tests/test_static_rt_bfd.py | 74 ++++++++++++++++++- 2 files changed, 83 insertions(+), 8 deletions(-) diff --git a/src/sonic-bgpcfgd/staticroutebfd/main.py b/src/sonic-bgpcfgd/staticroutebfd/main.py index e67a76bff7bd..e3b2ed10be30 100644 --- a/src/sonic-bgpcfgd/staticroutebfd/main.py +++ b/src/sonic-bgpcfgd/staticroutebfd/main.py @@ -400,6 +400,18 @@ def static_route_set_handler(self, key, data): self.handle_bfd_change(key, data_copy, True) data['bfd_nh_hold'] = "true" + # preprocess empty nexthop-vrf list before save to LOCAL_CONFIG_TABLE, bfd session need this information + nh_list = arg_list(data['nexthop']) if 'nexthop' in data else None + nh_vrf_list = arg_list(data['nexthop-vrf']) if 'nexthop-vrf' in data else None + if nh_vrf_list is None: + nh_vrf_list = [vrf] * len(nh_list) if len(nh_list) > 0 else None + data['nexthop-vrf'] = ','.join(nh_vrf_list) if nh_vrf_list else '' + else: # preprocess empty nexthop-vrf member + for index in range(len(nh_vrf_list)): + if len(nh_vrf_list[index]) == 0: + nh_vrf_list[index] = vrf + data['nexthop-vrf'] = ','.join(nh_vrf_list) + if not bfd_enabled: #skip if bfd is not enabled, but store it to local_db to detect bfd field dynamic change data['bfd'] = "false" @@ -407,13 +419,8 @@ def static_route_set_handler(self, key, data): return True bkh_list = arg_list(data['blackhole']) if 'blackhole' in data else None - nh_list = arg_list(data['nexthop']) if 'nexthop' in data else None intf_list = arg_list(data['ifname']) if 'ifname' in data else None dist_list = arg_list(data['distance']) if 'distance' in data else None - nh_vrf_list = arg_list(data['nexthop-vrf']) if 'nexthop-vrf' in data else None - if nh_vrf_list is None: - nh_vrf_list = [vrf] * len(nh_list) if len(nh_list) > 0 else None - data['nexthop-vrf'] = ','.join(nh_vrf_list) if nh_vrf_list else '' if intf_list is None or nh_list is None or nh_vrf_list is None or \ len(intf_list) != len(nh_list) or len(intf_list) != len(nh_vrf_list): log_err("Static route bfd set Failed, nexthop, interface and vrf lists do not match.") diff --git a/src/sonic-bgpcfgd/tests/test_static_rt_bfd.py b/src/sonic-bgpcfgd/tests/test_static_rt_bfd.py index 748386089541..0e4d62475988 100644 --- a/src/sonic-bgpcfgd/tests/test_static_rt_bfd.py +++ b/src/sonic-bgpcfgd/tests/test_static_rt_bfd.py @@ -1,4 +1,5 @@ from unittest.mock import patch +#from unittest.mock import MagicMock, patch from staticroutebfd.main import * from swsscommon import swsscommon @@ -182,12 +183,78 @@ def test_set_del(): # test delete a non-bfd static route set_del_test(dut, "srt", "DEL", - ("3.3.3.0/24", { + ("3.3.3.0/24", {}), + {}, + {} + ) + +def test_set_del_vrf(): + dut = constructor() + intf_setup(dut) + + set_del_test(dut, "srt", + "SET", + ("vrfred|2.2.2.0/24", { + "bfd": "true", + "nexthop": "192.168.1.2 , 192.168.2.2, 192.168.3.2", + "ifname": "if1, if2, if3", + "nexthop-vrf": "testvrf1, , default", + }), + { + "set_testvrf1:default:192.168.1.2" : {'multihop': 'true', 'rx_interval': '50', 'tx_interval': '50', 'multiplier': '3', 'local_addr': '192.168.1.1'}, + "set_vrfred:default:192.168.2.2" : {'multihop': 'true', 'rx_interval': '50', 'tx_interval': '50', 'multiplier': '3', 'local_addr': '192.168.2.1'}, + "set_default:default:192.168.3.2" : {'multihop': 'true', 'rx_interval': '50', 'tx_interval': '50', 'multiplier': '3', 'local_addr': '192.168.3.1'} + }, + {} + ) + + set_del_test(dut, "bfd", + "SET", + ("testvrf1|default|192.168.1.2", { + "state": "Up" + }), + {}, + {'set_vrfred:2.2.2.0/24': {'nexthop': '192.168.1.2', 'ifname': 'if1', 'nexthop-vrf': 'testvrf1', 'expiry': 'false'}} + ) + set_del_test(dut, "bfd", + "SET", + ("vrfred|default|192.168.2.2", { + "state": "Up" + }), + {}, + {'set_vrfred:2.2.2.0/24': {'nexthop': '192.168.2.2,192.168.1.2 ', 'ifname': 'if2,if1', 'nexthop-vrf': 'testvrf1,vrfred', 'expiry': 'false'}} + ) + set_del_test(dut, "bfd", + "SET", + ("default|default|192.168.3.2", { + "state": "Up" + }), + {}, + {'set_vrfred:2.2.2.0/24': {'nexthop': '192.168.2.2,192.168.1.2,192.168.3.2 ', 'ifname': 'if2,if1,if3', 'nexthop-vrf': 'testvrf1,vrfred,default', 'expiry': 'false'}} + ) + + set_del_test(dut, "srt", + "SET", + ("vrfred|2.2.2.0/24", { + "bfd": "true", "nexthop": "192.168.1.2 , 192.168.2.2", "ifname": "if1, if2", + "nexthop-vrf": "testvrf1,", }), - {}, - {} + { + "del_default:default:192.168.3.2" : {} + }, + {'set_vrfred:2.2.2.0/24': {'nexthop': '192.168.2.2,192.168.1.2 ', 'ifname': 'if2,if1', 'nexthop-vrf': 'vrfred, testvrf1', 'expiry': 'false'}} + ) + + set_del_test(dut, "srt", + "DEL", + ("vrfred|2.2.2.0/24", { }), + { + "del_testvrf1:default:192.168.1.2" : {}, + "del_vrfred:default:192.168.2.2" : {} + }, + {'del_vrfred:2.2.2.0/24': {}} ) def test_bfd_del(): @@ -535,3 +602,4 @@ def test_set_bfd_change_no_hold(): ) +