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

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

Author: temawi

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.
  */