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

[Andrea Ieri]

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.

[Jose Guedez (jfguedez)]

LGTM +1

review: Approve

[Alvaro Uria (aluria)]

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

[Drew Freiberger (afreiberger)]

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

review: Needs Fixing

[Drew Freiberger (afreiberger)]

LGTM!

review: Approve

[Alvaro Uria (aluria)]

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

[Jeremy Lounder (jldev)]

review: Approve

diff --git a/src/reactive/prometheus.py b/src/reactive/prometheus.py
index b176191..15bcd28 100644
--- a/src/reactive/prometheus.py
+++ b/src/reactive/prometheus.py
@@ -1,30 +1,25 @@
 """Charm Prometheus hooks."""
 import glob
 import grp
+from hashlib import sha1
 import os
+from pathlib import Path
 import pwd
 import re
 import shutil
 import socket
 import subprocess
 import time
-from hashlib import sha1
-from pathlib import Path
 from urllib.parse import urlparse
 
 from charmhelpers import fetch
 from charmhelpers.contrib.charmsupport import nrpe
 from charmhelpers.core import hookenv, host, unitdata
 from charmhelpers.core.templating import render
-
 from charms.layer import snap
 from charms.reactive import hook, is_state, remove_state, set_state, when, when_not
 from charms.reactive.helpers import any_file_changed, data_changed
-
 from jinja2 import Template
-
-import requests
-
 import yaml
 
 
@@ -416,7 +411,9 @@ def write_prometheus_config_yml():
 
     if config.changed("prometheus_registration_listen"):
         write_rendered_template(
-            source="promreg.yaml.j2", target=PATHS["promreg_yml"], context=options,
+            source="promreg.yaml.j2",
+            target=PATHS["promreg_yml"],
+            context=options,
         )
         hookenv.open_port(config["prometheus_registration_port"])
         set_state("promreg.do-restart")
@@ -575,10 +572,9 @@ def restart_prometheus():
 def reload_prometheus():
     """Reload prometheus configuration."""
     if host.service_running(PATHS["service_name"]):
-        # Use /-/reload endpoint to reload config gracefully
-        port = unitdata.kv().get("prometheus.port", 9090)
-        url = "http://127.0.0.1:{}/-/reload".format(port)
-        requests.get(url)
+        subprocess.call(
+            "pkill -HUP -f '/snap/prometheus/.*/bin/prometheus '", shell=True
+        )
         remove_state("prometheus.do-reload")
         hookenv.status_set("active", "Ready")
     else:
diff --git a/src/tests/functional/tests/tests_prometheus.py b/src/tests/functional/tests/tests_prometheus.py
index 306b760..9794ce0 100644
--- a/src/tests/functional/tests/tests_prometheus.py
+++ b/src/tests/functional/tests/tests_prometheus.py
@@ -1,9 +1,10 @@
 #!/usr/bin/env python3
 """Zaza functional tests for prometheus."""
+import json
 import unittest
 
+import requests
 import yaml
-
 from zaza import model
 from zaza.charm_lifecycle import utils as lifecycle_utils
 
@@ -68,3 +69,24 @@ class BasicPrometheusCharmTest(PrometheusCharmTestBase):
         label_addr = result["Stdout"].strip()
         telegraf_addr = model.get_lead_unit_ip("telegraf")
         self.assertEqual(label_addr, telegraf_addr)
+
+    def test_service_reload_after_change(self):
+        """Test service reload."""
+        model.add_unit("ubuntu", wait_appear=True)
+        model.block_until_all_units_idle(timeout=600)
+
+        action = model.run_on_unit("ubuntu/1", "hostname -f")
+        new_target = action["Stdout"].strip()
+
+        prom_ip = model.get_lead_unit_ip("prometheus2")
+        prom_port = 9090
+        url = "http://{}:{}/api/v1/targets".format(prom_ip, prom_port)
+        response = requests.get(url)
+        content = json.loads(response.content)
+
+        target_found = False
+        for targets in content["data"]["activeTargets"]:
+            if targets["labels"].get("dns_name", None) == new_target:
+                target_found = True
+
+        self.assertTrue(target_found)
diff --git a/src/tests/unit/test_reactive_prometheus.py b/src/tests/unit/test_reactive_prometheus.py
index 30e1ca9..84e3144 100644
--- a/src/tests/unit/test_reactive_prometheus.py
+++ b/src/tests/unit/test_reactive_prometheus.py
@@ -7,11 +7,8 @@ import unittest
 
 from charmhelpers.core import hookenv, host, unitdata
 from charmhelpers.core.templating import render
-
 from charms.reactive import bus
-
 import mock
-
 import yaml