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

Added home, data, config paths #1373

Merged
merged 2 commits into from Apr 13, 2016

Conversation

Projects
None yet
5 participants
@tsg
Copy link
Collaborator

commented Apr 11, 2016

Implements part of #1371.

  • Adds the CLI flags for 'path.home', 'path.data', 'path.config'
  • These can be also set in the configuration file:
path:
  home: /tmp/
  data: /tmp/data/
  config: /tmp/config/
  • Filebeat registry location is now by default {data_path}/registry. Note: this is
    a breaking change from .filebeat previously in 1.X.
  • The ES templates are searched by default in the config path

@tsg tsg referenced this pull request Apr 11, 2016

Closed

Directory layout and data path #1371

14 of 14 tasks complete
@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Apr 11, 2016

Thinking about migration. Should we have a script that checks for the old registry file on startup and moves it to the new place in 5.0?

@tsg

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 11, 2016

For the non-deb/rpm case, the old location is anyone's guess. It was .filebeat in the working directory, so it depends on how filebeat was executed. For the deb/rpm case, BC is maintained. I hope most people used deb/rpm.

@tsg tsg added the review label Apr 12, 2016

@tsg

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 12, 2016

This is now ready for reviews. The rest of the items #1371 will be done in other PRs.

@@ -14,7 +14,7 @@ import (

// Defaults for config variables which are not set
const (
DefaultRegistryFile = ".filebeat"
DefaultRegistryFile = "registry"

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Apr 12, 2016

Member

I think we should add the .json extension to the name.

This comment has been minimized.

Copy link
@tsg

tsg Apr 12, 2016

Author Collaborator

The reason I used registry is that on DEB/RPM installations the file name already used is /usr/share/filebeat/registry. By using registry we can keep BC for those cases at least.

In addition, JSON is an implementation detail which we might change in the future. Also, adding the extension could be inviting people to edit the file, which I guess is the last thing we want.

I see the three +1, but is there a strong reason to add the .json?

This comment has been minimized.

Copy link
@ruflin

ruflin Apr 13, 2016

Collaborator

Removed my +1 as I think the point about implementation and potentially changing in the future is a good point. So even if we switch to the format "xyz" the file name stays the same.

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Apr 13, 2016

Member

I don't have a strong opinion here. So leaving it as registry is fine with me. (I never meant to +1 my own comment anyways; I didn't realize clicking the thumb icon added a +1 until after I clicked it, and then I couldn't figure out how to undo.)

The reason I used registry is that on DEB/RPM installations the file name already used is /usr/share/filebeat/registry. By using registry we can keep BC for those cases at least.

Those installations were using a non-default value so they should not be affected by the change.

In addition, JSON is an implementation detail which we might change in the future.

True, leaving it without an extension makes it easier to change underlying file type.

This comment has been minimized.

Copy link
@tsg

tsg Apr 13, 2016

Author Collaborator

Those installations were using a non-default value so they should not be affected by the change.

Good point. But if it's just registry I can get rid of one of those nasty sed commands on the configuration file at package time by just setting the data path in the init script.

// InitPaths sets the default paths in the configuration based on CLI flags,
// configuration file and default values. It also tries to create the data
// path with mode 0755 and returns an error on failure.
func InitPaths(cfg *Path) error {

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Apr 12, 2016

Member

My preference is to declare this as a method on a *Path receiver. Then make a singleton Path somewhere. So instead of having a function that affects global state you have singleton object with methods on it. This gives users of the class a bit more freedom in their implementation. For example, if I wanted to decouple my class from the global state, I could inject a *Path into my class making testing easier.

This comment has been minimized.

Copy link
@tsg

tsg Apr 12, 2016

Author Collaborator

@andrewkroh Thanks for the review. I put the public methods on Path and made a public Paths singleton. I still kept the package level functions for convenience, following the model of flag and log packages from the Golang stdlib. WDYT?

This comment has been minimized.

Copy link
@andrewkroh
// This is a separate method because it needs to be called after
// logging was initialized. InitPaths is typically called before
// logging is initialized.
func LogPaths() {

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Apr 12, 2016

Member

Consider decoupling this method from the logger. Instead have a method that returns a string representation of the Path then allow the caller to log it.

This comment has been minimized.

Copy link
@urso

urso Apr 12, 2016

Collaborator

e.g. Strings method? Or some method returning map[string]string with pathes and types (automatically printable)

This comment has been minimized.

Copy link
@tsg

tsg Apr 12, 2016

Author Collaborator

Replaced with a String() method.

@andrewkroh

This comment has been minimized.

Copy link
Member

commented Apr 12, 2016

It would be good to do a search for .filebeat in all files and update any references to the new registry file name.

@tsg

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 12, 2016

I replaced all occurrences of .filebeat with the commit above, and in the process updated the docs a little. We'll need a "guide" on the paths, though, similar with what ES has. That will come in a separate PR (cc @dedemorton, just FYI).

@@ -390,13 +390,13 @@ filebeat:

===== registry_file

The name of the registry file. By default, the registry file is put in the current
working directory. If the working directory changes for subsequent runs of Filebeat, indexing starts from the beginning again.
The name of the registry file. If a relative path is used, it is considered relative to the

This comment has been minimized.

Copy link
@dedemorton

dedemorton Apr 12, 2016

Contributor

I'm not sure what you mean by data path here. Do you mean relative to the folders that are being crawled? Does that mean there is more than one registry file?

@@ -39,6 +39,15 @@ Start http server for profiling. This option is useful for troubleshooting and p
Write memory profile data to the specified output file. This option is useful for
troubleshooting the Beat.

*`-path.config`*::

This comment has been minimized.

Copy link
@dedemorton

dedemorton Apr 12, 2016

Contributor

I haven't been following the discussion about these changes, which is probably good because I'm confused by some things that might also confuse users. I'm not sure what you mean here with some of these options. What are the data files that you're referring to? Is this a default location for the files you want Filebeat to read? And by setting the default location for configuration, do you mean mean that this option sets the default location for config files? And when you talk about miscellaneous files, can you be more specific? Maybe you are already planning to cover this in the other PR. :-)

This comment has been minimized.

Copy link
@tsg

tsg Apr 13, 2016

Author Collaborator

Yes, we need an extra "guide" to explain this things, I'll make that in a separate PR and then put links to that page from the reference sections. This is the issue explaining the paths and this is the Elasticsearch explanations page that I plan to mostly follow for our docs as well.

@tsg tsg force-pushed the tsg:directory_layout branch from 7946c92 to de4c8c9 Apr 13, 2016

tsg added some commits Apr 11, 2016

Added home, data, config paths
* Adds the CLI flags for 'path.home', 'path.data', 'path.config'
* These can be also set in the configuration file:

```
path:
  home: /tmp/
  data: /tmp/data/
  config: /tmp/config/
```

* Filebeat registry location is now by default `{data_path}/registry`. Note: this is
a breaking change from `.filebeat` previously in 1.X.
* The ES templates are searched by default in the config path

@tsg tsg force-pushed the tsg:directory_layout branch from de4c8c9 to 2023850 Apr 13, 2016

@tsg

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 13, 2016

Cleaned history and rebased to master.

@andrewkroh

This comment has been minimized.

Copy link
Member

commented Apr 13, 2016

LGTM

@urso urso merged commit 56a0296 into elastic:master Apr 13, 2016

4 checks passed

CLA Commit author is a member of Elasticsearch
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details

@tsg tsg deleted the tsg:directory_layout branch Aug 25, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.