cloud-init-dev team mailing list archive

[Dan Watkins <daniel.watkins@xxxxxxxxxxxxx>]

Thanks for the reviews!  Responses inline.

Diff comments:

> diff --git a/cloudinit/sources/DataSourceOracle.py b/cloudinit/sources/DataSourceOracle.py
> index 76cfa38..f18e5a0 100644
> --- a/cloudinit/sources/DataSourceOracle.py
> +++ b/cloudinit/sources/DataSourceOracle.py
> @@ -28,8 +28,70 @@ import re
>  
>  LOG = logging.getLogger(__name__)
>  
> +BUILTIN_DS_CONFIG = {
> +    # Don't use IMDS to configure secondary NICs by default
> +    'configure_secondary_nics': False,
> +}
>  CHASSIS_ASSET_TAG = "OracleCloud.com"
>  METADATA_ENDPOINT = "http://169.254.169.254/openstack/";
> +VNIC_METADATA_URL = 'http://169.254.169.254/opc/v1/vnics/'
> +
> +
> +def _network_config_from_opc_imds(network_config):
> +    """
> +    Fetch data from Oracle's IMDS and generate secondary NIC config.
> +
> +    The primary NIC configuration should not be modified based on the IMDS
> +    values, as it should continue to be configured for DHCP.  As such, this
> +    takes an existing network_config dict which is expected to have the primary
> +    NIC configuration already present.
> +
> +    :param network_config:
> +        A v1 network config dict with the primary NIC already configured.  This
> +        dict will be mutated.
> +
> +    :raises:
> +        Exceptions are not handled within this function.  Likely exceptions are
> +        those raised by url_helper.readurl (if communicating with the IMDS
> +        fails), ValueError/JSONDecodeError (if the IMDS returns invalid JSON),
> +        and KeyError/IndexError (if the IMDS returns valid JSON with unexpected
> +        contents).
> +
> +    :return:
> +        The ``network_config`` dict with secondary NICs added.

"This is v1 config, because cloudinit.net.cmdline generates v1 config and we need to integrate the secondary NICs into that configuration."  I've filed https://bugs.launchpad.net/cloud-init/+bug/1839537 for that migration, and included a note about this in there.

(I worked so hard on that commit message and you're telling me you didn't even read it????? ;)

> +    """
> +    resp = readurl(VNIC_METADATA_URL)
> +    vnics = json.loads(str(resp))
> +
> +    if 'nicIndex' in vnics[0]:
> +        LOG.debug('VNIC metadata indicates this is a bare metal machine;'

This is a good question.  Our eventual aim is to have secondary NIC generation enabled by default.  In that scenario, I don't think this should be a warning, because cloud-init is operating exactly as expected.  However, as it's disabled by default at the moment, I agree that a WARNING probably is more appropriate.

(This does make me wonder, though, if this indicates that the configuration option name is still too broad.  When we come to implementing this for Bare Metal Machines, do we really want the same toggle for both code paths?  I'm going to noodle on this some.)

> +                  ' skipping secondary VNIC configuration.')
> +        return network_config
> +
> +    interfaces_by_mac = get_interfaces_by_mac()
> +
> +    for vnic_dict in vnics[1:]:
> +        mac_address = vnic_dict['macAddr'].lower()
> +        if mac_address not in interfaces_by_mac:
> +            LOG.info('Interface with MAC %s not found; skipping', mac_address)
> +            continue
> +        name = interfaces_by_mac[mac_address]
> +        subnet = {
> +            'type': 'static',
> +            'address': vnic_dict['privateIp'],
> +            'netmask': vnic_dict['subnetCidrBlock'].split('/')[1],
> +            'gateway': vnic_dict['virtualRouterIp'],
> +            'control': 'manual',
> +        }
> +        network_config['config'].append({
> +            'name': name,
> +            'type': 'physical',
> +            'mac_address': mac_address,
> +            'mtu': 9000,

That's what The Script uses, and Si-Wei also made sure to call it out.  I've also found it in their documentation[0], shall I add a comment pointing there?


[0] https://docs.cloud.oracle.com/iaas/Content/Network/Troubleshoot/connectionhang.htm#Overview

> +            'subnets': [subnet],
> +        })
> +
> +    return network_config
>  
>  
>  class DataSourceOracle(sources.DataSource):
> @@ -121,6 +190,14 @@ class DataSourceOracle(sources.DataSource):
>              self._network_config = cmdline.read_initramfs_config()
>              if not self._network_config:
>                  self._network_config = self.distro.generate_fallback_config()
> +            if self.ds_cfg.get('configure_secondary_nics'):

Yes, I think we should.  Looking around the codebase, there are a few other places that should be doing this too, so I'll file a bug for those.  Good catch!

> +                try:
> +                    self._network_config = _network_config_from_opc_imds(
> +                        self._network_config)
> +                except Exception:
> +                    util.logexc(
> +                        LOG,
> +                        "Failed to fetch secondary network configuration!")
>          return self._network_config
>  
>  


-- 
https://code.launchpad.net/~daniel-thewatkins/cloud-init/+git/cloud-init/+merge/371053
Your team cloud-init commiters is requested to review the proposed merge of ~daniel-thewatkins/cloud-init/+git/cloud-init:oci-vnic into cloud-init:master.