From ce9d87906c337543f82f6b6521ca673f96ef556c Mon Sep 17 00:00:00 2001 From: Danny Allen Date: Sun, 26 Jan 2020 12:38:57 -0800 Subject: [PATCH] [debugcounterorch] Add checks for supported counter types and drop reasons (#1173) - Refactor drop reason capability query to trim SAI prefixes - Store device capabilities in orchagent to perform safety checks Fixes #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 Azure/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 --- orchagent/debug_counter/drop_counter.cpp | 17 +++++-- orchagent/debug_counter/drop_counter.h | 5 +- orchagent/debugcounterorch.cpp | 60 +++++++++++++++++------- orchagent/debugcounterorch.h | 4 ++ 4 files changed, 61 insertions(+), 25 deletions(-) diff --git a/orchagent/debug_counter/drop_counter.cpp b/orchagent/debug_counter/drop_counter.cpp index aab9c4b54719..d68c7081cfff 100644 --- 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 + 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 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 DropCounter::getSupportedDropReasons(sai_debug_counter_attr_t drop_reason_type) +unordered_set 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 DropCounter::getSupportedDropReasons(sai_debug_counter_attr_t dro return {}; } - vector supported_drop_reasons; + unordered_set 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(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(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,7 +337,7 @@ vector 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 drop_reasons) +string DropCounter::serializeSupportedDropReasons(unordered_set drop_reasons) { if (drop_reasons.size() == 0) { @@ -338,7 +345,7 @@ string DropCounter::serializeSupportedDropReasons(vector drop_reasons) } 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 index 8ae34e3d4b69..bc548b34c79f 100644 --- 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 -#include #include #include #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 getSupportedDropReasons(sai_debug_counter_attr_t drop_reason_type); - static std::string serializeSupportedDropReasons(std::vector drop_reasons); + static std::unordered_set getSupportedDropReasons(sai_debug_counter_attr_t drop_reason_type); + static std::string serializeSupportedDropReasons(std::unordered_set drop_reasons); static uint64_t getSupportedDebugCounterAmounts(sai_debug_counter_type_t counter_type); private: diff --git a/orchagent/debugcounterorch.cpp b/orchagent/debugcounterorch.cpp index fd1005d45273..b4f2407ca473 100644 --- a/orchagent/debugcounterorch.cpp +++ b/orchagent/debugcounterorch.cpp @@ -181,35 +181,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 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 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); } } @@ -228,12 +239,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); @@ -286,7 +304,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 index 49ca64b7f73f..e5b512c8e46b 100644 --- a/orchagent/debugcounterorch.h +++ b/orchagent/debugcounterorch.h @@ -77,6 +77,10 @@ class DebugCounterOrch: public Orch std::shared_ptr m_counterNameToPortStatMap = nullptr; std::shared_ptr m_counterNameToSwitchStatMap = nullptr; + std::unordered_set supported_counter_types; + std::unordered_set supported_ingress_drop_reasons; + std::unordered_set supported_egress_drop_reasons; + FlexCounterStatManager flex_counter_manager; std::unordered_map> debug_counters;