[CLI][PFC] Add multi ASIC options for pfcstat and 'show pfc counters' #1057
Conversation
|
Unit test is pending on #1006 , which contains changes to multi ASIC mock DB APIs. |
|
This pull request introduces 1 alert when merging ae7516e into 5263b54 - view on LGTM.com new alerts:
|
|
retest this please |
|
|
||
| pfcstat = Pfcstat() | ||
| pfcstat = Pfcstat(args.namespace, args.show) | ||
|
|
||
| if delete_all_stats: |
There was a problem hiding this comment.
I think pfcstat -c should clear stats on all ports and all namespace and save all ports stats in the cached file.
If not, it might lead to confusing output.
For example. if user does the following sequence
- pfcstat -c
Since the internal ports stats are not cleared, they will not be reset to zero, - pfcstat -s all
External ports stats will start from zero while the Internal ports will still have the old value. This might lead to confusion.
There was a problem hiding this comment.
that's right, will change it. By default non-show commands should apply to all ports and namespaces, and should not have an option for 'namespace' or 'display'.
There was a problem hiding this comment.
I looked at it further. It doesn't really clear the stats from counters DB. It updates that stats in "/tmp" and prints the diff from previous values. Since the script takes multiple arguments the 'namespace', 'display' also applies to '-c' arg. We can ignore '-c' option when 'namespace' is provided, but this will work as is as all the keys are different. Meaning, if a user runs '-c' with a namespace option and then runs -c with a different namespace option or no namespace, the command will output the 'diff' in counts for the cached ports, and prints as is for un-cached ports or new ports with counts.
There was a problem hiding this comment.
the prints the "Last cached time was " which might me misleading since some of the ports were not cached.
The change I'm making is to collect stats from all ports when '-c' option is provided. In short the 'namespace' option will be ignored silently when '-c' arg is specified. Comments?
There was a problem hiding this comment.
I am fine with this. I don't think we will be adding the multi asic options to sonic-clear commands. So the util script will not get the namespace or display, so we should be ok. I follow the similar approach for show interface counters
There was a problem hiding this comment.
If we use '-c' with a namespace and then use the display for another namespace, the problem we have is the cached time that we display will be incorrect because we aren't actually caching any of those ports info.
Either we ignore the namespace for '-c' option and get the stats for all ports and write them to the file or we need to maintain separate files for the namespaces
There was a problem hiding this comment.
Ignore my comment. saw your change
| @@ -235,7 +272,7 @@ Examples: | |||
| else: | |||
| pfcstat.cnstat_print(cnstat_dict_rx, True) | |||
|
|
|||
| print() | |||
| print("") | |||
There was a problem hiding this comment.
This is just to add a 'line' between tx and rx outputs.
|
LGTM, will wait for the unit test for approval |
|
retest this please |
1 similar comment
|
retest this please |
|
@smaheshm Create PR for 201911 |
- What I did
Added 'multi ASIC' support to the 'show pfc counters' CLI. Modified 'pfcstat' command to include multi ASIC args.
By default 'show pfc counters' or 'pfcstat' display the counters for the front end ports. To display on all ports "-s all" option must be specified. On single ASIC platforms by default all port counters are shown, there's no effect of '-s' (show) option.
- How I did it
Modified show/main.py and 'pfcstat' to include multi ASIC options and args respectively.
Used multi ASIC common APIs and decorator to collect information from specified namespaces or all namespaces (by default).
- How to verify it
Verified the commands on multi ASIC platform.
- 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)
Unit test results: