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

Always use absolute path for event and registry #3328

Merged
merged 1 commit into from
Feb 7, 2017

Conversation

ruflin
Copy link
Member

@ruflin ruflin commented Jan 10, 2017

During some testing with filebeat I realised that when a relative path glob is put into the filebeat config, the event will contain the relative path and also the state. In most cases this should not be an issue and so far no issues were reported.

For the state itself it is not an issue as they are compared based on inode/device. It could become an issue on restart in case a config was changed from a relative to an absolute path and the prospector does not detect, that the state would belong to the same prospector. This could also have an affect when migrating to this solutions. Old states could be left over in the registry file. But this requires, that someone was using relative paths before.

@ruflin ruflin added discuss Issue needs further discussion. Filebeat Filebeat labels Jan 10, 2017
@@ -137,6 +137,7 @@ func (h *Harvester) Harvest(r reader.Reader) {
event.InputType = h.config.InputType
event.DocumentType = h.config.DocumentType
event.JSONConfig = h.config.JSON
event.Name = h.state.Fileinfo.Name()
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this for discussion as it was requested several times in the past.

@ruflin ruflin force-pushed the relative-state-path branch 2 times, most recently from 95a37cf to 11d60f0 Compare January 17, 2017 08:15
@ruflin ruflin force-pushed the relative-state-path branch 2 times, most recently from feac103 to 5fc6fc5 Compare January 31, 2017 14:43
@ruflin ruflin added the review label Jan 31, 2017
@@ -10,6 +10,11 @@
The file from which the line was read. This field contains the full path to the file.
Copy link
Member

Choose a reason for hiding this comment

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

Can you drop the "absolute" keyword somewhere in the source description.

Copy link
Member Author

Choose a reason for hiding this comment

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

I replaced full with absolute.

@ruflin ruflin force-pushed the relative-state-path branch 5 times, most recently from bcde6c0 to c295289 Compare February 7, 2017 12:03
For example: `/var/log/system.log`.

- name: name
Copy link
Contributor

Choose a reason for hiding this comment

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

Note sure about calling it "name" since it might ambiguous. How about "filename"?

Copy link
Member Author

Choose a reason for hiding this comment

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

agree

@ruflin ruflin force-pushed the relative-state-path branch from c295289 to ab1e217 Compare February 7, 2017 12:24
@ruflin
Copy link
Member Author

ruflin commented Feb 7, 2017

I removed the Name part from this PR again as it is not directly related to this change.

@ruflin ruflin added v5.3.0 and removed discuss Issue needs further discussion. labels Feb 7, 2017
Copy link
Contributor

@tsg tsg left a comment

Choose a reason for hiding this comment

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

LGTM, waiting for green.

During some testing with filebeat I realised that when a relative path glob is put into the filebeat config, the event will contain the relative path and also the state. In most cases this should not be an issue and so far no issues were reported.

For the state itself it is not an issue as they are compared based on inode/device. It could become an issue on restart in case a config was changed from a relative to an absolute path and the prospector does not detect, that the state would belong to the same prospector. This could also have an affect when migrating to this solutions. Old states could be left over in the registry file. But this requires, that someone was using relative paths before which was never recommended.
@ruflin ruflin force-pushed the relative-state-path branch from ab1e217 to 33ec85b Compare February 7, 2017 16:12
@tsg tsg merged commit cd5dc81 into elastic:master Feb 7, 2017
@ruflin ruflin deleted the relative-state-path branch February 7, 2017 16:15
pcfens added a commit to pcfens/puppet-filebeat that referenced this pull request Mar 28, 2017
The default registry file must use an absolute path
(elastic/beats#3328). This is probably a breaking
change.

Fixes #46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants