Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion common/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,9 @@ libswsscommon_la_SOURCES = \
exec.cpp \
subscriberstatetable.cpp \
timestamp.cpp \
warm_restart.cpp
warm_restart.cpp \
errormap.cpp \
errorlistener.cpp
Copy link
Contributor

@qiluo-msft qiluo-msft Oct 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


libswsscommon_la_CXXFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(LIBNL_CFLAGS)
libswsscommon_la_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(LIBNL_CPPFLAGS)
Expand Down
127 changes: 127 additions & 0 deletions common/errorlistener.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
/*
* Copyright 2019 Broadcom. The term Broadcom refers to Broadcom Inc. and/or
* its subsidiaries.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include <vector>
#include "errorlistener.h"
#include "common/json.h"
#include "common/json.hpp"
using json = nlohmann::json;

namespace swss {

/**
* Function Description:
* @brief To create Error listener object that listens for h/w programming
* errors on table entries in the APP_DB table
*
* Arguments:
* @param[in] appTableName - APP DB table name
* @param[in] errFlags - flags
*
*/
ErrorListener::ErrorListener(std::string appTableName, int errFlags)
{
SWSS_LOG_ENTER();

m_errorFlags = errFlags;
m_errorChannelName = getErrorChannelName(appTableName);
m_errorNotificationsDb = std::unique_ptr<swss::DBConnector>(new DBConnector(ERROR_DB, DBConnector::DEFAULT_UNIXSOCKET, 0));
m_errorNotificationConsumer = std::unique_ptr<swss::NotificationConsumer>
(new swss::NotificationConsumer (m_errorNotificationsDb.get(), m_errorChannelName));
}

/**
* Function Description:
* @brief Get error channel name for a given APP DB table
*
* Arguments:
* @param[in] appTableName - APP DB table name
*
* Return Values:
* @return Error channel name
*/
std::string ErrorListener::getErrorChannelName(std::string& appTableName)
{
SWSS_LOG_ENTER();
std::string errorChannel = "ERROR_";
errorChannel += appTableName + "_CHANNEL";

return errorChannel;
}

