cloud-init-dev team mailing list archive

[Scott Moser <ssmoser2+ubuntu@xxxxxxxxx>]

Some small comments.
Unit tests and a few fixes and we'll be good.
Thanks.


Diff comments:

> diff --git a/cloudinit/sources/DataSourceHetzner.py b/cloudinit/sources/DataSourceHetzner.py
> new file mode 100644
> index 0000000..5d63d0d
> --- /dev/null
> +++ b/cloudinit/sources/DataSourceHetzner.py
> @@ -0,0 +1,97 @@
> +# Author: Jonas Keidel <jonas.keidel@xxxxxxxxxxx>
> +# Author: Markus Schade <markus.schade@xxxxxxxxxxx>
> +#
> +# This file is part of cloud-init. See LICENSE file for license information.
> +#
> +# Hetzner Cloud API:

Make this a docstring
"""Hetzner Cloud API Datasource.
   https://docs.hetzner.cloud/""";

I just took  a quick look at the  url and I didn't see anything on the metadata service there... did I just miss it?

> +# https://docs.hetzner.cloud/
> +
> +from cloudinit import log as logging
> +from cloudinit import sources
> +from cloudinit import util
> +
> +import cloudinit.sources.helpers.hetzner as hc_helper
> +
> +LOG = logging.getLogger(__name__)
> +
> +BASE_URL_V1 = 'http://169.254.169.254/hetzner/v1'
> +
> +BUILTIN_DS_CONFIG = {
> +    'metadata_url': BASE_URL_V1 + '/metadata',
> +    'userdata_url': BASE_URL_V1 + '/userdata',
> +}
> +
> +MD_RETRIES = 60
> +MD_TIMEOUT = 2
> +MD_WAIT_RETRY = 2
> +
> +
> +class DataSourceHetzner(sources.DataSource):
> +    def __init__(self, sys_cfg, distro, paths):
> +        sources.DataSource.__init__(self, sys_cfg, distro, paths)
> +        self.distro = distro
> +        self.metadata = dict()
> +        self.ds_cfg = util.mergemanydict([
> +            util.get_cfg_by_path(sys_cfg, ["datasource", "Hetzner"], {}),
> +            BUILTIN_DS_CONFIG])
> +        self.metadata_address = self.ds_cfg['metadata_url']
> +        self.userdata_address = self.ds_cfg['userdata_url']
> +        self.retries = self.ds_cfg.get('retries', MD_RETRIES)
> +        self.timeout = self.ds_cfg.get('timeout', MD_TIMEOUT)
> +        self.wait_retry = self.ds_cfg.get('wait_retry', MD_WAIT_RETRY)
> +        self._network_config = None
> +        self.dsmode = sources.DSMODE_NETWORK
> +
> +    def get_data(self):
> +        ephv4 = hc_helper.add_local_ip()
> +

Is there a reason to not just use the context manager of EphemeralIPv4Network?

nic = cloudnet.find_fallback_nic()
if not nic:
   raise RuntimeError(...)
   
with cloudnet.EphemeralIPv4Network(nic, "169.254.0.1", 16, 169.254.255.255) as eph4:
    md = hc_helper.read_metadata(....)
    ud = hc_helper.read_userdata

self.userdata_raw = ud
self.metadata_raw = md
...

I also note that we should not require broadcast to EphemeralIPv4Network if prefix_or_mask is given.  But thats not your fault.
It would just maek it shorter to use if we fixed it (EphemeralIPv4Network(nic, "169.254.0.1", 16)).
And even more simple EphemeralIPv4Network(nic, "169.254.0.1/16")

> +        md = hc_helper.read_metadata(
> +            self.metadata_address, timeout=self.timeout,
> +            sec_between=self.wait_retry, retries=self.retries)
> +
> +        ud = hc_helper.read_userdata(
> +            self.userdata_address, timeout=self.timeout,
> +            sec_between=self.wait_retry, retries=self.retries)
> +        self.userdata_raw = ud
> +
> +        self.metadata_full = md
> +        self.metadata['instance-id'] = md.get('instance-id', None)
> +        self.metadata['local-hostname'] = md.get('hostname', None)

