You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by jr...@apache.org on 2021/02/26 16:10:51 UTC
[trafficserver] branch master updated: This PR updates parent
selection to limit the number of simultaneous (#7485)
This is an automated email from the ASF dual-hosted git repository.
jrushford pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new ee2731f This PR updates parent selection to limit the number of simultaneous (#7485)
ee2731f is described below
commit ee2731fe38c30721138edec010ff101fad84d68c
Author: John J. Rushford <jr...@apache.org>
AuthorDate: Fri Feb 26 09:10:40 2021 -0700
This PR updates parent selection to limit the number of simultaneous (#7485)
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.
---
doc/admin-guide/files/records.config.en.rst | 5 ++++
include/tscore/ConsistentHash.h | 3 +-
mgmt/RecordsConfig.cc | 2 ++
proxy/ParentConsistentHash.cc | 45 +++++++++++++++++------------
proxy/ParentRoundRobin.cc | 25 ++++++++++------
proxy/ParentSelection.cc | 41 ++++++++++++++++++++++++++
proxy/ParentSelection.h | 8 +++--
proxy/ParentSelectionStrategy.cc | 32 ++++++++++++--------
proxy/http/HttpTransact.cc | 3 ++
proxy/http/remap/NextHopSelectionStrategy.h | 2 +-
10 files changed, 122 insertions(+), 44 deletions(-)
diff --git a/doc/admin-guide/files/records.config.en.rst b/doc/admin-guide/files/records.config.en.rst
index 7e1f184..681d002 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 9946776..75d979c 100644
--- a/include/tscore/ConsistentHash.h
+++ b/include/tscore/ConsistentHash.h
@@ -21,6 +21,7 @@
#pragma once
+#include <atomic>
#include "Hash.h"
#include <cstdint>
#include <iostream>
@@ -31,7 +32,7 @@
*/
struct ATSConsistentHashNode {
- bool available;
+ std::atomic<bool> available;
char *name;
};
diff --git a/mgmt/RecordsConfig.cc b/mgmt/RecordsConfig.cc
index 9dff9c2..e51642d 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 edd929d..3c00c3f 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 <atomic>
#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<unsigned>(pRec->failedAt.load()),
+ static_cast<unsigned>(retry_time), static_cast<unsigned>(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<time_t>(0));
- int old_count = ink_atomic_swap(&pRec->failCount, 0);
+ pRec->failedAt = static_cast<time_t>(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 5c955bb..cb13b8f 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<int>(fail_threshold))) {
+ if ((parents[cur_index].failedAt.load() == 0) || (parents[cur_index].failCount.load() < static_cast<int>(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<unsigned>(parents[cur_index].failedAt.load()), retry_time,
+ parents[cur_index].retriers.load(), max_retriers, static_cast<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);
+ } else {
+ parents[cur_index].retriers--;
+ }
} else {
parentUp = false;
}
diff --git a/proxy/ParentSelection.cc b/proxy/ParentSelection.cc
index 61706b0..3d38dcd 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 010fcba..a9b8947 100644
--- a/proxy/ParentSelection.h
+++ b/proxy/ParentSelection.h
@@ -112,13 +112,14 @@ private:
struct pRecord : ATSConsistentHashNode {
char hostname[MAXDNAME + 1];
int port;
- time_t failedAt;
- int failCount;
+ std::atomic<time_t> failedAt = 0;
+ std::atomic<int> 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<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;
diff --git a/proxy/ParentSelectionStrategy.cc b/proxy/ParentSelectionStrategy.cc
index cf067b4..1c1c3a7 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<int>(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<time_t>(0));
- int old_count = ink_atomic_swap(&pRec->failCount, 0);
+ pRec->failedAt = static_cast<time_t>(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 a8bfd23..7f318f1 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 6076188..d430b41 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;
}