Merge ~paulgear/ntp-charm/+git/ntp-charm:autopeers into ntp-charm:master

Proposed by Paul Gear on 2017-10-16
Status: Merged
Merged at revision: 9825def33421f3177ee772e80f8f68dd1367d7f8
Proposed branch: ~paulgear/ntp-charm/+git/ntp-charm:autopeers
Merge into: ntp-charm:master
Diff against target: 1041 lines (+794/-62)
12 files modified
.gitignore (+1/-0)
Makefile (+18/-3)
README.md (+4/-5)
config.yaml (+8/-3)
files/nagios/check_ntpmon.py (+1/-1)
hooks/ntp_hooks.py (+131/-49)
hooks/ntp_scoring.py (+159/-0)
hooks/ntp_source_score.py (+198/-0)
tests/10-deploy-test.py (+1/-1)
unit_tests/test_ntp_hooks.py (+0/-0)
unit_tests/test_ntp_scoring.py (+106/-0)
unit_tests/test_ntp_source_score.py (+167/-0)
Reviewer Review Type Date Requested Status
Stuart Bishop 2017-10-16 Approve on 2017-11-07
Review via email: mp+332326@code.launchpad.net

Description of the Change

This MP introduces one major piece of functionality and some other minor improvements.

The previously-deprecated auto_peers option now enables new logic which tests connectivity to upstream NTP servers, then uses a simple scoring mechanism to select the most suitable servers to act as a service stratum between the upstream NTP servers and the rest of the juju environment.

This is primarily intended for use in medium-to-large OpenStack environments where Ceph needs a stable, nearby set of NTP sources. At present in a number of our production OpenStack environments this is achieved by creating two separate ntp services; the new auto_peers functionality achieves this without requiring manual setup. It's my expectation that this will reduce manual tuning in some of our less-well-connected OpenStacks, and result in fewer alerts.

In some environments, NTP is erroneously deployed to containers, conflicting with NTP on the host; this version of the charm automatically detects when it is running in a container and disables NTP.

Both of the above are reflected in juju status, to make it easy for the operator to see the results of the charm's automated decisions and correct if necessary.

To post a comment you must log in.
Paul Gear (paulgear) wrote :

I will rebase before merge, but the contents shouldn't change from this.

Tom Haddon (mthaddon) wrote :

Can you add a description of this change and why it's needed? Also some comments inline.

Paul Gear (paulgear) wrote :

Replies to comments inline.

Tom Haddon (mthaddon) wrote :

I get a "make lint" error from this:

hooks/ntp_hooks.py:92:1: C901 'write_config' is too complex (10)

You may want to consider ignoring that in the default lint target and adding a "complex-lint" target that doesn't ignore it so you can work it down to zero over time.

How does some use ntp_source_score.py? You mention in the comments for that file that it can be for diagnostic purposes. That sounds like something that would be nice to expose via juju actions. I don't think that should necessarily block the landing of this, but if you're planning to advertise that functionality at all, I think it should be done via juju actions first.

Tom Haddon (mthaddon) wrote :

Also, just noticed there's a unit_tests directory. Would be good to have a Makefile target for running tests.

Paul Gear (paulgear) wrote :

> I get a "make lint" error from this:
>
> hooks/ntp_hooks.py:92:1: C901 'write_config' is too complex (10)
>
> You may want to consider ignoring that in the default lint target and adding a
> "complex-lint" target that doesn't ignore it so you can work it down to zero
> over time.

Which version of flake8 are you running? On my system (flake8 3.2.1-1), I have to drop max-complexity from 10 to 8 to get a failure in that method.

> How does some use ntp_source_score.py? You mention in the comments for that
> file that it can be for diagnostic purposes. That sounds like something that
> would be nice to expose via juju actions. I don't think that should
> necessarily block the landing of this, but if you're planning to advertise
> that functionality at all, I think it should be done via juju actions first.

That would probably be important to have for receiving troubleshooting reports from others, but I wasn't planning to advertise it in the first instance. I'll try to get some time to add one in the next couple of weeks.

Tom Haddon (mthaddon) wrote :

> > I get a "make lint" error from this:
> >
> > hooks/ntp_hooks.py:92:1: C901 'write_config' is too complex (10)
> >
> > You may want to consider ignoring that in the default lint target and adding
> a
> > "complex-lint" target that doesn't ignore it so you can work it down to zero
> > over time.
>
> Which version of flake8 are you running? On my system (flake8 3.2.1-1), I
> have to drop max-complexity from 10 to 8 to get a failure in that method.

2.5.4.

$ dpkg -l | grep flake8
ii flake8 2.5.4-2 all code checker using pep8 and pyflakes
ii python-flake8 2.5.4-2 all code checker using pep8 and pyflakes - Python 2.x
ii python3-flake8 2.5.4-2 all code checker using pep8 and pyflakes - Python 3.x

> > How does some use ntp_source_score.py? You mention in the comments for that
> > file that it can be for diagnostic purposes. That sounds like something that
> > would be nice to expose via juju actions. I don't think that should
> > necessarily block the landing of this, but if you're planning to advertise
> > that functionality at all, I think it should be done via juju actions first.
>
> That would probably be important to have for receiving troubleshooting reports
> from others, but I wasn't planning to advertise it in the first instance.
> I'll try to get some time to add one in the next couple of weeks.

Stuart Bishop (stub) wrote :

Comments added inline. Leaving actual approval for Tom and test suite/flake issues.

review: Approve
Paul Gear (paulgear) wrote :

Some replies to Stuart's comments inline.

Paul Gear (paulgear) wrote :

Setting this back to WIP while I improve test suite coverage.

Paul Gear (paulgear) wrote :

I've added quite a few things to the test suite; it's not 100% coverage, but it should be enough to provide sufficient confidence in the code to merge.

Stuart Bishop (stub) wrote :

Looks good. A few minor comments. The psutil magic import at the start of ntp_scoring.py needs to be fixed, or it may fail the first time it is run.

review: Approve
Paul Gear (paulgear) wrote :

Pushed new version with updates.

Stuart Bishop (stub) wrote :

Looks good.

You probably want 'fatal=True' in install_packages(), which I missed last time. I doubt the rest of this charm will work if the packages are missing, so its better to fail early.

review: Approve
Paul Gear (paulgear) wrote :

