Merge ~aieri/charm-nagios:bug/1864192-rebase into ~nagios-charmers/charm-nagios:master

[Andrea Ieri]

[Peter Sabaini (peter-sabaini)]

LGTM, some nits but nothing blocking imo

review: Approve

[Andrea Ieri (aieri)]

Thanks, the charm helpers sync script actually comes from https://raw.githubusercontent.com/juju/charm-helpers/master/tools/charm_helpers_sync/charm_helpers_sync.py, it's not part of this repo.

[Adam Dyess (addyess)]

Great. No issues

review: Approve

[Chris Sanders (chris.sanders)]

A few comments inline, and while I hate to do this I think the charmhelpers and this change need to be split. While reviewing it I'm not convinced that this merge isn't confusing local vs charmhelpers functions. For example 'ingress_address' is defined in this change and I *believe* is only actually used from charmhelpers.

If I'm just having difficulty understanding and you *do* think the change is dependent on charmhelpers you can make this MR dependent on the charmhelpers MR so the changes specific to this bug can be reviewed seperately.

review: Needs Fixing

[Andrea Ieri (aieri)]

I have addressed the comments, rebased, and split this change into three separate MRs (see prerequisites)

[Adam Dyess (addyess)]

Yeah, this is much cleaner to read. Thanks Andrea

review: Approve

[Alvaro Uria (aluria)]

Change lgtm. However, there is no other way to test it than a manual deployment. If unit tests don't exist, I think we should at least convert amulet tests into Zaza tests.

Manual testing looks good, though. +1.

review: Approve

diff --git a/hooks/common.py b/hooks/common.py
index d569e33..a0b61e7 100644
--- a/hooks/common.py
+++ b/hooks/common.py
@@ -42,11 +42,6 @@ def check_ip(n):
         except socket.error:
             return False
 
-def ingress_address(relation_data):
-    if 'ingress-address' in relation_data:
-        return relation_data['ingress-address']
-    return relation_data['private-address']
-
 
 def get_local_ingress_address(binding='website'):
     # using network-get to retrieve the address details if available.
@@ -347,21 +342,6 @@ def apply_host_policy(target_id, owner_unit, owner_relation):
     ssh_service.save()
 
 
-def get_valid_relations():
-    for x in subprocess.Popen(['relation-ids', 'monitors'],
-                              stdout=subprocess.PIPE).stdout:
-        yield x.strip()
-    for x in subprocess.Popen(['relation-ids', 'nagios'],
-                              stdout=subprocess.PIPE).stdout:
-        yield x.strip()
-
-
-def get_valid_units(relation_id):
-    for x in subprocess.Popen(['relation-list', '-r', relation_id],
-                              stdout=subprocess.PIPE).stdout:
-        yield x.strip()
-
-
 def _replace_in_config(find_me, replacement):
     with open(INPROGRESS_CFG) as cf:
         with tempfile.NamedTemporaryFile(dir=INPROGRESS_DIR, delete=False) as new_cf:
diff --git a/hooks/monitors-relation-changed b/hooks/monitors-relation-changed
index c48cdbb..7bb0082 100755
--- a/hooks/monitors-relation-changed
+++ b/hooks/monitors-relation-changed
@@ -18,17 +18,81 @@
 
 import sys
 import os
-import subprocess
 import yaml
-import json
 import re
-
-
-from common import (customize_service, get_pynag_host,
-        get_pynag_service, refresh_hostgroups,
-        get_valid_relations, get_valid_units,
-        initialize_inprogress_config, flush_inprogress_config,
-        ingress_address)
+from collections import defaultdict
+
+from charmhelpers.core.hookenv import (
+    relation_get,
+    ingress_address,
+    related_units,
+    relation_ids,
+    log,
+    DEBUG
+)
+
+from common import (
+    customize_service,
+    get_pynag_host,
+    get_pynag_service,
+    refresh_hostgroups,
+    initialize_inprogress_config,
+    flush_inprogress_config
+)
+
+
+REQUIRED_REL_DATA_KEYS = [
+    'target-address',
+    'monitors',
+    'target-id',
+]
+
+
+def _prepare_relation_data(unit, rid):
+    relation_data = relation_get(unit=unit, rid=rid)
+
+    if not relation_data:
+        msg = (
+            'no relation data found for unit {} in relation {} - '
+            'skipping'.format(unit, rid)
+        )
+        log(msg, level=DEBUG)
+        return {}
+
+    if rid.split(':')[0] == 'nagios':
+        # Fake it for the more generic 'nagios' relation
+        relation_data['target-id'] = unit.replace('/', '-')
+        relation_data['monitors'] = {'monitors': {'remote': {}}}
+
+    if not relation_data.get('target-address'):
+        relation_data['target-address'] = ingress_address(unit=unit, rid=rid)
+
+    for key in REQUIRED_REL_DATA_KEYS:
+        if not relation_data.get(key):
+            # Note: it seems that some applications don't provide monitors over
+            # the relation at first (e.g. gnocchi). After a few hook runs,
+            # though, they add the key. For this reason I think using a logging
+            # level higher than DEBUG could be misleading
+            msg = (
+                '{} not found for unit {} in relation {} - '
+                'skipping'.format(key, unit, rid)
+            )
+            log(msg, level=DEBUG)
+            return {}
+
+    return relation_data
+
+
+def _collect_relation_data():
+    all_relations = defaultdict(dict)
+    for relname in ['nagios', 'monitors']:
+        for relid in relation_ids(relname):
+            for unit in related_units(relid):
+                relation_data = _prepare_relation_data(unit=unit, rid=relid)
+                if relation_data:
+                    all_relations[relid][unit] = relation_data
+
+    return all_relations
 
 
 def main(argv):
@@ -43,35 +107,7 @@ def main(argv):
             relation_settings['target-address'] = argv[3]
         all_relations = {'monitors:99': {'testing/0': relation_settings}}
     else:
-        all_relations = {}
-        for relid in get_valid_relations():
-            (relname, relnum) = relid.split(':')
-            for unit in get_valid_units(relid):
-                relation_settings = json.loads(
-                        subprocess.check_output(['relation-get', '--format=json',
-                            '-r', relid,
-                            '-',unit]).strip())
-
-                if relation_settings is None or relation_settings == '':
-                    continue
-
-                if relname == 'monitors':
-                    if ('monitors' not in relation_settings
-                            or 'target-id' not in relation_settings):
-                        continue
-                    if ('target-id' in relation_settings and 'target-address' not in relation_settings):
-                            relation_settings['target-address'] = ingress_address(relation_settings)
-
-                else:
-                    # Fake it for the more generic 'nagios' relation'
-                    relation_settings['target-id'] = unit.replace('/','-')
-                    relation_settings['target-address'] = ingress_address(relation_settings)
-                    relation_settings['monitors'] = {'monitors': {'remote': {} } }
-
-                if relid not in all_relations:
-                    all_relations[relid] = {}
-
-                all_relations[relid][unit] = relation_settings
+        all_relations = _collect_relation_data()
 
     # Hack to work around http://pad.lv/1025478
     targets_with_addresses = set()