Remove empty temp dirs at plugin startup. #39
Conversation
…es due to scaning tools such as antiviruses.
…dir removal used.
|
🔴 CI is not relative and even re-run doesn't kick new run. Analyzing the error a bit more provides a clear picture that it is due to docker 8.10 snapshot which uses JRuby 9.3.4.0 and has |
lib/logstash/outputs/s3.rb
Outdated
| .select { |path| ::File.directory?(path) } | ||
| .select { |path| (Dir.entries(path) - %w[ . .. ]).empty? } # current and parent dirs escape | ||
| .each do |path| | ||
| FileUtils.rm_rf(path, :secure => true) |
There was a problem hiding this comment.
If the directory is empty, do we need to use rm_rf here, or can we use rm_f?
There was a problem hiding this comment.
The intention was here to avoid handling StandardError cases, such as IOError.
There was a problem hiding this comment.
I guess my question was more around the need to use the recursive (and destructive) form of rm rather than the force functionality. Would you expect that the recursive form is appropriate, and worth the risk of data loss in the event of a coding error?
There was a problem hiding this comment.
Ah got it. Yup, we don't need recursive operation here. We are starting to remove the leaf of the tree. Our operation itself is recursive, will change it to FileUtils.rm_f.
Leave debug logs while removing an empty dir. Apply unit test suggestions. Co-authored-by: Rob Bavey <rob.bavey@elastic.co>
Release notes
[rn:skip]
What does this PR do?
For some environments, especially Windows OS, file scanning (such as antivirus enabled) tools hold temporarily created dirs' IO and a plugin cannot remove the temporarily dir after successfully uploading file to S3. Over time, there will be many empty temp dirs left under the
temporary_directory.This PR introduces a logic which, at the Logstash start up, plugin cleans up temporary dirs.
Why is it important/What is the impact to the user?
The users who are facing empty temp dirs left issues get benefit from this automation.
Checklist
[] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files (and/or docker env variables)Author's Checklist
How to test this PR locally
Related issues
Use cases
Screenshots
Logs