UA ReviewKit

Merge ~hopem/ua-reviewkit:juju-bundle-checker-minor-fixes into ua-reviewkit:master

  1. Git
  2. lp:~hopem/ua-reviewkit
  3. juju-bundle-checker-minor-fixes
  4. Merge into master
Proposed by Edward Hope-Morley
Status: Merged
Merged at revision: 018f84c82d64a00fdfb35b43fd96592ccff9acf6
Proposed branch: ~hopem/ua-reviewkit:juju-bundle-checker-minor-fixes
Merge into: ua-reviewkit:master
Diff against target: 388 lines (+102/-62)
2 files modified
juju/checks/openstack.yaml (+38/-24)
juju/ua-bundle-check.py (+64/-38)
Reviewer Review Type Date Requested Status
Jorge Niedbalski (community) Approve
UA Reviewers Pending
Review via email: mp+383419@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Jorge Niedbalski (niedbalski) wrote :

It works for the case of multiple applications matching the same charm and allow_default works better than the if-exists semantics. Minor fix required.

review: Needs Fixing
Revision history for this message
Jorge Niedbalski (niedbalski) wrote :

LGTM

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/juju/checks/openstack.yaml b/juju/checks/openstack.yaml
2index 35f4a4e..2893d86 100644
3--- a/juju/checks/openstack.yaml
4+++ b/juju/checks/openstack.yaml
5@@ -1,7 +1,7 @@
6 # See README_check_definitions.md
7 checks:
8 mysql-percona:
9- charm: "cs:.*percona-cluster[-]?[0-9]*"
10+ charm: "cs:.*percona-cluster[-]?[0-9]*$"
11 assertions:
12 ha:
13 assert_ha:
14@@ -17,7 +17,7 @@ checks:
15 source: local
16 value: 2000
17 mysql-innodb-cluster:
18- charm: "cs:.*mysql-innodb-cluster[-]?[0-9]*"
19+ charm: "cs:.*mysql-innodb-cluster[-]?[0-9]*$"
20 assertions:
21 ha:
22 assert_ha:
23@@ -33,102 +33,105 @@ checks:
24 source: local
25 value: 2000
26 rabbitmq-server:
27- charm: "cs:.*rabbitmq-server[-]?[0-9]*"
28+ charm: "cs:.*rabbitmq-server[-]?[0-9]*$"
29 assertions:
30 ha:
31 assert_ha:
32 type: application
33 cluster-partition-handling:
34+ allow_default:
35+ type: config
36+ source: local
37 eq:
38 type: config
39 source: local
40 value: "ignore"
41 etcd:
42- charm: "cs:.*etcd[-]?[0-9]*"
43+ charm: "cs:.*etcd[-]?[0-9]*$"
44 assertions:
45 ha:
46 assert_ha:
47 type: application
48 barbican:
49- charm: "cs:.*barbican[-]?[0-9]*"
50+ charm: "cs:.*barbican[-]?[0-9]*$"
51 assertions:
52 ha:
53 assert_ha:
54 type: application
55 ceph-mon:
56- charm: "cs:.*ceph-mon[-]?[0-9]*"
57+ charm: "cs:.*ceph-mon[-]?[0-9]*$"
58 assertions:
59 ha:
60 assert_ha:
61 type: application
62 ceph-osd:
63- charm: "cs:.*ceph-mon[-]?[0-9]*"
64+ charm: "cs:.*ceph-mon[-]?[0-9]*$"
65 assertions:
66 ceph-proxy:
67- charm: "cs:.*ceph-proxy[-]?[0-9]*"
68+ charm: "cs:.*ceph-proxy[-]?[0-9]*$"
69 assertions:
70 ceph-radosgw:
71- charm: "cs:.*ceph-radosgw[-]?[0-9]*"
72+ charm: "cs:.*ceph-radosgw[-]?[0-9]*$"
73 assertions:
74 cinder:
75- charm: "cs:.*cinder[-]?[0-9]*"
76+ charm: "cs:.*cinder[-]?[0-9]*$"
77 assertions:
78 ha:
79 assert_ha:
80 type: application
81 designate:
82- charm: "cs:.*designate[-]?[0-9]*"
83+ charm: "cs:.*designate[-]?[0-9]*$"
84 assertions:
85 ha:
86 assert_ha:
87 type: application
88 glance:
89- charm: "cs:.*glance[-]?[0-9]*"
90+ charm: "cs:.*glance[-]?[0-9]*$"
91 assertions:
92 ha:
93 assert_ha:
94 type: application
95 gnocchi:
96- charm: "cs:.*gnocchi[-]?[0-9]*"
97+ charm: "cs:.*gnocchi[-]?[0-9]*$"
98 assertions:
99 ha:
100 assert_ha:
101 type: application
102 heat:
103- charm: "cs:.*heat[-]?[0-9]*"
104+ charm: "cs:.*heat[-]?[0-9]*$"
105 assertions:
106 ha:
107 assert_ha:
108 type: application
109 keystone:
110- charm: "cs:.*keystone[-]?[0-9]*"
111+ charm: "cs:.*keystone[-]?[0-9]*$"
112 assertions:
113 ha:
114 assert_ha:
115 type: application
116 manilla:
117- charm: "cs:.*manilla[-]?[0-9]*"
118+ charm: "cs:.*manilla[-]?[0-9]*$"
119 assertions:
120 ha:
121 assert_ha:
122 type: application
123 neutron-api:
124- charm: "cs:.*neutron-api[-]?[0-9]*"
125+ charm: "cs:.*neutron-api[-]?[0-9]*$"
126 assertions:
127 ha:
128 assert_ha:
129 type: application
130 nova-cloud-controller:
131- charm: "cs:.*nova-cloud-controller[-]?[0-9]*"
132+ charm: "cs:.*nova-cloud-controller[-]?[0-9]*$"
133 assertions:
134 ha:
135 assert_ha:
136 type: application
137 nova-compute:
138- charm: "cs:.*nova-compute[-]?[0-9]*"
139+ charm: "cs:.*nova-compute[-]?[0-9]*$"
140 assertions:
141 vcpu-pin-set:
142- if-exists:
143+ allow_default:
144 type: config
145 source: local
146 eq:
147@@ -140,26 +143,37 @@ checks:
148 type: config
149 source: bundle
150 octavia:
151- charm: "cs:.*octavia[-]?[0-9]*"
152+ charm: "cs:.*octavia[-]?[0-9]*$"
153 assertions:
154 ha:
155 assert_ha:
156 type: application
157 placement:
158- charm: "cs:.*placement[-]?[0-9]*"
159+ charm: "cs:.*placement[-]?[0-9]*$"
160 assertions:
161 ha:
162 assert_ha:
163 type: application
164 swift-proxy:
165- charm: "cs:.*swift-proxy[-]?[0-9]*"
166+ charm: "cs:.*swift-proxy[-]?[0-9]*$"
167 assertions:
168 ha:
169 assert_ha:
170 type: application
171 vault:
172- charm: "cs:.*vault[-]?[0-9]*"
173+ charm: "cs:.*vault[-]?[0-9]*$"
174 assertions:
175 ha:
176 assert_ha:
177 type: application
178+ hacluster:
179+ charm: "cs:.*hacluster[-]?[0-9]*$"
180+ assertions:
181+ cluster_count:
182+ allow_default:
183+ type: config
184+ source: local
185+ gte:
186+ type: config
187+ source: local
188+ value: 3
189diff --git a/juju/ua-bundle-check.py b/juju/ua-bundle-check.py
190index 6678596..187b636 100755
191--- a/juju/ua-bundle-check.py
192+++ b/juju/ua-bundle-check.py
193@@ -124,7 +124,8 @@ class UABundleChecker(object):
194
195 def __init__(self, app, bundle_apps, charm_regex, assertions, fce_config,
196 logger):
197- self.app_name = app
198+ self.applications = []
199+ self.app_name = None
200 self.bundle_apps = bundle_apps
201 self.charm_regex = charm_regex
202 self.assertions = assertions
203@@ -137,55 +138,72 @@ class UABundleChecker(object):
204 if not self.results:
205 return
206
207- self.logger.log("=> application '{}'".format(self.app_name))
208- for category in self.results:
209- if ignore_pass and category == "PASS":
210- continue
211+ for app in self.results:
212+ results = self.results[app]
213+ self.logger.log("=> application '{}'".format(app))
214+ for category in results:
215+ if ignore_pass and category == "PASS":
216+ continue
217
218- for result in self.results[category]:
219- self.logger.log(result)
220+ for result in results[category]:
221+ self.logger.log(result)
222
223 def get_results_summary(self):
224 summary = {}
225- for category in self.results:
226- summary[category] = len(self.results[category])
227+ for app in self.results:
228+ results = self.results[app]
229+ for category in results:
230+ summary[category] = len(results[category])
231
232 return summary
233
234 def add_result(self, result):
235- if result.rc_str in self.results:
236- self.results[result.rc_str].append(result)
237+ if self.app_name not in self.results:
238+ self.results[self.app_name] = {}
239+
240+ results = self.results[self.app_name]
241+ if result.rc_str in results:
242+ results[result.rc_str].append(result)
243 else:
244- self.results[result.rc_str] = [result]
245+ results[result.rc_str] = [result]
246
247 def run_assertions(self):
248 if not self.assertions:
249 self.add_result(CheckResult(2, reason="no assertions defined"))
250 return
251
252- for opt in self.assertions:
253- if 'if-exists' in self.assertions[opt]:
254- if not self.exists(opt):
255- continue
256- else:
257- del self.assertions[opt]['if-exists']
258-
259- for method in self.assertions[opt]:
260- self.run(opt, method, self.assertions[opt][method])
261+ if not self.has_charm_matches():
262+ return
263
264- def exists(self, opt):
265- return opt in self.bundle_apps[self.app_name]['options']
266+ for app in self.applications:
267+ self.app_name = app
268+ defaults_key = "allow_default"
269+ for opt in self.assertions:
270+ if defaults_key in self.assertions[opt]:
271+ # if opt is not set in bundle and we have allowed charm default
272+ # then we log a PASS and continue to the next opt in the
273+ # assertions list.
274+ if not self.opt_exists(opt):
275+ reason = "using charm default"
276+ self.add_result(CheckResult(0, opt=opt, reason=reason))
277+ continue
278+ else:
279+ # otherwise we continue with asserting the value set.
280+ del self.assertions[opt][defaults_key]
281+
282+ for method in self.assertions[opt]:
283+ self.run(opt, method, self.assertions[opt][method])
284+
285+ def opt_exists(self, opt):
286+ return opt in self.bundle_apps[self.app_name].get('options', [])
287
288 def run(self, opt, method, assertion):
289
290- if not self.has_bundle_charm_name():
291- return
292-
293 if assertion['type'] == "application":
294 getattr(self, method)()
295 return
296
297- if not self.exists(opt):
298+ if not self.opt_exists(opt):
299 self.add_result(CheckResult(2, opt=opt, reason="not found"))
300 return
301
302@@ -243,17 +261,18 @@ class UABundleChecker(object):
303 return _int * conv[val[-1].lower()]
304
305 def assert_ha(self):
306+ min_units = 3
307 num_units = self.bundle_apps[self.app_name].get('num_units', -1)
308- ret = CheckResult(0, opt="ensure-ha")
309- if num_units < 3:
310- ret.reason = ("not enough units (value={}, expected='>3')".
311- format(num_units))
312+ ret = CheckResult(0, opt="HA (>={})".format(min_units))
313+ if num_units < min_units:
314+ ret.reason = ("not enough units (value={}, expected='>={}')".
315+ format(num_units, min_units))
316 ret.rc = 2
317
318 self.add_result(ret)
319
320 def gte(self, opt, value):
321- current = self.bundle_apps[self.app_name]['options'][opt]
322+ current = self.bundle_apps[self.app_name].get('options', [])[opt]
323 current = self.atoi(current)
324 expected = self.atoi(value)
325 ret = CheckResult(0, opt=opt, reason=("value={}".format(current)))
326@@ -264,7 +283,7 @@ class UABundleChecker(object):
327 self.add_result(ret)
328
329 def eq(self, opt, value):
330- current = self.bundle_apps[self.app_name]['options'][opt]
331+ current = self.bundle_apps[self.app_name].get('options', [])[opt]
332 current = self.atoi(current)
333 expected = self.atoi(value)
334 ret = CheckResult(0, opt=opt, reason=("value={}".format(current)))
335@@ -275,7 +294,7 @@ class UABundleChecker(object):
336 self.add_result(ret)
337
338 def isset(self, opt, value):
339- current = self.bundle_apps[self.app_name]['options'][opt]
340+ current = self.bundle_apps[self.app_name].get('options', [])[opt]
341 ret = CheckResult(0, opt=opt, reason="value={}".format(current))
342 if not current:
343 ret.reason = "no value set"
344@@ -283,14 +302,18 @@ class UABundleChecker(object):
345
346 self.add_result(ret)
347
348- def has_bundle_charm_name(self):
349+ def get_applications(self):
350+ self.applications = []
351 for app in self.bundle_apps:
352 r = re.match(re.compile(self.charm_regex),
353 self.bundle_apps[app].get('charm'))
354 if r:
355 self.charm_name = r[0]
356- self.app_name = app
357- return True
358+ self.applications.append(app)
359+
360+ def has_charm_matches(self):
361+ self.get_applications()
362+ return len(self.applications) > 0
363
364
365 if __name__ == "__main__":
366@@ -316,6 +339,8 @@ if __name__ == "__main__":
367 if args.fce_config:
368 if not args.bundle:
369 bundle = os.path.join(args.fce_config, "bundle.yaml")
370+ elif not os.path.exists(args.bundle):
371+ bundle = os.path.join(args.fce_config, args.bundle)
372 elif bundle and not os.path.exists(args.bundle):
373 raise Exception("ERROR: --bundle must be a path")
374
375@@ -341,11 +366,12 @@ if __name__ == "__main__":
376 charm = check_defs['checks'][label]['charm']
377 assertions = check_defs['checks'][label].get('assertions')
378 if not assertions:
379+ print("INFO: {} has no assertions defined".format(label))
380 continue
381
382 checker = UABundleChecker(label, bundle_apps, charm, assertions,
383 args.fce_config, logger)
384- matches = checker.has_bundle_charm_name()
385+ matches = checker.has_charm_matches()
386 if not matches:
387 logger.log("INFO: no match found for {} - skipping"
388 .format(checker.charm_regex))

Subscribers

People subscribed via source and target branches