Prometheus2 charm

Merge ~stub/charm-prometheus2:fix-restarts-bug-1847472 into ~prometheus-charmers/charm-prometheus2:master

  1. Git
  2. lp:~stub/charm-prometheus2
  3. fix-restarts-bug-1847472
  4. Merge into master
Proposed by Stuart Bishop
Status: Merged
Approved by: Stuart Bishop
Approved revision: b19e6ef3cd187a807ccee4342ec947c90178d21b
Merged at revision: f6d88291edba42d80dd6324a1c87192a3076614d
Proposed branch: ~stub/charm-prometheus2:fix-restarts-bug-1847472
Merge into: ~prometheus-charmers/charm-prometheus2:master
Diff against target: 50 lines (+8/-2)
2 files modified
reactive/prometheus.py (+5/-2)
tox.ini (+3/-0)
Reviewer Review Type Date Requested Status
Joel Sing (community) +1 Approve
Canonical IS Reviewers Pending
Review via email: mp+374260@code.staging.launchpad.net

Commit message

Only restart when related rules have changed

Only do reconfiguration and restart if rules provided
by the prometheus-rules relation have actually changed.

Remove some lint.

Addresses lp:1847472

To post a comment you must log in.
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote :

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

Revision history for this message
Stuart Bishop (stub) wrote :

This will need testing against a charm using the prometheus-rules relation (which I don't have handy) before promoting to stable. It is working if prometheus is not restarted every 5 minutes in the update-status hook when related to a charm that has set some rules.

Revision history for this message
Ed Stewart (emcs2) wrote :

Thanks for this - we understand Prometheus can reload its configuration at runtime without a full restart:

If the new configuration is not well-formed, the changes will not be applied. A configuration reload is triggered by sending a SIGHUP to the Prometheus process or sending a HTTP POST request to the /-/reload endpoint (when the --web.enable-lifecycle flag is enabled). This will also reload any configured rule files.

https://prometheus.io/docs/prometheus/latest/configuration/configuration/

so curl to

localhost:9090/-/reload

... should be enough to reload the rules rather than going for a full restart.
Could this be considered please?

Revision history for this message
Stuart Bishop (stub) wrote :

Issuing a SIGHUP to reload is certainly worth considering, but should be implemented in a separate branch to avoid bogging this fix down.

I don't know if there is a reason it was done with restart initially, or if this is a simple case of changing the set_state('prometheus.do-restart') to set_state('prometheus.do-reload'). Other handlers already do set_state('prometheus.do-reload').

Revision history for this message
Joel Sing (jsing) wrote :

LGTM

review: Approve (+1)
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote :

Change successfully merged at revision f6d88291edba42d80dd6324a1c87192a3076614d

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches