curtin-dev team mailing list archive

[Olivier Gayot <mp+494693@xxxxxxxxxxxxxxxxxx>]

Olivier Gayot has proposed merging ~ogayot/curtin:nvme-o-tcp-dracut-target into curtin:master.

Requested reviews:
  curtin developers (curtin-dev)

For more details, see:
https://code.launchpad.net/~ogayot/curtin/+git/curtin/+merge/494693

Update the legacy NVMe/TCP implementation to support a target system that uses dracut as the initramfs generation tool.

By legacy implementation, we mean the implementation that does not require firmware support, but only supports systems with the initramfs and kernel present on local storage.
-- 
Your team curtin developers is requested to review the proposed merge of ~ogayot/curtin:nvme-o-tcp-dracut-target into curtin:master.

diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
index a31ef41..0334402 100644
--- a/curtin/commands/curthooks.py
+++ b/curtin/commands/curthooks.py
@@ -1655,12 +1655,20 @@ def configure_mdadm(cfg, state_etcd, target, osfamily=DISTROS.debian):
 
 
 def configure_nvme_over_tcp(cfg, target: pathlib.Path) -> None:
-    """If any NVMe controller using the TCP transport is present in the storage
-    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."""
+    '''If any NVMe controller using the TCP transport is present in the storage
+    configuration, configure the target system in such a way that makes booting
+    possible. We have three different implementations which come with different
+    requirements and limitations:
+    1. an implementation that leans on firmware support which makes it possible
+    to boot with no local storage at all. This only works with dracut and leans
+    on the nvmf module.
+    2. one that does not require firmware support but can still be used if the
+    kernel and initramfs are located on local storage. This works with dracut
+    and initramfs-tools.
+    3. a user-space implementation (using nvme-stas) that only works if all
+    essential filesystems are located on remote storage.
+    '''
+
     controllers = nvme_tcp.get_nvme_stas_controller_directives(cfg)
 
     if not controllers:
@@ -1671,25 +1679,8 @@ def configure_nvme_over_tcp(cfg, target: pathlib.Path) -> None:
         # jq is needed for the nvmf dracut module.
         distro.install_packages(['dracut', 'dracut-network', 'jq'],
                                 target=str(target))
-        # Let's make sure initramfs-tools does not get reinstalled over dracut.
-        # intel-microcode (pulled by linux-generic) is known to have
-        # initramfs-tools as a recommends. LP: #2073125
-        preferences_d = target / 'etc/apt/preferences.d'
-        preferences_d.mkdir(parents=True, exist_ok=True)
-        (preferences_d / 'nvmeotcp-poc-initramfs').write_text('''\
-# The NVMe/TCP proof of concept on Ubuntu uses dracut instead of
-# initramfs-tools.
-# That said, dracut is a universe package and is not the supported tool for
-# initramfs management. Installing packages that explicitly depend on
-# initramfs-tools will cause dracut to be removed, making the system unable to
-# boot. Furthermore, installing packages that have initramfs-tools as a
-# recommends can also trigger removal of dracut. Let's make sure
-# initramfs-tools does not get installed. See LP: #2073125.
-
-Package: initramfs-tools
-Pin: version *
-Pin-Priority: -1
-''')
+        # Make sure initramfs-tools does not get reinstalled.
+        nvme_tcp.prevent_initramfs_tools_reinstallation(target=target)
         # This will take care of reading the network configuration from the
         # NBFT and pass it to systemd-networkd.
         nvme_tcp.dracut_add_systemd_network_cmdline(target)
@@ -1698,7 +1689,16 @@ Pin-Priority: -1
 
         nvme_tcp.dracut_adapt_netplan_config(cfg, target=target)
     elif nvme_tcp.need_network_in_initramfs(cfg):
-        nvme_tcp.initramfs_tools_configure(cfg, target)
+        installed_packages = distro.get_installed_packages(str(target))
+        if 'initramfs-tools' in installed_packages:
+            nvme_tcp.initramfs_tools_configure_no_firmware_support(cfg, target)
+        elif 'dracut' in installed_packages:
+            distro.install_packages(['dracut-network'], target=str(target))
+            nvme_tcp.dracut_configure_no_firmware_support(cfg, target)
+            nvme_tcp.dracut_adapt_netplan_config(cfg, target=target)
+        else:
+            raise RuntimeError('cannot configure NVMe/TCP:'
+                               ' unknown initramfs generation tool')
     else:
         # Do not bother configuring the initramfs, everything will be done in
         # userspace.
diff --git a/curtin/nvme_tcp.py b/curtin/nvme_tcp.py
index b7cfdc5..a285e09 100644
--- a/curtin/nvme_tcp.py
+++ b/curtin/nvme_tcp.py
@@ -229,12 +229,123 @@ def configure_nvme_stas(cfg, target: pathlib.Path) -> None:
     (stas_dir / 'stafd.conf').symlink_to('stafd-curtin.conf')
 
 
-def initramfs_tools_configure(cfg, target: pathlib.Path) -> None:
+def _deploy_shell_script(content: str, path: pathlib.Path) -> None:
+    full_content = f'''\
+#!/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`.
+
+{content}
+'''
+    path.write_text(full_content)
+    path.chmod(0o755)
+
+
+def _deploy_network_up_script(cfg, target: pathlib.Path) -> None:
+    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'
+
+    network_up_content = '\n'.join(
+            [shlex.join(cmd) for cmd in get_ip_commands(cfg)])
+
+    _deploy_shell_script(network_up_content, network_up_script)
+
+
+def _deploy_connect_nvme_script(cfg, target: pathlib.Path) -> None:
+    curtin_nvme_over_tcp_dir = target / 'etc' / 'curtin-nvme-over-tcp'
+    curtin_nvme_over_tcp_dir.mkdir(parents=True, exist_ok=True)
+    connect_nvme_script = curtin_nvme_over_tcp_dir / 'connect-nvme'
+
+    connect_nvme_content = '\n'.join(
+            [shlex.join(cmd) for cmd in get_nvme_commands(cfg)])
+
+    _deploy_shell_script(connect_nvme_content, connect_nvme_script)
+
+
+def dracut_configure_no_firmware_support(cfg, target: pathlib.Path) -> None:
+    '''Configure dracut for NVMe/TCP. This is a legacy approach where
+    nvme connect-all commands are manually crafted. Unlike the initramfs-tools
+    implementation, the network is configured using systemd-network.
+    This implementation does not require firmware support.'''
+    LOG.info('configuring dracut for NVMe over TCP without firmware support')
+
+    _deploy_connect_nvme_script(cfg, target=target)
+
+    module_setup_contents = '''\
+#!/bin/bash
+
+depends() {
+    return 0
+}
+
+install_netconf()
+{
+    # Install the network configuration
+    netplan generate
+
+    shopt -s nullglob
+
+    # Unfortunately, inst_* dracut builtin functions don't support installing a
+    # file in a different directory while preserving the filename.
+    for _f in /etc/systemd/network/*; do
+        mkdir --parents "$initdir"/etc/systemd/network
+        command install -t "$initdir"/etc/systemd/network --mode 644 "$_f"
+    done
+}
+
+install() {
+    inst_binary /usr/sbin/nvme
+    inst_simple /etc/nvme/hostid
+    inst_simple /etc/nvme/hostnqn
+    inst_simple /etc/curtin-nvme-over-tcp/connect-nvme
+
+    inst_hook initqueue/settled 99 "$moddir/connect-nvme.sh"
+
+    (install_netconf)
+}
+
+installkernel() {
+    hostonly='' instmods nvme_tcp
+}
+'''
+
+    connect_hook_contents = '''\
+#!/bin/bash
+
+modprobe nvme-tcp
+
+/usr/lib/systemd/systemd-networkd-wait-online
+
+/etc/curtin-nvme-over-tcp/connect-nvme
+'''
+    dracut_mods_dir = target / 'usr' / 'lib' / 'dracut' / 'modules.d'
+    dracut_curtin_mod = dracut_mods_dir / '35curtin-nvme-tcp'
+    dracut_curtin_mod.mkdir(parents=True, exist_ok=True)
+
+    module_setup = dracut_curtin_mod / 'module-setup.sh'
+    with module_setup.open('w', encoding='utf-8') as fh:
+        print(module_setup_contents, file=fh)
+    module_setup.chmod(0o755)
+
+    connect_hook = dracut_curtin_mod / 'connect-nvme.sh'
+    with connect_hook.open('w', encoding='utf-8') as fh:
+        print(connect_hook_contents, file=fh)
+    connect_hook.chmod(0o755)
+
+
+def initramfs_tools_configure_no_firmware_support(
+        cfg, target: pathlib.Path) -> None:
     """Configure initramfs-tools for NVMe/TCP. This is a legacy approach where
     the network is hardcoded and nvme connect-all commands are manually
     crafted. However, this implementation does not require firmware support."""
     LOG.info('configuring initramfs-tools for NVMe over TCP')
 
