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

[Paul Gear]

[Haw Loeung (hloeung)]

LGTM, +1

review: Approve

diff --git a/README.md b/README.md
index 59ee007..e635e7a 100644
--- a/README.md
+++ b/README.md
@@ -7,23 +7,23 @@ across computers.
 Usage
 -----
 
-The ntp charm is a subordinate charm that is design for use with other
-principle charms.  It can be used in two ways:
+The ntp charm is a subordinate charm which is designed for use with other
+principal charms.  It can be used in two ways:
 
 Standalone
 ==========
 
-In this mode the ntp charm is used to configure NTP in other service units to
+In this mode, the ntp charm is used to configure NTP in other service units to
 talk directly to a set of NTP time sources:
 
     juju deploy ntp
     juju add-relation ntp myservice
 
-By default this is the standard set of NTP pool servers that are configured in
-the ntp Ubuntu package.
+By default this charm uses the standard set of NTP pool servers which are
+configured in the ntp Ubuntu package.
 
 However, if you have a handy atomic clock on your network which you would prefer
-to trust then you can use that instead:
+to use then you can add that:
 
     juju set ntp source=myatomiclock.local.net
 
@@ -32,21 +32,26 @@ You can also specify multiple sources and peers:
     juju set ntp source="mac1.local.net mac2.local.net mac3.local.net"
     juju set ntp peers="mac1.local.net mac2.local.net mac3.local.net"
 
-Sources and peers should be space separated.
+To disable the default list of pool servers, set that to the empty string:
+
+    juju set ntp pools=""
+
+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 that use the same ntp subordinate will peer together.
+any set of services which use the same ntp subordinate will peer together.
 
     juju set ntp auto_peers=true
 
-This will add all the hosts as peers to each other.
+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.
 
 Mastered
 ========
 
 In the event that you don't wish every server on your network to talk directly to
-your trusted time sources, you can use this charm in-conjunction with the ntpmaster
+your configured time sources, you can use this charm in-conjunction with the ntpmaster
 charm:
 
     juju deploy ntp
@@ -59,7 +64,7 @@ This might have application in more secure network environments where general
 outbound network access to the Internet is not avaliable or desirable and you don't
 have a good internal time source such as an atomic clock.
 
-You can of course have more that one ntpmaster:
+You can of course have more than one ntpmaster:
 
     juju add-unit ntpmaster
 
@@ -67,3 +72,19 @@ All services that the ntp charm is subordinate to will be configured to sync wit
 all avaliable masters.
 
 The ntpmaster charm supports the same "source" configuration that the ntp charm does.
+
+The ntp charm can also be used in place of the ntpmaster charm when coupled with
+another primary charm:
+
+    juju deploy ntp ntp-masters
+    juju deploy ubuntu --num-units=4
+    juju add-relation ubuntu ntp-masters
+    juju set ntp-masters auto_peers=true
+
+    juju deploy ntp
+    juju deploy my-other-service --num-units=30
+    juju add-relation ntp my-other-service
+    juju add-relation ntp ntp-masters
+
+This is the recommended method, since this charm provides better monitoring and
+more flexible configuration than ntpmaster.
diff --git a/config.yaml b/config.yaml
index d9c9680..1405f4b 100644
--- a/config.yaml
+++ b/config.yaml
@@ -10,6 +10,18 @@ options:
     description: >
         Space-separated list of NTP servers to use as peers for time.
         Peers are allowed to query the local NTP server via ntpq.
+  pools:
+    default: >
+        0.ubuntu.pool.ntp.org
+        1.ubuntu.pool.ntp.org
+        2.ubuntu.pool.ntp.org
+        3.ubuntu.pool.ntp.org
+        ntp.ubuntu.com
+    type: string
+    description: >
+        Space-separated list of NTP servers to use as pool sources for
+        time.  Pool sources are recommended over normal sources for their
+        self-healing capabilities. Leave empty to disable pool sources.
   orphan_stratum:
     default: 6
     type: int
