Skip to content

Adjusting ACL test cases for DNX (J2c+) chipset on a T2 VoQ Chassis#6390

Merged
tjchadaga merged 4 commits intosonic-net:masterfrom
sanjair-git:acl
Nov 8, 2022
Merged

Adjusting ACL test cases for DNX (J2c+) chipset on a T2 VoQ Chassis#6390
tjchadaga merged 4 commits intosonic-net:masterfrom
sanjair-git:acl

Conversation

@sanjair-git
Copy link
Copy Markdown
Contributor

@sanjair-git sanjair-git commented Sep 21, 2022

Description of PR

Summary:
Fixes # (issue)
This PR addresses the following:

  • Don't skip egress ACL tests cases,
  • 8 ACL tests fail against DNX chipset due to its limitations. But added a fix to have unique source IPV6 and destination IPV6 fields in the ACL rules used.

Type of change

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

Back port request

  • 201911
  • 202012

Approach

What is the motivation for this PR?

Currently, ACL tests in egress direction are skipped for broadcom based chipsets. However, DNX chipset supports egress and ingress ACL's.
Due to the limitation of DNX chipset with regards to IPv6 src and dst address present in the ACL rule ( BRCM CSP #CS00012257197), 8 tests 'test_icmp_match_forwarded' are throwing errors in counters not incremented for RULE_3.
Hence, we added unique source and destination IPV6 address fields to the ACL rules used for the tests mentioned above.

How did you do it?

Added support for Egress ACL on broadcom-dnx chipset
Added unique 64 bit fields for the destination IPV6 addresses so that source and destination addresses won't match in the ACL rules being used.

How did you verify/test it?

Ran ACL tests against T2 chassis with linecards with J2c+ DNX chipset with PR# #5707 that allows to run ACL tests against a T2 chassis

Any platform specific information?

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

Documentation

@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla bot commented Sep 21, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: sanmalho-git (fedab2ccc0858efa66914a79c65ab1735d264b64, ef4effe0105e5040a53e01e37e4bd7c71ac1b148)
  • ✅ login: sanjair-git (29861b652297482ca121a0c255ef018e0a1c256a)

judyjoseph
judyjoseph previously approved these changes Sep 28, 2022
@judyjoseph
Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@tjchadaga
Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@sanjair-git - Since there is workaround for this limitation (sonic-net/sonic-swss#2178), these tests may not need to be skipped. Could you please check and update the PR?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@tjchadaga - the fix in (sonic-net/sonic-swss#2178), allows us to run any of the egress ACL tests against DNX chipset. However, due to the limitation of DNX chipset with regards to IPv6 src and dst address present in the ACL rule ( BRCM CSP #CS00012257197), 8 tests 'test_icmp_match_forwarded' are throwing errors in counters not incremented for RULE_3. Therefore, we have expect these tests to fail on DNX chipset.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@sanmalho-git - Is it possible to tweak the IP address to avoid this conflict with 64-bits?

@tjchadaga tjchadaga self-assigned this Oct 27, 2022
@azure-pipelines
Copy link
Copy Markdown

The pre-commit check detected issues in the files touched by this pull request.
The detected issues may be old or new. For new issues, please try to fix them.

For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame
author of this pull request. But if you can take extra effort to fix the old issues as well, that would be great!

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/acl/test_acl.py:416:121: E501 line too long (137 > 120 characters)

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>

@sanjair-git
Copy link
Copy Markdown
Contributor Author

Attaching the test run logs for reference,

icmp_32cases_console.txt

acl_icmp_results

wangxin pushed a commit that referenced this pull request Nov 11, 2022
…6390)

* Adding xfail for ACL tests that fail on DNX

* Supporting egress ACL on broadcom-dnx chipset

Added tests that were commented out accidentally

* Chaging comment from skipping to ignoring failure

* Added unique source and destination IPV6 fields for ACL rules. Ref CSP CS00012257197

Co-authored-by: sanmalho <[email protected]>
sanjair-git added a commit to sanjair-git/sonic-mgmt that referenced this pull request Nov 28, 2022
bingwang-ms pushed a commit to bingwang-ms/sonic-mgmt that referenced this pull request Jul 27, 2023
…ic-mgmt into internal-202205

Fix merge conflicts.
- [pre-commit] Fix style issues in test scripts under `tests/acl` folder (sonic-net#6679)
- Moving check for reboot cause after interface status check (sonic-net#6721)
- Adding watchdog timeout values for Cisco 8808 Supervisor and Different LCs (sonic-net#6776)
- add Ether check in macsec_dp_poll (sonic-net#6828)
- Disable PFC watchdog in test_cpu_memory_usage_counterpoll (sonic-net#6851)
- Testcase to verify that lossless traffic is not dropped during congesion. (sonic-net#6853)
- Ignore Broadcom sai sai unbind ERR log for now (sonic-net#6539)
- [chassis][multi-asic] update the loganalyser regex for multi asic (sonic-net#6885)
- [mx] Fix test_acl failed on mx topo (sonic-net#6971) (sonic-net#6983)
- [202205][mx] Add support for mx in test_null_route_helper (sonic-net#6967) (sonic-net#6982)
- [m0][everflow] Add m0 support for everflow and refactor everflow setup_info (sonic-net#6900)
- [ACL] Add acl stress test (sonic-net#6903)
- Enhance test_tor_ecn (sonic-net#6906)
- Fix erros - Added unique IPV6 address for the missed ACL rules PR sonic-net#6390 (sonic-net#6909)
- enabled bfd tests (sonic-net#6919)
- Skip bgp speaker test on backend topo (sonic-net#6922)
- [advanced-reboot] Handle logs in tmpfs: backup two log files before reboot (sonic-net#6923)
- Fix missing definition (sonic-net#6930)
- [Mellanox] Add minimal table definition for SN2201 (sonic-net#6943)
- Update qos test param for dualtor topology (sonic-net#6948)
- fix setup for single asic lc (sonic-net#6951)
- Fix QoS sai test for running with python3 (sonic-net#6961)
- Don't fail if logrotate cron job file isn't present (sonic-net#6964)
- Disable post sanity check for vxlan test (sonic-net#6980)
- Merge branch 'azure-202205' into dev/yaqiangzhu/202205_merge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants