curtin-dev team mailing list archive

[Dan Bungert <mp+475635@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/curtin/nvme_tcp.py b/curtin/nvme_tcp.py
> index 456bbb4..b030ac3 100644
> --- a/curtin/nvme_tcp.py
> +++ b/curtin/nvme_tcp.py
> @@ -315,3 +322,79 @@ modprobe nvme-tcp
>          print(script_header, file=fh)
>          for cmd in get_ip_commands(cfg):
>              print(shlex.join(cmd), file=fh)
> +
> +
> +class NetRuntimeError(RuntimeError):
> +    pass
> +
> +
> +def get_route_dest_ifname(dest: str) -> str:

there is room for a small utility function here, as this function and the next have the same structure, and it looks like it has good odds of being reused.  It may help the tests be a bit nicer as well.

> +    try:
> +        out, _ = util.subp(['ip', '-j', 'route', 'get', dest], capture=True)
> +        return json.loads(out)[0]['dev']
> +    except (util.ProcessExecutionError, IndexError, KeyError) as exc:
> +        raise NetRuntimeError(f'could not determine route to {dest}') from exc
> +
> +
> +def get_iface_hw_addr(ifname: str) -> str:
> +    try:
> +        out, _ = util.subp(['ip', '-j', 'link', 'show', 'dev', ifname],
> +                           capture=True)
> +        return json.loads(out)[0]['address']
> +    except (util.ProcessExecutionError, IndexError, KeyError) as exc:
> +        raise NetRuntimeError(f'could not retrieve MAC for {ifname}') from exc
> +
> +
> +def dracut_adapt_netplan_config(cfg, *, target: pathlib.Path):
> +    '''Modify the netplan configuration (which has already been written to
> +    disk at this point) so that:
> +    * critical network interfaces (those handled by dracut) are not brought
> +    down during boot.
> +    * netplan does not panic if such an interface gets renamed.
> +    '''
> +    ifnames: Set[str] = set()
> +    modified = False
> +
> +    for controller in _iter_nvme_tcp_controllers(cfg):
> +        try:
> +            ifnames.add(get_route_dest_ifname(controller['tcp_addr']))
> +        except NetRuntimeError as exc:
> +            LOG.debug('%s, ignoring', str(exc))

what sort of errors are we ignoring here?  also, is the `str()` redundant given the `%s`?

> +
> +    try:
> +        netplan_conf_path = pathlib.Path(
> +                target_path(
> +                    str(target),
> +                    cfg['write_files']['etc_netplan_installer']['path']))
> +    except KeyError:
> +        LOG.debug('could not find netplan configuration passed to cloud-init')
> +        return
> +
> +    config = yaml.safe_load(netplan_conf_path.read_text())
> +
> +    try:
> +        ethernets = config['network']['ethernets']
> +    except KeyError:
> +        LOG.debug('no ethernet interface in netplan configuration')

these next two sound fatal, would you elaborate on why we can safely return with just a debug log?

> +        return
> +
> +    macaddresses: Dict[str, str] = {}
> +
> +    for ifname in ifnames:
> +        try:
> +            macaddresses[ifname] = get_iface_hw_addr(ifname)
> +        except NetRuntimeError as exc:
> +            LOG.debug('%s, ignoring', str(exc))
> +
> +    for ifname, ifconfig in ethernets.items():
> +        if ifname not in ifnames:
> +            continue
> +        # Ensure the interface is not brought down
> +        ifconfig['critical'] = True
> +        modified = True
> +        # Ensure we match the HW address and not the ifname.
> +        if 'match' not in ifconfig:
> +            ifconfig['match'] = {'macaddress': macaddresses[ifname]}
> +
> +    if modified:
> +        netplan_conf_path.write_text(yaml.dump(config))


-- 
https://code.launchpad.net/~ogayot/curtin/+git/curtin/+merge/475635
Your team curtin developers is requested to review the proposed merge of ~ogayot/curtin:netplan-nbft into curtin:master.