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.
This commit is contained in:
rcourtman
parent
4c1d7a2797
commit
b44084af3c
3 changed files with 210 additions and 5 deletions
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue