[Dynamic buffer calc] Bug fix: Remove PGs from an administratively down port. #1652

stephenxs · 2021-02-25T01:19:08Z

What I did
Bug fixes: Remove PGs from an administratively down port.

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

Why I did it
To fix bugs

How I verified it
Run regression and vs test

Which release branch to backport (provide reason below if selected)

201811
201911
202006
202012

Details if related

Remove PGs from an administratively down port.
- Introduce a new state: PORT_ADMIN_DOWN which represents the port is administratively down.
- Remove all PGs when the port is shut down and re-add all configured PGs when port is started up
- Only record the new value but don't touch BUFFER_PG_TABLE if the following events come when a port is administratively down
  - a port's MTU, speed, or cable length is updated
  - a new PG is added to a port or an existing PG is removed from a port
- Optimize the port event handling flow since refreshPriorityGroupsForPort should be called only once in case more than one fields are updated
- Optimize the Lua plugin which calculates the buffer pool size accordingly

Bug: The buffermgrd can keep adding suffix to the buffer pool reference if the buffer pool isn't found when it is being referenced. In most of the cases, it's caused by wrong configuration. Cause: In handleBufferProfileTable, the value of each field is designated by a (lvalue) reference to the object in the tuple, which means the object in the tuple will be changed if the value is changed in the function. Fix: Pass the value of each field by value instead of reference. Signed-off-by: Stephen Sun <stephens@nvidia.com>

Introduce a new state: `PORT_ADMIN_DOWN` which represents the port is administratively down. Remove all PGs when the port is shut down and re-add all configured PGs when the port is started up Only record the new value but don't touch `BUFFER_PG_TABLE` if the following events come when a port is administratively down - a port's MTU, speed, or cable length is updated - a new PG is added to a port or an existing PG is removed from a port Optimize the port event handling flow since `refreshPriorityGroupsForPort` should be called only once in case more than one fields are updated Optimize the Lua plugin which calculates the buffer pool size accordingly Signed-off-by: Stephen Sun <stephens@nvidia.com>

Identify the case that the referenced profile doesn't exist and exit the plugin gracefully. Signed-off-by: Stephen Sun <stephens@nvidia.com>

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

liat-grozovik · 2021-03-02T19:19:49Z

/AzurePipelines run

azure-pipelines · 2021-03-02T19:20:02Z

Azure Pipelines successfully started running 1 pipeline(s).

stephenxs · 2021-03-03T06:32:20Z

VS test failed on the following cases. Can anyone help to check them?

test_route_nhg failed (1 runs remaining out of 2).
	<class 'AttributeError'>
	'DockerVirtualSwitch' object has no attribute 'appldb'
	[<TracebackEntry /home/vsts/work/1/s/tests/conftest.py:1546>, <TracebackEntry /home/vsts/work/1/s/tests/conftest.py:386>, <TracebackEntry /home/vsts/work/1/s/tests/conftest.py:429>, <TracebackEntry /home/vsts/work/1/s/tests/conftest.py:393>]
test_route_nhg failed; it passed 0 out of the required 1 times.
	<class 'AttributeError'>
	'DockerVirtualSwitch' object has no attribute 'appldb'
	[<TracebackEntry /usr/lib/python3/dist-packages/six.py:702>, <TracebackEntry /home/vsts/work/1/s/tests/conftest.py:1546>, <TracebackEntry /home/vsts/work/1/s/tests/conftest.py:386>, <TracebackEntry /home/vsts/work/1/s/tests/conftest.py:429>, <TracebackEntry /home/vsts/work/1/s/tests/conftest.py:393>]
test_route_nhg_exhaust failed (1 runs remaining out of 2).
	<class 'AttributeError'>
	'DockerVirtualSwitch' object has no attribute 'appldb'
	[<TracebackEntry /usr/lib/python3/dist-packages/six.py:702>, <TracebackEntry /home/vsts/work/1/s/tests/conftest.py:1546>, <TracebackEntry /home/vsts/work/1/s/tests/conftest.py:386>, <TracebackEntry /home/vsts/work/1/s/tests/conftest.py:429>, <TracebackEntry /home/vsts/work/1/s/tests/conftest.py:393>]
test_route_nhg_exhaust failed; it passed 0 out of the required 1 times.
	<class 'AttributeError'>
	'DockerVirtualSwitch' object has no attribute 'appldb'
	[<TracebackEntry /usr/lib/python3/dist-packages/six.py:702>, <TracebackEntry /home/vsts/work/1/s/tests/conftest.py:1546>, <TracebackEntry /home/vsts/work/1/s/tests/conftest.py:386>, <TracebackEntry /home/vsts/work/1/s/tests/conftest.py:429>, <TracebackEntry /home/vsts/work/1/s/tests/conftest.py:393>]

