cloud-init-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

The commit summary sound funny, dhclient returned network_data.json ?

Openstack: Allow Openstaack to run at init-local using dhclient in a sandbox

Which mirrors what we did for Ec2 and Azure, etc;  the network_data.json is sort of an implementation detail.

Some questions inline as well.

Diff comments:

> diff --git a/cloudinit/sources/DataSourceEc2.py b/cloudinit/sources/DataSourceEc2.py
> index 21e9ef8..6405847 100644
> --- a/cloudinit/sources/DataSourceEc2.py
> +++ b/cloudinit/sources/DataSourceEc2.py
> @@ -59,16 +57,17 @@ class DataSourceEc2(sources.DataSource):
>      # for extended metadata content. IPv6 support comes in 2016-09-02
>      extended_metadata_versions = ['2016-09-02']
>  
> +    # Setup read_url parameters per get_url_params.
> +    url_max_wait = 120
> +    url_timeout = 50
> +
>      _cloud_platform = None
>  
> -    _network_config = _unset  # Used for caching calculated network config v1
> +    _network_config = sources.UNSET  # Used to cache calculated network cfg v1

This may be helpful in Azure as well

>  
>      # Whether we want to get network configuration from the metadata service.
>      get_network_metadata = False
>  
> -    # Track the discovered fallback nic for use in configuration generation.
> -    _fallback_interface = None
> -
>      def __init__(self, sys_cfg, distro, paths):
>          super(DataSourceEc2, self).__init__(sys_cfg, distro, paths)
>          self.metadata_address = None
> diff --git a/cloudinit/sources/DataSourceOpenStack.py b/cloudinit/sources/DataSourceOpenStack.py
> index fb166ae..b81415e 100644
> --- a/cloudinit/sources/DataSourceOpenStack.py
> +++ b/cloudinit/sources/DataSourceOpenStack.py
> @@ -99,38 +79,58 @@ class DataSourceOpenStack(openstack.SourceMixin, sources.DataSource):
>          self.metadata_address = url2base.get(avail_url)
>          return bool(avail_url)
>  
> -    def _get_data(self):
> -        try:
> -            if not self.wait_for_metadata_service():
> -                return False
> -        except IOError:
> -            return False
> +    def check_instance_id(self, sys_cfg):
> +        # quickly (local check only) if self.instance_id is still valid
> +        return sources.instance_id_matches_system_uuid(self.get_instance_id())
>  
> -        (_max_wait, timeout, retries) = self._get_url_settings()
> +    @property
> +    def network_config(self):
> +        """Return a network config dict for rendering ENI or netplan files."""
> +        if self._network_config != sources.UNSET:
> +            return self._network_config
>  
> -        try:
> -            results = util.log_time(LOG.debug,
> -                                    'Crawl of openstack metadata service',
> -                                    read_metadata_service,
> -                                    args=[self.metadata_address],
> -                                    kwargs={'ssl_details': self.ssl_details,
> -                                            'retries': retries,
> -                                            'timeout': timeout})
> -        except openstack.NonReadable:
> -            return False
> -        except (openstack.BrokenMetadata, IOError):
> -            util.logexc(LOG, "Broken metadata address %s",
> -                        self.metadata_address)
> -            return False
> +        if self.network_json == sources.UNSET:
> +            # this would happen if get_data hadn't been called. leave as UNSET
> +            LOG.warning(
> +                "Unexpected call to network_config when network_json is None.")
> +            return None
> +
> +        LOG.debug("network config provided via network_json")
> +        self._network_config = openstack.convert_net_json(
> +            self.network_json, known_macs=None)
> +        return self._network_config
> +
> +    def _get_data(self):
> +        """Crawl metadata, parse and persist that data for this instance.
> +
> +        @return: True when metadata discovered indicates OpenStack datasource.
> +            False when unable to contact metadata service or when metadata
> +            format is invalid or disabled.
> +        """
> +        if self.get_network_metadata:  # Setup networking in init-local stage.

Why do we toggle this?  Wouldn't we always want to fetch this in _get_data calls?

I thought we only need to use the EphemeralDHCPv4 if we're in Local mode, otherwise, _get_data() just runs assuming networking isup.

> +            try:
> +                with EphemeralDHCPv4(self.fallback_interface):
> +                    results = util.log_time(
> +                        logfunc=LOG.debug, msg='Crawl of metadata service',
> +                        func=self._crawl_metadata)
> +            except (NoDHCPLeaseError, sources.InvalidMetaDataException) as e:
> +                util.logexc(LOG, str(e))
> +                return False
> +        else:
> +            try:
> +                results = self._crawl_metadata()
> +            except sources.InvalidMetaDataException as e:
> +                util.logexc(LOG, str(e))
> +                return False
>  
>          self.dsmode = self._determine_dsmode([results.get('dsmode')])
>          if self.dsmode == sources.DSMODE_DISABLED:
>              return False
> -
>          md = results.get('metadata', {})
>          md = util.mergemanydict([md, DEFAULT_METADATA])
>          self.metadata = md
>          self.ec2_metadata = results.get('ec2-metadata')
> +        self.network_json = results.get('networkdata')
>          self.userdata_raw = results.get('userdata')
>          self.version = results['version']
>          self.files.update(results.get('files', {}))
> @@ -145,9 +145,42 @@ class DataSourceOpenStack(openstack.SourceMixin, sources.DataSource):
>  
>          return True
>  
> -    def check_instance_id(self, sys_cfg):
> -        # quickly (local check only) if self.instance_id is still valid
> -        return sources.instance_id_matches_system_uuid(self.get_instance_id())
> +    def _crawl_metadata(self):
> +        """Crawl metadata service when available.
> +
> +        @returns: Dictionary with all metadata discovered for this datasource.
> +        @raise: InvalidMetaDataException on unreadable or broken
> +            metadata.
> +        """
> +        try:
> +            if not self.wait_for_metadata_service():
> +                raise sources.InvalidMetaDataException(
> +                    'No active metadata service found')
> +        except IOError as e:
> +            raise sources.InvalidMetaDataException(
> +                'IOError contacting metadata service: {error}'.format(
> +                    error=str(e)))
> +
> +        (_max_wait, timeout, retries) = self.get_url_params()
> +
> +        try:
> +            result = util.log_time(
> +                LOG.debug, 'Crawl of openstack metadata service',
> +                read_metadata_service, args=[self.metadata_address],
> +                kwargs={'ssl_details': self.ssl_details, 'retries': retries,
> +                        'timeout': timeout})
> +        except openstack.NonReadable as e:
> +            raise sources.InvalidMetaDataException(str(e))
> +        except (openstack.BrokenMetadata, IOError):
> +            msg = 'Broken metadata address {addr}'.format(
> +                addr=self.metadata_address)
> +            raise sources.InvalidMetaDataException(msg)
> +        return result
> +
> +
> +class DataSourceOpenStackLocal(DataSourceOpenStack):
> +
> +    get_network_metadata = True  # Get metadata network config if present

I think this isn't needed;  Are you trying to prevent fetching the network metadata twice ?
In the Ec2 class, don't we fetch everything at local and when the non-local DS runs, we just don't crawl/get_data since we already have the info,

>  
>  
>  def read_metadata_service(base_url, ssl_details=None,
> diff --git a/cloudinit/sources/__init__.py b/cloudinit/sources/__init__.py
> index df0b374..b3ff9fb 100644
> --- a/cloudinit/sources/__init__.py
> +++ b/cloudinit/sources/__init__.py
> @@ -149,6 +173,41 @@ class DataSource(object):
>              'Subclasses of DataSource must implement _get_data which'
>              ' sets self.metadata, vendordata_raw and userdata_raw.')
>  
> +    def get_url_params(self):
> +        """Return the Datasource's prefered url_read parameters.
> +
> +        Subclasses may override url_max_wait, url_timeout, url_retries.
> +
> +        @return: A 3-tuple of  max_wait_seconds, timeout_seconds, num_retries.

Is it worth doing a named tuple with fields so the order doesn't matter ?

> +        """
> +        max_wait = self.url_max_wait
> +        try:
> +            max_wait = int(self.ds_cfg.get("max_wait", self.url_max_wait))
> +        except ValueError:
> +            util.logexc(
> +                LOG, "Config max_wait '%s' is not an int, using default '%s'",
> +                self.ds_cfg.get("max_wait"), max_wait)
> +
> +        timeout = self.url_timeout
> +        try:
> +            timeout = max(
> +                0, int(self.ds_cfg.get("timeout", self.url_timeout)))
> +        except ValueError:
> +            timeout = self.url_timeout
> +            util.logexc(
> +                LOG, "Config timeout '%s' is not an int, using default '%s'",
> +                self.ds_cfg.get('timeout'), timeout)
> +
> +        retries = self.url_retries
> +        try:
> +            retries = int(self.ds_cfg.get("retries", self.url_retries))
> +        except Exception:
> +            util.logexc(
> +                LOG, "Config retries '%s' is not an int, using default '%s'",
> +                self.ds_cfg.get('retries'), retries)
> +
> +        return (max_wait, timeout, retries)
> +
>      def get_userdata(self, apply_filter=False):
>          if self.userdata is None:
>              self.userdata = self.ud_proc.process(self.get_userdata_raw())


-- 
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/345806
Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:feature/os-local into cloud-init:master.