Parser changes to support parsing of multi-asic device minigraph #4222

SuvarnaMeenakshi · 2020-03-04T23:05:15Z

- What I did
In case of multiasic platform, a single minigraph.xml will be present for the device. This minigraph.xml will include device specific data and internal asic specific data. The current parser assumes that the minigraph will have data specific to a single device. In a multiple asic minigraph.xml, each asic data will be modelled as a separate device. With this change, expectation is to be able to parse and generate configuration for all asics of the device.
Apart from this, port_config.ini will have a new column to provide "asic_port_name". This name be a combination of "port name in asic" and "asic name".

- How I did it

Made changes to portconfig.py to parse the new column if it exists and create a new mapping to map alias to asic_port_name which will be used in minigraph.py
minigraph.py updated to accept a new parameter "hostname" which will define the name of specific host/ASIC that is of interest and configuration will be generated for this host. If "hostname" is not provided, this will be the same as what is defined in minigraph.xml.

- How to verify it
Generated config_db.json before and after the parser changes for a single asic device. No change observed.
- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

to support multi-asic platforms. portconfig.py updated to parse additional column "asic_name" in port_config.ini minigraph.py updated to parse minigraph.xml of a multi-asic device which includes multiple asic data.

added in port table only if it is present in port_config.ini.

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>

lguohan · 2020-04-05T21:58:15Z

need unit test on the sonic-cfggen part to parse multi-asic minigraph.

lguohan · 2020-04-05T22:00:41Z

src/sonic-config-engine/sonic_device_util.py

+		num_npus = token[1].strip()
+   		return num_npus
+
+def get_namespaces():


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?

The plan is use this function by CLI commands.
Today the namespace are names as "asicX" , it could change in the future so didn't want to generate the name from asic.conf here.

src/sonic-config-engine/sonic-cfggen

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

lgtm-com · 2020-04-07T18:50:10Z

This pull request introduces 1 alert when merging d4fe8e0 into 2a59551 - view on LGTM.com

new alerts:

1 for Unused local variable

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>

arlakshm · 2020-04-13T20:44:38Z

need unit test on the sonic-cfggen part to parse multi-asic minigraph.

Added Unit test for parsing multi-npu minigraph.

rlhui · 2020-04-15T18:28:06Z

retest this please

src/sonic-config-engine/minigraph.py

lguohan · 2020-04-15T18:40:29Z

src/sonic-config-engine/sonic-cfggen

+    asic_id = None
+    if asic_name is not None:
+        asic_id = asic_name[-1]
+        asic_role = parse_asic_sub_role(args.minigraph if args.minigraph else '/etc/sonic/minigraph.xml', asic_name)


why do you need specify /etc/sonic/minigraph.xml, it is in line 196.

This is needed when the sonic-cfggen command is executed without -m option.

Should we show error in this case?

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.

I think In this case you need to use option --minigraph= and provide the required minigraph

Changed to use the minigraph only when --minigraph option is provided.

src/sonic-config-engine/sonic_device_util.py

src/sonic-config-engine/tests/multi_npu_data/sample_port_config-1.ini

lguohan · 2020-04-16T10:16:19Z

can you resolve conflict?

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>

…_changes Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

SuvarnaMeenakshi · 2020-04-16T17:13:42Z

can you resolve conflict?

resolved conflicts.

lgtm-com · 2020-04-16T17:24:49Z

This pull request fixes 1 alert when merging 9c9f8c2 into 695652c - view on LGTM.com

fixed alerts:

1 for Unused import

pavel-shirshov

As comments. I'll check minigraph part later

src/sonic-config-engine/tests/test_multinpu_cfggen.py

pavel-shirshov · 2020-04-17T00:26:24Z

src/sonic-config-engine/tests/test_multinpu_cfggen.py

+    def test_read_yaml(self):
+        argument = '-v yml_item -y ' + os.path.join(self.test_dir, 'test.yml')
+        output = self.run_script(argument)
+        self.assertEqual(output.strip(), '[\'value1\', \'value2\']')


assertListEqual
https://docs.python.org/2/library/unittest.html#unittest.TestCase.assertListEqual

