cloud-init-dev team mailing list archive

[Chad Smith <chad.smith@xxxxxxxxxxxxx>]

Generally let's try to get some unittest coverage on functions defined in cloudinit.net.freebsd.



Long term (separate branch after this) our eye should be toward gutting all the net-related class methods from cloudinit.distros.freebsd in favor of using this top-level cloudinit.net utility functions.

Since distros/__init__.py contains the method Distro.apply_network_config_names which calls net.apply_network_config_names which you are defining in net.freebsd. I think we can drop all net-related support methods from distros.freebsd to avoid all that duplication to support  our base distros class can rely on that implementation instead of all the duplicated net-relatapply_network_config_names

here's the patch I was thinking (not tested) http://paste.ubuntu.com/p/wwZgTxSY83/

 

Diff comments:

> diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py
> index 3642fb1..94fe6d9 100644
> --- a/cloudinit/net/__init__.py
> +++ b/cloudinit/net/__init__.py
> @@ -349,247 +211,53 @@ def apply_network_config_names(netcfg, strict_present=True, strict_busy=True):
>                         ' network config version: %s' % netcfg.get('version'))
>  
>  
> -def interface_has_own_mac(ifname, strict=False):
> -    """return True if the provided interface has its own address.
> -
> -    Based on addr_assign_type in /sys.  Return true for any interface
> -    that does not have a 'stolen' address. Examples of such devices
> -    are bonds or vlans that inherit their mac from another device.
> -    Possible values are:
> -      0: permanent address    2: stolen from another device
> -      1: randomly generated   3: set using dev_set_mac_address"""
> -
> -    assign_type = read_sys_net_int(ifname, "addr_assign_type")
> -    if assign_type is None:
> -        # None is returned if this nic had no 'addr_assign_type' entry.
> -        # if strict, raise an error, if not return True.
> -        if strict:
> -            raise ValueError("%s had no addr_assign_type.")
> -        return True
> -    return assign_type in (0, 1, 3)
> -
> -
> -def _get_current_rename_info(check_downable=True):
> -    """Collect information necessary for rename_interfaces.
> -
> -    returns a dictionary by mac address like:
> -       {name:
> -         {
> -          'downable': None or boolean indicating that the
> -                      device has only automatically assigned ip addrs.
> -          'device_id': Device id value (if it has one)
> -          'driver': Device driver (if it has one)
> -          'mac': mac address (in lower case)
> -          'name': name
> -          'up': boolean: is_up(name)
> -         }}
> -    """
> -    cur_info = {}
> -    for (name, mac, driver, device_id) in get_interfaces():
> -        cur_info[name] = {
> -            'downable': None,
> -            'device_id': device_id,
> -            'driver': driver,
> -            'mac': mac.lower(),
> -            'name': name,
> -            'up': is_up(name),
> -        }
> -
> -    if check_downable:
> -        nmatch = re.compile(r"[0-9]+:\s+(\w+)[@:]")
> -        ipv6, _err = util.subp(['ip', '-6', 'addr', 'show', 'permanent',
> -                                'scope', 'global'], capture=True)
> -        ipv4, _err = util.subp(['ip', '-4', 'addr', 'show'], capture=True)
> -
> -        nics_with_addresses = set()
> -        for bytes_out in (ipv6, ipv4):
> -            nics_with_addresses.update(nmatch.findall(bytes_out))
> -
> -        for d in cur_info.values():
> -            d['downable'] = (d['up'] is False or
> -                             d['name'] not in nics_with_addresses)
> -
> -    return cur_info
> -
> -
> -def _rename_interfaces(renames, strict_present=True, strict_busy=True,
> -                       current_info=None):
> -
> -    if not len(renames):
> -        LOG.debug("no interfaces to rename")
> -        return
> -
> -    if current_info is None:
> -        current_info = _get_current_rename_info()
> -
> -    cur_info = {}
> -    for name, data in current_info.items():
> -        cur = data.copy()
> -        if cur.get('mac'):
> -            cur['mac'] = cur['mac'].lower()
> -        cur['name'] = name
> -        cur_info[name] = cur
> -
> -    def update_byname(bymac):
> -        return dict((data['name'], data)
> -                    for data in cur_info.values())
> -
> -    def rename(cur, new):
> -        util.subp(["ip", "link", "set", cur, "name", new], capture=True)
> -
> -    def down(name):
> -        util.subp(["ip", "link", "set", name, "down"], capture=True)
> -
> -    def up(name):
> -        util.subp(["ip", "link", "set", name, "up"], capture=True)
> -
> -    ops = []
> -    errors = []
> -    ups = []
> -    cur_byname = update_byname(cur_info)
> -    tmpname_fmt = "cirename%d"
> -    tmpi = -1
> -
> -    def entry_match(data, mac, driver, device_id):
> -        """match if set and in data"""
> -        if mac and driver and device_id:
> -            return (data['mac'] == mac and
> -                    data['driver'] == driver and
> -                    data['device_id'] == device_id)
> -        elif mac and driver:
> -            return (data['mac'] == mac and
> -                    data['driver'] == driver)
> -        elif mac:
> -            return (data['mac'] == mac)
> -
> -        return False
> +def get_ib_hwaddrs_by_interface():
> +    """Build a dictionary mapping Infiniband interface names to their hardware
> +    address."""
> +    ret = {}
> +    from cloudinit.net import common
> +    for name, _, _, _ in common.get_interfaces():
> +        ib_mac = get_ib_interface_hwaddr(name, False)
> +        if ib_mac:
> +            if ib_mac in ret:
> +                raise RuntimeError(
> +                    "duplicate mac found! both '%s' and '%s' have mac '%s'" %
> +                    (name, ret[ib_mac], ib_mac))
> +            ret[name] = ib_mac
> +    return ret
>  
> -    def find_entry(mac, driver, device_id):
> -        match = [data for data in cur_info.values()
> -                 if entry_match(data, mac, driver, device_id)]
> -        if len(match):
> -            if len(match) > 1:
> -                msg = ('Failed to match a single device. Matched devices "%s"'
> -                       ' with search values "(mac:%s driver:%s device_id:%s)"'
> -                       % (match, mac, driver, device_id))
> -                raise ValueError(msg)
> -            return match[0]
> +def get_ib_interface_hwaddr(ifname, ethernet_format):

