Adding global-timeout, individual command timeout, log files collection by FuzailBrcm · Pull Request #1249 · sonic-net/sonic-utilities

FuzailBrcm · 2020-11-18T03:31:01Z

and some other enhancements to techsupport

- What I did
Following is the brief description of the changes,

Adding a ‘--silent’ option to ‘show techsupport’ command. Various tar/untar, addition and removal logs appear on the console by default. This option would disable above logs.
Adding global and per-command timeouts. This would provide more user control on ‘show techsupport’ CLI.
Adding time profiling information for the commands in techsupport. Time profiling information would be part of the tarball and helps to analyse the time consumption per command.
Sometimes ‘syncd’ docker is down and bcmshell is unavailable. In such cases all the bcmcmd commands would timeout and result in tremendous increase in the total techsupport collection time. We provided an option to skip rest of the bcmcmd commands once one command times out.
Added ‘show services’, ‘show reboot-cause’ and various BGP, BFD, bcm shell and other commands
Optimised the /var/log files collection. If the number of files are large in /var/log folder, it takes a long time to add each individually to the tarball. If the folder is tar'ed at once, the time taken reduces significantly.
Following error was observed while tar'ing softlinks inside .etc folder.
** Tar append operation failed. Aborting for safety. **
This issue was due to softlinks present at /etc folder where the destination file is absent. Fixed this issue by deleting such softlinks before adding them to the tarball.

- How I did it

Added new options to the CLICK command 'show techsupport'
Modified the 'generate_dump' script to accomodate other changes

- How to verify it
Here are some outputs,
root@sonic:/home/admin# show techsupport --silent
Techsupport is running with silent option. This command might take a long time.

HW Mgmt dump script /usr/bin/hw-management-generate-dump.sh does not exist
/var/dump/sonic_dump_sonic_20201117_161246.tar.gz
root@sonic:/home/admin#
root@sonic:~# show techsupport -h
Usage: show techsupport [OPTIONS]

Gather information for troubleshooting

Options:
--since TEXT Collect logs and core files since given date
-g, --global-timeout INTEGER Global timeout for techsupport in minutes.
Default 30 mins
-c, --cmd-timeout INTEGER Command timeout for techsupport in minutes.
Default 5 mins
--verbose Enable verbose output
--silent Run techsupport in silent mode
-?, -h, --help Show this message and exit.
root@sonic:~#

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

Previous command "show techsupport" works as is
- New command output (if the output of a command-line utility has changed)

FuzailBrcm · 2020-11-18T04:31:47Z

retest default please

FuzailBrcm · 2020-11-18T04:38:14Z

retest this please

FuzailBrcm · 2020-11-18T10:46:17Z

retest this please

smaheshm · 2020-12-09T21:23:23Z

Thanks for enhancing the tech support collection. One suggestion I have is next time PRs like this can be split into multiple PRs with each PR having a specific fix or enhancement. This makes it easier to revert a PR when there's an issue, also the PR looks cleaner and easier on the reviewer.

smaheshm · 2020-12-09T21:31:14Z

Have you raised or going to raise a PR for tech support unit tests
https://github.com/Azure/sonic-mgmt/tree/master/tests/show_techsupport

smaheshm

I'd suggest to break this into multiple PRs, up to you.

FuzailBrcm

As most of the review comments are regarding the multi_asic, I would like to keep all this in single PR.
I have done manual testing
I am adding some tests in sonic-mgmt for the changes

smaheshm · 2020-12-14T21:44:35Z

Thanks for making the changes. Looks good.
My main concern is the unit test for existing test cases. Would like to see the output of pytest.

Since you added new features, in case you are still working on writing unit tests for the new features, would like to see the output. For example.. the output of "global timeout" case and the "command timeout" case. I'm sure you've tested it, just 'copy/paste' the output.

Regarding "I am adding some tests in sonic-mgmt for the changes" -- have you raised a PR or are working on it.

Adding @qiluo-msft for another pair of eyes.

Rest looks good to me.

lguohan · 2020-12-16T22:13:53Z

+
    save_cmd "cat /proc/bcm/knet/debug" "broadcom.knet.debug"
    save_cmd "cat /proc/bcm/knet/dma" "broadcom.knet.dma"
-    save_cmd "cat /proc/bcm/knet/dstats" "broadcom.knet.dstats"


not sure why this is removed

It is not removed. These stats were moved under save_counter_snapshot() function. We need to take two snapshots of the counter stats to get the counter trend.

ben-gale · 2020-12-16T22:31:40Z

Note that this PR does not contain sonic-utilities UT because we are extending a command that does not have such a test today. However we will fix this in a follow-up PR.

FuzailBrcm · 2020-12-17T01:58:20Z

retest this please

lguohan · 2020-12-17T03:25:12Z

Thanks for enhancing the tech support collection. One suggestion I have is next time PRs like this can be split into multiple PRs with each PR having a specific fix or enhancement. This makes it easier to revert a PR when there's an issue, also the PR looks cleaner and easier on the reviewer.

agree, that should be the rule. @ben-gale

FuzailBrcm · 2020-12-17T16:06:07Z

retest this please

FuzailBrcm · 2020-12-18T03:54:13Z

retest this please

smaheshm

Looks good. Please wait for one more approval.

ben-gale · 2020-12-18T20:53:01Z

@lguohan - Can this one merge now pls?

qiluo-msft · 2020-12-19T18:39:18Z

+    local filepath="${LOGDIR}/$filename"
+    local do_gzip=${3:-false}
+    local tarpath="${BASE}/dump/$filename"
+    local timeout_cmd="timeout --foreground ${TIMEOUT_MIN}m"


timeout [](start = 23, length = 7)

bcmcmd support timeout -t. ~~Could you use that instead of timeout command?~~ Could you use that and maybe together with timeout command

@qiluo-msft
We can't predict the behaviour of 'bcmcmd' in case syncd is crashed/stuck/in bad shape. The change you are suggesting might require very extensive testing.

This is the my only concern, others lgtm.

In reply to: 546269345 [](ancestors = 546269345)

'timeout' supersedes the '-t' option of 'bcmcmd'. Is the concern regarding 'bcmcmd' may end up in a bad state like leaving the socket open?

bcmcmd is not well tested with signal termination. I am not sure.

In reply to: 550679331 [](ancestors = 550679331,546269345)

@qiluo-msft
"Could you use that and maybe together with timeout command" --- The bcmcmd has a default timeout of 30 sec. If no argument is given using -t option, '-t 30' is automatically taken. So the outside 'timeout' command is being used together with bcmcmd timeout option.

root@sonic:/home/admin# bcmcmd USAGE: bcmcmd [-f <sun_path>] -v <cmd> -v verbose mode -f domain socket filename, default /var/run/sswsyncd/sswsyncd.socket -t timeout in seconds, default 30 RETURN VALUE: 0 success 5 socket io error 22 invalid argument 62 timeout root@sonic:/home/admin#

qiluo-msft · 2020-12-22T05:46:41Z

