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.

Sign up for GitHub

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

[BGPD]: add bgp dynamic neighbor configuration #708

Merged
merged 5 commits into from
Jun 22, 2017
Merged

[BGPD]: add bgp dynamic neighbor configuration #708

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ec004b1
add bgp dynamic neighbor configuration
Jun 15, 2017
17f7dc8
[bgpd]: update as comments
Jun 19, 2017
6e80fbd
update as comment
Jun 20, 2017
48a4971
update to deployment_id_asn_map
Jun 21, 2017
d7791fd
minor change
Jun 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions dockers/docker-fpm-quagga/bgpd.conf.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ log facility local4
!
! bgp multiple-instance
!
route-map FROM_BGP_SPEAKER_V4 permit 10
!
route-map TO_BGP_SPEAKER_V4 deny 10
!
router bgp {{ minigraph_bgp_asn }}
bgp log-neighbor-changes
bgp bestpath as-path multipath-relax
Expand Down Expand Up @@ -65,6 +69,21 @@ router bgp {{ minigraph_bgp_asn }}
{% endif %}
{% endfor %}
{% endblock bgp_sessions %}
{% block bgp_peers_with_range %}
{% if deploymentId and deploymentId_asn_map[deploymentId] %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe it is better to make the failure explicit? when there is bgp peers with range, then we need to generate the passive peer. if deploymentId is not available, it is probably good to make the failure explicitly?

{% for bgp_peer in minigraph_bgp_peers_with_range %}
neighbor {{ bgp_peer['name'] }} peer-group
neighbor {{ bgp_peer['name'] }} passive
neighbor {{ bgp_peer['name'] }} remote-as {{deploymentId_asn_map[deploymentId] }}
neighbor {{ bgp_peer['name'] }} ebgp-multihop 255
neighbor {{ bgp_peer['name'] }} soft-reconfiguration inbound
neighbor {{ bgp_peer['name'] }} update-source Loopback0
neighbor {{ bgp_peer['name'] }} route-map FROM_BGP_SPEAKER_V4 in
neighbor {{ bgp_peer['name'] }} route-map TO_BGP_SPEAKER_V4 out
bgp listen range {{ bgp_peer['range'] }} peer-group {{ bgp_peer['name'] }}
{% endfor %}
{% endif %}
{% endblock bgp_peers_with_range %}
!
{% if minigraph_bgp_asn is not none %}
maximum-paths 64
Expand Down
4 changes: 2 additions & 2 deletions dockers/docker-fpm-quagga/start.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@

mkdir -p /etc/quagga
if [ -f /etc/sonic/bgp_admin.yml ]; then
sonic-cfggen -m /etc/sonic/minigraph.xml -y /etc/sonic/bgp_admin.yml -t /usr/share/sonic/templates/bgpd.conf.j2 > /etc/quagga/bgpd.conf
sonic-cfggen -m /etc/sonic/minigraph.xml -y /etc/sonic/bgp_admin.yml -y /etc/sonic/deploymentId_asn_map.yml -t /usr/share/sonic/templates/bgpd.conf.j2 > /etc/quagga/bgpd.conf
Copy link
Contributor

@taoyl-ms taoyl-ms Jun 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Do we allow the case that deploymentId_asn_map.yml does not exist? -y option will fail when the specified file does not exist so we'll need to do manual check beforehand.

I plan to add a -Y option into sonic-cfggen that silently ignore it when yml file does not exist, but that's not happened yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we will allow the case that deploymentId_asn_map doesn't exist since we will copy a sample deploymentId_asn_map.yml when building the debian package (as what we have done for snmp.yml). I don't think we do manual check for snmp.yml (correct me if I am wrong). So we might don't have to check for this yml file as well?

else
sonic-cfggen -m /etc/sonic/minigraph.xml -t /usr/share/sonic/templates/bgpd.conf.j2 > /etc/quagga/bgpd.conf
sonic-cfggen -m /etc/sonic/minigraph.xml -y /etc/sonic/deploymentId_asn_map.yml -t /usr/share/sonic/templates/bgpd.conf.j2 > /etc/quagga/bgpd.conf
fi
sonic-cfggen -m /etc/sonic/minigraph.xml -t /usr/share/sonic/templates/zebra.conf.j2 > /etc/quagga/zebra.conf

Expand Down
3 changes: 3 additions & 0 deletions files/build_templates/sonic_debian_extension.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,9 @@ sudo bash -c "echo ' all: off' >> $FILESYSTEM_ROOT/etc/sonic/bgp_admin.yml"
# Copy SNMP configuration files
sudo cp $IMAGE_CONFIGS/snmp/snmp.yml $FILESYSTEM_ROOT/etc/sonic/

# Copy ASN configuration files
sudo cp $IMAGE_CONFIGS/asn/deploymentId_asn_map.yml $FILESYSTEM_ROOT/etc/sonic/

# Copy sudoers configuration file
sudo cp $IMAGE_CONFIGS/sudoers/sudoers $FILESYSTEM_ROOT/etc/

Expand Down
2 changes: 2 additions & 0 deletions files/image_config/asn/deploymentId_asn_map.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
deploymentId_asn_map:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we use a mixture of camelCaseNaming and underscore_naming 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.

I thought deployementId is one unique word. Will change the naming later.

"1" : 12345
26 changes: 22 additions & 4 deletions src/sonic-config-engine/minigraph.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@ def parse_dpg(dpg, hname):
def parse_cpg(cpg, hname):
bgp_sessions = []
myasn = None
bgp_peers_with_range = []
for child in cpg:
tag = child.tag
if tag == str(QName(ns, "PeeringSessions")):
Expand All @@ -270,12 +271,22 @@ def parse_cpg(cpg, hname):
hostname = router.find(str(QName(ns1, "Hostname"))).text
if hostname == hname:
myasn = int(asn)
peers = router.find(str(QName(ns1, "Peers")))
for bgpPeer in peers.findall(str(QName(ns, "BGPPeer"))):
addr = bgpPeer.find(str(QName(ns, "Address"))).text
if bgpPeer.find(str(QName(ns1, "PeersRange"))) is not None:
name = bgpPeer.find(str(QName(ns1, "Name"))).text
range = bgpPeer.find(str(QName(ns1, "PeersRange"))).text
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll recommend another var name rather than overriding built-in range.

bgp_peers_with_range.append({
'name': name,
'range': range
})
else:
for bgp_session in bgp_sessions:
if hostname == bgp_session['name']:
bgp_session['asn'] = int(asn)

return bgp_sessions, myasn
return bgp_sessions, myasn, bgp_peers_with_range


def parse_meta(meta, hname):
Expand All @@ -284,6 +295,7 @@ def parse_meta(meta, hname):
ntp_servers = []
mgmt_routes = []
erspan_dst = []
deploymentId = None
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll suggest we unify to underscore_naming

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do

device_metas = meta.find(str(QName(ns, "Devices")))
for device in device_metas.findall(str(QName(ns1, "DeviceMetadata"))):
if device.find(str(QName(ns1, "Name"))).text == hname:
Expand All @@ -302,7 +314,9 @@ def parse_meta(meta, hname):
mgmt_routes = value_group
elif name == "ErspanDestinationIpv4":
erspan_dst = value_group
return syslog_servers, dhcp_servers, ntp_servers, mgmt_routes, erspan_dst
elif name == "DeploymentId":
deploymentId = value
return syslog_servers, dhcp_servers, ntp_servers, mgmt_routes, erspan_dst, deploymentId


def get_console_info(devices, dev, port):
Expand Down Expand Up @@ -396,6 +410,8 @@ def parse_xml(filename, platform=None, port_config_file=None):
ntp_servers = []
mgmt_routes = []
erspan_dst = []
bgp_peers_with_range = None
deploymentId = None

hwsku_qn = QName(ns, "HwSku")
hostname_qn = QName(ns, "Hostname")
Expand All @@ -411,13 +427,13 @@ def parse_xml(filename, platform=None, port_config_file=None):
if child.tag == str(QName(ns, "DpgDec")):
(intfs, lo_intfs, mgmt_intf, vlans, pcs, acls) = parse_dpg(child, hostname)
elif child.tag == str(QName(ns, "CpgDec")):
(bgp_sessions, bgp_asn) = parse_cpg(child, hostname)
(bgp_sessions, bgp_asn, bgp_peers_with_range) = parse_cpg(child, hostname)
elif child.tag == str(QName(ns, "PngDec")):
(neighbors, devices, console_dev, console_port, mgmt_dev, mgmt_port) = parse_png(child, hostname)
elif child.tag == str(QName(ns, "UngDec")):
(u_neighbors, u_devices, _, _, _, _) = parse_png(child, hostname)
elif child.tag == str(QName(ns, "MetadataDeclaration")):
(syslog_servers, dhcp_servers, ntp_servers, mgmt_routes, erspan_dst) = parse_meta(child, hostname)
(syslog_servers, dhcp_servers, ntp_servers, mgmt_routes, erspan_dst, deploymentId) = parse_meta(child, hostname)

Tree = lambda: defaultdict(Tree)

Expand All @@ -428,6 +444,7 @@ def parse_xml(filename, platform=None, port_config_file=None):
# TODO: alternatively (preferred), implement class containers for multiple-attribute entries, enabling sort by attr
results['minigraph_bgp'] = sorted(bgp_sessions, key=lambda x: x['addr'])
results['minigraph_bgp_asn'] = bgp_asn
results['minigraph_bgp_peers_with_range'] = bgp_peers_with_range
# TODO: sort does not work properly on all interfaces of varying lengths. Need to sort by integer group(s).

phyport_intfs = []
Expand Down Expand Up @@ -466,6 +483,7 @@ def parse_xml(filename, platform=None, port_config_file=None):
results['ntp_servers'] = ntp_servers
results['forced_mgmt_routes'] = mgmt_routes
results['erspan_dst'] = erspan_dst
results['deploymentId'] = deploymentId

return results

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a sample minigraph with bgp peers and corresponding test to the test folder?

Expand Down