Improve EQL Sequence circuit breaker precision by luigidellaquila · Pull Request #88538 · elastic/elasticsearch

luigidellaquila · 2022-07-14T11:09:17Z

~~The problem seems to be very sporadic though, I couldn't find a reproducer.~~

There are two good candidates as a cause for this problem, this PR addresses both:

The new memory calculation is added to the total count even if the CB fails, so a cleanup after a failure could lead to a negative value of the CB
The delta memory is calculated only between the beginning and the end of match() execution, but memory consumption change also happens outside that method and the initial value could be greater than the final one, so the delta added to the CB could be negative. In general, this second approach makes the memory calculation more accurate.

[edit] It turned out the first hypothesis was correct: the problem was on cleanup.

luigidellaquila · 2022-07-14T11:14:28Z

...plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/sequence/SequenceMatcher.java

     * Returns false if the process needs to be stopped.
     */
    boolean match(int stage, Iterable<Tuple<KeyAndOrdinal, HitReference>> hits) {
-        long ramBytesUsedInFlight = ramBytesUsedInFlight();


These values are > 0, if they reduce for some reason during the execution, at the end the memory diff will be negative.
Setting them to 0 as a global value, and avoiding to re-calculate them at the beginning of match(), guarantees that the memory added to the circuit breaker is always positive (unless Accountable objects return negative values, but checking the implementations it's not the case)

luigidellaquila · 2022-07-14T11:15:25Z

...plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/sequence/SequenceMatcher.java

    }

    private void addMemory(long bytes, String label) {
-        totalRamBytesUsed += bytes;


In case of breaker failure, the memory is not added to the circuit breaker, so the addition to totalRamBytesUsed should be done only after addEstimateBytesAndMaybeBreak succeeds.

This is a good point, should this be relatively easy reproducible?

elasticmachine · 2022-07-14T12:24:52Z

Pinging @elastic/es-ql (Team:QL)

elasticsearchmachine · 2022-07-14T12:25:14Z

Hi @luigidellaquila, I've created a changelog YAML for you.

bpintea

Left some comments.

bpintea · 2022-07-27T10:36:24Z

...plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/sequence/SequenceMatcher.java

+        long bytesDiff = ramBytesUsedInFlight() - prevRamBytesUsedInFlight;
        addMemory(bytesDiff, CB_INFLIGHT_LABEL);
+        prevRamBytesUsedInFlight += bytesDiff;


This'll leave prevRamBytesUsedInFlight == ramBytesUsedInFlight(), right? It might be clearer smth like:

Suggested change

long bytesDiff = ramBytesUsedInFlight() - prevRamBytesUsedInFlight;

addMemory(bytesDiff, CB_INFLIGHT_LABEL);

prevRamBytesUsedInFlight += bytesDiff;

long newRamBytesUsedInFlight = ramBytesUsedInFlight();

addMemory(newRamBytesUsedInFlight - prevRamBytesUsedInFlight, CB_INFLIGHT_LABEL);

prevRamBytesUsedInFlight = newRamBytesUsedInFlight;

Setting them to 0 as a global value, and avoiding to re-calculate them at the beginning of match(), guarantees that the memory added to the circuit breaker is always positive

Can the memory footprint not go down (trimmed?) in-between match() invocations? As that would still leave room for negative memory adjustments.

That's exactly the reason for this change: if we calculate the memory footprint change as the difference between the beginning and the end of match() execution, we risk to miss everything that happens outside of that.
Eg.

a match() is invoked, it calculates a delta of 10k: the real memory footprint is 10KB, the CB is updated with 10KB

a trim() is invoked: the new real memory footprint is 0KB (the trim released all the memory), the CB is still 10KB (!!!) as trim() does not update the CB

a match is invoked, it calculates another delta of 10k: the real memory footprint is 10KB again (the memory was cleared by the trim()), but the CB is updated to 20KB (!!!) because the new delta (10KB) is added to the CB value (10KB, not updated by trim)

Repeat this pattern for 100 times and at the end you'll have 10KB of real memory used, but the CB thinks you are using 1MB

The same could happen at the opposite, eg. something (outside of match() execution) allocates memory, but it does not update the CB (so the CB value is still zero!!!); after that, a match() execution REDUCES the memory footprint, so the delta is NEGATIVE. As soon as match() tries to update the CB with a negative value, you have the error we are chasing.

Not sure how likely it is to happen with current implementation, but I see it as a weakness of current algorithm.

With a global variable (that is what this PR does) you solve all of this: you just calculate the total memory every time and you compare it with the total memory calculated at previous round. As long as Accountable objects cannot return negative values, you are always guaranteed that the CB cannot go below zero

bpintea · 2022-07-27T10:37:24Z

...plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/sequence/SequenceMatcher.java

+        prevRamBytesUsedInFlight = 0;
+        prevRamBytesUsedCompleted = 0;
        totalRamBytesUsed = 0;


If keeping track of the in-flight and completed usage, totalRamBytesUsed can be removed (as it should always be the sum of the two).

bpintea · 2022-07-27T10:50:57Z

...plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/sequence/SequenceMatcher.java


    private boolean headLimit = false;
+
+    // ---------- CIRCUIT BREAKER -----------


Nit: imo, this could be a single liner // circuit breaker or // circuit breaker accounting

…_breaker_precision' into improve_eql_seq_circuit_breaker_precision

luigidellaquila · 2022-07-28T06:56:41Z

Finally reproduced (and added a test case), the problem happened on clearCircuitBreaker() after a circuit breaker failure happening in the parent circuit breaker service

astefan

LGTM. Good job on creating a test for this exact (hard to investigate) failure.
I've left two small comments.

astefan · 2022-07-28T11:41:24Z

...in/eql/src/test/java/org/elasticsearch/xpack/eql/execution/sequence/CircuitBreakerTests.java

        }
    }

+    public void testParentCircuitBreakerOnClean() {


The test name imo is not descriptive enough. I'd suggest something like testEqlCBCleanedUp_on_ParentCBBreak.

Also, please add a comment to the method:
// test covering fix for https://github.com/elastic/elasticsearch/issues/88300

astefan · 2022-07-28T11:41:58Z

...in/eql/src/test/java/org/elasticsearch/xpack/eql/execution/sequence/CircuitBreakerTests.java

+    public void testParentCircuitBreakerOnClean() {
+        int sequenceFiltersCount = 2;
+        final int SEARCH_REQUESTS_EXPECTED_COUNT = 2;
+        List<BreakerSettings> eqlBreakerSettings = Collections.singletonList(


The eqlBreakerSettings can be extracted in a separate method. This code block is used in two places in this class.

astefan · 2022-07-28T11:42:46Z

...plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/sequence/SequenceMatcher.java

@@ -305,34 +305,42 @@ public void clear() {
        clearCircuitBreaker();
    }



Add a comment to these two methods stating that they are protected for testing purposes.

bpintea

Nice catch.
I've left one comment, otherwise lgtm!

bpintea · 2022-07-28T12:14:33Z

...plugin/eql/src/main/java/org/elasticsearch/xpack/eql/execution/sequence/SequenceMatcher.java

+        if (CB_COMPLETED_LABEL.equals(label)) {
+            prevRamBytesUsedCompleted += bytes;
+        } else {
+            prevRamBytesUsedInFlight += bytes;
+        }


This isn't needed, as the values are assigned after the function call to the actual memory consumption.

Good point, it's completely redundant. We don't even need an addMemory() method actually

bpintea · 2022-07-28T12:24:18Z

...in/eql/src/test/java/org/elasticsearch/xpack/eql/execution/sequence/CircuitBreakerTests.java


+    public void testParentCircuitBreakerOnClean() {
+        int sequenceFiltersCount = 2;
+        final int SEARCH_REQUESTS_EXPECTED_COUNT = 2;


Nit: I guess this is c&p'd and that's why it's final/capitalized, but it looks a bit off that, comparatively, the var above is only effectively final and not actually so.

luigidellaquila · 2022-07-28T15:23:45Z

@elasticmachine update branch

Fixes elastic#88300

elasticsearchmachine · 2022-08-01T06:28:52Z

💚 Backport successful

Status	Branch	Result
✅	8.3
✅	8.4

Fixes #88300

Improve EQL sequence circuit breaker precision

996b7aa

elasticsearchmachine added the v8.4.0 label Jul 14, 2022

luigidellaquila commented Jul 14, 2022

View reviewed changes

luigidellaquila added >bug :Analytics/EQL EQL querying labels Jul 14, 2022

luigidellaquila marked this pull request as ready for review July 14, 2022 12:24

elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Jul 14, 2022

luigidellaquila requested review from Luegg, astefan, bpintea and costin July 14, 2022 12:24

Update docs/changelog/88538.yaml

25c8c99

elasticsearchmachine changed the base branch from master to main July 22, 2022 23:05

bpintea reviewed Jul 27, 2022

View reviewed changes

mark-vieira added v8.5.0 and removed v8.4.0 labels Jul 27, 2022

luigidellaquila added 2 commits July 27, 2022 17:58

Add tests and implement review suggestions

13a307d

Merge remote-tracking branch 'luigidellaquila/improve_eql_seq_circuit…

ed726a7

…_breaker_precision' into improve_eql_seq_circuit_breaker_precision

luigidellaquila mentioned this pull request Jul 28, 2022

Fix memory calculation on circuit breaker addMemory() #88882

Merged

astefan approved these changes Jul 28, 2022

View reviewed changes

bpintea approved these changes Jul 28, 2022

View reviewed changes

Implement review suggestions

1b275db

elasticmachine and others added 2 commits July 29, 2022 00:53

Merge branch 'main' into improve_eql_seq_circuit_breaker_precision

33c072f

Rename test method

a949772

astefan added the v8.3.4 label Jul 29, 2022

astefan added v8.4.1 auto-backport-and-merge labels Jul 29, 2022

mark-vieira added v8.3.3 v8.3.4 and removed v8.3.4 v8.3.3 labels Jul 29, 2022

luigidellaquila merged commit aa7d0a6 into elastic:main Aug 1, 2022

luigidellaquila mentioned this pull request Aug 1, 2022

[8.3] Improve EQL Sequence circuit breaker precision (#88538) #88976

Merged

luigidellaquila added a commit to luigidellaquila/elasticsearch that referenced this pull request Aug 1, 2022

Improve EQL Sequence circuit breaker precision (elastic#88538)

e0fbec3

Fixes elastic#88300

luigidellaquila added a commit to luigidellaquila/elasticsearch that referenced this pull request Aug 1, 2022

Improve EQL Sequence circuit breaker precision (elastic#88538)

6188b5f

Fixes elastic#88300

luigidellaquila mentioned this pull request Aug 1, 2022

[8.4] Improve EQL Sequence circuit breaker precision (#88538) #88977

Merged

elasticsearchmachine pushed a commit that referenced this pull request Aug 1, 2022

Improve EQL Sequence circuit breaker precision (#88538) (#88977)

f5313ae

Fixes #88300

elasticsearchmachine pushed a commit that referenced this pull request Aug 1, 2022

Improve EQL Sequence circuit breaker precision (#88538) (#88976)

890b8f4

Fixes #88300

mark-vieira added v8.4.0 and removed v8.4.1 labels Aug 15, 2022


		private boolean headLimit = false;

		// ---------- CIRCUIT BREAKER -----------

		@@ -305,34 +305,42 @@ public void clear() {
		clearCircuitBreaker();
		}

Conversation

luigidellaquila commented Jul 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

elasticmachine commented Jul 14, 2022

Uh oh!

elasticsearchmachine commented Jul 14, 2022

Uh oh!

bpintea 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!

luigidellaquila commented Jul 28, 2022

Uh oh!

astefan 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!

bpintea 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!

luigidellaquila commented Jul 28, 2022

Uh oh!

elasticsearchmachine commented Aug 1, 2022

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

luigidellaquila commented Jul 14, 2022 •

edited

Loading