Merge lp:~paulgear/charms/trusty/ntp/sync-canonical-is-charms into lp:charms/trusty/ntp

[Paul Gear]

This has the following features/fixes:

- The use_iburst and auto_peers config options are now true by default. In environments which perform outbound UDP rate-limiting, it is recommended to change this to false.

- If all Nagios checks are enabled, they are merged into a single check, reducing noise and increasing efficiency.

- Should fix https://bugs.launchpad.net/charms/+source/ntp/+bug/1480564 due to consolidated handling of peers & sources in hook code.

This eliminates the differences in canonical-is-charms and will allow us to use the trunk charm.

[Review Queue (review-queue)]

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-lxc/1392/

review: Needs Fixing (automated testing)

[lp:~paulgear/charms/trusty/ntp/sync-canonical-is-charms]

29. By Paul Gear on 2015-11-16

Run of make lint with vanilla flake8 config

[Review Queue (review-queue)]

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-lxc/1448/

review: Needs Fixing (automated testing)

[Review Queue (review-queue)]

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-aws/1433/

review: Needs Fixing (automated testing)

[Stuart Bishop (stub)]

This all looks good and makes sense.

The test suite is all green at http://reports.vapour.ws/all-bundle-and-charm-results/ntp

Will land.

review: Approve

=== modified file 'config.yaml'
--- config.yaml	2015-03-27 07:09:38 +0000
+++ config.yaml	2015-11-16 04:00:52 +0000
@@ -17,7 +17,7 @@
           juju-myservice-0
       If you're running multiple environments with the same services in them
       this allows you to differentiate between them.
-  nagios_servicegroups: 
+  nagios_servicegroups:
     default: ""
     type: string
     description: |
@@ -27,13 +27,14 @@
     default: "offset peers reachability sync"
     type: string
     description: |
-        A space-seperated list of nagios ntpmon checks to enable.
-        If left empty, no ntpmon checks will be used.
+        A space-separated list of nagios ntpmon checks to enable.
+        Options are "offset peers reachability sync"; if all are specified,
+        they are combined into a single check; leave empty for no checks.
   auto_peers:
-    default: false
+    default: true
     type: boolean
     description: Will ntp auto peer between the environment
   use_iburst:
-    default: false
+    default: true
     type: boolean
     description: Use iburst for all servers, not just ntpmaster

=== modified file 'hooks/ntp_hooks.py'
--- hooks/ntp_hooks.py	2015-08-31 01:39:26 +0000
+++ hooks/ntp_hooks.py	2015-11-16 04:00:52 +0000
@@ -37,6 +37,22 @@
     shutil.copy(NTP_CONF, NTP_CONF_ORIG)
 
 
+def get_sources(sources, iburst=True, source_list=None):
+    if source_list is None:
+        source_list = []
+    if sources:
+        # allow both strings and lists
+        if isinstance(sources, basestring):
+            sources = sources.split(" ")
+        for s in sources:
+            if len(s) > 0:
+                if iburst:
+                    source_list.append({'name': s, 'iburst': 'iburst'})
+                else:
+                    source_list.append({'name': s, 'iburst': ''})
+    return source_list
+
+
 @hooks.hook('upgrade-charm')
 @hooks.hook('config-changed')
 @hooks.hook('master-relation-changed')
@@ -47,40 +63,30 @@
 def write_config():
     use_iburst = hookenv.config('use_iburst')
     source = hookenv.config('source')
-    remote_sources = []
-    if source:
-        for s in source.split(" "):
-            if len(s) > 0:
-                if use_iburst:
-                    remote_sources.append({'name': '%s iburst' % s})
-                else:
-                    remote_sources.append({'name': s})
+    remote_sources = get_sources(source, 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': '%s iburst' % u_addr})
+            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')
-    peers = hookenv.config('peers')
-    remote_peers = []
-    if peers:
-        for p in peers.split(" "):
-            if len(p) > 0:
-                remote_peers.append(p)
-    if hookenv.relation_ids('ntp-peers'):
-        if auto_peers:
-            for rp in get_peer_nodes():
-                remote_peers.append(rp)
+    if hookenv.relation_ids('ntp-peers') and auto_peers:
+        remote_peers = get_sources(get_peer_nodes(), iburst=use_iburst,
+                                   source_list=remote_peers)
 
-    if len(remote_sources) == 0:
+    if len(remote_sources) == 0 and len(remote_peers) == 0:
+        # we have no peers/servers; restore default ntp.conf provided by OS
         shutil.copy(NTP_CONF_ORIG, NTP_CONF)
     else:
+        # otherwise, write our own configuration
         with open(NTP_CONF, "w") as ntpconf:
             ntpconf.write(render(os.path.basename(NTP_CONF),
                                  {'servers': remote_sources,
                                   'peers': remote_peers,
-                                  'use_iburst': use_iburst}))
+                                  }))
 
     if hookenv.relation_ids('nrpe-external-master'):
         update_nrpe_config()
