From 2290ff14aadec4ae62afc86d2429b0ddae74dbd0 Mon Sep 17 00:00:00 2001 From: John Rushford Date: Mon, 6 Jan 2020 17:51:16 +0000 Subject: [PATCH] Fixes a corner case where the NextHop consistent hash ring may not be searched in it's entirety for an available host due to a premature wrapped ring indication. --- proxy/http/remap/NextHopConsistentHash.cc | 13 ++++++++++--- .../remap/unit-tests/test_NextHopConsistentHash.cc | 8 ++++---- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/proxy/http/remap/NextHopConsistentHash.cc b/proxy/http/remap/NextHopConsistentHash.cc index ca900385e1f..f36605cab54 100644 --- a/proxy/http/remap/NextHopConsistentHash.cc +++ b/proxy/http/remap/NextHopConsistentHash.cc @@ -36,7 +36,7 @@ constexpr std::string_view hash_key_cache = "cache_key"; static HostRecord * chash_lookup(std::shared_ptr ring, uint64_t hash_key, ATSConsistentHashIter *iter, bool *wrapped, - ATSHash64Sip24 *hash, bool *hash_init, uint64_t sm_id) + ATSHash64Sip24 *hash, bool *hash_init, bool *mapWrapped, uint64_t sm_id) { HostRecord *host_rec = nullptr; @@ -46,6 +46,11 @@ chash_lookup(std::shared_ptr ring, uint64_t hash_key, ATSCons } else { host_rec = static_cast(ring->lookup(nullptr, iter, wrapped, hash)); } + bool wrap_around = *wrapped; + *wrapped = (*mapWrapped && *wrapped) ? true : false; + if (!*mapWrapped && wrap_around) { + *mapWrapped = true; + } return host_rec; } @@ -261,7 +266,8 @@ NextHopConsistentHash::findNextHop(const uint64_t sm_id, ParentResult &result, R do { // search until we've selected a different parent if !firstcall std::shared_ptr r = rings[cur_ring]; - hostRec = chash_lookup(r, hash_key, &result.chashIter[cur_ring], &wrapped, &hash, &result.chash_init[cur_ring], sm_id); + 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++; // the 'available' flag is maintained in 'host_groups' and not the hash ring. @@ -322,7 +328,8 @@ NextHopConsistentHash::findNextHop(const uint64_t sm_id, ParentResult &result, R break; } std::shared_ptr r = rings[cur_ring]; - hostRec = chash_lookup(r, hash_key, &result.chashIter[cur_ring], &wrapped, &hash, &result.chash_init[cur_ring], sm_id); + 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) { diff --git a/proxy/http/remap/unit-tests/test_NextHopConsistentHash.cc b/proxy/http/remap/unit-tests/test_NextHopConsistentHash.cc index cbd29bdd30c..08f11794383 100644 --- a/proxy/http/remap/unit-tests/test_NextHopConsistentHash.cc +++ b/proxy/http/remap/unit-tests/test_NextHopConsistentHash.cc @@ -257,17 +257,17 @@ SCENARIO("Testing NextHopConsistentHash class (all firstcalls), using policy 'co result.reset(); strategy->findNextHop(20004, result, request, fail_threshold, retry_time); CHECK(result.result == ParentResultType::PARENT_SPECIFIED); - CHECK(strcmp(result.hostname, "q1.bar.com") == 0); + CHECK(strcmp(result.hostname, "s1.bar.com") == 0); - // mark down q1.bar.com + // mark down s1.bar.com strategy->markNextHopDown(20004, result, 1, fail_threshold); // fifth request br(&request, "rabbit.net/asset1"); result.reset(); strategy->findNextHop(20005, result, request, fail_threshold, retry_time); - CHECK(result.result == ParentResultType::PARENT_DIRECT); - CHECK(result.hostname == nullptr); + CHECK(result.result == ParentResultType::PARENT_SPECIFIED); + CHECK(strcmp(result.hostname, "q1.bar.com") == 0); // sixth request - wait and p1 should now become available time_t now = time(nullptr) + 5;