cloud-init-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

Ryan Harper has proposed merging ~raharper/cloud-init:ntp-configure-timesyncd-fallback-lp-1686485 into cloud-init:master.

Requested reviews:
  Server Team CI bot (server-team-bot): continuous-integration
  cloud-init commiters (cloud-init-dev)
Related bugs:
  Bug #1686485 in cloud-init: "cc_ntp fails to work when deploying ubuntu-core"
  https://bugs.launchpad.net/cloud-init/+bug/1686485

For more details, see:
https://code.launchpad.net/~raharper/cloud-init/+git/cloud-init/+merge/328427

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
-- 
Your team cloud-init commiters is requested to review the proposed merge of ~raharper/cloud-init:ntp-configure-timesyncd-fallback-lp-1686485 into cloud-init:master.

diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py
index 31ed64e..f28fbb0 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,51 @@ 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'
+        target_conf = NTP_CONF
+        template_name = None
+        packages = ['ntp']
+        check_exe = 'ntpd'
+    else:
+        service_name = 'systemd-timesyncd'
+        target_conf = 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, template=template_name,
+                              target=target_conf)
+    install_ntp(cloud.distro.install_packages, packages=packages,
+                check_exe=check_exe)
+
     try:
-        reload_ntp(systemd=cloud.distro.uses_systemd())
+        reload_ntp(systemd=cloud.distro.uses_systemd(), service=service_name)
     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 +188,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 +203,7 @@ def generate_server_names(distro):
     return names
 
 
-def write_ntp_config_template(cfg, cloud):
+def write_ntp_config_template(cfg, cloud, template=None, target=None):
     servers = cfg.get('servers', [])
     pools = cfg.get('pools', [])
 
@@ -185,19 +217,26 @@ def write_ntp_config_template(cfg, cloud):
         'pools': pools,
     }
 
-    template_fn = cloud.get_template_filename('ntp.conf.%s' %
-                                              (cloud.distro.name))
+    print(params)
+    if template is None:
+        template = 'ntp.conf.%s' % cloud.distro.name
+
+    if target is None:
+        target = NTP_CONF
+
+    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"), target)
 
-    templater.render_to_file(template_fn, NTP_CONF, params)
+    templater.render_to_file(template_fn, target, params)
 
 
-def reload_ntp(systemd=False):
-    service = 'ntp'
+def reload_ntp(systemd=False, service=None):
+    if service is None:
+        service = 'ntp'
     if systemd:
         cmd = ['systemctl', 'reload-or-restart', service]
     else:
diff --git a/templates/timesysncd.conf.tmpl b/templates/timesysncd.conf.tmpl
new file mode 100644
index 0000000..6b98301
--- /dev/null
+++ b/templates/timesysncd.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..76ad609 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()
+        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(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(systemd=True, service=service)
+        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)
@@ -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,31 @@ 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,
+                                             template='timesyncd.conf',
+                                             target=tsyncd_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