Skip to content

Commit

Permalink
[staticroutebfd] fix static route uninstall issue when all nexthops a…
Browse files Browse the repository at this point in the history 
…re 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.
  • Loading branch information
@baorliu @pull
baorliu authored and pull[bot] committed Aug 30, 2024
1 parent 921b713 commit 424212b
Show file tree
Hide file tree
Showing 2 changed files with 174 additions and 2 deletions.
15 changes: 13 additions & 2 deletions src/sonic-bgpcfgd/bgpcfgd/managers_static_rt.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand All @@ -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)
Expand Down
161 changes: 161 additions & 0 deletions src/sonic-bgpcfgd/tests/test_static_rt.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down

0 comments on commit 424212b

Please sign in to comment.