From eddb06e3ce412e0fdcaf76c394c75cb545bb578f Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 29 Aug 2020 00:30:26 +0000 Subject: [PATCH 01/37] [pyext] Add more OUTPUT type --- pyext/swsscommon.i | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pyext/swsscommon.i b/pyext/swsscommon.i index 04aedaa3f..21db07826 100644 --- a/pyext/swsscommon.i +++ b/pyext/swsscommon.i @@ -64,9 +64,11 @@ swss::RedisSelect *CastSelectableToRedisSelectObj(swss::Selectable *temp) { %apply std::vector& OUTPUT {std::vector &keys}; %apply std::vector>& OUTPUT {std::vector> &ovalues}; +%apply std::string& OUTPUT {std::string &value}; %include "table.h" %clear std::vector &keys; %clear std::vector> &values; +%clear std::string &value; %include "producertable.h" %include "producerstatetable.h" From 639599d00d4dda33b1e53e2454622e94eac85674 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 29 Aug 2020 00:31:26 +0000 Subject: [PATCH 02/37] Refactor: add new class RedisConnector --- common/dbconnector.cpp | 149 +++++++++++++++++++++++++++++++---------- common/dbconnector.h | 51 ++++++++++---- common/redisreply.cpp | 8 +-- common/redisreply.h | 8 +-- 4 files changed, 161 insertions(+), 55 deletions(-) diff --git a/common/dbconnector.cpp b/common/dbconnector.cpp index 4aec7b729..f3e6ffc57 100644 --- a/common/dbconnector.cpp +++ b/common/dbconnector.cpp @@ -370,25 +370,19 @@ unordered_map> SonicDBConfig::m_db_separator; bool SonicDBConfig::m_init = false; bool SonicDBConfig::m_global_init = false; -constexpr const char *DBConnector::DEFAULT_UNIXSOCKET; +constexpr const char *RedisConnector::DEFAULT_UNIXSOCKET; -void DBConnector::select(DBConnector *db) +RedisConnector::~RedisConnector() { - string select("SELECT "); - select += to_string(db->getDbId()); - - RedisReply r(db, select, REDIS_REPLY_STATUS); - r.checkStatusOK(); + redisFree(m_conn); } -DBConnector::~DBConnector() +RedisConnector::RedisConnector() { - redisFree(m_conn); } -DBConnector::DBConnector(int dbId, const string& hostname, int port, +RedisConnector::RedisConnector(const string& hostname, int port, unsigned int timeout) : - m_dbId(dbId), m_namespace(EMPTY_NAMESPACE) { struct timeval tv = {0, (suseconds_t)timeout * 1000}; @@ -401,13 +395,9 @@ DBConnector::DBConnector(int dbId, const string& hostname, int port, if (m_conn->err) throw system_error(make_error_code(errc::address_not_available), "Unable to connect to redis"); - - select(this); } -DBConnector::DBConnector(int dbId, const string& unixPath, unsigned int timeout) : - m_dbId(dbId), - m_namespace(EMPTY_NAMESPACE) +RedisConnector::RedisConnector(const string& unixPath, unsigned int timeout) { struct timeval tv = {0, (suseconds_t)timeout * 1000}; @@ -419,36 +409,108 @@ DBConnector::DBConnector(int dbId, const string& unixPath, unsigned int timeout) if (m_conn->err) throw system_error(make_error_code(errc::address_not_available), "Unable to connect to redis (unix-socket)"); +} + +redisContext *RedisConnector::getContext() const +{ + return m_conn; +} + +void RedisConnector::setContext(redisContext *conn) +{ + m_conn = conn; +} + +string RedisConnector::getNamespace() const +{ + return m_namespace; +} + +void RedisConnector::setNamespace(const string& netns) +{ + m_namespace = netns; +} +void RedisConnector::setClientName(const string& clientName) +{ + string command("CLIENT SETNAME "); + command += clientName; + + RedisReply r(this, command, REDIS_REPLY_STATUS); + r.checkStatusOK(); +} + +string RedisConnector::getClientName() +{ + string command("CLIENT GETNAME"); + + RedisReply r(this, command); + + auto ctx = r.getContext(); + if (ctx->type == REDIS_REPLY_STRING) + { + return r.getReply(); + } + else + { + if (ctx->type != REDIS_REPLY_NIL) + SWSS_LOG_ERROR("Unable to obtain Redis client name"); + + return ""; + } +} + +void DBConnector::select(DBConnector *db) +{ + string select("SELECT "); + select += to_string(db->getDbId()); + + RedisReply r(db, select, REDIS_REPLY_STATUS); + r.checkStatusOK(); +} + +DBConnector::DBConnector(int dbId, const string& hostname, int port, + unsigned int timeout) : + RedisConnector(hostname, port, timeout), + m_dbId(dbId) +{ + select(this); +} + +DBConnector::DBConnector(int dbId, const string& unixPath, unsigned int timeout) : + RedisConnector(unixPath, timeout), + m_dbId(dbId) +{ select(this); } DBConnector::DBConnector(const string& dbName, unsigned int timeout, bool isTcpConn, const string& netns) : m_dbId(SonicDBConfig::getDbId(dbName, netns)) , m_dbName(dbName) - , m_namespace(netns) { struct timeval tv = {0, (suseconds_t)timeout * 1000}; + redisContext *conn; if (timeout) { if (isTcpConn) - m_conn = redisConnectWithTimeout(SonicDBConfig::getDbHostname(dbName, netns).c_str(), SonicDBConfig::getDbPort(dbName, netns), tv); + conn = redisConnectWithTimeout(SonicDBConfig::getDbHostname(dbName, netns).c_str(), SonicDBConfig::getDbPort(dbName, netns), tv); else - m_conn = redisConnectUnixWithTimeout(SonicDBConfig::getDbSock(dbName, netns).c_str(), tv); + conn = redisConnectUnixWithTimeout(SonicDBConfig::getDbSock(dbName, netns).c_str(), tv); } else { if (isTcpConn) - m_conn = redisConnect(SonicDBConfig::getDbHostname(dbName, netns).c_str(), SonicDBConfig::getDbPort(dbName, netns)); + conn = redisConnect(SonicDBConfig::getDbHostname(dbName, netns).c_str(), SonicDBConfig::getDbPort(dbName, netns)); else - m_conn = redisConnectUnix(SonicDBConfig::getDbSock(dbName, netns).c_str()); + conn = redisConnectUnix(SonicDBConfig::getDbSock(dbName, netns).c_str()); } - if (m_conn->err) + if (conn->err) throw system_error(make_error_code(errc::address_not_available), "Unable to connect to redis"); - + setContext(conn); + setNamespace(netns); select(this); } @@ -458,11 +520,6 @@ DBConnector::DBConnector(const string& dbName, unsigned int timeout, bool isTcpC // Empty contructor } -redisContext *DBConnector::getContext() const -{ - return m_conn; -} - int DBConnector::getDbId() const { return m_dbId; @@ -473,11 +530,6 @@ string DBConnector::getDbName() const return m_dbName; } -string DBConnector::getNamespace() const -{ - return m_namespace; -} - DBConnector *DBConnector::newConnector(unsigned int timeout) const { DBConnector *ret; @@ -493,7 +545,7 @@ DBConnector *DBConnector::newConnector(unsigned int timeout) const timeout); ret->m_dbName = m_dbName; - ret->m_namespace = m_namespace; + ret->setNamespace(getNamespace()); return ret; } @@ -688,4 +740,33 @@ shared_ptr DBConnector::blpop(const string &list, int timeout) throw runtime_error("GET failed, memory exception"); } +void DBConnector::setClientName(const string& clientName) +{ + string command("CLIENT SETNAME "); + command += clientName; + + RedisReply r(this, command, REDIS_REPLY_STATUS); + r.checkStatusOK(); +} + +string DBConnector::getClientName() +{ + string command("CLIENT GETNAME"); + + RedisReply r(this, command); + + auto ctx = r.getContext(); + if (ctx->type == REDIS_REPLY_STRING) + { + return r.getReply(); + } + else + { + if (ctx->type != REDIS_REPLY_NIL) + SWSS_LOG_ERROR("Unable to obtain Redis client name"); + + return ""; + } +} + } diff --git a/common/dbconnector.h b/common/dbconnector.h index 84859d3ec..3360fdf20 100644 --- a/common/dbconnector.h +++ b/common/dbconnector.h @@ -69,7 +69,7 @@ class SonicDBConfig static RedisInstInfo& getRedisInfo(const std::string &dbName, const std::string &netns = EMPTY_NAMESPACE); }; -class DBConnector +class RedisConnector { public: static constexpr const char *DEFAULT_UNIXSOCKET = "/var/run/redis/redis.sock"; @@ -81,22 +81,14 @@ class DBConnector * Timeout - The time in milisecond until exception is been thrown. For * infinite wait, set this value to 0 */ - DBConnector(int dbId, const std::string &hostname, int port, unsigned int timeout); - DBConnector(int dbId, const std::string &unixPath, unsigned int timeout); - DBConnector(const std::string &dbName, unsigned int timeout, bool isTcpConn = false); - DBConnector(const std::string &dbName, unsigned int timeout, bool isTcpConn, const std::string &netns); + RedisConnector(const std::string &hostname, int port, unsigned int timeout); + RedisConnector(const std::string &unixPath, unsigned int timeout); - ~DBConnector(); + ~RedisConnector(); redisContext *getContext() const; - int getDbId() const; - std::string getDbName() const; std::string getNamespace() const; - static void select(DBConnector *db); - - /* Create new context to DB */ - DBConnector *newConnector(unsigned int timeout) const; /* * Assign a name to the Redis client used for this connection @@ -140,11 +132,44 @@ class DBConnector std::shared_ptr blpop(const std::string &list, int timeout); +protected: + RedisConnector(); + void setContext(redisContext *conn); + void setNamespace(const std::string &netns); + private: redisContext *m_conn; + std::string m_namespace; +}; + +class DBConnector : public RedisConnector +{ +public: + static constexpr const char *DEFAULT_UNIXSOCKET = "/var/run/redis/redis.sock"; + + /* + * Connect to Redis DB wither with a hostname:port or unix socket + * Select the database index provided by "db" + * + * Timeout - The time in milisecond until exception is been thrown. For + * infinite wait, set this value to 0 + */ + DBConnector(int dbId, const std::string &hostname, int port, unsigned int timeout); + DBConnector(int dbId, const std::string &unixPath, unsigned int timeout); + DBConnector(const std::string &dbName, unsigned int timeout, bool isTcpConn = false); + DBConnector(const std::string &dbName, unsigned int timeout, bool isTcpConn, const std::string &netns); + + int getDbId() const; + std::string getDbName() const; + + static void select(DBConnector *db); + + /* Create new context to DB */ + DBConnector *newConnector(unsigned int timeout) const; + +private: int m_dbId; std::string m_dbName; - std::string m_namespace; }; template diff --git a/common/redisreply.cpp b/common/redisreply.cpp index 81ed92f9f..14cc02806 100644 --- a/common/redisreply.cpp +++ b/common/redisreply.cpp @@ -28,7 +28,7 @@ inline void guard(FUNC func, const char* command) } } -RedisReply::RedisReply(DBConnector *db, const RedisCommand& command) +RedisReply::RedisReply(RedisConnector *db, const RedisCommand& command) { int rc = redisAppendFormattedCommand(db->getContext(), command.c_str(), command.length()); if (rc != REDIS_OK) @@ -46,7 +46,7 @@ RedisReply::RedisReply(DBConnector *db, const RedisCommand& command) guard([&]{checkReply();}, command.c_str()); } -RedisReply::RedisReply(DBConnector *db, const string &command) +RedisReply::RedisReply(RedisConnector *db, const string &command) { int rc = redisAppendCommand(db->getContext(), command.c_str()); if (rc != REDIS_OK) @@ -64,13 +64,13 @@ RedisReply::RedisReply(DBConnector *db, const string &command) guard([&]{checkReply();}, command.c_str()); } -RedisReply::RedisReply(DBConnector *db, const RedisCommand& command, int expectedType) +RedisReply::RedisReply(RedisConnector *db, const RedisCommand& command, int expectedType) : RedisReply(db, command) { guard([&]{checkReplyType(expectedType);}, command.c_str()); } -RedisReply::RedisReply(DBConnector *db, const string &command, int expectedType) +RedisReply::RedisReply(RedisConnector *db, const string &command, int expectedType) : RedisReply(db, command) { guard([&]{checkReplyType(expectedType);}, command.c_str()); diff --git a/common/redisreply.h b/common/redisreply.h index 33b5b0d12..750b7edd5 100644 --- a/common/redisreply.h +++ b/common/redisreply.h @@ -40,8 +40,8 @@ class RedisReply * Send a new command to redis and wait for reply * No reply type specified. */ - RedisReply(DBConnector *db, const RedisCommand& command); - RedisReply(DBConnector *db, const std::string &command); + RedisReply(RedisConnector *db, const RedisCommand& command); + RedisReply(RedisConnector *db, const std::string &command); /* * Send a new command to redis and waits for reply * The reply must be one of REDIS_REPLY_* format (e.g. REDIS_REPLY_STATUS, @@ -49,8 +49,8 @@ class RedisReply * isFormatted - Set to true if the command is already formatted in redis * protocol */ - RedisReply(DBConnector *db, const RedisCommand& command, int expectedType); - RedisReply(DBConnector *db, const std::string &command, int expectedType); + RedisReply(RedisConnector *db, const RedisCommand& command, int expectedType); + RedisReply(RedisConnector *db, const std::string &command, int expectedType); /* auto_ptr for native structue (Free the memory on destructor) */ RedisReply(redisReply *reply); From 27dc47661129a3259b4c04d0e9619e7214e14674 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 29 Aug 2020 02:03:04 +0000 Subject: [PATCH 03/37] dbconnector: remove emtpy line --- common/dbconnector.h | 1 - 1 file changed, 1 deletion(-) diff --git a/common/dbconnector.h b/common/dbconnector.h index 3360fdf20..9055e339b 100644 --- a/common/dbconnector.h +++ b/common/dbconnector.h @@ -89,7 +89,6 @@ class RedisConnector redisContext *getContext() const; std::string getNamespace() const; - /* * Assign a name to the Redis client used for this connection * This is helpful when debugging Redis clients using `redis-cli client list` From 9341111d9e7ad289cedbb0ee41c68016f4d47fac Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 29 Aug 2020 01:09:42 +0000 Subject: [PATCH 04/37] Refine script functions parameter --- common/redisapi.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/common/redisapi.h b/common/redisapi.h index d8a7c78af..0b759ab27 100644 --- a/common/redisapi.h +++ b/common/redisapi.h @@ -8,6 +8,8 @@ #include #include "logger.h" #include "rediscommand.h" +#include "redisreply.h" +#include "dbconnector.h" #ifdef HAVE_CONFIG_H #include @@ -15,7 +17,7 @@ namespace swss { -static inline std::string loadRedisScript(DBConnector* db, const std::string& script) +static inline std::string loadRedisScript(RedisConnector* db, const std::string& script) { SWSS_LOG_ENTER(); @@ -64,7 +66,7 @@ static inline std::string loadLuaScript(const std::string& path) return readTextFile("/usr/share/swss/" + path); } -static inline std::set runRedisScript(DBConnector &db, const std::string& sha, +static inline std::set runRedisScript(RedisConnector &db, const std::string& sha, const std::vector& keys, const std::vector& argv) { SWSS_LOG_ENTER(); From ee3a60945a2a4611dd3d5ca63feabeb07be4ea48 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Wed, 2 Sep 2020 22:11:17 +0000 Subject: [PATCH 05/37] Add copy constructor to RedisConnector --- common/dbconnector.cpp | 38 +++++++++++++++++++++++++++----------- common/dbconnector.h | 3 +++ 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/common/dbconnector.cpp b/common/dbconnector.cpp index f3e6ffc57..6c21e65ee 100644 --- a/common/dbconnector.cpp +++ b/common/dbconnector.cpp @@ -381,30 +381,46 @@ RedisConnector::RedisConnector() { } +RedisConnector::RedisConnector(const RedisConnector &other) +{ + auto octx = other.getContext(); + const char *unixPath = octx->unix_sock.path; + if (unixPath) + { + initContext(unixPath, *octx->timeout); + } + else + { + initContext(octx->tcp.host, octx->tcp.port, *octx->timeout); + } +} + RedisConnector::RedisConnector(const string& hostname, int port, unsigned int timeout) : m_namespace(EMPTY_NAMESPACE) { struct timeval tv = {0, (suseconds_t)timeout * 1000}; + initContext(hostname.c_str(), port, tv); +} - if (timeout) - m_conn = redisConnectWithTimeout(hostname.c_str(), port, tv); - else - m_conn = redisConnect(hostname.c_str(), port); +RedisConnector::RedisConnector(const string& unixPath, unsigned int timeout) +{ + struct timeval tv = {0, (suseconds_t)timeout * 1000}; + initContext(unixPath.c_str(), tv); +} + +void RedisConnector::initContext(const char *host, int port, const timeval& tv) +{ + m_conn = redisConnectWithTimeout(host, port, tv); if (m_conn->err) throw system_error(make_error_code(errc::address_not_available), "Unable to connect to redis"); } -RedisConnector::RedisConnector(const string& unixPath, unsigned int timeout) +void RedisConnector::initContext(const char *path, const timeval &tv) { - struct timeval tv = {0, (suseconds_t)timeout * 1000}; - - if (timeout) - m_conn = redisConnectUnixWithTimeout(unixPath.c_str(), tv); - else - m_conn = redisConnectUnix(unixPath.c_str()); + m_conn = redisConnectUnixWithTimeout(path, tv); if (m_conn->err) throw system_error(make_error_code(errc::address_not_available), diff --git a/common/dbconnector.h b/common/dbconnector.h index 9055e339b..8a3ab78f1 100644 --- a/common/dbconnector.h +++ b/common/dbconnector.h @@ -83,6 +83,7 @@ class RedisConnector */ RedisConnector(const std::string &hostname, int port, unsigned int timeout); RedisConnector(const std::string &unixPath, unsigned int timeout); + RedisConnector(const RedisConnector &other); ~RedisConnector(); @@ -133,6 +134,8 @@ class RedisConnector protected: RedisConnector(); + void initContext(const char *host, int port, const timeval& tv); + void initContext(const char *path, const timeval &tv); void setContext(redisContext *conn); void setNamespace(const std::string &netns); From c358b24d9c67b8a3c505d61a8b208a75bc37b42c Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Wed, 2 Sep 2020 22:43:38 +0000 Subject: [PATCH 06/37] Optimize DBConnector ctor --- common/dbconnector.cpp | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/common/dbconnector.cpp b/common/dbconnector.cpp index 6c21e65ee..2300f5356 100644 --- a/common/dbconnector.cpp +++ b/common/dbconnector.cpp @@ -506,26 +506,15 @@ DBConnector::DBConnector(const string& dbName, unsigned int timeout, bool isTcpC { struct timeval tv = {0, (suseconds_t)timeout * 1000}; - redisContext *conn; - if (timeout) + if (isTcpConn) { - if (isTcpConn) - conn = redisConnectWithTimeout(SonicDBConfig::getDbHostname(dbName, netns).c_str(), SonicDBConfig::getDbPort(dbName, netns), tv); - else - conn = redisConnectUnixWithTimeout(SonicDBConfig::getDbSock(dbName, netns).c_str(), tv); + initContext(SonicDBConfig::getDbHostname(dbName, netns).c_str(), SonicDBConfig::getDbPort(dbName, netns), tv); } else { - if (isTcpConn) - conn = redisConnect(SonicDBConfig::getDbHostname(dbName, netns).c_str(), SonicDBConfig::getDbPort(dbName, netns)); - else - conn = redisConnectUnix(SonicDBConfig::getDbSock(dbName, netns).c_str()); + initContext(SonicDBConfig::getDbSock(dbName, netns).c_str(), tv); } - if (conn->err) - throw system_error(make_error_code(errc::address_not_available), - "Unable to connect to redis"); - setContext(conn); setNamespace(netns); select(this); } From eab9a9b6084e003820ea4c123724824c10f3797b Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Wed, 2 Sep 2020 23:27:04 +0000 Subject: [PATCH 07/37] Revert back m_namespace Signed-off-by: Qi Luo --- common/dbconnector.cpp | 31 ++++++++++++++++--------------- common/dbconnector.h | 7 ++++--- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/common/dbconnector.cpp b/common/dbconnector.cpp index 2300f5356..4113529cf 100644 --- a/common/dbconnector.cpp +++ b/common/dbconnector.cpp @@ -396,8 +396,7 @@ RedisConnector::RedisConnector(const RedisConnector &other) } RedisConnector::RedisConnector(const string& hostname, int port, - unsigned int timeout) : - m_namespace(EMPTY_NAMESPACE) + unsigned int timeout) { struct timeval tv = {0, (suseconds_t)timeout * 1000}; initContext(hostname.c_str(), port, tv); @@ -437,16 +436,6 @@ void RedisConnector::setContext(redisContext *conn) m_conn = conn; } -string RedisConnector::getNamespace() const -{ - return m_namespace; -} - -void RedisConnector::setNamespace(const string& netns) -{ - m_namespace = netns; -} - void RedisConnector::setClientName(const string& clientName) { string command("CLIENT SETNAME "); @@ -488,14 +477,16 @@ void DBConnector::select(DBConnector *db) DBConnector::DBConnector(int dbId, const string& hostname, int port, unsigned int timeout) : RedisConnector(hostname, port, timeout), - m_dbId(dbId) + m_dbId(dbId), + m_namespace(EMPTY_NAMESPACE) { select(this); } DBConnector::DBConnector(int dbId, const string& unixPath, unsigned int timeout) : RedisConnector(unixPath, timeout), - m_dbId(dbId) + m_dbId(dbId), + m_namespace(EMPTY_NAMESPACE) { select(this); } @@ -503,6 +494,7 @@ DBConnector::DBConnector(int dbId, const string& unixPath, unsigned int timeout) DBConnector::DBConnector(const string& dbName, unsigned int timeout, bool isTcpConn, const string& netns) : m_dbId(SonicDBConfig::getDbId(dbName, netns)) , m_dbName(dbName) + , m_namespace(netns) { struct timeval tv = {0, (suseconds_t)timeout * 1000}; @@ -515,7 +507,6 @@ DBConnector::DBConnector(const string& dbName, unsigned int timeout, bool isTcpC initContext(SonicDBConfig::getDbSock(dbName, netns).c_str(), tv); } - setNamespace(netns); select(this); } @@ -535,6 +526,16 @@ string DBConnector::getDbName() const return m_dbName; } +void DBConnector::setNamespace(const string& netns) +{ + m_namespace = netns; +} + +string DBConnector::getNamespace() const +{ + return m_namespace; +} + DBConnector *DBConnector::newConnector(unsigned int timeout) const { DBConnector *ret; diff --git a/common/dbconnector.h b/common/dbconnector.h index 8a3ab78f1..9a9bbc78a 100644 --- a/common/dbconnector.h +++ b/common/dbconnector.h @@ -88,7 +88,6 @@ class RedisConnector ~RedisConnector(); redisContext *getContext() const; - std::string getNamespace() const; /* * Assign a name to the Redis client used for this connection @@ -137,11 +136,9 @@ class RedisConnector void initContext(const char *host, int port, const timeval& tv); void initContext(const char *path, const timeval &tv); void setContext(redisContext *conn); - void setNamespace(const std::string &netns); private: redisContext *m_conn; - std::string m_namespace; }; class DBConnector : public RedisConnector @@ -163,6 +160,7 @@ class DBConnector : public RedisConnector int getDbId() const; std::string getDbName() const; + std::string getNamespace() const; static void select(DBConnector *db); @@ -170,8 +168,11 @@ class DBConnector : public RedisConnector DBConnector *newConnector(unsigned int timeout) const; private: + void setNamespace(const std::string &netns); + int m_dbId; std::string m_dbName; + std::string m_namespace; }; template From 89bd455201ef57f0e51bc1803084e54ca0108e27 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Wed, 2 Sep 2020 23:54:00 +0000 Subject: [PATCH 08/37] Refactor: change name --- common/dbconnector.cpp | 30 +++++++++++++++--------------- common/dbconnector.h | 16 ++++++++-------- common/redisapi.h | 24 ++++++++++++------------ common/redisreply.cpp | 24 ++++++++++++------------ common/redisreply.h | 8 ++++---- 5 files changed, 51 insertions(+), 51 deletions(-) diff --git a/common/dbconnector.cpp b/common/dbconnector.cpp index 4113529cf..8661639bf 100644 --- a/common/dbconnector.cpp +++ b/common/dbconnector.cpp @@ -370,18 +370,18 @@ unordered_map> SonicDBConfig::m_db_separator; bool SonicDBConfig::m_init = false; bool SonicDBConfig::m_global_init = false; -constexpr const char *RedisConnector::DEFAULT_UNIXSOCKET; +constexpr const char *RedisContext::DEFAULT_UNIXSOCKET; -RedisConnector::~RedisConnector() +RedisContext::~RedisContext() { redisFree(m_conn); } -RedisConnector::RedisConnector() +RedisContext::RedisContext() { } -RedisConnector::RedisConnector(const RedisConnector &other) +RedisContext::RedisContext(const RedisContext &other) { auto octx = other.getContext(); const char *unixPath = octx->unix_sock.path; @@ -395,20 +395,20 @@ RedisConnector::RedisConnector(const RedisConnector &other) } } -RedisConnector::RedisConnector(const string& hostname, int port, +RedisContext::RedisContext(const string& hostname, int port, unsigned int timeout) { struct timeval tv = {0, (suseconds_t)timeout * 1000}; initContext(hostname.c_str(), port, tv); } -RedisConnector::RedisConnector(const string& unixPath, unsigned int timeout) +RedisContext::RedisContext(const string& unixPath, unsigned int timeout) { struct timeval tv = {0, (suseconds_t)timeout * 1000}; initContext(unixPath.c_str(), tv); } -void RedisConnector::initContext(const char *host, int port, const timeval& tv) +void RedisContext::initContext(const char *host, int port, const timeval& tv) { m_conn = redisConnectWithTimeout(host, port, tv); @@ -417,7 +417,7 @@ void RedisConnector::initContext(const char *host, int port, const timeval& tv) "Unable to connect to redis"); } -void RedisConnector::initContext(const char *path, const timeval &tv) +void RedisContext::initContext(const char *path, const timeval &tv) { m_conn = redisConnectUnixWithTimeout(path, tv); @@ -426,17 +426,17 @@ void RedisConnector::initContext(const char *path, const timeval &tv) "Unable to connect to redis (unix-socket)"); } -redisContext *RedisConnector::getContext() const +redisContext *RedisContext::getContext() const { return m_conn; } -void RedisConnector::setContext(redisContext *conn) +void RedisContext::setContext(redisContext *ctx) { - m_conn = conn; + m_conn = ctx; } -void RedisConnector::setClientName(const string& clientName) +void RedisContext::setClientName(const string& clientName) { string command("CLIENT SETNAME "); command += clientName; @@ -445,7 +445,7 @@ void RedisConnector::setClientName(const string& clientName) r.checkStatusOK(); } -string RedisConnector::getClientName() +string RedisContext::getClientName() { string command("CLIENT GETNAME"); @@ -476,7 +476,7 @@ void DBConnector::select(DBConnector *db) DBConnector::DBConnector(int dbId, const string& hostname, int port, unsigned int timeout) : - RedisConnector(hostname, port, timeout), + RedisContext(hostname, port, timeout), m_dbId(dbId), m_namespace(EMPTY_NAMESPACE) { @@ -484,7 +484,7 @@ DBConnector::DBConnector(int dbId, const string& hostname, int port, } DBConnector::DBConnector(int dbId, const string& unixPath, unsigned int timeout) : - RedisConnector(unixPath, timeout), + RedisContext(unixPath, timeout), m_dbId(dbId), m_namespace(EMPTY_NAMESPACE) { diff --git a/common/dbconnector.h b/common/dbconnector.h index 9a9bbc78a..57767b492 100644 --- a/common/dbconnector.h +++ b/common/dbconnector.h @@ -69,7 +69,7 @@ class SonicDBConfig static RedisInstInfo& getRedisInfo(const std::string &dbName, const std::string &netns = EMPTY_NAMESPACE); }; -class RedisConnector +class RedisContext { public: static constexpr const char *DEFAULT_UNIXSOCKET = "/var/run/redis/redis.sock"; @@ -81,11 +81,11 @@ class RedisConnector * Timeout - The time in milisecond until exception is been thrown. For * infinite wait, set this value to 0 */ - RedisConnector(const std::string &hostname, int port, unsigned int timeout); - RedisConnector(const std::string &unixPath, unsigned int timeout); - RedisConnector(const RedisConnector &other); + RedisContext(const std::string &hostname, int port, unsigned int timeout); + RedisContext(const std::string &unixPath, unsigned int timeout); + RedisContext(const RedisContext &other); - ~RedisConnector(); + ~RedisContext(); redisContext *getContext() const; @@ -132,16 +132,16 @@ class RedisConnector std::shared_ptr blpop(const std::string &list, int timeout); protected: - RedisConnector(); + RedisContext(); void initContext(const char *host, int port, const timeval& tv); void initContext(const char *path, const timeval &tv); - void setContext(redisContext *conn); + void setContext(redisContext *ctx); private: redisContext *m_conn; }; -class DBConnector : public RedisConnector +class DBConnector : public RedisContext { public: static constexpr const char *DEFAULT_UNIXSOCKET = "/var/run/redis/redis.sock"; diff --git a/common/redisapi.h b/common/redisapi.h index 0b759ab27..720c19c5a 100644 --- a/common/redisapi.h +++ b/common/redisapi.h @@ -17,13 +17,13 @@ namespace swss { -static inline std::string loadRedisScript(RedisConnector* db, const std::string& script) +static inline std::string loadRedisScript(RedisContext* ctx, const std::string& script) { SWSS_LOG_ENTER(); RedisCommand loadcmd; loadcmd.format("SCRIPT LOAD %s", script.c_str()); - RedisReply r(db, loadcmd, REDIS_REPLY_STRING); + RedisReply r(ctx, loadcmd, REDIS_REPLY_STRING); std::string sha = r.getReply(); @@ -66,7 +66,7 @@ static inline std::string loadLuaScript(const std::string& path) return readTextFile("/usr/share/swss/" + path); } -static inline std::set runRedisScript(RedisConnector &db, const std::string& sha, +static inline std::set runRedisScript(RedisContext &ctx, const std::string& sha, const std::vector& keys, const std::vector& argv) { SWSS_LOG_ENTER(); @@ -97,24 +97,24 @@ static inline std::set runRedisScript(RedisConnector &db, const std std::set ret; try { - RedisReply r(&db, command); - auto ctx = r.getContext(); + RedisReply r(&ctx, command); + auto reply = r.getContext(); SWSS_LOG_DEBUG("Running lua script %s", sha.c_str()); - if (ctx->type == REDIS_REPLY_NIL) + if (reply->type == REDIS_REPLY_NIL) { - SWSS_LOG_ERROR("Got EMPTY response type from redis %d", ctx->type); + SWSS_LOG_ERROR("Got EMPTY response type from redis %d", reply->type); } - else if (ctx->type != REDIS_REPLY_ARRAY) + else if (reply->type != REDIS_REPLY_ARRAY) { - SWSS_LOG_ERROR("Got invalid response type from redis %d", ctx->type); + SWSS_LOG_ERROR("Got invalid response type from redis %d", reply->type); } else { - for (size_t i = 0; i < ctx->elements; i++) + for (size_t i = 0; i < reply->elements; i++) { - SWSS_LOG_DEBUG("Got element %zu %s", i, ctx->element[i]->str); - ret.emplace(ctx->element[i]->str); + SWSS_LOG_DEBUG("Got element %zu %s", i, reply->element[i]->str); + ret.emplace(reply->element[i]->str); } } } diff --git a/common/redisreply.cpp b/common/redisreply.cpp index 14cc02806..33e397b85 100644 --- a/common/redisreply.cpp +++ b/common/redisreply.cpp @@ -28,9 +28,9 @@ inline void guard(FUNC func, const char* command) } } -RedisReply::RedisReply(RedisConnector *db, const RedisCommand& command) +RedisReply::RedisReply(RedisContext *ctx, const RedisCommand& command) { - int rc = redisAppendFormattedCommand(db->getContext(), command.c_str(), command.length()); + int rc = redisAppendFormattedCommand(ctx->getContext(), command.c_str(), command.length()); if (rc != REDIS_OK) { // The only reason of error is REDIS_ERR_OOM (Out of memory) @@ -38,17 +38,17 @@ RedisReply::RedisReply(RedisConnector *db, const RedisCommand& command) throw bad_alloc(); } - rc = redisGetReply(db->getContext(), (void**)&m_reply); + rc = redisGetReply(ctx->getContext(), (void**)&m_reply); if (rc != REDIS_OK) { - throw RedisError("Failed to redisGetReply with " + string(command.c_str()), db->getContext()); + throw RedisError("Failed to redisGetReply with " + string(command.c_str()), ctx->getContext()); } guard([&]{checkReply();}, command.c_str()); } -RedisReply::RedisReply(RedisConnector *db, const string &command) +RedisReply::RedisReply(RedisContext *ctx, const string &command) { - int rc = redisAppendCommand(db->getContext(), command.c_str()); + int rc = redisAppendCommand(ctx->getContext(), command.c_str()); if (rc != REDIS_OK) { // The only reason of error is REDIS_ERR_OOM (Out of memory) @@ -56,22 +56,22 @@ RedisReply::RedisReply(RedisConnector *db, const string &command) throw bad_alloc(); } - rc = redisGetReply(db->getContext(), (void**)&m_reply); + rc = redisGetReply(ctx->getContext(), (void**)&m_reply); if (rc != REDIS_OK) { - throw RedisError("Failed to redisGetReply with " + command, db->getContext()); + throw RedisError("Failed to redisGetReply with " + command, ctx->getContext()); } guard([&]{checkReply();}, command.c_str()); } -RedisReply::RedisReply(RedisConnector *db, const RedisCommand& command, int expectedType) - : RedisReply(db, command) +RedisReply::RedisReply(RedisContext *ctx, const RedisCommand& command, int expectedType) + : RedisReply(ctx, command) { guard([&]{checkReplyType(expectedType);}, command.c_str()); } -RedisReply::RedisReply(RedisConnector *db, const string &command, int expectedType) - : RedisReply(db, command) +RedisReply::RedisReply(RedisContext *ctx, const string &command, int expectedType) + : RedisReply(ctx, command) { guard([&]{checkReplyType(expectedType);}, command.c_str()); } diff --git a/common/redisreply.h b/common/redisreply.h index 750b7edd5..f613f31c7 100644 --- a/common/redisreply.h +++ b/common/redisreply.h @@ -40,8 +40,8 @@ class RedisReply * Send a new command to redis and wait for reply * No reply type specified. */ - RedisReply(RedisConnector *db, const RedisCommand& command); - RedisReply(RedisConnector *db, const std::string &command); + RedisReply(RedisContext *ctx, const RedisCommand& command); + RedisReply(RedisContext *ctx, const std::string &command); /* * Send a new command to redis and waits for reply * The reply must be one of REDIS_REPLY_* format (e.g. REDIS_REPLY_STATUS, @@ -49,8 +49,8 @@ class RedisReply * isFormatted - Set to true if the command is already formatted in redis * protocol */ - RedisReply(RedisConnector *db, const RedisCommand& command, int expectedType); - RedisReply(RedisConnector *db, const std::string &command, int expectedType); + RedisReply(RedisContext *ctx, const RedisCommand& command, int expectedType); + RedisReply(RedisContext *ctx, const std::string &command, int expectedType); /* auto_ptr for native structue (Free the memory on destructor) */ RedisReply(redisReply *reply); From f11bca4eb9eaba207dd96a933ea28f2d48724d74 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Fri, 11 Sep 2020 22:30:34 +0000 Subject: [PATCH 09/37] Add new ctor for DBConnector from RedisContext and dbId --- common/dbconnector.cpp | 8 ++++++++ common/dbconnector.h | 1 + 2 files changed, 9 insertions(+) diff --git a/common/dbconnector.cpp b/common/dbconnector.cpp index 8661639bf..66a6e5d09 100644 --- a/common/dbconnector.cpp +++ b/common/dbconnector.cpp @@ -474,6 +474,14 @@ void DBConnector::select(DBConnector *db) r.checkStatusOK(); } +DBConnector::DBConnector(int dbId, const RedisContext& ctx) + : RedisContext(ctx) + , m_dbId(dbId) + , m_namespace(EMPTY_NAMESPACE) +{ + select(this); +} + DBConnector::DBConnector(int dbId, const string& hostname, int port, unsigned int timeout) : RedisContext(hostname, port, timeout), diff --git a/common/dbconnector.h b/common/dbconnector.h index 57767b492..981a0109b 100644 --- a/common/dbconnector.h +++ b/common/dbconnector.h @@ -153,6 +153,7 @@ class DBConnector : public RedisContext * Timeout - The time in milisecond until exception is been thrown. For * infinite wait, set this value to 0 */ + DBConnector(int dbId, const RedisContext &ctx); DBConnector(int dbId, const std::string &hostname, int port, unsigned int timeout); DBConnector(int dbId, const std::string &unixPath, unsigned int timeout); DBConnector(const std::string &dbName, unsigned int timeout, bool isTcpConn = false); From 5f4e3da53b2ed8f8a4129b90b9e9d4435737880a Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Fri, 11 Sep 2020 22:31:28 +0000 Subject: [PATCH 10/37] (ongoing) Add DBInterface class --- common/dbinterface.h | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 common/dbinterface.h diff --git a/common/dbinterface.h b/common/dbinterface.h new file mode 100644 index 000000000..f8511deaf --- /dev/null +++ b/common/dbinterface.h @@ -0,0 +1,34 @@ +#include + +#include "dbconnector.h" +#include "redisclient.h" + +namespace swss +{ + +class DBInterface : public RedisContext +{ +public: + void connnect(int dbId, bool retry = true) + { + if (retry) + { + throw std::invalid_argument("retry"); + } + else + { + m_dbs.emplace(std::piecewise_construct + , std::forward_as_tuple(dbId) + , std::forward_as_tuple(dbId, *this)); + } + } + + void close(int dbId); + DBConnector *at(int dbId); + +private: + std::unordered_map m_dbs; + std::unordered_map m_redisClients; +}; + +} From f731fcf16838159f395b951492e1ca120ddb3f06 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 12 Sep 2020 05:23:50 +0000 Subject: [PATCH 11/37] (temp, not build) Signed-off-by: Qi Luo --- common/dbconnector.cpp | 5 + common/dbinterface.h | 239 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 241 insertions(+), 3 deletions(-) diff --git a/common/dbconnector.cpp b/common/dbconnector.cpp index 66a6e5d09..513f4a95f 100644 --- a/common/dbconnector.cpp +++ b/common/dbconnector.cpp @@ -784,3 +784,8 @@ string DBConnector::getClientName() } } + + +/////////////////// TEST code, do not checkin +#include "common/dbinterface.h" + diff --git a/common/dbinterface.h b/common/dbinterface.h index f8511deaf..3813ecd56 100644 --- a/common/dbinterface.h +++ b/common/dbinterface.h @@ -1,11 +1,31 @@ +#include #include #include "dbconnector.h" #include "redisclient.h" +#include "logger.h" namespace swss { +class UnavailableDataError : public std::runtime_error +{ +public: + UnavailableDataError(const std::string& message, const std::string& data) + : std::runtime_error(message) + , m_data(data) + { + } + + const char *getData() const + { + return m_data.c_str(); + } + +private: + const std::string m_data; +}; + class DBInterface : public RedisContext { public: @@ -13,20 +33,233 @@ class DBInterface : public RedisContext { if (retry) { - throw std::invalid_argument("retry"); + throw std::logic_error("retry not implemented"); } else { - m_dbs.emplace(std::piecewise_construct + auto rc = m_dbs.emplace(std::piecewise_construct , std::forward_as_tuple(dbId) , std::forward_as_tuple(dbId, *this)); + bool inserted = rc.second; + if (inserted) + { + DBConnector *db = &rc.first->second; + m_redisClients.emplace(std::piecewise_construct + , std::forward_as_tuple(dbId) + , std::forward_as_tuple(db)); + return; + } + } + } + + void close(int dbId) + { + m_dbs.erase(dbId); + m_redisClients.erase(dbId); + } + + int64_t del(int dbId, const std::string& key) + { + return m_redisClients.at(dbId).del(key); + } + + bool exists(int dbId, const std::string& key) + { + return m_redisClients.at(dbId).exists(key); + } + + std::string get(int dbId, const std::string& hash, const std::string& key) + { + auto pvalue = m_redisClients.at(dbId).hget(hash, key); + if (!pvalue) + { + std::string message = "Key '" + hash + "' field '" + key + "' unavailable in database '" + std::to_string(dbId) + "'"; + SWSS_LOG_WARN("%s", message.c_str()); + throw UnavailableDataError(message, hash); + } + const std::string& value = *pvalue; + return value == "None" ? "" : value; + } + + std::map get_all(int dbId, const std::string& hash) + { + std::map map; + m_redisClients.at(dbId).hgetall(hash, std::inserter(map, map.end())); + + if (map.empty()) + { + std::string message = "Key '{" + hash + "}' unavailable in database '{" + std::to_string(dbId) + "}'"; + SWSS_LOG_WARN("%s", message.c_str()); + throw UnavailableDataError(message, hash); + } + for (auto& i : map) + { + std::string& value = i.second; + if (value == "None") + { + value = ""; + } + } + + return map; + } + + std::vector keys(int dbId, const std::string& pattern = "*", bool blocking = false) + { + auto keys = m_redisClients.at(dbId).keys(pattern); + if (keys.empty()) + { + std::string message = "DB '{" + std::to_string(dbId) + "}' is empty!"; + SWSS_LOG_WARN("%s", message.c_str()); + throw UnavailableDataError(message, "keys"); } + return keys; + } + + int64_t publish(int dbId, const std::string& channel, const std::string& message) + { + throw std::logic_error("Not implemented"); + } + + int64_t set(int dbId, const std::string& hash, const std::string& key, const std::string& value) + { + m_redisClients.at(dbId).hset(hash, key, value); + // Return the number of fields that were added. + return 1; } - void close(int dbId); DBConnector *at(int dbId); private: + template + T blockable(FUNC f, int db_id, bool blocking = false) + { + int attempts = 0; + for (;;) + { + try + { + T ret_data = f(db_id); + _unsubscribe_keyspace_notification(db_id); + return ret_data; + } + catch (const UnavailableDataError& e) + { + if (blocking) + { + auto found = keyspace_notification_channels.find(db_id); + if (found != keyspace_notification_channels.end()) + { + bool result = _unavailable_data_handler(db_id, e.getData()); + if (result) + { + continue; // received updates, try to read data again + } + else + { + _unsubscribe_keyspace_notification(db_id); + throw; // No updates was received. Raise exception + } + } + else + { + // Subscribe to updates and try it again (avoiding race condition) + _subscribe_keyspace_notification(db_id); + } + } + else + { + return T(); + } + } + catch (const std::system_error&) + { + /* + Something is fundamentally wrong with the request itself. + Retrying the request won't pass unless the schema itself changes. In this case, the error + should be attributed to the application itself. Re-raise the error. + */ + SWSS_LOG_ERROR("Bad DB request [%d]", db_id); + throw; + } + catch (const RedisError&) + { + // Redis connection broken and we need to retry several times + attempts += 1; + _connection_error_handler(db_id); + std::string msg = "DB access failure by [" + std::to_string(db_id) + + "]"; + if (BLOCKING_ATTEMPT_ERROR_THRESHOLD < attempts && attempts < BLOCKING_ATTEMPT_SUPPRESSION) + { + // Repeated access failures implies the database itself is unhealthy. + SWSS_LOG_ERROR("%s", msg.c_str()); + } + else + { + SWSS_LOG_WARN("%s", msg.c_str()); + } + } + } + } + + void _unsubscribe_keyspace_notification(int dbId) + { + throw std::logic_error("Not implemented"); + } + + bool _unavailable_data_handler(int dbId, const char *data) + { + throw std::logic_error("Not implemented"); + } + + void _subscribe_keyspace_notification(int dbId) + { + throw std::logic_error("Not implemented"); + } + + void _connection_error_handler(int dbId) + { + throw std::logic_error("Not implemented"); + } + + static const int BLOCKING_ATTEMPT_ERROR_THRESHOLD = 10; + static const int BLOCKING_ATTEMPT_SUPPRESSION = BLOCKING_ATTEMPT_ERROR_THRESHOLD + 5; + + // Wait period in seconds before attempting to reconnect to Redis. + static const int CONNECT_RETRY_WAIT_TIME = 10; + + // Wait period in seconds to wait before attempting to retrieve missing data. + static const int DATA_RETRIEVAL_WAIT_TIME = 3; + + // Time to wait for any given message to arrive via pub-sub. + static constexpr double PUB_SUB_NOTIFICATION_TIMEOUT = 10.0; // seconds + + // Maximum allowable time to wait on a specific pub-sub notification. + static constexpr double PUB_SUB_MAXIMUM_DATA_WAIT = 60.0; // seconds + + // Pub-sub keyspace pattern + static constexpr const char *KEYSPACE_PATTERN = "__key*__:*"; + + // In Redis, by default keyspace events notifications are disabled because while not + // very sensible the feature uses some CPU power. Notifications are enabled using + // the notify-keyspace-events of redis.conf or via the CONFIG SET. + // In order to enable the feature a non-empty string is used, composed of multiple characters, + // where every character has a special meaning according to the following table: + // K - Keyspace events, published with __keyspace@__ prefix. + // E - Keyevent events, published with __keyevent@__ prefix. + // g - Generic commands (non-type specific) like DEL, EXPIRE, RENAME, ... + // $ - String commands + // l - List commands + // s - Set commands + // h - Hash commands + // z - Sorted set commands + // x - Expired events (events generated every time a key expires) + // e - Evicted events (events generated when a key is evicted for maxmemory) + // A - Alias for g$lshzxe, so that the "AKE" string means all the events. + // ACS Redis db mainly uses hash, therefore h is selected. + static constexpr const char *KEYSPACE_EVENTS = "KEA"; + + std::unordered_map keyspace_notification_channels; + std::unordered_map m_dbs; std::unordered_map m_redisClients; }; From c7b70235cb81803ad56ce802c863627b62ad6881 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Tue, 22 Sep 2020 02:54:20 +0000 Subject: [PATCH 12/37] Fix build Signed-off-by: Qi Luo --- common/dbconnector.cpp | 58 ----------------------------------- common/dbconnector.h | 68 +++++++++++++++++++++--------------------- common/redisreply.cpp | 4 +-- common/redisreply.h | 6 ++-- common/redistran.cpp | 1 + common/redistran.h | 2 +- 6 files changed, 41 insertions(+), 98 deletions(-) diff --git a/common/dbconnector.cpp b/common/dbconnector.cpp index 513f4a95f..6062e1bf5 100644 --- a/common/dbconnector.cpp +++ b/common/dbconnector.cpp @@ -564,35 +564,6 @@ DBConnector *DBConnector::newConnector(unsigned int timeout) const return ret; } -void DBConnector::setClientName(const string& clientName) -{ - string command("CLIENT SETNAME "); - command += clientName; - - RedisReply r(this, command, REDIS_REPLY_STATUS); - r.checkStatusOK(); -} - -string DBConnector::getClientName() -{ - string command("CLIENT GETNAME"); - - RedisReply r(this, command); - - auto ctx = r.getContext(); - if (ctx->type == REDIS_REPLY_STRING) - { - return r.getReply(); - } - else - { - if (ctx->type != REDIS_REPLY_NIL) - SWSS_LOG_ERROR("Unable to obtain Redis client name"); - - return ""; - } -} - int64_t DBConnector::del(const string &key) { RedisCommand sdel; @@ -754,35 +725,6 @@ shared_ptr DBConnector::blpop(const string &list, int timeout) throw runtime_error("GET failed, memory exception"); } -void DBConnector::setClientName(const string& clientName) -{ - string command("CLIENT SETNAME "); - command += clientName; - - RedisReply r(this, command, REDIS_REPLY_STATUS); - r.checkStatusOK(); -} - -string DBConnector::getClientName() -{ - string command("CLIENT GETNAME"); - - RedisReply r(this, command); - - auto ctx = r.getContext(); - if (ctx->type == REDIS_REPLY_STRING) - { - return r.getReply(); - } - else - { - if (ctx->type != REDIS_REPLY_NIL) - SWSS_LOG_ERROR("Unable to obtain Redis client name"); - - return ""; - } -} - } diff --git a/common/dbconnector.h b/common/dbconnector.h index 981a0109b..f15e46b88 100644 --- a/common/dbconnector.h +++ b/common/dbconnector.h @@ -97,40 +97,6 @@ class RedisContext std::string getClientName(); - int64_t del(const std::string &key); - - bool exists(const std::string &key); - - int64_t hdel(const std::string &key, const std::string &field); - - int64_t hdel(const std::string &key, const std::vector &fields); - - std::unordered_map hgetall(const std::string &key); - - template - void hgetall(const std::string &key, OutputIterator result); - - std::vector keys(const std::string &key); - - void set(const std::string &key, const std::string &value); - - void hset(const std::string &key, const std::string &field, const std::string &value); - - template - void hmset(const std::string &key, InputIterator start, InputIterator stop); - - std::shared_ptr get(const std::string &key); - - std::shared_ptr hget(const std::string &key, const std::string &field); - - int64_t incr(const std::string &key); - - int64_t decr(const std::string &key); - - int64_t rpush(const std::string &list, const std::string &item); - - std::shared_ptr blpop(const std::string &list, int timeout); - protected: RedisContext(); void initContext(const char *host, int port, const timeval& tv); @@ -168,6 +134,40 @@ class DBConnector : public RedisContext /* Create new context to DB */ DBConnector *newConnector(unsigned int timeout) const; + int64_t del(const std::string &key); + + bool exists(const std::string &key); + + int64_t hdel(const std::string &key, const std::string &field); + + int64_t hdel(const std::string &key, const std::vector &fields); + + std::unordered_map hgetall(const std::string &key); + + template + void hgetall(const std::string &key, OutputIterator result); + + std::vector keys(const std::string &key); + + void set(const std::string &key, const std::string &value); + + void hset(const std::string &key, const std::string &field, const std::string &value); + + template + void hmset(const std::string &key, InputIterator start, InputIterator stop); + + std::shared_ptr get(const std::string &key); + + std::shared_ptr hget(const std::string &key, const std::string &field); + + int64_t incr(const std::string &key); + + int64_t decr(const std::string &key); + + int64_t rpush(const std::string &list, const std::string &item); + + std::shared_ptr blpop(const std::string &list, int timeout); + private: void setNamespace(const std::string &netns); diff --git a/common/redisreply.cpp b/common/redisreply.cpp index 33e397b85..7fd90e89f 100644 --- a/common/redisreply.cpp +++ b/common/redisreply.cpp @@ -46,7 +46,7 @@ RedisReply::RedisReply(RedisContext *ctx, const RedisCommand& command) guard([&]{checkReply();}, command.c_str()); } -RedisReply::RedisReply(RedisContext *ctx, const string &command) +RedisReply::RedisReply(RedisContext *ctx, const string& command) { int rc = redisAppendCommand(ctx->getContext(), command.c_str()); if (rc != REDIS_OK) @@ -70,7 +70,7 @@ RedisReply::RedisReply(RedisContext *ctx, const RedisCommand& command, int expec guard([&]{checkReplyType(expectedType);}, command.c_str()); } -RedisReply::RedisReply(RedisContext *ctx, const string &command, int expectedType) +RedisReply::RedisReply(RedisContext *ctx, const string& command, int expectedType) : RedisReply(ctx, command) { guard([&]{checkReplyType(expectedType);}, command.c_str()); diff --git a/common/redisreply.h b/common/redisreply.h index f613f31c7..8bf8487a5 100644 --- a/common/redisreply.h +++ b/common/redisreply.h @@ -8,7 +8,7 @@ namespace swss { -class DBConnector; +class RedisContext; class RedisError : public std::runtime_error { @@ -41,7 +41,7 @@ class RedisReply * No reply type specified. */ RedisReply(RedisContext *ctx, const RedisCommand& command); - RedisReply(RedisContext *ctx, const std::string &command); + RedisReply(RedisContext *ctx, const std::string& command); /* * Send a new command to redis and waits for reply * The reply must be one of REDIS_REPLY_* format (e.g. REDIS_REPLY_STATUS, @@ -50,7 +50,7 @@ class RedisReply * protocol */ RedisReply(RedisContext *ctx, const RedisCommand& command, int expectedType); - RedisReply(RedisContext *ctx, const std::string &command, int expectedType); + RedisReply(RedisContext *ctx, const std::string& command, int expectedType); /* auto_ptr for native structue (Free the memory on destructor) */ RedisReply(redisReply *reply); diff --git a/common/redistran.cpp b/common/redistran.cpp index 73d697cea..8c3c87936 100644 --- a/common/redistran.cpp +++ b/common/redistran.cpp @@ -1,4 +1,5 @@ #include "redistran.h" +#include "dbconnector.h" namespace swss { diff --git a/common/redistran.h b/common/redistran.h index 00b852f94..e02a5deaa 100644 --- a/common/redistran.h +++ b/common/redistran.h @@ -2,7 +2,7 @@ #include #include -#include "redisreply.h" +#include "dbconnector.h" #include "rediscommand.h" #include "logger.h" From 9b4d414513a61fffcc98e6db5b1753713e5911eb Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Wed, 23 Sep 2020 06:45:38 +0000 Subject: [PATCH 13/37] Extract psubscribe and subscribe function into DBConnector class --- common/dbconnector.cpp | 14 ++++++++++++++ common/dbconnector.h | 4 ++++ common/redisselect.cpp | 9 ++------- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/common/dbconnector.cpp b/common/dbconnector.cpp index 6062e1bf5..ffab4c6d1 100644 --- a/common/dbconnector.cpp +++ b/common/dbconnector.cpp @@ -725,6 +725,20 @@ shared_ptr DBConnector::blpop(const string &list, int timeout) throw runtime_error("GET failed, memory exception"); } +void DBConnector::subscribe(const std::string &pattern) +{ + std::string s("SUBSCRIBE "); + s += pattern; + RedisReply r(this, s, REDIS_REPLY_ARRAY); +} + +void DBConnector::psubscribe(const std::string &pattern) +{ + std::string s("PSUBSCRIBE "); + s += pattern; + RedisReply r(this, s, REDIS_REPLY_ARRAY); +} + } diff --git a/common/dbconnector.h b/common/dbconnector.h index f15e46b88..4b5010844 100644 --- a/common/dbconnector.h +++ b/common/dbconnector.h @@ -168,6 +168,10 @@ class DBConnector : public RedisContext std::shared_ptr blpop(const std::string &list, int timeout); + void subscribe(const std::string &pattern); + + void psubscribe(const std::string &pattern); + private: void setNamespace(const std::string &netns); diff --git a/common/redisselect.cpp b/common/redisselect.cpp index 8d0283ab5..a2dec94a1 100644 --- a/common/redisselect.cpp +++ b/common/redisselect.cpp @@ -78,23 +78,18 @@ void RedisSelect::subscribe(DBConnector* db, const std::string &channelName) m_subscribe.reset(db->newConnector(SUBSCRIBE_TIMEOUT)); /* Send SUBSCRIBE #channel command */ - std::string s("SUBSCRIBE "); - s += channelName; - RedisReply r(m_subscribe.get(), s, REDIS_REPLY_ARRAY); + m_subscribe->subscribe(channelName); } /* PSUBSCRIBE */ void RedisSelect::psubscribe(DBConnector* db, const std::string &channelName) { m_subscribe.reset(db->newConnector(SUBSCRIBE_TIMEOUT)); - /* * Send PSUBSCRIBE #channel command on the * non-blocking subscriber DBConnector */ - std::string s("PSUBSCRIBE "); - s += channelName; - RedisReply r(m_subscribe.get(), s, REDIS_REPLY_ARRAY); + m_subscribe->psubscribe(channelName); } void RedisSelect::setQueueLength(long long int queueLength) From d901fe5213af3303848d94eb960d7015a757a3a1 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Thu, 24 Sep 2020 00:28:51 +0000 Subject: [PATCH 14/37] Implement _subscribe_keyspace_notification, _unsubscribe_keyspace_notification and _connection_error_handler --- common/dbconnector.cpp | 8 ++++++ common/dbconnector.h | 1 + common/dbinterface.h | 60 +++++++++++++++++++++++------------------- 3 files changed, 42 insertions(+), 27 deletions(-) diff --git a/common/dbconnector.cpp b/common/dbconnector.cpp index ffab4c6d1..c36f9404e 100644 --- a/common/dbconnector.cpp +++ b/common/dbconnector.cpp @@ -474,6 +474,14 @@ void DBConnector::select(DBConnector *db) r.checkStatusOK(); } +DBConnector::DBConnector(const DBConnector &other) + : RedisContext(other) + , m_dbId(other.m_dbId) + , m_namespace(other.m_namespace) +{ + select(this); +} + DBConnector::DBConnector(int dbId, const RedisContext& ctx) : RedisContext(ctx) , m_dbId(dbId) diff --git a/common/dbconnector.h b/common/dbconnector.h index 4b5010844..7107ab0e5 100644 --- a/common/dbconnector.h +++ b/common/dbconnector.h @@ -119,6 +119,7 @@ class DBConnector : public RedisContext * Timeout - The time in milisecond until exception is been thrown. For * infinite wait, set this value to 0 */ + DBConnector(const DBConnector &other); DBConnector(int dbId, const RedisContext &ctx); DBConnector(int dbId, const std::string &hostname, int port, unsigned int timeout); DBConnector(int dbId, const std::string &unixPath, unsigned int timeout); diff --git a/common/dbinterface.h b/common/dbinterface.h index 3813ecd56..0e3f92dae 100644 --- a/common/dbinterface.h +++ b/common/dbinterface.h @@ -1,4 +1,5 @@ #include +#include #include #include "dbconnector.h" @@ -29,7 +30,7 @@ class UnavailableDataError : public std::runtime_error class DBInterface : public RedisContext { public: - void connnect(int dbId, bool retry = true) + void connect(int dbId, bool retry = true) { if (retry) { @@ -37,40 +38,30 @@ class DBInterface : public RedisContext } else { - auto rc = m_dbs.emplace(std::piecewise_construct - , std::forward_as_tuple(dbId) - , std::forward_as_tuple(dbId, *this)); - bool inserted = rc.second; - if (inserted) - { - DBConnector *db = &rc.first->second; - m_redisClients.emplace(std::piecewise_construct - , std::forward_as_tuple(dbId) - , std::forward_as_tuple(db)); - return; - } + m_redisClient.emplace(std::piecewise_construct + , std::forward_as_tuple(dbId) + , std::forward_as_tuple(dbId, *this)); } } void close(int dbId) { - m_dbs.erase(dbId); - m_redisClients.erase(dbId); + m_redisClient.erase(dbId); } int64_t del(int dbId, const std::string& key) { - return m_redisClients.at(dbId).del(key); + return m_redisClient.at(dbId).del(key); } bool exists(int dbId, const std::string& key) { - return m_redisClients.at(dbId).exists(key); + return m_redisClient.at(dbId).exists(key); } std::string get(int dbId, const std::string& hash, const std::string& key) { - auto pvalue = m_redisClients.at(dbId).hget(hash, key); + auto pvalue = m_redisClient.at(dbId).hget(hash, key); if (!pvalue) { std::string message = "Key '" + hash + "' field '" + key + "' unavailable in database '" + std::to_string(dbId) + "'"; @@ -84,7 +75,7 @@ class DBInterface : public RedisContext std::map get_all(int dbId, const std::string& hash) { std::map map; - m_redisClients.at(dbId).hgetall(hash, std::inserter(map, map.end())); + m_redisClient.at(dbId).hgetall(hash, std::inserter(map, map.end())); if (map.empty()) { @@ -106,7 +97,7 @@ class DBInterface : public RedisContext std::vector keys(int dbId, const std::string& pattern = "*", bool blocking = false) { - auto keys = m_redisClients.at(dbId).keys(pattern); + auto keys = m_redisClient.at(dbId).keys(pattern); if (keys.empty()) { std::string message = "DB '{" + std::to_string(dbId) + "}' is empty!"; @@ -123,7 +114,7 @@ class DBInterface : public RedisContext int64_t set(int dbId, const std::string& hash, const std::string& key, const std::string& value) { - m_redisClients.at(dbId).hset(hash, key, value); + m_redisClient.at(dbId).hset(hash, key, value); // Return the number of fields that were added. return 1; } @@ -201,9 +192,16 @@ class DBInterface : public RedisContext } } + // Unsubscribe the chosent client from keyspace event notifications void _unsubscribe_keyspace_notification(int dbId) { - throw std::logic_error("Not implemented"); + auto found = keyspace_notification_channels.find(dbId); + if (found != keyspace_notification_channels.end()) + { + SWSS_LOG_DEBUG("Unsubscribe from keyspace notification"); + + keyspace_notification_channels.erase(found); + } } bool _unavailable_data_handler(int dbId, const char *data) @@ -211,14 +209,23 @@ class DBInterface : public RedisContext throw std::logic_error("Not implemented"); } + // Subscribe the chosent client to keyspace event notifications void _subscribe_keyspace_notification(int dbId) { - throw std::logic_error("Not implemented"); + SWSS_LOG_DEBUG("Subscribe to keyspace notification"); + auto& client = m_redisClient.at(dbId); + DBConnector *pubsub = client.newConnector(0); + pubsub->psubscribe(KEYSPACE_PATTERN); + keyspace_notification_channels.emplace(std::piecewise_construct, std::forward_as_tuple(dbId), std::forward_as_tuple(pubsub)); } + // In the event Redis is unavailable, close existing connections, and try again. void _connection_error_handler(int dbId) { - throw std::logic_error("Not implemented"); + SWSS_LOG_WARN("Could not connect to Redis--waiting before trying again."); + close(dbId); + sleep(CONNECT_RETRY_WAIT_TIME); + connect(dbId, true); } static const int BLOCKING_ATTEMPT_ERROR_THRESHOLD = 10; @@ -258,10 +265,9 @@ class DBInterface : public RedisContext // ACS Redis db mainly uses hash, therefore h is selected. static constexpr const char *KEYSPACE_EVENTS = "KEA"; - std::unordered_map keyspace_notification_channels; + std::unordered_map> keyspace_notification_channels; - std::unordered_map m_dbs; - std::unordered_map m_redisClients; + std::unordered_map m_redisClient; }; } From 03b99c5d11ca2a0e8119ccacc86f2d96218f611b Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Thu, 24 Sep 2020 03:08:41 +0000 Subject: [PATCH 15/37] Implement blockable Signed-off-by: Qi Luo --- common/Makefile.am | 2 +- common/dbinterface.h | 89 +++++++++++++++++++++++++------------------- 2 files changed, 52 insertions(+), 39 deletions(-) diff --git a/common/Makefile.am b/common/Makefile.am index fec265852..a18f99d44 100644 --- a/common/Makefile.am +++ b/common/Makefile.am @@ -59,7 +59,7 @@ libswsscommon_la_SOURCES = \ timestamp.cpp \ warm_restart.cpp -libswsscommon_la_CXXFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(LIBNL_CFLAGS) +libswsscommon_la_CXXFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(LIBNL_CFLAGS) -std=c++14 libswsscommon_la_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(LIBNL_CPPFLAGS) libswsscommon_la_LIBADD = -lpthread $(LIBNL_LIBS) diff --git a/common/dbinterface.h b/common/dbinterface.h index 0e3f92dae..716a53cae 100644 --- a/common/dbinterface.h +++ b/common/dbinterface.h @@ -72,39 +72,47 @@ class DBInterface : public RedisContext return value == "None" ? "" : value; } - std::map get_all(int dbId, const std::string& hash) + std::map get_all(int dbId, const std::string& hash, bool blocking = false) { - std::map map; - m_redisClient.at(dbId).hgetall(hash, std::inserter(map, map.end())); - - if (map.empty()) - { - std::string message = "Key '{" + hash + "}' unavailable in database '{" + std::to_string(dbId) + "}'"; - SWSS_LOG_WARN("%s", message.c_str()); - throw UnavailableDataError(message, hash); - } - for (auto& i : map) + auto innerfunc = [&] { - std::string& value = i.second; - if (value == "None") + std::map map; + m_redisClient.at(dbId).hgetall(hash, std::inserter(map, map.end())); + + if (map.empty()) { - value = ""; + std::string message = "Key '{" + hash + "}' unavailable in database '{" + std::to_string(dbId) + "}'"; + SWSS_LOG_WARN("%s", message.c_str()); + throw UnavailableDataError(message, hash); + } + for (auto& i : map) + { + std::string& value = i.second; + if (value == "None") + { + value = ""; + } } - } - return map; + return map; + }; + return blockable(innerfunc, dbId, blocking); } std::vector keys(int dbId, const std::string& pattern = "*", bool blocking = false) { - auto keys = m_redisClient.at(dbId).keys(pattern); - if (keys.empty()) + auto innerfunc = [&] { - std::string message = "DB '{" + std::to_string(dbId) + "}' is empty!"; - SWSS_LOG_WARN("%s", message.c_str()); - throw UnavailableDataError(message, "keys"); - } - return keys; + auto keys = m_redisClient.at(dbId).keys(pattern); + if (keys.empty()) + { + std::string message = "DB '{" + std::to_string(dbId) + "}' is empty!"; + SWSS_LOG_WARN("%s", message.c_str()); + throw UnavailableDataError(message, "keys"); + } + return keys; + }; + return blockable(innerfunc, dbId, blocking); } int64_t publish(int dbId, const std::string& channel, const std::string& message) @@ -112,50 +120,55 @@ class DBInterface : public RedisContext throw std::logic_error("Not implemented"); } - int64_t set(int dbId, const std::string& hash, const std::string& key, const std::string& value) + int64_t set(int dbId, const std::string& hash, const std::string& key, const std::string& value, bool blocking = false) { - m_redisClient.at(dbId).hset(hash, key, value); - // Return the number of fields that were added. - return 1; + auto innerfunc = [&] + { + m_redisClient.at(dbId).hset(hash, key, value); + // Return the number of fields that were added. + return 1; + }; + return blockable(innerfunc, dbId, blocking); } DBConnector *at(int dbId); private: - template - T blockable(FUNC f, int db_id, bool blocking = false) + template + auto blockable(FUNC f, int dbId, bool blocking = false) -> decltype(f()) { + typedef decltype(f()) T; int attempts = 0; for (;;) { try { - T ret_data = f(db_id); - _unsubscribe_keyspace_notification(db_id); + T ret_data = f(); + _unsubscribe_keyspace_notification(dbId); return ret_data; } catch (const UnavailableDataError& e) { if (blocking) { - auto found = keyspace_notification_channels.find(db_id); + auto found = keyspace_notification_channels.find(dbId); if (found != keyspace_notification_channels.end()) { - bool result = _unavailable_data_handler(db_id, e.getData()); + bool result = _unavailable_data_handler(dbId, e.getData()); if (result) { continue; // received updates, try to read data again } else { - _unsubscribe_keyspace_notification(db_id); + _unsubscribe_keyspace_notification(dbId); throw; // No updates was received. Raise exception } } else { // Subscribe to updates and try it again (avoiding race condition) - _subscribe_keyspace_notification(db_id); + _subscribe_keyspace_notification(dbId); } } else @@ -170,15 +183,15 @@ class DBInterface : public RedisContext Retrying the request won't pass unless the schema itself changes. In this case, the error should be attributed to the application itself. Re-raise the error. */ - SWSS_LOG_ERROR("Bad DB request [%d]", db_id); + SWSS_LOG_ERROR("Bad DB request [%d]", dbId); throw; } catch (const RedisError&) { // Redis connection broken and we need to retry several times attempts += 1; - _connection_error_handler(db_id); - std::string msg = "DB access failure by [" + std::to_string(db_id) + + "]"; + _connection_error_handler(dbId); + std::string msg = "DB access failure by [" + std::to_string(dbId) + + "]"; if (BLOCKING_ATTEMPT_ERROR_THRESHOLD < attempts && attempts < BLOCKING_ATTEMPT_SUPPRESSION) { // Repeated access failures implies the database itself is unhealthy. From 1aa56b4afad9b2871111969a5cb9159aaf06028c Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Thu, 24 Sep 2020 20:10:25 +0000 Subject: [PATCH 16/37] Implement connect with retry Signed-off-by: Qi Luo --- common/dbinterface.h | 37 ++++++++++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/common/dbinterface.h b/common/dbinterface.h index 716a53cae..c15683f3f 100644 --- a/common/dbinterface.h +++ b/common/dbinterface.h @@ -34,13 +34,11 @@ class DBInterface : public RedisContext { if (retry) { - throw std::logic_error("retry not implemented"); + _persistent_connect(dbId); } else { - m_redisClient.emplace(std::piecewise_construct - , std::forward_as_tuple(dbId) - , std::forward_as_tuple(dbId, *this)); + _onetime_connect(dbId); } } @@ -131,7 +129,10 @@ class DBInterface : public RedisContext return blockable(innerfunc, dbId, blocking); } - DBConnector *at(int dbId); + DBConnector& get_redis_client(int dbId) + { + return m_redisClient.at(dbId); + } private: template @@ -241,6 +242,32 @@ class DBInterface : public RedisContext connect(dbId, true); } + void _onetime_connect(int dbId) + { + m_redisClient.emplace(std::piecewise_construct + , std::forward_as_tuple(dbId) + , std::forward_as_tuple(dbId, *this)); + } + + // Keep reconnecting to Database 'db_id' until success + void _persistent_connect(int dbId) + { + for (;;) + { + try + { + _onetime_connect(dbId); + } + catch (RedisError&) + { + const int wait = CONNECT_RETRY_WAIT_TIME; + SWSS_LOG_WARN("Connecting to DB '%d' failed, will retry in %d s", dbId, wait); + close(dbId); + sleep(wait); + } + } + } + static const int BLOCKING_ATTEMPT_ERROR_THRESHOLD = 10; static const int BLOCKING_ATTEMPT_SUPPRESSION = BLOCKING_ATTEMPT_ERROR_THRESHOLD + 5; From a8457a9e9eb6c01f41c5a883c4083b3f82e85024 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Fri, 25 Sep 2020 06:28:36 +0000 Subject: [PATCH 17/37] Implement DBConnector::publish(), DBInterface::_unavailable_data_handler() Signed-off-by: Qi Luo --- common/Makefile.am | 1 + common/dbconnector.cpp | 14 +- common/dbconnector.h | 2 + common/dbinterface.cpp | 300 ++++++++++++++++++++++++++++++++ common/dbinterface.h | 249 ++------------------------ common/notificationproducer.cpp | 5 +- 6 files changed, 329 insertions(+), 242 deletions(-) create mode 100644 common/dbinterface.cpp diff --git a/common/Makefile.am b/common/Makefile.am index a18f99d44..12489543e 100644 --- a/common/Makefile.am +++ b/common/Makefile.am @@ -29,6 +29,7 @@ libswsscommon_la_SOURCES = \ logger.cpp \ redisreply.cpp \ dbconnector.cpp \ + dbinterface.cpp \ table.cpp \ json.cpp \ producertable.cpp \ diff --git a/common/dbconnector.cpp b/common/dbconnector.cpp index c36f9404e..169851e1d 100644 --- a/common/dbconnector.cpp +++ b/common/dbconnector.cpp @@ -13,8 +13,7 @@ using json = nlohmann::json; using namespace std; - -namespace swss { +using namespace swss; void SonicDBConfig::parseDatabaseConfig(const string &file, std::unordered_map &inst_entry, @@ -747,9 +746,10 @@ void DBConnector::psubscribe(const std::string &pattern) RedisReply r(this, s, REDIS_REPLY_ARRAY); } +int64_t DBConnector::publish(const string &channel, const string &message) +{ + RedisCommand publish; + publish.format("PUBLISH %s %s", channel.c_str(), message.c_str()); + RedisReply r(this, publish, REDIS_REPLY_INTEGER); + return r.getReply(); } - - -/////////////////// TEST code, do not checkin -#include "common/dbinterface.h" - diff --git a/common/dbconnector.h b/common/dbconnector.h index 7107ab0e5..15e6139e8 100644 --- a/common/dbconnector.h +++ b/common/dbconnector.h @@ -173,6 +173,8 @@ class DBConnector : public RedisContext void psubscribe(const std::string &pattern); + int64_t publish(const std::string &channel, const std::string &message); + private: void setNamespace(const std::string &netns); diff --git a/common/dbinterface.cpp b/common/dbinterface.cpp new file mode 100644 index 000000000..957dff3ea --- /dev/null +++ b/common/dbinterface.cpp @@ -0,0 +1,300 @@ +#include +#include +#include "dbinterface.h" + +using namespace std; +using namespace std::chrono; +using namespace swss; + +void DBInterface::connect(int dbId, bool retry) +{ + if (retry) + { + _persistent_connect(dbId); + } + else + { + _onetime_connect(dbId); + } +} + +void DBInterface::close(int dbId) +{ + m_redisClient.erase(dbId); +} + +int64_t DBInterface::del(int dbId, const std::string& key) +{ + return m_redisClient.at(dbId).del(key); +} + +bool DBInterface::exists(int dbId, const std::string& key) +{ + return m_redisClient.at(dbId).exists(key); +} + +std::string DBInterface::get(int dbId, const std::string& hash, const std::string& key) +{ + auto pvalue = m_redisClient.at(dbId).hget(hash, key); + if (!pvalue) + { + std::string message = "Key '" + hash + "' field '" + key + "' unavailable in database '" + std::to_string(dbId) + "'"; + SWSS_LOG_WARN("%s", message.c_str()); + throw UnavailableDataError(message, hash); + } + const std::string& value = *pvalue; + return value == "None" ? "" : value; +} + +std::map DBInterface::get_all(int dbId, const std::string& hash, bool blocking) +{ + auto innerfunc = [&] + { + std::map map; + m_redisClient.at(dbId).hgetall(hash, std::inserter(map, map.end())); + + if (map.empty()) + { + std::string message = "Key '{" + hash + "}' unavailable in database '{" + std::to_string(dbId) + "}'"; + SWSS_LOG_WARN("%s", message.c_str()); + throw UnavailableDataError(message, hash); + } + for (auto& i : map) + { + std::string& value = i.second; + if (value == "None") + { + value = ""; + } + } + + return map; + }; + return blockable(innerfunc, dbId, blocking); +} + +std::vector DBInterface::keys(int dbId, const std::string& pattern, bool blocking) +{ + auto innerfunc = [&] + { + auto keys = m_redisClient.at(dbId).keys(pattern); + if (keys.empty()) + { + std::string message = "DB '{" + std::to_string(dbId) + "}' is empty!"; + SWSS_LOG_WARN("%s", message.c_str()); + throw UnavailableDataError(message, "hset"); + } + return keys; + }; + return blockable(innerfunc, dbId, blocking); +} + +int64_t DBInterface::publish(int dbId, const std::string& channel, const std::string& message) +{ + return m_redisClient.at(dbId).publish(channel, message); +} + +int64_t DBInterface::set(int dbId, const std::string& hash, const std::string& key, const std::string& value, bool blocking) +{ + auto innerfunc = [&] + { + m_redisClient.at(dbId).hset(hash, key, value); + // Return the number of fields that were added. + return 1; + }; + return blockable(innerfunc, dbId, blocking); +} + +DBConnector& DBInterface::get_redis_client(int dbId) +{ + return m_redisClient.at(dbId); +} + +template +auto DBInterface::blockable(FUNC f, int dbId, bool blocking) -> decltype(f()) +{ + typedef decltype(f()) T; + int attempts = 0; + for (;;) + { + try + { + T ret_data = f(); + _unsubscribe_keyspace_notification(dbId); + return ret_data; + } + catch (const UnavailableDataError& e) + { + if (blocking) + { + auto found = keyspace_notification_channels.find(dbId); + if (found != keyspace_notification_channels.end()) + { + bool result = _unavailable_data_handler(dbId, e.getData()); + if (result) + { + continue; // received updates, try to read data again + } + else + { + _unsubscribe_keyspace_notification(dbId); + throw; // No updates was received. Raise exception + } + } + else + { + // Subscribe to updates and try it again (avoiding race condition) + _subscribe_keyspace_notification(dbId); + } + } + else + { + return T(); + } + } + catch (const std::system_error&) + { + /* + Something is fundamentally wrong with the request itself. + Retrying the request won't pass unless the schema itself changes. In this case, the error + should be attributed to the application itself. Re-raise the error. + */ + SWSS_LOG_ERROR("Bad DB request [%d]", dbId); + throw; + } + catch (const RedisError&) + { + // Redis connection broken and we need to retry several times + attempts += 1; + _connection_error_handler(dbId); + std::string msg = "DB access failure by [" + std::to_string(dbId) + + "]"; + if (BLOCKING_ATTEMPT_ERROR_THRESHOLD < attempts && attempts < BLOCKING_ATTEMPT_SUPPRESSION) + { + // Repeated access failures implies the database itself is unhealthy. + SWSS_LOG_ERROR("%s", msg.c_str()); + } + else + { + SWSS_LOG_WARN("%s", msg.c_str()); + } + } + } +} + +// Unsubscribe the chosent client from keyspace event notifications +void DBInterface::_unsubscribe_keyspace_notification(int dbId) +{ + auto found = keyspace_notification_channels.find(dbId); + if (found != keyspace_notification_channels.end()) + { + SWSS_LOG_DEBUG("Unsubscribe from keyspace notification"); + + keyspace_notification_channels.erase(found); + } +} + +// When the queried config is not available in Redis--wait until it is available. +// Two timeouts are at work here: +// 1. Notification timeout - how long to wait before giving up on receiving any given pub-sub message. +// 2. Max data wait - swsssdk-specific. how long to wait for the data to populate (in absolute time) +bool DBInterface::_unavailable_data_handler(int dbId, const char *data) +{ + auto start = system_clock::now(); + SWSS_LOG_DEBUG("Listening on pubsub channel '%d'", dbId); + auto wait = duration(PUB_SUB_MAXIMUM_DATA_WAIT); + while (system_clock::now() - start < wait) + { + auto& channel = keyspace_notification_channels.at(dbId); + auto ctx = channel->getContext(); + redisReply *reply; + int rc = redisGetReply(ctx, reinterpret_cast(&reply)); + if (rc == REDIS_ERR && ctx->err == REDIS_ERR_IO && errno == EAGAIN) + { + // Timeout + continue; + } + if (rc != REDIS_OK) + { + throw RedisError("Failed to redisGetReply with on pubsub channel on dbId=" + to_string(dbId), ctx); + } + + RedisReply r(reply); + // r is an array of: + // 0. 'type': 'pmessage', + // 1. 'pattern': '__key*__:*' + // 2. 'channel': + // 3. 'data': + redisReply& r3 = *r.getChild(3); + if (r3.type != REDIS_REPLY_STRING) + { + throw system_error(make_error_code(errc::io_error), + "Wrong expected type of result"); + } + + if (strcmp(r3.str, data) == 0) + { + SWSS_LOG_INFO("'%s' acquired via pub-sub dbId=%d. Unblocking...", data, dbId); + // Wait for a "settling" period before releasing the wait. + sleep(DATA_RETRIEVAL_WAIT_TIME); + return true; + } + } + + SWSS_LOG_WARN("No notification for '%s' from '%d' received before timeout.", data, dbId); + return false; +} + +// Subscribe the chosent client to keyspace event notifications +void DBInterface::_subscribe_keyspace_notification(int dbId) +{ + SWSS_LOG_DEBUG("Subscribe to keyspace notification"); + auto& client = m_redisClient.at(dbId); + DBConnector *pubsub = client.newConnector(0); + pubsub->psubscribe(KEYSPACE_PATTERN); + + // Set the timeout of the pubsub channel, so future redisGetReply will be impacted + struct timeval tv = { 0, (suseconds_t)(1000 * PUB_SUB_NOTIFICATION_TIMEOUT) }; + int rc = redisSetTimeout(pubsub->getContext(), tv); + if (rc != REDIS_OK) + { + throw RedisError("Failed to redisSetTimeout", pubsub->getContext()); + } + + keyspace_notification_channels.emplace(std::piecewise_construct, std::forward_as_tuple(dbId), std::forward_as_tuple(pubsub)); +} + +// In the event Redis is unavailable, close existing connections, and try again. +void DBInterface::_connection_error_handler(int dbId) +{ + SWSS_LOG_WARN("Could not connect to Redis--waiting before trying again."); + close(dbId); + sleep(CONNECT_RETRY_WAIT_TIME); + connect(dbId, true); +} + +void DBInterface::_onetime_connect(int dbId) +{ + m_redisClient.emplace(std::piecewise_construct + , std::forward_as_tuple(dbId) + , std::forward_as_tuple(dbId, *this)); +} + +// Keep reconnecting to Database 'dbId' until success +void DBInterface::_persistent_connect(int dbId) +{ + for (;;) + { + try + { + _onetime_connect(dbId); + return; + } + catch (RedisError&) + { + const int wait = CONNECT_RETRY_WAIT_TIME; + SWSS_LOG_WARN("Connecting to DB '%d' failed, will retry in %d s", dbId, wait); + close(dbId); + sleep(wait); + } + } +} diff --git a/common/dbinterface.h b/common/dbinterface.h index c15683f3f..ce1508000 100644 --- a/common/dbinterface.h +++ b/common/dbinterface.h @@ -30,243 +30,30 @@ class UnavailableDataError : public std::runtime_error class DBInterface : public RedisContext { public: - void connect(int dbId, bool retry = true) - { - if (retry) - { - _persistent_connect(dbId); - } - else - { - _onetime_connect(dbId); - } - } - - void close(int dbId) - { - m_redisClient.erase(dbId); - } - - int64_t del(int dbId, const std::string& key) - { - return m_redisClient.at(dbId).del(key); - } - - bool exists(int dbId, const std::string& key) - { - return m_redisClient.at(dbId).exists(key); - } - - std::string get(int dbId, const std::string& hash, const std::string& key) - { - auto pvalue = m_redisClient.at(dbId).hget(hash, key); - if (!pvalue) - { - std::string message = "Key '" + hash + "' field '" + key + "' unavailable in database '" + std::to_string(dbId) + "'"; - SWSS_LOG_WARN("%s", message.c_str()); - throw UnavailableDataError(message, hash); - } - const std::string& value = *pvalue; - return value == "None" ? "" : value; - } - - std::map get_all(int dbId, const std::string& hash, bool blocking = false) - { - auto innerfunc = [&] - { - std::map map; - m_redisClient.at(dbId).hgetall(hash, std::inserter(map, map.end())); - - if (map.empty()) - { - std::string message = "Key '{" + hash + "}' unavailable in database '{" + std::to_string(dbId) + "}'"; - SWSS_LOG_WARN("%s", message.c_str()); - throw UnavailableDataError(message, hash); - } - for (auto& i : map) - { - std::string& value = i.second; - if (value == "None") - { - value = ""; - } - } - - return map; - }; - return blockable(innerfunc, dbId, blocking); - } - - std::vector keys(int dbId, const std::string& pattern = "*", bool blocking = false) - { - auto innerfunc = [&] - { - auto keys = m_redisClient.at(dbId).keys(pattern); - if (keys.empty()) - { - std::string message = "DB '{" + std::to_string(dbId) + "}' is empty!"; - SWSS_LOG_WARN("%s", message.c_str()); - throw UnavailableDataError(message, "keys"); - } - return keys; - }; - return blockable(innerfunc, dbId, blocking); - } - - int64_t publish(int dbId, const std::string& channel, const std::string& message) - { - throw std::logic_error("Not implemented"); - } - - int64_t set(int dbId, const std::string& hash, const std::string& key, const std::string& value, bool blocking = false) - { - auto innerfunc = [&] - { - m_redisClient.at(dbId).hset(hash, key, value); - // Return the number of fields that were added. - return 1; - }; - return blockable(innerfunc, dbId, blocking); - } - - DBConnector& get_redis_client(int dbId) - { - return m_redisClient.at(dbId); - } + void connect(int dbId, bool retry = true); + void close(int dbId); + int64_t del(int dbId, const std::string& key); + bool exists(int dbId, const std::string& key); + std::string get(int dbId, const std::string& hash, const std::string& key); + std::map get_all(int dbId, const std::string& hash, bool blocking = false); + std::vector keys(int dbId, const std::string& pattern = "*", bool blocking = false); + int64_t publish(int dbId, const std::string& channel, const std::string& message); + int64_t set(int dbId, const std::string& hash, const std::string& key, const std::string& value, bool blocking = false); + DBConnector& get_redis_client(int dbId); private: template - auto blockable(FUNC f, int dbId, bool blocking = false) -> decltype(f()) - { - typedef decltype(f()) T; - int attempts = 0; - for (;;) - { - try - { - T ret_data = f(); - _unsubscribe_keyspace_notification(dbId); - return ret_data; - } - catch (const UnavailableDataError& e) - { - if (blocking) - { - auto found = keyspace_notification_channels.find(dbId); - if (found != keyspace_notification_channels.end()) - { - bool result = _unavailable_data_handler(dbId, e.getData()); - if (result) - { - continue; // received updates, try to read data again - } - else - { - _unsubscribe_keyspace_notification(dbId); - throw; // No updates was received. Raise exception - } - } - else - { - // Subscribe to updates and try it again (avoiding race condition) - _subscribe_keyspace_notification(dbId); - } - } - else - { - return T(); - } - } - catch (const std::system_error&) - { - /* - Something is fundamentally wrong with the request itself. - Retrying the request won't pass unless the schema itself changes. In this case, the error - should be attributed to the application itself. Re-raise the error. - */ - SWSS_LOG_ERROR("Bad DB request [%d]", dbId); - throw; - } - catch (const RedisError&) - { - // Redis connection broken and we need to retry several times - attempts += 1; - _connection_error_handler(dbId); - std::string msg = "DB access failure by [" + std::to_string(dbId) + + "]"; - if (BLOCKING_ATTEMPT_ERROR_THRESHOLD < attempts && attempts < BLOCKING_ATTEMPT_SUPPRESSION) - { - // Repeated access failures implies the database itself is unhealthy. - SWSS_LOG_ERROR("%s", msg.c_str()); - } - else - { - SWSS_LOG_WARN("%s", msg.c_str()); - } - } - } - } - + auto blockable(FUNC f, int dbId, bool blocking = false) -> decltype(f()); // Unsubscribe the chosent client from keyspace event notifications - void _unsubscribe_keyspace_notification(int dbId) - { - auto found = keyspace_notification_channels.find(dbId); - if (found != keyspace_notification_channels.end()) - { - SWSS_LOG_DEBUG("Unsubscribe from keyspace notification"); - - keyspace_notification_channels.erase(found); - } - } - - bool _unavailable_data_handler(int dbId, const char *data) - { - throw std::logic_error("Not implemented"); - } - + void _unsubscribe_keyspace_notification(int dbId); + bool _unavailable_data_handler(int dbId, const char *data); // Subscribe the chosent client to keyspace event notifications - void _subscribe_keyspace_notification(int dbId) - { - SWSS_LOG_DEBUG("Subscribe to keyspace notification"); - auto& client = m_redisClient.at(dbId); - DBConnector *pubsub = client.newConnector(0); - pubsub->psubscribe(KEYSPACE_PATTERN); - keyspace_notification_channels.emplace(std::piecewise_construct, std::forward_as_tuple(dbId), std::forward_as_tuple(pubsub)); - } - + void _subscribe_keyspace_notification(int dbId); // In the event Redis is unavailable, close existing connections, and try again. - void _connection_error_handler(int dbId) - { - SWSS_LOG_WARN("Could not connect to Redis--waiting before trying again."); - close(dbId); - sleep(CONNECT_RETRY_WAIT_TIME); - connect(dbId, true); - } - - void _onetime_connect(int dbId) - { - m_redisClient.emplace(std::piecewise_construct - , std::forward_as_tuple(dbId) - , std::forward_as_tuple(dbId, *this)); - } - - // Keep reconnecting to Database 'db_id' until success - void _persistent_connect(int dbId) - { - for (;;) - { - try - { - _onetime_connect(dbId); - } - catch (RedisError&) - { - const int wait = CONNECT_RETRY_WAIT_TIME; - SWSS_LOG_WARN("Connecting to DB '%d' failed, will retry in %d s", dbId, wait); - close(dbId); - sleep(wait); - } - } - } + void _connection_error_handler(int dbId); + void _onetime_connect(int dbId); + // Keep reconnecting to Database 'dbId' until success + void _persistent_connect(int dbId); static const int BLOCKING_ATTEMPT_ERROR_THRESHOLD = 10; static const int BLOCKING_ATTEMPT_SUPPRESSION = BLOCKING_ATTEMPT_ERROR_THRESHOLD + 5; diff --git a/common/notificationproducer.cpp b/common/notificationproducer.cpp index ca663c251..65587ffca 100644 --- a/common/notificationproducer.cpp +++ b/common/notificationproducer.cpp @@ -19,8 +19,5 @@ int64_t swss::NotificationProducer::send(const std::string &op, const std::strin SWSS_LOG_DEBUG("channel %s, publish: %s", m_channel.c_str(), msg.c_str()); - RedisCommand publish; - publish.format("PUBLISH %s %s", m_channel.c_str(), msg.c_str()); - RedisReply r(m_db, publish, REDIS_REPLY_INTEGER); - return r.getReply(); + return m_db->publish(m_channel, msg); } From 87bcf7fd429dccd9bf469740da3647469072c8d7 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 26 Sep 2020 00:00:48 +0000 Subject: [PATCH 18/37] Use c++11 syntax instead of c++14 --- common/Makefile.am | 2 +- common/dbinterface.cpp | 11 +++++------ common/dbinterface.h | 4 ++-- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/common/Makefile.am b/common/Makefile.am index 12489543e..b2ec1b53e 100644 --- a/common/Makefile.am +++ b/common/Makefile.am @@ -60,7 +60,7 @@ libswsscommon_la_SOURCES = \ timestamp.cpp \ warm_restart.cpp -libswsscommon_la_CXXFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(LIBNL_CFLAGS) -std=c++14 +libswsscommon_la_CXXFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(LIBNL_CFLAGS) libswsscommon_la_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(LIBNL_CPPFLAGS) libswsscommon_la_LIBADD = -lpthread $(LIBNL_LIBS) diff --git a/common/dbinterface.cpp b/common/dbinterface.cpp index 957dff3ea..be12990e1 100644 --- a/common/dbinterface.cpp +++ b/common/dbinterface.cpp @@ -70,7 +70,7 @@ std::map DBInterface::get_all(int dbId, const std::str return map; }; - return blockable(innerfunc, dbId, blocking); + return blockable>(innerfunc, dbId, blocking); } std::vector DBInterface::keys(int dbId, const std::string& pattern, bool blocking) @@ -86,7 +86,7 @@ std::vector DBInterface::keys(int dbId, const std::string& pattern, } return keys; }; - return blockable(innerfunc, dbId, blocking); + return blockable>(innerfunc, dbId, blocking); } int64_t DBInterface::publish(int dbId, const std::string& channel, const std::string& message) @@ -102,7 +102,7 @@ int64_t DBInterface::set(int dbId, const std::string& hash, const std::string& k // Return the number of fields that were added. return 1; }; - return blockable(innerfunc, dbId, blocking); + return blockable(innerfunc, dbId, blocking); } DBConnector& DBInterface::get_redis_client(int dbId) @@ -110,10 +110,9 @@ DBConnector& DBInterface::get_redis_client(int dbId) return m_redisClient.at(dbId); } -template -auto DBInterface::blockable(FUNC f, int dbId, bool blocking) -> decltype(f()) +template +T DBInterface::blockable(FUNC f, int dbId, bool blocking) { - typedef decltype(f()) T; int attempts = 0; for (;;) { diff --git a/common/dbinterface.h b/common/dbinterface.h index ce1508000..a72892923 100644 --- a/common/dbinterface.h +++ b/common/dbinterface.h @@ -42,8 +42,8 @@ class DBInterface : public RedisContext DBConnector& get_redis_client(int dbId); private: - template - auto blockable(FUNC f, int dbId, bool blocking = false) -> decltype(f()); + template + T blockable(FUNC f, int dbId, bool blocking = false); // Unsubscribe the chosent client from keyspace event notifications void _unsubscribe_keyspace_notification(int dbId); bool _unavailable_data_handler(int dbId, const char *data); From b0fbe0348958f2682f1eccb18fc3f79db8f8ecd3 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 26 Sep 2020 00:46:13 +0000 Subject: [PATCH 19/37] Implement blocking for get and del --- common/dbinterface.cpp | 30 +++++++++++++++++++----------- common/dbinterface.h | 6 ++++-- 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/common/dbinterface.cpp b/common/dbinterface.cpp index be12990e1..f3ae7bf70 100644 --- a/common/dbinterface.cpp +++ b/common/dbinterface.cpp @@ -23,9 +23,13 @@ void DBInterface::close(int dbId) m_redisClient.erase(dbId); } -int64_t DBInterface::del(int dbId, const std::string& key) +int64_t DBInterface::del(int dbId, const std::string& key, bool blocking) { - return m_redisClient.at(dbId).del(key); + auto innerfunc = [&] + { + return m_redisClient.at(dbId).del(key); + }; + return blockable(innerfunc, dbId, blocking); } bool DBInterface::exists(int dbId, const std::string& key) @@ -33,17 +37,21 @@ bool DBInterface::exists(int dbId, const std::string& key) return m_redisClient.at(dbId).exists(key); } -std::string DBInterface::get(int dbId, const std::string& hash, const std::string& key) +std::string DBInterface::get(int dbId, const std::string& hash, const std::string& key, bool blocking) { - auto pvalue = m_redisClient.at(dbId).hget(hash, key); - if (!pvalue) + auto innerfunc = [&] { - std::string message = "Key '" + hash + "' field '" + key + "' unavailable in database '" + std::to_string(dbId) + "'"; - SWSS_LOG_WARN("%s", message.c_str()); - throw UnavailableDataError(message, hash); - } - const std::string& value = *pvalue; - return value == "None" ? "" : value; + auto pvalue = m_redisClient.at(dbId).hget(hash, key); + if (!pvalue) + { + std::string message = "Key '" + hash + "' field '" + key + "' unavailable in database '" + std::to_string(dbId) + "'"; + SWSS_LOG_WARN("%s", message.c_str()); + throw UnavailableDataError(message, hash); + } + const std::string& value = *pvalue; + return value == "None" ? "" : value; + }; + return blockable(innerfunc, dbId, blocking); } std::map DBInterface::get_all(int dbId, const std::string& hash, bool blocking) diff --git a/common/dbinterface.h b/common/dbinterface.h index a72892923..44c0a58d2 100644 --- a/common/dbinterface.h +++ b/common/dbinterface.h @@ -1,3 +1,5 @@ +#pragma once + #include #include #include @@ -32,9 +34,9 @@ class DBInterface : public RedisContext public: void connect(int dbId, bool retry = true); void close(int dbId); - int64_t del(int dbId, const std::string& key); + int64_t del(int dbId, const std::string& key, bool blocking = false); bool exists(int dbId, const std::string& key); - std::string get(int dbId, const std::string& hash, const std::string& key); + std::string get(int dbId, const std::string& hash, const std::string& key, bool blocking = false); std::map get_all(int dbId, const std::string& hash, bool blocking = false); std::vector keys(int dbId, const std::string& pattern = "*", bool blocking = false); int64_t publish(int dbId, const std::string& channel, const std::string& message); From bf779bd999d2c69c35333c20ea7a80607231b982 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 26 Sep 2020 01:37:49 +0000 Subject: [PATCH 20/37] Add to pyext --- pyext/swsscommon.i | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pyext/swsscommon.i b/pyext/swsscommon.i index 21db07826..b03b40104 100644 --- a/pyext/swsscommon.i +++ b/pyext/swsscommon.i @@ -3,6 +3,7 @@ %{ #include "schema.h" #include "dbconnector.h" +#include "dbinterface.h" #include "select.h" #include "selectable.h" #include "rediscommand.h" @@ -95,3 +96,4 @@ swss::RedisSelect *CastSelectableToRedisSelectObj(swss::Selectable *temp) { %include "notificationproducer.h" %include "warm_restart.h" +%include "dbinterface.h" From c193a6a06b5690e0c1bbc17fb89f165278034b2c Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Mon, 28 Sep 2020 23:50:41 +0000 Subject: [PATCH 21/37] Add set_redis_kwargs(), fix _onetime_connect() --- common/dbinterface.cpp | 23 ++++++++++++++++++++--- common/dbinterface.h | 7 ++++++- tests/redis_ut.cpp | 8 ++++++++ 3 files changed, 34 insertions(+), 4 deletions(-) diff --git a/common/dbinterface.cpp b/common/dbinterface.cpp index f3ae7bf70..708ca36a6 100644 --- a/common/dbinterface.cpp +++ b/common/dbinterface.cpp @@ -6,6 +6,13 @@ using namespace std; using namespace std::chrono; using namespace swss; +void DBInterface::set_redis_kwargs(std::string unix_socket_path, std::string host, int port) +{ + m_unix_socket_path = unix_socket_path; + m_host = host; + m_port = port; +} + void DBInterface::connect(int dbId, bool retry) { if (retry) @@ -281,9 +288,19 @@ void DBInterface::_connection_error_handler(int dbId) void DBInterface::_onetime_connect(int dbId) { - m_redisClient.emplace(std::piecewise_construct - , std::forward_as_tuple(dbId) - , std::forward_as_tuple(dbId, *this)); + if (m_unix_socket_path.empty()) + { + m_redisClient.emplace(std::piecewise_construct + , std::forward_as_tuple(dbId) + , std::forward_as_tuple(dbId, m_host, m_port, 0)); + } + else + { + m_redisClient.emplace(std::piecewise_construct + , std::forward_as_tuple(dbId) + , std::forward_as_tuple(dbId, m_unix_socket_path, 0)); + } + } // Keep reconnecting to Database 'dbId' until success diff --git a/common/dbinterface.h b/common/dbinterface.h index 44c0a58d2..c37a7cbce 100644 --- a/common/dbinterface.h +++ b/common/dbinterface.h @@ -29,7 +29,7 @@ class UnavailableDataError : public std::runtime_error const std::string m_data; }; -class DBInterface : public RedisContext +class DBInterface { public: void connect(int dbId, bool retry = true); @@ -42,6 +42,7 @@ class DBInterface : public RedisContext int64_t publish(int dbId, const std::string& channel, const std::string& message); int64_t set(int dbId, const std::string& hash, const std::string& key, const std::string& value, bool blocking = false); DBConnector& get_redis_client(int dbId); + void set_redis_kwargs(std::string unix_socket_path, std::string host, int port); private: template @@ -97,6 +98,10 @@ class DBInterface : public RedisContext std::unordered_map> keyspace_notification_channels; std::unordered_map m_redisClient; + + std::string m_unix_socket_path; + std::string m_host = "127.0.0.1"; + int m_port = 6379; }; } diff --git a/tests/redis_ut.cpp b/tests/redis_ut.cpp index 917020eec..8e086bb18 100644 --- a/tests/redis_ut.cpp +++ b/tests/redis_ut.cpp @@ -13,6 +13,7 @@ #include "common/selectabletimer.h" #include "common/table.h" #include "common/redisclient.h" +#include "common/dbinterface.h" using namespace std; using namespace swss; @@ -311,6 +312,13 @@ TEST(DBConnector, RedisClientName) EXPECT_EQ(db.getClientName(), client_name); } +TEST(DBConnector, DBInterface) +{ + DBInterface dbintf; + dbintf.set_redis_kwargs("", "127.0.0.1", 6379); + dbintf.connect(15); +} + TEST(DBConnector, RedisClient) { DBConnector db("TEST_DB", 0, true); From 9f0eb3e68593aa793a1fc84ed43736fb13e4aab5 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Tue, 29 Sep 2020 06:20:00 +0000 Subject: [PATCH 22/37] Fix LGTM: delete implicitly-declared copy assignment operator --- common/dbconnector.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/common/dbconnector.h b/common/dbconnector.h index 15e6139e8..a31a17573 100644 --- a/common/dbconnector.h +++ b/common/dbconnector.h @@ -84,6 +84,7 @@ class RedisContext RedisContext(const std::string &hostname, int port, unsigned int timeout); RedisContext(const std::string &unixPath, unsigned int timeout); RedisContext(const RedisContext &other); + RedisContext& operator=(const RedisContext&) = delete; ~RedisContext(); @@ -125,6 +126,7 @@ class DBConnector : public RedisContext DBConnector(int dbId, const std::string &unixPath, unsigned int timeout); DBConnector(const std::string &dbName, unsigned int timeout, bool isTcpConn = false); DBConnector(const std::string &dbName, unsigned int timeout, bool isTcpConn, const std::string &netns); + DBConnector& operator=(const DBConnector&) = delete; int getDbId() const; std::string getDbName() const; From e126eb173e988fc37fc0512f7659880ddd3ea302 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Thu, 1 Oct 2020 01:08:17 +0000 Subject: [PATCH 23/37] update DBInterface redis_client index from db_id to db_name Signed-off-by: Qi Luo --- common/dbconnector.cpp | 7 +++ common/dbconnector.h | 2 + common/dbinterface.cpp | 137 ++++++++++++++++++++++------------------- common/dbinterface.h | 38 ++++++------ tests/redis_ut.cpp | 2 +- 5 files changed, 104 insertions(+), 82 deletions(-) diff --git a/common/dbconnector.cpp b/common/dbconnector.cpp index 169851e1d..9e9098a45 100644 --- a/common/dbconnector.cpp +++ b/common/dbconnector.cpp @@ -622,6 +622,13 @@ void DBConnector::set(const string &key, const string &value) RedisReply r(this, sset, REDIS_REPLY_STATUS); } +void DBConnector::config_set(const std::string &key, const std::string &value) +{ + RedisCommand sset; + sset.format("CONFIG SET %s %s", key.c_str(), value.c_str()); + RedisReply r(this, sset, REDIS_REPLY_STATUS); +} + unordered_map DBConnector::hgetall(const string &key) { unordered_map map; diff --git a/common/dbconnector.h b/common/dbconnector.h index a31a17573..06f74e9c5 100644 --- a/common/dbconnector.h +++ b/common/dbconnector.h @@ -177,6 +177,8 @@ class DBConnector : public RedisContext int64_t publish(const std::string &channel, const std::string &message); + void config_set(const std::string &key, const std::string &value); + private: void setNamespace(const std::string &netns); diff --git a/common/dbinterface.cpp b/common/dbinterface.cpp index 708ca36a6..80c6d4147 100644 --- a/common/dbinterface.cpp +++ b/common/dbinterface.cpp @@ -1,4 +1,5 @@ #include +#include #include #include "dbinterface.h" @@ -13,64 +14,64 @@ void DBInterface::set_redis_kwargs(std::string unix_socket_path, std::string hos m_port = port; } -void DBInterface::connect(int dbId, bool retry) +void DBInterface::connect(int dbId, const std::string& dbName, bool retry) { if (retry) { - _persistent_connect(dbId); + _persistent_connect(dbId, dbName); } else { - _onetime_connect(dbId); + _onetime_connect(dbId, dbName); } } -void DBInterface::close(int dbId) +void DBInterface::close(const std::string& dbName) { - m_redisClient.erase(dbId); + m_redisClient.erase(dbName); } -int64_t DBInterface::del(int dbId, const std::string& key, bool blocking) +int64_t DBInterface::del(const string& dbName, const std::string& key, bool blocking) { auto innerfunc = [&] { - return m_redisClient.at(dbId).del(key); + return m_redisClient.at(dbName).del(key); }; - return blockable(innerfunc, dbId, blocking); + return blockable(innerfunc, dbName, blocking); } -bool DBInterface::exists(int dbId, const std::string& key) +bool DBInterface::exists(const string& dbName, const std::string& key) { - return m_redisClient.at(dbId).exists(key); + return m_redisClient.at(dbName).exists(key); } -std::string DBInterface::get(int dbId, const std::string& hash, const std::string& key, bool blocking) +std::string DBInterface::get(const std::string& dbName, const std::string& hash, const std::string& key, bool blocking) { auto innerfunc = [&] { - auto pvalue = m_redisClient.at(dbId).hget(hash, key); + auto pvalue = m_redisClient.at(dbName).hget(hash, key); if (!pvalue) { - std::string message = "Key '" + hash + "' field '" + key + "' unavailable in database '" + std::to_string(dbId) + "'"; + std::string message = "Key '" + hash + "' field '" + key + "' unavailable in database '" + dbName + "'"; SWSS_LOG_WARN("%s", message.c_str()); throw UnavailableDataError(message, hash); } const std::string& value = *pvalue; return value == "None" ? "" : value; }; - return blockable(innerfunc, dbId, blocking); + return blockable(innerfunc, dbName, blocking); } -std::map DBInterface::get_all(int dbId, const std::string& hash, bool blocking) +std::map DBInterface::get_all(const std::string& dbName, const std::string& hash, bool blocking) { auto innerfunc = [&] { std::map map; - m_redisClient.at(dbId).hgetall(hash, std::inserter(map, map.end())); + m_redisClient.at(dbName).hgetall(hash, std::inserter(map, map.end())); if (map.empty()) { - std::string message = "Key '{" + hash + "}' unavailable in database '{" + std::to_string(dbId) + "}'"; + std::string message = "Key '{" + hash + "}' unavailable in database '{" + dbName + "}'"; SWSS_LOG_WARN("%s", message.c_str()); throw UnavailableDataError(message, hash); } @@ -85,48 +86,48 @@ std::map DBInterface::get_all(int dbId, const std::str return map; }; - return blockable>(innerfunc, dbId, blocking); + return blockable>(innerfunc, dbName, blocking); } -std::vector DBInterface::keys(int dbId, const std::string& pattern, bool blocking) +std::vector DBInterface::keys(const std::string& dbName, const std::string& pattern, bool blocking) { auto innerfunc = [&] { - auto keys = m_redisClient.at(dbId).keys(pattern); + auto keys = m_redisClient.at(dbName).keys(pattern); if (keys.empty()) { - std::string message = "DB '{" + std::to_string(dbId) + "}' is empty!"; + std::string message = "DB '{" + dbName + "}' is empty!"; SWSS_LOG_WARN("%s", message.c_str()); throw UnavailableDataError(message, "hset"); } return keys; }; - return blockable>(innerfunc, dbId, blocking); + return blockable>(innerfunc, dbName, blocking); } -int64_t DBInterface::publish(int dbId, const std::string& channel, const std::string& message) +int64_t DBInterface::publish(const std::string& dbName, const std::string& channel, const std::string& message) { - return m_redisClient.at(dbId).publish(channel, message); + return m_redisClient.at(dbName).publish(channel, message); } -int64_t DBInterface::set(int dbId, const std::string& hash, const std::string& key, const std::string& value, bool blocking) +int64_t DBInterface::set(const std::string& dbName, const std::string& hash, const std::string& key, const std::string& value, bool blocking) { auto innerfunc = [&] { - m_redisClient.at(dbId).hset(hash, key, value); + m_redisClient.at(dbName).hset(hash, key, value); // Return the number of fields that were added. return 1; }; - return blockable(innerfunc, dbId, blocking); + return blockable(innerfunc, dbName, blocking); } -DBConnector& DBInterface::get_redis_client(int dbId) +DBConnector& DBInterface::get_redis_client(const std::string& dbName) { - return m_redisClient.at(dbId); + return m_redisClient.at(dbName); } template -T DBInterface::blockable(FUNC f, int dbId, bool blocking) +T DBInterface::blockable(FUNC f, const std::string& dbName, bool blocking) { int attempts = 0; for (;;) @@ -134,31 +135,31 @@ T DBInterface::blockable(FUNC f, int dbId, bool blocking) try { T ret_data = f(); - _unsubscribe_keyspace_notification(dbId); + _unsubscribe_keyspace_notification(dbName); return ret_data; } catch (const UnavailableDataError& e) { if (blocking) { - auto found = keyspace_notification_channels.find(dbId); + auto found = keyspace_notification_channels.find(dbName); if (found != keyspace_notification_channels.end()) { - bool result = _unavailable_data_handler(dbId, e.getData()); + bool result = _unavailable_data_handler(dbName, e.getData()); if (result) { continue; // received updates, try to read data again } else { - _unsubscribe_keyspace_notification(dbId); + _unsubscribe_keyspace_notification(dbName); throw; // No updates was received. Raise exception } } else { // Subscribe to updates and try it again (avoiding race condition) - _subscribe_keyspace_notification(dbId); + _subscribe_keyspace_notification(dbName); } } else @@ -173,15 +174,15 @@ T DBInterface::blockable(FUNC f, int dbId, bool blocking) Retrying the request won't pass unless the schema itself changes. In this case, the error should be attributed to the application itself. Re-raise the error. */ - SWSS_LOG_ERROR("Bad DB request [%d]", dbId); + SWSS_LOG_ERROR("Bad DB request [%s]", dbName.c_str()); throw; } catch (const RedisError&) { // Redis connection broken and we need to retry several times attempts += 1; - _connection_error_handler(dbId); - std::string msg = "DB access failure by [" + std::to_string(dbId) + + "]"; + _connection_error_handler(dbName); + std::string msg = "DB access failure by [" + dbName + + "]"; if (BLOCKING_ATTEMPT_ERROR_THRESHOLD < attempts && attempts < BLOCKING_ATTEMPT_SUPPRESSION) { // Repeated access failures implies the database itself is unhealthy. @@ -196,9 +197,9 @@ T DBInterface::blockable(FUNC f, int dbId, bool blocking) } // Unsubscribe the chosent client from keyspace event notifications -void DBInterface::_unsubscribe_keyspace_notification(int dbId) +void DBInterface::_unsubscribe_keyspace_notification(const std::string& dbName) { - auto found = keyspace_notification_channels.find(dbId); + auto found = keyspace_notification_channels.find(dbName); if (found != keyspace_notification_channels.end()) { SWSS_LOG_DEBUG("Unsubscribe from keyspace notification"); @@ -211,14 +212,14 @@ void DBInterface::_unsubscribe_keyspace_notification(int dbId) // Two timeouts are at work here: // 1. Notification timeout - how long to wait before giving up on receiving any given pub-sub message. // 2. Max data wait - swsssdk-specific. how long to wait for the data to populate (in absolute time) -bool DBInterface::_unavailable_data_handler(int dbId, const char *data) +bool DBInterface::_unavailable_data_handler(const std::string& dbName, const char *data) { auto start = system_clock::now(); - SWSS_LOG_DEBUG("Listening on pubsub channel '%d'", dbId); + SWSS_LOG_DEBUG("Listening on pubsub channel '%s'", dbName.c_str()); auto wait = duration(PUB_SUB_MAXIMUM_DATA_WAIT); while (system_clock::now() - start < wait) { - auto& channel = keyspace_notification_channels.at(dbId); + auto& channel = keyspace_notification_channels.at(dbName); auto ctx = channel->getContext(); redisReply *reply; int rc = redisGetReply(ctx, reinterpret_cast(&reply)); @@ -229,7 +230,7 @@ bool DBInterface::_unavailable_data_handler(int dbId, const char *data) } if (rc != REDIS_OK) { - throw RedisError("Failed to redisGetReply with on pubsub channel on dbId=" + to_string(dbId), ctx); + throw RedisError("Failed to redisGetReply with on pubsub channel on dbName=" + dbName, ctx); } RedisReply r(reply); @@ -247,22 +248,22 @@ bool DBInterface::_unavailable_data_handler(int dbId, const char *data) if (strcmp(r3.str, data) == 0) { - SWSS_LOG_INFO("'%s' acquired via pub-sub dbId=%d. Unblocking...", data, dbId); + SWSS_LOG_INFO("'%s' acquired via pub-sub dbName=%s. Unblocking...", data, dbName.c_str()); // Wait for a "settling" period before releasing the wait. sleep(DATA_RETRIEVAL_WAIT_TIME); return true; } } - SWSS_LOG_WARN("No notification for '%s' from '%d' received before timeout.", data, dbId); + SWSS_LOG_WARN("No notification for '%s' from '%s' received before timeout.", data, dbName.c_str()); return false; } // Subscribe the chosent client to keyspace event notifications -void DBInterface::_subscribe_keyspace_notification(int dbId) +void DBInterface::_subscribe_keyspace_notification(const std::string& dbName) { SWSS_LOG_DEBUG("Subscribe to keyspace notification"); - auto& client = m_redisClient.at(dbId); + auto& client = m_redisClient.at(dbName); DBConnector *pubsub = client.newConnector(0); pubsub->psubscribe(KEYSPACE_PATTERN); @@ -274,50 +275,62 @@ void DBInterface::_subscribe_keyspace_notification(int dbId) throw RedisError("Failed to redisSetTimeout", pubsub->getContext()); } - keyspace_notification_channels.emplace(std::piecewise_construct, std::forward_as_tuple(dbId), std::forward_as_tuple(pubsub)); + keyspace_notification_channels.emplace(std::piecewise_construct, std::forward_as_tuple(dbName), std::forward_as_tuple(pubsub)); } // In the event Redis is unavailable, close existing connections, and try again. -void DBInterface::_connection_error_handler(int dbId) +void DBInterface::_connection_error_handler(const std::string& dbName) { SWSS_LOG_WARN("Could not connect to Redis--waiting before trying again."); - close(dbId); + int dbId = get_redis_client(dbName).getDbId(); + close(dbName); sleep(CONNECT_RETRY_WAIT_TIME); - connect(dbId, true); + connect(dbId, dbName, true); } -void DBInterface::_onetime_connect(int dbId) +void DBInterface::_onetime_connect(int dbId, const string& dbName) { + if (dbName.empty()) + { + throw invalid_argument("dbName"); + } + + pair rc; if (m_unix_socket_path.empty()) { - m_redisClient.emplace(std::piecewise_construct - , std::forward_as_tuple(dbId) + rc = m_redisClient.emplace(std::piecewise_construct + , std::forward_as_tuple(dbName) , std::forward_as_tuple(dbId, m_host, m_port, 0)); } else { - m_redisClient.emplace(std::piecewise_construct - , std::forward_as_tuple(dbId) + rc = m_redisClient.emplace(std::piecewise_construct + , std::forward_as_tuple(dbName) , std::forward_as_tuple(dbId, m_unix_socket_path, 0)); } - + bool inserted = rc.second; + if (inserted) + { + auto redisClient = rc.first->second; + redisClient.config_set("notify-keyspace-events", KEYSPACE_EVENTS); + } } // Keep reconnecting to Database 'dbId' until success -void DBInterface::_persistent_connect(int dbId) +void DBInterface::_persistent_connect(int dbId, const string& dbName) { for (;;) { try { - _onetime_connect(dbId); + _onetime_connect(dbId, dbName); return; } catch (RedisError&) { const int wait = CONNECT_RETRY_WAIT_TIME; - SWSS_LOG_WARN("Connecting to DB '%d' failed, will retry in %d s", dbId, wait); - close(dbId); + SWSS_LOG_WARN("Connecting to DB '%s(%d)' failed, will retry in %d s", dbName.c_str(), dbId, wait); + close(dbName); sleep(wait); } } diff --git a/common/dbinterface.h b/common/dbinterface.h index c37a7cbce..816b5a488 100644 --- a/common/dbinterface.h +++ b/common/dbinterface.h @@ -32,31 +32,31 @@ class UnavailableDataError : public std::runtime_error class DBInterface { public: - void connect(int dbId, bool retry = true); - void close(int dbId); - int64_t del(int dbId, const std::string& key, bool blocking = false); - bool exists(int dbId, const std::string& key); - std::string get(int dbId, const std::string& hash, const std::string& key, bool blocking = false); - std::map get_all(int dbId, const std::string& hash, bool blocking = false); - std::vector keys(int dbId, const std::string& pattern = "*", bool blocking = false); - int64_t publish(int dbId, const std::string& channel, const std::string& message); - int64_t set(int dbId, const std::string& hash, const std::string& key, const std::string& value, bool blocking = false); - DBConnector& get_redis_client(int dbId); + void connect(int dbId, const std::string& dbName, bool retry = true); + void close(const std::string& dbName); + int64_t del(const std::string& dbName, const std::string& key, bool blocking = false); + bool exists(const std::string& dbName, const std::string& key); + std::string get(const std::string& dbName, const std::string& hash, const std::string& key, bool blocking = false); + std::map get_all(const std::string& dbName, const std::string& hash, bool blocking = false); + std::vector keys(const std::string& dbName, const std::string& pattern = "*", bool blocking = false); + int64_t publish(const std::string& dbName, const std::string& channel, const std::string& message); + int64_t set(const std::string& dbName, const std::string& hash, const std::string& key, const std::string& value, bool blocking = false); + DBConnector& get_redis_client(const std::string& dbName); void set_redis_kwargs(std::string unix_socket_path, std::string host, int port); private: template - T blockable(FUNC f, int dbId, bool blocking = false); + T blockable(FUNC f, const std::string& dbName, bool blocking = false); // Unsubscribe the chosent client from keyspace event notifications - void _unsubscribe_keyspace_notification(int dbId); - bool _unavailable_data_handler(int dbId, const char *data); + void _unsubscribe_keyspace_notification(const std::string& dbName); + bool _unavailable_data_handler(const std::string& dbName, const char *data); // Subscribe the chosent client to keyspace event notifications - void _subscribe_keyspace_notification(int dbId); + void _subscribe_keyspace_notification(const std::string& dbName); // In the event Redis is unavailable, close existing connections, and try again. - void _connection_error_handler(int dbId); - void _onetime_connect(int dbId); + void _connection_error_handler(const std::string& dbName); + void _onetime_connect(int dbId, const std::string& dbName); // Keep reconnecting to Database 'dbId' until success - void _persistent_connect(int dbId); + void _persistent_connect(int dbId, const std::string& dbName); static const int BLOCKING_ATTEMPT_ERROR_THRESHOLD = 10; static const int BLOCKING_ATTEMPT_SUPPRESSION = BLOCKING_ATTEMPT_ERROR_THRESHOLD + 5; @@ -95,9 +95,9 @@ class DBInterface // ACS Redis db mainly uses hash, therefore h is selected. static constexpr const char *KEYSPACE_EVENTS = "KEA"; - std::unordered_map> keyspace_notification_channels; + std::unordered_map> keyspace_notification_channels; - std::unordered_map m_redisClient; + std::unordered_map m_redisClient; std::string m_unix_socket_path; std::string m_host = "127.0.0.1"; diff --git a/tests/redis_ut.cpp b/tests/redis_ut.cpp index 8e086bb18..f102d2c25 100644 --- a/tests/redis_ut.cpp +++ b/tests/redis_ut.cpp @@ -316,7 +316,7 @@ TEST(DBConnector, DBInterface) { DBInterface dbintf; dbintf.set_redis_kwargs("", "127.0.0.1", 6379); - dbintf.connect(15); + dbintf.connect(15, "TEST_DB"); } TEST(DBConnector, RedisClient) From c754f151bc43b0bdcad96aded856450435d22f8b Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Thu, 1 Oct 2020 23:43:32 +0000 Subject: [PATCH 24/37] Add DBInterface::delete_all_by_pattern() Signed-off-by: Qi Luo --- common/dbinterface.cpp | 10 ++++++++++ common/dbinterface.h | 2 ++ 2 files changed, 12 insertions(+) diff --git a/common/dbinterface.cpp b/common/dbinterface.cpp index 80c6d4147..4f62f9e7f 100644 --- a/common/dbinterface.cpp +++ b/common/dbinterface.cpp @@ -40,6 +40,16 @@ int64_t DBInterface::del(const string& dbName, const std::string& key, bool bloc return blockable(innerfunc, dbName, blocking); } +void DBInterface::delete_all_by_pattern(const string& dbName, const string& pattern) +{ + auto& client = m_redisClient.at(dbName); + auto keys = client.keys(pattern); + for (auto& key: keys) + { + client.del(key); + } +} + bool DBInterface::exists(const string& dbName, const std::string& key) { return m_redisClient.at(dbName).exists(key); diff --git a/common/dbinterface.h b/common/dbinterface.h index 816b5a488..9098baf3b 100644 --- a/common/dbinterface.h +++ b/common/dbinterface.h @@ -35,6 +35,8 @@ class DBInterface void connect(int dbId, const std::string& dbName, bool retry = true); void close(const std::string& dbName); int64_t del(const std::string& dbName, const std::string& key, bool blocking = false); + // Delete all keys which match %pattern from DB + void delete_all_by_pattern(const std::string& dbName, const std::string& pattern); bool exists(const std::string& dbName, const std::string& key); std::string get(const std::string& dbName, const std::string& hash, const std::string& key, bool blocking = false); std::map get_all(const std::string& dbName, const std::string& hash, bool blocking = false); From d282fbcadd62886f41b10f6d8835e8e0b7872556 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Fri, 2 Oct 2020 17:29:11 +0000 Subject: [PATCH 25/37] Add SonicV2Connector class --- common/dbconnector.cpp | 16 ++++++ common/dbconnector.h | 1 + common/dbinterface.cpp | 2 +- common/dbinterface.h | 113 ++++++++++++++++++++++++++++++++++++++++- 4 files changed, 130 insertions(+), 2 deletions(-) diff --git a/common/dbconnector.cpp b/common/dbconnector.cpp index 9e9098a45..94b1fa1cf 100644 --- a/common/dbconnector.cpp +++ b/common/dbconnector.cpp @@ -361,6 +361,22 @@ vector SonicDBConfig::getNamespaces() return list; } +std::vector SonicDBConfig::getDbList(const std::string &netns) +{ + if (!m_init) + { + initialize(); + } + validateNamespace(netns); + + std::vector dbNames; + for (auto& imap: m_db_info.at(netns)) + { + dbNames.push_back(imap.first); + } + return dbNames; +} + constexpr const char *SonicDBConfig::DEFAULT_SONIC_DB_CONFIG_FILE; constexpr const char *SonicDBConfig::DEFAULT_SONIC_DB_GLOBAL_CONFIG_FILE; unordered_map> SonicDBConfig::m_inst_info; diff --git a/common/dbconnector.h b/common/dbconnector.h index 06f74e9c5..b07b2499d 100644 --- a/common/dbconnector.h +++ b/common/dbconnector.h @@ -47,6 +47,7 @@ class SonicDBConfig static std::string getDbHostname(const std::string &dbName, const std::string &netns = EMPTY_NAMESPACE); static int getDbPort(const std::string &dbName, const std::string &netns = EMPTY_NAMESPACE); static std::vector getNamespaces(); + static std::vector getDbList(const std::string &netns = EMPTY_NAMESPACE); static bool isInit() { return m_init; }; static bool isGlobalInit() { return m_global_init; }; diff --git a/common/dbinterface.cpp b/common/dbinterface.cpp index 4f62f9e7f..dc86ffade 100644 --- a/common/dbinterface.cpp +++ b/common/dbinterface.cpp @@ -99,7 +99,7 @@ std::map DBInterface::get_all(const std::string& dbNam return blockable>(innerfunc, dbName, blocking); } -std::vector DBInterface::keys(const std::string& dbName, const std::string& pattern, bool blocking) +std::vector DBInterface::keys(const std::string& dbName, const char *pattern, bool blocking) { auto innerfunc = [&] { diff --git a/common/dbinterface.h b/common/dbinterface.h index 9098baf3b..b0fc5f785 100644 --- a/common/dbinterface.h +++ b/common/dbinterface.h @@ -40,7 +40,7 @@ class DBInterface bool exists(const std::string& dbName, const std::string& key); std::string get(const std::string& dbName, const std::string& hash, const std::string& key, bool blocking = false); std::map get_all(const std::string& dbName, const std::string& hash, bool blocking = false); - std::vector keys(const std::string& dbName, const std::string& pattern = "*", bool blocking = false); + std::vector keys(const std::string& dbName, const char *pattern = "*", bool blocking = false); int64_t publish(const std::string& dbName, const std::string& channel, const std::string& message); int64_t set(const std::string& dbName, const std::string& hash, const std::string& key, const std::string& value, bool blocking = false); DBConnector& get_redis_client(const std::string& dbName); @@ -106,4 +106,115 @@ class DBInterface int m_port = 6379; }; +class SonicV2Connector +{ +public: + SonicV2Connector(bool use_unix_socket_path = false, const char *netns = "") + : m_use_unix_socket_path(use_unix_socket_path) + , m_netns(netns) + { + } + + void connect(const std::string& db_name, bool retry_on = true) + { + if (m_use_unix_socket_path) + { + SWSS_LOG_INFO("connec1: %s", get_db_socket(db_name).c_str()); + dbintf.set_redis_kwargs(get_db_socket(db_name), "", 0); + } + else + { + SWSS_LOG_INFO("connec2: %s %d", get_db_hostname(db_name).c_str(), get_db_port(db_name)); + dbintf.set_redis_kwargs("", get_db_hostname(db_name), get_db_port(db_name)); + } + int db_id = get_dbid(db_name); + dbintf.connect(db_id, db_name, retry_on); + } + + void close(const std::string& db_name) + { + dbintf.close(db_name); + } + + std::vector get_db_list() + { + return SonicDBConfig::getDbList(m_netns); + } + + int get_dbid(const std::string& db_name) + { + return SonicDBConfig::getDbId(db_name, m_netns); + } + + std::string get_db_separator(const std::string& db_name) + { + return SonicDBConfig::getSeparator(db_name, m_netns); + } + + DBConnector& get_redis_client(const std::string& db_name) + { + return dbintf.get_redis_client(db_name); + } + + int64_t publish(const std::string& db_name, const std::string& channel, const std::string& message) + { + return dbintf.publish(db_name, channel, message); + } + + bool exists(const std::string& db_name, const std::string& key) + { + return dbintf.exists(db_name, key); + } + + std::vector keys(const std::string& db_name, const char *pattern="*") + { + return dbintf.keys(db_name, pattern); + } + + std::string get(const std::string& db_name, const std::string& _hash, const std::string& key) + { + return dbintf.get(db_name, _hash, key); + } + + std::map get_all(const std::string& db_name, const std::string& _hash) + { + return dbintf.get_all(db_name, _hash); + } + + int64_t set(const std::string& db_name, const std::string& _hash, const std::string& key, const std::string& val) + { + return dbintf.set(db_name, _hash, key, val); + } + + int64_t del(const std::string& db_name, const std::string& key) + { + return dbintf.del(db_name, key); + } + + void delete_all_by_pattern(const std::string& db_name, const std::string& pattern) + { + dbintf.delete_all_by_pattern(db_name, pattern); + } + +private: + std::string get_db_socket(const std::string& db_name) + { + return SonicDBConfig::getDbSock(db_name, m_netns); + } + + std::string get_db_hostname(const std::string& db_name) + { + return SonicDBConfig::getDbHostname(db_name, m_netns); + } + + int get_db_port(const std::string& db_name) + { + return SonicDBConfig::getDbPort(db_name, m_netns); + } + + DBInterface dbintf; + bool m_use_unix_socket_path; + std::string m_netns; +}; + } From c530fea3ccbfea4a5c15d6d089f391c7806db0f3 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 3 Oct 2020 00:41:22 +0000 Subject: [PATCH 26/37] Add unit test for SonicV2Connector --- tests/redis_ut.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/redis_ut.cpp b/tests/redis_ut.cpp index f102d2c25..2562e79d6 100644 --- a/tests/redis_ut.cpp +++ b/tests/redis_ut.cpp @@ -317,6 +317,14 @@ TEST(DBConnector, DBInterface) DBInterface dbintf; dbintf.set_redis_kwargs("", "127.0.0.1", 6379); dbintf.connect(15, "TEST_DB"); + + SonicV2Connector db; + db.connect("TEST_DB"); + db.set("TEST_DB", "key0", "field1", "value2"); + auto fvs = db.get_all("TEST_DB", "key0"); + auto rc = fvs.find("field1"); + EXPECT_EQ(rc != fvs.end(), true); + EXPECT_EQ(rc->second, "value2"); } TEST(DBConnector, RedisClient) From fdc256fd7175e800ef2323ce0cadf998b170ef15 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Mon, 5 Oct 2020 17:36:04 +0000 Subject: [PATCH 27/37] Make const strings public because they are used as public method default parameters --- common/dbconnector.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/dbconnector.h b/common/dbconnector.h index b07b2499d..224e60b75 100644 --- a/common/dbconnector.h +++ b/common/dbconnector.h @@ -35,6 +35,8 @@ class SonicDBInfo class SonicDBConfig { public: + static constexpr const char *DEFAULT_SONIC_DB_CONFIG_FILE = "/var/run/redis/sonic-db/database_config.json"; + static constexpr const char *DEFAULT_SONIC_DB_GLOBAL_CONFIG_FILE = "/var/run/redis/sonic-db/database_global.json"; static void initialize(const std::string &file = DEFAULT_SONIC_DB_CONFIG_FILE); static void initializeGlobalConfig(const std::string &file = DEFAULT_SONIC_DB_GLOBAL_CONFIG_FILE); static void validateNamespace(const std::string &netns); @@ -52,8 +54,6 @@ class SonicDBConfig static bool isGlobalInit() { return m_global_init; }; private: - static constexpr const char *DEFAULT_SONIC_DB_CONFIG_FILE = "/var/run/redis/sonic-db/database_config.json"; - static constexpr const char *DEFAULT_SONIC_DB_GLOBAL_CONFIG_FILE = "/var/run/redis/sonic-db/database_global.json"; // { namespace { instName, { unix_socket_path, hostname, port } } } static std::unordered_map> m_inst_info; // { namespace, { dbName, {instName, dbId, separator} } } From d4e442c2f451f654b38b3a8fdb0971391302af43 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Mon, 5 Oct 2020 17:37:56 +0000 Subject: [PATCH 28/37] SWIG supports keyword arguments in generated python module --- pyext/py2/Makefile.am | 2 +- pyext/py3/Makefile.am | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pyext/py2/Makefile.am b/pyext/py2/Makefile.am index abf5064a7..b01c75262 100644 --- a/pyext/py2/Makefile.am +++ b/pyext/py2/Makefile.am @@ -9,6 +9,6 @@ _swsscommon_la_LDFLAGS = -module _swsscommon_la_LIBADD = ../../common/libswsscommon.la -lpython$(PYTHON_VERSION) swsscommon_wrap.cpp: $(SWIG_SOURCES) - $(SWIG) -Wall -c++ -python -I../../common -o $@ $< + $(SWIG) -Wall -c++ -python -keyword -I../../common -o $@ $< CLEANFILES = swsscommon_wrap.cpp diff --git a/pyext/py3/Makefile.am b/pyext/py3/Makefile.am index d6f51b2be..b1a46b3d6 100644 --- a/pyext/py3/Makefile.am +++ b/pyext/py3/Makefile.am @@ -9,6 +9,6 @@ _swsscommon_la_LDFLAGS = -module _swsscommon_la_LIBADD = ../../common/libswsscommon.la $(PYTHON3_BLDLIBRARY) swsscommon_wrap.cpp: $(SWIG_SOURCES) - $(SWIG) -Wall -c++ -python -I../../common -o $@ $< + $(SWIG) -Wall -c++ -python -keyword -I../../common -o $@ $< CLEANFILES = swsscommon_wrap.cpp From 1d628ec4969db9e178b20235099c1eb51bf81de9 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Mon, 5 Oct 2020 19:28:54 +0000 Subject: [PATCH 29/37] Add python namespace property to DBConnector class, solve the paramter conflicting with C++ keyword by customizing python code generation --- common/dbconnector.h | 8 ++++++++ common/dbinterface.h | 8 ++++++++ 2 files changed, 16 insertions(+) diff --git a/common/dbconnector.h b/common/dbconnector.h index 224e60b75..958f78d2d 100644 --- a/common/dbconnector.h +++ b/common/dbconnector.h @@ -133,6 +133,14 @@ class DBConnector : public RedisContext std::string getDbName() const; std::string getNamespace() const; +#ifdef SWIG + %pythoncode %{ + __swig_getmethods__["namespace"] = getNamespace + __swig_setmethods__["namespace"] = None + if _newclass: namespace = property(getNamespace, None) + %} +#endif + static void select(DBConnector *db); /* Create new context to DB */ diff --git a/common/dbinterface.h b/common/dbinterface.h index b0fc5f785..5bbc480f9 100644 --- a/common/dbinterface.h +++ b/common/dbinterface.h @@ -109,11 +109,19 @@ class DBInterface class SonicV2Connector { public: +#ifdef SWIG + %pythoncode %{ + def __init__(self, use_unix_socket_path = False, namespace = None): + self.m_use_unix_socket_path = use_unix_socket_path + self.m_netns = namespace + %} +#else SonicV2Connector(bool use_unix_socket_path = false, const char *netns = "") : m_use_unix_socket_path(use_unix_socket_path) , m_netns(netns) { } +#endif void connect(const std::string& db_name, bool retry_on = true) { From 8c93402a83ada4a9766b3d8008a308525784466a Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Mon, 5 Oct 2020 23:26:30 +0000 Subject: [PATCH 30/37] Move SonicV2Connector to standalone .h/.cpp files Signed-off-by: Qi Luo --- common/Makefile.am | 1 + common/dbinterface.h | 119 ------------------------------------ common/sonicv2connector.cpp | 107 ++++++++++++++++++++++++++++++++ common/sonicv2connector.h | 64 +++++++++++++++++++ tests/redis_ut.cpp | 1 + 5 files changed, 173 insertions(+), 119 deletions(-) create mode 100644 common/sonicv2connector.cpp create mode 100644 common/sonicv2connector.h diff --git a/common/Makefile.am b/common/Makefile.am index b2ec1b53e..32532736a 100644 --- a/common/Makefile.am +++ b/common/Makefile.am @@ -30,6 +30,7 @@ libswsscommon_la_SOURCES = \ redisreply.cpp \ dbconnector.cpp \ dbinterface.cpp \ + sonicv2connector.cpp \ table.cpp \ json.cpp \ producertable.cpp \ diff --git a/common/dbinterface.h b/common/dbinterface.h index 5bbc480f9..12a249578 100644 --- a/common/dbinterface.h +++ b/common/dbinterface.h @@ -106,123 +106,4 @@ class DBInterface int m_port = 6379; }; -class SonicV2Connector -{ -public: -#ifdef SWIG - %pythoncode %{ - def __init__(self, use_unix_socket_path = False, namespace = None): - self.m_use_unix_socket_path = use_unix_socket_path - self.m_netns = namespace - %} -#else - SonicV2Connector(bool use_unix_socket_path = false, const char *netns = "") - : m_use_unix_socket_path(use_unix_socket_path) - , m_netns(netns) - { - } -#endif - - void connect(const std::string& db_name, bool retry_on = true) - { - if (m_use_unix_socket_path) - { - SWSS_LOG_INFO("connec1: %s", get_db_socket(db_name).c_str()); - dbintf.set_redis_kwargs(get_db_socket(db_name), "", 0); - } - else - { - SWSS_LOG_INFO("connec2: %s %d", get_db_hostname(db_name).c_str(), get_db_port(db_name)); - dbintf.set_redis_kwargs("", get_db_hostname(db_name), get_db_port(db_name)); - } - int db_id = get_dbid(db_name); - dbintf.connect(db_id, db_name, retry_on); - } - - void close(const std::string& db_name) - { - dbintf.close(db_name); - } - - std::vector get_db_list() - { - return SonicDBConfig::getDbList(m_netns); - } - - int get_dbid(const std::string& db_name) - { - return SonicDBConfig::getDbId(db_name, m_netns); - } - - std::string get_db_separator(const std::string& db_name) - { - return SonicDBConfig::getSeparator(db_name, m_netns); - } - - DBConnector& get_redis_client(const std::string& db_name) - { - return dbintf.get_redis_client(db_name); - } - - int64_t publish(const std::string& db_name, const std::string& channel, const std::string& message) - { - return dbintf.publish(db_name, channel, message); - } - - bool exists(const std::string& db_name, const std::string& key) - { - return dbintf.exists(db_name, key); - } - - std::vector keys(const std::string& db_name, const char *pattern="*") - { - return dbintf.keys(db_name, pattern); - } - - std::string get(const std::string& db_name, const std::string& _hash, const std::string& key) - { - return dbintf.get(db_name, _hash, key); - } - - std::map get_all(const std::string& db_name, const std::string& _hash) - { - return dbintf.get_all(db_name, _hash); - } - - int64_t set(const std::string& db_name, const std::string& _hash, const std::string& key, const std::string& val) - { - return dbintf.set(db_name, _hash, key, val); - } - - int64_t del(const std::string& db_name, const std::string& key) - { - return dbintf.del(db_name, key); - } - - void delete_all_by_pattern(const std::string& db_name, const std::string& pattern) - { - dbintf.delete_all_by_pattern(db_name, pattern); - } - -private: - std::string get_db_socket(const std::string& db_name) - { - return SonicDBConfig::getDbSock(db_name, m_netns); - } - - std::string get_db_hostname(const std::string& db_name) - { - return SonicDBConfig::getDbHostname(db_name, m_netns); - } - - int get_db_port(const std::string& db_name) - { - return SonicDBConfig::getDbPort(db_name, m_netns); - } - - DBInterface dbintf; - bool m_use_unix_socket_path; - std::string m_netns; -}; - } diff --git a/common/sonicv2connector.cpp b/common/sonicv2connector.cpp new file mode 100644 index 000000000..433947ae3 --- /dev/null +++ b/common/sonicv2connector.cpp @@ -0,0 +1,107 @@ +#include "sonicv2connector.h" +#include "dbconnector.h" +#include "logger.h" + +using namespace swss; + +SonicV2Connector::SonicV2Connector(bool use_unix_socket_path, const char *netns) + : m_use_unix_socket_path(use_unix_socket_path) + , m_netns(netns) +{ +} + +void SonicV2Connector::connect(const std::string& db_name, bool retry_on) +{ + if (m_use_unix_socket_path) + { + SWSS_LOG_INFO("connec1: %s", get_db_socket(db_name).c_str()); + m_dbintf.set_redis_kwargs(get_db_socket(db_name), "", 0); + } + else + { + SWSS_LOG_INFO("connec2: %s %d", get_db_hostname(db_name).c_str(), get_db_port(db_name)); + m_dbintf.set_redis_kwargs("", get_db_hostname(db_name), get_db_port(db_name)); + } + int db_id = get_dbid(db_name); + m_dbintf.connect(db_id, db_name, retry_on); +} + +void SonicV2Connector::close(const std::string& db_name) +{ + m_dbintf.close(db_name); +} + +std::vector SonicV2Connector::get_db_list() +{ + return SonicDBConfig::getDbList(m_netns); +} + +int SonicV2Connector::get_dbid(const std::string& db_name) +{ + return SonicDBConfig::getDbId(db_name, m_netns); +} + +std::string SonicV2Connector::get_db_separator(const std::string& db_name) +{ + return SonicDBConfig::getSeparator(db_name, m_netns); +} + +DBConnector& SonicV2Connector::get_redis_client(const std::string& db_name) +{ + return m_dbintf.get_redis_client(db_name); +} + +int64_t SonicV2Connector::publish(const std::string& db_name, const std::string& channel, const std::string& message) +{ + return m_dbintf.publish(db_name, channel, message); +} + +bool SonicV2Connector::exists(const std::string& db_name, const std::string& key) +{ + return m_dbintf.exists(db_name, key); +} + +std::vector SonicV2Connector::keys(const std::string& db_name, const char *pattern) +{ + return m_dbintf.keys(db_name, pattern); +} + +std::string SonicV2Connector::get(const std::string& db_name, const std::string& _hash, const std::string& key) +{ + return m_dbintf.get(db_name, _hash, key); +} + +std::map SonicV2Connector::get_all(const std::string& db_name, const std::string& _hash) +{ + return m_dbintf.get_all(db_name, _hash); +} + +int64_t SonicV2Connector::set(const std::string& db_name, const std::string& _hash, const std::string& key, const std::string& val) +{ + return m_dbintf.set(db_name, _hash, key, val); +} + +int64_t SonicV2Connector::del(const std::string& db_name, const std::string& key) +{ + return m_dbintf.del(db_name, key); +} + +void SonicV2Connector::delete_all_by_pattern(const std::string& db_name, const std::string& pattern) +{ + m_dbintf.delete_all_by_pattern(db_name, pattern); +} + +std::string SonicV2Connector::get_db_socket(const std::string& db_name) +{ + return SonicDBConfig::getDbSock(db_name, m_netns); +} + +std::string SonicV2Connector::get_db_hostname(const std::string& db_name) +{ + return SonicDBConfig::getDbHostname(db_name, m_netns); +} + +int SonicV2Connector::get_db_port(const std::string& db_name) +{ + return SonicDBConfig::getDbPort(db_name, m_netns); +} diff --git a/common/sonicv2connector.h b/common/sonicv2connector.h new file mode 100644 index 000000000..545339137 --- /dev/null +++ b/common/sonicv2connector.h @@ -0,0 +1,64 @@ +#pragma once + +#include +#include + +#include "dbinterface.h" + +namespace swss +{ + +class SonicV2Connector +{ +public: +#ifdef SWIG + %pythoncode %{ + def __init__(self, use_unix_socket_path = False, namespace = None): + self.m_use_unix_socket_path = use_unix_socket_path + self.m_netns = namespace + %} +#else + SonicV2Connector(bool use_unix_socket_path = false, const char *netns = ""); +#endif + + void connect(const std::string& db_name, bool retry_on = true); + + void close(const std::string& db_name); + + std::vector get_db_list(); + + int get_dbid(const std::string& db_name); + + std::string get_db_separator(const std::string& db_name); + + DBConnector& get_redis_client(const std::string& db_name); + + int64_t publish(const std::string& db_name, const std::string& channel, const std::string& message); + + bool exists(const std::string& db_name, const std::string& key); + + std::vector keys(const std::string& db_name, const char *pattern="*"); + + std::string get(const std::string& db_name, const std::string& _hash, const std::string& key); + + std::map get_all(const std::string& db_name, const std::string& _hash); + + int64_t set(const std::string& db_name, const std::string& _hash, const std::string& key, const std::string& val); + + int64_t del(const std::string& db_name, const std::string& key); + + void delete_all_by_pattern(const std::string& db_name, const std::string& pattern); + +private: + std::string get_db_socket(const std::string& db_name); + + std::string get_db_hostname(const std::string& db_name); + + int get_db_port(const std::string& db_name); + + DBInterface m_dbintf; + bool m_use_unix_socket_path; + std::string m_netns; +}; + +} diff --git a/tests/redis_ut.cpp b/tests/redis_ut.cpp index 2562e79d6..4ba1cf223 100644 --- a/tests/redis_ut.cpp +++ b/tests/redis_ut.cpp @@ -14,6 +14,7 @@ #include "common/table.h" #include "common/redisclient.h" #include "common/dbinterface.h" +#include "common/sonicv2connector.h" using namespace std; using namespace swss; From f84f07b718b3bbbb80d49a85eff13a39e3d65ab0 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Wed, 7 Oct 2020 04:47:22 +0000 Subject: [PATCH 31/37] Add missing include statements into SWIG inteface file --- pyext/swsscommon.i | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pyext/swsscommon.i b/pyext/swsscommon.i index b03b40104..a252aa077 100644 --- a/pyext/swsscommon.i +++ b/pyext/swsscommon.i @@ -4,6 +4,7 @@ #include "schema.h" #include "dbconnector.h" #include "dbinterface.h" +#include "sonicv2connector.h" #include "select.h" #include "selectable.h" #include "rediscommand.h" @@ -56,6 +57,7 @@ swss::RedisSelect *CastSelectableToRedisSelectObj(swss::Selectable *temp) { %include "schema.h" %include "dbconnector.h" +%include "sonicv2connector.h" %include "selectable.h" %include "select.h" %include "rediscommand.h" From 3a761e61f2cee336bfca6da83224bacebfc9b675 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Wed, 7 Oct 2020 19:42:38 +0000 Subject: [PATCH 32/37] Add pytest unit test for DBInterface and SonicV2Connector --- pyext/swsscommon.i | 2 ++ tests/test_redis_ut.py | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/pyext/swsscommon.i b/pyext/swsscommon.i index a252aa077..c0102c3c5 100644 --- a/pyext/swsscommon.i +++ b/pyext/swsscommon.i @@ -26,11 +26,13 @@ %include %include %include +%include %include %include %template(FieldValuePair) std::pair; %template(FieldValuePairs) std::vector>; +%template(FieldValueMap) std::map; %template(VectorString) std::vector; %apply int *OUTPUT {int *fd}; diff --git a/tests/test_redis_ut.py b/tests/test_redis_ut.py index aecf0a19a..b09364b6c 100644 --- a/tests/test_redis_ut.py +++ b/tests/test_redis_ut.py @@ -1,7 +1,17 @@ import time +import pytest from threading import Thread from pympler.tracker import SummaryTracker from swsscommon import swsscommon +from swsscommon.swsscommon import DBInterface, SonicV2Connector, SonicDBConfig + +existing_file = "./tests/redis_multi_db_ut_config/database_config.json" +nonexisting_file = "./tests/redis_multi_db_ut_config/database_config_nonexisting.json" +global_existing_file = "./tests/redis_multi_db_ut_config/database_global.json" + +@pytest.fixture(scope="session", autouse=True) +def prepare(request): + SonicDBConfig.initialize(existing_file) def test_ProducerTable(): db = swsscommon.DBConnector("APPL_DB", 0, True) @@ -122,3 +132,16 @@ def generator_SelectMemoryLeak(): cases.append("%s - %d objects for %d repeats" % (name, count, N)) thr.join() assert not cases + + +def test_DBInterface(): + dbintf = DBInterface() + dbintf.set_redis_kwargs("", "127.0.0.1", 6379) + dbintf.connect(15, "TEST_DB") + + db = SonicV2Connector() + db.connect("TEST_DB") + db.set("TEST_DB", "key0", "field1", "value2") + fvs = db.get_all("TEST_DB", "key0") + assert "field1" in fvs + assert fvs["field1"] == "value2" From 04545d7bdaebe01bf1d22698cd31260fe1245ecc Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Wed, 7 Oct 2020 23:18:26 +0000 Subject: [PATCH 33/37] Fix swig customization on SonicV2Connector ctor --- common/sonicv2connector.h | 20 ++++++++++++-------- tests/test_redis_ut.py | 2 +- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/common/sonicv2connector.h b/common/sonicv2connector.h index 545339137..f672ad2ab 100644 --- a/common/sonicv2connector.h +++ b/common/sonicv2connector.h @@ -11,15 +11,7 @@ namespace swss class SonicV2Connector { public: -#ifdef SWIG - %pythoncode %{ - def __init__(self, use_unix_socket_path = False, namespace = None): - self.m_use_unix_socket_path = use_unix_socket_path - self.m_netns = namespace - %} -#else SonicV2Connector(bool use_unix_socket_path = false, const char *netns = ""); -#endif void connect(const std::string& db_name, bool retry_on = true); @@ -61,4 +53,16 @@ class SonicV2Connector std::string m_netns; }; +#ifdef SWIG +// TRICK! +// Note: there is no easy way for SWIG to map ctor parameter netns(C++) to namespace(python), +// so we use python patch to achieve this +// TODO: implement it with formal SWIG syntax, which will be target language independent +%pythoncode %{ + _old_SonicV2Connector__init__ = SonicV2Connector.__init__ + def _new_SonicV2Connector__init__(self, use_unix_socket_path = False, namespace = None): + _old_SonicV2Connector__init__(self, use_unix_socket_path = use_unix_socket_path, netns = namespace) + SonicV2Connector.__init__ = _new_SonicV2Connector__init__ +%} +#endif } diff --git a/tests/test_redis_ut.py b/tests/test_redis_ut.py index b09364b6c..cca92731f 100644 --- a/tests/test_redis_ut.py +++ b/tests/test_redis_ut.py @@ -139,7 +139,7 @@ def test_DBInterface(): dbintf.set_redis_kwargs("", "127.0.0.1", 6379) dbintf.connect(15, "TEST_DB") - db = SonicV2Connector() + db = SonicV2Connector(use_unix_socket_path=True, namespace='') db.connect("TEST_DB") db.set("TEST_DB", "key0", "field1", "value2") fvs = db.get_all("TEST_DB", "key0") From 2c8b889a6ea9471ad1e2c95bf173a44f19d18f4c Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Wed, 7 Oct 2020 23:43:17 +0000 Subject: [PATCH 34/37] Add attrib SonicV2Connector.namespace --- common/sonicv2connector.cpp | 5 +++++ common/sonicv2connector.h | 10 ++++++++++ tests/test_redis_ut.py | 1 + 3 files changed, 16 insertions(+) diff --git a/common/sonicv2connector.cpp b/common/sonicv2connector.cpp index 433947ae3..364a452f6 100644 --- a/common/sonicv2connector.cpp +++ b/common/sonicv2connector.cpp @@ -10,6 +10,11 @@ SonicV2Connector::SonicV2Connector(bool use_unix_socket_path, const char *netns) { } +std::string SonicV2Connector::getNamespace() const +{ + return m_netns; +} + void SonicV2Connector::connect(const std::string& db_name, bool retry_on) { if (m_use_unix_socket_path) diff --git a/common/sonicv2connector.h b/common/sonicv2connector.h index f672ad2ab..40a98c564 100644 --- a/common/sonicv2connector.h +++ b/common/sonicv2connector.h @@ -13,6 +13,16 @@ class SonicV2Connector public: SonicV2Connector(bool use_unix_socket_path = false, const char *netns = ""); + std::string getNamespace() const; + +#ifdef SWIG + %pythoncode %{ + __swig_getmethods__["namespace"] = getNamespace + __swig_setmethods__["namespace"] = None + if _newclass: namespace = property(getNamespace, None) + %} +#endif + void connect(const std::string& db_name, bool retry_on = true); void close(const std::string& db_name); diff --git a/tests/test_redis_ut.py b/tests/test_redis_ut.py index cca92731f..a0ae4a6a1 100644 --- a/tests/test_redis_ut.py +++ b/tests/test_redis_ut.py @@ -140,6 +140,7 @@ def test_DBInterface(): dbintf.connect(15, "TEST_DB") db = SonicV2Connector(use_unix_socket_path=True, namespace='') + assert db.namespace == '' db.connect("TEST_DB") db.set("TEST_DB", "key0", "field1", "value2") fvs = db.get_all("TEST_DB", "key0") From 11658b9c88a1ac6b79b3560121acfc38df75b926 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Thu, 8 Oct 2020 00:16:42 +0000 Subject: [PATCH 35/37] Remove debug code --- common/sonicv2connector.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/common/sonicv2connector.cpp b/common/sonicv2connector.cpp index 364a452f6..8ca1af634 100644 --- a/common/sonicv2connector.cpp +++ b/common/sonicv2connector.cpp @@ -19,12 +19,10 @@ void SonicV2Connector::connect(const std::string& db_name, bool retry_on) { if (m_use_unix_socket_path) { - SWSS_LOG_INFO("connec1: %s", get_db_socket(db_name).c_str()); m_dbintf.set_redis_kwargs(get_db_socket(db_name), "", 0); } else { - SWSS_LOG_INFO("connec2: %s %d", get_db_hostname(db_name).c_str(), get_db_port(db_name)); m_dbintf.set_redis_kwargs("", get_db_hostname(db_name), get_db_port(db_name)); } int db_id = get_dbid(db_name); From 832ace790e73c0313c9197e313b077198d408c9f Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Thu, 8 Oct 2020 00:19:24 +0000 Subject: [PATCH 36/37] Use EXPECT_NE to simplify test --- tests/redis_ut.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/redis_ut.cpp b/tests/redis_ut.cpp index 4ba1cf223..9d8becf00 100644 --- a/tests/redis_ut.cpp +++ b/tests/redis_ut.cpp @@ -324,7 +324,7 @@ TEST(DBConnector, DBInterface) db.set("TEST_DB", "key0", "field1", "value2"); auto fvs = db.get_all("TEST_DB", "key0"); auto rc = fvs.find("field1"); - EXPECT_EQ(rc != fvs.end(), true); + EXPECT_NE(rc, fvs.end()); EXPECT_EQ(rc->second, "value2"); } From b789a219566a80bd8ac7a2d8d0f370a1c4a11045 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Mon, 12 Oct 2020 19:15:22 +0000 Subject: [PATCH 37/37] Remove unused code --- tests/test_redis_ut.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/test_redis_ut.py b/tests/test_redis_ut.py index a0ae4a6a1..2db614cd3 100644 --- a/tests/test_redis_ut.py +++ b/tests/test_redis_ut.py @@ -6,8 +6,6 @@ from swsscommon.swsscommon import DBInterface, SonicV2Connector, SonicDBConfig existing_file = "./tests/redis_multi_db_ut_config/database_config.json" -nonexisting_file = "./tests/redis_multi_db_ut_config/database_config_nonexisting.json" -global_existing_file = "./tests/redis_multi_db_ut_config/database_global.json" @pytest.fixture(scope="session", autouse=True) def prepare(request):