curtin-dev team mailing list archive

[Dan Bungert <mp+461452@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
> index 695ba30..132ccab 100644
> --- a/curtin/commands/curthooks.py
> +++ b/curtin/commands/curthooks.py
> @@ -1528,21 +1531,130 @@ def get_nvme_stas_controller_directives(cfg) -> Set[str]:
>      return directives
>  
>  
> -def configure_nvme_stas(cfg, target):
> +def nvmeotcp_get_nvme_commands(cfg) -> Set[Tuple[str]]:
> +    """Parse the storage configuration and return a set of commands
> +    to run to bring up the NVMe over TCP block devices."""
> +    commands: Set[Tuple[str]] = set()

Commands as a set is interesting.  Looking at the commands I can't imagine order mattering, but I wonder about it just the same.  If the set order changes then the commands would be run in a different order, and I wonder if that would break an install in a very hard to diagnose way.

What's your thoughts on dumping this to a list with a `sort()` so that the arbitrary nature of `set()` ordering is never a factor?  This would introduce consistency at least.

> +    if 'storage' not in cfg or not isinstance(cfg['storage'], dict):
> +        return commands
> +    storage = cfg['storage']
> +    if 'config' not in storage or storage['config'] == 'disabled':
> +        return commands
> +    config = storage['config']
> +    for item in config:
> +        if item['type'] != 'nvme_controller':
> +            continue
> +        if item['transport'] != 'tcp':
> +            continue
> +
> +        commands.add((
> +            'nvme', 'connect-all',
> +            '--transport', 'tcp',
> +            '--traddr', item['tcp_addr'],
> +            '--trsvcid', str(item['tcp_port']),
> +        ))
> +
> +    return commands
> +
> +
> +def nvmeotcp_need_network_in_initramfs(cfg) -> bool:
> +    """Parse the storage configuration and check if any of the mountpoints
> +    essential for booting requires network."""
> +    if 'storage' not in cfg or not isinstance(cfg['storage'], dict):
> +        return False
> +    storage = cfg['storage']
> +    if 'config' not in storage or storage['config'] == 'disabled':
> +        return False
> +    config = storage['config']
> +    for item in config:
> +        if item['type'] != 'mount':
> +            continue
> +        if not '_netdev' in item.get('options', '').split(','):
> +            continue
> +
> +        # We found a mountpoint that requires network. Let's check if it is
> +        # essential for booting.
> +        path = item['path']
> +        if path == '/' or path.startswith('/usr') or path.startswith('/var'):
> +            return True
> +
> +    return False
> +
> +
> +def nvmeotcp_get_ip_commands(cfg) -> List[Tuple[str]]:
> +    """Look for the netplan configuration (supplied by subiquity using
> +    write_files directives) and attempt to extrapolate a set of 'ip' + 'dhcpcd'
> +    commands that would produce more or less the expected network
> +    configuration. At the moment, only trivial network configurations are
> +    supported, which are ethernet interfaces with or without DHCP and optional
> +    static routes."""
> +    commands: List[Tuple[str]] = []
> +
> +    try:
> +        content = cfg['write_files']['etc_netplan_installer']['content']
> +    except KeyError:
> +        return []
> +
> +    config = yaml.safe_load(content)
> +
> +    try:
> +        ethernets = config['network']['ethernets']
> +    except KeyError:
> +        return []
> +
> +    for ifname, ifconfig in ethernets.items():
> +        # Handle static IP addresses
> +        for address in ifconfig.get('addresses', []):
> +            commands.append(('ip', 'address', 'add', address, 'dev', ifname))
> +
> +        # Handle DHCPv4 and DHCPv6
> +        dhcp4 = ifconfig.get('dhcp4', False)
> +        dhcp6 = ifconfig.get('dhcp6', False)
> +        if dhcp4 and dhcp6:
> +            commands.append(('dhcpcd', ifname))
> +        elif dhcp4:
> +            commands.append(('dhcpcd', '-4', ifname))
> +        elif dhcp6:
> +            commands.append(('dhcpcd', '-6', ifname))
> +        else:
> +            commands.append(('ip', 'link', 'set', ifname, 'up'))
> +
> +        # Handle static routes
> +        for route in ifconfig.get('routes', []):
> +            cmd = ['ip', 'route', 'add', route['to']]
> +            with contextlib.suppress(KeyError):
> +                cmd += ['via', route['via']]
> +            if route.get('on-link', False):
> +                cmd += ['dev', ifconfig]
> +            commands.append(tuple(cmd))
> +
> +    return commands
> +
> +def configure_nvme_over_tcp(cfg, target):
>      """If any NVMe controller using the TCP transport is present in the storage
> -    configuration, create a nvme-stas configuration so that the remote drives
> -    can be made available at boot."""
> +    configuration, create a nvme-stas configuration and configure the initramfs
> +    so that the remote drives can be made available at boot.
> +    Please note that the NVMe over TCP support in curtin is experimental and in
> +    active development. Currently, it only works with trivial network
> +    configurations ; supplied by Subiquity."""
>      controllers = get_nvme_stas_controller_directives(cfg)
>  
>      if not controllers:
>          return
>  
> -    LOG.info('NVMe-over-TCP configuration found'
> -             ' , writing nvme-stas configuration')
> +    LOG.info('NVMe-over-TCP configuration found')
> +    LOG.info('writing nvme-stas configuration')
>      target = pathlib.Path(target)
>      stas_dir = target / 'etc' / 'stas'
>      stas_dir.mkdir(parents=True, exist_ok=True)
>      with (stas_dir / 'stafd-curtin.conf').open('w', encoding='utf-8') as fh:
> +        header = '''\
> +# This file was created by curtin.
> +# If you make modifications to it, please remember to also update
> +# scripts in etc/curtin-nvme-over-tcp and then regenerate the initramfs using
> +# the command `update-initramfs -u`.
> +'''
> +        print(header, file=fh)
>          print('[Controllers]', file=fh)
>          for controller in controllers:
>              print(controller, file=fh)
> @@ -1551,6 +1663,97 @@ def configure_nvme_stas(cfg, target):
>          (stas_dir / 'stafd.conf').replace(stas_dir / '.stafd.conf.bak')
>      (stas_dir / 'stafd.conf').symlink_to('stafd-curtin.conf')
>  
> +    if not nvmeotcp_need_network_in_initramfs(cfg):
> +        # nvme-stas should be enough to boot.
> +        return
> +
> +    LOG.info('configuring network in initramfs for NVMe over TCP')
> +
> +    hook_contents = '''\
> +#!/bin/sh

Short term we could do this, but it feels like a workaround.  Is there an appropriate archive location for this?  A new binary package for the functionality may make sense.

> +
> +PREREQ="udev"
> +
> +prereqs()
> +{
> +    echo "$PREREQ"
> +}
> +
> +case "$1" in
> +prereqs)
> +    prereqs
> +    exit 0
> +    ;;
> +esac
> +
> +. /usr/share/initramfs-tools/hook-functions
> +
> +copy_exec /usr/sbin/nvme /usr/sbin
> +copy_file config /etc/nvme/hostid /etc/nvme/
> +copy_file config /etc/nvme/hostnqn /etc/nvme/
> +copy_file config /etc/curtin-nvme-over-tcp/network-up /etc/curtin-nvme-over-tcp/
> +copy_file config /etc/curtin-nvme-over-tcp/connect-nvme /etc/curtin-nvme-over-tcp/
> +
> +manual_add_modules nvme-tcp
> +'''
> +
> +    initramfs_hooks_dir = target / 'etc' / 'initramfs-tools' / 'hooks'
> +    initramfs_hooks_dir.mkdir(parents=True, exist_ok=True)
> +    with (initramfs_hooks_dir / 'curtin-nvme-over-tcp').open('w', encoding='utf-8') as fh:
> +        print(hook_contents, file=fh)
> +    (initramfs_hooks_dir / 'curtin-nvme-over-tcp').chmod(0o755)
> +
> +    bootscript_contents = '''\
> +#!/bin/sh
> +
> +    PREREQ=""
> +prereqs() { echo "$PREREQ"; }
> +case "$1" in
> +prereqs)
> +    prereqs
> +    exit 0
> +    ;;
> +esac
> +
> +. /etc/curtin-nvme-over-tcp/network-up
> +
> +modprobe nvme-tcp
> +
> +. /etc/curtin-nvme-over-tcp/connect-nvme
> +
> +'''
> +
> +    initramfs_init_premount_dir = target / 'etc' / 'initramfs-tools' / 'scripts' / 'init-premount'
> +    initramfs_init_premount_dir.mkdir(parents=True, exist_ok=True)
> +    bootscript = initramfs_init_premount_dir / 'curtin-nvme-over-tcp'
> +    with bootscript.open('w', encoding='utf-8') as fh:
> +        print(bootscript_contents, file=fh)
> +    bootscript.chmod(0o755)
> +
> +
> +
> +    curtin_nvme_over_tcp_dir = target / 'etc' / 'curtin-nvme-over-tcp'
> +    curtin_nvme_over_tcp_dir.mkdir(parents=True, exist_ok=True)
> +    network_up_script = curtin_nvme_over_tcp_dir / 'network-up'
> +    connect_nvme_script =  curtin_nvme_over_tcp_dir / 'connect-nvme'
> +
> +    script_header = '''\
> +#!/bin/sh
> +
> +# This file was created by curtin.
> +# If you make modifications to it, please remember to regenerate the initramfs
> +# using the command `update-initramfs -u`.
> +'''
> +    with open(connect_nvme_script, 'w', encoding='utf-8') as fh:
> +        print(script_header, file=fh)
> +        for cmd in nvmeotcp_get_nvme_commands(cfg):
> +            print(shlex.join(cmd), file=fh)
> +
> +    with open(network_up_script, 'w', encoding='utf-8') as fh:
> +        print(script_header, file=fh)
> +        for cmd in nvmeotcp_get_ip_commands(cfg):
> +            print(shlex.join(cmd), file=fh)
> +
>  
>  def handle_cloudconfig(cfg, base_dir=None):
>      """write cloud-init configuration files into base_dir.


-- 
https://code.launchpad.net/~curtin-dev/curtin/+git/curtin/+merge/461452
Your team curtin developers is requested to review the proposed merge of curtin:nvme-initramfs into curtin:master.