diff --git a/agent/smart.go b/agent/smart.go index 41abb14f..28a5ad81 100644 --- a/agent/smart.go +++ b/agent/smart.go @@ -375,9 +375,15 @@ func (sm *SmartManager) parseSmartOutput(deviceInfo *DeviceInfo, output []byte) Type string Parse func([]byte) (bool, int) }{ - {Type: "nvme", Parse: sm.parseSmartForNvme}, - {Type: "sat", Parse: sm.parseSmartForSata}, - {Type: "scsi", Parse: sm.parseSmartForScsi}, + {Type: "nvme", Parse: func(output []byte) (bool, int) { + return sm.parseSmartForNvme(output, deviceInfo.Type) + }}, + {Type: "sat", Parse: func(output []byte) (bool, int) { + return sm.parseSmartForSata(output, deviceInfo.Type) + }}, + {Type: "scsi", Parse: func(output []byte) (bool, int) { + return sm.parseSmartForScsi(output, deviceInfo.Type) + }}, } deviceType := normalizeParserType(deviceInfo.parserType) @@ -486,10 +492,11 @@ func (sm *SmartManager) CollectSmart(deviceInfo *DeviceInfo) error { return errNoValidSmartData } - // slog.Info("collecting SMART data", "device", deviceInfo.Name, "type", deviceInfo.Type, "has_existing_data", sm.hasDataForDevice(deviceInfo.Name)) + // slog.Info("collecting SMART data", "device", deviceInfo.Name, "type", deviceInfo.Type, "has_existing_data", sm.hasDataForDevice(deviceInfo)) - // Check if we have any existing data for this device - hasExistingData := sm.hasDataForDevice(deviceInfo.Name) + // Check if we have existing data for this exact device identity. Multiple + // bridge slots can share a path, so a name-only match is not sufficient. + hasExistingData := sm.hasDataForDevice(deviceInfo) ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) defer cancel() @@ -592,14 +599,18 @@ func (sm *SmartManager) smartctlArgs(deviceInfo *DeviceInfo, includeStandby bool return args } -// hasDataForDevice checks if we have cached SMART data for a specific device -func (sm *SmartManager) hasDataForDevice(deviceName string) bool { +// hasDataForDevice checks if we have cached SMART data for a specific device identity. +func (sm *SmartManager) hasDataForDevice(deviceInfo *DeviceInfo) bool { + if deviceInfo == nil { + return false + } + sm.Lock() defer sm.Unlock() - // Check if any cached data has this device name + deviceKey := makeDeviceKey(deviceInfo.Name, deviceInfo.Type) for _, data := range sm.SmartDataMap { - if data != nil && data.DiskName == deviceName { + if data != nil && makeDeviceKey(data.DiskName, data.DiskType) == deviceKey { return true } } @@ -747,7 +758,14 @@ func mergeDeviceLists(existing, scanned, configured []*DeviceInfo) []*DeviceInfo continue } if existingDev := deviceIndexByName[configuredDevice.Name]; existingDev != nil { + oldKey := makeDeviceKey(existingDev.Name, existingDev.Type) + if prev := existingIndex[key]; prev != nil { + preserveVerifiedType(existingDev, prev) + } applyConfiguredMetadata(existingDev, configuredDevice) + delete(deviceIndex, oldKey) + deviceIndex[makeDeviceKey(existingDev.Name, existingDev.Type)] = existingDev + delete(deviceIndexByName, configuredDevice.Name) continue } @@ -851,9 +869,11 @@ func (sm *SmartManager) isVirtualDeviceFromStrings(fields ...string) bool { return false } -// parseSmartForSata parses the output of smartctl --all -j for SATA/ATA devices and updates the SmartDataMap +// parseSmartForSata parses the output of smartctl --all -j for SATA/ATA devices and updates the SmartDataMap. +// deviceType is the exact type used to identify and query the device; when set, +// it takes precedence over the generic type reported by smartctl. // Returns hasValidData and exitStatus -func (sm *SmartManager) parseSmartForSata(output []byte) (bool, int) { +func (sm *SmartManager) parseSmartForSata(output []byte, deviceType string) (bool, int) { var data smart.SmartInfoForSata if err := json.Unmarshal(output, &data); err != nil { @@ -892,6 +912,9 @@ func (sm *SmartManager) parseSmartForSata(output []byte) (bool, int) { smartData.SmartStatus = getSmartStatus(smartData.Temperature, data.SmartStatus.Passed) smartData.DiskName = data.Device.Name smartData.DiskType = data.Device.Type + if deviceType != "" { + smartData.DiskType = deviceType + } // get values from ata_device_statistics if necessary var ataDeviceStats smart.AtaDeviceStatistics @@ -965,7 +988,7 @@ func findAtaDeviceStatisticsValue(data *smart.SmartInfoForSata, ataDeviceStats * return nil } -func (sm *SmartManager) parseSmartForScsi(output []byte) (bool, int) { +func (sm *SmartManager) parseSmartForScsi(output []byte, deviceType string) (bool, int) { var data smart.SmartInfoForScsi if err := json.Unmarshal(output, &data); err != nil { @@ -1000,6 +1023,9 @@ func (sm *SmartManager) parseSmartForScsi(output []byte) (bool, int) { smartData.SmartStatus = getSmartStatus(smartData.Temperature, data.SmartStatus.Passed) smartData.DiskName = data.Device.Name smartData.DiskType = data.Device.Type + if deviceType != "" { + smartData.DiskType = deviceType + } attributes := make([]*smart.SmartAttribute, 0, 10) attributes = append(attributes, &smart.SmartAttribute{Name: "PowerOnHours", RawValue: data.PowerOnTime.Hours}) @@ -1097,9 +1123,11 @@ func (sm *SmartManager) lookupDarwinNvmeCapacity(serial string) uint64 { return sm.darwinNvmeCapacity[serial] } -// parseSmartForNvme parses the output of smartctl --all -j /dev/nvmeX and updates the SmartDataMap +// parseSmartForNvme parses the output of smartctl --all -j /dev/nvmeX and updates the SmartDataMap. +// deviceType is the exact type used to identify and query the device; when set, +// it takes precedence over the generic type reported by smartctl. // Returns hasValidData and exitStatus -func (sm *SmartManager) parseSmartForNvme(output []byte) (bool, int) { +func (sm *SmartManager) parseSmartForNvme(output []byte, deviceType string) (bool, int) { data := &smart.SmartInfoForNvme{} if err := json.Unmarshal(output, &data); err != nil { @@ -1143,6 +1171,9 @@ func (sm *SmartManager) parseSmartForNvme(output []byte) (bool, int) { smartData.SmartStatus = getSmartStatus(smartData.Temperature, data.SmartStatus.Passed) smartData.DiskName = data.Device.Name smartData.DiskType = data.Device.Type + if deviceType != "" { + smartData.DiskType = deviceType + } // nvme attributes does not follow the same format as ata attributes, // so we manually map each field to SmartAttributes diff --git a/agent/smart_test.go b/agent/smart_test.go index 65ad4ac5..e2296db9 100644 --- a/agent/smart_test.go +++ b/agent/smart_test.go @@ -24,7 +24,7 @@ func TestParseSmartForScsi(t *testing.T) { SmartDataMap: make(map[string]*smart.SmartData), } - hasData, exitStatus := sm.parseSmartForScsi(data) + hasData, exitStatus := sm.parseSmartForScsi(data, "") if !hasData { t.Fatalf("expected SCSI data to parse successfully") } @@ -69,7 +69,7 @@ func TestParseSmartForSata(t *testing.T) { SmartDataMap: make(map[string]*smart.SmartData), } - hasData, exitStatus := sm.parseSmartForSata(data) + hasData, exitStatus := sm.parseSmartForSata(data, "") require.True(t, hasData) assert.Equal(t, 64, exitStatus) @@ -112,7 +112,7 @@ func TestParseSmartForSataDeviceStatisticsTemperature(t *testing.T) { }`) sm := &SmartManager{SmartDataMap: make(map[string]*smart.SmartData)} - hasData, exitStatus := sm.parseSmartForSata(jsonPayload) + hasData, exitStatus := sm.parseSmartForSata(jsonPayload, "") require.True(t, hasData) assert.Equal(t, 0, exitStatus) @@ -147,7 +147,7 @@ func TestParseSmartForSataAtaDeviceStatistics(t *testing.T) { }`) sm := &SmartManager{SmartDataMap: make(map[string]*smart.SmartData)} - hasData, exitStatus := sm.parseSmartForSata(jsonPayload) + hasData, exitStatus := sm.parseSmartForSata(jsonPayload, "") require.True(t, hasData) assert.Equal(t, 0, exitStatus) @@ -184,7 +184,7 @@ func TestParseSmartForSataNegativeDeviceStatistics(t *testing.T) { }`) sm := &SmartManager{SmartDataMap: make(map[string]*smart.SmartData)} - hasData, exitStatus := sm.parseSmartForSata(jsonPayload) + hasData, exitStatus := sm.parseSmartForSata(jsonPayload, "") require.True(t, hasData) assert.Equal(t, 0, exitStatus) @@ -223,7 +223,7 @@ func TestParseSmartForSataParentheticalRawValue(t *testing.T) { sm := &SmartManager{SmartDataMap: make(map[string]*smart.SmartData)} - hasData, exitStatus := sm.parseSmartForSata(jsonPayload) + hasData, exitStatus := sm.parseSmartForSata(jsonPayload, "") require.True(t, hasData) assert.Equal(t, 0, exitStatus) @@ -245,7 +245,7 @@ func TestParseSmartForNvme(t *testing.T) { SmartDataMap: make(map[string]*smart.SmartData), } - hasData, exitStatus := sm.parseSmartForNvme(data) + hasData, exitStatus := sm.parseSmartForNvme(data, "") require.True(t, hasData) assert.Equal(t, 0, exitStatus) @@ -268,13 +268,15 @@ func TestParseSmartForNvme(t *testing.T) { func TestHasDataForDevice(t *testing.T) { sm := &SmartManager{ SmartDataMap: map[string]*smart.SmartData{ - "serial-1": {DiskName: "/dev/sda"}, + "serial-1": {DiskName: "/dev/sda", DiskType: "jms56x,0"}, "serial-2": nil, }, } - assert.True(t, sm.hasDataForDevice("/dev/sda")) - assert.False(t, sm.hasDataForDevice("/dev/sdb")) + assert.True(t, sm.hasDataForDevice(&DeviceInfo{Name: "/dev/sda", Type: "jms56x,0"})) + assert.False(t, sm.hasDataForDevice(&DeviceInfo{Name: "/dev/sda", Type: "jms56x,1"})) + assert.False(t, sm.hasDataForDevice(&DeviceInfo{Name: "/dev/sdb", Type: "jms56x,0"})) + assert.False(t, sm.hasDataForDevice(nil)) } func TestDevicesSnapshotReturnsCopy(t *testing.T) { @@ -609,6 +611,74 @@ func TestMergeDeviceListsPrefersConfigured(t *testing.T) { assert.Equal(t, "sat", byName["/dev/sdb"].Type) } +func TestMergeDeviceListsExpandsConfiguredDevicesWithSamePath(t *testing.T) { + scanned := []*DeviceInfo{ + {Name: "/dev/sdb", Type: "sat", InfoName: "scan-info", Protocol: "ATA"}, + } + configured := []*DeviceInfo{ + {Name: "/dev/sdb", Type: "jms56x,0", explicitType: true}, + {Name: "/dev/sdb", Type: "jms56x,1", explicitType: true}, + } + + merged := mergeDeviceLists(nil, scanned, configured) + require.Len(t, merged, 2) + + byKey := make(map[deviceKey]*DeviceInfo, len(merged)) + for _, device := range merged { + byKey[makeDeviceKey(device.Name, device.Type)] = device + } + + first := byKey[makeDeviceKey("/dev/sdb", "jms56x,0")] + require.NotNil(t, first) + assert.Equal(t, "scan-info", first.InfoName) + assert.Equal(t, "ATA", first.Protocol) + assert.True(t, first.explicitType) + + second := byKey[makeDeviceKey("/dev/sdb", "jms56x,1")] + require.NotNil(t, second) + assert.True(t, second.explicitType) + assert.NotContains(t, byKey, makeDeviceKey("/dev/sdb", "sat")) +} + +func TestMergeDeviceListsPreservesSamePathVerificationAcrossRescan(t *testing.T) { + existing := []*DeviceInfo{ + {Name: "/dev/sdb", Type: "jms56x,0", parserType: "sat", typeVerified: true, explicitType: true}, + {Name: "/dev/sdb", Type: "jms56x,1", parserType: "sat", typeVerified: true, explicitType: true}, + } + scanned := []*DeviceInfo{ + {Name: "/dev/sdb", Type: "sat", Protocol: "ATA"}, + } + configured := []*DeviceInfo{ + {Name: "/dev/sdb", Type: "jms56x,0", explicitType: true}, + {Name: "/dev/sdb", Type: "jms56x,1", explicitType: true}, + } + + merged := mergeDeviceLists(existing, scanned, configured) + require.Len(t, merged, 2) + byKey := make(map[deviceKey]*DeviceInfo, len(merged)) + for _, device := range merged { + byKey[makeDeviceKey(device.Name, device.Type)] = device + assert.True(t, device.typeVerified, device.Type) + assert.Equal(t, "sat", device.parserType, device.Type) + assert.True(t, device.explicitType, device.Type) + } + assert.Contains(t, byKey, makeDeviceKey("/dev/sdb", "jms56x,0")) + assert.Contains(t, byKey, makeDeviceKey("/dev/sdb", "jms56x,1")) +} + +func TestMergeDeviceListsDeduplicatesConfiguredIdentityAfterRekey(t *testing.T) { + scanned := []*DeviceInfo{{Name: "/dev/sdb", Type: "sat"}} + configured := []*DeviceInfo{ + {Name: "/dev/sdb", Type: "jms56x,0", explicitType: true}, + {Name: "/dev/sdb", Type: "jms56x,0", explicitType: true}, + } + + merged := mergeDeviceLists(nil, scanned, configured) + require.Len(t, merged, 1) + assert.Equal(t, "/dev/sdb", merged[0].Name) + assert.Equal(t, "jms56x,0", merged[0].Type) +} + func TestMergeDeviceListsPreservesVerification(t *testing.T) { existing := []*DeviceInfo{ {Name: "/dev/sda", Type: "sat+megaraid", parserType: "sat", typeVerified: true}, @@ -753,6 +823,20 @@ func TestParseSmartOutputKeepsCustomType(t *testing.T) { assert.Equal(t, "sat+megaraid", device.Type) assert.Equal(t, "sat", device.parserType) assert.True(t, device.typeVerified) + assert.Equal(t, "sat+megaraid", sm.SmartDataMap["9C40918040082"].DiskType) +} + +func TestParseSmartOutputDoesNotNormalizeDeviceIdentity(t *testing.T) { + fixturePath := filepath.Join("test-data", "smart", "sda.json") + data, err := os.ReadFile(fixturePath) + require.NoError(t, err) + + sm := &SmartManager{SmartDataMap: make(map[string]*smart.SmartData)} + device := &DeviceInfo{Name: "/dev/sda", Type: "ata", explicitType: true} + + require.True(t, sm.parseSmartOutput(device, data)) + assert.Equal(t, "sat", device.parserType) + assert.Equal(t, "ata", sm.SmartDataMap["9C40918040082"].DiskType) } func TestParseSmartOutputResetsVerificationOnFailure(t *testing.T) { @@ -1300,7 +1384,7 @@ func TestParseSmartForNvmeAppleSSD(t *testing.T) { darwinNvmeProvider: fakeProvider, } - hasData, _ := sm.parseSmartForNvme(data) + hasData, _ := sm.parseSmartForNvme(data, "") require.True(t, hasData) deviceData, ok := sm.SmartDataMap["0ba0147940253c15"] @@ -1312,7 +1396,7 @@ func TestParseSmartForNvmeAppleSSD(t *testing.T) { assert.Equal(t, 1, providerCalls, "system_profiler should be called once") // Second parse: provider should NOT be called again (cache hit) - _, _ = sm.parseSmartForNvme(data) + _, _ = sm.parseSmartForNvme(data, "") assert.Equal(t, 1, providerCalls, "system_profiler should not be called again after caching") }