From 71fdee76c9ee3a2e7181571c782033d8508aa440 Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak <38952541+stepanblyschak@users.noreply.github.com> Date: Sun, 20 Feb 2022 16:25:50 +0200 Subject: [PATCH] [aclshow] fix aclshow when clear is called before counters are populated (#2037) - What I did Fixed a bug that in case ACL counters are cleared before counters are populated in COUNTERS DB the next aclshow will fail: admin@sonic:~$ aclshow -c -t DATAACL admin@sonic:~$ aclshow -a -t DATAACL 'NoneType' object is not subscriptable - How I did it Keep self.acl_counters in sync with COUNTERS DB and don't put empty values when no counters are available, so when dumping counters and loading them back there are no empty values. - How to verify it Without this change the added UT fails: Signed-off-by: Stepan Blyschak --- scripts/aclshow | 5 +- tests/aclshow_test.py | 141 +++++++++++++++++++---------- tests/mock_tables/config_db.json | 10 ++ tests/mock_tables/counters_db.json | 3 +- 4 files changed, 108 insertions(+), 51 deletions(-) diff --git a/scripts/aclshow b/scripts/aclshow index 1e7bfe4fcd..db0cc40ddf 100755 --- a/scripts/aclshow +++ b/scripts/aclshow @@ -134,7 +134,6 @@ class AclStat(object): counters_db_separator = self.db.get_db_separator(self.db.COUNTERS_DB) rule_to_counter_map = get_acl_rule_counter_map() for table, rule in self.acl_rules: - self.acl_counters[table, rule] = {} rule_identifier = table + counters_db_separator + rule if not rule_to_counter_map: continue @@ -143,6 +142,8 @@ class AclStat(object): continue counters_db_key = COUNTERS + counters_db_separator + counter_oid cnt_props = self.db.get_all(self.db.COUNTERS_DB, counters_db_key) + if not cnt_props: + continue self.acl_counters[table, rule] = cnt_props if verboseflag: @@ -158,7 +159,7 @@ class AclStat(object): """ return the counter value or the difference comparing with the saved value in string format """ - if not self.acl_counters[key]: + if key not in self.acl_counters: return 'N/A' if key in self.saved_acl_counters: diff --git a/tests/aclshow_test.py b/tests/aclshow_test.py index 9529be3689..d0a92174f4 100644 --- a/tests/aclshow_test.py +++ b/tests/aclshow_test.py @@ -34,28 +34,29 @@ """ # Expected output for aclshow -a -all_output = """\ -RULE NAME TABLE NAME PRIO PACKETS COUNT BYTES COUNT -------------------------------------- ------------- ------ --------------- ------------- -RULE_1 DATAACL 9999 101 100 -RULE_2 DATAACL 9998 201 200 -RULE_3 DATAACL 9997 301 300 -RULE_4 DATAACL 9996 401 400 -RULE_05 DATAACL 9995 0 0 -RULE_7 DATAACL 9993 701 700 -RULE_9 DATAACL 9991 901 900 -RULE_10 DATAACL 9989 1001 1000 -DEFAULT_RULE DATAACL 1 2 1 -RULE_6 EVERFLOW 9994 601 600 -RULE_08 EVERFLOW 9992 0 0 -RULE_1 NULL_ROUTE_V4 9999 N/A N/A -BLOCK_RULE_10.0.0.2/32 NULL_ROUTE_V4 9999 N/A N/A -BLOCK_RULE_10.0.0.3/32 NULL_ROUTE_V4 9999 N/A N/A -DEFAULT_RULE NULL_ROUTE_V4 1 N/A N/A -RULE_1 NULL_ROUTE_V6 9999 N/A N/A -BLOCK_RULE_1000:1000:1000:1000::2/128 NULL_ROUTE_V6 9999 N/A N/A -BLOCK_RULE_1000:1000:1000:1000::3/128 NULL_ROUTE_V6 9999 N/A N/A -DEFAULT_RULE NULL_ROUTE_V6 1 N/A N/A +all_output = '' + \ +"""RULE NAME TABLE NAME PRIO PACKETS COUNT BYTES COUNT +------------------------------------- ------------------ ------ --------------- ------------- +RULE_1 DATAACL 9999 101 100 +RULE_2 DATAACL 9998 201 200 +RULE_3 DATAACL 9997 301 300 +RULE_4 DATAACL 9996 401 400 +RULE_05 DATAACL 9995 0 0 +RULE_7 DATAACL 9993 701 700 +RULE_9 DATAACL 9991 901 900 +RULE_10 DATAACL 9989 1001 1000 +DEFAULT_RULE DATAACL 1 2 1 +RULE_NO_COUNTER DATAACL_NO_COUNTER 9995 N/A N/A +RULE_6 EVERFLOW 9994 601 600 +RULE_08 EVERFLOW 9992 0 0 +RULE_1 NULL_ROUTE_V4 9999 N/A N/A +BLOCK_RULE_10.0.0.2/32 NULL_ROUTE_V4 9999 N/A N/A +BLOCK_RULE_10.0.0.3/32 NULL_ROUTE_V4 9999 N/A N/A +DEFAULT_RULE NULL_ROUTE_V4 1 N/A N/A +RULE_1 NULL_ROUTE_V6 9999 N/A N/A +BLOCK_RULE_1000:1000:1000:1000::2/128 NULL_ROUTE_V6 9999 N/A N/A +BLOCK_RULE_1000:1000:1000:1000::3/128 NULL_ROUTE_V6 9999 N/A N/A +DEFAULT_RULE NULL_ROUTE_V6 1 N/A N/A """ # Expected output for aclshow -r RULE_1 -t DATAACL @@ -86,10 +87,10 @@ """ # Expected output for aclshow -r RULE_4,RULE_6 -vv -rule4_rule6_verbose_output = """\ -Reading ACL info... -Total number of ACL Tables: 10 -Total number of ACL Rules: 19 +rule4_rule6_verbose_output = '' + \ +"""Reading ACL info... +Total number of ACL Tables: 11 +Total number of ACL Rules: 20 RULE NAME TABLE NAME PRIO PACKETS COUNT BYTES COUNT ----------- ------------ ------ --------------- ------------- @@ -123,28 +124,54 @@ # Expected output for # aclshow -a -c ; aclshow -a -all_after_clear_output = """\ -RULE NAME TABLE NAME PRIO PACKETS COUNT BYTES COUNT -------------------------------------- ------------- ------ --------------- ------------- -RULE_1 DATAACL 9999 0 0 -RULE_2 DATAACL 9998 0 0 -RULE_3 DATAACL 9997 0 0 -RULE_4 DATAACL 9996 0 0 -RULE_05 DATAACL 9995 0 0 -RULE_7 DATAACL 9993 0 0 -RULE_9 DATAACL 9991 0 0 -RULE_10 DATAACL 9989 0 0 -DEFAULT_RULE DATAACL 1 0 0 -RULE_6 EVERFLOW 9994 0 0 -RULE_08 EVERFLOW 9992 0 0 -RULE_1 NULL_ROUTE_V4 9999 N/A N/A -BLOCK_RULE_10.0.0.2/32 NULL_ROUTE_V4 9999 N/A N/A -BLOCK_RULE_10.0.0.3/32 NULL_ROUTE_V4 9999 N/A N/A -DEFAULT_RULE NULL_ROUTE_V4 1 N/A N/A -RULE_1 NULL_ROUTE_V6 9999 N/A N/A -BLOCK_RULE_1000:1000:1000:1000::2/128 NULL_ROUTE_V6 9999 N/A N/A -BLOCK_RULE_1000:1000:1000:1000::3/128 NULL_ROUTE_V6 9999 N/A N/A -DEFAULT_RULE NULL_ROUTE_V6 1 N/A N/A +all_after_clear_output = '' + \ +"""RULE NAME TABLE NAME PRIO PACKETS COUNT BYTES COUNT +------------------------------------- ------------------ ------ --------------- ------------- +RULE_1 DATAACL 9999 0 0 +RULE_2 DATAACL 9998 0 0 +RULE_3 DATAACL 9997 0 0 +RULE_4 DATAACL 9996 0 0 +RULE_05 DATAACL 9995 0 0 +RULE_7 DATAACL 9993 0 0 +RULE_9 DATAACL 9991 0 0 +RULE_10 DATAACL 9989 0 0 +DEFAULT_RULE DATAACL 1 0 0 +RULE_NO_COUNTER DATAACL_NO_COUNTER 9995 N/A N/A +RULE_6 EVERFLOW 9994 0 0 +RULE_08 EVERFLOW 9992 0 0 +RULE_1 NULL_ROUTE_V4 9999 N/A N/A +BLOCK_RULE_10.0.0.2/32 NULL_ROUTE_V4 9999 N/A N/A +BLOCK_RULE_10.0.0.3/32 NULL_ROUTE_V4 9999 N/A N/A +DEFAULT_RULE NULL_ROUTE_V4 1 N/A N/A +RULE_1 NULL_ROUTE_V6 9999 N/A N/A +BLOCK_RULE_1000:1000:1000:1000::2/128 NULL_ROUTE_V6 9999 N/A N/A +BLOCK_RULE_1000:1000:1000:1000::3/128 NULL_ROUTE_V6 9999 N/A N/A +DEFAULT_RULE NULL_ROUTE_V6 1 N/A N/A +""" + +all_after_clear_and_populate_output = '' + \ +"""RULE NAME TABLE NAME PRIO PACKETS COUNT BYTES COUNT +------------------------------------- ------------------ ------ --------------- ------------- +RULE_1 DATAACL 9999 0 0 +RULE_2 DATAACL 9998 0 0 +RULE_3 DATAACL 9997 0 0 +RULE_4 DATAACL 9996 0 0 +RULE_05 DATAACL 9995 0 0 +RULE_7 DATAACL 9993 0 0 +RULE_9 DATAACL 9991 0 0 +RULE_10 DATAACL 9989 0 0 +DEFAULT_RULE DATAACL 1 0 0 +RULE_NO_COUNTER DATAACL_NO_COUNTER 9995 100 100 +RULE_6 EVERFLOW 9994 0 0 +RULE_08 EVERFLOW 9992 0 0 +RULE_1 NULL_ROUTE_V4 9999 N/A N/A +BLOCK_RULE_10.0.0.2/32 NULL_ROUTE_V4 9999 N/A N/A +BLOCK_RULE_10.0.0.3/32 NULL_ROUTE_V4 9999 N/A N/A +DEFAULT_RULE NULL_ROUTE_V4 1 N/A N/A +RULE_1 NULL_ROUTE_V6 9999 N/A N/A +BLOCK_RULE_1000:1000:1000:1000::2/128 NULL_ROUTE_V6 9999 N/A N/A +BLOCK_RULE_1000:1000:1000:1000::3/128 NULL_ROUTE_V6 9999 N/A N/A +DEFAULT_RULE NULL_ROUTE_V6 1 N/A N/A """ @@ -271,3 +298,21 @@ def test_all_after_clear(): nullify_on_start, nullify_on_exit = False, True test = Aclshow(nullify_on_start, nullify_on_exit, all=True, clear=False, rules=None, tables=None, verbose=None) assert test.result.getvalue() == all_after_clear_output + + +def test_clear_and_populate_counters_db(): + # No counters yet for DATAACL_NO_COUNTER:RULE_NO_COUNTER + nullify_on_start, nullify_on_exit = True, False + test = Aclshow(nullify_on_start, nullify_on_exit, all=True, clear=True, rules=None, tables=None, verbose=None) + assert test.result.getvalue() == clear_output + nullify_on_start, nullify_on_exit = False, True + + # Counters populated. + conn = dbconnector.SonicV2Connector() + conn.connect(conn.COUNTERS_DB) + conn.set(conn.COUNTERS_DB, aclshow.COUNTERS + ':oid:0x900000000000b', aclshow.COUNTER_PACKETS_ATTR, '100') + conn.set(conn.COUNTERS_DB, aclshow.COUNTERS + ':oid:0x900000000000b', aclshow.COUNTER_BYTES_ATTR, '100') + + with mock.patch('aclshow.SonicV2Connector', return_value=conn): + test = Aclshow(nullify_on_start, nullify_on_exit, all=True, clear=False, rules=None, tables=None, verbose=None) + assert test.result.getvalue() == all_after_clear_and_populate_output diff --git a/tests/mock_tables/config_db.json b/tests/mock_tables/config_db.json index 5c26ad4f73..780005a405 100644 --- a/tests/mock_tables/config_db.json +++ b/tests/mock_tables/config_db.json @@ -469,6 +469,11 @@ "priority": "9989", "SRC_IP": "10.0.0.3/32" }, + "ACL_RULE|DATAACL_NO_COUNTER|RULE_NO_COUNTER": { + "IP_PROTOCOL": "126", + "PACKET_ACTION": "FORWARD", + "PRIORITY": "9995" + }, "ACL_TABLE|NULL_ROUTE_V4": { "policy_desc": "DATAACL", "ports@": "PortChannel0002,PortChannel0005,PortChannel0008,PortChannel0011,PortChannel0014,PortChannel0017,PortChannel0020,PortChannel0023", @@ -484,6 +489,11 @@ "ports@": "PortChannel0002,PortChannel0005,PortChannel0008,PortChannel0011,PortChannel0014,PortChannel0017,PortChannel0020,PortChannel0023,Ethernet64,Ethernet68,Ethernet72,Ethernet76,Ethernet80,Ethernet84,Ethernet88,Ethernet92,Ethernet96,Ethernet100,Ethernet104,Ethernet108,Ethernet112,Ethernet116,Ethernet120,Ethernet124", "type": "L3" }, + "ACL_TABLE|DATAACL_NO_COUNTER": { + "policy_desc": "DATAACL", + "ports@": "PortChannel0002,PortChannel0005,PortChannel0008,PortChannel0011,PortChannel0014,PortChannel0017,PortChannel0020,PortChannel0023,Ethernet64,Ethernet68,Ethernet72,Ethernet76,Ethernet80,Ethernet84,Ethernet88,Ethernet92,Ethernet96,Ethernet100,Ethernet104,Ethernet108,Ethernet112,Ethernet116,Ethernet120,Ethernet124", + "type": "L3" + }, "ACL_TABLE|DATAACL_2": { "policy_desc": "DATAACL_2", "ports@": "PortChannel0002,PortChannel0005,PortChannel0008,PortChannel0011,PortChannel0014,PortChannel0017,PortChannel0020,PortChannel0023,Ethernet64,Ethernet68,Ethernet72,Ethernet76,Ethernet80,Ethernet84,Ethernet88,Ethernet92,Ethernet96,Ethernet100,Ethernet104,Ethernet108,Ethernet112,Ethernet116,Ethernet120,Ethernet124", diff --git a/tests/mock_tables/counters_db.json b/tests/mock_tables/counters_db.json index 186d07b646..931e001220 100644 --- a/tests/mock_tables/counters_db.json +++ b/tests/mock_tables/counters_db.json @@ -597,7 +597,8 @@ "DATAACL:RULE_7": "oid:0x9000000000007", "EVERFLOW:RULE_08": "oid:0x9000000000008", "DATAACL:RULE_9": "oid:0x9000000000009", - "DATAACL:RULE_10": "oid:0x900000000000a" + "DATAACL:RULE_10": "oid:0x900000000000a", + "DATAACL_NO_COUNTER:RULE_NO_COUNTER": "oid:0x900000000000b" }, "COUNTERS_TUNNEL_NAME_MAP": { "vtep1": "oid:0x2a0000000035e"