Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[staticroutebfd]fix an issue on deleting a non-bfd static route #15269

Merged
merged 5 commits into from
Jun 2, 2023
Merged

[staticroutebfd]fix an issue on deleting a non-bfd static route #15269

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1bf4dd8
[static_route][staticroutebfd]fix an issue on deleting a non-bfd stat…
baorliu May 31, 2023
a3bc84c
remove staticroutebfd from critical_processes and make it auto restart
baorliu May 31, 2023
64739f4
remove staticroutebfd from critical_processes and make it auto restart
baorliu May 31, 2023
b0e7c70
Merge branch 'cisco-baorliu-fix-srt-bfd' of https://github.com/baorli…
baorliu May 31, 2023
8a76f1f
[staticroute][bfd]add testcase for nexthop-vrf list
baorliu May 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion dockers/docker-fpm-frr/frr/supervisord/critical_processes.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,4 @@ program:pimd
program:frrcfgd
{%- else %}
program:bgpcfgd
program:staticroutebfd
Copy link
Contributor

@vivekrnv vivekrnv May 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But Isn't this a critical process? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

staticroutebfd itself supports recovery from restart/crash. So it is not necessary to set it to critical process. When a critical process crashes, the bgp container itself will restart, which has more impact. to reduce impact to the bgp container, removed it from critical process, and let supervisord restart staticroutebfd only (see change in supervisord.conf.j2 below).

{%- endif %}
2 changes: 1 addition & 1 deletion dockers/docker-fpm-frr/frr/supervisord/supervisord.conf.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
28 changes: 18 additions & 10 deletions src/sonic-bgpcfgd/staticroutebfd/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why aren't we removing the static route if bfd not enabled? What was the previous behavior? Does this mean if the user removes static route via CLI, won't it get removed from APP_DB?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If BFD is not enabled, staticroutebfd does not write static route entry to appl_db. so it should not delete it from appl_db. In general, prefix in config_db should not in appl_db. but to be safe, it is better that staticroutebfd does not touch that route in appl_db if it is not created by staticroutebfd.

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:
Expand Down
23 changes: 22 additions & 1 deletion src/sonic-bgpcfgd/tests/test_static_rt_bfd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
from unittest.mock import patch
#from unittest.mock import MagicMock, patch

from staticroutebfd.main import *
from swsscommon import swsscommon
Expand Down Expand Up @@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also suggest adding a testcase which has nexthop-vrf parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new testcases are added for nexthop-vrf list. thanks

"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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fv-pairs for del notification will be empty

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove that to prevent DUT using these values in case

"ifname": "if1, if2",
}),
{},
{}
)

def test_bfd_del():
dut = constructor()
intf_setup(dut)
Expand Down