Merge ~rjschwei/cloud-init:chrony into cloud-init:master

[Robert Schweikert]

 Support chrony configuration (lp#1731619)
  + Add a template for chrony configuration
  + Add new set_timesync_client to distros base class
    - Set the timesync client provided in the config by the user with
      system_info: ntp_client
    - If no user config set the timesync client to one of the supported
      clients if the executable is installed
    - Fall back to the distribution default
  + Handle the new settings in cc_ntp while retaining current behavior
    as the fallback until all distro implementations have switched to the
    new implementation
  + Use new way of ntp client configuration for openSUSE and SLES
  + Unit tests

Follows along the ideas discussed here: https://hackmd.io/CwEwHARgbAzDDsBaArPOjhQMYEZEEMBOHQjQkeAUxgCYb9h8og==

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:23f976be51ba9ad6e1e173f23c7220144beb942a
https://jenkins.ubuntu.com/server/job/cloud-init-ci/603/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/603/rebuild

review: Needs Fixing (continuous-integration)

d94392b... by Robert Schweikert on 2017-12-08

- Disable method deprecation warning for pylint

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:d94392bb6e54a6860c8b6ea7967e853d8e263d7a
https://jenkins.ubuntu.com/server/job/cloud-init-ci/605/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/605/rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

Thanks for getting this started. For reference more of these changes were
discussed here: https://hackmd.io/s/r1qN6HNbz

I'd like to drop the bulk of the logic in cc_ntp and push those into either the base distro class or specific distro classes as needed. cc_ntp would look much simpler, basically, checking the configuration dictionary/validate-schema and call into the distro with the config.

  def handle(name, cfg, cloud, log, _args)
     # check for 'ntp' in config and bail if not present
     valid_cloudconfig_schema(cfg, schema)
     distro.configure_ntp(cfg)

We won't need to check for attributes, introduce the configure_ntp as part of the base
class were we handle any cross-distro actions and then call into per-distro methods

     DistroBase

       def configure_ntp(cfg)
         client_name = _select_ntp_client(cfg) # abstract in base class
         _write_ntp_config(client_name, cfg)
         _restart_ntp_service(client_name, cfg)

The _select_ntp_client() is where each distro will examine the
ntp_cfg, systemcfg and state to determine the correct client.

The rest I think can be common given the client selection and configuration
since we're rendering configs with templates (or the config format is common
like in systemd-timesyncd).

The schema/example configuration needs an update, specifically we need to reference that
we not have:
  ntp:
    enabled: true

To allow users to explicitly enable/disable, specifically for the case of using the
distro default ntp configuration.

review: Needs Fixing

[Robert Schweikert (rjschwei)]

> Thanks for getting this started. For reference more of these changes were
> discussed here: https://hackmd.io/s/r1qN6HNbz
>
> I'd like to drop the bulk of the logic in cc_ntp and push those into either
> the base distro class or specific distro classes as needed. cc_ntp would look
> much simpler, basically, checking the configuration dictionary/validate-schema
> and call into the distro with the config.
>
> def handle(name, cfg, cloud, log, _args)
> # check for 'ntp' in config and bail if not present
> valid_cloudconfig_schema(cfg, schema)
> distro.configure_ntp(cfg)
>
> We won't need to check for attributes, introduce the configure_ntp as part of
> the base
> class were we handle any cross-distro actions and then call into per-distro
> methods
>
> DistroBase
>
> def configure_ntp(cfg)
> client_name = _select_ntp_client(cfg) # abstract in base class
> _write_ntp_config(client_name, cfg)
> _restart_ntp_service(client_name, cfg)
>
> The _select_ntp_client() is where each distro will examine the
> ntp_cfg, systemcfg and state to determine the correct client.
>
> The rest I think can be common given the client selection and configuration
> since we're rendering configs with templates (or the config format is common
> like in systemd-timesyncd).
>
>
> The schema/example configuration needs an update, specifically we need to
> reference that
> we not have:
> ntp:
> enabled: true
>
> To allow users to explicitly enable/disable, specifically for the case of
> using the
> distro default ntp configuration.

Two concerns here with doing all of this at once, although I do have to agree that we'll probably end up with cleaner code earlier.

1.) Doing all of this at once potentially causes lots of upheaval as the core logic of cc_ntp would change. Unless there is some other mechanism, other than checking attributes to split the code path between old behavior and new behavior.

2.) Config compatibility, if we enforce

ntp:
    enabled: true

which I think that is being suggested then basically we are making an incompatible config file change, meaning any one who has

ntp:
  .....

in their config at this point needs to add "enabled: true"

That's fair enough, just want to make sure everyone is on the same page.

[Ryan Harper (raharper)]

On Mon, Dec 18, 2017 at 12:50 PM, Robert Schweikert <email address hidden>
wrote:

