Fix multi-asic behaviour for dropstat #3059

bktsim-arista · 2023-11-30T18:06:06Z

What I did

Previously, dropstat was not behaving correctly on multi-asic devices as the correct namespaces were not being traversed and namespace option -n was not available. This change fixes the multi-asic behaviour of this command and adds the -n option, with relevant unit tests to ensure correct functionality.

This is a part of the set of changes being pushed for sonic-net/sonic-buildimage#15148

How I did it

Added -n argument for multi-asic devices and relevant unit tests.

How to verify it

Use dropstat -c show with -n.

wenyiz2021 · 2024-04-08T22:32:02Z

scripts/dropstat

+    if namespace and namespace not in namespaces:
+        raise Exception("Input arguments error. Namespaces must be one of", *namespaces)
+
+    if multi_asic.is_multi_asic() and not namespace:


this can be replaced with a more consistent way at line 411:
@click.option('-n', '--namespace', help='Namespace name',
required=True if multi_asic.is_multi_asic() else False, type=click.Choice(multi_asic.get_namespace_list()))

@wenyiz2021 are you sure we should be using the @click decorators within the scripts/ folder?
This dropstat file doesn't hold any CLI commands just the implementation code for querying the redis DBs

See latest change. I took your suggestion to use the click library. Thanks!

wenyiz2021 · 2024-04-08T22:41:11Z

tests/multi_asic_dropstat_test.py

+      os.environ["UTILITIES_UNIT_TESTING"] = "1"
+      os.environ["UTILITIES_UNIT_TESTING_TOPOLOGY"] = "multi_asic"
+      print("SETUP")
+


can you add edge cases like:

when no namespace is given for multi-asic

when namespace is given on single-asic

See latest change. Above two edges have been covered in new UTs.

vmittal-msft · 2024-05-16T19:04:22Z

@bktsim-arista Few things -

Can you please share how "Show dropcounter capabilites/configuration/counts" command is going to change ?
please add test for all options under dropcounter.
If no namespace specified, Will it show for all namespaces? if we specify, will it show for only that namespace ?
Is this verified to be working fine on pizza box as well as chassis?

vmittal-msft

Please check comment

arista-nwolfe · 2024-05-16T22:45:31Z

@bktsim-arista Few things -

Can you please share how "Show dropcounter capabilites/configuration/counts" command is going to change ?

please add test for all options under dropcounter.

If no namespace specified, Will it show for all namespaces? if we specify, will it show for only that namespace ?

Is this verified to be working fine on pizza box as well as chassis?

For questions 1 & 2: This PR only adds multi-asic support to show dropcounter counts so we shouldn't include test coverage for capabilities/configuration as it's not supported yet on multi-asic.

and ensuring that dropstat iterates through correct namespaces when 'show' command is run.

This reverts commit 4116cc0.

- Use click library as suggested by Wenyi to simplify argument parsing - Support --namespace argument conditionally. If not passed, the default behavior is to display all namespaces. - Utilize `run_on_multi_asic` decorator to abstract iterating over namespaces - Added and updating UTs

Multi-asic counter clear is supported by unconditionally caching counters from all namespaces. Also add new UTs to cover changes.

kenneth-arista · 2024-06-30T22:40:04Z

@arlakshm please review

arlakshm · 2024-06-30T23:19:00Z

scripts/dropstat

-    counter_type = args.type
-
-    dcstat = DropStat()
+@click.command(help='Display drop counters')


why do we need to add this change here?
dropstat is standalone python script. the click options need to present in show/dropcounter.py

The original script also supported argument parsing. Using the click simplified that logic significantly. It was suggested earlier by Wenyi and I agree this is an improvement over what was originally implemented. Furthermore, the UTs directly call this script.

arlakshm · 2024-06-30T23:43:30Z

show/dropcounters.py

@@ -1,5 +1,6 @@
 import click
 import utilities_common.cli as clicommon
+import utilities_common.multi_asic as multi_asic_util


Can you add some test to cover the single asic as well?

Also fixed test_show_pg_drop_masic_invalid_ns

To avoid interference of cached counters between UTs.

kenneth-arista · 2024-07-01T04:16:52Z

If no namespace specified, Will it show for all namespaces? if we specify, will it show for only that namespace ?

The latest changes support this.

wenyiz2021

LGTM

wenyiz2021 · 2024-07-02T17:42:42Z

@arlakshm @vmittal-msft please check if your review comments are resolved

kenneth-arista · 2024-07-04T02:29:23Z

