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

[Paul Gear]

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.

[Paul Gear (paulgear)]

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

[Tom Haddon (mthaddon)]

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

[Paul Gear (paulgear)]

Replies to comments inline.

[Tom Haddon (mthaddon)]

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)]

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

[Paul Gear (paulgear)]

> 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)]

> > 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)]

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

review: Approve

[Paul Gear (paulgear)]

Some replies to Stuart's comments inline.

[Paul Gear (paulgear)]

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

[Paul Gear (paulgear)]

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)]

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)]

Pushed new version with updates.

[Stuart Bishop (stub)]

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)]

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.

diff --git a/.gitignore b/.gitignore
index ba077a4..c644b99 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1 +1,2 @@
 bin
+__pycache__
diff --git a/Makefile b/Makefile
index 706d2ae..85b5df4 100644
--- a/Makefile
+++ b/Makefile
@@ -1,8 +1,13 @@
 #!/usr/bin/make
-PYTHON := /usr/bin/env python
+PYTHON := /usr/bin/env PYTHONPATH=$(PWD)/hooks python3
+CHARM_NAME := ntp
+CSDEST := cs:~$(LOGNAME)/$(CHARM_NAME)
 
-lint:
-	@python2 -m flake8 --exclude hooks/charmhelpers hooks
+test:
+	$(PYTHON) -m unittest unit_tests/test_ntp_*.py
+
+lint: test
+	@python3 -m flake8 --max-line-length=120 --exclude hooks/charmhelpers hooks
 	@charm proof
 
 bin/charm_helpers_sync.py:
@@ -12,3 +17,13 @@ bin/charm_helpers_sync.py:
 
 sync: bin/charm_helpers_sync.py
 	@$(PYTHON) bin/charm_helpers_sync.py -c charm-helpers-sync.yaml
+
+git:
+	git push $(LOGNAME)
+
+cspush: lint
+	version=`charm push . $(CSDEST) | awk '/^url:/ {print $$2}'` && \
+	    charm release $$version
+
+upgrade: cspush
+	juju upgrade-charm $(CHARM_NAME)
diff --git a/README.md b/README.md
index e635e7a..a2605cb 100644
--- a/README.md
+++ b/README.md
@@ -38,14 +38,13 @@ To disable the default list of pool servers, set that to the empty string:
 
 Sources, peers, and pools should be space separated.
 
-When you need a set of services to keep close time to each other, it may
-be useful to have them automatically peer with each other.  This means
-any set of services which use the same ntp subordinate will peer together.
+If you have a large number of nodes which need to keep close sync with one
+another but need to keep upstream traffic to a minimum, try auto_peers:
 
     juju set ntp auto_peers=true
 
-This will add all the hosts as peers to each other.  Using auto_peers is not
-recommended when more than 10 units are expected to be deployed.
+This will select the most suitable units for connecting with upstream, and
+configure the remaining units to receive time from those units.
 
 Mastered
 ========
diff --git a/config.yaml b/config.yaml
index 1405f4b..1bbc1ed 100644
--- a/config.yaml
+++ b/config.yaml
@@ -59,9 +59,14 @@ options:
     default: false
     type: boolean
     description: >
-        Automatically peer with other units in the same service.
-        DEPRECATED.  Please consider using the ntpmaster charm to provide
-        sufficient peers for your environment in favour of auto_peers.
+        Automatically select the most appropriate units in the service to
+        be a service stratum connecting with upstream NTP servers, and use
+        those units as time sources for the remaining units.
+  auto_peers_upstream:
+    default: 6
+    type: int
+    description: >
+        How many units should attempt to connect with upstream NTP servers?
   use_iburst:
     default: true
     type: boolean
diff --git a/files/nagios/check_ntpmon.py b/files/nagios/check_ntpmon.py
index 8d24df4..a3c64be 100755
--- a/files/nagios/check_ntpmon.py
+++ b/files/nagios/check_ntpmon.py
@@ -452,7 +452,7 @@ class NTPPeers(object):
             output = subprocess.check_output(["ntpq", "-pn"], stderr=null)
             if len(output) > 0:
                 lines = output.split("\n")
-        except:
+        except Exception:
             traceback.print_exc(file=sys.stdout)
         return lines
 
diff --git a/hooks/ntp_hooks.py b/hooks/ntp_hooks.py
index d0356c3..efde7ad 100755
--- a/hooks/ntp_hooks.py
+++ b/hooks/ntp_hooks.py
@@ -1,15 +1,14 @@
 #!/usr/bin/python3
 
-import sys
-import charmhelpers.core.hookenv as hookenv
-import charmhelpers.core.host as host
-import charmhelpers.fetch as fetch
-from charmhelpers.core.hookenv import UnregisteredHookError
+from charmhelpers.contrib.charmsupport import nrpe
 from charmhelpers.contrib.templating.jinja import render
-import shutil
+from charmhelpers.core import hookenv, host, unitdata
+import charmhelpers.fetch as fetch
 import os
+import shutil
+import sys
 
-from charmhelpers.contrib.charmsupport import nrpe
+import ntp_scoring
 
 NAGIOS_PLUGINS = '/usr/local/lib/nagios/plugins'
 
@@ -19,22 +18,56 @@ NTP_CONF_ORIG = '{}.orig'.format(NTP_CONF)
 hooks = hookenv.Hooks()
 
 
