Enhance test_tor_ecn by ZhaohuiS · Pull Request #6906 · sonic-net/sonic-mgmt

ZhaohuiS · 2022-11-28T14:46:00Z

Signed-off-by: Zhaohui Sun zhaohuisun@microsoft.com

Description of PR

Summary:
Fixes # (issue)

Type of change

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

Back port request

201911
202012
202205

Approach

What is the motivation for this PR?

test_tor_ecn failed because PCBB is enabled by default and outer dscp is different with inner dscp for lossy ones.

How did you do it?

Check if PCBB enabled, check dscp_to_tc_map, tc_to_queue_map, tc_to_dscp_map these tables to get expected dscp id, queue id. Replace the hardcode.
Send 100 packets instead of 10 and have 0.1 tolerance in case of noise packets.
Add packet_count parameter for tunnel_traffic_monitor, in test_tor_ecn we send 100 packets, but other scripts, it sends only 10. 10 was hardcoded in tunnel_traffic_monitor before.
Test dscp 3,4,2,6 one by one, not randomly choose one.
Only run this case on dualtor.

How did you verify/test it?

Run dualtor/test_tor_ecn.py

Any platform specific information?

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

Documentation

Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>

azure-pipelines · 2022-11-28T14:46:52Z

The pre-commit check detected issues in the files touched by this pull request.
The detected issues may be old or new. For new issues, please try to fix them.

For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame
author of this pull request. But if you can take extra effort to fix the old issues as well, that would be great!

Detailed pre-commit check results:
trim trailing whitespace.................................................Failed - hook id: trailing-whitespace - exit code: 1 - files were modified by this hook Fixing tests/dualtor/test_tor_ecn.py 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/common/dualtor/tunnel_traffic_utils.py:53:5: E722 do not use bare 'except' tests/common/dualtor/tunnel_traffic_utils.py:57:1: E302 expected 2 blank lines, found 1 tests/common/dualtor/tunnel_traffic_utils.py:74:5: E722 do not use bare 'except' tests/common/dualtor/tunnel_traffic_utils.py:93:1: E302 expected 2 blank lines, found 1 tests/common/dualtor/tunnel_traffic_utils.py:113:121: E501 line too long (128 > 120 characters) ... [truncated extra lines, please run pre-commit locally to view full check results]

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

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.
Ensure that the pre-commit package is installed:

sudo pip install pre-commit

Go to repository root folder
Install the pre-commit hooks:

pre-commit install

Use pre-commit to check staged file:

pre-commit

Alternatively, you can check committed files using:

pre-commit run --from-ref <commit_id> --to-ref <commit_id>

lgtm-com · 2022-11-28T15:06:06Z

This pull request introduces 2 alerts and fixes 1 when merging c3e25c9 into 6ab6dbc - view on LGTM.com

new alerts:

2 for Except block handles 'BaseException'

fixed alerts:

1 for Testing equality to None

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

tests/dualtor/test_tor_ecn.py

bingwang-ms · 2022-11-30T02:21:08Z

Also, as we discussed, please consider looping through all lossless DSCP values instead of ramdon choice.

Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>

azure-pipelines · 2022-11-30T08:14:58Z

The pre-commit check detected issues in the files touched by this pull request.
The detected issues may be old or new. For new issues, please try to fix them.

For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame
author of this pull request. But if you can take extra effort to fix the old issues as well, that would be great!

Detailed pre-commit check results:
trim trailing whitespace.................................................Failed - hook id: trailing-whitespace - exit code: 1 - files were modified by this hook Fixing tests/dualtor/test_tor_ecn.py 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/common/dualtor/tunnel_traffic_utils.py:53:5: E722 do not use bare 'except' tests/common/dualtor/tunnel_traffic_utils.py:57:1: E302 expected 2 blank lines, found 1 tests/common/dualtor/tunnel_traffic_utils.py:74:5: E722 do not use bare 'except' tests/common/dualtor/tunnel_traffic_utils.py:93:1: E302 expected 2 blank lines, found 1 tests/common/dualtor/tunnel_traffic_utils.py:113:121: E501 line too long (128 > 120 characters) ... [truncated extra lines, please run pre-commit locally to view full check results]

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

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.
Ensure that the pre-commit package is installed:

sudo pip install pre-commit

Go to repository root folder
Install the pre-commit hooks:

pre-commit install

Use pre-commit to check staged file:

pre-commit

Alternatively, you can check committed files using:

pre-commit run --from-ref <commit_id> --to-ref <commit_id>

lgtm-com · 2022-11-30T08:35:24Z

This pull request introduces 2 alerts and fixes 1 when merging c0214b8 into 4b26186 - view on LGTM.com

new alerts:

2 for Except block handles 'BaseException'

fixed alerts:

1 for Testing equality to None

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>

azure-pipelines · 2022-12-01T06:12:31Z

The pre-commit check detected issues in the files touched by this pull request.
The detected issues may be old or new. For new issues, please try to fix them.

