-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
[bgpcfgd] ECMP overlay VxLan with BGP support #10716
[bgpcfgd] ECMP overlay VxLan with BGP support #10716
Conversation
|
||
OP_DELETE = 'DELETE' | ||
OP_ADD = 'ADD' | ||
self.directory.subscribe( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we retain the original code as-is if its not related to this PR. This will help in reviewing just the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will keep original code as much as possible. Only change is to use " instead of ' to keep the style consistent.
src/sonic-bgpcfgd/bgpcfgd/main.py
Outdated
@@ -62,6 +63,8 @@ def do_work(): | |||
StaticRouteMgr(common_objs, "CONFIG_DB", "STATIC_ROUTE"), | |||
# Route Advertisement Managers | |||
AdvertiseRouteMgr(common_objs, "STATE_DB", swsscommon.STATE_ADVERTISE_NETWORK_TABLE_NAME), | |||
# TBD, Change to schema name from swsscommon (swsscommon.APP_BGP_PROFILE_TABLE_NAME) when the submodule is advanced. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix TBD before merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked, the code is already advanced in the master branch. Will make sure which is advanced before cherry pick to 202012 branch.
vrf, ip_prefix = self.split_key(key) | ||
self.remove_route_advertisement(vrf, ip_prefix) | ||
|
||
def _ip_addr_validate(self, key): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this mandatory? I mean APP_DB entries shall be already validated by the producer. So may be this is redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good one, add one comment in the code.
def _set_handler_validate(self, key, data): | ||
if data: | ||
if ("profile" in data and data["profile"] in ROUTE_MAPS) or data == {"":""}: | ||
return self._ip_addr_validate(key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, this could be just returning 'true' here. IP prefix should already be validated, and we may just be adding extra delay here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
|
||
def add_route_advertisement(self, vrf, ip_prefix): | ||
def _del_handler_validate(self, key): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
if data and "profile" in data: | ||
cmd_list.append(" network %s route-map %s" % (ip_prefix, "%s_RM" % data["profile"])) | ||
log_info( | ||
"BGPAdvertiseRouteMgr:: Update bgp %s network %s with route-map %s" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on scale, there could be significant number of route updates. So please reduce the log-level and ensure it won't show up in regular flow unless explicitly enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good one, change to debug.
self.cfg_mgr.push_list(cmds) | ||
log_info("BGPRouteMapMgr::Done") | ||
|
||
def _set_handler_validate(self, key, data): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets use consistent formatting for names. Can you check if its "__" vs "_" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, changed all internal function to "__"
This pull request introduces 1 alert when merging fb605a1 into 4a1e7d8 - view on LGTM.com new alerts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, minor comments
|
||
def add_route_advertisement(self, vrf, ip_prefix): | ||
def __set_handler_validate(self, key, data): | ||
if data: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If data is not present, we need to advertise without a community string (for backward compatibility). Can you please confirm.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prsunny confirmed, the check after the 'or' is for the case which doesn't have the community string.
if ("profile" in data and data["profile"] in ROUTE_MAPS) or data == {"":""}:
@@ -62,7 +62,7 @@ def test_set_del(): | |||
set_del_test( | |||
mgr, | |||
"SET", | |||
("fc00:10::/64", {}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we retain this case to cover empty data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prsunny I think the old unit test is inaccurate, from existing code of VNetRouteOrch, it send out {key:{"":""}} to the bgpcfgd for empty case as below. And also by manually testing, can't insert a key with a "empty" data to the redis.
void VNetRouteOrch::addRouteAdvertisement(IpPrefix& ipPrefix)
{
const string key = ipPrefix.to_string();
vector fvs;
fvs.push_back(FieldValueTuple("", ""));
state_vnet_rt_adv_table_->set(key, fvs);
}
Why I did it https://github.com/Azure/SONiC/blob/master/doc/vxlan/Overlay%20ECMP%20with%20BFD.md From the design, need to advertise the route with community string, the PR is to implement this. How I did it To use the route-map as the profile for the community string, all advertised routes can be associated with one route-map. Add one file, mangers_rm.py, which is to add/update/del the route-map. Modified the managers_advertise_rt.py file to associate profile with IP route. The route-map usage is very flexible, by this PR, we only support one fixed usage to add community string for route to simplify this design. How to verify it Implement new unit tests for mangers_rm.py and updated unit test for managers_advertise_rt.py. Manually verified the test case in the test plan section, will add testcase in sonic-mgmt later. sonic-net/sonic-mgmt#5581
…anch Related work items: #52, #71, #73, #75, #77, sonic-net#1306, sonic-net#1588, sonic-net#1991, sonic-net#2031, sonic-net#2040, sonic-net#2053, sonic-net#2066, sonic-net#2069, sonic-net#2087, sonic-net#2107, sonic-net#2110, sonic-net#2112, sonic-net#2113, sonic-net#2117, sonic-net#2124, sonic-net#2125, sonic-net#2126, sonic-net#2128, sonic-net#2130, sonic-net#2131, sonic-net#2132, sonic-net#2133, sonic-net#2134, sonic-net#2135, sonic-net#2136, sonic-net#2137, sonic-net#2138, sonic-net#2139, sonic-net#2140, sonic-net#2143, sonic-net#2158, sonic-net#2161, sonic-net#2233, sonic-net#2243, sonic-net#2250, sonic-net#2254, sonic-net#2260, sonic-net#2261, sonic-net#2267, sonic-net#2278, sonic-net#2282, sonic-net#2285, sonic-net#2288, sonic-net#2289, sonic-net#2292, sonic-net#2294, sonic-net#8887, sonic-net#9279, sonic-net#9390, sonic-net#9511, sonic-net#9700, sonic-net#10025, sonic-net#10322, sonic-net#10479, sonic-net#10484, sonic-net#10493, sonic-net#10500, sonic-net#10580, sonic-net#10595, sonic-net#10628, sonic-net#10634, sonic-net#10635, sonic-net#10644, sonic-net#10670, sonic-net#10691, sonic-net#10716, sonic-net#10731, sonic-net#10750, sonic-net#10751, sonic-net#10752, sonic-net#10761, sonic-net#10769, sonic-net#10775, sonic-net#10776, sonic-net#10779, sonic-net#10786, sonic-net#10792, sonic-net#10793, sonic-net#10800, sonic-net#10806, sonic-net#10826, sonic-net#10839, sonic-net#10840, sonic-net#10842, sonic-net#10844, sonic-net#10847, sonic-net#10849, sonic-net#10852, sonic-net#10865, sonic-net#10872, sonic-net#10877, sonic-net#10886, sonic-net#10889, sonic-net#10903, sonic-net#10904, sonic-net#10905, sonic-net#10913, sonic-net#10914, sonic-net#10916, sonic-net#10919, sonic-net#10925, sonic-net#10926, sonic-net#10929, sonic-net#10933, sonic-net#10934, sonic-net#10937, sonic-net#10941, sonic-net#10947, sonic-net#10952, sonic-net#10953, sonic-net#10957, sonic-net#10959, sonic-net#10971, sonic-net#10972, sonic-net#10980
Why I did it
https://github.com/Azure/SONiC/blob/master/doc/vxlan/Overlay%20ECMP%20with%20BFD.md
From the design, need to advertise the route with community string, the PR is to implement this.
How I did it
To use the route-map as the profile for the community string, all advertised routes can be associated with one route-map.
Add one file, mangers_rm.py, which is to add/update/del the route-map. Modified the managers_advertise_rt.py file to associate profile with IP route.
The route-map usage is very flexible, by this PR, we only support one fixed usage to add community string for route to simplify this design.
How to verify it
Implement new unit tests for mangers_rm.py and updated unit test for managers_advertise_rt.py.
Manually verified the test case in the test plan section, will add testcase in sonic-mgmt later. sonic-net/sonic-mgmt#5581
2.8.3 BGP advertising
The below cases are executed first for IPv4 and repeat the same for IPv6.
Description for the changelog
Link to config_db schema for YANG module changes
A picture of a cute animal (not mandatory but encouraged)