Fix flow counter out-of-order issue by notifying counter operations u…

…sing SelectableChannel (#1362)

What I did

Fix flow counter out-of-order issue by notifying counter operations using SelectableChannel

Signed-off-by: Stephen Sun stephens@nvidia.com

Why I did it

Currently, the operations of SAI objects and their counters (if any) are triggered by different channels, which introduces racing conditions:

the creation and destruction of the objects are notified using the SelectableChannel,
the operations of counters, including starting and stopping polling the counters, are notified by listening to the FLEX_COUNTER and FLEX_COUNTER_GROUP tables in the FLEX_COUNTER_DB
The orchagent always respects the order when starting/stopping counter-polling (which means to start counter-polling after creating the object and to stop counter-polling before destroying the object) but syncd can receive events in a wrong order, eg. it receives destroying an object first and then stopping counter polling on the object, it can poll counter for a non-exist object, which causes errors in vendor SAI.
The new solution is to extend SAI redis attributes on the SAI_SWITCH_OBJECT to notify counter polling. As a result, all the objects and their counters are notified using a unified channel, which is the SelectableChannel.

How I verified it

Unit test
Manual test
Regressions test

Details if related

There are two SAI Redis attributes introduced as below. There are some fields with const char * type for each attribute. Passing a field as nullptr means not to change it.

SAI_REDIS_SWITCH_ATTR_FLEX_COUNTER_GROUP for counters represented by FLEX_COUNTER_GROUP table in the FLEX_COUNTER_DB, including the following fields
counter_group_name, which is the key of the table, representing the group name.
poll_interval, which is the field POLL_INTERVAL of an entry, representing the polling interval of the group.
operation, which is the field FLEX_COUNTER_STATUS of an entry, representing whether the counter polling is enabled for the group
stats_mode, which is the field STATS_MODE of an entry, either STATS_MODE_READ or STATS_MODE_READ_AND_CLEAR
plugins, which represents the Lua plugin related to the group
plugin_name, which is the name of the plugins field. It differs among different groups
SAI_REDIS_SWITCH_ATTR_FLEX_COUNTER for counter groups represented by the FLEX_COUNTER table in the FLEX_COUNTER_DB, including the following fields
counter_key, which is the key of the table, with the name convention of <group-name>:oid:<oid-value>
counter_ids, which is a list of counter IDs to be polled for the object
counter_field_name, which is the name of the counter ID field. It differs among different groups
stats_mode, which is the field STATS_MODE of an entry, either STATS_MODE_READ or STATS_MODE_READ_AND_CLEAR
Both SAI attributes are terminated by the RedisRemoteSaiInterface object in the swss context, which serializes the SAI API call into the selectable channel.

REDIS_FLEX_COUNTER_COMMAND_SET_COUNTER_GROUP: represents the SET operation in the FLEX_COUNTER_GROUP table
REDIS_FLEX_COUNTER_COMMAND_DEL_COUNTER_GROUP: represents the DEL operation in the FLEX_COUNTER_GROUP table
REDIS_FLEX_COUNTER_COMMAND_START_POLL: represents the SET operation in the FLEX_COUNTER table
REDIS_FLEX_COUNTER_COMMAND_STOP_POLL: represents the DEL operation in the FLEX_COUNTER table
The Syncd will call flex counter functions to handle them on receiving the above-extended commands (representing both SAI extended attributes).

Gearbox flex counter database
Pass the Phy OID, an OID of a SAI switch object in syntax, when calling the SAI set API to set the extended attributes. By doing so, the SAI redis objects can choose in which context the SAI API call should be invoked and the corresponding gearbox syncd docker container will handle it.
(ps: THE ORIGINAL GEARBOX FLEX COUNTER IMPLEMENTATION IS BUGGY)

Context and critical section analysis
It does not change the critical section hierarchy

Performance analysis
The counter operations are handled in the same thread in both the new and old solutions.
In swss, the counter operation was asynchronous in the old solution and is synchronous now, which can introduce a bit more latency. However, as the number of counter operations is small, no performance degradation is observed.

lib/RedisRemoteSaiInterface.cpp

@@ -476,6 +476,14 @@ sai_status_t RedisRemoteSaiInterface::setRedisExtensionAttribute(
 
             return SAI_STATUS_SUCCESS;
 
+        case SAI_REDIS_SWITCH_ATTR_FLEX_COUNTER_GROUP:
+            return notifyCounterGroupOperations(objectId,
+                                                reinterpret_cast<sai_redis_flex_counter_group_parameter_t*>(attr->value.ptr));
+
+        case SAI_REDIS_SWITCH_ATTR_FLEX_COUNTER:
+            return notifyCounterOperations(objectId,
+                                           reinterpret_cast<sai_redis_flex_counter_parameter_t*>(attr->value.ptr));
+
         default:
             break;
     }
@@ -485,6 +493,126 @@ sai_status_t RedisRemoteSaiInterface::setRedisExtensionAttribute(
     return SAI_STATUS_FAILURE;
 }
 
+bool RedisRemoteSaiInterface::isSaiS8ListValidString(
+        _In_ const sai_s8_list_t &s8list)
+{
+    SWSS_LOG_ENTER();
+
+    if (s8list.list != nullptr && s8list.count > 0)
+    {
+        size_t len = strnlen((const char *)s8list.list, s8list.count);
+
+        if (len == (size_t)s8list.count)
+        {
+            return true;
+        }
+        else
+        {
+            SWSS_LOG_ERROR("Count (%u) is different than strnlen (%zu)", s8list.count, len);
+        }
+    }
+
+    return false;
+}
+
+bool RedisRemoteSaiInterface::emplaceStrings(
+        _In_ const sai_s8_list_t &field,
+        _In_ const sai_s8_list_t &value,
+        _Out_ std::vector<swss::FieldValueTuple> &entries)
+{
+    SWSS_LOG_ENTER();
+
+    bool result = false;
+
+    if (isSaiS8ListValidString(field) && isSaiS8ListValidString(value))
+    {
+        entries.emplace_back(std::string((const char*)field.list, field.count), std::string((const char*)value.list, value.count));
+        result = true;
+    }
+
+    return result;
+}
+
+bool RedisRemoteSaiInterface::emplaceStrings(
+        _In_ const char *field,
+        _In_ const sai_s8_list_t &value,
+        _Out_ std::vector<swss::FieldValueTuple> &entries)
+{
+    SWSS_LOG_ENTER();
+
+    bool result = false;
+
+    if (isSaiS8ListValidString(value))
+    {
+        entries.emplace_back(field, std::string((const char*)value.list, value.count));
+        result = true;
+    }
+
+    return result;
+}
+
+sai_status_t RedisRemoteSaiInterface::notifyCounterGroupOperations(
+        _In_ sai_object_id_t objectId,
+        _In_ const sai_redis_flex_counter_group_parameter_t *flexCounterGroupParam)
+{
+    SWSS_LOG_ENTER();
+
+    std::vector<swss::FieldValueTuple> entries;
+
+    if (flexCounterGroupParam == nullptr || !isSaiS8ListValidString(flexCounterGroupParam->counter_group_name))
+    {
+        SWSS_LOG_ERROR("Invalid parameters when handling counter group operation");
+        return SAI_STATUS_FAILURE;
+    }
+
+    std::string key((const char*)flexCounterGroupParam->counter_group_name.list, flexCounterGroupParam->counter_group_name.count);
+
+    emplaceStrings(POLL_INTERVAL_FIELD, flexCounterGroupParam->poll_interval, entries);
+    emplaceStrings(STATS_MODE_FIELD, flexCounterGroupParam->stats_mode, entries);
+    emplaceStrings(flexCounterGroupParam->plugin_name, flexCounterGroupParam->plugins, entries);
+    emplaceStrings(FLEX_COUNTER_STATUS_FIELD, flexCounterGroupParam->operation, entries);
+
+    m_recorder->recordGenericSet(key, entries);
+
+    m_communicationChannel->set(key,
+                                entries,
+                                (entries.size() != 0) ? REDIS_FLEX_COUNTER_COMMAND_SET_GROUP : REDIS_FLEX_COUNTER_COMMAND_DEL_GROUP);
+
+    return waitForResponse(SAI_COMMON_API_SET);
+}
+
+sai_status_t RedisRemoteSaiInterface::notifyCounterOperations(
+        _In_ sai_object_id_t objectId,
+        _In_ const sai_redis_flex_counter_parameter_t *flexCounterParam)
+{
+    SWSS_LOG_ENTER();
+
+    if (flexCounterParam == nullptr || !isSaiS8ListValidString(flexCounterParam->counter_key))
+    {
+        SWSS_LOG_ERROR("Invalid parameters when handling counter operation");
+        return SAI_STATUS_FAILURE;
+    }
+
+    std::vector<swss::FieldValueTuple> entries;
+    std::string key((const char*)flexCounterParam->counter_key.list, flexCounterParam->counter_key.count);
+    std::string command;
+
+    if (emplaceStrings(flexCounterParam->counter_field_name, flexCounterParam->counter_ids, entries))
+    {
+        command = REDIS_FLEX_COUNTER_COMMAND_START_POLL;
+        emplaceStrings(STATS_MODE_FIELD, flexCounterParam->stats_mode, entries);
+    }
+    else
+    {
+        command = REDIS_FLEX_COUNTER_COMMAND_STOP_POLL;
+    }
+
+    m_recorder->recordGenericSet(key, entries);
+    m_communicationChannel->set(key, entries, command);
+
+    return waitForResponse(SAI_COMMON_API_SET);
+}
+
 sai_status_t RedisRemoteSaiInterface::set(
         _In_ sai_object_type_t objectType,
         _In_ sai_object_id_t objectId,

lib/RedisRemoteSaiInterface.h

@@ -343,6 +343,27 @@ namespace sairedis
                     _In_ sai_object_id_t objectId,
                     _In_ const sai_attribute_t *attr);
 
+            bool isSaiS8ListValidString(
+                    _In_ const sai_s8_list_t &s8list);
+
+            bool emplaceStrings(
+                    _In_ const sai_s8_list_t &field,
+                    _In_ const sai_s8_list_t &value,
+                    _Out_ std::vector<swss::FieldValueTuple> &entries);
+
+            bool emplaceStrings(
+                    _In_ const char *field,
+                    _In_ const sai_s8_list_t &value,
+                    _Out_ std::vector<swss::FieldValueTuple> &entries);
+
+            sai_status_t notifyCounterGroupOperations(
+                    _In_ sai_object_id_t objectId,
+                    _In_ const sai_redis_flex_counter_group_parameter_t *flexCounterGroupParam);
+
+            sai_status_t notifyCounterOperations(
+                    _In_ sai_object_id_t objectId,
+                    _In_ const sai_redis_flex_counter_parameter_t *flexCounterParam);
+
         private:
 
             sai_status_t sai_redis_notify_syncd(

lib/sairedis.h

@@ -86,6 +86,88 @@ typedef enum _sai_redis_communication_mode_t
 
 } sai_redis_communication_mode_t;
 
+/**
+ * @brief Use Redis communication channel to handle counters.
+ *
+ * Originally, there are out-of-order issue between the objects and their counters.
+ * This is because the counters are handled on receiving flex counter database update.
+ * However, the objects are handled on receiving update from the redis communication channel.
+ *
+ * To resolve the issue, we use the redis communication channel to handle the counter updates.
+ *
+ * The struct sai_redis_flex_counter_group_parameter_t represents the counter group operations.
+ * The caller (usually orchagent) can change some or all the options of a counter group.
+ * The counter_group_name represents the counter group name which must be a valid list.
+ * For the rest fields, it means not changing it to pass an empty list.
+ */
+typedef struct _sai_redis_flex_counter_group_parameter_t
+{
+    /**
+     * @brief The flex counter group name.
+     *
+     * It is the key of FLEX_COUNTER_TABLE and FLEX_COUNTER_GROUP_TABLE table.
+     */
+    sai_s8_list_t counter_group_name;
+
+    /**
+     * @brief The polling interval of the counter group
+     *
+     * It should be a number representing the polling interval in seconds.
+     */
+    sai_s8_list_t poll_interval;
+
+    /**
+     * @brief The operation of the counter group
+     *
+     * It should be either "enable" or "disable"
+     */
+    sai_s8_list_t operation;
+
+    /**
+     * @brief The counter fetching mode.
+     *
+     * It should be either "STATS_MODE_READ" or "STATS_MODE_READ_AND_CLEAR"
+     */
+    sai_s8_list_t stats_mode;
+
+    /**
+     * @brief The name of the filed that represents the Lua plugin
+     */
+    sai_s8_list_t plugin_name;
+
+    /**
+     * @brief The SHA code of the Lua plugin
+     */
+    sai_s8_list_t plugins;
+
+} sai_redis_flex_counter_group_parameter_t;
+
+typedef struct _sai_redis_flex_counter_parameter_t
+{
+    /**
+     * @brief The key in the flex counter table
+     *
+     * It should be the serialized OID eg. "oid:0x15000000000001"
+     */
+    sai_s8_list_t counter_key;
+
+    /**
+     * @brief The list of counters' IDs that should be fetched.
+     */
+    sai_s8_list_t counter_ids;
+
+    /**
+     * @brief The name of the filed that represents the counters' IDs.
+     */
+    sai_s8_list_t counter_field_name;
+
+    /**
+     * @brief The counter fetch mode of the object.
+     */
+    sai_s8_list_t stats_mode;
+
+} sai_redis_flex_counter_parameter_t;
+
 typedef enum _sai_redis_switch_attr_t
 {
     /**
@@ -249,6 +331,25 @@ typedef enum _sai_redis_switch_attr_t
      * @default 60000
      */
     SAI_REDIS_SWITCH_ATTR_SYNC_OPERATION_RESPONSE_TIMEOUT,
+
+    /**
+     * @brief Flex counter group operations
+     *
+     * @type sai_redis_flex_counter_group_parameter_t
+     * @flags CREATE_AND_SET
+     * @default 0
+     */
+    SAI_REDIS_SWITCH_ATTR_FLEX_COUNTER_GROUP,
+
+    /**
+     * @brief Flex counter operations
+     *
+     * @type sai_redis_counter_parameter_t
+     * @flags CREATE_AND_SET
+     * @default 0
+     */
+    SAI_REDIS_SWITCH_ATTR_FLEX_COUNTER,
+
 } sai_redis_switch_attr_t;
 
 /**

lib/sairediscommon.h

@@ -52,6 +52,11 @@
 #define REDIS_ASIC_STATE_COMMAND_OBJECT_TYPE_GET_AVAILABILITY_QUERY     "object_type_get_availability_query"
 #define REDIS_ASIC_STATE_COMMAND_OBJECT_TYPE_GET_AVAILABILITY_RESPONSE  "object_type_get_availability_response"
 
+#define REDIS_FLEX_COUNTER_COMMAND_START_POLL       "start_poll"
+#define REDIS_FLEX_COUNTER_COMMAND_STOP_POLL        "stop_poll"
+#define REDIS_FLEX_COUNTER_COMMAND_SET_GROUP        "set_counter_group"
+#define REDIS_FLEX_COUNTER_COMMAND_DEL_GROUP        "del_counter_group"
+#define REDIS_FLEX_COUNTER_COMMAND_RESPONSE         "counter_response"
 /**
  * @brief Redis virtual object id counter key name.
  *