Prometheus2 charm

Merge ~aieri/charm-prometheus2:lp1891942-properly-reload-prom-process-via-sighup into charm-prometheus2:master

  1. Git
  2. lp:~aieri/charm-prometheus2
  3. lp1891942-properly-reload-prom-process-via-sighup
  4. Merge into master
Proposed by Andrea Ieri
Status: Merged
Approved by: Jeremy Lounder
Approved revision: ed9cebd991e402d4d4fd1199ea1bdc872f819b0d
Merged at revision: ed9cebd991e402d4d4fd1199ea1bdc872f819b0d
Proposed branch: ~aieri/charm-prometheus2:lp1891942-properly-reload-prom-process-via-sighup
Merge into: charm-prometheus2:master
Diff against target: 117 lines (+31/-16)
3 files modified
src/reactive/prometheus.py (+8/-12)
src/tests/functional/tests/tests_prometheus.py (+23/-1)
src/tests/unit/test_reactive_prometheus.py (+0/-3)
Reviewer Review Type Date Requested Status
Jeremy Lounder (community) Approve
Drew Freiberger Pending
Jose Guedez Pending
Alvaro Uria Pending
Review via email: mp+390044@code.staging.launchpad.net

This proposal supersedes a proposal from 2020-08-24.

Commit message

Properly reload all Prometheus processes. Fixes LP#1891942.

This solution is very similar to that which was *removed* in 30e80. The removed solution did not identify and reload all Prometheus processes. As per the official Prometheus documentation SIGHUP can be used to reload Prometheus: https://prometheus.io/docs/prometheus/latest/configuration/configuration/

An alternative solution was considered where Prometheus would be reloaded using the HTTP API. This solution was not implemented because it had additional security implications that needed to be considered.

To post a comment you must log in.
Revision history for this message
Jose Guedez (jfguedez) wrote : Posted in a previous version of this proposal

LGTM +1

review: Approve
Revision history for this message
Joel Sing (jsing) : Posted in a previous version of this proposal
Revision history for this message
Alvaro Uria (aluria) wrote : Posted in a previous version of this proposal

By security implications, I understand you mean that "--web.enable-lifecycle" arg is not safe because it would allow anybody to reload and shutdown the Prometheus instance. I see, the reload_prometheus function fix in this MP uses SIGHUP instead of the HTTP API endpoint, which looks good (please review comment from Joel to simplify the SIGHUP oneliner).

I also see that the daemon-args Juju config option [2] is empty by default. This means that the previous approach of using /-/reload could not work by default. Should we notify via status_set when --web.enable-lifecycle is used? I think banning its use would be too agressive, but some users may have set it up and with this fix, the daemon arg option would not be needed anymore.

I'll mark the MP as Needs fixing to use pkill to reload Prometheus. Once fixed, I will run all tests before approval.

Thank you,
-Alvaro.

1. https://prometheus.io/docs/operating/security/
2. https://git.launchpad.net/~exsdev/charm-prometheus2/tree/src/config.yaml?h=lp1891942-properly-reload-prom-process-via-sighup#n2

review: Needs Fixing
Revision history for this message
Drew Freiberger (afreiberger) wrote : Posted in a previous version of this proposal

tested joel's recommendation +1 to update to pkill -HUP -f ...

review: Needs Fixing
Revision history for this message
Drew Freiberger (afreiberger) wrote : Posted in a previous version of this proposal

LGTM!

review: Approve
Revision history for this message
Alvaro Uria (aluria) wrote : Posted in a previous version of this proposal

lint run-test: commands[1] | black --check --exclude '/(\.eggs|\.git|\.tox|\.venv|\.build|dist|charmhelpers|mod)/' .
would reformat /home/circleci/project/charm-prometheus2/src/reactive/prometheus.py
Oh no! 💥 💔 💥
1 file would be reformatted, 3 files would be left unchanged.

review: Needs Fixing
Revision history for this message
Jeremy Lounder (jldev) :
review: Approve

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

to all changes: