cloud-init-dev team mailing list archive

[Chad Smith <chad.smith@xxxxxxxxxxxxx>]

Diff comments:

> diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py
> index cbd0237..57a732f 100644
> --- a/cloudinit/config/cc_ntp.py
> +++ b/cloudinit/config/cc_ntp.py
> @@ -94,68 +264,84 @@ schema = {
>  __doc__ = get_schema_doc(schema)  # Supplement python help()
>  
>  
> -def handle(name, cfg, cloud, log, _args):
> -    """Enable and configure ntp."""
> -    if 'ntp' not in cfg:
> -        LOG.debug(
> -            "Skipping module named %s, not present or disabled by cfg", name)
> -        return
> -    ntp_cfg = cfg['ntp']
> -    if ntp_cfg is None:
> -        ntp_cfg = {}  # Allow empty config which will install the package
> +def distro_ntp_client_configs(distro):
> +    """Construct a distro-specific ntp client config dictionary by merging
> +       distro specific changes into base config.
>  
> -    # TODO drop this when validate_cloudconfig_schema is strict=True
> -    if not isinstance(ntp_cfg, (dict)):
> -        raise RuntimeError(
> -            "'ntp' key existed in config, but not a dictionary type,"
> -            " is a {_type} instead".format(_type=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, confpath, template=template_name)
> -    install_ntp(cloud.distro.install_packages, packages=packages,
> -                check_exe=check_exe)
> -
> -    try:
> -        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
> +    @param distro: String providing the distro class name.
> +    @returns: Dict of distro configurations for ntp clients.
> +    """
> +    dcfg = DISTRO_CLIENT_CONFIG
> +    cfg = copy.deepcopy(NTP_CLIENT_CONFIG)
> +    if distro in dcfg:
> +        cfg = util.mergemanydict([cfg, dcfg[distro]], reverse=True)
> +    return cfg
>  
>  
> -def ntp_installable():
> -    """Check if we can install ntp package
> +def select_ntp_client(usercfg, distro):

Can we make this api more explicit to only take a ntp_client_name instead of the whole config dict since that's the only value or None that we are looking at here?  the callsite  select_ntp_client(ntp_cfg.get('ntp_client', 'auto'), cloud.distro) would work here. Then we know what we are actually relying on without having to read all of select_ntp_client.

> +    """Determine which ntp client is to be used, consulting the distro
> +       for its preference.
>  
> -    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.
> +    @param usercfg: Dict of user-provided cloud-config.
> +    @param distro: Distro class instance.
> +    @returns: Dict of the selected ntp client or {} if none selected.
>      """
> -    if util.system_is_snappy():
> -        return False
>  
> -    if any(map(util.which, ['apt-get', 'dnf', 'yum', 'zypper'])):
> -        return True
> +    # construct distro-specific ntp_client_config dict
> +    distro_cfg = distro_ntp_client_configs(distro.name)
> +
> +    # user specified client, return its config
> +    if 'ntp_client' in usercfg:

This logic won't work if the cloud-config provides 'ntp_client': 'auto'. Can we change the logic a bit here to permit that value from the user. since we document it?

> +        LOG.debug('Selected NTP client "%s" via user-data configuration',
> +                  usercfg.get('ntp_client'))
> +        return distro_cfg.get(usercfg.get('ntp_client'))
> +
> +    # default to auto if unset in distro
> +    distro_ntp_client = distro.get_option('ntp_client', 'auto')
> +
> +    clientcfg = {}
> +    if distro_ntp_client == "auto":
> +        for client in distro.preferred_ntp_clients:
> +            cfg = distro_cfg.get(client)
> +            if not cfg:
> +                LOG.warning(
> +                    'Skipping NTP client "%s", no configuration available',
> +                    client)
> +                continue
> +            check = cfg.get('check_exe')
> +            if not check:

These checks are on the distro default client configuration, which is all hard-coded in cc_ntp.py right? Shouldn't we instead (or additionally) be validating user-data which could accidentally miss the key definitions? Maybe we just move all ntp configuration validation into a single validate_ntp_config function after we've merged user-provided into distro-provided builtin config as called from handle(). then we can be certain we cover both our builtins and user-config content.

> +                LOG.warning(
> +                    'Skipping NTP client "%s", missing "check_exe" option',
> +                    client)
> +                continue
> +            if util.which(check):
> +                LOG.debug('Selected NTP client "%s", already installed',
> +                          client)
> +                clientcfg = cfg
> +                break
> +
> +        if not clientcfg:
> +            # cloud-init has never automatically installed ntp clients without
> +            # user-data configuring ntp so we must keep the same behavior here.
> +            LOG.debug('ntp_client="auto" and no built-in NTP clients found,'
> +                      'skipping automatic configuration')
> +    else:
> +        LOG.debug('Selected NTP client "%s" via distro system config',
> +                  distro_ntp_client)
> +        clientcfg = distro_cfg.get(distro_ntp_client, {})
> +
> +    return clientcfg
>  
> -    return False
>  
> +def install_ntp_client(install_func, packages=None, check_exe="ntpd"):
> +    """Install ntp client package if not already installed.
>  
> -def install_ntp(install_func, packages=None, check_exe="ntpd"):
> +    @param install_func: function.  This parameter is invoked with the contents
> +    of the packages parameter.
> +    @param packages: list.  This parameter defaults to ['ntp'].
> +    @param check_exec: string.  The name of a binary that indicates the package
> +    the specified package is already installed.
> +    """
>      if util.which(check_exe):
>          return
>      if packages is None:
> @@ -185,34 +379,60 @@ def generate_server_names(distro):
>      return names
>  
>  
> -def write_ntp_config_template(cfg, cloud, path, template=None):
> -    servers = cfg.get('servers', [])
> -    pools = cfg.get('pools', [])
> +def write_ntp_config_template(distro_name, servers=None, pools=None,
> +                              path=None, template_fn=None, template=None):
> +    """Render a ntp client configuration for the specified client.
> +
> +    @param distro_name: string.  The distro class name.
> +    @param servers: A list of strings specifying ntp servers. Defaults to empty
> +    list.
> +    @param pools: A list of strings specifying ntp pools. Defaults to empty
> +    list.
> +    @param path: A string to specify where to write the rendered template.
> +    @param template_fn: A string to specify the template source file.
> +    @param template: A string specifying the contents of the template. This
> +    content will be written to a temporary file before being used to render
> +    the configuration file.
> +
> +    @raises: ValueError when path is None.
> +    @raises: ValueError when template_fn is None and template is None.
> +    """
> +    if not servers:
> +        servers = []
> +    if not pools:
> +        pools = []
>  
>      if len(servers) == 0 and len(pools) == 0:
> -        pools = generate_server_names(cloud.distro.name)
> +        pools = generate_server_names(distro_name)
>          LOG.debug(
>              'Adding distro default ntp pool servers: %s', ','.join(pools))
>  
> -    params = {
> -        'servers': servers,
> -        'pools': pools,
> -    }
> +    if not path:
> +        raise ValueError('Invalid value for path parameter')
>  
> -    if template is None:
> -        template = 'ntp.conf.%s' % cloud.distro.name
> +    if not template_fn and not template:

cloud be:     if not any([template_fn, template]):

> +        raise ValueError('Not template_fn or template provided')

'Not template_fn or template provided'  ->  'Missing values for both template_fn and template'

>  
> -    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 {path}'.format(path=path))
> +    params = {'servers': servers, 'pools': pools}
> +    if template:
> +        tfile = temp_utils.mkstemp(prefix='template_name-', suffix=".tmpl")
> +        template_fn = tfile[1]  # filepath is second item in tuple
> +        util.write_file(template_fn, content=template)
>  
>      templater.render_to_file(template_fn, path, params)
> +    # clean up temporary template
> +    if template:
> +        util.del_file(template_fn)
>  
>  
>  def reload_ntp(service, systemd=False):
> +    """Restart or reload an ntp system service.
> +
> +    @param service: A string specifying the name of the service to be affected.
> +    @param systemd: A boolean indicating if the distro uses systemd, defaults
> +    to False.
> +    @returns: A tuple of stdout, stderr results from executing the action.
> +    """
>      if systemd:
>          cmd = ['systemctl', 'reload-or-restart', service]
>      else:
> diff --git a/tests/cloud_tests/testcases/modules/ntp_timesyncd.py b/tests/cloud_tests/testcases/modules/ntp_timesyncd.py
> new file mode 100644
> index 0000000..d433eaf
> --- /dev/null
> +++ b/tests/cloud_tests/testcases/modules/ntp_timesyncd.py
> @@ -0,0 +1,22 @@
> +# This file is part of cloud-init. See LICENSE file for license information.
> +
> +"""cloud-init Integration Test Verify Script."""
> +from tests.cloud_tests.testcases import base
> +
> +
> +class TestNtpTimesyncd(base.CloudTestCase):
> +    """Test ntp module with systemd-timesyncd client"""
> +
> +# vi: ts=4 expandtab
> +
> +    def test_timesyncd_conf_exists(self):

could drop this test as we should expect content in test_timesyncd_entries below.

> +        """Test timesyncd configured"""
> +        out = self.get_data_file('timesyncd_conf_exists')
> +        self.assertEqual(0, int(out))
> +
> +    def test_timesyncd_entires(self):

spelling: entries

> +        """Test timesyncd config entries"""
> +        out = self.get_data_file('timesyncd_conf')
> +        self.assertIn('.pool.ntp.org', out)
> +
> +# vi: ts=4 expandtab


-- 
https://code.launchpad.net/~raharper/cloud-init/+git/cloud-init/+merge/339438
Your team cloud-init commiters is requested to review the proposed merge of ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master.