[master] [show] Option to add subcommands to show techsupport #2974

gvlaemyn · 2023-09-08T07:45:03Z

What I did

Implement option to add subcommands to "show techsupport", e.g. "show techsupport npu"
to have to possibility of restricting the collected data to a subset of the full techsupport data.

How I did it

* replace techsupport click command in show/main.py with click group to support subcommands
* update show/plugins/cisco-8000.py to support vendor specific commands
* subcommand functionality itself (e.g. npu) is implemented in a vendor specific repository and requires no further changes in sonic-utilities

How to verify it

E.g. for the "show techsupport npu" command on a cisco-8000 platform:

Check if the "npu" subcommand is available:
root@sonic:~# show techsupport -h
Usage: show techsupport [OPTIONS] COMMAND [ARGS]...

Gather information for troubleshooting

Options:
--since TEXT Collect logs and core files since given date
-g, --global-timeout INTEGER Global timeout in minutes. WARN: Dump might be
incomplete if enforced
-c, --cmd-timeout INTEGER Individual command timeout in minutes. Default
5 mins
--verbose Enable verbose output
--allow-process-stop Dump additional data which may require system
interruption
--silent Run techsupport in silent mode
--debug-dump Collect Debug Dump Output
-r, --redirect-stderr Redirect an intermediate errors to STDERR
-?, -h, --help Show this message and exit.

Commands:
npu Gather platform npu information

Run "show techsupport npu" and verify an archive with the collected data is created:
root@sonic:~# show techsupport npu
. . .
/var/dump/sonic_npu_dump_hostname_20230828_105958.tar.gz

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

* replace techsupport click command with click group to support subcommands * update plugins/cisco-8000.py to support vendor specific commands Signed-off-by: Geert Vlaemynck <[email protected]>

kevinskwang · 2023-09-27T00:58:31Z

show/main.py

    """Gather information for troubleshooting"""
+    if ctx.invoked_subcommand:
+        return


why return here?

We return here to run only the subcommand, e.g. "show techsupport npu" which can be defined in a vendor platform package, instead of the main "show techsupport".

anamehra · 2023-09-27T19:38:00Z

Thanks @kevinskwang ! Please help to rerun the build pipeline. Thanks

Signed-off-by: Geert Vlaemynck <[email protected]>

gvlaemyn · 2023-10-24T08:51:27Z

@kevinskwang Thanks for your previous review and approval. Merging was blocked because of a code coverage issue. This has been addressed in a test case in my latest commit. Could you review and approve if appropriate?

qiluo-msft · 2023-10-26T16:41:52Z

show/plugins/cisco-8000.py

+    from sonic_platform.cli import VENDOR_CLIS
+except ImportError:
+    VENDOR_CLIS = {}
+

 def register(cli):
    version_info = device_info.get_sonic_version_info()
    if version_info and version_info.get("asic_type") == "cisco-8000":
        for c in PLATFORM_CLIS:
            cli.commands["platform"].add_command(c)


platform

platform is a special case of command. Please unified two for-block.

The reason for the two separate loops is to not break backward compatibility in cisco platform specific code which only uses the PLATFORM_CLIS list. New cisco platform specific code will use the VENDOR_CLIS dict, so until all cisco code has been migrated to using VENDOR_CLIS we really do need both for loops.

Hi @qiluo-msft , if @gvlaemyn's explanation is acceptable could you help with the approval?

Hi @qiluo-msft , if Geert's explanation is acceptable, please help with the approval. Otherwise, we could arrange for a quick meeting to discuss. Thank you.

hi @qiluo-msft and @kevinskwang , could you review and approve.

What is the plan and ETA for "all cisco code has been migrated to using VENDOR_CLIS"? If I do not ask, will it happen finally?

Reading the VENDOR_CLIS dictionary in the cisco-8000 plugin was added in this PR just for being able to add the "show techsupport npu" command. My previous comment "until all cisco code has been migrated to using VENDOR_CLIS" may have given the impression that this PR is depending on a lot of internal cisco code to be modified: this is not the case. I should have commented the PLATFORM_CLIS list may not be required to be read anymore in the cisco-8000 plugin at some future time, but currently we need both PLATFORM_CLIS and VENDOR_CLIS to be read.

