Skip to content

[db_migrator] Support migrating database regarding buffer configuration for all Mellanox switches#993

Merged
liat-grozovik merged 11 commits intosonic-net:masterfrom
stephenxs:db_migrator-general
Aug 13, 2020
Merged

[db_migrator] Support migrating database regarding buffer configuration for all Mellanox switches#993
liat-grozovik merged 11 commits intosonic-net:masterfrom
stephenxs:db_migrator-general

Conversation

@stephenxs
Copy link
Copy Markdown
Collaborator

@stephenxs stephenxs commented Jul 17, 2020 •
edited
Loading

This is the updated version of #990. As the original branch has been forcely updated, we need to open a new PR for it.
All comments in that PR have been addressed in this one.

- What I did

  1. Move the code for Mellanox buffer configuration migrating to a separate file.
    The motivation is not to put too much code in db_migrator.py which can distract others from the main logic.
  2. Reorganize the code in a graceful and easy-to-maintain way.
    • Extract all the buffer configuration into a separate function so that it can be used in different rounds of migrations.
      The buffer configuration of each database version will be used twice:
      • the time when migrating from the old version to the current version
      • the time when from the current version to the new version.
    • The logic of Mellanox buffer migrator is:
      1. Compare the current buffer configuration with the default settings
      2. If they match, map the old value to the new one
      3. Insert the new setting into CONFIG_DB.
    • The granularity of matching is:
      • For buffer pool, the configuration of all pools is compared as a whole, which means the update is performed only if the configuration of all pools matches the default value.
      • For buffer profile for per port reserved ingress/egress buffer, the configuration of all profiles are compared as a whole, which means the update is performed only if the configuration of all that kind of profiles matches the default value
      • For profile for lossless PGs, each profile of a speed, cable length pair is compared separately.
  3. Support migrating database regarding buffer configuration (database version 1.0.4) for all Mellanox switches

Signed-off-by: Stephen Sun stephens@mellanox.com

- How I did it

- How to verify it

- 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)

Stephen Sun added 2 commits July 17, 2020 02:31
[db_migrator] Support migrate to single ingress buffer pool mode
53d7e05 
db_migrator supports migrating old configuration who has 2 ingress pools into the new configuration who has 1 ingress pool, including BUFFER_POOL, BUFFER_PROFILE and BUFFER_PORT_INGRESS_PROFILE_LIST

Signed-off-by: Stephen Sun <stephens@mellanox.com>
Update according to MSFT comments
e16a8e7 
1. Don't need to migrate for single buffer pool mode
2. Move buffer setting migration to another file
3. Remove unnecessary code/upgrading flows

Signed-off-by: Stephen Sun <stephens@mellanox.com>
@stephenxs stephenxs mentioned this pull request Jul 17, 2020
Closed
@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Jul 17, 2020

This pull request introduces 2 alerts and fixes 1 when merging e16a8e7 into e3d6ba0 - view on LGTM.com

new alerts:

  • 1 for Unused local variable
  • 1 for Unused argument in a formatting call

fixed alerts:

  • 1 for Unused local variable

Fix LGTM warning
955eac2 
Signed-off-by: Stephen Sun <stephens@mellanox.com>
@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Jul 17, 2020

This pull request fixes 1 alert when merging 955eac2 into e3d6ba0 - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable

Fix an error
daf8b8a 
Signed-off-by: Stephen Sun <stephens@mellanox.com>
@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Jul 17, 2020

This pull request fixes 1 alert when merging daf8b8a into e3d6ba0 - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable

prsunny
prsunny reviewed Jul 17, 2020
View reviewed changes
Comment thread scripts/mellanox_db_migrator.py Outdated
@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Jul 18, 2020

This pull request fixes 1 alert when merging 36ee64e into e3d6ba0 - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable

Fix issue: after migration the lossless profiles are lost
30f7faa 
This issue can fail warm reboot because after warm reboot the buffermgr
doesn't have time to generate lossless profiles and the following
orchagent bake can fail due to this

Signed-off-by: Stephen Sun <stephens@mellanox.com>
@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Jul 18, 2020

This pull request fixes 1 alert when merging 30f7faa into e3d6ba0 - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable

@keboliu
Copy link
Copy Markdown
Collaborator

keboliu commented Jul 20, 2020

retest this please

liat-grozovik
liat-grozovik reviewed Jul 20, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@liat-grozovik liat-grozovik left a comment

Choose a reason for hiding this comment

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

please extend your PR message with more information about motivation, which SKUs are affected and how to have this new values enforced.

@prsunny
Copy link
Copy Markdown
Contributor

prsunny commented Jul 21, 2020

retest this please

@stephenxs stephenxs mentioned this pull request Jul 21, 2020
Merged
@liat-grozovik
Copy link
Copy Markdown
Collaborator

liat-grozovik commented Jul 21, 2020

retest this please

@stephenxs
Copy link
Copy Markdown
Collaborator Author

