From a86a0264e0718130c838b397efba3f2594894c07 Mon Sep 17 00:00:00 2001 From: Baorong Liu Date: Fri, 7 Jul 2023 10:12:20 -0700 Subject: [PATCH] [staticroutebfd] fix static route uninstall issue when all nexthops are not reachable (#15575) fix static route uninstall issue when all nexthops are not reachable. the feature was working but the bug was introduced when support dynamic bfd enable/disable. Added UT testcase to guard this. --- .../bgpcfgd/managers_static_rt.py | 15 +- src/sonic-bgpcfgd/tests/test_static_rt.py | 161 ++++++++++++++++++ 2 files changed, 174 insertions(+), 2 deletions(-) diff --git a/src/sonic-bgpcfgd/bgpcfgd/managers_static_rt.py b/src/sonic-bgpcfgd/bgpcfgd/managers_static_rt.py index c5c55406d6f0..3f6a979eb0b1 100644 --- a/src/sonic-bgpcfgd/bgpcfgd/managers_static_rt.py +++ b/src/sonic-bgpcfgd/bgpcfgd/managers_static_rt.py @@ -88,7 +88,7 @@ def skip_appl_del(self, vrf, ip_prefix): so need this checking (skip appl_db deletion) to avoid race condition between StaticRouteMgr(appl_db) and StaticRouteMgr(config_db) For more detailed information: https://github.com/sonic-net/SONiC/blob/master/doc/static-route/SONiC_static_route_bfd_hld.md#bfd-field-changes-from-true-to-false - + but if the deletion is caused by no nexthop available (bfd field is true but all the sessions are down), need to allow this deletion. :param vrf: vrf from the split_key(key) return :param ip_prefix: ip_prefix from the split_key(key) return :return: True if the deletion comes from APPL_DB and the vrf|ip_prefix exists in CONFIG_DB, otherwise return False @@ -102,6 +102,17 @@ def skip_appl_del(self, vrf, ip_prefix): #just pop local cache if the route exist in config_db cfg_key = "STATIC_ROUTE|" + vrf + "|" + ip_prefix + if vrf == "default": + default_key = "STATIC_ROUTE|" + ip_prefix + bfd = self.config_db.get(self.config_db.CONFIG_DB, default_key, "bfd") + if bfd == "true": + log_debug("skip_appl_del: {}, key {}, bfd flag {}".format(self.db_name, default_key, bfd)) + return False + bfd = self.config_db.get(self.config_db.CONFIG_DB, cfg_key, "bfd") + if bfd == "true": + log_debug("skip_appl_del: {}, key {}, bfd flag {}".format(self.db_name, cfg_key, bfd)) + return False + nexthop = self.config_db.get(self.config_db.CONFIG_DB, cfg_key, "nexthop") if nexthop and len(nexthop)>0: self.static_routes.setdefault(vrf, {}).pop(ip_prefix, None) @@ -121,7 +132,7 @@ def del_handler(self, key): is_ipv6 = TemplateFabric.is_ipv6(ip_prefix) if self.skip_appl_del(vrf, ip_prefix): - log_debug("{} ignore appl_db static route deletion because of key {} exist in config_db".format(self.db_name, key)) + log_debug("{} ignore appl_db static route deletion because of key {} exist in config_db and bfd is not true".format(self.db_name, key)) return ip_nh_set = IpNextHopSet(is_ipv6) diff --git a/src/sonic-bgpcfgd/tests/test_static_rt.py b/src/sonic-bgpcfgd/tests/test_static_rt.py index 3d947a47ac73..422a3451c4fb 100644 --- a/src/sonic-bgpcfgd/tests/test_static_rt.py +++ b/src/sonic-bgpcfgd/tests/test_static_rt.py @@ -73,6 +73,167 @@ def test_set(): ] ) +@patch('bgpcfgd.managers_static_rt.log_debug') +def test_del_for_appl(mocked_log_debug): + class MockRedisConfigDbGet: + def __init__(self, cache=dict()): + self.cache = cache + self.CONFIG_DB = "CONFIG_DB" + + def get(self, db, key, field): + if key in self.cache: + if field in self.cache[key]["value"]: + return self.cache[key]["value"][field] + return None # return nil + + mgr = constructor() + + set_del_test( + mgr, + "SET", + ("10.1.0.0/24", { + "nexthop": "PortChannel0001", + }), + True, + [ + "ip route 10.1.0.0/24 PortChannel0001 tag 1", + "route-map STATIC_ROUTE_FILTER permit 10", + " match tag 1", + "router bgp 65100", + " address-family ipv4", + " redistribute static route-map STATIC_ROUTE_FILTER", + " address-family ipv6", + " redistribute static route-map STATIC_ROUTE_FILTER" + ] + ) + + #from "APPL_DB" instance, static route can not be uninstalled if the static route exists in config_db and "bfd"="false" (or no bfd field) + mgr.db_name = "APPL_DB" + cfg_db_cache = { + "STATIC_ROUTE|10.1.0.0/24": { + "value": { + "advertise": "false", + "nexthop": "PortChannel0001" + } + } + } + mgr.config_db = MockRedisConfigDbGet(cfg_db_cache) + + set_del_test( + mgr, + "DEL", + ("10.1.0.0/24",), + True, + [] + ) + mocked_log_debug.assert_called_with("{} ignore appl_db static route deletion because of key {} exist in config_db and bfd is not true".format(mgr.db_name, "10.1.0.0/24")) + + cfg_db_cache = { + "STATIC_ROUTE|10.1.0.0/24": { + "value": { + "advertise": "false", + "bfd": "false", + "nexthop": "PortChannel0001" + } + } + } + mgr.db_name = "APPL_DB" + mgr.config_db = MockRedisConfigDbGet(cfg_db_cache) + + set_del_test( + mgr, + "DEL", + ("10.1.0.0/24",), + True, + [] + ) + mocked_log_debug.assert_called_with("{} ignore appl_db static route deletion because of key {} exist in config_db and bfd is not true".format(mgr.db_name, "10.1.0.0/24")) + + #From "APPL_DB" instance, static route can be deleted if bfd field is true in config_db + set_del_test( + mgr, + "SET", + ("10.1.0.0/24", { + "nexthop": "PortChannel0001", + }), + True, + [ + "ip route 10.1.0.0/24 PortChannel0001 tag 1", + "route-map STATIC_ROUTE_FILTER permit 10", + " match tag 1", + "router bgp 65100", + " address-family ipv4", + " redistribute static route-map STATIC_ROUTE_FILTER", + " address-family ipv6", + " redistribute static route-map STATIC_ROUTE_FILTER" + ] + ) + cfg_db_cache = { + "STATIC_ROUTE|10.1.0.0/24": { + "value": { + "advertise": "false", + "bfd": "true", + "nexthop": "PortChannel0001" + } + } + } + mgr.db_name = "APPL_DB" + mgr.config_db = MockRedisConfigDbGet(cfg_db_cache) + set_del_test( + mgr, + "DEL", + ("10.1.0.0/24",), + True, + [ + "no ip route 10.1.0.0/24 PortChannel0001 tag 1", + "router bgp 65100", + " address-family ipv4", + " no redistribute static route-map STATIC_ROUTE_FILTER", + " address-family ipv6", + " no redistribute static route-map STATIC_ROUTE_FILTER", + "no route-map STATIC_ROUTE_FILTER" + ] + ) + + #From "APPL_DB" instance, static route can be deleted if the static route does not in config_db + set_del_test( + mgr, + "SET", + ("10.1.0.0/24", { + "nexthop": "PortChannel0001", + }), + True, + [ + "ip route 10.1.0.0/24 PortChannel0001 tag 1", + "route-map STATIC_ROUTE_FILTER permit 10", + " match tag 1", + "router bgp 65100", + " address-family ipv4", + " redistribute static route-map STATIC_ROUTE_FILTER", + " address-family ipv6", + " redistribute static route-map STATIC_ROUTE_FILTER" + ] + ) + + cfg_db_cache = {} + mgr.db_name = "APPL_DB" + mgr.config_db = MockRedisConfigDbGet(cfg_db_cache) + set_del_test( + mgr, + "DEL", + ("10.1.0.0/24",), + True, + [ + "no ip route 10.1.0.0/24 PortChannel0001 tag 1", + "router bgp 65100", + " address-family ipv4", + " no redistribute static route-map STATIC_ROUTE_FILTER", + " address-family ipv6", + " no redistribute static route-map STATIC_ROUTE_FILTER", + "no route-map STATIC_ROUTE_FILTER" + ] + ) + def test_set_nhportchannel(): mgr = constructor() set_del_test(