outlier-detection: Eject always failing nodes on success rate ejection

Author: incubos

util/src/main/java/io/grpc/util/OutlierDetectionLoadBalancer.java

@@ -569,8 +569,12 @@ long inactiveVolume() {
       return inactiveCallCounter.successCount.get() + inactiveCallCounter.failureCount.get();
     }
 
+    long inactiveSuccessCount() {
+      return inactiveCallCounter.successCount.get();
+    }
+
     double successRate() {
-      return ((double) inactiveCallCounter.successCount.get()) / inactiveVolume();
+      return ((double) inactiveSuccessCount()) / inactiveVolume();
     }
 
     double failureRate() {
@@ -817,6 +821,7 @@ public void ejectOutliers(EndpointTrackerMap trackerMap, long ejectionTimeNanos)
       double mean = mean(successRates);
       double stdev = standardDeviation(successRates, mean);
 
+      // Might be non-positive.
       double requiredSuccessRate =
           mean - stdev * (config.successRateEjection.stdevFactor / 1000f);
 
@@ -829,8 +834,8 @@ public void ejectOutliers(EndpointTrackerMap trackerMap, long ejectionTimeNanos)
           return;
         }
 
-        // If success rate is below the threshold, eject the address.
-        if (tracker.successRate() < requiredSuccessRate) {
+        // If success rate is zero or below the threshold, eject the address.
+        if (tracker.inactiveSuccessCount() == 0L || tracker.successRate() < requiredSuccessRate) {
           logger.log(ChannelLogLevel.DEBUG,
                   "SuccessRate algorithm detected outlier: {0}. "
                           + "Parameters: successRate={1}, mean={2}, stdev={3}, "

util/src/test/java/io/grpc/util/OutlierDetectionLoadBalancerTest.java

@@ -724,6 +724,35 @@ public void successRateTwoOutliers() {
         ImmutableSet.of(servers.get(1).getAddresses().get(0))));
   }
 
+  /**
+   * Two outliers get ejected with default stdevFactor.
+   */
+  @Test
+  public void successRateTwoOutliersWithDefaultStdevFactor() {
+    OutlierDetectionLoadBalancerConfig config = new OutlierDetectionLoadBalancerConfig.Builder()
+            .setMaxEjectionPercent(50)
+            .setSuccessRateEjection(
+                    // Default stdevFactor results in negative requiredSuccessRate,
+                    // but always failing endpoints still get ejected.
+                    new SuccessRateEjection.Builder()
+                            .setMinimumHosts(3)
+                            .setRequestVolume(10).build())
+            .setChildConfig(newChildConfig(roundRobinLbProvider, null)).build();
+
+    loadBalancer.acceptResolvedAddresses(buildResolvedAddress(config, servers));
+
+    generateLoad(ImmutableMap.of(
+            subchannel1, Status.DEADLINE_EXCEEDED,
+            subchannel2, Status.DEADLINE_EXCEEDED), 7);
+
+    // Move forward in time to a point where the detection timer has fired.
+    forwardTime(config);
+
+    // The one subchannel that was returning errors should be ejected.
+    assertEjectedSubchannels(ImmutableSet.of(ImmutableSet.of(servers.get(0).getAddresses().get(0)),
+            ImmutableSet.of(servers.get(1).getAddresses().get(0))));
+  }
+
   /**
    * Three outliers, second one ejected even if ejecting it goes above the max ejection percentage,
    * as this matches Envoy behavior. The third one should not get ejected.