Add IPv6 TC to queue mapping test by bktsim-arista · Pull Request #10941 · sonic-net/sonic-mgmt

bktsim-arista · 2023-12-04T22:23:24Z

Summary:
This change introduces an IPv6 test for TC to queue mapping in the QoS suite alongside the infrastructure needed to construct this test. Resolves #10939

Type of change

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

Back port request

201911
202012
202205
202305

Approach

What is the motivation for this PR?

There is a missing test gap for IPv6 DSCP to TC queue mapping. This change introduces a test to fill this test gap by modifying an existing test DscpToQueueMapping which previously tested DSCP to TC queue mapping on IPv4 to also test it for IPv6.

How did you do it?

Modified existing testQosSaiDscpQueueMapping to also test for IPv6, and added changes to active_ip_interfaces to also fetch IPv6 addresses to be used in this test.

How did you verify/test it?

Ran the test manually and it passed.

Any platform specific information?

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

Documentation

mssonicbld · 2023-12-04T22:24:32Z

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:771:121: E501 line too long (127 > 120 characters) tests/qos/qos_sai_base.py:876:121: E501 line too long (132 > 120 characters) tests/qos/qos_sai_base.py:991:121: E501 line too long (123 > 120 characters) tests/saitests/py3/sai_qos_tests.py:366:28: E128 continuation line under-indented for visual indent tests/saitests/py3/sai_qos_tests.py:369:29: E128 continuation line under-indented for visual indent 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:

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>

mssonicbld · 2023-12-04T22:31:14Z

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.................................................Failed - hook id: trailing-whitespace - exit code: 1 - files were modified by this hook Fixing tests/qos/qos_sai_base.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/qos/qos_sai_base.py:877:121: E501 line too long (132 > 120 characters) 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:

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>

mssonicbld · 2023-12-04T22:35:07Z

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.................................................Failed - hook id: trailing-whitespace - exit code: 1 - files were modified by this hook Fixing tests/qos/qos_sai_base.py fix end of files.........................................................Passed check yaml...........................................(no files to check)Skipped check for added large files..............................................Passed check python ast.........................................................Passed flake8...................................................................Passed 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:

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>

mssonicbld · 2023-12-04T22:44:33Z

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.................................................Failed - hook id: trailing-whitespace - exit code: 1 - files were modified by this hook Fixing tests/qos/qos_sai_base.py fix end of files.........................................................Passed check yaml...........................................(no files to check)Skipped check for added large files..............................................Passed check python ast.........................................................Passed flake8...................................................................Passed 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:

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>

rlhui · 2024-01-24T18:55:01Z

@vmittal-msft will review

abdosi · 2024-02-09T18:39:16Z

@alpeshspatel / @rraghav-cisco for viz

judyjoseph · 2024-02-23T02:49:24Z

@saksarav-nokia to review

mssonicbld · 2024-03-22T05:55:27Z

@bktsim-arista PR conflicts with 202205 branch

This change introduces an IPv6 test for TC to queue mapping in the QoS suite alongside the infrastructure needed to construct this test. Resolves sonic-net#10939

mssonicbld · 2024-03-22T05:55:28Z

@bktsim-arista PR conflicts with 202305 branch

mssonicbld · 2024-03-22T05:55:30Z

Cherry-pick PR to 202311: #12088

This change introduces an IPv6 test for TC to queue mapping in the QoS suite alongside the infrastructure needed to construct this test. Resolves #10939

bingwang-ms · 2024-04-05T23:27:08Z

@bktsim-arista The change caused some regression for other test cases in test_qos_sai. Can you please fix it or revert this change?

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'

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 · 2024-04-08T08:17:59Z

@bktsim-arista The change caused some regression for other test cases in test_qos_sai. Can you please fix it or revert this change?

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'

Hi @bingwang-ms, this was fixed in #12334.

* Regression introduced by: #10941 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.

rraghav-cisco · 2024-05-17T17:33:10Z

@vivekverma-arista :
The qos-sai explicitly disables IPv6 as shown by below code in qos/qos_sai_base.py:

1738     @pytest.fixture(scope='class', autouse=True)
1739     def dut_disable_ipv6(self, duthosts, get_src_dst_asic_and_duts, tbinfo, lower_tor_host, # noqa F811
1740                          swapSyncd_on_selected_duts):
1741         for duthost in get_src_dst_asic_and_duts['all_duts']:
1742             docker0_ipv6_addr = \
1743                 duthost.shell("sudo ip -6  addr show dev docker0 | grep global" + " | awk '{print $2}'")[
1744                     "stdout_lines"][0]
1745             duthost.shell("sysctl -w net.ipv6.conf.all.disable_ipv6=1")
1746 
1747         yield
1748 
1749         for duthost in get_src_dst_asic_and_duts['all_duts']:
1750             duthost.shell("sysctl -w net.ipv6.conf.all.disable_ipv6=0")
1751             logger.info("Adding docker0's IPv6 address since it was removed when disabing IPv6")
1752             duthost.shell("ip -6 addr add {} dev docker0".format(docker0_ipv6_addr))
1753             config_reload(duthost, config_source='config_db', safe_reload=True, check_intf_up_ports=True)

So even if minigraph has ipv6 addresses, the new test will fail. Can you pls attach a passing log for this ?

vivekverma-arista · 2024-05-17T18:30:10Z

@vivekverma-arista : The qos-sai explicitly disables IPv6 as shown by below code in qos/qos_sai_base.py:

