-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix line skipping issue in receive_lines method #4491
base: master
Are you sure you want to change the base?
Conversation
@yugeeklab Thanks for this fix! I see the intent of this fix as follows.
Surely, such a fix would allow us to limit memory consumption by the This PR would be effective to some extent, however I believe the problem of memory consumption will remain. Are these my understandings correct? |
Hi, @daipom I've just published an issue #4491 for more information.
When max_line_size isn't set, FIFO's @buffer can grow indefinitely. Or if max_line_size has large value, FIFO's buffer will be limited, but there's still a possibility of fluentd experiencing slowdowns. Summary: as-is: max_line_size helps you avoid buffer overflow configuring via buffer section. If you have any suggestions, such as the fifo_buffer_size parameter or any other ideas, please feel free to discuss them with me. Thank you for your review! |
Thanks so much!
Now I understand!
This fix clears
Thanks! |
Thank you for your comment!! @daipom Please let me know if there's any feedback on my code or idea. I'll review and accept your feedback as soon as possible. Thank you. |
@yugeeklab The CI issue has been resolved. Sorry for the trouble. |
Before this patch, long lines could cause breakdowns in fluentd, potentially posing a vulnerability. With this patch, max_line_size will be integrated into the FIFO, enabling the system to skip lines exceeding the maximum size before executing receive_lines. Co-authored-by: yugeeklab <yugeeklab@gmail.com> Co-authored-by: moggaa <donionbs7@gmail.com> Signed-off-by: been.zino <been.zino@kakaocorp.com>
Hi, @daipom Rebase is done!! Thank you for your review!! |
Sorry for waiting. |
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.
@yugeeklab Thanks for this fix!
This fix basically looks good to me!
I've commented on some minor details (about the following), please check!
- Keeping the same debug log as before
- Improving codes
- Improving tests
lib/fluent/plugin/in_tail.rb
Outdated
if !is_long_line | ||
lines << convert(rbuf) | ||
else | ||
@log.warn "received line length is longer than #{@max_line_size}" | ||
@log.debug "skipped line" | ||
@was_long_line = false | ||
end |
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.
if !is_long_line | |
lines << convert(rbuf) | |
else | |
@log.warn "received line length is longer than #{@max_line_size}" | |
@log.debug "skipped line" | |
@was_long_line = false | |
end | |
if is_long_line | |
@log.warn "received line length is longer than #{@max_line_size}" | |
@log.debug("skipped line: ") { convert(rbuf).chomp } | |
@was_long_line = false | |
next | |
end | |
lines << convert(rbuf) |
@@ -146,5 +146,36 @@ def create_watcher | |||
assert_equal 5, returned_lines[0].size | |||
assert_equal 3, returned_lines[1].size | |||
end | |||
|
|||
test 'call receive_lines some times when long line(more than 8192) and max_line_size is 4096' do |
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 like this test to be more clear about what is being tested.
This test does not seem to be intended to test if receive_lines
is called more than once (some times).
(Actually, it is called only once in this test.)
Perhaps it would be better to make this test independent of the when limit is 5
sub test.
lib/fluent/plugin/in_tail.rb
Outdated
) | ||
|
||
if is_long_line | ||
@buffer = ''.force_encoding(@from_encoding) |
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.
@buffer = ''.force_encoding(@from_encoding) | |
@buffer.clear |
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.
Hi @daipom
I discovered that the change in this suggestion(#4491 (comment)) causes the following test to fail:
data(
small: ["128", 8192],
)
test 'max_line_size_8192' do |(label, size)|
config = config_element("", "", {
"tag" => "max_line_size",
"path" => "#{@tmp_dir}/with_long_lines.txt",
"format" => "none",
"read_from_head" => true,
"max_line_size" => label,
"log_level" => "debug"
})
Fluent::FileWrapper.open("#{@tmp_dir}/with_long_lines.txt", "w+") do |f|
f.puts "foo"
f.puts "x" * size # 'x' * size + \n > @max_line_size
f.puts "bar"
end
d = create_driver(config, false)
timestamp = Time.parse("Mon Nov 29 11:22:33 UTC 2021")
Timecop.freeze(timestamp)
d.run(expect_records: 2)
assert_equal([
[{"message" => "foo"},{"message" => "bar"}],
[
"2021-11-29 11:22:33 +0000 [warn]: received line length is longer than #{label}\n",
"2021-11-29 11:22:33 +0000 [debug]: skipped line: #{'x' * size}\n"
]
],
[
d.events.collect { |event| event.last },
d.logs[-3..]
])
end
When the line_size is greater than max_line_size and line_size is greater than or equal to 8192, the test fails.
To log the long line even if it is skipped, you can log the line above this code like this:
if is_long_line
@log.warn "received line length is longer than #{@max_line_size}"
@log.debug("skipped line: ") { convert(@buffer).chomp }
@buffer.clear
@was_long_line = true
end
If you have any idea, please let me know.
Thank you for your review!!
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 see! Thanks for checking this behavior!
@log.debug("skipped line: ") { convert(rbuf).chomp }
sometimes can not output the entire line that is expected.
It is because some parts of the line are already cleared if @was_long_line
is true
,
I think this is simply a debug log issue, right?
It is less important, so it would be good if we could fix it within the possible range.
Co-authored-by: Daijiro Fukuda <fukuda@clear-code.com>
Co-authored-by: Daijiro Fukuda <fukuda@clear-code.com>
Co-authored-by: Daijiro Fukuda <fukuda@clear-code.com>
Co-authored-by: Daijiro Fukuda <fukuda@clear-code.com>
Co-authored-by: Daijiro Fukuda <fukuda@clear-code.com>
Co-authored-by: Daijiro Fukuda <fukuda@clear-code.com>
@yugeeklab |
Current CI failures have nothing to do with this PR. |
Which issue(s) this PR fixes:
Fixes #4494
What this PR does / why we need it:
Before this patch, long lines could cause breakdowns in fluentd, potentially posing a vulnerability. With this patch, max_line_size will be integrated into the FIFO, enabling the system to skip lines exceeding the maximum size before executing receive_lines.
Docs Changes:
Release Note: