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

Updates storage path for internal data to use subfolder in Logstash's data.path #106

Merged

Conversation

andsel
Copy link
Contributor

@andsel andsel commented Mar 4, 2022

Release notes

Change default path of 'last_run_metadata_path' to be rooted in the LS data.path folder and not in $HOME. Also apply a path forward action to move existing metadata file to the new destination.

What does this PR do?

Change default path of 'last_run_metadata_path' to be rooted in the LS data.path folder and not in $HOME, in the input plugin.
Provide a path forward for existing metada file in $HOME to be moved in the new data.path destination.

Updates the path used by derby inside the jdbc_static filter plugin to use Logstash data.path subfolder.

Why is it important/What is the impact to the user?

It avoid to store state data into path that Logstash process doesn't have the rights to access.
After PR elastic/logstash#12782, released with Logstash 7.13.0 , the owner of Logstash installation dir changed from logstash user to root, so the $HOME directory used by Logstash process is not writable by it.
The process must store data only in the data.path folder.
There's the expectation to create the smoothest path forward, so that users doesn't need to manually update their pipelines nor to manually intervene in moving files on their machines.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • [] I have made corresponding changes to the documentation
  • [ ] I have made corresponding change to the default configuration files (and/or docker env variables)
  • I have added tests that prove my fix is effective or that my feature works

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Use cases

As a user that uses the record_last_run I want to be able to upgrade to Logstash >= 7.13 without any throubles about access rights on internal storage files.

@andsel andsel marked this pull request as draft March 4, 2022 16:52
@andsel andsel marked this pull request as ready for review March 4, 2022 17:19
@andsel andsel added the bug Something isn't working label Mar 4, 2022
@andsel andsel changed the title Change default path of 'last_run_metadata_path' to be rooted in the L… Updates storage path for internal data to go under the Logstash's data folder Mar 4, 2022
lib/logstash/inputs/jdbc.rb Outdated Show resolved Hide resolved
yaauie
yaauie previously requested changes Mar 10, 2022
lib/logstash/inputs/jdbc.rb Outdated Show resolved Hide resolved
lib/logstash/filters/jdbc_static.rb Outdated Show resolved Hide resolved
lib/logstash/filters/jdbc_static.rb Outdated Show resolved Hide resolved
lib/logstash/filters/jdbc_static.rb Show resolved Hide resolved
lib/logstash/inputs/jdbc.rb Outdated Show resolved Hide resolved
lib/logstash/inputs/jdbc.rb Outdated Show resolved Hide resolved
@andsel
Copy link
Contributor Author

andsel commented Mar 15, 2022

The failure on 7.x and 8.x SNAPSHOT is tracked in elastic/logstash#13890

@andsel andsel requested a review from yaauie March 15, 2022 09:51
@andsel andsel changed the title Updates storage path for internal data to go under the Logstash's data folder Updates storage path for internal data to use subfolder in Logstash's data.path Mar 15, 2022
@andsel andsel force-pushed the fix/use_data_dir_to_store_last_run_file branch from 6cff405 to f210543 Compare March 18, 2022 13:52
Copy link
Contributor

@yaauie yaauie left a comment

Choose a reason for hiding this comment

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

Changes and flow looks good. I've left a couple nitpicks.

lib/logstash/filters/jdbc_static.rb Outdated Show resolved Hide resolved
lib/logstash/filters/jdbc_static.rb Outdated Show resolved Hide resolved
lib/logstash/filters/jdbc_static.rb Outdated Show resolved Hide resolved
spec/inputs/jdbc_spec.rb Outdated Show resolved Hide resolved
@andsel
Copy link
Contributor Author

andsel commented Mar 23, 2022

The 🔴 CI is due to a rubygems Bundler fix that land in 8.1.2(elastic/logstash#13904)

@andsel andsel requested a review from yaauie March 23, 2022 16:24
@roaksoax roaksoax dismissed yaauie’s stale review March 28, 2022 15:11

All changes have been implemented, requesting re-review.

Copy link
Contributor

@yaauie yaauie left a comment

Choose a reason for hiding this comment

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

One change about specifying which file to delete, and a couple nitpicks :)

lib/logstash/filters/jdbc_static.rb Outdated Show resolved Hide resolved
lib/logstash/filters/jdbc_static.rb Outdated Show resolved Hide resolved
lib/logstash/filters/jdbc_static.rb Outdated Show resolved Hide resolved
@andsel andsel requested a review from yaauie May 4, 2022 16:22
@andsel andsel force-pushed the fix/use_data_dir_to_store_last_run_file branch from 4f72e4e to 77f9c18 Compare May 5, 2022 07:51
@andsel
Copy link
Contributor Author

andsel commented May 5, 2022

@yaauie the PR is ready for another round of review :-)

@andsel andsel force-pushed the fix/use_data_dir_to_store_last_run_file branch from 77f9c18 to bf964f1 Compare May 19, 2022 14:42
Copy link
Contributor

@yaauie yaauie left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants