Script: Deprecate script context cache #79508

stu-elastic · 2021-10-19T18:59:21Z

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.

Refs: #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. Refs: elastic#62899

elasticmachine · 2021-10-19T18:59:25Z

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

rjernst

A few quick comments

rjernst · 2021-10-19T20:45:22Z

docs/reference/migration/migrate_7_16.asciidoc

@@ -321,4 +321,77 @@ avoid deprecation warnings and opt into the future behaviour, include the query
 parameter `?return_200_for_cluster_health_timeout` in your request.
 ====

+[[script-context-cache-removed]]
+.The `script.max_compilations_rate` setting cannot be `use-context`.


The value is deprecated, not removed right? The wording seems to imply it won't work, but it will, it should just be migrated away from.

Changed to "is a deprecated value".

rjernst · 2021-10-19T20:46:55Z

docs/reference/migration/migrate_7_16.asciidoc

+compilation rate limits and to compensate for an undersized general script
+cache.
+Now, system scripts are exempt from compilation rate limiting and
+the general script cache size is dynamically with a higher default


"size is dynamically with a higher default"? I'm not sure what this is supposed to mean.

changed to "is able to be set dynamically"

rjernst · 2021-10-19T20:51:37Z

docs/reference/migration/migrate_7_16.asciidoc

+`200/10m`, which allows 200 compilations for every 10 minute period.
+
+*Impact* +
+Discontinue use of the removed setting value. Specifying the setting value


This wording looks like a breakage, but it's just a deprecation. We should model it after other deprecation docs. For example, look at deprecate-transient-cluster-settings in this file. Ditto for all the other sections added in this file.

changed to To avoid deprecation warnings, discontinue use of the deprecated setting value.

rjernst · 2021-10-19T21:12:36Z

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