-def get_peer_nodes():
-    hosts = []
-    hosts.append(hookenv.unit_get('private-address'))
+def get_peer_sources(topN=6):
+    """
+    Get our score and put it on the peer relation.
+    Read the peer private addresses and their scores.
+    Determine whether we're in the top N scores;
+    if so, we're upstream - return None
+    Otherwise, return the list of the top N peers.
+    """
+    if topN is None:
+        topN = 6
+    ourscore = ntp_scoring.get_score()
+    if ourscore is None:
+        hookenv.log('[AUTO_PEER] Our score cannot be determined - check logs for reason')
+        return None
+
+    peers = []
     for relid in hookenv.relation_ids('ntp-peers'):
+        hookenv.relation_set(relation_id=relid, relation_settings={'score': ourscore['score']})
         for unit in hookenv.related_units(relid):
-            hosts.append(hookenv.relation_get('private-address',
-                         unit, relid))
-    hosts.sort()
-    return hosts
+            addr = hookenv.relation_get('private-address', unit, relid)
+            peerscore = hookenv.relation_get('score', unit, relid)
+            if peerscore is not None:
+                peers.append((addr, float(peerscore)))
+
+    if len(peers) < topN:
+        # we don't have enough peers to do auto-peering
+        hookenv.log('[AUTO_PEER] There are only {} peers; not doing auto-peering'.format(len(peers)))
+        return None
+
+    # list of hosts with scores better than ours
+    hosts = list(filter(lambda x: x[1] > ourscore['score'], peers))
+    hookenv.log('[AUTO_PEER] {} peers better than us, topN == {}'.format(len(hosts), topN))
+
+    # if the list is less than topN long, we're in the topN hosts
+    if len(hosts) < topN:
+        return None
+    else:
+        # sort list of hosts by score, keep only the topN
+        topNhosts = sorted(hosts, key=lambda x: x[1], reverse=True)[0:topN]
+        # return only the host addresses
+        return map(lambda x: x[0], topNhosts)
 
 
 @hooks.hook('install')
 def install():
     fetch.apt_update(fatal=True)
-    fetch.apt_install(["ntp"], fatal=True)
-    shutil.copy(NTP_CONF, NTP_CONF_ORIG)
+    ntp_scoring.install_packages()
+    if ntp_scoring.get_virt_type() != 'container':
+        fetch.apt_install(["ntp"], fatal=True)
+        shutil.copy(NTP_CONF, NTP_CONF_ORIG)
 
 
 def get_sources(sources, iburst=True, source_list=None):
@@ -61,25 +94,52 @@ def get_sources(sources, iburst=True, source_list=None):
             'ntp-peers-relation-changed')
 @host.restart_on_change({NTP_CONF: ['ntp']})
 def write_config():
+    ntp_scoring.install_packages()
+    if ntp_scoring.get_virt_type() == 'container':
+        host.service_stop('ntp')
+        host.service_pause('ntp')
+        hookenv.close_port(123, protocol="UDP")
+        return
+
+    host.service_resume('ntp')
     hookenv.open_port(123, protocol="UDP")
+
     use_iburst = hookenv.config('use_iburst')
-    source = hookenv.config('source')
     orphan_stratum = hookenv.config('orphan_stratum')
-    remote_sources = get_sources(source, iburst=use_iburst)
+    source = hookenv.config('source')
     pools = hookenv.config('pools')
-    remote_pools = get_sources(pools, iburst=use_iburst)
-    for relid in hookenv.relation_ids('master'):
-        for unit in hookenv.related_units(relid=relid):
-            u_addr = hookenv.relation_get(attribute='private-address',
-                                          unit=unit, rid=relid)
-            remote_sources.append({'name': u_addr, 'iburst': 'iburst'})
-
     peers = hookenv.config('peers')
-    remote_peers = get_sources(peers, iburst=use_iburst)
     auto_peers = hookenv.config('auto_peers')
-    if hookenv.relation_ids('ntp-peers') and auto_peers:
-        remote_peers = get_sources(get_peer_nodes(), iburst=use_iburst,
-                                   source_list=remote_peers)
+
+    remote_sources = get_sources(source, iburst=use_iburst)
+    remote_pools = get_sources(pools, iburst=use_iburst)
+    remote_peers = get_sources(peers, iburst=use_iburst)
+
+    kv = unitdata.kv()
+    hookenv.atexit(kv.flush)
+
+    if hookenv.relation_ids('master'):
+        # use master relation only
+        for relid in hookenv.relation_ids('master'):
+            for unit in hookenv.related_units(relid=relid):
+                u_addr = hookenv.relation_get(attribute='private-address',
+                                              unit=unit, rid=relid)
+                remote_sources.append({'name': u_addr, 'iburst': 'iburst'})
+    elif auto_peers and hookenv.relation_ids('ntp-peers'):
+        # use auto_peers
+        auto_peer_list = get_peer_sources(hookenv.config('auto_peers_upstream'))
+        if auto_peer_list is None:
+            # we are upstream - use configured sources, pools, peers
+            kv.set('auto_peer', 'upstream')
+        else:
+            # override all sources with auto_peer_list
+            kv.set('auto_peer', 'client')
+            remote_sources = get_sources(auto_peer_list, iburst=use_iburst)
+            remote_pools = []
+            remote_peers = []
+    else:
+        # use configured sources, pools, peers
+        kv.unset('auto_peer')
 
     if len(remote_sources) == 0 and len(remote_peers) == 0 and len(remote_pools) == 0:
         # we have no peers/pools/servers; restore default ntp.conf provided by OS