Is this verified to be working fine on pizza box as well as chassis?
Yes, I verified that the changes work on a pizza box with a single ASIC.

For example,

admin@pkz303:/$ show dropcounters counts  --help
Usage: show dropcounters counts [OPTIONS]

  Show drop counts

Options:
  -g, --group TEXT
  -t, --counter_type TEXT
  --verbose                Enable verbose output
  -n, --namespace []       Namespace name or all
  -h, -?, --help           Show this message and exit.

admin@pkz303:/$ show dropcounters counts
     IFACE    STATE    RX_ERR    RX_DROPS    TX_ERR    TX_DROPS    DEBUG1    DEBUG2    DEBUG3
----------  -------  --------  ----------  --------  ----------  --------  --------  --------
 Ethernet0        U         0           0         0           0         0         0         0
 Ethernet1        X         0           0         0           0         0         0         0
 Ethernet2        U         0           0         0           0         0         0         0
 Ethernet3        U         0           0         0           0         0         0         0
 Ethernet4        X         0           0         0           0         0         0         0
 Ethernet5        X         0           0         0           0         0         0         0
 Ethernet6        X         0           0         0           0         0         0         0
 Ethernet7        U         0           0         0           0         0         0         0
 Ethernet8        U         0           0         0           0         0         0         0
 Ethernet9        U         0           0         0           0         0         0         0
Ethernet10        U         0           0         0           0         0         0         0
Ethernet11        X         0           0         0           0         0         0         0
Ethernet12        U         0           0         0           0         0         0         0
Ethernet13        U         0           0         0           0         0         0         0
Ethernet14        U         0           0         0           0         0         0         0
Ethernet15        U         0           0         0           0         0         0         0
Ethernet16        X         0           0         0           0         0         0         0
Ethernet17        X         0           0         0           0         0         0         0
Ethernet18        U         0           0         0           0         0         0         0
Ethernet19        X         0           0         0           0         0         0         0
Ethernet20        U         0           0         0           0         0         0         0
Ethernet21        X         0           0         0           0         0         0         0
Ethernet22        U         0           0         0           0         0         0         0
Ethernet23        U         0           0         0           0         0         0         0
Ethernet24        U         0           0         0           0         0         0         0
Ethernet25        U         0           0         0           0         0         0         0
Ethernet26        U         0           0         0           0         0         0         0
Ethernet27        U         0           0         0           0         0         0         0
Ethernet28        U         0           0         0           0         0         0         0
Ethernet29        X         0           0         0           0         0         0         0
Ethernet30        U         0           0         0           0         0         0         0
Ethernet31        X         0           0         0           0         0         0         0
Ethernet32        U         0           0         0           0         0         0         0
Ethernet33        U         0           0         0           0         0         0         0
Ethernet34        X         0           0         0           0         0         0         0
Ethernet35        U         0           0         0           0         0         0         0
Ethernet36        U         0           0         0           0         0         0         0
Ethernet37        X         0           0         0           0         0         0         0
Ethernet38        U         0           0         0           0         0         0         0
Ethernet39        X         0           0         0           0         0         0         0
Ethernet40        X         0           0         0           0         0         0         0
Ethernet41        U         0           0         0           0         0         0         0
Ethernet42        U         0           0         0           0         0         0         0
Ethernet43        X         0           0         0           0         0         0         0
Ethernet44        X         0           0         0           0         0         0         0
Ethernet45        U         0           0         0           0         0         0         0
Ethernet46        X         0           0         0           0         0         0         0
Ethernet47        U         0           0         0           0         0         0         0
Ethernet48        U         0           0         0           0         0         0         0
Ethernet49        U         0           0         0           0         0         0         0
Ethernet50        U         0           0         0           0         0         0         0
Ethernet51        U         0           0         0           0         0         0         0

admin@pkz303:/$ show dropcounters counts -g TEST

     IFACE    STATE    DEBUG3
----------  -------  --------
 Ethernet0        U         0
 Ethernet1        X         0
 Ethernet2        U         0
 Ethernet3        U         0
 Ethernet4        X         0
 Ethernet5        X         0
 Ethernet6        X         0
 Ethernet7        U         0
 Ethernet8        U         0
 Ethernet9        U         0
