Error Handling Framework : sonic-swss-common library support #309

sivamukka · 2019-09-20T10:37:41Z

Error handling framework is responsible for notifying ASIC/SAI programming failures to the applications.
Error handling framework is responsible for notifying ASIC/SAI programming failures to the applications.

This repo defines the following classes as part of error handling feature.

Error listener class: This class provides functions for applications to register and receive error notifications from error handling framework.
Error map class: It converts SAI specific error codes to SWSS error codes that application can understand.

Related PRs:
sonic-net/sonic-utilities#666
sonic-net/sonic-swss#1100
sonic-net/sonic-sairedis#523

Signed-off-by: Siva Mukka ([email protected])

qiluo-msft · 2019-10-31T00:48:37Z

common/table.cpp

@@ -26,7 +26,8 @@ const TableNameSeparatorMap TableBase::tableNameSeparatorMap = {
   { CONFIG_DB,       TABLE_NAME_SEPARATOR_VBAR  },
   { PFC_WD_DB,       TABLE_NAME_SEPARATOR_COLON },
   { FLEX_COUNTER_DB, TABLE_NAME_SEPARATOR_COLON },
-   { STATE_DB,        TABLE_NAME_SEPARATOR_VBAR  }
+   { STATE_DB,        TABLE_NAME_SEPARATOR_VBAR  },
+   { ERROR_DB,        TABLE_NAME_SEPARATOR_COLON }


TABLE_NAME_SEPARATOR_COLON [](start = 22, length = 26)

prefer TABLE_NAME_SEPARATOR_VBAR for new comer #Closed

The key format in ERROR_DB is same as in APP_DB. This is the reason why COLON was chosen instead of VBAR. As you said, ERROR_DB delimiter will be moved to VBAR when APP_DB delimiter changes to VBAR in future.

qiluo-msft · 2019-10-31T01:00:00Z

common/errormap.cpp

+        { "SAI_STATUS_FAILURE",             "SWSS_RC_FAILURE"           }
+    };
+
+    ErrorMap &ErrorMap::getInstance()


getInstance [](start = 24, length = 11)

everything in ErrorMap is static, why we need an instance? #Closed

Removed getIntance() in ErrorMap

qiluo-msft · 2019-10-31T01:02:05Z

common/errorlistener.h

+#include "notificationconsumer.h"
+#include "selectable.h"
+#include "table.h"
+#include <dbconnector.h>


Suggested change

#include <dbconnector.h>

#include "dbconnector.h"

``` #Closed

qiluo-msft · 2019-10-31T01:03:32Z

common/errorlistener.h

+
+            static std::string getErrorChannelName(std::string& appTableName)
+            {
+                std::string errorChnl = "ERROR_";


errorChnl [](start = 28, length = 9)

errorChannel #Closed

qiluo-msft · 2019-10-31T01:07:39Z

common/errorlistener.cpp

+
+        // Filter the error notifications that the caller is not interested in.
+        if (!(((m_errorFlags & ERR_NOTIFY_POSITIVE_ACK) && (j["rc"] == "SWSS_RC_SUCCESS")) ||
+                    ((m_errorFlags & ERR_NOTIFY_FAIL) && (j["rc"] != "SWSS_RC_SUCCESS"))))


)))) [](start = 86, length = 4)

Too complex in single expression. Consider split to multiple statements. #Closed

Simplified by splitting into two statements.

qiluo-msft · 2019-10-31T01:09:05Z

common/errorlistener.cpp

+        m_errorNotificationConsumer->pop(op, data, values);
+        SWSS_LOG_DEBUG("ErrorListener op: %s data: %s", op.c_str(), data.c_str());
+
+        json j = json::parse(data);


parse [](start = 23, length = 5)

parse [](start = 23, length = 5)

possible to throw? should we return -1? #Closed

We need to return -1 here. This return code is ignored by applications processing the error (Ex: BGP). But the error can still be fetched from the error database.

I mean json::parse() will throw if it met some invalid input. I guess you need to catch it and return -1.

In reply to: 340924415 [](ancestors = 340924415)

qiluo-msft · 2019-10-31T01:09:11Z

common/errorlistener.cpp

+            return -1;
+        }
+
+        key = j["key"];


j["key"]; [](start = 14, length = 9)

possible to throw? should we return -1?

We need to return -1 here. This return code is ignored by applications processing the error (Ex: BGP). But the error can still be fetched from the error database.

qiluo-msft · 2019-10-31T01:13:09Z

common/Makefile.am

-    warm_restart.cpp
+    warm_restart.cpp          \
+    errormap.cpp              \
+    errorlistener.cpp


If they are only used in swss, how about move code to that repo? #Closed

It's placed in swss-common folder as the error listener could be in any docker. The docker that is interested in receiving the error needs to link swss-common package.

qiluo-msft

Commented

kcudnik · 2019-12-04T13:56:24Z

common/errorlistener.h


-                return errorChnl;
+                return errorChannel;


please don't put code to header files

Moved to corresponding .cpp files

kcudnik · 2019-12-04T13:56:39Z

common/errorlistener.h


-                return errorChnl;
+                return errorChannel;
            }

            int getFd() { return m_errorNotificationConsumer->getFd(); }


dont put code in headers, move to cpp file

Moved to corresponding .cpp file

kcudnik · 2019-12-04T13:58:20Z

common/errorlistener.cpp

+        delete m_errorNotificationsDb;
+    }
+
+    /* Returns the Error notification corresponding to an entry in the APP_DB.


make function comments in doxygen format with brief line and then main explanation

common/errorlistener.cpp

kcudnik · 2019-12-04T13:59:08Z

common/errorlistener.h

+#include "dbconnector.h"
+
+// Error notifications of interest to the error listener
+typedef enum


typedef enum _error_notify_flags_t

kcudnik · 2019-12-04T14:03:44Z

common/errormap.cpp

+
+    ErrorMap::SwssRC ErrorMap::getSwssRC(const std::string &swssRCStr)
+    {
+        for (auto &elem : m_swssStrToRC)


Used array of pairs for m_swssStrToRC instead of map, as we need functions to convert from SWSS RC string to SWSS RC code and vice versa. This is the reason why for loop is used to find the given input in the array.

kcudnik · 2019-12-04T14:04:14Z

common/errormap.cpp

+        /* Error mapping is not found */
+        SWSS_LOG_ERROR("Invalid SWSS error code %d is received", rc);
+
+        return "SWSS_RC_UNKNOWN";


used multiple times, actually you can introduce this error code and return it from map

The codes mentioned in m_swssStrToRC and m_saiToSwssRC are the codes generated by SAI and supported in error handling framework. In the case where SAI return code is not supported in error handling framework, we return UNKNOWN error code.

kcudnik · 2019-12-04T14:04:42Z

common/errormap.h

+
+        private:
+            ErrorMap() = default;
+            ~ErrorMap();


also = default,

Done. Removed the default constructor.

kcudnik · 2019-12-04T14:05:59Z

common/errormap.h

+            };
+
+            typedef std::map<std::string, std::string> SaiToSwssRCMap;
+            typedef std::vector<std::pair<std::string, SwssRC>> SwssStrToRCMap;


whyt this is vector and not actual map ?

Used array of pairs for m_swssStrToRC instead of map, as we need functions to convert from SWSS RC string to SWSS RC code and vice versa. This is the reason why for loop is used to find the given input in the array.

If we want to use map here, then we need to have two maps one taking SWSS RC code as key and another taking SWSS RC string as key. To avoid two maps, I have used array of pairs.

kcudnik · 2019-12-04T14:06:32Z

common/errormap.cpp

+        { std::make_pair("SWSS_RC_INVALID_OBJECT_ID",   SWSS_RC_INVALID_OBJECT_ID)}
+    };
+
+    const ErrorMap::SaiToSwssRCMap ErrorMap::m_saiToSwssRC = {


convert to std::map

Used array of pairs for m_swssStrToRC instead of map, as we need functions to convert from SWSS RC string to SWSS RC code and vice versa. This is the reason why for loop is used to find the given input in the array.

If we want to use map here, then we need to have two maps one taking SWSS RC code as key and another taking SWSS RC string as key. To avoid two maps, I have used array of pairs.

qiluo-msft

Looks good to me. Please check with other reviewers.

kcudnik · 2019-12-11T15:26:57Z

common/errorlistener.h

+// Error notifications of interest to the error listener
+typedef enum _error_notify_flags_t
+{
+    ERR_NOTIFY_FAIL = 1,


just curious why not have enum start from zero?

Added ERR_NOTIFY_NONE with value of zero to indicate that application is not interested in any notifications

Defined ERR_NOTIFY_NONE

sivamukka mentioned this pull request Sep 23, 2019

Error Handling Framework : CLI support sonic-net/sonic-utilities#666

Open

sivamukka force-pushed the error_handling branch 2 times, most recently from 54ab228 to c717a13 Compare October 17, 2019 09:33

This was referenced Oct 18, 2019

Error Handling Framework: Orchestration changes to report SAI API failures sonic-net/sonic-swss#1100

Open

Error Handling Framework: Syncd Changes to report SAI failures sonic-net/sonic-sairedis#523

Closed

sivamukka force-pushed the error_handling branch from c717a13 to d07b893 Compare October 18, 2019 07:13

sudhanshukumar22 mentioned this pull request Oct 18, 2019

fpmsyncd : Changes for supporting install-then-advertise behavior sonic-net/sonic-swss#1101

Open

sivamukka changed the title ~~Error Handling Framework changes~~ Error Handling Framework : sonic-swss-common library support Oct 18, 2019

lguohan requested a review from qiluo-msft October 30, 2019 20:28

qiluo-msft reviewed Oct 31, 2019

View reviewed changes

kcudnik requested changes Dec 4, 2019

View reviewed changes

sivamukka added 2 commits December 10, 2019 03:51

Error Handling Framework changes

6c4d633

Addressed review comments

6cf7dfd

sivamukka force-pushed the error_handling branch from 4a9d987 to 6cf7dfd Compare December 10, 2019 11:52

Addressed review comments

e17aab1

qiluo-msft previously approved these changes Dec 11, 2019

View reviewed changes

kcudnik reviewed Dec 11, 2019

View reviewed changes

Addressed review comments

4217469

Defined ERR_NOTIFY_NONE

qiluo-msft dismissed their stale review via 4217469 July 29, 2021 03:33

Error Handling Framework : sonic-swss-common library support #309

Are you sure you want to change the base?

Error Handling Framework : sonic-swss-common library support #309

Conversation

sivamukka commented Sep 20, 2019 • edited Loading

qiluo-msft Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

sivamukka Nov 28, 2019 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sivamukka commented Sep 20, 2019 •

edited

Loading

qiluo-msft Oct 31, 2019 •

edited

Loading

sivamukka Nov 28, 2019 •

edited

Loading

qiluo-msft Oct 31, 2019 •

edited

Loading

qiluo-msft Oct 31, 2019 •

edited

Loading

qiluo-msft Oct 31, 2019 •

edited

Loading

qiluo-msft Oct 31, 2019 •

edited

Loading

qiluo-msft Oct 31, 2019 •

edited

Loading

qiluo-msft Oct 31, 2019 •

edited

Loading