Merge ~raharper/cloud-init:ntp-configure-timesyncd-fallback-lp-1686485 into cloud-init:master

[Ryan Harper]

cc_ntp: fallback on timesyncd configuration if ntp is not installable

Some systems like Ubuntu-Core do not provide an ntp package for
installation but do include systemd-timesyncd (an ntp client).
On such systems cloud-init will generate a timesyncd configuration
using the 'servers' and 'pools' values as ntp hosts for timesyncd to use.

LP: #1686485

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

PASSED: Continuous integration, rev:2a2e3a9ea8b6e3dd84e523943ee663eaf42eb6bf
https://jenkins.ubuntu.com/server/job/cloud-init-ci/109/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: CentOS 6 & 7: Build & Test
    IN_PROGRESS: Declarative: Post Actions

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

review: Approve (continuous-integration)

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

PASSED: Continuous integration, rev:baadcc70ca7d4785768e01b0958dffc4378f217c
https://jenkins.ubuntu.com/server/job/cloud-init-ci/110/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: CentOS 6 & 7: Build & Test
    IN_PROGRESS: Declarative: Post Actions

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

review: Approve (continuous-integration)

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

PASSED: Continuous integration, rev:b1a0b8848d8cb7cc3f8dea7b45c319ef28e97db8
https://jenkins.ubuntu.com/server/job/cloud-init-ci/111/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: CentOS 6 & 7: Build & Test
    IN_PROGRESS: Declarative: Post Actions

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

review: Approve (continuous-integration)

[Scott Moser (smoser)]

some small things. i assume you were as much trying to avoid test changes as anything else, but the defaults we have there seem simplistic.

[Ryan Harper (raharper)]

Thanks for the review, I'll fix the issues you raised and push in an update.

[Ryan Harper (raharper)]

Fix suggestions, rebased and updated.

[Scott Moser (smoser)]

review: Approve

diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py
index 31ed64e..a02b4bf 100644
--- a/cloudinit/config/cc_ntp.py
+++ b/cloudinit/config/cc_ntp.py
@@ -50,6 +50,7 @@ LOG = logging.getLogger(__name__)
 
 frequency = PER_INSTANCE
 NTP_CONF = '/etc/ntp.conf'
+TIMESYNCD_CONF = '/etc/systemd/timesyncd.conf.d/cloud-init.conf'
 NR_POOL_SERVERS = 4
 distros = ['centos', 'debian', 'fedora', 'opensuse', 'ubuntu']
 
@@ -132,20 +133,50 @@ def handle(name, cfg, cloud, log, _args):
                             " is a %s %instead"), type_utils.obj_name(ntp_cfg))
 
     validate_cloudconfig_schema(cfg, schema)
+    if ntp_installable():
+        service_name = 'ntp'
+        confpath = NTP_CONF
+        template_name = None
+        packages = ['ntp']
+        check_exe = 'ntpd'
+    else:
+        service_name = 'systemd-timesyncd'
+        confpath = TIMESYNCD_CONF
+        template_name = 'timesyncd.conf'
+        packages = []
+        check_exe = '/lib/systemd/systemd-timesyncd'
+
     rename_ntp_conf()
     # 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)
-    install_ntp(cloud.distro.install_packages, packages=['ntp'],
-                check_exe="ntpd")
-    # if ntp was already installed, it may not have started
+    write_ntp_config_template(ntp_cfg, cloud, confpath, template=template_name)
+    install_ntp(cloud.distro.install_packages, packages=packages,
+                check_exe=check_exe)
+
     try:
-        reload_ntp(systemd=cloud.distro.uses_systemd())
+        reload_ntp(service_name, systemd=cloud.distro.uses_systemd())
     except util.ProcessExecutionError as e:
         LOG.exception("Failed to reload/start ntp service: %s", e)
         raise
 
 
+def ntp_installable():
+    """Check if we can install ntp package
+
+    Ubuntu-Core systems do not have an ntp package available, so
+    we always return False.  Other systems require package managers to install
+    the ntp package If we fail to find one of the package managers, then we
+    cannot install ntp.
+    """
+    if util.system_is_snappy():
+        return False
+
+    if any(map(util.which, ['apt-get', 'dnf', 'yum', 'zypper'])):
+        return True
+
+    return False
+
+
 def install_ntp(install_func, packages=None, check_exe="ntpd"):
     if util.which(check_exe):
         return
@@ -156,7 +187,7 @@ def install_ntp(install_func, packages=None, check_exe="ntpd"):
 
 
 def rename_ntp_conf(config=None):
-    """Rename any existing ntp.conf file and render from template"""
+    """Rename any existing ntp.conf file"""
     if config is None:  # For testing
         config = NTP_CONF
     if os.path.exists(config):
@@ -171,7 +202,7 @@ def generate_server_names(distro):
     return names
 
 
-def write_ntp_config_template(cfg, cloud):
+def write_ntp_config_template(cfg, cloud, path, template=None):
     servers = cfg.get('servers', [])
     pools = cfg.get('pools', [])
 
@@ -185,19 +216,20 @@ def write_ntp_config_template(cfg, cloud):
         'pools': pools,
     }
 
-    template_fn = cloud.get_template_filename('ntp.conf.%s' %
-                                              (cloud.distro.name))
+    if template is None:
+        template = 'ntp.conf.%s' % cloud.distro.name
+
+    template_fn = cloud.get_template_filename(template)
     if not template_fn:
         template_fn = cloud.get_template_filename('ntp.conf')
         if not template_fn:
             raise RuntimeError(("No template found, "
-                                "not rendering %s"), NTP_CONF)
+                                "not rendering %s"), path)
 
-    templater.render_to_file(template_fn, NTP_CONF, params)
+    templater.render_to_file(template_fn, path, params)
 
 
-def reload_ntp(systemd=False):
-    service = 'ntp'
+def reload_ntp(service, systemd=False):
     if systemd:
         cmd = ['systemctl', 'reload-or-restart', service]
     else:
diff --git a/templates/timesyncd.conf.tmpl b/templates/timesyncd.conf.tmpl
new file mode 100644
index 0000000..6b98301
--- /dev/null
+++ b/templates/timesyncd.conf.tmpl
@@ -0,0 +1,8 @@
+## template:jinja
+# cloud-init generated file
+# See timesyncd.conf(5) for details.
+
+[Time]
+{% if servers or pools -%}
+NTP={% for host in servers|list + pools|list %}{{ host }} {% endfor -%}
+{% endif -%}
diff --git a/tests/unittests/test_handler/test_handler_ntp.py b/tests/unittests/test_handler/test_handler_ntp.py
index 7f27864..83d5faa 100644
--- a/tests/unittests/test_handler/test_handler_ntp.py
+++ b/tests/unittests/test_handler/test_handler_ntp.py
@@ -16,6 +16,14 @@ servers {{servers}}
 pools {{pools}}
 """
 
+TIMESYNCD_TEMPLATE = b"""\
+## template:jinja
+[Time]
+{% if servers or pools -%}
+NTP={% for host in servers|list + pools|list %}{{ host }} {% endfor -%}
+{% endif -%}
+"""
+
 try:
     import jsonschema
     assert jsonschema  # avoid pyflakes error F401: import unused
@@ -59,6 +67,14 @@ class TestNtp(FilesystemMockingTestCase):
         cc_ntp.install_ntp(install_func, packages=['ntp'], check_exe='ntpd')
         install_func.assert_not_called()
 
+    @mock.patch("cloudinit.config.cc_ntp.util")
+    def test_ntp_install_no_op_with_empty_pkg_list(self, mock_util):
+        """ntp_install calls install_func with empty list"""
+        mock_util.which.return_value = None  # check_exe not found
+        install_func = mock.MagicMock()
+        cc_ntp.install_ntp(install_func, packages=[], check_exe='timesyncd')
+        install_func.assert_called_once_with([])
+
     def test_ntp_rename_ntp_conf(self):
         """When NTP_CONF exists, rename_ntp moves it."""
         ntpconf = self.tmp_path("ntp.conf", self.new_root)
@@ -68,6 +84,30 @@ class TestNtp(FilesystemMockingTestCase):
         self.assertFalse(os.path.exists(ntpconf))
         self.assertTrue(os.path.exists("{0}.dist".format(ntpconf)))
 
+    @mock.patch("cloudinit.config.cc_ntp.util")
+    def test_reload_ntp_defaults(self, mock_util):
+        """Test service is restarted/reloaded (defaults)"""
+        service = 'ntp'
+        cmd = ['service', service, 'restart']
+        cc_ntp.reload_ntp(service)
+        mock_util.subp.assert_called_with(cmd, capture=True)
+
+    @mock.patch("cloudinit.config.cc_ntp.util")
+    def test_reload_ntp_systemd(self, mock_util):
+        """Test service is restarted/reloaded (systemd)"""
+        service = 'ntp'
+        cmd = ['systemctl', 'reload-or-restart', service]
+        cc_ntp.reload_ntp(service, systemd=True)
+        mock_util.subp.assert_called_with(cmd, capture=True)
+
+    @mock.patch("cloudinit.config.cc_ntp.util")
+    def test_reload_ntp_systemd_timesycnd(self, mock_util):
+        """Test service is restarted/reloaded (systemd/timesyncd)"""
+        service = 'systemd-timesycnd'
+        cmd = ['systemctl', 'reload-or-restart', service]
+        cc_ntp.reload_ntp(service, systemd=True)
+        mock_util.subp.assert_called_with(cmd, capture=True)
+
     def test_ntp_rename_ntp_conf_skip_missing(self):
         """When NTP_CONF doesn't exist rename_ntp doesn't create a file."""
         ntpconf = self.tmp_path("ntp.conf", self.new_root)
@@ -94,7 +134,7 @@ class TestNtp(FilesystemMockingTestCase):
         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):
-            cc_ntp.write_ntp_config_template(cfg, mycloud)
+            cc_ntp.write_ntp_config_template(cfg, mycloud, ntp_conf)
         content = util.read_file_or_url('file://' + ntp_conf).contents
         self.assertEqual(
             "servers ['192.168.2.1', '192.168.2.2']\npools []\n",
@@ -120,7 +160,7 @@ class TestNtp(FilesystemMockingTestCase):
         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):
-            cc_ntp.write_ntp_config_template(cfg, mycloud)
+            cc_ntp.write_ntp_config_template(cfg, mycloud, ntp_conf)
         content = util.read_file_or_url('file://' + ntp_conf).contents
         self.assertEqual(
             "servers []\npools ['10.0.0.1', '10.0.0.2']\n",
@@ -139,7 +179,7 @@ class TestNtp(FilesystemMockingTestCase):
         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):
-            cc_ntp.write_ntp_config_template({}, mycloud)
+            cc_ntp.write_ntp_config_template({}, mycloud, ntp_conf)
         content = util.read_file_or_url('file://' + ntp_conf).contents
         default_pools = [
             "{0}.{1}.pool.ntp.org".format(x, distro)
@@ -152,7 +192,8 @@ class TestNtp(FilesystemMockingTestCase):
                 ",".join(default_pools)),
             self.logs.getvalue())
 
-    def test_ntp_handler_mocked_template(self):
+    @mock.patch("cloudinit.config.cc_ntp.ntp_installable")
+    def test_ntp_handler_mocked_template(self, m_ntp_install):
         """Test ntp handler renders ubuntu ntp.conf template."""
         pools = ['0.mycompany.pool.ntp.org', '3.mycompany.pool.ntp.org']
         servers = ['192.168.23.3', '192.168.23.4']
@@ -164,6 +205,8 @@ class TestNtp(FilesystemMockingTestCase):
         }
         mycloud = self._get_cloud('ubuntu')
         ntp_conf = self.tmp_path('ntp.conf', self.new_root)  # Doesn't exist
+        m_ntp_install.return_value = True
+
         # Create ntp.conf.tmpl
         with open('{0}.tmpl'.format(ntp_conf), 'wb') as stream:
             stream.write(NTP_TEMPLATE)
@@ -176,6 +219,34 @@ class TestNtp(FilesystemMockingTestCase):
             'servers {0}\npools {1}\n'.format(servers, pools),
             content.decode())
 
+    @mock.patch("cloudinit.config.cc_ntp.util")
+    def test_ntp_handler_mocked_template_snappy(self, m_util):
+        """Test ntp handler renders timesycnd.conf template on snappy."""
+        pools = ['0.mycompany.pool.ntp.org', '3.mycompany.pool.ntp.org']
+        servers = ['192.168.23.3', '192.168.23.4']
+        cfg = {
+            'ntp': {
+                'pools': pools,
+                'servers': servers
+            }
+        }
+        mycloud = self._get_cloud('ubuntu')
+        m_util.system_is_snappy.return_value = True
+
+        # Create timesyncd.conf.tmpl
+        tsyncd_conf = self.tmp_path("timesyncd.conf", self.new_root)
+        template = '{0}.tmpl'.format(tsyncd_conf)
+        with open(template, 'wb') as stream:
+            stream.write(TIMESYNCD_TEMPLATE)
+
+        with mock.patch('cloudinit.config.cc_ntp.TIMESYNCD_CONF', tsyncd_conf):
+            cc_ntp.handle('notimportant', cfg, mycloud, None, None)
+
+        content = util.read_file_or_url('file://' + tsyncd_conf).contents
+        self.assertEqual(
+            "[Time]\nNTP=%s %s \n" % (" ".join(servers), " ".join(pools)),
+            content.decode())
+
     def test_ntp_handler_real_distro_templates(self):
         """Test ntp handler renders the shipped distro ntp.conf templates."""
         pools = ['0.mycompany.pool.ntp.org', '3.mycompany.pool.ntp.org']
@@ -333,4 +404,30 @@ class TestNtp(FilesystemMockingTestCase):
             "pools ['0.mypool.org', '0.mypool.org']\n",
             content)
 
+    @mock.patch("cloudinit.config.cc_ntp.ntp_installable")
+    def test_ntp_handler_timesyncd(self, m_ntp_install):
+        """Test ntp handler configures timesyncd"""
+        m_ntp_install.return_value = False
+        distro = 'ubuntu'
+        cfg = {
+            'servers': ['192.168.2.1', '192.168.2.2'],
+            'pools': ['0.mypool.org'],
+        }
+        mycloud = self._get_cloud(distro)
+        tsyncd_conf = self.tmp_path("timesyncd.conf", self.new_root)
+        # Create timesyncd.conf.tmpl
+        template = '{0}.tmpl'.format(tsyncd_conf)
+        print(template)
+        with open(template, 'wb') as stream:
+            stream.write(TIMESYNCD_TEMPLATE)
+        with mock.patch('cloudinit.config.cc_ntp.TIMESYNCD_CONF', tsyncd_conf):
+            cc_ntp.write_ntp_config_template(cfg, mycloud, tsyncd_conf,
+                                             template='timesyncd.conf')
+
+        content = util.read_file_or_url('file://' + tsyncd_conf).contents
+        self.assertEqual(
+            "[Time]\nNTP=192.168.2.1 192.168.2.2 0.mypool.org \n",
+            content.decode())
+
+
 # vi: ts=4 expandtab