Dbconnector namespace support #376
Conversation
…-swss#1363 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.
…ey were creating DBConnector object using the earlier constructors before Multi-DB, Multi-NS design. eg: conf_db = swsscommon.DBConnector(swsscommon.CONFIG_DB, dvs.redis_sock, 0) Here the first argument is dbid, second is redis unix socket, third is timeout.
| cout<<"testing "<<dbName<<endl; | ||
| cout<<ns_name<<"#"<<dbId<<dbName<<"#"<<m_inst_info[instName].unixSocketPath<<"#"<<m_inst_info[instName].hostname<<"#"<<m_inst_info[instName].port<<endl; | ||
| // dbInst info matches between get api and context in json file | ||
| EXPECT_EQ(instName, SonicDBConfig::getDbInst(dbName, ns_name)); |
There was a problem hiding this comment.
dbName [](start = 64, length = 6)
Please also add some negative test cases, such as dbName=='' or dbName=='NOT_EXISTS' #Closed
There was a problem hiding this comment.
There was a problem hiding this comment.
It is already added earlier here https://github.com/Azure/sonic-swss-common/pull/376/files#diff-746f472569c0d21be541a1a591837684R33, let me know if this is the usecase.
common/dbconnector.h
Outdated
| @@ -60,12 +79,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, const std::string &nameSpace); | |||
There was a problem hiding this comment.
DBConnector [](start = 4, length = 11)
Add test cases for new code #Closed
There was a problem hiding this comment.
Had already added redis_multi_ns_ut.cpp which covers the cases with namespace, will add couple of negative cases for db_name not exists.
There was a problem hiding this comment.
This is still applicable. There is no test case for explicit netns.
In reply to: 471706833 [](ancestors = 471706833)
There was a problem hiding this comment.
Thanks, have added explicit testcase TEST(DBConnector, multi_ns_table_test) which does table/entry creation in a DB in a particular namespace.
* Adding back PR#364 * Kept the old definition of >>DBConnector(const std::string &dbName, unsigned int timeout, bool isTcpConn = false) << intact , so that we don't the issue (sonic-net/sonic-swss#1363) * 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. * Fixes to take care of case where certain SWSS tests where failing as they were creating DBConnector object using the earlier constructors before Multi-DB design where the first argument is dbid eg: conf_db = swsscommon.DBConnector(swsscommon.CONFIG_DB, dvs.redis_sock, 0) * Change the variable naming --> netns, use the old logic in newConnector() * Couple of more testcase scenarios added.
| @@ -20,6 +20,8 @@ class RedisSelect : public Selectable | |||
| bool hasCachedData() override; | |||
| bool initializedWithData() override; | |||
| void updateAfterRead() override; | |||
| int getDbConnectorId() override; | |||
| std::string getDbNamespace() override; | |||
There was a problem hiding this comment.
Why they are added? How are they used?
There was a problem hiding this comment.
@qiluo-msft
getDbNamespace() was added to find Namespace when select returns the object.
eg: https://github.com/Azure/sonic-buildimage/blob/master/files/image_config/caclmgrd/caclmgrd#L484
getDbConnectorId() is not being used as of now but it is useful if we want to check on DBId of select return object.
There was a problem hiding this comment.
another similar use case is in platform ledd daemon
https://github.com/Azure/sonic-platform-daemons/blob/master/sonic-ledd/scripts/ledd#L104
| virtual std::string getDbNamespace() | ||
| { | ||
| return std::string(); | ||
| } |
There was a problem hiding this comment.
In concept, Selectable has nothing todo with Redis. We need to remove these virtual functions.
There was a problem hiding this comment.
@qiluo-msft will check and update.
sonic-net#376 to remove DB specific API from selectable class which is generic. However to access to the Derviced Class Redis Select API via python we need to downcast the the Selectable Object. Added Helpfer function to do same. Ref: http://www.swig.org/Doc3.0/Python.html Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
* Address Review comments #376 to remove DB specific API from selectable class which is generic. However to access to the Derviced Class Redis Select API via python we need to downcast the the Selectable Object. Added Helpfer function to do same. Ref: http://www.swig.org/Doc3.0/Python.html Signed-off-by: Abhishek Dosi <abdosi@microsoft.com> * Align the code properly Signed-off-by: Abhishek Dosi <abdosi@microsoft.com> * Address Review Comments. Return the DBConnector Pointer. Since the DBConnector is unique_ptr return the reference to the contained object, or a non owning pointer Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
* Address Review comments #376 to remove DB specific API from selectable class which is generic. However to access to the Derviced Class Redis Select API via python we need to downcast the the Selectable Object. Added Helpfer function to do same. Ref: http://www.swig.org/Doc3.0/Python.html Signed-off-by: Abhishek Dosi <abdosi@microsoft.com> * Align the code properly Signed-off-by: Abhishek Dosi <abdosi@microsoft.com> * Address Review Comments. Return the DBConnector Pointer. Since the DBConnector is unique_ptr return the reference to the contained object, or a non owning pointer Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
Why:
The commit for support for namespaces in DBConnector (#371) class was reverted earlier as it failed couple of tests in swss. It was found that the SWSS docker virtual switch API's create the DBConnector object using the constructor API's which was present before the multi-DB changes went in. During those times, the DBConnector classes used dbId alone and dbName was not filled. (https://github.com/Azure/sonic-swss/blob/eac1d898e12d03040cd7b4e3c4059960dbf0ae8d/tests/dvslib/dvs_database.py#L30)
The reason for the tests failure was because the dbName was coming as NULL when we try to create newConnector().
Couple of sonic-build issues also was triaged and fixed while trying to push this commit ( These were causing the swss tests to fail when we tried to this namespace support PR earlier )
Azure/sonic-build-tools#152
Azure/sonic-build-tools#147
What:
Added the check to see if the dbName, namespace is empty, if so create the DBConnector using the old constructors which takes dbId as input. If not call the new constructor where we pass the dbName and namespaceName.
With the above changes -- no further change needed in sonic-swss/tests/mock_tests/mock_dbconnector.cpp
How:
Recreated the swss tests setup locally and saw the tests cases were passing.