Skip to content

Commit

Permalink
[aclshow] fix aclshow when clear is called before counters are popula…
Browse files Browse the repository at this point in the history
…ted (#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 <[email protected]>
  • Loading branch information
stepanblyschak authored and judyjoseph committed Feb 22, 2022
1 parent a48a027 commit 71fdee7
Show file tree
Hide file tree
Showing 4 changed files with 108 additions and 51 deletions.
5 changes: 3 additions & 2 deletions scripts/aclshow
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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:
Expand All @@ -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:
Expand Down
141 changes: 93 additions & 48 deletions tests/aclshow_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
----------- ------------ ------ --------------- -------------
Expand Down Expand Up @@ -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
"""


Expand Down Expand Up @@ -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
10 changes: 10 additions & 0 deletions tests/mock_tables/config_db.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand Down
3 changes: 2 additions & 1 deletion tests/mock_tables/counters_db.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down

0 comments on commit 71fdee7

Please sign in to comment.