core: outlier detection to min host request volume (#9490) (#9551)

When doing the failure percentage algorithm, ejections should be skipped
if there are not enough addresses that meet the minimum volume
requirement.

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

@@ -713,7 +713,8 @@ static class SuccessRateOutlierEjectionAlgorithm implements OutlierEjectionAlgor
     public void ejectOutliers(AddressTrackerMap trackerMap, long ejectionTimeNanos) {
 
       // Only consider addresses that have the minimum request volume specified in the config.
-      List<AddressTracker> trackersWithVolume = trackersWithVolume(trackerMap, config);
+      List<AddressTracker> trackersWithVolume = trackersWithVolume(trackerMap,
+          config.successRateEjection.requestVolume);
       // If we don't have enough addresses with significant volume then there's nothing to do.
       if (trackersWithVolume.size() < config.successRateEjection.minimumHosts
           || trackersWithVolume.size() == 0) {
@@ -749,18 +750,6 @@ public void ejectOutliers(AddressTrackerMap trackerMap, long ejectionTimeNanos)
       }
     }
 
-    /** Returns only the trackers that have the minimum configured volume to be considered. */
-    private List<AddressTracker> trackersWithVolume(AddressTrackerMap trackerMap,
-        OutlierDetectionLoadBalancerConfig config) {
-      List<AddressTracker> trackersWithVolume = new ArrayList<>();
-      for (AddressTracker tracker : trackerMap.values()) {
-        if (tracker.inactiveVolume() >= config.successRateEjection.requestVolume) {
-          trackersWithVolume.add(tracker);
-        }
-      }
-      return trackersWithVolume;
-    }
-
     /** Calculates the mean of the given values. */
     @VisibleForTesting
     static double mean(Collection<Double> values) {
@@ -797,13 +786,17 @@ static class FailurePercentageOutlierEjectionAlgorithm implements OutlierEjectio
     @Override
     public void ejectOutliers(AddressTrackerMap trackerMap, long ejectionTimeNanos) {
 
-      // If we don't have the minimum amount of addresses the config calls for, then return.
-      if (trackerMap.size() < config.failurePercentageEjection.minimumHosts) {
+      // Only consider addresses that have the minimum request volume specified in the config.
+      List<AddressTracker> trackersWithVolume = trackersWithVolume(trackerMap,
+          config.failurePercentageEjection.requestVolume);
+      // If we don't have enough addresses with significant volume then there's nothing to do.
+      if (trackersWithVolume.size() < config.failurePercentageEjection.minimumHosts
+          || trackersWithVolume.size() == 0) {
         return;
       }
 
       // If this address does not have enough volume to be considered, skip to the next one.
-      for (AddressTracker tracker : trackerMap.values()) {
+      for (AddressTracker tracker : trackersWithVolume) {
         // If we are above the max ejection percentage, don't eject any more. This will allow the
         // total ejections to go one above the max, but at the same time it assures at least one
         // ejection, which the spec calls for. This behavior matches what Envoy proxy does.
@@ -827,6 +820,18 @@ public void ejectOutliers(AddressTrackerMap trackerMap, long ejectionTimeNanos)
     }
   }
 
+  /** Returns only the trackers that have the minimum configured volume to be considered. */
+  private static List<AddressTracker> trackersWithVolume(AddressTrackerMap trackerMap,
+      int volume) {
+    List<AddressTracker> trackersWithVolume = new ArrayList<>();
+    for (AddressTracker tracker : trackerMap.values()) {
+      if (tracker.inactiveVolume() >= volume) {
+        trackersWithVolume.add(tracker);
+      }
+    }
+    return trackersWithVolume;
+  }
+
   /** Counts how many addresses are in a given address group. */
   private static boolean hasSingleAddress(List<EquivalentAddressGroup> addressGroups) {
     int addressCount = 0;

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

@@ -496,21 +496,26 @@ public void successRateOneOutlier_notEnoughVolume() {
   }
 
   /**
-   * The success rate algorithm does not apply if enough addresses have the required volume.
+   * The success rate algorithm does not apply if we don't have enough addresses that have the
+   * required volume.
    */
   @Test
   public void successRateOneOutlier_notEnoughAddressesWithVolume() {
     OutlierDetectionLoadBalancerConfig config = new OutlierDetectionLoadBalancerConfig.Builder()
         .setMaxEjectionPercent(50)
         .setSuccessRateEjection(
             new SuccessRateEjection.Builder()
-                .setMinimumHosts(6) // We don't have this many hosts...
-                .setRequestVolume(10).build())
+                .setMinimumHosts(5)
+                .setRequestVolume(20).build())
         .setChildPolicy(new PolicySelection(roundRobinLbProvider, null)).build();
 
     loadBalancer.handleResolvedAddresses(buildResolvedAddress(config, servers));
 
-    generateLoad(ImmutableMap.of(subchannel1, Status.DEADLINE_EXCEEDED), 7);
+    generateLoad(
+        ImmutableMap.of(subchannel1, Status.DEADLINE_EXCEEDED),
+        // subchannel2 has only 19 calls which results in success rate not triggering.
+        ImmutableMap.of(subchannel2, 19),
+        7);
 
     // Move forward in time to a point where the detection timer has fired.
     forwardTime(config);
@@ -682,6 +687,35 @@ public void failurePercentageOneOutlier_notEnoughVolume() {
     assertEjectedSubchannels(ImmutableSet.of());
   }
 
+  /**
+   * The failure percentage algorithm does not apply if we don't have enough addresses that have the
+   * required volume.
+   */
+  @Test
+  public void failurePercentageOneOutlier_notEnoughAddressesWithVolume() {
+    OutlierDetectionLoadBalancerConfig config = new OutlierDetectionLoadBalancerConfig.Builder()
+        .setMaxEjectionPercent(50)
+        .setFailurePercentageEjection(
+            new FailurePercentageEjection.Builder()
+                .setMinimumHosts(5)
+                .setRequestVolume(20).build())
+        .setChildPolicy(new PolicySelection(roundRobinLbProvider, null)).build();
+
+    loadBalancer.handleResolvedAddresses(buildResolvedAddress(config, servers));
+
+    generateLoad(
+        ImmutableMap.of(subchannel1, Status.DEADLINE_EXCEEDED),
+        // subchannel2 has only 19 calls which results in failure percentage not triggering.
+        ImmutableMap.of(subchannel2, 19),
+        7);
+
+    // Move forward in time to a point where the detection timer has fired.
+    forwardTime(config);
+
+    // No subchannels should have been ejected.
+    assertEjectedSubchannels(ImmutableSet.of());
+  }
+
   /**
    * The enforcementPercentage configuration should be honored.
    */