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

Windows: Fix a bug that the wrong log file is reopened with log rotate setting when flushing or graceful reloading #4054

Merged
merged 9 commits into from
Feb 16, 2023

Conversation

daipom
Copy link
Contributor

@daipom daipom commented Feb 13, 2023

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.

if @chuser || @chgroup
chuid = @chuser ? ServerEngine::Privilege.get_etc_passwd(@chuser).uid : nil
chgid = @chgroup ? ServerEngine::Privilege.get_etc_group(@chgroup).gid : nil
File.chown(chuid, chgid, @path)
end

def reopen!
if @path && @path != "-"
@logdev.reopen(@path, "a")
end
self
end

I can't assume that File.chown is used on Windows, but reopen 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

  • Prepare config
<system>
  <log>
    rotate_age 5
    rotate_size 2000
  </log>
</system>

 <source>
   @type sample
   tag test
 </source>
 
 <match test.**>
   @type stdout
 </match>
  • Run Fluentd
$ bundle exec fluentd -c /test/fluent.conf -o /test/log/fluent.log
  • We can find the log files
fluent-supervisor-0.log
fluent-0.log
fluent-0.log.0
...
  • Flush(SIGUSR1)
    • We can check the pid of the supervisor process in fluent-supervisor-0.log.
    • starting fluentd-1.15.3 pid=4208 ruby="2.6.6"
$ fluent-ctl flush {pid of supervisor process}
  • Then Fluentd reopens fluent.log for the worker process, although this should be fluent-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.

$ fluent-ctl reload {pid of supervisor process} (another shell)
log shifting failed. Permission denied @ rb_file_s_rename - (.\fluent\log\fluent-supervisor-0.log, .\fluent\log\fluent-supervisor-0.log.0)
log writing failed. closed stream

Docs Changes:
Not needed.

Release Note:
Same as the title.

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 <fukuda@clear-code.com>
@daipom
Copy link
Contributor Author

daipom commented Feb 13, 2023

Oh, I have to add tests.

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.

Good catch!

lib/fluent/supervisor.rb Show resolved Hide resolved
lib/fluent/supervisor.rb Show resolved Hide resolved
@ashie
Copy link
Member

ashie commented Feb 13, 2023

I'll wait merging this until you add tests for it.

@daipom
Copy link
Contributor Author

daipom commented Feb 13, 2023

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 <fukuda@clear-code.com>
to allow us to confirm the data after executing a test.

Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>
Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>
Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>
Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>
@daipom
Copy link
Contributor Author

daipom commented Feb 15, 2023

I added tests.

I confirmed one of those tests failed on master on Windows and this PR fixed it.

Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>
@daipom
Copy link
Contributor Author

daipom commented Feb 15, 2023

I wonder why using mock.proxy, not just mock.

When I change as follows, the test fails. I don't understand what the difference is.
What does the proxy being not passed a block mean?

- mock.proxy(File).chmod(0o777, path.parent.to_s).once
+ mock(File).chmod(0o777, path.parent.to_s).once

Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>
@daipom daipom force-pushed the fix-reopen-log-on-windows-with-rotate branch from 64022be to ade4857 Compare February 15, 2023 07:16
@ashie
Copy link
Member

ashie commented Feb 15, 2023

Probably it intends to check File.chmod is called just once.
And need to call real File.chmod for following checks.

@daipom
Copy link
Contributor Author

daipom commented Feb 15, 2023

need to call real File.chmod for following checks.

Oh, thanks! Just simple mock(File).chmod destroys the original behavior of File.chmod, so we have to use proxy here.

@ashie ashie added this to the v1.16.0 milestone Feb 16, 2023
Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>
@ashie ashie merged commit 34f2c55 into fluent:master Feb 16, 2023
@ashie
Copy link
Member

ashie commented Feb 16, 2023

Thanks!

@daipom daipom deleted the fix-reopen-log-on-windows-with-rotate branch February 16, 2023 09:05
@daipom
Copy link
Contributor Author

daipom commented Feb 16, 2023

Thanks for your review!

@daipom daipom changed the title Windows: Fix a bug that the wrong log file is reopened with log ratote setting when flushing or graceful reloading Windows: Fix a bug that the wrong log file is reopened with log rotate setting when flushing or graceful reloading Mar 29, 2023
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.

2 participants