cloud-init-dev team mailing list archive

[Chad Smith <chad.smith@xxxxxxxxxxxxx>]

Thanks for this branch James, I've got a first round of comments here and have a minor fixup needed for running in an lxc that I'll post tomorrrow when I get to the bottom of what's happening.


but the following breaks on lxcs. The way routes are listed breaks your existing logic a bit:



it seems to fallover parsing the second line (probably because the route has an alias name @if64  in it and doesn't match the original key.

It might be as simple as truncating any aliases in the interface name after @.....
 

 ip -o link list
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1\    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
63: eth0@if64: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000\    link/ether 00:16:3e:ee:c3:6b brd ff:ff:ff:ff:ff:ff link-netnsid 0




make deb
lxc launch ubuntu-daily:xenial x1
lxc file push cloud-init_17.2.*deb x1/cloud-init.deb
lxc exec x1 dpkg -i /cloud-init.deb 



Diff comments:

> diff --git a/cloudinit/netinfo.py b/cloudinit/netinfo.py
> index 993b26c..3743b91 100644
> --- a/cloudinit/netinfo.py
> +++ b/cloudinit/netinfo.py
> @@ -18,18 +20,72 @@ from cloudinit.simpletable import SimpleTable
>  LOG = logging.getLogger()
>  
>  
> -def netdev_info(empty=""):
> -    fields = ("hwaddr", "addr", "bcast", "mask")
> -    (ifcfg_out, _err) = util.subp(["ifconfig", "-a"], rcs=[0, 1])
> +def netdev_cidr_to_mask(cidr):
> +    mask = socket.inet_ntoa(
> +        struct.pack(">I", (0xffffffff << (32 - int(cidr)) & 0xffffffff)))
> +    return mask
> +
> +
> +def netdev_info_iproute(ipaddr_data, iplink_data):
> +    # fields that need to be returned in devs for each dev

Can you please add a docsting to functions you touch in this module to express parameter and returns of the function.

"""Get network device dicts from ip route and ip link info.

@param ipaddr_data: Output string from ip address command.
@param iplink_data: Output string from ip link command.

@returns: A dict of device info keyed by network device name containing device configuration values.
"""

> +    fields = ("hwaddr", "up", "addr", "bcast", "mask", "scope")
> +    devs = {}
> +    for line in str(ipaddr_data).splitlines():
> +        details = line.lower().strip().split()
> +        curdev = details[1]
> +        fieldpost = ""
> +        if curdev not in devs:
> +            devs[curdev] = {"up": False}

To avoid that pruning for loop at the end of this function and the other netdev_info_ifconfig, just set up the default device info dict with empty values like this pastebin: http://paste.ubuntu.com/26211530/

> +        if details[2] == "inet6":

I'm not understanding why we separate out the fieldpost calculation and then recalculate whether we are ipv4 ipv6 again inside the loop with that if fieldpost == "" check. We could just do all the work right there.


for i in range(len(defails)):
   if details[i] == 'inet':
       (addr, cidr) = details[i + 1].split("/")
       devs[curdev]["mask"] = netdev_cidr_to_mask(cidr)
       devs[curdev]["addr"] = addr
   elif details[i] == 'inet6':
       addr = details[i + 1]
       devs[curdev]["addr6"] = addr
   elif details[i] == 'siope':
     ...

> +            fieldpost = "6"
> +        for i in range(len(details)):
> +            if details[i] == "inet" or details[i] == "inet6":
> +                addr = ""
> +                if fieldpost == "":
> +                    (addr, cidr) = details[i + 1].split("/")
> +                    devs[curdev]["mask"] = netdev_cidr_to_mask(cidr)
> +                else:
> +                    addr = details[i + 1]
> +                devs[curdev]["addr" + fieldpost] = addr
> +            if details[i] == "scope":

the scope and brd check should be elif's right?

> +                devs[curdev]["scope" + fieldpost] = details[i + 1]
> +            if details[i] == "brd":
> +                devs[curdev]["bcast" + fieldpost] = details[i + 1]
> +
> +    for line in str(iplink_data).splitlines():
> +        details = line.lower().strip().split()
> +        curdev = details[1][:-1]

Can we change this to details[1].rstrip(':') to be explicit about what we think we are doing? I don't like the -1's on lists as we don't really know what we gobbled up

> +        for i in range(len(details)):
> +            if details[i] == curdev + ":":
> +                flags = details[i + 1].strip("<>").split(",")
> +                if "lower_up" in flags and "up" in flags:
> +                    devs[curdev]["up"] = True
> +            if details[i] == "link/ether":

Should be elif right?

> +                devs[curdev]["hwaddr"] = details[i + 1]
> +
> +    # Run through all devs and ensure all fields are defined
> +    for dev in devs:

Can drop this if using DEFAULT_NETDEV_INFO

> +        for field in fields:
> +            if field not in devs[dev]:
> +                devs[dev][field] = ""
> +
> +    return devs
> +
> +
> +def netdev_info_ifconfig(ifconfig_data):
> +    # fields that need to be returned in devs for each dev
> +    fields = ("hwaddr", "up", "addr", "bcast", "mask", "scope")
>      devs = {}
> -    for line in str(ifcfg_out).splitlines():
> +    for line in str(ifconfig_data).splitlines():
>          if len(line) == 0:
>              continue
>          if line[0] not in ("\t", " "):
>              curdev = line.split()[0]
> -            devs[curdev] = {"up": False}
> -            for field in fields:
> -                devs[curdev][field] = ""
> +            # current ifconfig pops a ':' on the end of the device
> +            if curdev.endswith(':'):
> +                curdev = curdev[:-1]
> +            if curdev not in devs:
> +                devs[curdev] = {"up": False}
>          toks = line.lower().strip().split()
>          if toks[0] == "up":
>              devs[curdev]['up'] = True
> @@ -39,41 +95,57 @@ def netdev_info(empty=""):
>              if re.search(r"flags=\d+<up,", toks[1]):
>                  devs[curdev]['up'] = True
>  
> -        fieldpost = ""
> -        if toks[0] == "inet6":
> -            fieldpost = "6"
> -
>          for i in range(len(toks)):
> -            # older net-tools (ubuntu) show 'inet addr:xx.yy',
> -            # newer (freebsd and fedora) show 'inet xx.yy'
> -            # just skip this 'inet' entry. (LP: #1285185)
> -            try:
> -                if ((toks[i] in ("inet", "inet6") and
> -                     toks[i + 1].startswith("addr:"))):
> -                    continue
> -            except IndexError:
> -                pass
> -
> -            # Couple the different items we're interested in with the correct
> -            # field since FreeBSD/CentOS/Fedora differ in the output.
> -            ifconfigfields = {
> -                "addr:": "addr", "inet": "addr",
> -                "bcast:": "bcast", "broadcast": "bcast",
> -                "mask:": "mask", "netmask": "mask",
> -                "hwaddr": "hwaddr", "ether": "hwaddr",
> -                "scope": "scope",
> -            }
> -            for origfield, field in ifconfigfields.items():
> -                target = "%s%s" % (field, fieldpost)
> -                if devs[curdev].get(target, ""):
> -                    continue
> -                if toks[i] == "%s" % origfield:
> -                    try:
> -                        devs[curdev][target] = toks[i + 1]
> -                    except IndexError:
> -                        pass
> -                elif toks[i].startswith("%s" % origfield):
> -                    devs[curdev][target] = toks[i][len(field) + 1:]
> +            if toks[i] == "inet":
> +                if toks[i + 1].startswith("addr:"):
> +                    devs[curdev]['addr'] = toks[i + 1].lstrip("addr:")
> +                else:
> +                    devs[curdev]['addr'] = toks[i + 1]
> +            elif toks[i].startswith("bcast:"):
> +                devs[curdev]['bcast'] = toks[i].lstrip("bcast:")
> +            elif toks[i] == "broadcast":
> +                devs[curdev]['bcast'] = toks[i + 1]
> +            elif toks[i].startswith("mask:"):
> +                devs[curdev]['mask'] = toks[i].lstrip("mask:")
> +            elif toks[i] == "netmask":
> +                devs[curdev]['mask'] = toks[i + 1]
> +            elif toks[i] == "hwaddr" or toks[i] == "ether":
> +                devs[curdev]['hwaddr'] = toks[i + 1]
> +            elif toks[i] == "inet6":
> +                if toks[i + 1] == "addr:":
> +                    devs[curdev]['addr6'] = toks[i + 2]
> +                else:
> +                    devs[curdev]['addr6'] = toks[i + 1]
> +            elif toks[i] == "prefixlen":
> +                addr6 = devs[curdev]['addr6'] + "/" + toks[i + 1]
> +                devs[curdev]['addr6'] = addr6
> +            elif toks[i].startswith("scope:"):
> +                devs[curdev]['scope6'] = toks[i].lstrip("scope:")
> +            elif toks[i] == "scopeid":
> +                res = re.match(".*<(\S+)>", toks[i + 1])
> +                if res:
> +                    devs[curdev]['scope6'] = res.group(1)
> +
> +    # Run through all devs and ensure all fields are defined
> +    for dev in devs:

Can drop this if using DEFAULT_NETDEV_INFO

> +        for field in fields:
> +            if field not in devs[dev]:
> +                devs[dev][field] = ""
> +
> +    return devs
> +
> +
> +def netdev_info(empty=""):
> +    devs = {}
> +    try:
> +        # Try iproute first of all
> +        (ipaddr_out, _err) = util.subp(["ip", "-o", "addr", "list"])
> +        (iplink_out, _err) = util.subp(["ip", "-o", "link", "list"])
> +        devs = netdev_info_iproute(ipaddr_out, iplink_out)
> +    except util.ProcessExecutionError:

Instead of enclosing this in a generic try/except on ProcessExecutionError, let's "look before we leap" and check to see if 'ip' exists:

if util.which('ip'):
    (ipaddr_out, _err) = util.subp(["ip", "-oneline", "addr", "list"])
    (iplink_out, _err) = util.subp(["ip", "-oneline", "link", "list"])
    devs = netdev_info_iproute(ipaddr_out, iplink_out)
else:
    # Fall back to net-tools if iproute2 is not present
    (ifcfg_out, _err) = util.subp(["ifconfig", "-a"], rcs=[0, 1])
    devs = netdev_info_ifconfig(ifcfg_out)

> +        # Fall back to net-tools if iproute2 is not present
> +        (ifcfg_out, _err) = util.subp(["ifconfig", "-a"], rcs=[0, 1])
> +        devs = netdev_info_ifconfig(ifcfg_out)
>  
>      if empty != "":
>          for (_devname, dev) in devs.items():
> @@ -84,14 +156,73 @@ def netdev_info(empty=""):
>      return devs
>  
>  
> -def route_info():
> -    (route_out, _err) = util.subp(["netstat", "-rn"], rcs=[0, 1])
> +def netdev_route_info_iproute(iproute_data):
> +    routes = {}

Your freindly neighborhood docstring police. Please document this function as mentioned above. Thanks again.

> +    routes['ipv4'] = []
> +    routes['ipv6'] = []
> +    entries = iproute_data.splitlines()
> +    for line in entries:
> +        entry = {}
> +        if not line:
> +            continue
> +        toks = line.split()
> +        if toks[0] == "default":
> +            entry['destination'] = "0.0.0.0"
> +            entry['genmask'] = "0.0.0.0"
> +            entry['flags'] = "UG"
> +        else:
> +            (addr, cidr) = toks[0].split("/")
> +            entry['destination'] = addr
> +            entry['genmask'] = netdev_cidr_to_mask(cidr)
> +            entry['gateway'] = "0.0.0.0"
> +            entry['flags'] = "U"
> +        for i in range(len(toks)):
> +            if toks[i] == "via":
> +                entry['gateway'] = toks[i + 1]
> +            if toks[i] == "dev":
> +                entry["iface"] = toks[i + 1]
> +            if toks[i] == "metric":
> +                entry['metric'] = toks[i + 1]
> +        routes['ipv4'].append(entry)
> +    try:
> +        (iproute_data6, _err6) = util.subp(["ip", "-o", "-6", "route", "list"],
> +                                           rcs=[0, 1])
> +    except util.ProcessExecutionError:
> +        pass

Hrm not sure when we'd get this data on systems that already provide us ipv4 route data from 'ip route list'. Maybe it's worth logging a LOG.debug message about our inability to see ipv6 routes.

> +    else:
> +        entries6 = iproute_data6.splitlines()
> +        for line in entries6:
> +            entry = {}
> +            if not line:
> +                continue
> +            toks = line.split()
> +            if toks[0] == "default":
> +                entry['destination'] = "::/0"
> +                entry['flags'] = "UG"
> +            else:
> +                entry['destination'] = toks[0]
> +                entry['gateway'] = "::"
> +                entry['flags'] = "U"
> +            for i in range(len(toks)):
> +                if toks[i] == "via":
> +                    entry['gateway'] = toks[i + 1]
> +                    entry['flags'] = "UG"
> +                if toks[i] == "dev":
> +                    entry["iface"] = toks[i + 1]
> +                if toks[i] == "metric":
> +                    entry['metric'] = toks[i + 1]
> +                if toks[i] == "expires":
> +                    entry['flags'] = entry['flags'] + 'e'
> +            routes['ipv6'].append(entry)
> +    return routes
> +
>  
> +def netdev_route_info_netstat(route_data):
>      routes = {}
>      routes['ipv4'] = []
>      routes['ipv6'] = []
>  
> -    entries = route_out.splitlines()[1:]
> +    entries = route_data.splitlines()
>      for line in entries:
>          if not line:
>              continue


-- 
https://code.launchpad.net/~james-hogarth/cloud-init/+git/cloud-init/+merge/333657
Your team cloud-init commiters is requested to review the proposed merge of ~james-hogarth/cloud-init:net-tools-deprecation into cloud-init:master.