[multi-DB] Part 3: Python API changes #52

dzhangalibaba · 2019-10-01T19:23:42Z

add database_config.json parsing logic
add get_hostname/port/sokcet/dbid APIs
make existing database.json contains all DB define, replace old database.json
remove unnecessary check in dbconnector.py to count duplicated dbid values
add SonicV2Connector connect() API to have the ability to choose db instances(host,port or socket) , then setting argvs and call DBInterface connector as what we did today.
add a load interface to allow test cases to load config file.
redesign DBInterface, SonicV2Connector
- DBinterface is the raw interface to connect a DB instance and accept the parameter as what we have today(hostname, port, socket) , the big changes is DBInterface APIs no longer consider DB name as parameter, it only accept raw dbid as parameter.
- SonicDBConfig class handle all the db configuration related data. It store the mapping between dbname and dbid as well as the isntance info and socket/hostname/port onformation. SonicDBConfig also provide get API for others to access these data.
- SonicV2Config class behaves as a bridge between SonicDBConfig and DBinterface. Its parent is DBinterface. Application use SonicV2Connector to connect to a db and write/read data from db. In SonicV2Connector, we do translation between dbname and dbid, also it provides the same signature as what the application is using today. Then we don't need to change the application codes.

Signed-off-by: Dong Zhang d.zhang@alibaba-inc.com

qiluo-msft · 2019-10-02T22:29:37Z

src/swsssdk/dbconnector.py

@@ -17,23 +18,29 @@ def __init__(self, **kwargs):

 SonicV1Connector.db_map = _connector_map[SonicV1Connector.__name__]['db_map']

-if len(SonicV1Connector.db_map) != len({v['db'] for k, v in SonicV1Connector.db_map.items()}):
-    raise RuntimeError("Duplicate DB index detected in configuration.")


Why remove this block? #Closed

Why remove this block?

I added the missing LOGLEVEL_DB(3) , PFC_WD_DB(5), FLEX_COUNTERS_DB(5) into existing database.json which are mainly used for python today. Since both PFC_WD_DB and FLEX_COUNTER_EB has value 5. This check will raise error. But these check is unnecessary, internally we are using string "DBNAME" as key to index(in SonicV2Connector/DBInterface), duplicated value index is fine,

"PFC_WD_DB": { "db": 5, "separator": ":" }, "FLEX_COUNTER_DB": { "db": 5, "separator": ":" }, #Closed

qiluo-msft · 2019-10-02T22:30:09Z

src/swsssdk/dbconnector.py

 class SonicV2Connector(DBInterface):
    def __init__(self, **kwargs):
        super(SonicV2Connector, self).__init__(**kwargs)

+    # after py test cases(e.g snmpagent/tests) are modified
+    # we can do the replacement on this new API via remnaming connect_new to connect


remnaming [](start = 52, length = 9)

typo #Closed

fix typo #Resolved

qiluo-msft · 2019-10-02T22:36:13Z

src/swsssdk/dbconnector.py

    pass


 SonicV2Connector.db_map = _connector_map[SonicV2Connector.__name__]['db_map']

-if len(SonicV2Connector.db_map) != len({v['db'] for k, v in SonicV2Connector.db_map.items()}):
-    raise RuntimeError("Duplicate DB index detected in configuration.")


Why remove? #Closed

Why remove?

I added the missing LOGLEVEL_DB(3) , PFC_WD_DB(5), FLEX_COUNTERS_DB(5) into existing database.json which are mainly used for python today. Since both PFC_WD_DB and FLEX_COUNTER_EB has value 5. This check will raise error. But these check is unnecessary, internally we are using string "DBNAME" as key to index(in SonicV2Connector/DBInterface), duplicated value index is fine, #Closed

qiluo-msft · 2019-10-02T23:35:56Z

src/swsssdk/dbconnector.py

+    # after py test cases(e.g snmpagent/tests) are modified
+    # we can do the replacement on this new API via renaming connect_new to connect
+    # def connect(self, db_name, retry_on=True, isTcpconn=False):
+    def connect_new(self, db_name, retry_on=True, isTcpconn=False):


connect_new [](start = 8, length = 11)

Is it possible to keep original function and change its implementation? #Closed

The original connect() is from DBInterface class, which is not modified. Here I just overwrite the connect() in SonicV2Connector which is the child class to prepare some parameters, finally it is still call the original connect in DBInterface. now it is named as connect_new for temporally, since we didn't fix the py test cases yet which will cause test failed. Finally we will use name connect() which is the original name. #Resolved

use connect() instead #Resolved

qiluo-msft · 2019-10-03T02:19:25Z

src/swsssdk/__init__.py

+    global _sonic_db_config_init
+    if _sonic_db_config_init == False:
+        _load_sonic_db_config()
+    return _sonic_db_config["DATABASES"][db_name]["id"]


Extract as a new class #Closed

dzhangalibaba · 2019-10-04T01:24:05Z

@qiluo-msft
for the other installing a default database_config.json via lib topic we discussed offline, I tired today. Looks python wheel distribute cannot copy file to location out of the dist-package directory. The config file in whl must be in dist-package itself. Below is the link for this 'issue':
pypa/wheel#92

so in the codes , I made some changes, if /var/run/redis/sonic-db/database_config.json is not there, we use the config at /usr/local/lib/python2.7/swsssdk/config/database_config.josn. This case only happened for py test cases, it is safe.

dzhangalibaba · 2019-10-04T21:48:04Z

BTW VS test passed as well. #Resolved

qiluo-msft · 2019-10-05T00:57:57Z

}

The "db" fields are duplication with "id" fields in database_config.json. Some suggestions:

remove SonicV1Connector code, not used (better in a standalone PR)
remove this config file, and move separator info into database_config.json

Refers to: src/swsssdk/config/database.json:54 in a6aab64. [](commit_id = a6aab64, deletion_comment = False)

qiluo-msft · 2019-10-05T00:58:58Z

src/swsssdk/dbconnector.py

+    _sonic_db_config = {}
+
+    @staticmethod
+    def load_sonic_db_config(sonic_db_file_path=SONIC_DB_CONFIG_FILE):


load_sonic_db_config [](start = 8, length = 20)

~~I guess you could rewrite _load_connector_map instead of creating a new function.~~

Should we remove SonicV2Connector.db_map and _load_connector_map at all? #Closed

qiluo-msft · 2019-10-05T01:00:09Z

src/swsssdk/dbconnector.py

+        return SonicDBConfig._sonic_db_config["INSTANCES"][inst_name]["port"]
+
+    @staticmethod
+    def get_sonic_db_id(db_name):


get_sonic_db_id [](start = 8, length = 15)

~~I guess you could override get_dbid instead of creating a new function.~~

Should we remove the member function get_dbid?
#Closed

qiluo-msft · 2019-10-05T01:18:07Z

src/swsssdk/dbconnector.py

-if len(SonicV1Connector.db_map) != len({v['db'] for k, v in SonicV1Connector.db_map.items()}):
-    raise RuntimeError("Duplicate DB index detected in configuration.")
-
-
 class SonicV2Connector(DBInterface):
    def __init__(self, **kwargs):
        super(SonicV2Connector, self).__init__(**kwargs)


super(SonicV2Connector, self).init(**kwargs) [](start = 8, length = 48)

It makes no sense to reuse parent class __init__. Suggest add one option is_unix_socket for each object with default value true, and remove isTcpconn from connect #Closed

done. invoking parent init is necessary , since it initialized some variable/struct

qiluo-msft · 2019-10-05T01:35:33Z

Even with the difficulty in python wheel, we should try workaround to achieve the goal we discussed. Please help explorer. At least I know python setup.py sdist works, and of course this solution need some makefile modification.

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

qiluo-msft

Commented

dzhangalibaba

redesign class DBinterface , SonivV2Connector and SonicDBConfig

2 DBinterface is the raw interface to connect a DB instance and accept the parameter as what we have today(hostname, port, socket) , the big changes is DBInterface APIs no longer consider DB name as parameter, it only accept raw dbid as parameter.

3 SonicDBConfig class handle all the db configuration related data. It store the mapping between dbname and dbid as well as the isntance info and socket/hostname/port onformation. SonicDBConfig also provide get API for others to access these data.

4 SonicV2Config class behaves as a bridge between SonicDBConfig and DBinterface. Its parent is DBinterface. Application use SonicV2Connector to connect to a db and write/read data from db. In SonicV2Connector, we do translation between dbname and dbid, also it provides the same signature as what the application is using today. Then we don't need to change the application codes.

dzhangalibaba · 2019-10-09T06:17:50Z

Here having some root access issue as we discussed offline, I suggest keep the library database_config.json file in the library domain , this won't affect anything on pyhsical machine. It helps solve the testing issue. For swss-common library, I suggest the same, with library there is a database_config.json file for testing only. Later when all the stuff are done, we can focus on how to solve the root access issue during building images and how to distribute the file to a single place. For now , to me it is OK, it doesn't affect any functionality .

@qiluo-msft
for the other installing a default database_config.json via lib topic we discussed offline, I tired today. Looks python wheel distribute cannot copy file to location out of the dist-package directory. The config file in whl must be in dist-package itself. Below is the link for this 'issue':
pypa/wheel#92

so in the codes , I made some changes, if /var/run/redis/sonic-db/database_config.json is not there, we use the config at /usr/local/lib/python2.7/swsssdk/config/database_config.josn. This case only happened for py test cases, it is safe.

dzhangalibaba · 2019-10-09T06:18:32Z

}

The "db" fields are duplication with "id" fields in database_config.json. Some suggestions:

remove SonicV1Connector code, not used (better in a standalone PR)

remove this config file, and move separator info into database_config.json

Refers to: src/swsssdk/config/database.json:54 in a6aab64. [](commit_id = a6aab64, deletion_comment = False)

DONE

dzhangalibaba · 2019-10-09T06:23:18Z

@qiluo-msft redesign the codes, the changes is a little bit more, we can have a chat to explain what the idea is and how it works if possible.

qiluo-msft · 2019-10-09T21:27:22Z

src/swsssdk/dbconnector.py

        super(SonicV2Connector, self).__init__(**kwargs)
+        self.use_unix_socket_path = unix_socket_path


unix_socket_path [](start = 36, length = 16)

Suggestion: unix_socket_path->use_unix_socket_path #Closed

qiluo-msft · 2019-10-09T22:13:50Z

src/swsssdk/dbconnector.py

+        return SonicDBConfig._sonic_db_config["DATABASES"].keys()
+
+    @staticmethod
+    def get_sonic_db_socket(db_name):


get_sonic_db_socket [](start = 8, length = 19)

Suggest rename: get_sonic_db_* -> get_* #Closed

qiluo-msft · 2019-10-09T22:16:23Z

src/swsssdk/dbconnector.py

+    def get_sonic_db_socket(db_name):
+        if SonicDBConfig._sonic_db_config_init == False:
+            SonicDBConfig.load_sonic_db_config()
+        inst_name = SonicDBConfig._sonic_db_config["DATABASES"][db_name]["instance"]


db_name [](start = 64, length = 7)

Should we handle index out of range explicitly?

if db_name does not exist, it's invalid argument

if inst_name does not exist, it's invalid config.

added db_name_validation() and inst_name_validation(), if invalid , raise a error

qiluo-msft · 2019-10-09T22:16:52Z

src/swsssdk/dbconnector.py

+    def get_sonic_db_socket(db_name):
+        if SonicDBConfig._sonic_db_config_init == False:
+            SonicDBConfig.load_sonic_db_config()
+        inst_name = SonicDBConfig._sonic_db_config["DATABASES"][db_name]["instance"]


Extract a common helper function like get_instance? #Closed

qiluo-msft · 2019-10-09T23:42:35Z

src/swsssdk/dbconnector.py

+
+        try:
+            if os.path.isfile(sonic_db_file_path) == False:
+                sonic_db_file_path = os.path.join(os.path.dirname(os.path.abspath(__file__)), 'config', 'database_config.json')


sonic_db_file_path = os.path.join(os.path.dirname(os.path.abspath(file)), 'config', 'database_config.json') [](start = 16, length = 111)

We don't want to see this case in production. Can we add a syslog warning? #Closed

added warning log

qiluo-msft

Thanks! Minor issues remain.

dzhangalibaba · 2019-10-10T05:14:44Z

Thanks! Minor issues remain.

updated

* multiDB part3 : python API * add validation check and update some minor suggestions

qiluo-msft reviewed Oct 2, 2019

View reviewed changes

qiluo-msft reviewed Oct 3, 2019

View reviewed changes

qiluo-msft reviewed Oct 5, 2019

View reviewed changes

multiDB part3 : python API

0f05483

dzhangalibaba force-pushed the multidb_pyapi branch from a6aab64 to 0f05483 Compare October 9, 2019 05:53

dzhangalibaba commented Oct 9, 2019

View reviewed changes

qiluo-msft reviewed Oct 9, 2019

View reviewed changes

add validation check and update some minor suggestions

48551df

qiluo-msft approved these changes Oct 10, 2019

View reviewed changes

qiluo-msft requested review from renukamanavalan and pavel-shirshov October 10, 2019 06:11

qiluo-msft requested review from jleveque and lguohan October 10, 2019 06:11

jleveque approved these changes Oct 10, 2019

View reviewed changes

qiluo-msft merged commit 7abaabf into sonic-net:master Oct 10, 2019

dzhangalibaba deleted the multidb_pyapi branch October 10, 2019 23:58

dzhangalibaba mentioned this pull request Nov 25, 2019

Config reload failed with "No client connected to db_id 'CONFIG_DB'" sonic-net/sonic-buildimage#3817

Closed

abdosi added the Request for 201911 label Jan 6, 2020

abdosi pushed a commit that referenced this pull request Jan 6, 2020

[multi-DB] Part 3: Python API changes (#52)

677c32b

* multiDB part3 : python API * add validation check and update some minor suggestions

abdosi added the Included in 201911 label Jan 7, 2020

		super(SonicV2Connector, self).__init__(**kwargs)
		self.use_unix_socket_path = unix_socket_path

[multi-DB] Part 3: Python API changes #52

[multi-DB] Part 3: Python API changes #52

Conversation

dzhangalibaba commented Oct 1, 2019 • edited

qiluo-msft Oct 2, 2019 • edited

Choose a reason for hiding this comment

dzhangalibaba Oct 2, 2019 • edited by qiluo-msft

Choose a reason for hiding this comment

qiluo-msft Oct 2, 2019 • edited

Choose a reason for hiding this comment

dzhangalibaba Oct 2, 2019 • edited by qiluo-msft

Choose a reason for hiding this comment

qiluo-msft Oct 2, 2019 • edited

Choose a reason for hiding this comment

dzhangalibaba Oct 2, 2019 • edited by qiluo-msft

Choose a reason for hiding this comment

qiluo-msft Oct 2, 2019 • edited

Choose a reason for hiding this comment

dzhangalibaba Oct 3, 2019 • edited by qiluo-msft

Choose a reason for hiding this comment

dzhangalibaba Oct 4, 2019 • edited by qiluo-msft

Choose a reason for hiding this comment

qiluo-msft Oct 3, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzhangalibaba commented Oct 4, 2019 • edited

dzhangalibaba commented Oct 4, 2019 • edited by qiluo-msft

qiluo-msft commented Oct 5, 2019

qiluo-msft Oct 5, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Oct 5, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Oct 5, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft commented Oct 5, 2019

qiluo-msft left a comment

Choose a reason for hiding this comment

dzhangalibaba left a comment

Choose a reason for hiding this comment

dzhangalibaba commented Oct 9, 2019

dzhangalibaba commented Oct 9, 2019

dzhangalibaba commented Oct 9, 2019

qiluo-msft Oct 9, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Oct 9, 2019 • edited

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

qiluo-msft Oct 9, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Oct 9, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft left a comment

Choose a reason for hiding this comment

dzhangalibaba commented Oct 10, 2019

dzhangalibaba commented Oct 1, 2019 •

edited

qiluo-msft Oct 2, 2019 •

edited

dzhangalibaba Oct 2, 2019 •

edited by qiluo-msft

qiluo-msft Oct 2, 2019 •

edited

dzhangalibaba Oct 2, 2019 •

edited by qiluo-msft

qiluo-msft Oct 2, 2019 •

edited

dzhangalibaba Oct 2, 2019 •

edited by qiluo-msft

qiluo-msft Oct 2, 2019 •

edited

dzhangalibaba Oct 3, 2019 •

edited by qiluo-msft

dzhangalibaba Oct 4, 2019 •

edited by qiluo-msft

qiluo-msft Oct 3, 2019 •

edited

dzhangalibaba commented Oct 4, 2019 •

edited

dzhangalibaba commented Oct 4, 2019 •

edited by qiluo-msft

qiluo-msft Oct 5, 2019 •

edited

qiluo-msft Oct 5, 2019 •

edited

qiluo-msft Oct 5, 2019 •

edited

qiluo-msft Oct 9, 2019 •

edited

qiluo-msft Oct 9, 2019 •

edited

qiluo-msft Oct 9, 2019 •

edited

qiluo-msft Oct 9, 2019 •

edited