parseHeapRatioOrDeprecatedByteSizeValue for indices.breaker.total.limit (#110236)

* parseHeapRatioOrDeprecatedByteSizeValue for indices.breaker.total.limit

* Fix tests for indices.breaker.total.limit warning

* Spotless

* Warn if below minimum percent

* Update docs/changelog/110236.yaml

* Changelog

* Pick correct area for changelog entry

* Spotless again dammit

* assertCriticalWarnings in circuit breaker test

* Expect another warning

docs/changelog/110236.yaml

@@ -0,0 +1,21 @@
+pr: 110236
+summary: '`ParseHeapRatioOrDeprecatedByteSizeValue` for `indices.breaker.total.limit`'
+area: Infra/Settings
+type: deprecation
+issues: []
+deprecation:
+  title: 'Deprecate absolute size values for `indices.breaker.total.limit` setting'
+  area: Cluster and node setting
+  details: Previously, the value of `indices.breaker.total.limit` could be specified as
+    an absolute size in bytes. This setting controls the overal amount of
+    memory the server is allowed to use before taking remedial actions. Setting
+    this to a specific number of bytes led to strange behaviour when the node
+    maximum heap size changed because the circut breaker limit would remain
+    unchanged. This would either leave the value too low, causing part of the
+    heap to remain unused; or it would leave the value too high, causing the
+    circuit breaker to be ineffective at preventing OOM errors.  The only
+    reasonable behaviour for this setting is that it scales with the size of
+    the heap, and so absolute byte limits are now deprecated.
+  impact: Users must change their configuration to specify a percentage instead of
+    an absolute number of bytes for `indices.breaker.total.limit`, or else
+    accept the default, which is already specified as a percentage.

server/src/main/java/org/elasticsearch/common/unit/MemorySizeValue.java

@@ -9,6 +9,9 @@
 package org.elasticsearch.common.unit;
 
 import org.elasticsearch.ElasticsearchParseException;
+import org.elasticsearch.cluster.routing.allocation.allocator.BalancedShardsAllocator;
+import org.elasticsearch.common.logging.DeprecationCategory;
+import org.elasticsearch.common.logging.DeprecationLogger;
 import org.elasticsearch.monitor.jvm.JvmInfo;
 
 import java.util.Objects;
@@ -25,18 +28,49 @@ public enum MemorySizeValue {
     public static ByteSizeValue parseBytesSizeValueOrHeapRatio(String sValue, String settingName) {
         settingName = Objects.requireNonNull(settingName);
         if (sValue != null && sValue.endsWith("%")) {
-            final String percentAsString = sValue.substring(0, sValue.length() - 1);
-            try {
-                final double percent = Double.parseDouble(percentAsString);
-                if (percent < 0 || percent > 100) {
-                    throw new ElasticsearchParseException("percentage should be in [0-100], got [{}]", percentAsString);
-                }
-                return ByteSizeValue.ofBytes((long) ((percent / 100) * JvmInfo.jvmInfo().getMem().getHeapMax().getBytes()));
-            } catch (NumberFormatException e) {
-                throw new ElasticsearchParseException("failed to parse [{}] as a double", e, percentAsString);
-            }
+            return parseHeapRatio(sValue, settingName, 0);
         } else {
             return parseBytesSizeValue(sValue, settingName);
         }
     }
+
+    public static ByteSizeValue parseHeapRatioOrDeprecatedByteSizeValue(String sValue, String settingName, double minHeapPercent) {
+        settingName = Objects.requireNonNull(settingName);
+        if (sValue != null && sValue.endsWith("%")) {
+            return parseHeapRatio(sValue, settingName, minHeapPercent);
+        } else {
+            DeprecationLogger.getLogger(BalancedShardsAllocator.class)
+                .critical(
+                    DeprecationCategory.SETTINGS,
+                    "absolute_size_not_supported",
+                    "[{}] should be specified using a percentage of the heap. Absolute size settings will be forbidden in a future release",
+                    settingName
+                );
+            return parseBytesSizeValue(sValue, settingName);
+        }
+    }
+
+    private static ByteSizeValue parseHeapRatio(String sValue, String settingName, double minHeapPercent) {
+        final String percentAsString = sValue.substring(0, sValue.length() - 1);
+        try {
+            final double percent = Double.parseDouble(percentAsString);
+            if (percent < 0 || percent > 100) {
+                throw new ElasticsearchParseException("percentage should be in [0-100], got [{}]", percentAsString);
+            } else if (percent < minHeapPercent) {
+                DeprecationLogger.getLogger(MemorySizeValue.class)
+                    .warn(
+                        DeprecationCategory.SETTINGS,
+                        "memory_size_below_minimum",
+                        "[{}] setting of [{}] is below the recommended minimum of {}% of the heap",
+                        settingName,
+                        sValue,
+                        minHeapPercent
+                    );
+            }
+            return ByteSizeValue.ofBytes((long) ((percent / 100) * JvmInfo.jvmInfo().getMem().getHeapMax().getBytes()));
+        } catch (NumberFormatException e) {
+            throw new ElasticsearchParseException("failed to parse [{}] as a double", e, percentAsString);
+        }
+    }
+
 }

server/src/main/java/org/elasticsearch/indices/breaker/HierarchyCircuitBreakerService.java

@@ -20,6 +20,7 @@ import org.elasticsearch.common.settings.Setting.Property;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.unit.ByteSizeUnit;
 import org.elasticsearch.common.unit.ByteSizeValue;
+import org.elasticsearch.common.unit.MemorySizeValue;
 import org.elasticsearch.common.util.concurrent.ReleasableLock;
 import org.elasticsearch.core.Booleans;
 import org.elasticsearch.core.SuppressForbidden;
@@ -65,7 +66,7 @@ public class HierarchyCircuitBreakerService extends CircuitBreakerService {
         Property.NodeScope
     );
 
-    public static final Setting<ByteSizeValue> TOTAL_CIRCUIT_BREAKER_LIMIT_SETTING = Setting.memorySizeSetting(
+    public static final Setting<ByteSizeValue> TOTAL_CIRCUIT_BREAKER_LIMIT_SETTING = new Setting<>(
         "indices.breaker.total.limit",
         settings -> {
             if (USE_REAL_MEMORY_USAGE_SETTING.get(settings)) {
@@ -74,6 +75,7 @@ public class HierarchyCircuitBreakerService extends CircuitBreakerService {
                 return "70%";
             }
         },
+        (s) -> MemorySizeValue.parseHeapRatioOrDeprecatedByteSizeValue(s, "indices.breaker.total.limit", 50),
         Property.Dynamic,
         Property.NodeScope
     );

server/src/test/java/org/elasticsearch/common/settings/MemorySizeSettingsTests.java

@@ -71,6 +71,11 @@ public class MemorySizeSettingsTests extends ESTestCase {
             "indices.breaker.total.limit",
             ByteSizeValue.ofBytes((long) (JvmInfo.jvmInfo().getMem().getHeapMax().getBytes() * defaultTotalPercentage))
         );
+        assertWarnings(
+            "[indices.breaker.total.limit] setting of [25%] is below the recommended minimum of 50.0% of the heap",
+            "[indices.breaker.total.limit] should be specified using a percentage of the heap. "
+                + "Absolute size settings will be forbidden in a future release"
+        );
         assertMemorySizeSetting(
             HierarchyCircuitBreakerService.FIELDDATA_CIRCUIT_BREAKER_LIMIT_SETTING,
             "indices.breaker.fielddata.limit",

server/src/test/java/org/elasticsearch/indices/breaker/HierarchyCircuitBreakerServiceTests.java

@@ -254,6 +254,8 @@ public class HierarchyCircuitBreakerServiceTests extends ESTestCase {
             assertThat(exception.getMessage(), containsString("request=157286400/150mb"));
             assertThat(exception.getDurability(), equalTo(CircuitBreaker.Durability.TRANSIENT));
         }
+
+        assertCircuitBreakerLimitWarning();
     }
 
     public void testParentBreaksOnRealMemoryUsage() {
@@ -325,6 +327,8 @@ public class HierarchyCircuitBreakerServiceTests extends ESTestCase {
         memoryUsage.set(100);
         requestBreaker.addEstimateBytesAndMaybeBreak(reservationInBytes, "request");
         assertEquals(0, requestBreaker.getTrippedCount());
+
+        assertCircuitBreakerLimitWarning();
     }
 
     /**
@@ -749,6 +753,7 @@ public class HierarchyCircuitBreakerServiceTests extends ESTestCase {
                 equalTo(expectedDurability)
             );
         }
+        assertCircuitBreakerLimitWarning();
     }
 
     public void testAllocationBucketsBreaker() {
@@ -785,6 +790,8 @@ public class HierarchyCircuitBreakerServiceTests extends ESTestCase {
             assertThat(exception.getMessage(), containsString("[parent] Data too large, data for [allocated_buckets] would be"));
             assertThat(exception.getMessage(), containsString("which is larger than the limit of [100/100b]"));
         }
+
+        assertCircuitBreakerLimitWarning();
     }
 
     public void testRegisterCustomCircuitBreakers_WithDuplicates() {
@@ -891,7 +898,7 @@ public class HierarchyCircuitBreakerServiceTests extends ESTestCase {
                 service.getParentLimit()
             );
 
-            // total.limit defaults to 70% of the JVM heap if use_real_memory set to true
+            // total.limit defaults to 95% of the JVM heap if use_real_memory set to true
             clusterSettings.applySettings(Settings.builder().put(useRealMemoryUsageSetting, true).build());
             assertEquals(
                 MemorySizeValue.parseBytesSizeValueOrHeapRatio("95%", totalCircuitBreakerLimitSetting).getBytes(),
@@ -900,6 +907,15 @@ public class HierarchyCircuitBreakerServiceTests extends ESTestCase {
         }
     }
 
+    public void testSizeBelowMinimumWarning() {
+        ByteSizeValue sizeValue = MemorySizeValue.parseHeapRatioOrDeprecatedByteSizeValue(
+            "19%",
+            HierarchyCircuitBreakerService.TOTAL_CIRCUIT_BREAKER_LIMIT_SETTING.getKey(),
+            20
+        );
+        assertWarnings("[indices.breaker.total.limit] setting of [19%] is below the recommended minimum of 20.0% of the heap");
+    }
+
     public void testBuildParentTripMessage() {
         class TestChildCircuitBreaker extends NoopCircuitBreaker {
             private final long used;
@@ -972,4 +988,12 @@ public class HierarchyCircuitBreakerServiceTests extends ESTestCase {
             HierarchyCircuitBreakerService.permitNegativeValues = false;
         }
     }
+
+    void assertCircuitBreakerLimitWarning() {
+        assertWarnings(
+            "[indices.breaker.total.limit] should be specified using a percentage of the heap. "
+                + "Absolute size settings will be forbidden in a future release"
+        );
+
+    }
 }

test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java

@@ -649,7 +649,7 @@ public abstract class ESTestCase extends LuceneTestCase {
     /**
      * Convenience method to assert warnings for settings deprecations and general deprecation warnings. All warnings passed to this method
      * are assumed to be at WARNING level.
-     * @param expectedWarnings expected general deprecation warnings.
+     * @param expectedWarnings expected general deprecation warning messages.
      */
     protected final void assertWarnings(String... expectedWarnings) {
         assertWarnings(
@@ -663,7 +663,7 @@ public abstract class ESTestCase extends LuceneTestCase {
     /**
      * Convenience method to assert warnings for settings deprecations and general deprecation warnings. All warnings passed to this method
      * are assumed to be at CRITICAL level.
-     * @param expectedWarnings expected general deprecation warnings.
+     * @param expectedWarnings expected general deprecation warning messages.
      */
     protected final void assertCriticalWarnings(String... expectedWarnings) {
         assertWarnings(

x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/execution/sequence/CircuitBreakerTests.java

@@ -245,7 +245,9 @@ public class CircuitBreakerTests extends ESTestCase {
         final int searchRequestsExpectedCount = 2;
 
         // let the parent circuit breaker fail, setting its limit to zero
-        Settings settings = Settings.builder().put(HierarchyCircuitBreakerService.TOTAL_CIRCUIT_BREAKER_LIMIT_SETTING.getKey(), 0).build();
+        Settings settings = Settings.builder()
+            .put(HierarchyCircuitBreakerService.TOTAL_CIRCUIT_BREAKER_LIMIT_SETTING.getKey(), "0%")
+            .build();
 
         try (
             CircuitBreakerService service = new HierarchyCircuitBreakerService(
@@ -277,6 +279,7 @@ public class CircuitBreakerTests extends ESTestCase {
             TumblingWindow window = new TumblingWindow(eqlClient, criteria, null, matcher, Collections.emptyList());
             window.execute(wrap(p -> fail(), ex -> assertTrue(ex instanceof CircuitBreakingException)));
         }
+        assertCriticalWarnings("[indices.breaker.total.limit] setting of [0%] is below the recommended minimum of 50.0% of the heap");
     }
 
     private List<BreakerSettings> breakerSettings() {