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

[db_migrator][201911] Support shared headroom in db_migrator on Mellanox platform #1261

Merged

Conversation

stephenxs
Copy link
Collaborator

@stephenxs stephenxs commented Nov 23, 2020

Added support to upgrade a switch from old version to one with shared headroom pool support

- What I did

  • Generic update: advance the version of database to 1.5.0
  • Mellanox specific change:
    • support shared headroom pool for MSFT SKUs
    • extend the infrastructure of Mellanox buffer migratory:
      • support mapping from old buffer pool configuration set to new one by SKU
      • support migrate headroom info based on SKU
      • condemns the representation of buffer pool configuration to reduce redundant code
      • organize the configuration dict in a better way

- How I did it

- How to verify it
Verify upgrade from 201811 to 201911 with shared headroom functionality
Verify upgrade from early 201911 to 201911 with shared headroom functionality

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

…oom pool

Signed-off-by: Stephen Sun <stephens@nvidia.com>
1. rewording:
partial => condensed
extract => extend
2. remove mapping method "platform" which is not used yet

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

lgtm-com bot commented Nov 23, 2020

This pull request introduces 1 alert when merging dba77b9 into c0df635 - view on LGTM.com

new alerts:

  • 1 for __init__ method returns a value

Signed-off-by: Stephen Sun <stephens@nvidia.com>
@liat-grozovik
Copy link
Collaborator

retest this please

Copy link
Contributor

@neethajohn neethajohn left a comment

Choose a reason for hiding this comment

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

Mellanox db migrator logic is getting complicated as we are adding more versions and skus. We will need a test to actually ensure that the db migrator is working as expected when trying to move across/between branches.

"spc1_t0_pool": {"doublepool": { "size": "5029836" }, "egress_lossless_pool": { "size": "14024599"}},
"spc1_t1_pool": {"doublepool": { "size": "2097100" }, "egress_lossless_pool": { "size": "14024599"}},
"spc2_t0_pool": {"doublepool": { "size": "14983147" }, "egress_lossless_pool": { "size": "34340822"}},
"spc2_t1_pool": {"doublepool": { "size": "9158635" }, "egress_lossless_pool": { "size": "34340822"}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is 3800 pool size excluded?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

AFAIK, 3800 isn't deployed into the production environment. So we don't need to support migrate configuration between different versions of 201911.
We will support migrating configuration from the latest 201911 to master for 3800.

"pg_lossless_50000_300m_profile": {"size": "121856", "xon": "18432"},
"pg_lossless_100000_300m_profile": {"size": "206848", "xon": "18432"},
"pg_lossless_200000_300m_profile": {"size": "376832", "xon": "18432"}}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

3800 headroom?

"q_lossy_profile": {"dynamic_th": "3", "pool": "[BUFFER_POOL|egress_lossy_pool]", "size": "0"}}
}
},
"version_1_0_5": {
Copy link
Contributor

Choose a reason for hiding this comment

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

update comment below

},
"version_1_0_4": {
# version 1.0.4 is introduced for updating the buffer settings
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be possible to add comments on which db migrator version maps to which SONiC branch? It will be easier to cross check all the buffer settings

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure. will add comments for that.

new_config_name = new_config_map
return new_config_name

def mlnx_migrate_extend_condensed_pool(self, pool_config, config_name = None):
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the spaces between assignment, config_name=None

Comment on lines +498 to +499
default_buffer_profiles_old = self.mlnx_default_buffer_parameters(old_version, "buffer_profiles")
default_buffer_profiles_new = self.mlnx_default_buffer_parameters(new_version, "buffer_profiles")
Copy link
Contributor

Choose a reason for hiding this comment

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

all versions do not seem to have "buffer_profiles" defined

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the version not having it, it will get None and the this part will be skipped

scripts/mellanox_buffer_migrator.py Show resolved Hide resolved
scripts/mellanox_buffer_migrator.py Outdated Show resolved Hide resolved
scripts/mellanox_buffer_migrator.py Outdated Show resolved Hide resolved
scripts/mellanox_buffer_migrator.py Show resolved Hide resolved
@stephenxs stephenxs marked this pull request as ready for review December 15, 2020 08:50
@qiluo-msft
Copy link
Contributor

Ask for some unit test in this PR.

Signed-off-by: Stephen Sun <stephens@nvidia.com>
Signed-off-by: Stephen Sun <stephens@nvidia.com>
@liat-grozovik
Copy link
Collaborator

@neethajohn please check if comments handled and you can approve.

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

lgtm-com bot commented Jan 4, 2021

This pull request introduces 7 alerts when merging bba5ce7 into a9ef1c5 - view on LGTM.com

new alerts:

  • 5 for Unused import
  • 1 for Unused local variable
  • 1 for Variable defined multiple times

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

@stephenxs , please fix the lgtm alerts. the new testcases are failing as well. Can you check?

@stephenxs
Copy link
Collaborator Author

@stephenxs , please fix the lgtm alerts. the new testcases are failing as well. Can you check?

Checking...

@lgtm-com
Copy link

lgtm-com bot commented Jan 5, 2021

This pull request introduces 3 alerts when merging 71aa4da into a9ef1c5 - view on LGTM.com

new alerts:

  • 1 for Unused local variable
  • 1 for Unused import
  • 1 for Variable defined multiple times

stephens and others added 3 commits January 5, 2021 04:27
Signed-off-by: stephens <stephens@contoso.com>
Signed-off-by: Stephen Sun <stephens@nvidia.com>
…ffer migrator

Signed-off-by: Stephen Sun <stephens@nvidia.com>
@liat-grozovik
Copy link
Collaborator

@stephenxs can you please confirm all the files on this PR indeed required? there are 83 files changed

@stephenxs
Copy link
Collaborator Author

@stephenxs can you please confirm all the files on this PR indeed required? there are 83 files changed

Yes, all of them are required. Most of the are the mock config_dbs for the db_migrator unit test for each version and SKUs.
We will have even more on master because we will need to verify warmreboot cases as well.

@liat-grozovik liat-grozovik merged commit 1dfb3e8 into sonic-net:201911 Jan 7, 2021
@stephenxs stephenxs deleted the db_migrator-shared-headroom-201911 branch January 7, 2021 07:43
stepanblyschak pushed a commit to stepanblyschak/sonic-utilities that referenced this pull request Apr 28, 2022
 Do not set PG to Buffer porfile mapping again if already exist. (sonic-net#1261)
[sub intf] Use m_lag_id to be the parent port object id when parent port
is LAG (sonic-net#1235)
stepanblyschak pushed a commit to stepanblyschak/sonic-utilities that referenced this pull request Apr 28, 2022
[Submodule update] sonic-utilities
- [db_migrator][201911] Support shared headroom in db_migrator on Mellanox platform (sonic-net#1261)
- Multi-ASIC support show ip/v6 route additional parameters (sonic-net#1333)

Signed-off-by: Stephen Sun <stephens@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants