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

baorliu · 2023-05-31T02:39:17Z

What I did it

Fix an issue for deleting a non-bfd static route
also remove the staticroutebfd from critical_processes list and make it auto restart in the case of crash.

fixes #15267

Why I did it

Current design access "None" object (nh_vrf_list) when delete a non-bfd static route

For supervisord, because staticroutebfd support restart and recovery. so it is not necessary to restart bgp container if there is any crash inside staticroutebfd. supervisord will restart it after crash.

Work item tracking

Microsoft ADO: https://msazure.visualstudio.com/One/_workitems/edit/17793093

How I did it

check if the route is BFD protected when delete this static route, and check if the nh_vrf_list is None.

How to verify it

add a UT deleting a non-bfd static route to reproduce the issue:

self = <staticroutebfd.main.StaticRouteBfd object at 0x7f2f1add8b80>, key = '3.3.3.0/24', redis_del = {'ifname': 'if1, if2', 'nexthop': '192.168.1.2 , 192.168.2.2'}

    def static_route_del_handler(self, key, redis_del):
        valid, vrf, ip_prefix = static_route_split_key(key)
        if not valid:
            return True
        route_cfg_key = vrf + "|" + ip_prefix
    
        valid, is_ipv4, ip = check_ip(ip_prefix)
        if not valid:
            return True
    
        data = self.get_local_db(LOCAL_CONFIG_TABLE, route_cfg_key)
        if len(data) == 0:
            # this route is not handled by StaticRouteBfd, skip
            return True
    
        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]
E           TypeError: 'NoneType' object is not subscriptable

staticroutebfd/main.py:507: TypeError

fix the issue and run the UT:

staticroutebfd/__init__.py              0      0   100%
staticroutebfd/main.py                577    204    65%
staticroutebfd/vars.py                  5      0   100%
-------------------------------------------------------
TOTAL                                2367   1689    29%

================================================================================================ 32 passed in 0.74s =================================================================================================

For supervisord, after changed the critical_processes.j2 and supervisord.conf.j2, restart bgp (sudo systemctl restart bgp),
manually kill the staticroutebfd process inside bgp container, it can be restarted without restart the whole bgp container.

Which release branch to backport (provide reason below if selected)

Tested branch (Please provide the tested image version)

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

…ic route

dgsudharsan · 2023-05-31T03:51:02Z

src/sonic-bgpcfgd/staticroutebfd/main.py

-                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:


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?

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.

…u/sonic-buildimage into cisco-baorliu-fix-srt-bfd

vivekrnv · 2023-05-31T05:05:39Z

src/sonic-bgpcfgd/tests/test_static_rt_bfd.py

