check allocated buckets when allocating empty buckets to avoid OOM#80789
check allocated buckets when allocating empty buckets to avoid OOM#80789boicehuang wants to merge 5 commits intoelastic:mainfrom
Conversation
…ucket in histogram
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
|
Sorry it took us so long to get to reviewing this one!
That sounds like they are intentionally trying to defeat the protection from search.max_buckets. I'm not sure we should really support this. @imotov, what do you think? |
imotov
left a comment
There was a problem hiding this comment.
I think it could be a helpful change for some users, but it kind of depends on side effects from two functions working together and it has no tests. So, I am not sure I would be comfortable merging it in like this.
|
@boicehuang please enable the option "Allow edits and access to secrets by maintainers" on your PR. For more information, see the documentation. |
|
@boicehuang This seems useful, would you please add tests to verify the functionality added here? |
fine， i will add some tests. |
|
@wchaparro I found it hard to add tests because it depends on real-memory parent breaker. can you help give me some examples? |
|
Hey folks! I put together some tests but couldn't push them to this repo. I'm not super proud of the tests, but they do seem to get the job done. Could you give repo owners permission to push to this branch? |
I see you've merged master. That's not enough to let me push to the repo - there's a button on the PR. Here are some instructions: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork If you do that I should be able to push my test. And the changelog bot can push a changelog entry. |
In addEmptyBuckets of date histogram, it introduces a counter to accumulate the number of empty buckets to trigger the "search.max_bucket" breaker, but the parent breaker would probably not be triggered because no bucket is allocated in the first accumulation process. When it allocates memory for empty buckets on the second iterateEmptyBuckets, without any memory circuit breaker check, the memory rises and may cause OOM.
For example, people may set search.max_buckets to 20w, but 20w empty buckets may fill up remaining heap memory when there are other write and search tasks in ES node. In this case, we can still use check allocated_buckets to prevent date histogram from causing OOM in advance.
This PR adds checks on real memory parent circuit breaker when allocating empty buckets.