Skip to content

Commit d8641b4

Browse files
baorliusonic-otn
baorliu
authored and
sonic-otn
committed
[staticroutebfd] fix static route uninstall issue when all nexthops are not reachable (sonic-net#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.
1 parent 8a4c296 commit d8641b4

File tree

2 files changed

+174
-2
lines changed

2 files changed

+174
-2
lines changed

‎src/sonic-bgpcfgd/bgpcfgd/managers_static_rt.py

+13-2
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ def skip_appl_del(self, vrf, ip_prefix):
8888
so need this checking (skip appl_db deletion) to avoid race condition between StaticRouteMgr(appl_db) and StaticRouteMgr(config_db)
8989
For more detailed information:
9090
https://github.com/sonic-net/SONiC/blob/master/doc/static-route/SONiC_static_route_bfd_hld.md#bfd-field-changes-from-true-to-false
91-
91+
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.
9292
:param vrf: vrf from the split_key(key) return
9393
:param ip_prefix: ip_prefix from the split_key(key) return
9494
: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):
102102

103103
#just pop local cache if the route exist in config_db
104104
cfg_key = "STATIC_ROUTE|" + vrf + "|" + ip_prefix
105+
if vrf == "default":
106+
default_key = "STATIC_ROUTE|" + ip_prefix
107+
bfd = self.config_db.get(self.config_db.CONFIG_DB, default_key, "bfd")
108+
if bfd == "true":
109+
log_debug("skip_appl_del: {}, key {}, bfd flag {}".format(self.db_name, default_key, bfd))
110+
return False
111+
bfd = self.config_db.get(self.config_db.CONFIG_DB, cfg_key, "bfd")
112+
if bfd == "true":
113+
log_debug("skip_appl_del: {}, key {}, bfd flag {}".format(self.db_name, cfg_key, bfd))
114+
return False
115+
105116
nexthop = self.config_db.get(self.config_db.CONFIG_DB, cfg_key, "nexthop")
106117
if nexthop and len(nexthop)>0:
107118
self.static_routes.setdefault(vrf, {}).pop(ip_prefix, None)
@@ -121,7 +132,7 @@ def del_handler(self, key):
121132
is_ipv6 = TemplateFabric.is_ipv6(ip_prefix)
122133

123134
if self.skip_appl_del(vrf, ip_prefix):
124-
log_debug("{} ignore appl_db static route deletion because of key {} exist in config_db".format(self.db_name, key))
135+
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))
125136
return
126137

127138
ip_nh_set = IpNextHopSet(is_ipv6)

‎src/sonic-bgpcfgd/tests/test_static_rt.py

+161
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,167 @@ def test_set():
7373
]
7474
)
7575

