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

Multi DB with namespace support, Introducing the database_global.json… #4477

Merged
merged 14 commits into from
May 9, 2020

Conversation

judyjoseph
Copy link
Contributor

@judyjoseph judyjoseph commented Apr 24, 2020

- Why I did it
To support applications running in the linux host to be able to connect to databases in the namespaces. This uses the changes done to python modules via sonic-net/sonic-py-swsssdk@b9cee36

- How I did it
The changes includes the following

The database_config.json file is a j2 template and dynamically generated
The database_global.json file is a j2 template and dynamically generated in the host database docker service directory /var/run/redis/sonic-db
Mount the various redis directories in the dockers both host based and those running in the namespace.
Update the sonic-cfggen to accept namespace parameter and connect to the database in namesapce using unix socket which is faster.

- How to verify it
Verified by booting up on the multi-asic platform and single asic platform.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@judyjoseph judyjoseph marked this pull request as ready for review April 24, 2020 07:42
@lguohan lguohan requested a review from qiluo-msft April 24, 2020 17:55
@rlhui
Copy link
Contributor

rlhui commented Apr 26, 2020

Retest this please

@@ -211,6 +211,7 @@ def main():
group.add_argument("--preset", help="generate sample configuration from a preset template", choices=get_available_config())
group = parser.add_mutually_exclusive_group()
group.add_argument("-K", "--key", help="Lookup for a specific key")
parser.add_argument("-n", "--namespace", help="Namespace string to use asic0/asic1.../asicn", default=None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you also explain the ignore behavior, and also the -namespace='' behavior in usage?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a simple suggestion on help text.


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

Copy link
Collaborator

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

As comments

@judyjoseph
Copy link
Contributor Author

retest vsimage please

@judyjoseph
Copy link
Contributor Author

retest vs please

1 similar comment
@judyjoseph
Copy link
Contributor Author

retest vs please

@judyjoseph
Copy link
Contributor Author

judyjoseph commented Apr 30, 2020

retest vsimage please

@judyjoseph
Copy link
Contributor Author

retest vsimage please

@judyjoseph
Copy link
Contributor Author

retest broadcom please

@judyjoseph
Copy link
Contributor Author

retest this please

@judyjoseph judyjoseph requested a review from lguohan May 3, 2020 22:06
@judyjoseph
Copy link
Contributor Author

retest broadcom please

@judyjoseph
Copy link
Contributor Author

pushed multiple fixes as a single commit.

@judyjoseph
Copy link
Contributor Author

Rebased the sonic-buildimage to get the PR (#4541) which has the updated logic with sonic-db-cli PING tests to check redis server.

qiluo-msft
qiluo-msft previously approved these changes May 7, 2020
lguohan
lguohan previously approved these changes May 7, 2020
Copy link
Collaborator

@dzhangalibaba dzhangalibaba left a comment

Choose a reason for hiding this comment

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

Please confirm

dockers/docker-database/database_config.json.j2 Outdated Show resolved Hide resolved
@judyjoseph judyjoseph dismissed stale reviews from lguohan and qiluo-msft via 73b6fac May 7, 2020 04:10
@judyjoseph
Copy link
Contributor Author

retest this please

@judyjoseph
Copy link
Contributor Author

retest broadcom please

@dzhangalibaba
Copy link
Collaborator

LGTM now

@judyjoseph
Copy link
Contributor Author

retest broadcom please

1 similar comment
@judyjoseph
Copy link
Contributor Author

retest broadcom please

@judyjoseph judyjoseph merged commit acf465b into sonic-net:master May 9, 2020
@judyjoseph judyjoseph deleted the multi_DB_NS branch May 9, 2020 19:25
@rlhui
Copy link
Contributor

rlhui commented May 10, 2020

@judyjoseph, there's conflict cherry-pick to 201911 branch. Please raise a separate PR for 201911 branch. Thanks.

abdosi pushed a commit that referenced this pull request May 10, 2020
#4477)

* Multi DB with namespace support, Introducing the database_global.json file
for supporting accessing DB's in other namespaces for service running in
linux host

* Updates based on comments

* Adding the j2 templates for database_config and database_global files.

* Updating to retrieve the redis DIR's to be mounted from database_global.json file.

* Additional check to see if asic.conf file exists before sourcing it.

* Updates based on PR comments discussion.

* Review comments update

* Updates to the argument "-n" for namespace used in both context of parsing minigraph and multi DB access.

* Update with the attribute "persistence_for_warm_boot" that was added to database_config.json file earlier.

* Removing the database_config.json file to avioid confusion in future.
We use the database_config.json.j2 file to generate database_config.json files dynamically.

* Update the comments for sudo usage in docker_image_ctrl.j2

* Update with the new logic in PING PONG tests using sonic-db-cli. With this we wait till the
PONG response is received when redis server is up.

* Similar changes in swss and syncd scripts for the PING tests with sonic-db-cli

* Updated with a missing , in the database_config.json.j2 file, Do pip install of j2cli in docker-base-buster.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants