feat(storcli2): logical volume drive add (expand) and remove by g-carre · Pull Request #70 · scality/raidmgmt

g-carre · 2026-06-25T09:11:50Z

Summary

Completes ports.LogicalVolumesManager on the storcli2/perccli2 manager (ARTESCA-17648). Stacked on #69.

storcli2 dropped storcli's start migrate command (per DESIGN.md, verified against the StorCLI2 User Guide and the official storcli-to-storcli2 command map), so the two drive-membership operations split:

AddPDsToLV — online capacity expansion via /cx/vx expand drives=e:s,.... The firmware preserves the RAID level; drives spanning multiple enclosures are rejected.
DeletePDsFromLV — returns ports.ErrFunctionNotSupportedByImplementation: there is no storcli2 replacement for start migrate option=remove.

Replaces the stale plain-text migrate/fail.json fixture (a unexpected TOKEN_MIGRATE syntax error proving start migrate no longer parses) with proper expand/{success,fail}.json fixtures, and updates the collection script and testdata README.

Testing

go build, go vet, gofmt, and the package test suite pass. New tests cover the expand happy path, command-failure, multi-enclosure rejection, and the unsupported-removal error.

Note: This PR targets feature/storcli2-lv-manager-create-delete (its base, #69), not main. The diff will narrow once #69 merges.

Issue: ARTESCA-17648

🤖 Generated with Claude Code

claude · 2026-06-30T12:35:52Z

collect_storcli2_testdata.sh:718 — The DESTRUCTIVE-skip echo message still references the deleted migrate/fail.json. Should be updated to expand/{success,fail}.json to match the renamed fixtures and the updated run_and_save call.

Review by Claude Code

claude · 2026-06-30T13:29:38Z

LGTM — AddPDsToLV and DeletePDsFromLV are clean: error wrapping is consistent, the compile-time interface check is a good addition, tests cover the happy path, command failure, multi-enclosure rejection, and the unsupported-removal sentinel. Fixtures and collection script are updated correctly.

Review by Claude Code

claude · 2026-06-30T14:42:40Z

-        "${C}/v1" start migrate type=raid0 option=add "drives=${FIRST_ENCLOSURE}:${INVALID_SLOT}"
+        "Expand VD (expected failure)" \
+        "${OUTPUT_DIR}/logicalvolumemanager/testdata/storcli2/expand/fail.json" \
+        "${C}/v1" expand "drives=${FIRST_ENCLOSURE}:${INVALID_SLOT}"


The collection step here was updated from migrate to expand, but the DESTRUCTIVE=false skip-list echo at line 718 still references the deleted migrate/fail.json path. It should be updated to expand/{success,fail}.json to stay in sync.

Also, expand/success.json is listed in the README fixture table as a script-produced file, but no collection step produces it (it would need a second free drive in the sacrifice-VD workflow). Consider either adding a collection step or noting it as hand-crafted.

— Claude Code

claude · 2026-06-30T14:42:47Z

collect_storcli2_testdata.sh:361 / :718 — The collection step was updated from migrate to expand, but the DESTRUCTIVE=false skip-list echo at line 718 still references the deleted migrate/fail.json. Also, expand/success.json is listed in the README fixture table but is not collected by the script.

The implementation code (AddPDsToLV, DeletePDsFromLV), interface compliance check, fixtures, and tests all look correct.

Review by Claude Code

claude · 2026-06-30T15:37:19Z

collect_storcli2_testdata.sh:127 — mkdir still creates the migrate directory, but the collection command (line 360) now writes to expand/fail.json. The expand directory won't exist and the write will fail. Change to: mkdir -p "${OUTPUT_DIR}/logicalvolumemanager/testdata/storcli2/expand"
- collect_storcli2_testdata.sh:718 — SAFE-mode skip message still references migrate/fail.json instead of expand/{success,fail}.json. Stale after the rename.

The Go implementation and tests look correct — error wrapping, interface compliance, sentinel error for unsupported removal, and fixture coverage are all solid.

Review by Claude Code

Complete ports.LogicalVolumesManager on the storcli2/perccli2 manager: - AddPDsToLV grows a volume through "expand drives=e:s,..." (online capacity expansion). storcli2 dropped "start migrate", so expansion is the only supported growth path; the firmware preserves the RAID level. Drives spanning multiple enclosures are rejected. - DeletePDsFromLV returns ErrFunctionNotSupportedByImplementation: the storcli-to-storcli2 command map drops "start migrate" with no replacement for removing drives from a volume (see DESIGN.md). Replace the stale plain-text migrate/fail.json fixture (captured with the v1 grammar that storcli2 rejects) with proper expand/{success,fail}.json fixtures, and update the collection script and testdata README accordingly. Issue: ARTESCA-17648

revive's unused-receiver rule flagged the named 's' receiver, which the not-supported stub never references. Issue: ARTESCA-17648

claude · 2026-06-30T16:40:45Z

collect_storcli2_testdata.sh:718 — The "skipped" echo still references the deleted logicalvolumemanager/testdata/storcli2/migrate/fail.json. Should be updated to expand/fail.json (and optionally expand/success.json) to match the fixtures this PR introduces.
collect_storcli2_testdata.sh — The script collects expand/fail.json (line 360) but never collects expand/success.json, while the README table documents both expand/{success,fail}.json as script-produced fixtures. Either add a collection step for the success case (requires a free drive + existing VD simultaneously, which may be hard to arrange in the sacrifice workflow) or add a comment noting the success fixture is hand-crafted.

Review by Claude Code

Implement ports.Blinker on the storcli2/perccli2 adapter: - StartBlink / StopBlink issue "/cx/ex/sx start locate" / "stop locate" (same grammar as storcli), choosing the enclosure or no-enclosure selector form from the drive's parsed slot, and surface the in-JSON failure payload via storcli2.Decode. A single implementation serves both binaries; the runner is injected. Issue: ARTESCA-17650

ezekiel-alexrod

We are mixing in the test file table-driven tests (e.g., DeleteLV) and one function per test case, the classical approach. Minor

Otherwise, LGTM

feat(storcli2): physical drive blinker

claude · 2026-07-01T15:44:31Z

+	err := b.StopBlink(storcli2PDMetadata(""))
+	require.Error(t, err)
+	mockRunner.AssertNotCalled(t, "Run")
+}