@@ -17,8 +29,8 @@ options:
         The NTP stratum at which ntp most lose connectivity to before it 
         considers itself orphaned, and starts communicating with local peers.
         The default value of 6 will enable orphaned operation when there are
-        no stratum 6 servers or servers of a higher stratum available, which is
-        two stratum below most internet NTP hosts.
+        no stratum 6 servers or servers of a higher stratum available, which
+        is two strata below most Internet NTP hosts.
         Set to 0 to disable orphan mode entirely.
   nagios_context:
     default: "juju"
diff --git a/hooks/ntp_hooks.py b/hooks/ntp_hooks.py
index 2a36c8c..61ba2ec 100755
--- a/hooks/ntp_hooks.py
+++ b/hooks/ntp_hooks.py
@@ -43,7 +43,7 @@ def get_sources(sources, iburst=True, source_list=None):
     if sources:
         # allow both strings and lists
         if isinstance(sources, str):
-            sources = sources.split(" ")
+            sources = sources.split()
         for s in sources:
             if len(s) > 0:
                 if iburst:
@@ -66,6 +66,8 @@ def write_config():
     source = hookenv.config('source')
     orphan_stratum = hookenv.config('orphan_stratum')
     remote_sources = get_sources(source, iburst=use_iburst)
+    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',
@@ -79,8 +81,8 @@ def write_config():
         remote_peers = get_sources(get_peer_nodes(), iburst=use_iburst,
                                    source_list=remote_peers)
 
-    if len(remote_sources) == 0 and len(remote_peers) == 0:
-        # we have no peers/servers; restore default ntp.conf provided by OS
+    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
         shutil.copy(NTP_CONF_ORIG, NTP_CONF)
     else:
         # otherwise, write our own configuration
@@ -88,6 +90,7 @@ def write_config():
             ntpconf.write(render(os.path.basename(NTP_CONF), {
                 'orphan_stratum': orphan_stratum,
                 'peers': remote_peers,
+                'pools': remote_pools,
                 'servers': remote_sources,
             }))
 
@@ -102,7 +105,7 @@ def update_nrpe_config():
     # 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').split(" ")
+    nagios_ntpmon_checks = hookenv.config('nagios_ntpmon_checks').split()
     if os.path.isdir(NAGIOS_PLUGINS):
         host.rsync(os.path.join(os.getenv('CHARM_DIR'), 'files', 'nagios',
                    'check_ntpmon.py'),
@@ -110,7 +113,7 @@ def update_nrpe_config():
 
     hostname = nrpe.get_nagios_hostname()
     current_unit = nrpe.get_nagios_unit_name()
-    nrpe_setup = nrpe.NRPE(hostname=hostname)
+    nrpe_setup = nrpe.NRPE(hostname=hostname, primary=False)
 
     allchecks = set(['offset', 'peers', 'reachability', 'sync'])
 
@@ -119,22 +122,12 @@ def update_nrpe_config():
     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",
-            description='Check NTPmon {}'.format(current_unit),
-            check_cmd='check_ntpmon.py'
-        )
-    else:
-        nrpe_setup.add_check(
-            shortname="ntpmon",
-            description='Check NTPmon {}'.format(current_unit),
-            check_cmd=('check_ntpmon.py --checks ' +
-                       ' '.join(nagios_ntpmon_checks))
-        )
-
+    nrpe_setup.add_check(
+        shortname="ntpmon",
+        description='Check NTPmon {}'.format(current_unit),
+        check_cmd=('check_ntpmon.py --checks ' +
+                   ' '.join(nagios_ntpmon_checks))
+    )
     nrpe_setup.write()
 
 
diff --git a/templates/ntp.conf b/templates/ntp.conf
index c28b8d5..edc36a9 100644
--- a/templates/ntp.conf
+++ b/templates/ntp.conf
@@ -22,3 +22,6 @@ orphan tos {{ orphan_stratum }}
 # PEERS
 {% for peer in peers %}peer {{ peer.name }} {{ peer.iburst }}
 {% endfor %}
+# POOLS
+{% for pool in pools %}pool {{ pool.name }} {{ pool.iburst }}
+{% endfor %}