cloud-init-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

Thanks for submitting this.  Some inline comments and questions.

In your description you mention networking configuration issues.  Have you looked at providing network-metadata and having your datasource provide a network-config property to allow cloud-init to render networking[1] for the instance?

1. https://cloudinit.readthedocs.io/en/latest/topics/network-config.html

Diff comments:

> diff --git a/cloudinit/sources/DataSourceExoscale.py b/cloudinit/sources/DataSourceExoscale.py
> new file mode 100644
> index 0000000..4588c9d
> --- /dev/null
> +++ b/cloudinit/sources/DataSourceExoscale.py
> @@ -0,0 +1,126 @@
> +import time
> +
> +from cloudinit import ec2_utils as ec2
> +from cloudinit import log as logging
> +from cloudinit import sources
> +from cloudinit import url_helper
> +
> +API_VERSION = "latest"

Do you have or plan to have versioned APIs  We typically discourage use of latest since the implementation may change on the user.
If you keep latest, could you include a URL comment here pointing to any info on your API stability/expectations?

> +LOG = logging.getLogger(__name__)
> +SERVICE_ADDRESS = "http://169.254.169.254";
> +
> +
> +class DataSourceExoscale(sources.DataSource):
> +
> +    dsname = 'Exoscale'
> +    url_timeout = 10
> +    url_retries = 6
> +    url_max_wait = 60
> +
> +    def __init__(self, sys_cfg, distro, paths):
> +        sources.DataSource.__init__(self, sys_cfg, distro, paths)
> +        LOG.info("Initializing the Exoscale datasource")
> +        self.extra_config = {}
> +
> +    def get_password(self):
> +        """Return the VM's passwords."""
> +        LOG.info("Fetching password from metadata service")

Could we drop the logging in get_password()?  I suspect that either users are going to be able to login with the password they expect
and if they can't these messages won't help them understand why they can't log in.

> +        password_url = "{}:8080".format(SERVICE_ADDRESS)
> +        response = url_helper.read_file_or_url(
> +            password_url,
> +            ssl_details=None,
> +            headers={"DomU_Request": "send_my_password"},
> +            timeout=self.url_timeout,
> +            retries=self.url_retries)
> +        password = response.contents.decode('utf-8')
> +        # the password is empty or already saved
> +        if password in ['', 'saved_password']:
> +            LOG.info("Password is missing or already saved")
> +            return None
> +        LOG.info("Found the password in metadata service")
> +        # save the password
> +        url_helper.read_file_or_url(
> +            password_url,
> +            ssl_details=None,
> +            headers={"DomU_Request": "saved_password"},
> +            timeout=self.url_timeout,
> +            retries=self.url_retries)
> +        LOG.info("password saved")
> +        return password
> +
> +    def wait_for_metadata_service(self):
> +        """Wait for the metadata service to be reachable."""
> +        LOG.info("waiting for the metadata service")
> +        start_time = time.time()
> +
> +        metadata_url = "{}/{}/meta-data/instance-id".format(
> +            SERVICE_ADDRESS,
> +            API_VERSION)
> +
> +        start_time = time.time()

You initialize start_time twice here.

You may want to use util.log_time() which takes a func. See cloudinit/sources/DataSourceEc2.py:_get_data
for usage.

> +        url = url_helper.wait_for_url(
> +            urls=[metadata_url],
> +            max_wait=self.url_max_wait,
> +            timeout=self.url_timeout,
> +            status_cb=LOG.critical)
> +
> +        if url:
> +            LOG.info("metadata service ok")

Could we drop this?  This is the most likely path we'll take and expect metadata services to be OK.

> +            return True
> +        else:
> +            wait_time = int(time.time() - start_time)
> +            LOG.critical(("Giving up on waiting for the metadata from %s"
> +                          " after %s seconds"),
> +                         url,
> +                         wait_time)
> +            return False
> +
> +    def _get_data(self):
> +        """Fetch the user data, the metadata and the VM password
> +        from the metadata service."""
> +        LOG.info("fetching data")

LOG.debug, maybe something like 'Crawl of metadata service'; other datasources have similar message when they hit their service url.

> +        if not self.wait_for_metadata_service():
> +            return False
> +        start_time = time.time()
> +        self.userdata_raw = ec2.get_instance_userdata(API_VERSION,

Could you move the fetching into a function called 'crawl_metadata' which just connects and fetches the current metadata and returns the collected data.
We typically then include a __main__ function to easily invoke the datasource crawl-metadata function to dump the current
state.  See cloudinit/sources/DataSourceOracle.py for current example.

> +                                                      SERVICE_ADDRESS,
> +                                                      timeout=self.url_timeout,
> +                                                      retries=self.url_retries)
> +        self.metadata = ec2.get_instance_metadata(API_VERSION,
> +                                                  SERVICE_ADDRESS,
> +                                                  timeout=self.url_timeout,
> +                                                  retries=self.url_retries)
> +        password = self.get_password()
> +        if password:
> +            self.extra_config = {
> +                'ssh_pwauth': True,
> +                'password': password,
> +                'chpasswd': {
> +                    'expire': False,
> +                },
> +            }
> +        get_data_time = int(time.time() - start_time)
> +        LOG.info("finished fetching the metadata in %s seconds",
> +                 get_data_time)
> +        return True
> +
> +    def get_config_obj(self):
> +        return self.extra_config
> +
> +    def get_instance_id(self):
> +        return self.metadata['instance-id']
> +
> +    @property
> +    def availability_zone(self):
> +        return self.metadata['availability-zone']
> +
> +
> +# Used to match classes to dependencies
> +datasources = [
> +    (DataSourceExoscale, (sources.DEP_FILESYSTEM, sources.DEP_NETWORK)),
> +]
> +
> +
> +# Return a list of data sources that match this set of dependencies
> +def get_datasource_list(depends):
> +    return sources.list_from_depends(depends, datasources)
> diff --git a/tests/unittests/test_datasource/test_exoscale.py b/tests/unittests/test_datasource/test_exoscale.py
> new file mode 100644
> index 0000000..4bb9379
> --- /dev/null
> +++ b/tests/unittests/test_datasource/test_exoscale.py
> @@ -0,0 +1,93 @@
> +# This file is part of cloud-init. See LICENSE file for license information.
> +from cloudinit import helpers
> +from cloudinit.sources.DataSourceExoscale import (
> +    API_VERSION,
> +    DataSourceExoscale,
> +    SERVICE_ADDRESS)
> +from cloudinit.tests.helpers import HttprettyTestCase
> +
> +import httpretty
> +
> +
> +@httpretty.activate
> +class TestDatasourceExoscale(HttprettyTestCase):
> +
> +    def setUp(self):
> +        super(TestDatasourceExoscale, self).setUp()
> +        self.tmp = self.tmp_dir()
> +
> +        self.password_url = "{}:8080/".format(SERVICE_ADDRESS)
> +        self.metadata_url = "{}/{}/meta-data/".format(SERVICE_ADDRESS,
> +                                                      API_VERSION)
> +        self.userdata_url = "{}/{}/user-data".format(SERVICE_ADDRESS,
> +                                                     API_VERSION)
> +
> +    def test_password_saved(self):
> +        """The password is not set when it is not found
> +        in the metadata service."""
> +        path = helpers.Paths({'run_dir': self.tmp})
> +        ds = DataSourceExoscale({}, None, path)
> +        httpretty.register_uri(httpretty.GET,
> +                               self.password_url,
> +                               body="saved_password")
> +        self.assertFalse(ds.get_password())
> +
> +    def test_password_empty(self):
> +        """No password is set if the metadata service returns
> +        an empty string."""
> +        path = helpers.Paths({'run_dir': self.tmp})
> +        ds = DataSourceExoscale({}, None, path)
> +        httpretty.register_uri(httpretty.GET,
> +                               self.password_url,
> +                               body="")
> +        self.assertFalse(ds.get_password())
> +
> +    def test_password(self):
> +        """The password is set to what is found in the metadata
> +        service."""
> +        path = helpers.Paths({'run_dir': self.tmp})
> +        ds = DataSourceExoscale({}, None, path)
> +        expected_password = "p@ssw0rd"
> +        httpretty.register_uri(httpretty.GET,
> +                               self.password_url,
> +                               body=expected_password)
> +        password = ds.get_password()
> +        self.assertEqual(expected_password, password)
> +
> +    def test_get_data(self):
> +        """The datasource conforms to expected behavior when supplied
> +        full test data."""
> +        path = helpers.Paths({'run_dir': self.tmp})
> +        ds = DataSourceExoscale({}, None, path)
> +        expected_password = "p@ssw0rd"
> +        expected_id = "12345"
> +        expected_hostname = "myname"
> +        expected_userdata = "#cloud-config"

Could you add a test to confirm tha if user-data provides values that you capture in self.extra_config that
user-config overrides the built-in?

> +        httpretty.register_uri(httpretty.GET,
> +                               self.userdata_url,
> +                               body=expected_userdata)
> +        httpretty.register_uri(httpretty.GET,
> +                               self.password_url,
> +                               body=expected_password)
> +        httpretty.register_uri(httpretty.GET,
> +                               self.metadata_url,
> +                               body="instance-id\nlocal-hostname")
> +        httpretty.register_uri(httpretty.GET,
> +                               "{}local-hostname".format(self.metadata_url),
> +                               body=expected_hostname)
> +        httpretty.register_uri(httpretty.GET,
> +                               "{}local-hostname".format(self.metadata_url),
> +                               body=expected_hostname)
> +        httpretty.register_uri(httpretty.GET,
> +                               "{}instance-id".format(self.metadata_url),
> +                               body=expected_id)
> +        ds._get_data()
> +        self.assertEqual(ds.userdata_raw.decode("utf-8"), "#cloud-config")
> +        self.assertEqual(ds.metadata, {"instance-id": expected_id,
> +                                       "local-hostname": expected_hostname})
> +        self.assertEqual(ds.get_config_obj(),
> +                         {'ssh_pwauth': True,
> +                          'password': expected_password,
> +                          'chpasswd': {
> +                              'expire': False,
> +                          }})


-- 
https://code.launchpad.net/~tribaal/cloud-init/+git/cloud-init/+merge/369516
Your team cloud-init commiters is requested to review the proposed merge of ~tribaal/cloud-init:feat/datasource-exoscale into cloud-init:master.