diff --git a/orchagent/qosorch.cpp b/orchagent/qosorch.cpp index 274003bd7219..14ca851e76a1 100644 --- a/orchagent/qosorch.cpp +++ b/orchagent/qosorch.cpp @@ -489,47 +489,140 @@ bool WredMapHandler::convertBool(string str, bool &val) return true; } +void WredMapHandler::appendThresholdToAttributeList(sai_attr_id_t type, + sai_uint32_t threshold, + bool needDefer, + vector &normalQueue, + vector &deferredQueue, + sai_uint32_t &newThreshold) +{ + sai_attribute_t attr; + + attr.id = type; + attr.value.u32 = threshold; + if (needDefer) + { + deferredQueue.push_back(attr); + } + else + { + normalQueue.push_back(attr); + } + newThreshold = threshold; +} + +WredMapHandler::qos_wred_thresholds_store_t WredMapHandler::m_wredProfiles; + bool WredMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &tuple, vector &attribs) { SWSS_LOG_ENTER(); sai_attribute_t attr; + vector deferred_attributes; + auto &key = kfvKey(tuple); + auto &storedProfile = WredMapHandler::m_wredProfiles[key]; + qos_wred_thresholds_t currentProfile = storedProfile; + sai_uint32_t threshold; + + /* + * Setting WRED profile can fail in case + * - the current min threshold is greater than the new max threshold + * - or the current max threshold is less than the new min threshold + * for any color at any time, on some vendor's platforms. + * + * The root cause + * There can be only one attribute in each SAI SET operation, which means + * the vendor SAI do not have a big picture regarding what attributes are being set + * and can only perform the sanity check against each SET operation. + * In the above case, the sanity check will fail. + * + * The fix + * The thresholds that have been applied to SAI will be stored in orchagent. + * + * The original logic is to handle each attribute to be set and append it to an attribute list. + * To resolve the issue, a 2nd half attribute list is introduced and + * will be appended to the original attribute list after all the attributes have been handled. + * + * In the new logic, each threshold to be set will be checked against the stored data. + * In case it violates the condition, the violating attribute will be deferred, done via putting it into the 2nd half attributes list. + * + * For any color, there can be only 1 threshold violating the condition. + * Otherwise, it means both new min > old max and new max > old min, which means either old max < old min or new max < new min, + * which means either old or new data is illegal. + * This can not happen because illegal data can not be applied and stored. + * + * By doing so, the other threshold will be applied first, which extends the threshold range and breaks the violating condition. + * A logic is also introduced to guarantee the min threshold is always less than the max threshold in the new profile to be set. + * + * For example: + * Current min=1M, max=2M, new min=3M, new max=4M + * The min is set first, so current max (2M) < new min (3M), which violates the condition + * By the new logic, min threshold will be deferred so the new max will be applied first and then the new min is applied and no violating. + * min = 1M, max = 2M + * => min = 1M, max = 4M + * => min = 3M, max = 4M + */ + for (auto i = kfvFieldsValues(tuple).begin(); i != kfvFieldsValues(tuple).end(); i++) { if (fvField(*i) == yellow_max_threshold_field_name) { - attr.id = SAI_WRED_ATTR_YELLOW_MAX_THRESHOLD; - attr.value.s32 = stoi(fvValue(*i)); - attribs.push_back(attr); + threshold = stoi(fvValue(*i)); + appendThresholdToAttributeList(SAI_WRED_ATTR_YELLOW_MAX_THRESHOLD, + threshold, + (storedProfile.yellow_min_threshold > threshold), + attribs, + deferred_attributes, + currentProfile.yellow_max_threshold); } else if (fvField(*i) == yellow_min_threshold_field_name) { - attr.id = SAI_WRED_ATTR_YELLOW_MIN_THRESHOLD; - attr.value.s32 = stoi(fvValue(*i)); - attribs.push_back(attr); + threshold = stoi(fvValue(*i)); + appendThresholdToAttributeList(SAI_WRED_ATTR_YELLOW_MIN_THRESHOLD, + threshold, + (storedProfile.yellow_max_threshold < threshold), + attribs, + deferred_attributes, + currentProfile.yellow_min_threshold); } else if (fvField(*i) == green_max_threshold_field_name) { - attr.id = SAI_WRED_ATTR_GREEN_MAX_THRESHOLD; - attr.value.s32 = stoi(fvValue(*i)); - attribs.push_back(attr); + threshold = stoi(fvValue(*i)); + appendThresholdToAttributeList(SAI_WRED_ATTR_GREEN_MAX_THRESHOLD, + threshold, + (storedProfile.green_min_threshold > threshold), + attribs, + deferred_attributes, + currentProfile.green_max_threshold); } else if (fvField(*i) == green_min_threshold_field_name) { - attr.id = SAI_WRED_ATTR_GREEN_MIN_THRESHOLD; - attr.value.s32 = stoi(fvValue(*i)); - attribs.push_back(attr); + threshold = stoi(fvValue(*i)); + appendThresholdToAttributeList(SAI_WRED_ATTR_GREEN_MIN_THRESHOLD, + threshold, + (storedProfile.green_max_threshold < threshold), + attribs, + deferred_attributes, + currentProfile.green_min_threshold); } else if (fvField(*i) == red_max_threshold_field_name) { - attr.id = SAI_WRED_ATTR_RED_MAX_THRESHOLD; - attr.value.s32 = stoi(fvValue(*i)); - attribs.push_back(attr); + threshold = stoi(fvValue(*i)); + appendThresholdToAttributeList(SAI_WRED_ATTR_RED_MAX_THRESHOLD, + threshold, + (storedProfile.red_min_threshold > threshold), + attribs, + deferred_attributes, + currentProfile.red_max_threshold); } else if (fvField(*i) == red_min_threshold_field_name) { - attr.id = SAI_WRED_ATTR_RED_MIN_THRESHOLD; - attr.value.s32 = stoi(fvValue(*i)); - attribs.push_back(attr); + threshold = stoi(fvValue(*i)); + appendThresholdToAttributeList(SAI_WRED_ATTR_RED_MIN_THRESHOLD, + threshold, + (storedProfile.red_max_threshold < threshold), + attribs, + deferred_attributes, + currentProfile.red_min_threshold); } else if (fvField(*i) == green_drop_probability_field_name) { @@ -588,6 +681,18 @@ bool WredMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &tupl return false; } } + + if ((currentProfile.green_min_threshold > currentProfile.green_max_threshold) + || (currentProfile.yellow_min_threshold > currentProfile.yellow_max_threshold) + || (currentProfile.red_min_threshold > currentProfile.red_max_threshold)) + { + SWSS_LOG_ERROR("Wrong wred profile: min threshold is greater than max threshold"); + return false; + } + + attribs.insert(attribs.end(), deferred_attributes.begin(), deferred_attributes.end()); + storedProfile = currentProfile; + return true; } diff --git a/orchagent/qosorch.h b/orchagent/qosorch.h index f8b97cd381cd..b5e2e1ad8666 100644 --- a/orchagent/qosorch.h +++ b/orchagent/qosorch.h @@ -112,6 +112,24 @@ class WredMapHandler : public QosMapHandler protected: bool convertEcnMode(string str, sai_ecn_mark_mode_t &ecn_val); bool convertBool(string str, bool &val); +private: + void appendThresholdToAttributeList(sai_attr_id_t type, + sai_uint32_t threshold, + bool needDefer, + vector &normalQueue, + vector &deferredQueue, + sai_uint32_t &newThreshold); + typedef struct { + sai_uint32_t green_max_threshold; + sai_uint32_t green_min_threshold; + sai_uint32_t yellow_max_threshold; + sai_uint32_t yellow_min_threshold; + sai_uint32_t red_max_threshold; + sai_uint32_t red_min_threshold; + } qos_wred_thresholds_t; + typedef map qos_wred_thresholds_store_t; + + static qos_wred_thresholds_store_t m_wredProfiles; }; diff --git a/tests/mock_tests/mock_orchagent_main.h b/tests/mock_tests/mock_orchagent_main.h index f41c5b29a588..0acba4ef1c20 100644 --- a/tests/mock_tests/mock_orchagent_main.h +++ b/tests/mock_tests/mock_orchagent_main.h @@ -12,8 +12,8 @@ #include "mirrororch.h" #define private public #include "bufferorch.h" -#undef private #include "qosorch.h" +#undef private #include "vrforch.h" #include "vnetorch.h" #include "vxlanorch.h" diff --git a/tests/mock_tests/qosorch_ut.cpp b/tests/mock_tests/qosorch_ut.cpp index 7461a29e3cad..13454cee56b5 100644 --- a/tests/mock_tests/qosorch_ut.cpp +++ b/tests/mock_tests/qosorch_ut.cpp @@ -23,11 +23,14 @@ namespace qosorch_test int sai_remove_qos_map_count; int sai_remove_wred_profile_count; int sai_remove_scheduler_count; + int sai_set_wred_attribute_count; sai_object_id_t switch_dscp_to_tc_map_id; sai_remove_scheduler_fn old_remove_scheduler; sai_scheduler_api_t ut_sai_scheduler_api, *pold_sai_scheduler_api; + sai_create_wred_fn old_create_wred; sai_remove_wred_fn old_remove_wred; + sai_set_wred_attribute_fn old_set_wred_attribute; sai_wred_api_t ut_sai_wred_api, *pold_sai_wred_api; sai_remove_qos_map_fn old_remove_qos_map; sai_qos_map_api_t ut_sai_qos_map_api, *pold_sai_qos_map_api; @@ -50,6 +53,102 @@ namespace qosorch_test return rc; } + bool testing_wred_thresholds; + WredMapHandler::qos_wred_thresholds_t saiThresholds; + void _ut_stub_sai_check_wred_attributes(const sai_attribute_t &attr) + { + if (!testing_wred_thresholds) + { + return; + } + + switch (attr.id) + { + case SAI_WRED_ATTR_GREEN_MAX_THRESHOLD: + ASSERT_TRUE(!saiThresholds.green_min_threshold || saiThresholds.green_min_threshold < attr.value.u32); + saiThresholds.green_max_threshold = attr.value.u32; + break; + case SAI_WRED_ATTR_GREEN_MIN_THRESHOLD: + ASSERT_TRUE(!saiThresholds.green_max_threshold || saiThresholds.green_max_threshold > attr.value.u32); + saiThresholds.green_min_threshold = attr.value.u32; + break; + case SAI_WRED_ATTR_YELLOW_MAX_THRESHOLD: + ASSERT_TRUE(!saiThresholds.yellow_min_threshold || saiThresholds.yellow_min_threshold < attr.value.u32); + saiThresholds.yellow_max_threshold = attr.value.u32; + break; + case SAI_WRED_ATTR_YELLOW_MIN_THRESHOLD: + ASSERT_TRUE(!saiThresholds.yellow_max_threshold || saiThresholds.yellow_max_threshold > attr.value.u32); + saiThresholds.yellow_min_threshold = attr.value.u32; + break; + case SAI_WRED_ATTR_RED_MAX_THRESHOLD: + ASSERT_TRUE(!saiThresholds.red_min_threshold || saiThresholds.red_min_threshold < attr.value.u32); + saiThresholds.red_max_threshold = attr.value.u32; + break; + case SAI_WRED_ATTR_RED_MIN_THRESHOLD: + ASSERT_TRUE(!saiThresholds.red_max_threshold || saiThresholds.red_max_threshold > attr.value.u32); + saiThresholds.red_min_threshold = attr.value.u32; + break; + default: + break; + } + } + + void checkWredProfileEqual(const string &name, WredMapHandler::qos_wred_thresholds_t &thresholds) + { + auto &oaThresholds = WredMapHandler::m_wredProfiles[name]; + + ASSERT_EQ(oaThresholds.green_min_threshold, thresholds.green_min_threshold); + ASSERT_EQ(oaThresholds.green_max_threshold, thresholds.green_max_threshold); + ASSERT_EQ(oaThresholds.yellow_min_threshold, thresholds.yellow_min_threshold); + ASSERT_EQ(oaThresholds.yellow_max_threshold, thresholds.yellow_max_threshold); + ASSERT_EQ(oaThresholds.red_min_threshold, thresholds.red_min_threshold); + ASSERT_EQ(oaThresholds.red_max_threshold, thresholds.red_max_threshold); + } + + void updateWredProfileAndCheck(vector &thresholdsVector, WredMapHandler::qos_wred_thresholds_t &thresholdsValue) + { + std::deque entries; + entries.push_back({"AZURE", "SET", thresholdsVector}); + auto consumer = dynamic_cast(gQosOrch->getExecutor(CFG_WRED_PROFILE_TABLE_NAME)); + consumer->addToSync(entries); + entries.clear(); + static_cast(gQosOrch)->doTask(); + checkWredProfileEqual("AZURE", saiThresholds); + checkWredProfileEqual("AZURE", thresholdsValue); + } + + void updateWrongWredProfileAndCheck(vector &thresholdsVector) + { + std::deque entries; + vector ts; + entries.push_back({"AZURE", "SET", thresholdsVector}); + auto consumer = dynamic_cast(gQosOrch->getExecutor(CFG_WRED_PROFILE_TABLE_NAME)); + consumer->addToSync(entries); + entries.clear(); + auto current_sai_wred_set_count = sai_set_wred_attribute_count; + static_cast(gQosOrch)->doTask(); + ASSERT_EQ(current_sai_wred_set_count, sai_set_wred_attribute_count); + static_cast(gQosOrch)->dumpPendingTasks(ts); + ASSERT_TRUE(ts.empty()); + } + + sai_status_t _ut_stub_sai_create_wred( + _Out_ sai_object_id_t *wred_id, + _In_ sai_object_id_t switch_id, + _In_ uint32_t attr_count, + _In_ const sai_attribute_t *attr_list) + { + auto rc = old_create_wred(wred_id, switch_id, attr_count, attr_list); + if (rc == SAI_STATUS_SUCCESS) + { + for (uint32_t i = 0; i < attr_count; i++) + { + _ut_stub_sai_check_wred_attributes(attr_list[i]); + } + } + return rc; + } + sai_status_t _ut_stub_sai_remove_wred(sai_object_id_t wred_id) { auto rc = old_remove_wred(wred_id); @@ -58,6 +157,19 @@ namespace qosorch_test return rc; } + sai_status_t _ut_stub_sai_set_wred_attribute( + _In_ sai_object_id_t wred_id, + _In_ const sai_attribute_t *attr) + { + auto rc = old_set_wred_attribute(wred_id, attr); + if (rc == SAI_STATUS_SUCCESS) + { + _ut_stub_sai_check_wred_attributes(*attr); + } + sai_set_wred_attribute_count++; + return rc; + } + sai_status_t _ut_stub_sai_remove_scheduler(sai_object_id_t scheduler_id) { auto rc = old_remove_scheduler(scheduler_id); @@ -133,6 +245,13 @@ namespace qosorch_test ReplaceSaiRemoveApi(sai_wred_api, ut_sai_wred_api, pold_sai_wred_api, _ut_stub_sai_remove_wred, sai_wred_api->remove_wred, old_remove_wred, ut_sai_wred_api.remove_wred); + // Mock other wred APIs + old_create_wred = pold_sai_wred_api->create_wred; + ut_sai_wred_api.create_wred = _ut_stub_sai_create_wred; + old_set_wred_attribute = pold_sai_wred_api->set_wred_attribute; + ut_sai_wred_api.set_wred_attribute = _ut_stub_sai_set_wred_attribute; + + // Mock switch API pold_sai_switch_api = sai_switch_api; ut_sai_switch_api = *pold_sai_switch_api; old_set_switch_attribute_fn = pold_sai_switch_api->set_switch_attribute; @@ -1070,4 +1189,152 @@ namespace qosorch_test ASSERT_EQ(ts.size(), 1); ts.clear(); } + + /* + * There are 4 ECN ranges + * ------------------------------------------------------------------------------- + * profile lower min=1M max=2M + * profile upper min=3M max=4M + * proile middle min=1.5M max=1.5M + * ------------------------------------------------------------------------------- + * Test step Test case + * 1. Initialize a wred profile with value lower set Wred profile intialization + * 2. Update the value to upper set The new min threshold is greater than the current max threshold + * 3. Update the value back to lower set The new max threshold is less than the current min threshold + * 4. Update the value to middle set Normal case to ensure nothing broken + * 5. Update the value back to lower set Normal case to ensure nothing broken + */ + TEST_F(QosOrchTest, QosOrchTestWredThresholdsTest) + { + testing_wred_thresholds = true; + + // The order of fields matters when the wred profile is updated from the upper set to the lower set + // It should be max, min for each color. In this order, the new max is less then the current min + // QoS orchagent should guarantee that the new min is configured first and then new max + vector lowerSetVector = { + {"ecn", "ecn_all"}, + {"green_drop_probability", "5"}, + {"green_max_threshold", "2097152"}, + {"green_min_threshold", "1048576"}, + {"wred_green_enable", "true"}, + {"yellow_drop_probability", "5"}, + {"yellow_max_threshold", "2097153"}, + {"yellow_min_threshold", "1048577"}, + {"wred_yellow_enable", "true"}, + {"red_drop_probability", "5"}, + {"red_max_threshold", "2097154"}, + {"red_min_threshold", "1048578"}, + {"wred_red_enable", "true"} + }; + WredMapHandler::qos_wred_thresholds_t lowerThresholds = { + 2097152, //green_max_threshold + 1048576, //green_min_threshold + 2097153, //yellow_max_threshold + 1048577, //yellow_min_threshold + 2097154, //red_max_threshold + 1048578 //red_min_threshold + }; + // The order of fields matters when the wred profile is updated from the lower set to the upper set + // It should be min, max for each color, in which the new min is larger then the current max + // QoS orchagent should guarantee that the new max is configured first and then new min + vector upperSetVector = { + {"ecn", "ecn_all"}, + {"green_drop_probability", "5"}, + {"green_min_threshold", "3145728"}, + {"green_max_threshold", "4194304"}, + {"wred_green_enable", "true"}, + {"yellow_drop_probability", "5"}, + {"yellow_min_threshold", "3145729"}, + {"yellow_max_threshold", "4194305"}, + {"wred_yellow_enable", "true"}, + {"red_drop_probability", "5"}, + {"red_min_threshold", "3145730"}, + {"red_max_threshold", "4194306"}, + {"wred_red_enable", "true"} + }; + WredMapHandler::qos_wred_thresholds_t upperThresholds = { + 4194304, //green_max_threshold + 3145728, //green_min_threshold + 4194305, //yellow_max_threshold + 3145729, //yellow_min_threshold + 4194306, //red_max_threshold + 3145730 //red_min_threshold + }; + // Order doesn't matter. + vector middleSetVector = { + {"ecn", "ecn_all"}, + {"green_drop_probability", "5"}, + {"green_min_threshold", "1572864"}, + {"green_max_threshold", "2621440"}, + {"wred_green_enable", "true"}, + {"yellow_drop_probability", "5"}, + {"yellow_min_threshold", "1572865"}, + {"yellow_max_threshold", "2621441"}, + {"wred_yellow_enable", "true"}, + {"red_drop_probability", "5"}, + {"red_min_threshold", "1572866"}, + {"red_max_threshold", "2621442"}, + {"wred_red_enable", "true"} + }; + WredMapHandler::qos_wred_thresholds_t middleThresholds = { + 2621440, //green_max_threshold + 1572864, //green_min_threshold + 2621441, //yellow_max_threshold + 1572865, //yellow_min_threshold + 2621442, //red_max_threshold + 1572866 //red_min_threshold + }; + + // Wrong profile + vector greenWrongVector = { + {"ecn", "ecn_green"}, + {"green_drop_probability", "5"}, + {"green_min_threshold", "2621440"}, + {"green_max_threshold", "1572864"}, + {"wred_green_enable", "true"} + }; + + vector yellowWrongVector = { + {"ecn", "ecn_yellow"}, + {"yellow_drop_probability", "5"}, + {"yellow_min_threshold", "2621441"}, + {"yellow_max_threshold", "1572865"}, + {"wred_yellow_enable", "true"} + }; + + vector redWrongVector = { + {"ecn", "ecn_red"}, + {"red_drop_probability", "5"}, + {"red_min_threshold", "2621442"}, + {"red_max_threshold", "1572866"}, + {"wred_red_enable", "true"} + }; + + std::deque entries; + // 1. Initialize + updateWredProfileAndCheck(lowerSetVector, lowerThresholds); + + // 2. Update the thresholds from the lower set to the upper set + updateWredProfileAndCheck(upperSetVector, upperThresholds); + + // 3. Update the thresholds from the upper set back to the lower set + updateWredProfileAndCheck(lowerSetVector, lowerThresholds); + + // 4. Update the thresholds from the lower set to the middle set + updateWredProfileAndCheck(middleSetVector, middleThresholds); + + // 5. Update the thresholds from the middle set back to the lower set + updateWredProfileAndCheck(lowerSetVector, lowerThresholds); + + // Wrong parameters + updateWrongWredProfileAndCheck(greenWrongVector); + updateWrongWredProfileAndCheck(yellowWrongVector); + updateWrongWredProfileAndCheck(redWrongVector); + + // Make sure the profiles in orchagent and SAI are not updated by the wrong profile + checkWredProfileEqual("AZURE", saiThresholds); + checkWredProfileEqual("AZURE", lowerThresholds); + + testing_wred_thresholds = false; + } }