From 9c6672661cdd4943d69d9f4d7551966cbe797094 Mon Sep 17 00:00:00 2001 From: Guillaume Carre Date: Thu, 25 Jun 2026 11:04:46 +0200 Subject: [PATCH 1/3] feat(storcli2): logical volume drive add (expand) and remove 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 --- .../logicalvolumemanager/storcli2.go | 43 +++++++++ .../logicalvolumemanager/storcli2_test.go | 90 +++++++++++++++++++ .../testdata/storcli2/expand/fail.json | 22 +++++ .../testdata/storcli2/expand/success.json | 13 +++ .../testdata/storcli2/migrate/fail.json | 5 -- tests/testdata-tools/README.md | 4 +- .../collect_storcli2_testdata.sh | 13 +-- 7 files changed, 176 insertions(+), 14 deletions(-) create mode 100644 pkg/implementation/logicalvolumemanager/testdata/storcli2/expand/fail.json create mode 100644 pkg/implementation/logicalvolumemanager/testdata/storcli2/expand/success.json delete mode 100644 pkg/implementation/logicalvolumemanager/testdata/storcli2/migrate/fail.json diff --git a/pkg/implementation/logicalvolumemanager/storcli2.go b/pkg/implementation/logicalvolumemanager/storcli2.go index 857902d..186d5cc 100644 --- a/pkg/implementation/logicalvolumemanager/storcli2.go +++ b/pkg/implementation/logicalvolumemanager/storcli2.go @@ -20,6 +20,10 @@ const ( storcli2CmdVD = "vd" // storcli2CmdDelete deletes the virtual drive addressed by the selector. storcli2CmdDelete = "delete" + // storcli2CmdExpand adds physical drives to a virtual drive (online capacity + // expansion). storcli2 dropped "start migrate", so this is the only way to + // grow a volume; the RAID level is preserved by the firmware. + storcli2CmdExpand = "expand" // storcli2ControllerSelector addresses a whole controller (used by "add vd"). storcli2ControllerSelector = "/c%d" // storcli2VolumeSelector addresses a single virtual drive by its number. @@ -43,6 +47,8 @@ type StorCLI2 struct { runner commandrunner.CommandRunner } +var _ ports.LogicalVolumesManager = &StorCLI2{} + // NewStorCLI2 returns a logical-volume manager backed by the given storcli2 / // perccli2 command runner and physical-drive and logical-volume getters. func NewStorCLI2( @@ -132,6 +138,43 @@ func (s *StorCLI2) DeleteLV(metadata *logicalvolume.Metadata) error { return nil } +// AddPDsToLV grows a logical volume with the given physical drives through +// "expand" (online capacity expansion). storcli2 dropped "start migrate", so +// expansion is the only supported path; the RAID level is preserved by the +// firmware. The drives must share a single enclosure. +func (s *StorCLI2) AddPDsToLV( + lvMetadata *logicalvolume.Metadata, + pdsMetadata ...*physicaldrive.Metadata, +) error { + drives, err := storcli2FormatDrives(pdsMetadata) + if err != nil { + return errors.Wrap(err, "failed to format drives") + } + + selector := fmt.Sprintf(storcli2VolumeSelector, lvMetadata.CtrlMetadata.ID, lvMetadata.ID) + + output, err := s.runner.Run([]string{selector, storcli2CmdExpand, drives}) + if err != nil { + return errors.Wrapf(err, "failed to run expand command for logical volume %s", lvMetadata.ID) + } + + if _, err := storcli2.Decode(output); err != nil { + return errors.Wrapf(err, "failed to expand logical volume %s", lvMetadata.ID) + } + + return nil +} + +// DeletePDsFromLV is not supported by storcli2: the storcli-to-storcli2 command +// map drops "start migrate" with no replacement for removing drives from a +// volume (see DESIGN.md). +func (s *StorCLI2) DeletePDsFromLV( + _ *logicalvolume.Metadata, + _ ...*physicaldrive.Metadata, +) error { + return ports.ErrFunctionNotSupportedByImplementation +} + // findNewLogicalVolume returns the volume whose physical-drive set is exactly // the request's. A physical drive belongs to a single virtual drive, so the // freshly created volume is the only match. diff --git a/pkg/implementation/logicalvolumemanager/storcli2_test.go b/pkg/implementation/logicalvolumemanager/storcli2_test.go index a59b236..67bbfa7 100644 --- a/pkg/implementation/logicalvolumemanager/storcli2_test.go +++ b/pkg/implementation/logicalvolumemanager/storcli2_test.go @@ -9,6 +9,7 @@ import ( "github.com/scality/raidmgmt/pkg/domain/entities/logicalvolume" "github.com/scality/raidmgmt/pkg/domain/entities/physicaldrive" "github.com/scality/raidmgmt/pkg/domain/entities/raidcontroller" + "github.com/scality/raidmgmt/pkg/domain/ports" "github.com/scality/raidmgmt/pkg/implementation/logicalvolumemanager" ) @@ -211,3 +212,92 @@ func TestStorCLI2DeleteLV(t *testing.T) { }) } } + +// TestStorCLI2AddPDsToLV covers the expand happy path: the drives are formatted +// into a single-enclosure list and submitted through "expand". +func TestStorCLI2AddPDsToLV(t *testing.T) { + t.Parallel() + + ctrl := storcli2Ctrl() + metadata := &logicalvolume.Metadata{CtrlMetadata: ctrl, ID: "25"} + pds := []*physicaldrive.Metadata{ + {CtrlMetadata: ctrl, ID: "252:3"}, + {CtrlMetadata: ctrl, ID: "252:4"}, + } + + mockRunner := new(MockCommandRunner) + mockRunner.On("Run", []string{"/c0/v25", "expand", "drives=252:3,4"}). + Return(storcli2Fixture(t, "expand/success.json"), nil) + + manager := logicalvolumemanager.NewStorCLI2( + mockRunner, new(MockPhysicalDrivesGetter), new(MockLogicalVolumesGetter), + ) + + err := manager.AddPDsToLV(metadata, pds...) + require.NoError(t, err) + mockRunner.AssertExpectations(t) +} + +// TestStorCLI2AddPDsToLVCommandError pins that an "expand" failure payload is +// surfaced. +func TestStorCLI2AddPDsToLVCommandError(t *testing.T) { + t.Parallel() + + ctrl := storcli2Ctrl() + metadata := &logicalvolume.Metadata{CtrlMetadata: ctrl, ID: "25"} + pd := &physicaldrive.Metadata{CtrlMetadata: ctrl, ID: "252:3"} + + mockRunner := new(MockCommandRunner) + mockRunner.On("Run", []string{"/c0/v25", "expand", "drives=252:3"}). + Return(storcli2Fixture(t, "expand/fail.json"), nil) + + manager := logicalvolumemanager.NewStorCLI2( + mockRunner, new(MockPhysicalDrivesGetter), new(MockLogicalVolumesGetter), + ) + + err := manager.AddPDsToLV(metadata, pd) + require.Error(t, err) +} + +// TestStorCLI2AddPDsToLVMultipleEnclosures pins that drives spanning two +// enclosures are rejected before "expand" is run. +func TestStorCLI2AddPDsToLVMultipleEnclosures(t *testing.T) { + t.Parallel() + + ctrl := storcli2Ctrl() + metadata := &logicalvolume.Metadata{CtrlMetadata: ctrl, ID: "25"} + pds := []*physicaldrive.Metadata{ + {CtrlMetadata: ctrl, ID: "252:3"}, + {CtrlMetadata: ctrl, ID: "253:4"}, + } + + mockRunner := new(MockCommandRunner) + + manager := logicalvolumemanager.NewStorCLI2( + mockRunner, new(MockPhysicalDrivesGetter), new(MockLogicalVolumesGetter), + ) + + err := manager.AddPDsToLV(metadata, pds...) + require.Error(t, err) + mockRunner.AssertNotCalled(t, "Run") +} + +// TestStorCLI2DeletePDsFromLV pins that drive removal is reported as unsupported: +// storcli2 has no replacement for storcli's "start migrate option=remove". +func TestStorCLI2DeletePDsFromLV(t *testing.T) { + t.Parallel() + + ctrl := storcli2Ctrl() + metadata := &logicalvolume.Metadata{CtrlMetadata: ctrl, ID: "25"} + pd := &physicaldrive.Metadata{CtrlMetadata: ctrl, ID: "252:3"} + + mockRunner := new(MockCommandRunner) + + manager := logicalvolumemanager.NewStorCLI2( + mockRunner, new(MockPhysicalDrivesGetter), new(MockLogicalVolumesGetter), + ) + + err := manager.DeletePDsFromLV(metadata, pd) + require.ErrorIs(t, err, ports.ErrFunctionNotSupportedByImplementation) + mockRunner.AssertNotCalled(t, "Run") +} diff --git a/pkg/implementation/logicalvolumemanager/testdata/storcli2/expand/fail.json b/pkg/implementation/logicalvolumemanager/testdata/storcli2/expand/fail.json new file mode 100644 index 0000000..0b7f2e3 --- /dev/null +++ b/pkg/implementation/logicalvolumemanager/testdata/storcli2/expand/fail.json @@ -0,0 +1,22 @@ +{ +"Controllers":[ +{ + "Command Status" : { + "CLI Version" : "008.0005.0000.0010 Feb 22, 2023", + "Operating system" : "Linux4.18.0-553.115.1.el8_10.x86_64", + "Controller" : "0", + "Status" : "Failure", + "Description" : "expand VD failed", + "Detailed Status" : [ + { + "VD" : 25, + "Status" : "Failed", + "ErrType" : "CLI", + "ErrCd" : 255, + "ErrMsg" : "Expansion not supported on this VD" + } + ] + } +} +] +} diff --git a/pkg/implementation/logicalvolumemanager/testdata/storcli2/expand/success.json b/pkg/implementation/logicalvolumemanager/testdata/storcli2/expand/success.json new file mode 100644 index 0000000..94fa7a6 --- /dev/null +++ b/pkg/implementation/logicalvolumemanager/testdata/storcli2/expand/success.json @@ -0,0 +1,13 @@ +{ +"Controllers":[ +{ + "Command Status" : { + "CLI Version" : "008.0005.0000.0010 Feb 22, 2023", + "Operating system" : "Linux4.18.0-553.115.1.el8_10.x86_64", + "Controller" : "0", + "Status" : "Success", + "Description" : "expand VD succeeded" + } +} +] +} diff --git a/pkg/implementation/logicalvolumemanager/testdata/storcli2/migrate/fail.json b/pkg/implementation/logicalvolumemanager/testdata/storcli2/migrate/fail.json deleted file mode 100644 index ffa24c5..0000000 --- a/pkg/implementation/logicalvolumemanager/testdata/storcli2/migrate/fail.json +++ /dev/null @@ -1,5 +0,0 @@ -CLI Version = 008.0005.0000.0010 Feb 22, 2023 -Operating system = Linux4.18.0-553.115.1.el8_10.x86_64 -Status = Failure -Description = syntax error, unexpected TOKEN_MIGRATE, expecting TOKEN_ERASE or TOKEN_INIT or TOKEN_CC or TOKEN_LOCATE - diff --git a/tests/testdata-tools/README.md b/tests/testdata-tools/README.md index 0d98f09..709b46a 100644 --- a/tests/testdata-tools/README.md +++ b/tests/testdata-tools/README.md @@ -52,7 +52,7 @@ The script writes each fixture under its owning component package's | `logicalvolumegetter` | `testdata/storcli2/show/v999_invalid.json` | VD not found | safe | | `logicalvolumemanager` | `testdata/storcli2/create/{success,fail}.json` | `add vd ...` | destructive | | `logicalvolumemanager` | `testdata/storcli2/delete/{success,fail_invalid,fail_vdNotExist}.json` | `delete` | destructive | -| `logicalvolumemanager` | `testdata/storcli2/migrate/fail.json` | `start migrate ...` | destructive | +| `logicalvolumemanager` | `testdata/storcli2/expand/{success,fail}.json` | `expand drives=...` | destructive | | `lvcachesetter` | `testdata/storcli2/cacheoptions/success_{wrcache,rdcache}.json` | `set wrcache/rdcache` | destructive | | `lvcachesetter` | `testdata/storcli2/cacheoptions/combined_syntax_error.json` | v1 combined `set` syntax (rejected) | destructive | | `blinker` | `testdata/storcli2/{start,stop}.json` | `start locate` / `stop locate` | destructive | @@ -68,8 +68,6 @@ storcli v1 (the script still uses the v1 syntax): - `lvcachesetter/testdata/storcli2/cacheoptions/combined_syntax_error.json` (`unexpected TOKEN_WRITE_CACHE`) -- `logicalvolumemanager/testdata/storcli2/migrate/fail.json` - (`unexpected TOKEN_MIGRATE`) - `jbodsetter/testdata/storcli2/jbod/disable/fail.json` (`unexpected TOKEN_JBOD`) diff --git a/tests/testdata-tools/collect_storcli2_testdata.sh b/tests/testdata-tools/collect_storcli2_testdata.sh index a387a40..742770e 100755 --- a/tests/testdata-tools/collect_storcli2_testdata.sh +++ b/tests/testdata-tools/collect_storcli2_testdata.sh @@ -351,13 +351,14 @@ if [ "${DESTRUCTIVE}" = "true" ]; then "${OUTPUT_DIR}/logicalvolumemanager/testdata/storcli2/delete/fail_vdNotExist.json" \ "${C}/v${INVALID_VD_ID}" delete - # Migrate - failure case (operation not supported/possible) - # Command: storcli2 /c0/v1 start migrate type=raid0 option=add drives=306:99 J - # Used by: adapter.migrate() error case + # Expand - failure case (invalid drive). storcli2 dropped "start migrate"; + # AddPDsToLV goes through "expand". + # Command: storcli2 /c0/v1 expand drives=306:99 J + # Used by: StorCLI2.AddPDsToLV() error case run_and_save \ - "Migrate VD (expected failure)" \ - "${OUTPUT_DIR}/logicalvolumemanager/testdata/storcli2/migrate/fail.json" \ - "${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}" # --- Step 0: Auto-detect sacrifice VD if needed --- if [ "${SACRIFICE_VD_ID}" = "auto" ]; then From 70f066184ec89312d0bbeada36770f79ec6b5c0f Mon Sep 17 00:00:00 2001 From: Guillaume Carre Date: Thu, 25 Jun 2026 11:18:09 +0200 Subject: [PATCH 2/3] fix(storcli2): drop unused receiver on DeletePDsFromLV revive's unused-receiver rule flagged the named 's' receiver, which the not-supported stub never references. Issue: ARTESCA-17648 --- pkg/implementation/logicalvolumemanager/storcli2.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/implementation/logicalvolumemanager/storcli2.go b/pkg/implementation/logicalvolumemanager/storcli2.go index 186d5cc..0d7c51b 100644 --- a/pkg/implementation/logicalvolumemanager/storcli2.go +++ b/pkg/implementation/logicalvolumemanager/storcli2.go @@ -168,7 +168,7 @@ func (s *StorCLI2) AddPDsToLV( // DeletePDsFromLV is not supported by storcli2: the storcli-to-storcli2 command // map drops "start migrate" with no replacement for removing drives from a // volume (see DESIGN.md). -func (s *StorCLI2) DeletePDsFromLV( +func (*StorCLI2) DeletePDsFromLV( _ *logicalvolume.Metadata, _ ...*physicaldrive.Metadata, ) error { From a40039e4ec835889a6836bc321cea247625d9864 Mon Sep 17 00:00:00 2001 From: Guillaume Carre Date: Thu, 25 Jun 2026 13:24:55 +0200 Subject: [PATCH 3/3] feat(storcli2): physical drive blinker 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 --- pkg/implementation/blinker/storcli2.go | 96 +++++++++++++++++ pkg/implementation/blinker/storcli2_test.go | 111 ++++++++++++++++++++ 2 files changed, 207 insertions(+) create mode 100644 pkg/implementation/blinker/storcli2.go create mode 100644 pkg/implementation/blinker/storcli2_test.go diff --git a/pkg/implementation/blinker/storcli2.go b/pkg/implementation/blinker/storcli2.go new file mode 100644 index 0000000..ea7f3be --- /dev/null +++ b/pkg/implementation/blinker/storcli2.go @@ -0,0 +1,96 @@ +package blinker + +import ( + "fmt" + + "github.com/pkg/errors" + + "github.com/scality/raidmgmt/pkg/domain/entities/physicaldrive" + "github.com/scality/raidmgmt/pkg/domain/ports" + "github.com/scality/raidmgmt/pkg/implementation/commandrunner" + "github.com/scality/raidmgmt/pkg/implementation/storcli2" +) + +const ( + // storcli2CmdStart and storcli2CmdStop begin and end a drive locate. + storcli2CmdStart = "start" + storcli2CmdStop = "stop" + // storcli2CmdLocate is the locate (blink) operation token. + storcli2CmdLocate = "locate" + // storcli2EnclosureSelector and storcli2NoEnclosureSelector address a single + // drive, with or without an enclosure component. + storcli2EnclosureSelector = "/c%d/e%s/s%s" + storcli2NoEnclosureSelector = "/c%d/s%s" +) + +// StorCLI2 blinks physical drives through a storcli2/perccli2 command runner. A +// single implementation serves both binaries; the concrete runner is injected +// at construction time. +type StorCLI2 struct { + runner commandrunner.CommandRunner +} + +var _ ports.Blinker = &StorCLI2{} + +// NewStorCLI2 returns a blinker backed by the given storcli2 / perccli2 command +// runner. +func NewStorCLI2(runner commandrunner.CommandRunner) *StorCLI2 { + return &StorCLI2{ + runner: runner, + } +} + +// StartBlink starts locating (blinking) a physical drive. +func (s *StorCLI2) StartBlink(metadata *physicaldrive.Metadata) error { + if err := s.locate(metadata, storcli2CmdStart); err != nil { + return errors.Wrap(err, "failed to start blinking physical drive") + } + + return nil +} + +// StopBlink stops locating (blinking) a physical drive. +func (s *StorCLI2) StopBlink(metadata *physicaldrive.Metadata) error { + if err := s.locate(metadata, storcli2CmdStop); err != nil { + return errors.Wrap(err, "failed to stop blinking physical drive") + } + + return nil +} + +// locate runs " locate" on a drive selector and surfaces the +// in-JSON failure that storcli2 may report regardless of its exit code. +func (s *StorCLI2) locate(metadata *physicaldrive.Metadata, action string) error { + selector, err := storcli2SelectorPD(metadata) + if err != nil { + return errors.Wrap(err, "failed to build drive selector") + } + + output, err := s.runner.Run([]string{selector, action, storcli2CmdLocate}) + if err != nil { + return errors.Wrapf(err, "failed to run %s locate command", action) + } + + if _, err := storcli2.Decode(output); err != nil { + return errors.Wrapf(err, "%s locate command failed", action) + } + + return nil +} + +// storcli2SelectorPD builds the storcli2 selector for a drive, choosing the +// enclosure or no-enclosure form from its parsed slot. +func storcli2SelectorPD(metadata *physicaldrive.Metadata) (string, error) { + slot, err := physicaldrive.ParseSlot(metadata.ID) + if err != nil { + return "", errors.Wrapf(err, "failed to parse slot %s", metadata.ID) + } + + if slot.Enclosure != "" { + return fmt.Sprintf( + storcli2EnclosureSelector, metadata.CtrlMetadata.ID, slot.Enclosure, slot.Bay, + ), nil + } + + return fmt.Sprintf(storcli2NoEnclosureSelector, metadata.CtrlMetadata.ID, slot.Bay), nil +} diff --git a/pkg/implementation/blinker/storcli2_test.go b/pkg/implementation/blinker/storcli2_test.go new file mode 100644 index 0000000..15c2c41 --- /dev/null +++ b/pkg/implementation/blinker/storcli2_test.go @@ -0,0 +1,111 @@ +package blinker_test + +import ( + "os" + "testing" + + "github.com/pkg/errors" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/scality/raidmgmt/pkg/domain/entities/physicaldrive" + "github.com/scality/raidmgmt/pkg/domain/entities/raidcontroller" + "github.com/scality/raidmgmt/pkg/implementation/blinker" +) + +type MockCommandRunner struct { + mock.Mock +} + +func (m *MockCommandRunner) Run(args []string) ([]byte, error) { + arguments := m.Called(args) + + return arguments.Get(0).([]byte), arguments.Error(1) +} + +// storcli2Fixture reads a storcli2 JSON fixture from the package testdata. +func storcli2Fixture(t *testing.T, name string) []byte { + t.Helper() + + data, err := os.ReadFile("testdata/storcli2/" + name) + require.NoError(t, err) + + return data +} + +// storcli2PDMetadata builds physical-drive metadata with the given EID:Slt id. +func storcli2PDMetadata(id string) *physicaldrive.Metadata { + return &physicaldrive.Metadata{ + CtrlMetadata: &raidcontroller.Metadata{ID: 0}, + ID: id, + } +} + +// TestStorCLI2Blink covers the start/stop locate happy paths, including the +// enclosure and no-enclosure selector forms. +func TestStorCLI2Blink(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + id string + selector string + action string + fixture string + start bool + }{ + {name: "start with enclosure", id: "252:0", selector: "/c0/e252/s0", action: "start", fixture: "start.json", start: true}, + {name: "stop with enclosure", id: "252:0", selector: "/c0/e252/s0", action: "stop", fixture: "stop.json", start: false}, + {name: "start without enclosure", id: "5", selector: "/c0/s5", action: "start", fixture: "start.json", start: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + mockRunner := new(MockCommandRunner) + mockRunner.On("Run", []string{tt.selector, tt.action, "locate"}). + Return(storcli2Fixture(t, tt.fixture), nil) + + b := blinker.NewStorCLI2(mockRunner) + + var err error + if tt.start { + err = b.StartBlink(storcli2PDMetadata(tt.id)) + } else { + err = b.StopBlink(storcli2PDMetadata(tt.id)) + } + + require.NoError(t, err) + mockRunner.AssertExpectations(t) + }) + } +} + +// TestStorCLI2BlinkCommandError pins that a runner failure is surfaced. +func TestStorCLI2BlinkCommandError(t *testing.T) { + t.Parallel() + + mockRunner := new(MockCommandRunner) + mockRunner.On("Run", []string{"/c0/e252/s0", "start", "locate"}). + Return([]byte(nil), errors.New("boom")) + + b := blinker.NewStorCLI2(mockRunner) + + err := b.StartBlink(storcli2PDMetadata("252:0")) + require.Error(t, err) +} + +// TestStorCLI2BlinkInvalidSlot pins that an unparseable drive id is rejected +// before any command is run. +func TestStorCLI2BlinkInvalidSlot(t *testing.T) { + t.Parallel() + + mockRunner := new(MockCommandRunner) + + b := blinker.NewStorCLI2(mockRunner) + + err := b.StopBlink(storcli2PDMetadata("")) + require.Error(t, err) + mockRunner.AssertNotCalled(t, "Run") +}