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.

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

Support redis databases in multiple namespaces #567

Merged
merged 27 commits into from
Apr 29, 2020

Conversation

lguohan
Copy link
Contributor

@lguohan lguohan commented Feb 29, 2020

No description provided.


The file database_config.json contains the startup configuration which dictates the redis server host/port/unix_socket configurations + various available databases ( eg: APP_DB, CONFIG_DB ) etc. With the introduction of multiple namespaces, there is a need for separate database_config.json per namespace. This config file is created in the "working redis directories" during the database docker service startup.

The database docker service started in the linux network namespace {NS} will use the "/var/run/redis{NS}" as the "working redis directory" to create the various files like redis.sock, sonic-db/database_config.json etc. For "globalDB" database service it would be at "/var/run/redis".
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the database docker service does not have to be started in the linux network namespace. suggest to change to "database service" for a NPU linux namespace should use /var/run/redis{NS{. as the working directory.

Copy link
Contributor

@qiluo-msft qiluo-msft Mar 6, 2020

Choose a reason for hiding this comment

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

@lguohan Could you clarify "the database docker service does not have to be started in the linux network namespace" ?
If not in namespace, how to specify one database container instance?

For example, currently we are using sonic-db-cli STATE_DB hset, what will be new command link like? #Closed

Copy link
Contributor

@judyjoseph judyjoseph Mar 6, 2020

Choose a reason for hiding this comment

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

The database docker service needs to be started in some network namespace. It could be a new "net_ns" which we create, or the linux host network namespace.

A new parameter is added to sonic-db-cli,
sonic-db-cli APPL_DB -namespace 'ns" HGET VLAN_TABLE:Vlan10 mtu

if we don't specify the namespace parameter, it executes the redis command in the DB instance in the database docker running that namespace where we executed the sonic-db-cli command.

Do let me know if we need more discussion on this.

This is an example of the database_config.json with 3 external DB references in 3 namespaces refered with namespaceID "0", "1" & "2".

```json
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the json file is not clean. I suggest to have only includes in the global database json. you can put the global db configuration into a separate file and use include to include it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have explicitly added details of the database_config.json file which will get generated for the global DB docker service and the NPU namespace DB docker service.

There is no particular file like global DB database_config.json file. The current approach I have taken is to generate the json file depending on where the database docker service is running (either in linux host, or NPU namesapce )

Please find that the "INCLUDES" section is only present for the globalDB database_config.json (created at /var/run/redis/sonic-db) as the applications like snmp,pmon running in the dockers in the Linux host needs to talk to DB in other NPU namespaces.

The database_config.json file for the NPU namespace docker won't have INCLUDES section. It will only have the default local DB instances, as the applications running in the namespace like swss, syncd, bgp etc will only talk to its local DB instances.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a use case where the applications/dockers running in a “particular NPU namespace” will need to access the DB instances in the database dockers running in either “linux host namespace” OR “other NPU namespaces” ?

Currently my assumption is that only the applications running in the host, like snmp, CLI, pmon etc “needs to access” the DB instances running in other NPU namespaces.

Or should we make it more generic, like any application running in any docker in host/NPU namespace should be able to access the DB instance running in a different NPU namespace ? Please could you share your thoughts.

If Yes, then I understand your ask. I would change accordinly and update the doc. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

example

{
   [
       {
	    "config" : "../redis/sonic-db/database_config.json"
	},
	{
	    "namespace" : "0",
	    "config" : "../redis0/sonic-db/database_config.json"
        },
	{
	    "namespace" : "1",
	    "config" : "../redis1/sonic-db/database_config.json"
        },
    ],
    "VERSION" : "1.1"
}

Copy link
Contributor

@judyjoseph judyjoseph Mar 5, 2020

Choose a reason for hiding this comment

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

Thanks for the example.

I have updated the document, by creating a new file "/var/run/redis/sonic-db/databases.json". This will contain the mapping between the namespace names and the database_config.json file references.

A change though is that, in this new file, I have removed the reference to global /var/run/redis/sonic-db/database_config.json.
{
"config" : "../redis/sonic-db/database_config.json"
},

If we have this entry, it will create a recursive reference to this file when I add it as the INCLUDES in the global database_config.json file /var/run/redis/sonic-db/database_config.json.

Please share your comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

The document is updated, Please review.

## Design of C++ Interface : DBConnector()
The C++ DBConnector interface is mainly used in the process context of swss/syncd etc in the dockers in their respective namespaces. In the namespaces currently we don't have any external DB refereces, hence not planning to extend the C++ DBConnector to handle multiple database_config.json files.

Will be taken up as the next phase activity to get this class also at par with the python SonicDBConfig/SonicV2Connector modules.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a must because we are deprecating the pure swsssdk library and plan to python-swsscommon library for all python related applications.

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated the doc, will take up the changes to DBConnector class as well and raise the PR for those changes.

Updates based on review comments
},
"INCLUDES" : [
{
"namespace_id" : "0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

namespace_id -> namespace

Copy link
Contributor

Choose a reason for hiding this comment

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

updated

Adding INCLUDES keyword and VERSION in database_global.json file.
Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

I only reviewed the section of "Enhancements to Multi-DB design to support Multiple namespaces". Other sections is still under discussion and will probably refined in future.

lguohan pushed a commit to sonic-net/sonic-py-swsssdk that referenced this pull request Apr 23, 2020
This PR is the changes needed to support multiple namespaces for the Multi-ASIC devices. Multi-DB namespace PR --> (sonic-net/SONiC#567)

The changes are mainly to these classes

SonicDBConfig
SonicV2Connector/ConfigDBConnector

A new parameter "namespace" is added to the SonicV2Connector class init , to pass the namespace name. The default value is None representing empty namespace.

class SonicV2Connector(DBInterface):
def init(self, use_unix_socket_path=False, namespace=None, **kwargs):

If the user don't explicitly set this parameter, namespace takes None as value, and connects to the db_name in the local context, which refers to the database docker running in the current namespace wherever you are running this application/script …. (It could be either Linux host or any specific network namespace ). In this way it is backward compatible and the existing behavior of APIs in these utility classes are maintained.

In the SonicDBConfig, a new API is introduced load_sonic_global_db_config() which loads the /var/run/redis/sonic-db/database_global.json file if present. This file has the mapping between the namespace name and the corresponding database_config.json file.
@lguohan lguohan changed the title Multi db ns Support redis databases in multiple namespaces Apr 29, 2020
@lguohan lguohan merged commit 54f8524 into sonic-net:master Apr 29, 2020
abdosi pushed a commit to sonic-net/sonic-py-swsssdk that referenced this pull request May 5, 2020
This PR is the changes needed to support multiple namespaces for the Multi-ASIC devices. Multi-DB namespace PR --> (sonic-net/SONiC#567)

The changes are mainly to these classes

SonicDBConfig
SonicV2Connector/ConfigDBConnector

A new parameter "namespace" is added to the SonicV2Connector class init , to pass the namespace name. The default value is None representing empty namespace.

class SonicV2Connector(DBInterface):
def init(self, use_unix_socket_path=False, namespace=None, **kwargs):

If the user don't explicitly set this parameter, namespace takes None as value, and connects to the db_name in the local context, which refers to the database docker running in the current namespace wherever you are running this application/script …. (It could be either Linux host or any specific network namespace ). In this way it is backward compatible and the existing behavior of APIs in these utility classes are maintained.

In the SonicDBConfig, a new API is introduced load_sonic_global_db_config() which loads the /var/run/redis/sonic-db/database_global.json file if present. This file has the mapping between the namespace name and the corresponding database_config.json file.
praveen-li pushed a commit to praveen-li/sonic-py-swsssdk that referenced this pull request May 4, 2021
This PR is the changes needed to support multiple namespaces for the Multi-ASIC devices. Multi-DB namespace PR --> (sonic-net/SONiC#567)

The changes are mainly to these classes

SonicDBConfig
SonicV2Connector/ConfigDBConnector

A new parameter "namespace" is added to the SonicV2Connector class init , to pass the namespace name. The default value is None representing empty namespace.

class SonicV2Connector(DBInterface):
def init(self, use_unix_socket_path=False, namespace=None, **kwargs):

If the user don't explicitly set this parameter, namespace takes None as value, and connects to the db_name in the local context, which refers to the database docker running in the current namespace wherever you are running this application/script …. (It could be either Linux host or any specific network namespace ). In this way it is backward compatible and the existing behavior of APIs in these utility classes are maintained.

In the SonicDBConfig, a new API is introduced load_sonic_global_db_config() which loads the /var/run/redis/sonic-db/database_global.json file if present. This file has the mapping between the namespace name and the corresponding database_config.json file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants