[acl][L3V4V6] PTF test for ACL table type L3V4V6 #9565
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
|
The pre-commit check detected issues in the files touched by this pull request. Detailed pre-commit check results: To run the pre-commit checks locally, you can follow below steps:
|
rck-innovium
force-pushed
the
acl_l3v4v6
branch
from
181102c to
13a8cf9
Compare
|
The pre-commit check detected issues in the files touched by this pull request. Detailed pre-commit check results: To run the pre-commit checks locally, you can follow below steps:
|
rck-innovium
force-pushed
the
acl_l3v4v6
branch
from
13a8cf9 to
bb11a86
Compare
|
The pre-commit check detected issues in the files touched by this pull request. Detailed pre-commit check results: To run the pre-commit checks locally, you can follow below steps:
|
rck-innovium
force-pushed
the
acl_l3v4v6
branch
2 times, most recently
from
67f87e0 to
fa62322
Compare
|
The pre-commit check detected issues in the files touched by this pull request. Detailed pre-commit check results: To run the pre-commit checks locally, you can follow below steps:
|
rck-innovium
force-pushed
the
acl_l3v4v6
branch
from
fa62322 to
90a72d6
Compare
|
Unrelated test failure. FAILED bgp/test_bgp_bbr.py::test_bbr_enabled_dut_asn_in_aspath - Failed: Chec... !!!!!!!!!!!!!!!!!!!!!!!!!! stopping after 1 failures !!!!!!!!!!!!!!!!!!!!!!!!!!! =================== 1 failed, 1 warning in 127.00s (0:02:06) =================== > pytest_assert(not failed_results, 'Checking route {} failed, failed_results: {}' .format(str(route), json.dumps(failed_results, indent=2))) E Failed: Checking route Route(prefix='172.16.10.0/24', nexthop='10.0.0.32', aspath='65100 64101') failed, failed_results: { |
rck-innovium
force-pushed
the
acl_l3v4v6
branch
from
90a72d6 to
f7a2491
Compare
rck-innovium
force-pushed
the
acl_l3v4v6
branch
from
f7a2491 to
3b70eb9
Compare
Unrelated failures:The two failed tests are due to flakiness in the test infra and are NOT related to the code changes. I have added the details below. @bingwang-ms Please help review these tests and also rerun /azp run Azure.sonic-mgmt. Test kvmtest-t0 by Elastictest
route/test_duplicate_route.py::test_duplicate_routes[4-Loopback-vlab-01-None] -------------------------------- live log setup -------------------------------- 14:26:58 __init__._fixture_generator_decorator L0088 ERROR | TypeError("generate_intf_neigh() missing 2 required positional arguments: 'mg_facts' and 'is_backend_topology'") Traceback (most recent call last): File "/var/src/sonic-mgmt/tests/common/plugins/log_section_start/__init__.py", line 84, in _fixture_generator_decorator res = next(it) File "/var/src/sonic-mgmt/tests/route/test_duplicate_route.py", line 123, in setup_routes intf_neighs, str_intf_nexthop = generate_intf_neigh( TypeError: generate_intf_neigh() missing 2 required positional arguments: 'mg_facts' and 'is_backend_topology' ERROR [ 25%] route/test_duplicate_route.py::test_duplicate_routes[4-Loopback-vlab-01-None] ERROR [ 25%]Wednesday 17 April 2024 14:27:03 +0000 (0:00:06.812) 0:00:54.336 ******* |
1 similar comment
Unrelated failures:The two failed tests are due to flakiness in the test infra and are NOT related to the code changes. I have added the details below. @bingwang-ms Please help review these tests and also rerun /azp run Azure.sonic-mgmt. Test kvmtest-t0 by Elastictest
route/test_duplicate_route.py::test_duplicate_routes[4-Loopback-vlab-01-None] -------------------------------- live log setup -------------------------------- 14:26:58 __init__._fixture_generator_decorator L0088 ERROR | TypeError("generate_intf_neigh() missing 2 required positional arguments: 'mg_facts' and 'is_backend_topology'") Traceback (most recent call last): File "/var/src/sonic-mgmt/tests/common/plugins/log_section_start/__init__.py", line 84, in _fixture_generator_decorator res = next(it) File "/var/src/sonic-mgmt/tests/route/test_duplicate_route.py", line 123, in setup_routes intf_neighs, str_intf_nexthop = generate_intf_neigh( TypeError: generate_intf_neigh() missing 2 required positional arguments: 'mg_facts' and 'is_backend_topology' ERROR [ 25%] route/test_duplicate_route.py::test_duplicate_routes[4-Loopback-vlab-01-None] ERROR [ 25%]Wednesday 17 April 2024 14:27:03 +0000 (0:00:06.812) 0:00:54.336 ******* |
|
/azp run Azure.sonic-mgmt |
1 similar comment
|
/azp run Azure.sonic-mgmt |
|
Commenter does not have sufficient privileges for PR 9565 in repo sonic-net/sonic-mgmt |
1 similar comment
|
Commenter does not have sufficient privileges for PR 9565 in repo sonic-net/sonic-mgmt |
|
Hi @rck-innovium Sorry I missed this review. Can you please let me know if the PR in sonic-swss has been merged? And can you please paste the test result into the PR, including both T0 and T1 topology? |
The code PRs have been merged in 202305 release. T0 Results:
T1 Results:
|
|
The pre-commit check detected issues in the files touched by this pull request. Detailed pre-commit check results: To run the pre-commit checks locally, you can follow below steps:
|
1 similar comment
|
The pre-commit check detected issues in the files touched by this pull request. Detailed pre-commit check results: To run the pre-commit checks locally, you can follow below steps:
|
|
The pre-commit check detected issues in the files touched by this pull request. Detailed pre-commit check results: To run the pre-commit checks locally, you can follow below steps:
|
|
Dual-tor tests are unstable (inconsistent mux status). The new scripts are not even enabled for Dual-TOR! Please see the details below and help suggest the way forward. test_pretest|||vms-kvm-dual-t0_117804 E Failed: !!!!!!!!!!!!!!!! Pre-test sanity check after recovery failed: !!!!!!!!!!!!!!!! E [ E { E "failed": true, E "failed_reason": "Inconsistent mux status for active-standby ports on dualtors, please check output of \"show mux status\"", E "check_item": "mux_simulator", E "action": "<not serializable>", E "hosts": [ E "vlab-05", E "vlab-06" E ] E } E ] |
|
/azpw run Azure.sonic-mgmt |
|
/AzurePipelines run Azure.sonic-mgmt |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ACL test-cases based on test plan for L3V4V6 type ACL table Refer Test Plan: sonic-net#8178 A new script test_acl_l3v4v6.py is added based on existing test_acl.py. This is to help parallel execution. Major changes from test_acl.py are: [1] Skip unsupported platforms based on STATE_DB capability checks [2] Add templates that have 'ethertype' to distinguish b/w v4 and v6 rules [3] Remove unsupported matches like L4 port ACL ranges Signed-off-by: Ravi Marvell [email protected]
rck-innovium
force-pushed
the
acl_l3v4v6
branch
from
f1a7f0f to
ba2bc7e
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@bingwang-ms Can you please review. |
bingwang-ms
reviewed
Feb 10, 2025
| "config acl remove table {}".format(TABLE_NAME), | ||
| "config save -y" | ||
| ] | ||
| duthost.shell_cmds(cmds=cmds) |
There was a problem hiding this comment.
Is it necessary to remove DATAACL to free TCAM?
bingwang-ms
reviewed
Feb 10, 2025
| downstream_ports_per_dut[namespace].append(interface) | ||
| downstream_port_ids.append(port_id) | ||
| downstream_port_id_to_router_mac_map[port_id] = router_mac | ||
| elif "T3" in neighbor["name"]: |
There was a problem hiding this comment.
Since the test is expected to run on T0 and T1, this check is probably not needed.
bingwang-ms
reviewed
Feb 10, 2025
| # Need to refresh below constants for two scenarios of M0 | ||
| global DOWNSTREAM_DST_IP, DOWNSTREAM_IP_TO_ALLOW, DOWNSTREAM_IP_TO_BLOCK | ||
|
|
||
| if topo == "mx": |
There was a problem hiding this comment.
If the test is limited to t0 and t1, do we still need to check mx and m0 ?
bingwang-ms
reviewed
Feb 10, 2025
| # For T1/M0_L3 scenario, no VLANs are present so using the router MAC is acceptable | ||
| downlink_dst_mac = vlan_mac if vlan_mac is not None else rand_selected_dut.facts["router_mac"] | ||
|
|
||
| if topo == "t2": |
There was a problem hiding this comment.
Same here. It seems there is some unnecessary topo check. Can you please do a cleanup?
bingwang-ms
reviewed
Feb 10, 2025
|
|
||
| @pytest.fixture(scope="module") | ||
| def acl_table(duthosts, rand_one_dut_hostname, setup, stage, ip_version, tbinfo): | ||
| """Apply ACL table configuration and remove after tests. |
There was a problem hiding this comment.
If the ACL table is L3V4V6, why do you need to distinguish ip_version here?
bingwang-ms
reviewed
Feb 10, 2025
| loganalyzer.expect_regex = [LOG_EXPECT_ACL_TABLE_CREATE_RE] | ||
| # Ignore any other errors to reduce noise | ||
| loganalyzer.ignore_regex = [r".*"] | ||
| with loganalyzer: |
There was a problem hiding this comment.
You may not need to check syslog. There is status for each ACL table. The table is created successfully if the status is active.
bingwang-ms
reviewed
Feb 10, 2025
| # Ignore any other errors to reduce noise | ||
| loganalyzer.ignore_regex = [r".*"] | ||
| with loganalyzer: | ||
| self.setup_rules(duthost, acl_table, ip_version) |
There was a problem hiding this comment.
Same here. In new version (202305 and above), we can check the status of ACL rule to determine if the rule is created successfully.
bingwang-ms
reviewed
Feb 10, 2025
| continue | ||
| counters_after[PACKETS_COUNT] += acl_facts[duthost]['after'][rule][PACKETS_COUNT] | ||
| counters_after[BYTES_COUNT] += acl_facts[duthost]['after'][rule][BYTES_COUNT] | ||
| if (duthost.facts["hwsku"] == "Cisco-8111-O64" or |
There was a problem hiding this comment.
Since this test is limited on Marvell, is this check needed?
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
ACL test-cases based on test plan for L3V4V6 type ACL table
Refer Test Plan: #8178
Signed-off-by: Ravi Marvell [email protected]
Summary:
Fixes # New Feature
Type of change
Back port request
Approach
What is the motivation for this PR?
ACL test-cases based on test plan for L3V4V6 type ACL table
How did you do it?
Based on community feedback, a new script test_acl_l3v4v6.py is added based on existing test_acl.py. This is to help parallel execution and avoid and PTF scale issues when we send a lot of small packet frames.
How did you verify/test it?
Ran the tests on supported platform (Marvell)
Any platform specific information?
Test cases are run/skipped based on platform capability
Supported testbed topology if it's a new test case?
t0, t1
Documentation
https://github.com/sonic-net/sonic-mgmt/blob/master/docs/testplan/Extend_L3V6ACL_test_plan.md