@@ -249,6 +263,10 @@ void registerClusterSettingsListeners(ClusterSettings clusterSettings) {
     */
    void validateCacheSettings(Settings settings) {
        boolean useContext = SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings).equals(USE_CONTEXT_RATE_VALUE);
+        if (useContext) {
+            deprecationLogger.critical(DeprecationCategory.SCRIPTING, "scripting-context-cache",


All of these critical should be warnings instead

stu-elastic · 2021-10-19T22:02:36Z

@elasticmachine update branch

stu-elastic · 2021-10-19T22:26:41Z

@elasticmachine update branch

…rated

rjernst

This looks good, but i would like @debadair to look over the migration docs wording/structure.

rjernst · 2021-10-19T22:49:00Z

docs/reference/migration/migrate_7_16.asciidoc

+
+Previously, you could set this setting to `use-context` to have a
+script cache, compilation rate limit settings, cache expiration and
+cache size per cluster.


per cluster -> per script context

rjernst · 2021-10-19T22:50:36Z

docs/reference/migration/migrate_7_16.asciidoc

+Now, system scripts are exempt from compilation rate limiting and
+the general script cache size is able to be set dynamically.
+
+Do not use `use-context` as a value for this setting.  Instead, remove


This paragraph should be moved/rephrased into the impact below. I would also remove the first sentence.

Removed first sentence, moved and expanded below.

rjernst · 2021-10-19T22:56:28Z

docs/reference/migration/migrate_7_16.asciidoc

+size.
+
+*Impact* +
+To avoid deprecation warnings, discontinue use of the deprecated settings.


The impact should be specific, not the same for every one of these migration notes. Maybe @debadair can provide some suggestions on wording. I'm also not sure if we should have a single note about script.context.$CONTEXT.* or have individual notes as you have here.

Made the impact more specific and recommended mitigation.

I'm also not sure if we should have a single note about script.context.$CONTEXT.* or have individual notes as you have here.

I made them different so the deprecation warnings could point to different sections. Since the deprecation warning URLs can easily be changed after release (they use https://ela.st/), I don't have a strong opinion.

I'd be inclined to consolidate this into a single entry. I think it can be streamlined quite a bit.

[[script-context-cache-deprecated]]
. The script context cache is deprecated
[%collapsible]
====
Details +
Setting script.max_compilations_rate to use-contextand
configuring context-specific caches is deprecated.
Context-specific caches are no longer needed to prevent system scripts
from triggering rate limits.
Configure the general cache to control the max complication rate, cache size, and
cache expiration for your scripts.

Impact +
Remove script.max_compilations_rate: use-context and the context-specific cache settings: script.context.$CONTEXT.cache_max_size, script.context.$CONTEXT.max_compilations_rate, script.context.$CONTEXT.cache_expire.

The general cache defaults to a max size of 3000 with a rate limit of 150/5m,
which allows 150 compilations per 5 minute period.
To override the defaults, configure the script.cache.max_size, script.max_compilations_rate, and script.cache.expire settings.
====

rjernst · 2021-10-19T22:58:19Z

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

@@ -277,7 +295,7 @@ void validateCacheSettings(Settings settings) {
                    String.join(", ", customRates) + "] if compile rates disabled via [" +
                    SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING.getKey() + "]");
            }
-            if (useContext == false) {
+            if (useContext == false && SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.exists(settings)) {


Shouldn't this branch not be an error anymore? The setting is the general one, not per context?

SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING is used for tests and not compatible with any custom settings. If that is set at all we need to error. I added this due to a test error.

Hrm, but the conditional here is checking SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING not SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING?

Oooh, this is inside a block for SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING, I think I see now.

debadair

Left some suggestions to streamline the deprecation entry & align the messages with the guidelines. LMK if you have any questions!

debadair · 2021-10-19T23:34:16Z

docs/reference/migration/migrate_7_16.asciidoc

+size.
+
+*Impact* +
+To avoid deprecation warnings, discontinue use of the deprecated settings.


I'd be inclined to consolidate this into a single entry. I think it can be streamlined quite a bit.

[[script-context-cache-deprecated]]
. The script context cache is deprecated
[%collapsible]
====
Details +
Setting script.max_compilations_rate to use-contextand
configuring context-specific caches is deprecated.
Context-specific caches are no longer needed to prevent system scripts
from triggering rate limits.
Configure the general cache to control the max complication rate, cache size, and
cache expiration for your scripts.

Impact +
Remove script.max_compilations_rate: use-context and the context-specific cache settings: script.context.$CONTEXT.cache_max_size, script.context.$CONTEXT.max_compilations_rate, script.context.$CONTEXT.cache_expire.

The general cache defaults to a max size of 3000 with a rate limit of 150/5m,
which allows 150 compilations per 5 minute period.
To override the defaults, configure the script.cache.max_size, script.max_compilations_rate, and script.cache.expire settings.
====

debadair · 2021-10-19T23:40:16Z

...gin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java

+                String.format(Locale.ROOT, "settings [" + maxSettings + "] for context specific rate limits are deprecated and will"
+                    + " not be available in a future version."
+                    + " Use [" + ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey() + "] to rate limit the compilation"
+                    + " of user scripts, system scripts are exempt.", maxSettings),


I'd edit slightly to align with the other deprecation messages:

Setting context-specific rate limits is deprecated. Use [setting] to rate limit the compilation of user scripts. Context-specific caches are no longer needed to prevent system scripts from triggering rate limits.

Much nicer. Done.

debadair · 2021-10-19T23:43:26Z

...gin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java

+                        + " deprecated and will not be available in a future version."
+                        + " Use [" + ScriptService.SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey() + "] to size the context cache for all"
+                        + " scripts.",
+                    cacheSizeSettings),


Similarly:

Setting a context-specific cache size is deprecated. Use [setting] to configure the size of the general cache for user scripts. Context-specific caches are no longer needed to prevent system scripts from triggering rate limits.

debadair · 2021-10-19T23:45:45Z

...gin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java

+                        + " Use [" + ScriptService.SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.getKey() + "]"
+                        + " to set the cache expiration all scripts.",
+                    cacheExpireSettings),
+                "https://ela.st/es-deprecation-7-script-context-expire-deprecated",


Similarly:

Setting a context-specific cache expiration is deprecated. Use [setting] to configure the expiration of the general cache. Context-specific caches are no longer needed to prevent system scripts from triggering rate limits.

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

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

stu-elastic requested a review from rjernst October 19, 2021 18:59

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

elasticsearchmachine added the v7.16.1 label Oct 19, 2021

script.max_size_in_bytes -> script.cache.max_size

2424419

rjernst reviewed Oct 19, 2021

View reviewed changes

stu-elastic added 2 commits October 19, 2021 16:41

CRITICAL -> WARNING, removed -> deprecated

85f3dd7

discontinue use of the [removed -> deprecated] setting

5b55d96

stu-elastic removed the v7.16.1 label Oct 19, 2021

elasticmachine and others added 2 commits October 19, 2021 18:02

Merge branch '7.x' into 7.x_pain_deprecate-context-cache-pr

1a1e757

Use new DeprecationWarning version of assertWarnings

a962ed5

elasticmachine and others added 2 commits October 19, 2021 18:26

Merge branch '7.x' into 7.x_pain_deprecate-context-cache-pr

1a0de9f

use-context in InternalTestCluster when context settings are gene…

deb9c44

…rated

rjernst approved these changes Oct 19, 2021

View reviewed changes

stu-elastic added 2 commits October 19, 2021 18:13

More specific breaking changes docs

3561a94

do not use deprecated settings in IT tests

23eb500

debadair reviewed Oct 19, 2021

View reviewed changes

stu-elastic added 2 commits October 19, 2021 18:58

ScriptModule unused

8a75a8a

Consolidate and clarify deprecation messages

ade5599

stu-elastic merged commit 005d8f0 into elastic:7.x Oct 20, 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

stu-elastic removed the >breaking label Oct 25, 2021

danhermann added the >deprecation label Dec 3, 2021

tvernum mentioned this pull request Dec 8, 2021

Script settings validation breaks mixed version cluster (7.15 + 7.16) #81486

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Script: Deprecate script context cache #79508

Script: Deprecate script context cache #79508

stu-elastic commented Oct 19, 2021 •

edited

Loading

elasticmachine commented Oct 19, 2021

rjernst left a comment

rjernst Oct 19, 2021

stu-elastic Oct 19, 2021

rjernst Oct 19, 2021

stu-elastic Oct 19, 2021

rjernst Oct 19, 2021

stu-elastic Oct 19, 2021

rjernst Oct 19, 2021

stu-elastic Oct 19, 2021

stu-elastic commented Oct 19, 2021

stu-elastic commented Oct 19, 2021

rjernst left a comment

rjernst Oct 19, 2021

stu-elastic Oct 19, 2021

rjernst Oct 19, 2021

stu-elastic Oct 19, 2021

rjernst Oct 19, 2021

stu-elastic Oct 19, 2021

debadair Oct 19, 2021 •

edited

Loading

rjernst Oct 19, 2021

stu-elastic Oct 19, 2021

rjernst Oct 19, 2021

rjernst Oct 19, 2021

debadair left a comment

debadair Oct 19, 2021 •

edited

Loading

debadair Oct 19, 2021

stu-elastic Oct 20, 2021

debadair Oct 19, 2021

stu-elastic Oct 20, 2021

debadair Oct 19, 2021

stu-elastic Oct 20, 2021

Script: Deprecate script context cache #79508

Script: Deprecate script context cache #79508

Conversation

stu-elastic commented Oct 19, 2021 • edited Loading

elasticmachine commented Oct 19, 2021

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stu-elastic commented Oct 19, 2021

stu-elastic commented Oct 19, 2021

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debadair Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debadair left a comment

Choose a reason for hiding this comment

debadair Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stu-elastic commented Oct 19, 2021 •

edited

Loading

debadair Oct 19, 2021 •

edited

Loading

debadair Oct 19, 2021 •

edited

Loading