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

feat: support stress routes test on T2 topo #13341

Merged
merged 3 commits into from
Jun 21, 2024

Conversation

cyw233
Copy link
Contributor

@cyw233 cyw233 commented Jun 18, 2024

Description of PR

Support test_stress_routes.py on T2 topology.

Summary:
Fixes # (issue) Microsoft ADO 28357470

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?

Currently, the test_stress_routes.py only runs on the following topologies: T0, T1, M0, MX. We wanted to support this test on T2 topology as well.

How did you do it?

Add T2 topology marker and make necessary changes to make the test pass on a T2 testbed.

How did you verify/test it?

I run the updated test on a T2 testbed and confirm it passed.

Any platform specific information?

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

Documentation

@mssonicbld
Copy link
Collaborator

The pre-commit check detected issues in the files touched by this pull request.
The pre-commit check is a mandatory check, please fix detected issues.

Detailed pre-commit check results:
trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check yaml...........................................(no files to check)Skipped
check for added large files..............................................Passed
check python ast.........................................................Passed
flake8...................................................................Failed
- hook id: flake8
- exit code: 1

tests/stress/conftest.py:3:1: F401 'time' imported but unused
tests/stress/conftest.py:6:1: F401 'tests.common.config_reload' imported but unused

flake8...............................................(no files to check)Skipped
check conditional mark sort..........................(no files to check)Skipped

To run the pre-commit checks locally, you can follow below steps:

  1. Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
    the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
    docker container.
  2. Ensure that the pre-commit package is installed:
sudo pip install pre-commit
  1. Go to repository root folder
  2. Install the pre-commit hooks:
pre-commit install
  1. Use pre-commit to check staged file:
pre-commit
  1. Alternatively, you can check committed files using:
pre-commit run --from-ref <commit_id> --to-ref <commit_id>

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

@@ -12,8 +14,8 @@
}


def get_crm_resources(duthost, resource, status):
return duthost.get_crm_resources().get("main_resources").get(resource).get(status)
def get_crm_resource_status(duthost, resource, status, namespace=DEFAULT_NAMESPACE):
Copy link
Contributor

Choose a reason for hiding this comment

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

since you changed this funciton name, can you make sure all other places that were using get_crm_resources are fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @wenyiz2021, I did some search in the repo and found the following tests are using function get_crm_resources() (same function name from various places):

  • arp/test_stress_arp.py
  • crm/test_crm.py
  • fdb/test_fdb_mac_move.py
  • vxlan/test_vxlan_crm.py
  • vxlan/test_vxlan_ecmp.py

Then I ran these tests and can confirm they are still working as expected.

Copy link
Contributor

@yutongzhang-microsoft yutongzhang-microsoft Jun 19, 2024

Choose a reason for hiding this comment

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

Hi, @cyw233 , should we change other test scripts to support T2 as well?

Copy link
Contributor Author

@cyw233 cyw233 Jun 19, 2024

Choose a reason for hiding this comment

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

Hey @yutongzhang-microsoft, we have checked all the skipped tests on T2 and documented the ones that indeed need to be supported on T2. For example, this test_stress_routes should run on T2, while test_stress_arp should always be skipped on T2 because it's supposed to be run on T0 only.
This is the very beginning of the T2 test gap and we will make more test cases supported on T2, for example, supporting test_bfd.py on T2 is my very next ticket.

Copy link
Contributor

@yutongzhang-microsoft yutongzhang-microsoft left a comment

Choose a reason for hiding this comment

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

LGTM

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
every util has its own get_crm_resources
duthost has its own get_crm_resources
this get_crm_resource_status is specific to tests/stress/xx tests

@yejianquan yejianquan merged commit 88ac7c3 into sonic-net:master Jun 21, 2024
19 checks passed
@mssonicbld
Copy link
Collaborator

@cyw233 PR conflicts with 202205 branch

@cyw233
Copy link
Contributor Author

cyw233 commented Jun 21, 2024

@cyw233 PR conflicts with 202205 branch

ack

cyw233 added a commit to cyw233/sonic-mgmt that referenced this pull request Jun 21, 2024
@cyw233
Copy link
Contributor Author

cyw233 commented Jun 21, 2024

Cherry pick PR to merge into 202205 branch: #13394

yejianquan pushed a commit that referenced this pull request Jun 24, 2024
Description of PR
Support test_stress_routes.py on T2 topology.

Summary:
Fixes # (issue) Microsoft ADO 28357470

Approach
What is the motivation for this PR?
Currently, the test_stress_routes.py only runs on the following topologies: T0, T1, M0, MX. We wanted to support this test on T2 topology as well.

How did you do it?
Add T2 topology marker and make necessary changes to make the test pass on a T2 testbed.

How did you verify/test it?
I run the updated test on a T2 testbed and confirm it passed.

co-authorized by: jianquanye@microsoft.com
mssonicbld pushed a commit to mssonicbld/sonic-mgmt that referenced this pull request Jun 24, 2024
Description of PR
Support test_stress_routes.py on T2 topology.

Summary:
Fixes # (issue) Microsoft ADO 28357470

Approach
What is the motivation for this PR?
Currently, the test_stress_routes.py only runs on the following topologies: T0, T1, M0, MX. We wanted to support this test on T2 topology as well.

How did you do it?
Add T2 topology marker and make necessary changes to make the test pass on a T2 testbed.

How did you verify/test it?
I run the updated test on a T2 testbed and confirm it passed.

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

Cherry-pick PR to 202405: #13421

mssonicbld pushed a commit that referenced this pull request Jun 26, 2024
Description of PR
Support test_stress_routes.py on T2 topology.

Summary:
Fixes # (issue) Microsoft ADO 28357470

Approach
What is the motivation for this PR?
Currently, the test_stress_routes.py only runs on the following topologies: T0, T1, M0, MX. We wanted to support this test on T2 topology as well.

How did you do it?
Add T2 topology marker and make necessary changes to make the test pass on a T2 testbed.

How did you verify/test it?
I run the updated test on a T2 testbed and confirm it passed.

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.

6 participants