You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by zw...@apache.org on 2021/10/07 15:01:24 UTC
[trafficserver] branch 9.2.x updated: Fixes issue #8329 crash in
NextHopConsistentHash where when the only (#8365)
This is an automated email from the ASF dual-hosted git repository.
zwoop pushed a commit to branch 9.2.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/9.2.x by this push:
new 476d50a Fixes issue #8329 crash in NextHopConsistentHash where when the only (#8365)
476d50a is described below
commit 476d50a2fd58d6d92ccfc86e6a1f3cb565464fbb
Author: John J. Rushford <jr...@apache.org>
AuthorDate: Thu Oct 7 08:33:05 2021 -0600
Fixes issue #8329 crash in NextHopConsistentHash where when the only (#8365)
host in a strategy is unavailable due to DNS lookup failures.
- refactored NextHopConsistentHash::findNextHop() to fix the crash
and simplify it.
- fixed the unit tests in test_NextHopConsistentHash.cc so that test
failure checks using strcmp() do not crash with a nullptr hostname
result.
(cherry picked from commit 6ccae6c52bde6f0af0ab385cac99ffbd35ce4f22)
---
proxy/http/remap/NextHopConsistentHash.cc | 356 +++++++++++----------
proxy/http/remap/NextHopConsistentHash.h | 5 +-
proxy/http/remap/NextHopHealthStatus.cc | 11 +-
.../remap/unit-tests/test_NextHopConsistentHash.cc | 53 +--
4 files changed, 217 insertions(+), 208 deletions(-)
diff --git a/proxy/http/remap/NextHopConsistentHash.cc b/proxy/http/remap/NextHopConsistentHash.cc
index f6f304a..33b0dea 100644
--- a/proxy/http/remap/NextHopConsistentHash.cc
+++ b/proxy/http/remap/NextHopConsistentHash.cc
@@ -23,7 +23,6 @@
#include <yaml-cpp/yaml.h>
-#include "tscore/HashSip.h"
#include "HttpSM.h"
#include "I_Machine.h"
#include "YamlCfg.h"
@@ -37,25 +36,46 @@ constexpr std::string_view hash_key_path_query = "path+query";
constexpr std::string_view hash_key_path_fragment = "path+fragment";
constexpr std::string_view hash_key_cache = "cache_key";
-static HostRecord *
-chash_lookup(const std::shared_ptr<ATSConsistentHash> &ring, uint64_t hash_key, ATSConsistentHashIter *iter, bool *wrapped,
- ATSHash64Sip24 *hash, bool *hash_init, bool *mapWrapped, uint64_t sm_id)
+static bool
+isWrapped(std::vector<bool> &wrap_around, uint32_t groups)
{
- HostRecord *host_rec = nullptr;
+ bool all_wrapped = true;
+ for (uint32_t c = 0; c < groups; c++) {
+ if (wrap_around[c] == false) {
+ all_wrapped = false;
+ }
+ }
+ return all_wrapped;
+}
+
+std::shared_ptr<HostRecord>
+NextHopConsistentHash::chashLookup(const std::shared_ptr<ATSConsistentHash> &ring, uint32_t cur_ring, ParentResult &result,
+ HttpRequestData &request_info, bool *wrapped, uint64_t sm_id)
+{
+ uint64_t hash_key = 0;
+ ATSHash64Sip24 hash;
+ HostRecord *host_rec = nullptr;
+ ATSConsistentHashIter *iter = &result.chashIter[cur_ring];
- if (*hash_init == false) {
- host_rec = static_cast<HostRecord *>(ring->lookup_by_hashval(hash_key, iter, wrapped));
- *hash_init = true;
+ if (result.chash_init[cur_ring] == false) {
+ hash_key = getHashKey(sm_id, request_info, &hash);
+ host_rec = static_cast<HostRecord *>(ring->lookup_by_hashval(hash_key, iter, wrapped));
+ result.chash_init[cur_ring] = true;
} else {
- host_rec = static_cast<HostRecord *>(ring->lookup(nullptr, iter, wrapped, hash));
+ host_rec = static_cast<HostRecord *>(ring->lookup(nullptr, iter, wrapped, &hash));
}
bool wrap_around = *wrapped;
- *wrapped = (*mapWrapped && *wrapped) ? true : false;
- if (!*mapWrapped && wrap_around) {
- *mapWrapped = true;
+ *wrapped = (result.mapWrapped[cur_ring] && *wrapped) ? true : false;
+ if (!result.mapWrapped[cur_ring] && wrap_around) {
+ result.mapWrapped[cur_ring] = true;
}
- return host_rec;
+ if (host_rec == nullptr) {
+ return nullptr;
+ } else {
+ std::shared_ptr<HostRecord> h = host_groups[host_rec->group_index][host_rec->host_index];
+ return h;
+ }
}
NextHopConsistentHash::~NextHopConsistentHash()
@@ -126,9 +146,9 @@ NextHopConsistentHash::Init(ts::Yaml::Map &n)
// returns a hash key calculated from the request and 'hash_key' configuration
// parameter.
uint64_t
-NextHopConsistentHash::getHashKey(uint64_t sm_id, HttpRequestData *hrdata, ATSHash64 *h)
+NextHopConsistentHash::getHashKey(uint64_t sm_id, const HttpRequestData &hrdata, ATSHash64 *h)
{
- URL *url = hrdata->hdr->url_get();
+ URL *url = hrdata.hdr->url_get();
URL *ps_url = nullptr;
int len = 0;
const char *url_string_ref = nullptr;
@@ -177,7 +197,7 @@ NextHopConsistentHash::getHashKey(uint64_t sm_id, HttpRequestData *hrdata, ATSHa
break;
// use the cache key created by the cache-key plugin.
case NH_CACHE_HASH_KEY:
- ps_url = *(hrdata->cache_info_parent_selection_url);
+ ps_url = *(hrdata.cache_info_parent_selection_url);
if (ps_url) {
url_string_ref = ps_url->string_get_ref(&len);
if (url_string_ref && len > 0) {
@@ -214,244 +234,230 @@ NextHopConsistentHash::findNextHop(TSHttpTxn txnp, void *ih, time_t now)
{
uint32_t const NO_RING_USE_POST_REMAP = uint32_t(0) - 1;
- HttpSM *sm = reinterpret_cast<HttpSM *>(txnp);
- ParentResult *result = &sm->t_state.parent_result;
- HttpRequestData request_info = sm->t_state.request_data;
- int64_t sm_id = sm->sm_id;
- int64_t retry_time = sm->t_state.txn_conf->parent_retry_time;
- time_t _now = now;
- bool firstcall = false;
- bool nextHopRetry = false;
- bool wrapped = false;
+ HttpSM *sm = reinterpret_cast<HttpSM *>(txnp);
+ ParentResult &result = sm->t_state.parent_result;
+ HttpRequestData &request_info = sm->t_state.request_data;
+ int64_t sm_id = sm->sm_id;
+ int64_t retry_time = sm->t_state.txn_conf->parent_retry_time;
+ time_t _now = now;
+ bool firstcall = false;
+ bool nextHopRetry = false;
+ bool wrapped = false;
std::vector<bool> wrap_around(groups, false);
- uint32_t cur_ring = 0; // there is a hash ring for each host group
- uint64_t hash_key = 0;
- uint32_t lookups = 0;
- ATSHash64Sip24 hash;
- HostRecord *hostRec = nullptr;
+ uint32_t cur_ring = 0; // there is a hash ring for each host group
+ uint32_t lookups = 0;
std::shared_ptr<HostRecord> pRec = nullptr;
HostStatus &pStatus = HostStatus::instance();
TSHostStatus host_stat = TSHostStatus::TS_HOST_STATUS_INIT;
HostStatRec *hst = nullptr;
Machine *machine = Machine::instance();
+ std::string_view first_call_host;
+ int first_call_port = 0;
- if (result->line_number == -1 && result->result == PARENT_UNDEFINED) {
+ if (result.line_number == -1 && result.result == PARENT_UNDEFINED) {
firstcall = true;
}
+ // firstcall indicates that this is the first time the state machine has called findNextHop() for this
+ // particular transaction so, a parent will be looked up using a hash from the request to locate a
+ // parent on the consistent hash ring. If not first call, then the transaction was unable to use the parent
+ // returned from the "firstcall" due to some error so, subsequent calls will not search using a hash but,
+ // will instead just increment the hash table iterator to find the next parent on the ring
if (firstcall) {
- NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] firstcall, line_number: %d, result: %s", sm_id, result->line_number,
- ParentResultStr[result->result]);
- result->line_number = distance;
- cur_ring = 0;
+ NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] firstcall, line_number: %d, result: %s", sm_id, result.line_number,
+ ParentResultStr[result.result]);
+ result.line_number = distance;
+ cur_ring = 0;
for (uint32_t i = 0; i < groups; i++) {
- result->chash_init[i] = false;
- wrap_around[i] = false;
+ result.chash_init[i] = false;
+ wrap_around[i] = false;
}
} else {
- NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] not firstcall, line_number: %d, result: %s", sm_id, result->line_number,
- ParentResultStr[result->result]);
+ // not first call, save the previously tried parent.
+ if (result.hostname) {
+ first_call_host = result.hostname;
+ first_call_port = result.port;
+ }
+ NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] not firstcall, line_number: %d, result: %s", sm_id, result.line_number,
+ ParentResultStr[result.result]);
switch (ring_mode) {
case NH_ALTERNATE_RING:
if (groups > 1) {
- cur_ring = (result->last_group + 1) % groups;
+ cur_ring = (result.last_group + 1) % groups;
} else {
- cur_ring = result->last_group;
+ cur_ring = result.last_group;
}
break;
case NH_PEERING_RING:
if (groups == 1) {
- result->last_group = cur_ring = NO_RING_USE_POST_REMAP;
+ result.last_group = cur_ring = NO_RING_USE_POST_REMAP;
} else {
ink_assert(groups == 2);
// look for the next parent on the
// upstream ring.
- result->last_group = cur_ring = 1;
+ result.last_group = cur_ring = 1;
}
break;
case NH_EXHAUST_RING:
default:
if (!wrapped) {
- cur_ring = result->last_group;
+ cur_ring = result.last_group;
} else if (groups > 1) {
- cur_ring = (result->last_group + 1) % groups;
+ cur_ring = (result.last_group + 1) % groups;
}
break;
}
}
if (cur_ring != NO_RING_USE_POST_REMAP) {
- // Do the initial parent look-up.
- hash_key = getHashKey(sm_id, &request_info, &hash);
+ do {
+ // all host groups have been searched and there are no available parents found
+ if (isWrapped(wrap_around, groups)) {
+ NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] No available parents.", sm_id);
+ pRec = nullptr;
+ break;
+ }
- do { // search until we've selected a different parent if !firstcall
+ // search for available parent
std::shared_ptr<ATSConsistentHash> r = rings[cur_ring];
- hostRec = chash_lookup(r, hash_key, &result->chashIter[cur_ring], &wrapped, &hash, &result->chash_init[cur_ring],
- &result->mapWrapped[cur_ring], sm_id);
- wrap_around[cur_ring] = wrapped;
+ pRec = chashLookup(r, cur_ring, result, request_info, &wrapped, sm_id);
+ hst = (pRec) ? pStatus.getHostStatus(pRec->hostname.c_str()) : nullptr;
+ wrap_around[cur_ring] = wrapped;
lookups++;
- // the 'available' flag is maintained in 'host_groups' and not the hash ring.
- if (hostRec) {
- pRec = host_groups[hostRec->group_index][hostRec->host_index];
+
+ // found a parent
+ if (pRec) {
+ bool is_self = machine->is_self(pRec->hostname.c_str());
+ host_stat = (hst) ? hst->status : TSHostStatus::TS_HOST_STATUS_UP;
+
+ // if the config ignore_self_detect is set to true and the host is down due to SELF_DETECT reason
+ // ignore the down status and mark it as available
+ if ((host_stat == TS_HOST_STATUS_DOWN && is_self && ignore_self_detect)) {
+ if (hst->reasons == Reason::SELF_DETECT) {
+ host_stat = TS_HOST_STATUS_UP;
+ }
+ }
+
if (firstcall) {
- hst = (pRec) ? pStatus.getHostStatus(pRec->hostname.c_str()) : nullptr;
- result->first_choice_status = (hst) ? hst->status : TSHostStatus::TS_HOST_STATUS_UP;
- // if peering and the selected host is myself, change rings and search for an upstream
- // parent.
- if (ring_mode == NH_PEERING_RING && (pRec->self || machine->is_self(pRec->hostname.c_str()))) {
+ result.first_choice_status = (hst) ? hst->status : TSHostStatus::TS_HOST_STATUS_UP;
+ // if peering and the selected host is myself, change rings and search for an upstream parent.
+ if (ring_mode == NH_PEERING_RING && (pRec->self || is_self)) {
if (groups == 1) {
// use host from post-remap URL
cur_ring = NO_RING_USE_POST_REMAP;
pRec = nullptr;
+ break;
} else {
- // switch to the upstream ring.
+ // switch to and search the upstream ring.
cur_ring = 1;
+ pRec = nullptr;
continue;
}
}
+ } else {
+ // not first call, make sure we're not re-using the same parent, search again if we are.
+ if (first_call_host.size() > 0 && first_call_host == pRec->hostname && first_call_port == pRec->getPort(scheme)) {
+ pRec = nullptr;
+ continue;
+ }
+ }
+ // if the parent is not available check to see if the retry window has expired making it available
+ // for retry.
+ 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;
+ }
+ }
+ }
+
+ // use the available selected parent
+ if (pRec->available.load() && host_stat == TS_HOST_STATUS_UP) {
break;
}
- } else {
- pRec = nullptr;
}
- } while (pRec && result->hostname && strcmp(pRec->hostname.c_str(), result->hostname) == 0);
-
- NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] Initial parent lookups: %d", sm_id, lookups);
-
- // ----------------------------------------------------------------------------------------------------
- // Validate initial parent look-up and perform additional look-ups if required.
- // ----------------------------------------------------------------------------------------------------
-
- if (cur_ring != NO_RING_USE_POST_REMAP) {
- hst = (pRec) ? pStatus.getHostStatus(pRec->hostname.c_str()) : nullptr;
- host_stat = (hst) ? hst->status : TSHostStatus::TS_HOST_STATUS_UP;
- // if the config ignore_self_detect is set to true and the host is down due to SELF_DETECT reason
- // ignore the down status and mark it as available
- if ((pRec && ignore_self_detect) && (hst && hst->status == TS_HOST_STATUS_DOWN)) {
- if (hst->reasons == Reason::SELF_DETECT) {
- host_stat = TS_HOST_STATUS_UP;
+ // try other rings per per the ring mode
+ switch (ring_mode) {
+ case NH_ALTERNATE_RING:
+ if (pRec && groups > 0) {
+ cur_ring = (pRec->group_index + 1) % groups;
+ } else {
+ cur_ring = 0;
+ }
+ break;
+ case NH_EXHAUST_RING:
+ default:
+ if (wrap_around[cur_ring] && groups > 1) {
+ cur_ring = (cur_ring + 1) % groups;
}
+ break;
}
- if (!pRec || (pRec && !pRec->available.load()) || host_stat == TS_HOST_STATUS_DOWN) {
- do {
- // check if an unavailable server is now retryable, use it.
- if (pRec && !pRec->available.load() && host_stat == TS_HOST_STATUS_UP) {
- // check if the host is retryable. It's retryable if the retry window has elapsed
- _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;
- }
- }
- }
- switch (ring_mode) {
- case NH_ALTERNATE_RING:
- if (pRec && groups > 0) {
- cur_ring = (pRec->group_index + 1) % groups;
- } else {
- cur_ring = 0;
- }
- break;
- case NH_EXHAUST_RING:
- default:
- if (wrap_around[cur_ring] && groups > 1) {
- cur_ring = (cur_ring + 1) % groups;
- }
- break;
- }
- std::shared_ptr<ATSConsistentHash> r = rings[cur_ring];
- hostRec = chash_lookup(r, hash_key, &result->chashIter[cur_ring], &wrapped, &hash, &result->chash_init[cur_ring],
- &result->mapWrapped[cur_ring], sm_id);
- wrap_around[cur_ring] = wrapped;
- lookups++;
- if (hostRec) {
- pRec = host_groups[hostRec->group_index][hostRec->host_index];
- hst = (pRec) ? pStatus.getHostStatus(pRec->hostname.c_str()) : nullptr;
- host_stat = (hst) ? hst->status : TSHostStatus::TS_HOST_STATUS_UP;
- // if the config ignore_self_detect is set to true and the host is down due to SELF_DETECT reason
- // ignore the down status and mark it as available
- if ((pRec && ignore_self_detect) && (hst && hst->status == TS_HOST_STATUS_DOWN)) {
- if (hst->reasons == Reason::SELF_DETECT) {
- host_stat = TS_HOST_STATUS_UP;
- }
- }
- NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] Selected a new parent: %s, available: %s, wrapped: %s, lookups: %d.", sm_id,
- pRec->hostname.c_str(), (pRec->available.load()) ? "true" : "false", (wrapped) ? "true" : "false", lookups);
- // use available host.
- if (pRec->available.load() && host_stat == TS_HOST_STATUS_UP) {
- break;
- }
- } else {
- pRec = nullptr;
- }
- bool all_wrapped = true;
- for (uint32_t c = 0; c < groups; c++) {
- if (wrap_around[c] == false) {
- all_wrapped = false;
- }
- }
- if (all_wrapped) {
- NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] No available parents.", sm_id);
- if (pRec) {
- pRec = nullptr;
- }
- break;
- }
- } while (!pRec || (pRec && !pRec->available.load()) || host_stat == TS_HOST_STATUS_DOWN);
+
+ if (pRec) {
+ // if the selected host is down, search again.
+ if (!pRec->available || host_stat == TS_HOST_STATUS_DOWN) {
+ NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] hostname: %s, available: %s, host_stat: %s", sm_id, pRec->hostname.c_str(),
+ pRec->available ? "true" : "false", HostStatusNames[host_stat]);
+ pRec = nullptr;
+ continue;
+ }
}
- }
+ } while (!pRec);
+
+ NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] Initial parent lookups: %d", sm_id, lookups);
}
// ----------------------------------------------------------------------------------------------------
// Validate and return the final result.
// ----------------------------------------------------------------------------------------------------
- if (pRec && host_stat == TS_HOST_STATUS_UP && (pRec->available.load() || result->retry)) {
- result->result = PARENT_SPECIFIED;
- result->hostname = pRec->hostname.c_str();
- result->last_parent = pRec->host_index;
- result->last_lookup = result->last_group = cur_ring;
+ if (pRec && host_stat == TS_HOST_STATUS_UP && (pRec->available.load() || result.retry)) {
+ result.result = PARENT_SPECIFIED;
+ result.hostname = pRec->hostname.c_str();
+ result.last_parent = pRec->host_index;
+ result.last_lookup = result.last_group = cur_ring;
switch (scheme) {
case NH_SCHEME_NONE:
case NH_SCHEME_HTTP:
- result->port = pRec->getPort(scheme);
+ result.port = pRec->getPort(scheme);
break;
case NH_SCHEME_HTTPS:
- result->port = pRec->getPort(scheme);
+ result.port = pRec->getPort(scheme);
break;
}
- result->retry = nextHopRetry;
+ result.retry = nextHopRetry;
// if using a peering ring mode and the parent selected came from the 'peering' group,
// cur_ring == 0, then if the config allows it, set the flag to not cache the result.
if (ring_mode == NH_PEERING_RING && !cache_peer_result && cur_ring == 0) {
- result->do_not_cache_response = true;
+ result.do_not_cache_response = true;
NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] setting do not cache response from a peer per config: %s", sm_id,
- (result->do_not_cache_response) ? "true" : "false");
+ (result.do_not_cache_response) ? "true" : "false");
}
- ink_assert(result->hostname != nullptr);
- ink_assert(result->port != 0);
- NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] result->result: %s Chosen parent: %s.%d", sm_id, ParentResultStr[result->result],
- result->hostname, result->port);
+ ink_assert(result.hostname != nullptr);
+ ink_assert(result.port != 0);
+ NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] result->result: %s Chosen parent: %s.%d", sm_id, ParentResultStr[result.result],
+ result.hostname, result.port);
} else {
if (go_direct == true) {
- result->result = PARENT_DIRECT;
+ result.result = PARENT_DIRECT;
} else {
- result->result = PARENT_FAIL;
+ result.result = PARENT_FAIL;
}
- result->hostname = nullptr;
- result->port = 0;
- result->retry = false;
- NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] result->result: %s set hostname null port 0 retry false", sm_id,
- ParentResultStr[result->result]);
+ result.hostname = nullptr;
+ result.port = 0;
+ result.retry = false;
+ NH_Debug(NH_DEBUG_TAG, "[%" PRIu64 "] result.result: %s set hostname null port 0 retry false", sm_id,
+ ParentResultStr[result.result]);
}
return;
diff --git a/proxy/http/remap/NextHopConsistentHash.h b/proxy/http/remap/NextHopConsistentHash.h
index c2adf12..7802fdc 100644
--- a/proxy/http/remap/NextHopConsistentHash.h
+++ b/proxy/http/remap/NextHopConsistentHash.h
@@ -25,6 +25,7 @@
#include <map>
#include <vector>
+#include "tscore/HashSip.h"
#include "NextHopSelectionStrategy.h"
enum NHHashKeyType {
@@ -40,7 +41,7 @@ class NextHopConsistentHash : public NextHopSelectionStrategy
{
std::vector<std::shared_ptr<ATSConsistentHash>> rings;
- uint64_t getHashKey(uint64_t sm_id, HttpRequestData *hrdata, ATSHash64 *h);
+ uint64_t getHashKey(uint64_t sm_id, const HttpRequestData &hrdata, ATSHash64 *h);
public:
NHHashKeyType hash_key = NH_PATH_HASH_KEY;
@@ -50,4 +51,6 @@ public:
~NextHopConsistentHash();
bool Init(ts::Yaml::Map &n);
void findNextHop(TSHttpTxn txnp, void *ih = nullptr, time_t now = 0) override;
+ std::shared_ptr<HostRecord> chashLookup(const std::shared_ptr<ATSConsistentHash> &ring, uint32_t cur_ring, ParentResult &result,
+ HttpRequestData &request_info, bool *wrapped, uint64_t sm_id);
};
diff --git a/proxy/http/remap/NextHopHealthStatus.cc b/proxy/http/remap/NextHopHealthStatus.cc
index 58490aa..19175d0 100644
--- a/proxy/http/remap/NextHopHealthStatus.cc
+++ b/proxy/http/remap/NextHopHealthStatus.cc
@@ -119,22 +119,21 @@ NextHopHealthStatus::markNextHop(TSHttpTxn txn, const char *hostname, const int
new_fail_count = h->failCount = 1;
}
} else if (result.retry == true) {
- h->failedAt = _now;
+ h->failedAt = _now;
+ new_fail_count = h->failCount += 1;
}
} // end lock guard
NH_Note("[%" PRId64 "] NextHop %s marked as down %s", sm_id, (result.retry) ? "retry" : "initially", h->hostname.c_str());
} else {
- int old_count = 0;
// if the last failure was outside the retry window, set the failcount to 1 and failedAt to now.
{ // lock guard
std::lock_guard<std::mutex> lock(h->_mutex);
if ((h->failedAt.load() + retry_time) < static_cast<unsigned>(_now)) {
- h->failCount = 1;
- h->failedAt = _now;
+ new_fail_count = h->failCount = 1;
+ h->failedAt = _now;
} else {
- old_count = h->failCount = 1;
+ new_fail_count = h->failCount += 1;
}
- new_fail_count = old_count + 1;
} // end of lock_guard
NH_Debug(NH_DEBUG_TAG, "[%" PRId64 "] Parent fail count increased to %d for %s", sm_id, new_fail_count, h->hostname.c_str());
}
diff --git a/proxy/http/remap/unit-tests/test_NextHopConsistentHash.cc b/proxy/http/remap/unit-tests/test_NextHopConsistentHash.cc
index ad4f0d2..460d648 100644
--- a/proxy/http/remap/unit-tests/test_NextHopConsistentHash.cc
+++ b/proxy/http/remap/unit-tests/test_NextHopConsistentHash.cc
@@ -94,7 +94,7 @@ SCENARIO("Testing NextHopConsistentHash class, using policy 'consistent_hash'",
result->reset();
strategy->findNextHop(txnp);
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "p1.foo.com") == 0);
// mark down p1.foo.com. markNextHop looks at the 'result'
@@ -107,7 +107,7 @@ SCENARIO("Testing NextHopConsistentHash class, using policy 'consistent_hash'",
build_request(10002, &sm, nullptr, "rabbit.net", nullptr);
strategy->findNextHop(txnp);
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "p2.foo.com") == 0);
// mark down p2.foo.com
@@ -118,7 +118,7 @@ SCENARIO("Testing NextHopConsistentHash class, using policy 'consistent_hash'",
build_request(10003, &sm, nullptr, "rabbit.net", nullptr);
strategy->findNextHop(txnp);
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "s2.bar.com") == 0);
// mark down s2.bar.com
@@ -129,7 +129,7 @@ SCENARIO("Testing NextHopConsistentHash class, using policy 'consistent_hash'",
build_request(10004, &sm, nullptr, "rabbit.net", nullptr);
strategy->findNextHop(txnp);
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "s1.bar.com") == 0);
// mark down s1.bar.com.
@@ -140,7 +140,7 @@ SCENARIO("Testing NextHopConsistentHash class, using policy 'consistent_hash'",
build_request(10005, &sm, nullptr, "rabbit.net", nullptr);
strategy->findNextHop(txnp);
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "q1.bar.com") == 0);
// mark down q1.bar.com
@@ -150,7 +150,7 @@ SCENARIO("Testing NextHopConsistentHash class, using policy 'consistent_hash'",
build_request(10006, &sm, nullptr, "rabbit.net", nullptr);
strategy->findNextHop(txnp);
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "q2.bar.com") == 0);
// mark down q2.bar.com
@@ -170,7 +170,7 @@ SCENARIO("Testing NextHopConsistentHash class, using policy 'consistent_hash'",
// simulating a failure triggers a search for another parent, not firstcall.
build_request(10008, &sm, nullptr, "rabbit.net", nullptr);
strategy->findNextHop(txnp, nullptr, now);
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "q2.bar.com") == 0);
// free up request resources.
@@ -225,7 +225,7 @@ SCENARIO("Testing NextHopConsistentHash class (all firstcalls), using policy 'co
build_request(20001, &sm, nullptr, "rabbit.net", nullptr);
result->reset();
strategy->findNextHop(txnp);
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "p1.foo.com") == 0);
// mark down p1.foo.com
@@ -234,7 +234,7 @@ SCENARIO("Testing NextHopConsistentHash class (all firstcalls), using policy 'co
build_request(20002, &sm, nullptr, "rabbit.net", nullptr);
result->reset();
strategy->findNextHop(txnp);
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "p2.foo.com") == 0);
// mark down p2.foo.com
@@ -244,7 +244,7 @@ SCENARIO("Testing NextHopConsistentHash class (all firstcalls), using policy 'co
result->reset();
build_request(20003, &sm, nullptr, "rabbit.net", nullptr);
strategy->findNextHop(txnp);
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "s2.bar.com") == 0);
// mark down s2.bar.com
@@ -254,7 +254,7 @@ SCENARIO("Testing NextHopConsistentHash class (all firstcalls), using policy 'co
result->reset();
build_request(20004, &sm, nullptr, "rabbit.net", nullptr);
strategy->findNextHop(txnp);
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "s1.bar.com") == 0);
// mark down s1.bar.com
@@ -264,7 +264,7 @@ SCENARIO("Testing NextHopConsistentHash class (all firstcalls), using policy 'co
result->reset();
build_request(20005, &sm, nullptr, "rabbit.net/asset1", nullptr);
strategy->findNextHop(txnp);
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "q1.bar.com") == 0);
// sixth request - wait and p1 should now become available
@@ -272,7 +272,7 @@ SCENARIO("Testing NextHopConsistentHash class (all firstcalls), using policy 'co
result->reset();
build_request(20006, &sm, nullptr, "rabbit.net", nullptr);
strategy->findNextHop(txnp, nullptr, now);
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "p1.foo.com") == 0);
}
// free up request resources.
@@ -372,7 +372,7 @@ SCENARIO("Testing NextHop ignore_self_detect true", "[NextHopConsistentHash]")
build_request(10001, &sm, nullptr, "rabbit.net", nullptr);
result->reset();
strategy->findNextHop(txnp);
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "localhost") == 0);
CHECK(result->port == 8000);
br_destroy(sm);
@@ -422,7 +422,7 @@ SCENARIO("Testing NextHopConsistentHash same host different port markdown", "[Ne
result->reset();
strategy->findNextHop(txnp);
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "localhost") == 0);
CHECK(result->port == 8000);
@@ -431,7 +431,7 @@ SCENARIO("Testing NextHopConsistentHash same host different port markdown", "[Ne
build_request(10002, &sm, nullptr, "rabbit.net", nullptr);
strategy->findNextHop(txnp);
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "localhost") == 0);
CHECK(result->port == 8002);
@@ -440,7 +440,7 @@ SCENARIO("Testing NextHopConsistentHash same host different port markdown", "[Ne
build_request(10003, &sm, nullptr, "rabbit.net", nullptr);
strategy->findNextHop(txnp);
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "localhost") == 0);
CHECK(result->port == 8004);
br_destroy(sm);
@@ -492,7 +492,7 @@ SCENARIO("Testing NextHopConsistentHash hash_string override", "[NextHopConsiste
// We happen to know that 'foo.test' will be first if the hostname is the hash
// and foo.test will be first for the hash 'first' and the bar.test hash 'second'.
// So, if the hash_string override isn't getting applied, this will fail.
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "bar.test") == 0);
CHECK(result->port == 80);
@@ -501,6 +501,7 @@ SCENARIO("Testing NextHopConsistentHash hash_string override", "[NextHopConsiste
build_request(10002, &sm, nullptr, "rabbit.net", nullptr);
strategy->findNextHop(txnp);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "foo.test") == 0);
CHECK(result->port == 80);
br_destroy(sm);
@@ -550,7 +551,7 @@ SCENARIO("Testing NextHopConsistentHash class (alternating rings), using policy
build_request(30001, &sm, nullptr, "bunny.net/asset1", nullptr);
result->reset();
strategy->findNextHop(txnp);
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "c2.foo.com") == 0);
// simulated failure, mark c2 down and retry request
@@ -559,7 +560,7 @@ SCENARIO("Testing NextHopConsistentHash class (alternating rings), using policy
// second request
build_request(30002, &sm, nullptr, "bunny.net.net/asset1", nullptr);
strategy->findNextHop(txnp);
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "c3.bar.com") == 0);
// mark down c3.bar.com
@@ -569,7 +570,7 @@ SCENARIO("Testing NextHopConsistentHash class (alternating rings), using policy
build_request(30003, &sm, nullptr, "bunny.net/asset2", nullptr);
result->reset();
strategy->findNextHop(txnp);
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "c6.bar.com") == 0);
// just mark it down and retry request
@@ -577,7 +578,7 @@ SCENARIO("Testing NextHopConsistentHash class (alternating rings), using policy
// fourth request
build_request(30004, &sm, nullptr, "bunny.net/asset2", nullptr);
strategy->findNextHop(txnp);
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "c1.foo.com") == 0);
// mark it down
@@ -586,7 +587,7 @@ SCENARIO("Testing NextHopConsistentHash class (alternating rings), using policy
build_request(30005, &sm, nullptr, "bunny.net/asset3", nullptr);
result->reset();
strategy->findNextHop(txnp);
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "c4.bar.com") == 0);
// mark it down and retry
@@ -595,7 +596,7 @@ SCENARIO("Testing NextHopConsistentHash class (alternating rings), using policy
result->reset();
build_request(30006, &sm, nullptr, "bunny.net/asset3", nullptr);
strategy->findNextHop(txnp);
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "c5.bar.com") == 0);
// mark it down
@@ -604,7 +605,7 @@ SCENARIO("Testing NextHopConsistentHash class (alternating rings), using policy
result->reset();
build_request(30007, &sm, nullptr, "bunny.net/asset4", nullptr);
strategy->findNextHop(txnp);
- CHECK(result->result == ParentResultType::PARENT_FAIL);
+ REQUIRE(result->result == ParentResultType::PARENT_FAIL);
CHECK(result->hostname == nullptr);
// eighth request - retry after waiting for the retry window to expire.
@@ -612,7 +613,7 @@ SCENARIO("Testing NextHopConsistentHash class (alternating rings), using policy
result->reset();
build_request(30008, &sm, nullptr, "bunny.net/asset4", nullptr);
strategy->findNextHop(txnp, nullptr, now);
- CHECK(result->result == ParentResultType::PARENT_SPECIFIED);
+ REQUIRE(result->result == ParentResultType::PARENT_SPECIFIED);
CHECK(strcmp(result->hostname, "c2.foo.com") == 0);
}
// free up request resources.