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

[Mellanox] Update dscp remapping cases for Nvidia platforms #8317

Merged
merged 2 commits into from
Jun 21, 2023

Conversation

congh-nvidia
Copy link
Contributor

Description of PR

Summary:
This PR contains the fix for two test cases in DSCP remapping test: test_tunnel_decap_dscp_to_pg_mapping and test_xoff_for_pcbb.
The reason that we need this fix:

  1. Nvidia's implementation for DSCP remapping is different from Community. We need some additional mapping and the tunnel mode is pipe. For details please check the PR: [Mellanox] Support DSCP remapping in dual ToR topo on T0 switch sonic-buildimage#12605.
  2. Nvidia's cell size is 144.
  3. Need to stop packet aging in buffer.
  4. Need to add qos parameters for pcbb xoff test case.

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?

Support dscp remapping cases test_tunnel_decap_dscp_to_pg_mapping and test_xoff_for_pcbb on Nvidia platforms.

How did you do it?

Changes:
For test case test_tunnel_decap_dscp_to_pg_mapping:

  1. Add a new mapping file tunnel_qos_map_nvidia.json and use it when testing Nvidia platforms.
  2. Use cell size 144 for Nvidia platforms.
  3. Use outer DSCP instead of inner DSCP.
  4. Add some logs for easier debugging.

For test_xoff_for_pcbb:

  1. Add test data in qos.yml file for 4600C. (data for 2700 will be added in another PR)
  2. Stop packet aging in buffer before test.
  3. Skip filling the leak on Nvidia platforms for it is not needed.
  4. Use cell size 144 and packet length 300, small packets will cause descriptor exhaustion.

How did you verify/test it?

By automation, all cases passed on 4600C dualtor setup.

Any platform specific information?

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

Documentation

This commit contains the fix for two test cases in DSCP remapping test. The reason that we need this fix is Nvidia implementation for DSCP remapping is different from Community due to the limitation that Nvidia platforms can't do the remap directly on tunnel.
For test case test_tunnel_decap_dscp_to_pg_mapping:
1. Add a new mapping file tunnel_qos_map_nvidia.json and use it when testing Nvidia platforms.
2. Use cell size 144 for Nvidia platforms.
3. Use outer DSCP instead of inner DSCP.
4. Add some logs for easier debugging.
For the xoff test case:
1. Add test data in qos.yml file for 4600C.
2. Stop packet aging in buffer before test.
3. Skip filling the leak on Nvidia platforms for it is not needed.
4. Use cell size 144 and packet length 300, small packets will cause descriptor exhaustion.

Change-Id: If0090bd5bd664222cc7399598f7f46c10acebd1f
@congh-nvidia congh-nvidia marked this pull request as ready for review May 16, 2023 07:34
@congh-nvidia congh-nvidia changed the title Update dscp remapping cases for Nvidia platforms [Nvidia] Update dscp remapping cases for Nvidia platforms May 16, 2023
for k, v in list(maps['DSCP_TO_TC_MAP'][TUNNEL_MAP_NAME].items()):
ret['inner_dscp_to_pg_map'][int(k)] = int(
maps['TC_TO_PRIORITY_GROUP_MAP'][TUNNEL_MAP_NAME][v])
if is_nvidia_platform:
Copy link
Contributor

Choose a reason for hiding this comment

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

better to avoid the if-else statements for Nvidia, and do it once at the beginning.

if nvidia:
param1 = A
param2= B
else:
param1 = C
param2 = D

func_a(param1, param2)

Copy link
Contributor Author

@congh-nvidia congh-nvidia Jun 8, 2023

Choose a reason for hiding this comment

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

Hi @roy-sror, can't do it here.
As you can see the params are named according to the mapping names defined in SONiC, and for Nvidia and other platforms, we are using different maps:

