Skip to content

Fix qos/test_qos_sai.py#12334

Merged
yxieca merged 2 commits intosonic-net:masterfrom
vivekverma-arista:fix-qos-sai-peer-ipv6
Apr 11, 2024
Merged

Fix qos/test_qos_sai.py#12334
yxieca merged 2 commits intosonic-net:masterfrom
vivekverma-arista:fix-qos-sai-peer-ipv6

Conversation

@vivekverma-arista
Copy link
Copy Markdown
Contributor

@vivekverma-arista vivekverma-arista commented Apr 8, 2024 •
edited
Loading

Description of PR

Summary: Fix qos/test_qos_sai.py
Fixes # (issue)

Type of change

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

Back port request

  • 201911
  • 202012
  • 202205
  • 202305
  • 202311

Approach

What is the motivation for this PR?

Regression introduced by #12088

qos/test_qos_sai.py fails with the following traceback -

14:44:01 __init__._fixture_generator_decorator    L0088 ERROR  | 
KeyError('peer_addr_ipv6')
Traceback (most recent call last):
  File "/var/src/sonic-mgmt_vms21-t0-2700_646f1402735219c3e5444094/tests/common/plugins/log_section_start/__init__.py", line 84, in _fixture_generator_decorator
    res = next(it)
  File "/var/src/sonic-mgmt_vms21-t0-2700_646f1402735219c3e5444094/tests/qos/qos_sai_base.py", line 1089, in dutConfig
    testPorts = self.__buildTestPorts(request, testPortIds, testPortIps,
  File "/var/src/sonic-mgmt_vms21-t0-2700_646f1402735219c3e5444094/tests/qos/qos_sai_base.py", line 766, in __buildTestPorts
    "dst_port_ipv6": dst_test_port_ips[dstPort]['peer_addr_ipv6'],
KeyError: 'peer_addr_ipv6'

How did you do it?

Downlink ports don't have BGP neighbors in case of T0, hence the above change is causing the key error.

The proposed fix is to modify the testPorts dictionary with peer_addr_ipv6 only if it is defined.

How did you verify/test it?

Verified on Arista-7260 with dualtor topology.

Any platform specific information?

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

Documentation

@vivekverma-arista
Regression introduced by: sonic-net#10941
66515b3 
Downlink ports don't have BGP neighbors in case of T0, hence the above change is
causing the key error.

The proposed fix is to modify the testPorts dictionary with peer_addr_ipv6 only if
it is defined.
@vivekverma-arista vivekverma-arista mentioned this pull request Apr 8, 2024
Merged
7 tasks
@mssonicbld
Copy link
Copy Markdown
Collaborator

mssonicbld commented Apr 8, 2024

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/qos/qos_sai_base.py:806:5: E303 too many blank lines (2)

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>

@vivekverma-arista
Copy link
Copy Markdown
Contributor Author

vivekverma-arista commented Apr 8, 2024

Hi @vmittal-msft @bingwang-ms, can you review this PR?

vmittal-msft
vmittal-msft reviewed Apr 8, 2024
View reviewed changes
@vmittal-msft vmittal-msft requested a review from bingwang-ms April 8, 2024 17:16
@vmittal-msft
Copy link
Copy Markdown
Contributor

vmittal-msft commented Apr 10, 2024

@yxieca @wangxin please help merge.

@yxieca yxieca merged commit fcd6aee into sonic-net:master Apr 11, 2024
@vivekverma-arista vivekverma-arista deleted the fix-qos-sai-peer-ipv6 branch April 12, 2024 04:14
@vivekverma-arista
Copy link
Copy Markdown
Contributor Author

vivekverma-arista commented Apr 12, 2024

Hi, @vmittal-msft can we backport this to 202311 as well, as the issue exists in that branch as well.

#10941, the PR that introduced regression has backport requests to 202205, 202311 and 202305 out of which only 202311 PR has merged till now.

@mssonicbld
Copy link
Copy Markdown
Collaborator

mssonicbld commented Apr 17, 2024

@vivekverma-arista PR conflicts with 202311 branch

@bingwang-ms
Copy link
Copy Markdown
Collaborator

bingwang-ms commented Apr 17, 2024

@vivekverma-arista Can you file another PR for 202311 branch to resolve the conflict?

@vivekverma-arista vivekverma-arista mentioned this pull request Apr 17, 2024
Merged
8 tasks
@vivekverma-arista
Copy link
Copy Markdown
Contributor Author

vivekverma-arista commented Apr 17, 2024

Created PR: #12481 for 202311

@bingwang-ms
Copy link
Copy Markdown
Collaborator

bingwang-ms commented Apr 29, 2024

@vivekverma-arista The issue is not completely fixed. I still see similar error. Can you please help fix?

  File "/var/src/sonic-mgmt_vms1-t0-2700-1_658533f09eb33c02b4ae9893/tests/qos/test_qos_sai.py", line 1337, in testQosSaiDscpQueueMapping
    "src_port_ip": dutConfig["testPorts"]["src_port_ipv6"],
KeyError: 'src_port_ipv6'

This was referenced May 23, 2024
Merged
Closed
yxieca pushed a commit that referenced this pull request May 23, 2024
@vivekverma-arista
Revert PR10941 (#12960)
c582013 
What is the motivation for this PR?
The new test is a poorly written IPV6 variant of testQosSaiDscpQueueMapping. It is expected to fail across all platforms/topolgies because qos/test_qos_sai.py cannot support IPV6 variant of any of it's testcase because IPV6 is disabled on the DUT by qos_sai_base.py: https://github.com/sonic-net/sonic-mgmt/blob/master/tests/qos/qos_sai_base.py#L1811-L1826 Also the changes made for this test touch class scoped fixtures which causes all the other testcases to error out as well.

How did you do it?
Revert #10941 as well as the following fixes that were made to get around the issue

Fix qos/test_qos_sai.py #12334
Skip IPV6 variant of testQosSaiDscpQueueMapping if IPV6 is not config… #12834

How did you verify/test it?
Verfied on T0, T1 and T0-dualTor, #126 was not seen.
@vivekverma-arista vivekverma-arista mentioned this pull request May 24, 2024
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vmittal-msft vmittal-msft vmittal-msft approved these changes

@bingwang-ms bingwang-ms bingwang-ms approved these changes

Assignees

No one assigned

Labels

Approved for 202311 branch Cherry Pick Conflict_202311 Request for 202311 branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vivekverma-arista @mssonicbld @vmittal-msft @bingwang-ms @yxieca