-
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
Parser changes to support parsing of multi-asic device minigraph #4222
Changes from 19 commits
3ce843a
ff6aa99
b784c81
9b4039e
d4fe8e0
53c6693
a9a06a8
9c9f8c2
5b6892e
78a2bf7
ce84879
882c251
4ae22be
30024ba
9744b85
4f4be8c
0895629
0b90dae
15228c1
0432f92
ebf2e8b
520c43b
110d1b8
247370b
cd3a678
06983e4
17a0d04
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,10 +37,12 @@ from functools import partial | |
from minigraph import minigraph_encoder | ||
from minigraph import parse_xml | ||
from minigraph import parse_device_desc_xml | ||
from minigraph import parse_asic_sub_role | ||
from portconfig import get_port_config | ||
from sonic_device_util import get_machine_info | ||
from sonic_device_util import get_platform_info | ||
from sonic_device_util import get_system_mac | ||
from sonic_device_util import get_npu_id_from_name | ||
from config_samples import generate_sample_config | ||
from config_samples import get_available_config | ||
from swsssdk import SonicV2Connector, ConfigDBConnector | ||
|
@@ -195,6 +197,7 @@ def main(): | |
group.add_argument("-m", "--minigraph", help="minigraph xml file", nargs='?', const='/etc/sonic/minigraph.xml') | ||
group.add_argument("-M", "--device-description", help="device description xml file") | ||
group.add_argument("-k", "--hwsku", help="HwSKU") | ||
parser.add_argument("-n", "--namespace", help="namespace name, used with -m or -k", nargs='?', const=None) | ||
parser.add_argument("-p", "--port-config", help="port config file, used with -m or -k", nargs='?', const=None) | ||
parser.add_argument("-y", "--yaml", help="yaml file that contains additional variables", action='append', default=[]) | ||
parser.add_argument("-j", "--json", help="json file that contains additional variables", action='append', default=[]) | ||
|
@@ -222,13 +225,18 @@ def main(): | |
|
||
data = {} | ||
hwsku = args.hwsku | ||
asic_name = args.namespace | ||
asic_id = None | ||
if asic_name is not None: | ||
asic_id = get_npu_id_from_name(asic_name) | ||
asic_role = parse_asic_sub_role(args.minigraph if args.minigraph else '/etc/sonic/minigraph.xml', asic_name) | ||
|
||
if hwsku is not None: | ||
hardware_data = {'DEVICE_METADATA': {'localhost': { | ||
'hwsku': hwsku | ||
}}} | ||
deep_update(data, hardware_data) | ||
(ports, _) = get_port_config(hwsku, platform, args.port_config) | ||
(ports, _, _) = get_port_config(hwsku, platform, args.port_config, asic_id) | ||
if not ports: | ||
print('Failed to get port config', file=sys.stderr) | ||
sys.exit(1) | ||
|
@@ -242,11 +250,11 @@ def main(): | |
minigraph = args.minigraph | ||
if platform: | ||
if args.port_config != None: | ||
deep_update(data, parse_xml(minigraph, platform, args.port_config)) | ||
deep_update(data, parse_xml(minigraph, platform, args.port_config, asic_name=asic_name)) | ||
else: | ||
deep_update(data, parse_xml(minigraph, platform)) | ||
deep_update(data, parse_xml(minigraph, platform, asic_name=asic_name)) | ||
else: | ||
deep_update(data, parse_xml(minigraph, port_config_file=args.port_config)) | ||
deep_update(data, parse_xml(minigraph, port_config_file=args.port_config, asic_name=asic_name)) | ||
|
||
if args.device_description != None: | ||
deep_update(data, parse_device_desc_xml(args.device_description)) | ||
|
@@ -267,11 +275,20 @@ def main(): | |
configdb.connect() | ||
deep_update(data, FormatConverter.db_to_output(configdb.get_config())) | ||
|
||
|
||
if args.platform_info: | ||
if asic_name is not None and asic_role.lower() == "backend": | ||
mac = get_system_mac(asic_name) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. namespace=asic_name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in latest commit |
||
else: | ||
mac = get_system_mac() | ||
|
||
hardware_data = {'DEVICE_METADATA': {'localhost': { | ||
'platform': platform, | ||
'mac': get_system_mac() | ||
'mac': mac, | ||
}}} | ||
# The ID needs to be passed to the SAI to identify the asic. | ||
if asic_name is not None: | ||
hardware_data['DEVICE_METADATA']['localhost'].update(asic_id=asic_id) | ||
deep_update(data, hardware_data) | ||
|
||
if args.template is not None: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ | |
import yaml | ||
import subprocess | ||
import re | ||
|
||
from natsort import natsorted | ||
DOCUMENTATION = ''' | ||
--- | ||
module: sonic_device_util | ||
|
@@ -17,6 +17,9 @@ | |
TODO: this file shall be renamed and moved to other places in future | ||
to have it shared with multiple applications. | ||
''' | ||
SONIC_DEVICE_PATH = '/usr/share/sonic/device' | ||
NPU_NAME_PREFIX = 'asic' | ||
|
||
def get_machine_info(): | ||
if not os.path.isfile('/host/machine.conf'): | ||
return None | ||
|
@@ -27,7 +30,45 @@ def get_machine_info(): | |
if len(tokens) < 2: | ||
continue | ||
machine_vars[tokens[0]] = tokens[1].strip() | ||
return machine_vars | ||
return machine_vars | ||
|
||
def get_npu_id_from_name(npu_name): | ||
if npu_name.startswith(NPU_NAME_PREFIX): | ||
return npu_name[len(NPU_NAME_PREFIX):] | ||
else: | ||
return None | ||
|
||
def get_num_npus(): | ||
platform = get_platform_info(get_machine_info()) | ||
asic_conf_file_path = os.path.join(SONIC_DEVICE_PATH, platform, 'asic.conf') | ||
if not os.path.isfile(asic_conf_file_path): | ||
return 1 | ||
with open(asic_conf_file_path) as asic_conf_file: | ||
lguohan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
for line in asic_conf_file: | ||
pavel-shirshov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
tokens = line.split('=') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mixed tab and spaces There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in latest commit |
||
if len(tokens) < 2: | ||
continue | ||
if tokens[0].lower() == 'num_asic': | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mixed tab and spaces, suggest to use retab to correct all of them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in latest commit |
||
num_npus = tokens[1].strip() | ||
return num_npus | ||
|
||
def get_namespaces(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i do not see this function used anywhere else in you code, in the multi npu system, why not use asic.conf to derive the namepsace you have on the system? why use ip netns list to query? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The plan is use this function by CLI commands. |
||
""" | ||
In a multi NPU platform, each NPU is in a Linux Namespace. | ||
This method returns list of all the Namespace present on the device | ||
""" | ||
ns_list = [] | ||
try: | ||
proc = subprocess.Popen('ip netns list | cut -d"(" -f1', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggest to use files to look files under /run/netns There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed in latest commit |
||
stdout=subprocess.PIPE, | ||
shell=True, | ||
stderr=subprocess.STDOUT) | ||
stdout = proc.communicate()[0] | ||
proc.wait() | ||
ns_list = [n for n in stdout.split()] | ||
except OSError,e: | ||
raise OSError("Unable to get namespace list") | ||
return natsorted(ns_list) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need to sort here. you do not know what order the user wants There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function is used mainly by CLI, in that case it is better if namespace list is sorted |
||
|
||
def get_platform_info(machine_info): | ||
if machine_info != None: | ||
|
@@ -51,7 +92,7 @@ def get_sonic_version_info(): | |
def valid_mac_address(mac): | ||
return bool(re.match("^([0-9A-Fa-f]{2}[:-]){5}([0-9A-Fa-f]{2})$", mac)) | ||
|
||
def get_system_mac(): | ||
def get_system_mac(namespace=None): | ||
version_info = get_sonic_version_info() | ||
|
||
if (version_info['asic_type'] == 'mellanox'): | ||
|
@@ -73,10 +114,14 @@ def get_system_mac(): | |
# Try valid mac in eeprom, else fetch it from eth0 | ||
platform = get_platform_info(get_machine_info()) | ||
hwsku = get_machine_info()['onie_machine'] | ||
profile_cmd = 'cat /usr/share/sonic/device/' + platform +'/'+ hwsku +'/profile.ini | cut -f2 -d=' | ||
profile_cmd = 'cat' + SONIC_DEVICE_PATH + '/' + platform +'/'+ hwsku +'/profile.ini | cut -f2 -d=' | ||
hw_mac_entry_cmds = [ profile_cmd, "sudo decode-syseeprom -m", "ip link show eth0 | grep ether | awk '{print $2}'" ] | ||
else: | ||
hw_mac_entry_cmds = [ "ip link show eth0 | grep ether | awk '{print $2}'" ] | ||
mac_address_cmd = "cat /sys/class/net/eth0/address" | ||
if namespace is not None: | ||
mac_address_cmd = "sudo ip netns exec {} {}".format(namespace, mac_address_cmd) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is the container eth0 mac address? how can make sure it is unique? is it randomly generated? i think it is hard to guarantee the mac is unique across different system in the dc and we could end up with duplicate macs. for frontend chip, you can still use the eth0 mac, only for backend chip you can use for docker generated mac. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The container eth0 mac is a dummy mac. Corresponding change in sonic-cfggen
|
||
|
||
hw_mac_entry_cmds = [mac_address_cmd] | ||
|
||
for get_mac_cmd in hw_mac_entry_cmds: | ||
proc = subprocess.Popen(get_mac_cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) | ||
|
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.
why do you need specify /etc/sonic/minigraph.xml, it is in line 196.
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 is needed when the sonic-cfggen command is executed without -m option.
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.
Should we show error in this case?
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.
There are a few platform related methods which execute 'sonic-cfggen -H' to get the platform data and other information from device_metadata.
To support these cases for multi-npu platforms, the miigraph from the default location will be used if no minigraph is provided.
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 In this case you need to use option --minigraph= and provide the required minigraph
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 to use the minigraph only when --minigraph option is provided.