From adcf3289c4453a0be6a8ce0bfd6f18c8f19270af Mon Sep 17 00:00:00 2001 From: Gao Binlong Date: Thu, 29 Aug 2024 16:21:46 +0800 Subject: [PATCH] Fix updating search backpressure settings crashing the cluster Signed-off-by: Gao Binlong --- CHANGELOG.md | 1 + .../test/cluster.put_settings/10_basic.yml | 113 ++++++++++++++++++ .../SearchBackpressureService.java | 6 +- .../backpressure/SearchBackpressureState.java | 5 +- .../settings/SearchBackpressureSettings.java | 11 ++ .../settings/SearchShardTaskSettings.java | 11 ++ .../settings/SearchTaskSettings.java | 11 ++ .../SearchBackpressureSettingsTests.java | 28 +++++ 8 files changed, 183 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b7e4548100df3..d871c268fdfd7 100644 --- 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 index 2bc5e98465e16..2dc5c243ff176 100644 --- 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 index a85bc69b766cb..f563cad947354 100644 --- 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 index 5f086bd498036..36f5b25e002c3 100644 --- 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/server/src/main/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettings.java b/server/src/main/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettings.java index 79494eb0d3c24..e7c68d5e34e1f 100644 --- a/server/src/main/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettings.java +++ b/server/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/server/src/main/java/org/opensearch/search/backpressure/settings/SearchShardTaskSettings.java b/server/src/main/java/org/opensearch/search/backpressure/settings/SearchShardTaskSettings.java index 105023a20173f..cf82a6a19fe8f 100644 --- a/server/src/main/java/org/opensearch/search/backpressure/settings/SearchShardTaskSettings.java +++ b/server/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 index 4b12eda9fb59d..f6fa8378822da 100644 --- 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/server/src/test/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettingsTests.java b/server/src/test/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettingsTests.java index a02ca3cf877ad..65c7c5edf395b 100644 --- a/server/src/test/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettingsTests.java +++ b/server/src/test/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)) + ); + } }