[Snappi]: New base config function to accomodate mixed-speed ingress and egress tests. #14856
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@rawal01 , @sdszhang , @selldinesh FYI. |
amitpawar12
force-pushed
the
mixed-speed-sys_base_config
branch
from
cb14d14 to
81bda2f
Compare
sdszhang
reviewed
Dec 10, 2024
| port_config_list=port_config_list, | ||
| duthost=duthost, | ||
| snappi_ports=new_snappi_ports) | ||
| pytest_assert(config_result is True, 'Fail to configure IP interfaces') |
There was a problem hiding this comment.
seems a lot of duplication with snappi_dut_base_config, setup_dut_ports. Can we reuse them?
There was a problem hiding this comment.
Hi @sdszhang ,
Thanks for your comments.
For mixed-speed ports, both the ports need to be part of l1_config and hence included in the for loop. If this not done, snappi crashes. We had raised an issue for this (#12966 ). As part of the fix, both the ports are added in l1_config so that snappi application knows that both the interfaces are of different speeds. This is not true with snappi_dut_base_config as it is handling ports of the same speed.
The snappi_dut_base_config has assert if the ports are of different speed. This function just prints a log in case interfaces used are of different speeds.
As discussed, I will add the port-channel and VLAN configuration for the same.
Thanks
amitpawar12
changed the title
amitpawar12
force-pushed
the
mixed-speed-sys_base_config
branch
from
81bda2f to
cab9f21
Compare
sdszhang
approved these changes
Dec 11, 2024
mssonicbld
pushed a commit
to mssonicbld/sonic-mgmt
that referenced
this pull request
Dec 12, 2024
…and egress tests. (sonic-net#14856) Description of PR Summary: Existing snappi_dut_base_config in tests/common/snappi_tests/snappi_fixtures.py has an assert in case, mixed-speed ingress and egress interfaces are selected. Since the interface speeds were same, the L1 configuration was done ONLY once. With mixed-speed interfaces being used as ingress and egress, the assert needs to be removed. Second issue with existing snappi_multi_base_config was that speed was set to ONLY one of the interfaces being used for the test. This was incorrect for mixed speed interfaces, causing Snappi API itself to crash. Fixes # (issue) sonic-net#12966 Approach What is the motivation for this PR? Existing snappi_dut_base_config asserts when ingress and egress interface speeds are different. Furthermore, snappi framework itself did not support mixed-speed interfaces for the test and crashed (Please see issue sonic-net#12966 ) for the same. How did you do it? Added a new function - snappi_sys_base_config which replaces the assert with info level log indicating that interfaces are of different speeds. The L1 configuration is done for all the snappi_ports and set appropriate speed for all the snappi_ports. Ideally, existing snappi_dut_base_config could be modified with additional argument mixed-speed=NONE, and then selectively run the code for the mixed-speed=TRUE. However, this being frequently used function, I will keep it as is, and add a new function to ensure, existing function is not broken. How did you verify/test it? Ran on the local clone with mixed and same speed interfaces. No issues seen. Any platform specific information? Supported testbed topology if it's a new test case? Documentation co-authorized by: jianquanye@microsoft.com
|
Cherry-pick PR to 202405: #16030 |
8 tasks
mssonicbld
pushed a commit
that referenced
this pull request
Dec 12, 2024
…and egress tests. (#14856) Description of PR Summary: Existing snappi_dut_base_config in tests/common/snappi_tests/snappi_fixtures.py has an assert in case, mixed-speed ingress and egress interfaces are selected. Since the interface speeds were same, the L1 configuration was done ONLY once. With mixed-speed interfaces being used as ingress and egress, the assert needs to be removed. Second issue with existing snappi_multi_base_config was that speed was set to ONLY one of the interfaces being used for the test. This was incorrect for mixed speed interfaces, causing Snappi API itself to crash. Fixes # (issue) #12966 Approach What is the motivation for this PR? Existing snappi_dut_base_config asserts when ingress and egress interface speeds are different. Furthermore, snappi framework itself did not support mixed-speed interfaces for the test and crashed (Please see issue #12966 ) for the same. How did you do it? Added a new function - snappi_sys_base_config which replaces the assert with info level log indicating that interfaces are of different speeds. The L1 configuration is done for all the snappi_ports and set appropriate speed for all the snappi_ports. Ideally, existing snappi_dut_base_config could be modified with additional argument mixed-speed=NONE, and then selectively run the code for the mixed-speed=TRUE. However, this being frequently used function, I will keep it as is, and add a new function to ensure, existing function is not broken. How did you verify/test it? Ran on the local clone with mixed and same speed interfaces. No issues seen. Any platform specific information? Supported testbed topology if it's a new test case? Documentation co-authorized by: jianquanye@microsoft.com
mssonicbld
added
Included in 202405 branch
and removed
Created PR to 202405 branch
labels
mssonicbld
pushed a commit
to mssonicbld/sonic-mgmt
that referenced
this pull request
Jan 2, 2025
…and egress tests. (sonic-net#14856) Description of PR Summary: Existing snappi_dut_base_config in tests/common/snappi_tests/snappi_fixtures.py has an assert in case, mixed-speed ingress and egress interfaces are selected. Since the interface speeds were same, the L1 configuration was done ONLY once. With mixed-speed interfaces being used as ingress and egress, the assert needs to be removed. Second issue with existing snappi_multi_base_config was that speed was set to ONLY one of the interfaces being used for the test. This was incorrect for mixed speed interfaces, causing Snappi API itself to crash. Fixes # (issue) sonic-net#12966 Approach What is the motivation for this PR? Existing snappi_dut_base_config asserts when ingress and egress interface speeds are different. Furthermore, snappi framework itself did not support mixed-speed interfaces for the test and crashed (Please see issue sonic-net#12966 ) for the same. How did you do it? Added a new function - snappi_sys_base_config which replaces the assert with info level log indicating that interfaces are of different speeds. The L1 configuration is done for all the snappi_ports and set appropriate speed for all the snappi_ports. Ideally, existing snappi_dut_base_config could be modified with additional argument mixed-speed=NONE, and then selectively run the code for the mixed-speed=TRUE. However, this being frequently used function, I will keep it as is, and add a new function to ensure, existing function is not broken. How did you verify/test it? Ran on the local clone with mixed and same speed interfaces. No issues seen. Any platform specific information? Supported testbed topology if it's a new test case? Documentation co-authorized by: jianquanye@microsoft.com
|
Cherry-pick PR to 202411: #16296 |
8 tasks
mssonicbld
pushed a commit
that referenced
this pull request
Jan 3, 2025
…and egress tests. (#14856) Description of PR Summary: Existing snappi_dut_base_config in tests/common/snappi_tests/snappi_fixtures.py has an assert in case, mixed-speed ingress and egress interfaces are selected. Since the interface speeds were same, the L1 configuration was done ONLY once. With mixed-speed interfaces being used as ingress and egress, the assert needs to be removed. Second issue with existing snappi_multi_base_config was that speed was set to ONLY one of the interfaces being used for the test. This was incorrect for mixed speed interfaces, causing Snappi API itself to crash. Fixes # (issue) #12966 Approach What is the motivation for this PR? Existing snappi_dut_base_config asserts when ingress and egress interface speeds are different. Furthermore, snappi framework itself did not support mixed-speed interfaces for the test and crashed (Please see issue #12966 ) for the same. How did you do it? Added a new function - snappi_sys_base_config which replaces the assert with info level log indicating that interfaces are of different speeds. The L1 configuration is done for all the snappi_ports and set appropriate speed for all the snappi_ports. Ideally, existing snappi_dut_base_config could be modified with additional argument mixed-speed=NONE, and then selectively run the code for the mixed-speed=TRUE. However, this being frequently used function, I will keep it as is, and add a new function to ensure, existing function is not broken. How did you verify/test it? Ran on the local clone with mixed and same speed interfaces. No issues seen. Any platform specific information? Supported testbed topology if it's a new test case? Documentation co-authorized by: jianquanye@microsoft.com
mssonicbld
added
Included in 202411 branch
and removed
Created PR to 202411 branch
labels
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
Summary:
Existing snappi_dut_base_config in tests/common/snappi_tests/snappi_fixtures.py has an assert in case, mixed-speed ingress and egress interfaces are selected.
Since the interface speeds were same, the L1 configuration was done ONLY once.
With mixed-speed interfaces being used as ingress and egress, the assert needs to be removed.
Second issue with existing snappi_multi_base_config was that speed was set to ONLY one of the interfaces being used for the test. This was incorrect for mixed speed interfaces, causing Snappi API itself to crash.
Fixes # (issue)
#12966
Type of change
Back port request
Approach
What is the motivation for this PR?
Existing snappi_dut_base_config asserts when ingress and egress interface speeds are different.
Furthermore, snappi framework itself did not support mixed-speed interfaces for the test and crashed (Please see issue #12966 ) for the same.
How did you do it?
Added a new function - snappi_sys_base_config which replaces the assert with info level log indicating that interfaces are of different speeds.
The L1 configuration is done for all the snappi_ports and set appropriate speed for all the snappi_ports. Ideally, existing snappi_dut_base_config could be modified with additional argument mixed-speed=NONE, and then selectively run the code for the mixed-speed=TRUE. However, this being frequently used function, I will keep it as is, and add a new function to ensure, existing function is not broken.
How did you verify/test it?
Ran on the local clone with mixed and same speed interfaces. No issues seen.
Any platform specific information?
Supported testbed topology if it's a new test case?
Documentation