[trafficserver] 08/12: 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.

[zw...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

zwoop pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git

commit 0c9f08c2b6f66e7afc8229e41211f16994673936
Author: John Rushford <jr...@apache.org>
AuthorDate: Mon Jan 6 17:51:16 2020 +0000

    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.
    
    (cherry picked from commit 62e4cb18d61b77558a7c2e4fba5ac61f12451be7)
---
 proxy/http/remap/NextHopConsistentHash.cc                 | 13 ++++++++++---
 proxy/http/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 ca90038..f36605c 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<ATSConsistentHash> 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<ATSConsistentHash> ring, uint64_t hash_key, ATSCons
   } else {
     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;
+  }
 
   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<ATSConsistentHash> 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<ATSConsistentHash> 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 cbd29bd..08f1179 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;