Diff comments:
> diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py > index cbd0237..099b8d8 100644 > --- a/cloudinit/config/cc_ntp.py > +++ b/cloudinit/config/cc_ntp.py > @@ -103,6 +171,9 @@ def handle(name, cfg, cloud, log, _args): > ntp_cfg = cfg['ntp'] > if ntp_cfg is None: > ntp_cfg = {} # Allow empty config which will install the package > + else: > + # do not allow handle updates to modify the cfg object > + ntp_cfg = cfg['ntp'].copy() I'm happy to change, but I'm not aware of the difference between copy/deep_copy when it comes to a dictionary of basic types per the schema. > > # TODO drop this when validate_cloudconfig_schema is strict=True > if not isinstance(ntp_cfg, (dict)): > diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py > index 55260ea..59bb9f9 100755 > --- a/cloudinit/distros/__init__.py > +++ b/cloudinit/distros/__init__.py > @@ -49,6 +49,48 @@ LOG = logging.getLogger(__name__) > # It could break when Amazon adds new regions and new AZs. > _EC2_AZ_RE = re.compile('^[a-z][a-z]-(?:[a-z]+-)+[0-9][a-z]$') > > +# Default NTP Client Configurations > +PREFERRED_NTP_CLIENTS = ['systemd-timesyncd', 'ntp', 'ntpdate'] The thought here is what clients might by default alread be installed. While chrony is the new shiny, I think more distros are likley to have these clients installed by default; this was a reasonable default vs. distro specific policy. > +NTP_CONF = '/etc/ntp.conf' > +NTP_CLIENT_CONFIG = { > + 'chrony': { > + 'check_exe': 'chronyd', > + 'confpath': '/etc/chrony.conf', > + 'name': 'chrony', > + 'packages': ['chrony'], > + 'service_name': 'chrony', > + 'template_name': 'chrony.conf.{distro}', > + 'template': None, > + }, > + 'ntp': { > + 'check_exe': 'ntpd', > + 'confpath': NTP_CONF, > + 'name': 'ntp', > + 'packages': ['ntp'], > + 'service_name': 'ntp', > + 'template_name': 'ntp.conf.{distro}', > + 'template': None, > + }, > + 'ntpdate': { > + 'check_exe': 'ntpdate', > + 'confpath': NTP_CONF, > + 'name': 'ntpdate', > + 'packages': ['ntpdate'], > + 'service_name': 'ntpdate', > + 'template_name': 'ntp.conf.{distro}', > + 'template': None, > + }, > + 'systemd-timesyncd': { > + 'check_exe': '/lib/systemd/systemd-timesyncd', > + 'confpath': '/etc/systemd/timesyncd.conf.d/cloud-init.conf', > + 'name': 'systemd-timesyncd', > + 'packages': [], > + 'service_name': 'systemd-timesyncd', > + 'template_name': 'timesyncd.conf', > + 'template': None, > + }, > +} > + > > @six.add_metaclass(abc.ABCMeta) > class Distro(object): > @@ -109,6 +153,17 @@ class Distro(object): > """Wrapper to report whether this distro uses systemd or sysvinit.""" > return uses_systemd() > > + def package_installer(self): > + """Wrapper to report whether this distro has a package installer.""" > + return package_installer() If this was named 'has_package_installer' would that hep? Recall this is to ask the distro if it _can_ install packages, not _install_ this package. > + > + def find_programs(self, programs=None, search=None, target=None): > + """Wrapper to find binaries on system, search for each element > + in programs, testing each path in search, under directory > + target. > + """ > + return find_programs(programs=programs, search=search, target=target) > + > @abc.abstractmethod > def package_command(self, cmd, args=None, pkgs=None): > raise NotImplementedError() > @@ -196,6 +251,69 @@ class Distro(object): > def get_locale(self): > raise NotImplementedError() > > + def get_ntp_server_name(self): > + return '%s.pool.ntp.org' % self.name We could incorporate the pool name into the dictionary; this was mostly to deal with the special case for opensuse/sles where sles must use the opensuse pool servers. > + > + def get_ntp_client_info(self, usercfg=None): > + """Search for installed clients from distro preferred list > + if client is set to 'auto' (default for unset) we search the > + list of clients from distro, if a client name is provied in Ack to spelling. This is where the cc_ntp passes in the combined user-data configuration, which is used to override the default distro configuration. > + config, then we return that. > + > + returns a dictionary of the selected ntp client configuration""" > + if not usercfg: > + usercfg = {} > + > + ntp_client = self.get_option("ntp_client", "auto") > + if 'ntp_client' in usercfg: > + ntp_client = usercfg.get('ntp_client') > + > + if ntp_client == "auto": > + # - Preference will be given to clients that are already > installed. > + # - If multiple ntp client packages are installed, the behavior > is > + # not defined other than that one will be selected and > configured > + # - If no ntp client packages are installed behavior is again > + # undefined. > + clients = self.preferred_ntp_clients > + else: > + # user provided client name > + clients = [ntp_client] > + # TODO: allow usercfg to include a config dict, merge with system > + > + # for each client, extract default config, use 'check_exe' option > + # to determine if client is already installed. > + for client in clients: > + # allow user to define config for a client > + if 'config' in usercfg: > + cfg = usercfg.get('config') > + else: > + cfg = self.ntp_client_config.get(client, {}) > + if not cfg: > + LOG.warning( It should; In the case where we may SRU this to a release which does not have json schema support, this is still reasonable no? It's warning because it's not fatal, if you want error and not fatal, that's fine. > + 'Skipping ntp client %s, no configuration available', > + client) > + continue > + check = cfg.get('check_exe') > + if not check: > + LOG.warning( > + 'Skipping ntp client %s, missing "check_exe" option', > + client) > + continue > + LOG.debug('Checking if ntp client "%s" is installed', check) > + if self.find_programs(programs=[check]): > + LOG.debug('Found ntp client installed: %s', client) > + return cfg > + > + # if the system has a package installer, then return the > configuration > + # of the first (possibly only) ntp client. > + if self.package_installer(): > + client = clients[0] > + LOG.debug('No preferred NTP clients found installed, selecting' > + ' "%s" for install', client) > + return self.ntp_client_config.get(client, {}) > + > + raise RuntimeError('No ntp client installed or available') > + > @abc.abstractmethod > def _read_hostname(self, filename, default=None): > raise NotImplementedError() > @@ -758,6 +876,41 @@ def set_etc_timezone(tz, tz_file=None, > tz_conf="/etc/timezone", > return > > > +def find_programs(programs=None, search=None, target=None): > + """Look for each program in path search under target""" > + if not programs: > + return [] > + > + if not search: This was mostly to allow pass-through down to util.which; In the case where we defined a custom config, the check_exec may be found in say, /opt/bin which is not typical to default system paths, but none-the-less, may be present in the image. > + search = os.environ.get('PATH') > + if not search: > + search = "/sbin:/usr/sbin:/bin:/usr/bin" > + > + found = [] > + for program in programs: > + if not util.which(program, search=search, target=target): > + continue > + found.append(program) > + > + return found > + > + > +def package_installer(): > + """Check if we can install packages > + > + Ubuntu-Core systems do not have an package installer available for the > core > + system, so we always return False. Other systems have package managers > to > + install additional packages. > + """ > + if util.system_is_snappy(): > + return False > + > + if any(map(util.which, ['apt-get', 'dnf', 'yum', 'zypper'])): > + return True > + > + return False > + > + > def uses_systemd(): > try: > res = os.lstat('/run/systemd/system') > diff --git a/cloudinit/distros/opensuse.py b/cloudinit/distros/opensuse.py > index a219e9f..92dfbc9 100644 > --- a/cloudinit/distros/opensuse.py > +++ b/cloudinit/distros/opensuse.py > @@ -35,6 +65,7 @@ class Distro(distros.Distro): > systemd_hostname_conf_fn = '/etc/hostname' > systemd_locale_conf_fn = '/etc/locale.conf' > tz_local_fn = '/etc/localtime' > + preferred_ntp_clients = ['ntp', 'systemd-timesyncd', 'chrony'] Ordered, and yes. > > def __init__(self, name, cfg, paths): > distros.Distro.__init__(self, name, cfg, paths) > diff --git a/tests/unittests/test_handler/test_handler_ntp.py > b/tests/unittests/test_handler/test_handler_ntp.py > index 28a8455..d7f313f 100644 > --- a/tests/unittests/test_handler/test_handler_ntp.py > +++ b/tests/unittests/test_handler/test_handler_ntp.py > @@ -147,43 +173,61 @@ class TestNtp(FilesystemMockingTestCase): > renders the value from the keys servers and pools. When no > servers value is present, template is rendered using an empty list. > """ > - distro = 'ubuntu' > cfg = { > - 'pools': ['10.0.0.1', '10.0.0.2'] > + 'pools': ['10.0.0.1', '10.0.0.2'], > } > - mycloud = self._get_cloud(distro) > - ntp_conf = self.tmp_path('ntp.conf', self.new_root) # Doesn't exist > - # Create ntp.conf.tmpl which isn't read > - with open('{0}.tmpl'.format(ntp_conf), 'wb') as stream: > - stream.write(b'NOT READ: ntp.conf.<distro>.tmpl is primary') > - # Create ntp.conf.tmpl.<distro> > - 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, 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", > - content.decode()) > > - def test_write_ntp_config_template_defaults_pools_when_empty_lists(self): > + for distro in cc_ntp.distros: > + mycloud = self._get_cloud(distro) > + confpath = self._write_ntp_template(NTP_TEMPLATE, mycloud.distro) > + > + cc_ntp.write_ntp_config_template(cfg, mycloud) > + content = util.read_file_or_url('file://' + confpath).contents > + self.assertEqual( > + "servers []\npools ['10.0.0.1', '10.0.0.2']\n", > + content.decode()) > + > + def test_write_ntp_config_template_defaults_pools_w_empty_lists(self): > """write_ntp_config_template defaults pools servers upon empty > config. > > When both pools and servers are empty, default NR_POOL_SERVERS get > configured. > """ > - distro = 'ubuntu' > + cfg = { > + 'pools': [], > + 'servers': [], > + } > + > + for distro in cc_ntp.distros: > + mycloud = self._get_cloud(distro) > + pools = cc_ntp.generate_server_names(mycloud.distro) > + cfg.update({'pools': pools}) > + > + confpath = self._write_ntp_template(NTP_TEMPLATE, mycloud.distro) > + > + cc_ntp.write_ntp_config_template(cfg, mycloud) > + content = util.read_file_or_url('file://' + confpath).contents > + self.assertEqual( > + "servers []\npools {}\n".format(pools), > + content.decode()) > + > + def test_defaults_pools_empty_lists_sles(self): > + """write_ntp_config_template defaults opensuse pools upon empty > config. > + > + When both pools and servers are empty, default NR_POOL_SERVERS get > + configured. > + """ > + distro = 'sles' > mycloud = self._get_cloud(distro) > - ntp_conf = self.tmp_path('ntp.conf', self.new_root) # Doesn't exist > - # Create ntp.conf.tmpl > - 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, ntp_conf) > - content = util.read_file_or_url('file://' + ntp_conf).contents > - default_pools = [ > - "{0}.{1}.pool.ntp.org".format(x, distro) > - for x in range(0, cc_ntp.NR_POOL_SERVERS)] > + default_pools = cc_ntp.generate_server_names(mycloud.distro) > + confpath = self._write_ntp_template(NTP_TEMPLATE, mycloud.distro) > + > + cc_ntp.write_ntp_config_template({}, mycloud) > + > + content = util.read_file_or_url('file://' + confpath).contents We could, this was code-motion. > + > + for pool in default_pools: > + self.assertIn('opensuse', pool) > self.assertEqual( > "servers []\npools {0}\n".format(default_pools), > content.decode()) > @@ -257,31 +270,56 @@ class TestNtp(FilesystemMockingTestCase): > 'servers': servers > } > } > - ntp_conf = self.tmp_path('ntp.conf', self.new_root) # Doesn't exist > - for distro in ('debian', 'ubuntu', 'fedora', 'rhel', 'sles'): > - mycloud = self._get_cloud(distro) > - root_dir = dirname(dirname(os.path.realpath(util.__file__))) > - tmpl_file = os.path.join( > - '{0}/templates/ntp.conf.{1}.tmpl'.format(root_dir, distro)) > - # Create a copy in our tmp_dir > - shutil.copy( > - tmpl_file, > - os.path.join(self.new_root, 'ntp.conf.%s.tmpl' % distro)) > - with mock.patch('cloudinit.config.cc_ntp.NTP_CONF', ntp_conf): > - with mock.patch.object(util, 'which', return_value=[True]): > - cc_ntp.handle('notimportant', cfg, mycloud, None, None) > - > - content = util.read_file_or_url('file://' + ntp_conf).contents > - expected_servers = '\n'.join([ > - 'server {0} iburst'.format(server) for server in servers]) > - self.assertIn( > - expected_servers, content.decode(), > - 'failed to render ntp.conf for distro:{0}'.format(distro)) > - expected_pools = '\n'.join([ > - 'pool {0} iburst'.format(pool) for pool in pools]) > - self.assertIn( > - expected_pools, content.decode(), > - 'failed to render ntp.conf for distro:{0}'.format(distro)) > + > + for client in ['ntp', 'systemd-timesyncd']: > + for distro in cc_ntp.distros: > + mycloud = self._get_cloud(distro) > + > + # FIXME: use _write_ntp_template here I wrote a helper to setup/configure the mocks later; it could be refactored to use that helper. > + self._patch_preferred_client(mycloud.distro, client=client) > + ntpclient = mycloud.distro.get_ntp_client_info() > + confpath = ntpclient.get('confpath') > + > + template = ntpclient.get('template_name') > + # find sourcetree template file > + root_dir = ( > + dirname(dirname(os.path.realpath(util.__file__))) + > + '/templates') > + source_fn = self._get_template_path(template, mycloud.distro, > + basepath=root_dir) > + template_fn = self._get_template_path(template, > mycloud.distro) > + > + # don't fail if cloud-init doesn't have a template > + if not os.path.exists(source_fn): > + reason = ("Distro '%s' does not have template" > + " for ntp client '%s'" % (distro, client)) > + raise unittest.SkipTest(reason) > + > + # Create a copy in our tmp_dir > + shutil.copy(source_fn, template_fn) > + > + cc_ntp.handle('notimportant', cfg, mycloud, None, None) > + > + content = util.read_file_or_url('file://' + > confpath).contents > + if client == 'ntp': > + expected_servers = '\n'.join([ > + 'server {0} iburst'.format(server) > + for server in servers]) > + self.assertIn(expected_servers, content.decode(), > + ('failed to render ntp.conf' > + ' for distro:{0}'.format(distro))) > + expected_pools = '\n'.join([ > + 'pool {0} iburst'.format(pool) for pool in pools]) > + self.assertIn(expected_pools, content.decode(), > + ('failed to render ntp.conf' > + ' for distro:{0}'.format(distro))) > + elif client == 'systemd-timesyncd': > + expected_content = ( > + "# cloud-init generated file\n" + > + "# See timesyncd.conf(5) for details.\n\n" + > + "[Time]\nNTP=%s %s \n" % (" ".join(servers), > + " ".join(pools))) > + self.assertEqual(expected_content, content.decode()) > > def test_no_ntpcfg_does_nothing(self): > """When no ntp section is defined handler logs a warning and > noops.""" -- 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