if is_nvidia_platform:
for k, v in maps['DSCP_TO_TC_MAP'][UPLINK_MAP_NAME].items():
ret['inner_dscp_to_pg_map'][int(k)] = int(
maps['TC_TO_PRIORITY_GROUP_MAP'][MAP_NAME][v])
else:
for k, v in maps['DSCP_TO_TC_MAP'][TUNNEL_MAP_NAME].items():
ret['inner_dscp_to_pg_map'][int(k)] = int(
maps['TC_TO_PRIORITY_GROUP_MAP'][TUNNEL_MAP_NAME][v])

It's very hard to unify the map names, and even if we can do that, the names will be long and the code will be hard to understand.

tests/qos/tunnel_qos_remap_base.py Show resolved Hide resolved
tests/saitests/py3/sai_qos_tests.py Show resolved Hide resolved
For Nvidia(Mellanox) platforms, packets in buffer will be aged after a timeout. Need to disable this before any buffer tests.
@bingwang-ms
Copy link
Collaborator

@neethajohn Could you help review the change? Thanks!

@liat-grozovik
Copy link
Collaborator

/azp run Azure.sonic-mgmt

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liat-grozovik liat-grozovik changed the title [Nvidia] Update dscp remapping cases for Nvidia platforms [Mellanox] Update dscp remapping cases for Nvidia platforms Jun 21, 2023
@liat-grozovik liat-grozovik merged commit 00a268b into sonic-net:master Jun 21, 2023
@liat-grozovik
Copy link
Collaborator

@congh-nvidia is it needed for other branches?

@congh-nvidia
Copy link
Contributor Author

@congh-nvidia is it needed for other branches?

Yes, it's needed for 202205, but there will be conflict, I'll open a new PR for it. Thanks.

@congh-nvidia
Copy link
Contributor Author

@liat-grozovik @bingwang-ms @neethajohn , this is the manual cherry-pick for 202205 #8713, please review, thanks.

ZhaohuiS added a commit that referenced this pull request Jul 4, 2023
What is the motivation for this PR?
Fix the following issue, it only happened in py3 env, not for py2.
The error is introduced by #8317
Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
@congh-nvidia congh-nvidia deleted the dscp_remapping branch July 17, 2023 04:56
mrkcmo pushed a commit to Azarack/sonic-mgmt that referenced this pull request Oct 3, 2023
…t#8317)

This PR contains the fix for two test cases in DSCP remapping test: test_tunnel_decap_dscp_to_pg_mapping and test_xoff_for_pcbb.
The reason that we need this fix:

Nvidia's implementation for DSCP remapping is different from Community. We need some additional mapping and the tunnel mode is pipe. For details please check the PR: [Mellanox] Support DSCP remapping in dual ToR topo on T0 switch sonic-buildimage#12605.
Nvidia's cell size is 144.
Need to stop packet aging in buffer.
Need to add qos parameters for pcbb xoff test case.
mrkcmo pushed a commit to Azarack/sonic-mgmt that referenced this pull request Oct 3, 2023
What is the motivation for this PR?
Fix the following issue, it only happened in py3 env, not for py2.
The error is introduced by sonic-net#8317
Signed-off-by: Zhaohui Sun <zhaohuisun@microsoft.com>
AharonMalkin pushed a commit to AharonMalkin/sonic-mgmt that referenced this pull request Jan 25, 2024
…t#8317)

This PR contains the fix for two test cases in DSCP remapping test: test_tunnel_decap_dscp_to_pg_mapping and test_xoff_for_pcbb.
The reason that we need this fix:

Nvidia's implementation for DSCP remapping is different from Community. We need some additional mapping and the tunnel mode is pipe. For details please check the PR: [Mellanox] Support DSCP remapping in dual ToR topo on T0 switch sonic-buildimage#12605.
Nvidia's cell size is 144.
Need to stop packet aging in buffer.
Need to add qos parameters for pcbb xoff test case.
AharonMalkin pushed a commit to AharonMalkin/sonic-mgmt that referenced this pull request Jan 25, 2024
What is the motivation for this PR?
Fix the following issue, it only happened in py3 env, not for py2.
The error is introduced by sonic-net#8317
Signed-off-by: Zhaohui Sun <zhaohuisun@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