Fix updating search backpressure settings crashing the cluster

Signed-off-by: Gao Binlong <gbinlong@amazon.com>
opensearch-project · Aug 29, 2024 · adcf328 · adcf328
1 parent 2b84305
commit adcf328
Show file tree

Hide file tree

Showing 8 changed files with 183 additions and 3 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -80,6 +80,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Fix indexing error when flat_object field is explicitly null ([#15375](https://github.com/opensearch-project/OpenSearch/pull/15375))
 - Fix split response processor not included in allowlist ([#15393](https://github.com/opensearch-project/OpenSearch/pull/15393))
 - Fix unchecked cast in dynamic action map getter ([#15394](https://github.com/opensearch-project/OpenSearch/pull/15394))
+- Fix updating search backpressure settings crashing the cluster
 
 ### Security
 

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.3.99"
+      reason: "Introduced in 2.4.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 greater than zero/
+      cluster.put_settings:
+        body:
+          transient:
+            search_backpressure.search_task.cancellation_rate: 0.0
+
+  - do:
+      catch: /search_backpressure.search_task.cancellation_ratio must be greater than zero/
+      cluster.put_settings:
+        body:
+          transient:
+            search_backpressure.search_task.cancellation_ratio: 0.0
+
+  - do:
+      catch: /search_backpressure.search_shard_task.cancellation_rate must be greater than zero/
+      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 greater than zero/
+      cluster.put_settings:
+        body:
+          transient:
+            search_backpressure.search_shard_task.cancellation_ratio: 0.0
diff --git a/server/src/main/java/org/opensearch/search/backpressure/SearchBackpressureService.java b/server/src/main/java/org/opensearch/search/backpressure/SearchBackpressureService.java
@@ -160,14 +160,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
@@ -63,6 +63,11 @@ private static class Defaults {
         Defaults.CANCELLATION_RATIO,
         0.0,
         1.0,
+        value -> {
+            if (value <= 0.0) {
+                throw new IllegalArgumentException("search_backpressure.cancellation_ratio must be greater than zero");
+            }
+        },
         Setting.Property.Deprecated,
         Setting.Property.Dynamic,
         Setting.Property.NodeScope
@@ -79,6 +84,12 @@ private static class Defaults {
         "search_backpressure.cancellation_rate",
         Defaults.CANCELLATION_RATE,
         0.0,
+        Double.MAX_VALUE,
+        value -> {
+            if (value <= 0.0) {
+                throw new IllegalArgumentException("search_backpressure.cancellation_rate must be greater than zero");
+            }
+        },
         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
@@ -50,6 +50,11 @@ private static class Defaults {
         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 greater than zero");
+            }
+        },
         Setting.Property.Dynamic,
         Setting.Property.NodeScope
     );
@@ -63,6 +68,12 @@ private static class Defaults {
         "search_backpressure.search_shard_task.cancellation_rate",
         SearchBackpressureSettings.SETTING_CANCELLATION_RATE,
         0.0,
+        Double.MAX_VALUE,
+        value -> {
+            if (value <= 0.0) {
+                throw new IllegalArgumentException("search_backpressure.search_shard_task.cancellation_rate must be greater than zero");
+            }
+        },
         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
@@ -51,6 +51,11 @@ private static class Defaults {
         Defaults.CANCELLATION_RATIO,
         0.0,
         1.0,
+        value -> {
+            if (value <= 0.0) {
+                throw new IllegalArgumentException("search_backpressure.search_task.cancellation_ratio must be greater than zero");
+            }
+        },
         Setting.Property.Dynamic,
         Setting.Property.NodeScope
     );
@@ -64,6 +69,12 @@ private static class Defaults {
         "search_backpressure.search_task.cancellation_rate",
         Defaults.CANCELLATION_RATE,
         0.0,
+        Double.MAX_VALUE,
+        value -> {
+            if (value <= 0.0) {
+                throw new IllegalArgumentException("search_backpressure.search_task.cancellation_rate must be greater than zero");
+            }
+        },
         Setting.Property.Dynamic,
         Setting.Property.NodeScope
     );

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", 0.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", 0.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", 0.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", 0.0).build();
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> new SearchBackpressureSettings(settings2, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS))
+        );
+    }
 }