← Back to team overview

cloud-init-dev team mailing list archive

Re: [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master

 

round 2 inline comments. please rebase too to reduce the diff delta with zfs changes which have landed.

Diff comments:

> diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py
> index cbd0237..a6f8a1d 100644
> --- a/cloudinit/config/cc_ntp.py
> +++ b/cloudinit/config/cc_ntp.py
> @@ -83,7 +183,75 @@ 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',
> +                    'default': 'auto',
> +                    'description': dedent("""\
> +                        Name of an NTP client to use to configure system NTP.
> +                        When unprovided or 'auto' the default client preferred

Same leading/trailing space comment below. tox -e doc shows you that multiple lines are being joined without a space between.

> +                        by the distribution will be used. The following
> +                        built-in client names can be used to override existing
> +                        configuration defaults: chrony, ntp, ntpdate,
> +                        systemd-timesyncd."""),
> +                },
> +                'enabled': {
> +                    'type': 'boolean',
> +                    'default': True,
> +                    'description': dedent("""\
> +                        Attempt to enable ntp clients if set to True.  If set
> +                        to False, ntp client will not be configured or

you either need a leading space on the 2nd and 3rd lines, or a trailing space on the 1st and 2nd lines of a text block, otherwise dedent is squashing them together.

> +                        installed"""),
> +                },
> +                'config': {
> +                    'description': dedent("""\
> +                        Configuration settings or overrides for the
> +                        ``ntp_client`` specified."""),
> +                    'type': ['object', 'null'],
> +                    'properties': {
> +                        'confpath': {
> +                            'type': 'string',
> +                            'description': dedent("""\
> +                                The path to where the ``ntp_client``
> +                                configuration is written."""),
> +                        },
> +                        'check_exe': {
> +                            'type': 'string',
> +                            'description': dedent("""\
> +                                The executable name for the ``ntp_client``.
> +                                For exampe, ntp service ``check_exec`` is
> +                                'ntpd' because it runs the ntpd binary."""),
> +                        },
> +                        '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': dedent("""\
> +                                The systemd or sysvinit service name used to
> +                                start and stop the ``ntp_client`` service."""),
> +                        },
> +                        'template': {
> +                            'type': 'string',
> +                            'description': dedent("""\
> +                                Inline template allowing users to define their
> +                                own ``ntp_client`` configuration template.
> +                                The value should start with '## template:jinja'
> +                                to enable use of cloud-init templating support.
> +                                """),
> +                        },
> +                    },
> +                    'required': [],
> +                    'minProperties': 1,  # If we have config, define something
> +                    'additionalProperties': False
> +                },
>              },
>              'required': [],
>              'additionalProperties': False
> @@ -220,4 +438,71 @@ def reload_ntp(service, systemd=False):
>      util.subp(cmd, capture=True)
>  
>  
> +def handle(name, cfg, cloud, log, _args):
> +    """Enable and configure ntp."""
> +    if 'ntp' not in cfg:
> +        LOG.debug(
> +            "Skipping module named %s, not present or disabled by cfg", name)
> +        return
> +    ntp_cfg = cfg['ntp']
> +    if ntp_cfg is None:
> +        ntp_cfg = {}  # Allow empty config which will install the package
> +
> +    # TODO drop this when validate_cloudconfig_schema is strict=True
> +    if not isinstance(ntp_cfg, (dict)):
> +        raise RuntimeError(
> +            "'ntp' key existed in config, but not a dictionary type,"
> +            " is a {_type} instead".format(_type=type_utils.obj_name(ntp_cfg)))
> +
> +    validate_cloudconfig_schema(cfg, schema)

could pull this validation up above your isinstance(dict) check as it emits a warning that might add context too.

