diff --git a/doc/admin-guide/files/records.config.en.rst b/doc/admin-guide/files/records.config.en.rst index 8519daf1ed5..e8e133de393 100644 --- a/doc/admin-guide/files/records.config.en.rst +++ b/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: diff --git a/include/tscore/ConsistentHash.h b/include/tscore/ConsistentHash.h index 9946776cc61..75d979c0aed 100644 --- a/include/tscore/ConsistentHash.h +++ b/include/tscore/ConsistentHash.h @@ -21,6 +21,7 @@ #pragma once +#include #include "Hash.h" #include #include @@ -31,7 +32,7 @@ */ struct ATSConsistentHashNode { - bool available; + std::atomic available; char *name; }; diff --git a/mgmt/RecordsConfig.cc b/mgmt/RecordsConfig.cc index a7f7db2e379..5e06b2c0671 100644 --- a/mgmt/RecordsConfig.cc +++ b/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} diff --git a/proxy/ParentConsistentHash.cc b/proxy/ParentConsistentHash.cc index edd929d9bac..3c00c3f6ec4 100644 --- a/proxy/ParentConsistentHash.cc +++ b/proxy/ParentConsistentHash.cc @@ -20,6 +20,7 @@ See the License for the specific language governing permissions and limitations under the License. */ +#include #include "HostStatus.h" #include "ParentConsistentHash.h" @@ -226,22 +227,27 @@ ParentConsistentHash::selectParent(bool first_call, ParentResult *result, Reques host_stat = HOST_STATUS_UP; } } - if (!pRec || (pRec && !pRec->available) || host_stat == HOST_STATUS_DOWN) { + if (!pRec || (pRec && !pRec->available.load()) || host_stat == HOST_STATUS_DOWN) { do { // 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) { - parentRetry = true; - // make sure that the proper state is recorded in the result structure - result->last_parent = pRec->idx; - result->last_lookup = last_lookup; - result->retry = parentRetry; - result->result = PARENT_SPECIFIED; - Debug("parent_select", "Down parent %s is now retryable, marked it available.", pRec->hostname); - break; + if (pRec && !pRec->available.load() && host_stat == HOST_STATUS_UP) { + Debug("parent_select", "Parent.failedAt = %u, retry = %u, xact_start = %u", static_cast(pRec->failedAt.load()), + static_cast(retry_time), static_cast(request_info->xact_start)); + if ((pRec->failedAt.load() + retry_time) < request_info->xact_start) { + if (pRec->retriers.fetch_add(1, std::memory_order_relaxed) < max_retriers) { + parentRetry = true; + // make sure that the proper state is recorded in the result structure + result->last_parent = pRec->idx; + result->last_lookup = last_lookup; + result->retry = parentRetry; + result->result = PARENT_SPECIFIED; + Debug("parent_select", "Down parent %s is now retryable, retriers = %d, max_retriers = %d", pRec->hostname, + pRec->retriers.load(), max_retriers); + break; + } else { + pRec->retriers--; + } } } Debug("parent_select", "wrap_around[PRIMARY]: %d, wrap_around[SECONDARY]: %d", wrap_around[PRIMARY], wrap_around[SECONDARY]); @@ -306,7 +312,7 @@ ParentConsistentHash::selectParent(bool first_call, ParentResult *result, Reques host_stat = HOST_STATUS_UP; } } - } while (!pRec || !pRec->available || host_stat == HOST_STATUS_DOWN); + } while (!pRec || !pRec->available.load() || host_stat == HOST_STATUS_DOWN); } Debug("parent_select", "Additional parent lookups: %d", lookups); @@ -322,7 +328,7 @@ ParentConsistentHash::selectParent(bool first_call, ParentResult *result, Reques host_stat = HOST_STATUS_UP; } } - if (pRec && host_stat == HOST_STATUS_UP && (pRec->available || result->retry)) { + if (pRec && host_stat == HOST_STATUS_UP && (pRec->available.load() || result->retry)) { result->result = PARENT_SPECIFIED; result->hostname = pRec->hostname; result->port = pRec->port; @@ -383,12 +389,13 @@ ParentConsistentHash::markParentUp(ParentResult *result) } ink_assert((result->last_parent) < numParents(result)); - pRec = parents[result->last_lookup] + result->last_parent; - ink_atomic_swap(&pRec->available, true); + pRec = parents[result->last_lookup] + result->last_parent; + pRec->available = true; Debug("parent_select", "%s:%s(): marked %s:%d available.", __FILE__, __func__, pRec->hostname, pRec->port); - ink_atomic_swap(&pRec->failedAt, static_cast(0)); - int old_count = ink_atomic_swap(&pRec->failCount, 0); + pRec->failedAt = static_cast(0); + int old_count = pRec->failCount.exchange(0, std::memory_order_relaxed); + pRec->retriers = 0; if (old_count > 0) { Note("http parent proxy %s:%d restored", pRec->hostname, pRec->port); diff --git a/proxy/ParentRoundRobin.cc b/proxy/ParentRoundRobin.cc index 5c955bb2415..cb13b8f60ce 100644 --- a/proxy/ParentRoundRobin.cc +++ b/proxy/ParentRoundRobin.cc @@ -147,22 +147,29 @@ ParentRoundRobin::selectParent(bool first_call, ParentResult *result, RequestDat } Debug("parent_select", "cur_index: %d, result->start_parent: %d", cur_index, result->start_parent); // DNS ParentOnly inhibits bypassing the parent so always return that t - if ((parents[cur_index].failedAt == 0) || (parents[cur_index].failCount < static_cast(fail_threshold))) { + if ((parents[cur_index].failedAt.load() == 0) || (parents[cur_index].failCount.load() < static_cast(fail_threshold))) { if (host_stat == HOST_STATUS_UP) { Debug("parent_select", "FailThreshold = %d", fail_threshold); Debug("parent_select", "Selecting a parent due to little failCount (faileAt: %u failCount: %d)", - (unsigned)parents[cur_index].failedAt, parents[cur_index].failCount); + (unsigned)parents[cur_index].failedAt.load(), parents[cur_index].failCount.load()); parentUp = true; } } 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); - // Reuse the parent - parentUp = true; - parentRetry = true; - Debug("parent_select", "Parent marked for retry %s:%d", parents[cur_index].hostname, parents[cur_index].port); + (((parents[cur_index].failedAt + retry_time) < request_info->xact_start) && host_stat == HOST_STATUS_UP)) { + if (parents[cur_index].retriers.fetch_add(1, std::memory_order_relaxed) < 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(parents[cur_index].failedAt.load()), retry_time, + parents[cur_index].retriers.load(), max_retriers, static_cast(request_info->xact_start), + result->wrap_around); + // Reuse the parent + parentUp = true; + parentRetry = true; + Debug("parent_select", "Parent marked for retry %s:%d", parents[cur_index].hostname, parents[cur_index].port); + } else { + parents[cur_index].retriers--; + } } else { parentUp = false; } diff --git a/proxy/ParentSelection.cc b/proxy/ParentSelection.cc index 61706b0eba5..3d38dcd268c 100644 --- a/proxy/ParentSelection.cc +++ b/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,7 @@ EXCLUSIVE_REGRESSION_TEST(PARENTSELECTION)(RegressionTest * /* t ATS_UNUSED */, _st.setHostStatus("curly", HOST_STATUS_UP, 0, Reason::MANUAL); // Test 1 + SET_MAX_RETRIERS(20); tbl[0] = '\0'; ST(1); T("dest_domain=. parent=red:37412,orange:37412,yellow:37412 round_robin=strict\n"); @@ -1831,6 +1839,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 + 3); + ST(214); + REINIT; + br(request, "i.am.mouse.com"); + FP; + RE(verify(result, PARENT_SPECIFIED, "goofy", 80), 214); + delete request; delete result; delete params; diff --git a/proxy/ParentSelection.h b/proxy/ParentSelection.h index 010fcba509a..a9b894786ce 100644 --- a/proxy/ParentSelection.h +++ b/proxy/ParentSelection.h @@ -112,13 +112,14 @@ struct SimpleRetryResponseCodes { struct pRecord : ATSConsistentHashNode { char hostname[MAXDNAME + 1]; int port; - time_t failedAt; - int failCount; + std::atomic failedAt = 0; + std::atomic failCount = 0; int32_t upAt; const char *scheme; // for which parent matches (if any) int idx; float weight; char hash_string[MAXDNAME + 1]; + std::atomic retriers = 0; }; typedef ControlMatcher 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; diff --git a/proxy/ParentSelectionStrategy.cc b/proxy/ParentSelectionStrategy.cc index cf067b41ea3..1c1c3a7e4e7 100644 --- a/proxy/ParentSelectionStrategy.cc +++ b/proxy/ParentSelectionStrategy.cc @@ -53,18 +53,23 @@ ParentSelectionStrategy::markParentDown(ParentResult *result, unsigned int fail_ // handle this condition. If this was the result of a retry, we // must update move the failedAt timestamp to now so that we continue // negative cache the parent - if (pRec->failedAt == 0 || result->retry == true) { + if (pRec->failedAt.load() == 0 || result->retry == true) { // Reread the current time. We want this to be accurate since // it relates to how long the parent has been down. now = time(nullptr); // Mark the parent failure time. - ink_atomic_swap(&pRec->failedAt, now); + pRec->failedAt = now; // If this is clean mark down and not a failed retry, we // 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 ((pRec->retriers--) < 0) { + pRec->retriers = 0; + } } Note("Parent %s marked as down %s:%d", (result->retry) ? "retry" : "initially", pRec->hostname, pRec->port); @@ -75,12 +80,12 @@ ParentSelectionStrategy::markParentDown(ParentResult *result, unsigned int fail_ // if the last failure was outside the retry window, set the failcount to 1 // and failedAt to now. - if ((pRec->failedAt + retry_time) < now) { + if ((pRec->failedAt.load() + retry_time) < now) { // coverity[check_return] - ink_atomic_swap(&pRec->failCount, 1); - ink_atomic_swap(&pRec->failedAt, now); + pRec->failCount = 1; + pRec->failedAt = now; } else { - old_count = ink_atomic_increment(&pRec->failCount, 1); + old_count = pRec->failCount.fetch_add(1, std::memory_order_relaxed); } Debug("parent_select", "Parent fail count increased to %d for %s:%d", old_count + 1, pRec->hostname, pRec->port); @@ -90,8 +95,9 @@ ParentSelectionStrategy::markParentDown(ParentResult *result, unsigned int fail_ if (new_fail_count > 0 && new_fail_count >= static_cast(fail_threshold)) { Note("Failure threshold met failcount:%d >= threshold:%d, http parent proxy %s:%d marked down", new_fail_count, fail_threshold, pRec->hostname, pRec->port); - ink_atomic_swap(&pRec->available, false); - Debug("parent_select", "Parent %s:%d marked unavailable, pRec->available=%d", pRec->hostname, pRec->port, pRec->available); + pRec->available = false; + Debug("parent_select", "Parent %s:%d marked unavailable, pRec->available=%d", pRec->hostname, pRec->port, + pRec->available.load()); } } @@ -116,11 +122,13 @@ ParentSelectionStrategy::markParentUp(ParentResult *result) } ink_assert((int)(result->last_parent) < num_parents); - pRec = parents + result->last_parent; - ink_atomic_swap(&pRec->available, true); + pRec = parents + result->last_parent; + pRec->available = true; - ink_atomic_swap(&pRec->failedAt, static_cast(0)); - int old_count = ink_atomic_swap(&pRec->failCount, 0); + pRec->failedAt = static_cast(0); + int old_count = pRec->failCount.exchange(0, std::memory_order_relaxed); + // a retry succeeded, just reset retriers + pRec->retriers = 0; if (old_count > 0) { Note("http parent proxy %s:%d restored", pRec->hostname, pRec->port); diff --git a/proxy/http/HttpTransact.cc b/proxy/http/HttpTransact.cc index 279c532e260..82df535d1f7 100644 --- a/proxy/http/HttpTransact.cc +++ b/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. diff --git a/proxy/http/remap/NextHopSelectionStrategy.h b/proxy/http/remap/NextHopSelectionStrategy.h index 6076188f00c..d430b413669 100644 --- a/proxy/http/remap/NextHopSelectionStrategy.h +++ b/proxy/http/remap/NextHopSelectionStrategy.h @@ -141,7 +141,7 @@ struct HostRecord : ATSConsistentHashNode { hash_string = o.hash_string; host_index = o.host_index; group_index = o.group_index; - available = o.available; + available = o.available.load(); protocols = o.protocols; return *this; }