add more documents

Author: a114j0y

orchagent/orch.cpp

@@ -189,22 +189,31 @@ bool Orch::addToRetry(const std::string &executorName, const Task &task, const C
     return false;
 }
 
-size_t Orch::retryToSync(const std::string &executorName, size_t threshold)
+/**
+ * @brief Check the consumer's RetryCache, if the set of resolved constraints is not empty,
+ * query RetryMap for failed tasks indexed by these resolved constraints,
+ * and move them back to the consumer's SyncMap, such that they can be retried in the next iteration.
+ * @param executorName - name of the consumer
+ * @param quota - maximum number of tasks to be moved back to SyncMap in a single call
+ * @return number of tasks moved back to SyncMap
+ */
+size_t Orch::retryToSync(const std::string &executorName, size_t quota)
 {
     auto retryCache = getRetryCache(executorName);
 
-    if (!retryCache || threshold <= 0)
+    // directly return 0 if no retry cache for this executor or quota is non-positive
+    if (!retryCache || quota <= 0)
         return 0;
 
     std::unordered_set<Constraint>& constraints = retryCache->getResolvedConstraints();
 
     size_t count = 0;
 
-    while (!constraints.empty() && count < threshold)
+    while (!constraints.empty() && count < quota)
     {
         auto cst = *constraints.begin();
 
-        auto tasks = retryCache->resolve(cst, threshold - count);
+        auto tasks = retryCache->resolve(cst, quota - count);
 
         count += tasks->size();
 
@@ -812,7 +821,8 @@ string Orch::objectReferenceInfo(
 
 void Orch::doTask()
 {
-
+    // limit the number of tasks moved from RetryMap to SyncMap in one iteration 
+    // to avoid starvation of new tasks in SyncMap
     auto threshold = gBatchSize == 0 ? 30000 : gBatchSize;
 
     size_t count = 0;

orchagent/orch.h

@@ -183,9 +183,9 @@ class ConsumerBase : public Executor {
     size_t addToSync(std::shared_ptr<std::deque<swss::KeyOpFieldsValuesTuple>> entries, bool onRetry=false); 
 
     /**
-     * Move a task to retry cache for future processing
-     * @param task a task tuple
-     * @param cst the constraint for the task
+     * @brief Add the failed task and its constraint to the consumer's RetryCache
+     * @param task - the task that has failed due to the unmet constraint
+     * @param cst - the unmet constraint that blocks the task from being retried
      */
     bool addToRetry(const Task &task, const Constraint &cst);
 
@@ -322,19 +322,30 @@ class Orch
     RetryCache* getRetryCache(const std::string &executorName);
     ConsumerBase* getConsumerBase(const std::string &executorName);
 
-    // Add a task and its constraint to the retry cache 
+    /** 
+     * @brief Add the failed task and its constraint to the consumer's RetryCache
+     * @param executorName - name of the consumer
+     * @param task - the task that has failed due to the unmet constraint
+     * @param cst - the unmet constraint that blocks the task from being retried
+     * @return true only if the consumer has initialized a retry cache and the task is successfully added into it
+     */
     bool addToRetry(const std::string &executorName, const Task &task, const Constraint &cst);
 
-    /** Delete tasks whose constraints are resolved in this executor's retry cache , then add them back to its m_toSync.
-     * @param executorName name of the executor (actually a ConsumerBase instance)
-     * @param cst task constraint **/
-    virtual size_t retryToSync(const std::string &executorName, size_t threshold=30000);
+    /** 
+     * @brief Check the consumer's RetryCache for pending tasks with constraints already resolved.
+     * If any, move them from RetryMap back to SyncMap, such that they can be retried when the consumer's execute() method is invoked.
+     * Make sure to limit the number of tasks added to SyncMap in one call, to avoid starvation of new tasks in SyncMap.
+     * @param executorName - name of the consumer
+     * @param quota - maximum number of tasks to be moved back to SyncMap in a single call
+     */
+    virtual size_t retryToSync(const std::string &executorName, size_t quota=30000);
 
-    /** Notify the executor that the constraint is already resolved
-     * @param retryOrch the orch to be notified
-     * @param executorName name of the executor to be notified
-     * @param cst the constraint that can be resolved
-     * **/
+    /** 
+     * @brief Notify the consumer that the constraint is already resolved
+     * @param retryOrch - the consumer's Orch instance, used to get the consumer's RetryCache
+     * @param executorName - name of the consumer to be notified
+     * @param cst - the constraint that is resolved
+     */
     virtual void notifyRetry(Orch *retryOrch, const std::string &executorName, const Constraint &cst);
 
     /**

orchagent/retrycache.h

@@ -10,21 +10,15 @@ using namespace swss;
 enum ConstraintType
 {
     RETRY_CST_DUMMY,
-    RETRY_CST_NHG,          // nhg doesn't exist
-    RETRY_CST_NHG_REF,      // nhg refcnt nonzero
     RETRY_CST_PIC,          // context doesn't exist
-    RETRY_CST_PIC_REF,      // context refcnt nonzero
-    RETRY_CST_ECMP          // ecmp resources exhausted
+    RETRY_CST_PIC_REF      // context refcnt nonzero
 };
 
 static inline std::ostream& operator<<(std::ostream& os, ConstraintType t) {
     switch(t) {
         case ConstraintType::RETRY_CST_DUMMY:   return os << "RETRY_CST_DUMMY";
-        case ConstraintType::RETRY_CST_NHG:   return os << "RETRY_CST_NHG";
-        case ConstraintType::RETRY_CST_NHG_REF:   return os << "RETRY_CST_NHG_REF";
         case ConstraintType::RETRY_CST_PIC: return os << "RETRY_CST_PIC";
         case ConstraintType::RETRY_CST_PIC_REF:  return os << "RETRY_CST_PIC_REF";
-        case ConstraintType::RETRY_CST_ECMP:   return os << "RETRY_CST_ECMP";
         default:           return os << "UNKNOWN";
     }
 }

orchagent/routeorch.cpp

@@ -1171,7 +1171,7 @@ void RouteOrch::doTask(ConsumerBase& consumer)
                 {
                     /* If any existing routes are updated to point to the
                      * above interfaces, remove them from the ASIC. */
-                     if (removeRoutePost(ctx) || rc_inserted)
+                    if (removeRoutePost(ctx) || rc_inserted)
                         it_prev = consumer.m_toSync.erase(it_prev);
                     else
                         it_prev++;
@@ -1182,7 +1182,7 @@ void RouteOrch::doTask(ConsumerBase& consumer)
 
                 if (nhg.getSize() == 1 && nhg.hasIntfNextHop())
                 {
-                     if (addRoutePost(ctx, nhg) || rc_inserted)
+                    if (addRoutePost(ctx, nhg) || rc_inserted)
                         it_prev = consumer.m_toSync.erase(it_prev);
                     else
                         it_prev++;

tests/mock_tests/retrycache_ut.cpp

@@ -38,7 +38,7 @@ namespace retrycache_test
                         std::cout << "  CstResolver [Selected]: resolves the constraint and notifies." << std::endl;
                         auto it = consumer.m_toSync.begin();
                         consumer.m_toSync.erase(it);
-                        Constraint cst = make_constraint(RETRY_CST_NHG, "2");
+                        Constraint cst = make_constraint(RETRY_CST_PIC, "2");
                         notifyRetry(this, "Dependent", cst);
                     }
                     else if (consumer.getName() == "Dependent")
@@ -64,7 +64,7 @@ namespace retrycache_test
             TestOrch* testOrch;
             Consumer *oftenFail, *cstResolver;
             RetryCache* cache;
-            Constraint cst = make_constraint(RETRY_CST_NHG, "2");
+            Constraint cst = make_constraint(RETRY_CST_PIC, "2");
 
             void SetUp() override
             {
@@ -103,7 +103,7 @@ namespace retrycache_test
         ASSERT_TRUE(cache->getRetryMap().empty());
 
         cache->insert({"key", "DEL", {}}, cst);
-        Constraint _cst = make_constraint(RETRY_CST_NHG, "3");
+        Constraint _cst = make_constraint(RETRY_CST_PIC, "3");
         cache->insert({"key", "SET", {{"nexthop_index", "3"}}}, _cst);
         tasks = cache->resolve(cst);
         ASSERT_EQ(tasks->size(), 1);
@@ -121,9 +121,9 @@ namespace retrycache_test
         std::cout << "Orchdaemon Event Loop 1: " << std::endl;
 
         // Assume the Dependent Executor receives a task to process
-        Task task{"TEST_ROUTE", "SET", {{"NHG", "2"}}};
+        Task task{"TEST_ROUTE", "SET", {{"PIC", "2"}}};
         oftenFail->addToSync(task);
-        // But it fails because of the constraint: nhg 2 doesn't exist
+        // But it fails because of the constraint: PIC 2 doesn't exist
         // It moves the task from m_toSync to m_toRetry
         oftenFail->drain();
 
@@ -150,7 +150,7 @@ namespace retrycache_test
         std::cout << "Orchdaemon Event Loop 2: " << std::endl;
 
         // Assume in the next event loop, the cstResolver a task to process, which resolves the constraint for oftenFail.
-        cstResolver->addToSync({"TEST_NHG", "SET", {{"ID", "2"}}});
+        cstResolver->addToSync({"TEST_PIC", "SET", {{"ID", "2"}}});
         cstResolver->drain();
 
         // Post-resolution
@@ -221,7 +221,7 @@ namespace retrycache_test
 
         // Assume there are a SET and a DEL task in the retry cache
         cache->insert(setTask, cst);
-        Constraint del_cst = make_constraint(RETRY_CST_NHG_REF, "");
+        Constraint del_cst = make_constraint(RETRY_CST_PIC_REF, "");
         cache->insert(delTask, del_cst);
         ASSERT_EQ(cache->getRetryMap().size(), 2);
 
@@ -239,24 +239,24 @@ namespace retrycache_test
     TEST_F(RetryCacheTest, NewSetTask)
     {
         // Assume there is an old SET task in the retry cache
-        Task setTask{"TEST_ROUTE", "SET", {{"NHG", "1"}}};
-        Constraint cst_nhg_1 = make_constraint(RETRY_CST_NHG, "1");
-        cache->insert(setTask, cst_nhg_1);
+        Task setTask{"TEST_ROUTE", "SET", {{"PIC", "1"}}};
+        Constraint cst_pic_1 = make_constraint(RETRY_CST_PIC, "1");
+        cache->insert(setTask, cst_pic_1);
 
         // Assume there is a new task with same field, received by the consumer
-        Task setTask2{"TEST_ROUTE", "SET", {{"NHG", "2"}}};
+        Task setTask2{"TEST_ROUTE", "SET", {{"PIC", "2"}}};
         oftenFail->addToSync(setTask2);
         // Check if it clears the retrycache
         ASSERT_TRUE(cache->getRetryMap().empty());
-        ASSERT_EQ(cache->m_retryKeys.find(cst_nhg_1), cache->m_retryKeys.end());
+        ASSERT_EQ(cache->m_retryKeys.find(cst_pic_1), cache->m_retryKeys.end());
         // Check if it adds the task 2 into the sync queue
         auto iter = oftenFail->m_toSync.find("TEST_ROUTE");
         ASSERT_EQ(iter->second, setTask2);
 
-        // Assume NHG 2 also doesn't exist, the new task fails too
+        // Assume PIC 2 also doesn't exist, the new task fails too
         oftenFail->m_toSync.erase(iter);
-        Constraint cst_nhg2 = make_constraint(RETRY_CST_NHG, "2");
-        cache->insert(setTask2, cst_nhg2);
+        Constraint cst_pic_2 = make_constraint(RETRY_CST_PIC, "2");
+        cache->insert(setTask2, cst_pic_2);
 
         // Assume there is a new SET task with a new field, received by the consumer
         Task setTask3{"TEST_ROUTE", "SET", {{"VRF", "1"}}};
@@ -274,7 +274,7 @@ namespace retrycache_test
         {
             if (fvField(fv) == "VRF")
                 ASSERT_EQ(fvValue(fv), "1");
-            else if (fvField(fv) == "NHG")
+            else if (fvField(fv) == "PIC")
                 ASSERT_EQ(fvValue(fv), "2");
             else
                 ASSERT_FALSE(true); // unexpected field
@@ -287,8 +287,8 @@ namespace retrycache_test
         // simulate by firstly adding a DEL, then adding a SET into the retry cache
         Task delTask{"TEST_ROUTE", "DEL", {{"", ""}}};
         cache->insert(delTask, DUMMY_CONSTRAINT);
-        Task setTask{"TEST_ROUTE", "SET", {{"NHG", "1"}}};
-        Constraint cst = make_constraint(RETRY_CST_NHG, "1");
+        Task setTask{"TEST_ROUTE", "SET", {{"PIC", "1"}}};
+        Constraint cst = make_constraint(RETRY_CST_PIC, "1");
         cache->insert(setTask, cst);
 
         ASSERT_EQ(cache->getRetryMap().size(), 2);
@@ -308,7 +308,7 @@ namespace retrycache_test
         {
             if (fvField(fv) == "VRF")
                 ASSERT_EQ(fvValue(fv), "1");
-            else if (fvField(fv) == "NHG")
+            else if (fvField(fv) == "PIC")
                 ASSERT_EQ(fvValue(fv), "1");
              else
                 ASSERT_FALSE(true); // unexpected field