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.kei...@hetzner.com> > +# Author: Markus Schade <markus.sch...@hetzner.com> > +# > +# 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.kei...@hetzner.com> > +# Author: Markus Schade <markus.sch...@hetzner.com> > +# > +# 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. _______________________________________________ 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
