Skip to content

baremetal: give clean --raid three named modes - #2633

Merged
berendt merged 2 commits into
mainfrom
baremetal-raid-flag-shape
Aug 27, 2026
Merged

baremetal: give clean --raid three named modes#2633
berendt merged 2 commits into
mainfrom
baremetal-raid-flag-shape

Conversation

@ideaship

@ideaship ideaship commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Follow-up to the RAID clean work, and release-coupled to it:

That PR added --raid / --no-raid as booleans. This replaces them with three named
modes before either reaches a release, so no operator ever sees the boolean form and
removing --no-raid costs nothing. If a release ships in between, --no-raid becomes a
released flag on a destructive command and has to be carried as a deprecated alias
instead.

Why a boolean is not enough

"Leave the existing array alone" and "tear it down without rebuilding" are different
outcomes, and one flag cannot express both. --no-raid suppressed the delete step as
well, which made it less destructive than the pre-change default rather than equal
to it — so the pre-#2609 sequence (delete, erase, leave the disks bare) became
unreachable for exactly the nodes it used to apply to.

The modes

--raid steps added for a RAID-capable node
delete delete_configuration
keep none; the erase runs through the existing array
recreate delete_configuration + create_configuration

The value is optional and bare --raid still means recreate, so
clean --metadata-only --raid keeps working.

The default resolves to delete on a full clean and keep with --metadata-only. That
tracks the erase depth in one sentence and restores the behaviour the command had before
the create step existed. create_configuration now needs an explicit
--raid recreate
, which is the substantive change: a target_raid_config can be
declared fleet-wide from a single 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.

Migration

was becomes same steps?
clean (node with a declaration) clean --raid recreate no, create is no longer implied
clean --raid unchanged yes
clean --metadata-only --raid unchanged yes
clean --no-raid clean --raid keep yes

recreate delivers or refuses

It never degrades to delete, which would be the destructive opposite of what was
asked for. Upstream shows why this is worth a preflight rather than a warning:
AgentRAID.create_configuration calls raid.filter_target_raid_config, which raises
MissingParameterValue on an empty target_raid_config; the conductor's
cleaning_error_handler then puts the node in CLEANFAIL and sets maintenance mode.
Because create runs last, a node passed through would be erased first and parked in
maintenance afterwards. Nothing gates it earlier — step validation only checks argument
names, and both of this step's arguments are optional.

invocation no-raid node RAID-capable, no declaration
named node refuse, exit 1, no state change refuse, exit 1, no state change
--all cleaned with the erase step alone, silent skipped and reported, exit non-zero

Refusals land before any provision-state transition, not merely before the clean call —
an available node is otherwise moved to manageable and waited for first. Nodes in a
state the loop would not clean anyway are left out of the preflight, so an active
undeclared node cannot fail a run over a node that was never going to be touched.

--all skips rather than aborts on purpose. "RAID-capable but undeclared" is a
legitimate state — it is what delete and keep do today — and clean has no node
selection, so aborting would strand an entire fleet on one such node with no way to
exclude it.

One rough edge, documented rather than fixed

An optional value competes with the optional positional: argparse claims the next
non-- token, so clean --raid node101 is rejected. It cannot be fixed while --raid
has both an optional value and an optional positional beside it, and a required value
would break bare --raid. The error names the two spellings that work instead of
leaving argparse's invalid choice: 'node101'.

Testing

tests/unit/commands/test_baremetal.py goes 140 → 168; full unit suite 3176 passed, 4
xfailed (pre-existing, unrelated).

The first commit is a test on its own, added before the behaviour changed while it
was still a duplicate assertion. create_configuration on a full clean was previously
asserted only by the default path, and this change edits exactly that expectation, so
bare --raid would otherwise have lost its only witness.

Every guard was mutation-checked — removing the preflight, dropping the named no-raid
refusal, mapping bare --raid to delete, restoring the old default, aborting instead
of skipping, letting recreate degrade, dropping the loop skip, dropping the preflight
state filter, and dropping the ordering error each fail the intended test and no others.

Docs

The osism.github.io pages documented none of these flags. Covered in:

Draft until both are reviewed. They should land together — and in the same release as
#2609, per the flag-shape coupling above.

`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 <luethi@osism.tech>
@ideaship ideaship self-assigned this Aug 27, 2026
@ideaship
ideaship marked this pull request as ready for review August 27, 2026 15:11

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 1 issue

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="osism/commands/baremetal.py" line_range="1286" />
<code_context>
+              wipe a node completely, leaving no array behind
+                osism baremetal clean node101
+
+              recycle a node into the pool with its array intact
+                osism baremetal clean --raid recreate node101
+
+              wipe the data but leave the existing array in place
</code_context>
<issue_to_address>
**nitpick:** The help example describes `--raid recreate` as recycling a node with its array intact, but that mode explicitly deletes the existing RAID configuration before creating a new one, so the example misleads operators about the destructive operation it performs.

**Suggested fix:** Describe the example as deleting and rebuilding the declared array, or use `--raid keep` for an array-preserving recycle.

```suggestion
              delete and rebuild the declared array
```
</issue_to_address>

Sourcery assessment

Needs a human reviewer. The new RAID modes can delete RAID configurations and erase disk data, and a mistaken recreate or keep operation can leave selected bare-metal nodes with irreversible data loss or a changed storage layout. Reverting the code would not restore deleted data or the prior RAID state, although the affected scope is limited to nodes on which the command is run.


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread osism/commands/baremetal.py Outdated
`--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 <luethi@osism.tech>
@ideaship
ideaship force-pushed the baremetal-raid-flag-shape branch from 8497fb5 to 1b9afcf Compare August 27, 2026 15:22
@ideaship ideaship moved this from New to Ready for review in Human Board Aug 27, 2026
@berendt
berendt merged commit a5abdea into main Aug 27, 2026
3 checks passed
@berendt
berendt deleted the baremetal-raid-flag-shape branch August 27, 2026 20:01
@github-project-automation github-project-automation Bot moved this from Ready for review to Done in Human Board Aug 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants