Skip to content
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

Make stage_size & stage computation thread safe #2734

Merged
merged 2 commits into from
Jan 6, 2020

Conversation

hlakshmi
Copy link

@hlakshmi hlakshmi commented Dec 12, 2019

Which issue(s) this PR fixes:
#2712
Fixes #

What this PR does / why we need it:
When writing chunks to buffer, the stage_size computation can be thread-unsafe as mentioned in the bug which can result in BufferOverflowErrors even though the buffer is not actually full. This happened on a live long running fluentd. The method buffer.write is supposed to write to a chunk atmost once and hence stage_size should be added for a chunk only once but due to write_step_by_step the block which updates the value of staged_bytesize may be called multiple times and the chunk can be rollbacked but the value of staged_bytesize is not reverted causing the stage_size to be more than the actual value.

This PR addresses this issue.

Docs Changes:

Release Note:

Signed-off-by: Harish Nelakurthi <harish.nelakurthi@illumio.com>
@repeatedly
Copy link
Member

Thanks!. We will check issue and patch later.

Signed-off-by: Harish Nelakurthi <harish.nelakurthi@illumio.com>
@hlakshmi
Copy link
Author

@repeatedly Can we get this merged into the upcoming release if it looks good as this is causing a critical issue of data loss. We have tested this in large scale environment and the fix seems to work. Thanks!

Copy link
Member

@ganmacs ganmacs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay. I added a comment.

# but this block **may** run multiple times from write_step_by_step and previous write may be rollbacked
# So we should be counting the stage_size only for the last successful write
#
staged_bytesizes_by_chunk[chunk] = adding_bytesize
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the following situation can happen. what do you think of this?

  1. staged_bytesizes_by_chunk stores chunk1
  2. rollback happens in write_step_by_step (ShouldRetry raises)
  3. another thread euqueues chunk1 before this thread enters into
    chunk = get_next_chunk.call
  4. create new chunk(chunk2) in
    synchronize{ @stage[metadata] ||= generate_chunk(metadata).staged! }
  5. staged_bytesizes_by_chunk stores chunk2 (still remtains chunk1 in staged_bytesizes_by_chunk)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ganmacs Steps (3) can't happen because enqueue_chunk can't get a lock and waits until the lock is released by the thread which is writing to the chunk.

The lock is acquired here: https://github.com/fluent/fluentd/blob/master/lib/fluent/plugin/buffer.rb#L279
And released here:
https://github.com/fluent/fluentd/blob/master/lib/fluent/plugin/buffer.rb#L319

So enqueue_chunk can happen only after the commit is done.
Hope this helps.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense. You're right. other thread can get the chunk but can't enter

chunk.synchronize do
.

Copy link
Member

@ganmacs ganmacs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

# but this block **may** run multiple times from write_step_by_step and previous write may be rollbacked
# So we should be counting the stage_size only for the last successful write
#
staged_bytesizes_by_chunk[chunk] = adding_bytesize
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense. You're right. other thread can get the chunk but can't enter

chunk.synchronize do
.

@repeatedly repeatedly merged commit 24ddf6e into fluent:master Jan 6, 2020
@repeatedly
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants