Small changes after code review.

cpakulski · cpakulski · commit fe83128004ec · 2020-01-15T12:28:28.000-05:00
Added unit tests to make sure that excluded_hosts are not
taken into account when calculating load in TotalPanic mode.

Signed-off-by: Christoph Pakulski &lt;paker8848@gmail.com&gt;
diff --git a/source/common/upstream/load_balancer_impl.cc b/source/common/upstream/load_balancer_impl.cc
@@ -291,15 +291,22 @@ void LoadBalancerBase::recalculateLoadInTotalPanic() {
   uint32_t total_load = 100;
   for (size_t i = 0; i < per_priority_panic_.size(); i++) {
     const HostSet& host_set = *priority_set_.hostSetsPerPriority()[i];
-    auto hosts_num = host_set.hosts().size() - host_set.excludedHosts().size();
+    const auto hosts_num = host_set.hosts().size();
 
-    uint32_t priority_load = 100 * hosts_num / total_hosts_num;
+    const uint32_t priority_load = 100 * hosts_num / total_hosts_num;
     per_priority_load_.healthy_priority_load_.get()[i] = priority_load;
     per_priority_load_.degraded_priority_load_.get()[i] = 0;
     total_load -= priority_load;
   }
-  // Add to priority zero whatever if left.
-  per_priority_load_.healthy_priority_load_.get()[0] += total_load;
+
+  // Find the first priority which is not empty and add remaining load.
+  for (size_t i = 0; i < per_priority_panic_.size(); i++) {
+    const HostSet& host_set = *priority_set_.hostSetsPerPriority()[i];
+    if (0 != host_set.hosts().size()) {
+      per_priority_load_.healthy_priority_load_.get()[i] += total_load;
+      break;
+    }
+  }
 }
 
 std::pair<HostSet&, LoadBalancerBase::HostAvailability>
diff --git a/test/common/upstream/load_balancer_impl_test.cc b/test/common/upstream/load_balancer_impl_test.cc
@@ -86,7 +86,6 @@ class LoadBalancerBaseTest : public LoadBalancerTestBase {
     }
 
     for (; i < (num_healthy_hosts + num_degraded_hosts + num_excluded_hosts); ++i) {
-      host_set.degraded_hosts_.push_back(host_set.hosts_[i]);
       host_set.excluded_hosts_.push_back(host_set.hosts_[i]);
     }
     host_set.runCallbacks({}, {});
@@ -310,7 +309,6 @@ TEST_P(LoadBalancerBaseTest, GentleFailover) {
   ASSERT_THAT(getPanic(), ElementsAre(false, false));
 
   // Health P=0 == 100*1.4 == 35 P=1 == 35
-  // Since 4 hosts are excluded and are unhealthy, P=0 should be considered fully unavailable.
   // Total health = 35% is less than 100%.
   // All priorities are in panic mode (situation called TotalPanic)
   // Load is distributed based on number of hosts regardless of their health status.
@@ -320,6 +318,19 @@ TEST_P(LoadBalancerBaseTest, GentleFailover) {
   updateHostSet(failover_host_set_, 4 /* num_hosts */, 1 /* num_healthy_hosts */);
   ASSERT_THAT(getLoadPercentage(), ElementsAre(50, 50));
   ASSERT_THAT(getPanic(), ElementsAre(true, true));
+
+  // Make sure that in TotalPanic mode (all levels are in Panic),
+  // load distribution depends only on number of hosts.
+  // excluded_hosts should not be taken into account.
+  // P=0 has 4 hosts with 1 excluded, P=1 has 6 hosts with 2 excluded.
+  // P=0 should receive 4/(4+6)=40% of traffic
+  // P=1 should receive 6/(4+6)=60% of traffic
+  updateHostSet(host_set_, 4 /* num_hosts */, 0 /* num_healthy_hosts */, 0 /* num_degraded_hosts */,
+                1 /* num_excluded_hosts */);
+  updateHostSet(failover_host_set_, 6 /* num_hosts */, 1 /* num_healthy_hosts */,
+                0 /* num_degraded_hosts */, 2 /* num_excluded_hosts */);
+  ASSERT_THAT(getLoadPercentage(), ElementsAre(40, 60));
+  ASSERT_THAT(getPanic(), ElementsAre(true, true));
 }
 
 TEST_P(LoadBalancerBaseTest, GentleFailoverWithExtraLevels) {
