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. _______________________________________________ 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