From b44084af3c3d0f777e43dab1c5c59cc191907f13 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Wed, 5 Nov 2025 19:32:44 +0000 Subject: [PATCH] Skip false health alerts for Samsung 980/990 SSDs and improve Docker CPU calculation Related to #547 and #622 ## Samsung SSD Fix (#547) Samsung 980 and 990 series SSDs have known firmware bugs that cause them to report incorrect health status (typically FAILED or critical warnings) even when the drives are actually healthy. This is commonly due to incorrect temperature threshold reporting in the firmware. This change adds special handling to detect these drives and skip health status alerts while still monitoring wearout metrics, which remain reliable. The fix also clears any existing false alerts for these drives. Users experiencing these false alerts should update their Samsung SSD firmware to the latest version from Samsung, which typically resolves the issue. ## Docker Agent CPU Fix (#622) Addresses issue where Docker container CPU usage shows 0%. The Docker agent uses ContainerStatsOneShot which typically doesn't populate PreCPUStats, requiring manual delta tracking between collection cycles. Changes: - Fix logic bug where prevContainerCPU was updated before checking if previous sample existed, causing incorrect delta calculations - Add comprehensive debug logging showing which calculation method succeeded (PreCPUStats, system delta, or time-based fallback) - Add warning after 10 PreCPUStats failures to inform about manual tracking mode (normal for one-shot stats) - Add detailed failure logging when CPU calculation cannot complete Expected behavior: First collection cycle returns 0% (no previous sample), subsequent cycles show accurate CPU metrics. --- internal/alerts/alerts.go | 46 ++++++++++++++ internal/alerts/alerts_test.go | 112 +++++++++++++++++++++++++++++++++ internal/dockeragent/agent.go | 57 +++++++++++++++-- 3 files changed, 210 insertions(+), 5 deletions(-) diff --git a/internal/alerts/alerts.go b/internal/alerts/alerts.go index 1395393..ea5371e 100644 --- a/internal/alerts/alerts.go +++ b/internal/alerts/alerts.go @@ -7902,6 +7902,34 @@ func (m *Manager) periodicSaveAlerts() { } } +// hasKnownFirmwareBug checks if a disk model is known to have firmware bugs that cause +// false health status reports. These drives may report FAILED or other error states +// due to firmware issues (e.g., incorrect temperature thresholds) even when the drive +// is actually healthy. This prevents false alerts while still monitoring wearout. +// +// Related to GitHub issue #547: Samsung 980/990 SSDs report false health failures +func hasKnownFirmwareBug(model string) bool { + normalizedModel := strings.ToUpper(strings.TrimSpace(model)) + + // Samsung 980/990 series drives have known firmware bugs causing false health reports + // These drives report incorrect health status due to temperature threshold bugs + // even when functioning normally. Users should update firmware to latest version. + knownProblematicModels := []string{ + "SAMSUNG SSD 980", + "SAMSUNG 980", + "SAMSUNG SSD 990", + "SAMSUNG 990", + } + + for _, problematic := range knownProblematicModels { + if strings.Contains(normalizedModel, problematic) { + return true + } + } + + return false +} + // CheckDiskHealth checks disk health and creates alerts if needed func (m *Manager) CheckDiskHealth(instance, node string, disk proxmox.Disk) { // Create unique alert ID for this disk @@ -7913,6 +7941,23 @@ func (m *Manager) CheckDiskHealth(instance, node string, disk proxmox.Disk) { // Check if disk health is not PASSED normalizedHealth := strings.ToUpper(strings.TrimSpace(disk.Health)) if normalizedHealth != "" && normalizedHealth != "UNKNOWN" && normalizedHealth != "PASSED" && normalizedHealth != "OK" { + // Skip health alerts for drives with known firmware bugs that cause false reports + // These drives may report FAILED status due to firmware issues even when healthy + // We still monitor wearout below, which is more reliable for these drives + if hasKnownFirmwareBug(disk.Model) { + log.Debug(). + Str("node", node). + Str("disk", disk.DevPath). + Str("model", disk.Model). + Str("health", disk.Health). + Msg("Skipping health alert for drive with known firmware bug - health status unreliable") + + // Clear any existing health alert since we now recognize this is a false positive + m.clearAlertNoLock(alertID) + + // Continue to wearout check below - wearout monitoring is still valid + goto checkWearout + } // Check if alert already exists if _, exists := m.activeAlerts[alertID]; !exists { // Create new health alert @@ -7959,6 +8004,7 @@ func (m *Manager) CheckDiskHealth(instance, node string, disk proxmox.Disk) { m.clearAlertNoLock(alertID) } +checkWearout: // Check for low wearout (SSD life remaining) if disk.Wearout > 0 && disk.Wearout < 10 { wearoutAlertID := fmt.Sprintf("disk-wearout-%s-%s-%s", instance, node, disk.DevPath) diff --git a/internal/alerts/alerts_test.go b/internal/alerts/alerts_test.go index 0d30fce..db5394a 100644 --- a/internal/alerts/alerts_test.go +++ b/internal/alerts/alerts_test.go @@ -9,6 +9,7 @@ import ( "time" "github.com/rcourtman/pulse-go-rewrite/internal/models" + "github.com/rcourtman/pulse-go-rewrite/pkg/proxmox" ) func TestAcknowledgePersistsThroughCheckMetric(t *testing.T) { @@ -1543,3 +1544,114 @@ func TestDockerResourceID(t *testing.T) { }) } } + +func TestHasKnownFirmwareBug(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + model string + want bool + }{ + {name: "Samsung 980 with SSD prefix", model: "Samsung SSD 980 1TB", want: true}, + {name: "Samsung 980 without SSD prefix", model: "Samsung 980 PRO 2TB", want: true}, + {name: "Samsung 990 with SSD prefix", model: "Samsung SSD 990 PRO 2TB", want: true}, + {name: "Samsung 990 without SSD prefix", model: "Samsung 990 EVO 1TB", want: true}, + {name: "Samsung 980 lowercase", model: "samsung ssd 980 1tb", want: true}, + {name: "Samsung 990 mixed case", model: "SAMSUNG 990 PRO", want: true}, + {name: "Samsung 970 (not affected)", model: "Samsung SSD 970 EVO Plus", want: false}, + {name: "Samsung 870 (not affected)", model: "Samsung 870 QVO", want: false}, + {name: "Other manufacturer", model: "WD Blue SN570", want: false}, + {name: "Empty model", model: "", want: false}, + } + + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + if got := hasKnownFirmwareBug(tc.model); got != tc.want { + t.Fatalf("hasKnownFirmwareBug(%q) = %v, want %v", tc.model, got, tc.want) + } + }) + } +} + +func TestCheckDiskHealthSkipsSamsung980FalseAlerts(t *testing.T) { + m := NewManager() + m.ClearActiveAlerts() + + // Samsung 980 reporting FAILED health (firmware bug) but actually healthy + disk := proxmox.Disk{ + DevPath: "/dev/nvme0n1", + Model: "Samsung SSD 980 1TB", + Serial: "S649NF0R123456", + Type: "nvme", + Health: "FAILED", // False report due to firmware bug + Wearout: 99, // Drive is actually healthy with 99% life remaining + Size: 1000204886016, + } + + // Should not create an alert for health status + m.CheckDiskHealth("test-instance", "pve-node1", disk) + + m.mu.RLock() + healthAlertID := "disk-health-test-instance-pve-node1-/dev/nvme0n1" + if _, exists := m.activeAlerts[healthAlertID]; exists { + m.mu.RUnlock() + t.Fatalf("expected no health alert for Samsung 980 with known firmware bug") + } + m.mu.RUnlock() + + // Now test that wearout alerts still work for these drives + disk.Wearout = 5 // Low wearout should still trigger alert + m.CheckDiskHealth("test-instance", "pve-node1", disk) + + m.mu.RLock() + wearoutAlertID := "disk-wearout-test-instance-pve-node1-/dev/nvme0n1" + if _, exists := m.activeAlerts[wearoutAlertID]; !exists { + m.mu.RUnlock() + t.Fatalf("expected wearout alert to still work for Samsung 980") + } + m.mu.RUnlock() +} + +func TestCheckDiskHealthClearsExistingSamsung980Alerts(t *testing.T) { + m := NewManager() + m.ClearActiveAlerts() + + disk := proxmox.Disk{ + DevPath: "/dev/nvme0n1", + Model: "Samsung SSD 990 PRO 2TB", + Serial: "S6Z0NF0R654321", + Type: "nvme", + Health: "FAILED", + Wearout: 98, + Size: 2000398934016, + } + + alertID := "disk-health-test-instance-pve-node1-/dev/nvme0n1" + + // Manually create an existing alert (simulating alert from before the fix) + m.mu.Lock() + m.activeAlerts[alertID] = &Alert{ + ID: alertID, + Type: "disk-health", + Level: AlertLevelCritical, + ResourceID: "pve-node1-/dev/nvme0n1", + ResourceName: "Samsung SSD 990 PRO 2TB (/dev/nvme0n1)", + Node: "pve-node1", + Instance: "test-instance", + Message: "Disk health check failed: FAILED", + } + m.mu.Unlock() + + // Check disk health - should clear the existing false alert + m.CheckDiskHealth("test-instance", "pve-node1", disk) + + m.mu.RLock() + defer m.mu.RUnlock() + if _, exists := m.activeAlerts[alertID]; exists { + t.Fatalf("expected existing Samsung 990 health alert to be cleared") + } +} diff --git a/internal/dockeragent/agent.go b/internal/dockeragent/agent.go index 38a152c..1048619 100644 --- a/internal/dockeragent/agent.go +++ b/internal/dockeragent/agent.go @@ -95,8 +95,9 @@ type Agent struct { targets []TargetConfig allowedStates map[string]struct{} stateFilters []string - hostID string - prevContainerCPU map[string]cpuSample + hostID string + prevContainerCPU map[string]cpuSample + preCPUStatsFailures int } // ErrStopRequested indicates the agent should terminate gracefully after acknowledging a stop command. @@ -1291,18 +1292,40 @@ func (a *Agent) calculateContainerCPUPercent(id string, stats containertypes.Sta read: stats.Read, } + // Try to use PreCPUStats if available percent := calculateCPUPercent(stats, a.cpuCount) if percent > 0 { a.prevContainerCPU[id] = current + a.logger.Debug(). + Str("container_id", id[:12]). + Float64("cpu_percent", percent). + Msg("CPU calculated from PreCPUStats") return percent } + // PreCPUStats not available or invalid, use manual tracking + a.preCPUStatsFailures++ + if a.preCPUStatsFailures == 10 { + a.logger.Warn(). + Str("runtime", string(a.runtime)). + Msg("PreCPUStats consistently unavailable from Docker API - using manual CPU tracking (this is normal for one-shot stats)") + } prev, ok := a.prevContainerCPU[id] - a.prevContainerCPU[id] = current if !ok { + // First time seeing this container - store current sample and return 0 + // On next collection cycle we'll have a previous sample to compare against + a.prevContainerCPU[id] = current + a.logger.Debug(). + Str("container_id", id[:12]). + Uint64("total_usage", current.totalUsage). + Uint64("system_usage", current.systemUsage). + Msg("First CPU sample collected, no previous data for delta calculation") return 0 } + // We have a previous sample - update it after calculation + a.prevContainerCPU[id] = current + var totalDelta float64 if current.totalUsage >= prev.totalUsage { totalDelta = float64(current.totalUsage - prev.totalUsage) @@ -1334,20 +1357,44 @@ func (a *Agent) calculateContainerCPUPercent(id string, stats containertypes.Sta } if systemDelta > 0 { - return safeFloat((totalDelta / systemDelta) * float64(onlineCPUs) * 100.0) + cpuPercent := safeFloat((totalDelta / systemDelta) * float64(onlineCPUs) * 100.0) + a.logger.Debug(). + Str("container_id", id[:12]). + Float64("cpu_percent", cpuPercent). + Float64("total_delta", totalDelta). + Float64("system_delta", systemDelta). + Uint32("online_cpus", onlineCPUs). + Msg("CPU calculated from system delta") + return cpuPercent } + // Fall back to time-based calculation if !prev.read.IsZero() && !current.read.IsZero() { elapsed := current.read.Sub(prev.read).Seconds() if elapsed > 0 { denominator := elapsed * float64(onlineCPUs) * 1e9 if denominator > 0 { cpuPercent := (totalDelta / denominator) * 100.0 - return safeFloat(cpuPercent) + result := safeFloat(cpuPercent) + a.logger.Debug(). + Str("container_id", id[:12]). + Float64("cpu_percent", result). + Float64("total_delta", totalDelta). + Float64("elapsed_seconds", elapsed). + Uint32("online_cpus", onlineCPUs). + Msg("CPU calculated from time-based delta") + return result } } } + a.logger.Debug(). + Str("container_id", id[:12]). + Float64("total_delta", totalDelta). + Float64("system_delta", systemDelta). + Bool("prev_read_zero", prev.read.IsZero()). + Bool("current_read_zero", current.read.IsZero()). + Msg("CPU calculation failed: no valid delta method available") return 0 }