[Dynamic buffer calc] Test cases for dynamic buffer calculation #1971

stephenxs · 2020-07-27T03:18:56Z

Description of PR

Summary:
Support test cases for dynamic buffer calculation

Type of change

Bug fix
Testbed and Framework(new/improvement)
Test case(new/improvement)

Approach

What is the motivation for this PR?

Support test cases for dynamic buffer calculation

How did you do it?

To test whether the buffer configuration (BUFFER_POOL, BUFFER_PROFILE, BUFFER_PG) has been correctly deployed to APPL_DB on receiving the following event:

a port's speed, cable length, MTU updated
a port's lossless PG updated
headroom override configured or removed

QoS test has been updated accordingly (fetch tables from tables in APPL_DB instead of CONFIG_DB

How did you verify/test it?

Any platform specific information?

Supported testbed topology if it's a new test case?

all topo.

Documentation

liat-grozovik · 2020-08-13T10:16:07Z

@neethajohn will you be able to review this PR? this is a good enhancement IMO which can help to run on different SKUs same test.
@stephenxs as the pending PR is already merged, i think you can move this one from draft.

stephenxs · 2020-08-13T10:50:41Z

@neethajohn will you be able to review this PR? this is a good enhancement IMO which can help to run on different SKUs same test.
@stephenxs as the pending PR is already merged, i think you can move this one from draft.

Hi @liat-grozovik
I'll mark it as ready to review after rebasing it.
This is test cases for dynamic buffer calculation, so it can't be merged ahead of the feature itself. The one you mentioned, I guess, should be the PR this one depended on.

tests/qos/test_buffer.py

stephenxs · 2020-10-28T00:41:02Z

This PR depends on PR #2418

lgtm-com · 2020-10-31T08:56:04Z

This pull request introduces 1 alert when merging 549c00faf5d44a9d1176731ded5cac8848904ac1 into 9507f72 - view on LGTM.com

new alerts:

1 for Unused local variable

neethajohn

Will the values for xon, xoff and headroom that are configured as part of various tests work for all vendors?

Other minor comments:
Please use pytest_assert instead of assert.
Use google style doc strings

tests/qos/qos_sai_base.py

tests/qos/test_buffer.py

liat-grozovik

fix typo [Dyanmic] to [Dynamic]

tests/qos/test_buffer.py

1. Add test cases for dynamic buffer calculation - changing speed/cable length test - headroom override - additional lossless PG - exceeding the accumulative headroom size of a port - default buffer model 2. Adjust qos test, fetching configuration from: - APPL_DB if there is buffer_model defined, this is the case in master - CONF_DB otherwise, for 201911 3. Additional checking script to test whether the buffer model is as expected. This can be used by warm-reboot Signed-off-by: Stephen Sun <stephens@nvidia.com>

use pytest_assert and google style doc strings verify buffer profile and pool against ASIC_DB move hardcoded parameters to a predefined json file add comments to make it easy to understand Signed-off-by: Stephen Sun <stephens@nvidia.com>

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

**What I did** ***Support dynamic buffer calculation*** 1. Extend the CLI options for buffermgrd: - -a: asic_table provided, - -p: peripheral_table provided The `buffermgrd` will start the dynamic headroom calculation mode with -a provided. Otherwise, it will start the legacy mode (pg_headroom_profile looking up) 2. A new class is provided for dynamic buffer calculation while the old one remains. The daemon will instantiate the corresponding class according to the CLI option when it starts. 3. In both modes, the `buffermgrd` will copy BUFFER_XXX tables from CONFIG_DB to APPL_DB and the `bufferorch` will consume BUFFER_XXX tables from APPL_DB ***Backward compatibility*** For legacy mode, the backward compatibility is provided. As mentioned above, `buffermgrd` will check whether the json file representing the `ASIC_TABLE` exists when it starts. - If yes it will start the dynamic buffer calculating mode - Otherwise, it will start the compatible mode which is the old looking up mode in the new code committed in this PR. This logic is in `cfgmgr/buffermgrd.cpp`. The logic of buffer handling in `buffermgrd` isn't changed in the legacy mode. The differences are: - in legacy mode which is the old code, there isn't any buffer related table in `APPL_DB`. All tables are in `CONFIG_DB`. - `buffermgrd` listens to `PORT` and `CABLE_LENGTH` tables in `CONFIG_DB` and inserts the buffer profiles into `BUFFER_PROFILE` table. - `bufferorch` listens to buffer related tables in `CONFIG_DB` and call SAI API correspondingly. - In the compatible mode, `buffermgrd` listens to tables in `CONFIG_DB` and copies them into `APPL_DB` - `buffermgrd` - listens to `PORT` and `CABLE_LENGTH` tables in `CONFIG_DB` and inserts the buffer profiles into `BUFFER_PROFILE` table in `CONFIG_DB` (not changed) - listens to buffer related tables in `CONFIG_DB` and copies them into `APPL_DB` - `bufferorch` listens to `APPL_DB` and call SAI API correspondingly. (the difference is the db it listens to). - `db_migrator` is responsible to copy the buffer related tables from `CONFIG_DB` to `APPL_DB` when system is warmbooted from the old image to the new image for the first time. The compatible code is in `cfgmgr/buffermgr.cpp`, `orchagent/bufferorch.cpp` and `db_migrator` (in the [sonic-utilities PR](sonic-net/sonic-utilities#973)). **Why I did it** **How I verified it** 1. vs test 2. regression test [PR: [Dynamic buffer calc] Test cases for dynamic buffer calculation](sonic-net/sonic-mgmt#1971) **Dynamic buffer details** 1. In the dynamic buffer calculation mode, there are 3 lua plugins are provided for vendor-specific operations: - buffer_headroom_<vendor>.lua, for calculating headroom size. - buffer_pool_<vendor>.lua, for calculating buffer pool size. - buffer_check_headroom_<vendor>.lua, for checking whether headroom exceeds the limit 2. During initialization, The daemon will: - load asic_table and peripheral_table from the given json file, parse them and push them into STATE_DB.ASIC_TABLE and STATE_DB.PERIPHERAL_TABLE respectively - load all plugins - try to load the STATE_DB.BUFFER_MAX_PARAM.mmu_size which is used for updating buffer pool size - a timer will be started for periodic buffer pool size audit 3. The daemon will listen to and handle the following tables from CONFIG_DB The tables will be cached internally in the daemon for the purpose of saving access time - BUFFER_POOL: - if the size is provided: insert the entry to APPL_DB - otherwise: cache them and push to APPL_DB after the size is calculated by lua plugin - BUFFER_PROFILE and BUFFER_PG: - items for ingress lossless headroom need to be cached and handled (according to the design) - other items will be inserted to the APPL_DB directly - PORT_TABLE, for ports' speed and MTU update - CABLE_LENGTH, for ports' cable length 4. Other tables will be copied to APPL_DB directly: - BUFFER_QUEUE - BUFFER_PORT_INGRESS_PROFILE_LIST - BUFFER_PORT_EGRESS_PROFILE_LIST 5. BufferOrch modified accordingly: - Consume buffer relevant tables from APPL_DB instead of CONFIG_DB - For BUFFER_POOL, don't set ingress/egress and static/dynamic to sai if the pool has already existed because they are create-only - For BUFFER_PROFILE, don't set pool for the same reason 6. Warm reboot: - db_migrator is responsible for copying the data from CONFIG_DB to APPL_DB if the switch is warm-rebooted from an old image to the new image for the first time - no specific handling in the daemon side 7. Provide vstest script

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

tests/qos/test_buffer.py

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

liat-grozovik · 2020-12-30T06:46:54Z

@neethajohn would you like to review and provide comments or should I go and merge it?

It's more convenent to leave it as a class member for users Signed-off-by: Stephen Sun <stephens@nvidia.com>

liat-grozovik · 2021-01-06T06:12:34Z

retest vsimage please

tests/qos/test_buffer.py

- Use capital letters for global variable names - Remove spaces between "=" in argument list Signed-off-by: Stephen Sun <stephens@nvidia.com>

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

stephenxs changed the title ~~Dynamic buffer calculation~~ Test cases for dynamic buffer calculation Jul 27, 2020

SavchukRomanLv reviewed Aug 14, 2020

View reviewed changes

tests/qos/test_buffer.py Show resolved Hide resolved

stephenxs marked this pull request as ready for review August 26, 2020 02:49

stephenxs changed the title ~~Test cases for dynamic buffer calculation~~ [Dyanmic buffer calc] Test cases for dynamic buffer calculation Aug 31, 2020

stephenxs mentioned this pull request Sep 5, 2020

[Dynamic buffer calc] Support dynamic buffer calculation sonic-net/sonic-swss#1338

Merged

yxieca force-pushed the master branch from 885053e to 8f9cb56 Compare October 31, 2020 02:57

neethajohn reviewed Nov 20, 2020

View reviewed changes

stephenxs requested a review from neethajohn December 3, 2020 16:00

liat-grozovik reviewed Dec 8, 2020

View reviewed changes

liat-grozovik requested changes Dec 8, 2020

View reviewed changes

stephenxs changed the title ~~[Dyanmic buffer calc] Test cases for dynamic buffer calculation~~ [Dynamic buffer calc] Test cases for dynamic buffer calculation Dec 9, 2020

Stephen Sun and others added 2 commits December 15, 2020 10:59

Address comments:

8ee5033

use pytest_assert and google style doc strings verify buffer profile and pool against ASIC_DB move hardcoded parameters to a predefined json file add comments to make it easy to understand Signed-off-by: Stephen Sun <stephens@nvidia.com>

stephenxs requested a review from liat-grozovik December 15, 2020 08:00

Fix review comments

aa5c072

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

Fix a typo and remove unused code

b1ea1e9

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

liat-grozovik reviewed Dec 28, 2020

View reviewed changes

tests/qos/test_buffer.py Show resolved Hide resolved

tests/qos/test_buffer.py Outdated Show resolved Hide resolved

tests/qos/test_buffer.py Outdated Show resolved Hide resolved

tests/qos/test_buffer.py Outdated Show resolved Hide resolved

stephenxs added 2 commits December 28, 2020 18:18

Fix a syntax error by replacing "has" with "have"

5bbf23a

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

Use the capital letters at the beginning of the sentence.

6d7e3a2

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

liat-grozovik approved these changes Dec 28, 2020

View reviewed changes

liat-grozovik approved these changes Dec 30, 2020

View reviewed changes

Don't make isBufferInApplDb a fixture.

25b7e18

It's more convenent to leave it as a class member for users Signed-off-by: Stephen Sun <stephens@nvidia.com>

neethajohn reviewed Jan 6, 2021

View reviewed changes

tests/qos/test_buffer.py Outdated Show resolved Hide resolved

tests/qos/test_buffer.py Outdated Show resolved Hide resolved

tests/qos/test_buffer.py Outdated Show resolved Hide resolved

tests/qos/test_buffer.py Show resolved Hide resolved

stephenxs added 2 commits January 6, 2021 17:39

Fix review comments

6a0600c

- Use capital letters for global variable names - Remove spaces between "=" in argument list Signed-off-by: Stephen Sun <stephens@nvidia.com>

detect_DEFAULT_MTU => detect_default_mtu

bbbe22f

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

neethajohn approved these changes Jan 6, 2021

View reviewed changes

liat-grozovik merged commit 26dab0a into sonic-net:master Jan 7, 2021

stephenxs deleted the dynamic-buffer-calculation branch January 8, 2021 06:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dynamic buffer calc] Test cases for dynamic buffer calculation #1971

[Dynamic buffer calc] Test cases for dynamic buffer calculation #1971

stephenxs commented Jul 27, 2020 •

edited

Loading

liat-grozovik commented Aug 13, 2020

stephenxs commented Aug 13, 2020 •

edited

Loading

stephenxs commented Oct 28, 2020

lgtm-com bot commented Oct 31, 2020

neethajohn left a comment

liat-grozovik left a comment

liat-grozovik commented Dec 30, 2020

liat-grozovik commented Jan 6, 2021

[Dynamic buffer calc] Test cases for dynamic buffer calculation #1971

[Dynamic buffer calc] Test cases for dynamic buffer calculation #1971

Conversation

stephenxs commented Jul 27, 2020 • edited Loading

Description of PR

Type of change

Approach

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

liat-grozovik commented Aug 13, 2020

stephenxs commented Aug 13, 2020 • edited Loading

stephenxs commented Oct 28, 2020

lgtm-com bot commented Oct 31, 2020

neethajohn left a comment

Choose a reason for hiding this comment

liat-grozovik left a comment

Choose a reason for hiding this comment

liat-grozovik commented Dec 30, 2020

liat-grozovik commented Jan 6, 2021

stephenxs commented Jul 27, 2020 •

edited

Loading

stephenxs commented Aug 13, 2020 •

edited

Loading