cloud-init-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

Most of this has already been discussed, but saving my comments for history.

Diff comments:

> 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']

Yeah, I think it's reasonable to have this list be populated by any ntp client for which we include templates.  Distros can still override this list with their preference.

> +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 is really a boolean w.r.t whether the distro we're running on supports installing packages.  We don't currently query the distro installer if a package is present, only that it has an installer; we defer the question to the install_packages() which will fail if the specified package isn't available;  cloud-init hasn't yet queried the distro package installer to see if a package is installable so I don't see why we'd change that here.

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

w.r.t the config; I think it may be how we're testing/mocking.

The _get_cloud() method takes a sys_cfg, and we're using a DataSourceNone which does not *read* or accept any user-data config, so in effect, it looks like user-data couldn't get merged to override a system setting.

I'll refactor the _get_cloud() method to accept a sys_cfg and user-data config, and merge them there and validate that we can drop passing in the usercfg.

> +        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(
> +                    'Skipping ntp client %s, no configuration available',
> +                    client)
> +                continue
> +            check = cfg.get('check_exe')
> +            if not check:
> +                LOG.warning(

Why not?  THe user may have specified a custom configuration but forgot the include a check_exe?  I suppose we could drop this with a schema that enforced custom configs to provide a check_exe, but for the SRU-case to non-schema enforced it seems reasonable to warn the user that for this particular client, the configuration was missing a check_exe setting.

I guess, it's a configuration error; it may not be fatal if some other ntp client is found and used.  THat's where the debate is.

> +                    '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:

Sure, I'll drop.

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


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