[staticroutebfd]fix an issue on deleting a non-bfd static route (soni…

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

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

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

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

src/sonic-bgpcfgd/staticroutebfd/main.py

@@ -400,20 +400,27 @@ 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"
             self.set_local_db(LOCAL_CONFIG_TABLE, route_cfg_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.")
@@ -502,18 +509,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:

src/sonic-bgpcfgd/tests/test_static_rt_bfd.py

@@ -169,6 +169,94 @@ 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", {}),
+        {},
+        {}
+    )
+
+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():
     dut = constructor()
     intf_setup(dut)
@@ -514,3 +602,4 @@ def test_set_bfd_change_no_hold():
     )
 
 
+