Skip to content

backout down parent retry limiting in parent selection and nexthop strategies #8546

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jrushford merged 1 commit into from
Dec 8, 2021
Merged

backout down parent retry limiting in parent selection and nexthop strategies #8546

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions mgmt/RecordsConfig.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,8 +441,6 @@ 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}
Expand Down
19 changes: 7 additions & 12 deletions proxy/ParentConsistentHash.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,17 +235,13 @@ ParentConsistentHash::selectParent(bool first_call, ParentResult *result, Reques
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.inc(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(), max_retriers);
break;
}
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;
break;
}
}
Debug("parent_select", "wrap_around[PRIMARY]: %d, wrap_around[SECONDARY]: %d", wrap_around[PRIMARY], wrap_around[SECONDARY]);
Expand Down Expand Up @@ -393,7 +389,6 @@ ParentConsistentHash::markParentUp(ParentResult *result)

pRec->failedAt = static_cast<time_t>(0);
int old_count = pRec->failCount.exchange(0, std::memory_order_relaxed);
pRec->retriers.clear();

if (old_count > 0) {
Note("http parent proxy %s:%d restored", pRec->hostname, pRec->port);
Expand Down
17 changes: 7 additions & 10 deletions proxy/ParentRoundRobin.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,16 +157,13 @@ 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 == TS_HOST_STATUS_UP)) {
if (parents[cur_index].retriers.inc(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(),
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);
}
Debug("parent_select", "Parent[%d].failedAt = %u, retry = %u, xact_start = %" PRId64 " but wrap = %d", cur_index,
static_cast<unsigned>(parents[cur_index].failedAt.load()), retry_time, 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 {
parentUp = false;
}
Expand Down
15 changes: 0 additions & 15 deletions proxy/ParentSelection.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,6 @@ 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.clear();
if (tmp3) {
memcpy(this->parents[i].hash_string, tmp3 + 1, strlen(tmp3));
this->parents[i].name = this->parents[i].hash_string;
Expand All @@ -555,7 +554,6 @@ 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.clear();
if (tmp3) {
memcpy(this->secondary_parents[i].hash_string, tmp3 + 1, strlen(tmp3));
this->secondary_parents[i].name = this->secondary_parents[i].hash_string;
Expand Down Expand Up @@ -1839,9 +1837,6 @@ 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);
Expand All @@ -1862,16 +1857,6 @@ EXCLUSIVE_REGRESSION_TEST(PARENTSELECTION)(RegressionTest * /* t ATS_UNUSED */,
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;
Expand Down
83 changes: 0 additions & 83 deletions proxy/ParentSelection.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,64 +105,7 @@ struct SimpleRetryResponseCodes {
private:
std::vector<int> codes;
};
// class pRetriers
//
// Count of retriers with atomic read, increment, and decrement.
//
class pRetriers
{
public:
int
operator()() const
{
return _v.load();
}

void
clear()
{
_v = 0;
}

bool
inc(int max_retriers)
{
ink_assert(max_retriers > 0);

int r = _v.load(std::memory_order_relaxed);
while (r < max_retriers) {
if (_v.compare_exchange_weak(r, r + 1)) {
return true;
}
}
return false;
}

void
dec()
{
int r = _v.load(std::memory_order_relaxed);
while (r > 0) {
if (_v.compare_exchange_weak(r, r - 1)) {
break;
}
}
}

pRetriers() = default;

pRetriers(pRetriers const &o) { _v = o._v.load(); }

pRetriers &
operator=(pRetriers const &o)
{
_v = o._v.load();
return *this;
}

private:
std::atomic<int> _v = 0;
};
// struct pRecord
//
// A record for an individual parent
Expand All @@ -178,13 +121,6 @@ struct pRecord : ATSConsistentHashNode {
int idx;
float weight;
char hash_string[MAXDNAME + 1];
pRetriers retriers;

void
retryComplete()
{
retriers.dec();
}
};

typedef ControlMatcher<ParentRecord, ParentResult> P_table;
Expand Down Expand Up @@ -424,16 +360,6 @@ class ParentSelectionStrategy

// virtual destructor.
virtual ~ParentSelectionStrategy(){};

void
retryComplete(ParentResult *result)
{
pRecord *p = getParents(result);
uint32_t n = numParents(result);
if (p != nullptr && result->last_parent < n) {
p[result->last_parent].retryComplete();
}
}
};

class ParentConfigParams : public ConfigInfo
Expand Down Expand Up @@ -486,15 +412,6 @@ class ParentConfigParams : public ConfigInfo
}
}

void
retryComplete(ParentResult *result)
{
if (!result->is_api_result()) {
ink_release_assert(result != nullptr);
result->rec->selection_strategy->retryComplete(result);
}
}

P_table *parent_table;
ParentRecord *DefaultParent;
ParentSelectionPolicy policy;
Expand Down
1 change: 0 additions & 1 deletion proxy/ParentSelectionStrategy.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,6 @@ ParentSelectionStrategy::markParentUp(ParentResult *result)
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.clear();

if (old_count > 0) {
Note("http parent proxy %s:%d restored", pRec->hostname, pRec->port);
Expand Down
13 changes: 1 addition & 12 deletions proxy/http/HttpTransact.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,17 +286,6 @@ nextParent(HttpTransact::State *s)
}
}

inline static void
retryComplete(HttpTransact::State *s)
{
url_mapping *mp = s->url_map.getMapping();
if (mp && mp->strategy) {
mp->strategy->retryComplete(reinterpret_cast<TSHttpTxn>(s->state_machine), s->parent_result.hostname, s->parent_result.port);
} else if (s->parent_params) {
s->parent_params->retryComplete(&s->parent_result);
}
}