@@ -134,7 +194,7 @@ def update_nrpe_config():
 # Hyper-V host clock sync handling - workaround until https://bugs.launchpad.net/bugs/1676635 is SRUed for xenial
 # See also:
 # - https://patchwork.kernel.org/patch/9525945/
-# - 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
+# - 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
 _device_class = '9527e630-d0ae-497b-adce-e80ab0175caf'
 _vmbus_dir = '/sys/bus/vmbus/'
 
@@ -146,11 +206,11 @@ def find_hyperv_host_sync_device():
             try:
                 f = open(os.path.join(_vmbus_dir, 'devices', d, 'class_id'), 'r')
                 if _device_class in f.readline():
-                    hookenv.log('Hyper-V host time sync device is {}'.format(f.name), level=hookenv.DEBUG)
+                    hookenv.log('Hyper-V host time sync device is {}'.format(f.name))
                     return d
-            except:
+            except Exception:
                 pass
-    except:
+    except Exception:
         pass
     return None
 
@@ -164,13 +224,13 @@ def check_hyperv_host_sync(device_id):
             firstline = f.readline().strip()
             result = firstline == '3'
             enabled = 'enabled' if result else 'disabled'
-            hookenv.log('Hyper-V host time sync is ' + enabled, level=hookenv.DEBUG)
+            hookenv.log('Hyper-V host time sync is ' + enabled)
             if result:
                 return device_id
             else:
                 return None
-        except:
-            hookenv.log('Hyper-V host time sync status file {} not found'.format(statefile), level=hookenv.DEBUG)
+        except Exception:
+            hookenv.log('Hyper-V host time sync status file {} not found'.format(statefile))
             return None
     else:
         return None
@@ -179,12 +239,12 @@ def check_hyperv_host_sync(device_id):
 def disable_hyperv_host_sync(device_id):
     """Unbind the Hyper-V host clock sync driver"""
     try:
-        hookenv.log('Disabling Hyper-V host time sync', level=hookenv.DEBUG)
+        hookenv.log('Disabling Hyper-V host time sync')
         path = os.path.join(_vmbus_dir, 'drivers', 'hv_util', 'unbind')
         f = open(path, 'w')
         print(device_id, file=f)
         return True
-    except:
+    except Exception:
         return False
 
 
@@ -203,21 +263,43 @@ def hyperv_sync_status():
 
 @hooks.hook('update-status')
 def assess_status():
-    hookenv.application_version_set(
-        fetch.get_upstream_version('ntp')
-    )
-    if host.service_running('ntp'):
-        status = 'Unit is ready'
-        status_extra = hyperv_sync_status()
-        if status_extra:
-            status = status + '; ' + status_extra
-        hookenv.status_set('active', status)
+    version = fetch.get_upstream_version('ntp')
+    if version is not None:
+        hookenv.application_version_set(version)
+
+    # create base status
+    if ntp_scoring.get_virt_type() == 'container':
+        state = 'blocked'
+        status = 'NTP not supported in containers: please configure on host'
+    elif host.service_running('ntp'):
+        state = 'active'
+        status = 'Ready'
     else:
-        hookenv.status_set('blocked', 'ntp not running')
+        state = 'blocked'
+        status = 'Not running'
+
+    # append Hyper-V status, if any
+    hyperv_status = hyperv_sync_status()
+    if hyperv_status is not None:
+        status += ', ' + hyperv_status
+
+    # append scoring status, if any
+    # (don't force update of the score from update-status more than once a month)
+    max_age = 31 * 86400
+    scorestr = ntp_scoring.get_score_string(max_seconds=max_age)
+    if scorestr is not None:
+        status += ', ' + scorestr
+
+    # append auto_peer status
+    autopeer = unitdata.kv().get('auto_peer')
+    if autopeer is not None:
+        status += ' [{}]'.format(autopeer)
+
+    hookenv.status_set(state, status)
 
 
 if __name__ == '__main__':
     try:
         hooks.execute(sys.argv)
-    except UnregisteredHookError as e:
+    except hookenv.UnregisteredHookError as e:
         hookenv.log('Unknown hook {} - skipping.'.format(e))
diff --git a/hooks/ntp_scoring.py b/hooks/ntp_scoring.py
 new file mode 100755
index 0000000..af55103
--- /dev/null
+++ b/hooks/ntp_scoring.py
@@ -0,0 +1,159 @@
+
+# Copyright (c) 2017 Canonical Ltd
+# Author: Paul Gear
+
+# This module retrieves the score calculated in ntp_source_score, and
+# creates an overall node weighting based on the machine type (bare metal,
+# container, or VM) and software running locally.  It reduces the score
+# for nodes with OpenStack ceph, nova, or swift services running, in order
+# to decrease the likelihood that they will be selected as upstreams.
+
+from charmhelpers.core import hookenv, unitdata
+import charmhelpers.fetch as fetch
+import json
+import time
+
+import ntp_source_score
+
+
+def install_packages():
+    fetch.apt_install(["facter", "ntpdate", "python3-psutil", "virt-what"], fatal=False)
+
+
+def get_virt_type():
+    """Work out what type of environment we're running in"""
+    for line in ntp_source_score.run_cmd('facter virtual'):
+        fields = str(line).split()
+        if len(fields) > 0:
+            if fields[0] in ['physical', 'xen0']:
+                return 'physical'
+            if fields[0] in ['docker', 'lxc', 'openvz']:
+                return 'container'
+    # Anything not one of the above-mentioned types is assumed to be a VM
+    return 'vm'
+
+
+def get_virt_multiplier():
+    virt_type = get_virt_type()
+    if virt_type == 'container':
+        # containers should be synchronized from their host
+        return -1
+    elif virt_type == 'physical':
+        hookenv.log('[SCORE] running on physical host - score bump 25%')
+        return 1.25
+    else:
+        hookenv.log('[SCORE] probably running in a VM - score bump 0%')
+        return 1
+
+
+def get_package_divisor():
+    """Check for running ceph, swift, & nova-compute services,
+    and increase divisor for each."""
+    try:
+        import psutil
+    except Exception:
+        # If we can't read the process table, assume a worst-case.
+        # (Normally, if every process is running, this will return
+        # 1.1 * 1.1 * 1.25 * 1.25 = 1.890625.)
+        return 2
+
+    # set the weight for each process (regardless of how many there are running)
+    running = {}
+    for p in psutil.process_iter():
+        if p.name().startswith('nova-compute'):
+            running['nova-compute'] = 1.25
+        if p.name().startswith('ceph-osd'):
+            running['ceph-osd'] = 1.25
+        elif p.name().startswith('ceph-'):
+            running['ceph'] = 1.1
+        elif p.name().startswith('swift-'):
+            running['swift'] = 1.1
+
+    # increase the divisor for each discovered process type
+    divisor = 1
+    for r in running:
+        hookenv.log('[SCORE] %s running - score divisor %.3f' % (r, running[r]))
+        divisor *= running[r]
+    return divisor
+
+
+def check_score(seconds=None):
+    if seconds is None:
+        seconds = time.time()
+    score = {
+        'divisor': 1,
+        'multiplier': 0,
+        'score': -999,
+        'time': seconds,
+    }
+
+    # skip scoring if we have an explicitly configured master
+    relation_sources = hookenv.relation_ids('master')
+    score['master-relations'] = len(relation_sources)
+    if relation_sources is not None and len(relation_sources) > 0:
+        hookenv.log('[SCORE] master relation configured - skipped scoring')
+        return score
+
+    # skip scoring if we're in a container
+    multiplier = get_virt_multiplier()
+    score['multiplier'] = multiplier
+    if multiplier <= 0:
+        hookenv.log('[SCORE] running in a container - skipped scoring')
+        return score
+
+    # skip scoring if auto_peers is off
+    auto_peers = hookenv.config('auto_peers')
+    score['auto-peers'] = auto_peers
+    if not auto_peers:
+        hookenv.log('[SCORE] auto_peers is disabled - skipped scoring')
+        return score
+
+    # skip scoring if we have no sources
+    sources = hookenv.config('source').split()
+    peers = hookenv.config('peers').split()
+    pools = hookenv.config('pools').split()
+    host_list = sources + peers + pools
+    if len(host_list) == 0:
+        hookenv.log('[SCORE] No sources configured')
+        return score
+
+    # Now that we've passed all those checks, check upstreams, calculate a score, and return the result
+    divisor = get_package_divisor()
+    score['divisor'] = divisor
+    score['host-list'] = host_list
+    score['raw'] = ntp_source_score.get_source_score(host_list, verbose=True)
+    score['score'] = score['raw'] * multiplier / divisor
+    hookenv.log('[SCORE] Suitability score: %.3f' % (score['score'],))
+    return score
+
+
+def get_score(max_seconds=86400):
+    # Remove this if/when we convert the charm to reactive
+    kv = unitdata.kv()
+    hookenv.atexit(kv.flush)
+
+    score = kv.get('ntp_score')
+    if score is not None:
+        saved_time = score.get('time', 0)
+    else:
+        saved_time = 0
+
+    now = time.time()
+    if score is None or now - saved_time > max_seconds:
+        score = check_score(now)
+        kv.set('ntp_score', score)
+        hookenv.log('[SCORE] saved %s' % (json.dumps(score),))
+
+    return score
+
+
+def get_score_string(score=None, max_seconds=86400):
+    if score is None:
+        score = get_score(max_seconds)
+    if not hookenv.config('auto_peers') or 'raw' not in score:
+        return None
+    return 'score %.3f (%.1f) at %s' % (
+        score['score'],
+        score['multiplier'] / score['divisor'],
+        time.ctime(score['time'])
+    )
diff --git a/hooks/ntp_source_score.py b/hooks/ntp_source_score.py
 new file mode 100755
index 0000000..71b1cef
--- /dev/null
+++ b/hooks/ntp_source_score.py
@@ -0,0 +1,198 @@
+#!/usr/bin/python3
+
+# Copyright (c) 2017 Canonical Ltd
+# Author: Paul Gear
+
+# This module runs ntpdate in test mode against the provided list of sources
+# in order to determine this node's suitability as an NTP server, based on the
+# number of reachable sources, and the network delay in reaching them.  Up to
+# MAX_THREADS (default 32) threads will be spawned to run ntpdate, in order
+# to minimise the time taken to calculate a score.
+
+# A main method is included to allow this module to be called separately from
+# juju hooks for diagnostic purposes.  It has no dependencies on juju,
+# charmhelpers, or the other modules in this charm.
+
+import argparse
+import math
+import queue
+import random
+import statistics
+import subprocess
+import threading
+import time
+
+rand = random.SystemRandom()
+MAX_THREADS = 32
+
+
+def rms(l):
+    """Return the root mean square of the list"""
+    if len(l) > 0:
+        squares = [x ** 2 for x in l]
+        return math.sqrt(statistics.mean(squares))
+    else:
+        return float('nan')
+
+
+def run_cmd(cmd):
+    """Run the output, return a list of lines returned; ignore errors"""
+    lines = []
+    try:
+        output = subprocess.check_output(cmd.split(), stderr=subprocess.DEVNULL).decode('UTF-8')
+        lines = output.split('\n')
+    except Exception:
+        pass
+    return lines
+
+
+def get_source_delays(source):
+    """Run ntpdate on the source, which may resolve to multiple addresses;
+    return the list of delay values. This can take several seconds, depending
+    on bandwidth and distance of the sources."""
+    delays = []
+    cmd = 'ntpdate -d -t 0.2 ' + source
+    for line in run_cmd(cmd):
+        fields = line.split()
+        if len(fields) >= 2 and fields[0] == 'delay':
+            delay = float(fields[1].split(',')[0])
+            if delay > 0:
+                delays.append(delay)
+    return delays
+
+
+def worker(num, src, dst, debug=False):
+    """Thread worker for parallelising ntpdate runs.  Gets host name
+    from src queue and places host and delay list in dst queue."""
+    if debug:
+        print('[%d] Starting' % (num,))
+    while True:
+        host = src.get()
+        if host is None:
+            break
+
+        # lower-numbered threads sleep for a shorter time, on average
+        s = rand.random() * num / MAX_THREADS
+        if debug:
+            print('[%d] Sleeping %.3f' % (num, s))
+        time.sleep(s)
+
+        if debug:
+            print('[%d] Getting results for [%s]' % (num, host))
+        delays = get_source_delays(host)
+        src.task_done()
+        if len(delays):
+            result = (host, delays)
+            dst.put(result)
+
+
+def get_delay_score(delay):
+    """Take a delay in seconds and return a score.  Under most sane NTP setups
+    will return a value between 0 and 10, where 10 is better and 0 is worse."""
+    return -math.log(delay)
+
+
+def start_workers(threads, num_threads, src, dst, debug=False):
+    """Start all of the worker threads."""
+    for i in range(num_threads):
+        t = threading.Thread(target=worker, args=(i, src, dst, debug))
+        t.start()
+        threads.append(t)
+
+
+def stop_workers(threads, src):
+    """Send the workers a None object, causing them to stop work.
+    We enqueue one stop object for each worker."""
+    for i in range(len(threads)):
+        src.put(None)
+
+
+def calculate_score(delays):
+    """Return the rms, mean, standard deviation, and overall
+    score for the passed list of delay values."""
+    score = 0
+    if len(delays) > 0:
+        r = rms(delays)
+        m = statistics.mean(delays)
+        s = statistics.pstdev(delays, m)
+        source_score = get_delay_score(r)
+        score += source_score
+    else:
+        r = m = s = score = 0
+    return (r, m, s, score)
+
+
+def calculate_results(q, verbose=False):
+    """Get the scores for all the hosts.
+    Return a hash of hosts and their cumulative scores."""
+    results = {}
+    while not q.empty():
+        (host, delays) = q.get()
+        (rms, mean, stdev, score) = calculate_score(delays)
+        delaystrings = [str(x) for x in delays]
+        if verbose:
+            print('%s score=%.3f rms=%.3f mean=%.3f stdevp=%.3f [%s]' %
+                  (host, score, rms, mean, stdev, ", ".join(delaystrings)))
+        if host in results:
+            results[host] += score
+        else:
+            results[host] = score
+    return results
+
+
+def wait_workers(threads):
+    """Wait for the given list of threads to complete."""
+    for t in threads:
+        t.join()
+
+
+def run_checks(hosts, debug=False, numthreads=None, verbose=False):
+    """Perform a check of the listed hosts.
+    Can take several seconds per host."""
+    sources = queue.Queue()
+    results = queue.Queue()
+    threads = []
+    for h in hosts:
+        sources.put(h)
+    if numthreads is None:
+        numthreads = min(len(hosts), MAX_THREADS)
+    start_workers(threads, numthreads, sources, results, debug)
+    sources.join()
+    stop_workers(threads, sources)
+    # wait_workers(threads)
+    return calculate_results(results, verbose)
+
+
+def get_source_score(hosts, debug=False, numthreads=None, verbose=False):
+    """Check NTP connectivity to the given list of sources - return a single overall score"""
+    results = run_checks(hosts, debug, numthreads, verbose)
+    if results is None:
+        return 0
+
+    total = 0
+    for host in results:
+        total += results[host]
+    return total
+
+
+def display_results(results):
+    """Sort the hash by value.  Print the results."""
+    # http://stackoverflow.com/a/2258273
+    result = sorted(results.items(), key=lambda x: x[1], reverse=True)
+    for i in result:
+        print("%s %.3f" % (i[0], i[1]))
+
+
+def get_args():
+    parser = argparse.ArgumentParser(description='Get NTP server/peer/pool scores')
+    parser.add_argument('--debug', '-d', action='store_true', help='Enable thread debug output')
+    parser.add_argument('--verbose', '-v', action='store_true', help='Display scoring detail')
+    parser.add_argument('hosts', nargs=argparse.REMAINDER, help='List of hosts to check')
+    return parser.parse_args()
+
+
+if __name__ == '__main__':
+    args = get_args()
+    results = run_checks(args.hosts, debug=args.debug, verbose=args.verbose)
+    if results:
+        display_results(results)
diff --git a/tests/10-deploy-test.py b/tests/10-deploy-test.py
index db65f8c..efa5d99 100755
--- a/tests/10-deploy-test.py
+++ b/tests/10-deploy-test.py
@@ -39,7 +39,7 @@ except amulet.helpers.TimeoutError:
     message = 'The environment did not setup in %d seconds.' % seconds
     # The SKIP status enables skip or fail the test based on configuration.
     amulet.raise_status(amulet.SKIP, msg=message)
