From a8aecb8b22d249815ba29b485ea3126e415f47dd Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Thu, 9 Jul 2020 16:34:28 -0500 Subject: [PATCH] Scripting: Unlimited compilation rate for ingest (#59267) * `ingest` and `processor_conditional` default to unlimited compilation rate Fixes: #50152 --- .../script/IngestConditionalScript.java | 3 +- .../elasticsearch/script/IngestScript.java | 3 +- .../org/elasticsearch/script/ScriptCache.java | 88 +++++++++++++++++-- .../elasticsearch/script/ScriptService.java | 53 ++--------- .../script/ScriptCacheTests.java | 17 ++-- .../script/ScriptServiceTests.java | 36 +++----- 6 files changed, 109 insertions(+), 91 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/script/IngestConditionalScript.java b/server/src/main/java/org/elasticsearch/script/IngestConditionalScript.java index ed224204f8933..1174b75fb507a 100644 --- a/server/src/main/java/org/elasticsearch/script/IngestConditionalScript.java +++ b/server/src/main/java/org/elasticsearch/script/IngestConditionalScript.java @@ -19,7 +19,6 @@ package org.elasticsearch.script; -import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.unit.TimeValue; import java.util.Map; @@ -33,7 +32,7 @@ public abstract class IngestConditionalScript { /** The context used to compile {@link IngestConditionalScript} factories. */ public static final ScriptContext CONTEXT = new ScriptContext<>("processor_conditional", Factory.class, - 200, TimeValue.timeValueMillis(0), new Tuple<>(375, TimeValue.timeValueMinutes(5))); + 200, TimeValue.timeValueMillis(0), ScriptCache.UNLIMITED_COMPILATION_RATE.asTuple()); /** The generic runtime parameters for the script. */ private final Map params; diff --git a/server/src/main/java/org/elasticsearch/script/IngestScript.java b/server/src/main/java/org/elasticsearch/script/IngestScript.java index ffd3475fc52c3..6af53e19614fa 100644 --- a/server/src/main/java/org/elasticsearch/script/IngestScript.java +++ b/server/src/main/java/org/elasticsearch/script/IngestScript.java @@ -20,7 +20,6 @@ package org.elasticsearch.script; -import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.unit.TimeValue; import java.util.Map; @@ -34,7 +33,7 @@ public abstract class IngestScript { /** The context used to compile {@link IngestScript} factories. */ public static final ScriptContext CONTEXT = new ScriptContext<>("ingest", Factory.class, - 200, TimeValue.timeValueMillis(0), new Tuple<>(375, TimeValue.timeValueMinutes(5))); + 200, TimeValue.timeValueMillis(0), ScriptCache.UNLIMITED_COMPILATION_RATE.asTuple()); /** The generic runtime parameters for the script. */ private final Map params; diff --git a/server/src/main/java/org/elasticsearch/script/ScriptCache.java b/server/src/main/java/org/elasticsearch/script/ScriptCache.java index 58fb9ed16cee3..e156cfa125b19 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptCache.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptCache.java @@ -42,7 +42,7 @@ public class ScriptCache { private static final Logger logger = LogManager.getLogger(ScriptService.class); - static final Tuple UNLIMITED_COMPILATION_RATE = new Tuple<>(0, TimeValue.ZERO); + static final CompilationRate UNLIMITED_COMPILATION_RATE = new CompilationRate(0, TimeValue.ZERO); private final Cache cache; private final ScriptMetrics scriptMetrics; @@ -51,14 +51,14 @@ public class ScriptCache { // Cache settings or derived from settings final int cacheSize; final TimeValue cacheExpire; - final Tuple rate; + final CompilationRate rate; private final double compilesAllowedPerNano; private final String contextRateSetting; ScriptCache( int cacheMaxSize, TimeValue cacheExpire, - Tuple maxCompilationRate, + CompilationRate maxCompilationRate, String contextRateSetting ) { this.cacheSize = cacheMaxSize; @@ -78,9 +78,9 @@ public class ScriptCache { this.cache = cacheBuilder.removalListener(new ScriptCacheRemovalListener()).build(); this.rate = maxCompilationRate; - this.compilesAllowedPerNano = ((double) rate.v1()) / rate.v2().nanos(); + this.compilesAllowedPerNano = ((double) rate.count) / rate.time.nanos(); this.scriptMetrics = new ScriptMetrics(); - this.tokenBucketState = new AtomicReference(new TokenBucketState(this.rate.v1())); + this.tokenBucketState = new AtomicReference(new TokenBucketState(this.rate.count)); } FactoryType compile( @@ -156,8 +156,8 @@ void checkCompilationLimit() { double scriptsPerTimeWindow = current.availableTokens + (timePassed) * compilesAllowedPerNano; // It's been over the time limit anyway, readjust the bucket to be level - if (scriptsPerTimeWindow > rate.v1()) { - scriptsPerTimeWindow = rate.v1(); + if (scriptsPerTimeWindow > rate.count) { + scriptsPerTimeWindow = rate.count; } // If there is enough tokens in the bucket, allow the request and decrease the tokens by 1 @@ -173,7 +173,7 @@ void checkCompilationLimit() { scriptMetrics.onCompilationLimit(); // Otherwise reject the request throw new CircuitBreakingException("[script] Too many dynamic script compilations within, max: [" + - rate.v1() + "/" + rate.v2() +"]; please use indexed, or scripts with parameters instead; " + + rate + "]; please use indexed, or scripts with parameters instead; " + "this limit can be changed by the [" + contextRateSetting + "] setting", CircuitBreaker.Durability.TRANSIENT); } @@ -243,4 +243,76 @@ private TokenBucketState(long lastInlineCompileTime, double availableTokens, boo this.tokenSuccessfullyTaken = tokenSuccessfullyTaken; } } + + public static class CompilationRate { + public final int count; + public final TimeValue time; + private final String source; + + public CompilationRate(Integer count, TimeValue time) { + this.count = count; + this.time = time; + this.source = null; + } + + public CompilationRate(Tuple rate) { + this(rate.v1(), rate.v2()); + } + + /** + * Parses a string as a non-negative int count and a {@code TimeValue} as arguments split by a slash + */ + public CompilationRate(String value) { + if (value.contains("/") == false || value.startsWith("/") || value.endsWith("/")) { + throw new IllegalArgumentException("parameter must contain a positive integer and a timevalue, i.e. 10/1m, but was [" + + value + "]"); + } + int idx = value.indexOf("/"); + String count = value.substring(0, idx); + String time = value.substring(idx + 1); + try { + int rate = Integer.parseInt(count); + if (rate < 0) { + throw new IllegalArgumentException("rate [" + rate + "] must be positive"); + } + TimeValue timeValue = TimeValue.parseTimeValue(time, "script.max_compilations_rate"); + if (timeValue.nanos() <= 0) { + throw new IllegalArgumentException("time value [" + time + "] must be positive"); + } + // protect against a too hard to check limit, like less than a minute + if (timeValue.seconds() < 60) { + throw new IllegalArgumentException("time value [" + time + "] must be at least on a one minute resolution"); + } + this.count = rate; + this.time = timeValue; + this.source = value; + } catch (NumberFormatException e) { + // the number format exception message is so confusing, that it makes more sense to wrap it with a useful one + throw new IllegalArgumentException("could not parse [" + count + "] as integer in value [" + value + "]", e); + } + } + + public Tuple asTuple() { + return new Tuple<>(this.count, this.time); + } + + @Override + public String toString() { + return source != null ? source : count + "/" + time.toHumanReadableString(0); + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + CompilationRate that = (CompilationRate) o; + return count == that.count && + Objects.equals(time, that.time); + } + + @Override + public int hashCode() { + return Objects.hash(count, time); + } + } } diff --git a/server/src/main/java/org/elasticsearch/script/ScriptService.java b/server/src/main/java/org/elasticsearch/script/ScriptService.java index c62b2ad899cde..254512e5b73f2 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptService.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptService.java @@ -34,7 +34,6 @@ import org.elasticsearch.cluster.metadata.Metadata; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.Strings; -import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Setting.Property; @@ -64,43 +63,6 @@ public class ScriptService implements Closeable, ClusterStateApplier { static final String DISABLE_DYNAMIC_SCRIPTING_SETTING = "script.disable_dynamic"; - // Special setting value for SCRIPT_GENERAL_MAX_COMPILATIONS_RATE to indicate the script service should use context - // specific caches - static final Tuple USE_CONTEXT_RATE_VALUE = new Tuple<>(-1, TimeValue.MINUS_ONE); - static final String USE_CONTEXT_RATE_KEY = "use-context"; - - // a parsing function that requires a non negative int and a timevalue as arguments split by a slash - // this allows you to easily define rates - static final Function> MAX_COMPILATION_RATE_FUNCTION = - (String value) -> { - if (value.contains("/") == false || value.startsWith("/") || value.endsWith("/")) { - throw new IllegalArgumentException("parameter must contain a positive integer and a timevalue, i.e. 10/1m, but was [" + - value + "]"); - } - int idx = value.indexOf("/"); - String count = value.substring(0, idx); - String time = value.substring(idx + 1); - try { - - int rate = Integer.parseInt(count); - if (rate < 0) { - throw new IllegalArgumentException("rate [" + rate + "] must be positive"); - } - TimeValue timeValue = TimeValue.parseTimeValue(time, "script.context.$CONTEXT.max_compilations_rate"); - if (timeValue.nanos() <= 0) { - throw new IllegalArgumentException("time value [" + time + "] must be positive"); - } - // protect against a too hard to check limit, like less than a minute - if (timeValue.seconds() < 60) { - throw new IllegalArgumentException("time value [" + time + "] must be at least on a one minute resolution"); - } - return Tuple.tuple(rate, timeValue); - } catch (NumberFormatException e) { - // the number format exception message is so confusing, that it makes more sense to wrap it with a useful one - throw new IllegalArgumentException("could not parse [" + count + "] as integer in value [" + value + "]", e); - } - }; - public static final Setting SCRIPT_MAX_SIZE_IN_BYTES = Setting.intSetting("script.max_size_in_bytes", 65535, 0, Property.Dynamic, Property.NodeScope); @@ -120,15 +82,15 @@ public class ScriptService implements Closeable, ClusterStateApplier { // Unlimited compilation rate for context-specific script caches static final String UNLIMITED_COMPILATION_RATE_KEY = "unlimited"; - public static final Setting.AffixSetting> SCRIPT_MAX_COMPILATIONS_RATE_SETTING = + public static final Setting.AffixSetting SCRIPT_MAX_COMPILATIONS_RATE_SETTING = Setting.affixKeySetting(CONTEXT_PREFIX, "max_compilations_rate", - key -> new Setting<>(key, "75/5m", + key -> new Setting(key, "75/5m", (String value) -> value.equals(UNLIMITED_COMPILATION_RATE_KEY) ? ScriptCache.UNLIMITED_COMPILATION_RATE: - MAX_COMPILATION_RATE_FUNCTION.apply(value), + new ScriptCache.CompilationRate(value), Property.NodeScope, Property.Dynamic)); - private static final Tuple SCRIPT_COMPILATION_RATE_ZERO = new Tuple<>(0, TimeValue.ZERO); + private static final ScriptCache.CompilationRate SCRIPT_COMPILATION_RATE_ZERO = new ScriptCache.CompilationRate(0, TimeValue.ZERO); public static final Setting SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING = Setting.boolSetting("script.disable_max_compilations_rate", false, Property.NodeScope); @@ -560,14 +522,15 @@ ScriptCache contextCache(Settings settings, ScriptContext context) { TimeValue cacheExpire = cacheExpireSetting.existsOrFallbackExists(settings) ? cacheExpireSetting.get(settings) : context.cacheExpireDefault; - Setting> rateSetting = SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(context.name); - Tuple rate = null; + Setting rateSetting = + SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(context.name); + ScriptCache.CompilationRate rate = null; if (SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING.get(settings) || compilationLimitsEnabled() == false) { rate = SCRIPT_COMPILATION_RATE_ZERO; } else if (rateSetting.existsOrFallbackExists(settings)) { rate = rateSetting.get(settings); } else { - rate = context.maxCompilationRateDefault; + rate = new ScriptCache.CompilationRate(context.maxCompilationRateDefault); } return new ScriptCache(cacheSize, cacheExpire, rate, rateSetting.getKey()); diff --git a/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java b/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java index e57096d2e6175..17dfa08b93fd2 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java @@ -19,7 +19,6 @@ package org.elasticsearch.script; import org.elasticsearch.common.breaker.CircuitBreakingException; -import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; @@ -38,27 +37,29 @@ public void testCompilationCircuitBreaking() throws Exception { ).name; final TimeValue expire = ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace(context).get(Settings.EMPTY); final Integer size = ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(context).get(Settings.EMPTY); - Setting> rateSetting = + Setting rateSetting = ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(context); - Tuple rate = + ScriptCache.CompilationRate rate = ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(context).get(Settings.EMPTY); String rateSettingName = rateSetting.getKey(); - ScriptCache cache = new ScriptCache(size, expire, Tuple.tuple(1, TimeValue.timeValueMinutes(1)), rateSettingName); + ScriptCache cache = new ScriptCache(size, expire, + new ScriptCache.CompilationRate(1, TimeValue.timeValueMinutes(1)), rateSettingName); cache.checkCompilationLimit(); // should pass expectThrows(CircuitBreakingException.class, cache::checkCompilationLimit); - cache = new ScriptCache(size, expire, (Tuple.tuple(2, TimeValue.timeValueMinutes(1))), rateSettingName); + cache = new ScriptCache(size, expire, new ScriptCache.CompilationRate(2, TimeValue.timeValueMinutes(1)), rateSettingName); cache.checkCompilationLimit(); // should pass cache.checkCompilationLimit(); // should pass expectThrows(CircuitBreakingException.class, cache::checkCompilationLimit); int count = randomIntBetween(5, 50); - cache = new ScriptCache(size, expire, (Tuple.tuple(count, TimeValue.timeValueMinutes(1))), rateSettingName); + cache = new ScriptCache(size, expire, new ScriptCache.CompilationRate(count, TimeValue.timeValueMinutes(1)), rateSettingName); for (int i = 0; i < count; i++) { cache.checkCompilationLimit(); // should pass } expectThrows(CircuitBreakingException.class, cache::checkCompilationLimit); - cache = new ScriptCache(size, expire, (Tuple.tuple(0, TimeValue.timeValueMinutes(1))), rateSettingName); + cache = new ScriptCache(size, expire, new ScriptCache.CompilationRate(0, TimeValue.timeValueMinutes(1)), rateSettingName); expectThrows(CircuitBreakingException.class, cache::checkCompilationLimit); - cache = new ScriptCache(size, expire, (Tuple.tuple(Integer.MAX_VALUE, TimeValue.timeValueMinutes(1))), rateSettingName); + cache = new ScriptCache(size, expire, + new ScriptCache.CompilationRate(Integer.MAX_VALUE, TimeValue.timeValueMinutes(1)), rateSettingName); int largeLimit = randomIntBetween(1000, 10000); for (int i = 0; i < largeLimit; i++) { cache.checkCompilationLimit(); diff --git a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java index 3248df1bbaa05..2bc5d7105a01f 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java @@ -26,7 +26,6 @@ import org.elasticsearch.common.breaker.CircuitBreakingException; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; -import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; @@ -46,7 +45,6 @@ import java.util.function.Function; import java.util.stream.Collectors; -import static org.elasticsearch.script.ScriptService.MAX_COMPILATION_RATE_FUNCTION; import static org.elasticsearch.script.ScriptService.SCRIPT_CACHE_EXPIRE_SETTING; import static org.elasticsearch.script.ScriptService.SCRIPT_CACHE_SIZE_SETTING; import static org.elasticsearch.script.ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING; @@ -105,8 +103,8 @@ protected StoredScriptSource getScriptFromClusterState(String id) { } public void testMaxCompilationRateSetting() throws Exception { - assertThat(MAX_COMPILATION_RATE_FUNCTION.apply("10/1m"), is(Tuple.tuple(10, TimeValue.timeValueMinutes(1)))); - assertThat(MAX_COMPILATION_RATE_FUNCTION.apply("10/60s"), is(Tuple.tuple(10, TimeValue.timeValueMinutes(1)))); + assertThat(new ScriptCache.CompilationRate("10/1m"), is(new ScriptCache.CompilationRate(10, TimeValue.timeValueMinutes(1)))); + assertThat(new ScriptCache.CompilationRate("10/60s"), is(new ScriptCache.CompilationRate(10, TimeValue.timeValueMinutes(1)))); assertException("10/m", IllegalArgumentException.class, "failed to parse [m]"); assertException("6/1.6m", IllegalArgumentException.class, "failed to parse [1.6m], fractional time values are not supported"); assertException("foo/bar", IllegalArgumentException.class, "could not parse [foo] as integer in value [foo/bar]"); @@ -126,7 +124,7 @@ public void testMaxCompilationRateSetting() throws Exception { } private void assertException(String rate, Class clazz, String message) { - Exception e = expectThrows(clazz, () -> MAX_COMPILATION_RATE_FUNCTION.apply(rate)); + Exception e = expectThrows(clazz, () -> new ScriptCache.CompilationRate(rate)); assertThat(e.getMessage(), is(message)); } @@ -379,19 +377,6 @@ public void testMaxSizeLimit() throws Exception { iae.getMessage()); } - public void testUseContextSettingValue() { - Settings s = Settings.builder() - .put(ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("foo").getKey(), - ScriptService.USE_CONTEXT_RATE_KEY) - .build(); - - IllegalArgumentException illegal = expectThrows(IllegalArgumentException.class, () -> { - ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getAsMap(s); - }); - - assertEquals("parameter must contain a positive integer and a timevalue, i.e. 10/1m, but was [use-context]", illegal.getMessage()); - } - public void testCacheHolderContextConstructor() throws IOException { String a = randomFrom(contexts.keySet()); String b = randomValueOtherThan(a, () -> randomFrom(contexts.keySet())); @@ -406,9 +391,9 @@ public void testCacheHolderContextConstructor() throws IOException { assertNotNull(scriptService.cacheHolder.get().contextCache); assertEquals(contexts.keySet(), scriptService.cacheHolder.get().contextCache.keySet()); - assertEquals(ScriptService.MAX_COMPILATION_RATE_FUNCTION.apply(aCompilationRate), + assertEquals(new ScriptCache.CompilationRate(aCompilationRate), scriptService.cacheHolder.get().contextCache.get(a).get().rate); - assertEquals(ScriptService.MAX_COMPILATION_RATE_FUNCTION.apply(bCompilationRate), + assertEquals(new ScriptCache.CompilationRate(bCompilationRate), scriptService.cacheHolder.get().contextCache.get(b).get().rate); } @@ -457,10 +442,9 @@ public void testCacheHolderChangeSettings() throws IOException { ); assertEquals(contexts.keySet(), scriptService.cacheHolder.get().contextCache.keySet()); - String d = randomValueOtherThanMany(Set.of(a, b, c)::contains, () -> randomFrom(contextNames)); - assertEquals(ScriptService.MAX_COMPILATION_RATE_FUNCTION.apply(aRate), + assertEquals(new ScriptCache.CompilationRate(aRate), scriptService.cacheHolder.get().contextCache.get(a).get().rate); - assertEquals(ScriptService.MAX_COMPILATION_RATE_FUNCTION.apply(bRate), + assertEquals(new ScriptCache.CompilationRate(bRate), scriptService.cacheHolder.get().contextCache.get(b).get().rate); assertEquals(ScriptCache.UNLIMITED_COMPILATION_RATE, scriptService.cacheHolder.get().contextCache.get(c).get().rate); @@ -468,13 +452,13 @@ public void testCacheHolderChangeSettings() throws IOException { scriptService.cacheHolder.get().set(b, scriptService.contextCache(Settings.builder() .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(b).getKey(), aRate).build(), contexts.get(b))); - assertEquals(ScriptService.MAX_COMPILATION_RATE_FUNCTION.apply(aRate), + assertEquals(new ScriptCache.CompilationRate(aRate), scriptService.cacheHolder.get().contextCache.get(b).get().rate); } public void testFallbackToContextDefaults() throws IOException { String contextRateStr = randomIntBetween(10, 1024) + "/" + randomIntBetween(10, 200) + "m"; - Tuple contextRate = MAX_COMPILATION_RATE_FUNCTION.apply(contextRateStr); + ScriptCache.CompilationRate contextRate = new ScriptCache.CompilationRate(contextRateStr); int contextCacheSize = randomIntBetween(1, 1024); TimeValue contextExpire = TimeValue.timeValueMinutes(randomIntBetween(10, 200)); @@ -511,7 +495,7 @@ public void testFallbackToContextDefaults() throws IOException { assertNotNull(holder.contextCache.get(name)); assertNotNull(holder.contextCache.get(name).get()); - assertEquals(ingest.maxCompilationRateDefault, holder.contextCache.get(name).get().rate); + assertEquals(ingest.maxCompilationRateDefault, holder.contextCache.get(name).get().rate.asTuple()); assertEquals(ingest.cacheSizeDefault, holder.contextCache.get(name).get().cacheSize); assertEquals(ingest.cacheExpireDefault, holder.contextCache.get(name).get().cacheExpire); }