Skip to content

Changes for LLDP docker to support multi-npu platforms #4530

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

Merged
merged 3 commits into from
May 11, 2020
Merged

Changes for LLDP docker to support multi-npu platforms #4530

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
60aa5c5
Changes for LLDP for Multi NPU Platoforms:-
abdosi May 4, 2020
f1bc3eb
Address Review Comment with following changes:
abdosi May 6, 2020
358b2e4
Address Review Comments
abdosi May 10, 2020
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
5 changes: 3 additions & 2 deletions dockers/docker-lldp-sv2/Dockerfile.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,13 @@ RUN apt-get purge -y python-pip && \
/python-wheels \
~/.cache

COPY ["docker-lldp-init.sh", "/usr/local/bin/"]
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please put the script into /usr/bin?
We never use /usr/local for anything (but agree it is more appropriate to put our own code).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pavel-shirshov Done

COPY ["start.sh", "/usr/bin/"]
COPY ["supervisord.conf", "/etc/supervisor/conf.d/"]
COPY ["supervisord.conf.j2", "/usr/share/sonic/templates/"]
COPY ["lldpd.conf.j2", "/usr/share/sonic/templates/"]
COPY ["lldpd", "/etc/default/"]
COPY ["lldpmgrd", "/usr/bin/"]
COPY ["files/supervisor-proc-exit-listener", "/usr/bin"]
COPY ["critical_processes", "/etc/supervisor"]

ENTRYPOINT ["/usr/bin/supervisord"]
ENTRYPOINT ["/usr/local/bin/docker-lldp-init.sh"]
5 changes: 5 additions & 0 deletions dockers/docker-lldp-sv2/docker-lldp-init.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#!/usr/bin/env bash
#Generate supervisord.conf based on device metadata
mkdir -p /etc/supervisor/conf.d/
sonic-cfggen -d -t /usr/share/sonic/templates/supervisord.conf.j2 > /etc/supervisor/conf.d/supervisord.conf
exec /usr/bin/supervisord
4 changes: 4 additions & 0 deletions dockers/docker-lldp-sv2/supervisord.conf → dockers/docker-lldp-sv2/supervisord.conf.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,11 @@ stderr_logfile=syslog
# - `-dd` means to stay in foreground, log warnings to console
# - `-ddd` means to stay in foreground, log warnings and info to console
# - `-dddd` means to stay in foreground, log all to console
{% if DEVICE_METADATA['localhost']['sub_role'] is defined and DEVICE_METADATA['localhost']['sub_role']|length %}
command=/usr/sbin/lldpd -d -I Ethernet* -C Ethernet*
{% else %}
command=/usr/sbin/lldpd -d -I Ethernet*,eth0 -C eth0
Copy link
Contributor

Choose a reason for hiding this comment

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

what about rewritong this command line using sed,inside of supervisord.conf? And don't use jinja2 template for that?
We can change supervisor config in start.sh

Copy link
Collaborator

Choose a reason for hiding this comment

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

i suggest to make this code generic, detect if there is eth0, if there is then use eth0, if there is no, do not add eth0 into the command line. it is better than templating based on the sub_role.

Copy link
Contributor Author

@abdosi abdosi May 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@pavel-shirshov : I tried couple of things to avoid jinja2 template
a) supervisor environment variable
b) sed command
However both does not seems to work. supervisos.conf gets loaded whens supervisor comes up and seems there is some dependency we can't change run time from start.sh

@lguohan: eth0 is present both in container and host. So we cannot distinguish that, Do we need to check if container is using --net= host or not ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@pavel-shirshov , what is the concern for using jinja2 template for supervisord.conf? if we can first generate and start supervisord, then we do not need to reload.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pavel-shirshov Based on above comment will be using jinja template for suerpvisor.conf

Copy link
Contributor

Choose a reason for hiding this comment

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

@lguohan It looks like we're going to have another snowflake in SONiC. Everywhere we start with "start.sh" script. Here the "start.sh" is not run on the start (So the name is misleading). And we have another start script, with another name. This is confusing.

{% endif %}
priority=3
autostart=false
autorestart=false
Expand Down
1 change: 1 addition & 0 deletions files/build_templates/lldp.service.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
per_namespace/lldp.service.j2
24 changes: 16 additions & 8 deletions src/sonic-config-engine/minigraph.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,14 +248,24 @@ def parse_asic_png(png, asic_name, hostname):
return (neighbors, devices, port_speeds)

def parse_dpg(dpg, hname):
aclintfs = None
mgmtintfs = None
for child in dpg:
"""In Multi-NPU platforms the acl intfs are defined only for the host not for individual asic.
"""
In Multi-NPU platforms the acl intfs are defined only for the host not for individual asic.
There is just one aclintf node in the minigraph
Get the aclintfs node first.
Get the aclintfs node first.
"""
if child.find(str(QName(ns, "AclInterfaces"))) is not None:
if aclintfs is None and child.find(str(QName(ns, "AclInterfaces"))) is not None:
aclintfs = child.find(str(QName(ns, "AclInterfaces")))

"""
In Multi-NPU platforms the mgmt intfs are defined only for the host not for individual asic
There is just one mgmtintf node in the minigraph
Get the mgmtintfs node first. We need mgmt intf to get mgmt ip in per asic dockers.
"""
if mgmtintfs is None and child.find(str(QName(ns, "ManagementIPInterfaces"))) is not None:
mgmtintfs = child.find(str(QName(ns, "ManagementIPInterfaces")))

hostname = child.find(str(QName(ns, "Hostname")))
if hostname.text.lower() != hname.lower():
continue
Expand Down Expand Up @@ -291,7 +301,6 @@ def parse_dpg(dpg, hname):
mvrf_en_flag = mv.find(str(QName(ns, "mgmtVrfEnabled"))).text
mvrf["vrf_global"] = {"mgmtVrfEnabled": mvrf_en_flag}

mgmtintfs = child.find(str(QName(ns, "ManagementIPInterfaces")))
mgmt_intf = {}
for mgmtintf in mgmtintfs.findall(str(QName(ns1, "ManagementIPInterface"))):
intfname = mgmtintf.find(str(QName(ns, "AttachTo"))).text
Expand Down Expand Up @@ -767,18 +776,16 @@ def parse_xml(filename, platform=None, port_config_file=None, asic_name=None):

if asic_name is None:
current_device = [devices[key] for key in devices if key.lower() == hostname.lower()][0]
name = hostname
else:
current_device = [devices[key] for key in devices if key.lower() == asic_name.lower()][0]
name = asic_name

results = {}
results['DEVICE_METADATA'] = {'localhost': {
'bgp_asn': bgp_asn,
'deployment_id': deployment_id,
'region': region,
'docker_routing_config_mode': docker_routing_config_mode,
'hostname': name,
'hostname': hostname,
'hwsku': hwsku,
'type': current_device['type']
}
Expand All @@ -788,6 +795,7 @@ def parse_xml(filename, platform=None, port_config_file=None, asic_name=None):
if sub_role is not None:
current_device['sub_role'] = sub_role
results['DEVICE_METADATA']['localhost']['sub_role'] = sub_role
results['DEVICE_METADATA']['localhost']['asic_name'] = asic_name
results['BGP_NEIGHBOR'] = bgp_sessions
results['BGP_MONITORS'] = bgp_monitors
results['BGP_PEER_RANGE'] = bgp_peers_with_range
Expand Down
5 changes: 3 additions & 2 deletions src/sonic-config-engine/tests/test_multinpu_cfggen.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ def test_mgmt_port(self):
self.assertDictEqual(output, {'eth0': {'alias': 'eth0', 'admin_status': 'up'}})
for asic in range(NUM_ASIC):
output = json.loads(self.run_script_for_asic(argument, asic, self.port_config[asic]))
self.assertDictEqual(output, {})
self.assertDictEqual(output, {'eth0': {'alias': 'eth0', 'admin_status': 'up'}})

def test_frontend_asic_portchannels(self):
argument = "-m {} -p {} -n asic0 --var-json \"PORTCHANNEL\"".format(self.sample_graph, self.port_config[0])
Expand Down Expand Up @@ -213,7 +213,8 @@ def test_device_asic_metadata(self):
for asic in range(NUM_ASIC):
output = json.loads(self.run_script_for_asic(argument, asic,self.port_config[asic]))
asic_name = "asic{}".format(asic)
self.assertEqual(output['localhost']['hostname'], asic_name)
self.assertEqual(output['localhost']['hostname'], 'multi_npu_platform_01')
self.assertEqual(output['localhost']['asic_name'], asic_name)
self.assertEqual(output['localhost']['type'], 'Asic')
if asic == 0 or asic == 1:
self.assertEqual(output['localhost']['sub_role'], 'FrontEnd')
Expand Down