-except:
+except Exception:
     raise
 
 # Unable to get the sentry unit for ntp because it is a subordinate.
diff --git a/unit_tests/test_ntp_hooks.py b/unit_tests/test_ntp_hooks.py
 new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/unit_tests/test_ntp_hooks.py
diff --git a/unit_tests/test_ntp_scoring.py b/unit_tests/test_ntp_scoring.py
 new file mode 100644
index 0000000..5840252
--- /dev/null
+++ b/unit_tests/test_ntp_scoring.py
@@ -0,0 +1,106 @@
+#!/usr/bin/env python3
+
+from random import shuffle
+from unittest.mock import Mock, patch
+import unittest
+
+import ntp_scoring
+
+
+class TestNtpScoring(unittest.TestCase):
+
+    def setUp(self):
+        patcher = patch('charmhelpers.core.hookenv.log')
+        patcher.start()
+        self.addCleanup(patcher.stop)
+        patcher = patch('charmhelpers.core.unitdata.kv')
+        patcher.start()
+        self.addCleanup(patcher.stop)
+
+    @patch('ntp_source_score.run_cmd')
+    def testGetVirtTypeValues(self, run_cmd):
+        def virt_test(expected, return_value):
+            run_cmd.return_value = [return_value]
+            self.assertEqual(ntp_scoring.get_virt_type(), expected)
+            run_cmd.assert_called_once_with('facter virtual')
+            run_cmd.reset_mock()
+
+        virt_test('container', 'docker')
+        virt_test('container', 'lxc')
+        virt_test('container', 'openvz')
+        virt_test('physical', 'physical')
+        virt_test('physical', 'xen0')
+        virt_test('vm', '')
+        virt_test('vm', [])
+        virt_test('vm', 1.23)
+        virt_test('vm', 'a')
+        virt_test('vm', 'kvm')
+        virt_test('vm', None)
+        virt_test('vm', 'something-else')
+        virt_test('vm', 'The quick brown fox jumps over the lazy dogs')
+
+    @patch('ntp_source_score.run_cmd')
+    def testGetVirtTypeEmptyList(self, run_cmd):
+        run_cmd.return_value = []
+        self.assertEqual(ntp_scoring.get_virt_type(), 'vm')
+        run_cmd.assert_called_once_with('facter virtual')
+
+    @patch('ntp_source_score.run_cmd')
+    def testGetVirtTypeWrongType(self, run_cmd):
+        run_cmd.return_value = {}
+        self.assertEqual(ntp_scoring.get_virt_type(), 'vm')
+        run_cmd.assert_called_once_with('facter virtual')
+
+    @patch('ntp_source_score.run_cmd')
+    def testGetVirtMultiplier(self, run_cmd):
+        def multiplier_test(expected, return_value):
+            run_cmd.return_value = [return_value]
+            self.assertEqual(ntp_scoring.get_virt_multiplier(), expected)
+            run_cmd.assert_called_once_with('facter virtual')
+            run_cmd.reset_mock()
+
+        multiplier_test(-1, 'docker')
+        multiplier_test(-1, 'lxc')
+        multiplier_test(-1, 'openvz')
+        multiplier_test(1.25, 'physical')
+        multiplier_test(1.25, 'xen0')
+        multiplier_test(1, '')
+        multiplier_test(1, [])
+        multiplier_test(1, 1.23)
+        multiplier_test(1, 'a')
+        multiplier_test(1, 'kvm')
+        multiplier_test(1, None)
+        multiplier_test(1, 'something-else')
+        multiplier_test(1, 'The quick brown fox jumps over the lazy dogs')
+
+    def testGetPackageDivisor(self):
+
+        def test_divisor(expected, pslist, precision=6):
+            def fake_pslist():
+                """yield a list of objects for which name() returns the given list"""
+                shuffle(pslist)
+                for p in pslist:
+                    m = Mock()
+                    m.name.return_value = p
+                    yield m
+
+            with patch('psutil.process_iter', side_effect=fake_pslist):
+                divisor = round(ntp_scoring.get_package_divisor(), precision)
+                self.assertEqual(round(expected, precision), divisor)
+
+        with self.assertRaises(TypeError):
+            test_divisor(1, None)
+
+        test_divisor(1, [])
+        test_divisor(1, ['a', 'b', 'c'])
+        test_divisor(1, 'The quick brown fox jumps over the lazy dogs'.split())
+        test_divisor(1.1, 'The quick brown fox jumps over the lazy dogs'.split() + ['swift-1'])
+        test_divisor(1.1, ['swift-1'])
+        test_divisor(1.1, ['ceph-1', 'ceph-2'])
+        test_divisor(1.25, ['ceph-osd-1', 'ceph-osd-2', 'ceph-osd-3'])
+        test_divisor(1.25, ['nova-compute-1', 'nova-compute-2', 'nova-compute-3', 'nova-compute-4'])
+        test_divisor(1.1 * 1.25, ['swift-1', 'nova-compute-2'])
+        test_divisor(1.1 * 1.25, ['systemd', 'bind', 'swift-1', 'nova-compute-2', 'test'])
+        test_divisor(1.1 * 1.25 * 1.1, ['swift-1', 'nova-compute-2', 'ceph-3'])
+        test_divisor(1.1 * 1.25 * 1.25, ['swift-1', 'nova-compute-2', 'ceph-osd-3'])
+        test_divisor(1.1 * 1.25 * 1.1 * 1.25, ['swift-1', 'nova-compute-2', 'ceph-3', 'ceph-osd-4'])
diff --git a/unit_tests/test_ntp_source_score.py b/unit_tests/test_ntp_source_score.py
 new file mode 100644
index 0000000..378faa0
--- /dev/null
+++ b/unit_tests/test_ntp_source_score.py
@@ -0,0 +1,167 @@
+#!/usr/bin/env python3
+
+from unittest.mock import patch
+import math
+import unittest
+
+from ntp_source_score import (
+    get_delay_score,
+    get_source_delays,
+    rms,
+    run_cmd,
+)
+
+ntpdate_output = """
+...
+reference time:    dda179ee.3ec34fdd  Mon, Oct 30 2017 20:14:06.245
+originate timestamp: dda17a5b.af7c528b  Mon, Oct 30 2017 20:15:55.685
+transmit timestamp:  dda17a5b.80b4dc04  Mon, Oct 30 2017 20:15:55.502
+filter delay:  0.54126  0.36757  0.36655  0.36743 
+         0.00000  0.00000  0.00000  0.00000 
+filter offset: 0.099523 0.012978 0.011831 0.011770
+         0.000000 0.000000 0.000000 0.000000
+delay 0.36655, dispersion 0.01126
+offset 0.011831
+...
+reference time:    dda17695.69e65b2f  Mon, Oct 30 2017 19:59:49.413
+originate timestamp: dda17a5b.afcec2dd  Mon, Oct 30 2017 20:15:55.686
+transmit timestamp:  dda17a5b.80bb2488  Mon, Oct 30 2017 20:15:55.502
+filter delay:  0.36520  0.36487  0.36647  0.36604 
+         0.00000  0.00000  0.00000  0.00000 
+filter offset: 0.012833 0.013758 0.013731 0.013629
+         0.000000 0.000000 0.000000 0.000000
+delay 0.36487, dispersion 0.00049
+offset 0.013758
+...
+reference time:    dda1782c.6aec9646  Mon, Oct 30 2017 20:06:36.417
+originate timestamp: dda17a5b.d2d04ef4  Mon, Oct 30 2017 20:15:55.823
+transmit timestamp:  dda17a5b.b37c4098  Mon, Oct 30 2017 20:15:55.701
+filter delay:  0.28581  0.28406  0.28551  0.28596 
+         0.00000  0.00000  0.00000  0.00000 
+filter offset: -0.00802 -0.00854 -0.00791 -0.00787
+         0.000000 0.000000 0.000000 0.000000
+delay 0.28406, dispersion 0.00050
+offset -0.008544
+...
+reference time:    dda17735.4a03e3ca  Mon, Oct 30 2017 20:02:29.289
+originate timestamp: dda17a5c.1634d231  Mon, Oct 30 2017 20:15:56.086
+transmit timestamp:  dda17a5b.e6934fad  Mon, Oct 30 2017 20:15:55.900
+filter delay:  0.37044  0.37077  0.37050  0.37086 
+         0.00000  0.00000  0.00000  0.00000 
+filter offset: 0.013993 0.013624 0.013425 0.013362
+         0.000000 0.000000 0.000000 0.000000
+delay 0.37044, dispersion 0.00046
+offset 0.013993
+...
+reference time:    dda17695.69e65b2f  Mon, Oct 30 2017 19:59:49.413
+originate timestamp: dda17a5c.4944bb52  Mon, Oct 30 2017 20:15:56.286
+transmit timestamp:  dda17a5c.19cf5199  Mon, Oct 30 2017 20:15:56.100
+filter delay:  0.36873  0.36823  0.36911  0.36781 
+         0.00000  0.00000  0.00000  0.00000 
+filter offset: 0.014635 0.014599 0.014166 0.014239
+         0.000000 0.000000 0.000000 0.000000
+delay 0.36781, dispersion 0.00026
+offset 0.014239
+...
+reference time:    dda179ee.3ec34fdd  Mon, Oct 30 2017 20:14:06.245
+originate timestamp: dda17a5c.7bbd3828  Mon, Oct 30 2017 20:15:56.483
+transmit timestamp:  dda17a5c.4cf92e99  Mon, Oct 30 2017 20:15:56.300
+filter delay:  0.36554  0.36617  0.36673  0.36618 
+         0.00000  0.00000  0.00000  0.00000 
+filter offset: 0.012466 0.012691 0.012863 0.012346
+         0.000000 0.000000 0.000000 0.000000
+delay 0.36554, dispersion 0.00018
+offset 0.012466
+...
+"""
+ntpdate_delays = [0.36655, 0.36487, 0.28406, 0.37044, 0.36781, 0.36554]
+
+
+class TestNtpSourceScore(unittest.TestCase):
+
+    def test_rms(self):
+        self.assertEqual(rms([0, 0, 0, 0, 0]), 0)
+        self.assertEqual(rms([0, 1, 0, 1, 0]), math.sqrt(0.4))
+        self.assertEqual(rms([1, 1, 1, 1, 1]), 1)
+        self.assertEqual(rms([1, 2, 3, 4, 5]), math.sqrt(11))
+        self.assertEqual(rms([0.01, 0.02]), math.sqrt(0.00025))
+        self.assertEqual(rms([0.02766, 0.0894, 0.02657, 0.02679]), math.sqrt(0.00254527615))
+        self.assertEqual(rms([80, 50, 30]), math.sqrt(3266.66666666666666667))
+        self.assertEqual(rms([81, 53, 32]), math.sqrt(3464.66666666666666667))
+        self.assertEqual(rms([81.1, 53.9, 32.3]), math.sqrt(3508.57))
+        self.assertEqual(rms([81.14, 53.93, 32.30]), math.sqrt(3511.8115))
+        self.assertEqual(rms([81.141, 53.935, 32.309]), math.sqrt(3512.23919566666666667))
+        self.assertTrue(math.isnan(rms([])))
+        with self.assertRaises(TypeError):
+            rms(['a', 'b', 'c'])
+
+    @patch('subprocess.check_output')
+    def test_run_cmd(self, patched):
+        patched.return_value = b'a\nb\nc\n'
+        self.assertEqual(run_cmd('ls'), ['a', 'b', 'c', ''])
+
+        patched.return_value = b'4.13.0-14-generic\n'
+        self.assertEqual(run_cmd('uname -r'), ['4.13.0-14-generic', ''])
+
+        self.assertEqual(patched.call_count, 2)
+
+    def test_get_source_delays(self):
+
+        @patch('ntp_source_score.run_cmd')
+        def test_source_delay(data, expect, patched):
+            patched.return_value = data
+            self.assertEqual(get_source_delays('ntp.example.com'), expect)
+            patched.assert_called_once_with('ntpdate -d -t 0.2 ntp.example.com')
+
+        @patch('ntp_source_score.run_cmd')
+        def test_source_delay_error(data, e, patched):
+            patched.return_value = data
+            with self.assertRaises(e):
+                get_source_delays('ntp.example.com')
+            patched.assert_called_once_with('ntpdate -d -t 0.2 ntp.example.com')
+
+        test_source_delay([], [])
+        test_source_delay('', [])
+        test_source_delay('123', [])
+        test_source_delay(['123 678', '234 asdf', 'yaled 345 901'], [])
+        test_source_delay(['123 678', 'delay 345 901', '234 asdf'], [345])
+        test_source_delay(['delay 123 678', 'delay 234 asdf', 'delay 345 901'], [123, 234, 345])
+        test_source_delay(ntpdate_output.split('\n'), ntpdate_delays)
+
+        test_source_delay_error(None, TypeError)
+        test_source_delay_error(123, TypeError)
+
+    def test_get_delay_score_error(self):
+        # You can't have a negative or zero response time
+        with self.assertRaises(ValueError):
+            get_delay_score(-100)
+        with self.assertRaises(ValueError):
+            get_delay_score(-1)
+        with self.assertRaises(ValueError):
+            get_delay_score(-0.1)
+        with self.assertRaises(ValueError):
+            get_delay_score(0)
+
+    def test_get_delay_scores(self):
+        scores = [
+            get_delay_score(0.001),     # 1ms delay
+            get_delay_score(0.01),
+            get_delay_score(0.025),
+            get_delay_score(0.05),
+            get_delay_score(0.1),
+            get_delay_score(0.333),     # anything beyond this should never happen
+            get_delay_score(0.999),
+            get_delay_score(1),
+            get_delay_score(3),         # 3s delay - probably on the moon
+            get_delay_score(10),
+            get_delay_score(9999),      # 2.79h delay - are you orbiting Saturn or something?
+        ]
+
+        for i in range(len(scores)):
+            # all lower delays should get a higher score
+            for higher in range(i):
+                self.assertLess(scores[i], scores[higher])
+            # all higher delays should get a lower score
+            if i < len(scores) - 1:
+                for lower in range(i + 1, len(scores)):
+                    self.assertGreater(scores[i], scores[lower])