Fixbug: EVPN issue in FRR template #4260
Conversation
Signed-off-by: zegan <zegan@microsoft.com>
|
|
||
|
|
||
| class InterfaceMgr(Manager): | ||
| def __init__(self, daemon, directory, interface_table = swsscommon.CFG_INTF_TABLE_NAME): |
There was a problem hiding this comment.
We would also want to consider VLAN_INTERFACE as well - CFG_VLAN_INTF_TABLE_NAME
There was a problem hiding this comment.
Thanks for your suggestion. Have added VlanInterfaceMgr.
Signed-off-by: zegan <zegan@microsoft.com>
|
retest vsimage please |
|
retest vsimage please |
|
it looks like vs test failed on bgp tests, please check. likely this pr breaks existing functions. |
|
@Pterosaur, please look into the vsimage test. the pr cannot be merged until the test passing. |
Signed-off-by: zegan <zegan@microsoft.com>
|
retest vsimage please |
…h set in the same interface Signed-off-by: zegan <zegan@microsoft.com>
dockers/docker-fpm-frr/bgpcfgd
Outdated
| if interface.has_key("vnet_name") and interface["vnet_name"]: | ||
| # Ignore the route that is in a vnet | ||
| syslog.syslog(syslog.LOG_INFO, 'Peer {} in vnet {}'.format(key, interface["vnet_name"])) | ||
| return True |
There was a problem hiding this comment.
i finally understood what you are trying to do in this block (spend about 30 minutes)! it is so difficult to understand because people do not understand what local_addresses and interfaces mean. In fact, what you really should do here is to create two functions.
one function is
find_local_interface(ip). The input is a ip address, and return value is interface name. return None if you cannot find a interface.
second function is
check_interface_type(interface_name). You can return vnet interface it is vnet interface, or default if the interface is in default vrf.
since you do not have such abstract, it is difficult for others to understand the code and maintain.
Please put code readability into priority.
dockers/docker-fpm-frr/bgpcfgd
Outdated
| @@ -390,6 +420,79 @@ class BGPPeerMgr(Manager): | |||
| return peers | |||
|
|
|||
|
|
|||
| def prefix_attr(attr, value): | |||
There was a problem hiding this comment.
this is really a bad interface design! the function name is generic. in fact the fuction does very specific thing, it assume the value is ip prefix and convert the string into ipnetwork, and then get an attribute, and finally convert it into a string.
to evaulate whether a function provide a good abstraction or not. People should be able to understand the what the function's job by just looking at the function name at the caller without looking into the implementation of the function.
I think you need to either get rid of this function, or have a function get_ip_from_ipprefix function to do the specific job you want.
There was a problem hiding this comment.
Yes, I agree with your comment. And I think this is a very common tool function maybe we should extract it to a common python package.
Because we did also use it at : https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-config-engine/sonic-cfggen#L81
Should I do that?
There was a problem hiding this comment.
you are right, that was my miss. it is not a good code.
In reply to: 400696041 [](ancestors = 400696041)
Signed-off-by: zegan <zegan@microsoft.com>
Signed-off-by: zegan <zegan@microsoft.com>
Signed-off-by: zegan <zegan@microsoft.com>
| # So we need to check whether this bgp session belongs to a vnet. | ||
| interface = InterfaceMgr.get_local_interface(self.directory, data["local_addr"]) | ||
| if not interface: | ||
| return False |
There was a problem hiding this comment.
what should be the log message here?
There was a problem hiding this comment.
Actually, this is an intermediate state. Maybe it should add some log with INFO level to display this state. Same problem at here https://github.com/Azure/sonic-buildimage/blob/master/dockers/docker-fpm-frr/bgpcfgd#L295
Signed-off-by: zegan <zegan@microsoft.com>
Signed-off-by: zegan <zegan@microsoft.com>
Signed-off-by: zegan <zegan@microsoft.com>
|
retest vsimage please |
|
can you add more description in the commit message? |
|
retest vsimage please |
* Fixbug: EVPN issue in FRR template
* Fixbug: EVPN issue in FRR template
- What I did
Fixes EVPN issue in FRR template to avoid the routes that belong to VNet are broadcasted by FRR.
- How I did it
Add interface manager in BGPCFGD daemon to separate the routes that belong to VNet or not according to interface table in config db.
- How to verify it
vtysh -c "show running-config"
- Description for the changelog
Fixes EVPN issue in FRR template to avoid the routes that belong to VNet are broadcasted by FRR.
- A picture of a cute animal (not mandatory but encouraged)
🦏
Signed-off-by: zegan zegan@microsoft.com