Need two empty lines between function definitions.

> +    """Returns the string value of an Infiniband interface's hardware address.
>  
> +    If ethernet_format is True, an Ethernet MAC-style 6 byte
> +    representation of the address will be returned.
> +    """
> +    if not is_infiniband(ifname):
>          return None
>  
> -    for mac, new_name, driver, device_id in renames:
> -        if mac:
> -            mac = mac.lower()
> -        cur_ops = []
> -        cur = find_entry(mac, driver, device_id)
> -        if not cur:
> -            if strict_present:
> -                errors.append(
> -                    "[nic not present] Cannot rename mac=%s to %s"
> -                    ", not available." % (mac, new_name))
> -            continue
> -
> -        cur_name = cur.get('name')
> -        if cur_name == new_name:
> -            # nothing to do
> -            continue
> -
> -        if not cur_name:
> -            if strict_present:
> -                errors.append(
> -                    "[nic not present] Cannot rename mac=%s to %s"
> -                    ", not available." % (mac, new_name))
> -            continue
> -
> -        if cur['up']:
> -            msg = "[busy] Error renaming mac=%s from %s to %s"
> -            if not cur['downable']:
> -                if strict_busy:
> -                    errors.append(msg % (mac, cur_name, new_name))
> -                continue
> -            cur['up'] = False
> -            cur_ops.append(("down", mac, new_name, (cur_name,)))
> -            ups.append(("up", mac, new_name, (new_name,)))
> -
> -        if new_name in cur_byname:
> -            target = cur_byname[new_name]
> -            if target['up']:
> -                msg = "[busy-target] Error renaming mac=%s from %s to %s."
> -                if not target['downable']:
> -                    if strict_busy:
> -                        errors.append(msg % (mac, cur_name, new_name))
> -                    continue
> -                else:
> -                    cur_ops.append(("down", mac, new_name, (new_name,)))
> -
> -            tmp_name = None
> -            while tmp_name is None or tmp_name in cur_byname:
> -                tmpi += 1
> -                tmp_name = tmpname_fmt % tmpi
> -
> -            cur_ops.append(("rename", mac, new_name, (new_name, tmp_name)))
> -            target['name'] = tmp_name
> -            cur_byname = update_byname(cur_info)
> -            if target['up']:
> -                ups.append(("up", mac, new_name, (tmp_name,)))
> -
> -        cur_ops.append(("rename", mac, new_name, (cur['name'], new_name)))
> -        cur['name'] = new_name
> -        cur_byname = update_byname(cur_info)
> -        ops += cur_ops
> -
> -    opmap = {'rename': rename, 'down': down, 'up': up}
> -
> -    if len(ops) + len(ups) == 0:
> -        if len(errors):
> -            LOG.debug("unable to do any work for renaming of %s", renames)
> -        else:
> -            LOG.debug("no work necessary for renaming of %s", renames)
> -    else:
> -        LOG.debug("achieving renaming of %s with ops %s", renames, ops + ups)
> -
> -        for op, mac, new_name, params in ops + ups:
> -            try:
> -                opmap.get(op)(*params)
> -            except Exception as e:
> -                errors.append(
> -                    "[unknown] Error performing %s%s for %s, %s: %s" %
> -                    (op, params, mac, new_name, e))
> -
> -    if len(errors):
> -        raise Exception('\n'.join(errors))
> +    mac = get_interface_mac(ifname)
> +    if mac and ethernet_format:
> +        # Use bytes 13-15 and 18-20 of the hardware address.
> +        mac = mac[36:-14] + mac[51:]
> +    return mac
>  
>  
>  def get_interface_mac(ifname):
> -    """Returns the string value of an interface's MAC Address"""
> -    path = "address"
> -    if os.path.isdir(sys_dev_path(ifname, "bonding_slave")):
> -        # for a bond slave, get the nic's hwaddress, not the address it
> -        # is using because its part of a bond.
> -        path = "bonding_slave/perm_hwaddr"
> -    return read_sys_net_safe(ifname, path)
> +    return netimpl.get_interface_mac(ifname)
>  
>  
> -def get_ib_interface_hwaddr(ifname, ethernet_format):
> -    """Returns the string value of an Infiniband interface's hardware
> -    address. If ethernet_format is True, an Ethernet MAC-style 6 byte
> -    representation of the address will be returned.
> -    """
> -    # Type 32 is Infiniband.
> -    if read_sys_net_safe(ifname, 'type') == '32':
> -        mac = get_interface_mac(ifname)
> -        if mac and ethernet_format:
> -            # Use bytes 13-15 and 18-20 of the hardware address.
> -            mac = mac[36:-14] + mac[51:]
> -        return mac
> +def net_setup_link(run=False):
> +    return netimpl.net_setup_link(run)
>  
>  
>  def get_interfaces_by_mac():
>      """Build a dictionary of tuples {mac: name}.
>  
> -    Bridges and any devices that have a 'stolen' mac are excluded."""
> +    Bridges and any devices that have a 'stolen' mac are excluded.
> +    """
>      ret = {}
> -    for name, mac, _driver, _devid in get_interfaces():
> +    from cloudinit.net import common
> +    for name, mac, _driver, _devid in common.get_interfaces():
>          if mac in ret:
>              raise RuntimeError(
>                  "duplicate mac found! both '%s' and '%s' have mac '%s'" %


-- 
https://code.launchpad.net/~i.galic/cloud-init/+git/cloud-init/+merge/358228
Your team cloud-init commiters is requested to review the proposed merge of ~i.galic/cloud-init:refactor/net-fbsd into cloud-init:master.