+
+handle_signal()
+{
+    echo "Generate Dump received interrupt"


echo [](start = 4, length = 4)

echo to stderr? #Closed

qiluo-msft · 2020-12-22T05:47:24Z

+handle_signal()
+{
+    echo "Generate Dump received interrupt"
+    $RM $V -rf $TARDIR


$RM $V [](start = 4, length = 6)

These variables should be defined before this function. #Closed

qiluo-msft · 2020-12-22T05:52:09Z

+    if [[ ( "$NUM_ASICS" > 1 ) ]]; then
+        for (( i=0; i<$NUM_ASICS; i++ ))
+        do
+            local cmd="bcmcmd -n $i $1"


bcmcmd -n [](start = 23, length = 9)

bcmcmd does not have -n option. #Closed

The change was requested by @smaheshm.

bcmcmd has been modified to include the ASIC number. sonic-net/sonic-buildimage#4926

qiluo-msft · 2020-12-22T05:54:14Z

        ns=" -n  ${asic_id}"
    fi
-    echo "$ns"
+    #echo "$ns"


#echo "$ns" [](start = 4, length = 11)

The function stdout is changed. #Closed

Removed the comment

qiluo-msft · 2020-12-22T05:54:43Z

+    save_vtysh "show bfd peers json" "frr.bfd.peers.json"
+    save_vtysh "show bfd peers counters json" "frr.bfd.peers.counters.json"
+
+}


Remove extra blank line #Closed

qiluo-msft · 2020-12-22T06:05:31Z

 ###############################################################################
 # Dumps all fields and values from given Redis DB.
 # Arguments:
+#  DB id: id of DB for sonic-db-cli


DB id [](start = 3, length = 5)

The option has nothing to do with DB id. Why add? Could you remove? #Closed

I changed the comments appropriately

FuzailBrcm · 2020-12-22T17:04:34Z

retest this please

smaheshm · 2020-12-23T20:58:38Z

retest this please

FuzailBrcm · 2020-12-24T03:33:00Z

retest this please

ben-gale · 2020-12-31T17:08:58Z

@qiluo-msft - can this proceed to approve/merge now? All checks are passing and I believe review comments have been addressed.

ben-gale · 2021-01-04T12:11:12Z

Pls merge

…on (sonic-net#1249) and some other enhancements to techsupport **- What I did** Following is the brief description of the changes, - Adding a ‘--silent’ option to ‘show techsupport’ command. Various tar/untar, addition and removal logs appear on the console by default. This option would disable above logs. - Adding global and per-command timeouts. This would provide more user control on ‘show techsupport’ CLI. - Adding time profiling information for the commands in techsupport. Time profiling information would be part of the tarball and helps to analyse the time consumption per command. - Sometimes ‘syncd’ docker is down and bcmshell is unavailable. In such cases all the bcmcmd commands would timeout and result in tremendous increase in the total techsupport collection time. We provided an option to skip rest of the bcmcmd commands once one command times out. - Added ‘show services’, ‘show reboot-cause’ and various BGP, BFD, bcm shell and other commands - Optimised the /var/log files collection. If the number of files are large in /var/log folder, it takes a long time to add each individually to the tarball. If the folder is tar'ed at once, the time taken reduces significantly. - Following error was observed while tar'ing softlinks inside .etc folder. ** Tar append operation failed. Aborting for safety. ** This issue was due to softlinks present at /etc folder where the destination file is absent. Fixed this issue by deleting such softlinks before adding them to the tarball. **- How I did it** - Added new options to the CLICK command 'show techsupport' - Modified the 'generate_dump' script to accomodate other changes **- How to verify it** Here are some outputs, root@sonic:/home/admin# show techsupport --silent Techsupport is running with silent option. This command might take a long time. HW Mgmt dump script /usr/bin/hw-management-generate-dump.sh does not exist /var/dump/sonic_dump_sonic_20201117_161246.tar.gz root@sonic:/home/admin# root@sonic:~# show techsupport -h Usage: show techsupport [OPTIONS] Gather information for troubleshooting Options: --since TEXT Collect logs and core files since given date -g, --global-timeout INTEGER Global timeout for techsupport in minutes. Default 30 mins -c, --cmd-timeout INTEGER Command timeout for techsupport in minutes. Default 5 mins --verbose Enable verbose output --silent Run techsupport in silent mode -?, -h, --help Show this message and exit. root@sonic:~# **- Previous command output (if the output of a command-line utility has changed)** - Previous command "show techsupport" works as is **- New command output (if the output of a command-line utility has changed)**

rlhui assigned smaheshm Dec 9, 2020

smaheshm reviewed Dec 9, 2020

View reviewed changes

Comment thread scripts/generate_dump Outdated

smaheshm reviewed Dec 9, 2020

View reviewed changes

Comment thread scripts/generate_dump Outdated

smaheshm reviewed Dec 9, 2020

View reviewed changes

Comment thread scripts/generate_dump Outdated

smaheshm reviewed Dec 9, 2020

View reviewed changes

Comment thread scripts/generate_dump Outdated

smaheshm reviewed Dec 9, 2020

View reviewed changes

Comment thread scripts/generate_dump Outdated

smaheshm reviewed Dec 9, 2020

View reviewed changes

Comment thread scripts/generate_dump Outdated

smaheshm reviewed Dec 9, 2020

View reviewed changes

Comment thread scripts/generate_dump Outdated

smaheshm reviewed Dec 10, 2020

View reviewed changes

Comment thread scripts/generate_dump Outdated

smaheshm reviewed Dec 10, 2020

View reviewed changes

Comment thread scripts/generate_dump Outdated

smaheshm reviewed Dec 10, 2020

View reviewed changes

Comment thread scripts/generate_dump Outdated

smaheshm reviewed Dec 10, 2020

View reviewed changes

Comment thread scripts/generate_dump Outdated

smaheshm reviewed Dec 10, 2020

View reviewed changes

Comment thread scripts/generate_dump Outdated

smaheshm reviewed Dec 10, 2020

View reviewed changes

Comment thread scripts/generate_dump Outdated

smaheshm reviewed Dec 10, 2020

View reviewed changes

Comment thread scripts/generate_dump Outdated

smaheshm reviewed Dec 10, 2020

View reviewed changes

Comment thread scripts/generate_dump Outdated

smaheshm suggested changes Dec 10, 2020

View reviewed changes

FuzailBrcm commented Dec 11, 2020

View reviewed changes

smaheshm mentioned this pull request Dec 11, 2020

[show][techsupport][multi-ASIC] Add support to collect tech support on multi ASIC platform #1193

Merged

smaheshm reviewed Dec 14, 2020

View reviewed changes

Comment thread scripts/generate_dump Outdated

smaheshm reviewed Dec 14, 2020

View reviewed changes

Comment thread scripts/generate_dump Outdated

smaheshm reviewed Dec 14, 2020

View reviewed changes

Comment thread show/main.py Outdated

smaheshm reviewed Dec 14, 2020

View reviewed changes

Comment thread scripts/generate_dump Outdated

smaheshm reviewed Dec 14, 2020

View reviewed changes

Comment thread show/main.py Outdated

lguohan reviewed Dec 16, 2020

View reviewed changes

smaheshm previously approved these changes Dec 18, 2020

View reviewed changes

qiluo-msft reviewed Dec 19, 2020

View reviewed changes

qiluo-msft reviewed Dec 22, 2020

View reviewed changes

Comment thread scripts/generate_dump Outdated

qiluo-msft reviewed Dec 22, 2020

View reviewed changes

Comment thread scripts/generate_dump Outdated

qiluo-msft reviewed Dec 22, 2020

View reviewed changes

Comment thread scripts/generate_dump Outdated

qiluo-msft reviewed Dec 22, 2020

View reviewed changes

Comment thread scripts/generate_dump

More review comments

af6190a

FuzailBrcm dismissed smaheshm’s stale review via af6190a December 22, 2020 10:41

smaheshm approved these changes Dec 31, 2020

View reviewed changes

qiluo-msft approved these changes Jan 4, 2021

View reviewed changes

qiluo-msft merged commit 22d79f3 into sonic-net:master Jan 4, 2021

Conversation

FuzailBrcm commented Nov 18, 2020

Uh oh!

FuzailBrcm commented Nov 18, 2020

Uh oh!

FuzailBrcm commented Nov 18, 2020

Uh oh!

FuzailBrcm commented Nov 18, 2020

Uh oh!

smaheshm commented Dec 9, 2020

Uh oh!

smaheshm commented Dec 9, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smaheshm left a comment

Choose a reason for hiding this comment

Uh oh!

FuzailBrcm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

smaheshm commented Dec 14, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ben-gale commented Dec 16, 2020

Uh oh!

FuzailBrcm commented Dec 17, 2020

Uh oh!

lguohan commented Dec 17, 2020

Uh oh!

FuzailBrcm commented Dec 17, 2020

Uh oh!

FuzailBrcm commented Dec 18, 2020

Uh oh!

smaheshm left a comment

Choose a reason for hiding this comment

Uh oh!

ben-gale commented Dec 18, 2020

Uh oh!

qiluo-msft Dec 19, 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!

Uh oh!

qiluo-msft Dec 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

qiluo-msft Dec 19, 2020 •

edited

Loading

qiluo-msft Dec 22, 2020 •

edited

Loading

qiluo-msft Dec 22, 2020 •

edited

Loading

qiluo-msft Dec 22, 2020 •

edited

Loading

qiluo-msft Dec 22, 2020 •

edited

Loading

qiluo-msft Dec 22, 2020 •

edited

Loading

qiluo-msft Dec 22, 2020 •

edited

Loading