Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix multi-asic behaviour for pg-drop #3058

Merged
merged 5 commits into from
Jul 10, 2024
Merged

Fix multi-asic behaviour for pg-drop #3058

merged 5 commits into from
Jul 10, 2024

Conversation

bktsim-arista
Copy link
Contributor

What I did

show priority-group drop is not behaving correctly on multi-asic devices, as the namespace option '-n' is not available and correct namespaces were not traversed to retrieve drop counters. This change fixes the multi-asic behaviour of this command and adds unit tests to ensure that the functionality is correct.

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

How I did it

Added multi-asic support and relevant unit tests for pg-drop script.

How to verify it

Run unit test, or pg-drop -c show with -n

@vmittal-msft vmittal-msft self-requested a review December 13, 2023 01:08
@vmittal-msft
Copy link
Contributor

@bktsim-arista Few things -

  1. Can you please share how "Show priority-group drop/persistent-watermark/watermark" command is going to change ?
  2. please add test for all options under priority-group.
  3. If no namespace specified, Will it show for all namespaces? if we specify, will it show for only that namespace ?
  4. Is this verified to be working fine on pizza box as well as chassis?

vmittal-msft
vmittal-msft reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@vmittal-msft vmittal-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check comments

@arista-nwolfe
Copy link

@bktsim-arista Few things -

  1. Can you please share how "Show priority-group drop/persistent-watermark/watermark" command is going to change ?
  2. please add test for all options under priority-group.
  3. If no namespace specified, Will it show for all namespaces? if we specify, will it show for only that namespace ?
  4. 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 for show priority-group drop so we shouldn't have tests for persistent-watermark/watermark commands. Support for multi-asic on those commands along with their UT are in #3060

@rdjeric-arista rdjeric-arista mentioned this pull request May 17, 2024
Closed
@kenneth-arista
Copy link
Contributor

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

Yes, confirmed that the changes work on a pizza box as well.

@kenneth-arista
Copy link
Contributor

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

If --namespace is specified, then only counters relevant to that namespace will be shown or cleared. If the namespace arg is not passed, the command affects all namespaces.

For example,

$ show priority-group  drop counters --namespace asic0
Ingress PG dropped packets:
         Port    PG0    PG1    PG2    PG3    PG4    PG5    PG6    PG7
-------------  -----  -----  -----  -----  -----  -----  -----  -----
    Ethernet0      0      0      0      0      0      0      0      0
    Ethernet8      0      0      0      0      0      0      0      0
   Ethernet16      0      0      0      0      0      0      0      0
   Ethernet24      0      0      0      0      0      0      0      0
   Ethernet32      0      0      0      0      0      0      0      0
   Ethernet40      0      0      0      0      0      0      0      0
   Ethernet48      0      0      0      0      0      0      0      0
   Ethernet56      0      0      0      0      0      0      0      0
   Ethernet64      0      0      0      0      0      0      0      0
   Ethernet72      0      0      0      0      0      0      0      0
   Ethernet80      0      0      0      0      0      0      0      0
   Ethernet88      0      0      0      0      0      0      0      0
   Ethernet96      0      0      0      0      0      0      0      0
  Ethernet104      0      0      0      0      0      0      0      0
  Ethernet112      0      0      0      0      0      0      0      0
  Ethernet120      0      0      0      0      0      0      0      0
  Ethernet128      0      0      0      0      0      0      0      0
  Ethernet136      0      0      0      0      0      0      0      0
 Ethernet-IB0      0      0      0      0      0      0      0      0
Ethernet-Rec0      0      0      0      0      0      0      0      0

$ show priority-group  drop counters --namespace asic1
Ingress PG dropped packets:
         Port    PG0    PG1    PG2    PG3    PG4    PG5    PG6    PG7
-------------  -----  -----  -----  -----  -----  -----  -----  -----
  Ethernet144      0      0      0      0      0      0      0      0
  Ethernet152      0      0      0      0      0      0      0      0
  Ethernet160      0      0      0      0      0      0      0      0
  Ethernet168      0      0      0      0      0      0      0      0
  Ethernet176      0      0      0      0      0      0      0      0
  Ethernet184      0      0      0      0      0      0      0      0
  Ethernet192      0      0      0      0      0      0      0      0
  Ethernet200      0      0      0      0      0      0      0      0
  Ethernet208      0      0      0      0      0      0      0      0
  Ethernet216      0      0      0      0      0      0      0      0
  Ethernet224      0      0      0      0      0      0      0      0
  Ethernet232      0      0      0      0      0      0      0      0
  Ethernet240      0      0      0      0      0      0      0      0
  Ethernet248      0      0      0      0      0      0      0      0
  Ethernet256      0      0      0      0      0      0      0      0
  Ethernet264      0      0      0      0      0      0      0      0
  Ethernet272      0      0      0      0      0      0      0      0
  Ethernet280      0      0      0      0      0      0      0      0
 Ethernet-IB1      0      0      0      0      0      0      0      0
Ethernet-Rec1      0      0      0      0      0      0      0      0

$ show priority-group  drop counters
Ingress PG dropped packets:
         Port    PG0    PG1    PG2    PG3    PG4    PG5    PG6    PG7
