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.

Sign up for GitHub

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

sonic-py-swsssdk: Changes to support SONiC Gearbox Manager #79

Closed
wants to merge 1 commit into from
Closed

sonic-py-swsssdk: Changes to support SONiC Gearbox Manager #79

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions src/swsssdk/config/database_config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,26 @@
"id" : 7,
"separator": "|",
"instance" : "redis"
},
"ASIC_DB2" : {
Copy link
Contributor

Choose a reason for hiding this comment

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

ASIC_DB2 [](start = 9, length = 8)

How many ASIC DBs in long future? We have only 16 databases in Redis.

Copy link
Author

Choose a reason for hiding this comment

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

We have no plans immediately for increasing the count any time soon. However, there may be an issue with this - sairedis now supports multiple switches, and what we may need in the future to deal with the redis limitation is multiple redis databases, one per switch/phy. There is discussion along these lines occurring, but to answer the question, this implementation for VS platforms requires only these new tables, no more, and no more are planned for.

Copy link
Contributor

Choose a reason for hiding this comment

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

The use case "for VS platforms" is quite limited and you should change /var/run/redis/sonic-db/database_config.json in that environment.

The design of the config file is to maintain backward-compatible with previous single redis and hardcoded databse mapping if there is no /var/run/redis/sonic-db/database_config.json. So we have no plan to extend it to support new feature.

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

Copy link
Author

Choose a reason for hiding this comment

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

We are planning to submit changes to the database-docker (sonic-buildimage) to create these tables (via database_config.json.j2 in vs and other build targets).

But for now, the unit tests in sonic-utilities are failing based on what will be an incompatibility with REDIS database as it will be once we complete pushing all gearbox changes through SONiC. And those unit tests failing at gate are dependent on only the db config offered by SonicV2Connector. Not sure I see a way out of this, unless there are APIs in SonicV2Connector that maybe we can extend the list of databases by appending them in some unit test setup function (I looked at the SonicV2Connector methods, see getters and setters but nothing that can be used to extend the list of databases). Maybe the design doesn't allow for that, and not even sure it is the right answer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, you are talking the use case "the unit tests in sonic-utilities". You need to modify /var/run/redis/sonic-db/database_config.json in that environment.

Otherwise if you want more flexibility, such as use different config for different unit test cases. You are expected to mock the classes. ref: https://github.com/Azure/sonic-snmpagent/blob/master/tests/mock_tables/dbconnector.py

In reply to: 434874842 [](ancestors = 434874842,434791844)

Copy link
Author

Choose a reason for hiding this comment

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

We are testing an alternate implementation which does not impact this project, if it works, will drop this pull request. Thanks for the good review comments, very helpful.

"id" : 8,
"separator": ":",
"instance" : "redis"
},
"COUNTERS_DB2" : {
"id" : 9,
"separator": ":",
"instance" : "redis"
},
"FLEX_COUNTER_DB2" : {
"id" : 10,
"separator": ":",
"instance" : "redis"
},
"STATE_DB2" : {
"id" : 11,
"separator": "|",
"instance" : "redis"
}
},
"VERSION" : "1.0"
Expand Down