cloud-init-dev team mailing list archive

[Scott Moser <ssmoser2+ubuntu@xxxxxxxxx>]

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()

well, the dictionary has at least 'packages', which is an array.
and so a copy() will not give you a new copy of taht array.

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

One way or another, I think what we're after is something like:
if one of ALL_SUPPORTED_NTP_CLIENTS is installed:
  configure it
elif one of ALL_SUPPORTED_NTP_CLIENTS is installable
  install it
  configure it.

so adding chrony to the list of things that would be used if present should not hurt anything.
right?

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

maybe a 'can_install_package(pkg)' function would make sense.
that would allow you to figure out which of the list of packages are installable.
And in ubuntu core case, that would always return False (until 'snap install ntp' works).

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

I'd say its worth DEBUG that config doesnt look right.
its worth WARN or higher if you dont find a client and can't get one.

I guess i'm fine with WARN here as long as we dont ever get the warning by default.
ie, the user had to have done something to cause it.

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

managing PATH any where other than PATH just doesn't make sense.
if there was some reason that cloud-init had to maintain its
own search path separate from the environment PATH then we
could do that, but i dont think there is justification for that
here.

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

well please dont leave the FIXME here.

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