changed in latest commit

src/sonic-config-engine/tests/test_multinpu_cfggen.py

pavel-shirshov · 2020-04-17T00:29:00Z

src/sonic-config-engine/tests/test_multinpu_cfggen.py

+    def test_metadata_tacacs(self):
+        argument = '-m "' + self.sample_graph +  '" -v "TACPLUS_SERVER"'
+        output = self.run_script(argument)
+        self.assertEqual(output.strip(),"{'123.46.98.21': {'priority': '1', 'tcp_port': '49'}}")


assertDictEqual

changed in latest commit

src/sonic-config-engine/tests/test_multinpu_cfggen.py

src/sonic-config-engine/sonic_device_util.py

pavel-shirshov · 2020-04-17T01:44:55Z

src/sonic-config-engine/sonic-cfggen

+    asic_id = None
+    if asic_name is not None:
+        asic_id = asic_name[-1]
+        asic_role = parse_asic_sub_role(args.minigraph if args.minigraph else '/etc/sonic/minigraph.xml', asic_name)


Should we show error in this case?

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>

lgtm-com · 2020-04-17T11:02:15Z

This pull request introduces 1 alert and fixes 1 when merging 5b6892e into 744d33d - view on LGTM.com

new alerts:

1 for Variable defined multiple times

fixed alerts:

1 for Unused import

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>

lgtm-com · 2020-04-19T00:37:04Z

This pull request introduces 1 alert and fixes 1 when merging 78a2bf7 into d8b7166 - view on LGTM.com

new alerts:

1 for Variable defined multiple times

fixed alerts:

1 for Unused import

concise and readable. Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

…ic-buildimage into comment_fix

lgtm-com · 2020-05-02T00:52:01Z

This pull request fixes 1 alert when merging 110d1b8 into c55603f - view on LGTM.com

fixed alerts:

1 for Unused import

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

lgtm-com · 2020-05-02T01:27:22Z

This pull request fixes 1 alert when merging 247370b into c55603f - view on LGTM.com

fixed alerts:

1 for Unused import

lguohan · 2020-05-02T01:49:56Z

src/sonic-config-engine/minigraph.py

+            elif child.tag == str(QName(ns, "CpgDec")):
+                (bgp_sessions, bgp_asn, bgp_peers_with_range, bgp_monitors) = parse_cpg(child, asic_name)
+            elif child.tag == str(QName(ns, "UngDec")):
+                (u_neighbors, u_devices, _, _) = parse_asic_png(child, asic_name, hostname) 


fixed as per comment

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

rlhui · 2020-05-02T17:24:47Z

retest vsimage please

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>

lgtm-com · 2020-05-03T05:34:08Z

This pull request fixes 1 alert when merging 06983e4 into 30bbbbf - view on LGTM.com

fixed alerts:

1 for Unused import

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>

lgtm-com · 2020-05-04T08:42:41Z

This pull request fixes 1 alert when merging 17a0d04 into 30bbbbf - view on LGTM.com

fixed alerts:

1 for Unused import

lguohan

