Skip to content
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

Fixbug: EVPN issue in FRR template #4260

Merged
merged 13 commits into from
Apr 3, 2020
100 changes: 100 additions & 0 deletions dockers/docker-fpm-frr/bgpcfgd
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,7 @@ class BGPPeerMgr(Manager):
[
("meta", "localhost/bgp_asn"),
("neigmeta", ""),
("interface", "")
],
"CONFIG_DB",
swsscommon.CFG_BGP_NEIGHBOR_TABLE_NAME
Expand All @@ -290,6 +291,25 @@ class BGPPeerMgr(Manager):
vrf, nbr = key.split('|', 1)
if key not in self.peers:
cmd = None

if "local_addr" not in data:
syslog.syslog(syslog.LOG_WARNING, 'Peer {}. Error in missing required attribute "local_addr"'.format(key))
else:

# Check if this route is belong to a vnet
interfaces = self.directory.get_slot("interface")
target_interface = None
for metadata in interfaces.values():
if metadata["ip"] == data["local_addr"]:
target_interface = metadata
break
if target_interface is None:
return False
Copy link
Collaborator

Choose a reason for hiding this comment

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

what should be the log message here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

elif target_interface.has_key("vnet_name") and target_interface["vnet_name"]:
# Ignore the route that is in a vnet
syslog.syslog(syslog.LOG_INFO, 'Peer {} in vnet {}'.format(key, target_interface["vnet_name"]))
return True

neigmeta = self.directory.get_slot("neigmeta")
if 'name' in data and data["name"] not in neigmeta:
return False
Expand Down Expand Up @@ -390,6 +410,82 @@ class BGPPeerMgr(Manager):
return peers


def prefix_attr(attr, value):
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

you are right, that was my miss. it is not a good code.


In reply to: 400696041 [](ancestors = 400696041)

if not value:
return None
else:
try:
prefix = netaddr.IPNetwork(str(value))
except:
return None
return str(getattr(prefix, attr))


class InterfaceMgr(Manager):
def __init__(self, daemon, directory, interface_table = swsscommon.CFG_INTF_TABLE_NAME):
Copy link
Contributor

Choose a reason for hiding this comment

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

We would also want to consider VLAN_INTERFACE as well - CFG_VLAN_INTF_TABLE_NAME

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your suggestion. Have added VlanInterfaceMgr.

super(InterfaceMgr, self).__init__(
daemon,
directory,
[],
"CONFIG_DB",
interface_table
)
self.interfaces = {}

def set_handler(self, key, data):
# There are two entries for an interface in CFG_INTF_TABLE.
# One of them is to specify ip and prefix.
# In this case, the key contains '|', like "Ethernet0|192.168.0.6/30".
# Another one is to specify whether this interface belongs to a vnet
# In this case, the key only includes the interface name
if '|' in key:
key, network = key.split('|', 1)
data["ip"] = prefix_attr("ip", network)
data["prefixlen"] = prefix_attr("prefixlen", network)

# Put the interface data into directory only if two entries have both been set
# Otherwise cache the first come entry
if key in self.interfaces:
self.interfaces[key].update(data)
self.directory.put("interface", key, self.interfaces[key])
del self.interfaces[key]
else:
self.interfaces[key] = data
return True

def del_handler(self, key):
if '|' in key:
key, _ = key.split('|', 1)
self.directory.remove("interface", key)


class LoopbackInterfaceMgr(InterfaceMgr):
def __init__(self, daemon, directory):
super(LoopbackInterfaceMgr, self).__init__(
daemon,
directory,
swsscommon.CFG_LOOPBACK_INTERFACE_TABLE_NAME
)


class VlanInterfaceMgr(InterfaceMgr):
def __init__(self, daemon, directory):
super(VlanInterfaceMgr, self).__init__(
daemon,
directory,
swsscommon.CFG_VLAN_INTF_TABLE_NAME
)


class PortChannelInterfaceMgr(InterfaceMgr):
def __init__(self, daemon, directory):
super(PortChannelInterfaceMgr, self).__init__(
daemon,
directory,
swsscommon.CFG_LAG_INTF_TABLE_NAME
)


def wait_for_bgpd():
# wait for 20 seconds
stop_time = datetime.datetime.now() + datetime.timedelta(seconds=20)
Expand All @@ -408,6 +504,10 @@ def main():
BGPDeviceMetaMgr,
BGPNeighborMetaMgr,
BGPPeerMgr,
InterfaceMgr,
LoopbackInterfaceMgr,
VlanInterfaceMgr,
PortChannelInterfaceMgr,
]
wait_for_bgpd()
daemon = Daemon()
Expand Down
3 changes: 1 addition & 2 deletions dockers/docker-fpm-frr/bgpd.peer.conf.j2
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,7 @@
neighbor {{ neighbor_addr }} next-hop-self
{% endif %}
{% if bgp_session["asn"] == DEVICE_METADATA['localhost']['bgp_asn']
and DEVICE_METADATA['localhost']['type'] == "SpineChassisFrontendRouter"
and (not bgp_session.has_key("local_addr") or bgp_session["local_addr"] not in interfaces_in_vnets) %}
and DEVICE_METADATA['localhost']['type'] == "SpineChassisFrontendRouter" %}
address-family l2vpn evpn
neighbor {{ neighbor_addr }} activate
advertise-all-vni
Expand Down