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 }