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

in_tail: Ensure to detach correct watcher on rotation with follow_inodes #4208

Merged
merged 1 commit into from
Jun 27, 2023

Conversation

daipom
Copy link
Contributor

@daipom daipom commented Jun 20, 2023

Which issue(s) this PR fixes:

What this PR does / why we need it:
If refresh_watchers run before update_watcher, the old implementation of update_watcher detach wrongly the new TailWatcher which is added in refresh_watcher. This causes the problem of stopping tailing log and handle leak.

The test case test_updateTW_after_refreshTW reproduces this problem.
This PR solves it.

NOTE: There is another BUG about unwatching. (I adjusted some expected values of the tests for this BUG.)
When refresh_watcher find the rotated old file AFTER unwatching it (rotate_wait), then the logs will be collected in duplicate.
If refresh_watcher find it BEFORE unwatching it (rotate_wait), this problem doesn't occur because the position entry is still alive.
We need to fix this and fix the adjusted expected values.

Docs Changes:
Not needed.

Release Note:
Same as the title.

OTHERS:
This fix is based on the content and discussion of the following issue and PR:

Thanks to those who have given me much insight, especially @kattz-kawa, @masaki-hatada, and @garyzjq!

@daipom
Copy link
Contributor Author

daipom commented Jun 20, 2023

The scenario of this problem (basically from #4185 (comment))
Please refer to the test case test_updateTW_after_refreshTW.

When follow_inodes is enabled.

  • Files: file.log (inode-A)
    • @tails[file.log] => Watcher(file.log, inode-A)
  • Rotation!
    • file.log is moved to file.log.1.
    • A new file is created as file.log.
  • Files: file.log (inode-B), file.log.1 (inode-A)
  • refresh_watcher and update_watcher can be processed at the same time...

If refresh_watcher is processed before update_watcher

  1. refresh_watcher
    • Find file.log (inode-B) as a new target.
    • OVERWRITE @tails[file.log] by Watcher(file.log, inode-B)
      • @tails[file.log] already has a watcher. We can't overwrite it here.
    • Result:
      • @tails[file.log] => Watcher(file.log, inode-B)
      • Watcher(file.log, inode-A) remains, but it does NOT EXIST in @tails.
        • This was causing handle leaks.
  2. update_watcher
    • Watcher(file.log, inode-A) will call it's update_watcher
      • This will update the inode of file.log to inode-B.
      • Old implementation
        • Detach @tails[file.log] (Watcher(file.log, inode-B)) WRONG
          • This stops tailing the new file!!
      • This PR
        • Detach self (Watcher(file.log, inode-A)). CORRECT
        • This solves the problem both of stopping tailing and handle leaks.

lib/fluent/plugin/in_tail.rb Outdated Show resolved Hide resolved
@daipom daipom force-pushed the in_tail-ensure-detach-correct-watcher branch from 8ec5545 to 684632b Compare June 23, 2023 10:19
@kattz-kawa
Copy link
Contributor

Hi @daipom

Thank you for the disuccsing regarding #3614.
And I appreciate the adding co-authored sign on this PR's commit message.

@masaki-hatada and Me have checked and tested heavily last week.
It looks good.
There is no concern for us.
I hope that this PR merged in the future release for fixing incorrect file handle issue.

Copy link
Contributor

@masaki-hatada masaki-hatada left a comment

Choose a reason for hiding this comment

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

Looks good for me.

test/plugin/test_in_tail.rb Outdated Show resolved Hide resolved
@daipom
Copy link
Contributor Author

daipom commented Jun 26, 2023

@kattz-kawa @masaki-hatada Thanks for your review!

@daipom
Copy link
Contributor Author

daipom commented Jun 26, 2023

@ashie Thanks for your review! I'll check it.

Copy link
Member

@ashie ashie left a comment

Choose a reason for hiding this comment

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

LGTM (above my comments are nitpicking)

If `refresh_watchers` run before `update_watcher`,
the old implementation of `update_watcher` detach wrongly the
new TailWatcher which is added in `refresh_watcher`.
This causes the problem of stopping tailing log and handle leak.

The test case `test_updateTW_after_refreshTW` reproduces this
problem.
This fix solves it.

There are another BUG about unwatching.
I adjusted some expected values of the tests for this BUG.
When `refresh_watcher` find the rotated old file AFTER
unwatching it (`rotate_wait`), then the logs will be collected in
duplicate.
If `refresh_watcher` find it BEFORE unwatching it (`rotate_wait`),
this problem doesn't occur because the position entry is still
alive.
We need to fix this and fix the adjusted expected values.

This fix is based on the content and discussion of the following
issue and PR:

* fluent#3614
* fluent#4185
* fluent#4191

Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>
Co-authored-by: Katuya Kawakami <kkawakami-je@nec.com>
Co-authored-by: Masaki Hatada <mas-hatada@nec.com>
Co-authored-by: Gary Zhu <garyzjq@outlook.com>
Co-authored-by: Takuro Ashie <ashie@clear-code.com>
@daipom daipom force-pushed the in_tail-ensure-detach-correct-watcher branch from 684632b to 1a86721 Compare June 26, 2023 08:56
@ashie ashie merged commit badef56 into fluent:master Jun 27, 2023
@ashie
Copy link
Member

ashie commented Jun 27, 2023

Thanks to all contributors who help investigating this issue!

@ashie
Copy link
Member

ashie commented Jun 27, 2023

TODO:

  • Apply it to v1.16 branch
  • How about follow_inode false?

@daipom daipom deleted the in_tail-ensure-detach-correct-watcher branch June 27, 2023 01:43
@daipom
Copy link
Contributor Author

daipom commented Jun 27, 2023

Thanks to all contributors who have given me much insight, especially @kattz-kawa, @masaki-hatada, and @garyzjq!
And thanks for your review!

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