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

Add configuration management service #674

Closed
wants to merge 2 commits into from
Closed

Conversation

timraymond
Copy link
Contributor

This sets up a configuration managemet service on a configureable port
(which itself is configureable by the configuration management
service!). It sets up an HTTP handler on this port whereby GETs to /
will deliver the current TOML configuration that Telegraf booted from.
POSTs to / will update that configuration. Telegraf can then be
restarted from the configuration by sending SIGHUP through existing
mechanisms.

This configuration management service is not authenticated in any way!
It is assumed that this service will only be available behind the
firewall.

This sets up a configuration managemet service on a configureable port
(which itself is configureable by the configuration management
service!). It sets up an HTTP handler on this port whereby GETs to /
will deliver the current TOML configuration that Telegraf booted from.
POSTs to / will update that configuration. Telegraf can then be
restarted from the configuration by sending SIGHUP through existing
mechanisms.

This configuration management service is not authenticated in any way!
It is assumed that this service will only be available behind the
firewall.
@@ -175,6 +178,9 @@ var header = `# Telegraf configuration
# Override default hostname, if empty use os.Hostname()
hostname = ""

# Set configuration management port
Copy link
Contributor

Choose a reason for hiding this comment

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

these should be indented by 2 spaces rather than tabs

@sparrc
Copy link
Contributor

sparrc commented Feb 10, 2016

we have a separate PR for doing configuration management via etcd: #651, would that accomplish the same task?

@timraymond
Copy link
Contributor Author

It would, but Managed Hosting isn't quite at the point where they can make use of it at the moment. Configuration management via Etcd would likely be combined with some larger changes on their side. I'm told that will happen eventually, but this will be easier for them to use for the time being.

This was previously config-port, which is incorrect. naoina/toml does
snake case <-> camel case. Also changed leading tabs to 2 spaces.
@sparrc
Copy link
Contributor

sparrc commented Feb 12, 2016

The service already has the SIGHUP method of telling it to reload it's config, why not just change the config file on disk before sending the SIGHUP? What does having this service running do?

I guess what I'm saying is that it doesn't seem any easier to change the file via a POST vs. just changing it on disk directly.

@timraymond
Copy link
Contributor Author

That seems simpler to me... @kfitzpatrick does that ☝️ work for you?

@sparrc
Copy link
Contributor

sparrc commented Feb 18, 2016

I'm going to close this unless there is significant opposition. Exposing and changing the config file over HTTP seems like a mess IMHO.

The SIGHUP mechanism exists for the purpose of reloading the config file on disk, I see this PR creating the following problem very easily:

  1. admin starts telegraf service
  2. admin POSTs new config to endpoint
  3. admin SIGHUPs telegraf service so that it reloads the new config
  4. admin restarts telegraf service or reboots box (or service dies and restarts itself).
  5. telegraf is now running again with the on disk config, and has no memory of the POSTed config.

My main question is: why create this problem? Why not just change the file on disk?

@sparrc sparrc closed this Feb 18, 2016
@sparrc sparrc deleted the tr-config-mgmt branch February 26, 2016 15:22
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