From db86bac9efb10ca11177a1cf00621a8ea91dc6aa Mon Sep 17 00:00:00 2001 From: Thomas Way Date: Tue, 23 Jan 2024 22:02:02 +0000 Subject: [PATCH] fix(collector): show correct nvme capacity Some nvme devices do not report their capacity through the usual 'user_capacity' field, instead the total capacity is reported with the 'nvme_total_capacity' field. Fixes: #466 --- collector/pkg/detect/detect.go | 10 +- collector/pkg/detect/detect_test.go | 135 +++++++++++++----- .../detect/testdata/smartctl_info_nvme.json | 48 +++++++ webapp/backend/pkg/models/collector/smart.go | 38 +++-- .../pkg/models/collector/smart_test.go | 33 +++++ 5 files changed, 212 insertions(+), 52 deletions(-) create mode 100644 collector/pkg/detect/testdata/smartctl_info_nvme.json create mode 100644 webapp/backend/pkg/models/collector/smart_test.go diff --git a/collector/pkg/detect/detect.go b/collector/pkg/detect/detect.go index 971805e..529ee3e 100644 --- a/collector/pkg/detect/detect.go +++ b/collector/pkg/detect/detect.go @@ -3,13 +3,14 @@ package detect import ( "encoding/json" "fmt" + "os" + "strings" + "github.com/analogj/scrutiny/collector/pkg/common/shell" "github.com/analogj/scrutiny/collector/pkg/config" "github.com/analogj/scrutiny/collector/pkg/models" "github.com/analogj/scrutiny/webapp/backend/pkg/models/collector" "github.com/sirupsen/logrus" - "os" - "strings" ) type Detect struct { @@ -47,7 +48,7 @@ func (d *Detect) SmartctlScan() ([]models.Device, error) { return detectedDevices, nil } -//updates a device model with information from smartctl --scan +// updates a device model with information from smartctl --scan // It has a couple of issues however: // - WWN is provided as component data, rather than a "string". We'll have to generate the WWN value ourselves // - WWN from smartctl only provided for ATA protocol drives, NVMe and SCSI drives do not include WWN. @@ -81,8 +82,9 @@ func (d *Detect) SmartCtlInfo(device *models.Device) error { device.SerialNumber = availableDeviceInfo.SerialNumber device.Firmware = availableDeviceInfo.FirmwareVersion device.RotationSpeed = availableDeviceInfo.RotationRate - device.Capacity = availableDeviceInfo.UserCapacity.Bytes + device.Capacity = availableDeviceInfo.Capacity() device.FormFactor = availableDeviceInfo.FormFactor.Name + device.DeviceType = availableDeviceInfo.Device.Type device.DeviceProtocol = availableDeviceInfo.Device.Protocol if len(availableDeviceInfo.Vendor) > 0 { device.Manufacturer = availableDeviceInfo.Vendor diff --git a/collector/pkg/detect/detect_test.go b/collector/pkg/detect/detect_test.go index 1ef6d92..c2976f5 100644 --- a/collector/pkg/detect/detect_test.go +++ b/collector/pkg/detect/detect_test.go @@ -1,19 +1,22 @@ package detect_test import ( + "os" + "strings" + "testing" + mock_shell "github.com/analogj/scrutiny/collector/pkg/common/shell/mock" mock_config "github.com/analogj/scrutiny/collector/pkg/config/mock" "github.com/analogj/scrutiny/collector/pkg/detect" "github.com/analogj/scrutiny/collector/pkg/models" "github.com/golang/mock/gomock" "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "io/ioutil" - "testing" ) func TestDetect_SmartctlScan(t *testing.T) { - //setup + // setup mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() fakeConfig := mock_config.NewMockInterface(mockCtrl) @@ -23,7 +26,7 @@ func TestDetect_SmartctlScan(t *testing.T) { fakeConfig.EXPECT().GetString("commands.metrics_scan_args").AnyTimes().Return("--scan --json") fakeShell := mock_shell.NewMockInterface(mockCtrl) - testScanResults, err := ioutil.ReadFile("testdata/smartctl_scan_simple.json") + testScanResults, err := os.ReadFile("testdata/smartctl_scan_simple.json") fakeShell.EXPECT().Command(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().Return(string(testScanResults), err) d := detect.Detect{ @@ -32,17 +35,17 @@ func TestDetect_SmartctlScan(t *testing.T) { Config: fakeConfig, } - //test + // test scannedDevices, err := d.SmartctlScan() - //assert + // assert require.NoError(t, err) require.Equal(t, 7, len(scannedDevices)) require.Equal(t, "scsi", scannedDevices[0].DeviceType) } func TestDetect_SmartctlScan_Megaraid(t *testing.T) { - //setup + // setup mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() fakeConfig := mock_config.NewMockInterface(mockCtrl) @@ -52,7 +55,7 @@ func TestDetect_SmartctlScan_Megaraid(t *testing.T) { fakeConfig.EXPECT().GetString("commands.metrics_scan_args").AnyTimes().Return("--scan --json") fakeShell := mock_shell.NewMockInterface(mockCtrl) - testScanResults, err := ioutil.ReadFile("testdata/smartctl_scan_megaraid.json") + testScanResults, err := os.ReadFile("testdata/smartctl_scan_megaraid.json") fakeShell.EXPECT().Command(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().Return(string(testScanResults), err) d := detect.Detect{ @@ -61,20 +64,20 @@ func TestDetect_SmartctlScan_Megaraid(t *testing.T) { Config: fakeConfig, } - //test + // test scannedDevices, err := d.SmartctlScan() - //assert + // assert require.NoError(t, err) require.Equal(t, 2, len(scannedDevices)) require.Equal(t, []models.Device{ - models.Device{DeviceName: "bus/0", DeviceType: "megaraid,0"}, - models.Device{DeviceName: "bus/0", DeviceType: "megaraid,1"}, + {DeviceName: "bus/0", DeviceType: "megaraid,0"}, + {DeviceName: "bus/0", DeviceType: "megaraid,1"}, }, scannedDevices) } func TestDetect_SmartctlScan_Nvme(t *testing.T) { - //setup + // setup mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() fakeConfig := mock_config.NewMockInterface(mockCtrl) @@ -84,7 +87,7 @@ func TestDetect_SmartctlScan_Nvme(t *testing.T) { fakeConfig.EXPECT().GetString("commands.metrics_scan_args").AnyTimes().Return("--scan --json") fakeShell := mock_shell.NewMockInterface(mockCtrl) - testScanResults, err := ioutil.ReadFile("testdata/smartctl_scan_nvme.json") + testScanResults, err := os.ReadFile("testdata/smartctl_scan_nvme.json") fakeShell.EXPECT().Command(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().Return(string(testScanResults), err) d := detect.Detect{ @@ -93,19 +96,19 @@ func TestDetect_SmartctlScan_Nvme(t *testing.T) { Config: fakeConfig, } - //test + // test scannedDevices, err := d.SmartctlScan() - //assert + // assert require.NoError(t, err) require.Equal(t, 1, len(scannedDevices)) require.Equal(t, []models.Device{ - models.Device{DeviceName: "nvme0", DeviceType: "nvme"}, + {DeviceName: "nvme0", DeviceType: "nvme"}, }, scannedDevices) } func TestDetect_TransformDetectedDevices_Empty(t *testing.T) { - //setup + // setup mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() fakeConfig := mock_config.NewMockInterface(mockCtrl) @@ -129,16 +132,16 @@ func TestDetect_TransformDetectedDevices_Empty(t *testing.T) { Config: fakeConfig, } - //test + // test transformedDevices := d.TransformDetectedDevices(detectedDevices) - //assert + // assert require.Equal(t, "sda", transformedDevices[0].DeviceName) require.Equal(t, "scsi", transformedDevices[0].DeviceType) } func TestDetect_TransformDetectedDevices_Ignore(t *testing.T) { - //setup + // setup mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() fakeConfig := mock_config.NewMockInterface(mockCtrl) @@ -162,15 +165,15 @@ func TestDetect_TransformDetectedDevices_Ignore(t *testing.T) { Config: fakeConfig, } - //test + // test transformedDevices := d.TransformDetectedDevices(detectedDevices) - //assert + // assert require.Equal(t, []models.Device{}, transformedDevices) } func TestDetect_TransformDetectedDevices_Raid(t *testing.T) { - //setup + // setup mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() fakeConfig := mock_config.NewMockInterface(mockCtrl) @@ -187,7 +190,8 @@ func TestDetect_TransformDetectedDevices_Raid(t *testing.T) { Device: "/dev/twa0", DeviceType: []string{"3ware,0", "3ware,1", "3ware,2", "3ware,3", "3ware,4", "3ware,5"}, Ignore: false, - }}) + }, + }) detectedDevices := models.Scan{ Devices: []models.ScanDevice{ { @@ -203,15 +207,15 @@ func TestDetect_TransformDetectedDevices_Raid(t *testing.T) { Config: fakeConfig, } - //test + // test transformedDevices := d.TransformDetectedDevices(detectedDevices) - //assert + // assert require.Equal(t, 12, len(transformedDevices)) } func TestDetect_TransformDetectedDevices_Simple(t *testing.T) { - //setup + // setup mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() fakeConfig := mock_config.NewMockInterface(mockCtrl) @@ -234,17 +238,17 @@ func TestDetect_TransformDetectedDevices_Simple(t *testing.T) { Config: fakeConfig, } - //test + // test transformedDevices := d.TransformDetectedDevices(detectedDevices) - //assert + // assert require.Equal(t, 1, len(transformedDevices)) require.Equal(t, "sat+megaraid", transformedDevices[0].DeviceType) } // test https://github.com/AnalogJ/scrutiny/issues/255#issuecomment-1164024126 func TestDetect_TransformDetectedDevices_WithoutDeviceTypeOverride(t *testing.T) { - //setup + // setup mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() fakeConfig := mock_config.NewMockInterface(mockCtrl) @@ -267,16 +271,16 @@ func TestDetect_TransformDetectedDevices_WithoutDeviceTypeOverride(t *testing.T) Config: fakeConfig, } - //test + // test transformedDevices := d.TransformDetectedDevices(detectedDevices) - //assert + // assert require.Equal(t, 1, len(transformedDevices)) require.Equal(t, "scsi", transformedDevices[0].DeviceType) } func TestDetect_TransformDetectedDevices_WhenDeviceNotDetected(t *testing.T) { - //setup + // setup mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() fakeConfig := mock_config.NewMockInterface(mockCtrl) @@ -290,10 +294,69 @@ func TestDetect_TransformDetectedDevices_WhenDeviceNotDetected(t *testing.T) { Config: fakeConfig, } - //test + // test transformedDevices := d.TransformDetectedDevices(detectedDevices) - //assert + // assert require.Equal(t, 1, len(transformedDevices)) require.Equal(t, "ata", transformedDevices[0].DeviceType) } + +func TestDetect_SmartCtlInfo(t *testing.T) { + t.Run("should report nvme info", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + const ( + someArgs = "--info --json" + + // device info + someDeviceName = "some-device-name" + someModelName = "KCD61LUL3T84" + someSerialNumber = "61Q0A05UT7B8" + someFirmware = "8002" + someDeviceProtocol = "NVMe" + someDeviceType = "nvme" + someCapacity int64 = 3840755982336 + ) + + fakeConfig := mock_config.NewMockInterface(ctrl) + fakeConfig.EXPECT(). + GetCommandMetricsInfoArgs("/dev/" + someDeviceName). + Return(someArgs) + fakeConfig.EXPECT(). + GetString("commands.metrics_smartctl_bin"). + Return("smartctl") + + someLogger := logrus.WithFields(logrus.Fields{}) + + smartctlInfoResults, err := os.ReadFile("testdata/smartctl_info_nvme.json") + require.NoError(t, err) + + fakeShell := mock_shell.NewMockInterface(ctrl) + fakeShell.EXPECT(). + Command(someLogger, "smartctl", append(strings.Split(someArgs, " "), "/dev/"+someDeviceName), "", gomock.Any()). + Return(string(smartctlInfoResults), err) + + d := detect.Detect{ + Logger: someLogger, + Shell: fakeShell, + Config: fakeConfig, + } + + someDevice := &models.Device{ + WWN: "some wwn", + DeviceName: someDeviceName, + } + + require.NoError(t, d.SmartCtlInfo(someDevice)) + + assert.Equal(t, someDeviceName, someDevice.DeviceName) + assert.Equal(t, someModelName, someDevice.ModelName) + assert.Equal(t, someSerialNumber, someDevice.SerialNumber) + assert.Equal(t, someFirmware, someDevice.Firmware) + assert.Equal(t, someDeviceProtocol, someDevice.DeviceProtocol) + assert.Equal(t, someDeviceType, someDevice.DeviceType) + assert.Equal(t, someCapacity, someDevice.Capacity) + }) +} diff --git a/collector/pkg/detect/testdata/smartctl_info_nvme.json b/collector/pkg/detect/testdata/smartctl_info_nvme.json new file mode 100644 index 0000000..8a3ceb4 --- /dev/null +++ b/collector/pkg/detect/testdata/smartctl_info_nvme.json @@ -0,0 +1,48 @@ +{ + "json_format_version": [ + 1, + 0 + ], + "smartctl": { + "version": [ + 7, + 2 + ], + "svn_revision": "5155", + "platform_info": "x86_64-linux-6.1.69-talos", + "build_info": "(local build)", + "argv": [ + "smartctl", + "--info", + "--json", + "/dev/nvme4" + ], + "exit_status": 0 + }, + "device": { + "name": "/dev/nvme4", + "info_name": "/dev/nvme4", + "type": "nvme", + "protocol": "NVMe" + }, + "model_name": "KCD61LUL3T84", + "serial_number": "61Q0A05UT7B8", + "firmware_version": "8002", + "nvme_pci_vendor": { + "id": 7695, + "subsystem_id": 7695 + }, + "nvme_ieee_oui_identifier": 9233294, + "nvme_total_capacity": 3840755982336, + "nvme_unallocated_capacity": 0, + "nvme_controller_id": 1, + "nvme_version": { + "string": "1.4", + "value": 66560 + }, + "nvme_number_of_namespaces": 16, + "local_time": { + "time_t": 1706045146, + "asctime": "Tue Jan 23 21:25:46 2024 UTC" + } +} diff --git a/webapp/backend/pkg/models/collector/smart.go b/webapp/backend/pkg/models/collector/smart.go index 4f7ae3d..c466ca4 100644 --- a/webapp/backend/pkg/models/collector/smart.go +++ b/webapp/backend/pkg/models/collector/smart.go @@ -27,14 +27,11 @@ type SmartInfo struct { Oui uint64 `json:"oui"` ID uint64 `json:"id"` } `json:"wwn"` - FirmwareVersion string `json:"firmware_version"` - UserCapacity struct { - Blocks int64 `json:"blocks"` - Bytes int64 `json:"bytes"` - } `json:"user_capacity"` - LogicalBlockSize int `json:"logical_block_size"` - PhysicalBlockSize int `json:"physical_block_size"` - RotationRate int `json:"rotation_rate"` + FirmwareVersion string `json:"firmware_version"` + UserCapacity UserCapacity `json:"user_capacity"` + LogicalBlockSize int `json:"logical_block_size"` + PhysicalBlockSize int `json:"physical_block_size"` + RotationRate int `json:"rotation_rate"` FormFactor struct { AtaValue int `json:"ata_value"` Name string `json:"name"` @@ -210,9 +207,10 @@ type SmartInfo struct { ID int `json:"id"` SubsystemID int `json:"subsystem_id"` } `json:"nvme_pci_vendor"` - NvmeIeeeOuiIdentifier int `json:"nvme_ieee_oui_identifier"` - NvmeControllerID int `json:"nvme_controller_id"` - NvmeNumberOfNamespaces int `json:"nvme_number_of_namespaces"` + NvmeIeeeOuiIdentifier int `json:"nvme_ieee_oui_identifier"` + NvmeTotalCapacity int64 `json:"nvme_total_capacity"` + NvmeControllerID int `json:"nvme_controller_id"` + NvmeNumberOfNamespaces int `json:"nvme_number_of_namespaces"` NvmeNamespaces []struct { ID int `json:"id"` Size struct { @@ -239,7 +237,23 @@ type SmartInfo struct { ScsiErrorCounterLog ScsiErrorCounterLog `json:"scsi_error_counter_log"` } -//Primary Attribute Structs +// Capacity finds the total capacity of the device in bytes, or 0 if unknown. +func (s *SmartInfo) Capacity() int64 { + switch { + case s.NvmeTotalCapacity > 0: + return s.NvmeTotalCapacity + case s.UserCapacity.Bytes > 0: + return s.UserCapacity.Bytes + } + return 0 +} + +type UserCapacity struct { + Blocks int64 `json:"blocks"` + Bytes int64 `json:"bytes"` +} + +// Primary Attribute Structs type AtaSmartAttributesTableItem struct { ID int `json:"id"` Name string `json:"name"` diff --git a/webapp/backend/pkg/models/collector/smart_test.go b/webapp/backend/pkg/models/collector/smart_test.go new file mode 100644 index 0000000..9ca8761 --- /dev/null +++ b/webapp/backend/pkg/models/collector/smart_test.go @@ -0,0 +1,33 @@ +package collector + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestSmartInfo_Capacity(t *testing.T) { + t.Run("should report nvme capacity", func(t *testing.T) { + smartInfo := SmartInfo{ + UserCapacity: UserCapacity{ + Bytes: 1234, + }, + NvmeTotalCapacity: 5678, + } + assert.Equal(t, int64(5678), smartInfo.Capacity()) + }) + + t.Run("should report user capacity", func(t *testing.T) { + smartInfo := SmartInfo{ + UserCapacity: UserCapacity{ + Bytes: 1234, + }, + } + assert.Equal(t, int64(1234), smartInfo.Capacity()) + }) + + t.Run("should report 0 for unknown capacities", func(t *testing.T) { + var smartInfo SmartInfo + assert.Zero(t, smartInfo.Capacity()) + }) +}