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.

_______________________________________________
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

Reply via email to