Add validation for the search backpressure cancellation settings (ope…

…nsearch-project#15501) * Fix updating search backpressure settings crashing the cluster Signed-off-by: Gao Binlong <[email protected]> * Modify change log Signed-off-by: Gao Binlong <[email protected]> * Fix version check Signed-off-by: Gao Binlong <[email protected]> * Increase test coverage Signed-off-by: Gao Binlong <[email protected]> * Format the code Signed-off-by: Gao Binlong <[email protected]> * Optimize some code Signed-off-by: Gao Binlong <[email protected]> * Fix typo Signed-off-by: Gao Binlong <[email protected]> --------- Signed-off-by: Gao Binlong <[email protected]>
gaiksaya · Sep 17, 2024 · 8347d0e · 8347d0e
1 parent 81288b1
commit 8347d0e
Show file tree

Hide file tree

Showing 10 changed files with 231 additions and 12 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -27,6 +27,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 ### Fixed
 - Fix wildcard query containing escaped character ([#15737](https://github.com/opensearch-project/OpenSearch/pull/15737))
 - Fix case-insensitive query on wildcard field ([#15882](https://github.com/opensearch-project/OpenSearch/pull/15882))
+- Add validation for the search backpressure cancellation settings ([#15501](https://github.com/opensearch-project/OpenSearch/pull/15501))
+
 ### Security
 
 [Unreleased 2.x]: https://github.com/opensearch-project/OpenSearch/compare/2.17...2.x
diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/cluster.put_settings/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/cluster.put_settings/10_basic.yml
@@ -98,3 +98,116 @@
   - match: {error.root_cause.0.type: "illegal_argument_exception"}
   - match: { error.root_cause.0.reason: "Invalid SearchBackpressureMode: monitor-only" }
   - match: { status: 400 }
+
+
+---
+"Test setting search backpressure cancellation settings":
+  - skip:
+      version: "- 2.99.99"
+      reason: "Fixed in 3.0.0"
+
+  - do:
+      cluster.put_settings:
+        body:
+          transient:
+            search_backpressure.search_task.cancellation_burst: 11
+  - is_true: acknowledged
+
+  - do:
+      cluster.get_settings:
+        flat_settings: false
+  - match: {transient.search_backpressure.search_task.cancellation_burst: "11"}
+
+  - do:
+      cluster.put_settings:
+        body:
+          transient:
+            search_backpressure.search_task.cancellation_rate: 0.1
+  - is_true: acknowledged
+
+  - do:
+      cluster.get_settings:
+        flat_settings: false
+  - match: {transient.search_backpressure.search_task.cancellation_rate: "0.1"}
+
+  - do:
+      cluster.put_settings:
+        body:
+          transient:
+            search_backpressure.search_task.cancellation_ratio: 0.2
+  - is_true: acknowledged
+
+  - do:
+      cluster.get_settings:
+        flat_settings: false
+  - match: {transient.search_backpressure.search_task.cancellation_ratio: "0.2"}
+
+  - do:
+      cluster.put_settings:
+        body:
+          transient:
+            search_backpressure.search_shard_task.cancellation_burst: 12
+  - is_true: acknowledged
+
+  - do:
+      cluster.get_settings:
+        flat_settings: false
+  - match: {transient.search_backpressure.search_shard_task.cancellation_burst: "12"}
+
+  - do:
+      cluster.put_settings:
+        body:
+          transient:
+            search_backpressure.search_shard_task.cancellation_rate: 0.3
+  - is_true: acknowledged
+
+  - do:
+      cluster.get_settings:
+        flat_settings: false
+  - match: {transient.search_backpressure.search_shard_task.cancellation_rate: "0.3"}
+
+  - do:
+      cluster.put_settings:
+        body:
+          transient:
+            search_backpressure.search_shard_task.cancellation_ratio: 0.4
+  - is_true: acknowledged
+
+  - do:
+      cluster.get_settings:
+        flat_settings: false
+  - match: {transient.search_backpressure.search_shard_task.cancellation_ratio: "0.4"}
+
+---
+"Test setting invalid search backpressure cancellation_rate and cancellation_ratio":
+  - skip:
+      version: "- 2.99.99"
+      reason: "Fixed in 3.0.0"
+
+  - do:
+      catch: /search_backpressure.search_task.cancellation_rate must be > 0/
+      cluster.put_settings:
+        body:
+          transient:
+            search_backpressure.search_task.cancellation_rate: 0.0
+
+  - do:
+      catch: /search_backpressure.search_task.cancellation_ratio must be > 0/
+      cluster.put_settings:
+        body:
+          transient:
+            search_backpressure.search_task.cancellation_ratio: 0.0
+
+  - do:
+      catch: /search_backpressure.search_shard_task.cancellation_rate must be > 0/
+      cluster.put_settings:
+        body:
+          transient:
+            search_backpressure.search_shard_task.cancellation_rate: 0.0
+
+  - do:
+      catch: /search_backpressure.search_shard_task.cancellation_ratio must be > 0/
+      cluster.put_settings:
+        body:
+          transient:
+            search_backpressure.search_shard_task.cancellation_ratio: 0.0
diff --git a/server/src/main/java/org/opensearch/common/settings/Setting.java b/server/src/main/java/org/opensearch/common/settings/Setting.java
@@ -1855,6 +1855,10 @@ public static Setting<Double> doubleSetting(
         );
     }
 
+    public static Setting<Double> doubleSetting(String key, double defaultValue, Validator<Double> validator, Property... properties) {
+        return new Setting<>(key, Double.toString(defaultValue), Double::parseDouble, validator, properties);
+    }
+
     /**
      * A writeable parser for double
      *
@@ -1961,6 +1965,15 @@ public static Setting<Double> doubleSetting(
         );
     }
 
+    public static Setting<Double> doubleSetting(
+        String key,
+        Setting<Double> fallbackSetting,
+        Validator<Double> validator,
+        Property... properties
+    ) {
+        return new Setting<>(new SimpleKey(key), fallbackSetting, fallbackSetting::getRaw, Double::parseDouble, validator, properties);
+    }
+
     /// simpleString
 
     public static Setting<String> simpleString(String key, Property... properties) {

diff --git a/server/src/main/java/org/opensearch/search/backpressure/SearchBackpressureService.java b/server/src/main/java/org/opensearch/search/backpressure/SearchBackpressureService.java
@@ -158,14 +158,16 @@ public SearchBackpressureService(
                 timeNanosSupplier,
                 getSettings().getSearchTaskSettings().getCancellationRateNanos(),
                 getSettings().getSearchTaskSettings().getCancellationBurst(),
-                getSettings().getSearchTaskSettings().getCancellationRatio()
+                getSettings().getSearchTaskSettings().getCancellationRatio(),
+                getSettings().getSearchTaskSettings().getCancellationRate()
             ),
             SearchShardTask.class,
             new SearchBackpressureState(
                 timeNanosSupplier,
                 getSettings().getSearchShardTaskSettings().getCancellationRateNanos(),
                 getSettings().getSearchShardTaskSettings().getCancellationBurst(),
-                getSettings().getSearchShardTaskSettings().getCancellationRatio()
+                getSettings().getSearchShardTaskSettings().getCancellationRatio(),
+                getSettings().getSearchShardTaskSettings().getCancellationRate()
             )
         );
         this.settings.getSearchTaskSettings().addListener(searchBackpressureStates.get(SearchTask.class));

diff --git a/server/src/main/java/org/opensearch/search/backpressure/SearchBackpressureState.java b/server/src/main/java/org/opensearch/search/backpressure/SearchBackpressureState.java
@@ -43,12 +43,15 @@ public class SearchBackpressureState implements CancellationSettingsListener {
         LongSupplier timeNanosSupplier,
         double cancellationRateNanos,
         double cancellationBurst,
-        double cancellationRatio
+        double cancellationRatio,
+        double cancellationRate
     ) {
         rateLimiter = new AtomicReference<>(new TokenBucket(timeNanosSupplier, cancellationRateNanos, cancellationBurst));
         ratioLimiter = new AtomicReference<>(new TokenBucket(this::getCompletionCount, cancellationRatio, cancellationBurst));
         this.timeNanosSupplier = timeNanosSupplier;
         this.cancellationBurst = cancellationBurst;
+        this.cancellationRatio = cancellationRatio;
+        this.cancellationRate = cancellationRate;
     }
 
     public long getCompletionCount() {

diff --git a/...src/main/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettings.java b/...src/main/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettings.java
@@ -61,8 +61,14 @@ private static class Defaults {
     public static final Setting<Double> SETTING_CANCELLATION_RATIO = Setting.doubleSetting(
         "search_backpressure.cancellation_ratio",
         Defaults.CANCELLATION_RATIO,
-        0.0,
-        1.0,
+        value -> {
+            if (value <= 0.0) {
+                throw new IllegalArgumentException("search_backpressure.cancellation_ratio must be > 0");
+            }
+            if (value > 1.0) {
+                throw new IllegalArgumentException("search_backpressure.cancellation_ratio must be <= 1.0");
+            }
+        },
         Setting.Property.Deprecated,
         Setting.Property.Dynamic,
         Setting.Property.NodeScope
@@ -78,7 +84,11 @@ private static class Defaults {
     public static final Setting<Double> SETTING_CANCELLATION_RATE = Setting.doubleSetting(
         "search_backpressure.cancellation_rate",
         Defaults.CANCELLATION_RATE,
-        0.0,
+        value -> {
+            if (value <= 0.0) {
+                throw new IllegalArgumentException("search_backpressure.cancellation_rate must be > 0");
+            }
+        },
         Setting.Property.Deprecated,
         Setting.Property.Dynamic,
         Setting.Property.NodeScope

diff --git a/...er/src/main/java/org/opensearch/search/backpressure/settings/SearchShardTaskSettings.java b/...er/src/main/java/org/opensearch/search/backpressure/settings/SearchShardTaskSettings.java
@@ -44,8 +44,14 @@ private static class Defaults {
     public static final Setting<Double> SETTING_CANCELLATION_RATIO = Setting.doubleSetting(
         "search_backpressure.search_shard_task.cancellation_ratio",
         SearchBackpressureSettings.SETTING_CANCELLATION_RATIO,
-        0.0,
-        1.0,
+        value -> {
+            if (value <= 0.0) {
+                throw new IllegalArgumentException("search_backpressure.search_shard_task.cancellation_ratio must be > 0");
+            }
+            if (value > 1.0) {
+                throw new IllegalArgumentException("search_backpressure.search_shard_task.cancellation_ratio must be <= 1.0");
+            }
+        },
         Setting.Property.Dynamic,
         Setting.Property.NodeScope
     );
@@ -58,7 +64,11 @@ private static class Defaults {
     public static final Setting<Double> SETTING_CANCELLATION_RATE = Setting.doubleSetting(
         "search_backpressure.search_shard_task.cancellation_rate",
         SearchBackpressureSettings.SETTING_CANCELLATION_RATE,
-        0.0,
+        value -> {
+            if (value <= 0.0) {
+                throw new IllegalArgumentException("search_backpressure.search_shard_task.cancellation_rate must be > 0");
+            }
+        },
         Setting.Property.Dynamic,
         Setting.Property.NodeScope
     );

diff --git a/server/src/main/java/org/opensearch/search/backpressure/settings/SearchTaskSettings.java b/server/src/main/java/org/opensearch/search/backpressure/settings/SearchTaskSettings.java
@@ -48,8 +48,14 @@ private static class Defaults {
     public static final Setting<Double> SETTING_CANCELLATION_RATIO = Setting.doubleSetting(
         "search_backpressure.search_task.cancellation_ratio",
         Defaults.CANCELLATION_RATIO,
-        0.0,
-        1.0,
+        value -> {
+            if (value <= 0.0) {
+                throw new IllegalArgumentException("search_backpressure.search_task.cancellation_ratio must be > 0");
+            }
+            if (value > 1.0) {
+                throw new IllegalArgumentException("search_backpressure.search_task.cancellation_ratio must be <= 1.0");
+            }
+        },
         Setting.Property.Dynamic,
         Setting.Property.NodeScope
     );
@@ -62,7 +68,11 @@ private static class Defaults {
     public static final Setting<Double> SETTING_CANCELLATION_RATE = Setting.doubleSetting(
         "search_backpressure.search_task.cancellation_rate",
         Defaults.CANCELLATION_RATE,
-        0.0,
+        value -> {
+            if (value <= 0.0) {
+                throw new IllegalArgumentException("search_backpressure.search_task.cancellation_rate must be > 0");
+            }
+        },
         Setting.Property.Dynamic,
         Setting.Property.NodeScope
     );

diff --git a/server/src/test/java/org/opensearch/common/settings/SettingTests.java b/server/src/test/java/org/opensearch/common/settings/SettingTests.java
@@ -1274,6 +1274,20 @@ public void testFloatParser() throws Exception {
     public void testDoubleWithDefaultValue() {
         Setting<Double> doubleSetting = Setting.doubleSetting("foo.bar", 42.1);
         assertEquals(doubleSetting.get(Settings.EMPTY), Double.valueOf(42.1));
+
+        Setting<Double> doubleSettingWithValidator = Setting.doubleSetting("foo.bar", 42.1, value -> {
+            if (value <= 0.0) {
+                throw new IllegalArgumentException("The setting foo.bar must be >0");
+            }
+        });
+        try {
+            assertThrows(
+                IllegalArgumentException.class,
+                () -> doubleSettingWithValidator.get(Settings.builder().put("foo.bar", randomFrom(-1, 0)).build())
+            );
+        } catch (IllegalArgumentException ex) {
+            assertEquals("The setting foo.bar must be >0", ex.getMessage());
+        }
     }
 
     public void testDoubleWithFallbackValue() {
@@ -1282,6 +1296,20 @@ public void testDoubleWithFallbackValue() {
         assertEquals(doubleSetting.get(Settings.EMPTY), Double.valueOf(2.1));
         assertEquals(doubleSetting.get(Settings.builder().put("foo.bar", 3.2).build()), Double.valueOf(3.2));
         assertEquals(doubleSetting.get(Settings.builder().put("foo.baz", 3.2).build()), Double.valueOf(3.2));
+
+        Setting<Double> doubleSettingWithValidator = Setting.doubleSetting("foo.bar", fallbackSetting, value -> {
+            if (value <= 0.0) {
+                throw new IllegalArgumentException("The setting foo.bar must be >0");
+            }
+        });
+        try {
+            assertThrows(
+                IllegalArgumentException.class,
+                () -> doubleSettingWithValidator.get(Settings.builder().put("foo.bar", randomFrom(-1, 0)).build())
+            );
+        } catch (IllegalArgumentException ex) {
+            assertEquals("The setting foo.bar must be >0", ex.getMessage());
+        }
     }
 
     public void testDoubleWithMinMax() throws Exception {

diff --git a/...est/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettingsTests.java b/...est/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettingsTests.java
@@ -37,4 +37,32 @@ public void testSearchBackpressureSettingValidateInvalidMode() {
             () -> new SearchBackpressureSettings(settings, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS))
         );
     }
+
+    public void testInvalidCancellationRate() {
+        Settings settings1 = Settings.builder().put("search_backpressure.search_task.cancellation_rate", randomFrom(-1, 0)).build();
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> new SearchBackpressureSettings(settings1, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS))
+        );
+
+        Settings settings2 = Settings.builder().put("search_backpressure.search_shard_task.cancellation_rate", randomFrom(-1, 0)).build();
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> new SearchBackpressureSettings(settings2, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS))
+        );
+    }
+
+    public void testInvalidCancellationRatio() {
+        Settings settings1 = Settings.builder().put("search_backpressure.search_task.cancellation_ratio", randomFrom(-1, 0)).build();
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> new SearchBackpressureSettings(settings1, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS))
+        );
+
+        Settings settings2 = Settings.builder().put("search_backpressure.search_shard_task.cancellation_ratio", randomFrom(-1, 0)).build();
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> new SearchBackpressureSettings(settings2, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS))
+        );
+    }
 }