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
Stream compression - Deactivate compression at runtime in case of a compressor buffer overflow #12037
Stream compression - Deactivate compression at runtime in case of a compressor buffer overflow #12037
Conversation
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.
Tested it with child <-> parent <-> gparent
.
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.
Documentation: LGTM
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 tested proposed scenario and I did not observe problems with SPAWN server!
@odynik we have conflicts with |
Thanks @thiagoftsm. For now, we decided to keep this PR on hold. |
4705561
0851eec
to
4705561
Compare
PR rebased on latest master |
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.
Docs: LGTM
…o stream_compression_buff_overflow_fix
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 tested all the possible combinations for the proposed scenario, and I did not observe any crash in spawn server. I also observed a downgrade when buffer overflow happens. LGTM!
error("STREAM_COMPRESSION: Deactivating compression to avoid stream corruption"); | ||
default_compression_enabled = 0; | ||
s->rrdpush_compression = 0; | ||
s->version = STREAM_VERSION_CLABELS; |
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.
So currently CLABELS is 4 and compression is 5. What will happen with version 6 ? It will support compression but when it fails it will fallback to 4 (losing the functionally added by version 6).
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 would prefer to merge this PR in the master and respect the stream versions (4 - clabels, 5 - compression, 6 - gap filling) in the gap filling PR.
The stream version checks need to be gathered in a function because if we keep adding features in streaming the if/elif statements are going to increase the code complexity.
Summary
This PR catches a stream compression buffer overflow and escapes from a stream corruption by deactivating stream compression at runtime. If the sender thread experiences a compressor buffer overflow will deactivate stream compression and re-establish a fresh link with stream version protocol 4.
Test Plan
To test this PR you are going to need,
Steps
1 Enable stream compression in the
stream.conf
file for all the agents.OR/AND
2.1
cd /etc/netdata
2.2
sudo ./edit-config go.d.conf
sudo ./edit-config go.d/example.conf
error.log
for the message sequence,8.1 See streaming data in parent and grandparent agent dashboards.
8.2 In parent agent, you should see a healthy sender thread shutdown sequence,
8.3 Please report any sequence of messages containing
Additional Information
This PR is a temporary work around for this issue #12020. I am not linking the issue here since it is not the permanent solution.
Stream Compression: YES
stream-compression: enabled/disabled/N/A
at web api/api/v1/info