cloud-init-dev team mailing list archive

[Chad Smith <chad.smith@xxxxxxxxxxxxx>]

minor inline comments, I'll have a pastebin of the updated schema dictionary as I see it shaping up.

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("""\

It looks like the distro logic defaults to auto if unspecfied. So add 'default': 'auto',

> +                        Name of an NTP client to use to configure system NTP

Description should probably document: if unspecified or 'auto' it will default to distribution's preference.  Maybe we also want to mention known builtin options ntp, chrony, ntpdate, systemd-timesyncd.

> +                        """),
> +                },
> +                'enabled': {
> +                    'type': 'boolean',

Add 'default': True,   here for future schema docs

> +                    'description': "",
> +                },

For schema docs, it might be nice to also use the default key here with 'default': True

> +                '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
> 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
> @@ -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
> +
> +    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
> +        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')

Do we want to actually merge usercfg with ntp_client_config to allow people to override only some of the default values? I think we decided in hangout that 'yes' we want to merge usercfg with any present ntp_client_config if it exists.

> +            else:
> +                cfg = self.ntp_client_config.get(client, {})
> +            if not cfg:
> +                LOG.warning(
> +                    '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):

agreed here. Let's just limit ourselves to util.which and not handle the corner case of external timesync programs not living in PATH.

> +    """Look for each program in path search under target"""
> +    if not programs:
> +        return []
> +
> +    if not search:
> +        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():

naming bike shed: has_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')


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