+    set_del_test(dut, "srt",
+        "DEL",
+        ("3.3.3.0/24", {
+            "nexthop": "192.168.1.2 , 192.168.2.2",


fv-pairs for del notification will be empty

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

vivekrnv · 2023-05-31T05:06:40Z

src/sonic-bgpcfgd/tests/test_static_rt_bfd.py

@@ -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",


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

new testcases are added for nexthop-vrf list. thanks

vivekrnv · 2023-05-31T16:20:26Z

dockers/docker-fpm-frr/frr/supervisord/critical_processes.j2

@@ -9,5 +9,4 @@ program:pimd
 program:frrcfgd
 {%- else %}
 program:bgpcfgd
-program:staticroutebfd


But Isn't this a critical process? #Resolved

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).

dgsudharsan · 2023-06-02T18:33:56Z

@prsunny Can we merge this?

abdosi · 2023-06-07T00:30:23Z

@gechiang needed for msft repo 202205 branch

abdosi · 2023-06-07T23:32:24Z

@baorliu can you create direct PR for 202205 branch on msft repo ?

That way we need not have to cherry-pick.

cc @anamehra @gechiang

Double commit the PRs from sonic-net/sonic-buildimage. Original PR for staticroutebfd: sonic-net/sonic-buildimage#13789 And a fix for staticroutebfd: sonic-net/sonic-buildimage#15269

…c-net#15269) * [static_route][staticroutebfd]fix an issue on deleting a non-bfd static route Fix an issue for deleting a non-bfd static route also remove the staticroutebfd from critical_processes list and make it auto restart in the case of crash.

…c-buildimage

[static_route][staticroutebfd]fix an issue on deleting a non-bfd stat…

1bf4dd8

…ic route

dgsudharsan suggested changes May 31, 2023

View reviewed changes

dgsudharsan previously approved these changes May 31, 2023

View reviewed changes

dgsudharsan added Request for 202205 Branch and removed Request for 202205 Branch labels May 31, 2023

dgsudharsan mentioned this pull request May 31, 2023

[202205]implement staticroutebfd process #13764

Open

8 tasks

baorliu dismissed dgsudharsan’s stale review via a825447 May 31, 2023 06:19

baorliu added 3 commits May 30, 2023 23:23

remove staticroutebfd from critical_processes and make it auto restart

a3bc84c

remove staticroutebfd from critical_processes and make it auto restart

64739f4

Merge branch 'cisco-baorliu-fix-srt-bfd' of https://github.com/baorli…

b0e7c70

…u/sonic-buildimage into cisco-baorliu-fix-srt-bfd

baorliu marked this pull request as ready for review May 31, 2023 16:14

baorliu requested review from StormLiangMS and lguohan as code owners May 31, 2023 16:14

vivekrnv suggested changes May 31, 2023

View reviewed changes

vivekrnv reviewed May 31, 2023

View reviewed changes

[staticroute][bfd]add testcase for nexthop-vrf list

8a76f1f

prsunny mentioned this pull request Jun 1, 2023

staticroutebfd fails to handle static route del notification #15267

Closed

vivekrnv approved these changes Jun 1, 2023

View reviewed changes

dgsudharsan approved these changes Jun 2, 2023

View reviewed changes

prsunny approved these changes Jun 2, 2023

View reviewed changes

prsunny merged commit acb423b into sonic-net:master Jun 2, 2023

abdosi added Request for 202205 Branch Request for 202305 Branch Request for 202211 Branch Chassis for 202205 branch PRs needed for 202205 branch in msft repo and removed Request for 202205 Branch labels Jun 3, 2023

baorliu mentioned this pull request Jun 8, 2023

[master]staticroutebfd process implementation #13789

Merged

8 tasks

baorliu mentioned this pull request Jun 9, 2023

Double commit #13789 and #15269 from sonic-net/sonic-buildimage sonic-net/sonic-buildimage-msft#30

Merged

5 tasks

gechiang added the Included in Chassis for 202205 Branch Indicate PR is already in MSFT repo 202205 branch label Jun 12, 2023

StormLiangMS added the Included in 202305 Branch label Jun 23, 2023

mlok-nokia pushed a commit to mlok-nokia/sonic-buildimage that referenced this pull request Jun 5, 2024

double commit sonic-net#13789 and sonic-net#15269 from sonic-net/soni…

481a878

…c-buildimage

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

baorliu commented May 31, 2023 •

edited by abdosi

Loading

dgsudharsan May 31, 2023

baorliu May 31, 2023

vivekrnv May 31, 2023

baorliu May 31, 2023

vivekrnv May 31, 2023

baorliu May 31, 2023

vivekrnv May 31, 2023 •

edited

Loading

baorliu May 31, 2023

dgsudharsan commented Jun 2, 2023

abdosi commented Jun 7, 2023

abdosi commented Jun 7, 2023

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

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

Conversation

baorliu commented May 31, 2023 • edited by abdosi Loading

What I did it

Why I did it

Work item tracking

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

Tested branch (Please provide the tested image version)

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

dgsudharsan May 31, 2023

Choose a reason for hiding this comment

baorliu May 31, 2023

Choose a reason for hiding this comment

vivekrnv May 31, 2023

Choose a reason for hiding this comment

baorliu May 31, 2023

Choose a reason for hiding this comment

vivekrnv May 31, 2023

Choose a reason for hiding this comment

baorliu May 31, 2023

Choose a reason for hiding this comment

vivekrnv May 31, 2023 • edited Loading

Choose a reason for hiding this comment

baorliu May 31, 2023

Choose a reason for hiding this comment

dgsudharsan commented Jun 2, 2023

abdosi commented Jun 7, 2023

abdosi commented Jun 7, 2023

baorliu commented May 31, 2023 •

edited by abdosi

Loading

vivekrnv May 31, 2023 •

edited

Loading