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

Proposed by Andrea Ieri
Status: Merged
Merged at revision: 233cd954f1327a47c5dca132c10a1a55c299c44f
Proposed branch: ~aieri/charm-nagios:bug/1864192-rebase
Merge into: ~nagios-charmers/charm-nagios:master
Prerequisite: ~aieri/charm-nagios:bug/1864192-charmhelpers-sync
Diff against target: 170 lines (+74/-58)
2 files modified
hooks/common.py (+0/-20)
hooks/monitors-relation-changed (+74/-38)
Reviewer Review Type Date Requested Status
Alvaro Uria Approve
Adam Dyess (community) Approve
Chris Sanders Pending
Peter Sabaini Pending
Review via email: mp+387312@code.launchpad.net

This proposal supersedes a proposal from 2020-07-13.

To post a comment you must log in.
Revision history for this message
Peter Sabaini (peter-sabaini) wrote : Posted in a previous version of this proposal

LGTM, some nits but nothing blocking imo

review: Approve
Revision history for this message
Andrea Ieri (aieri) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Adam Dyess (addyess) wrote : Posted in a previous version of this proposal

Great. No issues

review: Approve
Revision history for this message
Chris Sanders (chris.sanders) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Andrea Ieri (aieri) wrote :

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

Revision history for this message
Adam Dyess (addyess) wrote :

Yeah, this is much cleaner to read. Thanks Andrea

review: Approve
Revision history for this message
Alvaro Uria (aluria) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/hooks/common.py b/hooks/common.py
2index d569e33..a0b61e7 100644
3--- a/hooks/common.py
4+++ b/hooks/common.py
5@@ -42,11 +42,6 @@ def check_ip(n):
6 except socket.error:
7 return False
8
9-def ingress_address(relation_data):
10- if 'ingress-address' in relation_data:
11- return relation_data['ingress-address']
12- return relation_data['private-address']
13-
14
15 def get_local_ingress_address(binding='website'):
16 # using network-get to retrieve the address details if available.
17@@ -347,21 +342,6 @@ def apply_host_policy(target_id, owner_unit, owner_relation):
18 ssh_service.save()
19
20
21-def get_valid_relations():
22- for x in subprocess.Popen(['relation-ids', 'monitors'],
23- stdout=subprocess.PIPE).stdout:
24- yield x.strip()
25- for x in subprocess.Popen(['relation-ids', 'nagios'],
26- stdout=subprocess.PIPE).stdout:
27- yield x.strip()
28-
29-
30-def get_valid_units(relation_id):
31- for x in subprocess.Popen(['relation-list', '-r', relation_id],
32- stdout=subprocess.PIPE).stdout:
33- yield x.strip()
34-
35-
36 def _replace_in_config(find_me, replacement):
37 with open(INPROGRESS_CFG) as cf:
38 with tempfile.NamedTemporaryFile(dir=INPROGRESS_DIR, delete=False) as new_cf:
39diff --git a/hooks/monitors-relation-changed b/hooks/monitors-relation-changed
40index c48cdbb..7bb0082 100755
41--- a/hooks/monitors-relation-changed
42+++ b/hooks/monitors-relation-changed
43@@ -18,17 +18,81 @@
44
45 import sys
46 import os
47-import subprocess
48 import yaml
49-import json
50 import re
51-
52-
53-from common import (customize_service, get_pynag_host,
54- get_pynag_service, refresh_hostgroups,
55- get_valid_relations, get_valid_units,
56- initialize_inprogress_config, flush_inprogress_config,
57- ingress_address)
58+from collections import defaultdict
59+
60+from charmhelpers.core.hookenv import (
61+ relation_get,
62+ ingress_address,
63+ related_units,
64+ relation_ids,
65+ log,
66+ DEBUG
67+)
68+
69+from common import (
70+ customize_service,
71+ get_pynag_host,
72+ get_pynag_service,
73+ refresh_hostgroups,
74+ initialize_inprogress_config,
75+ flush_inprogress_config
76+)
77+
78+
79+REQUIRED_REL_DATA_KEYS = [
80+ 'target-address',
81+ 'monitors',
82+ 'target-id',
83+]
84+
85+
86+def _prepare_relation_data(unit, rid):
87+ relation_data = relation_get(unit=unit, rid=rid)
88+
89+ if not relation_data:
90+ msg = (
91+ 'no relation data found for unit {} in relation {} - '
92+ 'skipping'.format(unit, rid)
93+ )
94+ log(msg, level=DEBUG)
95+ return {}
96+
97+ if rid.split(':')[0] == 'nagios':
98+ # Fake it for the more generic 'nagios' relation
99+ relation_data['target-id'] = unit.replace('/', '-')
100+ relation_data['monitors'] = {'monitors': {'remote': {}}}
101+
102+ if not relation_data.get('target-address'):
103+ relation_data['target-address'] = ingress_address(unit=unit, rid=rid)
104+
105+ for key in REQUIRED_REL_DATA_KEYS:
106+ if not relation_data.get(key):
107+ # Note: it seems that some applications don't provide monitors over
108+ # the relation at first (e.g. gnocchi). After a few hook runs,
109+ # though, they add the key. For this reason I think using a logging
110+ # level higher than DEBUG could be misleading
111+ msg = (
112+ '{} not found for unit {} in relation {} - '
113+ 'skipping'.format(key, unit, rid)
114+ )
115+ log(msg, level=DEBUG)
116+ return {}
117+
118+ return relation_data
119+
120+
121+def _collect_relation_data():
122+ all_relations = defaultdict(dict)
123+ for relname in ['nagios', 'monitors']:
124+ for relid in relation_ids(relname):
125+ for unit in related_units(relid):
126+ relation_data = _prepare_relation_data(unit=unit, rid=relid)
127+ if relation_data:
128+ all_relations[relid][unit] = relation_data
129+
130+ return all_relations
131
132
133 def main(argv):
134@@ -43,35 +107,7 @@ def main(argv):
135 relation_settings['target-address'] = argv[3]
136 all_relations = {'monitors:99': {'testing/0': relation_settings}}
137 else:
138- all_relations = {}
139- for relid in get_valid_relations():
140- (relname, relnum) = relid.split(':')
141- for unit in get_valid_units(relid):
142- relation_settings = json.loads(
143- subprocess.check_output(['relation-get', '--format=json',
144- '-r', relid,
145- '-',unit]).strip())
146-
147- if relation_settings is None or relation_settings == '':
148- continue
149-
150- if relname == 'monitors':
151- if ('monitors' not in relation_settings
152- or 'target-id' not in relation_settings):
153- continue
154- if ('target-id' in relation_settings and 'target-address' not in relation_settings):
155- relation_settings['target-address'] = ingress_address(relation_settings)
156-
157- else:
158- # Fake it for the more generic 'nagios' relation'
159- relation_settings['target-id'] = unit.replace('/','-')
160- relation_settings['target-address'] = ingress_address(relation_settings)
161- relation_settings['monitors'] = {'monitors': {'remote': {} } }
162-
163- if relid not in all_relations:
164- all_relations[relid] = {}
165-
166- all_relations[relid][unit] = relation_settings
167+ all_relations = _collect_relation_data()
168
169 # Hack to work around http://pad.lv/1025478
170 targets_with_addresses = set()

Subscribers

People subscribed via source and target branches