Skip to content

Commit 8347d0e

Browse files
gaobinlonggaobinlong
authored
Add validation for the search backpressure cancellation settings (opensearch-project#15501)
* Fix updating search backpressure settings crashing the cluster Signed-off-by: Gao Binlong <gbinlong@amazon.com> * Modify change log Signed-off-by: Gao Binlong <gbinlong@amazon.com> * Fix version check Signed-off-by: Gao Binlong <gbinlong@amazon.com> * Increase test coverage Signed-off-by: Gao Binlong <gbinlong@amazon.com> * Format the code Signed-off-by: Gao Binlong <gbinlong@amazon.com> * Optimize some code Signed-off-by: Gao Binlong <gbinlong@amazon.com> * Fix typo Signed-off-by: Gao Binlong <gbinlong@amazon.com> --------- Signed-off-by: Gao Binlong <gbinlong@amazon.com>
1 parent 81288b1 commit 8347d0e

10 files changed

Lines changed: 231 additions & 12 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
2727
### Fixed
2828
- Fix wildcard query containing escaped character ([#15737](https://github.com/opensearch-project/OpenSearch/pull/15737))
2929
- Fix case-insensitive query on wildcard field ([#15882](https://github.com/opensearch-project/OpenSearch/pull/15882))
30+
- Add validation for the search backpressure cancellation settings ([#15501](https://github.com/opensearch-project/OpenSearch/pull/15501))
31+
3032
### Security
3133

3234
[Unreleased 2.x]: https://github.com/opensearch-project/OpenSearch/compare/2.17...2.x

‎rest-api-spec/src/main/resources/rest-api-spec/test/cluster.put_settings/10_basic.yml‎

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,3 +98,116 @@
9898
- match: {error.root_cause.0.type: "illegal_argument_exception"}
9999
- match: { error.root_cause.0.reason: "Invalid SearchBackpressureMode: monitor-only" }
100100
- match: { status: 400 }
101+
102+
103+
---
104+
"Test setting search backpressure cancellation settings":
105+
- skip:
106+
version: "- 2.99.99"
107+
reason: "Fixed in 3.0.0"
108+
109+
- do:
110+
cluster.put_settings:
111+
body:
112+
transient:
113+
search_backpressure.search_task.cancellation_burst: 11
114+
- is_true: acknowledged
115+
116+
- do:
117+
cluster.get_settings:
118+
flat_settings: false
119+
- match: {transient.search_backpressure.search_task.cancellation_burst: "11"}
120+
121+
- do:
122+
cluster.put_settings:
123+
body:
124+
transient:
125+
search_backpressure.search_task.cancellation_rate: 0.1
126+
- is_true: acknowledged
127+
128+
- do:
129+
cluster.get_settings:
130+
flat_settings: false
131+
- match: {transient.search_backpressure.search_task.cancellation_rate: "0.1"}
132+
133+
- do:
134+
cluster.put_settings:
135+
body:
136+
transient:
137+
search_backpressure.search_task.cancellation_ratio: 0.2
138+
- is_true: acknowledged
139+
140+
- do:
141+
cluster.get_settings:
142+
flat_settings: false
143+
- match: {transient.search_backpressure.search_task.cancellation_ratio: "0.2"}
144+
145+
- do:
146+
cluster.put_settings:
147+
body:
148+
transient:
149+
search_backpressure.search_shard_task.cancellation_burst: 12
150+
- is_true: acknowledged
151+
152+
- do:
153+
cluster.get_settings:
154+
flat_settings: false
155+
- match: {transient.search_backpressure.search_shard_task.cancellation_burst: "12"}
156+
157+
- do:
158+
cluster.put_settings:
159+
body:
160+
transient:
161+
search_backpressure.search_shard_task.cancellation_rate: 0.3
162+
- is_true: acknowledged
163+
164+
- do:
165+
cluster.get_settings:
166+
flat_settings: false
167+
- match: {transient.search_backpressure.search_shard_task.cancellation_rate: "0.3"}
168+
169+
- do:
170+
cluster.put_settings:
171+
body:
172+
transient:
173+
search_backpressure.search_shard_task.cancellation_ratio: 0.4
174+
- is_true: acknowledged
175+
176+
- do:
177+
cluster.get_settings:
178+
flat_settings: false
179+
- match: {transient.search_backpressure.search_shard_task.cancellation_ratio: "0.4"}
180+
181+
---
182+
"Test setting invalid search backpressure cancellation_rate and cancellation_ratio":
183+
- skip:
184+
version: "- 2.99.99"
185+
reason: "Fixed in 3.0.0"
186+
187+
- do:
188+
catch: /search_backpressure.search_task.cancellation_rate must be > 0/
189+
cluster.put_settings:
190+
body:
191+
transient:
192+
search_backpressure.search_task.cancellation_rate: 0.0
193+
194+
- do:
195+
catch: /search_backpressure.search_task.cancellation_ratio must be > 0/
196+
cluster.put_settings:
197+
body:
198+
transient:
199+
search_backpressure.search_task.cancellation_ratio: 0.0
200+
201+
- do:
202+
catch: /search_backpressure.search_shard_task.cancellation_rate must be > 0/
203+
cluster.put_settings:
204+
body:
205+
transient:
206+
search_backpressure.search_shard_task.cancellation_rate: 0.0
207+
208+
- do:
209+
catch: /search_backpressure.search_shard_task.cancellation_ratio must be > 0/
210+
cluster.put_settings:
211+
body:
212+
transient:
213+
search_backpressure.search_shard_task.cancellation_ratio: 0.0

‎server/src/main/java/org/opensearch/common/settings/Setting.java‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1855,6 +1855,10 @@ public static Setting<Double> doubleSetting(
18551855
);
18561856
}
18571857

1858+
public static Setting<Double> doubleSetting(String key, double defaultValue, Validator<Double> validator, Property... properties) {
1859+
return new Setting<>(key, Double.toString(defaultValue), Double::parseDouble, validator, properties);
1860+
}
1861+
18581862
/**
18591863
* A writeable parser for double
18601864
*
@@ -1961,6 +1965,15 @@ public static Setting<Double> doubleSetting(
19611965
);
19621966
}
19631967

1968+
public static Setting<Double> doubleSetting(
1969+
String key,
1970+
Setting<Double> fallbackSetting,
1971+
Validator<Double> validator,
1972+
Property... properties
1973+
) {
1974+
return new Setting<>(new SimpleKey(key), fallbackSetting, fallbackSetting::getRaw, Double::parseDouble, validator, properties);
1975+
}
1976+
19641977
/// simpleString
19651978

19661979
public static Setting<String> simpleString(String key, Property... properties) {

‎server/src/main/java/org/opensearch/search/backpressure/SearchBackpressureService.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -158,14 +158,16 @@ public SearchBackpressureService(
158158
timeNanosSupplier,
159159
getSettings().getSearchTaskSettings().getCancellationRateNanos(),
160160
getSettings().getSearchTaskSettings().getCancellationBurst(),
161-
getSettings().getSearchTaskSettings().getCancellationRatio()
161+
getSettings().getSearchTaskSettings().getCancellationRatio(),
162+
getSettings().getSearchTaskSettings().getCancellationRate()
162163
),
163164
SearchShardTask.class,
164165
new SearchBackpressureState(
165166
timeNanosSupplier,
166167
getSettings().getSearchShardTaskSettings().getCancellationRateNanos(),
167168
getSettings().getSearchShardTaskSettings().getCancellationBurst(),
168-
getSettings().getSearchShardTaskSettings().getCancellationRatio()
169+
getSettings().getSearchShardTaskSettings().getCancellationRatio(),
170+
getSettings().getSearchShardTaskSettings().getCancellationRate()
169171
)
170172
);
171173
this.settings.getSearchTaskSettings().addListener(searchBackpressureStates.get(SearchTask.class));

‎server/src/main/java/org/opensearch/search/backpressure/SearchBackpressureState.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,12 +43,15 @@ public class SearchBackpressureState implements CancellationSettingsListener {
4343
LongSupplier timeNanosSupplier,
4444
double cancellationRateNanos,
4545
double cancellationBurst,
46-
double cancellationRatio
46+
double cancellationRatio,
47+
double cancellationRate
4748
) {
4849
rateLimiter = new AtomicReference<>(new TokenBucket(timeNanosSupplier, cancellationRateNanos, cancellationBurst));
4950
ratioLimiter = new AtomicReference<>(new TokenBucket(this::getCompletionCount, cancellationRatio, cancellationBurst));
5051
this.timeNanosSupplier = timeNanosSupplier;
5152
this.cancellationBurst = cancellationBurst;
53+
this.cancellationRatio = cancellationRatio;
54+
this.cancellationRate = cancellationRate;
5255
}
5356

5457
public long getCompletionCount() {

‎server/src/main/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettings.java‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -61,8 +61,14 @@ private static class Defaults {
6161
public static final Setting<Double> SETTING_CANCELLATION_RATIO = Setting.doubleSetting(
6262
"search_backpressure.cancellation_ratio",
6363
Defaults.CANCELLATION_RATIO,
64-
0.0,
65-
1.0,
64+
value -> {
65+
if (value <= 0.0) {
66+
throw new IllegalArgumentException("search_backpressure.cancellation_ratio must be > 0");
67+
}
68+
if (value > 1.0) {
69+
throw new IllegalArgumentException("search_backpressure.cancellation_ratio must be <= 1.0");
70+
}
71+
},
6672
Setting.Property.Deprecated,
6773
Setting.Property.Dynamic,
6874
Setting.Property.NodeScope
@@ -78,7 +84,11 @@ private static class Defaults {
7884
public static final Setting<Double> SETTING_CANCELLATION_RATE = Setting.doubleSetting(
7985
"search_backpressure.cancellation_rate",
8086
Defaults.CANCELLATION_RATE,
81-
0.0,
87+
value -> {
88+
if (value <= 0.0) {
89+
throw new IllegalArgumentException("search_backpressure.cancellation_rate must be > 0");
90+
}
91+
},
8292
Setting.Property.Deprecated,
8393
Setting.Property.Dynamic,
8494
Setting.Property.NodeScope

‎server/src/main/java/org/opensearch/search/backpressure/settings/SearchShardTaskSettings.java‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,14 @@ private static class Defaults {
4444
public static final Setting<Double> SETTING_CANCELLATION_RATIO = Setting.doubleSetting(
4545
"search_backpressure.search_shard_task.cancellation_ratio",
4646
SearchBackpressureSettings.SETTING_CANCELLATION_RATIO,
47-
0.0,
48-
1.0,
47+
value -> {
48+
if (value <= 0.0) {
49+
throw new IllegalArgumentException("search_backpressure.search_shard_task.cancellation_ratio must be > 0");
50+
}
51+
if (value > 1.0) {
52+
throw new IllegalArgumentException("search_backpressure.search_shard_task.cancellation_ratio must be <= 1.0");
53+
}
54+
},
4955
Setting.Property.Dynamic,
5056
Setting.Property.NodeScope
5157
);
@@ -58,7 +64,11 @@ private static class Defaults {
5864
public static final Setting<Double> SETTING_CANCELLATION_RATE = Setting.doubleSetting(
5965
"search_backpressure.search_shard_task.cancellation_rate",
6066
SearchBackpressureSettings.SETTING_CANCELLATION_RATE,
61-
0.0,
67+
value -> {
68+
if (value <= 0.0) {
69+
throw new IllegalArgumentException("search_backpressure.search_shard_task.cancellation_rate must be > 0");
70+
}
71+
},
6272
Setting.Property.Dynamic,
6373
Setting.Property.NodeScope
6474
);

‎server/src/main/java/org/opensearch/search/backpressure/settings/SearchTaskSettings.java‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,14 @@ private static class Defaults {
4848
public static final Setting<Double> SETTING_CANCELLATION_RATIO = Setting.doubleSetting(
4949
"search_backpressure.search_task.cancellation_ratio",
5050
Defaults.CANCELLATION_RATIO,
51-
0.0,
52-
1.0,
51+
value -> {
52+
if (value <= 0.0) {
53+
throw new IllegalArgumentException("search_backpressure.search_task.cancellation_ratio must be > 0");
54+
}
55+
if (value > 1.0) {
56+
throw new IllegalArgumentException("search_backpressure.search_task.cancellation_ratio must be <= 1.0");
57+
}
58+
},
5359
Setting.Property.Dynamic,
5460
Setting.Property.NodeScope
5561
);
@@ -62,7 +68,11 @@ private static class Defaults {
6268
public static final Setting<Double> SETTING_CANCELLATION_RATE = Setting.doubleSetting(
6369
"search_backpressure.search_task.cancellation_rate",
6470
Defaults.CANCELLATION_RATE,
65-
0.0,
71+
value -> {
72+
if (value <= 0.0) {
73+
throw new IllegalArgumentException("search_backpressure.search_task.cancellation_rate must be > 0");
74+
}
75+
},
6676
Setting.Property.Dynamic,
6777
Setting.Property.NodeScope
6878
);

‎server/src/test/java/org/opensearch/common/settings/SettingTests.java‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1274,6 +1274,20 @@ public void testFloatParser() throws Exception {
12741274
public void testDoubleWithDefaultValue() {
12751275
Setting<Double> doubleSetting = Setting.doubleSetting("foo.bar", 42.1);
12761276
assertEquals(doubleSetting.get(Settings.EMPTY), Double.valueOf(42.1));
1277+
1278+
Setting<Double> doubleSettingWithValidator = Setting.doubleSetting("foo.bar", 42.1, value -> {
1279+
if (value <= 0.0) {
1280+
throw new IllegalArgumentException("The setting foo.bar must be >0");
1281+
}
1282+
});
1283+
try {
1284+
assertThrows(
1285+
IllegalArgumentException.class,
1286+
() -> doubleSettingWithValidator.get(Settings.builder().put("foo.bar", randomFrom(-1, 0)).build())
1287+
);
1288+
} catch (IllegalArgumentException ex) {
1289+
assertEquals("The setting foo.bar must be >0", ex.getMessage());
1290+
}
12771291
}
12781292

12791293
public void testDoubleWithFallbackValue() {
@@ -1282,6 +1296,20 @@ public void testDoubleWithFallbackValue() {
12821296
assertEquals(doubleSetting.get(Settings.EMPTY), Double.valueOf(2.1));
12831297
assertEquals(doubleSetting.get(Settings.builder().put("foo.bar", 3.2).build()), Double.valueOf(3.2));
12841298
assertEquals(doubleSetting.get(Settings.builder().put("foo.baz", 3.2).build()), Double.valueOf(3.2));
1299+
1300+
Setting<Double> doubleSettingWithValidator = Setting.doubleSetting("foo.bar", fallbackSetting, value -> {
1301+
if (value <= 0.0) {
1302+
throw new IllegalArgumentException("The setting foo.bar must be >0");
1303+
}
1304+
});
1305+
try {
1306+
assertThrows(
1307+
IllegalArgumentException.class,
1308+
() -> doubleSettingWithValidator.get(Settings.builder().put("foo.bar", randomFrom(-1, 0)).build())
1309+
);
1310+
} catch (IllegalArgumentException ex) {
1311+
assertEquals("The setting foo.bar must be >0", ex.getMessage());
1312+
}
12851313
}
12861314

12871315
public void testDoubleWithMinMax() throws Exception {

‎server/src/test/java/org/opensearch/search/backpressure/settings/SearchBackpressureSettingsTests.java‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,4 +37,32 @@ public void testSearchBackpressureSettingValidateInvalidMode() {
3737
() -> new SearchBackpressureSettings(settings, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS))
3838
);
3939
}
40+
41+
public void testInvalidCancellationRate() {
42+
Settings settings1 = Settings.builder().put("search_backpressure.search_task.cancellation_rate", randomFrom(-1, 0)).build();
43+
assertThrows(
44+
IllegalArgumentException.class,
45+
() -> new SearchBackpressureSettings(settings1, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS))
46+
);
47+
48+
Settings settings2 = Settings.builder().put("search_backpressure.search_shard_task.cancellation_rate", randomFrom(-1, 0)).build();
49+
assertThrows(
50+
IllegalArgumentException.class,
51+
() -> new SearchBackpressureSettings(settings2, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS))
52+
);
53+
}
54+
55+
public void testInvalidCancellationRatio() {
56+
Settings settings1 = Settings.builder().put("search_backpressure.search_task.cancellation_ratio", randomFrom(-1, 0)).build();
57+
assertThrows(
58+
IllegalArgumentException.class,
59+
() -> new SearchBackpressureSettings(settings1, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS))
60+
);
61+
62+
Settings settings2 = Settings.builder().put("search_backpressure.search_shard_task.cancellation_ratio", randomFrom(-1, 0)).build();
63+
assertThrows(
64+
IllegalArgumentException.class,
65+
() -> new SearchBackpressureSettings(settings2, new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS))
66+
);
67+
}
4068
}

0 commit comments

Comments
 (0)