From 237b89cd4e0bb1bada0f9e9b845f412fb7c6d6d3 Mon Sep 17 00:00:00 2001
From: Stephen Sun <5379172+stephenxs@users.noreply.github.com>
Date: Thu, 29 Jul 2021 16:31:01 +0800
Subject: [PATCH] [Dynamic Buffer Calc] Bug fix: Don't create lossless buffer
 profile for active ports without speed configured (#1822)

**What I did**
Bugfix: Don't create lossless buffer profiles for active ports without speed 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, 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 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.
- Explicitly set port's state to `PORT_READY` in `handlePortStateTable`. This has been done by all other callers.

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.
---
 cfgmgr/buffermgrdyn.cpp | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/cfgmgr/buffermgrdyn.cpp b/cfgmgr/buffermgrdyn.cpp
index 7b4b19388dbf..071fec6f788a 100644
--- a/cfgmgr/buffermgrdyn.cpp
+++ b/cfgmgr/buffermgrdyn.cpp
@@ -987,8 +987,6 @@ task_process_status BufferMgrDynamic::refreshPgsForPort(const string &port, cons
         SWSS_LOG_DEBUG("Nothing to do for port %s since no PG configured on it", port.c_str());
     }
 
-    portInfo.state = PORT_READY;
-
     // Remove the old profile which is probably not referenced anymore.
     if (!profilesToBeReleased.empty())
     {
@@ -1217,6 +1215,9 @@ task_process_status BufferMgrDynamic::doUpdateBufferProfileForDynamicTh(buffer_p
             SWSS_LOG_DEBUG("Checking PG %s for dynamic profile %s", key.c_str(), profileName.c_str());
             portsChecked.insert(portName);
 
+            if (port.state != PORT_READY)
+                continue;
+
             rc = refreshPgsForPort(portName, port.effective_speed, port.cable_length, port.mtu);
             if (task_process_status::task_success != rc)
             {
@@ -1452,6 +1453,7 @@ task_process_status BufferMgrDynamic::handlePortStateTable(KeyOpFieldsValuesTupl
                     {
                         if (isNonZero(portInfo.cable_length) && portInfo.state != PORT_ADMIN_DOWN)
                         {
+                            portInfo.state = PORT_READY;
                             refreshPgsForPort(port, portInfo.effective_speed, portInfo.cable_length, portInfo.mtu);
                         }
                     }