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.

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

Fix multi-asic behaviour for watermarkstat #3060

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bktsim-arista
Copy link

What I did

Added multi-asic support to watermarkstat, fixing watermark/persistent-watermark related commands. Previously, the following commands were not behaving correctly on multi-asic devices, as the '-n' namespace option was not available, and correct namespaces were not traversed on multi-asic devices.

  • show buffer_pool watermark/persistent-watermark
  • show headroom-pool watermark/persistent-watermark
  • show priority-group persistent-watermark/watermark
  • show queue persistent-watermark/watermark

This change fixes multi-asic behaviour of CLI commands that rely on watermarkstat as listed above.

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

How I did it

Added namespace option -n and used multi_asic library to implement multi_asic behaviour. Added relevant unit tests to ensure functionality.

How to verify it

Run unit tests, or the commands as listed above.

bktsim-arista and others added 3 commits March 25, 2024 15:06
…-watermark related commands.

Previously, the following commands were not behaving correctly on multi-asic devices,
as the '-n' namespace option was not available, and correct namespaces were not traversed
on multi-asic devices.

* show buffer_pool watermark/persistent-watermark
* show headroom-pool watermark/persistent-watermark
* show priority-group persistent-watermark/watermark
* show queue persistent-watermark/watermark

This change fixes multi-asic behaviour of CLI commands that rely on watermarkstat,
as listed above.
Copy link
Contributor

@wenyiz2021 wenyiz2021 left a comment

Choose a reason for hiding this comment

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

LGTM.
can you please provide proof that this change also works on single asic? I think there is UT that has run to prove

self.execute(['watermarkstat', '-t', 'headroom_pool', '-n', 'asic1'],
expected_result=show_hdrm_pool_wm_output)

def test_show_invalid_asic_masic(self):

Choose a reason for hiding this comment

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

@bktsim-arista Few things -

  1. If no namespace specified, Will it show for all namespaces? if we specify, will it show for only that namespace ?
  2. Is this verified to be working fine on pizza box as well as chassis?

@vmittal-msft
Copy link

@bktsim-arista Can you please upate sonic-net/sonic-buildimage#15148 on which PR is handling which command for easy tracking ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

3 participants