In modular chassis, add CHASSIS_STATE_DB on control card

[mprabhu-nokia]

HLD: sonic-net/SONiC#646

In modular chassis, add CHASSIS_STATE_DB on control card

• Why I did it
  Modular Chassis has control-cards, line-cards and fabric-cards along with other peripherals. Control-Card CHASSIS_STATE_DB will be the central DB to maintain any state information of cards that is accessible to control-card/

• How I did it
  Adding another DB on an existing REDIS instance running on port 6380.

[lguohan]

@BrynXu can you help to review?

[lguohan]

retest vsimage please

[lguohan]

can you resolve conflict?

[qiluo-msft]

There is no GLOBAL_STATE_DB mentioned in sonic-net/SONiC#646.
Why not use existing STATE_DB? You are free to create new table in that DB. #Closed

[mprabhu-nokia]

There is no GLOBAL_STATE_DB mentioned in Azure/SONiC#646.
Why not use existing STATE_DB? You are free to create new table in that DB.

Added additonal sections in the document with the temperature_info-schema for the table in CHASSIS_STATE_DB . This was discussed in Section 3.2.4.

[shyam77git]

There is no GLOBAL_STATE_DB mentioned in Azure/SONiC#646.
Why not use existing STATE_DB? You are free to create new table in that DB.

Added additonal sections in the document with the temperature_info-schema for the table in CHASSIS_STATE_DB . This was discussed in Section 3.2.4.

Enhancing CC/Supervisor's local redis-DB to add one more table catering to sensors contents? or
existing approach of keeping Global redis-DB separate from local 'redis-DB' on CC/Supervisor?

IMO, local redis-DB of CC is initialized and maintained by PMON's chassisD whereas Global redis-DB of CC is initialized and maintained by PMON's thermalctlD. Since these two are seperate daemons inside PMON container so their respective DB's to be kept independent and restart of one should not impact other

[mprabhu-nokia]

retest vsimage please

[mprabhu-nokia]

retest vsimage please

[mprabhu-nokia]

retest baseimage please

[mprabhu-nokia]

retest vsimage please

[mprabhu-nokia]

retest mellanox please

[mprabhu-nokia]

retest vsimage please

[mprabhu-nokia]

retest vsimage please

[mprabhu-nokia]

retest vsimage please

[mprabhu-nokia]

@BrynXu @judyjoseph

With this change, it fails in below test. Since you made changes to CHASSIS_APP_DB, are you aware why this could fail?
I thought there was a dependency on swss-common. Do we need to add this to "platform/vs/docker-sonic-vs/database_config.json" as well?

TASK [execute cli "config load_minigraph -y" to apply new minigraph] ***********

[mprabhu-nokia]

@BrynXu @judyjoseph

With this change, it fails in below test. Since you made changes to CHASSIS_APP_DB, are you aware why this could fail?
I thought there was a dependency on swss-common. Do we need to add this to "platform/vs/docker-sonic-vs/database_config.json" as well?

TASK [execute cli "config load_minigraph -y" to apply new minigraph] ***********

Looks like the error was because it was not able to find redis_chassis. Will update "files/scripts/update_chassisdb_config" to delete this from the config.

[mprabhu-nokia]

retest baseimage please

[mprabhu-nokia]

retest vs please

[mprabhu-nokia]

retest vsimage please

[mprabhu-nokia]

@BrynXu @qiluo-msft please review

[mprabhu-nokia]

retest baseimage please

[mprabhu-nokia]

retest vsimage please

[mprabhu-nokia]

retest vsimage please

[mprabhu-nokia]

@qiluo-msft @judyjoseph @jleveque could one of you please review/approve this?

[judyjoseph]

Can you check this PR ..#5283 if you need references of CHASSIS_STATE_DB in vs_platform as well ?

[mprabhu-nokia]

Can you check this PR ..#5283 if you need references of CHASSIS_STATE_DB in vs_platform as well ?

@judyjoseph since the CHASSIS_STATE_DB provider/consumers are pmon processes, there was no need to add references in vs platform.

[qiluo-msft]

The only concern is occupying new Redis database index. I prefer other reviewers familiar with this topic to approve.

[mprabhu-nokia]

retest vs please

[judyjoseph]

Discussed with @Qi on this, the suggestion he was making is not to define a new DB for chassis APP/STATE DB .. instead use the existing STATE_DB/APP_DB , and created tables in them with CHASSIS_ prefix in them.
@BrynXu was there any discussion why CHASSIS_APP_DB was added as a new DB with redis instance.

[minionatwork]

