From 4ae67e090996d8ade62914c397e4e1e188264243 Mon Sep 17 00:00:00 2001 From: bingwang Date: Tue, 7 Jun 2022 09:04:32 +0000 Subject: [PATCH 1/5] Apply DSCP_TO_TC_MAP|AZURE to switch level Signed-off-by: bingwang --- orchagent/qosorch.cpp | 14 +++++++++-- orchagent/qosorch.h | 2 +- tests/test_qos_map.py | 55 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 3 deletions(-) diff --git a/orchagent/qosorch.cpp b/orchagent/qosorch.cpp index 5cb9d8ad2e..d834763241 100644 --- a/orchagent/qosorch.cpp +++ b/orchagent/qosorch.cpp @@ -104,6 +104,8 @@ map qos_to_ref_table_map = { #define DSCP_MAX_VAL 63 #define EXP_MAX_VAL 7 +#define DEFAULT_DSCP_TO_TC_MAP_NAME "AZURE" + task_process_status QosMapHandler::processWorkItem(Consumer& consumer, KeyOpFieldsValuesTuple &tuple) { SWSS_LOG_ENTER(); @@ -148,6 +150,16 @@ task_process_status QosMapHandler::processWorkItem(Consumer& consumer, KeyOpFiel freeAttribResources(attributes); return task_process_status::task_failed; } + if ((qos_map_type_name == CFG_DSCP_TO_TC_MAP_TABLE_NAME) && (qos_object_name == DEFAULT_DSCP_TO_TC_MAP_NAME)) + { + DscpToTcMapHandler *handler = dynamic_cast(this); + if (handler) + { + handler->applyDscpToTcMapToSwitch(SAI_SWITCH_ATTR_QOS_DSCP_TO_TC_MAP, sai_object); + SWSS_LOG_NOTICE("Applied DSCP_TO_TC_MAP %s to switch level", DEFAULT_DSCP_TO_TC_MAP_NAME); + } + } + (*(QosOrch::getTypeMap()[qos_map_type_name]))[qos_object_name].m_saiObjectId = sai_object; (*(QosOrch::getTypeMap()[qos_map_type_name]))[qos_object_name].m_pendingRemove = false; SWSS_LOG_NOTICE("Created [%s:%s]", qos_map_type_name.c_str(), qos_object_name.c_str()); @@ -292,8 +304,6 @@ sai_object_id_t DscpToTcMapHandler::addQosItem(const vector &at } SWSS_LOG_DEBUG("created QosMap object:%" PRIx64, sai_object); - applyDscpToTcMapToSwitch(SAI_SWITCH_ATTR_QOS_DSCP_TO_TC_MAP, sai_object); - return sai_object; } diff --git a/orchagent/qosorch.h b/orchagent/qosorch.h index 2b348cc030..c670b5016b 100644 --- a/orchagent/qosorch.h +++ b/orchagent/qosorch.h @@ -78,7 +78,7 @@ class DscpToTcMapHandler : public QosMapHandler bool convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &tuple, vector &attributes) override; sai_object_id_t addQosItem(const vector &attributes) override; bool removeQosItem(sai_object_id_t sai_object); -protected: + void applyDscpToTcMapToSwitch(sai_attr_id_t attr_id, sai_object_id_t sai_dscp_to_tc_map); }; diff --git a/tests/test_qos_map.py b/tests/test_qos_map.py index 301bd3c6d6..95cc53d0d3 100644 --- a/tests/test_qos_map.py +++ b/tests/test_qos_map.py @@ -370,6 +370,61 @@ def test_port_mpls_tc(self, dvs): port_cnt = len(swsscommon.Table(self.config_db, CFG_PORT_TABLE_NAME).getKeys()) assert port_cnt == cnt +class TestDscpToTcMap(object): + ASIC_QOS_MAP_STR = "ASIC_STATE:SAI_OBJECT_TYPE_QOS_MAP" + ASIC_PORT_STR = "ASIC_STATE:SAI_OBJECT_TYPE_PORT" + ASIC_SWITCH_STR = "ASIC_STATE:SAI_OBJECT_TYPE_SWITCH" + + def init_test(self, dvs): + dvs.setup_db() + self.asic_db = dvs.get_asic_db() + self.config_db = dvs.get_config_db() + self.asic_qos_map_ids = self.asic_db.get_keys(self.ASIC_QOS_MAP_STR) + self.asic_qos_map_count = len(self.asic_qos_map_ids) + self.dscp_to_tc_table = swsscommon.Table(self.config_db.db_connection, swsscommon.CFG_DSCP_TO_TC_MAP_TABLE_NAME) + + def get_qos_id(self): + diff = set(self.asic_db.get_keys(self.ASIC_QOS_MAP_STR)) - set(self.asic_qos_map_ids) + assert len(diff) <= 1 + return None if len(diff) == 0 else diff.pop() + + def test_dscp_to_tc_map_applied_to_switch(self, dvs): + self.init_test(dvs) + dscp_to_tc_map_id = None + created_new_map = False + try: + existing_map = self.dscp_to_tc_table.getKeys() + if "AZURE" not in existing_map: + # Create a DSCP_TO_TC map + dscp_to_tc_map = [(str(i), str(i)) for i in range(0, 63)] + self.dscp_to_tc_table.set("AZURE", swsscommon.FieldValuePairs(dscp_to_tc_map)) + + self.asic_db.wait_for_n_keys(self.ASIC_QOS_MAP_STR, self.asic_qos_map_count + 1) + + # Get the DSCP_TO_TC map ID + dscp_to_tc_map_id = self.get_qos_id() + assert(dscp_to_tc_map_id is not None) + + # Assert the expected values + fvs = self.asic_db.get_entry(self.ASIC_QOS_MAP_STR, dscp_to_tc_map_id) + assert(fvs.get("SAI_QOS_MAP_ATTR_TYPE") == "SAI_QOS_MAP_TYPE_DSCP_TO_TC") + created_new_map = True + else: + for id in self.asic_qos_map_ids: + fvs = self.asic_db.get_entry(self.ASIC_QOS_MAP_STR, id) + if fvs.get("SAI_QOS_MAP_ATTR_TYPE") == "SAI_QOS_MAP_TYPE_DSCP_TO_TC": + dscp_to_tc_map_id = id + break + + # Check the switch level DSCP_TO_TC_MAP is applied + switch_oid = dvs.getSwitchOid() + fvs = self.asic_db.get_entry(self.ASIC_SWITCH_STR, switch_oid) + assert(fvs.get("SAI_SWITCH_ATTR_QOS_DSCP_TO_TC_MAP") == dscp_to_tc_map_id) + + finally: + if created_new_map: + self.dscp_to_tc_table._del("AZURE") + # Add Dummy always-pass test at end as workaroud # for issue when Flaky fail on final test it invokes module tear-down before retrying From 012350b27e7b8df37cb47244179b4bf4622ba580 Mon Sep 17 00:00:00 2001 From: bingwang Date: Wed, 8 Jun 2022 01:12:54 +0000 Subject: [PATCH 2/5] Fix UT Signed-off-by: bingwang --- tests/mock_tests/qosorch_ut.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/mock_tests/qosorch_ut.cpp b/tests/mock_tests/qosorch_ut.cpp index a2152f8d7c..3813aa9d02 100644 --- a/tests/mock_tests/qosorch_ut.cpp +++ b/tests/mock_tests/qosorch_ut.cpp @@ -947,7 +947,8 @@ namespace qosorch_test entries.clear(); // Drain DSCP_TO_TC_MAP table static_cast(gQosOrch)->doTask(); - ASSERT_EQ((*QosOrch::getTypeMap()[CFG_DSCP_TO_TC_MAP_TABLE_NAME])["AZURE_1"].m_saiObjectId, switch_dscp_to_tc_map_id); + // As we hardcode the default map name to AZURE, pushing AZURE_1 makes no change + ASSERT_EQ((*QosOrch::getTypeMap()[CFG_DSCP_TO_TC_MAP_TABLE_NAME])["AZURE"].m_saiObjectId, switch_dscp_to_tc_map_id); entries.push_back({"AZURE_1", "DEL", {}}); consumer->addToSync(entries); From d4a6607a5b14373286dc907ecf8c6a03f42c3202 Mon Sep 17 00:00:00 2001 From: bingwang Date: Thu, 9 Jun 2022 05:46:26 +0000 Subject: [PATCH 3/5] Read global entry Signed-off-by: bingwang --- cfgmgr/buffermgr.cpp | 7 ++ orchagent/qosorch.cpp | 141 +++++++++++++++++++------------- orchagent/qosorch.h | 6 +- tests/mock_tests/qosorch_ut.cpp | 27 ++++-- tests/test_qos_map.py | 10 ++- 5 files changed, 119 insertions(+), 72 deletions(-) diff --git a/cfgmgr/buffermgr.cpp b/cfgmgr/buffermgr.cpp index 5c7d6ae9e6..be8a661c87 100644 --- a/cfgmgr/buffermgr.cpp +++ b/cfgmgr/buffermgr.cpp @@ -15,6 +15,8 @@ using namespace std; using namespace swss; +#define PORT_NAME_GLOBAL "global" + BufferMgr::BufferMgr(DBConnector *cfgDb, DBConnector *applDb, string pg_lookup_file, const vector &tableNames) : Orch(cfgDb, tableNames), m_cfgPortTable(cfgDb, CFG_PORT_TABLE_NAME), @@ -391,6 +393,11 @@ void BufferMgr::doPortQosTableTask(Consumer &consumer) { KeyOpFieldsValuesTuple tuple = it->second; string port_name = kfvKey(tuple); + if (port_name == PORT_NAME_GLOBAL) + { + // Ignore the entry for global level + continue; + } string op = kfvOp(tuple); if (op == SET_COMMAND) { diff --git a/orchagent/qosorch.cpp b/orchagent/qosorch.cpp index d834763241..45f89b7942 100644 --- a/orchagent/qosorch.cpp +++ b/orchagent/qosorch.cpp @@ -104,7 +104,7 @@ map qos_to_ref_table_map = { #define DSCP_MAX_VAL 63 #define EXP_MAX_VAL 7 -#define DEFAULT_DSCP_TO_TC_MAP_NAME "AZURE" +#define PORT_NAME_GLOBAL "global" task_process_status QosMapHandler::processWorkItem(Consumer& consumer, KeyOpFieldsValuesTuple &tuple) { @@ -150,16 +150,6 @@ task_process_status QosMapHandler::processWorkItem(Consumer& consumer, KeyOpFiel freeAttribResources(attributes); return task_process_status::task_failed; } - if ((qos_map_type_name == CFG_DSCP_TO_TC_MAP_TABLE_NAME) && (qos_object_name == DEFAULT_DSCP_TO_TC_MAP_NAME)) - { - DscpToTcMapHandler *handler = dynamic_cast(this); - if (handler) - { - handler->applyDscpToTcMapToSwitch(SAI_SWITCH_ATTR_QOS_DSCP_TO_TC_MAP, sai_object); - SWSS_LOG_NOTICE("Applied DSCP_TO_TC_MAP %s to switch level", DEFAULT_DSCP_TO_TC_MAP_NAME); - } - } - (*(QosOrch::getTypeMap()[qos_map_type_name]))[qos_object_name].m_saiObjectId = sai_object; (*(QosOrch::getTypeMap()[qos_map_type_name]))[qos_object_name].m_pendingRemove = false; SWSS_LOG_NOTICE("Created [%s:%s]", qos_map_type_name.c_str(), qos_object_name.c_str()); @@ -248,37 +238,6 @@ bool DscpToTcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple & return true; } -void DscpToTcMapHandler::applyDscpToTcMapToSwitch(sai_attr_id_t attr_id, sai_object_id_t map_id) -{ - SWSS_LOG_ENTER(); - bool rv = true; - - /* Query DSCP_TO_TC QoS map at switch capability */ - rv = gSwitchOrch->querySwitchDscpToTcCapability(SAI_OBJECT_TYPE_SWITCH, SAI_SWITCH_ATTR_QOS_DSCP_TO_TC_MAP); - if (rv == false) - { - SWSS_LOG_ERROR("Switch level DSCP to TC QoS map configuration is not supported"); - return; - } - - /* Apply DSCP_TO_TC QoS map at switch */ - sai_attribute_t attr; - attr.id = attr_id; - attr.value.oid = map_id; - - sai_status_t status = sai_switch_api->set_switch_attribute(gSwitchId, &attr); - if (status != SAI_STATUS_SUCCESS) - { - SWSS_LOG_ERROR("Failed to apply DSCP_TO_TC QoS map to switch rv:%d", status); - return; - } - - if (map_id != gQosOrch->m_globalDscpToTcMap) - gQosOrch->m_globalDscpToTcMap = map_id; - - SWSS_LOG_NOTICE("Applied DSCP_TO_TC QoS map to switch successfully"); -} - sai_object_id_t DscpToTcMapHandler::addQosItem(const vector &attributes) { SWSS_LOG_ENTER(); @@ -313,23 +272,9 @@ bool DscpToTcMapHandler::removeQosItem(sai_object_id_t sai_object) if (sai_object == gQosOrch->m_globalDscpToTcMap) { - // The current global dscp to tc map is about to be removed. - // Find another one to set to the switch or NULL in case this is the last one - const auto &dscpToTcObjects = (*QosOrch::getTypeMap()[CFG_DSCP_TO_TC_MAP_TABLE_NAME]); - bool found = false; - for (const auto &ref : dscpToTcObjects) - { - if (ref.second.m_saiObjectId == sai_object) - continue; - SWSS_LOG_NOTICE("Current global dscp_to_tc map is about to be removed, set it to %s %" PRIx64, ref.first.c_str(), ref.second.m_saiObjectId); - applyDscpToTcMapToSwitch(SAI_SWITCH_ATTR_QOS_DSCP_TO_TC_MAP, ref.second.m_saiObjectId); - found = true; - break; - } - if (!found) - { - applyDscpToTcMapToSwitch(SAI_SWITCH_ATTR_QOS_DSCP_TO_TC_MAP, SAI_NULL_OBJECT_ID); - } + // The current global dscp to tc map is still referenced + SWSS_LOG_NOTICE("Current global dscp_to_tc map is still being referenced %" PRIx64, sai_object); + return false; } SWSS_LOG_DEBUG("Removing DscpToTcMap object:%" PRIx64, sai_object); @@ -1727,12 +1672,90 @@ task_process_status QosOrch::handleQueueTable(Consumer& consumer, KeyOpFieldsVal return task_process_status::task_success; } +bool QosOrch::applyDscpToTcMapToSwitch(sai_attr_id_t attr_id, sai_object_id_t map_id) +{ + SWSS_LOG_ENTER(); + + /* Query DSCP_TO_TC QoS map at switch capability */ + bool rv = gSwitchOrch->querySwitchDscpToTcCapability(SAI_OBJECT_TYPE_SWITCH, SAI_SWITCH_ATTR_QOS_DSCP_TO_TC_MAP); + if (rv == false) + { + SWSS_LOG_ERROR("Switch level DSCP to TC QoS map configuration is not supported"); + return true; + } + + /* Apply DSCP_TO_TC QoS map at switch */ + sai_attribute_t attr; + attr.id = attr_id; + attr.value.oid = map_id; + + sai_status_t status = sai_switch_api->set_switch_attribute(gSwitchId, &attr); + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to apply DSCP_TO_TC QoS map to switch rv:%d", status); + return false; + } + + if (map_id != gQosOrch->m_globalDscpToTcMap) + gQosOrch->m_globalDscpToTcMap = map_id; + + SWSS_LOG_NOTICE("Applied DSCP_TO_TC QoS map to switch successfully"); + return true; +} + +task_process_status QosOrch::handleGlobalQosMap(const string &OP, KeyOpFieldsValuesTuple &tuple) +{ + task_process_status task_status = task_process_status::task_success; + if (OP == DEL_COMMAND) + { + // Ignore the DEL operation for switch level mapping + SWSS_LOG_NOTICE("Ignore DEL operation for global qos map"); + return task_status; + } + + for (auto it = kfvFieldsValues(tuple).begin(); it != kfvFieldsValues(tuple).end(); it++) + { + string map_type_name = fvField(*it); + string map_name = fvValue(*it); + if (map_type_name != dscp_to_tc_field_name) + { + SWSS_LOG_WARN("Qos map %s is not supported at global level", map_type_name.c_str()); + continue; + } + if (qos_to_attr_map.find(map_type_name) != qos_to_attr_map.end()) + { + sai_object_id_t id; + string object_name; + ref_resolve_status status = resolveFieldRefValue(m_qos_maps, map_type_name, qos_to_ref_table_map.at(map_type_name), tuple, id, object_name); + + if (status != ref_resolve_status::success) + { + SWSS_LOG_INFO("Global QoS map %s is not yet created", map_name.c_str()); + task_status = task_process_status::task_need_retry; + } + + if (applyDscpToTcMapToSwitch(SAI_SWITCH_ATTR_QOS_DSCP_TO_TC_MAP, id)) + { + task_status = task_process_status::task_success; + } + setObjectReference(m_qos_maps, CFG_PORT_QOS_MAP_TABLE_NAME, PORT_NAME_GLOBAL, map_type_name, object_name); + } + } + return task_status; +} + task_process_status QosOrch::handlePortQosMapTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple) { SWSS_LOG_ENTER(); string key = kfvKey(tuple); string op = kfvOp(tuple); + + if (key == PORT_NAME_GLOBAL) + { + return handleGlobalQosMap(op, tuple); + } + vector port_names = tokenize(key, list_item_delimiter); if (op == DEL_COMMAND) diff --git a/orchagent/qosorch.h b/orchagent/qosorch.h index c670b5016b..5f80359296 100644 --- a/orchagent/qosorch.h +++ b/orchagent/qosorch.h @@ -78,8 +78,6 @@ class DscpToTcMapHandler : public QosMapHandler bool convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &tuple, vector &attributes) override; sai_object_id_t addQosItem(const vector &attributes) override; bool removeQosItem(sai_object_id_t sai_object); - - void applyDscpToTcMapToSwitch(sai_attr_id_t attr_id, sai_object_id_t sai_dscp_to_tc_map); }; class MplsTcToTcMapHandler : public QosMapHandler @@ -195,11 +193,13 @@ class QosOrch : public Orch task_process_status handleExpToFcTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple); task_process_status handleTcToDscpTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple); + task_process_status handleGlobalQosMap(const string &op, KeyOpFieldsValuesTuple &tuple); + sai_object_id_t getSchedulerGroup(const Port &port, const sai_object_id_t queue_id); bool applySchedulerToQueueSchedulerGroup(Port &port, size_t queue_ind, sai_object_id_t scheduler_profile_id); bool applyWredProfileToQueue(Port &port, size_t queue_ind, sai_object_id_t sai_wred_profile); - + bool applyDscpToTcMapToSwitch(sai_attr_id_t attr_id, sai_object_id_t sai_dscp_to_tc_map); private: qos_table_handler_map m_qos_handler_map; diff --git a/tests/mock_tests/qosorch_ut.cpp b/tests/mock_tests/qosorch_ut.cpp index 3813aa9d02..9cf35d68c9 100644 --- a/tests/mock_tests/qosorch_ut.cpp +++ b/tests/mock_tests/qosorch_ut.cpp @@ -723,8 +723,6 @@ namespace qosorch_test static_cast(gQosOrch)->doTask(); ASSERT_EQ(++current_sai_remove_qos_map_count, sai_remove_qos_map_count); ASSERT_EQ((*QosOrch::getTypeMap()[CFG_DSCP_TO_TC_MAP_TABLE_NAME]).count("AZURE"), 0); - // Global dscp to tc map should not be cleared - ASSERT_EQ((*QosOrch::getTypeMap()[CFG_DSCP_TO_TC_MAP_TABLE_NAME])["AZURE_1"].m_saiObjectId, switch_dscp_to_tc_map_id); // Make sure other dependencies are not touched CheckDependency(CFG_PORT_QOS_MAP_TABLE_NAME, "Ethernet0", "pfc_to_pg_map", CFG_PFC_PRIORITY_TO_PRIORITY_GROUP_MAP_TABLE_NAME, "AZURE"); @@ -931,12 +929,9 @@ namespace qosorch_test TEST_F(QosOrchTest, QosOrchTestGlobalDscpToTcMap) { - // Make sure dscp to tc map is correct - ASSERT_EQ((*QosOrch::getTypeMap()[CFG_DSCP_TO_TC_MAP_TABLE_NAME])["AZURE"].m_saiObjectId, switch_dscp_to_tc_map_id); - // Create a new dscp to tc map std::deque entries; - entries.push_back({"AZURE_1", "SET", + entries.push_back({"AZURE", "SET", { {"1", "0"}, {"0", "1"} @@ -945,16 +940,30 @@ namespace qosorch_test auto consumer = dynamic_cast(gQosOrch->getExecutor(CFG_DSCP_TO_TC_MAP_TABLE_NAME)); consumer->addToSync(entries); entries.clear(); - // Drain DSCP_TO_TC_MAP table + + entries.push_back({"global", "SET", + { + {"dscp_to_tc_map", "AZURE"} + }}); + consumer = dynamic_cast(gQosOrch->getExecutor(CFG_PORT_QOS_MAP_TABLE_NAME)); + consumer->addToSync(entries); + entries.clear(); + + // Drain DSCP_TO_TC_MAP and PORT_QOS_MAP table static_cast(gQosOrch)->doTask(); - // As we hardcode the default map name to AZURE, pushing AZURE_1 makes no change + // Check DSCP_TO_TC_MAP|AZURE is applied to switch ASSERT_EQ((*QosOrch::getTypeMap()[CFG_DSCP_TO_TC_MAP_TABLE_NAME])["AZURE"].m_saiObjectId, switch_dscp_to_tc_map_id); - entries.push_back({"AZURE_1", "DEL", {}}); + entries.push_back({"AZURE", "DEL", {}}); + consumer = dynamic_cast(gQosOrch->getExecutor(CFG_DSCP_TO_TC_MAP_TABLE_NAME)); consumer->addToSync(entries); entries.clear(); + auto current_sai_remove_qos_map_count = sai_remove_qos_map_count; // Drain DSCP_TO_TC_MAP table static_cast(gQosOrch)->doTask(); + // Check DSCP_TO_TC_MAP|AZURE is not removed because it's still referenced by switch + ASSERT_EQ(current_sai_remove_qos_map_count, sai_remove_qos_map_count); + ASSERT_EQ((*QosOrch::getTypeMap()[CFG_DSCP_TO_TC_MAP_TABLE_NAME]).count("AZURE"), 1); ASSERT_EQ((*QosOrch::getTypeMap()[CFG_DSCP_TO_TC_MAP_TABLE_NAME])["AZURE"].m_saiObjectId, switch_dscp_to_tc_map_id); } diff --git a/tests/test_qos_map.py b/tests/test_qos_map.py index 95cc53d0d3..1493f59e6b 100644 --- a/tests/test_qos_map.py +++ b/tests/test_qos_map.py @@ -382,6 +382,7 @@ def init_test(self, dvs): self.asic_qos_map_ids = self.asic_db.get_keys(self.ASIC_QOS_MAP_STR) self.asic_qos_map_count = len(self.asic_qos_map_ids) self.dscp_to_tc_table = swsscommon.Table(self.config_db.db_connection, swsscommon.CFG_DSCP_TO_TC_MAP_TABLE_NAME) + self.port_qos_table = swsscommon.Table(self.config_db.db_connection, swsscommon.CFG_PORT_QOS_MAP_TABLE_NAME) def get_qos_id(self): diff = set(self.asic_db.get_keys(self.ASIC_QOS_MAP_STR)) - set(self.asic_qos_map_ids) @@ -415,9 +416,16 @@ def test_dscp_to_tc_map_applied_to_switch(self, dvs): if fvs.get("SAI_QOS_MAP_ATTR_TYPE") == "SAI_QOS_MAP_TYPE_DSCP_TO_TC": dscp_to_tc_map_id = id break + switch_oid = dvs.getSwitchOid() + # Check switch level DSCP_TO_TC_MAP doesn't before PORT_QOS_MAP|global is created + fvs = self.asic_db.get_entry(self.ASIC_SWITCH_STR, switch_oid) + assert("SAI_SWITCH_ATTR_QOS_DSCP_TO_TC_MAP" not in fvs) + + # Insert switch level map entry + self.port_qos_table.set("global", [("dscp_to_tc_map", "AZURE")]) + time.sleep(1) # Check the switch level DSCP_TO_TC_MAP is applied - switch_oid = dvs.getSwitchOid() fvs = self.asic_db.get_entry(self.ASIC_SWITCH_STR, switch_oid) assert(fvs.get("SAI_SWITCH_ATTR_QOS_DSCP_TO_TC_MAP") == dscp_to_tc_map_id) From 4aa431aa73ee58ab5f4d6e0ad9af4d3719290ce0 Mon Sep 17 00:00:00 2001 From: bingwang Date: Mon, 13 Jun 2022 04:28:59 +0000 Subject: [PATCH 4/5] Address comment Signed-off-by: bingwang --- cfgmgr/buffermgr.cpp | 3 ++- orchagent/qosorch.cpp | 35 ++++++++++++++++++++++++++++----- tests/mock_tests/qosorch_ut.cpp | 19 ++++++++++++++---- tests/test_qos_map.py | 7 +++++++ 4 files changed, 54 insertions(+), 10 deletions(-) diff --git a/cfgmgr/buffermgr.cpp b/cfgmgr/buffermgr.cpp index be8a661c87..f3d358fd8a 100644 --- a/cfgmgr/buffermgr.cpp +++ b/cfgmgr/buffermgr.cpp @@ -395,7 +395,8 @@ void BufferMgr::doPortQosTableTask(Consumer &consumer) string port_name = kfvKey(tuple); if (port_name == PORT_NAME_GLOBAL) { - // Ignore the entry for global level + // Ignore the entry for global level + it = consumer.m_toSync.erase(it); continue; } string op = kfvOp(tuple); diff --git a/orchagent/qosorch.cpp b/orchagent/qosorch.cpp index 45f89b7942..730c12ffb0 100644 --- a/orchagent/qosorch.cpp +++ b/orchagent/qosorch.cpp @@ -1705,23 +1705,42 @@ bool QosOrch::applyDscpToTcMapToSwitch(sai_attr_id_t attr_id, sai_object_id_t ma task_process_status QosOrch::handleGlobalQosMap(const string &OP, KeyOpFieldsValuesTuple &tuple) { + SWSS_LOG_ENTER(); + task_process_status task_status = task_process_status::task_success; + if (OP == DEL_COMMAND) { - // Ignore the DEL operation for switch level mapping - SWSS_LOG_NOTICE("Ignore DEL operation for global qos map"); + string referenced_obj; + if (!doesObjectExist(m_qos_maps, CFG_PORT_QOS_MAP_TABLE_NAME, PORT_NAME_GLOBAL, dscp_to_tc_field_name, referenced_obj)) + { + return task_status; + } + // Set SAI_NULL_OBJECT_ID to switch level if PORT_QOS_MAP|global is removed + if (applyDscpToTcMapToSwitch(SAI_SWITCH_ATTR_QOS_DSCP_TO_TC_MAP, SAI_NULL_OBJECT_ID)) + { + removeObject(m_qos_maps, CFG_PORT_QOS_MAP_TABLE_NAME, PORT_NAME_GLOBAL); + task_status = task_process_status::task_success; + SWSS_LOG_INFO("Global QoS map type %s is removed", dscp_to_tc_field_name.c_str()); + } + else + { + task_status = task_process_status::task_failed; + SWSS_LOG_WARN("Failed to remove switch level QoS map type %s", dscp_to_tc_field_name.c_str()); + } return task_status; } - + for (auto it = kfvFieldsValues(tuple).begin(); it != kfvFieldsValues(tuple).end(); it++) { string map_type_name = fvField(*it); string map_name = fvValue(*it); if (map_type_name != dscp_to_tc_field_name) { - SWSS_LOG_WARN("Qos map %s is not supported at global level", map_type_name.c_str()); + SWSS_LOG_WARN("Qos map type %s is not supported at global level", map_type_name.c_str()); continue; } + if (qos_to_attr_map.find(map_type_name) != qos_to_attr_map.end()) { sai_object_id_t id; @@ -1736,9 +1755,15 @@ task_process_status QosOrch::handleGlobalQosMap(const string &OP, KeyOpFieldsVal if (applyDscpToTcMapToSwitch(SAI_SWITCH_ATTR_QOS_DSCP_TO_TC_MAP, id)) { + setObjectReference(m_qos_maps, CFG_PORT_QOS_MAP_TABLE_NAME, PORT_NAME_GLOBAL, map_type_name, object_name); task_status = task_process_status::task_success; + SWSS_LOG_INFO("Applied QoS map type %s name %s to switch level", map_type_name.c_str(), object_name.c_str()); + } + else + { + task_status = task_process_status::task_failed; + SWSS_LOG_INFO("Failed to apply QoS map type %s name %s to switch level", map_type_name.c_str(), object_name.c_str()); } - setObjectReference(m_qos_maps, CFG_PORT_QOS_MAP_TABLE_NAME, PORT_NAME_GLOBAL, map_type_name, object_name); } } return task_status; diff --git a/tests/mock_tests/qosorch_ut.cpp b/tests/mock_tests/qosorch_ut.cpp index 9cf35d68c9..7461a29e3c 100644 --- a/tests/mock_tests/qosorch_ut.cpp +++ b/tests/mock_tests/qosorch_ut.cpp @@ -954,17 +954,28 @@ namespace qosorch_test // Check DSCP_TO_TC_MAP|AZURE is applied to switch ASSERT_EQ((*QosOrch::getTypeMap()[CFG_DSCP_TO_TC_MAP_TABLE_NAME])["AZURE"].m_saiObjectId, switch_dscp_to_tc_map_id); + // Remove global DSCP_TO_TC_MAP + entries.push_back({"global", "DEL", {}}); + consumer = dynamic_cast(gQosOrch->getExecutor(CFG_PORT_QOS_MAP_TABLE_NAME)); + consumer->addToSync(entries); + entries.clear(); + // Drain PORT_QOS_TABLE table + static_cast(gQosOrch)->doTask(); + // Check switch_level dscp_to_tc_map is set to NULL + ASSERT_EQ(SAI_NULL_OBJECT_ID, switch_dscp_to_tc_map_id); + entries.push_back({"AZURE", "DEL", {}}); consumer = dynamic_cast(gQosOrch->getExecutor(CFG_DSCP_TO_TC_MAP_TABLE_NAME)); consumer->addToSync(entries); entries.clear(); + auto current_sai_remove_qos_map_count = sai_remove_qos_map_count; // Drain DSCP_TO_TC_MAP table static_cast(gQosOrch)->doTask(); - // Check DSCP_TO_TC_MAP|AZURE is not removed because it's still referenced by switch - ASSERT_EQ(current_sai_remove_qos_map_count, sai_remove_qos_map_count); - ASSERT_EQ((*QosOrch::getTypeMap()[CFG_DSCP_TO_TC_MAP_TABLE_NAME]).count("AZURE"), 1); - ASSERT_EQ((*QosOrch::getTypeMap()[CFG_DSCP_TO_TC_MAP_TABLE_NAME])["AZURE"].m_saiObjectId, switch_dscp_to_tc_map_id); + // Check DSCP_TO_TC_MAP|AZURE is removed, and the switch_level dscp_to_tc_map is set to NULL + ASSERT_EQ(current_sai_remove_qos_map_count + 1, sai_remove_qos_map_count); + ASSERT_EQ((*QosOrch::getTypeMap()[CFG_DSCP_TO_TC_MAP_TABLE_NAME]).count("AZURE"), 0); + } TEST_F(QosOrchTest, QosOrchTestRetryFirstItem) diff --git a/tests/test_qos_map.py b/tests/test_qos_map.py index 1493f59e6b..6b236c4fb2 100644 --- a/tests/test_qos_map.py +++ b/tests/test_qos_map.py @@ -429,6 +429,13 @@ def test_dscp_to_tc_map_applied_to_switch(self, dvs): fvs = self.asic_db.get_entry(self.ASIC_SWITCH_STR, switch_oid) assert(fvs.get("SAI_SWITCH_ATTR_QOS_DSCP_TO_TC_MAP") == dscp_to_tc_map_id) + # Remove the global level DSCP_TO_TC_MAP + self.port_qos_table._del("global") + time.sleep(1) + + # Check the global level DSCP_TO_TC_MAP is set to SAI_ + fvs = self.asic_db.get_entry(self.ASIC_SWITCH_STR, switch_oid) + assert(fvs.get("SAI_SWITCH_ATTR_QOS_DSCP_TO_TC_MAP") == "oid:0x0") finally: if created_new_map: self.dscp_to_tc_table._del("AZURE") From 801853a6004b90882e04a8e56c461f8f04f5d06f Mon Sep 17 00:00:00 2001 From: bingwang Date: Mon, 13 Jun 2022 04:32:35 +0000 Subject: [PATCH 5/5] Rm globaldscptotcmapid Signed-off-by: bingwang --- orchagent/qosorch.cpp | 10 ---------- orchagent/qosorch.h | 3 --- 2 files changed, 13 deletions(-) diff --git a/orchagent/qosorch.cpp b/orchagent/qosorch.cpp index 730c12ffb0..274003bd72 100644 --- a/orchagent/qosorch.cpp +++ b/orchagent/qosorch.cpp @@ -270,13 +270,6 @@ bool DscpToTcMapHandler::removeQosItem(sai_object_id_t sai_object) { SWSS_LOG_ENTER(); - if (sai_object == gQosOrch->m_globalDscpToTcMap) - { - // The current global dscp to tc map is still referenced - SWSS_LOG_NOTICE("Current global dscp_to_tc map is still being referenced %" PRIx64, sai_object); - return false; - } - SWSS_LOG_DEBUG("Removing DscpToTcMap object:%" PRIx64, sai_object); sai_status_t sai_status = sai_qos_map_api->remove_qos_map(sai_object); if (SAI_STATUS_SUCCESS != sai_status) @@ -1696,9 +1689,6 @@ bool QosOrch::applyDscpToTcMapToSwitch(sai_attr_id_t attr_id, sai_object_id_t ma return false; } - if (map_id != gQosOrch->m_globalDscpToTcMap) - gQosOrch->m_globalDscpToTcMap = map_id; - SWSS_LOG_NOTICE("Applied DSCP_TO_TC QoS map to switch successfully"); return true; } diff --git a/orchagent/qosorch.h b/orchagent/qosorch.h index 5f80359296..f8b97cd381 100644 --- a/orchagent/qosorch.h +++ b/orchagent/qosorch.h @@ -212,9 +212,6 @@ class QosOrch : public Orch std::unordered_map m_scheduler_group_port_info; - // SAI OID of the global dscp to tc map - sai_object_id_t m_globalDscpToTcMap; - friend QosMapHandler; friend DscpToTcMapHandler; };