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

rule_files does not seem to detect file changes #4235

Closed
avonwyss opened this Issue Jun 7, 2018 · 13 comments

Comments

Projects
None yet
4 participants
@avonwyss
Copy link

avonwyss commented Jun 7, 2018

Proposal

Use case. Why is this important?
Changes to rule files specified in rule_files should ideally take effect automatically, without the need to trigger a configuration reload manually. This would allow external tools/scripts to generate of modify these files without requiring direct interaction with Prometheus.

I would expect it to behave the same as scrape_configs.*.file_sd_configs.files which detects file changes as well as new files when using a wildcard.

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jun 7, 2018

You need to reload Prometheus's configuration for changes to rules to take effect, just as for prometheus.yml changes.

@avonwyss

This comment has been minimized.

Copy link
Author

avonwyss commented Jun 7, 2018

I'm aware of that, which is why I have made a proposal to add file change detection.

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jun 7, 2018

The problem with that is that it wouldn't allow for atomic changes of multiple files, which is why it's currently tied into the general reload logic.

@avonwyss

This comment has been minimized.

Copy link
Author

avonwyss commented Jun 7, 2018

I don't mind the general reload. Would it be a problem if changes to any of the change monitored files/patterns caused a general reload? That would keep it atomic...

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jun 7, 2018

Yes, that'd also prevent users from doing a multi-file atomic change as the first file changed would trigger a reload before the rest were done.

@avonwyss

This comment has been minimized.

Copy link
Author

avonwyss commented Jun 7, 2018

I understand what you are saying, but I'm not convinced that this is actually a problem. If I remember correctly the docs say that, whenever the config is not valid on a reload, it will not change the running configuration. Therefore an inconsistent multi-file change should not break anything. Also, usually file monitors triggers use some debouncing/quiet wait time which also mitigates this kind of issues.

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jun 7, 2018

It is a problem, say I want to replace file a.rules with b.rules. That's not possible atomically with your suggestion.

Also, usually file monitors triggers use some debouncing/quiet wait time which also mitigates this kind of issues.

That makes it into a race condition, which isn't much better. In addition you'd be breaking all the users who depend on the current documented behaviour.

@krasi-georgiev

This comment has been minimized.

Copy link
Member

krasi-georgiev commented Jun 7, 2018

@avonwyss if you use tools that generate these, can't it send a reload request to Prom as well?

@avonwyss

This comment has been minimized.

Copy link
Author

avonwyss commented Jun 7, 2018

@krasi-georgiev Of course I can create some mechanism to trigger the reload externally, but I try to keep the tools and systems as decoupled as possible. Since file monitoring actually exists in other places of the configuration it did not seem to be such a far-fetched idea.

@krasi-georgiev

This comment has been minimized.

Copy link
Member

krasi-georgiev commented Jun 7, 2018

try to keep the tools and systems as decoupled

To me it seems logical that the same tool that generates the config should be responsible for actually enabling the new config.

And @brian-brazil's argument about the race condition is a rather big blocker against changing the current behaviour.

@roidelapluie

This comment has been minimized.

Copy link
Contributor

roidelapluie commented Jun 10, 2018

PSA: We need that atomic change thing. To us changing that would cause troubles.

@avonwyss

This comment has been minimized.

Copy link
Author

avonwyss commented Jun 10, 2018

Since this proposal has apparently significant problem potential I'm closing it.
I have now added the --web.enable-lifecycle flag and trigger the reload externally via web request.

@avonwyss avonwyss closed this Jun 10, 2018

@lock

This comment has been minimized.

Copy link

lock bot commented Mar 22, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Mar 22, 2019

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