From 063bd833b335cb871f9517e2dc356ee15c82eb7f Mon Sep 17 00:00:00 2001 From: Guillaume Carre Date: Thu, 25 Jun 2026 10:50:42 +0200 Subject: [PATCH] feat(storcli2): logical volume create and delete Implement the create/delete part of ports.LogicalVolumesManager on the storcli2/perccli2 logical-volume manager: - CreateLV fills the request drives through the physical-drive getter, validates the RAID creation, formats a single-enclosure "drives=e:s,..." list (rejecting multi-enclosure requests), issues "add vd" with the bare RAID-level and cache tokens, and rediscovers the new volume by its physical-drive set. - DeleteLV issues "/cx/vx delete" and surfaces the in-JSON failure payload. The manager now also takes a PhysicalDrivesGetter; command verbs follow the storcli2 command map documented in DESIGN.md. Issue: ARTESCA-17647 --- .../logicalvolumemanager/storcli2.go | 229 ++++++++++++++++++ .../logicalvolumemanager/storcli2_test.go | 188 ++++++++++++++ 2 files changed, 417 insertions(+) create mode 100644 pkg/implementation/logicalvolumemanager/storcli2.go create mode 100644 pkg/implementation/logicalvolumemanager/storcli2_test.go diff --git a/pkg/implementation/logicalvolumemanager/storcli2.go b/pkg/implementation/logicalvolumemanager/storcli2.go new file mode 100644 index 0000000..34a38cf --- /dev/null +++ b/pkg/implementation/logicalvolumemanager/storcli2.go @@ -0,0 +1,229 @@ +package logicalvolumemanager + +import ( + "fmt" + "strings" + + "github.com/pkg/errors" + + "github.com/scality/raidmgmt/pkg/domain/entities/logicalvolume" + "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 ( + // storcli2CmdAdd and storcli2CmdVD compose the "add vd" command that creates + // a virtual drive. + storcli2CmdAdd = "add" + storcli2CmdVD = "vd" + // storcli2CmdDelete deletes the virtual drive addressed by the selector. + storcli2CmdDelete = "delete" + // storcli2ControllerSelector addresses a whole controller (used by "add vd"). + storcli2ControllerSelector = "/c%d" + // storcli2VolumeSelector addresses a single virtual drive by its number. + storcli2VolumeSelector = "/c%d/v%s" + // storcli2DrivesFlag prefixes the "e:s,s,..." drive list of "add vd". + storcli2DrivesFlag = "drives=" + // storcli2RAIDLevelFormat builds the bare RAID-level token of "add vd" + // (e.g. "r0"); storcli's "type=raidN" form is gone in storcli2. + storcli2RAIDLevelFormat = "r%d" +) + +// StorCLI2 manages logical volumes through a storcli2/perccli2 command runner. +// A single implementation serves both binaries; the concrete runner is injected +// at construction time. The current state is read through injected getters: the +// physical-drive getter fills the request drives so creation can be validated, +// and the logical-volume getter discovers the virtual drive created by "add vd". +type StorCLI2 struct { + ports.PhysicalDrivesGetter + ports.LogicalVolumesGetter + + StorCLI2 commandrunner.CommandRunner +} + +// NewStorCLI2 returns a logical-volume manager backed by the given storcli2 / +// perccli2 command runner and physical-drive and logical-volume getters. +func NewStorCLI2( + runner commandrunner.CommandRunner, + physicalDrivesGetter ports.PhysicalDrivesGetter, + logicalVolumesGetter ports.LogicalVolumesGetter, +) *StorCLI2 { + return &StorCLI2{ + PhysicalDrivesGetter: physicalDrivesGetter, + LogicalVolumesGetter: logicalVolumesGetter, + StorCLI2: runner, + } +} + +// CreateLV creates a logical volume from a request through "add vd". The +// request drives are filled via the physical-drive getter so the RAID creation +// can be validated, formatted into storcli2's "e:s,s,..." drive list (a single +// enclosure only) and submitted with the bare RAID-level and cache tokens. +// storcli2 reports the new virtual drive's number but not its full state, so +// the volume is rediscovered by matching the request's physical-drive set. +func (s *StorCLI2) CreateLV(request *logicalvolume.Request) (*logicalvolume.LogicalVolume, error) { + physicalDrives := make([]*physicaldrive.PhysicalDrive, 0, len(request.PDrivesMetadata)) + + for _, pdMetadata := range request.PDrivesMetadata { + pd, err := s.PhysicalDrive(pdMetadata) + if err != nil { + return nil, errors.Wrapf(err, "failed to get physical drive %s", pdMetadata.ID) + } + + physicalDrives = append(physicalDrives, pd) + } + + if err := logicalvolume.ValidateRAIDCreation(physicalDrives, request.RAIDLevel); err != nil { + return nil, errors.Wrap(err, "failed to validate RAID creation") + } + + drives, err := storcli2FormatDrives(request.PDrivesMetadata) + if err != nil { + return nil, errors.Wrap(err, "failed to format drives") + } + + args := []string{ + fmt.Sprintf(storcli2ControllerSelector, request.CtrlMetadata.ID), + storcli2CmdAdd, + storcli2CmdVD, + fmt.Sprintf(storcli2RAIDLevelFormat, request.RAIDLevel.Level()), + drives, + } + args = append(args, storcli2CreateCacheFlags(request.CacheOptions)...) + + output, err := s.StorCLI2.Run(args) + if err != nil { + return nil, errors.Wrap(err, "failed to run add vd command") + } + + if _, err := storcli2.Decode(output); err != nil { + return nil, errors.Wrap(err, "add vd command failed") + } + + newLV, err := s.findNewLogicalVolume(request) + if err != nil { + return nil, errors.Wrap(err, "failed to find new logical volume") + } + + return newLV, nil +} + +// DeleteLV deletes the logical volume addressed by the given metadata. A +// nonexistent virtual drive yields a failure payload surfaced by Decode. +func (s *StorCLI2) DeleteLV(metadata *logicalvolume.Metadata) error { + selector := fmt.Sprintf(storcli2VolumeSelector, metadata.CtrlMetadata.ID, metadata.ID) + + output, err := s.StorCLI2.Run([]string{selector, storcli2CmdDelete}) + if err != nil { + return errors.Wrapf(err, "failed to run delete command for logical volume %s", metadata.ID) + } + + if _, err := storcli2.Decode(output); err != nil { + return errors.Wrapf(err, "failed to delete logical volume %s", metadata.ID) + } + + return nil +} + +// 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. +func (s *StorCLI2) findNewLogicalVolume(request *logicalvolume.Request) ( + *logicalvolume.LogicalVolume, + error, +) { + logicalVolumes, err := s.LogicalVolumes(request.CtrlMetadata) + if err != nil { + return nil, errors.Wrap(err, "failed to list logical volumes") + } + + wanted := make(map[string]struct{}, len(request.PDrivesMetadata)) + for _, pdMetadata := range request.PDrivesMetadata { + wanted[pdMetadata.ID] = struct{}{} + } + + for _, lv := range logicalVolumes { + if storcli2MatchesPDSet(lv.PDrivesMetadata, wanted) { + return lv, nil + } + } + + return nil, errors.New("no logical volume matches the requested physical drives") +} + +// storcli2MatchesPDSet reports whether the physical drives of a volume are +// exactly the wanted set. +func storcli2MatchesPDSet(lvPDs []*physicaldrive.Metadata, wanted map[string]struct{}) bool { + if len(lvPDs) != len(wanted) { + return false + } + + for _, pd := range lvPDs { + if _, ok := wanted[pd.ID]; !ok { + return false + } + } + + return true +} + +// storcli2FormatDrives renders physical-drive metadata into the "drives=e:s,..." +// argument of "add vd". storcli2 names every drive by its "EID:Slt" pair and +// accepts a single shared enclosure, so drives spanning multiple enclosures are +// rejected. +func storcli2FormatDrives(pdsMetadata []*physicaldrive.Metadata) (string, error) { + if len(pdsMetadata) == 0 { + return "", errors.New("no physical drives") + } + + var enclosure string + + slots := make([]string, 0, len(pdsMetadata)) + + for _, pdMetadata := range pdsMetadata { + slot, err := physicaldrive.ParseSlot(pdMetadata.ID) + if err != nil { + return "", errors.Wrapf(err, "failed to parse slot %s", pdMetadata.ID) + } + + if slot.Enclosure == "" { + return "", errors.Errorf("missing enclosure in drive %s", pdMetadata.ID) + } + + switch enclosure { + case "": + enclosure = slot.Enclosure + case slot.Enclosure: + default: + return "", errors.New("multiple enclosures not supported") + } + + slots = append(slots, slot.Bay) + } + + return fmt.Sprintf("%s%s:%s", storcli2DrivesFlag, enclosure, strings.Join(slots, ",")), nil +} + +// storcli2CreateCacheFlags returns the bare write- and read-policy tokens of +// "add vd" for the policies that are known. storcli2 takes the policy as a bare +// token at creation time (the "wrcache="/"rdpolicy=" forms are gone) and has no +// IO policy. +func storcli2CreateCacheFlags(cache *logicalvolume.CacheOptions) []string { + if cache == nil { + return nil + } + + var flags []string + + if cache.WritePolicy != logicalvolume.WritePolicyUnknown { + flags = append(flags, string(cache.WritePolicy)) + } + + if cache.ReadPolicy != logicalvolume.ReadPolicyUnknown { + flags = append(flags, string(cache.ReadPolicy)) + } + + return flags +} diff --git a/pkg/implementation/logicalvolumemanager/storcli2_test.go b/pkg/implementation/logicalvolumemanager/storcli2_test.go new file mode 100644 index 0000000..e65e10a --- /dev/null +++ b/pkg/implementation/logicalvolumemanager/storcli2_test.go @@ -0,0 +1,188 @@ +package logicalvolumemanager_test + +import ( + "os" + "testing" + + "github.com/stretchr/testify/require" + + "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/implementation/logicalvolumemanager" +) + +// 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 +} + +// storcli2Ctrl is the controller every storcli2 manager fixture is scoped to. +func storcli2Ctrl() *raidcontroller.Metadata { + return &raidcontroller.Metadata{ID: 0} +} + +// storcli2CreateRequest builds a two-drive RAID1 create request on the given +// controller; both drives share enclosure 252. +func storcli2CreateRequest(ctrl *raidcontroller.Metadata) *logicalvolume.Request { + return &logicalvolume.Request{ + CtrlMetadata: ctrl, + RAIDLevel: logicalvolume.RAIDLevel1, + PDrivesMetadata: []*physicaldrive.Metadata{ + {CtrlMetadata: ctrl, ID: "252:0"}, + {CtrlMetadata: ctrl, ID: "252:1"}, + }, + CacheOptions: &logicalvolume.CacheOptions{ + WritePolicy: logicalvolume.WritePolicyWriteBack, + ReadPolicy: logicalvolume.ReadPolicyReadAhead, + }, + } +} + +// storcli2AvailablePD returns an unassigned-good drive of uniform size so +// ValidateRAIDCreation accepts it. +func storcli2AvailablePD(metadata *physicaldrive.Metadata) *physicaldrive.PhysicalDrive { + return &physicaldrive.PhysicalDrive{ + Metadata: metadata, + Size: 1000, + Status: physicaldrive.PDStatusUnassignedGood, + } +} + +// TestStorCLI2CreateLV covers the happy path: each request drive is filled and +// validated, "add vd" is issued with the bare RAID-level and cache tokens and a +// single-enclosure drive list, and the new volume is rediscovered by its +// physical-drive set. +func TestStorCLI2CreateLV(t *testing.T) { + t.Parallel() + + ctrl := storcli2Ctrl() + request := storcli2CreateRequest(ctrl) + + mockRunner := new(MockCommandRunner) + mockPDGetter := new(MockPhysicalDrivesGetter) + mockLVGetter := new(MockLogicalVolumesGetter) + + for _, pdMetadata := range request.PDrivesMetadata { + mockPDGetter.On("PhysicalDrive", pdMetadata).Return(storcli2AvailablePD(pdMetadata), nil) + } + + mockRunner.On("Run", []string{"/c0", "add", "vd", "r1", "drives=252:0,1", "wb", "ra"}). + Return(storcli2Fixture(t, "create/success.json"), nil) + + newLV := &logicalvolume.LogicalVolume{ + Metadata: &logicalvolume.Metadata{CtrlMetadata: ctrl, ID: "25"}, + PDrivesMetadata: request.PDrivesMetadata, + } + other := &logicalvolume.LogicalVolume{ + Metadata: &logicalvolume.Metadata{CtrlMetadata: ctrl, ID: "7"}, + PDrivesMetadata: []*physicaldrive.Metadata{{CtrlMetadata: ctrl, ID: "252:9"}}, + } + mockLVGetter.On("LogicalVolumes", ctrl). + Return([]*logicalvolume.LogicalVolume{other, newLV}, nil) + + manager := logicalvolumemanager.NewStorCLI2(mockRunner, mockPDGetter, mockLVGetter) + + lv, err := manager.CreateLV(request) + require.NoError(t, err) + require.Equal(t, "25", lv.ID) + mockRunner.AssertExpectations(t) +} + +// TestStorCLI2CreateLVCommandError pins that an "add vd" failure payload aborts +// before any volume discovery. +func TestStorCLI2CreateLVCommandError(t *testing.T) { + t.Parallel() + + ctrl := storcli2Ctrl() + request := storcli2CreateRequest(ctrl) + + mockRunner := new(MockCommandRunner) + mockPDGetter := new(MockPhysicalDrivesGetter) + mockLVGetter := new(MockLogicalVolumesGetter) + + for _, pdMetadata := range request.PDrivesMetadata { + mockPDGetter.On("PhysicalDrive", pdMetadata).Return(storcli2AvailablePD(pdMetadata), nil) + } + + mockRunner.On("Run", []string{"/c0", "add", "vd", "r1", "drives=252:0,1", "wb", "ra"}). + Return(storcli2Fixture(t, "create/fail.json"), nil) + + manager := logicalvolumemanager.NewStorCLI2(mockRunner, mockPDGetter, mockLVGetter) + + _, err := manager.CreateLV(request) + require.Error(t, err) + mockLVGetter.AssertNotCalled(t, "LogicalVolumes") +} + +// TestStorCLI2CreateLVMultipleEnclosures pins that a request spanning two +// enclosures is rejected before "add vd" is run. +func TestStorCLI2CreateLVMultipleEnclosures(t *testing.T) { + t.Parallel() + + ctrl := storcli2Ctrl() + request := storcli2CreateRequest(ctrl) + request.PDrivesMetadata[1].ID = "253:1" + + mockRunner := new(MockCommandRunner) + mockPDGetter := new(MockPhysicalDrivesGetter) + mockLVGetter := new(MockLogicalVolumesGetter) + + for _, pdMetadata := range request.PDrivesMetadata { + mockPDGetter.On("PhysicalDrive", pdMetadata).Return(storcli2AvailablePD(pdMetadata), nil) + } + + manager := logicalvolumemanager.NewStorCLI2(mockRunner, mockPDGetter, mockLVGetter) + + _, err := manager.CreateLV(request) + require.Error(t, err) + mockRunner.AssertNotCalled(t, "Run") +} + +// TestStorCLI2DeleteLV covers the delete happy path and the two documented +// failure payloads (an invalid VD number and a nonexistent VD). +func TestStorCLI2DeleteLV(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + id string + fixture string + wantErr bool + }{ + {name: "success", id: "25", fixture: "delete/success.json", wantErr: false}, + {name: "invalid", id: "299", fixture: "delete/fail_invalid.json", wantErr: true}, + {name: "not exist", id: "999", fixture: "delete/fail_vdNotExist.json", wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctrl := storcli2Ctrl() + metadata := &logicalvolume.Metadata{CtrlMetadata: ctrl, ID: tt.id} + + mockRunner := new(MockCommandRunner) + mockRunner.On("Run", []string{"/c0/v" + tt.id, "delete"}). + Return(storcli2Fixture(t, tt.fixture), nil) + + manager := logicalvolumemanager.NewStorCLI2( + mockRunner, new(MockPhysicalDrivesGetter), new(MockLogicalVolumesGetter), + ) + + err := manager.DeleteLV(metadata) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + + mockRunner.AssertExpectations(t) + }) + } +}