> +
> +    # Allow users to explicitly enable/disable
> +    enabled = ntp_cfg.get('enabled', True)
> +    if util.is_false(enabled):
> +        LOG.debug("Skipping module named %s, disabled by cfg", name)
> +        return
> +
> +    # Select which client is going to be used and get the configuration
> +    ntp_client_config = select_ntp_client(ntp_cfg, cloud.distro)
> +
> +    # Allow user ntp config to override distro configurations
> +    ntp_client_config = util.mergemanydict(
> +        [ntp_client_config, ntp_cfg.get('config', {})], reverse=True)
> +
> +    # ntp_client = 'auto' and no installed clients and no user client config
> +    if not ntp_client_config:
> +        LOG.debug(
> +            "Skipping module named %s, no built-in ntp client to config", name)
> +        return
> +
> +    rename_ntp_conf(config=ntp_client_config.get('confpath'))
> +
> +    template_fn = None
> +    if not ntp_client_config.get('template'):
> +        template_name = (
> +            ntp_client_config.get('template_name').replace('{distro}',
> +                                                           cloud.distro.name))
> +        template_fn = cloud.get_template_filename(template_name)
> +        if not template_fn:
> +            msg = ('No template found, not rendering %s' %
> +                   ntp_client_config.get('template_name'))
> +            raise RuntimeError(msg)
> +
> +    write_ntp_config_template(cloud.distro.name,
> +                              servers=ntp_cfg.get('servers', []),
> +                              pools=ntp_cfg.get('pools', []),
> +                              path=ntp_client_config.get('confpath'),
> +                              template_fn=template_fn,
> +                              template=ntp_client_config.get('template'))
> +
> +    install_ntp_client(cloud.distro.install_packages,
> +                       packages=ntp_client_config['packages'],
> +                       check_exe=ntp_client_config['check_exe'])
> +    try:
> +        reload_ntp(ntp_client_config['service_name'],
> +                   systemd=cloud.distro.uses_systemd())
> +    except util.ProcessExecutionError as e:
> +        LOG.exception("Failed to reload/start ntp service: %s", e)
> +        raise
> +
>  # vi: ts=4 expandtab
> diff --git a/cloudinit/config/cc_resizefs.py b/cloudinit/config/cc_resizefs.py
> index c8e1752..013e69b 100644
> --- a/cloudinit/config/cc_resizefs.py
> +++ b/cloudinit/config/cc_resizefs.py
> @@ -251,6 +251,8 @@ def handle(name, cfg, _cloud, log, args):
>      if fs_type == 'zfs':
>          zpool = devpth.split('/')[0]
>          devpth = util.get_device_info_from_zpool(zpool)
> +        if not devpth:

probably want to rebase and your resizefs has landed.

> +            return  # could not find device from zpool
>          resize_what = zpool
>  
>      info = "dev=%s mnt_point=%s path=%s" % (devpth, mount_point, resize_what)
> diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py
> index 55260ea..a407cdf 100755
> --- a/cloudinit/distros/__init__.py
> +++ b/cloudinit/distros/__init__.py
> @@ -60,6 +64,7 @@ class Distro(object):
>      tz_zone_dir = "/usr/share/zoneinfo"
>      init_cmd = ['service']  # systemctl, service etc
>      renderer_configs = {}
> +    preferred_ntp_clients = copy.deepcopy(PREFERRED_NTP_CLIENTS)

just need copy.copy here. deepcopy is for nested iterables, which PREFERRED_NTP_CLIENTS isn't

>  
>      def __init__(self, name, cfg, paths):
>          self._paths = paths
> diff --git a/cloudinit/distros/ubuntu.py b/cloudinit/distros/ubuntu.py
> index 82ca34f..8cd0b84 100644
> --- a/cloudinit/distros/ubuntu.py
> +++ b/cloudinit/distros/ubuntu.py
> @@ -14,8 +14,14 @@ from cloudinit import log as logging
>  
>  LOG = logging.getLogger(__name__)
>  
> +PREFERRED_NTP_CLIENTS = ['chrony', 'systemd-timesyncd', 'ntp', 'ntpdate']

let's drop this global definition PREFERRED_NTP_CLIENTS, I don't like to see the same named global coming from two different modules cloudinit/distros/ubuntu.py and __init__.py

> +
>  
>  class Distro(debian.Distro):
> +
> +    # Ubuntu ntp client priority
> +    preferred_ntp_clients = PREFERRED_NTP_CLIENTS
> +
>      pass
>  
>  # vi: ts=4 expandtab
> diff --git a/cloudinit/util.py b/cloudinit/util.py
> index 0ab2c48..1b62433 100644
> --- a/cloudinit/util.py
> +++ b/cloudinit/util.py
> @@ -157,6 +157,13 @@ def encode_text(text, encoding='utf-8'):
>      return text.encode(encoding)
>  
>  
> +def decode_text(text, encoding='utf-8'):

This is a duplicate function of util.decode_binary. We can drop it in favor of the existing.

