From 2595b18131c32afcc96dbc862f2839a2374ae911 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Wed, 8 Dec 2021 11:39:26 -0600 Subject: [PATCH] [7.16] Script: Implicit context cache (#81552) The script context cache is deprecated. In 7.16 the default value of `script.max_compilation_rate` was switched from `"use-context"`, to `"150/5m". That means uses would have to explicitly set "use-context" to use any of the context cache families of settings: `script.context.*.max_compilations_rate` `script.context.*.cache_max_size` `script.context.*.cache_expire` On upgrades to 7.16, if the customer was using the default and had set any of those settings without setting `script.max_compilation_rate: "use-context"`, the upgrade would be rejected. To avoid an unintentional breaking change, the script service will now **implicitly** use the script context cache if `script.max_compilation_rate` is **unset** and any of the context cache family of settings is set. The context cache will also be used if `script.max_compilation_rate: "use-context"`, as before. Fixes: #81486 Backport: ba6c17d --- .../elasticsearch/script/ScriptService.java | 197 ++++++++++++++---- .../script/ScriptServiceTests.java | 90 ++++++-- .../deprecation/NodeDeprecationChecks.java | 16 +- .../NodeDeprecationChecksTests.java | 27 +++ 4 files changed, 271 insertions(+), 59 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/script/ScriptService.java b/server/src/main/java/org/elasticsearch/script/ScriptService.java index cb7fa217b4151..5d7479737fe8d 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptService.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptService.java @@ -277,6 +277,14 @@ public static boolean isUseContextCacheSet(Settings settings) { return SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings).equals(USE_CONTEXT_RATE_VALUE); } + public static boolean isImplicitContextCacheSet(Settings settings) { + return new ScriptService.ContextSettings(settings).implicitContextCache(); + } + + public static String contextDeprecationMessage(Settings settings) { + return new ScriptService.ContextSettings(settings).deprecationMessage(); + } + /** * This is overridden in tests to disable compilation rate limiting. */ @@ -323,53 +331,26 @@ void registerClusterSettingsListeners(ClusterSettings clusterSettings) { * when using the general cache. */ void validateCacheSettings(Settings settings) { - boolean useContext = SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings).equals(USE_CONTEXT_RATE_VALUE); - if (useContext) { + ContextSettings contextSettings = new ContextSettings(settings, contexts.keySet()); + if (contextSettings.useContextSet) { deprecationLogger.warn(DeprecationCategory.SCRIPTING, "scripting-context-cache", USE_CONTEXT_RATE_KEY_DEPRECATION_MESSAGE); + } else if (contextSettings.hasContextSettings()) { + deprecationLogger.warn(DeprecationCategory.SCRIPTING, "scripting-context-cache", contextSettings.deprecationMessage()); } - List> affixes = Arrays.asList( - SCRIPT_MAX_COMPILATIONS_RATE_SETTING, - SCRIPT_CACHE_EXPIRE_SETTING, - SCRIPT_CACHE_SIZE_SETTING - ); - List customRates = new ArrayList<>(); - List keys = new ArrayList<>(); - for (Setting.AffixSetting affix : affixes) { - for (String context : affix.getAsMap(settings).keySet()) { - String s = affix.getConcreteSettingForNamespace(context).getKey(); - if (contexts.containsKey(context) == false) { - throw new IllegalArgumentException("Context [" + context + "] doesn't exist for setting [" + s + "]"); - } - keys.add(s); - if (affix.equals(SCRIPT_MAX_COMPILATIONS_RATE_SETTING)) { - customRates.add(s); - } - } - } - if (useContext == false && keys.isEmpty() == false) { - keys.sort(Comparator.naturalOrder()); - throw new IllegalArgumentException( - "Context cache settings [" - + String.join(", ", keys) - + "] requires [" - + SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey() - + "] to be [" - + USE_CONTEXT_RATE_KEY - + "]" - ); + if (contextSettings.incompatibleSettings()) { + throw new IllegalArgumentException(contextSettings.incompatibleSettingsMessage()); } if (SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING.get(settings)) { - if (customRates.size() > 0) { - customRates.sort(Comparator.naturalOrder()); + if (contextSettings.compilationContexts.size() > 0) { throw new IllegalArgumentException( "Cannot set custom context compilation rates [" - + String.join(", ", customRates) + + String.join(", ", contextSettings.contextCompilationKeys()) + "] if compile rates disabled via [" + SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING.getKey() + "]" ); } - if (useContext == false && SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.exists(settings)) { + if (contextSettings.useContextSet == false && contextSettings.isGeneralCompilationRateSet) { throw new IllegalArgumentException( "Cannot set custom general compilation rates [" + SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey() @@ -383,6 +364,144 @@ void validateCacheSettings(Settings settings) { } } + /** + * Collect settings related to script context and general caches. + * + * The general cache is used by default. + * The context cache is used if {@code script.max_compilations_rate} is {@code "use-context"}, a deprecated value. + * The context cache is used implicitly if {@code script.max_compilations_rate} is unset and any of the context + * cache family of settings is used: + * {@code script.context.*.max_compilations_rate}, {@link ScriptService#SCRIPT_MAX_COMPILATIONS_RATE_SETTING} + * {@code script.context.*.cache_max_size}, {@link ScriptService#SCRIPT_CACHE_SIZE_SETTING} + * {@code script.context.*.cache_expire}, {@link ScriptService#SCRIPT_CACHE_EXPIRE_SETTING} + */ + public static class ContextSettings { + public final Settings settings; + public final boolean useContextSet; + public final boolean isGeneralCompilationRateSet; + public final ScriptCache.CompilationRate generalCompilationRate; + public final List compilationContexts; + public final List sizeContexts; + public final List expireContexts; + + public ContextSettings(Settings settings, Set contexts) { + this.settings = settings; + generalCompilationRate = SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings); + useContextSet = generalCompilationRate.equals(USE_CONTEXT_RATE_VALUE); + isGeneralCompilationRateSet = SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.exists(settings); + compilationContexts = getContexts(SCRIPT_MAX_COMPILATIONS_RATE_SETTING, settings, contexts); + sizeContexts = getContexts(SCRIPT_CACHE_SIZE_SETTING, settings, contexts); + expireContexts = getContexts(SCRIPT_CACHE_EXPIRE_SETTING, settings, contexts); + } + + public ContextSettings(Settings settings) { + this(settings, Collections.emptySet()); + } + + protected static List getContexts(Setting.AffixSetting setting, Settings settings, Set contexts) { + List contextSettings = new ArrayList<>(); + for (String context : setting.getAsMap(settings).keySet()) { + if (contexts.isEmpty() == false && contexts.contains(context) == false) { + String settingKey = setting.getConcreteSettingForNamespace(context).getKey(); + throw new IllegalArgumentException("Context [" + context + "] doesn't exist for setting [" + settingKey + "]"); + } + contextSettings.add(context); + } + contextSettings.sort(Comparator.naturalOrder()); + return contextSettings; + } + + /** Are there any context specific settings */ + public boolean hasContextSettings() { + return compilationContexts.isEmpty() == false || expireContexts.isEmpty() == false || sizeContexts.isEmpty() == false; + } + + /** deprecation message for implicitly using the context cache */ + public String deprecationMessage() { + // Implicitly using the script context cache is deprecated, remove the following deprecated settings to use the script general + // cache. + if (hasContextSettings() == false) { + return ""; + } + List settingsKeys = new ArrayList<>(); + settingsKeys.addAll(fullKeys(SCRIPT_MAX_COMPILATIONS_RATE_SETTING, compilationContexts)); + settingsKeys.addAll(fullKeys(SCRIPT_CACHE_SIZE_SETTING, sizeContexts)); + settingsKeys.addAll(fullKeys(SCRIPT_CACHE_EXPIRE_SETTING, expireContexts)); + settingsKeys.sort(Comparator.naturalOrder()); + return "Implicitly using the script context cache is deprecated, remove settings " + + "[" + + String.join(", ", settingsKeys) + + "] to use the script general cache."; + } + + /** the context specific max compilation keys */ + public List contextCompilationKeys() { + return fullKeys(SCRIPT_MAX_COMPILATIONS_RATE_SETTING, compilationContexts); + } + + /** the full keys for the contexts in the context affix setting */ + protected static List fullKeys(Setting.AffixSetting affix, List contexts) { + return contexts.stream().map(ctx -> affix.getConcreteSettingForNamespace(ctx).getKey()).collect(Collectors.toList()); + } + + /** + * Should the context cache be used? This is true if "use-context" is set explicitly or implicitly, see above for implicit + * definition. + */ + public boolean useContextCache() { + return useContextSet || implicitContextCache(); + } + + /** + * Implicitly use the script context cache. False if context cache is explicitly used as well as context cache is unused. + */ + public boolean implicitContextCache() { + return useContextSet == false && hasContextSettings() && isGeneralCompilationRateSet == false; + } + + /** + * Is the set of settings incompatible? This is the case if: + * 1) {@code script.max_compilations_rate}, {@link ScriptService#SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING} is set but not + * set to "use-context". + * 2) Any of the context cache family of settings is set. + */ + public boolean incompatibleSettings() { + return useContextSet == false && hasContextSettings() && isGeneralCompilationRateSet; + } + + /** + * All context specific settings + */ + public List contextSettings() { + List contextSettings = fullKeys(SCRIPT_MAX_COMPILATIONS_RATE_SETTING, compilationContexts); + contextSettings.addAll(fullKeys(SCRIPT_CACHE_SIZE_SETTING, sizeContexts)); + contextSettings.addAll(fullKeys(SCRIPT_CACHE_EXPIRE_SETTING, expireContexts)); + return contextSettings; + } + + /** + * Error message if there are incompatible settings. + */ + public String incompatibleSettingsMessage() { + if (incompatibleSettings() == false) { + return ""; + } + List incompatible = contextSettings(); + return "Context cache settings [" + + String.join(",", incompatible) + + "] are incompatible with [" + + SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey() + + "] set to non-default value [" + + generalCompilationRate + + "]." + + " Either remove the incompatible settings (recommended) or set [" + + SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey() + + "] to [" + + USE_CONTEXT_RATE_KEY + + "] to use per-context settings"; + } + } + @Override public void close() throws IOException { IOUtils.close(engines.values()); @@ -670,10 +789,10 @@ public void applyClusterState(ClusterChangedEvent event) { void setCacheHolder(Settings settings) { CacheHolder current = cacheHolder.get(); - boolean useContext = SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings).equals(USE_CONTEXT_RATE_VALUE); + ContextSettings contextSettings = new ContextSettings(settings, contexts.keySet()); if (current == null) { - if (useContext) { + if (contextSettings.useContextCache()) { cacheHolder.set(contextCacheHolder(settings)); } else { cacheHolder.set(generalCacheHolder(settings)); @@ -682,7 +801,7 @@ void setCacheHolder(Settings settings) { } // Update - if (useContext) { + if (contextSettings.useContextCache()) { if (current.general != null) { // Flipping to context specific cacheHolder.set(contextCacheHolder(settings)); diff --git a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java index 2ce18979099f6..e412b245b276a 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java @@ -27,6 +27,7 @@ import org.junit.Before; import java.io.IOException; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -500,45 +501,44 @@ public void testMaxSizeLimit() throws Exception { } public void testConflictContextSettings() throws IOException { + String fieldCacheKey = ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace("field").getKey(); IllegalArgumentException illegal = expectThrows(IllegalArgumentException.class, () -> { buildScriptService( - Settings.builder() - .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), "10/1m") - .put(ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace("field").getKey(), 123) - .build() + Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), "10/1m").put(fieldCacheKey, 123).build() ); }); assertEquals( - "Context cache settings [script.context.field.cache_max_size] requires " + "[script.max_compilations_rate] to be [use-context]", + "Context cache settings [script.context.field.cache_max_size] are incompatible with [script.max_compilations_rate]" + + " set to non-default value [10/1m]. Either remove the incompatible settings (recommended) or set" + + " [script.max_compilations_rate] to [use-context] to use per-context settings", illegal.getMessage() ); + String ingestExpireKey = ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace("ingest").getKey(); illegal = expectThrows(IllegalArgumentException.class, () -> { buildScriptService( - Settings.builder() - .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), "10/1m") - .put(ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace("ingest").getKey(), "5m") - .build() + Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), "10/1m").put(ingestExpireKey, "5m").build() ); }); assertEquals( - "Context cache settings [script.context.ingest.cache_expire] requires " + "[script.max_compilations_rate] to be [use-context]", + "Context cache settings [script.context.ingest.cache_expire] are incompatible with [script.max_compilations_rate]" + + " set to non-default value [10/1m]. Either remove the incompatible settings (recommended) or set" + + " [script.max_compilations_rate] to [use-context] to use per-context settings", illegal.getMessage() ); + String scoreCompileKey = ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("score").getKey(); illegal = expectThrows(IllegalArgumentException.class, () -> { buildScriptService( - Settings.builder() - .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), "10/1m") - .put(ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("score").getKey(), "50/5m") - .build() + Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), "10/1m").put(scoreCompileKey, "50/5m").build() ); }); assertEquals( - "Context cache settings [script.context.score.max_compilations_rate] requires " - + "[script.max_compilations_rate] to be [use-context]", + "Context cache settings [script.context.score.max_compilations_rate] are incompatible with [script.max_compilations_rate]" + + " set to non-default value [10/1m]. Either remove the incompatible settings (recommended) or set" + + " [script.max_compilations_rate] to [use-context] to use per-context settings", illegal.getMessage() ); @@ -556,10 +556,19 @@ public void testConflictContextSettings() throws IOException { ); assertSettingDeprecationsAndWarnings( new Setting[] { ingestExpire, fieldSize, scoreCompilation }, - new DeprecationWarning(Level.WARN, USE_CONTEXT_RATE_KEY_DEPRECATION_MESSAGE) + new DeprecationWarning(Level.WARN, USE_CONTEXT_RATE_KEY_DEPRECATION_MESSAGE), + new DeprecationWarning(Level.WARN, implicitContextCacheMessage(fieldCacheKey)), + new DeprecationWarning(Level.WARN, implicitContextCacheMessage(ingestExpireKey)), + new DeprecationWarning(Level.WARN, implicitContextCacheMessage(scoreCompileKey)) ); } + protected static String implicitContextCacheMessage(String... settings) { + return "Implicitly using the script context cache is deprecated, remove settings [" + + Arrays.stream(settings).sorted().collect(Collectors.joining(", ")) + + "] to use the script general cache."; + } + public void testFallbackContextSettings() { int cacheSizeBackup = randomIntBetween(0, 1024); int cacheSizeFoo = randomValueOtherThan(cacheSizeBackup, () -> randomIntBetween(0, 1024)); @@ -643,6 +652,53 @@ public void testCacheHolderContextConstructor() throws IOException { ); } + public void testImplicitContextCache() throws IOException { + String a = randomFrom(rateLimitedContexts.keySet()); + String b = randomValueOtherThan(a, () -> randomFrom(rateLimitedContexts.keySet())); + String aCompilationRate = "77/5m"; + String bCompilationRate = "78/6m"; + + Setting aSetting = SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(a); + Setting bSetting = SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(b); + buildScriptService(Settings.builder().put(aSetting.getKey(), aCompilationRate).put(bSetting.getKey(), bCompilationRate).build()); + + assertNull(scriptService.cacheHolder.get().general); + assertNotNull(scriptService.cacheHolder.get().contextCache); + assertEquals(contexts.keySet(), scriptService.cacheHolder.get().contextCache.keySet()); + + assertEquals(new ScriptCache.CompilationRate(aCompilationRate), scriptService.cacheHolder.get().contextCache.get(a).get().rate); + assertEquals(new ScriptCache.CompilationRate(bCompilationRate), scriptService.cacheHolder.get().contextCache.get(b).get().rate); + assertSettingDeprecationsAndWarnings( + new Setting[] { aSetting, bSetting }, + new DeprecationWarning(Level.WARN, implicitContextCacheMessage(aSetting.getKey(), bSetting.getKey())) + ); + } + + public void testImplicitContextCacheWithoutCompilationRate() throws IOException { + String a = randomFrom(rateLimitedContexts.keySet()); + String b = randomValueOtherThan(a, () -> randomFrom(rateLimitedContexts.keySet())); + String aExpire = "20m"; + int bSize = 2000; + + Setting aSetting = SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace(a); + Setting bSetting = SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(b); + buildScriptService(Settings.builder().put(aSetting.getKey(), aExpire).put(bSetting.getKey(), bSize).build()); + + assertNull(scriptService.cacheHolder.get().general); + assertNotNull(scriptService.cacheHolder.get().contextCache); + assertEquals(contexts.keySet(), scriptService.cacheHolder.get().contextCache.keySet()); + + assertEquals( + TimeValue.parseTimeValue("20m", aSetting.getKey()), + scriptService.cacheHolder.get().contextCache.get(a).get().cacheExpire + ); + assertEquals(bSize, scriptService.cacheHolder.get().contextCache.get(b).get().cacheSize); + assertSettingDeprecationsAndWarnings( + new Setting[] { aSetting, bSetting }, + new DeprecationWarning(Level.WARN, implicitContextCacheMessage(aSetting.getKey(), bSetting.getKey())) + ); + } + public void testCompilationRateUnlimitedContextOnly() throws IOException { IllegalArgumentException illegal = expectThrows(IllegalArgumentException.class, () -> { buildScriptService( diff --git a/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java b/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java index 6b2cb0edfe599..d4ceafdced163 100644 --- a/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java +++ b/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java @@ -1251,14 +1251,24 @@ static DeprecationIssue checkScriptContextCache( final ClusterState clusterState, final XPackLicenseState licenseState ) { + String detail = "Remove the context-specific cache settings and set [script.max_compilations_rate] to configure the rate limit for the " + + "general cache. If no limit is set, the rate defaults to 150 compilations per five minutes: 150/5m. Context-specific " + + "caches are no longer needed to prevent system scripts from triggering rate limits."; if (ScriptService.isUseContextCacheSet(settings)) { return new DeprecationIssue( DeprecationIssue.Level.WARNING, ScriptService.USE_CONTEXT_RATE_KEY_DEPRECATION_MESSAGE, "https://ela.st/es-deprecation-7-script-context-cache", - "Remove the context-specific cache settings and set [script.max_compilations_rate] to configure the rate limit for the " - + "general cache. If no limit is set, the rate defaults to 150 compilations per five minutes: 150/5m. Context-specific " - + "caches are no longer needed to prevent system scripts from triggering rate limits.", + detail, + false, + null + ); + } else if (ScriptService.isImplicitContextCacheSet(settings)) { + return new DeprecationIssue( + DeprecationIssue.Level.WARNING, + ScriptService.contextDeprecationMessage(settings), + "https://ela.st/es-deprecation-7-script-context-cache", + detail, false, null ); diff --git a/x-pack/plugin/deprecation/src/test/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecksTests.java b/x-pack/plugin/deprecation/src/test/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecksTests.java index 2ca87119ce62e..b58ed0f8d84ab 100644 --- a/x-pack/plugin/deprecation/src/test/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecksTests.java +++ b/x-pack/plugin/deprecation/src/test/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecksTests.java @@ -1615,6 +1615,33 @@ public void testScriptContextCacheSetting() { ); } + public void testImplicitScriptContextCacheSetting() { + List contexts = org.elasticsearch.core.List.of("update", "filter"); + Settings settings = Settings.builder() + .put(ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(contexts.get(0)).getKey(), "123/5m") + .put(ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(contexts.get(1)).getKey(), "2453") + .build(); + + assertThat( + NodeDeprecationChecks.checkScriptContextCache(settings, null, null, null), + equalTo( + new DeprecationIssue( + DeprecationIssue.Level.WARNING, + "Implicitly using the script context cache is deprecated, remove settings " + + "[script.context.update.max_compilations_rate, script.context.filter.cache_max_size] " + + "to use the script general cache.", + "https://ela.st/es-deprecation-7-script-context-cache", + "Remove the context-specific cache settings and set [script.max_compilations_rate] to configure the rate limit for " + + "the general cache. If no limit is set, the rate defaults to 150 compilations per five minutes: 150/5m. " + + "Context-specific caches are no longer needed to prevent system scripts from triggering rate limits.", + false, + null + ) + ) + ); + + } + public void testScriptContextCompilationsRateLimitSetting() { List contexts = org.elasticsearch.core.List.of("field", "score"); Settings settings = Settings.builder()