cloud-init-dev team mailing list archive

[Chad Smith <chad.smith@xxxxxxxxxxxxx>]

More minor inline comments and updated schema @ http://paste.ubuntu.com/p/2by6yyn8dT/



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

Two user-visible config keys that don't make sense and I think can be dropped.  "name" seems unused (and it is also a duplicate value of the top-level ntp_client in all of our default cases).

"template_name" only makes sense if someone delivers a new template into the same cloud-init templates directory and then references it. I don't want to encourage people to write additional content into cloud-init's directories. They should just use template instead and provide their entire content if they really want to roll their own. I also don't really see a use case for someone trying to override template_name for one distro with another distro's cloud-init template file.

> +                            'type': 'string',
> +                            'description': "",
> +                        },
> +                        'template': {
> +                            'type': 'string',
> +                            'description': "",
> +                        },
> +                    },
> +                },
>              },
>              'required': [],
>              'additionalProperties': False
> @@ -199,17 +253,27 @@ def write_ntp_config_template(cfg, cloud, path, template=None):
>          'pools': pools,
>      }
>  
> -    if template is None:
> -        template = 'ntp.conf.%s' % cloud.distro.name
> +    path = clientcfg.get('confpath')
> +    template_name = clientcfg.get('template_name').replace('{distro}',
> +                                                           cloud.distro.name)
> +    template = clientcfg.get('template')
> +    if template:
> +        tfile = tempfile.NamedTemporaryFile(delete=False,

We should probably use temp_util.mkstmp here as we might hit race conditions with temp files being blown away during the initial boot process.

> +                                            prefix='template_name',
> +                                            suffix=".tmpl")
> +        template_fn = tfile.name
> +        util.write_file(template_fn, content=template)
> +    else:
> +        template_fn = cloud.get_template_filename(template_name)
>  
> -    template_fn = cloud.get_template_filename(template)
>      if not template_fn:
> -        template_fn = cloud.get_template_filename('ntp.conf')
> -        if not template_fn:
> -            raise RuntimeError(
> -                'No template found, not rendering {path}'.format(path=path))
> +        raise RuntimeError(
> +            'No template found, not rendering {}'.format(path))
>  
>      templater.render_to_file(template_fn, path, params)
> +    # clean up temporary template
> +    if template:
> +        util.del_file(template_fn)
>  
>  
>  def reload_ntp(service, systemd=False):
> diff --git a/cloudinit/distros/debian.py b/cloudinit/distros/debian.py
> index 33cc0bf..2d5fd74 100644
> --- a/cloudinit/distros/debian.py
> +++ b/cloudinit/distros/debian.py
> @@ -39,6 +39,18 @@ ENI_HEADER = """# This file is generated from information provided by
>  NETWORK_CONF_FN = "/etc/network/interfaces.d/50-cloud-init.cfg"
>  LOCALE_CONF_FN = "/etc/default/locale"
>  
> +NTP_CLIENT_CONFIG = {
> +    'chrony': {
> +        'check_exe': 'chronyd',
> +        'confpath': '/etc/chrony/chrony.conf',

The only difference we are updating, from the base distro definition, is the confpath. So, why don't we just declare NTP_CLIENT_CONF_PATH = '/etc/chrony/chrony.conf' and self.ntp_client_config['confpath'] = NTP_CLIENT_CONF_PATH instead. It'll make it clear what's really being updated.

> +        'name': 'chrony',
> +        'packages': ['chrony'],
> +        'service_name': 'chrony',
> +        'template_name': 'chrony.conf.{distro}',
> +        'template': None,
> +    },
> +}
> +
>  
>  class Distro(distros.Distro):
>      hostname_conf_fn = "/etc/hostname"


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