baremetal: create the raid configuration during clean, per node

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

tests/unit/commands/test_baremetal.py

@@ -280,6 +280,71 @@ 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")]
+
+
 # --- _apply_metalbox_vars ---
 
 
@@ -1325,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):
@@ -1383,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")