> > Thanks for getting this started. For reference more of these changes
> were
> > discussed here: https://hackmd.io/s/r1qN6HNbz
> >
> > I'd like to drop the bulk of the logic in cc_ntp and push those into
> either
> > the base distro class or specific distro classes as needed. cc_ntp
> would look
> > much simpler, basically, checking the configuration
> dictionary/validate-schema
> > and call into the distro with the config.
> >
> > def handle(name, cfg, cloud, log, _args)
> > # check for 'ntp' in config and bail if not present
> > valid_cloudconfig_schema(cfg, schema)
> > distro.configure_ntp(cfg)
> >
> > We won't need to check for attributes, introduce the configure_ntp as
> part of
> > the base
> > class were we handle any cross-distro actions and then call into
> per-distro
> > methods
> >
> > DistroBase
> >
> > def configure_ntp(cfg)
> > client_name = _select_ntp_client(cfg) # abstract in base class
> > _write_ntp_config(client_name, cfg)
> > _restart_ntp_service(client_name, cfg)
> >
> > The _select_ntp_client() is where each distro will examine the
> > ntp_cfg, systemcfg and state to determine the correct client.
> >
> > The rest I think can be common given the client selection and
> configuration
> > since we're rendering configs with templates (or the config format is
> common
> > like in systemd-timesyncd).
> >
> >
> > The schema/example configuration needs an update, specifically we need to
> > reference that
> > we not have:
> > ntp:
> > enabled: true
> >
> > To allow users to explicitly enable/disable, specifically for the case of
> > using the
> > distro default ntp configuration.
>
> Two concerns here with doing all of this at once, although I do have to
> agree that we'll probably end up with cleaner code earlier.
>
> 1.) Doing all of this at once potentially causes lots of upheaval as the
> core logic of cc_ntp would change. Unless there is some other mechanism,
> other than checking attributes to split the code path between old behavior
> and new behavior.
>

The "old" path is maintained by including a patch to configure the "legacy"
client name during packaging. Even if the code moves from cc_ntp into
distro; the input/output remains the same, and can be exercised with
a system config that specifies the legacy ntp_client value.

That is, the existing unittests which take the config dictionary of
servers/pools would also need a system_config which sets ntp_client =
'isc-ntp' and validates the 'old behavior'.
We'd also add new tests which leave system_config unset (using code
defaults, or set system config to other values) and will validate the new
behavior.

> 2.) Config compatibility, if we enforce
>
> ntp:
> enabled: true
>
> which I think that is being suggested then basically we are making an
> incompatible config file change, meaning any one who has
>
> ntp:
> .....
>
> in their config at this point needs to add "enabled: true"
>

This will need compat patches included in previous release updates to
maintain behavior (and likely emitting a warning).
On master, such a...

42cb184... by Robert Schweikert on 2017-12-18

