From 901211fef1cc637bdcaccbd0b39df093abc8ec6d Mon Sep 17 00:00:00 2001 From: Neetha John Date: Fri, 19 Mar 2021 20:23:43 -0700 Subject: [PATCH] Revert "[Dynamic buffer calc] Bug fix: Remove PGs from an administratively down port. (#1652)" (#1676) This reverts commit 908e0c6de6ac1b5df3a9f1da64d583a26c983f05. --- cfgmgr/buffer_pool_mellanox.lua | 73 +++---- cfgmgr/buffermgrdyn.cpp | 331 +++++++++----------------------- cfgmgr/buffermgrdyn.h | 7 +- tests/test_buffer_dynamic.py | 109 ++--------- 4 files changed, 147 insertions(+), 373 deletions(-) diff --git a/cfgmgr/buffer_pool_mellanox.lua b/cfgmgr/buffer_pool_mellanox.lua index bbf49367d0e3..9adbb15a6a16 100644 --- a/cfgmgr/buffer_pool_mellanox.lua +++ b/cfgmgr/buffer_pool_mellanox.lua @@ -12,7 +12,7 @@ local lossypg_400g = 0 local result = {} local profiles = {} -local total_port = 0 +local count_up_port = 0 local mgmt_pool_size = 256 * 1024 local egress_mirror_headroom = 10 * 1024 @@ -30,38 +30,43 @@ end local function iterate_all_items(all_items) table.sort(all_items) + local prev_port = "None" local port + local is_up local fvpairs + local status + local admin_down_ports = 0 for i = 1, #all_items, 1 do - -- Count the number of priorities or queues in each BUFFER_PG or BUFFER_QUEUE item - -- For example, there are: - -- 3 queues in 'BUFFER_QUEUE_TABLE:Ethernet0:0-2' - -- 2 priorities in 'BUFFER_PG_TABLE:Ethernet0:3-4' + -- Check whether the port on which pg or tc hosts is admin down port = string.match(all_items[i], "Ethernet%d+") if port ~= nil then - local range = string.match(all_items[i], "Ethernet%d+:([^%s]+)$") - local profile = redis.call('HGET', all_items[i], 'profile') - local index = find_profile(profile) - if index == 0 then - -- Indicate an error in case the referenced profile hasn't been inserted or has been removed - -- It's possible when the orchagent is busy - -- The buffermgrd will take care of it and retry later - return 1 - end - local size - if string.len(range) == 1 then - size = 1 - else - size = 1 + tonumber(string.sub(range, -1)) - tonumber(string.sub(range, 1, 1)) + if prev_port ~= port then + status = redis.call('HGET', 'PORT_TABLE:'..port, 'admin_status') + prev_port = port + if status == "down" then + is_up = false + else + is_up = true + end end - profiles[index][2] = profiles[index][2] + size - local speed = redis.call('HGET', 'PORT_TABLE:'..port, 'speed') - if speed == '400000' and profile == '[BUFFER_PROFILE_TABLE:ingress_lossy_profile]' then - lossypg_400g = lossypg_400g + size + if is_up == true then + local range = string.match(all_items[i], "Ethernet%d+:([^%s]+)$") + local profile = redis.call('HGET', all_items[i], 'profile') + local index = find_profile(profile) + local size + if string.len(range) == 1 then + size = 1 + else + size = 1 + tonumber(string.sub(range, -1)) - tonumber(string.sub(range, 1, 1)) + end + profiles[index][2] = profiles[index][2] + size + local speed = redis.call('HGET', 'PORT_TABLE:'..port, 'speed') + if speed == '400000' and profile == '[BUFFER_PROFILE_TABLE:ingress_lossy_profile]' then + lossypg_400g = lossypg_400g + size + end end end end - return 0 end -- Connect to CONFIG_DB @@ -69,7 +74,12 @@ redis.call('SELECT', config_db) local ports_table = redis.call('KEYS', 'PORT|*') -total_port = #ports_table +for i = 1, #ports_table do + local status = redis.call('HGET', ports_table[i], 'admin_status') + if status == "up" then + count_up_port = count_up_port + 1 + end +end local egress_lossless_pool_size = redis.call('HGET', 'BUFFER_POOL|egress_lossless_pool', 'size') @@ -104,12 +114,8 @@ end local all_pgs = redis.call('KEYS', 'BUFFER_PG*') local all_tcs = redis.call('KEYS', 'BUFFER_QUEUE*') -local fail_count = 0 -fail_count = fail_count + iterate_all_items(all_pgs) -fail_count = fail_count + iterate_all_items(all_tcs) -if fail_count > 0 then - return {} -end +iterate_all_items(all_pgs) +iterate_all_items(all_tcs) local statistics = {} @@ -124,7 +130,7 @@ for i = 1, #profiles, 1 do size = size + lossypg_reserved end if profiles[i][1] == "BUFFER_PROFILE_TABLE:egress_lossy_profile" then - profiles[i][2] = total_port + profiles[i][2] = count_up_port end if size ~= 0 then if shp_enabled and shp_size == 0 then @@ -146,7 +152,7 @@ local lossypg_extra_for_400g = (lossypg_reserved_400g - lossypg_reserved) * loss accumulative_occupied_buffer = accumulative_occupied_buffer + lossypg_extra_for_400g -- Accumulate sizes for egress mirror and management pool -local accumulative_egress_mirror_overhead = total_port * egress_mirror_headroom +local accumulative_egress_mirror_overhead = count_up_port * egress_mirror_headroom accumulative_occupied_buffer = accumulative_occupied_buffer + accumulative_egress_mirror_overhead + mgmt_pool_size -- Fetch mmu_size @@ -234,6 +240,5 @@ table.insert(result, "debug:egress_mirror:" .. accumulative_egress_mirror_overhe table.insert(result, "debug:shp_enabled:" .. tostring(shp_enabled)) table.insert(result, "debug:shp_size:" .. shp_size) table.insert(result, "debug:accumulative xoff:" .. accumulative_xoff) -table.insert(result, "debug:total port:" .. total_port) return result diff --git a/cfgmgr/buffermgrdyn.cpp b/cfgmgr/buffermgrdyn.cpp index 6906e3c43295..fb00a8a7799e 100644 --- a/cfgmgr/buffermgrdyn.cpp +++ b/cfgmgr/buffermgrdyn.cpp @@ -290,12 +290,6 @@ void BufferMgrDynamic::calculateHeadroomSize(buffer_profile_t &headroom) { auto ret = swss::runRedisScript(*m_applDb, m_headroomSha, keys, argv); - if (ret.empty()) - { - SWSS_LOG_ERROR("Failed to calculate headroom for %s", headroom.name.c_str()); - return; - } - // The format of the result: // a list of strings containing key, value pairs with colon as separator // each is a field of the profile @@ -352,12 +346,6 @@ void BufferMgrDynamic::recalculateSharedBufferPool() // 3. debug information: // debug: - if (ret.empty()) - { - SWSS_LOG_WARN("Failed to recalculate shared buffer pool size"); - return; - } - for ( auto i : ret) { auto pairs = tokenize(i, ':'); @@ -659,9 +647,9 @@ void BufferMgrDynamic::releaseProfile(const string &profile_name) bool BufferMgrDynamic::isHeadroomResourceValid(const string &port, const buffer_profile_t &profile, const string &new_pg = "") { - // port: used to fetch the maximum headroom size - // profile: the profile referenced by the new_pg (if provided) or all PGs - // new_pg: which pg is newly added? + //port: used to fetch the maximum headroom size + //profile: the profile referenced by the new_pg (if provided) or all PGs + //new_pg: which pg is newly added? if (!profile.lossless) { @@ -694,12 +682,6 @@ bool BufferMgrDynamic::isHeadroomResourceValid(const string &port, const buffer_ // a list of strings containing key, value pairs with colon as separator // each is the size of a buffer pool - if (ret.empty()) - { - SWSS_LOG_WARN("Failed to check headroom for %s", profile.name.c_str()); - return result; - } - for ( auto i : ret) { auto pairs = tokenize(i, ':'); @@ -729,44 +711,7 @@ bool BufferMgrDynamic::isHeadroomResourceValid(const string &port, const buffer_ return result; } -task_process_status BufferMgrDynamic::removeAllPgsFromPort(const string &port) -{ - buffer_pg_lookup_t &portPgs = m_portPgLookup[port]; - set profilesToBeReleased; - - SWSS_LOG_INFO("Removing all PGs from port %s", port.c_str()); - - for (auto it = portPgs.begin(); it != portPgs.end(); ++it) - { - auto &key = it->first; - auto &portPg = it->second; - - SWSS_LOG_INFO("Removing PG %s from port %s", key.c_str(), port.c_str()); - - if (portPg.running_profile_name.empty()) - continue; - - m_bufferProfileLookup[portPg.running_profile_name].port_pgs.erase(key); - updateBufferPgToDb(key, portPg.running_profile_name, false); - profilesToBeReleased.insert(portPg.running_profile_name); - portPg.running_profile_name.clear(); - } - - checkSharedBufferPoolSize(); - - // Remove the old profile which is probably not referenced anymore. - if (!profilesToBeReleased.empty()) - { - for (auto &oldProfile : profilesToBeReleased) - { - releaseProfile(oldProfile); - } - } - - return task_process_status::task_success; -} - -// Called when speed/cable length updated from CONFIG_DB +//Called when speed/cable length updated from CONFIG_DB // Update buffer profile of a certain PG of a port or all PGs of the port according to its speed, cable_length and mtu // Called when // - port's speed, cable_length or mtu updated @@ -786,7 +731,7 @@ task_process_status BufferMgrDynamic::removeAllPgsFromPort(const string &port) // 2. Update port's info: speed, cable length and mtu // 3. If any of the PGs is updated, recalculate pool size // 4. try release each profile in to-be-released profile set -task_process_status BufferMgrDynamic::refreshPgsForPort(const string &port, const string &speed, const string &cable_length, const string &mtu, const string &exactly_matched_key = "") +task_process_status BufferMgrDynamic::refreshPriorityGroupsForPort(const string &port, const string &speed, const string &cable_length, const string &mtu, const string &exactly_matched_key = "") { port_info_t &portInfo = m_portInfoLookup[port]; string &gearbox_model = portInfo.gearbox_model; @@ -794,12 +739,6 @@ task_process_status BufferMgrDynamic::refreshPgsForPort(const string &port, cons buffer_pg_lookup_t &portPgs = m_portPgLookup[port]; set profilesToBeReleased; - if (portInfo.state == PORT_ADMIN_DOWN) - { - SWSS_LOG_INFO("Nothing to be done since the port %s is administratively down", port.c_str()); - return task_process_status::task_success; - } - // Iterate all the lossless PGs configured on this port for (auto it = portPgs.begin(); it != portPgs.end(); ++it) { @@ -813,11 +752,16 @@ task_process_status BufferMgrDynamic::refreshPgsForPort(const string &port, cons string newProfile, oldProfile; oldProfile = portPg.running_profile_name; + if (!oldProfile.empty()) + { + // Clear old profile + portPg.running_profile_name = ""; + } if (portPg.dynamic_calculated) { string threshold; - // Calculate new headroom size + //Calculate new headroom size if (portPg.static_configured) { // static_configured but dynamic_calculated means non-default threshold value @@ -842,8 +786,8 @@ task_process_status BufferMgrDynamic::refreshPgsForPort(const string &port, cons SWSS_LOG_DEBUG("Handling PG %s port %s, for static profile %s", key.c_str(), port.c_str(), newProfile.c_str()); } - // Calculate whether accumulative headroom size exceeds the maximum value - // Abort if it does + //Calculate whether accumulative headroom size exceeds the maximum value + //Abort if it does if (!isHeadroomResourceValid(port, m_bufferProfileLookup[newProfile], exactly_matched_key)) { SWSS_LOG_ERROR("Update speed (%s) and cable length (%s) for port %s failed, accumulative headroom size exceeds the limit", @@ -867,12 +811,12 @@ task_process_status BufferMgrDynamic::refreshPgsForPort(const string &port, cons profilesToBeReleased.insert(oldProfile); m_bufferProfileLookup[oldProfile].port_pgs.erase(key); } - - // buffer pg needs to be updated as well - portPg.running_profile_name = newProfile; } - // appl_db Database operation: set item BUFFER_PG|| + // buffer pg needs to be updated as well + portPg.running_profile_name = newProfile; + + //appl_db Database operation: set item BUFFER_PG|| updateBufferPgToDb(key, newProfile, true); isHeadroomUpdated = true; } @@ -892,7 +836,8 @@ task_process_status BufferMgrDynamic::refreshPgsForPort(const string &port, cons portInfo.state = PORT_READY; - // Remove the old profile which is probably not referenced anymore. + //Remove the old profile which is probably not referenced anymore. + //TODO release all profiles in to-be-removed map if (!profilesToBeReleased.empty()) { for (auto &oldProfile : profilesToBeReleased) @@ -1022,7 +967,7 @@ task_process_status BufferMgrDynamic::doUpdatePgTask(const string &pg_key, const // Not having profile_name but both speed and cable length have been configured for that port // This is because the first PG on that port is configured after speed, cable length configured // Just regenerate the profile - task_status = refreshPgsForPort(port, portInfo.speed, portInfo.cable_length, portInfo.mtu, pg_key); + task_status = refreshPriorityGroupsForPort(port, portInfo.speed, portInfo.cable_length, portInfo.mtu, pg_key); if (task_status != task_process_status::task_success) return task_status; @@ -1035,16 +980,12 @@ task_process_status BufferMgrDynamic::doUpdatePgTask(const string &pg_key, const } else { - task_status = refreshPgsForPort(port, portInfo.speed, portInfo.cable_length, portInfo.mtu, pg_key); + task_status = refreshPriorityGroupsForPort(port, portInfo.speed, portInfo.cable_length, portInfo.mtu, pg_key); if (task_status != task_process_status::task_success) return task_status; } break; - case PORT_ADMIN_DOWN: - SWSS_LOG_NOTICE("Skip setting BUFFER_PG for %s because the port is administratively down", port.c_str()); - break; - default: // speed and cable length hasn't been configured // In that case, we just skip the this update and return success. @@ -1056,7 +997,7 @@ task_process_status BufferMgrDynamic::doUpdatePgTask(const string &pg_key, const return task_process_status::task_success; } -// Remove the currently configured lossless pg +//Remove the currently configured lossless pg task_process_status BufferMgrDynamic::doRemovePgTask(const string &pg_key, const string &port) { auto &bufferPgs = m_portPgLookup[port]; @@ -1069,24 +1010,15 @@ task_process_status BufferMgrDynamic::doRemovePgTask(const string &pg_key, const SWSS_LOG_NOTICE("Remove BUFFER_PG %s (profile %s, %s)", pg_key.c_str(), bufferPg.running_profile_name.c_str(), bufferPg.configured_profile_name.c_str()); - // Recalculate pool size + // recalculate pool size checkSharedBufferPoolSize(); - if (portInfo.state != PORT_ADMIN_DOWN) - { - if (!portInfo.speed.empty() && !portInfo.cable_length.empty()) - portInfo.state = PORT_READY; - else - portInfo.state = PORT_INITIALIZING; - } - - // The bufferPg.running_profile_name can be empty if the port is admin down. - // In that case, releaseProfile should not be called - if (!bufferPg.running_profile_name.empty()) - { - SWSS_LOG_NOTICE("Try removing the original profile %s", bufferPg.running_profile_name.c_str()); - releaseProfile(bufferPg.running_profile_name); - } + if (!portInfo.speed.empty() && !portInfo.cable_length.empty()) + portInfo.state = PORT_READY; + else + portInfo.state = PORT_INITIALIZING; + SWSS_LOG_NOTICE("try removing the original profile %s", bufferPg.running_profile_name.c_str()); + releaseProfile(bufferPg.running_profile_name); return task_process_status::task_success; } @@ -1111,7 +1043,7 @@ 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); - rc = refreshPgsForPort(portName, port.speed, port.cable_length, port.mtu); + rc = refreshPriorityGroupsForPort(portName, port.speed, port.cable_length, port.mtu); if (task_process_status::task_success != rc) { SWSS_LOG_ERROR("Update the profile on %s failed", key.c_str()); @@ -1281,22 +1213,16 @@ task_process_status BufferMgrDynamic::handleCableLenTable(KeyOpFieldsValuesTuple SWSS_LOG_INFO("Updating BUFFER_PG for port %s due to cable length updated", port.c_str()); - // Try updating the buffer information + //Try updating the buffer information switch (portInfo.state) { case PORT_INITIALIZING: portInfo.state = PORT_READY; - task_status = refreshPgsForPort(port, speed, cable_length, mtu); + task_status = refreshPriorityGroupsForPort(port, speed, cable_length, mtu); break; case PORT_READY: - task_status = refreshPgsForPort(port, speed, cable_length, mtu); - break; - - case PORT_ADMIN_DOWN: - // Nothing to be done here - SWSS_LOG_INFO("Nothing to be done when port %s's cable length updated", port.c_str()); - task_status = task_process_status::task_success; + task_status = refreshPriorityGroupsForPort(port, speed, cable_length, mtu); break; } @@ -1341,9 +1267,9 @@ task_process_status BufferMgrDynamic::handlePortTable(KeyOpFieldsValuesTuple &tu { auto &port = kfvKey(tuple); string op = kfvOp(tuple); - bool speed_updated = false, mtu_updated = false, admin_status_updated = false, admin_up; + bool speed_updated = false, mtu_updated = false, admin_status_updated = false; - SWSS_LOG_DEBUG("Processing command:%s PORT table key %s", op.c_str(), port.c_str()); + SWSS_LOG_DEBUG("processing command:%s PORT table key %s", op.c_str(), port.c_str()); port_info_t &portInfo = m_portInfoLookup[port]; @@ -1355,30 +1281,21 @@ task_process_status BufferMgrDynamic::handlePortTable(KeyOpFieldsValuesTuple &tu if (op == SET_COMMAND) { - string old_speed; - string old_mtu; - for (auto i : kfvFieldsValues(tuple)) { - if (fvField(i) == "speed" && fvValue(i) != portInfo.speed) + if (fvField(i) == "speed") { speed_updated = true; - old_speed = move(portInfo.speed); portInfo.speed = fvValue(i); } - - if (fvField(i) == "mtu" && fvValue(i) != portInfo.mtu) + else if (fvField(i) == "mtu") { mtu_updated = true; - old_mtu = move(portInfo.mtu); portInfo.mtu = fvValue(i); } - - if (fvField(i) == "admin_status") + else if (fvField(i) == "admin_status") { - admin_up = (fvValue(i) == "up"); - auto old_admin_up = (portInfo.state != PORT_ADMIN_DOWN); - admin_status_updated = (admin_up != old_admin_up); + admin_status_updated = true; } } @@ -1386,100 +1303,46 @@ task_process_status BufferMgrDynamic::handlePortTable(KeyOpFieldsValuesTuple &tu string &mtu = portInfo.mtu; string &speed = portInfo.speed; - bool need_refresh_all_pgs = false, need_remove_all_pgs = false; - if (speed_updated || mtu_updated) { - if (!cable_length.empty() && !speed.empty()) + if (cable_length.empty() || speed.empty()) { - if (speed_updated) - { - if (mtu_updated) - { - SWSS_LOG_INFO("Updating BUFFER_PG for port %s due to speed updated from %s to %s and MTU updated from %s to %s", - port.c_str(), old_speed.c_str(), portInfo.speed.c_str(), old_mtu.c_str(), portInfo.mtu.c_str()); - } - else - { - SWSS_LOG_INFO("Updating BUFFER_PG for port %s due to speed updated from %s to %s", - port.c_str(), old_speed.c_str(), portInfo.speed.c_str()); - } - } - else - { - SWSS_LOG_INFO("Updating BUFFER_PG for port %s due to MTU updated from %s to %s", - port.c_str(), old_mtu.c_str(), portInfo.mtu.c_str()); - } - - // Try updating the buffer information - switch (portInfo.state) - { - case PORT_INITIALIZING: - portInfo.state = PORT_READY; - if (mtu.empty()) - { - // It's the same case as that in handleCableLenTable - mtu = DEFAULT_MTU_STR; - } - need_refresh_all_pgs = true; - break; - - case PORT_READY: - need_refresh_all_pgs = true; - break; - - case PORT_ADMIN_DOWN: - SWSS_LOG_INFO("Nothing to be done when port %s's speed or cable length updated since the port is administratively down", port.c_str()); - break; - - default: - SWSS_LOG_ERROR("Port %s: invalid port state %d when handling port update", port.c_str(), portInfo.state); - break; - } - - SWSS_LOG_DEBUG("Port Info for %s after handling speed %s cable %s gb %s", - port.c_str(), - portInfo.speed.c_str(), portInfo.cable_length.c_str(), portInfo.gearbox_model.c_str()); - } - else - { - SWSS_LOG_WARN("Cable length or speed for %s hasn't been configured yet, unable to calculate headroom", port.c_str()); - // We don't retry here because it doesn't make sense until both cable length and speed are configured. + // we still need to update pool size when port with headroom override is shut down + // even if its cable length or speed isn't configured + // so cable length and speed isn't tested for shutdown + SWSS_LOG_WARN("Cable length for %s hasn't been configured yet, unable to calculate headroom", port.c_str()); + // We don't retry here because it doesn't make sense until the cable length is configured. + return task_process_status::task_success; } - } - if (admin_status_updated) - { - if (admin_up) - { - if (!portInfo.speed.empty() && !portInfo.cable_length.empty()) - portInfo.state = PORT_READY; - else - portInfo.state = PORT_INITIALIZING; + SWSS_LOG_INFO("Updating BUFFER_PG for port %s due to speed or port updated", port.c_str()); - need_refresh_all_pgs = true; - } - else + //Try updating the buffer information + switch (portInfo.state) { - portInfo.state = PORT_ADMIN_DOWN; + case PORT_INITIALIZING: + portInfo.state = PORT_READY; + if (mtu.empty()) + { + // It's the same case as that in handleCableLenTable + mtu = DEFAULT_MTU_STR; + } + task_status = refreshPriorityGroupsForPort(port, speed, cable_length, mtu); + break; - need_remove_all_pgs = true; + case PORT_READY: + task_status = refreshPriorityGroupsForPort(port, speed, cable_length, mtu); + break; } - SWSS_LOG_INFO("Recalculate shared buffer pool size due to port %s's admin_status updated to %s", - port.c_str(), (admin_up ? "up" : "down")); - } - - // In case both need_remove_all_pgs and need_refresh_all_pgs are true, the need_remove_all_pgs will take effect. - // This can happen when both speed (or mtu) is changed and the admin_status is down. - // In this case, we just need record the new speed (or mtu) but don't need to refresh all PGs on the port since the port is administratively down - if (need_remove_all_pgs) - { - task_status = removeAllPgsFromPort(port); + SWSS_LOG_DEBUG("Port Info for %s after handling speed %s cable %s gb %s", + port.c_str(), + portInfo.speed.c_str(), portInfo.cable_length.c_str(), portInfo.gearbox_model.c_str()); } - else if (need_refresh_all_pgs) + else if (admin_status_updated) { - task_status = refreshPgsForPort(port, portInfo.speed, portInfo.cable_length, portInfo.mtu); + SWSS_LOG_INFO("Recalculate shared buffer pool size due to port %s's admin_status updated", port.c_str()); + checkSharedBufferPoolSize(); } } @@ -1493,7 +1356,7 @@ task_process_status BufferMgrDynamic::handleBufferPoolTable(KeyOpFieldsValuesTup string op = kfvOp(tuple); vector fvVector; - SWSS_LOG_DEBUG("Processing command:%s table BUFFER_POOL key %s", op.c_str(), pool.c_str()); + SWSS_LOG_DEBUG("processing command:%s table BUFFER_POOL key %s", op.c_str(), pool.c_str()); if (op == SET_COMMAND) { // For set command: @@ -1508,7 +1371,7 @@ task_process_status BufferMgrDynamic::handleBufferPoolTable(KeyOpFieldsValuesTup string &field = fvField(*i); string &value = fvValue(*i); - SWSS_LOG_DEBUG("Field:%s, value:%s", field.c_str(), value.c_str()); + SWSS_LOG_DEBUG("field:%s, value:%s", field.c_str(), value.c_str()); if (field == buffer_size_field_name) { bufferPool.dynamic_size = false; @@ -1592,12 +1455,12 @@ task_process_status BufferMgrDynamic::handleBufferProfileTable(KeyOpFieldsValues string op = kfvOp(tuple); vector fvVector; - SWSS_LOG_DEBUG("Processing command:%s BUFFER_PROFILE table key %s", op.c_str(), profileName.c_str()); + SWSS_LOG_DEBUG("processing command:%s BUFFER_PROFILE table key %s", op.c_str(), profileName.c_str()); if (op == SET_COMMAND) { - // For set command: - // 1. Create the corresponding table entries in APPL_DB - // 2. Record the table in the internal cache m_bufferProfileLookup + //For set command: + //1. Create the corresponding table entries in APPL_DB + //2. Record the table in the internal cache m_bufferProfileLookup buffer_profile_t &profileApp = m_bufferProfileLookup[profileName]; profileApp.static_configured = true; @@ -1609,10 +1472,10 @@ task_process_status BufferMgrDynamic::handleBufferProfileTable(KeyOpFieldsValues } for (auto i = kfvFieldsValues(tuple).begin(); i != kfvFieldsValues(tuple).end(); i++) { - const string &field = fvField(*i); - string value = fvValue(*i); + string &field = fvField(*i); + string &value = fvValue(*i); - SWSS_LOG_DEBUG("Field:%s, value:%s", field.c_str(), value.c_str()); + SWSS_LOG_DEBUG("field:%s, value:%s", field.c_str(), value.c_str()); if (field == buffer_pool_field_name) { if (!value.empty()) @@ -1762,7 +1625,7 @@ task_process_status BufferMgrDynamic::handleOneBufferPgEntry(const string &key, vector fvVector; buffer_pg_t &bufferPg = m_portPgLookup[port][key]; - SWSS_LOG_DEBUG("Processing command:%s table BUFFER_PG key %s", op.c_str(), key.c_str()); + SWSS_LOG_DEBUG("processing command:%s table BUFFER_PG key %s", op.c_str(), key.c_str()); if (op == SET_COMMAND) { bool ignored = false; @@ -1786,7 +1649,7 @@ task_process_status BufferMgrDynamic::handleOneBufferPgEntry(const string &key, const string &field = fvField(*i); string value = fvValue(*i); - SWSS_LOG_DEBUG("Field:%s, value:%s", field.c_str(), value.c_str()); + SWSS_LOG_DEBUG("field:%s, value:%s", field.c_str(), value.c_str()); if (field == buffer_profile_field_name && value != "NULL") { // Headroom override @@ -1829,13 +1692,6 @@ task_process_status BufferMgrDynamic::handleOneBufferPgEntry(const string &key, bufferPg.static_configured = true; bufferPg.configured_profile_name = profileName; } - - if (field != buffer_profile_field_name) - { - SWSS_LOG_ERROR("BUFFER_PG: Invalid field %s", field.c_str()); - return task_process_status::task_invalid_entry; - } - fvVector.emplace_back(field, value); SWSS_LOG_INFO("Inserting BUFFER_PG table field %s value %s", field.c_str(), value.c_str()); } @@ -1850,17 +1706,16 @@ task_process_status BufferMgrDynamic::handleOneBufferPgEntry(const string &key, if (!ignored && bufferPg.lossless) { doUpdatePgTask(key, port); + + if (!bufferPg.configured_profile_name.empty()) + { + m_bufferProfileLookup[bufferPg.configured_profile_name].port_pgs.insert(key); + } } else { SWSS_LOG_NOTICE("Inserting BUFFER_PG table entry %s into APPL_DB directly", key.c_str()); m_applBufferPgTable.set(key, fvVector); - bufferPg.running_profile_name = bufferPg.configured_profile_name; - } - - if (!bufferPg.configured_profile_name.empty()) - { - m_bufferProfileLookup[bufferPg.configured_profile_name].port_pgs.insert(key); } } else if (op == DEL_COMMAND) @@ -1868,16 +1723,12 @@ task_process_status BufferMgrDynamic::handleOneBufferPgEntry(const string &key, // For del command: // 1. Removing it from APPL_DB // 2. Update internal caches - string &runningProfileName = bufferPg.running_profile_name; - string &configProfileName = bufferPg.configured_profile_name; + string &profileName = bufferPg.running_profile_name; - if (!runningProfileName.empty()) - { - m_bufferProfileLookup[runningProfileName].port_pgs.erase(key); - } - if (!configProfileName.empty() && configProfileName != runningProfileName) + m_bufferProfileLookup[profileName].port_pgs.erase(key); + if (!bufferPg.configured_profile_name.empty()) { - m_bufferProfileLookup[configProfileName].port_pgs.erase(key); + m_bufferProfileLookup[bufferPg.configured_profile_name].port_pgs.erase(key); } if (bufferPg.lossless) @@ -1891,11 +1742,11 @@ task_process_status BufferMgrDynamic::handleOneBufferPgEntry(const string &key, } m_portPgLookup[port].erase(key); - SWSS_LOG_DEBUG("Profile %s has been removed from port %s PG %s", runningProfileName.c_str(), port.c_str(), key.c_str()); + SWSS_LOG_DEBUG("Profile %s has been removed from port %s PG %s", profileName.c_str(), port.c_str(), key.c_str()); if (m_portPgLookup[port].empty()) { m_portPgLookup.erase(port); - SWSS_LOG_DEBUG("Profile %s has been removed from port %s on all lossless PG", runningProfileName.c_str(), port.c_str()); + SWSS_LOG_DEBUG("Profile %s has been removed from port %s on all lossless PG", profileName.c_str(), port.c_str()); } } else @@ -1985,7 +1836,7 @@ task_process_status BufferMgrDynamic::doBufferTableTask(KeyOpFieldsValuesTuple & string key = kfvKey(tuple); const string &name = applTable.getTableName(); - // Transform the separator in key from "|" to ":" + //transform the separator in key from "|" to ":" transformSeperator(key); string op = kfvOp(tuple); @@ -1997,7 +1848,7 @@ task_process_status BufferMgrDynamic::doBufferTableTask(KeyOpFieldsValuesTuple & for (auto i : kfvFieldsValues(tuple)) { - // Transform the separator in values from "|" to ":" + //transform the separator in values from "|" to ":" if (fvField(i) == "pool") transformReference(fvValue(i)); if (fvField(i) == "profile") diff --git a/cfgmgr/buffermgrdyn.h b/cfgmgr/buffermgrdyn.h index a0ddcca7b9ad..a5ffe39b1e44 100644 --- a/cfgmgr/buffermgrdyn.h +++ b/cfgmgr/buffermgrdyn.h @@ -82,9 +82,7 @@ typedef enum { // Port is under initializing, which means its info hasn't been comprehensive for calculating headroom PORT_INITIALIZING, // All necessary information for calculating headroom is ready - PORT_READY, - // Port is admin down. All PGs programmed to APPL_DB should be removed from the port - PORT_ADMIN_DOWN + PORT_READY } port_state_t; typedef struct { @@ -236,8 +234,7 @@ class BufferMgrDynamic : public Orch void refreshSharedHeadroomPool(bool enable_state_updated_by_ratio, bool enable_state_updated_by_size); // Main flows - task_process_status removeAllPgsFromPort(const std::string &port); - task_process_status refreshPgsForPort(const std::string &port, const std::string &speed, const std::string &cable_length, const std::string &mtu, const std::string &exactly_matched_key); + task_process_status refreshPriorityGroupsForPort(const std::string &port, const std::string &speed, const std::string &cable_length, const std::string &mtu, const std::string &exactly_matched_key); task_process_status doUpdatePgTask(const std::string &pg_key, const std::string &port); task_process_status doRemovePgTask(const std::string &pg_key, const std::string &port); task_process_status doAdminStatusTask(const std::string port, const std::string adminStatus); diff --git a/tests/test_buffer_dynamic.py b/tests/test_buffer_dynamic.py index 61e6cbd6126c..932247de37d5 100644 --- a/tests/test_buffer_dynamic.py +++ b/tests/test_buffer_dynamic.py @@ -102,17 +102,10 @@ def setup_asic_db(self, dvs): self.ingress_lossless_pool_oid = key def check_new_profile_in_asic_db(self, dvs, profile): - retry_count = 0 - self.newProfileInAsicDb = None - while retry_count < 5: - retry_count += 1 - diff = set(self.asic_db.get_keys("ASIC_STATE:SAI_OBJECT_TYPE_BUFFER_PROFILE")) - self.initProfileSet - if len(diff) == 1: - self.newProfileInAsicDb = diff.pop() - break - else: - time.sleep(1) - assert self.newProfileInAsicDb, "Can't get SAI OID for newly created profile {} after retry {} times".format(profile, retry_count) + diff = set(self.asic_db.get_keys("ASIC_STATE:SAI_OBJECT_TYPE_BUFFER_PROFILE")) - self.initProfileSet + if len(diff) == 1: + self.newProfileInAsicDb = diff.pop() + assert self.newProfileInAsicDb, "Can't get SAI OID for newly created profile {}".format(profile) # in case diff is empty, we just treat the newProfileInAsicDb cached the latest one fvs = self.app_db.get_entry("BUFFER_PROFILE_TABLE", profile) @@ -148,10 +141,7 @@ def change_cable_length(self, cable_length): def test_changeSpeed(self, dvs, testlog): self.setup_db(dvs) - # Startup interface - dvs.runcmd('config interface startup Ethernet0') - - # Configure lossless PG 3-4 on interface + # configure lossless PG 3-4 on interface self.config_db.update_entry('BUFFER_PG', 'Ethernet0|3-4', {'profile': 'NULL'}) # Change speed to speed1 and verify whether the profile has been updated @@ -189,20 +179,14 @@ def test_changeSpeed(self, dvs, testlog): self.check_new_profile_in_asic_db(dvs, expectedProfile) self.app_db.wait_for_field_match("BUFFER_PG_TABLE", "Ethernet0:3-4", {"profile": "[BUFFER_PROFILE_TABLE:" + expectedProfile + "]"}) - # Remove lossless PG 3-4 on interface + # remove lossless PG 3-4 on interface self.config_db.delete_entry('BUFFER_PG', 'Ethernet0|3-4') self.app_db.wait_for_deleted_entry("BUFFER_PG_TABLE", "Ethernet0:3-4") - # Shutdown interface - dvs.runcmd('config interface shutdown Ethernet0') - def test_changeCableLen(self, dvs, testlog): self.setup_db(dvs) - # Startup interface - dvs.runcmd('config interface startup Ethernet0') - - # Configure lossless PG 3-4 on interface + # configure lossless PG 3-4 on interface self.config_db.update_entry('BUFFER_PG', 'Ethernet0|3-4', {'profile': 'NULL'}) # Change to new cable length @@ -240,19 +224,13 @@ def test_changeCableLen(self, dvs, testlog): self.app_db.wait_for_entry("BUFFER_PROFILE_TABLE", expectedProfile) self.app_db.wait_for_field_match("BUFFER_PG_TABLE", "Ethernet0:3-4", {"profile": "[BUFFER_PROFILE_TABLE:" + expectedProfile + "]"}) - # Remove lossless PG 3-4 on interface + # remove lossless PG 3-4 on interface self.config_db.delete_entry('BUFFER_PG', 'Ethernet0|3-4') - # Shutdown interface - dvs.runcmd('config interface shutdown Ethernet0') - def test_MultipleLosslessPg(self, dvs, testlog): self.setup_db(dvs) - # Startup interface - dvs.runcmd('config interface startup Ethernet0') - - # Configure lossless PG 3-4 on interface + # configure lossless PG 3-4 on interface self.config_db.update_entry('BUFFER_PG', 'Ethernet0|3-4', {'profile': 'NULL'}) # Add another lossless PG @@ -260,14 +238,14 @@ def test_MultipleLosslessPg(self, dvs, testlog): expectedProfile = self.make_lossless_profile_name(self.originalSpeed, self.originalCableLen) self.app_db.wait_for_field_match("BUFFER_PG_TABLE", "Ethernet0:6", {"profile": "[BUFFER_PROFILE_TABLE:" + expectedProfile + "]"}) - # Change speed and check + # change speed and check dvs.runcmd("config interface speed Ethernet0 " + self.speedToTest1) expectedProfile = self.make_lossless_profile_name(self.speedToTest1, self.originalCableLen) self.app_db.wait_for_entry("BUFFER_PROFILE_TABLE", expectedProfile) self.app_db.wait_for_field_match("BUFFER_PG_TABLE", "Ethernet0:3-4", {"profile": "[BUFFER_PROFILE_TABLE:" + expectedProfile + "]"}) self.app_db.wait_for_field_match("BUFFER_PG_TABLE", "Ethernet0:6", {"profile": "[BUFFER_PROFILE_TABLE:" + expectedProfile + "]"}) - # Change cable length and check + # change cable length and check self.change_cable_length(self.cableLenTest1) self.app_db.wait_for_deleted_entry("BUFFER_PROFILE_TABLE", expectedProfile) expectedProfile = self.make_lossless_profile_name(self.speedToTest1, self.cableLenTest1) @@ -276,7 +254,7 @@ def test_MultipleLosslessPg(self, dvs, testlog): self.app_db.wait_for_field_match("BUFFER_PG_TABLE", "Ethernet0:3-4", {"profile": "[BUFFER_PROFILE_TABLE:" + expectedProfile + "]"}) self.app_db.wait_for_field_match("BUFFER_PG_TABLE", "Ethernet0:6", {"profile": "[BUFFER_PROFILE_TABLE:" + expectedProfile + "]"}) - # Revert the speed and cable length and check + # revert the speed and cable length and check self.change_cable_length(self.originalCableLen) dvs.runcmd("config interface speed Ethernet0 " + self.originalSpeed) self.app_db.wait_for_deleted_entry("BUFFER_PROFILE_TABLE", expectedProfile) @@ -286,19 +264,13 @@ def test_MultipleLosslessPg(self, dvs, testlog): self.app_db.wait_for_field_match("BUFFER_PG_TABLE", "Ethernet0:3-4", {"profile": "[BUFFER_PROFILE_TABLE:" + expectedProfile + "]"}) self.app_db.wait_for_field_match("BUFFER_PG_TABLE", "Ethernet0:6", {"profile": "[BUFFER_PROFILE_TABLE:" + expectedProfile + "]"}) - # Remove lossless PG 3-4 and 6 on interface + # remove lossless PG 3-4 and 6 on interface self.config_db.delete_entry('BUFFER_PG', 'Ethernet0|3-4') self.config_db.delete_entry('BUFFER_PG', 'Ethernet0|6') - # Shutdown interface - dvs.runcmd('config interface shutdown Ethernet0') - def test_headroomOverride(self, dvs, testlog): self.setup_db(dvs) - # Startup interface - dvs.runcmd('config interface startup Ethernet0') - # Configure static profile self.config_db.update_entry('BUFFER_PROFILE', 'test', {'xon': '18432', @@ -356,7 +328,7 @@ def test_headroomOverride(self, dvs, testlog): self.app_db.wait_for_deleted_entry("BUFFER_PG_TABLE", "Ethernet0:6") # readd lossless PG with dynamic profile - self.config_db.update_entry('BUFFER_PG', 'Ethernet0|3-4', {'profile': 'NULL'}) + self.config_db.update_entry('BUFFER_PG', 'Ethernet0|3-4', {'profie': 'NULL'}) self.app_db.wait_for_field_match("BUFFER_PG_TABLE", "Ethernet0:3-4", {"profile": "[BUFFER_PROFILE_TABLE:" + expectedProfile + "]"}) # remove the headroom override profile @@ -373,15 +345,9 @@ def test_headroomOverride(self, dvs, testlog): # remove lossless PG 3-4 on interface self.config_db.delete_entry('BUFFER_PG', 'Ethernet0|3-4') - # Shutdown interface - dvs.runcmd('config interface shutdown Ethernet0') - def test_mtuUpdate(self, dvs, testlog): self.setup_db(dvs) - # Startup interface - dvs.runcmd('config interface startup Ethernet0') - test_mtu = '1500' default_mtu = '9100' expectedProfileMtu = self.make_lossless_profile_name(self.originalSpeed, self.originalCableLen, mtu = test_mtu) @@ -407,15 +373,9 @@ def test_mtuUpdate(self, dvs, testlog): # clear configuration self.config_db.delete_entry('BUFFER_PG', 'Ethernet0|3-4') - # Shutdown interface - dvs.runcmd('config interface shutdown Ethernet0') - def test_nonDefaultAlpha(self, dvs, testlog): self.setup_db(dvs) - # Startup interface - dvs.runcmd('config interface startup Ethernet0') - test_dynamic_th_1 = '1' expectedProfile_th1 = self.make_lossless_profile_name(self.originalSpeed, self.originalCableLen, dynamic_th = test_dynamic_th_1) test_dynamic_th_2 = '2' @@ -449,17 +409,12 @@ def test_nonDefaultAlpha(self, dvs, testlog): self.config_db.delete_entry('BUFFER_PG', 'Ethernet0|3-4') self.config_db.delete_entry('BUFFER_PROFILE', 'non-default-dynamic') - # Shutdown interface - dvs.runcmd('config interface shutdown Ethernet0') - def test_sharedHeadroomPool(self, dvs, testlog): self.setup_db(dvs) - # Startup interface - dvs.runcmd('config interface startup Ethernet0') - # configure lossless PG 3-4 on interface and start up the interface self.config_db.update_entry('BUFFER_PG', 'Ethernet0|3-4', {'profile': 'NULL'}) + dvs.runcmd('config interface startup Ethernet0') expectedProfile = self.make_lossless_profile_name(self.originalSpeed, self.originalCableLen) self.app_db.wait_for_entry("BUFFER_PROFILE_TABLE", expectedProfile) @@ -545,37 +500,3 @@ def test_sharedHeadroomPool(self, dvs, testlog): # remove lossless PG 3-4 on interface self.config_db.delete_entry('BUFFER_PG', 'Ethernet0|3-4') dvs.runcmd('config interface shutdown Ethernet0') - - # Shutdown interface - dvs.runcmd('config interface shutdown Ethernet0') - - def test_shutdownPort(self, dvs, testlog): - self.setup_db(dvs) - - # Startup interface - dvs.runcmd('config interface startup Ethernet0') - - # Configure lossless PG 3-4 on interface - self.config_db.update_entry('BUFFER_PG', 'Ethernet0|3-4', {'profile': 'NULL'}) - expectedProfile = self.make_lossless_profile_name(self.originalSpeed, self.originalCableLen) - self.app_db.wait_for_field_match("BUFFER_PG_TABLE", "Ethernet0:3-4", {"profile": "[BUFFER_PROFILE_TABLE:" + expectedProfile + "]"}) - - # Shutdown port and check whether all the PGs have been removed - dvs.runcmd("config interface shutdown Ethernet0") - self.app_db.wait_for_deleted_entry("BUFFER_PG_TABLE", "Ethernet0:3-4") - self.app_db.wait_for_deleted_entry("BUFFER_PROFILE", expectedProfile) - - # Add another PG when port is administratively down - self.config_db.update_entry('BUFFER_PG', 'Ethernet0|6', {'profile': 'NULL'}) - - # Startup port and check whether all the PGs haved been added - dvs.runcmd("config interface startup Ethernet0") - self.app_db.wait_for_field_match("BUFFER_PG_TABLE", "Ethernet0:3-4", {"profile": "[BUFFER_PROFILE_TABLE:" + expectedProfile + "]"}) - self.app_db.wait_for_field_match("BUFFER_PG_TABLE", "Ethernet0:6", {"profile": "[BUFFER_PROFILE_TABLE:" + expectedProfile + "]"}) - - # Remove lossless PG 3-4 on interface - self.config_db.delete_entry('BUFFER_PG', 'Ethernet0|3-4') - self.config_db.delete_entry('BUFFER_PG', 'Ethernet0|6') - - # Shutdown interface - dvs.runcmd("config interface shutdown Ethernet0")