+    _deploy_network_up_script(cfg, target=target)
+    _deploy_connect_nvme_script(cfg, target=target)
+
     hook_contents = '''\
 #!/bin/sh
 
@@ -301,28 +412,6 @@ modprobe nvme-tcp
         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 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 get_ip_commands(cfg):
-            print(shlex.join(cmd), file=fh)
-
 
 class NetRuntimeError(RuntimeError):
     pass
@@ -404,3 +493,26 @@ def dracut_adapt_netplan_config(cfg, *, target: pathlib.Path):
 
     if modified:
         netplan_conf_path.write_text(yaml.dump(config))
+
+
+def prevent_initramfs_tools_reinstallation(target: pathlib.Path) -> None:
+    '''Ensure that initramfs-tools does not get reinstalled over dracut, using
+    APT pinning.'''
+    # intel-microcode on 24.04 (pulled by linux-generic) is known to have
+    # initramfs-tools as a recommends. LP: #2073125
+    preferences_d = target / 'etc/apt/preferences.d'
+    preferences_d.mkdir(parents=True, exist_ok=True)
+    (preferences_d / 'nvmeotcp-poc-initramfs').write_text('''\
+# The NVMe/TCP proof of concept on Ubuntu uses dracut instead of
+# initramfs-tools.
+# That said, dracut is a universe package and is not the supported tool for
+# initramfs management. Installing packages that explicitly depend on
+# initramfs-tools will cause dracut to be removed, making the system unable to
+# boot. Furthermore, installing packages that have initramfs-tools as a
+# recommends can also trigger removal of dracut. Let's make sure
+# initramfs-tools does not get installed. See LP: #2073125.
+
+Package: initramfs-tools
+Pin: version *
+Pin-Priority: -1
+''')
diff --git a/tests/unittests/test_curthooks.py b/tests/unittests/test_curthooks.py
index a775573..f4f3112 100644
--- a/tests/unittests/test_curthooks.py
+++ b/tests/unittests/test_curthooks.py
@@ -2505,11 +2505,11 @@ class TestCurthooksNVMeOverTCP(CiTestCase):
     @patch('curtin.nvme_tcp.requires_firmware_support',
            Mock(return_value=True))
     @patch('curtin.nvme_tcp.dracut_add_systemd_network_cmdline')
-    @patch('curtin.nvme_tcp.initramfs_tools_configure')
+    @patch('curtin.nvme_tcp.initramfs_tools_configure_no_firmware_support')
     @patch('curtin.nvme_tcp.need_network_in_initramfs', Mock())
     @patch('curtin.nvme_tcp.configure_nvme_stas')
     @patch('curtin.distro.install_packages')
-    def test_configure_nvme_over_tcp__dracut(
+    def test_configure_nvme_over_tcp__firmware_support(
             self, m_install_pkgs, m_config_stas, m_initramfs_tools_config,
             m_dracut_add_module):
 
@@ -2526,18 +2526,23 @@ class TestCurthooksNVMeOverTCP(CiTestCase):
     @patch('curtin.nvme_tcp.requires_firmware_support',
            Mock(return_value=False))
     @patch('curtin.nvme_tcp.dracut_add_systemd_network_cmdline')
-    @patch('curtin.nvme_tcp.initramfs_tools_configure')
+    @patch('curtin.nvme_tcp.initramfs_tools_configure_no_firmware_support')
+    @patch('curtin.nvme_tcp.dracut_configure_no_firmware_support')
     @patch('curtin.nvme_tcp.need_network_in_initramfs',
            Mock(return_value=True))
     @patch('curtin.nvme_tcp.configure_nvme_stas')
     @patch('curtin.distro.install_packages')
+    @patch('curtin.distro.get_installed_packages',
+           Mock(return_value={'foo', 'initramfs-tools'}))
     def test_configure_nvme_over_tcp__initramfs_tools(
-            self, m_install_pkgs, m_config_stas, m_initramfs_tools_config,
+            self, m_install_pkgs, m_config_stas,
+            m_dracut_config, m_initramfs_tools_config,
             m_dracut_add_module):
 
         curthooks.configure_nvme_over_tcp({}, Path('/tmp'))
 
         m_initramfs_tools_config.assert_called_once_with({}, Path('/tmp'))
+        m_dracut_config.assert_not_called()
 
         m_dracut_add_module.assert_not_called()
         m_config_stas.assert_not_called()
@@ -2547,7 +2552,34 @@ class TestCurthooksNVMeOverTCP(CiTestCase):
     @patch('curtin.nvme_tcp.requires_firmware_support',
            Mock(return_value=False))
     @patch('curtin.nvme_tcp.dracut_add_systemd_network_cmdline')
-    @patch('curtin.nvme_tcp.initramfs_tools_configure')
+    @patch('curtin.nvme_tcp.initramfs_tools_configure_no_firmware_support')
+    @patch('curtin.nvme_tcp.dracut_configure_no_firmware_support')
+    @patch('curtin.nvme_tcp.need_network_in_initramfs',
+           Mock(return_value=True))
+    @patch('curtin.nvme_tcp.configure_nvme_stas')
+    @patch('curtin.distro.install_packages')
+    @patch('curtin.distro.get_installed_packages',
+           Mock(return_value={'foo', 'dracut'}))
+    def test_configure_nvme_over_tcp__dracut(
+            self, m_install_pkgs, m_config_stas,
+            m_dracut_config, m_initramfs_tools_config,
+            m_dracut_add_module):
+
+        curthooks.configure_nvme_over_tcp({}, Path('/tmp'))
+
+        m_initramfs_tools_config.assert_not_called()
+        m_dracut_config.assert_called_once_with({}, Path('/tmp'))
+
+        m_dracut_add_module.assert_not_called()
+        m_config_stas.assert_not_called()
+        m_install_pkgs.assert_called_once_with(
+                ['dracut-network'], target='/tmp')
+
+    @patch('curtin.nvme_tcp.get_nvme_stas_controller_directives', Mock())
+    @patch('curtin.nvme_tcp.requires_firmware_support',
+           Mock(return_value=False))
+    @patch('curtin.nvme_tcp.dracut_add_systemd_network_cmdline')
+    @patch('curtin.nvme_tcp.initramfs_tools_configure_no_firmware_support')
     @patch('curtin.nvme_tcp.need_network_in_initramfs',
            Mock(return_value=False))
     @patch('curtin.nvme_tcp.configure_nvme_stas')
diff --git a/tests/unittests/test_nvme_tcp.py b/tests/unittests/test_nvme_tcp.py
index 227e317..8975b65 100644
--- a/tests/unittests/test_nvme_tcp.py
+++ b/tests/unittests/test_nvme_tcp.py
@@ -267,7 +267,7 @@ network:
         self.assertTrue(cmdline_sh.exists())
         self.assertTrue(setup_sh.exists())
 
-    def test_initramfs_tools_configure(self):
+    def test_initramfs_tools_configure_no_firmware_support(self):
         target = self.tmp_dir()
 
         nvme_cmds = [
@@ -284,7 +284,8 @@ network:
 
         with (patch(get_nvme_cmds_sym, return_value=nvme_cmds),
               patch(get_ip_cmds_sym, return_value=ip_cmds)):
-            nvme_tcp.initramfs_tools_configure({}, target=Path(target))
+            nvme_tcp.initramfs_tools_configure_no_firmware_support(
+                    {}, target=Path(target))
 
         init_premount_dir = 'etc/initramfs-tools/scripts/init-premount'