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

chronograf not creating a pidfile when requested with the "-pidfile=" flag #2075

Closed
petercelentano opened this issue Oct 4, 2017 · 14 comments
Assignees
Milestone

Comments

@petercelentano
Copy link

My current version of Chronograf is:

2017/10/04 19:24:39 Chronograf 1.3.8.3 (git: e63f073c8fcc309abcc5a3b04aad1eb692b69864)

Platform: Linux, Centos 7.3
My start command is:

./chronograf --host 0.0.0.0 --port 8888 -b lib/chronograf/chronograf-v1.db -c share/chronograf/canned --influxdb-url=http://127.0.0.1:8086 --kapacitor-url=http://127.0.0.1:9092 -pidfile=chronograf.pid

If using:

-pidfile=chronograf.pid

is not the correct syntax, what is? Using the start command listed above, Chronograf will not create a pidfile

@petercelentano
Copy link
Author

Is this a bug? Is Chronograf supposed to be able to create a pidfile with the above flag?

@thibodux
Copy link

thibodux commented Oct 6, 2017

It doesn't look like that option is currently supported like the rest of the TICK stack. Maybe this should be an enhancement request.

@petercelentano
Copy link
Author

In that case it should probably be a feature request.

@goller
Copy link
Contributor

goller commented Oct 6, 2017

@petercelentano thanks for writing in! Did you want to try your hand at adding code to the project?

If so, you'd add an option in server/server.go for the PID file.

@timhallinflux I'm looking at this library: https://github.com/facebookgo/pidfile to handle generating process id files. However, it has facebook's patent clause here: https://github.com/facebookgo/pidfile/blob/master/patents Is this an issue?

@petercelentano
Copy link
Author

I'm not too familiar with GO yet, and I don't have a build environment setup yet. If adding this functionality would be trivial, I would appreciate help with it. I will get a build env together though so that I can test the fix :)

@thibodux
Copy link

thibodux commented Oct 9, 2017

@goller why not just take parts of code from TICK stack like cmd.writePIDFile found here: https://github.com/influxdata/kapacitor/blob/0a8abecf0fd73c7b3a5b930a0c2923033ac92b80/cmd/kapacitord/run/command.go

@goller
Copy link
Contributor

goller commented Oct 9, 2017

@thibodux even better! Thank you!

@petercelentano
Copy link
Author

Any progress on this all? I'm willing to do testing as soon as a fix is available :)

@goller
Copy link
Contributor

goller commented Oct 18, 2017

@nhaugo nhaugo added this to the 1.3.11 milestone Oct 18, 2017
@thibodux
Copy link

thibodux commented Oct 18, 2017

The Kapacitor implementation (https://github.com/influxdata/kapacitor/blob/0a8abecf0fd73c7b3a5b930a0c2923033ac92b80/cmd/kapacitord/run/command.go#L187) matches the InfluxDB implementation.

The discrepancy is because InfluxDB and Kapacitor seem to assume that they are running as services on *nix, and think the service manager will remove the PID file (see https://github.com/influxdata/influxdb/blob/276a26656e316b1068fe1a66952dff8a17f39fa1/scripts/init.sh).

On the other hand Telegraf takes care of removing the PID file whenever it closes (via the defer call in the code).

At this point, I would also agree that the telegraf implementation makes more sense just because I don't like the assumption. In fact, it would be best if they could use the same logic across all four, but I don't know if that change would suddenly break a lot of systemd or init.d stuff in the wild (hopefully not).

@thibodux
Copy link

After rethinking, I retract my previous statement about which implementation I prefer. I think Kapacitor and Influx is generally cleaner. Telegraf startup code seems to be a bit more thrown together compared to the others and how it reads configuration settings.

The one caveat would be how the pid file gets removed would need to be changed if Telegraf was made to look more like the others.

Just my two cents

@goller
Copy link
Contributor

goller commented Oct 19, 2017

@thibodux Agreed. Adding the PID file removal would be good.

@nhaugo nhaugo added the ready label Oct 23, 2017
@nhaugo nhaugo modified the milestones: 1.3.11, 1.4.0 Nov 6, 2017
@nhaugo nhaugo modified the milestones: 1.4.0, 1.4.x Nov 30, 2017
@russorat russorat assigned desa and unassigned jsternberg Jul 20, 2018
@russorat russorat removed this from the 1.4.x milestone Nov 28, 2018
@stale
Copy link

stale bot commented Jul 23, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jul 23, 2019
@stale
Copy link

stale bot commented Jul 30, 2019

This issue has been automatically closed because it has not had recent activity. Feel free to reopen if this issue is still important to you. Thank you for your contributions.

@stale stale bot closed this as completed Jul 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants