cloud-init-dev team mailing list archive

[Scott Moser <ssmoser2+ubuntu@xxxxxxxxx>]

some little things/food for thought.
and you have some conflicts.


Diff comments:

> diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py
> index 7007d9e..d30435e 100644
> --- a/cloudinit/sources/DataSourceAzure.py
> +++ b/cloudinit/sources/DataSourceAzure.py
> @@ -373,41 +390,86 @@ class DataSourceAzure(sources.DataSource):
>              except NonAzureDataSource:
>                  continue
>              except BrokenAzureDataSource as exc:
> -                raise exc
> +                msg = 'BrokenAzureDataSource: %s' % exc
> +                raise sources.InvalidMetaDataException(msg)
>              except util.MountFailedError:
>                  LOG.warning("%s was not mountable", cdev)
>                  continue
>  
>              if reprovision or self._should_reprovision(ret):
>                  ret = self._reprovision()
> -            (md, self.userdata_raw, cfg, files) = ret
> +            imds_md = get_metadata_from_imds(
> +                self.fallback_interface, retries=3)
> +            (md, userdata_raw, cfg, files) = ret
>              self.seed = cdev
> -            self.metadata = util.mergemanydict([md, DEFAULT_METADATA])
> -            self.cfg = util.mergemanydict([cfg, BUILTIN_CLOUD_CONFIG])
> +            crawled_data.update({
> +                'cfg': cfg,
> +                'files': files,
> +                'metadata': util.mergemanydict(
> +                    [md, {'imds': imds_md}]),
> +                'userdata_raw': userdata_raw})
>              found = cdev
>  
>              LOG.debug("found datasource in %s", cdev)
>              break
>  
>          if not found:
> -            return False
> +            raise sources.InvalidMetaDataException('No Azure metadata found')
>  
>          if found == ddir:
>              LOG.debug("using files cached in %s", ddir)
>  
>          seed = _get_random_seed()
>          if seed:
> -            self.metadata['random_seed'] = seed
> +            crawled_data['metadata']['random_seed'] = seed
> +        crawled_data['metadata']['instance-id'] = util.read_dmi_data(
> +            'system-uuid')
> +        return crawled_data
> +
> +    def is_platform_viable(self):

i like this idea, but I think that this is at best a @classmethod.
Ie, it doesn't use 'self' at all, and doesn't need to.

I think I prefer just having a method in the DataSourceAzure.py 
named 'is_platform_viable', and then having this call that.

thoughts?

> +        """Check platform environment to report if this datasource may run."""
> +        asset_tag = util.read_dmi_data('chassis-asset-tag')
> +        if asset_tag != AZURE_CHASSIS_ASSET_TAG:
> +            LOG.debug("Non-Azure DMI asset tag '%s' discovered.", asset_tag)
> +            return False
> +        return True
> +
> +    def clear_cached_attrs(self):
> +        """Reset any cached class attributes to defaults."""
> +        super(DataSourceAzure, self).clear_cached_attrs()
> +        self._metadata_imds = sources.UNSET
> +
> +    def _get_data(self):
> +        """Crawl and process datasource metadata caching metadata as attrs.
> +
> +        @return: True on success, False on error, invalid or disabled
> +            datasource.
> +        """
> +        if not self.is_platform_viable():
> +            return False
> +        try:
> +            crawled_data = util.log_time(
> +                        logfunc=LOG.debug, msg='Crawl of metadata service',
> +                        func=self.crawl_metadata)
> +        except sources.InvalidMetaDataException as e:
> +            LOG.warning('Could not crawl Azure metadata: %s', e)
> +            return False
> +
> +        # Process crawled data and augment with various config defaults
> +        self.cfg = util.mergemanydict(
> +            [crawled_data['cfg'], BUILTIN_CLOUD_CONFIG])
> +        self._metadata_imds = crawled_data['metadata']['imds']
> +        self.metadata = util.mergemanydict(
> +            [crawled_data['metadata'], DEFAULT_METADATA])
> +        self.userdata_raw = crawled_data['userdata_raw']
>  
>          user_ds_cfg = util.get_cfg_by_path(self.cfg, DS_CFG_PATH, {})
>          self.ds_cfg = util.mergemanydict([user_ds_cfg, self.ds_cfg])
>  
>          # walinux agent writes files world readable, but expects
>          # the directory to be protected.
> -        write_files(ddir, files, dirmode=0o700)
> -
> -        self.metadata['instance-id'] = util.read_dmi_data('system-uuid')
> -
> +        write_files(
> +            self.ds_cfg['data_dir'], crawled_data['files'], dirmode=0o700)
>          return True
>  
>      def device_name_to_device(self, name):
> @@ -543,22 +605,56 @@ class DataSourceAzure(sources.DataSource):
>      @property
>      def network_config(self):
>          """Generate a network config like net.generate_fallback_network() with
> -           the following execptions.
> +           the following exceptions.
>  
>             1. Probe the drivers of the net-devices present and inject them in
>                the network configuration under params: driver: <driver> value
>             2. Generate a fallback network config that does not include any of
>                the blacklisted devices.
>          """
> -        blacklist = ['mlx4_core']
>          if not self._network_config:
> -            LOG.debug('Azure: generating fallback configuration')
> -            # generate a network config, blacklist picking any mlx4_core devs
> -            netconfig = net.generate_fallback_config(
> -                blacklist_drivers=blacklist, config_driver=True)
> +            if self._metadata_imds != sources.UNSET and self._metadata_imds:
> +                netconfig = {'version': 2, 'ethernets': {}}
> +                LOG.debug('Azure: generating network configuration from IMDS')
> +                if self.distro and self.distro.name == 'ubuntu':
> +                    maybe_remove_ubuntu_network_config_scripts()

this doesnt seem the right place for this.
network_config() doesn't seem like it should have these side effects.
it is supposed to return a network config not potentially delete things.

i dont know wher those actuaons fit, but It doesn't feel right here.

> +                network_metadata = self._metadata_imds['network']
> +                for idx, intf in enumerate(network_metadata['interface']):

other datasources have this in a non-class method also, which makes it easier to test
the simple conversion.
  self.assertEqual(expected, convert_network_data(some_data))

> +                    nicname = 'eth{idx}'.format(idx=idx)
> +                    dev_config = {}
> +                    for addr4 in intf['ipv4']['ipAddress']:
> +                        privateIpv4 = addr4['privateIpAddress']
> +                        if privateIpv4:
> +                            if dev_config.get('dhcp4', False):
> +                                # Append static address config for nic > 1
> +                                netPrefix = intf['ipv4']['subnet'][0].get(
> +                                    'prefix', '24')
> +                                if not dev_config.get('addresses'):
> +                                    dev_config['addresses'] = []
> +                                dev_config['addresses'].append(
> +                                    '{ip}/{prefix}'.format(
> +                                        ip=privateIpv4, prefix=netPrefix))
> +                            else:
> +                                dev_config['dhcp4'] = True
> +                    for addr6 in intf['ipv6']['ipAddress']:
> +                        privateIpv6 = addr6['privateIpAddress']
> +                        if privateIpv6:
> +                            dev_config['dhcp6'] = True
> +                            break
> +                    if dev_config:
> +                        mac = ':'.join(re.findall(r'..', intf['macAddress']))
> +                        dev_config.update(
> +                            {'match': {'macaddress': mac.lower()},
> +                             'set-name': nicname})
> +                        netconfig['ethernets'][nicname] = dev_config
> +            else:
> +                blacklist = ['mlx4_core']
> +                LOG.debug('Azure: generating fallback configuration')
> +                # generate a network config, blacklist picking mlx4_core devs
> +                netconfig = net.generate_fallback_config(
> +                    blacklist_drivers=blacklist, config_driver=True)
>  
>              self._network_config = netconfig
> -
>          return self._network_config
>  
>  
> diff --git a/tests/unittests/test_datasource/test_azure.py b/tests/unittests/test_datasource/test_azure.py
> index e82716e..1258d10 100644
> --- a/tests/unittests/test_datasource/test_azure.py
> +++ b/tests/unittests/test_datasource/test_azure.py
> @@ -314,6 +511,20 @@ fdescfs            /dev/fd          fdescfs rw              0 0
>          self.assertTrue(ret)
>          self.assertEqual(data['agent_invoked'], cfg['agent_command'])
>  
> +    def test_network_config_set_from_imds(self):
> +        """Datasource.network_config returns IMDS network data."""
> +        odata = {}
> +        data = {'ovfcontent': construct_valid_ovf_env(data=odata)}
> +        expected_network_config = {
> +            'ethernets': {
> +                'eth0': {'set-name': 'eth0',
> +                         'match': {'macaddress': '00:0d:3a:04:75:98'},
> +                         'dhcp4': True}},
> +            'version': 2}
> +        dsrc = self._get_ds(data)
> +        dsrc.get_data()
> +        self.assertEqual(expected_network_config, dsrc.network_config)

this is what i was talking about, if you have a 'convert_network_config' function in DataSourceAzure.py
then you can just test it with:
   self.assertEqual(expected_network_config, convert_network_config("That Config"))

> +
>      def test_user_cfg_set_agent_command(self):
>          # set dscfg in via base64 encoded yaml
>          cfg = {'agent_command': "my_command"}


-- 
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/348704
Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:feature/azure-network-on-boot into cloud-init:master.