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.
_______________________________________________
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
