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

[dvs] Use DB namespace to construct DVSDatabase objects #1514

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

daall
Copy link
Contributor

@daall daall commented Nov 20, 2020

Signed-off-by: Danny Allen daall@microsoft.com

What I did
I updated the DVSDatabase to refer to DBs by name and depend on the database config as opposed to manually providing the IDs and redis socket path.

Why I did it
This is the preferred way of addressing databases now, and it's also a much cleaner abstraction than ID/redis socket.

How I verified it
Re-ran the tests w/ this change. Also verified that concurrent tests work as expected.

Details if related

Signed-off-by: Danny Allen <daall@microsoft.com>
@daall
Copy link
Contributor Author

daall commented Nov 20, 2020

retest this please

@daall daall marked this pull request as draft November 20, 2020 02:12
vs_db_conf_file = f"/tmp/database_config-{self.pid}.json"

with open(vs_db_conf_file, "w") as f:
json.dump(db_conf, f)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not create a vs_db_conf_file under tests directory, as source code, and initialize from it directly?

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.

As comment

EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
Prevent potential kernel oops if drivers are removed/devices are deinitialized while PMon daemons are still trying to access those devices.
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.

None yet

2 participants