-
Notifications
You must be signed in to change notification settings - Fork 744
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
[tests/copp]: Fix copp tests #12836
[tests/copp]: Fix copp tests #12836
Conversation
The pre-commit check detected issues in the files touched by this pull request. Detailed pre-commit check results: To run the pre-commit checks locally, you can follow below steps:
|
a384552
to
fab5392
Compare
The pre-commit check detected issues in the files touched by this pull request. Detailed pre-commit check results: To run the pre-commit checks locally, you can follow below steps:
|
fab5392
to
3dab091
Compare
The pre-commit check detected issues in the files touched by this pull request. Detailed pre-commit check results: To run the pre-commit checks locally, you can follow below steps:
|
Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
This reverts commit 31101a5.
3dab091
to
742787d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall lgtm, @StormLiangMS , @ZhaohuiS , can one of you review and signoff?
The pre-commit check detected issues in the files touched by this pull request. Detailed pre-commit check results: To run the pre-commit checks locally, you can follow below steps:
|
@prabhataravind could you please fix the pre-commit failures?
ansible/roles/test/files/ptftests/py3/copp_tests.py:267:121: E501 line too long (128 > 120 characters) |
The pre-commit check detected issues in the files touched by this pull request. Detailed pre-commit check results: To run the pre-commit checks locally, you can follow below steps:
|
Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
04b3be6
to
46d2d78
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/azpw run |
@StormLiangMS, could you please help cherry-pick to 202305 as well when ready? |
@ZhaohuiS for viz |
What is the motivation for this PR? Recently, a change was made in the test_policer testcase in copp_tests.py to retain the default value of cir/cbs for queue4_group3 (default value for mgmt is 300) instead of changing it to 600 on the DUT during the testcase run. The PR for this change is here - #12836. For marvell based platforms like Nokia-7215-T1 and Nokia-7215-A1, the cir/cbs values are supported in steps of 125. Therefore, a value of 300 in the copp_cfg.json will set it to 250 internally since 300 is not in steps of 125. This was confirmed by Marvell - "Per talk, the policer/ traffic metering can work in packet per-second mode. The granularity is in step of 125 pps. So it’s 125, 250 …etc Minimum is 125." The PPS_LIMIT_MIN and PPS_LIMIT_MAX is therefore set in the testcase to 270,390 respectively which is completely out of range and causes this testcase to fail. Therefore, this PR sets cir/cbs to 250 for Marvell based platforms. How did you do it? Added a platform specific check in copp_tests.py to change the PPS_LIMIT to 250 for Nokia platforms. How did you verify/test it? Ran test_policer testcase for DHCP, DHCP6, LLDP and UDLD. Verified that the testcase passes.
What is the motivation for this PR? Recently, a change was made in the test_policer testcase in copp_tests.py to retain the default value of cir/cbs for queue4_group3 (default value for mgmt is 300) instead of changing it to 600 on the DUT during the testcase run. The PR for this change is here - sonic-net#12836. For marvell based platforms like Nokia-7215-T1 and Nokia-7215-A1, the cir/cbs values are supported in steps of 125. Therefore, a value of 300 in the copp_cfg.json will set it to 250 internally since 300 is not in steps of 125. This was confirmed by Marvell - "Per talk, the policer/ traffic metering can work in packet per-second mode. The granularity is in step of 125 pps. So it’s 125, 250 …etc Minimum is 125." The PPS_LIMIT_MIN and PPS_LIMIT_MAX is therefore set in the testcase to 270,390 respectively which is completely out of range and causes this testcase to fail. Therefore, this PR sets cir/cbs to 250 for Marvell based platforms. How did you do it? Added a platform specific check in copp_tests.py to change the PPS_LIMIT to 250 for Nokia platforms. How did you verify/test it? Ran test_policer testcase for DHCP, DHCP6, LLDP and UDLD. Verified that the testcase passes.
What is the motivation for this PR? Recently, a change was made in the test_policer testcase in copp_tests.py to retain the default value of cir/cbs for queue4_group3 (default value for mgmt is 300) instead of changing it to 600 on the DUT during the testcase run. The PR for this change is here - #12836. For marvell based platforms like Nokia-7215-T1 and Nokia-7215-A1, the cir/cbs values are supported in steps of 125. Therefore, a value of 300 in the copp_cfg.json will set it to 250 internally since 300 is not in steps of 125. This was confirmed by Marvell - "Per talk, the policer/ traffic metering can work in packet per-second mode. The granularity is in step of 125 pps. So it’s 125, 250 …etc Minimum is 125." The PPS_LIMIT_MIN and PPS_LIMIT_MAX is therefore set in the testcase to 270,390 respectively which is completely out of range and causes this testcase to fail. Therefore, this PR sets cir/cbs to 250 for Marvell based platforms. How did you do it? Added a platform specific check in copp_tests.py to change the PPS_LIMIT to 250 for Nokia platforms. How did you verify/test it? Ran test_policer testcase for DHCP, DHCP6, LLDP and UDLD. Verified that the testcase passes.
What is the motivation for this PR? Recently, a change was made in the test_policer testcase in copp_tests.py to retain the default value of cir/cbs for queue4_group3 (default value for mgmt is 300) instead of changing it to 600 on the DUT during the testcase run. The PR for this change is here - sonic-net#12836. For marvell based platforms like Nokia-7215-T1 and Nokia-7215-A1, the cir/cbs values are supported in steps of 125. Therefore, a value of 300 in the copp_cfg.json will set it to 250 internally since 300 is not in steps of 125. This was confirmed by Marvell - "Per talk, the policer/ traffic metering can work in packet per-second mode. The granularity is in step of 125 pps. So it’s 125, 250 …etc Minimum is 125." The PPS_LIMIT_MIN and PPS_LIMIT_MAX is therefore set in the testcase to 270,390 respectively which is completely out of range and causes this testcase to fail. Therefore, this PR sets cir/cbs to 250 for Marvell based platforms. How did you do it? Added a platform specific check in copp_tests.py to change the PPS_LIMIT to 250 for Nokia platforms. How did you verify/test it? Ran test_policer testcase for DHCP, DHCP6, LLDP and UDLD. Verified that the testcase passes.
What is the motivation for this PR? Recently, a change was made in the test_policer testcase in copp_tests.py to retain the default value of cir/cbs for queue4_group3 (default value for mgmt is 300) instead of changing it to 600 on the DUT during the testcase run. The PR for this change is here - #12836. For marvell based platforms like Nokia-7215-T1 and Nokia-7215-A1, the cir/cbs values are supported in steps of 125. Therefore, a value of 300 in the copp_cfg.json will set it to 250 internally since 300 is not in steps of 125. This was confirmed by Marvell - "Per talk, the policer/ traffic metering can work in packet per-second mode. The granularity is in step of 125 pps. So it’s 125, 250 …etc Minimum is 125." The PPS_LIMIT_MIN and PPS_LIMIT_MAX is therefore set in the testcase to 270,390 respectively which is completely out of range and causes this testcase to fail. Therefore, this PR sets cir/cbs to 250 for Marvell based platforms. How did you do it? Added a platform specific check in copp_tests.py to change the PPS_LIMIT to 250 for Nokia platforms. How did you verify/test it? Ran test_policer testcase for DHCP, DHCP6, LLDP and UDLD. Verified that the testcase passes.
This PR does the following: Enhance copp test cases so that they pass consistently on Broadcom platforms. For Broadcom platforms, when BGP and IP2ME traps are removed, BGP packets hit the default trap group, so a new DefaultPolicy is introduced in check_constraints specifically for Broadcom ASIC types. The test logic for non-Broadcom platforms is left unchanged. A necessary side change of this is that the CIR for default trap needs to be changed to a value other than 600 only for the test. This is reverted back at the end of the test. Remove xfail for copp tests on broadcom platforms Skip updating default sonic config for queue4_group3 in scripts/update_copp_config.py as the new rate-limit for this copp group is lower than the default value of 600 PPS used by copp tests. This value is 300PPS for Mgmt devices and 100 PPS for other devices. The constraint checks for protocols that map to queue4_group3 are also changed accordingly in ansible/roles/test/files/ptftests/py3/copp_tests.py Summary: Fixes # 27262099 What is the motivation for this PR? When BGP and IP2ME traps are removed, BGP packets hit the default trap group on broadcom platforms and trap to CPU queue0. This change modifies copp test to verify this behavior. How did you do it? By changing the rate limit used in the test for default trap group, it is easy to verify that packets are hitting that trap group. If all trap groups use the same rate limit of 600 pps as is done currently, it is rather difficult to identify which trap group got hit. How did you verify/test it? By running tests locally and using azure pipeline
Cherry-pick PR to 202305: #14290 |
This PR does the following: Enhance copp test cases so that they pass consistently on Broadcom platforms. For Broadcom platforms, when BGP and IP2ME traps are removed, BGP packets hit the default trap group, so a new DefaultPolicy is introduced in check_constraints specifically for Broadcom ASIC types. The test logic for non-Broadcom platforms is left unchanged. A necessary side change of this is that the CIR for default trap needs to be changed to a value other than 600 only for the test. This is reverted back at the end of the test. Remove xfail for copp tests on broadcom platforms Skip updating default sonic config for queue4_group3 in scripts/update_copp_config.py as the new rate-limit for this copp group is lower than the default value of 600 PPS used by copp tests. This value is 300PPS for Mgmt devices and 100 PPS for other devices. The constraint checks for protocols that map to queue4_group3 are also changed accordingly in ansible/roles/test/files/ptftests/py3/copp_tests.py Summary: Fixes # 27262099 What is the motivation for this PR? When BGP and IP2ME traps are removed, BGP packets hit the default trap group on broadcom platforms and trap to CPU queue0. This change modifies copp test to verify this behavior. How did you do it? By changing the rate limit used in the test for default trap group, it is easy to verify that packets are hitting that trap group. If all trap groups use the same rate limit of 600 pps as is done currently, it is rather difficult to identify which trap group got hit. How did you verify/test it? By running tests locally and using azure pipeline
Upstream PR sonic-net#12836 changed the copp policer limit for some traffic classes from 300 to 100, for all DUTs except M0. Pkz should be included in their M0 list as well (from testing applies to mx too)
What is the motivation for this PR? Recently, a change was made in the test_policer testcase in copp_tests.py to retain the default value of cir/cbs for queue4_group3 (default value for mgmt is 300) instead of changing it to 600 on the DUT during the testcase run. The PR for this change is here - sonic-net#12836. For marvell based platforms like Nokia-7215-T1 and Nokia-7215-A1, the cir/cbs values are supported in steps of 125. Therefore, a value of 300 in the copp_cfg.json will set it to 250 internally since 300 is not in steps of 125. This was confirmed by Marvell - "Per talk, the policer/ traffic metering can work in packet per-second mode. The granularity is in step of 125 pps. So it’s 125, 250 …etc Minimum is 125." The PPS_LIMIT_MIN and PPS_LIMIT_MAX is therefore set in the testcase to 270,390 respectively which is completely out of range and causes this testcase to fail. Therefore, this PR sets cir/cbs to 250 for Marvell based platforms. How did you do it? Added a platform specific check in copp_tests.py to change the PPS_LIMIT to 250 for Nokia platforms. How did you verify/test it? Ran test_policer testcase for DHCP, DHCP6, LLDP and UDLD. Verified that the testcase passes.
What is the motivation for this PR? Recently, a change was made in the test_policer testcase in copp_tests.py to retain the default value of cir/cbs for queue4_group3 (default value for mgmt is 300) instead of changing it to 600 on the DUT during the testcase run. The PR for this change is here - sonic-net#12836. For marvell based platforms like Nokia-7215-T1 and Nokia-7215-A1, the cir/cbs values are supported in steps of 125. Therefore, a value of 300 in the copp_cfg.json will set it to 250 internally since 300 is not in steps of 125. This was confirmed by Marvell - "Per talk, the policer/ traffic metering can work in packet per-second mode. The granularity is in step of 125 pps. So it’s 125, 250 …etc Minimum is 125." The PPS_LIMIT_MIN and PPS_LIMIT_MAX is therefore set in the testcase to 270,390 respectively which is completely out of range and causes this testcase to fail. Therefore, this PR sets cir/cbs to 250 for Marvell based platforms. How did you do it? Added a platform specific check in copp_tests.py to change the PPS_LIMIT to 250 for Nokia platforms. How did you verify/test it? Ran test_policer testcase for DHCP, DHCP6, LLDP and UDLD. Verified that the testcase passes.
Description of PR
This PR does the following:
DefaultPolicy is introduced in check_constraints specifically for Broadcom ASIC types. The test logic for non-Broadcom
platforms is left unchanged. A necessary side change of this is that the CIR for default trap needs to be changed to a
value other than 600 only for the test. This is reverted back at the end of the test.
group is lower than the default value of 600 PPS used by copp tests. This value is 300PPS for Mgmt devices and 100 PPS for
other devices. The constraint checks for protocols that map to queue4_group3 are also changed accordingly in
ansible/roles/test/files/ptftests/py3/copp_tests.py
Summary:
Fixes # 27262099
Type of change
Back port request
Approach
What is the motivation for this PR?
When BGP and IP2ME traps are removed, BGP packets hit the default trap group on broadcom platforms and trap to CPU queue0. This change modifies copp test to verify this behavior.
How did you do it?
By changing the rate limit used in the test for default trap group, it is easy to verify that packets are hitting that trap group. If all trap groups use the same rate limit of 600 pps as is done currently, it is rather difficult to identify which trap group got hit.
How did you verify/test it?
By running tests locally and using azure pipeline
Any platform specific information?
Supported testbed topology if it's a new test case?
Documentation