Merge ~xavpaice/charm-grafana:bug/1873105 into charm-grafana:master

[Xav Paice]

Replace the process monitoring with functional

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

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

[Peter Sabaini (peter-sabaini)]

Two minor comments inline, otherwise lgtm, cheers

review: Approve

[Alvaro Uria (aluria)]

Hi, there are a few things to be reviewed:
* Makefile does not have a unittest target (probably because unit tests didn't exist). However, tox.ini does have a "unit" env. I think the Makefile misses the target, running "tox -e unit"

* Functional tests miss nagios-{{series}} and nrpe-{{series}} in the overlay.yaml.j2 template. It would be good to test when grafana gets related to nrpe, by checking that the check_grafana_http.cfg file exists in /etc/nagios/nrpe.d. There are examples of such check (file_stat) in ie. charm-openstack-service-checks

* Functional tests don't test a deployment of cs:grafana + juju upgrade-charm to the local version. I spotted a bug that can be seen at [1]. "nrpe-external-master.available" state gets removed and wipe_nrpe_checks is called. Looking at the debug lines, the http interface checks endpoint.website.joined state and removes website.available. I don't see any trace of calling the nrpe-external-master interface and do a similar thing (which OTOH only occurs when n-e-m-relation-{departed,broken} hooks are triggered - and debug lines don't show such run).

On this 3rd test (upgrade-charm) it would be could to verify that the old checks (check_grafana-server.cfg [apt] or check_snap.grafana.grafana.cfg [snap]) are removed (backward compatibility with a previous deployed environment). On fresh deploys, such checks won't exist, only check_grafana_http.cfg.

1. https://pastebin.ubuntu.com/p/2smQt4X3Wd/

review: Needs Fixing

[Alvaro Uria (aluria)]

BTW, when I tried to workaround the issue, by manually setting the nrpe-external-master.available flag, I got this error [1].

1. https://pastebin.ubuntu.com/p/Fj5zzRJt2C/

[Xav Paice (xavpaice)]

Change needs a rebase, and rework to the tests. WIP.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

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

[Joe Guo (guoqiao)]

made 2 trivial comments inline.

[Paul Goins (vultaire)]

Replying to previous comments:

* Re: unit tests: presently "make unittest" calls "tox -e unit"; this may be because of the rebase that it does work now. That being said, the unit tests fail; one of them needs to be repaired.

* Re: functional tests:

  i. NRPE-related tests exist; I'm guessing this is due to the rebase.
  ii. I still don't see any upgrade-charm-related tests. I haven't reviewed the sources about whether the bug in question exists in the current version, but this could be considered a gap.
  iii. Uncertain re: check_grafana-server.cfg and check_snap.grafana.grafana.cfg; are these issues?

At a minimum, I think the unit tests should be fixed. I'm not 100% sure whether the upgrade-charm tests should be addressed now or perhaps after the next charm series release.

review: Needs Fixing

[Xav Paice (xavpaice)]

Unit tests fixed, plus one or to minor lints.

"ii. I still don't see any upgrade-charm-related tests. I haven't reviewed the sources about whether the bug in question exists in the current version, but this could be considered a gap."

Testing for upgrade-charm is way out of scope for this change, we don't run that test for a single one of the LMA charm stack right now - though it would be an excellent addition and make life a heck of a lot easier during the release process. I don't think we can reasonably block this change because of something so far out of scope.

review: Needs Resubmitting

[Paul Goins (vultaire)]

LGTM (and agreed with scope sentiment)

review: Approve

[Xav Paice (xavpaice)]

info: When running lint on this, Black wants to change a number of items, but I've not included that linting in this change since we're going to do a separate commit for lint changes.

[Joe Guo (guoqiao)]

LGTM, +1.

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision 2078b8c32cf6c26b0ad3e4444b69cf553039757c

diff --git a/.gitignore b/.gitignore
index 32e2995..34da750 100644
--- a/.gitignore
+++ b/.gitignore
@@ -20,3 +20,4 @@ repo-info
 
 # reports
 report/*
+.unit-state.db
diff --git a/Makefile b/Makefile
index 60f593c..1334496 100644
--- a/Makefile
+++ b/Makefile
@@ -21,7 +21,7 @@ lint:
 	@echo "Running flake8"
 	@tox -e lint
 
-test: lint
+unittest:
 	@echo "Running unit tests..."
 	@tox -e unit
 
diff --git a/lib/charms/layer/grafana.py b/lib/charms/layer/grafana.py
index 9853dc2..cb7fd2c 100644
--- a/lib/charms/layer/grafana.py
+++ b/lib/charms/layer/grafana.py
@@ -32,9 +32,7 @@ SNAP_CODENAME_MAP = {
 
 def get_admin_password():
     """Get admin password from config, fall back to kv."""
-    return config("admin_password") or unitdata.kv().get(
-        "grafana.admin_password"
-    )
+    return config("admin_password") or unitdata.kv().get("grafana.admin_password")
 
 
 def import_dashboard(dashboard, name=None):
@@ -132,9 +130,7 @@ def check_snap_channel(snap_name, channel):
     UNCHANGED: Channel unchanged
     DOWNGRADE: Would result in downgrade of major version
     """
-    installed_ver = parse_snap_major_version(
-        snap.get_installed_channel(snap_name)
-    )
+    installed_ver = parse_snap_major_version(snap.get_installed_channel(snap_name))
     new_ver = parse_snap_major_version(channel)
     is_refresh_available = snap.is_refresh_available(snap_name)
     if new_ver == -2:
@@ -163,8 +159,7 @@ def check_snap_channel(snap_name, channel):
         return ChangeStatus.UNCHANGED
     if installed_ver < new_ver:
         msg = (
-            "PACKAGE UPGRADE REQUIRES MANUAL INTERVENTION: see {} for "
-            "more info"
+            "PACKAGE UPGRADE REQUIRES MANUAL INTERVENTION: see {} for more info"
         ).format(VERSION_CHANGE_URL)
         status_set("blocked", msg)
         log(msg)
diff --git a/reactive/grafana.py b/reactive/grafana.py
index 17dfad1..4434b17 100644
--- a/reactive/grafana.py
+++ b/reactive/grafana.py
@@ -97,6 +97,7 @@ from charms.reactive import (
     remove_state,
     set_state,
     when,
+    when_any,
     when_not,
 )
 from charms.reactive.helpers import (
@@ -184,8 +185,7 @@ def install_packages():
 
         if config.changed("install_file") and config.get("install_file"):
             hookenv.status_set(
-                "maintenance",
-                "Installing deb pkgs since install_file changed",
+                "maintenance", "Installing deb pkgs since install_file changed",
             )
 
             url = config.get("install_file")
@@ -249,9 +249,7 @@ def install_plugins():
 
     if os.path.exists(plugins_dir):
         hookenv.log("Cleaning up currently installed plugins")
-        hookenv.status_set(
-            "maintenance", "Cleaning up currently installed plugins"
-        )
+        hookenv.status_set("maintenance", "Cleaning up currently installed plugins")
         for entry in os.listdir(plugins_dir):
             entry_path = os.path.join(plugins_dir, entry)
             if os.path.isfile(entry_path):
@@ -283,12 +281,12 @@ def install_plugins():
 @when_not("grafana.change-block")
 def upgrade_charm():
     hookenv.status_set(
-        "maintenance",
-        "Forcing package update and reconfiguration on upgrade-charm",
+        "maintenance", "Forcing package update and reconfiguration on upgrade-charm",
     )
     remove_state("grafana.installed")
     remove_state("grafana.backup.configured")
     remove_state("grafana.configured")
+    remove_state("grafana.nagios-setup.completed")
 
 
 @hook("config-changed")
@@ -314,9 +312,7 @@ def config_changed():
         else:
             # Install from requested channel
             snap.install(
-                SNAP_NAME,
-                channel=config["snap_channel"],
-                force_dangerous=False,
+                SNAP_NAME, channel=config["snap_channel"], force_dangerous=False,
             )
     remove_state("grafana.configured")
     if config.changed("dashboards_backup_schedule") or config.changed(
@@ -327,6 +323,7 @@ def config_changed():
         remove_state("grafana.admin_password.set")
     if config.changed("install_file") and config.get("install_file"):
         remove_state("grafana.installed")
+    remove_state("grafana.nagios-setup.completed")
 
 
 def check_ports(new_port):
@@ -401,15 +398,11 @@ def add_backup_api_keys():
             "SELECT id FROM api_key WHERE org_id=? AND name=?", [org_id, name],
         ):
             hookenv.log(
-                "API key {} in org {} already exists, skipping".format(
-                    name, org_id
-                )
+                "API key {} in org {} already exists, skipping".format(name, org_id)
             )
             continue
         j = {"n": name, "k": passwd, "id": org_id}
-        encoded = base64.b64encode(json.dumps(j).encode("ascii")).decode(
-            "ascii"
-        )
+        encoded = base64.b64encode(json.dumps(j).encode("ascii")).decode("ascii")
         stmt = (
             "INSERT INTO api_key (org_id, name, key, role, created, updated)"
             + ' VALUES (?,?,?,"Viewer",?,?)'
@@ -473,9 +466,7 @@ def setup_backup_shedule():
 
     config = hookenv.config()
     if config.get("dashboards_backup_schedule", False):
-        hookenv.status_set(
-            "maintenance", "Configuring grafana dashboard backup"
-        )
+        hookenv.status_set("maintenance", "Configuring grafana dashboard backup")
         hookenv.log("Setting up dashboards backup job...")
         host.rsync(
             "files/dashboards_backup", "/usr/local/bin/dashboards_backup",
@@ -494,9 +485,7 @@ def setup_backup_shedule():
             group="root",
             perms=0o640,
         )
-        hookenv.status_set(
-            "active", "Completed configuring grafana dashboard backup"
-        )
+        hookenv.status_set("active", "Completed configuring grafana dashboard backup")
     set_state("grafana.backup.configured")
 
 
@@ -518,9 +507,7 @@ def restart_grafana():
         hookenv.status_set("maintenance", msg)
         hookenv.log(msg)
         host.service_start(svcname)
-    elif any_file_changed([grafana_ini]) or is_state(
-        "config.changed.install_plugins"
-    ):
+    elif any_file_changed([grafana_ini]) or is_state("config.changed.install_plugins"):
         msg = "Restarting {}".format(svcname)
         hookenv.log(msg)
         hookenv.status_set("maintenance", msg)
@@ -531,30 +518,33 @@ def restart_grafana():
 
 
 @when("grafana.started")
-@when("nrpe-external-master.available")
+@when_any("nrpe-external-master.available", "nrpe-external-master.connected")
 @when_not("grafana.change-block")
-def update_nrpe_config(svc):
-    kv = unitdata.kv()
-    source = kv.get("install_method")
-    if source in ("snap", "apt"):
-        svcname = SVCNAME[source]
-    else:
-        hookenv.status_set("blocked", "Unsupported install_method")
-        return
+@when_not("grafana.nagios-setup.completed")
+def update_nrpe_config():
     # python-dbus is used by check_upstart_job
     fetch.apt_install("python-dbus", fatal=True)
     hostname = nrpe.get_nagios_hostname()
-    current_unit = nrpe.get_nagios_unit_name()
     nrpe_setup = nrpe.NRPE(hostname=hostname)
-    nrpe.add_init_service_checks(nrpe_setup, [svcname], current_unit)
+    config = hookenv.config()
+
+    # write the http check
+    nrpe_setup.add_check(
+        "grafana_http",
+        "Grafana HTTP check",
+        "check_http -I 127.0.0.1 -p {} -u /login".format(config["port"]),
+    )
     nrpe_setup.write()
+    set_state("grafana.nagios-setup.completed")
 
 
-@when_not("nrpe-external-master.available")
+@when_not("nrpe-external-master.connected")
 def wipe_nrpe_checks():
     checks = [
         "/etc/nagios/nrpe.d/check_grafana-server.cfg",
         "/etc/nagios/nrpe.d/check_grafana.cfg",
+        "/etc/nagios/nrpe.d/check_grafana_http.cfg",
+        "/etc/nagios/nrpe.d/check_snap.grafana.grafana.cfg",
         "/var/lib/nagios/export/service__*_grafana-server.cfg",
         "/var/lib/nagios/export/service__*_grafana.cfg",
     ]
@@ -562,6 +552,7 @@ def wipe_nrpe_checks():
         for f in glob.glob(check):
             if os.path.isfile(f):
                 os.unlink(f)
+    remove_state("grafana.nagios-setup.completed")
 
 
 @when("grafana.started")
@@ -602,8 +593,7 @@ def sources_gone(relation):
         conn = sqlite3.connect("{}/grafana.db".format(data_dir), timeout=30)
         cur = conn.cursor()
         cur.execute(
-            "DELETE FROM DATA_SOURCE WHERE  type=? AND url=?",
-            (ds["type"], ds["url"]),
+            "DELETE FROM DATA_SOURCE WHERE  type=? AND url=?", (ds["type"], ds["url"]),
         )
         conn.commit()
         cur.close()
@@ -668,16 +658,12 @@ def check_datasource(ds):
         return
     conn = sqlite3.connect("{}/grafana.db".format(data_dir), timeout=30)
     cur = conn.cursor()
-    query = cur.execute(
-        "SELECT id, type, name, url, is_default FROM DATA_SOURCE"
-    )
+    query = cur.execute("SELECT id, type, name, url, is_default FROM DATA_SOURCE")
     rows = query.fetchall()
     ds_name = "{} - {}".format(ds["service_name"], ds["description"])
     for row in rows:
         if row[1] == ds["type"] and row[3] == ds["url"]:
-            hookenv.log(
-                "Datasource already exist, updating: {}".format(ds_name)
-            )
+            hookenv.log("Datasource already exist, updating: {}".format(ds_name))
             stmt, values = generate_query(ds, row[4], id=row[0])
             print(stmt, values)
             cur.execute(stmt, values)
@@ -705,16 +691,12 @@ def render_custom(source, context, **parameters):
     template_folder = os.path.join(
         hookenv.charm_dir(), "templates/dashboards/prometheus"
     )
-    environment = Environment(
-        loader=FileSystemLoader(template_folder), **parameters
-    )
+    environment = Environment(loader=FileSystemLoader(template_folder), **parameters)
     try:
         template = environment.get_template(source)
     except exceptions.TemplateNotFound as e:
         hookenv.log(
-            "Could not load template {} from {}".format(
-                source, template_folder
-            )
+            "Could not load template {} from {}".format(source, template_folder)
         )
         raise e
     return template.render(context)
@@ -817,9 +799,7 @@ def check_and_add_dashboard(
     dashboard_changed = diff(new, curr)
     if not dashboard_changed:
         hookenv.log(
-            "Skipping Dashboard Template: already up to date: {}".format(
-                filename
-            )
+            "Skipping Dashboard Template: already up to date: {}".format(filename)
         )
         return
 
@@ -835,15 +815,11 @@ def check_and_add_dashboard(
 
     hookenv.log("Using Dashboard Template: {}".format(filename))
     post_req = "http://127.0.0.1:{}/api/dashboards/db".format(port)
-    r = requests.post(
-        post_req, json=dashboard_json, auth=("admin", gf_adminpasswd)
-    )
+    r = requests.post(post_req, json=dashboard_json, auth=("admin", gf_adminpasswd))
 
     if r.status_code != 200:
         hookenv.log(
-            "Posting template {} failed with error: {}".format(
-                filename, r.text
-            ),
+            "Posting template {} failed with error: {}".format(filename, r.text),
             hookenv.ERROR,
         )
 
@@ -862,9 +838,7 @@ def generate_prometheus_dashboards(gf_adminpasswd, ds):
     config = hookenv.config()
 
     # https://prometheus.io/docs/prometheus/latest/querying/api/#querying-label-values
-    response = requests.get(
-        "{}/api/v1/label/__name__/values".format(ds["url"])
-    )
+    response = requests.get("{}/api/v1/label/__name__/values".format(ds["url"]))
     if response.status_code != 200:
         hookenv.log(
             "Could not reach prometheus API status code: "
@@ -1105,9 +1079,7 @@ def check_adminuser():
                     conn.commit()
                     hookenv.log("[*] admin password updated on database")
                 else:
-                    hookenv.log(
-                        "Could not update user table: hpwgen func failed"
-                    )
+                    hookenv.log("Could not update user table: hpwgen func failed")
                 break
         conn.close()
     except sqlite3.OperationalError as e:
diff --git a/tests/functional/tests/test_grafana.py b/tests/functional/tests/test_grafana.py
index 31d3b4d..06a1942 100644
--- a/tests/functional/tests/test_grafana.py
+++ b/tests/functional/tests/test_grafana.py
@@ -27,9 +27,7 @@ class BaseGrafanaTest(unittest.TestCase):
         cls.lead_unit_name = model.get_lead_unit_name(
             cls.application_name, model_name=cls.model_name
         )
-        cls.units = model.get_units(
-            cls.application_name, model_name=cls.model_name
-        )
+        cls.units = model.get_units(cls.application_name, model_name=cls.model_name)
         cls.grafana_ip = model.get_app_ips(cls.application_name)[0]
 
     def get_unit_status(self, unit):
@@ -84,9 +82,7 @@ class CharmOperationTest(BaseGrafanaTest):
         We'll retry until the TEST_TIMEOUT.
         """
         test_command = "{} -I 127.0.0.1 -p {} -u {}".format(
-            "/usr/lib/nagios/plugins/check_http",
-            DEFAULT_API_PORT,
-            DEFAULT_API_URL,
+            "/usr/lib/nagios/plugins/check_http", DEFAULT_API_PORT, DEFAULT_API_URL,
         )
         timeout = time.time() + TEST_TIMEOUT
         while time.time() < timeout:
@@ -94,9 +90,7 @@ class CharmOperationTest(BaseGrafanaTest):
             if response["Code"] == "0":
                 return
             logging.warning(
-                "Unexpected check_http response: {}. Retrying in 30s.".format(
-                    response
-                )
+                "Unexpected check_http response: {}. Retrying in 30s.".format(response)
             )
             time.sleep(30)
 
@@ -104,9 +98,7 @@ class CharmOperationTest(BaseGrafanaTest):
         self.fail(
             "http port didn't respond to the command \n"
             "'{test_command}' as expected.\n"
-            "Result: {result}".format(
-                test_command=test_command, result=response
-            )
+            "Result: {result}".format(test_command=test_command, result=response)
         )
 
     def test_02_nrpe_http_check(self):
@@ -114,21 +106,11 @@ class CharmOperationTest(BaseGrafanaTest):
         logging.debug(
             "Verify the nrpe check is created and has the required content..."
         )
-        # Check if this is snap or apt
-        cmd = "config-get install_method"
-        install_method = model.run_on_unit(self.lead_unit_name, cmd)
-        if "apt" in install_method["Stdout"]:
-            expected_nrpe_check = (
-                "command[check_grafana-server]=/usr/local/lib/nagios/"
-                "plugins/check_systemd.py grafana-server"
-            )
-            cmd = "cat /etc/nagios/nrpe.d/check_grafana-server.cfg"
-        else:  # must be snap
-            expected_nrpe_check = (
-                "command[check_snap.grafana.grafana]=/usr/local/lib/"
-                "nagios/plugins/check_systemd.py snap.grafana.grafana"
-            )
-            cmd = "cat /etc/nagios/nrpe.d/check_snap.grafana.grafana.cfg"
+        expected_nrpe_check = (
+            "command[check_grafana_http]=/usr/lib/nagios/plugins/check_http "
+            "-I 127.0.0.1 -p 3000 -u /login"
+        )
+        cmd = "cat /etc/nagios/nrpe.d/check_grafana_http.cfg"
         result = model.run_on_unit(self.lead_unit_name, cmd)
         code = result.get("Code")
         if code != "0":
@@ -232,9 +214,7 @@ class SnappedGrafanaTest(BaseGrafanaTest):
         model.block_until_unit_wl_status(self.lead_unit_name, "blocked")
         model.block_until_all_units_idle()
         status_message = self.get_unit_status(self.lead_unit_name)
-        self.assertTrue(
-            "PACKAGE DOWNGRADES ARE NOT SUPPORTED" in status_message
-        )
+        self.assertTrue("PACKAGE DOWNGRADES ARE NOT SUPPORTED" in status_message)
         model.set_application_config(  # unblock unit again
             self.application_name, {"snap_channel": "7/stable"}
         )
diff --git a/tests/unit/requirements.txt b/tests/unit/requirements.txt
index 29f9318..cb975bd 100644
--- a/tests/unit/requirements.txt
+++ b/tests/unit/requirements.txt
@@ -4,3 +4,6 @@ mock
 pytest
 pytest-cov
 requests
+jsondiff
+pbkdf2
+pycrypto
diff --git a/tests/unit/test_grafana.py b/tests/unit/test_grafana.py
index 57e730d..0c8a658 100644
--- a/tests/unit/test_grafana.py
+++ b/tests/unit/test_grafana.py
@@ -1,18 +1,18 @@
-from os.path import isfile
-from unittest import mock
 import sys
 import unittest
-
+from os.path import isfile
+from unittest import mock
+from unittest.mock import call
 
 sys.modules["charms.layer.snap"] = mock.Mock()
 
-
 from charms.layer.grafana import (  # noqa E402
     download_file,
     get_cmd_output,
     get_deb_package_version,
     get_installed_package_version,
 )
+from reactive import grafana  # noqa E402
 
 
 class GrafanaTestCase(unittest.TestCase):
@@ -45,3 +45,26 @@ class GrafanaTestCase(unittest.TestCase):
         self.assertTrue(isfile(path))
         version = get_deb_package_version(path)
         self.assertEqual(version, "2.10-2")
+
+    @mock.patch("charmhelpers.fetch.apt_install")
+    @mock.patch("charmhelpers.contrib.charmsupport.nrpe.NRPE")
+    @mock.patch("charmhelpers.contrib.charmsupport.nrpe.get_nagios_hostname")
+    @mock.patch("charmhelpers.core.hookenv.config")
+    def test_update_nrpe_config(
+        self, mock_hookenv_config, mock_get_nagios_hostname, mock_nrpe, *args
+    ):
+        sys.modules["charms.layer"] = mock.MagicMock()
+        sys.modules["charms.layer.grafana"] = mock.MagicMock()
+        mock_hookenv_config.return_value = {"port": 1234}
+        mock_get_nagios_hostname.return_value = "testunit"
+        grafana.update_nrpe_config()
+        calls = [
+            call(hostname="testunit"),
+            call().add_check(
+                "grafana_http",
+                "Grafana HTTP check",
+                "check_http -I 127.0.0.1 -p 1234 -u /login",
+            ),
+            call().write(),
+        ]
+        mock_nrpe.assert_has_calls(calls)
diff --git a/tox.ini b/tox.ini
index 486c489..c62ec88 100644
--- a/tox.ini
+++ b/tox.ini
@@ -6,7 +6,7 @@ skip_missing_interpreters = True
 [testenv]
 basepython = python3
 setenv =
-  PYTHONPATH = .
+  PYTHONPATH = .:lib
 passenv =
   HOME
   CHARM_BUILD_DIR
@@ -24,7 +24,6 @@ commands = pytest -v --ignore {toxinidir}/tests/functional \
 	   --cov-report=html:report/html
 deps = -r{toxinidir}/tests/unit/requirements.txt
        -r{toxinidir}/requirements.txt
-setenv = PYTHONPATH={toxinidir}/lib
 
 [testenv:func]
 changedir = {toxinidir}/tests/functional
@@ -39,7 +38,7 @@ deps = -r{toxinidir}/tests/functional/requirements.txt
 
 [testenv:lint]
 commands =
-    black --check --line-length 79 reactive lib tests
+    black --check --line-length 88 reactive lib tests
     flake8
 deps =
     black
@@ -75,5 +74,5 @@ exclude =
 # I100: Import statements are in the wrong order
 
 ignore = H405,D100,D101,D102,D103,D104,D105,D107,D200,D202,D203,D204,D205,D208,D400,D401,I100,I201,W503,W504
-max-line-length = 79
+max-line-length = 88
 max-complexity = 10