DBConnector classes to understand the namespace. #364

judyjoseph · 2020-07-09T00:29:48Z

Why:
The dbconnector classes here need to understand the namespace, to connect to redis databases running in database containers in different linux network namespaces in case of multi-asic platforms.

What:
The class data structures are modified to handle the new dimension of namespace. New api's are added to parse the database_global.json file and to retrieve the list of namespaces + parse the respective database_config.json file. The namespaces will be ""(default namespace for the linux host) and "asic0", "aisic1" .... as present in the /var/run/redis/sonic-db/database_global.json file.

How verified:
Verification was done on a multi-asic platform with pmon scripts which uses swsscommon dbconnector classes.
Note : testcases to be updated, in this or in a follow up PR.

qiluo-msft · 2020-07-11T16:31:36Z

common/dbconnector.h

@@ -59,13 +78,14 @@ class DBConnector
     */
    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 = false, const std::string &nameSpace = EMPTY_NAMESPACE);


EMPTY_NAMESPACE [](start = 120, length = 15)

To make default value more efficient, check https://stackoverflow.com/a/32851509/2514803 #Closed

Thanks Qi, have updated the #define EMPTY_NAMESPACE std::string().

qiluo-msft · 2020-07-11T16:39:28Z

common/dbconnector.h

@@ -84,6 +104,7 @@ class DBConnector
    redisContext *m_conn;
    int m_dbId;
    std::string m_dbName;
+    std::string m_nameSpace;


m_nameSpace [](start = 16, length = 11)

m_namespace or m_nsName (then getNamespace-> getNsName) #Closed

qiluo-msft · 2020-07-11T16:50:10Z