Discussed with @Qi on this, the suggestion he was making is not to define a new DB for chassis APP/STATE DB .. instead use the existing STATE_DB/APP_DB , and created tables in them with CHASSIS_ prefix in them.
@BrynXu was there any discussion why CHASSIS_APP_DB was added as a new DB with redis instance.

We introduced new DB because this new DB is hosted in supervisor card and reachable from all linecards. Existing ones aren't reachable to all linecards sonic instances and we would want to them to be isolated than shared.

CHASSIS_STATE_DB is added as part of this PR to keep pmon inline with using STATE_DB instead of APP_DB.

[judyjoseph]

Discussed with @Qi on this, the suggestion he was making is not to define a new DB for chassis APP/STATE DB .. instead use the existing STATE_DB/APP_DB , and created tables in them with CHASSIS_ prefix in them.
@BrynXu was there any discussion why CHASSIS_APP_DB was added as a new DB with redis instance.

We introduced new DB because this new DB is hosted in supervisor card and reachable from all linecards. Existing ones aren't reachable to all linecards sonic instances and we would want to them to be isolated than shared.

CHASSIS_STATE_DB is added as part of this PR to keep pmon inline with using STATE_DB instead of APP_DB.

Thanks ! I am fine with that then. A question though is Can we start the id from 0, 1 for this instance "redis_chassis", instead of continuing the numbering as 12, 13 with the original "redis" instance. Did you see any issues in your tests ?

[BrynXu]

we have an open issue,Duplicated dbid in redis multi-instances causes error <#5351>, to prevent us using duplicated dbid even they are in different redis instance.

…

On Wed, Dec 9, 2020 at 12:00 PM judyjoseph ***@***.***> wrote: Discussed with @Qi <https://github.com/Qi> on this, the suggestion he was making is not to define a new DB for chassis APP/STATE DB .. instead use the existing STATE_DB/APP_DB , and created tables in them with CHASSIS_ prefix in them. @BrynXu <https://github.com/BrynXu> was there any discussion why CHASSIS_APP_DB was added as a new DB with redis instance. We introduced new DB because this new DB is hosted in supervisor card and reachable from all linecards. Existing ones aren't reachable to all linecards sonic instances and we would want to them to be isolated than shared. CHASSIS_STATE_DB is added as part of this PR to keep pmon inline with using STATE_DB instead of APP_DB. Thanks ! I am fine with that then. A question though is Can we start the id from 0, 1 for this instance "redis_chassis", instead of continuing the numbering as 12, 13 with the original "redis" instance. Did you see any issues in your tests ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5624 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADTR7VJEBDEON6OL3Y6BOX3ST7JOZANCNFSM4SQIE7IA> .

[minionatwork]

Discussed with @Qi on this, the suggestion he was making is not to define a new DB for chassis APP/STATE DB .. instead use the existing STATE_DB/APP_DB , and created tables in them with CHASSIS_ prefix in them.
@BrynXu was there any discussion why CHASSIS_APP_DB was added as a new DB with redis instance.

We introduced new DB because this new DB is hosted in supervisor card and reachable from all linecards. Existing ones aren't reachable to all linecards sonic instances and we would want to them to be isolated than shared.
CHASSIS_STATE_DB is added as part of this PR to keep pmon inline with using STATE_DB instead of APP_DB.

Thanks ! I am fine with that then. A question though is Can we start the id from 0, 1 for this instance "redis_chassis", instead of continuing the numbering as 12, 13 with the original "redis" instance. Did you see any issues in your tests ?

As BrynXu mentioned, there is some limitation in reusing ID. thats why it has to be unique across all DB instances even if its different redis server.

[judyjoseph]

Discussed with @Qi on this, the suggestion he was making is not to define a new DB for chassis APP/STATE DB .. instead use the existing STATE_DB/APP_DB , and created tables in them with CHASSIS_ prefix in them.
@BrynXu was there any discussion why CHASSIS_APP_DB was added as a new DB with redis instance.

We introduced new DB because this new DB is hosted in supervisor card and reachable from all linecards. Existing ones aren't reachable to all linecards sonic instances and we would want to them to be isolated than shared.
CHASSIS_STATE_DB is added as part of this PR to keep pmon inline with using STATE_DB instead of APP_DB.

Thanks ! I am fine with that then. A question though is Can we start the id from 0, 1 for this instance "redis_chassis", instead of continuing the numbering as 12, 13 with the original "redis" instance. Did you see any issues in your tests ?

As BrynXu mentioned, there is some limitation in reusing ID. thats why it has to be unique across all DB instances even if its different redis server.

Ok, we will revisit this along this issue resolution, #5351.