Skip to content

[ACL] Correct rule ID used to check ACL counter in teardown phase.#2124

Closed
shihhsien-wang wants to merge 1 commit intosonic-net:masterfrom
shihhsien-wang:master
Closed

[ACL] Correct rule ID used to check ACL counter in teardown phase.#2124
shihhsien-wang wants to merge 1 commit intosonic-net:masterfrom
shihhsien-wang:master

Conversation

@shihhsien-wang
Copy link
Contributor

- What I did

Correct rule ID used to check ACL counter in teardown phase. Without the modification, counter sanity check in teardown phase will fail, because pytest check wrong ACL rule counter.

- How I did it

According to rules listed in tests/acl/templates/acltb_test_rules.j2, we verify test cases one by one and fix five following test cases:

  • test_dest_ip_match_dropped
  • test_dest_ip_match_forwarded
  • test_l4_dport_match_forwarded
  • test_l4_sport_match_dropped
  • test_tcp_flags_match_dropped

- How to verify it

Before modification:

============================= test session starts ==============================
platform linux2 -- Python 2.7.12, pytest-4.6.7, py-1.8.0, pluggy-0.13.1 -- /usr/bin/python2
cachedir: .pytest_cache
ansible: 2.8.7
rootdir: /var/sonic/sonic-mgmt/tests, inifile: pytest.ini
plugins: ansible-2.2.2, pyfakefs-3.7.2, csv-2.0.2
collecting ... collected 1 item

acl/test_acl.py::TestBasicAcl::test_dest_ip_match_forwarded[ingress-spine->tor] 
-------------------------------- live log setup --------------------------------
...
==================================== ERRORS ====================================
_ ERROR at teardown of TestBasicAcl.test_dest_ip_match_forwarded[ingress-spine->tor] _

self = <test_acl.TestBasicAcl object at 0x7f864e6ead50>
duthost = <common.devices.SonicHost object at 0x7f864dcaba90>, acl_rules = None
acl_table = {'config_file': 'tmp/acl/acl_table_DATAINGRESS.json', 'name': 'DATAINGRESS'}

    @pytest.yield_fixture(scope='class', autouse=True)
    def counters_sanity_check(self, duthost, acl_rules, acl_table):
...
        for rule in rule_list:
            rule = 'RULE_{}'.format(rule)
            counters_after = acl_facts_after_traffic[rule]
            counters_before = acl_facts_before_traffic[rule]
            logger.info('counters for {} before traffic:\n{}'.format(rule, pprint.pformat(counters_before)))
            logger.info('counters for {} after traffic:\n{}'.format(rule, pprint.pformat(counters_after)))
>           assert counters_after['packets_count'] > counters_before['packets_count']
E           assert 0 > 0
...
acl/test_acl.py:349: AssertionError
...
===================== 1 passed, 1 error in 125.54 seconds ======================

After modification:

============================= test session starts ==============================
platform linux2 -- Python 2.7.12, pytest-4.6.7, py-1.8.0, pluggy-0.13.1 -- /usr/bin/python
cachedir: .pytest_cache
ansible: 2.8.7
rootdir: /var/sonic/sonic-mgmt/tests, inifile: pytest.ini
plugins: ansible-2.2.2, csv-2.0.2
collecting ... collected 1 item

acl/test_acl_new.py::TestBasicAcl::test_dest_ip_match_forwarded[ingress-spine->tor] 
-------------------------------- live log call ---------------------------------
WARNING  dataplane:dataplane.py:873 Dataplane poll with exp_pkt but no port number
PASSED                                                                   [100%]

============================ slowest test durations ============================
99.34s setup    acl/test_acl_new.py::TestBasicAcl::test_dest_ip_match_forwarded[ingress-spine->tor]
29.57s teardown acl/test_acl_new.py::TestBasicAcl::test_dest_ip_match_forwarded[ingress-spine->tor]
0.08s call     acl/test_acl_new.py::TestBasicAcl::test_dest_ip_match_forwarded[ingress-spine->tor]
========================== 1 passed in 129.05 seconds ==========================

@wangxin wangxin requested a review from a team August 21, 2020 11:32
@wangxin
Copy link
Collaborator

wangxin commented Aug 26, 2020

@shihhsien-wang Which topology was used in your testing, "t1" or "t1-lag"?
If "t1-lag" was used, the original code should be fine. There is no need to update the ACL test script.
If "t1" was used, ACL testing would not pass because currently "t1" topology is still using static IP routes. But the issue should be fixed in the VM startup config template for "t1" topology. Please refer to #1785

@shihhsien-wang
Copy link
Contributor Author

@shihhsien-wang Which topology was used in your testing, "t1" or "t1-lag"?
If "t1-lag" was used, the original code should be fine. There is no need to update the ACL test script.
If "t1" was used, ACL testing would not pass because currently "t1" topology is still using static IP routes. But the issue should be fixed in the VM startup config template for "t1" topology. Please refer to #1785

The problem occurred in both t1 and t1-lag because it just checks the wrong received counter of the match rules in teardown phase. Thanks.

@wangxin
Copy link
Collaborator

wangxin commented Nov 23, 2020

Are you using the latest code? The ACL testing is passing from our side.

@shihhsien-wang
Copy link
Contributor Author

Are you using the latest code? The ACL testing is passing from our side.

At the time of submitting ticket, Yes.
Now, I check the latest code and found that #2401 update the same code location which is merged before your test. And I think that the bug should be fixed. Thanks for your help. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants