From 44d8d0ba238cbccf0f4c94370f087b7d0d06be43 Mon Sep 17 00:00:00 2001 From: Manuel Mendez Date: Wed, 29 May 2024 17:09:33 -0400 Subject: [PATCH] Define and use our own Drive type Going to invert how drive wiping works by having the drive know how to wipe itself, so we need our own type to be able to give it methods. --- actions/interface.go | 2 +- actions/inventory.go | 18 ++++++++++++++---- model/drive.go | 32 ++++++++++++++++++++++++++++++++ utils/lsblk.go | 20 +++++++------------- utils/lsblk_test.go | 19 ++++++++++--------- utils/msecli.go | 9 ++++----- utils/msecli_test.go | 30 ++++++++++++------------------ utils/mvcli.go | 13 +++++-------- utils/mvcli_test.go | 11 ++++++----- utils/nvme.go | 42 +++++++++++++++++++----------------------- utils/nvme_test.go | 12 +++++++----- utils/smartctl.go | 24 +++++++++++------------- utils/smartctl_test.go | 13 +++++++------ 13 files changed, 135 insertions(+), 110 deletions(-) create mode 100644 model/drive.go diff --git a/actions/interface.go b/actions/interface.go index 0f3736980..15ea54717 100644 --- a/actions/interface.go +++ b/actions/interface.go @@ -84,7 +84,7 @@ type InventoryCollector interface { // DriveCollector defines an interface to return disk drive inventory type DriveCollector interface { UtilAttributeGetter - Drives(ctx context.Context) ([]*common.Drive, error) + Drives(ctx context.Context) ([]*model.Drive, error) } // DriveCapabilityCollector defines an interface to collect disk drive capability attributes diff --git a/actions/inventory.go b/actions/inventory.go index a0e92b780..38c6a353d 100644 --- a/actions/inventory.go +++ b/actions/inventory.go @@ -402,19 +402,19 @@ func (a *InventoryCollectorAction) CollectDrives(ctx context.Context) (err error // add drive if it isn't part of the drives slice based on its serial for _, new := range ndrives { - found := a.findDriveBySerial(new.Serial, a.device.Drives) + found := a.findCommonDriveBySerial(new.Serial, a.device.Drives) if found != nil && found.Serial != "" { continue } - a.device.Drives = append(a.device.Drives, new) + a.device.Drives = append(a.device.Drives, &new.Drive) } } return nil } -func (a *InventoryCollectorAction) findDriveBySerial(serial string, drives []*common.Drive) *common.Drive { +func (a *InventoryCollectorAction) findCommonDriveBySerial(serial string, drives []*common.Drive) *common.Drive { for _, drive := range drives { if strings.EqualFold(serial, drive.Serial) { return drive @@ -424,7 +424,17 @@ func (a *InventoryCollectorAction) findDriveBySerial(serial string, drives []*co return nil } -func (a *InventoryCollectorAction) findDriveByLogicalName(logicalName string, drives []*common.Drive) *common.Drive { +func (a *InventoryCollectorAction) findDriveBySerial(serial string, drives []*model.Drive) *model.Drive { + for _, drive := range drives { + if strings.EqualFold(serial, drive.Serial) { + return drive + } + } + + return nil +} + +func (a *InventoryCollectorAction) findDriveByLogicalName(logicalName string, drives []*model.Drive) *model.Drive { for _, drive := range drives { if strings.EqualFold(logicalName, drive.LogicalName) { return drive diff --git a/model/drive.go b/model/drive.go new file mode 100644 index 000000000..2ba356ec0 --- /dev/null +++ b/model/drive.go @@ -0,0 +1,32 @@ +package model + +import ( + "github.com/bmc-toolbox/common" + "github.com/metal-toolbox/ironlib/actions/wipe" +) + +type Drive struct { + common.Drive + wipersGetter WipersGetter +} + +type WipersGetter interface { + Wipers(*Drive) []wipe.Wiper +} + +func NewDrive(d *common.Drive, w WipersGetter) *Drive { + if d == nil { + return &Drive{} + } + return &Drive{ + Drive: *d, + wipersGetter: w, + } +} + +func (d *Drive) Wipers() []wipe.Wiper { + if d.wipersGetter == nil { + return nil + } + return d.wipersGetter.Wipers(d) +} diff --git a/utils/lsblk.go b/utils/lsblk.go index 2abb26c5d..d3fc3d23c 100644 --- a/utils/lsblk.go +++ b/utils/lsblk.go @@ -57,10 +57,8 @@ func (l *Lsblk) Attributes() (utilName model.CollectorUtility, absolutePath stri return "lsblk", l.Executor.CmdPath(), er } -// Executes lsblk list, parses the output and returns a slice of *common.Drive -func (l *Lsblk) Drives(ctx context.Context) ([]*common.Drive, error) { - drives := make([]*common.Drive, 0) - +// Executes lsblk list, parses the output and returns a slice of *model.Drive +func (l *Lsblk) Drives(ctx context.Context) ([]*model.Drive, error) { out, err := l.list(ctx) if err != nil { return nil, err @@ -73,18 +71,16 @@ func (l *Lsblk) Drives(ctx context.Context) ([]*common.Drive, error) { return nil, err } - for _, d := range items["blockdevices"] { + drives := make([]*model.Drive, len(items["blockdevices"])) + for i, d := range items["blockdevices"] { dModel := d.Model var vendor string - - modelTokens := strings.Split(d.Model, " ") - - if len(modelTokens) > 1 { + if modelTokens := strings.Split(d.Model, " "); len(modelTokens) > 1 { vendor = modelTokens[1] } - drive := &common.Drive{ + drives[i] = model.NewDrive(&common.Drive{ Protocol: strings.ToLower(d.Transport), Common: common.Common{ LogicalName: strings.TrimSpace(d.Device), @@ -93,9 +89,7 @@ func (l *Lsblk) Drives(ctx context.Context) ([]*common.Drive, error) { Model: strings.TrimSpace(dModel), }, StorageControllerDriveID: -1, - } - - drives = append(drives, drive) + }, nil) } return drives, nil diff --git a/utils/lsblk_test.go b/utils/lsblk_test.go index d7a10f058..ff32e052e 100644 --- a/utils/lsblk_test.go +++ b/utils/lsblk_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/bmc-toolbox/common" + "github.com/metal-toolbox/ironlib/model" "github.com/stretchr/testify/assert" ) @@ -19,8 +20,8 @@ func Test_lsblk_Drives(t *testing.T) { assert.Equal(t, fixtureLsblkDrives, drives) } -var fixtureLsblkDrives = []*common.Drive{ - { +var fixtureLsblkDrives = []*model.Drive{ + {Drive: common.Drive{ Common: common.Common{ Model: "MTFDDAV240TDU", Serial: "203329F89392", @@ -28,8 +29,8 @@ var fixtureLsblkDrives = []*common.Drive{ }, Protocol: "sata", StorageControllerDriveID: -1, - }, - { + }}, + {Drive: common.Drive{ Common: common.Common{ Model: "MTFDDAV240TDU", Serial: "203329F89796", @@ -37,8 +38,8 @@ var fixtureLsblkDrives = []*common.Drive{ }, Protocol: "sata", StorageControllerDriveID: -1, - }, - { + }}, + {Drive: common.Drive{ Common: common.Common{ Model: "Micron_9300_MTFDHAL3T8TDP", Serial: "202728F691F5", @@ -46,8 +47,8 @@ var fixtureLsblkDrives = []*common.Drive{ }, Protocol: "nvme", StorageControllerDriveID: -1, - }, - { + }}, + {Drive: common.Drive{ Common: common.Common{ Model: "Micron_9300_MTFDHAL3T8TDP", Serial: "202728F691C6", @@ -55,5 +56,5 @@ var fixtureLsblkDrives = []*common.Drive{ }, Protocol: "nvme", StorageControllerDriveID: -1, - }, + }}, } diff --git a/utils/msecli.go b/utils/msecli.go index 71408f960..46f5a004c 100644 --- a/utils/msecli.go +++ b/utils/msecli.go @@ -58,15 +58,14 @@ func (m *Msecli) Attributes() (utilName model.CollectorUtility, absolutePath str } // Drives returns a slice of drive components identified -func (m *Msecli) Drives(ctx context.Context) ([]*common.Drive, error) { +func (m *Msecli) Drives(ctx context.Context) ([]*model.Drive, error) { devices, err := m.Query(ctx) if err != nil { return nil, err } - drives := []*common.Drive{} - - for _, d := range devices { + drives := make([]*model.Drive, len(devices)) + for i, d := range devices { item := &common.Drive{ Common: common.Common{ Model: d.ModelNumber, @@ -80,7 +79,7 @@ func (m *Msecli) Drives(ctx context.Context) ([]*common.Drive, error) { Type: model.DriveTypeSlug(d.ModelNumber), } - drives = append(drives, item) + drives[i] = model.NewDrive(item, nil) } return drives, nil diff --git a/utils/msecli_test.go b/utils/msecli_test.go index 81d0acea7..22267da9c 100644 --- a/utils/msecli_test.go +++ b/utils/msecli_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/bmc-toolbox/common" + "github.com/metal-toolbox/ironlib/model" "github.com/pkg/errors" "github.com/stretchr/testify/assert" ) @@ -24,36 +25,29 @@ func newFakeMsecli() (*Msecli, error) { } func Test_MsecliDrives(t *testing.T) { - expected := []*common.Drive{ - { + expected := []*model.Drive{ + {Drive: common.Drive{ Common: common.Common{ Serial: "193423710BDA", Vendor: "micron", Model: "Micron_5200_MTFDDAK480TDN", Description: "Micron_5200_MTFDDAK480TDN", - Firmware: &common.Firmware{ - Installed: "D1MU020", - }, - Metadata: map[string]string{}, + Firmware: &common.Firmware{Installed: "D1MU020"}, + Metadata: map[string]string{}, }, - Type: common.SlugDriveTypeSATASSD, - }, - { + }}, + {Drive: common.Drive{ Common: common.Common{ - Serial: "193423711167", - Vendor: "micron", - + Serial: "193423711167", + Vendor: "micron", Model: "Micron_5200_MTFDDAK480TDN", Description: "Micron_5200_MTFDDAK480TDN", - Firmware: &common.Firmware{ - Installed: "D1MU020", - }, - Metadata: map[string]string{}, + Firmware: &common.Firmware{Installed: "D1MU020"}, + Metadata: map[string]string{}, }, - Type: common.SlugDriveTypeSATASSD, - }, + }}, } m, err := newFakeMsecli() diff --git a/utils/mvcli.go b/utils/mvcli.go index 59b482409..ddae11df9 100644 --- a/utils/mvcli.go +++ b/utils/mvcli.go @@ -187,16 +187,15 @@ func (m *Mvcli) StorageControllers(ctx context.Context) ([]*common.StorageContro return hbas, nil } -func (m *Mvcli) Drives(ctx context.Context) ([]*common.Drive, error) { +func (m *Mvcli) Drives(ctx context.Context) ([]*model.Drive, error) { devices, err := m.Info(ctx, "pd") if err != nil { return nil, err } - drives := []*common.Drive{} - - for _, d := range devices { - drive := &common.Drive{ + drives := make([]*model.Drive, len(devices)) + for i, d := range devices { + drives[i] = model.NewDrive(&common.Drive{ Common: common.Common{ Model: d.Model, Vendor: common.VendorFromString(d.Model), @@ -211,9 +210,7 @@ func (m *Mvcli) Drives(ctx context.Context) ([]*common.Drive, error) { Type: m.processDriveType(d.Type, d.SSDType), NegotiatedSpeedGbps: d.CurrentSpeed, StorageControllerDriveID: d.ID, - } - - drives = append(drives, drive) + }, nil) } return drives, nil diff --git a/utils/mvcli_test.go b/utils/mvcli_test.go index 02262317e..07dc95fcf 100644 --- a/utils/mvcli_test.go +++ b/utils/mvcli_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/bmc-toolbox/common" + "github.com/metal-toolbox/ironlib/model" "github.com/stretchr/testify/assert" ) @@ -61,8 +62,8 @@ func Test_MvcliStorageControllers(t *testing.T) { } func Test_MvcliDrives(t *testing.T) { - expected := []*common.Drive{ - { + expected := []*model.Drive{ + {Drive: common.Drive{ Common: common.Common{ Description: "MTFDDAV240TCB", Vendor: "micron", @@ -80,8 +81,8 @@ func Test_MvcliDrives(t *testing.T) { Type: common.SlugDriveTypeSATASSD, NegotiatedSpeedGbps: 6, StorageControllerDriveID: 0, - }, - { + }}, + {Drive: common.Drive{ Common: common.Common{ Description: "MTFDDAV240TCB", Vendor: "micron", @@ -99,7 +100,7 @@ func Test_MvcliDrives(t *testing.T) { Type: common.SlugDriveTypeSATASSD, NegotiatedSpeedGbps: 6, StorageControllerDriveID: 1, - }, + }}, } cli := newFakeMvcli(t, "info-pd") diff --git a/utils/nvme.go b/utils/nvme.go index 245de520f..e327a08f1 100644 --- a/utils/nvme.go +++ b/utils/nvme.go @@ -72,10 +72,8 @@ func (n *Nvme) Attributes() (utilName model.CollectorUtility, absolutePath strin return "nvme", n.Executor.CmdPath(), er } -// Executes nvme list, parses the output and returns a slice of *common.Drive -func (n *Nvme) Drives(ctx context.Context) ([]*common.Drive, error) { - drives := make([]*common.Drive, 0) - +// Executes nvme list, parses the output and returns a slice of *model.Drive +func (n *Nvme) Drives(ctx context.Context) ([]*model.Drive, error) { out, err := n.list(ctx) if err != nil { return nil, err @@ -88,17 +86,27 @@ func (n *Nvme) Drives(ctx context.Context) ([]*common.Drive, error) { return nil, err } - for _, d := range list.Devices { + drives := make([]*model.Drive, len(list.Devices)) + for i, d := range list.Devices { dModel := d.ModelNumber var vendor string + if modelTokens := strings.Split(d.ModelNumber, " "); len(modelTokens) > 1 { + vendor = modelTokens[1] + } - modelTokens := strings.Split(d.ModelNumber, " ") + // Collect drive capabilitiesFound + capabilitiesFound, err := n.DriveCapabilities(ctx, d.DevicePath) + if err != nil { + return nil, err + } - if len(modelTokens) > 1 { - vendor = modelTokens[1] + metadata := map[string]string{} + for _, f := range capabilitiesFound { + metadata[f.Description] = strconv.FormatBool(f.Enabled) } - drive := &common.Drive{ + + drives[i] = model.NewDrive(&common.Drive{ Common: common.Common{ Serial: d.SerialNumber, Vendor: vendor, @@ -108,21 +116,9 @@ func (n *Nvme) Drives(ctx context.Context) ([]*common.Drive, error) { Firmware: &common.Firmware{ Installed: d.Firmware, }, - Metadata: map[string]string{}, + Metadata: metadata, }, - } - - // Collect drive capabilitiesFound - capabilitiesFound, err := n.DriveCapabilities(ctx, d.DevicePath) - if err != nil { - return nil, err - } - - for _, f := range capabilitiesFound { - drive.Common.Metadata[f.Description] = strconv.FormatBool(f.Enabled) - } - - drives = append(drives, drive) + }, nil) } return drives, nil diff --git a/utils/nvme_test.go b/utils/nvme_test.go index 88453409b..115b8a522 100644 --- a/utils/nvme_test.go +++ b/utils/nvme_test.go @@ -8,14 +8,16 @@ import ( "testing" "github.com/bmc-toolbox/common" + "github.com/metal-toolbox/ironlib/model" tlogrus "github.com/sirupsen/logrus/hooks/test" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func Test_NvmeComponents(t *testing.T) { - expected := []*common.Drive{ - {Common: common.Common{ + // nolint:dupl + expected := []*model.Drive{ + {Drive: common.Drive{Common: common.Common{ Serial: "Z9DF70I8FY3L", Vendor: "TOSHIBA", Model: "KXG60ZNV256G TOSHIBA", Description: "KXG60ZNV256G TOSHIBA", Firmware: &common.Firmware{Installed: "AGGA4104"}, ProductName: "NULL", Metadata: map[string]string{ "Block Erase Sanitize Operation Supported": "false", @@ -26,8 +28,8 @@ func Test_NvmeComponents(t *testing.T) { "No-Deallocate After Sanitize bit in Sanitize command Supported": "false", "Overwrite Sanitize Operation Supported": "false", }, - }}, - {Common: common.Common{ + }}}, + {Drive: common.Drive{Common: common.Common{ Serial: "Z9DF70I9FY3L", Vendor: "TOSHIBA", Model: "KXG60ZNV256G TOSHIBA", Description: "KXG60ZNV256G TOSHIBA", Firmware: &common.Firmware{Installed: "AGGA4104"}, ProductName: "NULL", Metadata: map[string]string{ "Block Erase Sanitize Operation Supported": "false", @@ -38,7 +40,7 @@ func Test_NvmeComponents(t *testing.T) { "No-Deallocate After Sanitize bit in Sanitize command Supported": "false", "Overwrite Sanitize Operation Supported": "false", }, - }}, + }}}, } n := NewFakeNvme() diff --git a/utils/smartctl.go b/utils/smartctl.go index a9fee1c28..e741adc2d 100644 --- a/utils/smartctl.go +++ b/utils/smartctl.go @@ -1,6 +1,7 @@ package utils import ( + "cmp" "context" "encoding/json" "fmt" @@ -84,24 +85,26 @@ func (s *Smartctl) Attributes() (utilName model.CollectorUtility, absolutePath s } // Drives returns drives identified by smartctl -func (s *Smartctl) Drives(ctx context.Context) ([]*common.Drive, error) { - drives := make([]*common.Drive, 0) - +func (s *Smartctl) Drives(ctx context.Context) ([]*model.Drive, error) { DrivesList, err := s.Scan(ctx) if err != nil { return nil, err } - for _, drive := range DrivesList.Drives { + drives := make([]*model.Drive, 0, len(DrivesList.Drives)) + for _, d := range DrivesList.Drives { // collect drive information with smartctl -a - smartctlAll, err := s.All(ctx, drive.Name) + smartctlAll, err := s.All(ctx, d.Name) if err != nil { return nil, err } item := &common.Drive{ Common: common.Common{ - Vendor: common.VendorFromString(smartctlAll.ModelName), + Vendor: cmp.Or( + common.VendorFromString(smartctlAll.ModelName), + common.VendorFromString(smartctlAll.ModelFamily), + ), Model: smartctlAll.ModelName, Serial: smartctlAll.SerialNumber, ProductName: smartctlAll.ModelName, @@ -115,14 +118,9 @@ func (s *Smartctl) Drives(ctx context.Context) ([]*common.Drive, error) { Type: model.DriveTypeSlug(smartctlAll.ModelName), SmartStatus: common.SmartStatusUnknown, StorageControllerDriveID: -1, + OemID: strings.TrimSpace(smartctlAll.OemProductID), } - if item.Vendor == "" { - item.Vendor = common.VendorFromString(smartctlAll.ModelFamily) - } - - item.OemID = strings.TrimSpace(smartctlAll.OemProductID) - if smartctlAll.Status != nil { if smartctlAll.Status.Passed { item.SmartStatus = common.SmartStatusOK @@ -135,7 +133,7 @@ func (s *Smartctl) Drives(ctx context.Context) ([]*common.Drive, error) { item.SmartErrors = smartctlAll.Errors } - drives = append(drives, item) + drives = append(drives, model.NewDrive(item, nil)) } return drives, nil diff --git a/utils/smartctl_test.go b/utils/smartctl_test.go index a72da2b83..18b6b6cfc 100644 --- a/utils/smartctl_test.go +++ b/utils/smartctl_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/bmc-toolbox/common" + "github.com/metal-toolbox/ironlib/model" "github.com/stretchr/testify/assert" ) @@ -60,12 +61,12 @@ func Test_SmartctlAllNVME(t *testing.T) { } func Test_SmartctlDeviceAttributes(t *testing.T) { - expected := []*common.Drive{ - {Common: common.Common{Serial: "2013273A99BD", Vendor: common.VendorMicron, Model: "Micron_5200_MTFDDAK960TDN", ProductName: "Micron_5200_MTFDDAK960TDN", Firmware: &common.Firmware{Installed: "D1MU020"}}, Type: common.SlugDriveTypeSATASSD, SmartStatus: "ok", StorageControllerDriveID: -1}, - {Common: common.Common{Serial: "VDJ6SU9K", Vendor: common.VendorHGST, Model: "HGST HUS728T8TALE6L4", ProductName: "HGST HUS728T8TALE6L4", Firmware: &common.Firmware{Installed: "V8GNW460"}}, Type: common.SlugDriveTypeSATAHDD, SmartStatus: "ok", StorageControllerDriveID: -1}, - {Common: common.Common{Serial: "PHYH1016001D240J", Vendor: common.VendorDell, Model: "SSDSCKKB240G8R", ProductName: "SSDSCKKB240G8R", Firmware: &common.Firmware{Installed: "XC31DL6R"}}, Type: "Unknown", SmartStatus: "ok", OemID: "DELL(tm)", StorageControllerDriveID: -1}, - {Common: common.Common{Serial: "Z9DF70I8FY3L", Vendor: common.VendorToshiba, Model: "KXG60ZNV256G TOSHIBA", ProductName: "KXG60ZNV256G TOSHIBA", Firmware: &common.Firmware{Installed: "AGGA4104"}}, Type: common.SlugDriveTypePCIeNVMEeSSD, SmartStatus: "ok", StorageControllerDriveID: -1}, - {Common: common.Common{Serial: "Z9DF70I9FY3L", Vendor: common.VendorToshiba, Model: "KXG60ZNV256G TOSHIBA", ProductName: "KXG60ZNV256G TOSHIBA", Firmware: &common.Firmware{Installed: "AGGA4104"}}, Type: common.SlugDriveTypePCIeNVMEeSSD, SmartStatus: "ok", StorageControllerDriveID: -1}, + expected := []*model.Drive{ + {Drive: common.Drive{Common: common.Common{Serial: "2013273A99BD", Vendor: common.VendorMicron, Model: "Micron_5200_MTFDDAK960TDN", ProductName: "Micron_5200_MTFDDAK960TDN", Firmware: &common.Firmware{Installed: "D1MU020"}}, Type: common.SlugDriveTypeSATASSD, SmartStatus: "ok", StorageControllerDriveID: -1}}, + {Drive: common.Drive{Common: common.Common{Serial: "VDJ6SU9K", Vendor: common.VendorHGST, Model: "HGST HUS728T8TALE6L4", ProductName: "HGST HUS728T8TALE6L4", Firmware: &common.Firmware{Installed: "V8GNW460"}}, Type: common.SlugDriveTypeSATAHDD, SmartStatus: "ok", StorageControllerDriveID: -1}}, + {Drive: common.Drive{Common: common.Common{Serial: "PHYH1016001D240J", Vendor: common.VendorDell, Model: "SSDSCKKB240G8R", ProductName: "SSDSCKKB240G8R", Firmware: &common.Firmware{Installed: "XC31DL6R"}}, Type: "Unknown", SmartStatus: "ok", OemID: "DELL(tm)", StorageControllerDriveID: -1}}, + {Drive: common.Drive{Common: common.Common{Serial: "Z9DF70I8FY3L", Vendor: common.VendorToshiba, Model: "KXG60ZNV256G TOSHIBA", ProductName: "KXG60ZNV256G TOSHIBA", Firmware: &common.Firmware{Installed: "AGGA4104"}}, Type: common.SlugDriveTypePCIeNVMEeSSD, SmartStatus: "ok", StorageControllerDriveID: -1}}, + {Drive: common.Drive{Common: common.Common{Serial: "Z9DF70I9FY3L", Vendor: common.VendorToshiba, Model: "KXG60ZNV256G TOSHIBA", ProductName: "KXG60ZNV256G TOSHIBA", Firmware: &common.Firmware{Installed: "AGGA4104"}}, Type: common.SlugDriveTypePCIeNVMEeSSD, SmartStatus: "ok", StorageControllerDriveID: -1}}, } s := newFakeSmartctl()