Adding global-timeout, individual command timeout, log files collection #1249

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

scripts/generate_dump

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

scripts/generate_dump

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.

show/main.py

scripts/generate_dump

show/main.py

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

scripts/generate_dump

+    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#

scripts/generate_dump

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

scripts/generate_dump

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


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

echo to stderr? #Closed

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

scripts/generate_dump

+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

scripts/generate_dump

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

scripts/generate_dump

+    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

scripts/generate_dump

@@ -135,7 +233,7 @@ get_vtysh_namespace() {
    else
        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

scripts/generate_dump

+    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

scripts/generate_dump

@@ -335,13 +512,19 @@ save_proc() {
 ###############################################################################
 # 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

scripts/generate_dump

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

scripts/generate_dump Outdated Show resolved Hide resolved

smaheshm reviewed Dec 9, 2020

View reviewed changes

scripts/generate_dump Outdated Show resolved Hide resolved

smaheshm reviewed Dec 9, 2020

View reviewed changes

scripts/generate_dump Outdated Show resolved Hide resolved

smaheshm reviewed Dec 9, 2020

View reviewed changes

scripts/generate_dump Outdated Show resolved Hide resolved

smaheshm reviewed Dec 9, 2020

View reviewed changes

scripts/generate_dump Outdated Show resolved Hide resolved

smaheshm reviewed Dec 9, 2020

View reviewed changes

scripts/generate_dump Show resolved Hide resolved

smaheshm reviewed Dec 9, 2020

View reviewed changes

scripts/generate_dump Outdated Show resolved Hide resolved

smaheshm reviewed Dec 10, 2020

View reviewed changes

scripts/generate_dump Outdated Show resolved Hide resolved

smaheshm reviewed Dec 10, 2020

View reviewed changes

scripts/generate_dump Outdated Show resolved Hide resolved

smaheshm reviewed Dec 10, 2020

View reviewed changes

scripts/generate_dump Outdated Show resolved Hide resolved

smaheshm reviewed Dec 10, 2020

View reviewed changes

scripts/generate_dump Outdated Show resolved Hide resolved

smaheshm reviewed Dec 10, 2020

View reviewed changes

scripts/generate_dump Outdated Show resolved Hide resolved

smaheshm reviewed Dec 10, 2020

View reviewed changes

scripts/generate_dump Outdated Show resolved Hide resolved

smaheshm reviewed Dec 10, 2020

View reviewed changes

scripts/generate_dump Outdated Show resolved Hide resolved

smaheshm reviewed Dec 10, 2020

View reviewed changes

scripts/generate_dump Outdated Show resolved Hide resolved

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

scripts/generate_dump Show resolved Hide resolved

smaheshm reviewed Dec 14, 2020

View reviewed changes

scripts/generate_dump Outdated Show resolved Hide resolved

smaheshm reviewed Dec 14, 2020

View reviewed changes

show/main.py Outdated Show resolved Hide resolved

smaheshm reviewed Dec 14, 2020

View reviewed changes

scripts/generate_dump Outdated Show resolved Hide resolved

smaheshm reviewed Dec 14, 2020

View reviewed changes

show/main.py Outdated Show resolved Hide resolved

smaheshm previously approved these changes Dec 18, 2020

View reviewed changes

brcm upstream automation moved this from Review in progress to Reviewer approved Dec 18, 2020

qiluo-msft reviewed Dec 19, 2020

View reviewed changes

qiluo-msft reviewed Dec 22, 2020

View reviewed changes

scripts/generate_dump Outdated Show resolved Hide resolved

qiluo-msft reviewed Dec 22, 2020

View reviewed changes

scripts/generate_dump Outdated Show resolved Hide resolved

qiluo-msft reviewed Dec 22, 2020

View reviewed changes

scripts/generate_dump Outdated Show resolved Hide resolved

qiluo-msft reviewed Dec 22, 2020

View reviewed changes

scripts/generate_dump Show resolved Hide resolved

More review comments

af6190a

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

brcm upstream automation moved this from Reviewer approved to Review in progress Dec 22, 2020

brcm upstream automation moved this from Review in progress to Reviewer approved Dec 31, 2020

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

brcm upstream automation moved this from Reviewer approved to Done Jan 4, 2021

Adding global-timeout, individual command timeout, log files collection #1249

Adding global-timeout, individual command timeout, log files collection #1249

Conversation

FuzailBrcm commented Nov 18, 2020

FuzailBrcm commented Nov 18, 2020

FuzailBrcm commented Nov 18, 2020

FuzailBrcm commented Nov 18, 2020

smaheshm commented Dec 9, 2020

smaheshm commented Dec 9, 2020

smaheshm left a comment

Choose a reason for hiding this comment

FuzailBrcm left a comment

Choose a reason for hiding this comment

smaheshm commented Dec 14, 2020

FuzailBrcm commented Dec 17, 2020

FuzailBrcm commented Dec 18, 2020

smaheshm left a comment

Choose a reason for hiding this comment

ben-gale commented Dec 18, 2020

qiluo-msft Dec 19, 2020 • edited

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

qiluo-msft Dec 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Dec 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Dec 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Dec 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Dec 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Dec 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FuzailBrcm commented Dec 22, 2020

smaheshm commented Dec 23, 2020

FuzailBrcm commented Dec 24, 2020

ben-gale commented Dec 31, 2020

ben-gale commented Jan 4, 2021

qiluo-msft Dec 19, 2020 •

edited

qiluo-msft Dec 22, 2020 •

edited

qiluo-msft Dec 22, 2020 •

edited

qiluo-msft Dec 22, 2020 •

edited

qiluo-msft Dec 22, 2020 •

edited

qiluo-msft Dec 22, 2020 •

edited

qiluo-msft Dec 22, 2020 •

edited