Missing test for the storcli2.Decode failure path in locate(). The runner-error case (TestStorCLI2BlinkCommandError) and the invalid-slot case are covered, but there is no test where the runner succeeds and the JSON payload contains "Status": "Failure" — i.e. the storcli2.Decode error branch at line 77 of storcli2.go. The logicalvolumemanager tests cover this pattern for expand (TestStorCLI2AddPDsToLVCommandError with expand/fail.json); the blinker should have an analogous fixture (e.g. testdata/storcli2/start_fail.json) and test.

— Claude Code

claude · 2026-07-01T15:44:37Z

pkg/implementation/blinker/storcli2_test.go:111 — Missing test for the storcli2.Decode in-JSON failure path in locate(). The runner-error and invalid-slot cases are covered, but no test exercises the branch where the runner succeeds and the JSON response contains a failure status. Add a failure fixture and test, matching the pattern used by TestStorCLI2AddPDsToLVCommandError in the logicalvolumemanager package.

Review by Claude Code

g-carre requested a review from a team as a code owner June 25, 2026 09:11

g-carre mentioned this pull request Jun 25, 2026

feat(storcli2): physical drive blinker #72

Merged

g-carre force-pushed the feature/storcli2-lv-manager-create-delete branch from e5ba890 to 063bd83 Compare June 30, 2026 09:26

ezekiel-alexrod mentioned this pull request Jun 30, 2026

feat(storcli2): logical volume create and delete #69

Merged

g-carre force-pushed the feature/storcli2-lv-manager-pd-membership branch from d468745 to e66400a Compare June 30, 2026 12:33

g-carre force-pushed the feature/storcli2-lv-manager-create-delete branch from 063bd83 to 7de356c Compare June 30, 2026 13:26

g-carre force-pushed the feature/storcli2-lv-manager-pd-membership branch from e66400a to 2c5ab13 Compare June 30, 2026 13:26

g-carre force-pushed the feature/storcli2-lv-manager-create-delete branch from 7de356c to ddc8be4 Compare June 30, 2026 14:38

g-carre force-pushed the feature/storcli2-lv-manager-pd-membership branch from 2c5ab13 to 0c6c272 Compare June 30, 2026 14:39

claude Bot reviewed Jun 30, 2026

View reviewed changes

g-carre force-pushed the feature/storcli2-lv-manager-create-delete branch from ddc8be4 to 13d1232 Compare June 30, 2026 15:31

g-carre force-pushed the feature/storcli2-lv-manager-pd-membership branch from 0c6c272 to 2131206 Compare June 30, 2026 15:35

g-carre force-pushed the feature/storcli2-lv-manager-create-delete branch from e2a38b2 to cae6f00 Compare June 30, 2026 16:35

g-carre added 2 commits June 30, 2026 18:35

fix(storcli2): drop unused receiver on DeletePDsFromLV

70f0661

revive's unused-receiver rule flagged the named 's' receiver, which the not-supported stub never references. Issue: ARTESCA-17648

g-carre force-pushed the feature/storcli2-lv-manager-pd-membership branch from 2131206 to 70f0661 Compare June 30, 2026 16:37

ezekiel-alexrod approved these changes Jul 1, 2026

View reviewed changes

Merge pull request #72 from scality/feature/storcli2-blinker

eb0c35f

feat(storcli2): physical drive blinker

claude Bot reviewed Jul 1, 2026

View reviewed changes

g-carre merged commit ebfb743 into feature/storcli2-lv-manager-create-delete Jul 1, 2026
7 checks passed

g-carre deleted the feature/storcli2-lv-manager-pd-membership branch July 1, 2026 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(storcli2): logical volume drive add (expand) and remove#70

feat(storcli2): logical volume drive add (expand) and remove#70
g-carre merged 4 commits into
feature/storcli2-lv-manager-create-deletefrom
feature/storcli2-lv-manager-pd-membership

g-carre commented Jun 25, 2026

Uh oh!

claude Bot commented Jun 30, 2026

Uh oh!

claude Bot commented Jun 30, 2026

Uh oh!

claude Bot Jun 30, 2026

Uh oh!

claude Bot commented Jun 30, 2026

Uh oh!

claude Bot commented Jun 30, 2026

Uh oh!

claude Bot commented Jun 30, 2026

Uh oh!

ezekiel-alexrod left a comment

Uh oh!

claude Bot Jul 1, 2026

Uh oh!

claude Bot commented Jul 1, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

g-carre commented Jun 25, 2026

Summary

Testing

Uh oh!

claude Bot commented Jun 30, 2026

Uh oh!

claude Bot commented Jun 30, 2026

Uh oh!

claude Bot Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Jun 30, 2026

Uh oh!

claude Bot commented Jun 30, 2026

Uh oh!

claude Bot commented Jun 30, 2026

Uh oh!

ezekiel-alexrod left a comment

Choose a reason for hiding this comment

Uh oh!

claude Bot Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Jul 1, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants