Skip to content

[db_migrator]Do DB migration for buffer pool size change on Mellanox platform #833

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

Merged
merged 10 commits into from
Mar 20, 2020
Merged

[db_migrator]Do DB migration for buffer pool size change on Mellanox platform #833

merged 10 commits into from
Mar 20, 2020

Conversation

keboliu
Copy link
Collaborator

@keboliu keboliu commented Mar 10, 2020

- What I did
Do DB migration for buffer pool size change introduced by new SDK, this is specifically for the Mellanox platform.

- How I did it
Advance the DB version to a new version 1_0_3, and add Mellanox specific DB migration functions.
Inside the function to migrate the buffer pool size to the latest values.

- How to verify it
test sonic to sonic upgrade from between various version and different topology.

- Previous command output (if the output of a command-line utility has changed)

- New command output (if the output of a command-line utility has changed)

@yxieca
Copy link
Contributor

yxieca commented Mar 12, 2020

Can this change survive following scenarios:

  1. Newly installed sonic image boots up, no minigraph, no config_db.json.
  2. 0 size config_db.json (might be json parsing error in this case)
  3. empty config_db.json file (contents is '{}')

db_migration towards config_db.json could be too late? migration is invoked after database has loaded the config_db.json into memory.

@keboliu
Copy link
Collaborator Author

keboliu commented Mar 15, 2020

Can this change survive following scenarios:

  1. Newly installed sonic image boots up, no minigraph, no config_db.json.
  2. 0 size config_db.json (might be json parsing error in this case)
  3. empty config_db.json file (contents is '{}')

db_migration towards config_db.json could be too late? migration is invoked after database has loaded the config_db.json into memory.

enhanced the logic to handle more corner cases. the cases you mentioned have been covered.
per my test and code reading, it's true that db_migrator only update the Redis DB, and will keep config_db.json untouched, shall we add a step to dump configuration to config_db.json as the last step of db migration?

simplify the way to generate new buffer configuration.
@liat-grozovik
Copy link
Collaborator

retest this please

1 similar comment
@keboliu
Copy link
Collaborator Author

keboliu commented Mar 17, 2020

retest this please

@yxieca
Copy link
Contributor

yxieca commented Mar 19, 2020

per my test and code reading, it's true that db_migrator only update the Redis DB, and will keep config_db.json untouched, shall we add a step to dump configuration to config_db.json as the last step of db migration?

I don't think we need to save in-memory db back to config_db.json. Because we can always migrate and migrate is one step of load config_db.json. If the migration failed, we definitely don't want to write back. right?

As such, I think you should leave /etc/sonic/config_db.sjon alone. So that it is consistent by itself. And an updated migration code could handle it in the future.

@prsunny
Copy link
Contributor

prsunny commented Mar 19, 2020

per my test and code reading, it's true that db_migrator only update the Redis DB, and will keep config_db.json untouched, shall we add a step to dump configuration to config_db.json as the last step of db migration?

I don't think we need to save in-memory db back to config_db.json. Because we can always migrate and migrate is one step of load config_db.json. If the migration failed, we definitely don't want to write back. right?

As such, I think you should leave /etc/sonic/config_db.sjon alone. So that it is consistent by itself. And an updated migration code could handle it in the future.

Agree to Ying.

@keboliu
Copy link
Collaborator Author

keboliu commented Mar 20, 2020

per my test and code reading, it's true that db_migrator only update the Redis DB, and will keep config_db.json untouched, shall we add a step to dump configuration to config_db.json as the last step of db migration?

I don't think we need to save in-memory db back to config_db.json. Because we can always migrate and migrate is one step of load config_db.json. If the migration failed, we definitely don't want to write back. right?

As such, I think you should leave /etc/sonic/config_db.sjon alone. So that it is consistent by itself. And an updated migration code could handle it in the future.

@yxieca @prsunny then we are ok, I will not do further change to save migrated DB to config_db.json. Thanks.

3800 platform need special buffer configuration
@yxieca yxieca merged commit 07dc201 into sonic-net:master Mar 20, 2020
abdosi pushed a commit that referenced this pull request Mar 24, 2020
…platform (#833)

* do DB migration for buffer pool size change with new SDK version

* fix review comments
enhace migration fail case

* make migrator can work with warm reboot case

* ehnahce the logic to cover more corner case
simplify the way to generate new buffer configuration.

* remove code to get info from config_db.json since it's not necessary
3800 platform need special buffer configuration
abdosi pushed a commit to abdosi/sonic-utilities that referenced this pull request Aug 4, 2020
…platform (sonic-net#833)

* do DB migration for buffer pool size change with new SDK version

* fix review comments
enhace migration fail case

* make migrator can work with warm reboot case

* ehnahce the logic to cover more corner case
simplify the way to generate new buffer configuration.

* remove code to get info from config_db.json since it's not necessary
3800 platform need special buffer configuration
@keboliu keboliu deleted the sonic-to-sonic-upgrade branch January 16, 2021 01:20
prsunny pushed a commit that referenced this pull request Dec 3, 2021
*CLI based on Routed subinterface enhancements HLD #833
*Added support for configuring routed subinterface in short name and long name format
*Updated show command to display user configured subinterfaces in correct format.
abdosi pushed a commit that referenced this pull request Dec 8, 2021
*CLI based on Routed subinterface enhancements HLD #833
*Added support for configuring routed subinterface in short name and long name format
*Updated show command to display user configured subinterfaces in correct format.
stepanblyschak pushed a commit to stepanblyschak/sonic-utilities that referenced this pull request Apr 28, 2022
[fwutil]: Use overlay driver when mounting next image filesystem (sonic-net#825)
Fix for adding L3 interface to Vlan group (sonic-net#826)Fix for adding L3 interface to Vlan group (sonic-net#826)
[db_migrator]Do DB migration for buffer pool size change on Mellanox platform (sonic-net#833)
explicitly specify command with underscores (sonic-net#846)
[intfutil] set speed to 0 when interface speed is not available (sonic-net#839)
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.

5 participants