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.

Sign up for GitHub

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

Jump to bottom

in_tail: Check detaching inode when follow_inodes #4191

Merged
merged 2 commits into from
Jun 30, 2023
Merged

in_tail: Check detaching inode when follow_inodes #4191

merged 2 commits into from
Jun 30, 2023

Conversation

garyzjq
Copy link
Contributor

@garyzjq garyzjq commented May 31, 2023 •
edited by daipom
Loading

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

What this PR does / why we need it:
Add validation to make sure detach_watcher is detaching expected watcher. This can avoid unexpectedly detach new watcher created for new log file and lead to log stuck transiently.

Add log to check that detaching inode is the same as the detaching TailWatcher's inode when enabling follow_inodes.

Note: If they do not match, canceling the detach (by adding return) may prevent an incorrect detach.
Since #4208 will prevent an incorrect detach, we will only add the warning log in this PR for now.

Docs Changes:
N/A

Release Note:
Fix transient log stuck in in_tail when log file rotated and follow_inodes is enabled
Same as the title.

@ashie ashie self-requested a review May 31, 2023 06:20
@garyzjq garyzjq force-pushed the jiaqz/inodestuck branch 2 times, most recently from 6e8bbde to fbc3c2d Compare May 31, 2023 06:30
This was referenced Jun 6, 2023
Closed
Closed
daipom added a commit to daipom/fluentd that referenced this pull request Jun 20, 2023
@daipom @kattz-kawa
@masaki-hatada @garyzjq @ashie
in_tail: Ensure to detach correct watcher on rotation with follow_inodes
8ec5545 
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 <[email protected]>
Co-authored-by: Katuya Kawakami <[email protected]>
Co-authored-by: Masaki Hatada <[email protected]>
Co-authored-by: Gary Zhu <[email protected]>
Co-authored-by: Takuro Ashie <[email protected]>
@daipom daipom mentioned this pull request Jun 20, 2023
Merged
daipom added a commit to daipom/fluentd that referenced this pull request Jun 23, 2023
@daipom @kattz-kawa
@masaki-hatada @garyzjq @ashie
in_tail: Ensure to detach correct watcher on rotation with follow_inodes
684632b 
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 <[email protected]>
Co-authored-by: Katuya Kawakami <[email protected]>
Co-authored-by: Masaki Hatada <[email protected]>
Co-authored-by: Gary Zhu <[email protected]>
Co-authored-by: Takuro Ashie <[email protected]>
@ashie
Copy link
Member

ashie commented Jun 26, 2023 •
edited
Loading

@garyzjq Sorry for my late response.
We are going to fix this issue with #4208, please check it.
I believe it resolves both stuck issue & leak issue.

daipom added a commit to daipom/fluentd that referenced this pull request Jun 26, 2023
@daipom @kattz-kawa
@masaki-hatada @garyzjq @ashie
in_tail: Ensure to detach correct watcher on rotation with follow_inodes
1a86721 
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 <[email protected]>
Co-authored-by: Katuya Kawakami <[email protected]>
Co-authored-by: Masaki Hatada <[email protected]>
Co-authored-by: Gary Zhu <[email protected]>
Co-authored-by: Takuro Ashie <[email protected]>
@ashie
Copy link
Member

ashie commented Jun 27, 2023

Although the issue will be fixed by #4208, this fix still might be useful as a last guard.
Especially, it's might better to add the log even if we don't return here.
So we'll keep open this for a while to consider about it.

@garyzjq
Copy link
Contributor Author

garyzjq commented Jun 28, 2023

Thanks @ashie . I also checked #4208 and looks good. The watcher to detach is directly passed to detach_watcher method instead of find from tail map, so should be correct by design.
I think I can change my PR to add a log but won't return (for safety), to see whether there's any other corner case which may lead to detach unexpected watcher.

ashie
ashie approved these changes Jun 28, 2023
View reviewed changes
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.

I think I can change my PR to add a log but won't return (for safety), to see whether there's any other corner case which may lead to detach unexpected watcher.

Thanks, it's worth to merge to check other cases.

@ashie ashie requested a review from daipom June 29, 2023 00:18
daipom
daipom approved these changes Jun 29, 2023
View reviewed changes
Copy link
Contributor

@daipom daipom left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@garyzjq
LGTM! Thanks!

Could you please do the following?

@garyzjq
Copy link
Contributor Author

garyzjq commented Jun 29, 2023

@garyzjq LGTM! Thanks!

Could you please do the following?

sure, done for DCO

@daipom daipom changed the title Check inode expectation to detach correct watcher when follow_inodes in_tail: Check detaching inode when follow_inodes Jun 30, 2023
@daipom daipom merged commit f436211 into fluent:master Jun 30, 2023
@daipom
Copy link
Contributor

daipom commented Jun 30, 2023

@garyzjq Thanks! I fixed the title and some issue comments and merge this!
#4190 and this PR have given us much insight to improve in_tail!
Thanks so much!

@garyzjq
Copy link
Contributor Author

garyzjq commented Jun 30, 2023

thanks @daipom and @ashie, really a nice experience for me to contribute to fluentd :)

@garyzjq garyzjq deleted the jiaqz/inodestuck branch July 2, 2023 16:20
@daipom daipom added this to the v1.16.2 milestone Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daipom daipom daipom approved these changes

@ashie ashie ashie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.16.2
3 participants
@garyzjq @ashie @daipom