-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement a strategy to handle OOM in direct memory #475
base: main
Are you sure you want to change the base?
Conversation
if (isDirectMemoryOOM(cause)) { | ||
DirectMemoryUsage direct = DirectMemoryUsage.capture(); | ||
logger.info("Direct memory status, used: {}, pinned: {}, ratio: {}", direct.used, direct.pinned, direct.ratio); | ||
logger.warn("Dropping connection {} because run out of direct memory. To fix it, in Filebeat you can" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should also consider that the source is not filebeat. either it's another beat like winlogbeat, or Logstash itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 8f3b08c
ctx.close(); | ||
logger.info("Dropping connection {} due to high resource consumption", ctx.channel()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be the order? also it's likely worth bumping to warn as it's a near OOM situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's almost idempotent, but in e general we could have that pinned is close to used, but far from max direct. In such a case I think don't want to enter too much frequently the check on max memory.
For example considering:
- max direct is 128MB
- used is 64MB
- pinned is constantly at 56 MB
The system is balanced for the work it's doing to consume ~50MB and it's far from OOM condition, so it's not in harmful situation, I think.
@@ -134,7 +134,11 @@ public void initChannel(SocketChannel socket){ | |||
new IdleStateHandler(localClientInactivityTimeoutSeconds, IDLESTATE_WRITER_IDLE_TIME_SECONDS, localClientInactivityTimeoutSeconds)); | |||
pipeline.addLast(BEATS_ACKER, new AckEncoder()); | |||
pipeline.addLast(CONNECTION_HANDLER, new ConnectionHandler()); | |||
pipeline.addLast(beatsHandlerExecutorGroup, new BeatsParser(), new BeatsHandler(localMessageListener)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like us to be a bit more intentional in why we're removing the executor group here. in my test PR I removed it to simplify the number of threads I had to reason about, and having a single pool for both boss/worker loops would mean blocking the workers would stop boss from accepting new connections too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be the complete pipeline executed in the worker group, instead just BeatsParser and BeatsHandler, while BeatsHacker and Connectionhandler are still executed by the boss group?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean shouldn't use the ServerBootstrap.group(bossGroup, workerGroup)
instead of assigning the group for just those 2 Beats handler? If we do this, we have at least one thread context switch on every pipeline. Maybe it's something I'm not grasping.
something I'm observing is that maybe linger is not being set in the client connections, as I continue to see the same connection ids reporting oom errors well after the script that does load generation is gone: https://gist.github.com/jsvd/a047320ff68bbe064e93dec0d6a251f7 |
@jsvd I used the PR #477 to test in isolation the
I think that when we reach an OOM error and we have the client that terminates immediately, we expect that all the 1500 channels also terminates immediately on Netty side, but in reality it takes minutes and this gives the idea of a looping error on the connections. Why it takes 5 minutes to stop logging channel's exceptions is not clear to me, maybe it's due to memory shortage. Do you think that on server side, on first OOM error notification, should we close immediately all the other connections? |
2637198
to
bd9d0b1
Compare
…utors group (beatsHandlerExecutorGroup)
…omes not writable, to excert backpressure to the sender system
On new channel registration (that correspond to a new client connection), verifies the direct memory stastus to understand if almost the totality max direct memory is reached and also if the majoproity of that space is used by pinned byte buffers. If the codition is verified that means direct memory avvailable is terminating, so no new connection would help in the situation, and the incoming new connections are closed.
…ble status due to that offload every message to the outbound list
…undering and going to OOM condition
Co-authored-by: João Duarte <[email protected]>
… during the read opeartion and not only on exception
…otocol processing from the boss group that accepts and listed to new sockets
…f OOM checking or not. Enabled by default.
bd9d0b1
to
d75a0df
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some 1:1 discussion we think that splitting this PR into two would facilitate moving forward here.
- One PR would disable autoread
- Another would introduce the protect_direct_memory setting
Maybe we can start with PR1 and leave 2 for later. My suggestion would be to start with a scenario we know breaks in the current main beats input, but is handled better with the PR.
For example, the follow script causes OOM consistently on 256M direct memory: https://gist.github.com/jsvd/9cba50e01bb840d41dba07b5862be358
I tested PR475, manually merged it to 6.8.0 and started with logstash-7.17.17, then when calling V2Batch.realease I got a reference counting issue.
|
Release notes
Direct memory used by plugin weren't tracked and this could generate out-of-memory crashes when unattended spikes or traffic loads reached the plugin.
The plugin is changed to monitor how much direct memory it's using and in case it's close to the limit dropping connection to free some space.
To control spikes of memory usage, the incoming message reads switched from push mode to pull, so that the plugin has control on rate of ingestion and it's not the determined by the clients.
What does this PR do?
Changes the way the plugin pulls data and handle incoming connections to respect the max direct memory size and avoid out of memory errors.
This work is based on the existing #410.
Why is it important/What is the impact to the user?
Provides a set of countermeasures to limit the probability of OutOfMemory errors when creating new buffers in direct memory.
In addition to this, it introduces a minimal amount fo direct memory (256MB) that's required to start processing data, if not respected, the pipeline used fails to start.
Checklist
Author's Checklist
How to test this PR locally
The test plan has some steps:
create TLS certificates
make
build the plugin and configure Logstash
./gradlew clean vendor
Gemfile
adding:bin/logstash-plugin install --no-verify
set up memory size limits, configure pipeline
config/jvm.options
add:run Logstash and benchmark it
From a terminal launch Logstash process
bin/logstash -f path_to_config
and the benchmark script from another:Expected outcome
The expectation is that direct memory consumption never goes up to the limit and if the client doesn't consume the ACKs messages (
-a no
) is the client that goes in error and not the Logstash side.Test with real Filebeat
To start a bunch of Filebeat clients sending data to Beats input, just use the script
ingest_all_hands/filebeat/multiple_filebeats.rb
present in PRhttps://github.com/elastic/logstash/pull/15151It download a filebeat distribution, unpacks, generate some input file, prepare the
data
andlogs
folder for each Filebeat instance and run the processes.Related issues
Use cases
Screenshots
Logs