From 3d33d5eabb2bed36b5ff252057a96647dbd8c8c7 Mon Sep 17 00:00:00 2001 From: spatiumstas <79056064+spatiumstas@users.noreply.github.com> Date: Mon, 12 Oct 2026 16:28:44 +0300 Subject: [PATCH] fix(agent): preserve NVMe namespace fallbacks across rescans (#2597) --- agent/smart.go | 66 ++++++++++++++++++++----- agent/smart_test.go | 116 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 168 insertions(+), 14 deletions(-) diff --git a/agent/smart.go b/agent/smart.go index 0834d19e4..fb7e97776 100644 --- a/agent/smart.go +++ b/agent/smart.go @@ -31,6 +31,7 @@ type SmartManager struct { lastScanTime time.Time smartctlPath string excludedDevices map[string]struct{} + nvmeNamespacePaths map[string]string // controller to working namespace; guarded by Mutex darwinNvmeOnce sync.Once darwinNvmeCapacity map[string]uint64 // serial → bytes cache, written once via darwinNvmeOnce darwinNvmeProvider func() ([]byte, error) // overridable for testing @@ -216,6 +217,8 @@ func (sm *SmartManager) ScanDevices(force bool) error { hasValidScan = true } + scannedDevices = sm.resolveNvmeNamespacePaths(scannedDevices) + configuredDevices = sm.resolveNvmeNamespacePaths(configuredDevices) finalDevices := mergeDeviceLists(currentDevices, scannedDevices, configuredDevices) finalDevices = sm.filterExcludedDevices(finalDevices) sm.updateSmartDevices(finalDevices) @@ -231,6 +234,39 @@ func (sm *SmartManager) ScanDevices(force bool) error { return nil } +// resolveNvmeNamespacePaths keeps successful namespace fallbacks when a later +// scan (or SMART_DEVICES) reports the original controller path again. Preserve +// the controller's presence in the scan so removed drives can still be pruned. +func (sm *SmartManager) resolveNvmeNamespacePaths(devices []*DeviceInfo) []*DeviceInfo { + sm.Lock() + defer sm.Unlock() + + if len(sm.nvmeNamespacePaths) == 0 { + return devices + } + resolved := make([]*DeviceInfo, 0, len(devices)) + seen := make(map[deviceKey]struct{}, len(devices)) + for _, device := range devices { + if device == nil { + continue + } + // Keep excluded controller entries unchanged until filterExcludedDevices + // runs, so an empty input cannot make mergeDeviceLists retain old devices. + if namespace := sm.nvmeNamespacePaths[device.Name]; namespace != "" && !sm.isExcludedDevice(device.Name) { + copyDevice := *device + copyDevice.Name = namespace + device = ©Device + } + key := makeDeviceKey(device.Name, device.Type) + if _, ok := seen[key]; ok { + continue + } + seen[key] = struct{}{} + resolved = append(resolved, device) + } + return resolved +} + func (sm *SmartManager) parseConfiguredDevices(config string) ([]*DeviceInfo, error) { splitChar, _ := utils.GetEnv("SMART_DEVICES_SEPARATOR") if splitChar == "" { @@ -535,24 +571,26 @@ func (sm *SmartManager) CollectSmart(deviceInfo *DeviceInfo) error { controllerPath := deviceInfo.Name namespacePath := controllerPath + "n1" if !sm.isExcludedDevice(namespacePath) { - deviceInfo.Name = namespacePath + namespaceDevice := *deviceInfo + namespaceDevice.Name = namespacePath ctx3, cancel3 := context.WithTimeout(context.Background(), 15*time.Second) defer cancel3() - args = sm.smartctlArgs(deviceInfo, false) + args = sm.smartctlArgs(&namespaceDevice, false) cmd = exec.CommandContext(ctx3, sm.smartctlPath, args...) output, err = cmd.CombinedOutput() - hasValidData = sm.parseSmartOutput(deviceInfo, output) + hasValidData = sm.parseSmartOutput(&namespaceDevice, output) - // Auto-exclude the controller path so future scans don't re-add it + // Resolve future controller scan results to this working namespace. if hasValidData { sm.Lock() - if sm.excludedDevices == nil { - sm.excludedDevices = make(map[string]struct{}) + *deviceInfo = namespaceDevice + if sm.nvmeNamespacePaths == nil { + sm.nvmeNamespacePaths = make(map[string]string) } - sm.excludedDevices[controllerPath] = struct{}{} + sm.nvmeNamespacePaths[controllerPath] = namespacePath sm.Unlock() - slog.Debug("auto-excluded NVMe controller path", "path", controllerPath) + slog.Debug("using NVMe namespace path", "controller", controllerPath, "namespace", namespacePath) } } } @@ -802,10 +840,6 @@ func (sm *SmartManager) updateSmartDevices(devices []*DeviceInfo) { sm.SmartDevices = devices - if len(sm.SmartDataMap) == 0 { - return - } - validKeys := make(map[deviceKey]struct{}, len(devices)) nameCounts := make(map[string]int, len(devices)) for _, device := range devices { @@ -816,6 +850,14 @@ func (sm *SmartManager) updateSmartDevices(devices []*DeviceInfo) { nameCounts[device.Name]++ } + // Forget aliases when their device disappears, so a replacement controller + // can be queried normally instead of inheriting an old namespace fallback. + for controller, namespace := range sm.nvmeNamespacePaths { + if nameCounts[controller] == 0 && nameCounts[namespace] == 0 { + delete(sm.nvmeNamespacePaths, controller) + } + } + for key, data := range sm.SmartDataMap { if data == nil { delete(sm.SmartDataMap, key) diff --git a/agent/smart_test.go b/agent/smart_test.go index f403abb7f..7bbc5673d 100644 --- a/agent/smart_test.go +++ b/agent/smart_test.go @@ -3,6 +3,7 @@ package agent import ( + "encoding/json" "errors" "fmt" "os" @@ -10,6 +11,7 @@ import ( "regexp" "strings" "testing" + "time" "github.com/henrygd/beszel/internal/entities/smart" "github.com/stretchr/testify/assert" @@ -1516,7 +1518,15 @@ func init() { if fixture == "" { return } - if isNvmeControllerPath(os.Args[len(os.Args)-1]) { + if os.Args[1] == "--scan" { + if scan := os.Getenv("BESZEL_SMART_TEST_SCAN"); scan != "" { + fmt.Print(scan) + } else { + fmt.Print(`{"devices":[{"name":"/dev/nvme0","type":"nvme","protocol":"NVMe"}]}`) + } + os.Exit(0) + } + if isNvmeControllerPath(os.Args[len(os.Args)-1]) || os.Getenv("BESZEL_SMART_TEST_NAMESPACE_FAIL") == "true" { fmt.Print(`{"smartctl":{"exit_status":2,"messages":[{"string":"Permission denied","severity":"error"}]}}`) os.Exit(2) } @@ -1545,7 +1555,109 @@ func TestCollectSmartNvmeNamespaceFallback(t *testing.T) { require.NoError(t, sm.CollectSmart(device)) assert.Equal(t, "/dev/nvme0n1", device.Name) assert.Equal(t, "/dev/nvme0n1", sm.SmartDataMap["2024031600129"].DiskName) - assert.True(t, sm.isExcludedDevice("/dev/nvme0")) + assert.Equal(t, "/dev/nvme0n1", sm.nvmeNamespacePaths["/dev/nvme0"]) + assert.False(t, sm.isExcludedDevice("/dev/nvme0")) + }) + } +} + +func TestRefreshNvmeNamespaceFallbackSurvivesRescan(t *testing.T) { + fixture, err := filepath.Abs(filepath.Join("test-data", "smart", "nvme0.json")) + require.NoError(t, err) + t.Setenv("BESZEL_SMART_TEST_FIXTURE", fixture) + executable, err := os.Executable() + require.NoError(t, err) + sm := &SmartManager{smartctlPath: executable, SmartDataMap: make(map[string]*smart.SmartData)} + + complete, err := sm.Refresh(false) + require.NoError(t, err) + require.True(t, complete) + require.Len(t, sm.SmartDevices, 1) + require.Equal(t, "/dev/nvme0n1", sm.SmartDevices[0].Name) + + // Return a newer health reading to prove the next refresh queries the disk, + // rather than only preserving the previous cached data. + fixtureData, err := os.ReadFile(fixture) + require.NoError(t, err) + var updatedData smart.SmartInfoForNvme + require.NoError(t, json.Unmarshal(fixtureData, &updatedData)) + require.NotNil(t, updatedData.NVMeSmartHealthInformationLog) + updatedData.NVMeSmartHealthInformationLog.Temperature++ + fixtureData, err = json.Marshal(updatedData) + require.NoError(t, err) + updatedFixture := filepath.Join(t.TempDir(), "nvme.json") + require.NoError(t, os.WriteFile(updatedFixture, fixtureData, 0o600)) + t.Setenv("BESZEL_SMART_TEST_FIXTURE", updatedFixture) + + // Simulate the next refresh after the 30-minute scan cache has expired. + sm.lastScanTime = time.Now().Add(-31 * time.Minute) + complete, err = sm.Refresh(false) + require.NoError(t, err) + require.True(t, complete) + require.Len(t, sm.SmartDevices, 1) + assert.Equal(t, "/dev/nvme0n1", sm.SmartDevices[0].Name) + require.Contains(t, sm.SmartDataMap, "2024031600129") + assert.Equal(t, "/dev/nvme0n1", sm.SmartDataMap["2024031600129"].DiskName) + assert.Equal(t, updatedData.NVMeSmartHealthInformationLog.Temperature, sm.SmartDataMap["2024031600129"].Temperature) + assert.True(t, sm.SmartDevices[0].typeVerified) + assert.Equal(t, "nvme", sm.SmartDevices[0].parserType) + + // Explicit controller configuration must keep resolving to the namespace. + t.Setenv("SMART_DEVICES", "/dev/nvme0:nvme") + require.NoError(t, sm.ScanDevices(true)) + require.Len(t, sm.SmartDevices, 1) + assert.Equal(t, "/dev/nvme0n1", sm.SmartDevices[0].Name) + require.Contains(t, sm.SmartDataMap, "2024031600129") + + // A scan that reports both aliases must not duplicate the drive. + t.Setenv("SMART_DEVICES", "") + require.NoError(t, os.Unsetenv("SMART_DEVICES")) + t.Setenv("BESZEL_SMART_TEST_SCAN", `{"devices":[{"name":"/dev/nvme0","type":"nvme"},{"name":"/dev/nvme0n1","type":"nvme"}]}`) + require.NoError(t, sm.ScanDevices(true)) + require.Len(t, sm.SmartDevices, 1) + + // Retaining a fallback must not retain a drive absent from a valid scan. + t.Setenv("BESZEL_SMART_TEST_SCAN", `{"devices":[{"name":"/dev/sda","type":"sat"}]}`) + require.NoError(t, sm.ScanDevices(true)) + require.Len(t, sm.SmartDevices, 1) + assert.Equal(t, "/dev/sda", sm.SmartDevices[0].Name) + assert.Empty(t, sm.SmartDataMap) + assert.Empty(t, sm.nvmeNamespacePaths) +} + +func TestCollectSmartNvmeNamespaceFailureKeepsController(t *testing.T) { + t.Setenv("BESZEL_SMART_TEST_FIXTURE", "unused") + t.Setenv("BESZEL_SMART_TEST_NAMESPACE_FAIL", "true") + executable, err := os.Executable() + require.NoError(t, err) + device := &DeviceInfo{Name: "/dev/nvme0", Type: "nvme"} + sm := &SmartManager{smartctlPath: executable, SmartDataMap: make(map[string]*smart.SmartData)} + + require.Error(t, sm.CollectSmart(device)) + assert.Equal(t, "/dev/nvme0", device.Name) + assert.Empty(t, sm.nvmeNamespacePaths) + assert.Empty(t, sm.SmartDataMap) +} + +func TestNvmeNamespaceFallbackHonorsExclusions(t *testing.T) { + fixture, err := filepath.Abs(filepath.Join("test-data", "smart", "nvme0.json")) + require.NoError(t, err) + t.Setenv("BESZEL_SMART_TEST_FIXTURE", fixture) + executable, err := os.Executable() + require.NoError(t, err) + for _, excluded := range []string{"/dev/nvme0", "/dev/nvme0n1"} { + t.Run(excluded, func(t *testing.T) { + sm := &SmartManager{ + smartctlPath: executable, + SmartDevices: []*DeviceInfo{{Name: "/dev/nvme0n1", Type: "nvme"}}, + SmartDataMap: map[string]*smart.SmartData{"2024031600129": {DiskName: "/dev/nvme0n1", DiskType: "nvme"}}, + nvmeNamespacePaths: map[string]string{"/dev/nvme0": "/dev/nvme0n1"}, + excludedDevices: map[string]struct{}{excluded: {}}, + } + require.ErrorIs(t, sm.ScanDevices(true), errNoValidSmartData) + assert.Empty(t, sm.SmartDevices) + assert.Empty(t, sm.SmartDataMap) + assert.Empty(t, sm.nvmeNamespacePaths) }) } }