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

[Wouter van Bommel]

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

73de6b4... by Wouter van Bommel on 2020-04-16

Minor update in the way tests are run.

With the changes created, we no longer need to specify the PYTHONPATH
in the functional tests

[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.

Unmerged commits

73de6b4... by Wouter van Bommel on 2020-04-16

Minor update in the way tests are run.

With the changes created, we no longer need to specify the PYTHONPATH
in the functional tests

194e953... by Wouter van Bommel on 2020-04-16

Replace the process monitoring with functional

By checking that the grafana server actually responds to http calls, we
can ensure that both the process is running and functional.

Added unittest for the creation of the nagios check, updated unittest to
make it possible

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/reactive/grafana.py b/reactive/grafana.py
index 303b10e..09b7a31 100644
--- a/reactive/grafana.py
+++ b/reactive/grafana.py
@@ -218,6 +218,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):
@@ -371,21 +372,19 @@ def restart_grafana():
 
 @when('grafana.started')
 @when('nrpe-external-master.available')
-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
-    # python-dbus is used by check_upstart_job
-    fetch.apt_install('python-dbus', fatal=True)
+@when_not('grafana.nagios-setup.completed')
+def update_nrpe_config(_):
     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)
+
+    # write the http check
+    nrpe_setup.add_check(
+        'grafana_http',
+        'Grafana HTTP check',
+        'check_http -I 127.0.0.1 -p {} -u /login'.format(hookenv.config()['port'])
+    )
     nrpe_setup.write()
+    set_state('grafana.nagios-setup.completed')
 
 
 @when_not('nrpe-external-master.available')
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 e03d9e1..bed2a4a 100644
--- a/tests/unit/test_grafana.py
+++ b/tests/unit/test_grafana.py
@@ -1,7 +1,9 @@
 from os.path import isfile
 import unittest
+from unittest.mock import call
+from unittest import mock
 
-from charms.layer.grafana import (
+from lib.charms.layer.grafana import (
     download_file,
     get_cmd_output,
     get_deb_package_version,
@@ -37,3 +39,30 @@ class GrafanaTestCase(unittest.TestCase):
         self.assertTrue(isfile(path))
         version = get_deb_package_version(path)
         self.assertEqual(version, '2.10-2')
+
+
+class TestGrafanaNagios(unittest.TestCase):
+    @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):
+        import sys
+        sys.modules['charms.layer'] = mock.MagicMock()
+        sys.modules['charms.layer.grafana'] = mock.MagicMock()
+        from reactive import grafana
+        mock_hookenv_config.return_value = {'port': 1234}
+        mock_get_nagios_hostname.return_value = 'testunit'
+        svc = object()
+        grafana.update_nrpe_config(svc)
+        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 288fd77..898ca41 100644
--- a/tox.ini
+++ b/tox.ini
@@ -18,7 +18,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:functional]
 passenv =