[debugcounterorch] Add checks for supported counter types and drop re…

…asons (sonic-net#1173) - Refactor drop reason capability query to trim SAI prefixes - Store device capabilities in orchagent to perform safety checks Fixes sonic-net#1136 - Rather than depending on each ASIC vendor to follow the same error handling doctrine, this PR validates HW support in orchagent, which should be more reliable. Related to sonic-net/sonic-utilities#785 - In order to validate user input, we need to remove the SAI prefixes before we store the results. This removes the need for the CLI to perform these checks. Signed-off-by: Danny Allen <[email protected]>
raphaelt-nvidia · Jan 28, 2020 · bc55d4a · bc55d4a
1 parent 4edcc4d
commit bc55d4a
Show file tree

Hide file tree

Showing 4 changed files with 61 additions and 25 deletions.
diff --git a/orchagent/debug_counter/drop_counter.cpp b/orchagent/debug_counter/drop_counter.cpp
@@ -3,6 +3,8 @@
 #include "logger.h"
 #include "sai_serialize.h"
 
+#include <vector>
+
 using std::runtime_error;
 using std::string;
 using std::unordered_map;
@@ -12,6 +14,9 @@ using std::vector;
 extern sai_object_id_t gSwitchId;
 extern sai_debug_counter_api_t *sai_debug_counter_api;
 
+#define INGRESS_DROP_REASON_PREFIX_LENGTH 19 // "SAI_IN_DROP_REASON_"
+#define EGRESS_DROP_REASON_PREFIX_LENGTH  20 // "SAI_OUT_DROP_REASON_"
+
 const unordered_map<string, sai_in_drop_reason_t> DropCounter::ingress_drop_reason_lookup =
 {
     { L2_ANY,               SAI_IN_DROP_REASON_L2_ANY },
@@ -290,7 +295,7 @@ void DropCounter::updateDropReasonsInSAI()
 //
 // If the device does not support querying drop reasons, this method will
 // return an empty list.
-vector<string> DropCounter::getSupportedDropReasons(sai_debug_counter_attr_t drop_reason_type)
+unordered_set<string> DropCounter::getSupportedDropReasons(sai_debug_counter_attr_t drop_reason_type)
 {
     sai_s32_list_t drop_reason_list;
     int32_t        supported_reasons[maxDropReasons];
@@ -306,20 +311,22 @@ vector<string> DropCounter::getSupportedDropReasons(sai_debug_counter_attr_t dro
         return {};
     }
 
-    vector<string> supported_drop_reasons;
+    unordered_set<string> supported_drop_reasons;
     for (uint32_t i = 0; i < drop_reason_list.count; i++)
     {
         string drop_reason;
         if (drop_reason_type == SAI_DEBUG_COUNTER_ATTR_IN_DROP_REASON_LIST)
         {
             drop_reason = sai_serialize_ingress_drop_reason(static_cast<sai_in_drop_reason_t>(drop_reason_list.list[i]));
+            drop_reason = drop_reason.substr(INGRESS_DROP_REASON_PREFIX_LENGTH);
         }
         else
         {
             drop_reason = sai_serialize_egress_drop_reason(static_cast<sai_out_drop_reason_t>(drop_reason_list.list[i]));
+            drop_reason = drop_reason.substr(EGRESS_DROP_REASON_PREFIX_LENGTH);
         }
 
-        supported_drop_reasons.push_back(drop_reason);
+        supported_drop_reasons.emplace(drop_reason);
     }
 
     return supported_drop_reasons;
@@ -330,15 +337,15 @@ vector<string> DropCounter::getSupportedDropReasons(sai_debug_counter_attr_t dro
 //
 // e.g. { "SMAC_EQUALS_DMAC", "INGRESS_VLAN_FILTER" } -> "["SMAC_EQUALS_DMAC","INGRESS_VLAN_FILTER"]"
 // e.g. { } -> "[]"
-string DropCounter::serializeSupportedDropReasons(vector<string> drop_reasons)
+string DropCounter::serializeSupportedDropReasons(unordered_set<string> drop_reasons)
 {
     if (drop_reasons.size() == 0)
     {
         return "[]";
     }
 
     string supported_drop_reasons;
-    for (auto const &drop_reason : drop_reasons)
+    for (auto const& drop_reason : drop_reasons)
     {
         supported_drop_reasons += ',';
         supported_drop_reasons += drop_reason;

diff --git a/orchagent/debug_counter/drop_counter.h b/orchagent/debug_counter/drop_counter.h
@@ -2,7 +2,6 @@
 #define SWSS_UTIL_DROP_COUNTER_H_
 
 #include <string>
-#include <vector>
 #include <unordered_set>
 #include <unordered_map>
 #include "debug_counter.h"
@@ -33,8 +32,8 @@ class DropCounter : public DebugCounter
         static bool isIngressDropReasonValid(const std::string& drop_reason);
         static bool isEgressDropReasonValid(const std::string& drop_reason);
 
-        static std::vector<std::string> getSupportedDropReasons(sai_debug_counter_attr_t drop_reason_type);
-        static std::string serializeSupportedDropReasons(std::vector<std::string> drop_reasons);
+        static std::unordered_set<std::string> getSupportedDropReasons(sai_debug_counter_attr_t drop_reason_type);
+        static std::string serializeSupportedDropReasons(std::unordered_set<std::string> drop_reasons);
         static uint64_t getSupportedDebugCounterAmounts(sai_debug_counter_type_t counter_type);
 
     private:

diff --git a/orchagent/debugcounterorch.cpp b/orchagent/debugcounterorch.cpp
@@ -182,35 +182,46 @@ void DebugCounterOrch::doTask(Consumer& consumer)
 // DROP_COUNTER_CAPABILITIES table in STATE_DB.
 void DebugCounterOrch::publishDropCounterCapabilities()
 {
-    string supported_ingress_drop_reasons = DropCounter::serializeSupportedDropReasons(
-            DropCounter::getSupportedDropReasons(SAI_DEBUG_COUNTER_ATTR_IN_DROP_REASON_LIST));
-    string supported_egress_drop_reasons  = DropCounter::serializeSupportedDropReasons(
-            DropCounter::getSupportedDropReasons(SAI_DEBUG_COUNTER_ATTR_OUT_DROP_REASON_LIST));
+    supported_ingress_drop_reasons = DropCounter::getSupportedDropReasons(SAI_DEBUG_COUNTER_ATTR_IN_DROP_REASON_LIST);
+    supported_egress_drop_reasons  = DropCounter::getSupportedDropReasons(SAI_DEBUG_COUNTER_ATTR_OUT_DROP_REASON_LIST);
+
+    string ingress_drop_reason_str = DropCounter::serializeSupportedDropReasons(supported_ingress_drop_reasons);
+    string egress_drop_reason_str = DropCounter::serializeSupportedDropReasons(supported_egress_drop_reasons);
 
     for (auto const &counter_type : DebugCounter::getDebugCounterTypeLookup())
     {
-        string num_counters = std::to_string(DropCounter::getSupportedDebugCounterAmounts(counter_type.second));
-
         string drop_reasons;
         if (counter_type.first == PORT_INGRESS_DROPS || counter_type.first == SWITCH_INGRESS_DROPS)
         {
-            drop_reasons = supported_ingress_drop_reasons;
+            drop_reasons = ingress_drop_reason_str;
         }
         else
         {
-            drop_reasons = supported_egress_drop_reasons;
+            drop_reasons = egress_drop_reason_str;
         }
 
-        // Only include available capabilities in State DB
-        if (num_counters != "0" && !drop_reasons.empty())
+        // Don't bother publishing counters that have no drop reasons
+        if (drop_reasons.empty())
         {
-            vector<FieldValueTuple> fieldValues;
-            fieldValues.push_back(FieldValueTuple("count", num_counters));
-            fieldValues.push_back(FieldValueTuple("reasons", drop_reasons));
+            continue;
+        }
 
-            SWSS_LOG_DEBUG("Setting '%s' capabilities to count='%s', reasons='%s'", counter_type.first.c_str(), num_counters.c_str(), drop_reasons.c_str());
-            m_debugCapabilitiesTable->set(counter_type.first, fieldValues);
+        string num_counters = std::to_string(DropCounter::getSupportedDebugCounterAmounts(counter_type.second));
+
+        // Don't bother publishing counters that aren't available.
+        if (num_counters == "0")
+        {
+            continue;
         }
+
+        supported_counter_types.emplace(counter_type.first);
+
+        vector<FieldValueTuple> fieldValues;
+        fieldValues.push_back(FieldValueTuple("count", num_counters));
+        fieldValues.push_back(FieldValueTuple("reasons", drop_reasons));
+
+        SWSS_LOG_DEBUG("Setting '%s' capabilities to count='%s', reasons='%s'", counter_type.first.c_str(), num_counters.c_str(), drop_reasons.c_str());
+        m_debugCapabilitiesTable->set(counter_type.first, fieldValues);
     }
 }
 
@@ -229,12 +240,19 @@ task_process_status DebugCounterOrch::installDebugCounter(const string& counter_
         return task_process_status::task_success;
     }
 
-    // Note: this method currently assumes that all counters are drop counters.
+    // NOTE: this method currently assumes that all counters are drop counters.
     // If you are adding support for a non-drop counter than it may make sense
     // to either: a) dispatch to different handlers in doTask or b) dispatch to
     // different helper methods in this method.
 
     string counter_type = getDebugCounterType(attributes);
+
+    if (supported_counter_types.find(counter_type) == supported_counter_types.end())
+    {
+        SWSS_LOG_ERROR("Specified counter type '%s' is not supported.", counter_type.c_str());
+        return task_process_status::task_failed;
+    }
+
     addFreeCounter(counter_name, counter_type);
     reconcileFreeDropCounters(counter_name);
 
@@ -287,7 +305,15 @@ task_process_status DebugCounterOrch::addDropReason(const string& counter_name,
 
     if (!isDropReasonValid(drop_reason))
     {
-        return task_failed;
+        SWSS_LOG_ERROR("Specified drop reason '%s' is invalid.", drop_reason.c_str());
+        return task_process_status::task_failed;
+    }
+
+    if (supported_ingress_drop_reasons.find(drop_reason) == supported_ingress_drop_reasons.end() &&
+        supported_egress_drop_reasons.find(drop_reason) == supported_egress_drop_reasons.end())
+    {
+        SWSS_LOG_ERROR("Specified drop reason '%s' is not supported.", drop_reason.c_str());
+        return task_process_status::task_failed;
     }
 
     auto it = debug_counters.find(counter_name);

diff --git a/orchagent/debugcounterorch.h b/orchagent/debugcounterorch.h
@@ -77,6 +77,10 @@ class DebugCounterOrch: public Orch
     std::shared_ptr<swss::Table> m_counterNameToPortStatMap = nullptr;
     std::shared_ptr<swss::Table> m_counterNameToSwitchStatMap = nullptr;
 
+    std::unordered_set<std::string> supported_counter_types;
+    std::unordered_set<std::string> supported_ingress_drop_reasons;
+    std::unordered_set<std::string> supported_egress_drop_reasons;
+
     FlexCounterStatManager flex_counter_manager;
 
     std::unordered_map<std::string, std::unique_ptr<DebugCounter>> debug_counters;