For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame
author of this pull request. But if you can take extra effort to fix the old issues as well, that would be great!

Detailed pre-commit check results:
trim trailing whitespace.................................................Failed - hook id: trailing-whitespace - exit code: 1 - files were modified by this hook Fixing tests/dualtor/test_tor_ecn.py 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/common/dualtor/tunnel_traffic_utils.py:53:5: E722 do not use bare 'except' tests/common/dualtor/tunnel_traffic_utils.py:57:1: E302 expected 2 blank lines, found 1 tests/common/dualtor/tunnel_traffic_utils.py:74:5: E722 do not use bare 'except' tests/common/dualtor/tunnel_traffic_utils.py:93:1: E302 expected 2 blank lines, found 1 tests/common/dualtor/tunnel_traffic_utils.py:113:121: E501 line too long (128 > 120 characters) ... [truncated extra lines, please run pre-commit locally to view full check results]

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

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.
Ensure that the pre-commit package is installed:

sudo pip install pre-commit

Go to repository root folder
Install the pre-commit hooks:

pre-commit install

Use pre-commit to check staged file:

pre-commit

Alternatively, you can check committed files using:

pre-commit run --from-ref <commit_id> --to-ref <commit_id>

ZhaohuiS · 2022-12-01T08:57:02Z

Also, as we discussed, please consider looping through all lossless DSCP values instead of ramdon choice.

Done.

Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>

azure-pipelines · 2022-12-01T09:34:18Z

The pre-commit check detected issues in the files touched by this pull request.
The detected issues may be old or new. For new issues, please try to fix them.

For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame
author of this pull request. But if you can take extra effort to fix the old issues as well, that would be great!

Detailed pre-commit check results:
trim trailing whitespace.................................................Failed - hook id: trailing-whitespace - exit code: 1 - files were modified by this hook Fixing tests/dualtor/test_tor_ecn.py 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/common/dualtor/tunnel_traffic_utils.py:57:1: E302 expected 2 blank lines, found 1 tests/common/dualtor/tunnel_traffic_utils.py:75:121: E501 line too long (124 > 120 characters) tests/common/dualtor/tunnel_traffic_utils.py:93:1: E302 expected 2 blank lines, found 1 tests/common/dualtor/tunnel_traffic_utils.py:113:121: E501 line too long (128 > 120 characters) tests/common/dualtor/tunnel_traffic_utils.py:131:50: F821 undefined name 'reduce' ... [truncated extra lines, please run pre-commit locally to view full check results]

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

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.
Ensure that the pre-commit package is installed:

sudo pip install pre-commit

Go to repository root folder
Install the pre-commit hooks:

pre-commit install

Use pre-commit to check staged file:

pre-commit

Alternatively, you can check committed files using:

pre-commit run --from-ref <commit_id> --to-ref <commit_id>

lgtm-com · 2022-12-01T09:47:31Z

This pull request fixes 1 alert when merging df698db into e4ba888 - view on LGTM.com

fixed alerts:

1 for Testing equality to None

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

What is the motivation for this PR? test_tor_ecn failed because PCBB is enabled by default and outer dscp is different with inner dscp for lossy ones. How did you do it? Check if PCBB enabled, check dscp_to_tc_map, tc_to_queue_map, tc_to_dscp_map these tables to get expected dscp id, queue id. Replace the hardcode. Send 100 packets instead of 10 and have 0.1 tolerance in case of noise packets. Add packet_count parameter for tunnel_traffic_monitor, in test_tor_ecn we send 100 packets, but other scripts, it sends only 10. 10 was hardcoded in tunnel_traffic_monitor before. Test dscp 3,4,2,6 one by one, not randomly choose one. Only run this case on dualtor. How did you verify/test it? Run dualtor/test_tor_ecn.py Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>

What is the motivation for this PR? In #6906, it fixed test_tor_ecn failures. No need to xfail it. How did you do it? Remove test_tor_ecn from xfail condition mark How did you verify/test it? Run dualtor/test_tor_ecn.py Any platform specific information? Supported testbed topology if it's a new test case?

What is the motivation for this PR? In #6906, it fixed test_tor_ecn failures. No need to xfail it. How did you do it? Remove test_tor_ecn from xfail condition mark How did you verify/test it? Run dualtor/test_tor_ecn.py Any platform specific information?

