[staticroutebfd] fix static route uninstall issue if no nexthop #15575
Conversation
…re not reachable 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.
|
@jcaiMR could you help to take a look? |
| if bfd == "true": | ||
| log_debug("skip_appl_del: {}, key {}, bfd flag {}".format(self.db_name, cfg_key, bfd)) | ||
| return False | ||
|
|
There was a problem hiding this comment.
Can we migrate the code in "if vrf == "default"" and the code out of the if logic.
Since they are doing the same work.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I see, thanks for explanation.
abdosi
added
the
Chassis for 202205 branch
|
@StormLiangMS / @prsunny can you help with merge of this. |
|
@yxieca Add Ying for review and code merge. |
…15574 [staticroutebfd] fix static route uninstall issue Double commit the PR from sonic-net/sonic-buildimage. Original PR for staticroutebfd: sonic-net/sonic-buildimage#15575
|
labeling this as already included for chassis 202205 branch via the PR that @abdosi merged: sonic-net/sonic-buildimage-msft#43 which went directly into the msft repo 202205 branch. |
gechiang
added
the
Included in Chassis for 202205 Branch
|
@gechiang could we have ADO for cherry-pick ask to 202305? |
|
@StormLiangMS |
…re 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.
|
Cherry-pick PR to 202305: #16133 |
…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.
mssonicbld
added
Included in 202305 Branch
and removed
Created PR to 202305 Branch
labels
…re 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.
Related work items: sonic-net#94, sonic-net#13789, sonic-net#14149, sonic-net#14515, sonic-net#14788, sonic-net#14922, sonic-net#14933, sonic-net#15284, sonic-net#15383, sonic-net#15464, sonic-net#15519, sonic-net#15521, sonic-net#15575, sonic-net#15636, sonic-net#15652, sonic-net#15684, sonic-net#15708, sonic-net#15725, sonic-net#15739, sonic-net#15755, sonic-net#15756, sonic-net#15757
fixes #15574
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.
Why I did it
If a static route is bfd protected, when all the bfd sessions are down and all the nexthop need to be removed from the static route, it fails because a kip condition is missing.
Work item tracking
How I did it
To support bfd config dynamic change and avoid race condition when delete static route, there is a skip checking for staticRouteMgr(appl_db) to check if the deletion should be skipped. Besides checking if the route exists in config_db, also need to check if the route is bfd protected.
How to verify it
verified it both in UT and in real testbed.
Which release branch to backport (provide reason below if selected)
Tested branch (Please provide the tested image version)
Description for the changelog
Link to config_db schema for YANG module changes
A picture of a cute animal (not mandatory but encouraged)