Skip to content

Commit 1e59f38

Browse files
gaobinlonggaobinlong
committed
Add validation for the search backpressure cancellation settings (opensearch-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]> (cherry picked from commit 8347d0e)
1 parent bfdc30c commit 1e59f38

File tree

10 files changed

+230
-12
lines changed

10 files changed

+230
-12
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
2626

2727
### Fixed
2828
- Fix wildcard query containing escaped character ([#15737](https://github.com/opensearch-project/OpenSearch/pull/15737))
29+
- Add validation for the search backpressure cancellation settings ([#15501](https://github.com/opensearch-project/OpenSearch/pull/15501))
2930

3031
### Security
3132

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