[QoS] Enforce drop probability only for colors whose WRED are enabled (#2422)

What I did
Do not enforce drop probability for a color whose WRED is disabled.

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

Why I did it
Currently, there is a logic to enforce the drop probability if it is not explicitly designated for a color. However, the drop probability is not a mandatory attribute. It can incur vendor SAI complaints to set it when the color is disabled.
The logic was introduced from the very beginning (by PR #571) because no drop probability was defined in the QoS template at the time, which is no longer true.
So we will enforce drop probability only if it is not configured and the color is enabled.

How I verified it
Unit test and manual test

Author: stephenxs

orchagent/qosorch.cpp

@@ -46,6 +46,12 @@ enum {
     RED_DROP_PROBABILITY_SET    = (1U << 2)
 };
 
+enum {
+    GREEN_WRED_ENABLED  = (1U << 0),
+    YELLOW_WRED_ENABLED = (1U << 1),
+    RED_WRED_ENABLED    = (1U << 2)
+};
+
 // field_name is what is expected in CONFIG_DB PORT_QOS_MAP table
 map<string, sai_port_attr_t> qos_to_attr_map = {
     {dscp_to_tc_field_name, SAI_PORT_ATTR_QOS_DSCP_TO_TC_MAP},
@@ -720,6 +726,7 @@ sai_object_id_t WredMapHandler::addQosItem(const vector<sai_attribute_t> &attrib
     sai_attribute_t attr;
     vector<sai_attribute_t> attrs;
     uint8_t drop_prob_set = 0;
+    uint8_t wred_enable_set = 0;
 
     attr.id = SAI_WRED_ATTR_WEIGHT;
     attr.value.s32 = 0;
@@ -729,32 +736,53 @@ sai_object_id_t WredMapHandler::addQosItem(const vector<sai_attribute_t> &attrib
     {
         attrs.push_back(attrib);
 
-        if (attrib.id == SAI_WRED_ATTR_GREEN_DROP_PROBABILITY)
+        switch (attrib.id)
         {
+        case SAI_WRED_ATTR_GREEN_ENABLE:
+            if (attrib.value.booldata)
+            {
+                wred_enable_set |= GREEN_WRED_ENABLED;
+            }
+            break;
+        case SAI_WRED_ATTR_YELLOW_ENABLE:
+            if (attrib.value.booldata)
+            {
+                wred_enable_set |= YELLOW_WRED_ENABLED;
+            }
+            break;
+        case SAI_WRED_ATTR_RED_ENABLE:
+            if (attrib.value.booldata)
+            {
+                wred_enable_set |= RED_WRED_ENABLED;
+            }
+            break;
+        case SAI_WRED_ATTR_GREEN_DROP_PROBABILITY:
             drop_prob_set |= GREEN_DROP_PROBABILITY_SET;
-        }
-        else if (attrib.id == SAI_WRED_ATTR_YELLOW_DROP_PROBABILITY)
-        {
+            break;
+        case SAI_WRED_ATTR_YELLOW_DROP_PROBABILITY:
             drop_prob_set |= YELLOW_DROP_PROBABILITY_SET;
-        }
-        else if (attrib.id == SAI_WRED_ATTR_RED_DROP_PROBABILITY)
-        {
+            break;
+        case SAI_WRED_ATTR_RED_DROP_PROBABILITY:
             drop_prob_set |= RED_DROP_PROBABILITY_SET;
+            break;
+        default:
+            break;
         }
     }
-    if (!(drop_prob_set & GREEN_DROP_PROBABILITY_SET))
+
+    if (!(drop_prob_set & GREEN_DROP_PROBABILITY_SET) && (wred_enable_set & GREEN_WRED_ENABLED))
     {
         attr.id = SAI_WRED_ATTR_GREEN_DROP_PROBABILITY;
         attr.value.s32 = 100;
         attrs.push_back(attr);
     }
-    if (!(drop_prob_set & YELLOW_DROP_PROBABILITY_SET))
+    if (!(drop_prob_set & YELLOW_DROP_PROBABILITY_SET) && (wred_enable_set & YELLOW_WRED_ENABLED))
     {
         attr.id = SAI_WRED_ATTR_YELLOW_DROP_PROBABILITY;
         attr.value.s32 = 100;
         attrs.push_back(attr);
     }
-    if (!(drop_prob_set & RED_DROP_PROBABILITY_SET))
+    if (!(drop_prob_set & RED_DROP_PROBABILITY_SET) && (wred_enable_set & RED_WRED_ENABLED))
     {
         attr.id = SAI_WRED_ATTR_RED_DROP_PROBABILITY;
         attr.value.s32 = 100;

tests/mock_tests/qosorch_ut.cpp

@@ -37,6 +37,13 @@ namespace qosorch_test
     sai_set_switch_attribute_fn old_set_switch_attribute_fn;
     sai_switch_api_t ut_sai_switch_api, *pold_sai_switch_api;
 
+    typedef struct
+    {
+        sai_uint32_t green_max_drop_probability;
+        sai_uint32_t yellow_max_drop_probability;
+        sai_uint32_t red_max_drop_probability;
+    } qos_wred_max_drop_probability_t;
+
     sai_status_t _ut_stub_sai_set_switch_attribute(sai_object_id_t switch_id, const sai_attribute_t *attr)
     {
         auto rc = old_set_switch_attribute_fn(switch_id, attr);
@@ -55,6 +62,7 @@ namespace qosorch_test
 
     bool testing_wred_thresholds;
     WredMapHandler::qos_wred_thresholds_t saiThresholds;
+    qos_wred_max_drop_probability_t saiMaxDropProbabilities;
     void _ut_stub_sai_check_wred_attributes(const sai_attribute_t &attr)
     {
         if (!testing_wred_thresholds)
@@ -88,6 +96,15 @@ namespace qosorch_test
             ASSERT_TRUE(!saiThresholds.red_max_threshold || saiThresholds.red_max_threshold > attr.value.u32);
             saiThresholds.red_min_threshold = attr.value.u32;
             break;
+        case SAI_WRED_ATTR_GREEN_DROP_PROBABILITY:
+            saiMaxDropProbabilities.green_max_drop_probability = attr.value.u32;
+            break;
+        case SAI_WRED_ATTR_YELLOW_DROP_PROBABILITY:
+            saiMaxDropProbabilities.yellow_max_drop_probability = attr.value.u32;
+            break;
+        case SAI_WRED_ATTR_RED_DROP_PROBABILITY:
+            saiMaxDropProbabilities.red_max_drop_probability = attr.value.u32;
+            break;
         default:
             break;
         }
@@ -132,6 +149,23 @@ namespace qosorch_test
         ASSERT_TRUE(ts.empty());
     }
 
+    void updateMaxDropProbabilityAndCheck(string name, vector<FieldValueTuple> &maxDropProbabilityVector, qos_wred_max_drop_probability_t &maxDropProbabilities)
+    {
+        std::deque<KeyOpFieldsValuesTuple> entries;
+        vector<string> ts;
+        entries.push_back({name, "SET", maxDropProbabilityVector});
+        auto consumer = dynamic_cast<Consumer *>(gQosOrch->getExecutor(CFG_WRED_PROFILE_TABLE_NAME));
+        consumer->addToSync(entries);
+        entries.clear();
+        saiMaxDropProbabilities.green_max_drop_probability = 0;
+        saiMaxDropProbabilities.yellow_max_drop_probability = 0;
+        saiMaxDropProbabilities.red_max_drop_probability = 0;
+        static_cast<Orch *>(gQosOrch)->doTask();
+        ASSERT_EQ(saiMaxDropProbabilities.green_max_drop_probability, maxDropProbabilities.green_max_drop_probability);
+        ASSERT_EQ(saiMaxDropProbabilities.yellow_max_drop_probability, maxDropProbabilities.yellow_max_drop_probability);
+        ASSERT_EQ(saiMaxDropProbabilities.red_max_drop_probability, maxDropProbabilities.red_max_drop_probability);
+    }
+
     sai_status_t _ut_stub_sai_create_wred(
         _Out_ sai_object_id_t *wred_id,
         _In_ sai_object_id_t switch_id,
@@ -1338,6 +1372,62 @@ namespace qosorch_test
         testing_wred_thresholds = false;
     }
 
+    TEST_F(QosOrchTest, QosOrchTestWredDropProbability)
+    {
+        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<FieldValueTuple> greenProfile = {
+            {"wred_green_enable", "true"},
+            {"wred_yellow_enable", "false"},
+        };
+        qos_wred_max_drop_probability_t greenProbabilities = {
+            100, // green_max_drop_probability
+            0,   // yellow_max_drop_probability
+            0    // red_max_drop_probability
+        };
+        updateMaxDropProbabilityAndCheck("green_default", greenProfile, greenProbabilities);
+
+        greenProfile.push_back({"green_drop_probability", "5"});
+        greenProbabilities.green_max_drop_probability = 5;
+        updateMaxDropProbabilityAndCheck("green", greenProfile, greenProbabilities);
+
+        vector<FieldValueTuple> yellowProfile = {
+            {"wred_yellow_enable", "true"},
+            {"wred_red_enable", "false"},
+        };
+        qos_wred_max_drop_probability_t yellowProbabilities = {
+            0,   // green_max_drop_probability
+            100, // yellow_max_drop_probability
+            0    // red_max_drop_probability
+        };
+        updateMaxDropProbabilityAndCheck("yellow_default", yellowProfile, yellowProbabilities);
+
+        yellowProfile.push_back({"yellow_drop_probability", "5"});
+        yellowProbabilities.yellow_max_drop_probability = 5;
+        updateMaxDropProbabilityAndCheck("yellow", yellowProfile, yellowProbabilities);
+
+        vector<FieldValueTuple> redProfile = {
+            {"wred_green_enable", "false"},
+            {"wred_red_enable", "true"},
+        };
+        qos_wred_max_drop_probability_t redProbabilities = {
+            0,   // green_max_drop_probability
+            0,   // yellow_max_drop_probability
+            100  // red_max_drop_probability
+        };
+        updateMaxDropProbabilityAndCheck("red_default", redProfile, redProbabilities);
+
+        redProfile.push_back({"red_drop_probability", "5"});
+        redProbabilities.red_max_drop_probability = 5;
+        updateMaxDropProbabilityAndCheck("red", redProfile, redProbabilities);
+
+        testing_wred_thresholds = false;
+    }
+
+
     /*
      * Make sure empty fields won't cause orchagent crash
      */