cloud-init-dev team mailing list archive
-
cloud-init-dev team
-
Mailing list archive
-
Message #04322
Re: [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
fix descriptions where they are "".
I comments in line. I'm not sure if integration with the distro is beneficial or not compared to just knowing about distros in the ntp module itself.
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
> @@ -83,7 +102,56 @@ schema = {
> List of ntp servers. If both pools and servers are
> empty, 4 default pool servers will be provided with
> the format ``{0-3}.{distro}.pool.ntp.org``.""")
> - }
> + },
> + 'ntp_client': {
> + 'type': 'string',
> + 'description': dedent("""\
> + Name of an NTP client to use to configure system NTP
> + """),
> + },
> + 'enabled': {
> + 'type': 'boolean',
> + 'description': "",
need a description of this.
and as we talked, not sure what it means.
> + },
> + 'config': {
> + 'type': ['object', 'null'],
> + 'properties': {
> + 'confpath': {
> + 'type': 'string',
> + 'description': "",
> + },
> + 'check_exe': {
> + 'type': 'string',
> + 'description': "",
> + },
> + 'name': {
> + 'type': 'string',
> + 'description': "",
> + },
> + 'packages': {
> + 'type': 'array',
> + 'items': {
> + 'type': 'string',
> + },
> + 'uniqueItems': True,
> + 'description': dedent("""\
> + List of packages needed to be installed for the
> + selected ``ntp_client``."""),
> + },
> + 'service_name': {
> + 'type': 'string',
> + 'description': "",
> + },
> + 'template_name': {
> + 'type': 'string',
> + 'description': "",
> + },
> + 'template': {
> + 'type': 'string',
> + 'description': "",
> + },
> + },
> + },
> },
> 'required': [],
> 'additionalProperties': False
> @@ -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()
probably
import copy
ntp_cfg = copy.deepcopy(cfg['ntp'])
>
> # 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']
i think chrony should be in this list, shouldnt it ?
if we have chrony installed and no ntp, then surely we should prefer that (I realize this is not ubuntu).
> +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()
this seems wierd in addition to 'install_package' which may (or should) raise a NotImplementedError.
seems redundant to also have 'package_installer'
> +
> + 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
having 2 things , 'get_ntp_server_name' and 'get_ntp_client_info' seems maybe redundant.
why not get_ntp_info() which returns a dictionary?
> +
> + 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
provied -> provided
I'm not sure that you need to take 'usercfg' here. I think the config dictionary
that you have is already updated with any config that comes from user-data.
did you specifically find otherwise?
> + 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(
this doesnt feel like a warning.
and wouldn't the schema figure that out and warn?
> + 'Skipping ntp client %s, no configuration available',
> + client)
> + continue
> + check = cfg.get('check_exe')
> + if not check:
> + LOG.warning(
doesnt feel like 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:
if path is messed up, stuff is foobarred. i wouldnt bother with setting it here.
and actually *doing* that here only causes problems because if you did have /usr/sbin/myprog
but /usr/sbin wasnt in your path, then you're going to return 'myprog', but that
is not going to work (because it is not in PATH).
> + 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']
is this list ordered? increasing preference?
>
> 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
isnt that just util.load_file ?
> +
> + 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
FIXME ?
> + 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.
References
