[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.
This commit is contained in:
parent
7b7aae981a
commit
6016b2ba57
@ -88,7 +88,7 @@ class StaticRouteMgr(Manager):
|
|||||||
so need this checking (skip appl_db deletion) to avoid race condition between StaticRouteMgr(appl_db) and StaticRouteMgr(config_db)
|
so need this checking (skip appl_db deletion) to avoid race condition between StaticRouteMgr(appl_db) and StaticRouteMgr(config_db)
|
||||||
For more detailed information:
|
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
|
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 vrf: vrf from the split_key(key) return
|
||||||
:param ip_prefix: ip_prefix 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
|
: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 @@ class StaticRouteMgr(Manager):
|
|||||||
|
|
||||||
#just pop local cache if the route exist in config_db
|
#just pop local cache if the route exist in config_db
|
||||||
cfg_key = "STATIC_ROUTE|" + vrf + "|" + ip_prefix
|
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")
|
nexthop = self.config_db.get(self.config_db.CONFIG_DB, cfg_key, "nexthop")
|
||||||
if nexthop and len(nexthop)>0:
|
if nexthop and len(nexthop)>0:
|
||||||
self.static_routes.setdefault(vrf, {}).pop(ip_prefix, None)
|
self.static_routes.setdefault(vrf, {}).pop(ip_prefix, None)
|
||||||
@ -121,7 +132,7 @@ class StaticRouteMgr(Manager):
|
|||||||
is_ipv6 = TemplateFabric.is_ipv6(ip_prefix)
|
is_ipv6 = TemplateFabric.is_ipv6(ip_prefix)
|
||||||
|
|
||||||
if self.skip_appl_del(vrf, 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
|
return
|
||||||
|
|
||||||
ip_nh_set = IpNextHopSet(is_ipv6)
|
ip_nh_set = IpNextHopSet(is_ipv6)
|
||||||
|
@ -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():
|
def test_set_nhportchannel():
|
||||||
mgr = constructor()
|
mgr = constructor()
|
||||||
set_del_test(
|
set_del_test(
|
||||||
|
Reference in New Issue
Block a user