- Distro dependent chrony config file
  + We all like to stor ethe drift file in different places and name it
    differently :(

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:42cb1841035befa5b5823b3321c8fe92f2cb9087
https://jenkins.ubuntu.com/server/job/cloud-init-ci/641/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/641/rebuild

review: Needs Fixing (continuous-integration)

Unmerged commits

42cb184... by Robert Schweikert on 2017-12-18

- Distro dependent chrony config file
  + We all like to stor ethe drift file in different places and name it
    differently :(

d94392b... by Robert Schweikert on 2017-12-08

- Disable method deprecation warning for pylint

23f976b... by Robert Schweikert on 2017-11-14

- Support chrony configuration (lp#1731619)
  + Add a template for chrony configuration
  + Add new set_timesync_client to distros base class
    - Set the timesync client provided in the config by the user with
      system_info: ntp_client
    - If no user config set the timesync client to one of the supported
      clients if the executable is installed
    - Fall back to the distribution default
  + Handle the new settings in cc_ntp while retaining current behavior
    as the fallback until all distro implementations have switched to the
    new implementation
  + Use new way of ntp client configuration for openSUSE and SLES
  + Unit tests

diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py
index cbd0237..747b6f2 100644
--- a/cloudinit/config/cc_ntp.py
+++ b/cloudinit/config/cc_ntp.py
@@ -20,8 +20,9 @@ from textwrap import dedent
 LOG = logging.getLogger(__name__)
 
 frequency = PER_INSTANCE
-NTP_CONF = '/etc/ntp.conf'
-TIMESYNCD_CONF = '/etc/systemd/timesyncd.conf.d/cloud-init.conf'
+CHRONY_CONF_FILE = '/etc/chrony.conf'
+NTP_CONF_FILE = '/etc/ntp.conf'
+TIMESYNCD_CONF_FILE = '/etc/systemd/timesyncd.conf.d/cloud-init.conf'
 NR_POOL_SERVERS = 4
 distros = ['centos', 'debian', 'fedora', 'opensuse', 'sles', 'ubuntu']
 
@@ -49,6 +50,7 @@ schema = {
     'examples': [
         dedent("""\
         ntp:
+          enabled: true
           pools: [0.int.pool.ntp.org, 1.int.pool.ntp.org, ntp.myorg.org]
           servers:
             - ntp.server.local
@@ -60,6 +62,9 @@ schema = {
         'ntp': {
             'type': ['object', 'null'],
             'properties': {
+                'enabled':  {
+                    "type": "boolean"
+                },
                 'pools': {
                     'type': 'array',
                     'items': {
@@ -106,30 +111,58 @@ def handle(name, cfg, cloud, log, _args):
 
     # TODO drop this when validate_cloudconfig_schema is strict=True
     if not isinstance(ntp_cfg, (dict)):
+<<<<<<< cloudinit/config/cc_ntp.py
         raise RuntimeError(
             "'ntp' key existed in config, but not a dictionary type,"
             " is a {_type} instead".format(_type=type_utils.obj_name(ntp_cfg)))
+=======
+        raise RuntimeError(("'ntp' key existed in config,"
+                            " but not a dictionary type,"
+                            " is a %s instead"), type_utils.obj_name(ntp_cfg))
+>>>>>>> cloudinit/config/cc_ntp.py
+
+    if ntp_cfg.get('enabled') and ntp_cfg.get('enabled') == 'true':
+        cloud.distro.set_timesync_client()
+    else:
+        # When all distro implementations are switched return here
+        pass
 
     validate_cloudconfig_schema(cfg, schema)
-    if ntp_installable():
-        service_name = 'ntp'
-        confpath = NTP_CONF
-        template_name = None
-        packages = ['ntp']
-        check_exe = 'ntpd'
+    if hasattr(cloud.distro, 'timesync_client'):
+        client_name = cloud.distro.timesync_client
+        service_name = cloud.distro.timesync_service_name
+        if client_name == 'ntp':
+            confpath = NTP_CONF_FILE
+            template_name = 'ntp.conf.%s' % cloud.distro.name
+        elif client_name == 'systemd-timesyncd':
+            confpath = TIMESYNCD_CONF_FILE
+            template_name = 'timesyncd.conf'
+        elif client_name == 'chrony':
+            confpath = CHRONY_CONF_FILE
+            template_name = 'chrony.conf.%s' % cloud.distro.name
     else:
-        service_name = 'systemd-timesyncd'
-        confpath = TIMESYNCD_CONF
-        template_name = 'timesyncd.conf'
-        packages = []
-        check_exe = '/lib/systemd/systemd-timesyncd'
-
-    rename_ntp_conf()
+        if ntp_installable():
+            service_name = 'ntp'
+            confpath = NTP_CONF_FILE
+            template_name = None
+            packages = ['ntp']
+            check_exe = 'ntpd'
+        else:
+            service_name = 'systemd-timesyncd'
+            confpath = TIMESYNCD_CONF_FILE
+            template_name = 'timesyncd.conf'
+            packages = []
+            check_exe = '/lib/systemd/systemd-timesyncd'
+
+    rename_ntp_conf(confpath)
     # ensure when ntp is installed it has a configuration file
     # to use instead of starting up with packaged defaults
     write_ntp_config_template(ntp_cfg, cloud, confpath, template=template_name)
-    install_ntp(cloud.distro.install_packages, packages=packages,
-                check_exe=check_exe)
+    if not hasattr(cloud.distro, 'timesync_client'):
+        # Updated implementation installs a package is missing in
+        # distro._set_default_timesync_client
+        install_ntp(cloud.distro.install_packages, packages=packages,
+                    check_exe=check_exe)
 
     try:
         reload_ntp(service_name, systemd=cloud.distro.uses_systemd())
@@ -167,7 +200,7 @@ def install_ntp(install_func, packages=None, check_exe="ntpd"):
 def rename_ntp_conf(config=None):
     """Rename any existing ntp.conf file"""
     if config is None:  # For testing
-        config = NTP_CONF
+        config = NTP_CONF_FILE
     if os.path.exists(config):
         util.rename(config, config + ".dist")
 
diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py
index 55260ea..26b75c4 100755
--- a/cloudinit/distros/__init__.py
+++ b/cloudinit/distros/__init__.py
@@ -61,6 +61,9 @@ class Distro(object):
     init_cmd = ['service']  # systemctl, service etc
     renderer_configs = {}
 
+    __timesync_client_map = {}
+    __ntp_client_execs = []
+
     def __init__(self, name, cfg, paths):
         self._paths = paths
         self._cfg = cfg
@@ -90,6 +93,43 @@ class Distro(object):
         renderer.render_network_config(network_config=network_config)
         return []
 
+    def set_timesync_client(self):
+        system_info = self._cfg.get('system_info')
+        if system_info and isinstance(system_info, (dict)):
+            ntp_client = system_info.get('ntp_client')
+            if ntp_client and ntp_client in self.__timesync_client_map:
+                self.timesync_client, self.timesync_service_name = \
+                    self.__timesync_client_map.get(ntp_client)
+                LOG.debug('Using "%s" for timesync client per configuration',
+                          ntp_client)
+                return
+
+        found = False
+        for ntp_client in self.__ntp_client_execs:
+            ntp_exec = util.which(ntp_client)
+            if ntp_exec and not found:
+                found = ntp_client
+            # systemd-timesyncd is part of systemd and thus is probably
+            # always installed, do not consider it as a conflict
+            elif ntp_exec and found and 'systemd-timesyncd' not in ntp_exec:
+                msg = 'Found multiple timesync clients installed. Resolve '
+                msg += 'ambigutity by falling back to distro default'
+                LOG.debug(msg)
+                found = False
+                break
+
+        if found and found in self.__timesync_client_map:
+            self.timesync_client, self.timesync_service_name = \
+                self.__timesync_client_map.get(found)
+            LOG.debug('Using "%s" for timesync based on installed exec',
+                      ntp_client)
+            return
+
+        self._set_default_timesync_client()
+
+    def _set_default_timesync_client(self):
+        raise NotImplementedError()
+
     def _find_tz_file(self, tz):
         tz_file = os.path.join(self.tz_zone_dir, str(tz))
         if not os.path.isfile(tz_file):
diff --git a/cloudinit/distros/arch.py b/cloudinit/distros/arch.py
index f87a343..fffc1c9 100644
--- a/cloudinit/distros/arch.py
+++ b/cloudinit/distros/arch.py
@@ -153,6 +153,10 @@ class Distro(distros.Distro):
         self._runner.run("update-sources", self.package_command,
                          ["-y"], freq=PER_INSTANCE)
 
+    def _set_default_timesync_client(self):
+        # Fall back to previous implementation
+        return
+
 
 def _render_network(entries, target="/", conf_dir="etc/netctl",
                     resolv_conf="etc/resolv.conf", enable_func=None):
diff --git a/cloudinit/distros/debian.py b/cloudinit/distros/debian.py
index 33cc0bf..46dd417 100644
--- a/cloudinit/distros/debian.py
+++ b/cloudinit/distros/debian.py
@@ -212,6 +212,10 @@ class Distro(distros.Distro):
         (arch, _err) = util.subp(['dpkg', '--print-architecture'])
         return str(arch).strip()
 
+    def _set_default_timesync_client(self):
+        # Fall back to previous implementation
+        return
+
 
 def _get_wrapper_prefix(cmd, mode):
     if isinstance(cmd, str):
diff --git a/cloudinit/distros/freebsd.py b/cloudinit/distros/freebsd.py
index bad112f..00b3891 100644
--- a/cloudinit/distros/freebsd.py
+++ b/cloudinit/distros/freebsd.py
@@ -649,4 +649,8 @@ class Distro(distros.Distro):
         self._runner.run("update-sources", self.package_command,
                          ["update"], freq=PER_INSTANCE)
 
+    def _set_default_timesync_client(self):
+        # Fall back to previous implementation
+        return
+
 # vi: ts=4 expandtab
diff --git a/cloudinit/distros/gentoo.py b/cloudinit/distros/gentoo.py
index dc57717..5685b05 100644
--- a/cloudinit/distros/gentoo.py
+++ b/cloudinit/distros/gentoo.py
@@ -214,6 +214,10 @@ class Distro(distros.Distro):
         self._runner.run("update-sources", self.package_command,
                          ["-u", "world"], freq=PER_INSTANCE)
 
+    def _set_default_timesync_client(self):
+        # Fall back to previous implementation
+        return
+
 
 def convert_resolv_conf(settings):
     """Returns a settings string formatted for resolv.conf."""
diff --git a/cloudinit/distros/opensuse.py b/cloudinit/distros/opensuse.py
index a219e9f..86318ea 100644
--- a/cloudinit/distros/opensuse.py
+++ b/cloudinit/distros/opensuse.py
@@ -8,6 +8,12 @@
 #
 # This file is part of cloud-init. See LICENSE file for license information.
 
+# pylint: disable=W1505
+# platform.linux_distribution is deprecated (W1505) we need to decide if
+# cloud-init will implement it's own or add a new dependency on the
+# distro module
+import platform
+
 from cloudinit import distros
 
 from cloudinit.distros.parsers.hostname import HostnameConf
@@ -36,6 +42,23 @@ class Distro(distros.Distro):
     systemd_locale_conf_fn = '/etc/locale.conf'
     tz_local_fn = '/etc/localtime'
 
+    __timesync_client_map = {
+        # Map the system_info supported values
+        'chrony': ('chrony', 'chronyd'),
+        'isc-ntp': ('ntp', 'ntpd'),
+        'systemd-timesyncd': ('systemd-timesyncd', 'systemd-timesyncd'),
+        # Map the common names if different from system_info
+        'chronyd': ('chrony', 'chronyd'),
+        'ntpd': ('ntp', 'ntpd'),
+        '/usr/lib/systemd/systemd-timesyncd':
+        ('systemd-timesyncd', 'systemd-timesyncd')
+    }
+    __ntp_client_execs = [
+        'chronyd',
+        'ntpd',
+        '/usr/lib/systemd/systemd-timesyncd'
+    ]
+
     def __init__(self, name, cfg, paths):
         distros.Distro.__init__(self, name, cfg, paths)
         self._runner = helpers.Runners(paths)
@@ -145,6 +168,28 @@ class Distro(distros.Distro):
             host_fn = self.hostname_conf_fn
         return (host_fn, self._read_hostname(host_fn))
 
+    def _set_default_timesync_client(self):
+        """The default timesync client is dependent on the distribution."""
+        # When we get here the user has configured ntp to be enabled but
+        # no client is installed
+        distro_info = platform.linux_distribution()
+        name = distro_info[0]
+        major_ver = int(distro_info[1].split('.')[0])
+
+        # This is horribly complicated because of a case of
+        # "we do not care if versions should be increasing syndrome"
+        if (
+                (major_ver >= 15 and 'openSUSE' not in name) or
+                (major_ver >= 15 and 'openSUSE' in name and major_ver != 42)
+        ):
+            self.timesync_client = 'chrony'
+            self.timesync_service_name = 'chronyd'
+            self.install_packages(['chrony'])
+        else:
+            self.timesync_client = 'ntp'
+            self.timesync_service_name = 'ntpd'
+            self.install_packages(['ntp'])
+
     def _write_hostname(self, hostname, out_fn):
         if self.uses_systemd() and out_fn.endswith('/previous-hostname'):
             util.write_file(out_fn, hostname)
diff --git a/cloudinit/distros/rhel.py b/cloudinit/distros/rhel.py
index 1fecb61..6d9c9f6 100644
--- a/cloudinit/distros/rhel.py
+++ b/cloudinit/distros/rhel.py
@@ -218,4 +218,8 @@ class Distro(distros.Distro):
         self._runner.run("update-sources", self.package_command,
                          ["makecache"], freq=PER_INSTANCE)
 
+    def _set_default_timesync_client(self):
+        # Fall back to previous implementation
+        return
+
 # vi: ts=4 expandtab
diff --git a/templates/chrony.conf.opensuse.tmpl b/templates/chrony.conf.opensuse.tmpl
new file mode 100644
index 0000000..38e84d8
--- /dev/null
+++ b/templates/chrony.conf.opensuse.tmpl
@@ -0,0 +1,25 @@
+## template:jinja
+# cloud-init generated file
+# See chrony.conf(5)
+
+{% if pools %}# pools
+{% endif %}
+{% for pool in pools -%}
+pool {{pool}} iburst
+{% endfor %}
+{%- if servers %}# servers
+{% endif %}
+{% for server in servers -%}
+server {{server}} iburst
+{% endfor %}
+
+# Record the rate at which the the system clock gains/losses time
+driftfile /var/lib/chrony/drift
+
+# Allow the system clock to be stepped in the first three updates
+# if its offset is larger than 1 second.
+makestep 1.0 3
+
+# Enable kernel synchronization of the real-time clock (RTC).
+rtcsync
+
diff --git a/templates/chrony.conf.sles.tmpl b/templates/chrony.conf.sles.tmpl
new file mode 100644
index 0000000..38e84d8
--- /dev/null
+++ b/templates/chrony.conf.sles.tmpl
@@ -0,0 +1,25 @@
+## template:jinja
+# cloud-init generated file
+# See chrony.conf(5)
+
+{% if pools %}# pools
+{% endif %}
+{% for pool in pools -%}
+pool {{pool}} iburst
+{% endfor %}
+{%- if servers %}# servers
+{% endif %}
+{% for server in servers -%}
+server {{server}} iburst
+{% endfor %}
+
+# Record the rate at which the the system clock gains/losses time
+driftfile /var/lib/chrony/drift
+
+# Allow the system clock to be stepped in the first three updates
+# if its offset is larger than 1 second.
+makestep 1.0 3
+
+# Enable kernel synchronization of the real-time clock (RTC).
+rtcsync
+
diff --git a/tests/unittests/test_distros/test_generic.py b/tests/unittests/test_distros/test_generic.py
index 791fe61..cdee4b1 100644
--- a/tests/unittests/test_distros/test_generic.py
+++ b/tests/unittests/test_distros/test_generic.py
@@ -4,16 +4,12 @@ from cloudinit import distros
 from cloudinit import util
 
 from cloudinit.tests import helpers
+from cloudinit.tests.helpers import mock
 
 import os
 import shutil
 import tempfile
 
-try:
-    from unittest import mock
-except ImportError:
-    import mock
-
 unknown_arch_info = {
     'arches': ['default'],
     'failsafe': {'primary': 'http://fs-primary-default',
@@ -35,6 +31,24 @@ package_mirrors = [
     unknown_arch_info
 ]
 
+timesync_user_cfg_chrony = {
+    'system_info': {
+        'ntp_client': 'chrony'
+    }
+}
+
+timesync_user_cfg_ntp = {
+    'system_info': {
+        'ntp_client': 'isc-ntp'
+    }
+}
+
+timesync_user_cfg_systemd = {
+    'system_info': {
+        'ntp_client': 'systemd-timesyncd'
+    }
+}
+
 gpmi = distros._get_package_mirror_info
 gapmi = distros._get_arch_package_mirror_info
 
@@ -244,5 +258,82 @@ class TestGenericDistro(helpers.FilesystemMockingTestCase):
         with self.assertRaises(NotImplementedError):
             d.get_locale()
 
+    def test_set_timesync_client_user_config_chrony_sles(self):
+        """Test sles distro sets proper values for chrony"""
+        cls = distros.fetch("sles")
+        d = cls("sles", timesync_user_cfg_chrony, None)
+        d.set_timesync_client()
+        self.assertEqual(d.timesync_client, 'chrony')
+        self.assertEqual(d.timesync_service_name, 'chronyd')
+
+    def test_set_timesync_client_user_config_ntp_sles(self):
+        """Test sles distro sets proper values for ntp"""
+        cls = distros.fetch("sles")
+        d = cls("sles", timesync_user_cfg_ntp, None)
+        d.set_timesync_client()
+        self.assertEqual(d.timesync_client, 'ntp')
+        self.assertEqual(d.timesync_service_name, 'ntpd')
+
+    def test_set_timesync_client_user_config_timesyncd_sles(self):
+        """Test sles distro sets proper values for timesyncd"""
+        cls = distros.fetch("sles")
+        d = cls("sles", timesync_user_cfg_systemd, None)
+        d.set_timesync_client()
+        self.assertEqual(d.timesync_client, 'systemd-timesyncd')
+        self.assertEqual(d.timesync_service_name, 'systemd-timesyncd')
+
+    @mock.patch("cloudinit.distros.util")
+    def test_set_timesync_client_chrony_installed_sles(self, mock_util):
+        """Test sles distro sets proper values for chrony if chrony is
+           installed"""
+        mock_util.which.side_effect = side_effect_client_is_chrony
+        cls = distros.fetch("sles")
+        d = cls("sles", {}, None)
+        d.set_timesync_client()
+        self.assertEqual(d.timesync_client, 'chrony')
+        self.assertEqual(d.timesync_service_name, 'chronyd')
+
+    @mock.patch("cloudinit.distros.util")
+    def test_set_timesync_client_ntp_installed_sles(self, mock_util):
+        """Test sles distro sets proper values for ntp if ntpd is
+           installed"""
+        mock_util.which.side_effect = side_effect_client_is_ntp
+        cls = distros.fetch("sles")
+        d = cls("sles", {}, None)
+        d.set_timesync_client()
+        self.assertEqual(d.timesync_client, 'ntp')
+        self.assertEqual(d.timesync_service_name, 'ntpd')
+
+    @mock.patch("cloudinit.distros.util")
+    def test_set_timesync_client_timesycd_installed_sles(self, mock_util):
+        """Test sles distro sets proper values for timesycd if timesyncd is
+           installed"""
+        mock_util.which.side_effect = side_effect_client_is_timesyncd
+        cls = distros.fetch("sles")
+        d = cls("sles", {}, None)
+        d.set_timesync_client()
+        self.assertEqual(d.timesync_client, 'systemd-timesyncd')
+        self.assertEqual(d.timesync_service_name, 'systemd-timesyncd')
+
+
+def side_effect_client_is_chrony(ntp_client):
+    if 'chrony' in ntp_client:
+        return '/usr/sbin/chronyd'
+    else:
+        return False
+
+
+def side_effect_client_is_ntp(ntp_client):
+    if 'ntp' in ntp_client:
+        return '/usr/sbin/ntpd'
+    else:
+        return False
+
+
+def side_effect_client_is_timesyncd(ntp_client):
+    if 'timesyncd' in ntp_client:
+        return ntp_client
+    else:
+        return False
 
 # vi: ts=4 expandtab
diff --git a/tests/unittests/test_distros/test_opensuse.py b/tests/unittests/test_distros/test_opensuse.py
index b9bb9b3..9ed10af 100644
--- a/tests/unittests/test_distros/test_opensuse.py
+++ b/tests/unittests/test_distros/test_opensuse.py
@@ -1,6 +1,6 @@
 # This file is part of cloud-init. See LICENSE file for license information.
 
-from cloudinit.tests.helpers import CiTestCase
+from cloudinit.tests.helpers import CiTestCase, mock
 
 from . import _get_distro
 
@@ -10,3 +10,45 @@ class TestopenSUSE(CiTestCase):
     def test_get_distro(self):
         distro = _get_distro("opensuse")
         self.assertEqual(distro.osfamily, 'suse')
+
+    @mock.patch("cloudinit.distros.opensuse.Distro.install_packages")
+    @mock.patch("platform.linux_distribution")
+    def test_set_default_timesync_client_osl42(
+            self,
+            mock_distro,
+            mock_install
+    ):
+        mock_distro.return_value = ('openSUSE ', '42.3', 'x86_64')
+        mock_install.return_value = True
+        distro = _get_distro("opensuse")
+        distro._set_default_timesync_client()
+        self.assertEqual(distro.timesync_client, 'ntp')
+        self.assertEqual(distro.timesync_service_name, 'ntpd')
+
+    @mock.patch("cloudinit.distros.opensuse.Distro.install_packages")
+    @mock.patch("platform.linux_distribution")
+    def test_set_default_timesync_client_os13(
+            self,
+            mock_distro,
+            mock_install
+    ):
+        mock_distro.return_value = ('openSUSE ', '13.1', 'x86_64')
+        mock_install.return_value = True
+        distro = _get_distro("opensuse")
+        distro._set_default_timesync_client()
+        self.assertEqual(distro.timesync_client, 'ntp')
+        self.assertEqual(distro.timesync_service_name, 'ntpd')
+
+    @mock.patch("cloudinit.distros.opensuse.Distro.install_packages")
+    @mock.patch("platform.linux_distribution")
+    def test_set_default_timesync_client_osl15(
+            self,
+            mock_distro,
+            mock_install
+    ):
+        mock_distro.return_value = ('openSUSE ', '15.1', 'x86_64')
+        mock_install.return_value = True
+        distro = _get_distro("opensuse")
+        distro._set_default_timesync_client()
+        self.assertEqual(distro.timesync_client, 'chrony')
+        self.assertEqual(distro.timesync_service_name, 'chronyd')
diff --git a/tests/unittests/test_distros/test_sles.py b/tests/unittests/test_distros/test_sles.py
index 33e3c45..13237a2 100644
--- a/tests/unittests/test_distros/test_sles.py
+++ b/tests/unittests/test_distros/test_sles.py
@@ -1,6 +1,6 @@
 # This file is part of cloud-init. See LICENSE file for license information.
 
-from cloudinit.tests.helpers import CiTestCase
+from cloudinit.tests.helpers import CiTestCase, mock
 
 from . import _get_distro
 
@@ -10,3 +10,31 @@ class TestSLES(CiTestCase):
     def test_get_distro(self):
         distro = _get_distro("sles")
         self.assertEqual(distro.osfamily, 'suse')
+
+    @mock.patch("cloudinit.distros.opensuse.Distro.install_packages")
+    @mock.patch("platform.linux_distribution")
+    def test_set_default_timesync_client_osl42(
+            self,
+            mock_distro,
+            mock_install
+    ):
+        mock_distro.return_value = ('SLES ', '12.3', 'x86_64')
+        mock_install.return_value = True
+        distro = _get_distro("sles")
+        distro._set_default_timesync_client()
+        self.assertEqual(distro.timesync_client, 'ntp')
+        self.assertEqual(distro.timesync_service_name, 'ntpd')
+
+    @mock.patch("cloudinit.distros.opensuse.Distro.install_packages")
+    @mock.patch("platform.linux_distribution")
+    def test_set_default_timesync_client_os13(
+            self,
+            mock_distro,
+            mock_install
+    ):
+        mock_distro.return_value = ('SLES ', '15', 'x86_64')
+        mock_install.return_value = True
+        distro = _get_distro("sles")
+        distro._set_default_timesync_client()
+        self.assertEqual(distro.timesync_client, 'chrony')
+        self.assertEqual(distro.timesync_service_name, 'chronyd')
diff --git a/tests/unittests/test_handler/test_handler_ntp.py b/tests/unittests/test_handler/test_handler_ntp.py
index 28a8455..33fab8c 100644
--- a/tests/unittests/test_handler/test_handler_ntp.py
+++ b/tests/unittests/test_handler/test_handler_ntp.py
@@ -10,6 +10,20 @@ import os
 from os.path import dirname
 import shutil
 
+CHRONY_TEMPLATE = b"""\
+## template: jinja
+{% if pools %}# pools
+{% endif %}
+{% for pool in pools -%}
+pool {{pool}} iburst
+{% endfor %}
+{%- if servers %}# servers
+{% endif %}
+{% for server in servers -%}
+server {{server}} iburst
+{% endfor %}
+"""
+
 NTP_TEMPLATE = b"""\
 ## template: jinja
 servers {{servers}}
@@ -79,7 +93,7 @@ class TestNtp(FilesystemMockingTestCase):
         """When NTP_CONF exists, rename_ntp moves it."""
         ntpconf = self.tmp_path("ntp.conf", self.new_root)
         util.write_file(ntpconf, "")
-        with mock.patch("cloudinit.config.cc_ntp.NTP_CONF", ntpconf):
+        with mock.patch("cloudinit.config.cc_ntp.NTP_CONF_FILE", ntpconf):
             cc_ntp.rename_ntp_conf()
         self.assertFalse(os.path.exists(ntpconf))
         self.assertTrue(os.path.exists("{0}.dist".format(ntpconf)))
@@ -112,7 +126,7 @@ class TestNtp(FilesystemMockingTestCase):
         """When NTP_CONF doesn't exist rename_ntp doesn't create a file."""
         ntpconf = self.tmp_path("ntp.conf", self.new_root)
         self.assertFalse(os.path.exists(ntpconf))
-        with mock.patch("cloudinit.config.cc_ntp.NTP_CONF", ntpconf):
+        with mock.patch("cloudinit.config.cc_ntp.NTP_CONF_FILE", ntpconf):
             cc_ntp.rename_ntp_conf()
         self.assertFalse(os.path.exists("{0}.dist".format(ntpconf)))
         self.assertFalse(os.path.exists(ntpconf))
@@ -133,7 +147,7 @@ class TestNtp(FilesystemMockingTestCase):
         # Create ntp.conf.tmpl
         with open('{0}.tmpl'.format(ntp_conf), 'wb') as stream:
             stream.write(NTP_TEMPLATE)
-        with mock.patch('cloudinit.config.cc_ntp.NTP_CONF', ntp_conf):
+        with mock.patch('cloudinit.config.cc_ntp.NTP_CONF_FILE', ntp_conf):
             cc_ntp.write_ntp_config_template(cfg, mycloud, ntp_conf)
         content = util.read_file_or_url('file://' + ntp_conf).contents
         self.assertEqual(
@@ -159,7 +173,7 @@ class TestNtp(FilesystemMockingTestCase):
         # Create ntp.conf.tmpl.<distro>
         with open('{0}.{1}.tmpl'.format(ntp_conf, distro), 'wb') as stream:
             stream.write(NTP_TEMPLATE)
-        with mock.patch('cloudinit.config.cc_ntp.NTP_CONF', ntp_conf):
+        with mock.patch('cloudinit.config.cc_ntp.NTP_CONF_FILE', ntp_conf):
             cc_ntp.write_ntp_config_template(cfg, mycloud, ntp_conf)
         content = util.read_file_or_url('file://' + ntp_conf).contents
         self.assertEqual(
@@ -178,7 +192,7 @@ class TestNtp(FilesystemMockingTestCase):
         # Create ntp.conf.tmpl
         with open('{0}.tmpl'.format(ntp_conf), 'wb') as stream:
             stream.write(NTP_TEMPLATE)
-        with mock.patch('cloudinit.config.cc_ntp.NTP_CONF', ntp_conf):
+        with mock.patch('cloudinit.config.cc_ntp.NTP_CONF_FILE', ntp_conf):
             cc_ntp.write_ntp_config_template({}, mycloud, ntp_conf)
         content = util.read_file_or_url('file://' + ntp_conf).contents
         default_pools = [
@@ -210,7 +224,7 @@ class TestNtp(FilesystemMockingTestCase):
         # Create ntp.conf.tmpl
         with open('{0}.tmpl'.format(ntp_conf), 'wb') as stream:
             stream.write(NTP_TEMPLATE)
-        with mock.patch('cloudinit.config.cc_ntp.NTP_CONF', ntp_conf):
+        with mock.patch('cloudinit.config.cc_ntp.NTP_CONF_FILE', ntp_conf):
             with mock.patch.object(util, 'which', return_value=None):
                 cc_ntp.handle('notimportant', cfg, mycloud, None, None)
 
@@ -239,7 +253,10 @@ class TestNtp(FilesystemMockingTestCase):
         with open(template, 'wb') as stream:
             stream.write(TIMESYNCD_TEMPLATE)
 
-        with mock.patch('cloudinit.config.cc_ntp.TIMESYNCD_CONF', tsyncd_conf):
+        with mock.patch(
+                'cloudinit.config.cc_ntp.TIMESYNCD_CONF_FILE',
+                tsyncd_conf
+        ):
             cc_ntp.handle('notimportant', cfg, mycloud, None, None)
 
         content = util.read_file_or_url('file://' + tsyncd_conf).contents
@@ -267,7 +284,7 @@ class TestNtp(FilesystemMockingTestCase):
             shutil.copy(
                 tmpl_file,
                 os.path.join(self.new_root, 'ntp.conf.%s.tmpl' % distro))
-            with mock.patch('cloudinit.config.cc_ntp.NTP_CONF', ntp_conf):
+            with mock.patch('cloudinit.config.cc_ntp.NTP_CONF_FILE', ntp_conf):
                 with mock.patch.object(util, 'which', return_value=[True]):
                     cc_ntp.handle('notimportant', cfg, mycloud, None, None)
 
@@ -300,7 +317,7 @@ class TestNtp(FilesystemMockingTestCase):
         with open('{0}.tmpl'.format(ntp_conf), 'wb') as stream:
             stream.write(NTP_TEMPLATE)
         for valid_empty_config in valid_empty_configs:
-            with mock.patch('cloudinit.config.cc_ntp.NTP_CONF', ntp_conf):
+            with mock.patch('cloudinit.config.cc_ntp.NTP_CONF_FILE', ntp_conf):
                 cc_ntp.handle('cc_ntp', valid_empty_config, cc, None, [])
             with open(ntp_conf) as stream:
                 content = stream.read()
@@ -323,7 +340,7 @@ class TestNtp(FilesystemMockingTestCase):
         ntp_conf = os.path.join(self.new_root, 'ntp.conf')
         with open('{0}.tmpl'.format(ntp_conf), 'wb') as stream:
             stream.write(NTP_TEMPLATE)
-        with mock.patch('cloudinit.config.cc_ntp.NTP_CONF', ntp_conf):
+        with mock.patch('cloudinit.config.cc_ntp.NTP_CONF_FILE', ntp_conf):
             cc_ntp.handle('cc_ntp', invalid_config, cc, None, [])
         self.assertIn(
             "Invalid config:\nntp.pools.0: 123 is not of type 'string'\n"
@@ -344,7 +361,7 @@ class TestNtp(FilesystemMockingTestCase):
         ntp_conf = os.path.join(self.new_root, 'ntp.conf')
         with open('{0}.tmpl'.format(ntp_conf), 'wb') as stream:
             stream.write(NTP_TEMPLATE)
-        with mock.patch('cloudinit.config.cc_ntp.NTP_CONF', ntp_conf):
+        with mock.patch('cloudinit.config.cc_ntp.NTP_CONF_FILE', ntp_conf):
             cc_ntp.handle('cc_ntp', invalid_config, cc, None, [])
         self.assertIn(
             "Invalid config:\nntp.pools: 123 is not of type 'array'\n"
@@ -366,7 +383,7 @@ class TestNtp(FilesystemMockingTestCase):
         ntp_conf = os.path.join(self.new_root, 'ntp.conf')
         with open('{0}.tmpl'.format(ntp_conf), 'wb') as stream:
             stream.write(NTP_TEMPLATE)
-        with mock.patch('cloudinit.config.cc_ntp.NTP_CONF', ntp_conf):
+        with mock.patch('cloudinit.config.cc_ntp.NTP_CONF_FILE', ntp_conf):
             cc_ntp.handle('cc_ntp', invalid_config, cc, None, [])
         self.assertIn(
             "Invalid config:\nntp: Additional properties are not allowed "
@@ -391,7 +408,7 @@ class TestNtp(FilesystemMockingTestCase):
         ntp_conf = os.path.join(self.new_root, 'ntp.conf')
         with open('{0}.tmpl'.format(ntp_conf), 'wb') as stream:
             stream.write(NTP_TEMPLATE)
-        with mock.patch('cloudinit.config.cc_ntp.NTP_CONF', ntp_conf):
+        with mock.patch('cloudinit.config.cc_ntp.NTP_CONF_FILE', ntp_conf):
             cc_ntp.handle('cc_ntp', invalid_config, cc, None, [])
         self.assertIn(
             "Invalid config:\nntp.pools: ['0.mypool.org', '0.mypool.org'] has "
@@ -421,7 +438,10 @@ class TestNtp(FilesystemMockingTestCase):
         print(template)
         with open(template, 'wb') as stream:
             stream.write(TIMESYNCD_TEMPLATE)
-        with mock.patch('cloudinit.config.cc_ntp.TIMESYNCD_CONF', tsyncd_conf):
+        with mock.patch(
+                'cloudinit.config.cc_ntp.TIMESYNCD_CONF_FILE',
+                tsyncd_conf
+        ):
             cc_ntp.write_ntp_config_template(cfg, mycloud, tsyncd_conf,
                                              template='timesyncd.conf')
 
@@ -442,7 +462,7 @@ class TestNtp(FilesystemMockingTestCase):
         # Create ntp.conf.tmpl
         with open('{0}.tmpl'.format(ntp_conf), 'wb') as stream:
             stream.write(NTP_TEMPLATE)
-        with mock.patch('cloudinit.config.cc_ntp.NTP_CONF', ntp_conf):
+        with mock.patch('cloudinit.config.cc_ntp.NTP_CONF_FILE', ntp_conf):
             cc_ntp.write_ntp_config_template({}, mycloud, ntp_conf)
         content = util.read_file_or_url('file://' + ntp_conf).contents
         default_pools = [
@@ -456,5 +476,35 @@ class TestNtp(FilesystemMockingTestCase):
                 ",".join(default_pools)),
             self.logs.getvalue())
 
+    def test_ntp_handler_chrony(self):
+        """Test ntp handler configures chrony"""
+        distro = 'opensuse'
+        cfg = {
+            'servers': ['192.168.2.1', '192.168.2.2'],
+            'pools': ['0.mypool.org'],
+        }
+        mycloud = self._get_cloud(distro)
+        mycloud.timesync_client = 'chrony'
+        mycloud.timesync_service_name = 'chronyd'
+        chrony_conf = self.tmp_path("chrony.conf", self.new_root)
+        # Create chrony.conf.tmpl
+        template = '{0}.tmpl'.format(chrony_conf)
+        print(template)
+        with open(template, 'wb') as stream:
+            stream.write(CHRONY_TEMPLATE)
+        with mock.patch(
+                'cloudinit.config.cc_ntp.CHRONY_CONF_FILE',
+                chrony_conf
+        ):
+            cc_ntp.write_ntp_config_template(cfg, mycloud, chrony_conf,
+                                             template='chrony.conf')
+
+        content = util.read_file_or_url('file://' + chrony_conf).contents
+        expected = '# pools\n'
+        expected += 'pool 0.mypool.org iburst\n'
+        expected += '# servers\n'
+        expected += 'server 192.168.2.1 iburst\n'
+        expected += 'server 192.168.2.2 iburst\n\n'
+        self.assertEqual(expected, content.decode())
 
 # vi: ts=4 expandtab