inline static bool
is_localhost(const char *name, int len)
{
Expand Down Expand Up @@ -3637,7 +3626,7 @@ HttpTransact::handle_response_from_parent(State *s)
// if this parent was retried from a markdown, then
// notify that the retry has completed.
if (s->parent_result.retry) {
retryComplete(s);
markParentUp(s);
}

simple_or_unavailable_server_retry(s);
Expand Down
19 changes: 7 additions & 12 deletions proxy/http/remap/NextHopConsistentHash.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,18 +366,13 @@ NextHopConsistentHash::findNextHop(TSHttpTxn txnp, void *ih, time_t now)
if (!pRec->available.load() && host_stat == TS_HOST_STATUS_UP) {
_now == 0 ? _now = time(nullptr) : _now = now;
if ((pRec->failedAt.load() + retry_time) < static_cast<unsigned>(_now)) {
NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] next hop %s, retriers: %d", sm_id, pRec->hostname.c_str(), pRec->retriers());
if (pRec->retriers.inc(max_retriers)) {
nextHopRetry = true;
result.last_parent = pRec->host_index;
result.last_lookup = pRec->group_index;
result.retry = nextHopRetry;
result.result = PARENT_SPECIFIED;
NH_Debug(NH_DEBUG_TAG,
"[%" PRIu64 "] next hop %s is now retryable, marked it available, retriers: %d, max_retriers: %d.", sm_id,
pRec->hostname.c_str(), pRec->retriers(), max_retriers);
break;
}
nextHopRetry = true;
result.last_parent = pRec->host_index;
result.last_lookup = pRec->group_index;
result.retry = nextHopRetry;
result.result = PARENT_SPECIFIED;
NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] next hop %s is now retryable", sm_id, pRec->hostname.c_str());
break;
}
}

Expand Down
22 changes: 0 additions & 22 deletions proxy/http/remap/NextHopHealthStatus.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,25 +148,3 @@ NextHopHealthStatus::markNextHop(TSHttpTxn txn, const char *hostname, const int
break;
}
}

void
NextHopHealthStatus::retryComplete(TSHttpTxn txn, const char *hostname, const int port)
{
HttpSM *sm = reinterpret_cast<HttpSM *>(txn);
ParentResult result = sm->t_state.parent_result;
int64_t sm_id = sm->sm_id;

// make sure we're called back with a result structure for a parent that is being retried.
if (result.result != PARENT_SPECIFIED && !result.retry) {
return;
}

const std::string host_port = HostRecord::makeHostPort(hostname, port);
auto iter = host_map.find(host_port);
if (iter == host_map.end()) {
NH_Debug(NH_DEBUG_TAG, "[%" PRId64 "] no host named %s found in host_map", sm_id, host_port.c_str());
return;
}

iter->second->retriers.dec();
}
13 changes: 5 additions & 8 deletions proxy/http/remap/NextHopRoundRobin.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,14 +150,11 @@ NextHopRoundRobin::findNextHop(TSHttpTxn txnp, void *ih, time_t now)
_now == 0 ? _now = time(nullptr) : _now = now;
if (((result->wrap_around) || (cur_host->failedAt + retry_time) < static_cast<unsigned>(_now)) &&
host_stat == TS_HOST_STATUS_UP) {
if (cur_host->retriers.inc(max_retriers)) {
// Reuse the parent
parentUp = true;
parentRetry = true;
NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] NextHop marked for retry %s:%d, max_retriers: %d, retriers: %d", sm_id,
cur_host->hostname.c_str(), host_groups[cur_grp_index][cur_hst_index]->getPort(scheme), max_retriers,
cur_host->retriers());
}
// Reuse the parent
parentUp = true;
parentRetry = true;
NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] NextHop marked for retry %s:%d", sm_id, cur_host->hostname.c_str(),
host_groups[cur_grp_index][cur_hst_index]->getPort(scheme));
} else { // not retryable or available.
parentUp = false;
}
Expand Down
21 changes: 4 additions & 17 deletions proxy/http/remap/NextHopSelectionStrategy.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,17 +43,10 @@ constexpr const char *policy_strings[] = {"NH_UNDEFINED", "NH_FIRST_LIVE", "NH_R

NextHopSelectionStrategy::NextHopSelectionStrategy(const std::string_view &name, const NHPolicyType &policy)
{
int _max_retriers = 0;
strategy_name = name;
policy_type = policy;
NH_GetConfig(_max_retriers, "proxy.config.http.parent_proxy.max_trans_retries");

// config settings may not be available when running unit tests.
// so use the max_retriers default setting.
if (_max_retriers > 0) {
max_retriers = _max_retriers;
}
NH_Debug(NH_DEBUG_TAG, "Using a selection strategy of type %s, max_retriers: %d", policy_strings[policy], max_retriers);
strategy_name = name;
policy_type = policy;

NH_Debug(NH_DEBUG_TAG, "Using a selection strategy of type %s", policy_strings[policy]);
}

//
Expand Down Expand Up @@ -316,12 +309,6 @@ NextHopSelectionStrategy::responseIsRetryable(int64_t sm_id, HttpTransact::Curre
return PARENT_RETRY_NONE;
}

void
NextHopSelectionStrategy::retryComplete(TSHttpTxn txnp, const char *hostname, const int port)
{
return passive_health.retryComplete(txnp, hostname, port);
}

namespace YAML
{
template <> struct convert<HostRecord> {
Expand Down
Loading