1738     @pytest.fixture(scope='class', autouse=True)
1739     def dut_disable_ipv6(self, duthosts, get_src_dst_asic_and_duts, tbinfo, lower_tor_host, # noqa F811
1740                          swapSyncd_on_selected_duts):
1741         for duthost in get_src_dst_asic_and_duts['all_duts']:
1742             docker0_ipv6_addr = \
1743                 duthost.shell("sudo ip -6  addr show dev docker0 | grep global" + " | awk '{print $2}'")[
1744                     "stdout_lines"][0]
1745             duthost.shell("sysctl -w net.ipv6.conf.all.disable_ipv6=1")
1746 
1747         yield
1748 
1749         for duthost in get_src_dst_asic_and_duts['all_duts']:
1750             duthost.shell("sysctl -w net.ipv6.conf.all.disable_ipv6=0")
1751             logger.info("Adding docker0's IPv6 address since it was removed when disabing IPv6")
1752             duthost.shell("ip -6 addr add {} dev docker0".format(docker0_ipv6_addr))
1753             config_reload(duthost, config_source='config_db', safe_reload=True, check_intf_up_ports=True)

So even if minigraph has ipv6 addresses, the new test will fail. Can you pls attach a passing log for this ?

Hi @rraghav-cisco, we are skipping the test in master and 202311 #12834

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.

* Revert PR10941 from master * Removed redundant statment and fixed other pylint errors

sonic-net#12980) * Revert PR10941 from master * Removed redundant statment and fixed other pylint errors

bktsim-arista force-pushed the ipv6-test branch from 1ad5345 to a45bef1 Compare December 4, 2023 22:34

bktsim-arista force-pushed the ipv6-test branch from a45bef1 to fdf3fd8 Compare December 4, 2023 22:43

bktsim-arista force-pushed the ipv6-test branch from fdf3fd8 to a11739f Compare December 4, 2023 22:50

judyjoseph requested review from abdosi and vmittal-msft December 14, 2023 21:37

judyjoseph added Request for 202205 branch Request for 202305 branch labels Dec 14, 2023

Add IPv6 DSCP to TC queue mapping test.

d089312

bktsim-arista force-pushed the ipv6-test branch from a11739f to d089312 Compare December 19, 2023 17:02

bktsim-arista marked this pull request as ready for review December 19, 2023 17:05

bktsim-arista requested review from wangxin and yxieca as code owners December 19, 2023 17:05

rlhui assigned bktsim-arista Jan 11, 2024

abdosi approved these changes Mar 7, 2024

View reviewed changes

abdosi added the Request for 202311 branch label Mar 7, 2024

abdosi merged commit 9cc78f9 into sonic-net:master Mar 7, 2024

wangxin added Approved for 202205 branch Approved for 202305 branch Approved for 202311 branch labels Mar 22, 2024

mssonicbld added the Cherry Pick Conflict_202205 label Mar 22, 2024

mssonicbld added the Cherry Pick Conflict_202305 label Mar 22, 2024

mssonicbld added the Created PR to 202311 branch label Mar 22, 2024

mssonicbld mentioned this pull request Mar 22, 2024

[action] [PR:10941] Add IPv6 TC to queue mapping test #12088

Merged

7 tasks

mssonicbld pushed a commit that referenced this pull request Mar 22, 2024

Add IPv6 DSCP to TC queue mapping test. (#10941)

ca286cf

This change introduces an IPv6 test for TC to queue mapping in the QoS suite alongside the infrastructure needed to construct this test. Resolves #10939

mssonicbld added Included in 202311 branch and removed Created PR to 202311 branch labels Mar 22, 2024

vivekverma-arista mentioned this pull request Apr 12, 2024

Fix qos/test_qos_sai.py #12334

Merged

8 tasks

This was referenced May 23, 2024

Revert "[action] [PR:10941] Add IPv6 TC to queue mapping test #12088" #12960

Merged

Revert PR10941 from master #12961

Closed

vivekverma-arista mentioned this pull request May 24, 2024

Revert "Add IPv6 TC to queue mapping test #10941" from master #12980

Merged

8 tasks

yxieca pushed a commit that referenced this pull request May 28, 2024

Revert "Add IPv6 TC to queue mapping test #10941" from master (#12980)

56531ac

* Revert PR10941 from master * Removed redundant statment and fixed other pylint errors

mrkcmo pushed a commit to Azarack/sonic-mgmt that referenced this pull request Jul 17, 2024

Revert "Add IPv6 TC to queue mapping test sonic-net#10941" from master (

4924e59

sonic-net#12980) * Revert PR10941 from master * Removed redundant statment and fixed other pylint errors

Conversation

bktsim-arista commented Dec 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

mssonicbld commented Dec 4, 2023

Uh oh!

mssonicbld commented Dec 4, 2023

Uh oh!

mssonicbld commented Dec 4, 2023

Uh oh!

mssonicbld commented Dec 4, 2023

Uh oh!

rlhui commented Jan 24, 2024

Uh oh!

abdosi commented Feb 9, 2024

Uh oh!

judyjoseph commented Feb 23, 2024

Uh oh!

mssonicbld commented Mar 22, 2024

Uh oh!

mssonicbld commented Mar 22, 2024

Uh oh!

mssonicbld commented Mar 22, 2024

Uh oh!

bingwang-ms commented Apr 5, 2024

Uh oh!

vivekverma-arista commented Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rraghav-cisco commented May 17, 2024

Uh oh!

vivekverma-arista commented May 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

bktsim-arista commented Dec 4, 2023 •

edited

Loading

vivekverma-arista commented Apr 8, 2024 •

edited

Loading