…e minigraph (#4222) - Changes to minigraph.py to parse minigraph.xml of a multi asic platform - Changes to portconfig.py to parse additional column "asic_port_name" in port_config.ini - Add a new option -n to sonic-cfggen for multi asic platforms - Add unit tests for config generation for multi asic platforms Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com> Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>

qiluo-msft · 2020-05-07T04:04:13Z

src/sonic-config-engine/minigraph.py

+            elif child.tag == str(QName(ns, "PngDec")):
+                (neighbors, devices, console_dev, console_port, mgmt_dev, mgmt_port, port_speed_png, console_ports) = parse_png(child, hostname)
+            elif child.tag == str(QName(ns, "UngDec")):
+                (u_neighbors, u_devices, _, _, _, _, _, _) = parse_png(child, device_hostname)


If you use some local linter, you will find an error here.

[flake8] undefined name 'device_hostname' [Error]

What I did get_port_config was modified to return different set number of arguments in PR: sonic-net/sonic-buildimage#4222. Changes in this PR is: To address the different set of return values To get the ports from all namespaces To add unit-test How I did it Modify port2alias to read ports from all namespaces and also add unit-test How to verify it Test on single and multi-asic platforms. unit-test passed.

What I did get_port_config was modified to return different set number of arguments in PR: sonic-net/sonic-buildimage#4222. Changes in this PR is: To address the different set of return values To get the ports from all namespaces To add unit-test How I did it Modify port2alias to read ports from all namespaces and also add unit-test How to verify it Test on single and multi-asic platforms. unit-test passed. (cherry picked from commit 3714f63) Signed-off-by: Suvarna Meenakshi <sumeenak@microsoft.com>

What I did Cherry-pick of #1906 to 202012 branch Fix conflict also few changes done to cherry-pick: get_port_config function takes asic id as argument in 2019/202012 branches Keep load_source of port2alias as module, as is used in unit-test in 202012 branch get_port_config was modified to return different set number of arguments in PR: sonic-net/sonic-buildimage#4222. Changes in this PR is: To address the different set of return values To get the ports from all namespaces To add unit-test Additional change done over the cherry-pick: added setup_class method in test case to load single asic mock db, this was done in pfcwd_test teardown method in 201911 and master branches. It is not done in 202012 branch, hence added setup_class method. How I did it Modify port2alias to read ports from all namespaces and also add unit-test How to verify it Verified on single and multi-asic DUTs with 202012 image.

What I did get_port_config was modified to return different set number of arguments in PR: sonic-net/sonic-buildimage#4222. Changes in this PR is: To address the different set of return values To get the ports from all namespaces To add unit-test How I did it Modify port2alias to read ports from all namespaces and also add unit-test How to verify it Test on single and multi-asic platforms. unit-test passed.

SuvarnaMeenakshi added 2 commits March 3, 2020 18:48

[minigraph.py]: Changes added in minigraph and port_config parser

3ce843a

to support multi-asic platforms. portconfig.py updated to parse additional column "asic_name" in port_config.ini minigraph.py updated to parse minigraph.xml of a multi-asic device which includes multiple asic data.

[minigraph.py/portconfig.py]: Minor fixes

ff6aa99

SuvarnaMeenakshi assigned arlakshm Mar 4, 2020

SuvarnaMeenakshi changed the title ~~Parser changes to support parsing of mutli-asic device minigraph~~ Parser changes to support parsing of multi-asic device minigraph Mar 4, 2020

SuvarnaMeenakshi and others added 2 commits March 13, 2020 11:56

[minigraph.py]: Updated to make sure that interfaces names are

b784c81

added in port table only if it is present in port_config.ini.

Changes to sonic-cfggen for multi-NPU platforms

9b4039e

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>

lguohan reviewed Apr 5, 2020

View reviewed changes

src/sonic-config-engine/sonic-cfggen Outdated Show resolved Hide resolved

[sonic-cfggen]: Fix spacing.

d4fe8e0

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

Minor fixes and UT tests

53c6693

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>

lguohan reviewed Apr 15, 2020

View reviewed changes

src/sonic-config-engine/minigraph.py Outdated Show resolved Hide resolved

lguohan reviewed Apr 15, 2020

View reviewed changes

src/sonic-config-engine/sonic_device_util.py Outdated Show resolved Hide resolved

lguohan reviewed Apr 15, 2020

View reviewed changes

src/sonic-config-engine/tests/multi_npu_data/sample_port_config-1.ini Outdated Show resolved Hide resolved

lguohan added the minigraph 🗺️ label Apr 16, 2020

lguohan requested a review from pavel-shirshov April 16, 2020 10:15

arlakshm and others added 2 commits April 16, 2020 08:13

Addressing Review comments

a9a06a8

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>

Merge remote-tracking branch 'remotes/sumeenak/master' into minigraph…

9c9f8c2

…_changes Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

pavel-shirshov suggested changes Apr 17, 2020

View reviewed changes

addressing review comments

5b6892e

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>

addressing review comments

78a2bf7

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>

SuvarnaMeenakshi added 3 commits May 1, 2020 17:10

[minigraph.py]: Split parse_asic_png function to make it

ebf2e8b

concise and readable. Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

[minigraph.py]: Correction in parse_asic_png function

520c43b

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

Merge branch 'multiasic_minigraph' of github.com:SuvarnaMeenakshi/son…

110d1b8

…ic-buildimage into comment_fix

Minor fixes.

247370b

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

lguohan reviewed May 2, 2020

View reviewed changes

Minor fixes as per review comments.

cd3a678

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>

Addressing review comments

06983e4

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>

Fixes from testing

17a0d04

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>

lguohan approved these changes May 4, 2020

View reviewed changes

pavel-shirshov approved these changes May 4, 2020

View reviewed changes

SuvarnaMeenakshi added the Request for 201911 Branch label May 4, 2020

arlakshm merged commit 8ac1c60 into sonic-net:master May 4, 2020

abdosi added the Included in 201911 Branch label May 5, 2020

qiluo-msft reviewed May 7, 2020

View reviewed changes

SuvarnaMeenakshi mentioned this pull request May 7, 2020

[minigraph.py]: Fix undefined variable error. #4555

Merged

SuvarnaMeenakshi deleted the multiasic_minigraph branch December 14, 2020 09:28

This was referenced Nov 1, 2021

[port2alias]: Fix to get right number of return values sonic-net/sonic-utilities#1906

Merged

[201911][port2alias]: Fix to get right number of return values sonic-net/sonic-utilities#1907

Merged

SuvarnaMeenakshi mentioned this pull request May 27, 2022

[20212][port2alias]: Fix to get right number of return values sonic-net/sonic-utilities#2188

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parser changes to support parsing of multi-asic device minigraph #4222

Parser changes to support parsing of multi-asic device minigraph #4222

SuvarnaMeenakshi commented Mar 4, 2020

lguohan commented Apr 5, 2020

lguohan Apr 5, 2020

arlakshm Apr 13, 2020

lgtm-com bot commented Apr 7, 2020

arlakshm commented Apr 13, 2020

rlhui commented Apr 15, 2020

lguohan Apr 15, 2020

arlakshm Apr 16, 2020

pavel-shirshov Apr 17, 2020

arlakshm Apr 17, 2020

pavel-shirshov Apr 28, 2020

arlakshm May 3, 2020 •

edited

Loading

lguohan commented Apr 16, 2020

SuvarnaMeenakshi commented Apr 16, 2020

lgtm-com bot commented Apr 16, 2020

pavel-shirshov left a comment

pavel-shirshov Apr 17, 2020

arlakshm Apr 17, 2020

pavel-shirshov Apr 17, 2020

arlakshm Apr 17, 2020

pavel-shirshov Apr 17, 2020

lgtm-com bot commented Apr 17, 2020

lgtm-com bot commented Apr 19, 2020

lgtm-com bot commented May 2, 2020

lgtm-com bot commented May 2, 2020

lguohan May 2, 2020

SuvarnaMeenakshi May 2, 2020

rlhui commented May 2, 2020

lgtm-com bot commented May 3, 2020

lgtm-com bot commented May 4, 2020

lguohan left a comment

qiluo-msft May 7, 2020

Parser changes to support parsing of multi-asic device minigraph #4222

Parser changes to support parsing of multi-asic device minigraph #4222

Conversation

SuvarnaMeenakshi commented Mar 4, 2020

lguohan commented Apr 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Apr 7, 2020

arlakshm commented Apr 13, 2020

rlhui commented Apr 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arlakshm May 3, 2020 • edited Loading

Choose a reason for hiding this comment

lguohan commented Apr 16, 2020

SuvarnaMeenakshi commented Apr 16, 2020

lgtm-com bot commented Apr 16, 2020

pavel-shirshov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Apr 17, 2020

lgtm-com bot commented Apr 19, 2020

lgtm-com bot commented May 2, 2020

lgtm-com bot commented May 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rlhui commented May 2, 2020

lgtm-com bot commented May 3, 2020

lgtm-com bot commented May 4, 2020

lguohan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arlakshm May 3, 2020 •

edited

Loading