Fix prometheus not restarting after config changes on systemd based systems #390
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alexjfisher
commented
Nov 18, 2019
| File { | ||
| notify => Class['prometheus::run_service'], | ||
| } | ||
| $systemd_notify = [Exec['prometheus-systemd-reload'], Class['prometheus::run_service']] |
There was a problem hiding this comment.
Both this variable and the Exec resource disappeared 2 years ago in #90
alexjfisher
commented
Nov 18, 2019
| } | ||
| } | ||
| 'systemd' : { | ||
| include 'systemd' |
There was a problem hiding this comment.
systemd::unit_file already has an include systemd
alexjfisher
commented
Nov 18, 2019
| owner => 'root', | ||
| group => 'root', | ||
| content => template('prometheus/prometheus.debian.erb'), | ||
| 'sysv', 'redhat', 'debian', 'sles' : { |
There was a problem hiding this comment.
These all declare the same file, so I've tried to remove a bit of duplication here.
alexjfisher
commented
Nov 18, 2019
| @@ -232,6 +224,7 @@ | |||
| group => $prometheus::server::group, | |||
| purge => true, | |||
| recurse => true, | |||
| notify => Class['prometheus::service_reload'], # After purging, a reload is needed | |||
There was a problem hiding this comment.
Previously, a purge of scrape job files would have caused a full service restart.
alexjfisher
commented
Nov 18, 2019
| @@ -94,6 +94,6 @@ | |||
|
|
|||
| Class['prometheus::install'] | |||
| -> Class['prometheus::config'] | |||
| -> Class['prometheus::run_service'] | |||
| -> Class['prometheus::run_service'] # Note: config must *not* be configured here to notify run_service. Some resources in config.pp need to notify service_reload instead | |||
There was a problem hiding this comment.
My first thought was to add
if $restart_on_change {
Class['prometheus::config'] ~> Class['prometheus::run_service']
}to this class. This would have been a mistake. Hopefully this comment will stop someone else having the same idea in the future! :)
alexjfisher
commented
Nov 18, 2019
| @@ -2,7 +2,7 @@ | |||
|
|
|||
| describe 'prometheus server basics' do | |||
| it 'prometheus server via main class works idempotently with no errors' do | |||
| pp = "class{'prometheus': manage_prometheus_server => true, version => '1.5.2' }" | |||
| pp = "class{'prometheus': manage_prometheus_server => true }" | |||
There was a problem hiding this comment.
Default version (> 2.0.0) is needed to test the admin API.
alexjfisher
force-pushed
the
issue_382
branch
from
9ca2ec5
to
ddc9d7b
Compare
alexjfisher
changed the title
bastelfreak
reviewed
Nov 18, 2019
manifests/config.pp
Outdated
| notify => Class['prometheus::run_service'], | ||
| } | ||
| $systemd_notify = [Exec['prometheus-systemd-reload'], Class['prometheus::run_service']] | ||
| $notify = Class['prometheus::run_service'] |
There was a problem hiding this comment.
we should now use a selector statement here?
There was a problem hiding this comment.
sure. Updated. I could always go either way when deciding if a selector is more readable than a simple if/else when the control expression is a Boolean.
In systemd, the service is currently not being restarted after configuration changes. See voxpupuliGH-382 The acceptance test in this commit reconfigures prometheus with the admin API enabled and then attempts to access it. See https://prometheus.io/docs/prometheus/latest/querying/api/#tsdb-admin-apis
The [unreliable](https://puppet.com/docs/puppet/5.5/lang_defaults.html#behavior) resource default ``` File{ notify => Class['prometheus::run_service'] } ``` is replaced by a `$notify` variable that is set on the relevant file resources *and* `systemd::unit_file`. Some care was needed to make sure the reload behaviour wasn't broken. ie If the configuration change is just a new scrape job that is collected, the service should only be reloaded, not restarted. Fixes voxpupuli#382
alexjfisher
force-pushed
the
issue_382
branch
from
ddc9d7b
to
8588d88
Compare
alexjfisher
changed the title
alexjfisher
changed the title
Rovanion
pushed a commit
to Rovanion/puppet-prometheus
that referenced
this pull request
May 5, 2021
Fix prometheus not restarting after config changes on systemd based systems
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The unreliable resource default
is replaced by a
$notifyvariable that is set on the relevant fileresources and
systemd::unit_file. Some care was needed to make surethe reload behaviour wasn't broken. ie If the configuration change is
just a new scrape job that is collected, the service should only be
reloaded, not restarted.
Fixes #382
This PR supersedes #383