[drop counters] Fix configuration for counters with lowercase names (#…

…1103) Signed-off-by: Danny Allen <daall@microsoft.com>
sonic-net · Sep 10, 2020 · 144bccb · 144bccb
1 parent 8d802d4
commit 144bccb
Show file tree

Hide file tree

Showing 4 changed files with 36 additions and 29 deletions.
diff --git a/scripts/dropconfig b/scripts/dropconfig
@@ -30,7 +30,7 @@ except KeyError:
 
 # CONFIG_DB Tables
 DEBUG_COUNTER_CONFIG_TABLE = 'DEBUG_COUNTER'
-DROP_REASON_TABLE_PREFIX = 'DEBUG_COUNTER_DROP_REASON'
+DROP_REASON_CONFIG_TABLE = 'DEBUG_COUNTER_DROP_REASON'
 
 # STATE_DB Tables
 DEBUG_COUNTER_CAPABILITY_TABLE = 'DEBUG_COUNTER_CAPABILITIES'
@@ -136,10 +136,7 @@ class DropConfig(object):
             raise InvalidArgumentError('One or more provided drop reason not supported on this device')
 
         for reason in reasons:
-            self.config_db.set_entry(self.config_db.serialize_key(
-                                     (DROP_REASON_TABLE_PREFIX, counter_name)),
-                                     reason,
-                                     {})
+            self.config_db.set_entry(DROP_REASON_CONFIG_TABLE, (counter_name, reason), {})
 
         drop_counter_entry = {'type': counter_type}
 
@@ -168,8 +165,12 @@ class DropConfig(object):
         self.config_db.set_entry(DEBUG_COUNTER_CONFIG_TABLE,
                                  counter_name,
                                  None)
-        self.config_db.delete_table(self.config_db.serialize_key(
-                                    (DROP_REASON_TABLE_PREFIX, counter_name)))
+
+        # We can't use `delete_table` here because table names are normalized to uppercase.
+        # Counter names can be lowercase (e.g. "test_counter|ACL_ANY"), so we have to go
+        # through and treat each drop reason as an entry to get the correct behavior.
+        for reason in self.get_counter_drop_reasons(counter_name):
+            self.config_db.set_entry(DROP_REASON_CONFIG_TABLE, reason, None)
 
     def add_reasons(self, counter_name, reasons):
         """
@@ -192,10 +193,7 @@ class DropConfig(object):
             raise InvalidArgumentError('One or more provided drop reason not supported on this device')
 
         for reason in reasons:
-            self.config_db.set_entry(self.config_db.serialize_key(
-                                     (DROP_REASON_TABLE_PREFIX, counter_name)),
-                                     reason,
-                                     {})
+            self.config_db.set_entry(DROP_REASON_CONFIG_TABLE, (counter_name, reason), {})
 
     def remove_reasons(self, counter_name, reasons):
         """
@@ -212,10 +210,7 @@ class DropConfig(object):
             raise InvalidArgumentError('Counter \'{}\' not found'.format(counter_name))
 
         for reason in reasons:
-            self.config_db.set_entry(self.config_db.serialize_key(
-                                     (DROP_REASON_TABLE_PREFIX, counter_name)),
-                                     reason,
-                                     None)
+            self.config_db.set_entry(DROP_REASON_CONFIG_TABLE, (counter_name, reason), None)
 
     def get_config(self, group):
         """
@@ -236,7 +231,7 @@ class DropConfig(object):
             }
 
             # Get the drop reasons for this counter
-            drop_reason_keys = sorted(self.config_db.get_keys(self.config_db.serialize_key((DROP_REASON_TABLE_PREFIX, counter_name))), key=lambda x: x[1])
+            drop_reason_keys = sorted(self.get_counter_drop_reasons(counter_name), key=lambda x: x[1])
 
             # Fill in the first drop reason
             num_reasons = len(drop_reason_keys)
@@ -308,6 +303,11 @@ class DropConfig(object):
 
         return deserialize_reason_list(cap_query.get('reasons', ''))
 
+    def get_counter_drop_reasons(self, counter_name):
+        # get_keys won't filter on counter_name for us because the counter name is case sensitive and
+        # get_keys will normalize the table name to uppercase.
+        return [key for key in self.config_db.get_keys(DROP_REASON_CONFIG_TABLE) if key[0] == counter_name]
+
 
 def deserialize_reason_list(list_str):
     if list_str is None:

diff --git a/tests/drops_group_test.py b/tests/drops_group_test.py
@@ -28,11 +28,12 @@
 	L3_ANY
 """
 
-expected_counter_configuration = """Counter    Alias         Group         Type                 Reasons    Description
----------  ------------  ------------  -------------------  ---------  --------------------------------------------------
-DEBUG_0    DEBUG_0       N/A           PORT_INGRESS_DROPS   None       N/A
-DEBUG_1    SWITCH_DROPS  PACKET_DROPS  SWITCH_EGRESS_DROPS  None       Outgoing packet drops, tracked at the switch level
-DEBUG_2    DEBUG_2       N/A           PORT_INGRESS_DROPS   None
+expected_counter_configuration = """Counter            Alias              Group         Type                 Reasons    Description
+-----------------  -----------------  ------------  -------------------  ---------  --------------------------------------------------
+DEBUG_0            DEBUG_0            N/A           PORT_INGRESS_DROPS   None       N/A
+DEBUG_1            SWITCH_DROPS       PACKET_DROPS  SWITCH_EGRESS_DROPS  None       Outgoing packet drops, tracked at the switch level
+DEBUG_2            DEBUG_2            N/A           PORT_INGRESS_DROPS   None
+lowercase_counter  lowercase_counter  N/A           SWITCH_EGRESS_DROPS  None       N/A
 """
 
 expected_counter_configuration_with_group = """Counter    Alias         Group         Type                 Reasons    Description
@@ -46,9 +47,9 @@
 Ethernet4      N/A         0        1000         0           0        100        800
 Ethernet8      N/A       100          10         0           0          0         10
 
-          DEVICE    SWITCH_DROPS
-----------------  --------------
-sonic_drops_test            1000
+          DEVICE    SWITCH_DROPS    lowercase_counter
+----------------  --------------  -------------------
+sonic_drops_test            1000                    0
 """
 
 expected_counts_with_group = """
@@ -71,9 +72,9 @@
 Ethernet4      N/A         0           0         0           0          0          0
 Ethernet8      N/A         0           0         0           0          0          0
 
-          DEVICE    SWITCH_DROPS
-----------------  --------------
-sonic_drops_test               0
+          DEVICE    SWITCH_DROPS    lowercase_counter
+----------------  --------------  -------------------
+sonic_drops_test               0                    0
 """
 
 dropstat_path = "/tmp/dropstat"

diff --git a/tests/mock_tables/config_db.json b/tests/mock_tables/config_db.json
@@ -558,10 +558,14 @@
         "type": "PORT_INGRESS_DROPS",
         "desc": ""
     },
+    "DEBUG_COUNTER|lowercase_counter": {
+        "type": "SWITCH_EGRESS_DROPS"
+    },
     "DEBUG_COUNTER_DROP_REASON|DEBUG_0|IP_HEADER_ERROR": {},
     "DEBUG_COUNTER_DROP_REASON|DEBUG_1|ACL_ANY": {},
     "DEBUG_COUNTER_DROP_REASON|DEBUG_2|IP_HEADER_ERROR": {},
     "DEBUG_COUNTER_DROP_REASON|DEBUG_2|NO_L3_HEADER": {},
+    "DEBUG_COUNTER_DROP_REASON|lowercase_counter|L2_ANY": {},
     "FEATURE|bgp": {
         "state": "enabled",
         "auto_restart": "enabled",

diff --git a/tests/mock_tables/counters_db.json b/tests/mock_tables/counters_db.json
@@ -204,7 +204,8 @@
         "SAI_PORT_STAT_PFC_7_TX_PKTS": "807"
     },
     "COUNTERS:oid:0x21000000000000": {
-        "SAI_SWITCH_STAT_IN_DROP_REASON_RANGE_BASE": "1000"
+        "SAI_SWITCH_STAT_OUT_DROP_REASON_RANGE_BASE": "1000",
+        "SAI_SWITCH_STAT_OUT_CONFIGURED_DROP_REASONS_1_DROPPED_PKTS": "0"
     },
     "COUNTERS_PORT_NAME_MAP": {
         "Ethernet0": "oid:0x1000000000002",
@@ -227,7 +228,8 @@
         "DEBUG_2": "SAI_PORT_STAT_OUT_CONFIGURED_DROP_REASONS_1_DROPPED_PKTS"
     },
     "COUNTERS_DEBUG_NAME_SWITCH_STAT_MAP": {
-        "DEBUG_1": "SAI_SWITCH_STAT_IN_DROP_REASON_RANGE_BASE"
+        "DEBUG_1": "SAI_SWITCH_STAT_OUT_DROP_REASON_RANGE_BASE",
+        "lowercase_counter": "SAI_SWITCH_STAT_OUT_CONFIGURED_DROP_REASONS_1_DROPPED_PKTS"
     },
     "COUNTERS:oid:0x1500000000035b": {
         "PFC_WD_ACTION": "drop",