From 1e59f380560c98bf3847f16f0c7da0e5bb2e6d53 Mon Sep 17 00:00:00 2001 From: gaobinlong Date: Wed, 18 Sep 2024 03:12:05 +0800 Subject: [PATCH 1/2] Add validation for the search backpressure cancellation settings (#15501) * Fix updating search backpressure settings crashing the cluster Signed-off-by: Gao Binlong * Modify change log Signed-off-by: Gao Binlong * Fix version check Signed-off-by: Gao Binlong * Increase test coverage Signed-off-by: Gao Binlong * Format the code Signed-off-by: Gao Binlong * Optimize some code Signed-off-by: Gao Binlong * Fix typo Signed-off-by: Gao Binlong --------- Signed-off-by: Gao Binlong (cherry picked from commit 8347d0ec22aa865dea3bc4b2027fb7bdf486fab2) --- CHANGELOG.md | 1 + .../test/cluster.put_settings/10_basic.yml | 113 ++++++++++++++++++ .../opensearch/common/settings/Setting.java | 13 ++ .../SearchBackpressureService.java | 6 +- .../backpressure/SearchBackpressureState.java | 5 +- .../settings/SearchBackpressureSettings.java | 16 ++- .../settings/SearchShardTaskSettings.java | 16 ++- .../settings/SearchTaskSettings.java | 16 ++- .../common/settings/SettingTests.java | 28 +++++ .../SearchBackpressureSettingsTests.java | 28 +++++ 10 files changed, 230 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9039d64eed57f..3e60261ce012c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ 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)) +- Add validation for the search backpressure cancellation settings ([#15501](https://github.com/opensearch-project/OpenSearch/pull/15501)) ### 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 60081783bb346..ed50061a0778f 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 @@ -102,3 +102,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 index fea4c165809ba..081029c1c106c 100644 --- 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 doubleSetting( ); } + public static Setting doubleSetting(String key, double defaultValue, Validator 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 doubleSetting( ); } + public static Setting doubleSetting( + String key, + Setting fallbackSetting, + Validator validator, + Property... properties + ) { + return new Setting<>(new SimpleKey(key), fallbackSetting, fallbackSetting::getRaw, Double::parseDouble, validator, properties); + } + /// simpleString public static Setting 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 index e04a56ad4c84f..d0480f3dfad80 100644 --- 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 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 4c28d96d8289e..a12e021bcf9fe 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 @@ -61,8 +61,14 @@ private static class Defaults { public static final Setting 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 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/server/src/main/java/org/opensearch/search/backpressure/settings/SearchShardTaskSettings.java b/server/src/main/java/org/opensearch/search/backpressure/settings/SearchShardTaskSettings.java index 6d016c7466362..38213506c55b7 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 @@ -44,8 +44,14 @@ private static class Defaults { public static final Setting 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 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 index 4b34323b1ddc6..f9af7f9b59fdb 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 @@ -48,8 +48,14 @@ private static class Defaults { public static final Setting 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 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 index 7ebee680e8e52..c3c399a9d88b2 100644 --- 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 doubleSetting = Setting.doubleSetting("foo.bar", 42.1); assertEquals(doubleSetting.get(Settings.EMPTY), Double.valueOf(42.1)); + + Setting 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 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/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..683ada76c7683 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", 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)) + ); + } } From c97fcc41dc6672fcf5bafa0f1be3f3b629300c4a Mon Sep 17 00:00:00 2001 From: Gao Binlong Date: Wed, 18 Sep 2024 11:05:31 +0800 Subject: [PATCH 2/2] Update version check in yml test file Signed-off-by: Gao Binlong --- .../rest-api-spec/test/cluster.put_settings/10_basic.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 ed50061a0778f..cfcdb58a7764b 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 @@ -107,8 +107,8 @@ --- "Test setting search backpressure cancellation settings": - skip: - version: "- 2.99.99" - reason: "Fixed in 3.0.0" + version: "- 2.17.99" + reason: "Fixed in 2.18.0" - do: cluster.put_settings: @@ -185,8 +185,8 @@ --- "Test setting invalid search backpressure cancellation_rate and cancellation_ratio": - skip: - version: "- 2.99.99" - reason: "Fixed in 3.0.0" + version: "- 2.17.99" + reason: "Fixed in 2.18.0" - do: catch: /search_backpressure.search_task.cancellation_rate must be > 0/