@@ -92,23 +98,40 @@
     # python-dbus is used by check_upstart_job
     # python-psutil is used by check_ntpmon
     fetch.apt_install(['python-dbus', 'python-psutil'])
-    nagios_ntpmon_checks = hookenv.config('nagios_ntpmon_checks')
+    nagios_ntpmon_checks = hookenv.config('nagios_ntpmon_checks').split(" ")
     if os.path.isdir(NAGIOS_PLUGINS):
-        if nagios_ntpmon_checks:
-            host.rsync(os.path.join(os.getenv('CHARM_DIR'), 'files', 'nagios',
-                       'check_ntpmon.py'),
-                       os.path.join(NAGIOS_PLUGINS, 'check_ntpmon.py'))
+        host.rsync(os.path.join(os.getenv('CHARM_DIR'), 'files', 'nagios',
+                   'check_ntpmon.py'),
+                   os.path.join(NAGIOS_PLUGINS, 'check_ntpmon.py'))
 
     hostname = nrpe.get_nagios_hostname()
     current_unit = nrpe.get_nagios_unit_name()
     nrpe_setup = nrpe.NRPE(hostname=hostname)
     nrpe.add_init_service_checks(nrpe_setup, ['ntp'], current_unit)
-    for nc in nagios_ntpmon_checks.split(" "):
+
+    allchecks = set(['offset', 'peers', 'reachability', 'sync'])
+
+    # remove any previously-created ntpmon checks
+    nrpe_setup.remove_check(shortname="ntpmon")
+    for c in allchecks:
+        nrpe_setup.remove_check(shortname="ntpmon_%s" % c)
+
+    # If all checks are specified, combine them into a single check to reduce
+    # Nagios noise.
+    if set(nagios_ntpmon_checks) == allchecks:
         nrpe_setup.add_check(
-            shortname="ntpmon_%s" % nc,
-            description='Check NTPmon %s {%s}' % (nc, current_unit),
-            check_cmd='check_ntpmon.py --check %s' % nc
+            shortname="ntpmon",
+            description='Check NTPmon {}'.format(current_unit),
+            check_cmd='check_ntpmon.py'
         )
+    else:
+        for nc in nagios_ntpmon_checks:
+            if len(nc) > 0:
+                nrpe_setup.add_check(
+                    shortname="ntpmon_%s" % nc,
+                    description='Check NTPmon %s {%s}' % (nc, current_unit),
+                    check_cmd='check_ntpmon.py --check %s' % nc
+                )
 
     nrpe_setup.write()
 

=== modified file 'templates/ntp.conf'
--- templates/ntp.conf	2015-03-16 07:16:25 +0000
+++ templates/ntp.conf	2015-11-16 04:00:52 +0000
@@ -8,11 +8,11 @@
 restrict -6 default kod notrap nomodify nopeer noquery
 restrict 127.0.0.1
 restrict ::1
-{% for peer in peers %}restrict {{ peer }} kod notrap nomodify
+{% for peer in peers %}restrict {{ peer.name }} kod notrap nomodify
 {% endfor %}
 # SERVERS
-{% for server in servers %}server {{ server.name }}
+{% for server in servers %}server {{ server.name }} {{ server.iburst }}
 {% endfor %}
 # PEERS
-{% for peer in peers %}peer {{ peer }} {% if use_iburst %}iburst{% endif %}
+{% for peer in peers %}peer {{ peer.name }} {{ peer.iburst }}
 {% endfor %}