curtin-dev team mailing list archive
-
curtin-dev team
-
Mailing list archive
-
Message #03977
Re: [Merge] ~ogayot/curtin:netplan-nbft into curtin:master
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:
Thanks. I introduced a _iproute2 function.
> + 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?
* absence of iproute2
* absence of the `-j` option in iproute2
* potential network disconnect
* ...
For Subiquity, ignoring the errors here means the system will likely fail to boot (with the same symptoms as without this code).
But the reason we need this code in the first place is because the network config that Subiquity provides is not good enough for NVMe/TCP - so we try to fix it after the fact.
The reason why I decided to make it non fatal is because it's an optional step if the network configuration is already correct (I'm thinking of use-cases where curtin is not driven by Subiquity).
On the other hand, maybe moving the try: except block around the call to `dracut_adapt_netplan_config` would be more sensible? Do you have any suggestion?
> also, is the `str()` redundant given the `%s`?
yes, I think you're right :)
> +
> + 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')
Please see my answer for your comment on line 93 :)
> + 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 subscribed to branch curtin:master.
References