On 07/11/17 19:10, Stuart Bishop wrote:
> Review: Approve
>
> Looks good.
>
> You probably want 'fatal=True' in install_packages(), which I missed last time. I doubt the rest of this charm will work if the packages are missing, so its better to fail early.

The rest of the charm should work fine if those packages are missing,
and the scoring & auto-peering should fail gracefully if they are.

I've pushed a fix to handle the case where python3-psutil is missing.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/.gitignore b/.gitignore
2index ba077a4..c644b99 100644
3--- a/.gitignore
4+++ b/.gitignore
5@@ -1 +1,2 @@
6 bin
7+__pycache__
8diff --git a/Makefile b/Makefile
9index 706d2ae..85b5df4 100644
10--- a/Makefile
11+++ b/Makefile
12@@ -1,8 +1,13 @@
13 #!/usr/bin/make
14-PYTHON := /usr/bin/env python
15+PYTHON := /usr/bin/env PYTHONPATH=$(PWD)/hooks python3
16+CHARM_NAME := ntp
17+CSDEST := cs:~$(LOGNAME)/$(CHARM_NAME)
18
19-lint:
20- @python2 -m flake8 --exclude hooks/charmhelpers hooks
21+test:
22+ $(PYTHON) -m unittest unit_tests/test_ntp_*.py
23+
24+lint: test
25+ @python3 -m flake8 --max-line-length=120 --exclude hooks/charmhelpers hooks
26 @charm proof
27
28 bin/charm_helpers_sync.py:
29@@ -12,3 +17,13 @@ bin/charm_helpers_sync.py:
30
31 sync: bin/charm_helpers_sync.py
32 @$(PYTHON) bin/charm_helpers_sync.py -c charm-helpers-sync.yaml
33+
34+git:
35+ git push $(LOGNAME)
36+
37+cspush: lint
38+ version=`charm push . $(CSDEST) | awk '/^url:/ {print $$2}'` && \
39+ charm release $$version
40+
41+upgrade: cspush
42+ juju upgrade-charm $(CHARM_NAME)
43diff --git a/README.md b/README.md
44index e635e7a..a2605cb 100644
45--- a/README.md
46+++ b/README.md
47@@ -38,14 +38,13 @@ To disable the default list of pool servers, set that to the empty string:
48
49 Sources, peers, and pools should be space separated.
50
51-When you need a set of services to keep close time to each other, it may
52-be useful to have them automatically peer with each other. This means
53-any set of services which use the same ntp subordinate will peer together.
54+If you have a large number of nodes which need to keep close sync with one
55+another but need to keep upstream traffic to a minimum, try auto_peers:
56
57 juju set ntp auto_peers=true
58
59-This will add all the hosts as peers to each other. Using auto_peers is not
60-recommended when more than 10 units are expected to be deployed.
61+This will select the most suitable units for connecting with upstream, and
62+configure the remaining units to receive time from those units.
63
64 Mastered
65 ========
66diff --git a/config.yaml b/config.yaml
67index 1405f4b..1bbc1ed 100644
68--- a/config.yaml
69+++ b/config.yaml
70@@ -59,9 +59,14 @@ options:
71 default: false
72 type: boolean
73 description: >
74- Automatically peer with other units in the same service.
75- DEPRECATED. Please consider using the ntpmaster charm to provide
76- sufficient peers for your environment in favour of auto_peers.
77+ Automatically select the most appropriate units in the service to
78+ be a service stratum connecting with upstream NTP servers, and use
79+ those units as time sources for the remaining units.
80+ auto_peers_upstream:
81+ default: 6
82+ type: int
83+ description: >
84+ How many units should attempt to connect with upstream NTP servers?
85 use_iburst:
86 default: true
87 type: boolean
88diff --git a/files/nagios/check_ntpmon.py b/files/nagios/check_ntpmon.py
89index 8d24df4..a3c64be 100755
90--- a/files/nagios/check_ntpmon.py
91+++ b/files/nagios/check_ntpmon.py
92@@ -452,7 +452,7 @@ class NTPPeers(object):
93 output = subprocess.check_output(["ntpq", "-pn"], stderr=null)
94 if len(output) > 0:
95 lines = output.split("\n")
96- except:
97+ except Exception:
98 traceback.print_exc(file=sys.stdout)
99 return lines
100
101diff --git a/hooks/ntp_hooks.py b/hooks/ntp_hooks.py
102index d0356c3..efde7ad 100755
103--- a/hooks/ntp_hooks.py
104+++ b/hooks/ntp_hooks.py
105@@ -1,15 +1,14 @@
106 #!/usr/bin/python3
107
108-import sys
109-import charmhelpers.core.hookenv as hookenv
110-import charmhelpers.core.host as host
111-import charmhelpers.fetch as fetch
112-from charmhelpers.core.hookenv import UnregisteredHookError
113+from charmhelpers.contrib.charmsupport import nrpe
114 from charmhelpers.contrib.templating.jinja import render
115-import shutil
116+from charmhelpers.core import hookenv, host, unitdata
117+import charmhelpers.fetch as fetch
118 import os
119+import shutil
120+import sys
121
122-from charmhelpers.contrib.charmsupport import nrpe
123+import ntp_scoring
124
125 NAGIOS_PLUGINS = '/usr/local/lib/nagios/plugins'
126
127@@ -19,22 +18,56 @@ NTP_CONF_ORIG = '{}.orig'.format(NTP_CONF)
128 hooks = hookenv.Hooks()
129
130
131-def get_peer_nodes():
132- hosts = []
133- hosts.append(hookenv.unit_get('private-address'))
134+def get_peer_sources(topN=6):
135+ """
136+ Get our score and put it on the peer relation.
137+ Read the peer private addresses and their scores.
138+ Determine whether we're in the top N scores;
139+ if so, we're upstream - return None
140+ Otherwise, return the list of the top N peers.
141+ """
142+ if topN is None:
143+ topN = 6
144+ ourscore = ntp_scoring.get_score()
145+ if ourscore is None:
146+ hookenv.log('[AUTO_PEER] Our score cannot be determined - check logs for reason')
147+ return None
148+
149+ peers = []
150 for relid in hookenv.relation_ids('ntp-peers'):
151+ hookenv.relation_set(relation_id=relid, relation_settings={'score': ourscore['score']})
152 for unit in hookenv.related_units(relid):
153- hosts.append(hookenv.relation_get('private-address',
154- unit, relid))
155- hosts.sort()
156- return hosts
157+ addr = hookenv.relation_get('private-address', unit, relid)
158+ peerscore = hookenv.relation_get('score', unit, relid)
159+ if peerscore is not None:
160+ peers.append((addr, float(peerscore)))
161+
162+ if len(peers) < topN:
163+ # we don't have enough peers to do auto-peering
164+ hookenv.log('[AUTO_PEER] There are only {} peers; not doing auto-peering'.format(len(peers)))
165+ return None
166+
167+ # list of hosts with scores better than ours
168+ hosts = list(filter(lambda x: x[1] > ourscore['score'], peers))
169+ hookenv.log('[AUTO_PEER] {} peers better than us, topN == {}'.format(len(hosts), topN))
170+
171+ # if the list is less than topN long, we're in the topN hosts
172+ if len(hosts) < topN:
173+ return None
174+ else:
175+ # sort list of hosts by score, keep only the topN
176+ topNhosts = sorted(hosts, key=lambda x: x[1], reverse=True)[0:topN]
177+ # return only the host addresses
178+ return map(lambda x: x[0], topNhosts)
179
180
181 @hooks.hook('install')
182 def install():
183 fetch.apt_update(fatal=True)
184- fetch.apt_install(["ntp"], fatal=True)
185- shutil.copy(NTP_CONF, NTP_CONF_ORIG)
186+ ntp_scoring.install_packages()
187+ if ntp_scoring.get_virt_type() != 'container':
188+ fetch.apt_install(["ntp"], fatal=True)
189+ shutil.copy(NTP_CONF, NTP_CONF_ORIG)
190
191
192 def get_sources(sources, iburst=True, source_list=None):
193@@ -61,25 +94,52 @@ def get_sources(sources, iburst=True, source_list=None):
194 'ntp-peers-relation-changed')
195 @host.restart_on_change({NTP_CONF: ['ntp']})
196 def write_config():
197+ ntp_scoring.install_packages()
198+ if ntp_scoring.get_virt_type() == 'container':
199+ host.service_stop('ntp')
200+ host.service_pause('ntp')
201+ hookenv.close_port(123, protocol="UDP")
202+ return
203+
204+ host.service_resume('ntp')
205 hookenv.open_port(123, protocol="UDP")
206+
207 use_iburst = hookenv.config('use_iburst')
208- source = hookenv.config('source')
209 orphan_stratum = hookenv.config('orphan_stratum')
210- remote_sources = get_sources(source, iburst=use_iburst)
211+ source = hookenv.config('source')
212 pools = hookenv.config('pools')
213- remote_pools = get_sources(pools, iburst=use_iburst)
214- for relid in hookenv.relation_ids('master'):
215- for unit in hookenv.related_units(relid=relid):
216- u_addr = hookenv.relation_get(attribute='private-address',
217- unit=unit, rid=relid)
218- remote_sources.append({'name': u_addr, 'iburst': 'iburst'})
219-
220 peers = hookenv.config('peers')
221- remote_peers = get_sources(peers, iburst=use_iburst)
222 auto_peers = hookenv.config('auto_peers')
223- if hookenv.relation_ids('ntp-peers') and auto_peers:
224- remote_peers = get_sources(get_peer_nodes(), iburst=use_iburst,
225- source_list=remote_peers)
226+
227+ remote_sources = get_sources(source, iburst=use_iburst)
228+ remote_pools = get_sources(pools, iburst=use_iburst)
229+ remote_peers = get_sources(peers, iburst=use_iburst)
230+
231+ kv = unitdata.kv()
232+ hookenv.atexit(kv.flush)
233+
234+ if hookenv.relation_ids('master'):
235+ # use master relation only
236+ for relid in hookenv.relation_ids('master'):
237+ for unit in hookenv.related_units(relid=relid):
238+ u_addr = hookenv.relation_get(attribute='private-address',
239+ unit=unit, rid=relid)
240+ remote_sources.append({'name': u_addr, 'iburst': 'iburst'})
241+ elif auto_peers and hookenv.relation_ids('ntp-peers'):
242+ # use auto_peers
243+ auto_peer_list = get_peer_sources(hookenv.config('auto_peers_upstream'))
244+ if auto_peer_list is None:
245+ # we are upstream - use configured sources, pools, peers
246+ kv.set('auto_peer', 'upstream')
247+ else:
248+ # override all sources with auto_peer_list
249+ kv.set('auto_peer', 'client')
250+ remote_sources = get_sources(auto_peer_list, iburst=use_iburst)
251+ remote_pools = []
252+ remote_peers = []
253+ else:
254+ # use configured sources, pools, peers
255+ kv.unset('auto_peer')
256
257 if len(remote_sources) == 0 and len(remote_peers) == 0 and len(remote_pools) == 0:
258 # we have no peers/pools/servers; restore default ntp.conf provided by OS
259@@ -134,7 +194,7 @@ def update_nrpe_config():
260 # Hyper-V host clock sync handling - workaround until https://bugs.launchpad.net/bugs/1676635 is SRUed for xenial
261 # See also:
262 # - https://patchwork.kernel.org/patch/9525945/
263-# - https://social.msdn.microsoft.com/Forums/en-US/8c0a1026-0b02-405a-848e-628e68229eaf/i-have-a-lot-of-time-has-been-changed-in-the-journal-of-my-linux-boxes?forum=WAVirtualMachinesforWindows
264+# - https://social.msdn.microsoft.com/Forums/en-US/8c0a1026-0b02-405a-848e-628e68229eaf/i-have-a-lot-of-time-has-been-changed-in-the-journal-of-my-linux-boxes?forum=WAVirtualMachinesforWindows # NOQA: E501
265 _device_class = '9527e630-d0ae-497b-adce-e80ab0175caf'
266 _vmbus_dir = '/sys/bus/vmbus/'
267
268@@ -146,11 +206,11 @@ def find_hyperv_host_sync_device():
269 try:
270 f = open(os.path.join(_vmbus_dir, 'devices', d, 'class_id'), 'r')
271 if _device_class in f.readline():
272- hookenv.log('Hyper-V host time sync device is {}'.format(f.name), level=hookenv.DEBUG)
273+ hookenv.log('Hyper-V host time sync device is {}'.format(f.name))
274 return d
275- except:
276+ except Exception:
277 pass
278- except:
279+ except Exception:
280 pass
281 return None
282
283@@ -164,13 +224,13 @@ def check_hyperv_host_sync(device_id):
284 firstline = f.readline().strip()
285 result = firstline == '3'
286 enabled = 'enabled' if result else 'disabled'
287- hookenv.log('Hyper-V host time sync is ' + enabled, level=hookenv.DEBUG)
288+ hookenv.log('Hyper-V host time sync is ' + enabled)
289 if result:
290 return device_id
291 else:
292 return None
293- except:
294- hookenv.log('Hyper-V host time sync status file {} not found'.format(statefile), level=hookenv.DEBUG)
295+ except Exception:
296+ hookenv.log('Hyper-V host time sync status file {} not found'.format(statefile))
297 return None
298 else:
299 return None
300@@ -179,12 +239,12 @@ def check_hyperv_host_sync(device_id):
301 def disable_hyperv_host_sync(device_id):
302 """Unbind the Hyper-V host clock sync driver"""
303 try:
304- hookenv.log('Disabling Hyper-V host time sync', level=hookenv.DEBUG)
305+ hookenv.log('Disabling Hyper-V host time sync')
306 path = os.path.join(_vmbus_dir, 'drivers', 'hv_util', 'unbind')
307 f = open(path, 'w')
308 print(device_id, file=f)
309 return True
310- except:
311+ except Exception:
312 return False
313
314
315@@ -203,21 +263,43 @@ def hyperv_sync_status():
316
317 @hooks.hook('update-status')
318 def assess_status():
319- hookenv.application_version_set(
320- fetch.get_upstream_version('ntp')
321- )
322- if host.service_running('ntp'):
323- status = 'Unit is ready'
324- status_extra = hyperv_sync_status()
325- if status_extra:
326- status = status + '; ' + status_extra
327- hookenv.status_set('active', status)
328+ version = fetch.get_upstream_version('ntp')
329+ if version is not None:
330+ hookenv.application_version_set(version)
331+
332+ # create base status
333+ if ntp_scoring.get_virt_type() == 'container':
334+ state = 'blocked'
335+ status = 'NTP not supported in containers: please configure on host'
336+ elif host.service_running('ntp'):
337+ state = 'active'
338+ status = 'Ready'
339 else:
340- hookenv.status_set('blocked', 'ntp not running')
341+ state = 'blocked'
342+ status = 'Not running'
343+
344+ # append Hyper-V status, if any
345+ hyperv_status = hyperv_sync_status()
346+ if hyperv_status is not None:
347+ status += ', ' + hyperv_status
348+
349+ # append scoring status, if any
350+ # (don't force update of the score from update-status more than once a month)
351+ max_age = 31 * 86400
352+ scorestr = ntp_scoring.get_score_string(max_seconds=max_age)
353+ if scorestr is not None:
354+ status += ', ' + scorestr
355+
356+ # append auto_peer status
357+ autopeer = unitdata.kv().get('auto_peer')
358+ if autopeer is not None:
359+ status += ' [{}]'.format(autopeer)
360+
361+ hookenv.status_set(state, status)
362
363
364 if __name__ == '__main__':
365 try:
366 hooks.execute(sys.argv)
367- except UnregisteredHookError as e:
368+ except hookenv.UnregisteredHookError as e:
369 hookenv.log('Unknown hook {} - skipping.'.format(e))
370diff --git a/hooks/ntp_scoring.py b/hooks/ntp_scoring.py
371new file mode 100755
372index 0000000..af55103
373--- /dev/null
374+++ b/hooks/ntp_scoring.py
375@@ -0,0 +1,159 @@
376+
377+# Copyright (c) 2017 Canonical Ltd
378+# Author: Paul Gear
379+
380+# This module retrieves the score calculated in ntp_source_score, and
381+# creates an overall node weighting based on the machine type (bare metal,
382+# container, or VM) and software running locally. It reduces the score
383+# for nodes with OpenStack ceph, nova, or swift services running, in order
384+# to decrease the likelihood that they will be selected as upstreams.
385+
386+from charmhelpers.core import hookenv, unitdata
387+import charmhelpers.fetch as fetch
388+import json
389+import time
390+
391+import ntp_source_score
392+
393+
394+def install_packages():
395+ fetch.apt_install(["facter", "ntpdate", "python3-psutil", "virt-what"], fatal=False)
396+
397+
398+def get_virt_type():
399+ """Work out what type of environment we're running in"""
400+ for line in ntp_source_score.run_cmd('facter virtual'):
401+ fields = str(line).split()
402+ if len(fields) > 0:
403+ if fields[0] in ['physical', 'xen0']:
404+ return 'physical'
405+ if fields[0] in ['docker', 'lxc', 'openvz']:
406+ return 'container'
407+ # Anything not one of the above-mentioned types is assumed to be a VM
408+ return 'vm'
409+
410+
411+def get_virt_multiplier():
412+ virt_type = get_virt_type()
413+ if virt_type == 'container':
414+ # containers should be synchronized from their host
415+ return -1
416+ elif virt_type == 'physical':
417+ hookenv.log('[SCORE] running on physical host - score bump 25%')
418+ return 1.25
419+ else:
420+ hookenv.log('[SCORE] probably running in a VM - score bump 0%')
421+ return 1
422+
423+
424+def get_package_divisor():
425+ """Check for running ceph, swift, & nova-compute services,
426+ and increase divisor for each."""
427+ try:
428+ import psutil
429+ except Exception:
430+ # If we can't read the process table, assume a worst-case.
431+ # (Normally, if every process is running, this will return
432+ # 1.1 * 1.1 * 1.25 * 1.25 = 1.890625.)
433+ return 2
434+
435+ # set the weight for each process (regardless of how many there are running)
436+ running = {}
437+ for p in psutil.process_iter():
438+ if p.name().startswith('nova-compute'):
439+ running['nova-compute'] = 1.25
440+ if p.name().startswith('ceph-osd'):
441+ running['ceph-osd'] = 1.25
442+ elif p.name().startswith('ceph-'):
443+ running['ceph'] = 1.1
444+ elif p.name().startswith('swift-'):
445+ running['swift'] = 1.1
446+
447+ # increase the divisor for each discovered process type
448+ divisor = 1
449+ for r in running:
450+ hookenv.log('[SCORE] %s running - score divisor %.3f' % (r, running[r]))
451+ divisor *= running[r]
452+ return divisor
453+
454+
455+def check_score(seconds=None):
456+ if seconds is None:
457+ seconds = time.time()
458+ score = {
459+ 'divisor': 1,
460+ 'multiplier': 0,
461+ 'score': -999,
462+ 'time': seconds,
463+ }
464+
465+ # skip scoring if we have an explicitly configured master
466+ relation_sources = hookenv.relation_ids('master')
467+ score['master-relations'] = len(relation_sources)
468+ if relation_sources is not None and len(relation_sources) > 0:
469+ hookenv.log('[SCORE] master relation configured - skipped scoring')
470+ return score
471+
472+ # skip scoring if we're in a container
473+ multiplier = get_virt_multiplier()
474+ score['multiplier'] = multiplier
475+ if multiplier <= 0:
476+ hookenv.log('[SCORE] running in a container - skipped scoring')
477+ return score
478+
479+ # skip scoring if auto_peers is off
480+ auto_peers = hookenv.config('auto_peers')
481+ score['auto-peers'] = auto_peers
482+ if not auto_peers:
483+ hookenv.log('[SCORE] auto_peers is disabled - skipped scoring')
484+ return score
485+
486+ # skip scoring if we have no sources
487+ sources = hookenv.config('source').split()
488+ peers = hookenv.config('peers').split()
489+ pools = hookenv.config('pools').split()
490+ host_list = sources + peers + pools
491+ if len(host_list) == 0:
492+ hookenv.log('[SCORE] No sources configured')
493+ return score
494+
495+ # Now that we've passed all those checks, check upstreams, calculate a score, and return the result
496+ divisor = get_package_divisor()
497+ score['divisor'] = divisor
498+ score['host-list'] = host_list
499+ score['raw'] = ntp_source_score.get_source_score(host_list, verbose=True)
500+ score['score'] = score['raw'] * multiplier / divisor
501+ hookenv.log('[SCORE] Suitability score: %.3f' % (score['score'],))
502+ return score
503+
504+
505+def get_score(max_seconds=86400):
506+ # Remove this if/when we convert the charm to reactive
507+ kv = unitdata.kv()
508+ hookenv.atexit(kv.flush)
509+
510+ score = kv.get('ntp_score')
511+ if score is not None:
512+ saved_time = score.get('time', 0)
513+ else:
514+ saved_time = 0
515+
516+ now = time.time()
517+ if score is None or now - saved_time > max_seconds:
518+ score = check_score(now)
519+ kv.set('ntp_score', score)
520+ hookenv.log('[SCORE] saved %s' % (json.dumps(score),))
521+
522+ return score
523+
524+
525+def get_score_string(score=None, max_seconds=86400):
526+ if score is None:
527+ score = get_score(max_seconds)
528+ if not hookenv.config('auto_peers') or 'raw' not in score:
529+ return None
530+ return 'score %.3f (%.1f) at %s' % (
531+ score['score'],
532+ score['multiplier'] / score['divisor'],
533+ time.ctime(score['time'])
534+ )
535diff --git a/hooks/ntp_source_score.py b/hooks/ntp_source_score.py
536new file mode 100755
537index 0000000..71b1cef
538--- /dev/null
539+++ b/hooks/ntp_source_score.py
540@@ -0,0 +1,198 @@
541+#!/usr/bin/python3
542+
543+# Copyright (c) 2017 Canonical Ltd
544+# Author: Paul Gear
545+
546+# This module runs ntpdate in test mode against the provided list of sources
547+# in order to determine this node's suitability as an NTP server, based on the
548+# number of reachable sources, and the network delay in reaching them. Up to
549+# MAX_THREADS (default 32) threads will be spawned to run ntpdate, in order
550+# to minimise the time taken to calculate a score.
551+
552+# A main method is included to allow this module to be called separately from
553+# juju hooks for diagnostic purposes. It has no dependencies on juju,
554+# charmhelpers, or the other modules in this charm.
555+
556+import argparse
557+import math
558+import queue
559+import random
560+import statistics
561+import subprocess
562+import threading
563+import time
564+
565+rand = random.SystemRandom()
566+MAX_THREADS = 32
567+
568+
569+def rms(l):
570+ """Return the root mean square of the list"""
571+ if len(l) > 0:
572+ squares = [x ** 2 for x in l]
573+ return math.sqrt(statistics.mean(squares))
574+ else:
575+ return float('nan')
576+
577+
578+def run_cmd(cmd):
579+ """Run the output, return a list of lines returned; ignore errors"""
580+ lines = []
581+ try:
582+ output = subprocess.check_output(cmd.split(), stderr=subprocess.DEVNULL).decode('UTF-8')
583+ lines = output.split('\n')
584+ except Exception:
585+ pass
586+ return lines
587+
588+
589+def get_source_delays(source):
590+ """Run ntpdate on the source, which may resolve to multiple addresses;
591+ return the list of delay values. This can take several seconds, depending
592+ on bandwidth and distance of the sources."""
593+ delays = []
594+ cmd = 'ntpdate -d -t 0.2 ' + source
595+ for line in run_cmd(cmd):
596+ fields = line.split()
597+ if len(fields) >= 2 and fields[0] == 'delay':
598+ delay = float(fields[1].split(',')[0])
599+ if delay > 0:
600+ delays.append(delay)
601+ return delays
602+
603+
604+def worker(num, src, dst, debug=False):
605+ """Thread worker for parallelising ntpdate runs. Gets host name
606+ from src queue and places host and delay list in dst queue."""
607+ if debug:
608+ print('[%d] Starting' % (num,))
609+ while True:
610+ host = src.get()
611+ if host is None:
612+ break
613+
614+ # lower-numbered threads sleep for a shorter time, on average
615+ s = rand.random() * num / MAX_THREADS
616+ if debug:
617+ print('[%d] Sleeping %.3f' % (num, s))
618+ time.sleep(s)
619+
620+ if debug:
621+ print('[%d] Getting results for [%s]' % (num, host))
622+ delays = get_source_delays(host)
623+ src.task_done()
624+ if len(delays):
625+ result = (host, delays)
626+ dst.put(result)
627+
628+
629+def get_delay_score(delay):
630+ """Take a delay in seconds and return a score. Under most sane NTP setups
631+ will return a value between 0 and 10, where 10 is better and 0 is worse."""
632+ return -math.log(delay)
633+
634+
635+def start_workers(threads, num_threads, src, dst, debug=False):
636+ """Start all of the worker threads."""
637+ for i in range(num_threads):
638+ t = threading.Thread(target=worker, args=(i, src, dst, debug))
639+ t.start()
640+ threads.append(t)
641+
642+
643+def stop_workers(threads, src):
644+ """Send the workers a None object, causing them to stop work.
645+ We enqueue one stop object for each worker."""
646+ for i in range(len(threads)):
647+ src.put(None)
648+
649+
650+def calculate_score(delays):
651+ """Return the rms, mean, standard deviation, and overall
652+ score for the passed list of delay values."""
653+ score = 0
654+ if len(delays) > 0:
655+ r = rms(delays)
656+ m = statistics.mean(delays)
657+ s = statistics.pstdev(delays, m)
658+ source_score = get_delay_score(r)
659+ score += source_score
660+ else:
661+ r = m = s = score = 0
662+ return (r, m, s, score)
663+
664+
665+def calculate_results(q, verbose=False):
666+ """Get the scores for all the hosts.
667+ Return a hash of hosts and their cumulative scores."""
668+ results = {}
669+ while not q.empty():
670+ (host, delays) = q.get()
671+ (rms, mean, stdev, score) = calculate_score(delays)
672+ delaystrings = [str(x) for x in delays]
673+ if verbose:
674+ print('%s score=%.3f rms=%.3f mean=%.3f stdevp=%.3f [%s]' %
675+ (host, score, rms, mean, stdev, ", ".join(delaystrings)))
676+ if host in results:
677+ results[host] += score
678+ else:
679+ results[host] = score
680+ return results
681+
682+
683+def wait_workers(threads):
684+ """Wait for the given list of threads to complete."""
685+ for t in threads:
686+ t.join()
687+
688+
689+def run_checks(hosts, debug=False, numthreads=None, verbose=False):
690+ """Perform a check of the listed hosts.
691+ Can take several seconds per host."""
692+ sources = queue.Queue()
693+ results = queue.Queue()
694+ threads = []
695+ for h in hosts:
696+ sources.put(h)
697+ if numthreads is None:
698+ numthreads = min(len(hosts), MAX_THREADS)
699+ start_workers(threads, numthreads, sources, results, debug)
700+ sources.join()
701+ stop_workers(threads, sources)
702+ # wait_workers(threads)
703+ return calculate_results(results, verbose)
704+
705+
706+def get_source_score(hosts, debug=False, numthreads=None, verbose=False):
707+ """Check NTP connectivity to the given list of sources - return a single overall score"""
708+ results = run_checks(hosts, debug, numthreads, verbose)
709+ if results is None:
710+ return 0
711+
712+ total = 0
713+ for host in results:
714+ total += results[host]
715+ return total
716+
717+
718+def display_results(results):
719+ """Sort the hash by value. Print the results."""
720+ # http://stackoverflow.com/a/2258273
721+ result = sorted(results.items(), key=lambda x: x[1], reverse=True)
722+ for i in result:
723+ print("%s %.3f" % (i[0], i[1]))
724+
725+
726+def get_args():
727+ parser = argparse.ArgumentParser(description='Get NTP server/peer/pool scores')
728+ parser.add_argument('--debug', '-d', action='store_true', help='Enable thread debug output')
729+ parser.add_argument('--verbose', '-v', action='store_true', help='Display scoring detail')
730+ parser.add_argument('hosts', nargs=argparse.REMAINDER, help='List of hosts to check')
731+ return parser.parse_args()
732+
733+
734+if __name__ == '__main__':
735+ args = get_args()
736+ results = run_checks(args.hosts, debug=args.debug, verbose=args.verbose)
737+ if results:
738+ display_results(results)
739diff --git a/tests/10-deploy-test.py b/tests/10-deploy-test.py
740index db65f8c..efa5d99 100755
741--- a/tests/10-deploy-test.py
742+++ b/tests/10-deploy-test.py
743@@ -39,7 +39,7 @@ except amulet.helpers.TimeoutError:
744 message = 'The environment did not setup in %d seconds.' % seconds
745 # The SKIP status enables skip or fail the test based on configuration.
746 amulet.raise_status(amulet.SKIP, msg=message)
747-except:
748+except Exception:
749 raise
750
751 # Unable to get the sentry unit for ntp because it is a subordinate.
752diff --git a/unit_tests/test_ntp_hooks.py b/unit_tests/test_ntp_hooks.py
753new file mode 100644
754index 0000000..e69de29
755--- /dev/null
756+++ b/unit_tests/test_ntp_hooks.py
757diff --git a/unit_tests/test_ntp_scoring.py b/unit_tests/test_ntp_scoring.py
758new file mode 100644
759index 0000000..5840252
760--- /dev/null
761+++ b/unit_tests/test_ntp_scoring.py
762@@ -0,0 +1,106 @@
763+#!/usr/bin/env python3
764+
765+from random import shuffle
766+from unittest.mock import Mock, patch
767+import unittest
768+
769+import ntp_scoring
770+
771+
772+class TestNtpScoring(unittest.TestCase):
773+
774+ def setUp(self):
775+ patcher = patch('charmhelpers.core.hookenv.log')
776+ patcher.start()
777+ self.addCleanup(patcher.stop)
778+ patcher = patch('charmhelpers.core.unitdata.kv')
779+ patcher.start()
780+ self.addCleanup(patcher.stop)
781+
782+ @patch('ntp_source_score.run_cmd')
783+ def testGetVirtTypeValues(self, run_cmd):
784+ def virt_test(expected, return_value):
785+ run_cmd.return_value = [return_value]
786+ self.assertEqual(ntp_scoring.get_virt_type(), expected)
787+ run_cmd.assert_called_once_with('facter virtual')
788+ run_cmd.reset_mock()
789+
790+ virt_test('container', 'docker')
791+ virt_test('container', 'lxc')
792+ virt_test('container', 'openvz')
793+ virt_test('physical', 'physical')
794+ virt_test('physical', 'xen0')
795+ virt_test('vm', '')
796+ virt_test('vm', [])
797+ virt_test('vm', 1.23)
798+ virt_test('vm', 'a')
799+ virt_test('vm', 'kvm')
800+ virt_test('vm', None)
801+ virt_test('vm', 'something-else')
802+ virt_test('vm', 'The quick brown fox jumps over the lazy dogs')
803+
804+ @patch('ntp_source_score.run_cmd')
805+ def testGetVirtTypeEmptyList(self, run_cmd):
806+ run_cmd.return_value = []
807+ self.assertEqual(ntp_scoring.get_virt_type(), 'vm')
808+ run_cmd.assert_called_once_with('facter virtual')
809+
810+ @patch('ntp_source_score.run_cmd')
811+ def testGetVirtTypeWrongType(self, run_cmd):
812+ run_cmd.return_value = {}
813+ self.assertEqual(ntp_scoring.get_virt_type(), 'vm')
814+ run_cmd.assert_called_once_with('facter virtual')
815+
816+ @patch('ntp_source_score.run_cmd')
817+ def testGetVirtMultiplier(self, run_cmd):
818+ def multiplier_test(expected, return_value):
819+ run_cmd.return_value = [return_value]
820+ self.assertEqual(ntp_scoring.get_virt_multiplier(), expected)
821+ run_cmd.assert_called_once_with('facter virtual')
822+ run_cmd.reset_mock()
823+
824+ multiplier_test(-1, 'docker')
825+ multiplier_test(-1, 'lxc')
826+ multiplier_test(-1, 'openvz')
827+ multiplier_test(1.25, 'physical')
828+ multiplier_test(1.25, 'xen0')
829+ multiplier_test(1, '')
830+ multiplier_test(1, [])
831+ multiplier_test(1, 1.23)
832+ multiplier_test(1, 'a')
833+ multiplier_test(1, 'kvm')
834+ multiplier_test(1, None)
835+ multiplier_test(1, 'something-else')
836+ multiplier_test(1, 'The quick brown fox jumps over the lazy dogs')
837+
838+ def testGetPackageDivisor(self):
839+
840+ def test_divisor(expected, pslist, precision=6):
841+ def fake_pslist():
842+ """yield a list of objects for which name() returns the given list"""
843+ shuffle(pslist)
844+ for p in pslist:
845+ m = Mock()
846+ m.name.return_value = p
847+ yield m
848+
849+ with patch('psutil.process_iter', side_effect=fake_pslist):
850+ divisor = round(ntp_scoring.get_package_divisor(), precision)
851+ self.assertEqual(round(expected, precision), divisor)
852+
853+ with self.assertRaises(TypeError):
854+ test_divisor(1, None)
855+
856+ test_divisor(1, [])
857+ test_divisor(1, ['a', 'b', 'c'])
858+ test_divisor(1, 'The quick brown fox jumps over the lazy dogs'.split())
859+ test_divisor(1.1, 'The quick brown fox jumps over the lazy dogs'.split() + ['swift-1'])
860+ test_divisor(1.1, ['swift-1'])
861+ test_divisor(1.1, ['ceph-1', 'ceph-2'])
862+ test_divisor(1.25, ['ceph-osd-1', 'ceph-osd-2', 'ceph-osd-3'])
863+ test_divisor(1.25, ['nova-compute-1', 'nova-compute-2', 'nova-compute-3', 'nova-compute-4'])
864+ test_divisor(1.1 * 1.25, ['swift-1', 'nova-compute-2'])
865+ test_divisor(1.1 * 1.25, ['systemd', 'bind', 'swift-1', 'nova-compute-2', 'test'])
866+ test_divisor(1.1 * 1.25 * 1.1, ['swift-1', 'nova-compute-2', 'ceph-3'])
867+ test_divisor(1.1 * 1.25 * 1.25, ['swift-1', 'nova-compute-2', 'ceph-osd-3'])
868+ test_divisor(1.1 * 1.25 * 1.1 * 1.25, ['swift-1', 'nova-compute-2', 'ceph-3', 'ceph-osd-4'])
869diff --git a/unit_tests/test_ntp_source_score.py b/unit_tests/test_ntp_source_score.py
870new file mode 100644
871index 0000000..378faa0
872--- /dev/null
873+++ b/unit_tests/test_ntp_source_score.py
874@@ -0,0 +1,167 @@
875+#!/usr/bin/env python3
876+
877+from unittest.mock import patch
878+import math
879+import unittest
880+
881+from ntp_source_score import (
882+ get_delay_score,
883+ get_source_delays,
884+ rms,
885+ run_cmd,
886+)
887+
888+ntpdate_output = """
889+...
890+reference time: dda179ee.3ec34fdd Mon, Oct 30 2017 20:14:06.245
891+originate timestamp: dda17a5b.af7c528b Mon, Oct 30 2017 20:15:55.685
892+transmit timestamp: dda17a5b.80b4dc04 Mon, Oct 30 2017 20:15:55.502
893+filter delay: 0.54126 0.36757 0.36655 0.36743
894+ 0.00000 0.00000 0.00000 0.00000
895+filter offset: 0.099523 0.012978 0.011831 0.011770
896+ 0.000000 0.000000 0.000000 0.000000
897+delay 0.36655, dispersion 0.01126
898+offset 0.011831
899+...
900+reference time: dda17695.69e65b2f Mon, Oct 30 2017 19:59:49.413
901+originate timestamp: dda17a5b.afcec2dd Mon, Oct 30 2017 20:15:55.686
902+transmit timestamp: dda17a5b.80bb2488 Mon, Oct 30 2017 20:15:55.502
903+filter delay: 0.36520 0.36487 0.36647 0.36604
904+ 0.00000 0.00000 0.00000 0.00000
905+filter offset: 0.012833 0.013758 0.013731 0.013629
906+ 0.000000 0.000000 0.000000 0.000000
907+delay 0.36487, dispersion 0.00049
908+offset 0.013758
909+...
910+reference time: dda1782c.6aec9646 Mon, Oct 30 2017 20:06:36.417
911+originate timestamp: dda17a5b.d2d04ef4 Mon, Oct 30 2017 20:15:55.823
912+transmit timestamp: dda17a5b.b37c4098 Mon, Oct 30 2017 20:15:55.701
913+filter delay: 0.28581 0.28406 0.28551 0.28596
914+ 0.00000 0.00000 0.00000 0.00000
915+filter offset: -0.00802 -0.00854 -0.00791 -0.00787
916+ 0.000000 0.000000 0.000000 0.000000
917+delay 0.28406, dispersion 0.00050
918+offset -0.008544
919+...
920+reference time: dda17735.4a03e3ca Mon, Oct 30 2017 20:02:29.289
921+originate timestamp: dda17a5c.1634d231 Mon, Oct 30 2017 20:15:56.086
922+transmit timestamp: dda17a5b.e6934fad Mon, Oct 30 2017 20:15:55.900
923+filter delay: 0.37044 0.37077 0.37050 0.37086
924+ 0.00000 0.00000 0.00000 0.00000
925+filter offset: 0.013993 0.013624 0.013425 0.013362
926+ 0.000000 0.000000 0.000000 0.000000
927+delay 0.37044, dispersion 0.00046
928+offset 0.013993
929+...
930+reference time: dda17695.69e65b2f Mon, Oct 30 2017 19:59:49.413
931+originate timestamp: dda17a5c.4944bb52 Mon, Oct 30 2017 20:15:56.286
932+transmit timestamp: dda17a5c.19cf5199 Mon, Oct 30 2017 20:15:56.100
933+filter delay: 0.36873 0.36823 0.36911 0.36781
934+ 0.00000 0.00000 0.00000 0.00000
935+filter offset: 0.014635 0.014599 0.014166 0.014239
936+ 0.000000 0.000000 0.000000 0.000000
937+delay 0.36781, dispersion 0.00026
938+offset 0.014239
939+...
940+reference time: dda179ee.3ec34fdd Mon, Oct 30 2017 20:14:06.245
941+originate timestamp: dda17a5c.7bbd3828 Mon, Oct 30 2017 20:15:56.483
942+transmit timestamp: dda17a5c.4cf92e99 Mon, Oct 30 2017 20:15:56.300
943+filter delay: 0.36554 0.36617 0.36673 0.36618
944+ 0.00000 0.00000 0.00000 0.00000
945+filter offset: 0.012466 0.012691 0.012863 0.012346
946+ 0.000000 0.000000 0.000000 0.000000
947+delay 0.36554, dispersion 0.00018
948+offset 0.012466
949+...
950+"""
951+ntpdate_delays = [0.36655, 0.36487, 0.28406, 0.37044, 0.36781, 0.36554]
952+
953+
954+class TestNtpSourceScore(unittest.TestCase):
955+
956+ def test_rms(self):
957+ self.assertEqual(rms([0, 0, 0, 0, 0]), 0)
958+ self.assertEqual(rms([0, 1, 0, 1, 0]), math.sqrt(0.4))
959+ self.assertEqual(rms([1, 1, 1, 1, 1]), 1)
960+ self.assertEqual(rms([1, 2, 3, 4, 5]), math.sqrt(11))
961+ self.assertEqual(rms([0.01, 0.02]), math.sqrt(0.00025))
962+ self.assertEqual(rms([0.02766, 0.0894, 0.02657, 0.02679]), math.sqrt(0.00254527615))
963+ self.assertEqual(rms([80, 50, 30]), math.sqrt(3266.66666666666666667))
964+ self.assertEqual(rms([81, 53, 32]), math.sqrt(3464.66666666666666667))
965+ self.assertEqual(rms([81.1, 53.9, 32.3]), math.sqrt(3508.57))
966+ self.assertEqual(rms([81.14, 53.93, 32.30]), math.sqrt(3511.8115))
967+ self.assertEqual(rms([81.141, 53.935, 32.309]), math.sqrt(3512.23919566666666667))
968+ self.assertTrue(math.isnan(rms([])))
969+ with self.assertRaises(TypeError):
970+ rms(['a', 'b', 'c'])
971+
972+ @patch('subprocess.check_output')
973+ def test_run_cmd(self, patched):
974+ patched.return_value = b'a\nb\nc\n'
975+ self.assertEqual(run_cmd('ls'), ['a', 'b', 'c', ''])
976+
977+ patched.return_value = b'4.13.0-14-generic\n'
978+ self.assertEqual(run_cmd('uname -r'), ['4.13.0-14-generic', ''])
979+
980+ self.assertEqual(patched.call_count, 2)
981+
982+ def test_get_source_delays(self):
983+
984+ @patch('ntp_source_score.run_cmd')
985+ def test_source_delay(data, expect, patched):
986+ patched.return_value = data
987+ self.assertEqual(get_source_delays('ntp.example.com'), expect)
988+ patched.assert_called_once_with('ntpdate -d -t 0.2 ntp.example.com')
989+
990+ @patch('ntp_source_score.run_cmd')
991+ def test_source_delay_error(data, e, patched):
992+ patched.return_value = data
993+ with self.assertRaises(e):
994+ get_source_delays('ntp.example.com')
995+ patched.assert_called_once_with('ntpdate -d -t 0.2 ntp.example.com')
996+
997+ test_source_delay([], [])
998+ test_source_delay('', [])
999+ test_source_delay('123', [])
1000+ test_source_delay(['123 678', '234 asdf', 'yaled 345 901'], [])
1001+ test_source_delay(['123 678', 'delay 345 901', '234 asdf'], [345])
1002+ test_source_delay(['delay 123 678', 'delay 234 asdf', 'delay 345 901'], [123, 234, 345])
1003+ test_source_delay(ntpdate_output.split('\n'), ntpdate_delays)
1004+
1005+ test_source_delay_error(None, TypeError)
1006+ test_source_delay_error(123, TypeError)
1007+
1008+ def test_get_delay_score_error(self):
1009+ # You can't have a negative or zero response time
1010+ with self.assertRaises(ValueError):
1011+ get_delay_score(-100)
1012+ with self.assertRaises(ValueError):
1013+ get_delay_score(-1)
1014+ with self.assertRaises(ValueError):
1015+ get_delay_score(-0.1)
1016+ with self.assertRaises(ValueError):
1017+ get_delay_score(0)
1018+
1019+ def test_get_delay_scores(self):
1020+ scores = [
1021+ get_delay_score(0.001), # 1ms delay
1022+ get_delay_score(0.01),
1023+ get_delay_score(0.025),
1024+ get_delay_score(0.05),
1025+ get_delay_score(0.1),
1026+ get_delay_score(0.333), # anything beyond this should never happen
1027+ get_delay_score(0.999),
1028+ get_delay_score(1),
1029+ get_delay_score(3), # 3s delay - probably on the moon
1030+ get_delay_score(10),
1031+ get_delay_score(9999), # 2.79h delay - are you orbiting Saturn or something?
1032+ ]
1033+
1034+ for i in range(len(scores)):
1035+ # all lower delays should get a higher score
1036+ for higher in range(i):
1037+ self.assertLess(scores[i], scores[higher])
1038+ # all higher delays should get a lower score
1039+ if i < len(scores) - 1:
1040+ for lower in range(i + 1, len(scores)):
1041+ self.assertGreater(scores[i], scores[lower])

Subscribers

People subscribed via source and target branches