stephenxs commented Jul 21, 2020

please extend your PR message with more information about motivation, which SKUs are affected and how to have this new values enforced.

Done.

@liat-grozovik
Copy link
Copy Markdown
Collaborator

liat-grozovik commented Jul 21, 2020

Retest this please

prsunny
prsunny previously approved these changes Jul 21, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@prsunny prsunny left a comment

Choose a reason for hiding this comment

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

lgtm, @neethajohn to review the buffer pool values

@jleveque
Copy link
Copy Markdown
Contributor

jleveque commented Jul 22, 2020

Retest this please

@stephenxs stephenxs marked this pull request as draft July 22, 2020 06:30
Update buffer configuration for version 1.0.4
e28c823 
Signed-off-by: Stephen Sun <stephens@mellanox.com>
@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Jul 22, 2020

This pull request fixes 1 alert when merging e28c823 into 995cf39 - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable

Update the buffer setting for version 1.0.4
2a7b809 
Signed-off-by: Stephen Sun <stephens@mellanox.com>
@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Jul 29, 2020

This pull request fixes 1 alert when merging 2a7b809 into aa1b072 - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable

@stephenxs stephenxs marked this pull request as ready for review July 29, 2020 07:35
@liat-grozovik liat-grozovik requested a review from neethajohn July 30, 2020 11:29
@stephenxs stephenxs requested review from neethajohn and prsunny August 4, 2020 23:09
@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Aug 4, 2020

This pull request fixes 1 alert when merging e256e93 into 5d26391 - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable

@stephenxs
Copy link
Copy Markdown
Collaborator Author

stephenxs commented Aug 4, 2020

Hi @prsunny , @neethajohn ,
All comments are addressed. Could you please approve and merge?
Thank you!
Stephen

prsunny
prsunny previously approved these changes Aug 5, 2020
View reviewed changes
@liat-grozovik
Copy link
Copy Markdown
Collaborator

liat-grozovik commented Aug 6, 2020

@neethajohn could you please review and see if this can be merged?
we would like to have it also on 201911 and to make sure that if someone is upgrading from earlier image w/o the new buffer calculation will get the new values without doing any manual work, so it will be just simply working for users who are not going via the minigraph reload approach each upgrade.

neethajohn
neethajohn previously approved these changes Aug 7, 2020
View reviewed changes
@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Aug 10, 2020

This pull request introduces 1 alert and fixes 1 when merging 488173d into 0225c09 - view on LGTM.com

new alerts:

  • 1 for Unused import

fixed alerts:

  • 1 for Unused local variable

@liat-grozovik
Copy link
Copy Markdown
Collaborator

liat-grozovik commented Aug 10, 2020

retest this please

[db_migrator] Adjust db_migrator according to the latest master change
66229e5 
Signed-off-by: Stephen Sun <stephens@mellanox.com>
@stephenxs stephenxs dismissed stale reviews from neethajohn and prsunny via 66229e5 August 11, 2020 11:03
@stephenxs
Copy link
Copy Markdown
Collaborator Author

stephenxs commented Aug 11, 2020 •
edited
Loading

Hi @prsunny @neethajohn,
I push another commit, adjusting the code according to the latest db_migrator change.
Please review. Thanks.

@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Aug 11, 2020

This pull request fixes 1 alert when merging 66229e5 into 7ae8024 - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable

@stephenxs
Copy link
Copy Markdown
Collaborator Author

stephenxs commented Aug 11, 2020

retest this please

@liat-grozovik liat-grozovik merged commit d5fdd74 into sonic-net:master Aug 13, 2020
@stephenxs stephenxs mentioned this pull request Aug 14, 2020
Merged
3 tasks
@stephenxs stephenxs deleted the db_migrator-general branch August 14, 2020 03:18
stephenxs added a commit to stephenxs/sonic-utilities that referenced this pull request Aug 14, 2020
@stephenxs
[db_migrator] Support migrating database regarding buffer configurati…
5939c41 
…on for all Mellanox switches

This is to backport PR sonic-net#993 to 201911

Signed-off-by: Stephen Sun <stephens@nvidia.com>
@stephenxs stephenxs mentioned this pull request Aug 14, 2020
Merged
abdosi pushed a commit that referenced this pull request Aug 21, 2020
@stephenxs
[db_migrator] Support migrating database regarding buffer configurati…
323073e 
…on for all Mellanox switches (#1053)

This is to backport PR #993 to 201911

Signed-off-by: Stephen Sun <stephens@nvidia.com>
@abdosi
Copy link
Copy Markdown
Contributor

abdosi commented Sep 3, 2020

@stephenxs Please create PR for 201911

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@neethajohn neethajohn neethajohn approved these changes

@liat-grozovik liat-grozovik Awaiting requested review from liat-grozovik

@prsunny prsunny Awaiting requested review from prsunny

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@stephenxs @keboliu @prsunny @liat-grozovik @jleveque @abdosi @neethajohn