Will grab some of these as well, open questions inline.
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):
Yes, that makes sense; The path here evolved from merging the config within
this method to outside so it make perfect sense to only pass what's needed here.
> + """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:
Hrm. It does seem odd to say you want the distro to select which it selects by
default, so let's walk this out a bit. The one case that's not clear is if the
distro has selected a client that's not auto, and the user set's auto; does
that imply that we want to search the distro.preferred_ntp_clients list over
the singluar value in the distro ntp_client setting?
> + 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:
We could drop the checks here, they're provided in code for any of the distro
preferred clients. In the case the user is overriding the client config, as
long as they pass the schema check, then all should be fine here, no?
If the user is specifying a custom client, then the schema validation should
cover any thing there.
> + 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:
timeit says two inverted boolean checks are much faster than any of a list of
bools.
I also find it clearer code-wise.
> + raise ValueError('Not template_fn or template provided')
+1
>
> - 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:
--
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.
_______________________________________________
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
