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

[test_snmp_cpu]Use timeout configuration in all SNMP request, increase SNMP request timeout to 20 #16290

Merged
merged 3 commits into from
Jan 15, 2025

Conversation

yejianquan
Copy link
Collaborator

@yejianquan yejianquan commented Jan 2, 2025

Description of PR

Summary:
Fixes #30112399

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • Test case(new/improvement)

Back port request

  • 202012
  • 202205
  • 202305
  • 202311
  • 202405

Approach

What is the motivation for this PR?

Fix test_snmp_cpu failures on Cisco chassis.

How did you do it?

  1. Incorporate timeout setting in all SNMP commands
  2. Increase chassis SNMP request timeout to 20s

How did you verify/test it?

Run on Cisco chassis and it stably pass.

Any platform specific information?

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

Documentation

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yejianquan
Copy link
Collaborator Author

Test on chassis:
snmp/test_snmp_cpu.py::test_snmp_cpu[xx-sup-1] PASSED
--------------------------------------------------------------------------------------------- generated xml file: /var/src/sonic-mgmt-int/tests/logs/tr.xml ---------------------------------------------------------------------------------------------
====================================================================================================== 1 passed, 3 warnings in 1617.15s (0:26:57) =======================================================================================================
DEBUG:tests.conftest:[log_custom_msg] item: <Function test_snmp_cpu[xx-sup-1]>

@yejianquan
Copy link
Collaborator Author

Test on pizza box:

snmp/test_snmp_cpu.py::test_snmp_cpu[x-msn4600c-x PASSED

--------------------------------------------------------------------------------------------- generated xml file: /var/src/sonic-mgmt-int/tests/logs/tr.xml ---------------------------------------------------------------------------------------------
======================================================================================================= 1 passed, 3 warnings in 355.69s (0:05:55) =======================================================================================================

@@ -51,9 +51,14 @@ def test_snmp_cpu(duthosts, enum_rand_one_per_hwsku_hostname, localhost, creds_a
# Wait for load to reflect in SNMP
time.sleep(20)

# Give chassis longer timeout because it has more interfaces.
is_chassis = duthost.get_facts().get("modular_chassis")
snmp_timeout = 20 if is_chassis else 5
Copy link
Contributor

@qiluo-msft qiluo-msft Jan 8, 2025

Choose a reason for hiding this comment

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

all hwsku are using the same snmp/agentx timeout config, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @qiluo-msft ,
Yes, with sonic-net/sonic-buildimage#21316, all hwsku will share the 20s timeout.
But previously the other hwsku are not using the long timeout(20s) setting with SNMP tests.
That's why I limit the scope to chassis only incase hiding something on other platforms.

I'm ok if you suggest to leverage 20 to all hwskus, please let me know your suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

The test should use the normal timeout of normal clients in the industry. Should you add "retry" instead of "timeout"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @qiluo-msft , updated, make the 20s the default value and add comments.
For the timeout setting in the all the snmp requests, I think they should use the timeout setting of the snmp_fact.py by default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @qiluo-msft , the 'retry' in snmp is actually the logic of snmp pdus,
Here the timeout is the timeout for snmp walker requests.
Per my observation, take interface snmp walker requests as example, 1 walker request has 1600 snmp pdu requests behind.

And in our production, the timeout is greater than 20s.
So with 20s timeout, we can cover the scenario well.

@yejianquan yejianquan requested a review from qiluo-msft January 8, 2025 10:56
@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yejianquan yejianquan changed the title [test_snmp_cpu]Use timeout configuration in all SNMP request, increase chassis SNMP request timeout to 20 [test_snmp_cpu]Use timeout configuration in all SNMP request, increase SNMP request timeout to 20 Jan 14, 2025
Copy link
Contributor

@sdszhang sdszhang left a comment

Choose a reason for hiding this comment

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

lgtm

@yejianquan yejianquan merged commit c4d6a7c into sonic-net:master Jan 15, 2025
19 checks passed
mssonicbld pushed a commit to mssonicbld/sonic-mgmt that referenced this pull request Jan 15, 2025
…e SNMP request timeout to 20 (sonic-net#16290)

Description of PR
Summary:
Fixes #30112399

Approach
What is the motivation for this PR?
Fix test_snmp_cpu failures on Cisco chassis.

How did you do it?
Incorporate timeout setting in all SNMP commands
Increase chassis SNMP request timeout to 20s
How did you verify/test it?
Run on Cisco chassis and it stably pass.

co-authorized by: jianquanye@microsoft.com
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202411: #16518

mssonicbld pushed a commit to mssonicbld/sonic-mgmt that referenced this pull request Jan 15, 2025
…e SNMP request timeout to 20 (sonic-net#16290)

Description of PR
Summary:
Fixes #30112399

Approach
What is the motivation for this PR?
Fix test_snmp_cpu failures on Cisco chassis.

How did you do it?
Incorporate timeout setting in all SNMP commands
Increase chassis SNMP request timeout to 20s
How did you verify/test it?
Run on Cisco chassis and it stably pass.

co-authorized by: jianquanye@microsoft.com
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202405: #16519

mssonicbld pushed a commit that referenced this pull request Jan 15, 2025
…e SNMP request timeout to 20 (#16290)

Description of PR
Summary:
Fixes #30112399

Approach
What is the motivation for this PR?
Fix test_snmp_cpu failures on Cisco chassis.

How did you do it?
Incorporate timeout setting in all SNMP commands
Increase chassis SNMP request timeout to 20s
How did you verify/test it?
Run on Cisco chassis and it stably pass.

co-authorized by: jianquanye@microsoft.com
mssonicbld pushed a commit that referenced this pull request Jan 15, 2025
…e SNMP request timeout to 20 (#16290)

Description of PR
Summary:
Fixes #30112399

Approach
What is the motivation for this PR?
Fix test_snmp_cpu failures on Cisco chassis.

How did you do it?
Incorporate timeout setting in all SNMP commands
Increase chassis SNMP request timeout to 20s
How did you verify/test it?
Run on Cisco chassis and it stably pass.

co-authorized by: jianquanye@microsoft.com
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants