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

[Paul Gear]

Add basic chrony support to ntp charm in preparation for bionic release. The Nagios check does not currently work with chrony.

First cut at adding chrony support to ntp charm in preparation for bionic release.

[Seth Arnold (seth-arnold)]

Nice changes, thanks

[Stuart Bishop (stub)]

Looks good.

I do strongly believe that the ntp_implementation config option should be dropped, as its only use case is to create broken or unsupported deployments. The function in ntp_implementation.py can keep its auto mode, since it is really external/shared code rather than part of the charm, but the charm should always call it with 'auto'. Not only does a foot-gun option not encode best practice like the charm should, but it will also cause problems in a few years when we drop Xenial support and the NTP code along with it.

review: Approve

[Paul Gear (paulgear)]

That all makes good sense to me; will push a new rev shortly.

[Stuart Bishop (stub)]

+1

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Unable to determine commit message from repository - please click "Set commit message" and enter the commit message manually.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision ff753b4081d20cbf0ddc2fabf3a6b933a3006d23

diff --git a/config.yaml b/config.yaml
index 1bbc1ed..993673d 100644
--- a/config.yaml
+++ b/config.yaml
@@ -3,13 +3,13 @@ options:
     default: ""
     type: string
     description: >
-        Space-separated list of NTP servers to use as sources for time.
+        Space-separated list of NTP servers to use as time sources.
   peers:
     default: ""
     type: string
     description: >
-        Space-separated list of NTP servers to use as peers for time.
-        Peers are allowed to query the local NTP server via ntpq.
+        Space-separated list of NTP servers to use as peers.  Under ntpd,
+        peers are allowed to query the local NTP server via ntpq.
   pools:
     default: >
         0.ubuntu.pool.ntp.org
@@ -19,25 +19,27 @@ options:
         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.
+        Space-separated list of NTP servers to use as pool sources. These
+        are recommended over normal sources for their self-healing
+        capabilities. Leave empty to disable pool sources.
   orphan_stratum:
-    default: 6
+    default: 0
     type: int
     description: >
-        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 strata below most Internet NTP hosts.
-        Set to 0 to disable orphan mode entirely.
+        The stratum at which NTP must lose connectivity to before it considers
+        itself orphaned, and starts determining the reference time with local
+        peers.  A typical value is 6, which will enable orphaned operation
+        when there are 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.  You must enable at least one peer
+        in order to use orphan mode, but four or more is recommended for best
+        results.
   nagios_context:
     default: "juju"
     type: string
     description: |
       Used by the nrpe-external-master subordinate charm.
-      A string that will be prepended to instance name to set the host name
+      A string which will be prepended to instance name to set the host name
       in nagios. So for instance the hostname would be something like:
           juju-myservice-0
       If you're running multiple environments with the same services in them
diff --git a/hooks/ntp_hooks.py b/hooks/ntp_hooks.py
index efde7ad..e9f537d 100755
--- a/hooks/ntp_hooks.py
+++ b/hooks/ntp_hooks.py
@@ -1,21 +1,18 @@
 #!/usr/bin/python3
 
 from charmhelpers.contrib.charmsupport import nrpe
-from charmhelpers.contrib.templating.jinja import render
 from charmhelpers.core import hookenv, host, unitdata
 import charmhelpers.fetch as fetch
 import os
-import shutil
 import sys
 
+import ntp_implementation
 import ntp_scoring
 
 NAGIOS_PLUGINS = '/usr/local/lib/nagios/plugins'
 
-NTP_CONF = '/etc/ntp.conf'
-NTP_CONF_ORIG = '{}.orig'.format(NTP_CONF)
-
 hooks = hookenv.Hooks()
+implementation = ntp_implementation.get_implementation()
 
 
 def get_peer_sources(topN=6):
@@ -65,9 +62,8 @@ def get_peer_sources(topN=6):
 def install():
     fetch.apt_update(fatal=True)
     ntp_scoring.install_packages()
-    if ntp_scoring.get_virt_type() != 'container':
-        fetch.apt_install(["ntp"], fatal=True)
-        shutil.copy(NTP_CONF, NTP_CONF_ORIG)
+    fetch.apt_install(implementation.packages_to_install(), fatal=True)
+    implementation.save_config()
 
 
 def get_sources(sources, iburst=True, source_list=None):
@@ -92,16 +88,20 @@ def get_sources(sources, iburst=True, source_list=None):
 @hooks.hook('master-relation-departed')
 @hooks.hook('ntp-peers-relation-joined',
             'ntp-peers-relation-changed')
[e-mail]_on_change({NTP_CONF: ['ntp']})
[e-mail]_on_change({implementation.config_file(): [implementation.service_name()]})
 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
+        is_container = 1
+    else:
+        is_container = 0
 
-    host.service_resume('ntp')
+    implementation.set_startup_options({
+        'is_container': is_container,
+    })
+
+    host.service_resume(implementation.service_name())
     hookenv.open_port(123, protocol="UDP")
 
     use_iburst = hookenv.config('use_iburst')
@@ -142,17 +142,17 @@ def write_config():
         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
-        shutil.copy(NTP_CONF_ORIG, NTP_CONF)
+        # we have no peers/pools/servers; restore default config
+        implementation.restore_config()
     else:
         # otherwise, write our own configuration
-        with open(NTP_CONF, "w") as ntpconf:
-            ntpconf.write(render(os.path.basename(NTP_CONF), {
-                'orphan_stratum': orphan_stratum,
-                'peers': remote_peers,
-                'pools': remote_pools,
-                'servers': remote_sources,
-            }))
+        implementation.set_config({
+            'is_container': is_container,
+            'orphan_stratum': orphan_stratum,
+            'peers': remote_peers,
+            'pools': remote_pools,
+            'servers': remote_sources,
+        })
 
     if hookenv.relation_ids('nrpe-external-master'):
         update_nrpe_config()
@@ -263,27 +263,32 @@ def hyperv_sync_status():
 
 @hooks.hook('update-status')
 def assess_status():
-    version = fetch.get_upstream_version('ntp')
+    package = implementation.package_name()
+    version = fetch.get_upstream_version(package)
     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'):
+    # base status
+    status = package + ': '
+
+    # append service status
+    if host.service_running(implementation.service_name()):
         state = 'active'
-        status = 'Ready'
+        status += 'Ready'
     else:
         state = 'blocked'
-        status = 'Not running'
+        status += 'Not running'
+
+    # append container status
+    if ntp_scoring.get_virt_type() == 'container':
+        status += ', time sync disabled in container'
 
-    # append Hyper-V status, if any
+    # append Hyper-V status
     hyperv_status = hyperv_sync_status()
     if hyperv_status is not None:
         status += ', ' + hyperv_status
 
-    # append scoring status, if any
+    # append scoring status
     # (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)
diff --git a/hooks/ntp_implementation.py b/hooks/ntp_implementation.py
 new file mode 100644
index 0000000..e565b0e
--- /dev/null
+++ b/hooks/ntp_implementation.py
@@ -0,0 +1,132 @@
+
+# Copyright (c) 2018 Canonical Ltd
+# License: GPLv3
+# Author: Paul Gear
+
+# NTP implementation details
+
+import charmhelpers.contrib.templating.jinja as templating
+import charmhelpers.core.host
+import charmhelpers.osplatform
+import os.path
+import shutil
+
+
+class NTPImplementation:
+    """Base class for NTP implementations."""
+
+    def config_file(self):
+        raise NotImplementedError
+
+    def _config_file_backup(self):
+        return self.config_file() + ".bak"
+
+    def _config_file_orig(self):
+        return self.config_file() + ".orig"
+
+    def _config_file_template(self):
+        raise NotImplementedError
+
+    def package_name(self):
+        raise NotImplementedError
+
+    def packages_to_install(self):
+        return [self.package_name()]
+
+    def restore_config(self):
+        for path in [self._config_file_orig(), self._config_file_backup()]:
+            if os.path.exists(path):
+                shutil.copy(path, self.config_file())
+
+    def save_config(self):
+        backup = self._config_file_backup()
+        if not os.path.exists(backup):
+            shutil.copy(self.config_file(), backup)
+
+    def service_name(self):
+        raise NotImplementedError
+
+    def set_config(self, config):
+        with open(self.config_file(), "w") as conffile:
+            conffile.write(templating.render(self._config_file_template(), config))
+
+    def set_startup_options(self, config):
+        with open(self._startup_config_file(), "w") as startup:
+            startup.write(templating.render(self._startup_template_file(), config))
+
+    def _startup_config_file(self):
+        raise NotImplementedError
+
+    def _startup_template_file(self):
+        raise NotImplementedError
+
+
+class Chronyd(NTPImplementation):
+
+    def config_file(self):
+        return "/etc/chrony/chrony.conf"
+
+    def _config_file_orig(self):
+        return "/usr/share/chrony/chrony.conf"
+
+    def _config_file_template(self):
+        return "chrony.conf"
+
+    def package_name(self):
+        return "chrony"
+
+    def service_name(self):
+        return "chrony"
+
+    def _startup_config_file(self):
+        return "/etc/default/chrony"
+
+    def _startup_template_file(self):
+        return "chrony.default"
+
+
+class NTPd(NTPImplementation):
+
+    def config_file(self):
+        return "/etc/ntp.conf"
+
+    def _config_file_template(self):
+        return "ntp.conf"
+
+    def package_name(self):
+        return "ntp"
+
+    def service_name(self):
+        return "ntp"
+
+    def _startup_config_file(self):
+        return "/etc/default/ntp"
+
+    def _startup_template_file(self):
+        return "ntp.default"
+
+
+def get_implementation(implementation_name=None):
+    """Select the appropriate NTP implementation for this platform."""
+    if implementation_name is not None and implementation_name.lower() == 'chrony':
+        return Chronyd()
+    if implementation_name is not None and implementation_name.lower() == 'ntpd':
+        return NTPd()
+
+    # anything else: auto mode
+    platform = charmhelpers.osplatform.get_platform()
+    version = float(charmhelpers.core.host.lsb_release()['DISTRIB_RELEASE'])
+
+    if platform == 'ubuntu':
+        if version > 18:
+            # Ubuntu 18.04 or later: use chronyd
+            return Chronyd()
+        else:
+            # Ubuntu 17.10 or earlier: use ntpd
+            return NTPd()
+    elif platform == 'centos':
+        # CentOS: use chronyd
+        return Chronyd()
+    else:
+        # something else: use ntpd
+        return NTPd()
diff --git a/templates/chrony.conf b/templates/chrony.conf
 new file mode 100644
index 0000000..6e559cc
--- /dev/null
+++ b/templates/chrony.conf
@@ -0,0 +1,58 @@
+# juju-generated chrony configuration
+
+# This directive specify the location of the file containing ID/key pairs for
+# NTP authentication.
+keyfile /etc/chrony/chrony.keys
+
+# This directive specify the file into which chronyd will store the rate
+# information.
+driftfile /var/lib/chrony/chrony.drift
+
+# Enable logging
+log tracking measurements statistics rtc refclocks
+
+# Log files location.
+logdir /var/log/chrony
+
+# Step the system clock instead of slewing it if the adjustment is larger than
+# one second, but only in the first three clock updates.
+makestep 1 3
+
+# Stop bad estimates upsetting machine clock.
+maxupdateskew 100.0
+
+{%- if is_container == 0 %}
+
+# This directive enables kernel synchronisation (every 11 minutes) of the
+# real-time clock. Note that it can't be used along with the 'rtcfile' directive.
+rtcsync
+{%- endif %}
+
+# Save state about peers & servers
+dumpdir /var/lib/chrony
+
+{%- if orphan_stratum > 0 %}
+
+# Orphan mode enabled
+local stratum {{ orphan_stratum }} orphan
+{%- endif %}
+
+# allow any host to use us as an NTP server
+# (use juju unexpose to prevent access)
+allow
+
+# PEERS
+{%- for peer in peers %}
+peer {{ peer.name }} {{ peer.iburst }}
+{%- endfor %}
+
+# POOLS
+{%- for pool in pools %}
+pool {{ pool.name }} {{ pool.iburst }}
+{%- endfor %}
+
+# SERVERS
+{%- for server in servers %}
+server {{ server.name }} {{ server.iburst }}
+{%- endfor %}
+
diff --git a/templates/chrony.default b/templates/chrony.default
 new file mode 100644