…ic-mgmt into internal-202205 Fix merge conflicts. - [pre-commit] Fix style issues in test scripts under `tests/acl` folder (sonic-net#6679) - Moving check for reboot cause after interface status check (sonic-net#6721) - Adding watchdog timeout values for Cisco 8808 Supervisor and Different LCs (sonic-net#6776) - add Ether check in macsec_dp_poll (sonic-net#6828) - Disable PFC watchdog in test_cpu_memory_usage_counterpoll (sonic-net#6851) - Testcase to verify that lossless traffic is not dropped during congesion. (sonic-net#6853) - Ignore Broadcom sai sai unbind ERR log for now (sonic-net#6539) - [chassis][multi-asic] update the loganalyser regex for multi asic (sonic-net#6885) - [mx] Fix test_acl failed on mx topo (sonic-net#6971) (sonic-net#6983) - [202205][mx] Add support for mx in test_null_route_helper (sonic-net#6967) (sonic-net#6982) - [m0][everflow] Add m0 support for everflow and refactor everflow setup_info (sonic-net#6900) - [ACL] Add acl stress test (sonic-net#6903) - Enhance test_tor_ecn (sonic-net#6906) - Fix erros - Added unique IPV6 address for the missed ACL rules PR sonic-net#6390 (sonic-net#6909) - enabled bfd tests (sonic-net#6919) - Skip bgp speaker test on backend topo (sonic-net#6922) - [advanced-reboot] Handle logs in tmpfs: backup two log files before reboot (sonic-net#6923) - Fix missing definition (sonic-net#6930) - [Mellanox] Add minimal table definition for SN2201 (sonic-net#6943) - Update qos test param for dualtor topology (sonic-net#6948) - fix setup for single asic lc (sonic-net#6951) - Fix QoS sai test for running with python3 (sonic-net#6961) - Don't fail if logrotate cron job file isn't present (sonic-net#6964) - Disable post sanity check for vxlan test (sonic-net#6980) - Merge branch 'azure-202205' into dev/yaqiangzhu/202205_merge

Enhance test_tor_ecn with PCBB enabled

c3e25c9

Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>

ZhaohuiS added Request for 202012 branch Request for 202205 branch labels Nov 28, 2022

ZhaohuiS changed the title ~~Enhance test_tor_ecn with PCBB enabled~~ Enhance test_tor_ecn Nov 29, 2022

bingwang-ms reviewed Nov 29, 2022

View reviewed changes

tests/dualtor/test_tor_ecn.py Outdated Show resolved Hide resolved

bingwang-ms reviewed Nov 29, 2022

View reviewed changes

tests/dualtor/test_tor_ecn.py Outdated Show resolved Hide resolved

bingwang-ms reviewed Nov 29, 2022

View reviewed changes

tests/dualtor/test_tor_ecn.py Show resolved Hide resolved

Run lossless queues not randomly choose one

c0214b8

Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>

This was referenced Dec 1, 2022

Remove test_tor_ecn from xfail condition mark #6937

Merged

Remove test_tor_ecn from xfail condition mark #6939

Merged

Only run test_tor_ecn on dualtor testbed

55b8a66

Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>

bingwang-ms previously approved these changes Dec 1, 2022

View reviewed changes

Fix LGTM

df698db

Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>

ZhaohuiS dismissed bingwang-ms’s stale review via df698db December 1, 2022 09:27

ZhaohuiS removed Request for 202012 branch Request for 202205 branch labels Dec 2, 2022

bingwang-ms approved these changes Dec 2, 2022

View reviewed changes

ZhaohuiS merged commit be0f9ab into sonic-net:master Dec 2, 2022

ZhaohuiS added Request for 202012 branch Request for 202205 branch labels Dec 3, 2022

wangxin added the Included in 202012 branch label Dec 7, 2022

wangxin added the Included in 202205 branch label Dec 7, 2022

ZhaohuiS mentioned this pull request Dec 11, 2022

[202205 xfail] test_tor_ecn xfail for 202205 branch #6661

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance test_tor_ecn#6906

Enhance test_tor_ecn#6906
ZhaohuiS merged 4 commits intosonic-net:masterfrom
ZhaohuiS:fix/ecn_enhance

ZhaohuiS commented Nov 28, 2022 •

edited

Loading

Uh oh!

azure-pipelines bot commented Nov 28, 2022

Uh oh!

lgtm-com bot commented Nov 28, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bingwang-ms commented Nov 30, 2022

Uh oh!

azure-pipelines bot commented Nov 30, 2022

Uh oh!

lgtm-com bot commented Nov 30, 2022

Uh oh!

azure-pipelines bot commented Dec 1, 2022

Uh oh!

ZhaohuiS commented Dec 1, 2022

Uh oh!

azure-pipelines bot commented Dec 1, 2022

Uh oh!

lgtm-com bot commented Dec 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

ZhaohuiS commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of PR

Type of change

Back port request

Approach

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

Any platform specific information?

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

Documentation

Uh oh!

azure-pipelines bot commented Nov 28, 2022

Uh oh!

lgtm-com bot commented Nov 28, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bingwang-ms commented Nov 30, 2022

Uh oh!

azure-pipelines bot commented Nov 30, 2022

Uh oh!

lgtm-com bot commented Nov 30, 2022

Uh oh!

azure-pipelines bot commented Dec 1, 2022

Uh oh!

ZhaohuiS commented Dec 1, 2022

Uh oh!

azure-pipelines bot commented Dec 1, 2022

Uh oh!

lgtm-com bot commented Dec 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ZhaohuiS commented Nov 28, 2022 •

edited

Loading