Skip to content

[staticroutebfd] fix static route uninstall issue if no nexthop #15575

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 7, 2023
Merged

[staticroutebfd] fix static route uninstall issue if no nexthop #15575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions src/sonic-bgpcfgd/bgpcfgd/managers_static_rt.py
View file
Open in desktop
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

Copy link
Contributor

@jcaiMR jcaiMR Jul 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we migrate the code in "if vrf == "default"" and the code out of the if logic.
Since they are doing the same work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is that, in the redis STATIC ROUTE table, both of the following keys are valid if vrf is default:
STATIC_ROUTE|ip_prefix
STATIC_ROUTE|default|ip_prefix
so if vrf is default, check key STATIC_ROUTE|ip_prefix first, if bfd is not "true" then check the next legal key STATIC_ROUTE|default|ip_prefix.
3 possible states for bfd, only return False if bfd is "true". (again, two legal key formats, need to do redis get twice)
1, key is not in the table
2, key in in the table, bfd flag value if false
3, key in in the table, bfd flag value if true
The comment logic here is:
if bfd == "true":
return False
but if put it outside, need more logic to check the above 3 states.
Hope this can explain the code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks for explanation.

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
Open in desktop
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