I'm curious if this is ever *not* set.
Specifically because we are looking to have cloud-init set the hostname earlier. 
(https://pad.lv/1746455)
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/339720

It would be nice to state that 
a.) if local-hostname is always present
b.) if it is a fully qualified domain name.

> +        self.metadata['network-config'] = md.get('network-config', None)
> +        self.metadata['public-keys'] = md.get('public-keys', None)
> +        self.vendordata_raw = md.get("vendor_data", None)
> +
> +        hc_helper.remove_local_ip(ephv4)
> +
> +        return True
> +
> +    @property
> +    def network_config(self):
> +        """Configure the networking. This needs to be done each boot, since
> +           the IP information may have changed due to snapshot and/or
> +           migration.
> +        """
> +
> +        if self._network_config:
> +            return self._network_config
> +
> +        _net_config = self.metadata['network-config']
> +        if not _net_config:
> +            raise Exception("Unable to get meta-data from server....")
> +
> +        self._network_config = _net_config
> +
> +        return self._network_config
> +
> +
> +# Used to match classes to dependencies
> +datasources = [
> +    (DataSourceHetzner, (sources.DEP_FILESYSTEM, )),
> +]
> +
> +
> +# Return a list of data sources that match this set of dependencies
> +def get_datasource_list(depends):
> +    return sources.list_from_depends(depends, datasources)
> +
> +# vi: ts=4 expandtab
> diff --git a/cloudinit/sources/helpers/hetzner.py b/cloudinit/sources/helpers/hetzner.py
> new file mode 100644
> index 0000000..3058f71
> --- /dev/null
> +++ b/cloudinit/sources/helpers/hetzner.py
> @@ -0,0 +1,47 @@
> +# Author: Jonas Keidel <jonas.keidel@xxxxxxxxxxx>
> +# Author: Markus Schade <markus.schade@xxxxxxxxxxx>
> +#
> +# This file is part of cloud-init. See LICENSE file for license information.
> +
> +from cloudinit import log as logging
> +from cloudinit import net as cloudnet
> +from cloudinit import url_helper
> +from cloudinit import util
> +
> +LOG = logging.getLogger(__name__)
> +
> +

you dont *have* to have these in helpers/hetzner.  I realize you were largely just following other code, but I have no problem with including these in the DataSourceHetzner.py.

> +def read_metadata(url, timeout=2, sec_between=2, retries=30):
> +    response = url_helper.readurl(url, timeout=timeout,
> +                                  sec_between=sec_between, retries=retries)
> +    if not response.ok():
> +        raise RuntimeError("unable to read metadata at %s" % url)
> +    return util.load_yaml(response.contents.decode())
> +
> +
> +def read_userdata(url, timeout=2, sec_between=2, retries=30):
> +    response = url_helper.readurl(url, timeout=timeout,
> +                                  sec_between=sec_between, retries=retries)
> +    if not response.ok():
> +        raise RuntimeError("unable to read userdata at %s" % url)
> +    return response.contents
> +
> +
> +def add_local_ip():
> +    nic = cloudnet.find_fallback_nic()
> +    LOG.debug("selected interface '%s' for reading metadata", nic)
> +
> +    if not nic:
> +        raise RuntimeError("unable to find interfaces to access the"
> +                           "meta-data server. This server is broken.")
> +
> +    ephv4 = cloudnet.EphemeralIPv4Network(
> +        nic, "169.254.0.1", 16, "169.254.255.255"
> +    )
> +    ephv4.__enter__()
> +
> +    return ephv4
> +
> +
> +def remove_local_ip(ephv4=None):
> +    ephv4._delete_address("169.254.0.1", 16)


-- 
https://code.launchpad.net/~lp-markusschade/cloud-init/+git/cloud-init/+merge/338439
Your team cloud-init commiters is requested to review the proposed merge of ~lp-markusschade/cloud-init:hetznercloud_ds into cloud-init:master.