Ethernet10        U         0
Ethernet11        X         0
Ethernet12        U         0
Ethernet13        U         0
Ethernet14        U         0
Ethernet15        U         0
Ethernet16        X         0
Ethernet17        X         0
Ethernet18        U         0
Ethernet19        X         0
Ethernet20        U         0
Ethernet21        X         0
Ethernet22        U         0
Ethernet23        U         0
Ethernet24        U         0
Ethernet25        U         0
Ethernet26        U         0
Ethernet27        U         0
Ethernet28        U         0
Ethernet29        X         0
Ethernet30        U         0
Ethernet31        X         0
Ethernet32        U         0
Ethernet33        U         0
Ethernet34        X         0
Ethernet35        U         0
Ethernet36        U         0
Ethernet37        X         0
Ethernet38        U         0
Ethernet39        X         0
Ethernet40        X         0
Ethernet41        U         0
Ethernet42        U         0
Ethernet43        X         0
Ethernet44        X         0
Ethernet45        U         0
Ethernet46        X         0
Ethernet47        U         0
Ethernet48        U         0
Ethernet49        U         0
Ethernet50        U         0
Ethernet51        U         0

admin@pkz303:/$ show dropcounters counts -t PORT_INGRESS_DROPS
     IFACE    STATE    RX_ERR    RX_DROPS    DEBUG1    DEBUG2    DEBUG3
----------  -------  --------  ----------  --------  --------  --------
 Ethernet0        U         0           0         0         0         0
 Ethernet1        X         0           0         0         0         0
 Ethernet2        U         0           0         0         0         0
 Ethernet3        U         0           0         0         0         0
 Ethernet4        X         0           0         0         0         0
 Ethernet5        X         0           0         0         0         0
 Ethernet6        X         0           0         0         0         0
 Ethernet7        U         0           0         0         0         0
 Ethernet8        U         0           0         0         0         0
 Ethernet9        U         0           0         0         0         0
Ethernet10        U         0           0         0         0         0
Ethernet11        X         0           0         0         0         0
Ethernet12        U         0           0         0         0         0
Ethernet13        U         0           0         0         0         0
Ethernet14        U         0           0         0         0         0
Ethernet15        U         0           0         0         0         0
Ethernet16        X         0           0         0         0         0
Ethernet17        X         0           0         0         0         0
Ethernet18        U         0           0         0         0         0
Ethernet19        X         0           0         0         0         0
Ethernet20        U         0           0         0         0         0
Ethernet21        X         0           0         0         0         0
Ethernet22        U         0           0         0         0         0
Ethernet23        U         0           0         0         0         0
Ethernet24        U         0           0         0         0         0
Ethernet25        U         0           0         0         0         0
Ethernet26        U         0           0         0         0         0
Ethernet27        U         0           0         0         0         0
Ethernet28        U         0           0         0         0         0
Ethernet29        X         0           0         0         0         0
Ethernet30        U         0           0         0         0         0
Ethernet31        X         0           0         0         0         0
Ethernet32        U         0           0         0         0         0
Ethernet33        U         0           0         0         0         0
Ethernet34        X         0           0         0         0         0
Ethernet35        U         0           0         0         0         0
Ethernet36        U         0           0         0         0         0
Ethernet37        X         0           0         0         0         0
Ethernet38        U         0           0         0         0         0
Ethernet39        X         0           0         0         0         0
Ethernet40        X         0           0         0         0         0
Ethernet41        U         0           0         0         0         0
Ethernet42        U         0           0         0         0         0
Ethernet43        X         0           0         0         0         0
Ethernet44        X         0           0         0         0         0
Ethernet45        U         0           0         0         0         0
Ethernet46        X         0           0         0         0         0
Ethernet47        U         0           0         0         0         0
Ethernet48        U         0           0         0         0         0
Ethernet49        U         0           0         0         0         0
Ethernet50        U         0           0         0         0         0
Ethernet51        U         0           0         0         0         0

admin@pkz303:/$ dropstat --version
dropstat, version 1.0

Also some minor test renaming and cleanup for clarity

wenyiz2021 · 2024-07-10T23:19:53Z

@arlakshm has a concern on using click option in scripts directory which is outside of show directory.
otherwise LGTM.
thanks @kenneth-arista for adding test as well

kenneth-arista · 2024-07-12T02:36:26Z

@arlakshm has a concern on using click option in scripts directory which is outside of show directory. otherwise LGTM. thanks @kenneth-arista for adding test as well

Thanks @wenyiz2021 . The @click library usage in the script significantly improves the readability and maintainability of the standalone script. Thanks Wenyi for suggesting it. I think this is a good change. @arlakshm please let me know how you would like to proceed. I recommend keeping the change 😄

kenneth-arista · 2024-07-24T06:31:25Z

@wenyiz2021 can you help merge this change?