common/dbconnector.h


 namespace swss {

 class DBConnector;

+class SonicInstInfo


SonicInstInfo [](start = 6, length = 13)

RedisInstInfo #Closed

qiluo-msft · 2020-07-11T16:51:07Z

common/dbconnector.h

+class SonicInstInfo
+{
+public:
+    std::string socket;


socket [](start = 16, length = 6)

Please follow the names in config file "unixSocketPath" #Closed

camelCase will be better

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

qiluo-msft · 2020-07-11T16:53:41Z

common/dbconnector.h

+                             std::unordered_map<std::string, SonicDBInfo> &db_entry, 
+                             std::unordered_map<int, std::string> &separator_entry);
+    static void initialize(const std::string &file = DEFAULT_SONIC_DB_CONFIG_FILE, const std::string &nameSpace = EMPTY_NAMESPACE);
+    static void initialize_global_config(const std::string &file = DEFAULT_SONIC_DB_GLOBAL_CONFIG_FILE);


initialize_global_config [](start = 16, length = 24)

initializeGlobalConfig #Closed

qiluo-msft · 2020-07-11T16:55:02Z

common/dbconnector.h

-    static int getDbId(const std::string &dbName);
-    static std::string getSeparator(const std::string &dbName);
-    static std::string getSeparator(int dbId);
+    static void parse_config(const std::string &file, 


parse_config [](start = 16, length = 12)

Why public function? #Closed

qiluo-msft · 2020-07-11T17:00:27Z

common/dbconnector.h

-    static int getDbId(const std::string &dbName);
-    static std::string getSeparator(const std::string &dbName);
-    static std::string getSeparator(int dbId);
+    static void parse_config(const std::string &file, 


parse_config [](start = 16, length = 12)

parseDatabaseConfig #Closed

qiluo-msft · 2020-07-11T17:01:36Z

@dzhangalibaba Please help review #Closed

qiluo-msft · 2020-07-11T17:02:54Z

common/dbconnector.h

+    static std::string getDbInst(const std::string &dbName, const std::string &nameSpace = EMPTY_NAMESPACE);
+    static int getDbId(const std::string &dbName, const std::string &nameSpace = EMPTY_NAMESPACE);
+    static std::string getSeparator(const std::string &dbName, const std::string &nameSpace = EMPTY_NAMESPACE);
+    static std::string getSeparator(int dbId, const std::string &nameSpace = EMPTY_NAMESPACE);


getSeparator [](start = 23, length = 12)

Please cover new functionality in unit test. #Closed

dzhangalibaba

when to use m_global_init in get func ?

dzhangalibaba · 2020-07-19T16:27:01Z

common/dbconnector.cpp

+
+                if(element["namespace"].empty())
+                {
+                    ns_name = EMPTY_NAMESPACE;


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

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 ?

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.

judyjoseph · 2020-07-20T06:56:01Z

when to use m_global_init in get func ?

With the current approach using the flag m_init itself to cover the cases where initialization is already done once -- either with EMPTY namespace OR with a valid namespace name. m_global_init is getting used indirectly when a namespace argument is provided to initialize API.

qiluo-msft

Please check with other reviewers.

dzhangalibaba · 2020-07-20T21:02:14Z

common/dbconnector.cpp

+
+                if(element["namespace"].empty())
+                {
+                    ns_name = EMPTY_NAMESPACE;


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.

dzhangalibaba · 2020-07-20T21:05:16Z

common/dbconnector.cpp

+                local_file.append(dir_name);
+                local_file.append(element["include"]);
+
+                if(element["namespace"].empty())


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.

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

trying to try to connect multiple namespace DB. Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

It is helpful in application that are doing select and need to the event is generated from which DB and namespace so that any parituclar action can be taken if needed. Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

…torId() as it was clashing with swss::TableBase::getDbId()

judyjoseph · 2020-07-24T01:53:47Z

Rebased due to merge conflict .

qiluo-msft

Please also check with other reviewers.

abdosi · 2020-07-26T19:07:09Z

@judyjoseph Please update submodule pointer of master for sonic-swss-common

* DBConnector classes to understand the namespace. * Updates to the initial commit * Updates for comments * Add UT tests for the namespace usecases * Updates to the unit tests * Derive the DIR of the config files from the file path itself. * Fixes when testing and trying to try to connect multiple namespace DB. * Added Support to get DB Id and DB Namespace from Selectable Object. It is helpful in application that are doing select and need to the event is generated from which DB and namespace so that any parituclar action can be taken if needed. * Review comments update, for the variable name. * Further fixes and updating unit tests * Renaming the newly added function in swss::RedisSelect to getDbConnectorId() as it was clashing with swss::TableBase::getDbId() Co-authored-by: Abhishek Dosi <abdosi@microsoft.com>

judyjoseph · 2020-07-27T02:57:49Z

@judyjoseph Please update submodule pointer of master for sonic-swss-common

@abdosi , Will need to revert this commit temporarily - as this is causing a build issue in sonic-swss. This is the PR in swss to track that sonic-net/sonic-swss#1362. There are some test failures in swss which is preventing me from committing the fix. So please don't advance the submodule pointer in 201911 branch as well.

This reverts commit c0ba527.

abdosi · 2020-07-27T19:44:18Z

common/dbconnector.cpp

@@ -73,32 +67,152 @@ void SonicDBConfig::initialize(const string &file)
    }
 }

-string SonicDBConfig::getDbInst(const string &dbName)
+void SonicDBConfig::initializeGlobalConfig(const string &file)


@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.

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.

@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

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().

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

* Adding back PR#364 * Also changes to allign the dbConnector class here with that of sonic-py-swssdk 1. Ignore if the database_global.json file is not present. 2. validate the namespace before using it.

)" This reverts commit 1943e2b.

This reverts commit 71a3651.

judyjoseph requested review from qiluo-msft and jleveque July 10, 2020 06:44

judyjoseph marked this pull request as ready for review July 10, 2020 06:45

qiluo-msft reviewed Jul 11, 2020

View reviewed changes

qiluo-msft requested a review from shi-su July 11, 2020 17:01

qiluo-msft reviewed Jul 11, 2020

View reviewed changes

dzhangalibaba reviewed Jul 19, 2020

View reviewed changes

qiluo-msft previously approved these changes Jul 20, 2020

View reviewed changes

dzhangalibaba reviewed Jul 20, 2020

View reviewed changes

judyjoseph dismissed qiluo-msft’s stale review via 5241453 July 21, 2020 06:35

abdosi mentioned this pull request Jul 22, 2020

[caclmgrd] Add support for multi-ASIC platforms sonic-net/sonic-buildimage#5022

Merged

judyjoseph requested a review from qiluo-msft July 24, 2020 01:44

judyjoseph and others added 10 commits July 23, 2020 18:51

DBConnector classes to understand the namespace.

42d9913

Updates to the initial commit

7918b4a

Updates for comments

77e8918

Add UT tests for the namespace usecases

b188047

Updates to the unit tests

bbbf1d3

Derive the DIR of the config files from the file path itself.

b39d0dc

Fixes when testing and

ed202a1

trying to try to connect multiple namespace DB. Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

Review comments update, for the variable name.

a7898c3

Further fixes and updating unit tests

92eacae

Renaming the newly added function in swss::RedisSelect to getDbConnec…

4be4c39

…torId() as it was clashing with swss::TableBase::getDbId()

judyjoseph force-pushed the multi_ns_changes branch from 418b541 to 4be4c39 Compare July 24, 2020 01:53

qiluo-msft approved these changes Jul 24, 2020

View reviewed changes

dzhangalibaba approved these changes Jul 24, 2020

View reviewed changes

qiluo-msft merged commit c0ba527 into sonic-net:master Jul 24, 2020

judyjoseph added the Request for 201911 Branch label Jul 24, 2020

abdosi added the Included in 201911 Branch label Jul 26, 2020

judyjoseph added a commit that referenced this pull request Jul 27, 2020

Revert "DBConnector classes to understand the namespace. (#364)"

9c2449e

This reverts commit c0ba527.

This was referenced Jul 27, 2020

Revert "DBConnector classes to understand the namespace." #369

Merged

Platform daemon changes for multi asic platform sonic-net/sonic-platform-daemons#64

Merged

qiluo-msft pushed a commit that referenced this pull request Jul 27, 2020

Revert "DBConnector classes to understand the namespace. (#364)" (#369)

1e3f9ca

This reverts commit c0ba527.

abdosi reviewed Jul 27, 2020

View reviewed changes

judyjoseph mentioned this pull request Aug 3, 2020

Reintroducing PR #364 - DBconnector to support namespaces. #371

Merged

judyjoseph added a commit that referenced this pull request Aug 12, 2020

Revert "Reintroducing PR #364 - DBconnector to support namespaces. (#371

86ac502

)" This reverts commit 1943e2b.

abdosi added a commit that referenced this pull request Aug 19, 2020

Revert "DBConnector classes to understand the namespace. (#364)"

92976de

This reverts commit 71a3651.

judyjoseph deleted the multi_ns_changes branch September 11, 2020 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DBConnector classes to understand the namespace. #364

DBConnector classes to understand the namespace. #364

judyjoseph commented Jul 9, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020 •

edited

Loading

judyjoseph Jul 20, 2020

qiluo-msft Jul 11, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020 •

edited

Loading

qiluo-msft Jul 17, 2020

qiluo-msft Jul 11, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020 •

edited

Loading

qiluo-msft commented Jul 11, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020 •

edited

Loading

dzhangalibaba left a comment

dzhangalibaba Jul 19, 2020

judyjoseph Jul 20, 2020

dzhangalibaba Jul 20, 2020 •

edited by judyjoseph

Loading

judyjoseph commented Jul 20, 2020

qiluo-msft left a comment

dzhangalibaba Jul 20, 2020 •

edited by judyjoseph

Loading

dzhangalibaba Jul 20, 2020

judyjoseph Jul 20, 2020

judyjoseph commented Jul 24, 2020

qiluo-msft left a comment

abdosi commented Jul 26, 2020 •

edited

Loading

judyjoseph commented Jul 27, 2020

abdosi Jul 27, 2020

judyjoseph Jul 27, 2020 •

edited

Loading

abdosi Jul 27, 2020

judyjoseph Jul 27, 2020

abdosi Jul 27, 2020

DBConnector classes to understand the namespace. #364

DBConnector classes to understand the namespace. #364

Conversation

judyjoseph commented Jul 9, 2020 • edited Loading

qiluo-msft Jul 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jul 11, 2020 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Jul 11, 2020 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Jul 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jul 11, 2020 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Jul 11, 2020 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Jul 11, 2020 • edited Loading

Choose a reason for hiding this comment

qiluo-msft commented Jul 11, 2020 • edited Loading

qiluo-msft Jul 11, 2020 • edited Loading

Choose a reason for hiding this comment

dzhangalibaba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzhangalibaba Jul 20, 2020 • edited by judyjoseph Loading

Choose a reason for hiding this comment

judyjoseph commented Jul 20, 2020

qiluo-msft left a comment

Choose a reason for hiding this comment

dzhangalibaba Jul 20, 2020 • edited by judyjoseph Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

judyjoseph commented Jul 24, 2020

qiluo-msft left a comment

Choose a reason for hiding this comment

abdosi commented Jul 26, 2020 • edited Loading

judyjoseph commented Jul 27, 2020

Choose a reason for hiding this comment

judyjoseph Jul 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

judyjoseph commented Jul 9, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020 •

edited

Loading

qiluo-msft commented Jul 11, 2020 •

edited

Loading

qiluo-msft Jul 11, 2020 •

edited

Loading

dzhangalibaba Jul 20, 2020 •

edited by judyjoseph

Loading

dzhangalibaba Jul 20, 2020 •

edited by judyjoseph

Loading

abdosi commented Jul 26, 2020 •

edited

Loading

judyjoseph Jul 27, 2020 •

edited

Loading