cloud-init-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

Some feedback, I'll pick up some additional suggestions around cloud-test

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

Yeah, that was where it was before;  master is strict, I think, so we could and drop, but we'd need to retain for previous releases

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

Yes, current HEAD of this merge has it dropped.

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

OK

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

OK

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

as discussed, this is subtly different, we're decoding six.binary_type, when py27 loads the chrony template, it includes UTF-8 values which the default .decode() method in py27 does not handle (FOO not in ascii, errors, both when passing to jinja to render, and later in the unittests).  So this allows us to decode the text before rendering to Jinja.

The other thing I saw was some way to set the python global default encoding value such that jinja module would work with UTF-8 values, I've not tested to confirm if that also works.

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

Yes, already done on this branch HEAD

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

Yes, I think we could.

> +    #!/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

I guess I was thinking that in Xenial:

ntp: {}

Is equivalent to:

ntp:
  enabled: true
  ntp_client: ntp

But, yeah, we could drop the enabled key, it defaults to true, and if ntp_client is set, then we'll pick that over the preferred distro list.

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

OK

> +    #!/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.