Script: Opt-out system contexts from script compilation rate limit by stu-elastic · Pull Request #79459 · elastic/elasticsearch

stu-elastic · 2021-10-19T12:22:41Z

System script contexts can now opt-out of compilation rate limiting using a flag rather than a sentinel rate limit value.

Duplicated defaults for system contexts have been coalesced.

Refs: #62899

Restore the scripting general cache in preparation for deprecation of the context cache, then removal of the context cache in master. Brings back the general cache settings: * `script.max_compilations_rate` * `script.cache.expire` * `script.cache.max_size` `script.cache.max_size` and `script.cache.expire` are now dynamic settings. The context cache is still default, the switch to general cache as default will happen on context cache deprecation. System script contexts can now opt-out of compilation rate limiting using a flag rather than a sentinel rate limit value. Duplicated defaults for system contexts have been coalesced. Other than that, this code is the same as what was in 7.x to make a `master`-first commit and backport strategy easy. Refs: elastic#62899 Backport: elastic#79414

elasticmachine · 2021-10-19T16:11:54Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

rjernst

It seems like all but runtime fields have the flag backwards, it should be false to not use rate limited, right?

rjernst · 2021-10-19T16:24:43Z

server/src/main/java/org/elasticsearch/script/IngestConditionalScript.java

    /** The context used to compile {@link IngestConditionalScript} factories. */
-    public static final ScriptContext<Factory> CONTEXT = new ScriptContext<>("processor_conditional", Factory.class,
-        200, TimeValue.timeValueMillis(0), ScriptCache.UNLIMITED_COMPILATION_RATE.asTuple(), true);
+    public static final ScriptContext<Factory> CONTEXT = new ScriptContext<>("processor_conditional", Factory.class, true);


Shouldn't this be false to maintain not using compilation limits?

That's the flag for stored, there is no flag for not using compilation limits in that constructor.

Switched to full constructor, value is explicitly false.

rjernst · 2021-10-19T16:24:49Z

server/src/main/java/org/elasticsearch/script/IngestScript.java

    /** The context used to compile {@link IngestScript} factories. */
-    public static final ScriptContext<Factory> CONTEXT = new ScriptContext<>("ingest", Factory.class,
-        200, TimeValue.timeValueMillis(0), ScriptCache.UNLIMITED_COMPILATION_RATE.asTuple(), true);
+    public static final ScriptContext<Factory> CONTEXT = new ScriptContext<>("ingest", Factory.class, true);


Shouldn't this be false to maintain not using compilation limits?

It's calling the following constructor and allowing stored scripts.

https://github.com/elastic/elasticsearch/pull/79459/files#diff-b39b841c6481b6822b5d879ccf88d4b5b7d17c91448c2ffc27f82f0bfea7d138R120

public ScriptContext(String name, Class<FactoryType> factoryClazz, boolean allowStoredScript) { this(name, factoryClazz, 200, TimeValue.timeValueMillis(0), false, allowStoredScript); } }

https://github.com/elastic/elasticsearch/pull/79459/files#diff-b39b841c6481b6822b5d879ccf88d4b5b7d17c91448c2ffc27f82f0bfea7d138R109

public ScriptContext(String name, Class<FactoryType> factoryClazz, int cacheSizeDefault, TimeValue cacheExpireDefault, ScriptContext(String name, Class<FactoryType> factoryClazz, int cacheSizeDefault, TimeValue cacheExpireDefault, Tuple<Integer, TimeValue> maxCompilationRateDefault, boolean allowStoredScript) { boolean compilationRateLimited, boolean allowStoredScript)

Switched to full constructor, value is explicitly false.

rjernst · 2021-10-19T16:25:40Z

server/src/main/java/org/elasticsearch/script/ScriptStats.java

        builder.field(Fields.COMPILATIONS, compilations);
        builder.field(Fields.CACHE_EVICTIONS, cacheEvictions);
        builder.field(Fields.COMPILATION_LIMIT_TRIGGERED, compilationLimitTriggered);
+        /* TODO(stu): master only


rjernst · 2021-10-19T16:25:53Z

server/src/main/java/org/elasticsearch/script/TemplateScript.java

    // creating a new Script class (as would be customary), this context is used to avoid the default rate limit.
-    public static final ScriptContext<Factory> INGEST_CONTEXT = new ScriptContext<>("ingest_template", Factory.class,
-            200, TimeValue.timeValueMillis(0), ScriptCache.UNLIMITED_COMPILATION_RATE.asTuple(), true);
+    public static final ScriptContext<Factory> INGEST_CONTEXT = new ScriptContext<>("ingest_template", Factory.class, true);


Shouldn't this be false to maintain not using compilation limits?

That's for allowing it to be stored.

Switched to full constructor, value is explicitly false.

rjernst · 2021-10-19T16:26:31Z

server/src/test/java/org/elasticsearch/script/ScriptStatsTests.java

        stats.toXContent(builder, ToXContent.EMPTY_PARAMS);
        builder.endObject();

+        /* TODO(stu): master only


rjernst · 2021-10-19T16:26:41Z

x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java

    public static final ScriptContext<TemplateScript.Factory> SCRIPT_TEMPLATE_CONTEXT
-        = new ScriptContext<>("xpack_template", TemplateScript.Factory.class,
-        200, TimeValue.timeValueMillis(0), ScriptCache.UNLIMITED_COMPILATION_RATE.asTuple(), true);
+        = new ScriptContext<>("xpack_template", TemplateScript.Factory.class, true);


Shouldn't this be false to maintain not using compilation limits?

That's for allowing it to be stored.

Switched to full constructor, value is explicitly false.

rjernst · 2021-10-19T16:26:49Z

.../watcher/src/main/java/org/elasticsearch/xpack/watcher/condition/WatcherConditionScript.java


-    public static ScriptContext<Factory> CONTEXT = new ScriptContext<>("watcher_condition", Factory.class,
-        200, TimeValue.timeValueMillis(0), ScriptCache.UNLIMITED_COMPILATION_RATE.asTuple(), true);
+    public static ScriptContext<Factory> CONTEXT = new ScriptContext<>("watcher_condition", Factory.class, true);


Shouldn't this be false to maintain not using compilation limits?

That's for allowing it to be stored.

Switched to full constructor, value is explicitly false.

rjernst · 2021-10-19T16:26:59Z

...r/src/main/java/org/elasticsearch/xpack/watcher/transform/script/WatcherTransformScript.java


-    public static ScriptContext<Factory> CONTEXT = new ScriptContext<>("watcher_transform", Factory.class,
-        200, TimeValue.timeValueMillis(0), ScriptCache.UNLIMITED_COMPILATION_RATE.asTuple(), true);
+    public static ScriptContext<Factory> CONTEXT = new ScriptContext<>("watcher_transform", Factory.class, true);


Shouldn't this be false to maintain not using compilation limits?

That's for allowing it to be stored.

Switched to full constructor, value is explicitly false.

rjernst · 2021-10-19T16:34:22Z

server/src/main/java/org/elasticsearch/script/ScriptContext.java

+     *  - default cache expiration: no expiration
+     *  - unlimited compilation rate
+     */
+    public ScriptContext(String name, Class<FactoryType> factoryClazz, boolean allowStoredScript) {


I don't think we need another constructor? Let's just update the few places you already have modified in this PR to be explicit about the settings for each context.

I added this constructor to avoid having two boolean flags in a row, which (despite intellij) is a bit hard to read.

Will update.

rjernst

LGTM

Deprecate the script context cache in favor of the general cache. Users should use the following settings: `script.max_compilations_rate` to set the max compilation rate for user scripts such as filter scripts. Certain script contexts that submit scripts outside of the control of the user are exempted from this rate limit. Examples include runtime fields, ingest and watcher. `script.cache.max_size` to set the max size of the cache. `script.cache.expire` to set the expiration time for entries in the cache. Whats deprecated? `script.max_compilations_rate: use-context`. This special setting value was used to turn on the script context-specific caches. `script.context.$CONTEXT.cache_max_size`, use `script.cache.max_size` instead. `script.context.$CONTEXT.cache_expire`, use `script.cache.expire` instead. `script.context.$CONTEXT.max_compilations_rate`, use `script.max_compilations_rate` instead. The default cache size was increased from `100` to `3000`, which was approximately the max cache size when using context-specific caches. The default compilation rate limit was increased from `75/5m` to `150/5m` to account for increasing uses of scripts. System script contexts can now opt-out of compilation rate limiting using a flag rather than a sentinel rate limit value. 7.16: Script: Deprecate script context cache elastic#79508 Refs: elastic#62899 7.16: Script: Opt-out system contexts from script compilation rate limit elastic#79459 Refs: elastic#62899

Deprecate the script context cache in favor of the general cache. Users should use the following settings: `script.max_compilations_rate` to set the max compilation rate for user scripts such as filter scripts. Certain script contexts that submit scripts outside of the control of the user are exempted from this rate limit. Examples include runtime fields, ingest and watcher. `script.cache.max_size` to set the max size of the cache. `script.cache.expire` to set the expiration time for entries in the cache. Whats deprecated? `script.max_compilations_rate: use-context`. This special setting value was used to turn on the script context-specific caches. `script.context.$CONTEXT.cache_max_size`, use `script.cache.max_size` instead. `script.context.$CONTEXT.cache_expire`, use `script.cache.expire` instead. `script.context.$CONTEXT.max_compilations_rate`, use `script.max_compilations_rate` instead. The default cache size was increased from `100` to `3000`, which was approximately the max cache size when using context-specific caches. The default compilation rate limit was increased from `75/5m` to `150/5m` to account for increasing uses of scripts. System script contexts can now opt-out of compilation rate limiting using a flag rather than a sentinel rate limit value. 7.16: Script: Deprecate script context cache #79508 Refs: #62899 7.16: Script: Opt-out system contexts from script compilation rate limit #79459 Refs: #62899

elasticsearchmachine added the v7.16.1 label Oct 19, 2021

stu-elastic changed the title ~~Script: Restore the scripting general cache~~ Script: Restore the scripting general cache (backport) Oct 19, 2021

Import Collectors

0a81b51

stu-elastic changed the title ~~Script: Restore the scripting general cache (backport)~~ Script: Opt-out system contexts from script compilation rate limit Oct 19, 2021

stu-elastic added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v7.16.0 labels Oct 19, 2021

stu-elastic marked this pull request as ready for review October 19, 2021 16:11

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Oct 19, 2021

stu-elastic requested a review from rjernst October 19, 2021 16:11

rjernst reviewed Oct 19, 2021

View reviewed changes

Use full ScriptContext constructor, remove comments

3291623

rjernst approved these changes Oct 19, 2021

View reviewed changes

stu-elastic merged commit 8601c9b into elastic:7.x Oct 19, 2021

This was referenced Oct 20, 2021

Provide better compilation and cache stats to remove compiation rate limits #62899

Closed

Script: Restore the scripting general cache #79453

Merged

danhermann removed the v7.16.1 label Oct 27, 2021

danhermann added the >non-issue label Dec 3, 2021

Conversation

stu-elastic commented Oct 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Oct 19, 2021

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

stu-elastic commented Oct 19, 2021 •

edited

Loading