cloud-init-dev team mailing list archive

[Scott Moser <ssmoser2+ubuntu@xxxxxxxxx>]

This looks pretty good, but we need some unit test on it.
some comments inline.

I know it can be annoying when someone says "you should refactor this".
We can help you, but we just added the EphemeralIPv4Network with this use case in mind, so it'd be good to use it.

Diff comments:

> diff --git a/cloudinit/sources/DataSourceHetzner.py b/cloudinit/sources/DataSourceHetzner.py
> new file mode 100644
> index 0000000..ea81a0c
> --- /dev/null
> +++ b/cloudinit/sources/DataSourceHetzner.py
> @@ -0,0 +1,95 @@
> +# 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:
> +# https://docs.hetzner.cloud/
> +
> +from cloudinit import log as logging
> +from cloudinit import sources
> +from cloudinit import util
> +
> +import cloudinit.sources.helpers.hetzner as ho_helper
> +
> +LOG = logging.getLogger(__name__)
> +
> +BUILTIN_DS_CONFIG = {
> +    'metadata_url': 'http://169.254.169.254/hetzner/v1/metadata',
> +    'userdata_url': 'http://169.254.169.254/hetzner/v1/userdata',

Lets combine these 2 urls.
 BASE_URL_V1 = http://169.254.169.254/hetzner/v1

BUILTIN_DS_CFG = {
   'metadata_url': BASE_URL_V1 + "/metadata"
}

is therea ny reason to believe that they would ever differ?
that you'd read user-data from one and meta-data from another base url?

if not, then why not just have the ds config containe 'base_url' or something.

> +}
> +
> +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):
> +        local_ip_nic = ho_helper.add_local_ip()
> +
> +        md = ho_helper.read_metadata(
> +            self.metadata_address, timeout=self.timeout,
> +            sec_between=self.wait_retry, retries=self.retries)
> +
> +        ud = ho_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)
> +        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)
> +
> +        ho_helper.remove_local_ip(local_ip_nic)
> +
> +        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..e48e21f
> --- /dev/null
> +++ b/cloudinit/sources/helpers/hetzner.py
> @@ -0,0 +1,74 @@
> +# 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__)
> +
> +
> +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())

is it really yaml and not json?

> +
> +
> +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 metadata at %s" % url)
> +    return response.contents.decode()

you probably dont want to decode it. most of the time user-data is a binary
blob.  decoding it will cause exception if it is not utf-8.
cloud-init transparently supports user-data being compressed with gzip, 
which would cause issue here.

Also, this looks like a copy of read_metadata.  even the RuntimeError is identical.

> +
> +
> +def add_local_ip(nic=None):
> +    nic = get_local_ip_nic()

you take a nic, but then dont use it at all.

if nic is None:
   nic = get_local_ip_nic()
LOG.debug("using interface '%s' for reading metadata", 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 droplet is broken.")

droplet ?
do you call it that?

> +
> +    ip_addr_cmd = ['ip', 'addr', 'add', '169.254.0.1/16', 'dev', nic]
> +    ip_link_cmd = ['ip', 'link', 'set', 'dev', nic, 'up']
> +
> +    if not util.which('ip'):
> +        raise RuntimeError("No 'ip' command available to configure local ip "
> +                           "address")
> +
> +    try:
> +        (result, _err) = util.subp(ip_addr_cmd)
> +        LOG.debug("assigned local ip to '%s'", nic)
> +
> +        (result, _err) = util.subp(ip_link_cmd)
> +        LOG.debug("brought device '%s' up", nic)
> +    except Exception:
> +        util.logexc(LOG, "local ip address assignment to '%s' failed.", nic)
> +        raise

this stuff needs to be factored into use 
 EphemeralIPv4Network

So does the digital ocean one... we just hadnt gotten around to it.

> +
> +    return nic
> +
> +
> +def get_local_ip_nic():
> +    nics = [f for f in cloudnet.get_devicelist() if cloudnet.is_physical(f)]
> +    if not nics:
> +        return None
> +    return min(nics, key=lambda d: cloudnet.read_sys_net_int(d, 'ifindex'))

do you know if this is more or less reliable than natural_sort_key ? that is used in find_fallback_nic.

you might be able to just find_fallback_nic() , no?

> +
> +
> +def remove_local_ip(nic=None):
> +    ip_addr_cmd = ['ip', 'addr', 'flush', 'dev', nic]
> +    ip_link_cmd = ['ip', 'link', 'set', 'dev', nic, 'down']
> +    try:
> +        (result, _err) = util.subp(ip_addr_cmd)
> +        LOG.debug("removed all addresses from %s", nic)
> +        (result, _err) = util.subp(ip_link_cmd)
> +        LOG.debug("brought device '%s' down", nic)
> +    except Exception as e:
> +        util.logexc(LOG, "failed to remove all address from '%s'.", nic, e)


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