This PR updates parent selection to limit the number of simultaneous

transactions that will retry a parent once it's retry_time window
has elapsed.  The limiting prevents a thundering retry of a parent by
hundreds of transactions that could very will hang if the parent is
still unreachable following retry_time expiration,
'proxy.config.http.parent_proxy.max_trans_retries' which defaults to 2.

Author: jrushford

doc/admin-guide/files/records.config.en.rst

@@ -1194,6 +1194,11 @@ Parent Proxy Configuration
 
    The amount of time allowed between connection retries to a parent cache that is unavailable.
 
+.. ts:cv:: CONFIG proxy.config.http.parent_proxy.max_trans_retries INT 2
+
+   Limits the number of simultaneous transactions that may retry a parent once the parents
+   ``retry_time`` has expired.
+
 .. ts:cv:: CONFIG proxy.config.http.parent_proxy.fail_threshold INT 10
    :reloadable:
    :overridable:

mgmt/RecordsConfig.cc

@@ -425,6 +425,8 @@ static const RecordElement RecordsConfig[] =
   //#  the retry window for the parent to be marked down
   {RECT_CONFIG, "proxy.config.http.parent_proxy.fail_threshold", RECD_INT, "10", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
   ,
+  {RECT_CONFIG, "proxy.config.http.parent_proxy.max_trans_retries", RECD_INT, "2", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
+  ,
   {RECT_CONFIG, "proxy.config.http.parent_proxy.total_connect_attempts", RECD_INT, "4", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
   ,
   {RECT_CONFIG, "proxy.config.http.parent_proxy.per_parent_connect_attempts", RECD_INT, "2", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL}

proxy/ParentConsistentHash.cc

@@ -231,9 +231,11 @@ ParentConsistentHash::selectParent(bool first_call, ParentResult *result, Reques
       // check if the host is retryable.  It's retryable if the retry window has elapsed
       // and the global host status is HOST_STATUS_UP
       if (pRec && !pRec->available && host_stat == HOST_STATUS_UP) {
-        Debug("parent_select", "Parent.failedAt = %u, retry = %u, xact_start = %u", (unsigned int)pRec->failedAt,
-              (unsigned int)retry_time, (unsigned int)request_info->xact_start);
-        if ((pRec->failedAt + retry_time) < request_info->xact_start) {
+        Debug("parent_select", "Parent.failedAt = %u, retry = %u, retriers = %d, max_retriers = %u, xact_start = %u",
+              static_cast<unsigned>(pRec->failedAt), static_cast<unsigned>(retry_time), pRec->retriers, max_retriers,
+              static_cast<unsigned>(request_info->xact_start));
+        if (((pRec->failedAt + retry_time) < request_info->xact_start) && (pRec->retriers < max_retriers) &&
+            ink_atomic_increment(&pRec->retriers, 1) < max_retriers) {
           parentRetry = true;
           // make sure that the proper state is recorded in the result structure
           result->last_parent = pRec->idx;
@@ -389,6 +391,7 @@ ParentConsistentHash::markParentUp(ParentResult *result)
 
   ink_atomic_swap(&pRec->failedAt, static_cast<time_t>(0));
   int old_count = ink_atomic_swap(&pRec->failCount, 0);
+  ink_atomic_swap(&pRec->retriers, 0);
 
   if (old_count > 0) {
     Note("http parent proxy %s:%d restored", pRec->hostname, pRec->port);

proxy/ParentRoundRobin.cc

@@ -156,9 +156,12 @@ ParentRoundRobin::selectParent(bool first_call, ParentResult *result, RequestDat
       }
     } else {
       if ((result->wrap_around) ||
-          ((parents[cur_index].failedAt + retry_time) < request_info->xact_start && host_stat == HOST_STATUS_UP)) {
-        Debug("parent_select", "Parent[%d].failedAt = %u, retry = %u,xact_start = %" PRId64 " but wrap = %d", cur_index,
-              (unsigned)parents[cur_index].failedAt, retry_time, (int64_t)request_info->xact_start, result->wrap_around);
+          ((((parents[cur_index].failedAt + retry_time) < request_info->xact_start) && host_stat == HOST_STATUS_UP) &&
+           (parents[cur_index].retriers < max_retriers && ink_atomic_increment(&parents[cur_index].retriers, 1) < max_retriers))) {
+        Debug("parent_select",
+              "Parent[%d].failedAt = %u, retry = %u, retriers = %d, max_retriers = %u, xact_start = %" PRId64 " but wrap = %d",
+              cur_index, static_cast<unsigned>(parents[cur_index].failedAt), retry_time, parents[cur_index].retriers, max_retriers,
+              static_cast<int64_t>(request_info->xact_start), result->wrap_around);
         // Reuse the parent
         parentUp    = true;
         parentRetry = true;

proxy/ParentSelection.cc

@@ -539,6 +539,7 @@ ParentRecord::ProcessParents(char *val, bool isPrimary)
       this->parents[i].name                    = this->parents[i].hostname;
       this->parents[i].available               = true;
       this->parents[i].weight                  = weight;
+      this->parents[i].retriers                = 0;
       if (tmp3) {
         memcpy(this->parents[i].hash_string, tmp3 + 1, strlen(tmp3));
         this->parents[i].name = this->parents[i].hash_string;
@@ -554,6 +555,7 @@ ParentRecord::ProcessParents(char *val, bool isPrimary)
       this->secondary_parents[i].name                    = this->secondary_parents[i].hostname;
       this->secondary_parents[i].available               = true;
       this->secondary_parents[i].weight                  = weight;
+      this->secondary_parents[i].retriers                = 0;
       if (tmp3) {
         memcpy(this->secondary_parents[i].hash_string, tmp3 + 1, strlen(tmp3));
         this->secondary_parents[i].name = this->secondary_parents[i].hash_string;
@@ -1105,6 +1107,11 @@ EXCLUSIVE_REGRESSION_TEST(PARENTSELECTION)(RegressionTest * /* t ATS_UNUSED */,
     params->findParent(request, result, fail_threshold, retry_time); \
   } while (0)
 
+#define SET_MAX_RETRIERS(x)                                                                     \
+  do {                                                                                          \
+    RecSetRecordInt("proxy.config.http.parent_proxy.max_trans_retries", x, REC_SOURCE_DEFAULT); \
+  } while (0)
+
   // start tests by marking up all tests hosts that will be marked down
   // as part of testing.  This will insure that test hosts are not loaded
   // from records.snap as DOWN due to previous testing.
@@ -1117,6 +1124,8 @@ EXCLUSIVE_REGRESSION_TEST(PARENTSELECTION)(RegressionTest * /* t ATS_UNUSED */,
   _st.setHostStatus("curly", HOST_STATUS_UP, 0, Reason::MANUAL);
 
   // Test 1
+  SET_MAX_RETRIERS(20);
+  RecSetRecordInt("proxy.config.http.parent_proxy.max_trans_retries", 20, REC_SOURCE_DEFAULT);
   tbl[0] = '\0';
   ST(1);
   T("dest_domain=. parent=red:37412,orange:37412,yellow:37412 round_robin=strict\n");
@@ -1831,6 +1840,39 @@ EXCLUSIVE_REGRESSION_TEST(PARENTSELECTION)(RegressionTest * /* t ATS_UNUSED */,
   FP;
   RE(verify(result, PARENT_SPECIFIED, "carol", 80), 211);
 
+  // max_retriers tests
+  SET_MAX_RETRIERS(1);
+
+  // Test 212
+  tbl[0] = '\0';
+  ST(212);
+  T("dest_domain=mouse.com parent=mickey:80|0.33;minnie:80|0.33;goofy:80|0.33 "
+    "round_robin=consistent_hash go_direct=false\n");
+  REBUILD;
+  REINIT;
+  br(request, "i.am.mouse.com");
+  FP;
+  RE(verify(result, PARENT_SPECIFIED, "goofy", 80), 212);
+
+  // Test 213
+  // markdown goofy and minnie gets chosen.
+  ST(213);
+  params->markParentDown(result, fail_threshold, retry_time); // marked down goofy
+  REINIT;
+  br(request, "i.am.mouse.com");
+  FP;
+  RE(verify(result, PARENT_SPECIFIED, "minnie", 80), 213);
+
+  // Test 214
+  // goofy gets chosen because max_retriers was set to 1
+  // and goofy becomes available.
+  sleep(params->policy.ParentRetryTime + 2);
+  ST(214);
+  REINIT;
+  br(request, "i.am.mouse.com");
+  FP;
+  RE(verify(result, PARENT_SPECIFIED, "goofy", 80), 214);
+
   delete request;
   delete result;
   delete params;

proxy/ParentSelection.h

@@ -112,13 +112,14 @@ struct SimpleRetryResponseCodes {
 struct pRecord : ATSConsistentHashNode {
   char hostname[MAXDNAME + 1];
   int port;
-  time_t failedAt;
-  int failCount;
+  time_t failedAt = 0;
+  int failCount   = 0;
   int32_t upAt;
   const char *scheme; // for which parent matches (if any)
   int idx;
   float weight;
   char hash_string[MAXDNAME + 1];
+  int retriers = 0;
 };
 
 typedef ControlMatcher<ParentRecord, ParentResult> P_table;
@@ -332,6 +333,9 @@ struct ParentSelectionPolicy {
 class ParentSelectionStrategy
 {
 public:
+  int max_retriers = 0;
+
+  ParentSelectionStrategy() { REC_ReadConfigInteger(max_retriers, "proxy.config.http.parent_proxy.max_trans_retries"); }
   //
   // Return the pRecord.
   virtual pRecord *getParents(ParentResult *result) = 0;

proxy/ParentSelectionStrategy.cc

@@ -65,6 +65,11 @@ ParentSelectionStrategy::markParentDown(ParentResult *result, unsigned int fail_
     //   must set the count to reflect this
     if (result->retry == false) {
       new_fail_count = pRec->failCount = 1;
+    } else {
+      // this was a retry that failed, decrement the retriers count
+      if (ink_atomic_decrement(&pRec->retriers, 1) < 0) {
+        ink_atomic_swap(&pRec->retriers, 0);
+      }
     }
 
     Note("Parent %s marked as down %s:%d", (result->retry) ? "retry" : "initially", pRec->hostname, pRec->port);
@@ -121,6 +126,8 @@ ParentSelectionStrategy::markParentUp(ParentResult *result)
 
   ink_atomic_swap(&pRec->failedAt, static_cast<time_t>(0));
   int old_count = ink_atomic_swap(&pRec->failCount, 0);
+  // a retry succeeded, just reset retriers
+  ink_atomic_swap(&pRec->retriers, 0);
 
   if (old_count > 0) {
     Note("http parent proxy %s:%d restored", pRec->hostname, pRec->port);

proxy/http/HttpTransact.cc

@@ -236,6 +236,9 @@ parentExists(HttpTransact::State *s)
 inline static void
 nextParent(HttpTransact::State *s)
 {
+  TxnDebug("parent_down", "sm_id[%" PRId64 "] connection to parent %s failed, conn_state: %s, request to origin: %s",
+           s->state_machine->sm_id, s->parent_result.hostname, HttpDebugNames::get_server_state_name(s->current.state),
+           s->request_data.get_host());
   url_mapping *mp = s->url_map.getMapping();
   if (mp && mp->strategy) {
     // NextHop only has a findNextHop() function.