cloud-init-dev team mailing list archive
-
cloud-init-dev team
-
Mailing list archive
-
Message #05848
Re: [Merge] ~i.galic/cloud-init:refactor/net-fbsd into cloud-init:master
Diff comments:
> diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py
> index 3642fb1..c5e78a2 100644
> --- a/cloudinit/net/__init__.py
> +++ b/cloudinit/net/__init__.py
> @@ -14,171 +14,62 @@ from cloudinit.net.network_state import mask_to_net_prefix
> from cloudinit import util
> from cloudinit.url_helper import UrlError, readurl
>
> -LOG = logging.getLogger(__name__)
> -SYS_CLASS_NET = "/sys/class/net/"
> -DEFAULT_PRIMARY_INTERFACE = 'eth0'
> -
> -
> -def natural_sort_key(s, _nsre=re.compile('([0-9]+)')):
> - """Sorting for Humans: natural sort order. Can be use as the key to sort
> - functions.
> - This will sort ['eth0', 'ens3', 'ens10', 'ens12', 'ens8', 'ens0'] as
> - ['ens0', 'ens3', 'ens8', 'ens10', 'ens12', 'eth0'] instead of the simple
> - python way which will produce ['ens0', 'ens10', 'ens12', 'ens3', 'ens8',
> - 'eth0']."""
> - return [int(text) if text.isdigit() else text.lower()
> - for text in re.split(_nsre, s)]
> -
> -
> -def get_sys_class_path():
> - """Simple function to return the global SYS_CLASS_NET."""
> - return SYS_CLASS_NET
> -
> -
> -def sys_dev_path(devname, path=""):
> - return get_sys_class_path() + devname + "/" + path
> -
> -
> -def read_sys_net(devname, path, translate=None,
> - on_enoent=None, on_keyerror=None,
> - on_einval=None):
> - dev_path = sys_dev_path(devname, path)
> - try:
> - contents = util.load_file(dev_path)
> - except (OSError, IOError) as e:
> - e_errno = getattr(e, 'errno', None)
> - if e_errno in (errno.ENOENT, errno.ENOTDIR):
> - if on_enoent is not None:
> - return on_enoent(e)
> - if e_errno in (errno.EINVAL,):
> - if on_einval is not None:
> - return on_einval(e)
> - raise
> - contents = contents.strip()
> - if translate is None:
> - return contents
> - try:
> - return translate[contents]
> - except KeyError as e:
> - if on_keyerror is not None:
> - return on_keyerror(e)
> - else:
> - LOG.debug("Found unexpected (not translatable) value"
> - " '%s' in '%s", contents, dev_path)
> - raise
> -
> -
> -def read_sys_net_safe(iface, field, translate=None):
> - def on_excp_false(e):
> - return False
> - return read_sys_net(iface, field,
> - on_keyerror=on_excp_false,
> - on_enoent=on_excp_false,
> - on_einval=on_excp_false,
> - translate=translate)
> +if util.is_FreeBSD():
> + from cloudinit.net import freebsd as netimpl
> +else:
> + from cloudinit.net import linux as netimpl
>
>
> -def read_sys_net_int(iface, field):
> - val = read_sys_net_safe(iface, field)
> - if val is False:
> - return None
> - try:
> - return int(val)
> - except ValueError:
> - return None
> +LOG = logging.getLogger(__name__)
> +LO_DEVS = ['lo', 'lo0']
>
>
> def is_up(devname):
> - # The linux kernel says to consider devices in 'unknown'
> - # operstate as up for the purposes of network configuration. See
> - # Documentation/networking/operstates.txt in the kernel source.
> - translate = {'up': True, 'unknown': True, 'down': False}
> - return read_sys_net_safe(devname, "operstate", translate=translate)
> + return netimpl.is_up(devname)
>
>
> def is_wireless(devname):
> - return os.path.exists(sys_dev_path(devname, "wireless"))
> + return netimpl.is_wireless(devname)
>
>
> def is_bridge(devname):
> - return os.path.exists(sys_dev_path(devname, "bridge"))
> + return netimpl.is_wireless(devname)
>
>
> def is_bond(devname):
> - return os.path.exists(sys_dev_path(devname, "bonding"))
> + return netimpl.is_bond(devname)
>
>
> def is_renamed(devname):
> - """
In your rework, you've pulled a lot of function comments/docstrings directly into cloudinit/net/linux. Generally what I'd like to see in cloud-init is a top-level docstring per function describing it's intent, even if it is only a one-liner
"""Return True if a network device has been renamed."""
> - /* interface name assignment types (sysfs name_assign_type attribute) */
> - #define NET_NAME_UNKNOWN 0 /* unknown origin (not exposed to user) */
> - #define NET_NAME_ENUM 1 /* enumerated by kernel */
> - #define NET_NAME_PREDICTABLE 2 /* predictably named by the kernel */
> - #define NET_NAME_USER 3 /* provided by user-space */
> - #define NET_NAME_RENAMED 4 /* renamed by user-space */
> - """
> - name_assign_type = read_sys_net_safe(devname, 'name_assign_type')
> - if name_assign_type and name_assign_type in ['3', '4']:
> - return True
> - return False
> + return netimpl.is_renamed(devname)
>
>
> def is_vlan(devname):
> - uevent = str(read_sys_net_safe(devname, "uevent"))
> - return 'DEVTYPE=vlan' in uevent.splitlines()
> + return netimpl.is_renamed(devname)
Typo, is_vlan instead of is_renamed.
>
>
> def is_connected(devname):
> - # is_connected isn't really as simple as that. 2 is
> - # 'physically connected'. 3 is 'not connected'. but a wlan interface will
> - # always show 3.
> - iflink = read_sys_net_safe(devname, "iflink")
> - if iflink == "2":
> - return True
> - if not is_wireless(devname):
> - return False
> - LOG.debug("'%s' is wireless, basing 'connected' on carrier", devname)
> - return read_sys_net_safe(devname, "carrier",
> - translate={'0': False, '1': True})
> + return netimpl.is_connected(devname)
>
>
> def is_physical(devname):
> - return os.path.exists(sys_dev_path(devname, "device"))
> + return netimpl.is_physical(devname)
>
>
> def is_present(devname):
> - return os.path.exists(sys_dev_path(devname))
> + return netimpl.is_present(devname)
>
>
> def device_driver(devname):
> - """Return the device driver for net device named 'devname'."""
Here is an example of a simple docstring I'd like to see left in the top-level module if we can.
> - driver = None
> - driver_path = sys_dev_path(devname, "device/driver")
> - # driver is a symlink to the driver *dir*
> - if os.path.islink(driver_path):
> - driver = os.path.basename(os.readlink(driver_path))
> -
> - return driver
> + return netimpl.device_driver(devname)
>
>
> def device_devid(devname):
> - """Return the device id string for net device named 'devname'."""
> - dev_id = read_sys_net_safe(devname, "device/device")
> - if dev_id is False:
> - return None
> -
> - return dev_id
> + return netimpl.device_devid(devname)
>
>
> def get_devicelist():
> - try:
> - devs = os.listdir(get_sys_class_path())
> - except OSError as e:
> - if e.errno == errno.ENOENT:
> - devs = []
> - else:
> - raise
> - return devs
> + return netimpl.get_devicelist()
>
>
> class ParserError(Exception):
> diff --git a/cloudinit/net/common.py b/cloudinit/net/common.py
> new file mode 100644
> index 0000000..579739c
> --- /dev/null
> +++ b/cloudinit/net/common.py
> @@ -0,0 +1,40 @@
> +# Copyright (C) 2018 Canonical Ltd.
> +#
> +# This file is part of cloud-init. See LICENSE file for license information.
> +"""
> +This module is used for all high-level functions common to linux & freebsd,
> +that are also used in linux & freebsd!
> +They cannot be put them into net/__init__ because otherwise we'd end up with
> +cyclic imports.
> +"""
> +
> +from . import LO_DEVS, get_devicelist, interface_has_own_mac, is_bridge, is_vlan, get_interface_mac, device_driver, device_devid
> +
> +
> +def get_interfaces():
In this case I may be okay with the separate module given that we now have decoupled distro-speicifc modules. There are a couple ways we can avoid this circular import. I'll look a bit more into this tomorrow.
> + """Return list of interface tuples (name, mac, driver, device_id)
> +
> + Bridges and any devices that have a 'stolen' mac are excluded."""
> + ret = []
> + devs = get_devicelist()
> + # 16 somewhat arbitrarily chosen. Normally a mac is 6 '00:' tokens.
> + zero_mac = ':'.join(('00', ) * 16)
> + for name in devs:
> + if not interface_has_own_mac(name):
> + continue
> + if is_bridge(name):
> + continue
> + if is_vlan(name):
> + continue
> + mac = get_interface_mac(name)
> + # some devices may not have a mac (tun0)
> + if not mac:
> + continue
> + # skip nics that have no mac (00:00....)
> + if name not in LO_DEVS and mac == zero_mac[:len(mac)]:
> + continue
> + ret.append((name, mac, device_driver(name), device_devid(name)))
> + return ret
> +
> +
> +# vi: ts=4 expandtab
> diff --git a/cloudinit/net/freebsd.py b/cloudinit/net/freebsd.py
> new file mode 100644
> index 0000000..4f2179b
> --- /dev/null
> +++ b/cloudinit/net/freebsd.py
> @@ -0,0 +1,268 @@
> +# Copyright (C) 2018 Canonical Ltd.
Feel free to ditch to top two lines. For new files the follow header comment will suffice
# This file is part of cloud-init. See LICENSE file for license information.
> +#
> +# This file is part of cloud-init. See LICENSE file for license information.
> +
Add a docstring to this module and cloudinit.net.linux something like
# FreeBSD-specific network utility functions
> +import errno
> +import logging
> +import re
> +
> +from cloudinit.net.network_state import mask_to_net_prefix
> +from cloudinit import util
> +
> +LOG = logging.getLogger(__name__)
> +DEFAULT_PRIMARY_INTERFACE = 'vtnet0'
> +
> +
> +def _ifconfig(*args):
> + return util.subp(['ifconfig', *args])
> +
> +
> +def _sysctl(*args):
> + return util.subp(['sysctl', *args])
> +
> +
> +def _pciconf(*args):
> + return util.subp(['pciconf', *args])
> +
> +
> +def _route(*args):
> + return util.subp(['route', *args])
> +
> +
> +def _parse_devname(devname):
> + dev = re.match("^(?P<driver>[a-z]+)(?P<devid>[0-9]+)$", devname)
> + if dev is None:
> + return None
> + return (
> + dev.group("driver"),
> + dev.group("devid"),
> + )
> +
> +
> +def is_up(devname):
> + output = _ifconfig(['-u', devname])
Since util.subp returns a tuple, prevailing style in cloud-init suggests we do something like the following so our future selves understand the intent of the elements returned.
out, err = _ifconfig(['-u', devname])
if out is '' or err is not '':
return False.
Same comment throughout for _sysctl returns and _pcicconf
Alternatively, function calls cost more than inline code at the callsites. Since the function calss _ifconfig, _sysctl, _pciconf and _route are only short one-liners, maybe is makes sense to just drop those functions as they are only thin wrappers that further obfuscate what is being called.
Then is_up (and it's ilk) could be:
def is_up(devname):
out, err = util.subp(['ifconfig', '-u', devname])
if out == '' or err != '':
return False
> + if output[0] is '' or output[1] is not '':
> + return False
> +
> + return devname in output[0].split("\n")[0]
> +
> +
> +def is_wireless(devname):
> + output = _sysctl('-n', "net.wlan.devices")
> + if output[0] is '' or output[1] is not '':
> + return False
> +
> + wlan_devs = output[0].strip().split(" ")
> + return devname in wlan_devs
> +
> +
> +def is_bridge(devname):
> + output = _ifconfig(['-g', 'bridge'])
> + if output[0] is '' or output[1] is not '':
> + return False
> +
> + bridges = output[0].strip().split("\n")
> + return devname in bridges
> +
> +
> +def is_bond(devname):
> + output = _ifconfig(['-g', 'lagg'])
> + if output[0] is '' or output[1] is not '':
> + return False
> +
> + bonds = output[0].strip().split("\n")
> + return devname in bonds
> +
> +
> +def is_vlan(devname):
> + output = _ifconfig(['-g', 'vlan'])
> + if output[0] is '' or output[1] is not '':
> + return False
> +
> + vlans = output[0].strip().split("\n")
> + return devname in vlans
> +
> +
> +def is_renamed(devname):
> + # we can only tell if a *physical* device was renamed, so
> + if not is_physical(devname):
> + return False
> +
> + dev = _parse_devname(devname)
> + renamed = _syctl(['-n', "dev.", dev[0] + "." + dev[1] + ".%driver"])
> + if renamed[0] is '' or renamed[1] is not '':
Still looking into the duplication for my next review pass.
> + return False
> +
> + return dev[0] == renamed[0]
> +
> +
> +def is_connected(devname):
> + output = _ifconfig([devname])
> + if output[0] is '' or output[1] is not '':
> + return False
> +
> + ifcfg = output[0].split("\n")
> + status_re = re.compile("^\s+status: active$")
> + for l in ifcfg:
> + if status_re.match(l):
> + return True
> + return False
> +
> +
> +def is_physical(devname):
> + # reject interfaces like: vnet0:3
> + if ":" in devname:
> + return False
> +
> + dev = _parse_devname(devname)
> + if dev is None:
> + return False
> +
> + output = _sysctl("dev." + dev[0] + "." + dev[1])
> + if output[0] is '' or output[1] is not '':
> + return False
> +
> + return True
> +
> +
> +def is_present(devname):
> + return devname in get_devicelist()
> +
> +
> +def device_driver(devname):
> + dev = _parse_devname(devname)
> + driver = _syctl(['-n', "dev.", dev[0] + "." + dev[1] + ".%driver"])
> + if driver[0] is '' or output[1] is not '':
> + return None
> +
> + return driver[0]
> +
> +
> +def device_devid(devname):
> + if not is_physical(devname):
> + return None
> +
> + dev = _parse_devname(devname)
> + output = _sysctl(['-n', "dev." + dev[0] + "." + dev[1] + ".%parent"])
> + if output[0] is '' or output[1] is not '':
> + return None
> +
> + # well, isn't that clear? rather than parsing `pciconf -l` output, we just
> + # *read* (-r) a *halfword* (-h) 44 bytes away from the %parent
> + devid = _pciconf(['-rh', output[0], '44'])
> + if devid[0] is '' or devid[1] is not '':
> + return None
> + return "0x" + devid[0]
> +
> +
> +def get_devicelist():
> + maybe_devs = _ifconfig(['-l']).strip().split(' ')
> + if maybe_devs == ['']:
> + return []
> + return maybe_devs
> +
> +
> +def find_fallback_nic(blacklist_drivers=None):
> + output = _route(["-n", "show", "default"])
> + if output[0] is '' or output[1] is not '':
> + return None
> +
> + route = output[0].split("\n")
> + via_re = re.compile("^\s+interface: (?P<netif>.+)$")
> + for l in route:
> + maybe_netif = via_re.match(l)
> + if maybe_netif is None:
> + continue
> +
> + driver = device_driver(maybe_netif)
> + if driver not in blacklist_drivers:
> + return maybe_netif.group("netif")
> +
> + return None
> +
> +
> +def interface_has_own_mac(ifname, strict=False):
> + # breakdown:
> + # physical interfaces have their own MAC
> + # bridges have their own MAC, unless net.link.bridge.inherit_mac is non-zero
> + # vlan interfaces inherit MACs
> + # bond interfaces have ??
> + # tunnel interfaces have ??
> + # virtual interfaces *usually* have their own, unless set not to
> + if is_physical(ifname):
> + return True
> +
> + if is_bridge(ifname):
> + output = _sysctl(['-n', 'net.link.bridge.inherit_mac'])
> + inherit_mac = output[0].strip()
> + if inherit_mac == '0':
> + return False
> + return True
> + # for now:
> + return False
> +
> +
> +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)
> + }}
> + """
> + raise NotImplemented("TODO")
> +
> +
> +def _rename_interfaces(renames,
> + strict_present=True,
> + strict_busy=True,
> + current_info=None):
> +
> + raise NotImplemented("TODO")
> +
> +
> +def get_interface_mac(ifname):
> + """Returns the string value of an interface's MAC Address"""
> + output = _ifconfig([ifname, 'ether'])
> + mac_zero = '00:00:00:00:00:00'
> + if output[0] is '' or output[1] is not '':
> + return mac_zero
> +
> + mac_re = re.compile("\s+ether (?P<mac>([a-f0-9]{2}:){5}[a-f0-9]{2})", re.I)
> + ifcfg = output[0].split("\n")
> + for l in ifcfg:
> + maybe_mac = mac_re.match(l)
> + if maybe_mac is not None:
> + return maybe_mac.group('mac')
> +
> + return mac_zero
> +
> +
> +def get_ib_interface_hwaddr(ifname, ethernet_format):
> + """Since this isn't properly implemented yet, we're returning None for now
> + rather than raise NotImplemented("TODO")
> + That way, we can move get_interfaces_by_mac() into __init__"""
> + return None
Heh, IB is an Infiniband networking interface.
> +
> +
> +
> +class EphemeralIPv4Network(object):
> + """Context manager which sets up temporary static network configuration.
> +
> + No operations are performed if the provided interface is already connected.
> + If unconnected, bring up the interface with valid ip, prefix and broadcast.
> + If router is provided setup a default route for that interface. Upon
> + context exit, clean up the interface leaving no configuration behind.
> + """
> +
> + raise NotImplemented("TODO")
> +
> +
> +# vi: ts=4 expandtab
--
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.
References
