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

reload config on SIGHUP & /-/reload, implement /-/healthy #121

Merged
merged 4 commits into from
Apr 2, 2024

Conversation

carlosrodfern
Copy link
Contributor

Fixes: #118

@SuperQ
Copy link
Owner

SuperQ commented Dec 27, 2023

Hmm, I'm not sure I like re-exec as a way to do this.

@carlosrodfern
Copy link
Contributor Author

carlosrodfern commented Jan 12, 2024

@SuperQ , do you have any specific concern about the approach? I may be able to look into it.

This is not an uncommon solution in Unix, but I noticed that in the prometheus related programs world, the most common solution is purely programmatically resetting the state manually. However, I'm not sure that approach is followed because of some analysis of tradeoffs between re-exec or programmatic in every case. I understand if you prefer to close this PR, and look into a solution more aligned with the rest of prometheus related programs for uniformity, or into a solution that fulfills some functional requirements better.

@carlosrodfern
Copy link
Contributor Author

While dealing with another program, I thought of something. It would be best if the HUP signal would try to reload the config, and if it fails, keep using the previous one while outputting the error. That's not solved in this PR. Let me see if I can work on a better solution here.

@SuperQ
Copy link
Owner

SuperQ commented Mar 27, 2024

Yes, take a look at some of the other Prometheus exporters that support SIGHUP. There are some reasonable reload patterns.

Fixes: SuperQ#118
Signed-off-by: Carlos Rodriguez-Fernandez <[email protected]>
@carlosrodfern carlosrodfern marked this pull request as draft March 28, 2024 15:48
@carlosrodfern carlosrodfern marked this pull request as ready for review March 29, 2024 04:35
* `/-/healthy` indicates that the http server successfully started, and
  consequentially the config was successfully parsed.
* `/-/reload` allows to attempt to reload the config, which is expected
  to be called by a sidecar that watches for config file changes

Signed-off-by: Carlos Rodriguez-Fernandez <[email protected]>
@carlosrodfern
Copy link
Contributor Author

I also included a commit for /-/healthy and /-/reload

@carlosrodfern carlosrodfern changed the title reload config on SIGHUP reload config on SIGHUP & /-/reload, implement /-/healthy Mar 29, 2024
@SuperQ
Copy link
Owner

SuperQ commented Apr 1, 2024

Looks like there's a minor lint issue.

Signed-off-by: Carlos Rodriguez-Fernandez <[email protected]>
collector.go Outdated Show resolved Hide resolved
Signed-off-by: Carlos Rodriguez-Fernandez <[email protected]>
Copy link
Owner

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

Did some local testing, works great.

@SuperQ SuperQ merged commit 584df24 into SuperQ:master Apr 2, 2024
4 checks passed
SuperQ added a commit that referenced this pull request Apr 2, 2024
* [FEATURE] Support runtime config reload #121

Signed-off-by: SuperQ <[email protected]>
SuperQ added a commit that referenced this pull request Apr 2, 2024
* [FEATURE] Support runtime config reload #121

Signed-off-by: SuperQ <[email protected]>
@SuperQ SuperQ mentioned this pull request Apr 2, 2024
pingers = append(pingers, pinger)
}
}
sc.Unlock()
Copy link
Contributor

@dswarbrick dswarbrick Apr 2, 2024

Choose a reason for hiding this comment

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

The prepare function can return (with error) before it unlocks the mutex, leaving it perpetually locked. Since smokerpingers is reused within the main function, this could result in a deadlock when the prepare function is called a subsequent time. I suggest a defer sc.Unlock() immediately after the sc.Lock() line earlier.

Copy link
Owner

Choose a reason for hiding this comment

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

Good catch, anyone want to open a PR for this fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, indeed. I'll fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened PR #148 with the fix.

Thank you!

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.

[Feature Request] Reload configuration dynamically with signal HUP
3 participants