Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
oops lints/flakes fixed http://paste.ubuntu.com/p/5Zv2WTX9Rf/ -- 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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
> If you've got some changes to make the validation check tighter, I'm happy to > have those; and I agree that if status --long can emit exactly the error > that's really nice. Here's some tested code I ran on lxc's with sparse/incorrect user-data. it tightens up the error reporting so cloud-init status --long gives you an actionable traceback. http://paste.ubuntu.com/p/dcKcYrM5ty/ -- 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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
Update the commit message on the merge proposal tomake sure it is in sync with the current code. - I think the '_set_preferred_ntp_client' client is kind of wierd. could we get the same function with an attribute ? Then it would only be called if used. Rather than being called on init. - 'handle' in cc_ntp is longer than i'd like. the less that function does the easier things are to test. but I wont insist on that. Diff comments: > diff --git a/cloudinit/util.py b/cloudinit/util.py > index acdc0d8..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 doesnt feel right. it is so very close to 'decode_binary' and in general feels like a step backwards. We're trying very much to expect correct types rather than decoding/encoding whatever comes in. > +# 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. -- 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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
If you've got some changes to make the validation check tighter, I'm happy to have those; and I agree that if status --long can emit exactly the error that's really nice. -- 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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
I feel like we could do a bit better validation on the user-provided config
keys with a simple validate_config function that'd raise understandable errors
on misconfig for certain keys:
Here are some examples of easy and hard tracebacks to understand, as the person
can't just run cloud-init status --long to see the error message, they have to
walk through /var/log/cloud-init.log and read through a traceback stack to get
at what may be the problem.
What do you think about a bit more validation on required config values?
https://pastebin.ubuntu.com/p/4X9TJg3NSz/
Diff comments:
> diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py
> index cbd0237..7a7fd7e 100644
> --- a/cloudinit/config/cc_ntp.py
> +++ b/cloudinit/config/cc_ntp.py
> @@ -185,34 +369,60 @@ def generate_server_names(distro):
> return names
>
>
> -def write_ntp_config_template(cfg, cloud, path, template=None):
> -servers = cfg.get('servers', [])
> -pools = cfg.get('pools', [])
> +def write_ntp_config_template(distro_name, servers=None, pools=None,
> + path=None, template_fn=None, template=None):
> +"""Render a ntp client configuration for the specified client.
> +
> +@param distro_name: string. The distro class name.
> +@param servers: A list of strings specifying ntp servers. Defaults to
> empty
> +list.
> +@param pools: A list of strings specifying ntp pools. Defaults to empty
> +list.
> +@param path: A string to specify where to write the rendered template.
> +@param template_fn: A string to specify the template source file.
> +@param template: A string specifying the contents of the template. This
> +content will be written to a temporary file before being used to render
> +the configuration file.
> +
> +@raises: ValueError when path is None.
> +@raises: ValueError when template_fn is None and template is None.
> +"""
> +if not servers:
> +servers = []
> +if not pools:
> +pools = []
>
> if len(servers) == 0 and len(pools) == 0:
> -pools = generate_server_names(cloud.distro.name)
> +pools = generate_server_names(distro_name)
> LOG.debug(
> 'Adding distro default ntp pool servers: %s', ','.join(pools))
>
> -params = {
> -'servers': servers,
> -'pools': pools,
> -}
> +if not path:
> +raise ValueError('Invalid value for path parameter')
This should probably be "Missing value for path parameter"
>
> -if template is None:
> -template = 'ntp.conf.%s' % cloud.distro.name
> +if not template_fn and not template:
> +raise ValueError('Not template_fn or template provided')
>
> -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))
> +params = {'servers': servers, 'pools': pools}
> +if template:
> +tfile = temp_utils.mkstemp(prefix='template_name-', suffix=".tmpl")
> +template_fn = tfile[1] # filepath is second item in tuple
> +util.write_file(template_fn, content=template)
>
> 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):
> +"""Restart or reload an ntp system service.
> +
> +@param service: A string specifying the name of the service to be
> affected.
> +@param systemd: A boolean indicating if the distro uses systemd, defaults
> +to False.
> +@returns: A tuple of stdout, stderr results from executing the action.
> +"""
> if systemd:
> cmd = ['systemctl', 'reload-or-restart', service]
> else:
--
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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
Review: Approve continuous-integration PASSED: Continuous integration, rev:0f45961973a9778db7e32c07682de5d5cebcb87e https://jenkins.ubuntu.com/server/job/cloud-init-ci/982/ Executed test runs: SUCCESS: Checkout SUCCESS: Unit & Style Tests SUCCESS: Ubuntu LTS: Build SUCCESS: Ubuntu LTS: Integration SUCCESS: MAAS Compatability Testing IN_PROGRESS: Declarative: Post Actions Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/982/rebuild -- 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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
Review: Needs Fixing continuous-integration FAILED: Continuous integration, rev:46ae12701ae617ff92a83d17bc1fee5cafcd9316 https://jenkins.ubuntu.com/server/job/cloud-init-ci/981/ Executed test runs: SUCCESS: Checkout SUCCESS: Unit & Style Tests SUCCESS: Ubuntu LTS: Build SUCCESS: Ubuntu LTS: Integration FAILED: MAAS Compatability Testing Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/981/rebuild -- 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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
Review: Needs Fixing continuous-integration FAILED: Continuous integration, rev:9162c2e0a4040607af4cce666ebe94970c7337e4 https://jenkins.ubuntu.com/server/job/cloud-init-ci/978/ Executed test runs: SUCCESS: Checkout SUCCESS: Unit & Style Tests SUCCESS: Ubuntu LTS: Build SUCCESS: Ubuntu LTS: Integration FAILED: MAAS Compatability Testing Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/978/rebuild -- 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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
Review: Approve continuous-integration PASSED: Continuous integration, rev:815ce15352f26e752386b0738f707af29d589623 https://jenkins.ubuntu.com/server/job/cloud-init-ci/976/ Executed test runs: SUCCESS: Checkout SUCCESS: Unit & Style Tests SUCCESS: Ubuntu LTS: Build SUCCESS: Ubuntu LTS: Integration SUCCESS: MAAS Compatability Testing IN_PROGRESS: Declarative: Post Actions Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/976/rebuild -- 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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
Review: Needs Fixing continuous-integration FAILED: Continuous integration, rev:2dbc5d8638ff717b6a0eb891ac2b804fd1c2aed2 https://jenkins.ubuntu.com/server/job/cloud-init-ci/974/ Executed test runs: SUCCESS: Checkout SUCCESS: Unit & Style Tests SUCCESS: Ubuntu LTS: Build FAILED: Ubuntu LTS: Integration Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/974/rebuild -- 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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
Review: Needs Fixing continuous-integration FAILED: Continuous integration, rev:2dbc5d8638ff717b6a0eb891ac2b804fd1c2aed2 https://jenkins.ubuntu.com/server/job/cloud-init-ci/966/ Executed test runs: SUCCESS: Checkout SUCCESS: Unit & Style Tests SUCCESS: Ubuntu LTS: Build FAILED: Ubuntu LTS: Integration Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/966/rebuild -- 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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
Review: Needs Fixing continuous-integration FAILED: Continuous integration, rev:2dbc5d8638ff717b6a0eb891ac2b804fd1c2aed2 https://jenkins.ubuntu.com/server/job/cloud-init-ci/965/ Executed test runs: SUCCESS: Checkout SUCCESS: Unit & Style Tests SUCCESS: Ubuntu LTS: Build FAILED: Ubuntu LTS: Integration Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/965/rebuild -- 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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
Will grab some of these as well, open questions inline.
Diff comments:
> diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py
> index cbd0237..57a732f 100644
> --- a/cloudinit/config/cc_ntp.py
> +++ b/cloudinit/config/cc_ntp.py
> @@ -94,68 +264,84 @@ schema = {
> __doc__ = get_schema_doc(schema) # Supplement python help()
>
>
> -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
> +def distro_ntp_client_configs(distro):
> +"""Construct a distro-specific ntp client config dictionary by merging
> + distro specific changes into base config.
>
> -# 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)
> -if ntp_installable():
> -service_name = 'ntp'
> -confpath = NTP_CONF
> -template_name = None
> -packages = ['ntp']
> -check_exe = 'ntpd'
> -else:
> -service_name = 'systemd-timesyncd'
> -confpath = TIMESYNCD_CONF
> -template_name = 'timesyncd.conf'
> -packages = []
> -check_exe = '/lib/systemd/systemd-timesyncd'
> -
> -rename_ntp_conf()
> -# ensure when ntp is installed it has a configuration file
> -# to use instead of starting up with packaged defaults
> -write_ntp_config_template(ntp_cfg, cloud, confpath,
> template=template_name)
> -install_ntp(cloud.distro.install_packages, packages=packages,
> -check_exe=check_exe)
> -
> -try:
> -reload_ntp(service_name, systemd=cloud.distro.uses_systemd())
> -except util.ProcessExecutionError as e:
> -LOG.exception("Failed to reload/start ntp service: %s", e)
> -raise
> +@param distro: String providing the distro class name.
> +@returns: Dict of distro configurations for ntp clients.
> +"""
> +dcfg = DISTRO_CLIENT_CONFIG
> +cfg = copy.deepcopy(NTP_CLIENT_CONFIG)
> +if distro in dcfg:
> +cfg = util.mergemanydict([cfg, dcfg[distro]], reverse=True)
> +return cfg
>
>
> -def ntp_installable():
> -"""Check if we can install ntp package
> +def select_ntp_client(usercfg, distro):
Yes, that makes sense; The path here evolved from merging the config within
this method to outside so it make perfect sense to only pass what's needed here.
> +"""Determine which ntp client is to be used, consulting the distro
> + for its preference.
>
> -Ubuntu-Core systems do not have an ntp package available, so
> -we always return False. Other systems require package managers to
> install
> -the ntp package If we fail to find one of the package managers, then we
> -cannot install ntp.
> +@param usercfg: Dict of user-provided cloud-config.
> +@param distro: Distro class instance.
> +@returns: Dict of the selected ntp client or {} if none selected.
> """
> -if util.system_is_snappy():
> -return False
>
> -if any(map(util.which, ['apt-get', 'dnf', 'yum', 'zypper'])):
> -return True
> +# construct distro-specific ntp_client_config dict
> +distro_cfg = distro_ntp_client_configs(distro.name)
> +
> +# user specified client, return its config
> +if 'ntp_client' in usercfg:
Hrm. It does seem odd to say you want the distro to select which it selects by
default, so let's walk this out a bit. The one case that's not clear is if the
distro has selected a client that's not auto, and the user set's auto; does
that imply that we want to search the distro.preferred_ntp_clients list over
the singluar value in the distro ntp_client setting?
> +LOG.debug('Selected NTP client "%s" via user-data configuration',
> + usercfg.get('ntp_client'))
> +return distro_cfg.get(usercfg.get('ntp_client'))
> +
> +# default to auto if unset in distro
> +distro_ntp_client = distro.get_option('ntp_client', 'auto')
> +
> +clientcfg = {}
> +if distro_ntp_client == "auto":
> +for client in distro.preferred_ntp_clients:
> +cfg = distro_cfg.get(client)
> +if not cfg:
> +LOG.warning(
> +'Skipping NTP client "%s", no configuration available',
> +client)
> +continue
> +check = cfg.get('check_exe')
> +if not check:
We could drop the checks here, they're provided in code for any of the distro
preferred clients. In the case the user is
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
Diff comments:
> diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py
> index cbd0237..57a732f 100644
> --- a/cloudinit/config/cc_ntp.py
> +++ b/cloudinit/config/cc_ntp.py
> @@ -94,68 +264,84 @@ schema = {
> __doc__ = get_schema_doc(schema) # Supplement python help()
>
>
> -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
> +def distro_ntp_client_configs(distro):
> +"""Construct a distro-specific ntp client config dictionary by merging
> + distro specific changes into base config.
>
> -# 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)
> -if ntp_installable():
> -service_name = 'ntp'
> -confpath = NTP_CONF
> -template_name = None
> -packages = ['ntp']
> -check_exe = 'ntpd'
> -else:
> -service_name = 'systemd-timesyncd'
> -confpath = TIMESYNCD_CONF
> -template_name = 'timesyncd.conf'
> -packages = []
> -check_exe = '/lib/systemd/systemd-timesyncd'
> -
> -rename_ntp_conf()
> -# ensure when ntp is installed it has a configuration file
> -# to use instead of starting up with packaged defaults
> -write_ntp_config_template(ntp_cfg, cloud, confpath,
> template=template_name)
> -install_ntp(cloud.distro.install_packages, packages=packages,
> -check_exe=check_exe)
> -
> -try:
> -reload_ntp(service_name, systemd=cloud.distro.uses_systemd())
> -except util.ProcessExecutionError as e:
> -LOG.exception("Failed to reload/start ntp service: %s", e)
> -raise
> +@param distro: String providing the distro class name.
> +@returns: Dict of distro configurations for ntp clients.
> +"""
> +dcfg = DISTRO_CLIENT_CONFIG
> +cfg = copy.deepcopy(NTP_CLIENT_CONFIG)
> +if distro in dcfg:
> +cfg = util.mergemanydict([cfg, dcfg[distro]], reverse=True)
> +return cfg
>
>
> -def ntp_installable():
> -"""Check if we can install ntp package
> +def select_ntp_client(usercfg, distro):
Can we make this api more explicit to only take a ntp_client_name instead of
the whole config dict since that's the only value or None that we are looking
at here? the callsite select_ntp_client(ntp_cfg.get('ntp_client', 'auto'),
cloud.distro) would work here. Then we know what we are actually relying on
without having to read all of select_ntp_client.
> +"""Determine which ntp client is to be used, consulting the distro
> + for its preference.
>
> -Ubuntu-Core systems do not have an ntp package available, so
> -we always return False. Other systems require package managers to
> install
> -the ntp package If we fail to find one of the package managers, then we
> -cannot install ntp.
> +@param usercfg: Dict of user-provided cloud-config.
> +@param distro: Distro class instance.
> +@returns: Dict of the selected ntp client or {} if none selected.
> """
> -if util.system_is_snappy():
> -return False
>
> -if any(map(util.which, ['apt-get', 'dnf', 'yum', 'zypper'])):
> -return True
> +# construct distro-specific ntp_client_config dict
> +distro_cfg = distro_ntp_client_configs(distro.name)
> +
> +# user specified client, return its config
> +if 'ntp_client' in usercfg:
This logic won't work if the cloud-config provides 'ntp_client': 'auto'. Can we
change the logic a bit here to permit that value from the user. since we
document it?
> +LOG.debug('Selected NTP client "%s" via user-data configuration',
> + usercfg.get('ntp_client'))
> +return distro_cfg.get(usercfg.get('ntp_client'))
> +
> +# default to auto if unset in distro
> +distro_ntp_client = distro.get_option('ntp_client', 'auto')
> +
> +clientcfg = {}
> +if distro_ntp_client == "auto":
> +for client in distro.preferred_ntp_clients:
> +cfg = distro_cfg.get(client)
> +if not cfg:
> +LOG.warning(
> +'Skipping NTP client "%s", no configuration available',
> +client)
> +continue
> +check = cfg.get('check_exe')
> +if not check:
These checks are on the distro default client configuration, which is all
hard-coded in cc_ntp.py right? Shouldn't we instead (or additionally) be
validating user-data which could
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
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',
Re: [Cloud-init-dev] [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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
Thanks for the review, Ill fix what you pointed out, check what docs look like,
drop the zfs fix and rebase to master.
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
> @@ -21,9 +23,80 @@ LOG = logging.getLogger(__name__)
>
> frequency = PER_INSTANCE
> NTP_CONF = '/etc/ntp.conf'
> -TIMESYNCD_CONF = '/etc/systemd/timesyncd.conf.d/cloud-init.conf'
> NR_POOL_SERVERS = 4
> -distros = ['centos', 'debian', 'fedora', 'opensuse', 'sles', 'ubuntu']
> +distros = ['centos', 'debian', 'fedora', 'opensuse', 'rhel', 'sles',
> 'ubuntu']
> +
> +NTP_CLIENT_CONFIG = {
> +'chrony': {
> +'check_exe': 'chronyd',
> +'confpath': '/etc/chrony.conf',
> +'packages': ['chrony'],
> +'service_name': 'chrony',
> +'template_name': 'chrony.conf.{distro}',
> +'template': None,
> +},
> +'ntp': {
> +'check_exe': 'ntpd',
> +'confpath': NTP_CONF,
> +'packages': ['ntp'],
> +'service_name': 'ntp',
> +'template_name': 'ntp.conf.{distro}',
> +'template': None,
> +},
> +'ntpdate': {
> +'check_exe': 'ntpdate',
> +'confpath': NTP_CONF,
> +'packages': ['ntpdate'],
> +'service_name': 'ntpdate',
> +'template_name': 'ntp.conf.{distro}',
> +'template': None,
> +},
> +'systemd-timesyncd': {
> +'check_exe': '/lib/systemd/systemd-timesyncd',
> +'confpath': '/etc/systemd/timesyncd.conf.d/cloud-init.conf',
> +'packages': [],
> +'service_name': 'systemd-timesyncd',
> +'template_name': 'timesyncd.conf',
> +'template': None,
> +},
> +}
> +
> +DISTRO_CLIENT_CONFIG = {
+1
> +'debian': {
> +'chrony': {
> +'confpath': '/etc/chrony/chrony.conf',
> +},
> +},
> +'opensuse': {
> +'chrony': {
> +'service_name': 'chronyd',
> +},
> +'ntp': {
> +'confpath': '/etc/ntp.conf',
> +'service_name': 'ntpd',
> +},
> +'systemd-timesyncd': {
> +'check_exe': '/usr/lib/systemd/systemd-timesyncd',
> +},
> +},
> +'sles': {
> +'chrony': {
> +'service_name': 'chronyd',
> +},
> +'ntp': {
> +'confpath': '/etc/ntp.conf',
> +'service_name': 'ntpd',
> +},
> +'systemd-timesyncd': {
> +'check_exe': '/usr/lib/systemd/systemd-timesyncd',
> +},
> +},
> +'ubuntu': {
> +'chrony': {
> +'confpath': '/etc/chrony/chrony.conf',
> +},
> +},
> +}
>
>
> # The schema definition for each cloud-config module is a strict contract for
> @@ -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
+1
> +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
Thanks, I see it now
> +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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
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
> @@ -21,9 +23,80 @@ LOG = logging.getLogger(__name__)
>
> frequency = PER_INSTANCE
> NTP_CONF = '/etc/ntp.conf'
> -TIMESYNCD_CONF = '/etc/systemd/timesyncd.conf.d/cloud-init.conf'
> NR_POOL_SERVERS = 4
> -distros = ['centos', 'debian', 'fedora', 'opensuse', 'sles', 'ubuntu']
> +distros = ['centos', 'debian', 'fedora', 'opensuse', 'rhel', 'sles',
> 'ubuntu']
> +
> +NTP_CLIENT_CONFIG = {
> +'chrony': {
> +'check_exe': 'chronyd',
> +'confpath': '/etc/chrony.conf',
> +'packages': ['chrony'],
> +'service_name': 'chrony',
> +'template_name': 'chrony.conf.{distro}',
> +'template': None,
> +},
> +'ntp': {
> +'check_exe': 'ntpd',
> +'confpath': NTP_CONF,
> +'packages': ['ntp'],
> +'service_name': 'ntp',
> +'template_name': 'ntp.conf.{distro}',
> +'template': None,
> +},
> +'ntpdate': {
> +'check_exe': 'ntpdate',
> +'confpath': NTP_CONF,
> +'packages': ['ntpdate'],
> +'service_name': 'ntpdate',
> +'template_name': 'ntp.conf.{distro}',
> +'template': None,
> +},
> +'systemd-timesyncd': {
> +'check_exe': '/lib/systemd/systemd-timesyncd',
> +'confpath': '/etc/systemd/timesyncd.conf.d/cloud-init.conf',
> +'packages': [],
> +'service_name': 'systemd-timesyncd',
> +'template_name': 'timesyncd.conf',
> +'template': None,
> +},
> +}
> +
> +DISTRO_CLIENT_CONFIG = {
can we add a header comment here stating that this is just specific
distribution overrides for configuration values.
> +'debian': {
> +'chrony': {
> +'confpath': '/etc/chrony/chrony.conf',
> +},
> +},
> +'opensuse': {
> +'chrony': {
> +'service_name': 'chronyd',
> +},
> +'ntp': {
> +'confpath': '/etc/ntp.conf',
> +'service_name': 'ntpd',
> +},
> +'systemd-timesyncd': {
> +'check_exe': '/usr/lib/systemd/systemd-timesyncd',
> +},
> +},
> +'sles': {
> +'chrony': {
> +'service_name': 'chronyd',
> +},
> +'ntp': {
> +'confpath': '/etc/ntp.conf',
> +'service_name': 'ntpd',
> +},
> +'systemd-timesyncd': {
> +'check_exe': '/usr/lib/systemd/systemd-timesyncd',
> +},
> +},
> +'ubuntu': {
> +'chrony': {
> +'confpath': '/etc/chrony/chrony.conf',
> +},
> +},
> +}
>
>
> # The schema definition for each cloud-config module is a strict contract for
--
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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
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 any
multi-line text block is 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
--
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 :
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
Review: Approve continuous-integration PASSED: Continuous integration, rev:2230bd0046a795da56e3958b761dc4c6472a4d35 https://jenkins.ubuntu.com/server/job/cloud-init-ci/961/ Executed test runs: SUCCESS: Checkout SUCCESS: Unit & Style Tests SUCCESS: Ubuntu LTS: Build SUCCESS: Ubuntu LTS: Integration SUCCESS: MAAS Compatability Testing IN_PROGRESS: Declarative: Post Actions Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/961/rebuild -- 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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
Review: Approve continuous-integration PASSED: Continuous integration, rev:ce33a01512109e6a08cabdba1df3373922caf4bc https://jenkins.ubuntu.com/server/job/cloud-init-ci/960/ Executed test runs: SUCCESS: Checkout SUCCESS: Unit & Style Tests SUCCESS: Ubuntu LTS: Build SUCCESS: Ubuntu LTS: Integration SUCCESS: MAAS Compatability Testing IN_PROGRESS: Declarative: Post Actions Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/960/rebuild -- 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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
Review: Needs Fixing continuous-integration FAILED: Continuous integration, rev:3d28f5286612316cc210d23a59604e71d294d03d https://jenkins.ubuntu.com/server/job/cloud-init-ci/959/ Executed test runs: SUCCESS: Checkout SUCCESS: Unit & Style Tests FAILED: Ubuntu LTS: Build Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/959/rebuild -- 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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
Refactored all logic into cc_ntp module deferring only to distros for a preferred ordering of ntp clients. This passes all unit/ci-cloud tests locally. Next step is testing it in different datasources/clouds. -- 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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
Review: Needs Fixing continuous-integration FAILED: Continuous integration, rev:b94f8d7e2b3df1f72b2da69ccad1e115e260819a https://jenkins.ubuntu.com/server/job/cloud-init-ci/957/ Executed test runs: SUCCESS: Checkout SUCCESS: Unit & Style Tests FAILED: Ubuntu LTS: Build Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/957/rebuild -- 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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
Review: Approve continuous-integration PASSED: Continuous integration, rev:8ffcc66c74e2e7a622495575839b486f43a0222d https://jenkins.ubuntu.com/server/job/cloud-init-ci/949/ Executed test runs: SUCCESS: Checkout SUCCESS: Unit & Style Tests SUCCESS: Ubuntu LTS: Build SUCCESS: Ubuntu LTS: Integration SUCCESS: MAAS Compatability Testing IN_PROGRESS: Declarative: Post Actions Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/949/rebuild -- 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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
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 =
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
Review: Needs Fixing continuous-integration FAILED: Continuous integration, rev:5086bd7ac25ac0975f9fbfbfa7eabb0e372f75b0 https://jenkins.ubuntu.com/server/job/cloud-init-ci/948/ Executed test runs: SUCCESS: Checkout FAILED: Unit & Style Tests Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/948/rebuild -- 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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
Most of this has already been discussed, but saving my comments for history.
Diff comments:
> 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
> @@ -49,6 +49,48 @@ LOG = logging.getLogger(__name__)
> # It could break when Amazon adds new regions and new AZs.
> _EC2_AZ_RE = re.compile('^[a-z][a-z]-(?:[a-z]+-)+[0-9][a-z]$')
>
> +# Default NTP Client Configurations
> +PREFERRED_NTP_CLIENTS = ['systemd-timesyncd', 'ntp', 'ntpdate']
Yeah, I think it's reasonable to have this list be populated by any ntp client
for which we include templates. Distros can still override this list with
their preference.
> +NTP_CONF = '/etc/ntp.conf'
> +NTP_CLIENT_CONFIG = {
> +'chrony': {
> +'check_exe': 'chronyd',
> +'confpath': '/etc/chrony.conf',
> +'name': 'chrony',
> +'packages': ['chrony'],
> +'service_name': 'chrony',
> +'template_name': 'chrony.conf.{distro}',
> +'template': None,
> +},
> +'ntp': {
> +'check_exe': 'ntpd',
> +'confpath': NTP_CONF,
> +'name': 'ntp',
> +'packages': ['ntp'],
> +'service_name': 'ntp',
> +'template_name': 'ntp.conf.{distro}',
> +'template': None,
> +},
> +'ntpdate': {
> +'check_exe': 'ntpdate',
> +'confpath': NTP_CONF,
> +'name': 'ntpdate',
> +'packages': ['ntpdate'],
> +'service_name': 'ntpdate',
> +'template_name': 'ntp.conf.{distro}',
> +'template': None,
> +},
> +'systemd-timesyncd': {
> +'check_exe': '/lib/systemd/systemd-timesyncd',
> +'confpath': '/etc/systemd/timesyncd.conf.d/cloud-init.conf',
> +'name': 'systemd-timesyncd',
> +'packages': [],
> +'service_name': 'systemd-timesyncd',
> +'template_name': 'timesyncd.conf',
> +'template': None,
> +},
> +}
> +
>
> @six.add_metaclass(abc.ABCMeta)
> class Distro(object):
> @@ -109,6 +153,17 @@ class Distro(object):
> """Wrapper to report whether this distro uses systemd or sysvinit."""
> return uses_systemd()
>
> +def package_installer(self):
> +"""Wrapper to report whether this distro has a package installer."""
> +return package_installer()
This is really a boolean w.r.t whether the distro we're running on supports
installing packages. We don't currently query the distro installer if a
package is present, only that it has an installer; we defer the question to the
install_packages() which will fail if the specified package isn't available;
cloud-init hasn't yet queried the distro package installer to see if a package
is installable so I don't see why we'd change that here.
> +
> +def find_programs(self, programs=None, search=None, target=None):
> +"""Wrapper to find binaries on system, search for each element
> + in programs, testing each path in search, under directory
> + target.
> +"""
> +return find_programs(programs=programs, search=search, target=target)
> +
> @abc.abstractmethod
> def package_command(self, cmd, args=None, pkgs=None):
> raise NotImplementedError()
> @@ -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
w.r.t the config; I think it may be how we're testing/mocking.
The _get_cloud() method takes a sys_cfg, and we're using a DataSourceNone which
does not *read* or accept any user-data config, so in effect, it looks like
user-data couldn't get merged to override a system setting.
I'll refactor the _get_cloud() method to accept a sys_cfg and user-data config,
and merge them there and validate that we can drop passing in the usercfg.
> +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 =
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
Review: Needs Fixing continuous-integration FAILED: Continuous integration, rev:7f1fbcf0ad85e918939b22f30236ef2a296c8afe https://jenkins.ubuntu.com/server/job/cloud-init-ci/944/ Executed test runs: SUCCESS: Checkout SUCCESS: Unit & Style Tests SUCCESS: Ubuntu LTS: Build SUCCESS: Ubuntu LTS: Integration FAILED: MAAS Compatability Testing Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/944/rebuild -- 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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
Review: Needs Fixing continuous-integration FAILED: Continuous integration, rev:adf6244a6a0403e6204f8536aab4e9278a814e25 https://jenkins.ubuntu.com/server/job/cloud-init-ci/938/ Executed test runs: SUCCESS: Checkout SUCCESS: Unit & Style Tests SUCCESS: Ubuntu LTS: Build FAILED: Ubuntu LTS: Integration Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/938/rebuild -- 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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
OK, I think I've addressed most of the comments here, except I still need to: - switch to temp_utils.mkstemp() for the custom template; I didn't want to fight temp_utils for the unittest, but it's the Right Thing(TM) so that's up next. I think the current commit log will show you what things I did address, so please look that over and see if I missed anything. -- 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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
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:
> +
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
minor inline comments, I'll have a pastebin of the updated schema dictionary as
I see it shaping up.
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("""\
It looks like the distro logic defaults to auto if unspecfied. So add
'default': 'auto',
> +Name of an NTP client to use to configure system NTP
Description should probably document: if unspecified or 'auto' it will default
to distribution's preference. Maybe we also want to mention known builtin
options ntp, chrony, ntpdate, systemd-timesyncd.
> +"""),
> +},
> +'enabled': {
> +'type': 'boolean',
Add 'default': True, here for future schema docs
> +'description': "",
> +},
For schema docs, it might be nice to also use the default key here with
'default': True
> +'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
> +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
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
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
> @@ -103,6 +171,9 @@ def handle(name, cfg, cloud, log, _args):
> ntp_cfg = cfg['ntp']
> if ntp_cfg is None:
> ntp_cfg = {} # Allow empty config which will install the package
> +else:
> +# do not allow handle updates to modify the cfg object
> +ntp_cfg = cfg['ntp'].copy()
well, the dictionary has at least 'packages', which is an array.
and so a copy() will not give you a new copy of taht array.
>
> # TODO drop this when validate_cloudconfig_schema is strict=True
> if not isinstance(ntp_cfg, (dict)):
> 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
> @@ -49,6 +49,48 @@ LOG = logging.getLogger(__name__)
> # It could break when Amazon adds new regions and new AZs.
> _EC2_AZ_RE = re.compile('^[a-z][a-z]-(?:[a-z]+-)+[0-9][a-z]$')
>
> +# Default NTP Client Configurations
> +PREFERRED_NTP_CLIENTS = ['systemd-timesyncd', 'ntp', 'ntpdate']
One way or another, I think what we're after is something like:
if one of ALL_SUPPORTED_NTP_CLIENTS is installed:
configure it
elif one of ALL_SUPPORTED_NTP_CLIENTS is installable
install it
configure it.
so adding chrony to the list of things that would be used if present should not
hurt anything.
right?
> +NTP_CONF = '/etc/ntp.conf'
> +NTP_CLIENT_CONFIG = {
> +'chrony': {
> +'check_exe': 'chronyd',
> +'confpath': '/etc/chrony.conf',
> +'name': 'chrony',
> +'packages': ['chrony'],
> +'service_name': 'chrony',
> +'template_name': 'chrony.conf.{distro}',
> +'template': None,
> +},
> +'ntp': {
> +'check_exe': 'ntpd',
> +'confpath': NTP_CONF,
> +'name': 'ntp',
> +'packages': ['ntp'],
> +'service_name': 'ntp',
> +'template_name': 'ntp.conf.{distro}',
> +'template': None,
> +},
> +'ntpdate': {
> +'check_exe': 'ntpdate',
> +'confpath': NTP_CONF,
> +'name': 'ntpdate',
> +'packages': ['ntpdate'],
> +'service_name': 'ntpdate',
> +'template_name': 'ntp.conf.{distro}',
> +'template': None,
> +},
> +'systemd-timesyncd': {
> +'check_exe': '/lib/systemd/systemd-timesyncd',
> +'confpath': '/etc/systemd/timesyncd.conf.d/cloud-init.conf',
> +'name': 'systemd-timesyncd',
> +'packages': [],
> +'service_name': 'systemd-timesyncd',
> +'template_name': 'timesyncd.conf',
> +'template': None,
> +},
> +}
> +
>
> @six.add_metaclass(abc.ABCMeta)
> class Distro(object):
> @@ -109,6 +153,17 @@ class Distro(object):
> """Wrapper to report whether this distro uses systemd or sysvinit."""
> return uses_systemd()
>
> +def package_installer(self):
> +"""Wrapper to report whether this distro has a package installer."""
> +return package_installer()
maybe a 'can_install_package(pkg)' function would make sense.
that would allow you to figure out which of the list of packages are
installable.
And in ubuntu core case, that would always return False (until 'snap install
ntp' works).
> +
> +def find_programs(self, programs=None, search=None, target=None):
> +"""Wrapper to find binaries on system, search for each element
> + in programs, testing each path in search, under directory
> + target.
> +"""
> +return find_programs(programs=programs, search=search, target=target)
> +
> @abc.abstractmethod
> def package_command(self, cmd, args=None, pkgs=None):
> raise NotImplementedError()
> @@ -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
> +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
> +
Re: [Cloud-init-dev] [Merge] ~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master
Review: Needs Fixing continuous-integration FAILED: Continuous integration, rev:dad6bd58753bd7af4b4eacea08af818f07bd4284 https://jenkins.ubuntu.com/server/job/cloud-init-ci/791/ Executed test runs: SUCCESS: Checkout SUCCESS: Unit & Style Tests SUCCESS: Ubuntu LTS: Build FAILED: Ubuntu LTS: Integration Click here to trigger a rebuild: https://jenkins.ubuntu.com/server/job/cloud-init-ci/791/rebuild -- 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
