Common functions for Multi ASIC CLIs by arlakshm · Pull Request #4973 · sonic-net/sonic-buildimage

arlakshm · 2020-07-15T01:21:34Z

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan [email protected]

- Why I did it
The PR implements few common functions needed for CLI and other modules.
- How I did it

The following common APIs are added for multi ASIC

an API to check if a given port is a internal or external port
an API to check if a given port-channel is internal or external
an API to check if a bgp-session is internal or external
an API to connect to the config and other dbs in the a given namespace
added common APIs to the sonic_py_common library.
update the sample port-config.ini with role column and add corresponding test to verify the ports configuration is - generated properly.

- Description for the changelog

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

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <[email protected]>

smaheshm · 2020-07-16T16:25:51Z

src/sonic-config-engine/sonic_device_util.py

+    if namespace is None:
+        ns_list  = get_namespaces()
+    else:
+        ns_list = [namespace]


check of valid namepsace it not already checked. Raise exception if invalid namespace.

IMO, the namespace validation should be done by the caller ( CLI handler)

smaheshm · 2020-07-16T16:41:31Z

src/sonic-config-engine/sonic_device_util.py

+        members = port_channels[port_channel]['members']
+        for member in members:
+            if is_port_internal(member):
+                return True


This logic will be intensive for 'external' ports. It'll call 'get_port_config_from_all_asics()' for all member ports. Any reason why you are checking all members for 'external' role and only one member for 'internal' role.

will this work:

return is_port_internal(members[0])

Fixed in the latest commit

smaheshm · 2020-07-16T16:44:19Z

src/sonic-config-engine/sonic_device_util.py

+    if namespace is None:
+        ns_list  = get_namespaces()
+    else:
+        ns_list = [namespace]


validate namespace.

IMO, the namespace validation should be done by the caller ( CLI handler)

smaheshm · 2020-07-16T16:46:22Z

src/sonic-config-engine/sonic_device_util.py

+        bgp_neigh_name = bgp_sessions[bgp_neigh_ip]['name']
+        neighbor_metadata = config_db.get_table(NEIGH_DEVICE_METADATA_CFG_DB_TABLE)
+        if neighbor_metadata[bgp_neigh_name]['type'].lower() == 'asic':
+            return True


else:
return False?

Fixed in latest commit

smaheshm

Minor comments.

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <[email protected]>

arlakshm · 2020-07-20T14:38:15Z

retest vsimage please

judyjoseph · 2020-07-20T17:34:52Z

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

-Ethernet-BP8  21,22,23,24      Ethernet-BP8     Eth6-ASIC0
-Ethernet-BP12 25,26,27,28      Ethernet-BP12    Eth7-ASIC0
+# name          lanes          alias           asic_port_name    role 
+Ethernet0     33,34,35,36      Ethernet1/1      Eth0-ASIC0         E


I talked to Joe, as per him it would be good to add the "index" field also in port_config.ini files. There could be API's which use that.

What is the usage or significance of index here? There is a way an index is computed based on interface name, which is used in SNMP mainly.
https://github.com/Azure/sonic-py-swsssdk/blob/master/src/swsssdk/port_util.py#L26
Is the interface index compute in port_util same as index here?

@judyjoseph, @SuvarnaMeenakshi.
I have added port_index in the port_config.ini in the latest commit. The port index is a running number, there is no significance to it.

Thanks ! To mention the significance of "index" .. there are some places where the code check for presence of "index" in the port_config.ini eg: https://github.com/Azure/sonic-platform-common/blob/be1cc2475e65b764f7aab44785a779cc3d011ee9/sonic_platform_base/sonic_sfp/sfputilbase.py#L458

So it would be safer to have the index field.... as there are logic below which checks for len(line.split()) >= 4: etc. This will change for us as we add more columns now.

Also we would need to have separate index numbering for front-panel and back panel interfaces ...

Ethernet0 33,34,35,36 Ethernet1/1 0 Eth0-ASIC0 E
Ethernet4 29,30,31,32 Ethernet1/2 1 Eth1-ASIC0 E
...
Ethernet-BP0 13,14,15,16 Ethernet-BP0 0 Eth4-ASIC0 I
Ethernet-BP4 17,18,19,20 Ethernet-BP4 1 Eth5-ASIC0 I
....

Also we would need to have separate index numbering for front-panel and back panel interfaces ...

@judyjoseph, can you elaborate why we need separate index numbering for front-panel and back panel interface ?

In the platform code ( could be other places as well ) we are making a logical list of all the interfaces, and we are looking for only front panel interfaces which is significant. Currently when there is no "index" field in port_config.ini, the way it calculates the index is from the name "Ethernet0"(here it is 0/4 ==> 0), "Ethernet4" (here it is 4/4 ==> 1) etc.

So It would be ok to start the index from 0 separately per interface type and it will follow the interface naming/numbering convention also. Do you think there could be an issue ?

@judyjoseph, Changed the port_config.ini in the latest commit

jleveque · 2020-07-22T20:19:35Z

@arlakshm: Would it be possible to wait until #5003 merges, then add this functionality to that package instead?

arlakshm · 2020-07-22T20:49:46Z

@arlakshm: Would it be possible to wait until #5003 merges, then add this functionality to that package instead?

@jleveque : Sure, I will add them to sonic-py-common

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <[email protected]>

arlakshm · 2020-07-30T19:14:23Z

@arlakshm: Would it be possible to wait until #5003 merges, then add this functionality to that package instead?

@jleveque : Sure, I will add them to sonic-py-common

@jleveque: I moved the common functions to sonic-py-common. Please review and provide feedback

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <[email protected]>

abdosi · 2020-08-07T18:41:32Z

src/sonic-py-common/sonic_py_common/multi_asic.py