-------------  -----  -----  -----  -----  -----  -----  -----  -----
    Ethernet0      0      0      0      0      0      0      0      0
    Ethernet8      0      0      0      0      0      0      0      0
   Ethernet16      0      0      0      0      0      0      0      0
   Ethernet24      0      0      0      0      0      0      0      0
   Ethernet32      0      0      0      0      0      0      0      0
   Ethernet40      0      0      0      0      0      0      0      0
   Ethernet48      0      0      0      0      0      0      0      0
   Ethernet56      0      0      0      0      0      0      0      0
   Ethernet64      0      0      0      0      0      0      0      0
   Ethernet72      0      0      0      0      0      0      0      0
   Ethernet80      0      0      0      0      0      0      0      0
   Ethernet88      0      0      0      0      0      0      0      0
   Ethernet96      0      0      0      0      0      0      0      0
  Ethernet104      0      0      0      0      0      0      0      0
  Ethernet112      0      0      0      0      0      0      0      0
  Ethernet120      0      0      0      0      0      0      0      0
  Ethernet128      0      0      0      0      0      0      0      0
  Ethernet136      0      0      0      0      0      0      0      0
  Ethernet144      0      0      0      0      0      0      0      0
  Ethernet152      0      0      0      0      0      0      0      0
  Ethernet160      0      0      0      0      0      0      0      0
  Ethernet168      0      0      0      0      0      0      0      0
  Ethernet176      0      0      0      0      0      0      0      0
  Ethernet184      0      0      0      0      0      0      0      0
  Ethernet192      0      0      0      0      0      0      0      0
  Ethernet200      0      0      0      0      0      0      0      0
  Ethernet208      0      0      0      0      0      0      0      0
  Ethernet216      0      0      0      0      0      0      0      0
  Ethernet224      0      0      0      0      0      0      0      0
  Ethernet232      0      0      0      0      0      0      0      0
  Ethernet240      0      0      0      0      0      0      0      0
  Ethernet248      0      0      0      0      0      0      0      0
  Ethernet256      0      0      0      0      0      0      0      0
  Ethernet264      0      0      0      0      0      0      0      0
  Ethernet272      0      0      0      0      0      0      0      0
  Ethernet280      0      0      0      0      0      0      0      0
 Ethernet-IB0      0      0      0      0      0      0      0      0
 Ethernet-IB1      0      0      0      0      0      0      0      0
Ethernet-Rec0      0      0      0      0      0      0      0      0
Ethernet-Rec1      0      0      0      0      0      0      0      0

@kenneth-arista kenneth-arista force-pushed the multi-asic-pgdrop branch 2 times, most recently from 2ee7ae8 to 3d2aea0 Compare June 5, 2024 09:06
@kenneth-arista
Copy link
Contributor

For simplicity the clear command ignores the namespace argument and will always apply to all applicable namespaces. The reason is that clearing is accomplished via caching. If the namespace argument were honored, there will be unwieldy logic to cache only counters specific to the namespace. If the user mixes and matches clear and show commands with different namespace arguments, the end result will be confusing.

@kenneth-arista
Copy link
Contributor

@arlakshm please review

kenneth-arista
kenneth-arista reviewed Jun 14, 2024
View reviewed changes
scripts/pg-drop Outdated
def __init__(self, namespace):
self.namespace = namespace
self.ns_list = multi_asic.get_namespace_list(namespace)
self.configdb = ConfigDBConnector(namespace=namespace, use_unix_socket_path=True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not use unix_sockets everywhere as this will require users with elevated privilege to run this show command.

bktsim-arista and others added 5 commits June 17, 2024 13:36
@bktsim-arista @kenneth-arista
Fixes multi-asic behaviour for pg-drop script.
74a5296 
show priority-group drop is not behaving correctly on multi-asic devices,
as the namespace option '-n' is not available and correct namespaces were
not traversed to retrieve drop counters. This change fixes the multi-asic
behaviour of this command.
@bktsim-arista @kenneth-arista
add additional test and simplify branching
71e3dba
@bktsim-arista @kenneth-arista
fix: remove unused library
8e101a9
@kenneth-arista
Small tweaks to improve readability
4c5d658
@kenneth-arista
Don't use unix sockets
0c0b13c
@wenyiz2021 wenyiz2021 merged commit 1f94444 into sonic-net:master Jul 10, 2024
7 checks passed
This was referenced Aug 21, 2024
Open
Open
@saksarav-nokia
Copy link
Contributor

@vmittal-msft , When will this get cherry-picked to 202405?

@arista-hpandya arista-hpandya mentioned this pull request Sep 24, 2024
Merged
@vmittal-msft
Copy link
Contributor

@kenneth-arista @bktsim-arista Can you please raise a PR for 2020405 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kenneth-arista kenneth-arista kenneth-arista left review comments

@vmittal-msft vmittal-msft vmittal-msft left review comments

@wenyiz2021 wenyiz2021 wenyiz2021 approved these changes

@deepak-singhal0408 deepak-singhal0408 Awaiting requested review from deepak-singhal0408

Assignees

@bktsim-arista bktsim-arista

Labels
Request for 202405 Branch
Projects
SONiC Chassis
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bktsim-arista @vmittal-msft @arista-nwolfe @kenneth-arista @saksarav-nokia @wenyiz2021