Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

DBConnector classes to understand the namespace. #364

Merged
merged 11 commits into from
Jul 24, 2020
Merged

DBConnector classes to understand the namespace. #364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
42d9913
DBConnector classes to understand the namespace.
judyjoseph Jul 8, 2020
7918b4a
Updates to the initial commit
judyjoseph Jul 10, 2020
77e8918
Updates for comments
judyjoseph Jul 14, 2020
b188047
Add UT tests for the namespace usecases
judyjoseph Jul 17, 2020
bbbf1d3
Updates to the unit tests
judyjoseph Jul 20, 2020
b39d0dc
Derive the DIR of the config files from the file path itself.
judyjoseph Jul 20, 2020
ed202a1
Fixes when testing and
abdosi Jul 20, 2020
d79bcd3
Added Support to get DB Id and DB Namespace from Selectable Object.
abdosi Jul 20, 2020
a7898c3
Review comments update, for the variable name.
judyjoseph Jul 20, 2020
92eacae
Further fixes and updating unit tests
judyjoseph Jul 21, 2020
4be4c39
Renaming the newly added function in swss::RedisSelect to getDbConnec…
judyjoseph Jul 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
250 changes: 191 additions & 59 deletions common/dbconnector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,11 @@ using namespace std;

namespace swss {

void SonicDBConfig::initialize(const string &file)
void SonicDBConfig::parseDatabaseConfig(const string &file,
std::unordered_map<std::string, RedisInstInfo> &inst_entry,
std::unordered_map<std::string, SonicDBInfo> &db_entry,
std::unordered_map<int, std::string> &separator_entry)
{

SWSS_LOG_ENTER();

if (m_init)
{
SWSS_LOG_ERROR("SonicDBConfig already initialized");
throw runtime_error("SonicDBConfig already initialized");
}

ifstream i(file);
if (i.good())
{
Expand All @@ -40,7 +34,7 @@ void SonicDBConfig::initialize(const string &file)
string socket = it.value().at("unix_socket_path");
string hostname = it.value().at("hostname");
int port = it.value().at("port");
m_inst_info[instName] = {socket, {hostname, port}};
inst_entry[instName] = {socket, hostname, port};
}

for (auto it = j["DATABASES"].begin(); it!= j["DATABASES"].end(); it++)
Expand All @@ -49,12 +43,12 @@ void SonicDBConfig::initialize(const string &file)
string instName = it.value().at("instance");
int dbId = it.value().at("id");
string separator = it.value().at("separator");
m_db_info[dbName] = {instName, dbId, separator};
db_entry[dbName] = {instName, dbId, separator};

m_db_separator.emplace(dbId, separator);
separator_entry.emplace(dbId, separator);
}
m_init = true;
}

catch (domain_error& e)
{
SWSS_LOG_ERROR("key doesn't exist in json object, NULL value has no iterator >> %s\n", e.what());
Expand All @@ -73,32 +67,152 @@ void SonicDBConfig::initialize(const string &file)
}
}

string SonicDBConfig::getDbInst(const string &dbName)
void SonicDBConfig::initializeGlobalConfig(const string &file)
Copy link
Contributor

Choose a reason for hiding this comment

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

@judyjoseph Behavior is different when calling this api w.r.t sonic-py-swssdk for single-asic platforms.
In python version this is NOOP on single asic platforms but here it throws run-time exception of file not present.

I feel we should have same behavior.

For now we need to add check in caller to make sure to call this api for multi-asic platforms only.

Copy link
Collaborator Author

@judyjoseph judyjoseph Jul 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes it is different here.

No need to explicitly call SonicDBConfig::initializeGlobalConfig API. If the user db_connects with a namespace parameter it loads the Global config file automatically (https://github.com/judyjoseph/sonic-swss-common/blob/4be4c3970f8f70523ef85bc7e56b19f9b2d2b1d3/common/dbconnector.cpp#L184)

The next time he calls it will be a NOOP as this variable m_global_init is set already. I felt this will be easier to user ?

If user needs to call this API SonicDBConfig::initializeGlobalConfig() explicitly he need to make sure the file exists.

Copy link
Contributor

Choose a reason for hiding this comment

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

@judyjoseph
a) why it has to be different ? It has be consistent . Any particular reason why it need to be different ? Application should be doing same irrespective of library being used.

b)Also it does not work if we don't call SonicDBConfig::initializeGlobalConfig.
DBConnector Constructor with particular asic namespace gives seg fault
as SonicDBConfig::getDbId will fail since m_init will be set when call for host/global namespace and when next time called
it will not go for else condition (since m_init is already set)

https://github.com/judyjoseph/sonic-swss-common/blob/4be4c3970f8f70523ef85bc7e56b19f9b2d2b1d3/common/dbconnector.cpp#L336

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok got the use-case, thanks ! here in this case the same application we invoke with the global namespace followed by the namespace specific ones. Will update to make it similar to how we have it in sonic-py-swssdk of explicitly calling SonicDBConfig::initializeGlobalConfig().

Copy link
Contributor

Choose a reason for hiding this comment

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

@judyjoseph is it possible to add test-case to cover above scenario ?

{
std::string local_file, dir_name, ns_name;
std::unordered_map<std::string, SonicDBInfo> db_entry;
std::unordered_map<std::string, RedisInstInfo> inst_entry;
std::unordered_map<int, std::string> separator_entry;

SWSS_LOG_ENTER();

if (m_global_init)
{
SWSS_LOG_ERROR("SonicDBConfig Global config is already initialized");
return;
}


ifstream i(file);
if (i.good())
{
local_file = dir_name = std::string();

// Get the directory name from the file path given as input.
std::string::size_type pos = file.rfind("/");
if( pos != std::string::npos)
{
dir_name = file.substr(0,pos+1);
}

try
{
json j;
i >> j;

for (auto& element : j["INCLUDES"])
{
local_file.append(dir_name);
local_file.append(element["include"]);

if(element["namespace"].empty())
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw your changes, that fix one case, when initializeGlobalConfig() invoked first, initialize() won't parse empty case again. But when initialize() invoked first with empty namespace and then initializeGlobalConfig() is invoked, there is no check on m_init before handing empty case, double parsing again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, Dong -- I am currently adding that as well , thanks !

{
// If database_config.json is already initlized via SonicDBConfig::initialize
// skip initializing it here again.
if(m_init)
{
local_file.clear();
continue;
}
ns_name = EMPTY_NAMESPACE;
Copy link
Contributor

Choose a reason for hiding this comment

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

for the empty case, do we need set m_init somewhere ? otherwise, when using get func, it parse the database_config.json again ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here in this case when element["namespace"].empty(), I could add a check to see if the m_init flag is already set -- IF so don't do anything, as it is already loaded explicitly from database_config.json file earlier. Please let me know if this was what you were referring to ?

Copy link
Contributor

@dzhangalibaba dzhangalibaba Jul 20, 2020 •
edited by judyjoseph
Loading

Choose a reason for hiding this comment

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

I saw you added the m_init = true at the last. Just to confirm with you, database_global_config.json always has the namesapce EMPTY case in file, right ?

Thanks @abdosi for adding this as part of couple of fixes. Yes the database_global_config will have the first entry for the EMPTY namespace.

}
else
{
ns_name = element["namespace"];
}

parseDatabaseConfig(local_file, inst_entry, db_entry, separator_entry);
m_inst_info[ns_name] = inst_entry;
m_db_info[ns_name] = db_entry;
m_db_separator[ns_name] = separator_entry;

inst_entry.clear();
db_entry.clear();
separator_entry.clear();
local_file.clear();
}
}

catch (domain_error& e)
{
SWSS_LOG_ERROR("key doesn't exist in json object, NULL value has no iterator >> %s\n", e.what());
throw runtime_error("key doesn't exist in json object, NULL value has no iterator >> " + string(e.what()));
}
catch (exception &e)
{
SWSS_LOG_ERROR("Sonic database config file syntax error >> %s\n", e.what());
throw runtime_error("Sonic database config file syntax error >> " + string(e.what()));
}
}
else
{
SWSS_LOG_ERROR("Sonic database global config file doesn't exist at %s\n", file.c_str());
throw runtime_error("Sonic database global config file doesn't exist at " + file);
}

// Set it as the global config file is already parsed and init done.
m_global_init = true;

// Make regular init also done
m_init = true;
}

void SonicDBConfig::initialize(const string &file, const string &nameSpace)
{
std::unordered_map<std::string, SonicDBInfo> db_entry;
std::unordered_map<std::string, RedisInstInfo> inst_entry;
std::unordered_map<int, std::string> separator_entry;

SWSS_LOG_ENTER();

if (m_init)
{
SWSS_LOG_ERROR("SonicDBConfig already initialized");
throw runtime_error("SonicDBConfig already initialized");
}

// namespace string is empty, use the file given as input to parse.
if(nameSpace.empty())
{
parseDatabaseConfig(file, inst_entry, db_entry, separator_entry);
m_inst_info[EMPTY_NAMESPACE] = inst_entry;
m_db_info[EMPTY_NAMESPACE] = db_entry;
m_db_separator[EMPTY_NAMESPACE] = separator_entry;
}
else
// namespace is not empty, use DEFAULT_SONIC_DB_GLOBAL_CONFIG_FILE.
initializeGlobalConfig();

// Set it as the config file is already parsed and init done.
m_init = true;
}

string SonicDBConfig::getDbInst(const string &dbName, const string &nameSpace)
{
if (!m_init)
initialize();
return m_db_info.at(dbName).instName;
initialize(DEFAULT_SONIC_DB_CONFIG_FILE, nameSpace);
return m_db_info[nameSpace].at(dbName).instName;
}

int SonicDBConfig::getDbId(const string &dbName)
int SonicDBConfig::getDbId(const string &dbName, const string &nameSpace)
{
if (!m_init)
initialize();
return m_db_info.at(dbName).dbId;
initialize(DEFAULT_SONIC_DB_CONFIG_FILE, nameSpace);
return m_db_info[nameSpace].at(dbName).dbId;
}

string SonicDBConfig::getSeparator(const string &dbName)
string SonicDBConfig::getSeparator(const string &dbName, const string &nameSpace)
{
if (!m_init)
initialize();
return m_db_info.at(dbName).separator;
initialize(DEFAULT_SONIC_DB_CONFIG_FILE, nameSpace);
return m_db_info[nameSpace].at(dbName).separator;
}

string SonicDBConfig::getSeparator(int dbId)
string SonicDBConfig::getSeparator(int dbId, const string &nameSpace)
{
if (!m_init)
initialize();
return m_db_separator.at(dbId);
initialize(DEFAULT_SONIC_DB_CONFIG_FILE, nameSpace);
return m_db_separator[nameSpace].at(dbId);
}

string SonicDBConfig::getSeparator(const DBConnector* db)
Expand All @@ -109,42 +223,61 @@ string SonicDBConfig::getSeparator(const DBConnector* db)
}

string dbName = db->getDbName();
string nameSpace = db->getNamespace();
if (dbName.empty())
{
return getSeparator(db->getDbId());
return getSeparator(db->getDbId(), nameSpace);
}
else
{
return getSeparator(dbName);
return getSeparator(dbName, nameSpace);
}
}

string SonicDBConfig::getDbSock(const string &dbName)
string SonicDBConfig::getDbSock(const string &dbName, const string &nameSpace)
{
if (!m_init)
initialize();
return m_inst_info.at(getDbInst(dbName)).first;
initialize(DEFAULT_SONIC_DB_CONFIG_FILE, nameSpace);
return m_inst_info[nameSpace].at(getDbInst(dbName)).unixSocketPath;
}

string SonicDBConfig::getDbHostname(const string &dbName)
string SonicDBConfig::getDbHostname(const string &dbName, const string &nameSpace)
{
if (!m_init)
initialize();
return m_inst_info.at(getDbInst(dbName)).second.first;
initialize(DEFAULT_SONIC_DB_CONFIG_FILE, nameSpace);
return m_inst_info[nameSpace].at(getDbInst(dbName)).hostname;
}

int SonicDBConfig::getDbPort(const string &dbName)
int SonicDBConfig::getDbPort(const string &dbName, const string &nameSpace)
{
if (!m_init)
initialize();
return m_inst_info.at(getDbInst(dbName)).second.second;
initialize(DEFAULT_SONIC_DB_CONFIG_FILE, nameSpace);
return m_inst_info[nameSpace].at(getDbInst(dbName)).port;
}

vector<string> SonicDBConfig::getNamespaces()
{
vector<string> list;

if (!m_global_init)
initializeGlobalConfig();

// This API returns back non-empty namespaces.
for (auto it = m_inst_info.cbegin(); it != m_inst_info.cend(); ++it) {
if(!((it->first).empty()))
list.push_back(it->first);
}

return list;
}

constexpr const char *SonicDBConfig::DEFAULT_SONIC_DB_CONFIG_FILE;
unordered_map<string, pair<string, pair<string, int>>> SonicDBConfig::m_inst_info;
unordered_map<string, SonicDBInfo> SonicDBConfig::m_db_info;
unordered_map<int, string> SonicDBConfig::m_db_separator;
constexpr const char *SonicDBConfig::DEFAULT_SONIC_DB_GLOBAL_CONFIG_FILE;
unordered_map<string, unordered_map<string, RedisInstInfo>> SonicDBConfig::m_inst_info;
unordered_map<string, unordered_map<string, SonicDBInfo>> SonicDBConfig::m_db_info;
unordered_map<string, unordered_map<int, string>> SonicDBConfig::m_db_separator;
bool SonicDBConfig::m_init = false;
bool SonicDBConfig::m_global_init = false;

constexpr const char *DBConnector::DEFAULT_UNIXSOCKET;

Expand All @@ -164,7 +297,8 @@ DBConnector::~DBConnector()

DBConnector::DBConnector(int dbId, const string& hostname, int port,
unsigned int timeout) :
m_dbId(dbId)
m_dbId(dbId),
m_namespace(EMPTY_NAMESPACE)
{
struct timeval tv = {0, (suseconds_t)timeout * 1000};

Expand All @@ -181,7 +315,8 @@ DBConnector::DBConnector(int dbId, const string& hostname, int port,
}

DBConnector::DBConnector(int dbId, const string& unixPath, unsigned int timeout) :
m_dbId(dbId)
m_dbId(dbId),
m_namespace(EMPTY_NAMESPACE)
{
struct timeval tv = {0, (suseconds_t)timeout * 1000};

Expand All @@ -192,30 +327,31 @@ 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 (unixs-socket)");
"Unable to connect to redis (unix-socket)");

select(this);
}

DBConnector::DBConnector(const string& dbName, unsigned int timeout, bool isTcpConn)
: m_dbId(SonicDBConfig::getDbId(dbName))
DBConnector::DBConnector(const string& dbName, unsigned int timeout, bool isTcpConn, const string& nameSpace)
: m_dbId(SonicDBConfig::getDbId(dbName, nameSpace))
, m_dbName(dbName)
, m_namespace(nameSpace)
{
struct timeval tv = {0, (suseconds_t)timeout * 1000};

if (timeout)
{
if (isTcpConn)
m_conn = redisConnectWithTimeout(SonicDBConfig::getDbHostname(dbName).c_str(), SonicDBConfig::getDbPort(dbName), tv);
m_conn = redisConnectWithTimeout(SonicDBConfig::getDbHostname(dbName, nameSpace).c_str(), SonicDBConfig::getDbPort(dbName, nameSpace), tv);
else
m_conn = redisConnectUnixWithTimeout(SonicDBConfig::getDbSock(dbName).c_str(), tv);
m_conn = redisConnectUnixWithTimeout(SonicDBConfig::getDbSock(dbName, nameSpace).c_str(), tv);
}
else
{
if (isTcpConn)
m_conn = redisConnect(SonicDBConfig::getDbHostname(dbName).c_str(), SonicDBConfig::getDbPort(dbName));
m_conn = redisConnect(SonicDBConfig::getDbHostname(dbName, nameSpace).c_str(), SonicDBConfig::getDbPort(dbName, nameSpace));
else
m_conn = redisConnectUnix(SonicDBConfig::getDbSock(dbName).c_str());
m_conn = redisConnectUnix(SonicDBConfig::getDbSock(dbName, nameSpace).c_str());
}

if (m_conn->err)
Expand All @@ -240,19 +376,15 @@ string DBConnector::getDbName() const
return m_dbName;
}

string DBConnector::getNamespace() const
{
return m_namespace;
}

DBConnector *DBConnector::newConnector(unsigned int timeout) const
{
DBConnector *ret;
if (getContext()->connection_type == REDIS_CONN_TCP)
ret = new DBConnector(getDbId(),
getContext()->tcp.host,
getContext()->tcp.port,
timeout);
else
ret = new DBConnector(getDbId(),
getContext()->unix_sock.path,
timeout);
ret->m_dbName = m_dbName;
ret = new DBConnector(getDbName(), timeout, (getContext()->connection_type == REDIS_CONN_TCP), getNamespace());
return ret;
}

Expand Down
Loading