From 648a979a817e10cc1254b2a864d1b628556b1f37 Mon Sep 17 00:00:00 2001 From: Julian Nadeau Date: Wed, 21 Jan 2026 17:58:20 -0500 Subject: [PATCH] Add SMART_DEVICES_SEPARATOR + allow drives with the same name to be added with different types (e.g. raid controllers) (#1655) * Add SMART_DEVICES_SEPARATOR to override , * Allow composite keys in smart devices for raid controller support --- agent/smart.go | 39 ++++++++++++----- agent/smart_test.go | 100 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 129 insertions(+), 10 deletions(-) diff --git a/agent/smart.go b/agent/smart.go index bc133632..a59f9d43 100644 --- a/agent/smart.go +++ b/agent/smart.go @@ -202,7 +202,11 @@ func (sm *SmartManager) ScanDevices(force bool) error { } func (sm *SmartManager) parseConfiguredDevices(config string) ([]*DeviceInfo, error) { - entries := strings.Split(config, ",") + splitChar := os.Getenv("SMART_DEVICES_SEPARATOR") + if splitChar == "" { + splitChar = "," + } + entries := strings.Split(config, splitChar) devices := make([]*DeviceInfo, 0, len(entries)) for _, entry := range entries { entry = strings.TrimSpace(entry) @@ -326,6 +330,16 @@ func normalizeParserType(value string) string { } } +// deviceKey creates a composite key from device name and type. +// This allows multiple drives under the same device path (e.g., RAID controllers) +// to be tracked separately. +func deviceKey(name, deviceType string) string { + if deviceType == "" { + return name + } + return name + "|" + deviceType +} + // parseSmartOutput attempts each SMART parser, optionally detecting the type when // it is not provided, and updates the device info when a parser succeeds. func (sm *SmartManager) parseSmartOutput(deviceInfo *DeviceInfo, output []byte) bool { @@ -586,7 +600,7 @@ func mergeDeviceLists(existing, scanned, configured []*DeviceInfo) []*DeviceInfo if dev == nil || dev.Name == "" { continue } - existingIndex[dev.Name] = dev + existingIndex[deviceKey(dev.Name, dev.Type)] = dev } finalDevices := make([]*DeviceInfo, 0, len(scanned)+len(configured)) @@ -602,12 +616,14 @@ func mergeDeviceLists(existing, scanned, configured []*DeviceInfo) []*DeviceInfo // Work on a copy so we can safely adjust metadata without mutating the // input slices that may be reused elsewhere. copyDev := *dev - if prev := existingIndex[copyDev.Name]; prev != nil { + key := deviceKey(copyDev.Name, copyDev.Type) + if prev := existingIndex[key]; prev != nil { preserveVerifiedType(©Dev, prev) } finalDevices = append(finalDevices, ©Dev) - deviceIndex[copyDev.Name] = finalDevices[len(finalDevices)-1] + copyKey := deviceKey(copyDev.Name, copyDev.Type) + deviceIndex[copyKey] = finalDevices[len(finalDevices)-1] } // Merge configured devices on top so users can override scan results (except @@ -617,7 +633,8 @@ func mergeDeviceLists(existing, scanned, configured []*DeviceInfo) []*DeviceInfo continue } - if existingDev, ok := deviceIndex[dev.Name]; ok { + key := deviceKey(dev.Name, dev.Type) + if existingDev, ok := deviceIndex[key]; ok { // Only update the type if it has not been verified yet; otherwise we // keep the existing verified metadata intact. if dev.Type != "" && !existingDev.typeVerified { @@ -636,14 +653,16 @@ func mergeDeviceLists(existing, scanned, configured []*DeviceInfo) []*DeviceInfo } copyDev := *dev - if prev := existingIndex[copyDev.Name]; prev != nil { + key = deviceKey(copyDev.Name, copyDev.Type) + if prev := existingIndex[key]; prev != nil { preserveVerifiedType(©Dev, prev) } else if copyDev.Type != "" { copyDev.parserType = normalizeParserType(copyDev.Type) } finalDevices = append(finalDevices, ©Dev) - deviceIndex[copyDev.Name] = finalDevices[len(finalDevices)-1] + copyKey := deviceKey(copyDev.Name, copyDev.Type) + deviceIndex[copyKey] = finalDevices[len(finalDevices)-1] } return finalDevices @@ -661,12 +680,12 @@ func (sm *SmartManager) updateSmartDevices(devices []*DeviceInfo) { return } - validNames := make(map[string]struct{}, len(devices)) + validKeys := make(map[string]struct{}, len(devices)) for _, device := range devices { if device == nil || device.Name == "" { continue } - validNames[device.Name] = struct{}{} + validKeys[deviceKey(device.Name, device.Type)] = struct{}{} } for key, data := range sm.SmartDataMap { @@ -675,7 +694,7 @@ func (sm *SmartManager) updateSmartDevices(devices []*DeviceInfo) { continue } - if _, ok := validNames[data.DiskName]; ok { + if _, ok := validKeys[deviceKey(data.DiskName, data.DiskType)]; ok { continue } diff --git a/agent/smart_test.go b/agent/smart_test.go index 08d5d467..e75ec688 100644 --- a/agent/smart_test.go +++ b/agent/smart_test.go @@ -195,6 +195,24 @@ func TestDevicesSnapshotReturnsCopy(t *testing.T) { assert.Len(t, snapshot, 2) } +func TestScanDevicesWithEnvOverrideAndSeparator(t *testing.T) { + t.Setenv("SMART_DEVICES_SEPARATOR", "|") + t.Setenv("SMART_DEVICES", "/dev/sda:jmb39x-q,0|/dev/nvme0:nvme") + + sm := &SmartManager{ + SmartDataMap: make(map[string]*smart.SmartData), + } + + err := sm.ScanDevices(true) + require.NoError(t, err) + + require.Len(t, sm.SmartDevices, 2) + assert.Equal(t, "/dev/sda", sm.SmartDevices[0].Name) + assert.Equal(t, "jmb39x-q,0", sm.SmartDevices[0].Type) + assert.Equal(t, "/dev/nvme0", sm.SmartDevices[1].Name) + assert.Equal(t, "nvme", sm.SmartDevices[1].Type) +} + func TestScanDevicesWithEnvOverride(t *testing.T) { t.Setenv("SMART_DEVICES", "/dev/sda:sat, /dev/nvme0:nvme") @@ -442,6 +460,88 @@ func TestMergeDeviceListsUpdatesTypeWhenUnverified(t *testing.T) { assert.Equal(t, "", device.parserType) } +func TestMergeDeviceListsHandlesDevicesWithSameNameAndDifferentTypes(t *testing.T) { + // There are use cases where the same device name is re-used, + // for example, a RAID controller with multiple drives. + scanned := []*DeviceInfo{ + {Name: "/dev/sda", Type: "megaraid,0"}, + {Name: "/dev/sda", Type: "megaraid,1"}, + {Name: "/dev/sda", Type: "megaraid,2"}, + } + + merged := mergeDeviceLists(nil, scanned, nil) + require.Len(t, merged, 3, "should have 3 separate devices for RAID controller") + + byKey := make(map[string]*DeviceInfo, len(merged)) + for _, dev := range merged { + key := dev.Name + "|" + dev.Type + byKey[key] = dev + } + + assert.Contains(t, byKey, "/dev/sda|megaraid,0") + assert.Contains(t, byKey, "/dev/sda|megaraid,1") + assert.Contains(t, byKey, "/dev/sda|megaraid,2") +} + +func TestMergeDeviceListsHandlesMixedRAIDAndRegular(t *testing.T) { + // Test mixing RAID drives with regular devices + scanned := []*DeviceInfo{ + {Name: "/dev/sda", Type: "megaraid,0"}, + {Name: "/dev/sda", Type: "megaraid,1"}, + {Name: "/dev/sdb", Type: "sat"}, + {Name: "/dev/nvme0", Type: "nvme"}, + } + + merged := mergeDeviceLists(nil, scanned, nil) + require.Len(t, merged, 4, "should have 4 separate devices") + + byKey := make(map[string]*DeviceInfo, len(merged)) + for _, dev := range merged { + key := dev.Name + "|" + dev.Type + byKey[key] = dev + } + + assert.Contains(t, byKey, "/dev/sda|megaraid,0") + assert.Contains(t, byKey, "/dev/sda|megaraid,1") + assert.Contains(t, byKey, "/dev/sdb|sat") + assert.Contains(t, byKey, "/dev/nvme0|nvme") +} + +func TestUpdateSmartDevicesPreservesRAIDDrives(t *testing.T) { + // Test that updateSmartDevices correctly validates RAID drives using composite keys + sm := &SmartManager{ + SmartDevices: []*DeviceInfo{ + {Name: "/dev/sda", Type: "megaraid,0"}, + {Name: "/dev/sda", Type: "megaraid,1"}, + }, + SmartDataMap: map[string]*smart.SmartData{ + "serial-0": { + DiskName: "/dev/sda", + DiskType: "megaraid,0", + SerialNumber: "serial-0", + }, + "serial-1": { + DiskName: "/dev/sda", + DiskType: "megaraid,1", + SerialNumber: "serial-1", + }, + "serial-stale": { + DiskName: "/dev/sda", + DiskType: "megaraid,2", + SerialNumber: "serial-stale", + }, + }, + } + + sm.updateSmartDevices(sm.SmartDevices) + + // serial-0 and serial-1 should be preserved (matching devices exist) + assert.Contains(t, sm.SmartDataMap, "serial-0") + assert.Contains(t, sm.SmartDataMap, "serial-1") + // serial-stale should be removed (no matching device) + assert.NotContains(t, sm.SmartDataMap, "serial-stale") +} + func TestParseSmartOutputMarksVerified(t *testing.T) { fixturePath := filepath.Join("test-data", "smart", "nvme0.json") data, err := os.ReadFile(fixturePath)