From a5f183db03d826564e61a4b7a235fe48d38abf85 Mon Sep 17 00:00:00 2001 From: Timon Schnell Date: Thu, 20 Aug 2026 11:17:56 +0200 Subject: [PATCH 1/2] baremetal: create the raid configuration during clean, per node `osism baremetal clean` already prepends the raid clean step `delete_configuration` when a node has a raid interface, but nothing ever adds `create_configuration`. A node that had a software mirror therefore loses it on the first full clean and never gets it back, and a fleet whose `target_raid_config` was just declared cannot build its arrays with the OSISM CLI at all. Ironic exposes `create_configuration` only as a clean step, priority 0, so it has to be requested explicitly; the deploy step variant `apply_configuration` takes the configuration as a step argument through deploy templates rather than from `target_raid_config`. The step list now comes from `_build_clean_steps`, which builds `delete_configuration`, the erase step, `create_configuration`, which is the order the Ironic documentation prescribes: create does not remove existing disks and fails outright on a partitioned target, so delete and erase have to run first. `create_configuration` is only added when the node actually carries a `target_raid_config`, so a raid capable node without a declaration keeps its previous behaviour. Building the list per node also fixes an accumulation. It used to be built once before the node loop and then prepended to inside it, so with `--all` the second raid capable node got two `delete_configuration` steps, the third got three, and so on. Finally `--raid` and `--no-raid` make the choice explicit. The default keeps the current behaviour, raid steps on a full clean and none on `--metadata-only`, but the combination `--metadata-only --raid` is now expressible. That is what a pod needs whose data disks cannot be erased in band: on the Supermicro blades of one of our test pods the platform firmware freezes both ATA erase paths of the SATA SSDs during POST, so `erase_devices` has no working path there, while the mirror still has to be built. Verified locally: `tests/unit/commands/test_baremetal.py` goes from 131 to 138 passing tests, with cases for a node without a raid interface, a raid capable node with and without a declaration, both metadata only variants, `--no-raid` on a full clean, and a regression test for the accumulation. The remaining collection errors in `tests/unit` are missing optional dependencies in the local environment and are identical with and without this change. Signed-off-by: Timon Schnell Assisted-by: Claude:claude-opus-5 --- osism/commands/baremetal.py | 62 ++++++++++++++++----- tests/unit/commands/test_baremetal.py | 77 +++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 13 deletions(-) diff --git a/osism/commands/baremetal.py b/osism/commands/baremetal.py index 09aa8cb47..911ad86fa 100644 --- a/osism/commands/baremetal.py +++ b/osism/commands/baremetal.py @@ -16,6 +16,40 @@ from osism.utils.ssh import cleanup_ssh_known_hosts_for_node +def _build_clean_steps(node, metadata_only, raid=None): + """Build the clean step list for a single node. + + ``metadata_only`` selects the erase step. RAID capable nodes additionally + get ``delete_configuration`` in front of it and, when the node carries a + ``target_raid_config``, ``create_configuration`` behind it. That is the + order the Ironic documentation prescribes for software RAID: the create step + does not remove existing disks and fails outright on a partitioned target, + so delete and erase have to run first. + + ``raid`` overrides when the RAID steps are added. ``None`` keeps the + previous behaviour, RAID steps on a full clean and none on a metadata only + clean, which is what ``--raid`` and ``--no-raid`` make explicit. + + The list is built per node on purpose. Building it once and prepending to it + inside the node loop accumulated one ``delete_configuration`` per RAID + capable node when ``--all`` was used. + """ + if metadata_only: + steps = [{"interface": "deploy", "step": "erase_devices_metadata"}] + else: + steps = [{"interface": "deploy", "step": "erase_devices"}] + + raid_wanted = (not metadata_only) if raid is None else raid + if not raid_wanted or node.get("raid_interface", "no-raid") == "no-raid": + return steps + + steps = [{"interface": "raid", "step": "delete_configuration"}] + steps + if node.get("target_raid_config"): + steps = steps + [{"interface": "raid", "step": "create_configuration"}] + + return steps + + def _apply_metalbox_vars(play_vars, device): metalbox_ip = _get_metalbox_primary_ip4(device) if metalbox_ip: @@ -1189,6 +1223,16 @@ def get_parser(self, prog_name): help="Only erase metadata on disks", action="store_true", ) + parser.add_argument( + "--raid", + default=None, + help=( + "Include the raid clean steps, delete_configuration and, when the " + "node has a target_raid_config, create_configuration. Defaults to " + "on for a full clean and off for --metadata-only" + ), + action=BooleanOptionalAction, + ) parser.add_argument( "--all", default=False, @@ -1208,6 +1252,7 @@ def take_action(self, parsed_args): all_nodes = parsed_args.all name = parsed_args.name metadata_only = parsed_args.metadata_only + raid = parsed_args.raid yes_i_really_really_mean_it = parsed_args.yes_i_really_really_mean_it if not all_nodes and not name: @@ -1220,11 +1265,6 @@ def take_action(self, parsed_args): ) return 1 - if metadata_only: - clean_steps = [{"interface": "deploy", "step": "erase_devices_metadata"}] - else: - clean_steps = [{"interface": "deploy", "step": "erase_devices"}] - from osism.tasks.openstack import get_cloud_helpers setup_cloud_environment, get_openstack_connection, cleanup_cloud_environment = ( @@ -1252,14 +1292,10 @@ def take_action(self, parsed_args): if not node: continue - # NOTE: If the node has an agent raid interface, include step to delete the raid configuration - if ( - not metadata_only - and node.get("raid_interface", "no-raid") != "no-raid" - ): - clean_steps = [ - {"interface": "raid", "step": "delete_configuration"} - ] + clean_steps + # NOTE: The step list is built per node: a raid capable node gets + # delete_configuration in front of the erase step and, when it + # carries a target_raid_config, create_configuration behind it. + clean_steps = _build_clean_steps(node, metadata_only, raid) if node.provision_state in ["available"]: # NOTE: Clean is available in the "manageable" provision state, so we move the node into this state diff --git a/tests/unit/commands/test_baremetal.py b/tests/unit/commands/test_baremetal.py index 1f2f6a264..fe821c697 100644 --- a/tests/unit/commands/test_baremetal.py +++ b/tests/unit/commands/test_baremetal.py @@ -280,6 +280,83 @@ def _patch_cloud(setup, getconn, cleanup): ) +# --- _build_clean_steps --- + + +def _steps(node, metadata_only=False, raid=None): + return [ + (step["interface"], step["step"]) + for step in baremetal._build_clean_steps(node, metadata_only, raid) + ] + + +def test_clean_steps_without_raid_interface(): + node = FakeNode(raid_interface="no-raid", target_raid_config={"logical_disks": []}) + assert _steps(node) == [("deploy", "erase_devices")] + + +def test_clean_steps_raid_capable_without_target_config(): + """Unchanged behaviour: delete only, there is nothing to create.""" + node = FakeNode(raid_interface="agent", target_raid_config=None) + assert _steps(node) == [ + ("raid", "delete_configuration"), + ("deploy", "erase_devices"), + ] + + +def test_clean_steps_creates_configuration_when_declared(): + node = FakeNode( + raid_interface="agent", + target_raid_config={"logical_disks": [{"controller": "software"}]}, + ) + assert _steps(node) == [ + ("raid", "delete_configuration"), + ("deploy", "erase_devices"), + ("raid", "create_configuration"), + ] + + +def test_clean_steps_metadata_only_keeps_raid_untouched_by_default(): + node = FakeNode( + raid_interface="agent", + target_raid_config={"logical_disks": [{"controller": "software"}]}, + ) + assert _steps(node, metadata_only=True) == [("deploy", "erase_devices_metadata")] + + +def test_clean_steps_metadata_only_with_raid_requested(): + """The combination a fleet needs whose disks cannot be erased in band.""" + node = FakeNode( + raid_interface="agent", + target_raid_config={"logical_disks": [{"controller": "software"}]}, + ) + assert _steps(node, metadata_only=True, raid=True) == [ + ("raid", "delete_configuration"), + ("deploy", "erase_devices_metadata"), + ("raid", "create_configuration"), + ] + + +def test_clean_steps_no_raid_requested_on_full_clean(): + node = FakeNode( + raid_interface="agent", + target_raid_config={"logical_disks": [{"controller": "software"}]}, + ) + assert _steps(node, raid=False) == [("deploy", "erase_devices")] + + +def test_clean_steps_do_not_accumulate_across_nodes(): + """Regression: the list used to be built once and prepended to per node.""" + nodes = [ + FakeNode( + name=f"node{index}", raid_interface="agent", target_raid_config={"x": 1} + ) + for index in range(3) + ] + for node in nodes: + assert _steps(node).count(("raid", "delete_configuration")) == 1 + + # --- _apply_metalbox_vars --- From f36acff81c9f1e98cf350f75b425b2f725da0e0e Mon Sep 17 00:00:00 2001 From: Timon Schnell Date: Wed, 26 Aug 2026 16:38:02 +0200 Subject: [PATCH 2/2] baremetal: cover the per node clean steps at command level The helper level regression test could not detect the regression it was named for. `_build_clean_steps` returns fresh list literals with no module state and no mutable default, so calling it once per node in a test never accumulates. The accumulation happened in `take_action`'s node loop, and hoisting the helper call back out of that loop left the test green. Drop it and cover the loop where it runs. One `--all` run over three manageable nodes, one without a raid interface, one raid capable without a declaration and one with a `target_raid_config`, asserting the full ordered `clean_steps` of every call. The three kinds have to differ: three identical raid nodes would also pass with the call hoisted out of the loop. `--raid` and `--no-raid` had no coverage above the helper either, because `FakeNode` defaults `target_raid_config` to `None` and none of the command level clean tests set it. Add a single node case for each, `--metadata-only --raid` for the combination this change makes expressible and `--no-raid` on a full clean. Verified that both variants of the bug fail the new `--all` test: the pre change code gives the third node two `delete_configuration` steps, and hoisting the helper call out of the loop gives every node the first node's steps. The file goes from 138 to 140 passing tests. Assisted-by: Claude:claude-opus-5 Signed-off-by: Timon Schnell --- tests/unit/commands/test_baremetal.py | 84 +++++++++++++++++++++++---- 1 file changed, 72 insertions(+), 12 deletions(-) diff --git a/tests/unit/commands/test_baremetal.py b/tests/unit/commands/test_baremetal.py index fe821c697..ba177fd59 100644 --- a/tests/unit/commands/test_baremetal.py +++ b/tests/unit/commands/test_baremetal.py @@ -345,18 +345,6 @@ def test_clean_steps_no_raid_requested_on_full_clean(): assert _steps(node, raid=False) == [("deploy", "erase_devices")] -def test_clean_steps_do_not_accumulate_across_nodes(): - """Regression: the list used to be built once and prepended to per node.""" - nodes = [ - FakeNode( - name=f"node{index}", raid_interface="agent", target_raid_config={"x": 1} - ) - for index in range(3) - ] - for node in nodes: - assert _steps(node).count(("raid", "delete_configuration")) == 1 - - # --- _apply_metalbox_vars --- @@ -1402,6 +1390,7 @@ def test_burnin_unsupported_state_warns(loguru_logs): ERASE_DEVICES_STEP = {"interface": "deploy", "step": "erase_devices"} ERASE_METADATA_STEP = {"interface": "deploy", "step": "erase_devices_metadata"} RAID_DELETE_STEP = {"interface": "raid", "step": "delete_configuration"} +RAID_CREATE_STEP = {"interface": "raid", "step": "create_configuration"} def _run_baremetal_clean(args, conn): @@ -1460,6 +1449,77 @@ def test_clean_metadata_only_skips_delete_configuration_on_raid_node(): ) +def test_clean_all_builds_the_step_list_per_node(): + """Regression: the list used to be built once and prepended to per node. + + The three kinds have to differ. Three identical RAID nodes would also pass + with the call hoisted back out of the node loop. + """ + plain = FakeNode(id="uuid-1", name="node1", provision_state="manageable") + raid_only = FakeNode( + id="uuid-2", + name="node2", + provision_state="manageable", + raid_interface="agent", + ) + raid_declared = FakeNode( + id="uuid-3", + name="node3", + provision_state="manageable", + raid_interface="agent", + target_raid_config={"logical_disks": [{"controller": "software"}]}, + ) + conn = MagicMock() + conn.baremetal.nodes.return_value = [plain, raid_only, raid_declared] + + _run_baremetal_clean(["--all", "--yes-i-really-really-mean-it"], conn) + + assert conn.baremetal.set_node_provision_state.call_args_list == [ + call("uuid-1", "clean", clean_steps=[ERASE_DEVICES_STEP]), + call("uuid-2", "clean", clean_steps=[RAID_DELETE_STEP, ERASE_DEVICES_STEP]), + call( + "uuid-3", + "clean", + clean_steps=[RAID_DELETE_STEP, ERASE_DEVICES_STEP, RAID_CREATE_STEP], + ), + ] + + +def test_clean_metadata_only_with_raid_requested(): + """The combination a fleet needs whose disks cannot be erased in band.""" + node = FakeNode( + provision_state="manageable", + raid_interface="agent", + target_raid_config={"logical_disks": [{"controller": "software"}]}, + ) + conn = MagicMock() + conn.baremetal.find_node.return_value = node + + _run_baremetal_clean(["node1", "--metadata-only", "--raid"], conn) + + conn.baremetal.set_node_provision_state.assert_called_once_with( + node.id, + "clean", + clean_steps=[RAID_DELETE_STEP, ERASE_METADATA_STEP, RAID_CREATE_STEP], + ) + + +def test_clean_no_raid_skips_the_raid_steps_on_a_full_clean(): + node = FakeNode( + provision_state="manageable", + raid_interface="agent", + target_raid_config={"logical_disks": [{"controller": "software"}]}, + ) + conn = MagicMock() + conn.baremetal.find_node.return_value = node + + _run_baremetal_clean(["node1", "--no-raid"], conn) + + conn.baremetal.set_node_provision_state.assert_called_once_with( + node.id, "clean", clean_steps=[ERASE_DEVICES_STEP] + ) + + def test_clean_available_node_moved_to_manageable_first(loguru_logs): available = FakeNode(provision_state="available") manageable = FakeNode(provision_state="manageable")