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-asic][cli][chassis-db] Avoid connecting to chassis db when cli commands are executed from linecards #8065

Merged
merged 2 commits into from Sep 14, 2021

Conversation

vganesan-nokia
Copy link
Contributor

@vganesan-nokia vganesan-nokia commented Jul 5, 2021

Why I did it

Currently, for all the cli commands, we connect to all databases mentioned in the database_config.json. The database_config.json also includes the databases from chassis redis server from supervisor card. It is unnecessary to connect to databases from chassis redis server when cli commands are executed form linecard. But we need to allow connection to chassis databases when the cli commands are executed from supervisor card. This PR fixes this problem

How I did it

This PR requires that platform_env.conf in supervisor card includes a variable "supervisor" with value 1 to identify the supervisor card. The connect_to_all_dbs_for_ns() is changed to skip chassis databases form the list of collected databases if the card is not supervisor card. This PR requires sonic-utilities PR sonic-net/sonic-utilities#1707 to have complete fix

How to verify it

In voq chassis, execute cli commands (show, config etc.) and make sure that commands are successful irrespective whether there is supervisor card or not.

Which release branch to backport (provide reason below if selected)

N/A

Description for the changelog

multi-asic cli to skip connecting to chassis databases when commands are executed from line card.

@rlhui
Copy link
Contributor

rlhui commented Jul 7, 2021

@lguohan - would you like to opt out of reviewing this PR?

@anshuv-mfst
Copy link

Hi @mlorrillere - could you please review, thanks.

@mlorrillere
Copy link
Contributor

Hi @vganesan-nokia ,

It seems that on a multi-asic pizza box this would require to configure VOQ_SUPERVISOR=1 as well.

Would it make more sense to rely on start_chassis_db=1 set in chassisdb.conf to connect to chassis db from CLI?

@vganesan-nokia
Copy link
Contributor Author

Hi @vganesan-nokia ,

It seems that on a multi-asic pizza box this would require to configure VOQ_SUPERVISOR=1 as well.

Would it make more sense to rely on start_chassis_db=1 set in chassisdb.conf to connect to chassis db from CLI?

Hi @mlorrillere, In multi-asic pizza box systems which are not distributed VOQ systems, we always want to avoid connecting to chassis databases since they do not exist in pizza box systems. Simply not having this VOQ_SUPERVISOR variable in the asic.conf (as it is today in multi-asic pizza box systems) will be enough. If we do not have this variable defined, we'll skip connecting to chassis databases. Also chassisdb.conf may not exist in pizza box systems

@mlorrillere
Copy link
Contributor

Hi @vganesan-nokia ,
It seems that on a multi-asic pizza box this would require to configure VOQ_SUPERVISOR=1 as well.
Would it make more sense to rely on start_chassis_db=1 set in chassisdb.conf to connect to chassis db from CLI?

Hi @mlorrillere, In multi-asic pizza box systems which are not distributed VOQ systems, we always want to avoid connecting to chassis databases since they do not exist in pizza box systems. Simply not having this VOQ_SUPERVISOR variable in the asic.conf (as it is today in multi-asic pizza box systems) will be enough. If we do not have this variable defined, we'll skip connecting to chassis databases. Also chassisdb.conf may not exist in pizza box systems

Right, but on a multi-asic pizza box that is a VOQ system, we would need to set VOQ_SUPERVISOR=1. Am I correct?

@vganesan-nokia
Copy link
Contributor Author

Hi @vganesan-nokia ,
It seems that on a multi-asic pizza box this would require to configure VOQ_SUPERVISOR=1 as well.
Would it make more sense to rely on start_chassis_db=1 set in chassisdb.conf to connect to chassis db from CLI?

Hi @mlorrillere, In multi-asic pizza box systems which are not distributed VOQ systems, we always want to avoid connecting to chassis databases since they do not exist in pizza box systems. Simply not having this VOQ_SUPERVISOR variable in the asic.conf (as it is today in multi-asic pizza box systems) will be enough. If we do not have this variable defined, we'll skip connecting to chassis databases. Also chassisdb.conf may not exist in pizza box systems

Right, but on a multi-asic pizza box that is a VOQ system, we would need to set VOQ_SUPERVISOR=1. Am I correct?

I see. In that case, I imagine, we'll be running the redis-chassis server (where the chassis databases are hosted) along with other asic instance specific redis servers in the same box. The VOQ pizza box behaves like a supervisor card in addition to being a line card or vice versa. I think setting VOQ_SUPERVISOR=1 in this case is o.k. and connecting to the chassis databases should be fine.

Using chassisdb.conf vs asic,conf for VOQ_SUPERVISOR: The existing CLI implementation has tested infra apis for setting path for asic.conf for file access. I just made use of it. If we have to use chassisdb.conf, we need to add additional code similar to asic.conf path setting apis for file access. I see using chassisdb.conf avoids using an additional variable in asic.conf. Otherwise start_chassis_db in chassisdb.conf and VOQ_SUPERVISOR in asic.conf give same information (that the host is a supervisor card). I do not see any significant advantage in choosing chassisdb.conf over asic.conf. If avoiding using additional variable in asic.conf is preferred over adding additional code for chassisdb.conf, I can do the modifications.

mlorrillere
mlorrillere previously approved these changes Jul 12, 2021
Copy link
Contributor

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

src/sonic-py-common/sonic_py_common/multi_asic.py Outdated Show resolved Hide resolved
src/sonic-py-common/sonic_py_common/device_info.py Outdated Show resolved Hide resolved
vedganes added 2 commits August 18, 2021 12:02
Currently, for all the cli commands, we connect to all databases
mentioned in the database_config.json. The database_config.json also
includes the databases from chassis redis server from supervisor card.
It is unneccessary to connect to databases from chassis redis server
when cli commands are executed form linecard. But we need to allow
connection to chassis databases when the cli commands are executed from
supervisor card.

The changes in this PR fixes this problem. This PR requires that
asic.conf in supervisor card includes VOQ_SUPERVISOR with value 1 to
indentify the supervisor card. The connect_to_all_dbs_for_ns() is
changed to skip chassis databases form the list of collected databases
if the card is not supervisor card.

Signed-off-by: vedganes <vedavinayagam.ganesan@nokia.com>
- is_voq_supervisor() renamed to is_supervisor() since this API is
applicable to non VOQ chassis also
- Determination of supervisor card type is done using "supervisor"
variable from platfrom_env.conf instead of from asic.conf
- Removed changes unrelated to this fix.

Signed-off-by: vedganes <vedavinayagam.ganesan@nokia.com>
@arlakshm
Copy link
Contributor

@vganesan-nokia, can you resolve the conflicts ?

@vganesan-nokia
Copy link
Contributor Author

@arlakshm, I force pushed to resolve the conflict. But the build seems to be stuck. Would you please re-start the azure pipeline? Thanks

@arlakshm
Copy link
Contributor

/Azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

if not is_supervisor():
try:
db_list.remove('CHASSIS_APP_DB')
db_list.remove('CHASSIS_STATE_DB')
Copy link
Contributor

Choose a reason for hiding this comment

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

Good to have an function returning back a list of chassis db instances in supervisor.
But it is ok to do it later as well.

@judyjoseph judyjoseph merged commit 1863e1f into sonic-net:master Sep 14, 2021
judyjoseph pushed a commit that referenced this pull request Sep 27, 2021
… commands are executed from linecards (#8065)

* [multi-asic][cli][chassis-db] Avoiding connecting to chassis db

Currently, for all the cli commands, we connect to all databases
mentioned in the database_config.json. The database_config.json also
includes the databases from chassis redis server from supervisor card.
It is unneccessary to connect to databases from chassis redis server
when cli commands are executed form linecard. But we need to allow
connection to chassis databases when the cli commands are executed from
supervisor card.

The changes in this PR fixes this problem. This PR requires that
asic.conf in supervisor card includes VOQ_SUPERVISOR with value 1 to
indentify the supervisor card. The connect_to_all_dbs_for_ns() is
changed to skip chassis databases form the list of collected databases
if the card is not supervisor card.
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

6 participants