index 0000000..aaaf90e
--- /dev/null
+++ b/templates/chrony.default
@@ -0,0 +1,13 @@
+# juju-generated chrony startup config
+
+# This is a configuration file for /etc/init.d/chrony and
+# /lib/systemd/system/chrony.service; it allows you to pass various options to
+# the chrony daemon without editing the init script or service file.
+
+# Options to pass to chrony.
+{%- if is_container == 1 %}
+DAEMON_OPTS="-x"
+{%- else %}
+DAEMON_OPTS=""
+{%- endif %}
+
diff --git a/templates/ntp.conf b/templates/ntp.conf
index d5674c7..1a38ecf 100644
--- a/templates/ntp.conf
+++ b/templates/ntp.conf
@@ -1,28 +1,46 @@
 # juju-generated ntp configuration
 tinker panic 0
 driftfile /var/lib/ntp/ntp.drift
+
 statsdir /var/log/ntpstats
 statistics loopstats peerstats clockstats sysstats
 filegen loopstats file loopstats type day enable
 filegen peerstats file peerstats type day enable
 filegen clockstats file clockstats type day enable
 filegen sysstats file sysstats type day enable
+
 restrict -4 default kod notrap nomodify nopeer noquery limited
 restrict -6 default kod notrap nomodify nopeer noquery limited
 restrict source notrap nomodify noquery
 restrict 127.0.0.1
 restrict ::1
-{% if orphan_stratum > 0 %}
+
+{%- if is_container == 1 %}
+# running in a container - time adjustments disabled
+disable kernel
+{%- endif %}
+
+{%- if orphan_stratum > 0 %}
+
+# orphan mode enabled
 tos orphan {{ orphan_stratum }}
-{% endif %}
-{% for peer in peers %}restrict {{ peer.name }} kod notrap nomodify
-{% endfor %}
-# SERVERS
-{% for server in servers %}server {{ server.name }} {{ server.iburst }}
-{% endfor %}
+{%- endif %}
+
 # PEERS
-{% for peer in peers %}peer {{ peer.name }} {{ peer.iburst }}
-{% endfor %}
+{%- for peer in peers %}
+restrict {{ peer.name }} kod notrap nomodify
+{%- endfor %}
+{%- for peer in peers %}
+peer {{ peer.name }} {{ peer.iburst }}
+{%- endfor %}
+
 # POOLS
-{% for pool in pools %}pool {{ pool.name }} {{ pool.iburst }}
-{% endfor %}
+{%- for pool in pools %}
+pool {{ pool.name }} {{ pool.iburst }}
+{%- endfor %}
+
+# SERVERS
+{%- for server in servers %}
+server {{ server.name }} {{ server.iburst }}
+{%- endfor %}
+
diff --git a/templates/ntp.default b/templates/ntp.default
 new file mode 100644
index 0000000..f84c389
--- /dev/null
+++ b/templates/ntp.default
@@ -0,0 +1,2 @@
+# juju-generated ntp startup configuration
+NTPD_OPTS='-g'