Changes to make lldp show command for multi-npu platforms. by abdosi · Pull Request #914 · sonic-net/sonic-utilities

abdosi · 2020-05-13T23:37:41Z

- What I did
Enhanced below command to work for multi-npu platforms:-
show lldp table
show lldp neighbours [INTERFACE]

In the O/P we will filter out the LLDP information of Back plane Port/asic

Both this command are now wrapper of script/lldpshow which is now enhanced as below

usage: lldpshow [-h] [-v] [-d] [-p PORT]

Display the LLDP neighbors

optional arguments:
-h, --help show this help message and exit
-v, --version show program's version number and exit
-d, --detail LLDP neighbors detail information
-p PORT, --port PORT LLDP neighbors detail information for given port

                                  Examples:
                                  lldpshow
                                  lldpshow -d
                                  lldpshow -d -p Ethernet0

- How I did it
Updating script/lldpshow and using it
- How to verify it
Verified on both Single and Multi-NPU platforms
- Previous command output (if the output of a command-line utility has changed)
No change in O/P

We will display only front-panel port information.

abdosi · 2020-05-15T21:58:34Z

retest this please

abdosi · 2020-05-20T00:36:46Z

@jleveque @lguohan Can you please review this.

abdosi · 2020-05-22T05:29:22Z

@jleveque can you please review this.

jleveque

As comments. Also I notice this file uses "NPU" and "ASIC" interchangeably. As I have mentioned before, I think we need to unify this terminology. I have created an issue to track this: #922

scripts/lldpshow

abdosi · 2020-05-22T22:46:30Z

As comments. Also I notice this file uses "NPU" and "ASIC" interchangeably. As I have mentioned before, I think we need to unify this terminology. I have created an issue to track this: #922

@jleveque In this file made everything as ASIC

jleveque · 2020-05-22T22:50:17Z

As comments. Also I notice this file uses "NPU" and "ASIC" interchangeably. As I have mentioned before, I think we need to unify this terminology. I have created an issue to track this: #922

@jleveque In this file made everything as ASIC

@abdosi: I believe you mean you changed all references to "NPU", not "ASIC".

jleveque · 2020-05-22T22:50:41Z

scripts/lldpshow

Why is NPU namespace also an empty string? I thought that was reserved for the host namespace.

@jleveque This is just initial/default value to start with. The Interface list will get appended in the loop for asic namespace. We start with '' and then append to it.

I'm confused as to why the host namespace is the "default" in the NPU namespace. Maybe the naming is unclear?

@jleveque for LLDP we have instance in both host (for eth0) and per asic (for physical ports) namespace.

For host namespace using macro LLDP_INSTANCE_IN_HOST_NAMESPACE and LLDP_INTERFACE_LIST_IN_HOST_NAMESPACE.

For per asic namespace using macro LLDP_DEFAULT_INTERFACE_LIST_IN_ASIC_NAMESPACE which is just init value and the interface list gets populated in the loop.

@jleveque did I clarify your above concern ? let me know

I guess I'm still confused. So the empty string here does not denote the namespace, but rather denotes an interface name? Why do we need to populate an empty interface name here?

@jleveque Yes, We have empty string for both namespace and interface-list for host.

Empty interface-list is to keep logic generic since for host lldp docker as we do not filter of any interface and the command will be just 'lldpctl' + < Empty string >.
In case user has passed specific interface (-p option) command will be 'lldpctl' + < Interface-Num>.

For asic namespace we filter out Backplane interface (Ethernet-BP) and command will be 'lldpctl' + < Front Interface list >
or in case user has passed interface (-p option) command will be 'lldpctl' + < Interface-Num>.

I see. Thanks for the explanation. I guess it might just be the similarity of the constant names that is throwing me off here. I'd also like to get Pavel's input here, who is more of a SME on LLDP.

@jleveque can you please approve this. pavel has approved it.

abdosi · 2020-05-23T00:48:25Z

As comments. Also I notice this file uses "NPU" and "ASIC" interchangeably. As I have mentioned before, I think we need to unify this terminology. I have created an issue to track this: #922

@jleveque In this file made everything as ASIC

@abdosi: I believe you mean you changed all references to "NPU", not "ASIC".

@jleveque I think git had some issue with file update . It is "ASIC" since in minigraph we have ASIC already and we can not change that. Going foward as part of Issue #922 we will make NPU as ASIC

lgtm-com · 2020-05-23T00:52:04Z

This pull request introduces 1 alert when merging 2dd8bb7 into 221b593 - view on LGTM.com

new alerts:

1 for Syntax error

pavel-shirshov

As comment

pavel-shirshov · 2020-05-27T00:04:11Z

scripts/lldpshow

+    lldp_port = args.port
+
+    if lldp_port and not lldp_detail_info:
+        parser.error('-d is required when -p is set.')


why do we need this? Can we set -d automatically when -p?

@pavel-shirshov yes that is fine too. Updated accordingly.

@pavel-shirshov can you please review/approve the PR. Above comment has been addressed.

* Changes to make lldp show command for multi-npu platforms. We will display only front-panel port information. * Address Review Comments. * Added Comment * Fix LGTM error * Address Review Comments

…#914) * Changes to make lldp show command for multi-npu platforms. We will display only front-panel port information. * Address Review Comments. * Added Comment * Fix LGTM error * Address Review Comments

Make sure db_migrator is run after all config are loaded during (sonic-net#926) Vnet alias mapping (sonic-net#924) Changes to make lldp show command for multi-npu platforms. (sonic-net#914) [Mellanox] Fix thermal control issue: use natural sort for fan status and thermal status (sonic-net#836) [Mellanox] add document for thermal control related cli (sonic-net#832)

Changes to make lldp show command for multi-npu platforms.

becb87b

We will display only front-panel port information.

abdosi marked this pull request as ready for review May 13, 2020 23:41

abdosi requested a review from jleveque May 13, 2020 23:41

abdosi added Request for 201911 Branch Multi-ASIC 🔢 labels May 13, 2020

abdosi requested a review from lguohan May 14, 2020 17:54

jleveque suggested changes May 22, 2020

View reviewed changes

scripts/lldpshow Outdated Show resolved Hide resolved

scripts/lldpshow Outdated Show resolved Hide resolved

Address Review Comments.

32f934b

jleveque reviewed May 22, 2020

View reviewed changes

Added Comment

2dd8bb7

Fix LGTM error

5f6197a

jleveque requested a review from pavel-shirshov May 26, 2020 19:11

pavel-shirshov reviewed May 27, 2020

View reviewed changes

Address Review Comments

152f8f7

pavel-shirshov approved these changes May 28, 2020

View reviewed changes

jleveque approved these changes May 29, 2020

View reviewed changes

abdosi merged commit 811a4c6 into sonic-net:master May 29, 2020

abdosi deleted the master_lldp branch May 29, 2020 20:06

abdosi added the Included in 201911 Branch label Jun 3, 2020

Conversation

abdosi commented May 13, 2020

Uh oh!

abdosi commented May 15, 2020

Uh oh!

abdosi commented May 20, 2020

Uh oh!

abdosi commented May 22, 2020

Uh oh!

jleveque left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

abdosi commented May 22, 2020

Uh oh!

jleveque commented May 22, 2020

Uh oh!

jleveque May 22, 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!

abdosi May 26, 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!

abdosi commented May 23, 2020

Uh oh!

lgtm-com bot commented May 23, 2020

Uh oh!

pavel-shirshov left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jleveque May 22, 2020 •

edited

Loading

abdosi May 26, 2020 •

edited

Loading