vmittal-msft self-requested a review December 13, 2023 01:08

arlakshm requested a review from deepak-singhal0408 January 10, 2024 20:24

rlhui assigned bktsim-arista Jan 11, 2024

arista-nwolfe force-pushed the multi-asic-dropstat branch from 8ea458b to 913220a Compare March 25, 2024 22:05

wenyiz2021 reviewed Apr 8, 2024

View reviewed changes

vmittal-msft reviewed May 16, 2024

View reviewed changes

rdjeric-arista mentioned this pull request May 17, 2024

Support SONIC cli commands for QoS/MMU configuration/stats for multi-asic sonic-net/sonic-buildimage#15148

Closed

bktsim-arista and others added 5 commits June 30, 2024 15:22

Fixes dropstat multi-asic behaviour by using multi-asic helpers

954e31b

and ensuring that dropstat iterates through correct namespaces when 'show' command is run.

Change namespace arg processing per Wenyi request

776fa9d

Linter fixes

b93a1bd

Revert "Change namespace arg processing per Wenyi request"

44c4aa0

This reverts commit 4116cc0.

kenneth-arista force-pushed the multi-asic-dropstat branch from d750f06 to 030f466 Compare June 30, 2024 22:22

Support multi-asic clear command

b963c4e

Multi-asic counter clear is supported by unconditionally caching counters from all namespaces. Also add new UTs to cover changes.

kenneth-arista force-pushed the multi-asic-dropstat branch from 030f466 to b963c4e Compare June 30, 2024 22:27

arlakshm reviewed Jun 30, 2024

View reviewed changes

kenneth-arista added 2 commits June 30, 2024 19:26

Adding missing asic_db table for tests

8d2988c

Also fixed test_show_pg_drop_masic_invalid_ns

Added clearing of userCache

a2aadb0

To avoid interference of cached counters between UTs.

wenyiz2021 previously approved these changes Jul 2, 2024

View reviewed changes

kenneth-arista dismissed wenyiz2021’s stale review via 25a7293 July 4, 2024 03:23

kenneth-arista force-pushed the multi-asic-dropstat branch from 25a7293 to 76ce6c4 Compare July 4, 2024 03:30

Add UT for single ASIC

b2df7ae

Also some minor test renaming and cleanup for clarity

kenneth-arista force-pushed the multi-asic-dropstat branch from 76ce6c4 to b2df7ae Compare July 4, 2024 03:32

wenyiz2021 approved these changes Jul 10, 2024

View reviewed changes

wenyiz2021 merged commit a813215 into sonic-net:master Jul 24, 2024
7 checks passed

wenyiz2021 added the Request for 202405 Branch label Sep 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix multi-asic behaviour for dropstat #3059

Fix multi-asic behaviour for dropstat #3059

bktsim-arista commented Nov 30, 2023

wenyiz2021 Apr 8, 2024

arista-nwolfe May 16, 2024

kenneth-arista Jun 30, 2024

wenyiz2021 Apr 8, 2024

kenneth-arista Jun 30, 2024

vmittal-msft commented May 16, 2024

vmittal-msft left a comment

arista-nwolfe commented May 16, 2024

kenneth-arista commented Jun 30, 2024

arlakshm Jun 30, 2024

kenneth-arista Jul 1, 2024

arlakshm Jun 30, 2024

kenneth-arista commented Jul 1, 2024 •

edited

Loading

wenyiz2021 left a comment

wenyiz2021 commented Jul 2, 2024

kenneth-arista commented Jul 4, 2024

wenyiz2021 commented Jul 10, 2024

kenneth-arista commented Jul 12, 2024

kenneth-arista commented Jul 24, 2024

Fix multi-asic behaviour for dropstat #3059

Fix multi-asic behaviour for dropstat #3059

Conversation

bktsim-arista commented Nov 30, 2023

What I did

How I did it

How to verify it

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

vmittal-msft commented May 16, 2024

vmittal-msft left a comment

Choose a reason for hiding this comment

arista-nwolfe commented May 16, 2024

kenneth-arista commented Jun 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenneth-arista commented Jul 1, 2024 • edited Loading

wenyiz2021 left a comment

Choose a reason for hiding this comment

wenyiz2021 commented Jul 2, 2024

kenneth-arista commented Jul 4, 2024

wenyiz2021 commented Jul 10, 2024

kenneth-arista commented Jul 12, 2024

kenneth-arista commented Jul 24, 2024

kenneth-arista commented Jul 1, 2024 •

edited

Loading