+        neighbor_metadata = config_db.get_table(
+            NEIGH_DEVICE_METADATA_CFG_DB_TABLE)
+
+        if neighbor_metadata[bgp_neigh_name]['type'].lower() == 'asic':


do we need to check if neighbor_metadata exist ?

added a check in latest commit

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <[email protected]>

arlakshm · 2020-08-11T17:44:17Z

@abdosi, @lguohan , @jleveque . All review comments are addressed. Please approve if there are no other comments

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

src/sonic-py-common/sonic_py_common/multi_asic.py

signoff

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <[email protected]>

src/sonic-py-common/sonic_py_common/multi_asic.py

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <[email protected]>

src/sonic-py-common/sonic_py_common/multi_asic.py

signoff

arlakshm · 2020-08-14T04:12:01Z

retest vsimage please

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan [email protected] The following common APIs are added for multi ASIC - an API to check if a given port is a internal or external port - an API to check if a given port-channel is internal or external - an API to check if a bgp-session is internal or external - an API to connect to the config and other dbs in the a given namespace - added common APIs to the sonic_py_common library. - update the sample port-config.ini with role column and add corresponding test to verify the ports configuration is - generated properly.

Changes for identifing the cli object's role

0c7d22f

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <[email protected]>

arlakshm requested review from SuvarnaMeenakshi, abdosi, judyjoseph and lguohan July 15, 2020 01:21

This comment has been minimized.

Sign in to view

Fix lgtm warning

0620982

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <[email protected]>

smaheshm reviewed Jul 16, 2020

View reviewed changes

arlakshm added 2 commits July 16, 2020 14:46

address review comment

00e34d0

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <[email protected]>

Fixed review comments

b133dc1

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <[email protected]>

judyjoseph reviewed Jul 20, 2020

View reviewed changes

Add index to port_config and minor fixes

3c671c1

arlakshm mentioned this pull request Jul 20, 2020

Common functions for show CLI support on multi ASIC sonic-net/sonic-utilities#999

Merged

This was referenced Jul 24, 2020

show interface portchannel support for Multi ASIC sonic-net/sonic-utilities#1005

Merged

support show interface commands for multi ASIC platforms sonic-net/sonic-utilities#1006

Merged

arlakshm added 2 commits July 27, 2020 11:17

Merge remote-tracking branch 'arlakshm/master' into masic_port_role

51adf86

move common functions to sonic_py_common package

4985d2d

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <[email protected]>

arlakshm requested a review from jleveque July 30, 2020 19:14

This comment has been minimized.

Sign in to view

fix compilation issue

525b6b4

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <[email protected]>

This comment has been minimized.

Sign in to view

arlakshm mentioned this pull request Jul 30, 2020

show interface counters for multi ASIC devices sonic-net/sonic-utilities#1013

Merged

abdosi reviewed Aug 7, 2020

View reviewed changes

arlakshm added 4 commits August 7, 2020 15:30

address review commit

6a3009c

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <[email protected]>

change the port in port_config to Ext\Int

7628bbd

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <[email protected]>

Merge remote-tracking branch 'arlakshm/master' into masic_port_role

b4837ff

revert the changes in device_info.py

cabc2f3

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <[email protected]>

judyjoseph previously approved these changes Aug 11, 2020

View reviewed changes

abdosi previously approved these changes Aug 11, 2020

View reviewed changes

jleveque suggested changes Aug 12, 2020

View reviewed changes

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

src/sonic-py-common/sonic_py_common/multi_asic.py Outdated Show resolved Hide resolved

src/sonic-py-common/sonic_py_common/multi_asic.py Outdated Show resolved Hide resolved

arlakshm mentioned this pull request Aug 12, 2020

Support "show cli" for Multi Npu platforms. sonic-net/sonic-utilities#889

Closed

address review comments

789a548

signoff

arlakshm dismissed stale reviews from abdosi and judyjoseph via 789a548 August 12, 2020 17:25

arlakshm added 2 commits August 12, 2020 12:17

For cli get the namespace list from linux

b65012e

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <[email protected]>

Merge remote-tracking branch 'arlakshm/master' into masic_port_role

f1814c5

arlakshm requested review from abdosi and jleveque August 13, 2020 20:32

jleveque suggested changes Aug 13, 2020

View reviewed changes

src/sonic-py-common/sonic_py_common/multi_asic.py Outdated Show resolved Hide resolved

fix import spacing

f1a4f07

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <[email protected]>

jleveque suggested changes Aug 13, 2020

View reviewed changes

src/sonic-py-common/sonic_py_common/multi_asic.py Outdated Show resolved Hide resolved

address review comments

0b0cfd5

signoff

jleveque approved these changes Aug 13, 2020

View reviewed changes

arlakshm added Multi-ASIC Request for 201911 Branch labels Aug 14, 2020

arlakshm merged commit 6c89551 into sonic-net:master Aug 14, 2020

abdosi added the Included in 201911 Branch label Aug 19, 2020

gechiang mentioned this pull request Aug 21, 2020

Multi-ASIC support for show ip route sonic-net/sonic-utilities#1060

Closed

gechiang mentioned this pull request Sep 2, 2020

Multi-ASIC support for show ip route sonic-net/sonic-utilities#1089

Closed

Conversation

arlakshm commented Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smaheshm Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smaheshm left a comment

Choose a reason for hiding this comment

Uh oh!

arlakshm commented Jul 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jleveque commented Jul 22, 2020

Uh oh!

arlakshm commented Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arlakshm commented Jul 30, 2020

Uh oh!

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arlakshm commented Aug 11, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arlakshm commented Aug 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

arlakshm commented Jul 15, 2020 •

edited

Loading

smaheshm Jul 16, 2020 •

edited

Loading

arlakshm commented Jul 22, 2020 •

edited

Loading