@qiluo-msft @kevinskwang Does the updated code in plugins/cisco-8000.py address your concerns? Thanks.

Hi @qiluo-msft , please let us know if @gvlaemyn's comments addressed your concerns and if we can go ahead with this PR.
cc: @abdosi

anamehra · 2023-12-14T20:37:50Z

Hi @abdosi , @rlhui , could you please help with this PR?

jamesan47 · 2024-03-05T17:52:35Z

Hi @abdosi , @rlhui , could you please help with this PR for serviceability improvements?

jamesan47 · 2024-08-14T21:13:46Z

Hi @abdosi , @kevinskwang Please help with review, approval, and merge if no concerns. We would like to have it included in 202405.

Signed-off-by: Geert Vlaemynck <[email protected]>

gvlaemyn changed the title ~~[show] Option to add subcommands to show techsupport~~ [202205] [show] Option to add subcommands to show techsupport Sep 8, 2023

[show] Option to add subcommands to show techsupport

f30b50f

* replace techsupport click command with click group to support subcommands * update plugins/cisco-8000.py to support vendor specific commands Signed-off-by: Geert Vlaemynck <[email protected]>

gvlaemyn force-pushed the cisco-plugin branch from d0966ba to f30b50f Compare September 12, 2023 10:39

gvlaemyn changed the base branch from 202205 to master September 12, 2023 10:40

gvlaemyn changed the title ~~[202205] [show] Option to add subcommands to show techsupport~~ [master] [show] Option to add subcommands to show techsupport Sep 12, 2023

kevinskwang previously approved these changes Sep 27, 2023

View reviewed changes

gvlaemyn added 2 commits October 13, 2023 14:31

Merge branch 'sonic-net:master' into cisco-plugin

5ae04aa

Add unit test for show techsupport npu

6f0252a

Signed-off-by: Geert Vlaemynck <[email protected]>

gvlaemyn dismissed kevinskwang’s stale review via 6f0252a October 18, 2023 08:39

gvlaemyn requested a review from kevinskwang October 19, 2023 10:11

StormLiangMS requested a review from qiluo-msft October 25, 2023 13:23

qiluo-msft reviewed Oct 26, 2023

View reviewed changes

gvlaemyn added 2 commits August 22, 2024 10:05

Unify for-blocks in cisco-8000 plugin

1cac9ce

Signed-off-by: Geert Vlaemynck <[email protected]>

Add condition for using PLATFORM_CLIS

062649f

Signed-off-by: Geert Vlaemynck <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[master] [show] Option to add subcommands to show techsupport #2974

[master] [show] Option to add subcommands to show techsupport #2974

gvlaemyn commented Sep 8, 2023

kevinskwang Sep 27, 2023

gvlaemyn Sep 27, 2023

anamehra commented Sep 27, 2023

gvlaemyn commented Oct 24, 2023

qiluo-msft Oct 26, 2023

gvlaemyn Oct 27, 2023

jamesan47 Nov 2, 2023

jamesan47 Nov 20, 2023

jamesan47 Dec 14, 2023

qiluo-msft Apr 2, 2024

gvlaemyn Apr 9, 2024

gvlaemyn Aug 26, 2024

anamehra Feb 25, 2025

anamehra commented Dec 14, 2023

jamesan47 commented Mar 5, 2024

jamesan47 commented Aug 14, 2024

[master] [show] Option to add subcommands to show techsupport #2974

Are you sure you want to change the base?

[master] [show] Option to add subcommands to show techsupport #2974

Conversation

gvlaemyn commented Sep 8, 2023

What I did

How I did it

How to verify it

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anamehra commented Sep 27, 2023

gvlaemyn commented Oct 24, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anamehra commented Dec 14, 2023

jamesan47 commented Mar 5, 2024

jamesan47 commented Aug 14, 2024