test_VlanAddRemove failed (1 runs remaining out of 2).
	<class 'AssertionError'>
	
	[<TracebackEntry /home/vsts/work/1/s/tests/test_vlan.py:34>, <TracebackEntry /home/vsts/work/1/s/tests/dvslib/dvs_vlan.py:61>]

liat-grozovik · 2021-03-07T14:17:37Z

/AzurePipelines run

azure-pipelines · 2021-03-07T14:17:49Z

Azure Pipelines successfully started running 1 pipeline(s).

dprital · 2021-03-08T11:47:12Z

@neethajohn - Can you please approve and merge ? it is also required for 202012. Thanks,

dprital · 2021-03-10T08:56:15Z

@neethajohn - Kindly reminder. Thanks.

cfgmgr/buffermgrdyn.cpp

tests/test_buffer_dynamic.py

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

stephenxs · 2021-03-15T02:08:58Z

/AzurePipelines run

azure-pipelines · 2021-03-15T02:09:03Z

Commenter does not have sufficient privileges for PR 1652 in repo Azure/sonic-swss

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

dprital · 2021-03-19T12:03:56Z

@daall - Can you please merge to 202012 ? Thanks.

stephenxs · 2021-03-19T13:29:23Z

We are waiting for #1630 to be merged first.

…wn port. (#1652) Remove PGs from an administratively down port. - Introduce a new state: PORT_ADMIN_DOWN which represents the port is administratively down. - Remove all PGs when the port is shut down and re-add all configured PGs when port is started up - Only record the new value but don't touch BUFFER_PG_TABLE if the following events come when a port is administratively down, a port's MTU, speed, or cable length is updated, a new PG is added to a port or an existing PG is removed from a port - Optimize the port event handling flow since refreshPriorityGroupsForPort should be called only once in case more than one fields are updated - Optimize the Lua plugin which calculates the buffer pool size according Signed-off-by: Stephen Sun stephens@nvidia.com How I verified it Run regression and vs test

lguohan · 2021-03-19T20:21:24Z

how come this bug fix is so large. is this really a bug fix? @daall , let's not merge this into 202012 for now.

…ively down port. (#1652)" This reverts commit 908e0c6.

neethajohn · 2021-03-19T23:37:09Z

@stephenxs , I am reverting this merge. Please break this PR into multiple PRs of smaller commits - optimizations, port down handling, renaming func/log messages etc

stephenxs · 2021-03-20T01:00:51Z

@stephenxs , I am reverting this merge. Please break this PR into multiple PRs of smaller commits - optimizations, port down handling, renaming func/log messages etc

OK. Will do.

…ively down port. (#1652)" (#1676) This reverts commit 908e0c6.

…wn port. (sonic-net#1652) Remove PGs from an administratively down port. - Introduce a new state: PORT_ADMIN_DOWN which represents the port is administratively down. - Remove all PGs when the port is shut down and re-add all configured PGs when port is started up - Only record the new value but don't touch BUFFER_PG_TABLE if the following events come when a port is administratively down, a port's MTU, speed, or cable length is updated, a new PG is added to a port or an existing PG is removed from a port - Optimize the port event handling flow since refreshPriorityGroupsForPort should be called only once in case more than one fields are updated - Optimize the Lua plugin which calculates the buffer pool size according Signed-off-by: Stephen Sun stephens@nvidia.com How I verified it Run regression and vs test

…ively down port. (sonic-net#1652)" (sonic-net#1676) This reverts commit 908e0c6.

stephenxs closed this Feb 25, 2021

stephenxs reopened this Feb 26, 2021

stephenxs marked this pull request as draft February 26, 2021 23:33

stephenxs changed the title ~~[Dynamic buffer calc] Fix bugs: Remove PGs when port is admin down~~ [Dynamic buffer calc] Fix bugs Feb 28, 2021

stephenxs force-pushed the remove-pg-admin-down branch from f5bb63c to 6566369 Compare March 1, 2021 05:25

stephenxs marked this pull request as ready for review March 1, 2021 05:26

stephenxs changed the title ~~[Dynamic buffer calc] Fix bugs~~ [Dynamic buffer calc] Bug fix: Remove PGs from an administratively down port. Mar 1, 2021

stephenxs added 2 commits March 1, 2021 11:39

Make the buffer_pool_mellnox.lua be more stable

8ba5248

Identify the case that the referenced profile doesn't exist and exit the plugin gracefully. Signed-off-by: Stephen Sun <stephens@nvidia.com>

Fix a typo in test_buffer_dynamic.py and enhance a field name check

353975b

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

keboliu added the Bug 🐛 label Mar 2, 2021

liat-grozovik requested a review from neethajohn March 2, 2021 19:16

liat-grozovik added the Request for 202012 Branch label Mar 2, 2021

liat-grozovik requested changes Mar 10, 2021

View reviewed changes

Fix review comments

1aec3bf

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

stephenxs requested a review from liat-grozovik March 11, 2021 11:31

liat-grozovik previously approved these changes Mar 14, 2021

View reviewed changes

Stablize the test case

2dddae5

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

stephenxs dismissed liat-grozovik’s stale review via 2dddae5 March 15, 2021 11:42

liat-grozovik approved these changes Mar 15, 2021

View reviewed changes

neethajohn approved these changes Mar 18, 2021

View reviewed changes

neethajohn merged commit 908e0c6 into sonic-net:master Mar 18, 2021

stephenxs deleted the remove-pg-admin-down branch March 19, 2021 00:09

daall added the Included in 202012 Branch label Mar 19, 2021

neethajohn added a commit that referenced this pull request Mar 19, 2021

Revert "[Dynamic buffer calc] Bug fix: Remove PGs from an administrat…

a51d865

…ively down port. (#1652)" This reverts commit 908e0c6.

neethajohn mentioned this pull request Mar 19, 2021

Revert "[Dynamic buffer calc] Bug fix: Remove PGs from an administratively down port." #1676

Merged

neethajohn added a commit that referenced this pull request Mar 20, 2021

Revert "[Dynamic buffer calc] Bug fix: Remove PGs from an administrat…

901211f

…ively down port. (#1652)" (#1676) This reverts commit 908e0c6.

daall pushed a commit that referenced this pull request Mar 22, 2021

Revert "[Dynamic buffer calc] Bug fix: Remove PGs from an administrat…

5159a33

…ively down port. (#1652)" (#1676) This reverts commit 908e0c6.

raphaelt-nvidia pushed a commit to raphaelt-nvidia/sonic-swss that referenced this pull request Oct 5, 2021

Revert "[Dynamic buffer calc] Bug fix: Remove PGs from an administrat…

62312d4

…ively down port. (sonic-net#1652)" (sonic-net#1676) This reverts commit 908e0c6.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dynamic buffer calc] Bug fix: Remove PGs from an administratively down port. #1652

[Dynamic buffer calc] Bug fix: Remove PGs from an administratively down port. #1652

stephenxs commented Feb 25, 2021 •

edited

Loading

liat-grozovik commented Mar 2, 2021

azure-pipelines bot commented Mar 2, 2021

stephenxs commented Mar 3, 2021

liat-grozovik commented Mar 7, 2021

azure-pipelines bot commented Mar 7, 2021

dprital commented Mar 8, 2021

dprital commented Mar 10, 2021

stephenxs commented Mar 15, 2021

azure-pipelines bot commented Mar 15, 2021

dprital commented Mar 19, 2021

stephenxs commented Mar 19, 2021

lguohan commented Mar 19, 2021

neethajohn commented Mar 19, 2021

stephenxs commented Mar 20, 2021 •

edited

Loading

[Dynamic buffer calc] Bug fix: Remove PGs from an administratively down port. #1652

[Dynamic buffer calc] Bug fix: Remove PGs from an administratively down port. #1652

Conversation

stephenxs commented Feb 25, 2021 • edited Loading

liat-grozovik commented Mar 2, 2021

azure-pipelines bot commented Mar 2, 2021

stephenxs commented Mar 3, 2021

liat-grozovik commented Mar 7, 2021

azure-pipelines bot commented Mar 7, 2021

dprital commented Mar 8, 2021

dprital commented Mar 10, 2021

stephenxs commented Mar 15, 2021

azure-pipelines bot commented Mar 15, 2021

dprital commented Mar 19, 2021

stephenxs commented Mar 19, 2021

lguohan commented Mar 19, 2021

neethajohn commented Mar 19, 2021

stephenxs commented Mar 20, 2021 • edited Loading

stephenxs commented Feb 25, 2021 •

edited

Loading

stephenxs commented Mar 20, 2021 •

edited

Loading