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

[Dynamic Buffer Calc][202012]Bug fix: Don't create lossless buffer profile for active ports without speed configured #1820

Merged
merged 2 commits into from
Jul 29, 2021

Conversation

stephenxs
Copy link
Collaborator

@stephenxs stephenxs commented Jul 15, 2021

What I did
Bugfix: Don't create lossless buffer profiles for active ports without speed configured
This is to backport PR #1822 to 202012.

Root cause:

  • In handlePortTableUpdate, refreshPgsFromPort is called if admin status is updated even if the speed is not configured.
    This is reasonable because the port can be configured as headroom override and the profile should be applied in that case.
  • However, as a side-effect, the port's state is set to PORT_READY in refreshPgsForPort regardless of whether the speed is configured, which is not correct.
    This is should be avoided and PORT_READY should be set by the caller if necessary

Fix:

  • Don't set the port's state to PORT_READY in refreshPgsForPort and check the port's state before calling it.

Note:

  • The change is covered by the existing vs tests.
  • The scenario where the bug was originally found can not be covered by vs test because:
    • The speed is always configured in vs image by default
    • Removing speed is not handled in buffer manager since it's not a valid flow.
  • A regression test will be opened to cover this case.

Signed-off-by: Stephen Sun [email protected]

Why I did it

How I verified it
Regression test and vs test.

Details if related

… configured

Root cause:
- In handlePortTableUpdate, refreshPgsFromPort is called if admin status is updated even if the speed is not configured.
  This is reasonable because the port can be configured as headroom override and the profile should be applied in that case.
- However, the port's state is set to PORT_READY in refreshPgsForPort regardless whether speed is configured, which is not correct.
  This is should be avoided and PORT_READY should be set by caller if necessary
Fix:
- Don't set port's state to READY in refreshPgsForPort. This has been done by all callers.

Signed-off-by: Stephen Sun <[email protected]>
@stephenxs stephenxs changed the title [Dynamic Buffer Calc][202012]Bug fix: Don't create lossless buffer profile for ports without speed configured [Dynamic Buffer Calc][202012]Bug fix: Don't create lossless buffer profile for active ports without speed configured Jul 23, 2021
@qiluo-msft qiluo-msft merged commit f54b7d0 into sonic-net:202012 Jul 29, 2021
@stephenxs stephenxs deleted the fix-no-speed-issue-202012 branch July 30, 2021 03:19
qiluo-msft pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Jul 30, 2021
Update submodule for swss

f54b7d0b [Dynamic Buffer Calc][202012]Bug fix: Don't create lossless buffer profile for active ports without speed configured (sonic-net/sonic-swss#1820)
ac7f5cff Td2: Reclaim buffer from unused ports (sonic-net/sonic-swss#1830)
04105a4b [debugcounterorch] check if counter type is supported before querying (sonic-net/sonic-swss#1789)
a67d8af6 [202012][portsorch] fix errors when moving port from one lag to another. (sonic-net/sonic-swss#1819)
lguohan pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Jul 31, 2021
This PR includes the following commits

a67d8af [202012][portsorch] fix errors when moving port from one lag to another (sonic-net/sonic-swss#1819)
04105a4 [debugcounterorch] check if counter type is supported before querying (sonic-net/sonic-swss#1789)
ac7f5cff Td2: Reclaim buffer from unused ports (sonic-net/sonic-swss#1830)
f54b7d0 [Dynamic Buffer Calc][202012]Bug fix: Don't create lossless buffer profile for active ports without speed configured  (sonic-net/sonic-swss#1820)

Signed-off-by: Neetha John <[email protected]>
EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
…1820)

#### What I did

As discussed in this PR sonic-net/sonic-utilities#1814 (comment), only the stop.job should have job-mode set to replace irreversibly.

Otherwise,  simultaneous config reloads in the quick succession, can lead to the behavior.

Although ,when the restart job (and all the other dependent jobs) are finished and is taken out of systemd's job queue, the next stop job will not be cancelled.
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