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.


-- 
Mailing list: https://launchpad.net/~curtin-dev
Post to     : curtin-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~curtin-dev
More help   : https://help.launchpad.net/ListHelp

Reply via email to