76+
@patch('bgpcfgd.managers_static_rt.log_debug')
77+
def test_del_for_appl(mocked_log_debug):
78+
class MockRedisConfigDbGet:
79+
def __init__(self, cache=dict()):
80+
self.cache = cache
81+
self.CONFIG_DB = "CONFIG_DB"
82+
83+
def get(self, db, key, field):
84+
if key in self.cache:
85+
if field in self.cache[key]["value"]:
86+
return self.cache[key]["value"][field]
87+
return None # return nil
88+
89+
mgr = constructor()
90+
91+
set_del_test(
92+
mgr,
93+
"SET",
94+
("10.1.0.0/24", {
95+
"nexthop": "PortChannel0001",
96+
}),
97+
True,
98+
[
99+
"ip route 10.1.0.0/24 PortChannel0001 tag 1",
100+
"route-map STATIC_ROUTE_FILTER permit 10",
101+
" match tag 1",
102+
"router bgp 65100",
103+
" address-family ipv4",
104+
" redistribute static route-map STATIC_ROUTE_FILTER",
105+
" address-family ipv6",
106+
" redistribute static route-map STATIC_ROUTE_FILTER"
107+
]
108+
)
109+
110+
#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)
111+
mgr.db_name = "APPL_DB"
112+
cfg_db_cache = {
113+
"STATIC_ROUTE|10.1.0.0/24": {
114+
"value": {
115+
"advertise": "false",
116+
"nexthop": "PortChannel0001"
117+
}
118+
}
119+
}
120+
mgr.config_db = MockRedisConfigDbGet(cfg_db_cache)
121+
122+
set_del_test(
123+
mgr,
124+
"DEL",
125+
("10.1.0.0/24",),
126+
True,
127+
[]
128+
)
129+
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"))
130+
131+
cfg_db_cache = {
132+
"STATIC_ROUTE|10.1.0.0/24": {
133+
"value": {
134+
"advertise": "false",
135+
"bfd": "false",
136+
"nexthop": "PortChannel0001"
137+
}
138+
}
139+
}
140+
mgr.db_name = "APPL_DB"
141+
mgr.config_db = MockRedisConfigDbGet(cfg_db_cache)
142+
143+
set_del_test(
144+
mgr,
145+
"DEL",
146+
("10.1.0.0/24",),
147+
True,
148+
[]
149+
)
150+
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"))
151+
152+
#From "APPL_DB" instance, static route can be deleted if bfd field is true in config_db
153+
set_del_test(
154+
mgr,
155+
"SET",
156+
("10.1.0.0/24", {
157+
"nexthop": "PortChannel0001",
158+
}),
159+
True,
160+
[
161+
"ip route 10.1.0.0/24 PortChannel0001 tag 1",
162+
"route-map STATIC_ROUTE_FILTER permit 10",
163+
" match tag 1",
164+
"router bgp 65100",
165+
" address-family ipv4",
166+
" redistribute static route-map STATIC_ROUTE_FILTER",
167+
" address-family ipv6",
168+
" redistribute static route-map STATIC_ROUTE_FILTER"
169+
]
170+
)
171+
cfg_db_cache = {
172+
"STATIC_ROUTE|10.1.0.0/24": {
173+
"value": {
174+
"advertise": "false",
175+
"bfd": "true",
176+
"nexthop": "PortChannel0001"
177+
}
178+
}
179+
}
180+
mgr.db_name = "APPL_DB"
181+
mgr.config_db = MockRedisConfigDbGet(cfg_db_cache)
182+
set_del_test(
183+
mgr,
184+
"DEL",
185+
("10.1.0.0/24",),
186+
True,
187+
[
188+
"no ip route 10.1.0.0/24 PortChannel0001 tag 1",
189+
"router bgp 65100",
190+
" address-family ipv4",
191+
" no redistribute static route-map STATIC_ROUTE_FILTER",
192+
" address-family ipv6",
193+
" no redistribute static route-map STATIC_ROUTE_FILTER",
194+
"no route-map STATIC_ROUTE_FILTER"
195+
]
196+
)
197+
198+
#From "APPL_DB" instance, static route can be deleted if the static route does not in config_db
199+
set_del_test(
200+
mgr,
201+
"SET",
202+
("10.1.0.0/24", {
203+
"nexthop": "PortChannel0001",
204+
}),
205+
True,
206+
[
207+
"ip route 10.1.0.0/24 PortChannel0001 tag 1",
208+
"route-map STATIC_ROUTE_FILTER permit 10",
209+
" match tag 1",
210+
"router bgp 65100",
211+
" address-family ipv4",
212+
" redistribute static route-map STATIC_ROUTE_FILTER",
213+
" address-family ipv6",
214+
" redistribute static route-map STATIC_ROUTE_FILTER"
215+
]
216+
)
217+
218+
cfg_db_cache = {}
219+
mgr.db_name = "APPL_DB"
220+
mgr.config_db = MockRedisConfigDbGet(cfg_db_cache)
221+
set_del_test(
222+
mgr,
223+
"DEL",
224+
("10.1.0.0/24",),
225+
True,
226+
[
227+
"no ip route 10.1.0.0/24 PortChannel0001 tag 1",
228+
"router bgp 65100",
229+
" address-family ipv4",
230+
" no redistribute static route-map STATIC_ROUTE_FILTER",
231+
" address-family ipv6",
232+
" no redistribute static route-map STATIC_ROUTE_FILTER",
233+
"no route-map STATIC_ROUTE_FILTER"
234+
]
235+
)
236+
76237
def test_set_nhportchannel():
77238
mgr = constructor()
78239
set_del_test(

0 commit comments

Comments
 (0)