cloud-init-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

The branch should pass CI now, updates cloud-ci ntp test-cases to specify the ntp client, added tests for timesyncd and chrony.

I've a couple in-line comments that I think need some discussion before we can conclude the branch is ready.

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': "",

In practice in the code, if enabled: False then we're not configuring or installing NTP; however
we are NOT going and disabling any built-in NTP clients at this time.  That will be documented.

> +                },
> +                '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

After refactoring to not pass in the config the cloud object I've found that there *is* a difference w.r.t Distro._cfg and Cloud._cfg.  In particular, the Distro object has been given the system_config, that is, what's parsed from:  1) /etc/cloud/*.cfg + /etc/cloud/cloud.cfg.d  2) /run/cloud-init/*.cfg 3) /proc/commandline;  However it does *not* include any Datasource user-data/vendor-data.  Later in cmd/stages.py when we call cloudify() which creates the Cloud object, the datasource configuration is fetched which is what pulls and merges vendor-data/user-data into the Cloud._cfg which is passed to the config handlers.

Practically this means that to allow user-data to override system config, specifically ntp_client, and custom ntp client configurations, then we do need to pass the user config in.
I'd like some comments/thoughts on how best to structure this.

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


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