From 8be5f841a277d7d64e5011cc12b2104058753feb Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Thu, 27 Aug 2026 13:34:34 +0200 Subject: [PATCH 1/2] baremetal: anchor bare --raid on a full clean `create_configuration` on a full clean is currently asserted in exactly one place: node 3 of `test_clean_all_builds_the_step_list_per_node`, under the default. An upcoming change moves the create step off the default path and behind an explicit `--raid recreate`, which means editing that expectation. Bare `--raid` is documented to keep its current meaning across that change, but nothing pins it, so the edit would leave the full-clean rebuild with no test at all. Add that test now, while it still passes. It is deliberately a duplicate assertion against the current code: with `raid=None` and `raid=True` both resolving to the same thing on a full clean, no test at either level passes `raid=True` with `metadata_only=False`. It stops being a duplicate as soon as the default changes. Written before the behaviour change on purpose. A test added afterwards would only restate whatever the new code does; this one records what the code does today, so it can fail if the compatibility promise is broken. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi --- tests/unit/commands/test_baremetal.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/unit/commands/test_baremetal.py b/tests/unit/commands/test_baremetal.py index ba177fd59..e104f300f 100644 --- a/tests/unit/commands/test_baremetal.py +++ b/tests/unit/commands/test_baremetal.py @@ -1520,6 +1520,30 @@ def test_clean_no_raid_skips_the_raid_steps_on_a_full_clean(): ) +def test_clean_raid_requested_on_a_full_clean(): + """Bare ``--raid`` rebuilds the declared array on a full clean. + + Written before the default changes, while it is still a duplicate of the + default path, so that it anchors the behaviour bare ``--raid`` keeps rather + than restating whatever the new default does. + """ + 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", "--raid"], conn) + + conn.baremetal.set_node_provision_state.assert_called_once_with( + node.id, + "clean", + clean_steps=[RAID_DELETE_STEP, ERASE_DEVICES_STEP, RAID_CREATE_STEP], + ) + + def test_clean_available_node_moved_to_manageable_first(loguru_logs): available = FakeNode(provision_state="available") manageable = FakeNode(provision_state="manageable") From 1b9afcff899b97486fecf07da96587faf53ac36e Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Thu, 27 Aug 2026 13:47:00 +0200 Subject: [PATCH 2/2] baremetal: give clean --raid three named modes `--raid` / `--no-raid` could only express two of the three things an operator does with a RAID configuration during a clean. "Leave the existing array alone" and "tear it down without rebuilding" are different outcomes, and a boolean has to conflate them: `--no-raid` suppressed the delete step as well, making it less destructive than the pre-change default rather than equal to it. Replace them with `--raid {delete,keep,recreate}`, an optional value whose bare form still means recreate, so `clean --metadata-only --raid` keeps working. `--no-raid` is removed rather than aliased: it has not been in a release, so no operator has ever had it, and `--raid keep` says what it does. delete delete_configuration in front of the erase step keep no raid steps, erase through the existing array recreate delete_configuration in front, create_configuration behind The default resolves to delete on a full clean and keep with `--metadata-only`, so it tracks the erase depth and restores the behaviour the command had before the create step was added. That step now needs an explicit `--raid recreate`. A `target_raid_config` can be declared fleet-wide from one conductor.yml, so keying the default on it turned a routine `clean --all` into a fleet-wide array build, and create is the only one of the three outcomes that can fail into `clean failed` plus maintenance mode. recreate delivers its outcome or refuses; it never degrades to delete, which would be the destructive opposite of what was asked for. A named node that cannot be built is refused before any provision state transition, which is stricter than before the clean call because an available node is moved to manageable and waited for first. Under `--all` the node set is discovered rather than asserted, so an undeclared node is skipped and reported and the rest of the fleet still runs, with a non-zero exit so automation sees the gap; a node with no raid interface is out of scope there and is cleaned with the erase step alone. Nodes in a state the loop would not clean anyway are left out of the preflight entirely. `--raid` taking an optional value means argparse claims a node name written directly after it, so `clean --raid node101` cannot work. The error names the two spellings that do instead of leaving the stock "invalid choice" message. The five use cases are in the parser epilog, which is the only user-facing documentation these flags have; osism.github.io documents none of them. DocImpact Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi --- osism/commands/baremetal.py | 171 +++++++++-- tests/unit/commands/test_baremetal.py | 393 +++++++++++++++++++++++--- 2 files changed, 510 insertions(+), 54 deletions(-) diff --git a/osism/commands/baremetal.py b/osism/commands/baremetal.py index 911ad86fa..474df1e70 100644 --- a/osism/commands/baremetal.py +++ b/osism/commands/baremetal.py @@ -1,7 +1,12 @@ # SPDX-License-Identifier: Apache-2.0 from cliff.command import Command -from argparse import BooleanOptionalAction +from argparse import ( + ArgumentTypeError, + BooleanOptionalAction, + RawDescriptionHelpFormatter, +) +from textwrap import dedent import tempfile import os @@ -15,20 +20,35 @@ from osism.tasks.conductor.ironic import _get_metalbox_primary_ip4 from osism.utils.ssh import cleanup_ssh_known_hosts_for_node +RAID_MODES = ("delete", "keep", "recreate") + +# NOTE: The states the clean loop acts on. Anything else only draws a warning, +# so a node in such a state must not make a --raid recreate run fail. +CLEANABLE_PROVISION_STATES = ("available", "manageable") + 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. + ``metadata_only`` selects the erase step. ``raid`` names what to leave + behind on a RAID capable node: + + ``delete`` + ``delete_configuration`` in front of the erase step. + ``keep`` + no RAID steps; the erase runs through the existing array. + ``recreate`` + ``delete_configuration`` in front and ``create_configuration`` behind. + 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. + ``None`` resolves to ``delete`` on a full clean and ``keep`` with + ``metadata_only``, so the default tracks the erase depth and matches the + behaviour the command had before the mode became selectable. + + Nodes without a RAID interface cannot hold a configuration and get the + erase step alone whatever the mode. 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 @@ -39,17 +59,66 @@ def _build_clean_steps(node, metadata_only, raid=None): 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": + mode = raid or ("keep" if metadata_only else "delete") + if mode == "keep" or node.get("raid_interface", "no-raid") == "no-raid": return steps steps = [{"interface": "raid", "step": "delete_configuration"}] + steps - if node.get("target_raid_config"): + # NOTE: A node reaching this without a target_raid_config under "recreate" + # is refused before any provision state changes, see + # _raid_recreate_blocker. + if mode == "recreate" and node.get("target_raid_config"): steps = steps + [{"interface": "raid", "step": "create_configuration"}] return steps +def _raid_mode(value): + """Validate a ``--raid`` mode and explain the node name ordering trap. + + ``--raid`` takes an optional value, so argparse hands it the next token + even when that token is the node name. The stock ``choices`` message + ("invalid choice: 'node101'") does not tell the operator what to do about + it, and the shape that trips is a plausible one to type. + """ + if value not in RAID_MODES: + raise ArgumentTypeError( + f"invalid raid mode '{value}', choose from " + f"{', '.join(RAID_MODES)}. Note that --raid takes an optional " + f"value, so a node name cannot follow it directly: write " + f"'clean node101 --raid' to use the default mode, or name the " + f"mode as in 'clean --raid recreate node101'" + ) + return value + + +def _raid_recreate_blocker(node, raid, fleet): + """Why ``node`` cannot satisfy the requested ``raid`` mode, or ``None``. + + Only ``recreate`` promises to build something, so only ``recreate`` can be + unsatisfiable. A mode that names an outcome has to deliver it or refuse: + degrading to ``delete`` would hand the operator the opposite of what they + asked for, destructively. + + ``fleet`` selects the ``--all`` rule. There the node set is discovered + rather than asserted, so a node without a raid interface is simply out of + scope for the raid axis and is cleaned with the erase step alone. Naming + that node is an assertion about it, and the assertion is wrong, so it is + refused. A raid capable node with nothing declared is misconfigured either + way and is never silently cleaned under ``recreate``. + """ + if raid != "recreate": + return None + + if node.get("raid_interface", "no-raid") == "no-raid": + return None if fleet else "it has no raid interface" + + if not node.get("target_raid_config"): + return "it has no target_raid_config" + + return None + + def _apply_metalbox_vars(play_vars, device): metalbox_ip = _get_metalbox_primary_ip4(device) if metalbox_ip: @@ -1205,6 +1274,29 @@ class BaremetalClean(Command): def get_parser(self, prog_name): parser = super(BaremetalClean, self).get_parser(prog_name) + # NOTE: These examples are the only user-facing documentation the clean + # flags have; the published pages document none of them. Raw + # formatting keeps the invocations on their own lines. + parser.formatter_class = RawDescriptionHelpFormatter + parser.epilog = dedent("""\ + examples: + wipe a node completely, leaving no array behind + osism baremetal clean node101 + + recycle a node into the pool with its declared array rebuilt + osism baremetal clean --raid recreate node101 + + wipe the data but leave the existing array in place + osism baremetal clean --raid keep node101 + + fast recycle, erase disk metadata only + osism baremetal clean --metadata-only node101 + + build or rebuild the declared array on disks that cannot be + erased in band + osism baremetal clean --metadata-only --raid recreate node101 + """) + parser.add_argument( "--cloud", type=str, @@ -1225,13 +1317,22 @@ def get_parser(self, prog_name): ) parser.add_argument( "--raid", + nargs="?", + const="recreate", + type=_raid_mode, + choices=RAID_MODES, default=None, + metavar="{delete,keep,recreate}", 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" + "What to do with the raid configuration of raid capable nodes. " + "delete: remove the existing configuration (default for a full " + "clean). keep: leave it in place and erase through the existing " + "array (default with --metadata-only). recreate: remove it and " + "build the node's target_raid_config again. Given without a " + "value, --raid means recreate. Nodes without a raid interface " + "are unaffected. recreate refuses a named node that has nothing " + "to build, and skips such nodes under --all with a non-zero exit" ), - action=BooleanOptionalAction, ) parser.add_argument( "--all", @@ -1287,14 +1388,42 @@ def take_action(self, parsed_args): return 1 clean_nodes = [node] - failed = False + # NOTE: Preflight the whole set before touching anything. The refusal + # has to land before any provision state transition, not just + # before the clean call: an available node is moved to + # manageable and waited for further down. + blocked = {} for node in clean_nodes: if not node: continue - # 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. + if node.provision_state not in CLEANABLE_PROVISION_STATES: + continue + + blocker = _raid_recreate_blocker(node, raid, all_nodes) + if blocker: + blocked[node.id] = blocker + logger.error( + f"Node {node.name} ({node.id}) cannot satisfy --raid recreate, {blocker}" + ) + + if blocked and not all_nodes: + return 1 + + # NOTE: Under --all the remaining nodes are still cleaned, so one + # misconfigured node does not abort the fleet, but the run + # exits non-zero so the gap is visible in automation. + failed = bool(blocked) + for node in clean_nodes: + if not node: + continue + + if node.id in blocked: + continue + + # NOTE: The step list is built per node: the raid mode decides + # whether a raid capable node gets delete_configuration in + # front of the erase step and create_configuration behind it. clean_steps = _build_clean_steps(node, metadata_only, raid) if node.provision_state in ["available"]: diff --git a/tests/unit/commands/test_baremetal.py b/tests/unit/commands/test_baremetal.py index e104f300f..4fec10ec4 100644 --- a/tests/unit/commands/test_baremetal.py +++ b/tests/unit/commands/test_baremetal.py @@ -283,6 +283,9 @@ def _patch_cloud(setup, getconn, cleanup): # --- _build_clean_steps --- +DECLARED = {"logical_disks": [{"controller": "software"}]} + + def _steps(node, metadata_only=False, raid=None): return [ (step["interface"], step["step"]) @@ -295,8 +298,22 @@ def test_clean_steps_without_raid_interface(): assert _steps(node) == [("deploy", "erase_devices")] +def test_clean_steps_no_raid_interface_ignores_the_raid_mode(): + """A node that cannot hold a configuration is unaffected by any mode.""" + node = FakeNode(raid_interface="no-raid", target_raid_config=DECLARED) + assert _steps(node, raid="recreate") == [("deploy", "erase_devices")] + + +def test_clean_steps_default_deletes_without_rebuilding(): + """A full clean leaves nothing behind unless a rebuild is asked for.""" + node = FakeNode(raid_interface="agent", target_raid_config=DECLARED) + assert _steps(node) == [ + ("raid", "delete_configuration"), + ("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"), @@ -304,45 +321,49 @@ def test_clean_steps_raid_capable_without_target_config(): ] -def test_clean_steps_creates_configuration_when_declared(): - node = FakeNode( - raid_interface="agent", - target_raid_config={"logical_disks": [{"controller": "software"}]}, - ) - assert _steps(node) == [ +def test_clean_steps_delete_removes_the_configuration(): + node = FakeNode(raid_interface="agent", target_raid_config=DECLARED) + assert _steps(node, raid="delete") == [ + ("raid", "delete_configuration"), + ("deploy", "erase_devices"), + ] + + +def test_clean_steps_keep_leaves_the_configuration_in_place(): + node = FakeNode(raid_interface="agent", target_raid_config=DECLARED) + assert _steps(node, raid="keep") == [("deploy", "erase_devices")] + + +def test_clean_steps_recreate_rebuilds_the_declared_array(): + node = FakeNode(raid_interface="agent", target_raid_config=DECLARED) + assert _steps(node, raid="recreate") == [ ("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"}]}, - ) +def test_clean_steps_metadata_only_keeps_the_array_by_default(): + node = FakeNode(raid_interface="agent", target_raid_config=DECLARED) assert _steps(node, metadata_only=True) == [("deploy", "erase_devices_metadata")] -def test_clean_steps_metadata_only_with_raid_requested(): +def test_clean_steps_metadata_only_with_recreate(): """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) == [ + node = FakeNode(raid_interface="agent", target_raid_config=DECLARED) + assert _steps(node, metadata_only=True, raid="recreate") == [ ("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_metadata_only_with_delete(): + node = FakeNode(raid_interface="agent", target_raid_config=DECLARED) + assert _steps(node, metadata_only=True, raid="delete") == [ + ("raid", "delete_configuration"), + ("deploy", "erase_devices_metadata"), + ] # --- _apply_metalbox_vars --- @@ -1401,6 +1422,83 @@ def _run_baremetal_clean(args, conn): return cmd.take_action(parsed_args) +def _clean_parser(): + return baremetal.BaremetalClean(MagicMock(), MagicMock()).get_parser("test") + + +def test_clean_raid_flag_omitted_leaves_the_mode_unset(): + assert _clean_parser().parse_args(["node1"]).raid is None + + +def test_clean_raid_flag_without_a_value_means_recreate(): + """Bare ``--raid`` keeps the meaning it had as a boolean flag.""" + assert _clean_parser().parse_args(["node1", "--raid"]).raid == "recreate" + + +@pytest.mark.parametrize("mode", ["delete", "keep", "recreate"]) +def test_clean_raid_flag_accepts_each_mode(mode): + assert _clean_parser().parse_args(["node1", "--raid", mode]).raid == mode + + +def test_clean_raid_flag_rejects_an_unknown_mode(): + with pytest.raises(SystemExit): + _clean_parser().parse_args(["node1", "--raid", "rebuild"]) + + +def test_clean_raid_flag_explains_the_node_name_ordering(capsys): + """``--raid`` takes an optional value, so a node name after it is swallowed. + + Inherent to an optional-value flag beside an optional positional. The + error has to name the orders that work, because the bare argparse message + ("invalid choice: 'node101'") gives the operator nothing to act on. + """ + with pytest.raises(SystemExit): + _clean_parser().parse_args(["--metadata-only", "--raid", "node101"]) + + message = capsys.readouterr().err + assert "node101 --raid" in message + assert "--raid recreate node101" in message + + +@pytest.mark.parametrize( + "args", + [ + ["node101", "--raid"], + ["node101", "--metadata-only", "--raid"], + ["--all", "--metadata-only", "--raid"], + ["--raid", "recreate", "node101"], + ], +) +def test_clean_raid_flag_orders_that_work(args): + assert _clean_parser().parse_args(args).raid == "recreate" + + +def test_clean_no_raid_flag_is_gone(): + """Replaced by ``--raid keep``, which says what it does.""" + with pytest.raises(SystemExit): + _clean_parser().parse_args(["node1", "--no-raid"]) + + +def test_clean_help_lists_the_example_invocations(): + """The epilog is the only user-facing documentation these flags have.""" + help_text = _clean_parser().format_help() + for example in [ + "osism baremetal clean node101", + "osism baremetal clean --raid recreate node101", + "osism baremetal clean --raid keep node101", + "osism baremetal clean --metadata-only node101", + "osism baremetal clean --metadata-only --raid recreate node101", + ]: + assert example in help_text + + +def test_clean_help_states_the_recreate_refusal_rule(): + """The named-node and --all rules differ in exit status, so both are documented.""" + help_text = _clean_parser().format_help() + assert "refuses" in help_text + assert "skips" in help_text + + def test_clean_manageable_without_raid_interface(): node = FakeNode(provision_state="manageable") conn = MagicMock() @@ -1452,8 +1550,12 @@ 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. + The kinds have to differ. Three identical RAID nodes would also pass with + the call hoisted back out of the node loop; here the node without a RAID + interface is what makes a hoist visible. Under the default mode a declared + and an undeclared RAID node do get the same steps, because the default + deletes and does not rebuild — the two are told apart by + ``test_clean_all_recreate_skips_nodes_with_nothing_to_build``. """ plain = FakeNode(id="uuid-1", name="node1", provision_state="manageable") raid_only = FakeNode( @@ -1477,11 +1579,7 @@ def test_clean_all_builds_the_step_list_per_node(): 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], - ), + call("uuid-3", "clean", clean_steps=[RAID_DELETE_STEP, ERASE_DEVICES_STEP]), ] @@ -1504,7 +1602,7 @@ def test_clean_metadata_only_with_raid_requested(): ) -def test_clean_no_raid_skips_the_raid_steps_on_a_full_clean(): +def test_clean_raid_keep_skips_the_raid_steps_on_a_full_clean(): node = FakeNode( provision_state="manageable", raid_interface="agent", @@ -1513,7 +1611,7 @@ def test_clean_no_raid_skips_the_raid_steps_on_a_full_clean(): conn = MagicMock() conn.baremetal.find_node.return_value = node - _run_baremetal_clean(["node1", "--no-raid"], conn) + _run_baremetal_clean(["node1", "--raid", "keep"], conn) conn.baremetal.set_node_provision_state.assert_called_once_with( node.id, "clean", clean_steps=[ERASE_DEVICES_STEP] @@ -1544,6 +1642,235 @@ def test_clean_raid_requested_on_a_full_clean(): ) +def test_clean_recreate_refuses_a_named_node_without_a_declaration(loguru_logs): + """``recreate`` delivers its outcome or refuses; it never degrades. + + The node is ``available`` on purpose: the refusal has to land before the + move to ``manageable``, not merely before the clean call. + """ + node = FakeNode( + provision_state="available", raid_interface="agent", target_raid_config=None + ) + conn = MagicMock() + conn.baremetal.find_node.return_value = node + + rc = _run_baremetal_clean(["node1", "--raid", "recreate"], conn) + + assert rc == 1 + conn.baremetal.set_node_provision_state.assert_not_called() + assert any( + "no target_raid_config" in record["message"] and record["level"] == "ERROR" + for record in loguru_logs + ) + + +def test_clean_recreate_refuses_a_named_node_without_a_raid_interface(loguru_logs): + """A named node is an assertion about that node, and this one is wrong.""" + node = FakeNode(provision_state="available", raid_interface="no-raid") + conn = MagicMock() + conn.baremetal.find_node.return_value = node + + rc = _run_baremetal_clean(["node1", "--raid", "recreate"], conn) + + assert rc == 1 + conn.baremetal.set_node_provision_state.assert_not_called() + assert any( + "no raid interface" in record["message"] and record["level"] == "ERROR" + for record in loguru_logs + ) + + +def test_clean_recreate_accepts_a_named_node_with_a_declaration(): + node = FakeNode( + provision_state="manageable", + raid_interface="agent", + target_raid_config={"logical_disks": [{"controller": "software"}]}, + ) + conn = MagicMock() + conn.baremetal.find_node.return_value = node + + rc = _run_baremetal_clean(["node1", "--raid", "recreate"], conn) + + assert rc is None + conn.baremetal.set_node_provision_state.assert_called_once_with( + node.id, + "clean", + clean_steps=[RAID_DELETE_STEP, ERASE_DEVICES_STEP, RAID_CREATE_STEP], + ) + + +def test_clean_all_recreate_skips_nodes_with_nothing_to_build(loguru_logs): + """A fleet is a discovered set, so one undeclared node does not abort it. + + A node without a raid interface is out of scope for the raid axis and is + cleaned with the erase step alone. A raid capable node with nothing + declared is misconfigured, so it is left untouched and reported, and the + run exits non-zero so automation sees the gap. + """ + plain = FakeNode(id="uuid-1", name="node1", provision_state="manageable") + undeclared = FakeNode( + id="uuid-2", + name="node2", + provision_state="manageable", + raid_interface="agent", + ) + 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, undeclared, declared] + + rc = _run_baremetal_clean( + ["--all", "--yes-i-really-really-mean-it", "--raid", "recreate"], conn + ) + + assert rc == 1 + assert conn.baremetal.set_node_provision_state.call_args_list == [ + call("uuid-1", "clean", clean_steps=[ERASE_DEVICES_STEP]), + call( + "uuid-3", + "clean", + clean_steps=[RAID_DELETE_STEP, ERASE_DEVICES_STEP, RAID_CREATE_STEP], + ), + ] + assert any("node2" in record["message"] for record in loguru_logs) + assert not any( + "node1" in record["message"] + for record in loguru_logs + if record["level"] == "ERROR" + ) + + +def test_clean_all_recreate_ignores_nodes_it_would_not_clean(loguru_logs): + """A node in an unsupported state is not a raid problem. + + Only ``available`` and ``manageable`` nodes are ever cleaned; the rest get + a warning and do not affect the exit code. Preflighting them would fail a + run over the raid configuration of a node that was never going to be + touched. + """ + active = FakeNode( + id="uuid-1", + name="node1", + provision_state="active", + raid_interface="agent", + ) + declared = FakeNode( + id="uuid-2", + name="node2", + provision_state="manageable", + raid_interface="agent", + target_raid_config={"logical_disks": [{"controller": "software"}]}, + ) + conn = MagicMock() + conn.baremetal.nodes.return_value = [active, declared] + + rc = _run_baremetal_clean( + ["--all", "--yes-i-really-really-mean-it", "--raid", "recreate"], conn + ) + + assert rc is None + conn.baremetal.set_node_provision_state.assert_called_once_with( + "uuid-2", + "clean", + clean_steps=[RAID_DELETE_STEP, ERASE_DEVICES_STEP, RAID_CREATE_STEP], + ) + assert not any(record["level"] == "ERROR" for record in loguru_logs) + + +def test_clean_recreate_does_not_refuse_a_named_node_in_a_bad_state(loguru_logs): + """The state is the operator's problem to hear about, not the raid mode.""" + node = FakeNode( + provision_state="active", raid_interface="agent", target_raid_config=None + ) + conn = MagicMock() + conn.baremetal.find_node.return_value = node + + rc = _run_baremetal_clean(["node1", "--raid", "recreate"], conn) + + assert rc is None + conn.baremetal.set_node_provision_state.assert_not_called() + assert any("not in supported state" in record["message"] for record in loguru_logs) + assert not any(record["level"] == "ERROR" for record in loguru_logs) + + +def test_clean_all_recreate_preflights_before_transitioning_any_node(): + """The skip list is known before the first node is touched.""" + available = FakeNode( + id="uuid-1", + name="node1", + provision_state="available", + raid_interface="agent", + target_raid_config={"logical_disks": [{"controller": "software"}]}, + ) + undeclared = FakeNode( + id="uuid-2", + name="node2", + provision_state="available", + raid_interface="agent", + ) + conn = MagicMock() + conn.baremetal.nodes.return_value = [available, undeclared] + conn.baremetal.set_node_provision_state.return_value = available + conn.baremetal.wait_for_nodes_provision_state.return_value = [ + FakeNode( + id="uuid-1", + name="node1", + provision_state="manageable", + raid_interface="agent", + target_raid_config={"logical_disks": [{"controller": "software"}]}, + ) + ] + + rc = _run_baremetal_clean( + ["--all", "--yes-i-really-really-mean-it", "--raid", "recreate"], conn + ) + + assert rc == 1 + transitioned = [ + args[0] for args, _ in conn.baremetal.set_node_provision_state.call_args_list + ] + assert "uuid-2" not in transitioned + + +def test_clean_all_recreate_exits_zero_when_every_node_can_be_built(): + declared = FakeNode( + id="uuid-1", + name="node1", + provision_state="manageable", + raid_interface="agent", + target_raid_config={"logical_disks": [{"controller": "software"}]}, + ) + conn = MagicMock() + conn.baremetal.nodes.return_value = [declared] + + rc = _run_baremetal_clean( + ["--all", "--yes-i-really-really-mean-it", "--raid", "recreate"], conn + ) + + assert rc is None + + +def test_clean_delete_mode_does_not_preflight_the_declaration(loguru_logs): + """Only ``recreate`` promises to build something.""" + node = FakeNode( + provision_state="manageable", raid_interface="agent", target_raid_config=None + ) + conn = MagicMock() + conn.baremetal.find_node.return_value = node + + rc = _run_baremetal_clean(["node1", "--raid", "delete"], conn) + + assert rc is None + conn.baremetal.set_node_provision_state.assert_called_once_with( + node.id, "clean", clean_steps=[RAID_DELETE_STEP, ERASE_DEVICES_STEP] + ) + + def test_clean_available_node_moved_to_manageable_first(loguru_logs): available = FakeNode(provision_state="available") manageable = FakeNode(provision_state="manageable")