Skip to content

Multi_asic support for Counterpoll command #20875

Open
ansrajpu-git wants to merge 10 commits intosonic-net:masterfrom
ansrajpu-git:masic_counterpoll
Open

Multi_asic support for Counterpoll command #20875
ansrajpu-git wants to merge 10 commits intosonic-net:masterfrom
ansrajpu-git:masic_counterpoll

Conversation

@ansrajpu-git
Copy link
Copy Markdown
Contributor

Description of PR

Summary:
The existing "counterpoll" CLI command does not have support for multi asic.
This PR is raised to accommodate all changes required to support sonic-mgmt test w.r.t the sonic-utilities PR # sonic-net/sonic-utilities#4012 which was raised to enhance the feature for multi-asic
Fixes # (issue)

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • New Test case
    • Skipped for non-supported platforms
  • Test case improvement

Back port request

  • 202205
  • 202305
  • 202311
  • 202405
  • 202411
  • 202505

Approach

Verified the CLI used across all test suites and made changes accordingly

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

Executed related test and verified the results.

Any platform specific information?

Mutli_asic

Supported testbed topology if it's a new test case?

Documentation

@mssonicbld
Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@ansrajpu-git
Copy link
Copy Markdown
Contributor Author

@arlakshm @vmittal-msft , Kindly review this PR.

@mssonicbld
Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@ansrajpu-git
Copy link
Copy Markdown
Contributor Author

@vmittal-msft, Please review. Please note the build failure is happening because this PR has a dependency on PR# sonic-net/sonic-utilities#4012 which is still not merged.

@abdosi
Copy link
Copy Markdown
Contributor

abdosi commented Oct 29, 2025

@liamkearney-msft : please help review.

@saiarcot895
Copy link
Copy Markdown
Contributor

@ansrajpu-git This PR is required for sonic-utilities submodule update, as multi-ASIC T1 KVM tests are failing. Please help with getting this ready for merge.

@vmittal-msft FYI.

@vmittal-msft
Copy link
Copy Markdown
Contributor

@ansrajpu-git can you please share test results ? Also, is this only applicable for chassis or pizza box as well ? If generic changes, have we tested on pizza box ?

@vmittal-msft
Copy link
Copy Markdown
Contributor

/azpw run

@mssonicbld
Copy link
Copy Markdown
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@ansrajpu-git
Copy link
Copy Markdown
Contributor Author

ansrajpu-git commented Nov 26, 2025

@ansrajpu-git can you please share test results ? Also, is this only applicable for chassis or pizza box as well ? If generic changes, have we tested on pizza box ?

@vmittal-msft , Below is the snapshot of chassis results
image
image

Verified on T1 topo as well. Below are the results:
image

vmittal-msft
vmittal-msft previously approved these changes Nov 26, 2025
def disable_counterpoll(duthost, counter_type_list, asic_id=None):
for counterpoll_type in counter_type_list:
duthost.command(CounterpollConstants.COUNTERPOLL_DISABLE.format(counterpoll_type))
if duthost.is_multi_asic:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

just a thought - if asic_id == None but the duthost is multi_asic, should we iterate through the asics within this function?
as in, for multi asic duts, if you specify the asic we only operate on that asic, but if you dont it will operate on all the asics?
That way the caller of theses functions wont need to iterate / check for multi asic before calling, its all handled by the counterpoll helper functions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@liamkearney-msft , thanks for your review. The 'asic_id' is misleading, so made appropriate change as its 'asic' instance. As per my understanding multi_asic dut with single asic instance will have not None value & asic_index as '0'. Also, I kept the multi_asic check in caller function & removed from the called, to maintain a homogeneity in code.For ex., some function like -'verify_counterpoll_status' are nested.

for counterpoll_type in counter_type_list:
duthost.command(CounterpollConstants.COUNTERPOLL_DISABLE.format(counterpoll_type))
if duthost.is_multi_asic:
asic_index = " -n asic{}".format(asic_id.asic_index)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This needs an additional check to see if the image this test is running on supports the -n option. Otherwise, if this test case runs on an image without the sonic-utilities PR present (which is the current master branch of buildimage), then this command will fail, and all tests running this command will fail.

Copy link
Copy Markdown
Contributor

@saiarcot895 saiarcot895 left a comment

Choose a reason for hiding this comment

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

Please add support for checking if the image being used supports -n or not.

@mssonicbld
Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@ansrajpu-git
Copy link
Copy Markdown
Contributor Author

Please add support for checking if the image being used supports -n or not.

@saiarcot895 , this PR is dependent on sonic-utility PR-sonic-net/sonic-utilities#4012, which is already merged in master.

@saiarcot895
Copy link
Copy Markdown
Contributor

This PR will be tested against the last good build from sonic-buildimage master branch (which doesn't support the -n flag). The sonic-utilities change will not get merged/made available in sonic-buildimage unless all of the tests in sonic-mgmt master branch pass (which doesn't use the -n flag).

There is a deadlock there.

Signed-off-by: ansrajpu <anshu.rajput@nokia.com>
Signed-off-by: ansrajpu <anshu.rajput@nokia.com>
Signed-off-by: ansrajpu <anshu.rajput@nokia.com>
Signed-off-by: ansrajpu <anshu.rajput@nokia.com>
Signed-off-by: ansrajpu <anshu.rajput@nokia.com>
Signed-off-by: ansrajpu <anshu.rajput@nokia.com>
Signed-off-by: ansrajpu <anshu.rajput@nokia.com>
Signed-off-by: ansrajpu <anshu.rajput@nokia.com>
Signed-off-by: ansrajpu <anshu.rajput@nokia.com>
Signed-off-by: ansrajpu <anshu.rajput@nokia.com>
@mssonicbld
Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@ansrajpu-git
Copy link
Copy Markdown
Contributor Author

@vmittal-msft

@ansrajpu-git
Copy link
Copy Markdown
Contributor Author

This PR is depended on the new sonic-utility PR #sonic-net/sonic-utilities#4152.
Also, please update the backport release branch to 202511

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.

7 participants