/**
* Function Description:
* @brief Get the notification entry details
*
* Error listener object instantiated by the application gets selected on
* failure/success notitication from error handling framework. Then, the
* application uses this function to fetch more details about the notification.

* Arguments:
* @param[out] key - Key corresponding to the notification entry
* @param[out] opCode - operation corresponding to the entry
* @param[out] attrs - Attributes corresponding to the entry
*
* Return Values:
* @return 0, if the entry is read successfully
* @return -1, if failed to fetch the entry or the entry is not
* of interest to the caller
*/
int ErrorListener::getError(std::string &key, std::string &opCode,
std::vector<swss::FieldValueTuple> &attrs)
{
SWSS_LOG_ENTER();

std::string op, data;
kcudnik marked this conversation as resolved.
Show resolved Hide resolved
std::vector<swss::FieldValueTuple> values;

m_errorNotificationConsumer->pop(op, data, values);
SWSS_LOG_DEBUG("ErrorListener op: %s data: %s", op.c_str(), data.c_str());

json j;
try
{
j = json::parse(data);
}
catch (...)
{
SWSS_LOG_ERROR("Parse error on error handling data : %s", data.c_str());
return -2;
}

// Filter the error notifications that the caller is not interested in.
if ((j["rc"] == "SWSS_RC_SUCCESS") && !(m_errorFlags & ERR_NOTIFY_POSITIVE_ACK))
{
return -1;
}
if ((j["rc"] != "SWSS_RC_SUCCESS") && !(m_errorFlags & ERR_NOTIFY_FAIL))
{
return -1;
}

key = j["key"];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

possible to throw? should we return -1?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

opCode = j["operation"];
for (json::iterator it = j.begin(); it != j.end(); ++it)
{
if(it.key() != "key" && it.key() != "operation")
{
attrs.emplace_back(it.key(), it.value());
}
}
return 0;
}
}
57 changes: 57 additions & 0 deletions common/errorlistener.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/*
* Copyright 2019 Broadcom. The term Broadcom refers to Broadcom Inc. and/or
* its subsidiaries.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#ifndef SWSS_ERRORLISTENER_H
#define SWSS_ERRORLISTENER_H

#include "notificationconsumer.h"
#include "selectable.h"
#include "table.h"
#include "dbconnector.h"

// Error notifications of interest to the error listener
typedef enum _error_notify_flags_t
{
ERR_NOTIFY_NONE,
ERR_NOTIFY_FAIL,
ERR_NOTIFY_POSITIVE_ACK
} error_notify_flags_t;

namespace swss {

class ErrorListener : public Selectable
{
public:
ErrorListener(std::string appTableName, int errFlags);
std::string getErrorChannelName(std::string& appTableName);
int getFd() { return m_errorNotificationConsumer->getFd(); }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont put code in headers, move to cpp file

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to corresponding .cpp file

uint64_t readData() { return m_errorNotificationConsumer->readData(); }
bool hasData() { return m_errorNotificationConsumer->hasData(); }
bool hasCachedData() override { return m_errorNotificationConsumer->hasCachedData(); }
bool initializedWithData() override { return m_errorNotificationConsumer->initializedWithData(); }
void updateAfterRead() override { m_errorNotificationConsumer->updateAfterRead(); }
int getError(std::string &key, std::string &opCode, std::vector<swss::FieldValueTuple> &attrs);
Comment on lines +42 to +47
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move all code to cpp file


private:
std::unique_ptr<swss::NotificationConsumer> m_errorNotificationConsumer;
std::unique_ptr<swss::DBConnector> m_errorNotificationsDb;
int m_errorFlags;
std::string m_errorChannelName;
};

}
#endif /* SWSS_ERRORLISTENER_H */
130 changes: 130 additions & 0 deletions common/errormap.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
/*
* Copyright 2019 Broadcom. The term Broadcom refers to Broadcom Inc. and/or
* its subsidiaries.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include "error.h"
#include "logger.h"
#include "errormap.h"

namespace swss {

const ErrorMap::SwssStrToRCMap ErrorMap::m_swssStrToRC = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use sai_status_t indtead of intoducing new error enum and translations ?

{ std::make_pair("SWSS_RC_SUCCESS", SWSS_RC_SUCCESS) },
{ std::make_pair("SWSS_RC_INVALID_PARAM", SWSS_RC_INVALID_PARAM) },
{ std::make_pair("SWSS_RC_UNAVAIL", SWSS_RC_UNAVAIL) },
{ std::make_pair("SWSS_RC_NOT_FOUND", SWSS_RC_NOT_FOUND) },
{ std::make_pair("SWSS_RC_NO_MEMORY", SWSS_RC_NO_MEMORY) },
{ std::make_pair("SWSS_RC_EXISTS", SWSS_RC_EXISTS) },
{ std::make_pair("SWSS_RC_TABLE_FULL", SWSS_RC_TABLE_FULL) },
{ std::make_pair("SWSS_RC_IN_USE", SWSS_RC_IN_USE) },
{ std::make_pair("SWSS_RC_NOT_IMPLEMENTED", SWSS_RC_NOT_IMPLEMENTED) },
{ std::make_pair("SWSS_RC_FAILURE", SWSS_RC_FAILURE) },
{ std::make_pair("SWSS_RC_INVALID_OBJECT_ID", SWSS_RC_INVALID_OBJECT_ID)}
};

const ErrorMap::SaiToSwssRCMap ErrorMap::m_saiToSwssRC = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

convert to std::map

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{ "SAI_STATUS_SUCCESS", "SWSS_RC_SUCCESS" },
{ "SAI_STATUS_INVALID_PARAMETER", "SWSS_RC_INVALID_PARAM" },
{ "SAI_STATUS_NOT_SUPPORTED", "SWSS_RC_UNAVAIL" },
{ "SAI_STATUS_ITEM_NOT_FOUND", "SWSS_RC_NOT_FOUND" },
{ "SAI_STATUS_NO_MEMEORY", "SWSS_RC_NO_MEMORY" },
{ "SAI_STATUS_ITEM_ALREADY_EXISTS", "SWSS_RC_EXISTS" },
{ "SAI_STATUS_TABLE_FULL", "SWSS_RC_TABLE_FULL" },
{ "SAI_STATUS_OBJECT_IN_USE", "SWSS_RC_IN_USE" },
{ "SAI_STATUS_NOT_IMPLEMENTED", "SWSS_RC_NOT_IMPLEMENTED" },
{ "SAI_STATUS_FAILURE", "SWSS_RC_FAILURE" },
{ "SAI_STATUS_INVALID_OBJECT_ID", "SWSS_RC_INVALID_OBJECT_ID" }
};

/**
* Function Description:
* @brief Get SWSS return code from SAI return code
*
* Arguments:
* @param[in] saiRCStr - SAI return code
*
* Return Values:
* @return string, SWSS return code
*/
std::string ErrorMap::getSwssRCStr(const std::string &saiRCStr)
{
std::string swssRCStr;

if (m_saiToSwssRC.find(saiRCStr) == m_saiToSwssRC.end())
{
SWSS_LOG_ERROR("Failed to map SAI error %s to SWSS error code", saiRCStr.c_str());
swssRCStr = "SWSS_RC_UNKNOWN";
}
else
{
swssRCStr = m_saiToSwssRC.at(saiRCStr);
}
return swssRCStr;
}

/**
* Function Description:
* @brief Get SWSS return code from SWSS return code string
*
* Arguments:
* @param[in] swssRCStr - SWSS return code string
*
* Return Values:
* @return SwssRC, SWSS return code
*/
ErrorMap::SwssRC ErrorMap::getSwssRC(const std::string &swssRCStr)
{
for (auto &elem : m_swssStrToRC)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use find ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (elem.first == swssRCStr)
{
return elem.second;
break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed break

}
}

/* Error mapping is not found */
SWSS_LOG_ERROR("Invalid SWSS error string %s is received", swssRCStr.c_str());
return SWSS_RC_UNKNOWN;
}

/**
* Function Description:
* @brief Get SWSS return code string from SWSS return code
*
* Arguments:
* @param[in] rc - SWSS return code
*
* Return Values:
* @return string, SWSS return code string
*/
std::string ErrorMap::getSaiRCStr(SwssRC rc)
{
for (auto &elem : m_swssStrToRC)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use find ?

{
if (elem.second == rc)
{
return elem.first;
}
}

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

return "SWSS_RC_UNKNOWN";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

};
59 changes: 59 additions & 0 deletions common/errormap.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* Copyright 2019 Broadcom. The term Broadcom refers to Broadcom Inc. and/or
* its subsidiaries.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#ifndef SWSS_COMMON_ERRORMAP_H
#define SWSS_COMMON_ERRORMAP_H

#include <string>
#include <map>
#include <vector>

namespace swss {

class ErrorMap
{
public:
enum SwssRC
{
SWSS_RC_SUCCESS,
SWSS_RC_INVALID_PARAM,
SWSS_RC_UNAVAIL,
SWSS_RC_NOT_FOUND,
SWSS_RC_NO_MEMORY,
SWSS_RC_EXISTS,
SWSS_RC_TABLE_FULL,
SWSS_RC_IN_USE,
SWSS_RC_NOT_IMPLEMENTED,
SWSS_RC_FAILURE,
SWSS_RC_INVALID_OBJECT_ID,
SWSS_RC_UNKNOWN
};

typedef std::map<std::string, std::string> SaiToSwssRCMap;
typedef std::vector<std::pair<std::string, SwssRC>> SwssStrToRCMap;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whyt this is vector and not actual map ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


static const SwssStrToRCMap m_swssStrToRC;
static const SaiToSwssRCMap m_saiToSwssRC;

static std::string getSwssRCStr(const std::string &saiRCStr);
static SwssRC getSwssRC(const std::string &swssRCStr);
static std::string getSaiRCStr(SwssRC rc);
};

}

#endif /* SWSS_COMMON_ERRORMAP_H */
Loading