core: Update outlier detection max ejection logic. (1.49.x backport)

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

@@ -654,10 +654,9 @@ void maybeUnejectOutliers(Long detectionTimerStartNanos) {
     }
 
     /**
-     * How many percent of the addresses would have their subchannels ejected if we proceeded
-     * with the next ejection.
+     * How many percent of the addresses have been ejected.
      */
-    double nextEjectionPercentage() {
+    double ejectionPercentage() {
       if (trackerMap.isEmpty()) {
         return 0;
       }
@@ -669,7 +668,7 @@ void maybeUnejectOutliers(Long detectionTimerStartNanos) {
           ejectedAddresses++;
         }
       }
-      return ((double)(ejectedAddresses + 1) / totalAddresses) * 100;
+      return ((double)ejectedAddresses / totalAddresses) * 100;
     }
   }
 
@@ -733,8 +732,10 @@ public void ejectOutliers(AddressTrackerMap trackerMap, long ejectionTimeNanos)
           mean - stdev * (config.successRateEjection.stdevFactor / 1000f);
 
       for (AddressTracker tracker : trackersWithVolume) {
-        // If an ejection now would take us past the max configured ejection percentage, stop here.
-        if (trackerMap.nextEjectionPercentage() > config.maxEjectionPercent) {
+        // 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.
+        if (trackerMap.ejectionPercentage() > config.maxEjectionPercent) {
           return;
         }
 
@@ -803,8 +804,10 @@ public void ejectOutliers(AddressTrackerMap trackerMap, long ejectionTimeNanos)
 
       // If this address does not have enough volume to be considered, skip to the next one.
       for (AddressTracker tracker : trackerMap.values()) {
-        // If an ejection now would take us past the max configured ejection percentage stop here.
-        if (trackerMap.nextEjectionPercentage() > config.maxEjectionPercent) {
+        // 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.
+        if (trackerMap.ejectionPercentage() > config.maxEjectionPercent) {
           return;
         }
 

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

@@ -574,10 +574,11 @@ public void successRateTwoOutliers() {
   }
 
   /**
-   * Two outliers. but only one gets ejected because we have reached the max ejection percentage.
+   * 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.
    */
   @Test
-  public void successRateTwoOutliers_maxEjectionPercentage() {
+  public void successRateThreeOutliers_maxEjectionPercentage() {
     OutlierDetectionLoadBalancerConfig config = new OutlierDetectionLoadBalancerConfig.Builder()
         .setMaxEjectionPercent(20)
         .setSuccessRateEjection(
@@ -591,7 +592,8 @@ public void successRateTwoOutliers_maxEjectionPercentage() {
 
     generateLoad(ImmutableMap.of(
         subchannel1, Status.DEADLINE_EXCEEDED,
-        subchannel2, Status.DEADLINE_EXCEEDED), 7);
+        subchannel2, Status.DEADLINE_EXCEEDED,
+        subchannel3, Status.DEADLINE_EXCEEDED), 7);
 
     // Move forward in time to a point where the detection timer has fired.
     forwardTime(config);
@@ -603,10 +605,7 @@ public void successRateTwoOutliers_maxEjectionPercentage() {
               : 0;
     }
 
-    // Even if all subchannels were failing, we should have not ejected more than the configured
-    // maximum percentage.
-    assertThat((double) totalEjected / servers.size()).isAtMost(
-        (double) config.maxEjectionPercent / 100);
+    assertThat(totalEjected).isEqualTo(2);
   }