-
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
Windows: Fix a bug that the wrong log file is reopened with log rotate setting when flushing or graceful reloading #4054
Windows: Fix a bug that the wrong log file is reopened with log rotate setting when flushing or graceful reloading #4054
Conversation
When log rotation is enabled on Windows, the log path is separated for each process. But, the new path value is not applied to the instance variable, so it breaks the following behavior, since fluent#2663. * `File.chown(chuid, chgid, @path)` * `@logdev.reopen(@path, "a")`. I can't assume that `File.chown` is used on Windows, but `reopen` must be fixed. Signed-off-by: Daijiro Fukuda <[email protected]>
Oh, I have to add tests. |
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.
Good catch!
I'll wait merging this until you add tests for it. |
Thanks. Please. |
Especially on Windows, need closing to remove log files. Signed-off-by: Daijiro Fukuda <[email protected]>
to allow us to confirm the data after executing a test. Signed-off-by: Daijiro Fukuda <[email protected]>
Signed-off-by: Daijiro Fukuda <[email protected]>
Signed-off-by: Daijiro Fukuda <[email protected]>
Signed-off-by: Daijiro Fukuda <[email protected]>
I added tests. I confirmed one of those tests failed on master on Windows and this PR fixed it. |
Signed-off-by: Daijiro Fukuda <[email protected]>
I wonder why using When I change as follows, the test fails. I don't understand what the difference is. - mock.proxy(File).chmod(0o777, path.parent.to_s).once
+ mock(File).chmod(0o777, path.parent.to_s).once |
Signed-off-by: Daijiro Fukuda <[email protected]>
64022be
to
ade4857
Compare
Probably it intends to check |
Oh, thanks! Just simple |
Signed-off-by: Daijiro Fukuda <[email protected]>
Thanks! |
Thanks for your review! |
When log rotation is enabled on Windows, the log path is separated for each process.
But, the new path value is not applied to the instance variable, so it breaks the following behavior, since #2663.
fluentd/lib/fluent/supervisor.rb
Lines 576 to 580 in bf7498d
fluentd/lib/fluent/supervisor.rb
Lines 601 to 606 in bf7498d
I can't assume that
File.chown
is used on Windows, butreopen
must be fixed.(
--user
and--group
are not intended to be used on Windows, right? If so, I will add some comments to the doc and code.)Which issue(s) this PR fixes:
I found this problem during fixing:
I thought this should be fixed in a separate PR, so I made this PR first.
What this PR does / why we need it:
This bug breaks the function of reopening the log file with log rotation enabled on Windows, since
We need this fix to fix this problem.
How to Reproduce
$ bundle exec fluentd -c /test/fluent.conf -o /test/log/fluent.log
fluent-supervisor-0.log
.starting fluentd-1.15.3 pid=4208 ruby="2.6.6"
$ fluent-ctl flush {pid of supervisor process}
fluent.log
for the worker process, although this should befluent-0.log
.We can find a similar problem in graceful reloading(SIGUSR2), but this often causes an error in rotating supervisor log file.
This seems to have another bug, I would like to exclude this bug for now.
Docs Changes:
Not needed.
Release Note:
Same as the title.