> +    # Converts a binary type to string using given encoding
> +    if isinstance(text, six.binary_type):
> +        return text.decode(encoding)
> +    return text
> +
> +
>  def b64d(source):
>      # Base64 decode some data, accepting bytes or unicode/str, and returning
>      # str/unicode if the result is utf-8 compatible, otherwise returning bytes.
> @@ -2249,7 +2256,15 @@ def get_mount_info_freebsd(path):
>  
>  
>  def get_device_info_from_zpool(zpool):
> -    (zpoolstatus, err) = subp(['zpool', 'status', zpool])
> +    # zpool has 10 second timeout waiting for /dev/zfs LP: #1760173

Need to rebase master to drop this part of the diff.

> +    if not os.path.exists('/dev/zfs'):
> +        LOG.debug('Cannot get zpool info, no /dev/zfs')
> +        return None
> +    try:
> +        (zpoolstatus, err) = subp(['zpool', 'status', zpool])
> +    except ProcessExecutionError as err:
> +        LOG.warning("Unable to get zpool status of %s: %s", zpool, err)
> +        return None
>      if len(err):
>          return None
>      r = r'.*(ONLINE).*'
> diff --git a/tests/cloud_tests/testcases/modules/ntp_chrony.yaml b/tests/cloud_tests/testcases/modules/ntp_chrony.yaml
> new file mode 100644
> index 0000000..8866f4a
> --- /dev/null
> +++ b/tests/cloud_tests/testcases/modules/ntp_chrony.yaml
> @@ -0,0 +1,32 @@
> +#
> +# ntp enabled, chrony selected, check conf file
> +# as chrony won't start in a container
> +#
> +cloud_config: |
> +  #cloud-config
> +  ntp:
> +    enabled: true
> +    ntp_client: chrony
> +collect_scripts:
> +  chrony_conf_exists: |

Can we drop this file grab from the integration test as chrony_conf value would be non-empty if it exists right?

> +    #!/bin/bash
> +    paths=( '/etc/chrony.conf' '/etc/chrony/chrony.conf' )
> +    for p in ${paths[@]}; do
> +        test -e ${p}
> +        if [[ "$?" = "0" ]]; then
> +            echo 0
> +            exit
> +        fi
> +    done
> +    echo 1
> +  chrony_conf: |
> +    #!/bin/bash
> +    paths=( '/etc/chrony.conf' '/etc/chrony/chrony.conf' )
> +    for p in ${paths[@]}; do
> +        test -e ${p}
> +        if [[ "$?" = "0" ]]; then
> +            cat ${p}
> +            exit
> +        fi
> +    done
> +# vi: ts=4 expandtab
> diff --git a/tests/cloud_tests/testcases/modules/ntp_pools.yaml b/tests/cloud_tests/testcases/modules/ntp_pools.yaml
> index d490b22..0f9d665 100644
> --- a/tests/cloud_tests/testcases/modules/ntp_pools.yaml
> +++ b/tests/cloud_tests/testcases/modules/ntp_pools.yaml
> @@ -9,6 +9,8 @@ required_features:
>  cloud_config: |
>    #cloud-config
>    ntp:
> +    enabled: true

Can we drop this enabled:True? I'd like to see an integration test without enabled: true as we expect that to work by default anyway.

> +    ntp_client: ntp
>      pools:
>          - 0.cloud-init.mypool
>          - 1.cloud-init.mypool
> diff --git a/tests/cloud_tests/testcases/modules/ntp_timesyncd.yaml b/tests/cloud_tests/testcases/modules/ntp_timesyncd.yaml
> new file mode 100644
> index 0000000..9849181
> --- /dev/null
> +++ b/tests/cloud_tests/testcases/modules/ntp_timesyncd.yaml
> @@ -0,0 +1,19 @@
> +#
> +# ntp enabled, systemd-timesyncd selected, check conf file
> +# as systemd-timesyncd won't start in a container
> +#
> +cloud_config: |
> +  #cloud-config
> +  ntp:
> +    enabled: true
> +    ntp_client: systemd-timesyncd
> +collect_scripts:
> +  timesyncd_conf_exists: |

let's drop the timesyncd_conf_exists data value as we should be able to determine that from "timesyncd_conf" below

> +    #!/bin/bash
> +    test -e /etc/systemd/timesyncd.conf.d/cloud-init.conf
> +    echo $?
> +  timesyncd_conf: |
> +    #!/bin/bash
> +    cat /etc/systemd/timesyncd.conf.d/cloud-init.conf
> +
> +# vi: ts=4 expandtab


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


References