Olivier Gayot has proposed merging ~ogayot/curtin:nvme-stas-of-only into
curtin:master.
Commit message:
do not squash
Requested reviews:
curtin developers (curtin-dev)
Related bugs:
Bug #2056730 in curtin: "noble-daily (20240310) cannot be installed offline"
https://bugs.launchpad.net/curtin/+bug/2056730
For more details, see:
https://code.launchpad.net/~ogayot/curtin/+git/curtin/+merge/462257
Only install nvme-stas if there is a NVMe-oF controller
If a NVMe controller is present on the system, the storage configuration will
contain an object of type nvme_controller. Currently, the presence of such
object in the configuration leads to the installation of nvme-cli and
nvme-stas. However, the packages are not needed when the controller uses the
PCIe transport.
We now inspect the nvme_controller instances and install nvme-stas & nvme-cli
only if we detect a NVMe-oF controller.
Also we had too many functions parsing the storage configuration to find the
NVMe controllers, so I moved to a single implementation in curtin/block/nvme.py
(this is patch number #2).
--
Your team curtin developers is requested to review the proposed merge of
~ogayot/curtin:nvme-stas-of-only into curtin:master.
diff --git a/curtin/block/deps.py b/curtin/block/deps.py
index e5370b6..d1d9d21 100644
--- a/curtin/block/deps.py
+++ b/curtin/block/deps.py
@@ -1,7 +1,7 @@
# This file is part of curtin. See LICENSE file for copyright and license info.
from curtin.distro import DISTROS
-from curtin.block import iscsi
+from curtin.block import iscsi, nvme
def storage_config_required_packages(storage_config, mapping):
@@ -35,6 +35,11 @@ def storage_config_required_packages(storage_config, mapping):
if len(iscsi_vols) > 0:
needed_packages.extend(mapping['iscsi'])
+ # for NVMe controllers with transport != pcie, we need NVMe-oF tools
+ if nvme.get_nvme_controllers_from_config(storage_config,
+ exclude_pcie=True):
+ needed_packages.extend(mapping['nvme_of_controller'])
+
# for any format operations, check the fstype and
# determine if we need any mkfs tools as well.
format_configs = set([operation['fstype']
@@ -69,7 +74,8 @@ def detect_required_packages_mapping(osfamily=DISTROS.debian):
'lvm_partition': ['lvm2'],
'lvm_volgroup': ['lvm2'],
'ntfs': ['ntfs-3g'],
- 'nvme_controller': ['nvme-cli', 'nvme-stas'],
+ 'nvme_controller': [],
+ 'nvme_of_controller': ['nvme-cli', 'nvme-stas'],
'raid': ['mdadm'],
'reiserfs': ['reiserfsprogs'],
'xfs': ['xfsprogs'],
@@ -91,6 +97,7 @@ def detect_required_packages_mapping(osfamily=DISTROS.debian):
'lvm_volgroup': ['lvm2'],
'ntfs': [],
'nvme_controller': [],
+ 'nvme_of_controller': [],
'raid': ['mdadm'],
'reiserfs': [],
'xfs': ['xfsprogs'],
@@ -112,6 +119,7 @@ def detect_required_packages_mapping(osfamily=DISTROS.debian):
'lvm_volgroup': ['lvm2'],
'ntfs': [],
'nvme_controller': [],
+ 'nvme_of_controller': [],
'raid': ['mdadm'],
'reiserfs': [],
'xfs': ['xfsprogs'],
diff --git a/curtin/block/nvme.py b/curtin/block/nvme.py
new file mode 100644
index 0000000..d1e0e59
--- /dev/null
+++ b/curtin/block/nvme.py
@@ -0,0 +1,40 @@
+# This file is part of curtin. See LICENSE file for copyright and license info.
+
+from typing import Any, Iterator
+
+from curtin.log import LOG
+
+
+def _iter_nvme_controllers(cfg) -> Iterator[dict[str, Any]]:
+ if not cfg:
+ cfg = {}
+
+ if 'storage' in cfg:
+ if not isinstance(cfg['storage'], dict):
+ sconfig = {}
+ else:
+ sconfig = cfg['storage'].get('config', [])
+ else:
+ sconfig = cfg.get('config', [])
+
+ if not sconfig or not isinstance(sconfig, list):
+ LOG.warning('Configuration dictionary did not contain'
+ ' a storage configuration.')
+ return
+
+ for item in sconfig:
+ if item['type'] == 'nvme_controller':
+ yield item
+
+
+def get_nvme_controllers_from_config(
+ cfg, *, exclude_pcie=False) -> list[dict[str, Any]]:
+ '''Parse a curtin storage config and return a list of
+ NVMe controllers. If exclude_pcie is True, only return controllers that do
+ not use PCIe transport.'''
+ controllers = _iter_nvme_controllers(cfg)
+
+ if not exclude_pcie:
+ return list(controllers)
+
+ return [ctrler for ctrler in controllers if ctrler['transport'] != 'pcie']
diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
index 63a24fe..0175447 100644
--- a/curtin/commands/curthooks.py
+++ b/curtin/commands/curthooks.py
@@ -20,6 +20,7 @@ from curtin import block
from curtin import distro
from curtin.block import iscsi
from curtin.block import lvm
+from curtin.block import nvme
from curtin import net
from curtin import futil
from curtin.log import LOG
@@ -1508,21 +1509,13 @@ def get_nvme_stas_controller_directives(cfg) -> Set[str]:
directives to write in the [Controllers] section of a nvme-stas
configuration file."""
directives = set()
- if 'storage' not in cfg or not isinstance(cfg['storage'], dict):
- return directives
- storage = cfg['storage']
- if 'config' not in storage or storage['config'] == 'disabled':
- return directives
- config = storage['config']
- for item in config:
- if item['type'] != 'nvme_controller':
- continue
- if item['transport'] != 'tcp':
+ for controller in nvme.get_nvme_controllers_from_config(cfg):
+ if controller['transport'] != 'tcp':
continue
controller_props = {
'transport': 'tcp',
- 'traddr': item["tcp_addr"],
- 'trsvcid': item["tcp_port"],
+ 'traddr': controller["tcp_addr"],
+ 'trsvcid': controller["tcp_port"],
}
props_str = ';'.join([f'{k}={v}' for k, v in controller_props.items()])
@@ -1535,23 +1528,15 @@ def nvmeotcp_get_nvme_commands(cfg) -> List[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()
- if 'storage' not in cfg or not isinstance(cfg['storage'], dict):
- return sorted(commands)
- storage = cfg['storage']
- if 'config' not in storage or storage['config'] == 'disabled':
- return sorted(commands)
- config = storage['config']
- for item in config:
- if item['type'] != 'nvme_controller':
- continue
- if item['transport'] != 'tcp':
+ for controller in nvme.get_nvme_controllers_from_config(cfg):
+ if controller['transport'] != 'tcp':
continue
commands.add((
'nvme', 'connect-all',
'--transport', 'tcp',
- '--traddr', item['tcp_addr'],
- '--trsvcid', str(item['tcp_port']),
+ '--traddr', controller['tcp_addr'],
+ '--trsvcid', str(controller['tcp_port']),
))
return sorted(commands)
diff --git a/tests/unittests/test_block_nvme.py b/tests/unittests/test_block_nvme.py
new file mode 100644
index 0000000..070e28f
--- /dev/null
+++ b/tests/unittests/test_block_nvme.py
@@ -0,0 +1,86 @@
+# This file is part of curtin. See LICENSE file for copyright and license info.
+
+import curtin.block.nvme as nvme
+from .helpers import CiTestCase
+
+
+class TestGetNvmeControllersFromConfig(CiTestCase):
+ def test_no_controller(self):
+ self.assertFalse(nvme.get_nvme_controllers_from_config({}))
+ self.assertFalse(nvme.get_nvme_controllers_from_config(
+ {"storage": False}))
+ self.assertFalse(nvme.get_nvme_controllers_from_config(
+ {"storage": {}}))
+ self.assertFalse(nvme.get_nvme_controllers_from_config({
+ "storage": {
+ "config": "disabled",
+ },
+ }))
+ self.assertFalse(nvme.get_nvme_controllers_from_config({
+ "storage": {
+ "config": [
+ {"type": "partition"},
+ {"type": "mount"},
+ {"type": "disk"},
+ ],
+ },
+ }))
+
+ def test_one_controller(self):
+ expected = [{"type": "nvme_controller", "transport": "pcie"}]
+
+ self.assertEqual(expected, nvme.get_nvme_controllers_from_config({
+ "storage": {
+ "config": [
+ {"type": "partition"},
+ {"type": "mount"},
+ {"type": "disk"},
+ {"type": "nvme_controller", "transport": "pcie"},
+ ],
+ },
+ }))
+
+ def test_multiple_controllers(self):
+ cfg = {
+ "storage": {
+ "config": [
+ {"type": "partition"},
+ {
+ "type": "nvme_controller",
+ "id": "nvme-controller-nvme0",
+ "transport": "tcp",
+ "tcp_addr": "1.2.3.4",
+ "tcp_port": "1111",
+ }, {
+ "type": "nvme_controller",
+ "id": "nvme-controller-nvme1",
+ "transport": "tcp",
+ "tcp_addr": "4.5.6.7",
+ "tcp_port": "1212",
+ }, {
+ "type": "nvme_controller",
+ "id": "nvme-controller-nvme2",
+ "transport": "pcie",
+ },
+ ],
+ },
+ }
+ ctrlers_id: list[str] = []
+ for ctrler in nvme.get_nvme_controllers_from_config(cfg):
+ ctrlers_id.append(ctrler["id"])
+
+ self.assertEqual([
+ "nvme-controller-nvme0",
+ "nvme-controller-nvme1",
+ "nvme-controller-nvme2",
+ ], ctrlers_id)
+
+ ctrlers_id: list[str] = []
+ for ctrler in nvme.get_nvme_controllers_from_config(cfg,
+ exclude_pcie=True):
+ ctrlers_id.append(ctrler["id"])
+
+ self.assertEqual([
+ "nvme-controller-nvme0",
+ "nvme-controller-nvme1",
+ ], ctrlers_id)
diff --git a/tests/unittests/test_curthooks.py b/tests/unittests/test_curthooks.py
index 61cad9f..526a3fe 100644
--- a/tests/unittests/test_curthooks.py
+++ b/tests/unittests/test_curthooks.py
@@ -1413,7 +1413,13 @@ class TestDetectRequiredPackages(CiTestCase):
'btrfs': {
'id': 'format3', 'fstype': 'btrfs', 'type': 'format'},
'xfs': {
- 'id': 'format4', 'fstype': 'xfs', 'type': 'format'}}
+ 'id': 'format4', 'fstype': 'xfs', 'type': 'format'},
+ 'nvme_controller_pcie': {
+ 'id': 'nvme_controller0', 'transport': 'pcie',
+ 'type': 'nvme_controller'},
+ 'nvme_controller_tcp': {
+ 'id': 'nvme_controller1', 'transport': 'tcp',
+ 'type': 'nvme_controller'}}
},
'network': {
1: {
@@ -1506,6 +1512,14 @@ class TestDetectRequiredPackages(CiTestCase):
'version': 1,
'items': ('bcache', 'lvm_volgroup', 'lvm_partition', 'ext2')}},
('bcache-tools', 'lvm2', 'e2fsprogs')),
+ ({'storage': {
+ 'version': 1,
+ 'items': ('nvme_controller_pcie',)}},
+ ()),
+ ({'storage': {
+ 'version': 1,
+ 'items': ('nvme_controller_tcp',)}},
+ ('nvme-cli', 'nvme-stas')),
))
def test_network_v1_detect(self):
@@ -2018,126 +2032,52 @@ class TestCurthooksGrubDebconf(CiTestCase):
class TestCurthooksNVMeOverTCP(CiTestCase):
- def test_get_nvme_stas_controller_directives__no_nvme_controller(self):
- self.assertFalse(curthooks.get_nvme_stas_controller_directives({
- "storage": {
- "config": [
- {"type": "partition"},
- {"type": "mount"},
- {"type": "disk"},
- ],
- },
- }))
-
- def test_get_nvme_stas_controller_directives__pcie_controller(self):
- self.assertFalse(curthooks.get_nvme_stas_controller_directives({
- "storage": {
- "config": [
- {"type": "nvme_controller", "transport": "pcie"},
- ],
- },
- }))
-
- def test_get_nvme_stas_controller_directives__tcp_controller(self):
- expected = {"controller = transport=tcp;traddr=1.2.3.4;trsvcid=1111"}
-
- result = curthooks.get_nvme_stas_controller_directives({
- "storage": {
- "config": [
- {
- "type": "nvme_controller",
- "transport": "tcp",
- "tcp_addr": "1.2.3.4",
- "tcp_port": "1111",
- },
- ],
- },
- })
- self.assertEqual(expected, result)
-
- def test_get_nvme_stas_controller_directives__three_nvme_controllers(self):
- expected = {"controller = transport=tcp;traddr=1.2.3.4;trsvcid=1111",
- "controller = transport=tcp;traddr=4.5.6.7;trsvcid=1212"}
-
- result = curthooks.get_nvme_stas_controller_directives({
- "storage": {
- "config": [
- {
- "type": "nvme_controller",
- "transport": "tcp",
- "tcp_addr": "1.2.3.4",
- "tcp_port": "1111",
- }, {
- "type": "nvme_controller",
- "transport": "tcp",
- "tcp_addr": "4.5.6.7",
- "tcp_port": "1212",
- }, {
- "type": "nvme_controller",
- "transport": "pcie",
- },
- ],
- },
- })
- self.assertEqual(expected, result)
-
- def test_get_nvme_stas_controller_directives__empty_conf(self):
- self.assertFalse(curthooks.get_nvme_stas_controller_directives({}))
- self.assertFalse(curthooks.get_nvme_stas_controller_directives(
- {"storage": False}))
- self.assertFalse(curthooks.get_nvme_stas_controller_directives(
- {"storage": {}}))
- self.assertFalse(curthooks.get_nvme_stas_controller_directives({
- "storage": {
- "config": "disabled",
- },
- }))
-
- def test_nvmeotcp_get_nvme_commands__no_nvme_controller(self):
- self.assertFalse(curthooks.nvmeotcp_get_nvme_commands({
- "storage": {
- "config": [
- {"type": "partition"},
- {"type": "mount"},
- {"type": "disk"},
- ],
- },
- }))
-
- def test_nvmeotcp_get_nvme_commands__pcie_controller(self):
- self.assertFalse(curthooks.nvmeotcp_get_nvme_commands({
- "storage": {
- "config": [
- {"type": "nvme_controller", "transport": "pcie"},
- ],
- },
- }))
-
- def test_nvmeotcp_get_nvme_commands__tcp_controller(self):
- expected = [(
+ def test_no_nvme_controller(self):
+ with patch('curtin.block.nvme.get_nvme_controllers_from_config',
+ return_value=[]):
+ self.assertFalse(
+ curthooks.get_nvme_stas_controller_directives(None))
+ self.assertFalse(curthooks.nvmeotcp_get_nvme_commands(None))
+
+ def test_pcie_controller(self):
+ controllers = [{'type': 'nvme_controller', 'transport': 'pcie'}]
+ with patch('curtin.block.nvme.get_nvme_controllers_from_config',
+ return_value=controllers):
+ self.assertFalse(
+ curthooks.get_nvme_stas_controller_directives(None))
+ self.assertFalse(curthooks.nvmeotcp_get_nvme_commands(None))
+
+ def test_tcp_controller(self):
+ stas_expected = {
+ 'controller = transport=tcp;traddr=1.2.3.4;trsvcid=1111',
+ }
+ cmds_expected = [(
"nvme", "connect-all",
"--transport", "tcp",
"--traddr", "1.2.3.4",
"--trsvcid", "1111",
),
]
-
- result = curthooks.nvmeotcp_get_nvme_commands({
- "storage": {
- "config": [
- {
- "type": "nvme_controller",
- "transport": "tcp",
- "tcp_addr": "1.2.3.4",
- "tcp_port": "1111",
- },
- ],
- },
- })
- self.assertEqual(expected, result)
-
- def test_nvmeotcp_get_nvme_commands__three_nvme_controllers(self):
- expected = [(
+ controllers = [{
+ "type": "nvme_controller",
+ "transport": "tcp",
+ "tcp_addr": "1.2.3.4",
+ "tcp_port": "1111",
+ }]
+
+ with patch('curtin.block.nvme.get_nvme_controllers_from_config',
+ return_value=controllers):
+ stas_result = curthooks.get_nvme_stas_controller_directives(None)
+ cmds_result = curthooks.nvmeotcp_get_nvme_commands(None)
+ self.assertEqual(stas_expected, stas_result)
+ self.assertEqual(cmds_expected, cmds_result)
+
+ def test_three_nvme_controllers(self):
+ stas_expected = {
+ "controller = transport=tcp;traddr=1.2.3.4;trsvcid=1111",
+ "controller = transport=tcp;traddr=4.5.6.7;trsvcid=1212",
+ }
+ cmds_expected = [(
"nvme", "connect-all",
"--transport", "tcp",
"--traddr", "1.2.3.4",
@@ -2149,40 +2089,29 @@ class TestCurthooksNVMeOverTCP(CiTestCase):
"--trsvcid", "1212",
),
]
-
- result = curthooks.nvmeotcp_get_nvme_commands({
- "storage": {
- "config": [
- {
- "type": "nvme_controller",
- "transport": "tcp",
- "tcp_addr": "1.2.3.4",
- "tcp_port": "1111",
- }, {
- "type": "nvme_controller",
- "transport": "tcp",
- "tcp_addr": "4.5.6.7",
- "tcp_port": "1212",
- }, {
- "type": "nvme_controller",
- "transport": "pcie",
- },
- ],
+ controllers = [
+ {
+ "type": "nvme_controller",
+ "transport": "tcp",
+ "tcp_addr": "1.2.3.4",
+ "tcp_port": "1111",
+ }, {
+ "type": "nvme_controller",
+ "transport": "tcp",
+ "tcp_addr": "4.5.6.7",
+ "tcp_port": "1212",
+ }, {
+ "type": "nvme_controller",
+ "transport": "pcie",
},
- })
- self.assertEqual(expected, result)
+ ]
- def test_nvmeotcp_get_nvme_commands__empty_conf(self):
- self.assertFalse(curthooks.nvmeotcp_get_nvme_commands({}))
- self.assertFalse(curthooks.nvmeotcp_get_nvme_commands(
- {"storage": False}))
- self.assertFalse(curthooks.nvmeotcp_get_nvme_commands(
- {"storage": {}}))
- self.assertFalse(curthooks.nvmeotcp_get_nvme_commands({
- "storage": {
- "config": "disabled",
- },
- }))
+ with patch('curtin.block.nvme.get_nvme_controllers_from_config',
+ return_value=controllers):
+ stas_result = curthooks.get_nvme_stas_controller_directives(None)
+ cmds_result = curthooks.nvmeotcp_get_nvme_commands(None)
+ self.assertEqual(stas_expected, stas_result)
+ self.assertEqual(cmds_expected, cmds_result)
def test_nvmeotcp_get_ip_commands__ethernet_static(self):
netcfg = """\
--
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
