baremetal: create the raid configuration during clean, per node - #2609
baremetal: create the raid configuration during clean, per node#2609kingspeedy95 wants to merge 1 commit into
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/unit/commands/test_baremetal.py" line_range="283" />
<code_context>
)
+# --- _build_clean_steps ---
+
+
</code_context>
<issue_to_address>
**issue (testing):** There are no automated tests covering the new `--raid` CLI flag wiring into `take_action`; consider adding them.
The current tests cover `_build_clean_steps`, but not the new `--raid` flag wiring into `take_action`. Please add tests that build the parser, exercise `--raid`, `--no-raid`, and the default (no flag), and assert that `take_action` calls `_build_clean_steps` with the correct `raid` value in each case, matching the documented CLI behaviour (default on for full clean, off for `--metadata-only`, with explicit overrides).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ) | ||
|
|
||
|
|
||
| # --- _build_clean_steps --- |
There was a problem hiding this comment.
issue (testing): There are no automated tests covering the new --raid CLI flag wiring into take_action; consider adding them.
The current tests cover _build_clean_steps, but not the new --raid flag wiring into take_action. Please add tests that build the parser, exercise --raid, --no-raid, and the default (no flag), and assert that take_action calls _build_clean_steps with the correct raid value in each case, matching the documented CLI behaviour (default on for full clean, off for --metadata-only, with explicit overrides).
`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 <timon.schnell@btc-it-services.com> Assisted-by: Claude:claude-opus-5
724f673 to
a5f183d
Compare
ideaship
left a comment
ideaship
left a comment
There was a problem hiding this comment.
Thanks — the accumulation fix is right, and extracting _build_clean_steps is the shape that keeps it from coming back. The step order matches Ironic's software-RAID docs, and the frozen-ATA case behind --metadata-only --raid is real: ironic-python-agent raises on a frozen drive, so there's no working in-band full-erase path on that hardware.
One request, inline — the regression test can't detect the regression, and the new behaviour has no coverage above the helper.
Nothing to change here for the following, it's ours to handle: we plan to give --raid an optional value — --raid delete|keep|recreate — because "leave the existing array alone" and "tear it down without rebuilding" are different outcomes one boolean can't express, and to put the create step behind an explicit choice rather than the default. Bare --raid would keep meaning exactly what it means in your patch, so --metadata-only --raid stays valid for your pod. Two things would change: --no-raid becomes --raid keep, and a plain clean on a node with a target_raid_config goes back to delete + erase without the rebuild. We'll do that work and any compatibility handling separately.
| 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 |
There was a problem hiding this comment.
This test can't detect the regression it is named for. _build_clean_steps returns fresh list literals with no module state or mutable default, so calling it independently per node doesn't exercise the accumulation, which happened in take_action's node loop. Hoisting the helper call back out of that loop would leave this green.
The behaviour the PR adds also isn't reached at command level: FakeNode defaults target_raid_config to None (:246), so none of the clean tests from :1415 on exercise create_configuration, --raid or --no-raid.
Could you drop this test and add command-level ones next to those, via _run_baremetal_clean? For the loop, one --all run over three manageable nodes — raid_interface="no-raid", agent without a declaration, agent with one — asserting the full ordered clean_steps of every call in set_node_provision_state.call_args_list. The three kinds are the point: identical RAID nodes would also pass if the call were hoisted out of the loop. Then a single-node case each for --raid and --no-raid. Happy to sketch it if that's easier.
github-project-automation
Bot
moved this from Ready for review
to In review
in Human Board
4 participants
The gap
osism baremetal cleanalready prepends the raid clean stepdelete_configurationwhen a node has a raid interface:but nothing ever adds
create_configuration. Two consequences: a node that had a software mirror loses it on the first full clean and never gets it back, and a fleet whosetarget_raid_configwas just declared cannot build its arrays with the OSISM CLI at all.That is not an omission that can be worked around elsewhere. Ironic exposes
create_configurationonly as a clean step with priority 0, seeAgentRAIDin 2025.1, so it has to be requested explicitly. The deploy step variant isapply_configuration, which takes the configuration as a step argument through deploy templates rather than fromtarget_raid_config.The change
The step list now comes from
_build_clean_steps(node, metadata_only, raid), which producesdelete_configuration, the erase step,create_configuration. That is the order the Ironic documentation prescribes for software RAID: create does not remove existing disks and "Building RAID will fail if the target disks are already partitioned", so delete and erase have to run first.create_configurationis only added when the node actually carries atarget_raid_config, so a raid capable node without a declaration keeps exactly its previous behaviour.It also fixes an accumulation bug. The list used to be built once before the node loop and prepended to inside it, so with
--allthe second raid capable node got twodelete_configurationsteps, the third got three, and so on. Building per node removes that by construction, and there is a regression test.--raid/--no-raidmake the choice explicit, defaulting to today's behaviour: raid steps on a full clean, none on--metadata-only. The new part is that--metadata-only --raidbecomes expressible.Why that last combination matters
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_deviceshas no working path there at all, while the OS mirror on the NVMe pair still has to be built. Today that fleet has to choose between a clean that fails and a clean that silently skips the raid steps.Tested
tests/unit/commands/test_baremetal.py: 131 to 138 passing, with cases for a node without a raid interface, a raid capable node with and without a declaration, both--metadata-onlyvariants,--no-raidon a full clean, and the accumulation regression.--raid/--no-raid/ neither /--metadata-only --raidverified to parse as intended.tests/unitin my environment are missing optional dependencies and are identical with and without this change, checked by stashing.Not tested by me: a real clean against hardware. The fleet this came from is about to be rebuilt and I can report back once the arrays are built there.