Circuit break the number of inline scripts compiled per minute #19694

dakrone · 2016-07-29T20:34:30Z

When compiling many dynamically changing scripts, parameterized
scripts (https://www.elastic.co/guide/en/elasticsearch/reference/master/modules-scripting-using.html#prefer-params)
should be preferred. This enforces a limit to the number of scripts that
can be compiled within a minute. A new dynamic setting is added -
script.max_compilations_per_minute, which defaults to 15.

If more dynamic scripts are sent, a user will get the following
exception:

{
  "error" : {
    "root_cause" : [
      {
        "type" : "circuit_breaking_exception",
        "reason" : "[script] Too many dynamic script compilations within one minute, max: [30/min]; please use on-disk, indexed, or scripts with parameters instead",
        "bytes_wanted" : 0,
        "bytes_limit" : 0
      }
    ],
    "type" : "search_phase_execution_exception",
    "reason" : "all shards failed",
    "phase" : "query",
    "grouped" : true,
    "failed_shards" : [
      {
        "shard" : 0,
        "index" : "i",
        "node" : "a5V1eXcZRYiIk8lecjZ4Jw",
        "reason" : {
          "type" : "general_script_exception",
          "reason" : "Failed to compile inline script [\"aaaaaaaaaaaaaaaa\"] using lang [painless]",
          "caused_by" : {
            "type" : "circuit_breaking_exception",
            "reason" : "[script] Too many dynamic script compilations within one minute, max: [30/min]; please use on-disk, indexed, or scripts with parameters instead",
            "bytes_wanted" : 0,
            "bytes_limit" : 0
          }
        }
      }
    ],
    "caused_by" : {
      "type" : "general_script_exception",
      "reason" : "Failed to compile inline script [\"aaaaaaaaaaaaaaaa\"] using lang [painless]",
      "caused_by" : {
        "type" : "circuit_breaking_exception",
        "reason" : "[script] Too many dynamic script compilations within one minute, max: [30/min]; please use on-disk, indexed, or scripts with parameters instead",
        "bytes_wanted" : 0,
        "bytes_limit" : 0
      }
    }
  },
  "status" : 500
}

This also fixes a bug in ScriptService where requests being executed
concurrently on a single node could cause a script to be compiled
multiple times (many in the case of a powerful node with many shards)
due to no synchronization between checking the cache and compiling the
script. There is now synchronization so that a script being compiled
will only be compiled once regardless of the number of concurrent
searches on a node.

Relates to #19396

dakrone · 2016-07-29T20:36:00Z

As something to think about for this:

Is 30 the right number? It works out to one every 2 seconds, but maybe we want to be more or less strict?
Should this be as strict as throwing an exception? If we want to be more lenient this can be logging a warning instead (easy to change it to do that)

nik9000 · 2016-07-29T20:38:27Z

I think I'd prefer a warning. I'm fairly sure I'd have noticed it when running Elasticsearch.

nik9000 · 2016-07-29T20:46:03Z

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

+     * Check whether there have been too many compilations within the last minute, throwing a circuit breaking exception if so.
+     */
+    public void checkCompilationLimit() {
+        long now = System.nanoTime();


It'd be useful to describe the how here I think. I guess this works like a bucket of water slowly filling up. You get to compile a script if there is enough water in the bucket, otherwise you don't. If the bucket overflows then you don't get more water.

Yeah, it's a variant of the token bucket algorithm

Yeah, that link'd be a lovely comment.

clintongormley · 2016-08-02T15:40:25Z

I think people won't notice it unless it is an exception, just like all the other safeguards. The immediate workaround for the user is to update a dynamic setting, giving them time to update their application.

I think the default limit should be lower so that a user is more likely to trip this safeguard in dev instead of production.

dakrone · 2016-08-02T15:41:47Z

I think the default limit should be lower so that a user is more likely to trip this safeguard in dev instead of production.

Okay, I originally went with 10 a minute (before going for 30/min for this PR), does that sound reasonable or too low/high?

clintongormley · 2016-08-02T16:10:56Z

So one compilation every 6 seconds. That seems OK, even during script development. Maybe a tiny bit tight. 15 would definitely be ok though

dakrone · 2016-08-03T16:36:13Z

Okay, I pushed a new commit that sets the limit to 15/min

nik9000 · 2016-08-09T13:29:51Z

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

+     *
+     * It can be thought of as a bucket with water, every time the bucket is checked, water is added proportional to the amount of time that
+     * elapsed since the last time it was checked. If there is enough water, some is removed and the request is allowed. If there is not
+     * enough water the request is denied.


Maybe something like: The bucket can "overflow", in which case the excess capacity is just "spilled", rather than saved up. So it never holds more than a minute's capacity.

Definitely, I will add that

nik9000 · 2016-08-09T13:46:47Z

I looked at this again with new eyes this morning and left a few comments, mostly about funky side cases like "what if I set this to Long.MAX_VALUE?", "What if I set this to 0?" and "What if I set this to -123?".

dakrone · 2016-08-09T16:14:01Z

@nik9000 okay I pushed a bunch of commits that I tried to break into meaningful pieces with nice commit messages, addressing your concerns :)

nik9000 · 2016-08-09T16:18:44Z

core/src/test/java/org/elasticsearch/script/ScriptServiceTests.java

+        scriptService.setMaxCompilationsPerMinute(0);
+        expectThrows(CircuitBreakingException.class, () -> scriptService.checkCompilationLimit());
+        scriptService.setMaxCompilationsPerMinute(Integer.MAX_VALUE);
+        for (int i = 0; i < randomIntBetween(1000, 10000); i++) {


You might want to save the value of the limit rather than do it every iteration?

nik9000 · 2016-08-09T16:20:36Z

Left a small question/suggestion. Otherwise LGTM. Feel free to address my last point however you like and merge when you have a chance.

When compiling many dynamically changing scripts, parameterized scripts (<https://www.elastic.co/guide/en/elasticsearch/reference/master/modules-scripting-using.html#prefer-params>) should be preferred. This enforces a limit to the number of scripts that can be compiled within a minute. A new dynamic setting is added - `script.max_compilations_per_minute`, which defaults to 15. If more dynamic scripts are sent, a user will get the following exception: ```json { "error" : { "root_cause" : [ { "type" : "circuit_breaking_exception", "reason" : "[script] Too many dynamic script compilations within one minute, max: [15/min]; please use on-disk, indexed, or scripts with parameters instead", "bytes_wanted" : 0, "bytes_limit" : 0 } ], "type" : "search_phase_execution_exception", "reason" : "all shards failed", "phase" : "query", "grouped" : true, "failed_shards" : [ { "shard" : 0, "index" : "i", "node" : "a5V1eXcZRYiIk8lecjZ4Jw", "reason" : { "type" : "general_script_exception", "reason" : "Failed to compile inline script [\"aaaaaaaaaaaaaaaa\"] using lang [painless]", "caused_by" : { "type" : "circuit_breaking_exception", "reason" : "[script] Too many dynamic script compilations within one minute, max: [15/min]; please use on-disk, indexed, or scripts with parameters instead", "bytes_wanted" : 0, "bytes_limit" : 0 } } } ], "caused_by" : { "type" : "general_script_exception", "reason" : "Failed to compile inline script [\"aaaaaaaaaaaaaaaa\"] using lang [painless]", "caused_by" : { "type" : "circuit_breaking_exception", "reason" : "[script] Too many dynamic script compilations within one minute, max: [15/min]; please use on-disk, indexed, or scripts with parameters instead", "bytes_wanted" : 0, "bytes_limit" : 0 } } }, "status" : 500 } ``` This also fixes a bug in `ScriptService` where requests being executed concurrently on a single node could cause a script to be compiled multiple times (many in the case of a powerful node with many shards) due to no synchronization between checking the cache and compiling the script. There is now synchronization so that a script being compiled will only be compiled once regardless of the number of concurrent searches on a node. Relates to elastic#19396

dakrone added >enhancement :Core/Infra/Circuit Breakers Track estimates of memory consumption to prevent overload v5.0.0-beta1 labels Jul 29, 2016

nik9000 reviewed Jul 29, 2016
View reviewed changes

nik9000 reviewed Aug 9, 2016
View reviewed changes

dakrone force-pushed the compliation-breaker branch from 700a4bb to 2be52ef Compare August 9, 2016 16:27

dakrone merged commit 2be52ef into elastic:master Aug 9, 2016

nik9000 mentioned this pull request Aug 15, 2016

Groovy scripting - possible memory leak ES 2.3.3 #19396

Closed

clintongormley mentioned this pull request Aug 15, 2016

Add safeguards to prevent simple user errors #11511

Closed

24 tasks

dakrone deleted the compliation-breaker branch August 16, 2016 22:01

tlrx mentioned this pull request Sep 2, 2016

ES 2.3.5 Node gets unstable (memory issue) if role is master #20114

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Circuit break the number of inline scripts compiled per minute #19694

Circuit break the number of inline scripts compiled per minute #19694

Uh oh!

dakrone commented Jul 29, 2016 •

edited

Loading

Uh oh!

dakrone commented Jul 29, 2016

Uh oh!

nik9000 commented Jul 29, 2016

Uh oh!

nik9000 Jul 29, 2016

Uh oh!

dakrone Jul 29, 2016

Uh oh!

nik9000 Jul 29, 2016

Uh oh!

clintongormley commented Aug 2, 2016

Uh oh!

dakrone commented Aug 2, 2016 •

edited

Loading

Uh oh!

clintongormley commented Aug 2, 2016

Uh oh!

dakrone commented Aug 3, 2016

Uh oh!

nik9000 Aug 9, 2016

Uh oh!

dakrone Aug 9, 2016

Uh oh!

nik9000 commented Aug 9, 2016

Uh oh!

dakrone commented Aug 9, 2016

Uh oh!

nik9000 Aug 9, 2016

Uh oh!

nik9000 commented Aug 9, 2016

Uh oh!

Uh oh!

Circuit break the number of inline scripts compiled per minute #19694

Circuit break the number of inline scripts compiled per minute #19694

Uh oh!

Conversation

dakrone commented Jul 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dakrone commented Jul 29, 2016

Uh oh!

nik9000 commented Jul 29, 2016

Uh oh!

nik9000 Jul 29, 2016

Choose a reason for hiding this comment

Uh oh!

dakrone Jul 29, 2016

Choose a reason for hiding this comment

Uh oh!

nik9000 Jul 29, 2016

Choose a reason for hiding this comment

Uh oh!

clintongormley commented Aug 2, 2016

Uh oh!

dakrone commented Aug 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clintongormley commented Aug 2, 2016

Uh oh!

dakrone commented Aug 3, 2016

Uh oh!

nik9000 Aug 9, 2016

Choose a reason for hiding this comment

Uh oh!

dakrone Aug 9, 2016

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Aug 9, 2016

Uh oh!

dakrone commented Aug 9, 2016

Uh oh!

nik9000 Aug 9, 2016

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Aug 9, 2016

Uh oh!

Uh oh!

dakrone commented Jul 29, 2016 •

edited

Loading

dakrone commented Aug 2, 2016 •

edited

Loading