From 090d876c18fdc78e59086e784dabf7ea3a845120 Mon Sep 17 00:00:00 2001 From: Seth Johnson Date: Fri, 17 Jul 2026 11:08:22 -0400 Subject: [PATCH] feat: fail-safe hardening so the R730 is never left uncooled The controller puts the iDRAC into manual fan mode and is then solely responsible for cooling. Previously, several failure paths abandoned that responsibility with fans potentially pinned low. Now every failure path fails safe: - Restore BMC auto mode on every abnormal exit: API server errors route through an error channel (no more log.Fatalf skipping cleanup), the control loop recovers panics, and a sync.Once restore choke point fires exactly once. Only SIGKILL/power loss can skip it. - Sensor loss fails UP: read errors hold speed instead of reading as 0C, empty parses (CPU and GPU) are errors, and 3 consecutive failures enter failsafe (BMC auto mode restored, retried every tick until confirmed, surfaced as restore_pending in /api/status). - Write failures get the same watchdog; write-failure failsafe is sticky (no manual/auto oscillation) until restart. - All ipmitool/nvidia-smi execs have timeouts: max(3s, min(10s, interval)). - Critical temps (critical_cpu_temp/critical_gpu_temp, validated at load) bypass step-ramping straight to max speed, overriding manual overrides. - Config that parses but fails safety validation refuses to start (BMC keeps auto control) instead of silently falling back to defaults and discarding iDRAC credentials; missing config still starts on defaults. - Truthful status: currentSpeed only updates after confirmed writes; failsafe_active, failsafe_reason, restore_pending, last_write_failed exposed; currentZone race fixed. Tests: table-driven coverage for calculateTarget, parsers, failsafe thresholds/stickiness/restore-retry, panic recovery, config resolution. go vet and go test -race clean. Task: dex o03q6gcj - Tier 1: Fail-safe hardening Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01EQmqj1Hj8DL3GGAcBDNm8v --- cmd/controller/main.go | 130 +++++-- cmd/controller/main_test.go | 128 +++++++ internal/config/config.go | 157 +++++++-- internal/config/config_test.go | 101 ++++++ internal/controller/fan.go | 597 +++++++++++++++++++++++--------- internal/controller/fan_test.go | 493 ++++++++++++++++++++++++++ internal/controller/mock.go | 6 +- internal/monitor/cpu.go | 71 +++- internal/monitor/cpu_test.go | 82 +++++ internal/monitor/gpu.go | 50 ++- internal/monitor/gpu_test.go | 69 ++++ 11 files changed, 1626 insertions(+), 258 deletions(-) create mode 100644 cmd/controller/main_test.go create mode 100644 internal/config/config_test.go create mode 100644 internal/controller/fan_test.go create mode 100644 internal/monitor/cpu_test.go create mode 100644 internal/monitor/gpu_test.go diff --git a/cmd/controller/main.go b/cmd/controller/main.go index b0741bb..1c75d02 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -1,12 +1,15 @@ package main import ( + "errors" "flag" + "fmt" "log" "os" "os/signal" "strconv" "strings" + "sync" "syscall" "github.com/sethpjohnson/only-fan-controller/internal/api" @@ -86,29 +89,77 @@ func applyEnvOverrides(cfg *config.Config) { } } -// FanControllerInterface allows both real and mock controllers -type FanControllerInterface interface { - Run() - Stop() - RestoreAutoMode() error - GetStatus() *controller.Status - AddHint(hint *controller.WorkloadHint) - RemoveHint(source string) - SetOverride(speed int, duration interface{}, reason string) - ClearOverride() +func main() { + os.Exit(run()) } -func main() { +// runControlLoop runs the fan control loop and guarantees that a panic restores +// BMC automatic fan control before the process gives up. Any panic is reported +// on errCh so main can shut down through the same path that restores auto mode. +// The only ways to exit without restoring auto mode are SIGKILL / power loss. +func runControlLoop(loop func(), restore func() error, errCh chan<- error) { + defer func() { + if r := recover(); r != nil { + log.Printf("PANIC in control loop: %v", r) + if err := restore(); err != nil { + log.Printf("CRITICAL: failed to restore auto fan mode after panic: %v", err) + } + errCh <- fmt.Errorf("control loop panic: %v", r) + } + }() + loop() +} + +// restoreOnce wraps a restore function so it runs at most once across all +// callers. On a control-loop panic the recover handler restores auto mode AND +// the shutdown path calls restore again; the guard makes the second call a +// no-op so the BMC is toggled exactly once. +func restoreOnce(restore func() error) func() error { + var once sync.Once + var firstErr error + return func() error { + once.Do(func() { firstErr = restore() }) + return firstErr + } +} + +// resolveConfig loads the config, distinguishing a genuinely absent file (safe +// to fall back to defaults + env overrides) from a present-but-invalid file +// (parse or safety-validation failure). For the latter we REFUSE to start rather +// than silently fall back to Default(), which would discard the operator's real +// iDRAC host/credentials and come up pointed at `-H "" -U root -P ""` — making +// every IPMI call fail, including RestoreAutoMode itself. A process that never +// starts never enables manual mode, so the BMC keeps automatic control: that is +// the fail-safe outcome. +func resolveConfig(path string) (*config.Config, error) { + cfg, err := config.Load(path) + if err == nil { + return cfg, nil + } + if errors.Is(err, os.ErrNotExist) { + log.Printf("Config file %s not found; using default configuration", path) + return config.Default(), nil + } + // File exists but is unreadable, unparseable, or fails safety validation. + return nil, err +} + +// run holds the real program body so that deferred cleanup (e.g. store.Close) +// always runs, even on an abnormal exit. main() only translates the returned +// status into a process exit code. +func run() int { configPath := flag.String("config", "/etc/only-fan-controller/config.yaml", "Path to configuration file") demoMode := flag.Bool("demo", false, "Run in demo mode with simulated temperatures (no actual fan control)") flag.Parse() // Load configuration - cfg, err := config.Load(*configPath) + cfg, err := resolveConfig(*configPath) if err != nil { - log.Printf("Warning: Could not load config from %s: %v", *configPath, err) - log.Println("Using default configuration") - cfg = config.Default() + log.Printf("FATAL: refusing to start with an invalid config %s: %v", *configPath, err) + log.Println("The config file exists but is invalid. NOT falling back to defaults, because that would") + log.Println("discard your iDRAC credentials and leave the controller unable to talk to the BMC.") + log.Println("The BMC keeps automatic fan control until you fix the config and restart.") + return 1 } // Override with environment variables @@ -128,19 +179,25 @@ func main() { // Initialize storage for history store, err := storage.New(cfg.Storage.Path) if err != nil { - log.Fatalf("Failed to initialize storage: %v", err) + log.Printf("Failed to initialize storage: %v", err) + return 1 } defer store.Close() + // errCh carries any fatal error (API server failure, control-loop panic) + // back to the shutdown path so cleanup + auto-mode restore always run. + errCh := make(chan error, 1) + var fanCtrl *controller.FanController + var restore func() error if *demoMode { - // Use mock controller + // Use mock controller. Its RestoreAutoMode is log-only, so it is safe to + // call on every exit path without touching hardware. mockCtrl := controller.NewMockFanController(cfg, store) fanCtrl = mockCtrl.FanController - - // Start mock control loop - go mockCtrl.Run() + restore = restoreOnce(mockCtrl.RestoreAutoMode) + go runControlLoop(mockCtrl.Run, restore, errCh) } else { // Initialize real monitors cpuMon := monitor.NewCPUMonitor(cfg) @@ -148,38 +205,45 @@ func main() { // Initialize real fan controller fanCtrl = controller.NewFanController(cfg, cpuMon, gpuMon, store) - - // Start the control loop - go fanCtrl.Run() + restore = restoreOnce(fanCtrl.RestoreAutoMode) + go runControlLoop(fanCtrl.Run, restore, errCh) } // Initialize API server apiServer := api.NewServer(cfg, fanCtrl, store) - // Start API server + // Start API server. On error, report through errCh instead of os.Exit so the + // shutdown path (which restores BMC auto mode) still runs. go func() { if err := apiServer.Run(); err != nil { - log.Fatalf("API server error: %v", err) + errCh <- fmt.Errorf("API server error: %w", err) } }() log.Printf("API server listening on %s:%d", cfg.API.Host, cfg.API.Port) log.Printf("Dashboard: http://localhost:%d/dashboard/", cfg.API.Port) - // Wait for shutdown signal + // Wait for a shutdown signal or a fatal error. sigChan := make(chan os.Signal, 1) signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM) - <-sigChan - log.Println("Shutting down...") + exitCode := 0 + select { + case <-sigChan: + log.Println("Shutting down...") + case err := <-errCh: + log.Printf("Fatal: %v", err) + exitCode = 1 + } + fanCtrl.Stop() - // Restore Dell automatic fan control on exit (only in real mode) - if !*demoMode { - if err := fanCtrl.RestoreAutoMode(); err != nil { - log.Printf("Warning: Failed to restore auto fan mode: %v", err) - } + // Restore BMC automatic fan control on every exit path. This is the single + // choke point that keeps the machine cooled once we leave manual mode. + if err := restore(); err != nil { + log.Printf("Warning: Failed to restore auto fan mode: %v", err) } log.Println("Shutdown complete") + return exitCode } diff --git a/cmd/controller/main_test.go b/cmd/controller/main_test.go new file mode 100644 index 0000000..04eea7c --- /dev/null +++ b/cmd/controller/main_test.go @@ -0,0 +1,128 @@ +package main + +import ( + "errors" + "os" + "path/filepath" + "testing" + "time" +) + +func TestResolveConfigMissingFileFallsBackToDefaults(t *testing.T) { + missing := filepath.Join(t.TempDir(), "does-not-exist.yaml") + cfg, err := resolveConfig(missing) + if err != nil { + t.Fatalf("missing config file should fall back to defaults, got error: %v", err) + } + if cfg == nil { + t.Fatal("expected default config, got nil") + } +} + +func TestResolveConfigInvalidFileRefusesToStart(t *testing.T) { + // Valid YAML with REAL iDRAC credentials, but a safety-validation failure + // (critical_cpu_temp below the customized cpu_threshold). resolveConfig must + // NOT silently fall back to Default() (which would discard these credentials) + // — it must return an error so the process refuses to start. + path := filepath.Join(t.TempDir(), "config.yaml") + content := `idrac: + host: "192.168.1.50" + username: "operator" + password: "s3cret" +fan_control: + cpu_threshold: 90 + critical_cpu_temp: 85 +` + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatalf("failed to write temp config: %v", err) + } + + cfg, err := resolveConfig(path) + if err == nil { + t.Fatalf("expected refusal on invalid config, got cfg=%+v", cfg) + } + if cfg != nil { + t.Fatalf("invalid config must not yield a usable (fallback) config, got %+v", cfg) + } +} + +func TestRunControlLoopRestoresAutoModeOnPanic(t *testing.T) { + restored := make(chan struct{}, 1) + errCh := make(chan error, 1) + + run := func() { panic("boom in control loop") } + restore := func() error { + restored <- struct{}{} + return nil + } + + runControlLoop(run, restore, errCh) + + select { + case <-restored: + default: + t.Fatal("RestoreAutoMode was not called when the control loop panicked") + } + + select { + case err := <-errCh: + if err == nil { + t.Fatal("expected a non-nil error on the error channel after panic") + } + case <-time.After(time.Second): + t.Fatal("no error reported on the error channel after panic") + } +} + +func TestRunControlLoopReportsRestoreFailureButStillExits(t *testing.T) { + errCh := make(chan error, 1) + run := func() { panic("boom") } + restore := func() error { return errors.New("ipmitool unreachable") } + + // Must not itself panic even if restore fails. + runControlLoop(run, restore, errCh) + + select { + case err := <-errCh: + if err == nil { + t.Fatal("expected an error after panic") + } + default: + t.Fatal("expected an error to be reported after panic") + } +} + +func TestRestoreOnceFiresUnderlyingExactlyOnce(t *testing.T) { + calls := 0 + restore := restoreOnce(func() error { + calls++ + return nil + }) + + // Simulate the panic path (recover restores) followed by the shutdown path + // (run() restores again). The underlying BMC toggle must happen once. + _ = restore() + _ = restore() + _ = restore() + + if calls != 1 { + t.Fatalf("underlying restore called %d times, want 1", calls) + } +} + +func TestRunControlLoopNormalReturnDoesNotReportError(t *testing.T) { + errCh := make(chan error, 1) + run := func() {} // returns normally + restore := func() error { + t.Fatal("restore must not be called on a normal return") + return nil + } + + runControlLoop(run, restore, errCh) + + select { + case err := <-errCh: + t.Fatalf("unexpected error on normal return: %v", err) + default: + } +} diff --git a/internal/config/config.go b/internal/config/config.go index e14f018..721dcf8 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1,6 +1,7 @@ package config import ( + "fmt" "os" "gopkg.in/yaml.v3" @@ -25,12 +26,12 @@ type IDRACConfig struct { } type MonitoringConfig struct { - Interval int `yaml:"interval"` // Seconds between checks - HistoryRetention int `yaml:"history_retention"` // Seconds to keep history + Interval int `yaml:"interval"` // Seconds between checks + HistoryRetention int `yaml:"history_retention"` // Seconds to keep history } type GPUConfig struct { - Enabled bool `yaml:"enabled"` + Enabled bool `yaml:"enabled"` NvidiaSmiPath string `yaml:"nvidia_smi_path"` } @@ -42,17 +43,27 @@ type Zone struct { } type FanControlConfig struct { - MinSpeed int `yaml:"min_speed" json:"min_speed"` - MaxSpeed int `yaml:"max_speed" json:"max_speed"` - IdleSpeed int `yaml:"idle_speed" json:"idle_speed"` // Base fan speed when idle - CPUThreshold int `yaml:"cpu_threshold" json:"cpu_threshold"` // CPU temp that triggers fan increase - GPUThreshold int `yaml:"gpu_threshold" json:"gpu_threshold"` // GPU temp that triggers fan increase - StepSize int `yaml:"step_size" json:"step_size"` // Fan speed increment per threshold breach - CooldownDelay int `yaml:"cooldown_delay" json:"cooldown_delay"` // Seconds below threshold before ramping down + MinSpeed int `yaml:"min_speed" json:"min_speed"` + MaxSpeed int `yaml:"max_speed" json:"max_speed"` + IdleSpeed int `yaml:"idle_speed" json:"idle_speed"` // Base fan speed when idle + CPUThreshold int `yaml:"cpu_threshold" json:"cpu_threshold"` // CPU temp that triggers fan increase + GPUThreshold int `yaml:"gpu_threshold" json:"gpu_threshold"` // GPU temp that triggers fan increase + StepSize int `yaml:"step_size" json:"step_size"` // Fan speed increment per threshold breach + CooldownDelay int `yaml:"cooldown_delay" json:"cooldown_delay"` // Seconds below threshold before ramping down + // Critical temperatures trigger the emergency ramp (fans jump straight to + // MaxSpeed, bypassing step ramping and any manual override). These are the + // explicit, validated trigger thresholds — the emergency SPEED is always + // MaxSpeed and is never taken from operator-editable zone data. + CriticalCPUTemp int `yaml:"critical_cpu_temp" json:"critical_cpu_temp"` // CPU temp (>=) that forces the emergency ramp + CriticalGPUTemp int `yaml:"critical_gpu_temp" json:"critical_gpu_temp"` // GPU temp (>=) that forces the emergency ramp + // Fail-safe thresholds: after this many consecutive failures the controller + // hands cooling back to the BMC's automatic fan control. + SensorFailureLimit int `yaml:"sensor_failure_limit" json:"sensor_failure_limit"` // Consecutive sensor read failures before restoring auto mode + WriteFailureLimit int `yaml:"write_failure_limit" json:"write_failure_limit"` // Consecutive fan-write failures before restoring auto mode // Legacy fields (still supported) - RampUpStep int `yaml:"ramp_up_step" json:"ramp_up_step"` - RampDownStep int `yaml:"ramp_down_step" json:"ramp_down_step"` - ConstantIdle bool `yaml:"constant_idle" json:"constant_idle"` + RampUpStep int `yaml:"ramp_up_step" json:"ramp_up_step"` + RampDownStep int `yaml:"ramp_down_step" json:"ramp_down_step"` + ConstantIdle bool `yaml:"constant_idle" json:"constant_idle"` } type APIConfig struct { @@ -74,6 +85,32 @@ type LoggingConfig struct { File string `yaml:"file"` } +// Default normal-ramp thresholds, applied when the corresponding config field is +// left at its zero value. Kept here so config validation and the controller +// agree on the effective values. +const ( + defaultCPUThreshold = 65 + defaultGPUThreshold = 60 +) + +// EffectiveCPUThreshold returns the CPU ramp threshold actually in force, +// substituting the default when unset. +func (fc FanControlConfig) EffectiveCPUThreshold() int { + if fc.CPUThreshold > 0 { + return fc.CPUThreshold + } + return defaultCPUThreshold +} + +// EffectiveGPUThreshold returns the GPU ramp threshold actually in force, +// substituting the default when unset. +func (fc FanControlConfig) EffectiveGPUThreshold() int { + if fc.GPUThreshold > 0 { + return fc.GPUThreshold + } + return defaultGPUThreshold +} + // Load reads configuration from a YAML file func Load(path string) (*Config, error) { data, err := os.ReadFile(path) @@ -86,23 +123,69 @@ func Load(path string) (*Config, error) { return nil, err } + // Reject unsafe operator config so the caller can fall back to safe defaults. + // This is critical for the emergency-ramp path: a malformed thresholds/zones + // list must never be trusted to decide when to pin fans. + if err := cfg.Validate(); err != nil { + return nil, err + } + return cfg, nil } +// Validate checks that the safety-critical parts of the config are sane. It is +// intentionally strict about the fields that drive the emergency ramp and the +// fail-safe thresholds; an invalid config is rejected by Load so main falls back +// to the (always-valid) defaults rather than trusting bad operator input. +func (c *Config) Validate() error { + fc := c.FanControl + if fc.MinSpeed < 0 || fc.MaxSpeed > 100 || fc.MinSpeed > fc.MaxSpeed { + return fmt.Errorf("invalid fan speed bounds: min=%d max=%d (require 0<=min<=max<=100)", fc.MinSpeed, fc.MaxSpeed) + } + // Critical thresholds must be explicit and physically plausible. They are the + // sole trigger for the emergency ramp, so we do not accept 0 ("always + // critical") or absurd values. + if fc.CriticalCPUTemp < 40 || fc.CriticalCPUTemp > 120 { + return fmt.Errorf("invalid critical_cpu_temp: %d (require 40..120)", fc.CriticalCPUTemp) + } + if fc.CriticalGPUTemp < 40 || fc.CriticalGPUTemp > 120 { + return fmt.Errorf("invalid critical_gpu_temp: %d (require 40..120)", fc.CriticalGPUTemp) + } + // The critical trigger must sit above the normal ramp thresholds, otherwise + // the step-ramp band is unreachable. Compare against the EFFECTIVE thresholds + // (post-default) so an unset cpu_threshold: 0 does not silently skip the check + // — and so only genuinely conflicting customizations trip validation. + if cpuT := fc.EffectiveCPUThreshold(); fc.CriticalCPUTemp <= cpuT { + return fmt.Errorf("critical_cpu_temp (%d) must exceed effective cpu_threshold (%d)", fc.CriticalCPUTemp, cpuT) + } + if gpuT := fc.EffectiveGPUThreshold(); fc.CriticalGPUTemp <= gpuT { + return fmt.Errorf("critical_gpu_temp (%d) must exceed effective gpu_threshold (%d)", fc.CriticalGPUTemp, gpuT) + } + // Zones are informational (dashboard display), but if provided they must be + // monotonic non-decreasing so the display stays coherent. + for i := 1; i < len(c.Zones); i++ { + prev, cur := c.Zones[i-1], c.Zones[i] + if cur.CPUMax < prev.CPUMax || cur.GPUMax < prev.GPUMax || cur.FanSpeed < prev.FanSpeed { + return fmt.Errorf("zones must be monotonic non-decreasing; zone %q breaks ordering", cur.Name) + } + } + return nil +} + // Default returns a configuration with sensible defaults func Default() *Config { return &Config{ IDRAC: IDRACConfig{ - Host: "", // Must be set via config or IDRAC_HOST env var - Username: "root", // Dell iDRAC default - Password: "", // Must be set via config or IDRAC_PASSWORD env var + Host: "", // Must be set via config or IDRAC_HOST env var + Username: "root", // Dell iDRAC default + Password: "", // Must be set via config or IDRAC_PASSWORD env var }, Monitoring: MonitoringConfig{ Interval: 10, HistoryRetention: 3600, }, GPU: GPUConfig{ - Enabled: true, + Enabled: true, NvidiaSmiPath: "/usr/bin/nvidia-smi", }, Zones: []Zone{ @@ -113,16 +196,20 @@ func Default() *Config { {Name: "critical", CPUMax: 999, GPUMax: 999, FanSpeed: 100}, }, FanControl: FanControlConfig{ - MinSpeed: 5, - MaxSpeed: 100, - IdleSpeed: 20, // Base fan speed when idle (quiet) - CPUThreshold: 65, // Bump fans if CPU exceeds this - GPUThreshold: 60, // Bump fans if GPU exceeds this - StepSize: 10, // Increase fan by 10% per threshold breach - CooldownDelay: 60, // Wait 60s below threshold before ramping down - RampUpStep: 10, - RampDownStep: 5, - ConstantIdle: true, + MinSpeed: 5, + MaxSpeed: 100, + IdleSpeed: 20, // Base fan speed when idle (quiet) + CPUThreshold: 65, // Bump fans if CPU exceeds this + GPUThreshold: 60, // Bump fans if GPU exceeds this + StepSize: 10, // Increase fan by 10% per threshold breach + CooldownDelay: 60, // Wait 60s below threshold before ramping down + CriticalCPUTemp: 85, // Emergency ramp at/above this CPU temp + CriticalGPUTemp: 90, // Emergency ramp at/above this GPU temp + SensorFailureLimit: 3, // Restore BMC auto mode after 3 consecutive sensor read failures + WriteFailureLimit: 3, // Restore BMC auto mode after 3 consecutive fan-write failures + RampUpStep: 10, + RampDownStep: 5, + ConstantIdle: true, }, API: APIConfig{ Host: "0.0.0.0", @@ -142,14 +229,10 @@ func Default() *Config { } } -// GetZone returns the appropriate zone for given temperatures -func (c *Config) GetZone(cpuTemp, gpuTemp int) *Zone { - for i := range c.Zones { - zone := &c.Zones[i] - if cpuTemp <= zone.CPUMax && gpuTemp <= zone.GPUMax { - return zone - } - } - // Return last zone (critical) if nothing matches - return &c.Zones[len(c.Zones)-1] +// IsCritical reports whether the given temperatures have reached the explicit, +// validated critical thresholds. It deliberately does NOT consult the (operator- +// editable) Zones list: the trigger must not be steerable into pinning fans at a +// low speed. The emergency SPEED is decided separately (always MaxSpeed). +func (c *Config) IsCritical(cpuTemp, gpuTemp int) bool { + return cpuTemp >= c.FanControl.CriticalCPUTemp || gpuTemp >= c.FanControl.CriticalGPUTemp } diff --git a/internal/config/config_test.go b/internal/config/config_test.go new file mode 100644 index 0000000..b1ee7df --- /dev/null +++ b/internal/config/config_test.go @@ -0,0 +1,101 @@ +package config + +import "testing" + +func TestValidate(t *testing.T) { + tests := []struct { + name string + mutate func(c *Config) + wantErr bool + }{ + { + name: "default config is valid", + mutate: func(c *Config) {}, + wantErr: false, + }, + { + name: "critical CPU temp too low is rejected", + mutate: func(c *Config) { c.FanControl.CriticalCPUTemp = 30 }, + wantErr: true, + }, + { + name: "critical CPU temp absurdly high is rejected", + mutate: func(c *Config) { c.FanControl.CriticalCPUTemp = 200 }, + wantErr: true, + }, + { + name: "critical temp at/below normal threshold is rejected", + mutate: func(c *Config) { c.FanControl.CriticalCPUTemp = c.FanControl.CPUThreshold }, + wantErr: true, + }, + { + name: "unset cpu_threshold still checks critical against effective default", + mutate: func(c *Config) { + c.FanControl.CPUThreshold = 0 // unset -> effective default 65 + c.FanControl.CriticalCPUTemp = 60 // below effective default + }, + wantErr: true, + }, + { + name: "unset cpu_threshold with sane critical is valid", + mutate: func(c *Config) { + c.FanControl.CPUThreshold = 0 // effective default 65 + c.FanControl.CriticalCPUTemp = 70 // above effective default + }, + wantErr: false, + }, + { + name: "max speed above 100 is rejected", + mutate: func(c *Config) { c.FanControl.MaxSpeed = 120 }, + wantErr: true, + }, + { + name: "min speed above max is rejected", + mutate: func(c *Config) { c.FanControl.MinSpeed = 90; c.FanControl.MaxSpeed = 80 }, + wantErr: true, + }, + { + name: "non-monotonic zones are rejected", + mutate: func(c *Config) { + c.Zones = []Zone{ + {Name: "idle", CPUMax: 45, GPUMax: 40, FanSpeed: 10}, + {Name: "backwards", CPUMax: 30, GPUMax: 30, FanSpeed: 5}, + } + }, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := Default() + tt.mutate(c) + err := c.Validate() + if tt.wantErr && err == nil { + t.Fatal("expected validation error, got nil") + } + if !tt.wantErr && err != nil { + t.Fatalf("unexpected validation error: %v", err) + } + }) + } +} + +func TestIsCritical(t *testing.T) { + c := Default() // CriticalCPUTemp=85, CriticalGPUTemp=90 + tests := []struct { + cpu, gpu int + want bool + }{ + {cpu: 84, gpu: 50, want: false}, // just below critical CPU + {cpu: 85, gpu: 50, want: true}, // at critical CPU + {cpu: 40, gpu: 90, want: true}, // at critical GPU + {cpu: 40, gpu: 89, want: false}, // just below critical GPU + {cpu: 81, gpu: 50, want: false}, // hot-zone band stays reachable, not critical + } + for _, tt := range tests { + if got := c.IsCritical(tt.cpu, tt.gpu); got != tt.want { + t.Fatalf("IsCritical(%d, %d) = %v, want %v", tt.cpu, tt.gpu, got, tt.want) + } + } +} diff --git a/internal/controller/fan.go b/internal/controller/fan.go index 53ed088..85f653f 100644 --- a/internal/controller/fan.go +++ b/internal/controller/fan.go @@ -2,6 +2,7 @@ package controller import ( "bytes" + "context" "fmt" "log" "os/exec" @@ -13,28 +14,83 @@ import ( "github.com/sethpjohnson/only-fan-controller/internal/storage" ) +// cpuReader and gpuReader abstract the temperature sources so the control loop +// can be exercised with fakes in tests (the real implementations are the +// ipmitool/nvidia-smi backed monitors). +type cpuReader interface { + Read() (*monitor.CPUReading, error) +} + +type gpuReader interface { + Read() (*monitor.GPUReading, error) +} + +// runCommandFunc executes an external command with a context deadline. It is a +// field on FanController so tests can substitute a stub in place of real +// ipmitool invocations. +type runCommandFunc func(ctx context.Context, name string, args ...string) error + +// failsafeCause records why the controller handed cooling back to the BMC. +type failsafeCause int + +const ( + failsafeNone failsafeCause = iota + failsafeSensor // sensor loss — recoverable + failsafeWrite // fan-write failures — sticky until restart +) + +func (c failsafeCause) String() string { + switch c { + case failsafeSensor: + return "sensor-loss" + case failsafeWrite: + return "write-failure" + default: + return "none" + } +} + type FanController struct { - cfg *config.Config - cpuMon *monitor.CPUMonitor - gpuMon *monitor.GPUMonitor - store *storage.Store - + cfg *config.Config + cpuMon cpuReader + gpuMon gpuReader + store *storage.Store + + // runCommand runs external commands (ipmitool). Defaults to realRunCommand. + runCommand runCommandFunc + // State - mu sync.RWMutex - currentSpeed int - targetSpeed int - currentZone string - lastCPUReading *monitor.CPUReading - lastGPUReading *monitor.GPUReading - hints map[string]*WorkloadHint - override *Override - running bool - stopChan chan struct{} - + mu sync.RWMutex + currentSpeed int + targetSpeed int + currentZone string + lastCPUReading *monitor.CPUReading + lastGPUReading *monitor.GPUReading + hints map[string]*WorkloadHint + override *Override + running bool + stopChan chan struct{} + + // Fail-safe state. failsafeCause and lastWriteFailed are read by GetStatus + // (API goroutine) so they are guarded by mu. The consecutive-failure counters + // are only ever touched from the control loop goroutine. + // + // The cause is tracked (not a bare bool) so recovery is coherent per domain: + // - a SENSOR fail-safe is recoverable — a healthy sensor read reclaims + // manual control; + // - a WRITE fail-safe is STICKY — once the fan-write channel proves + // unreliable we leave the BMC in charge (no auto-recovery), because + // probing it every tick would flap the BMC between manual and auto. + failsafeCause failsafeCause + restoreConfirmed bool // true once RestoreAutoMode has actually succeeded for the current fail-safe + lastWriteFailed bool + sensorFailCount int + writeFailCount int + // History for trend analysis - cpuHistory []tempPoint - gpuHistory []tempPoint - + cpuHistory []tempPoint + gpuHistory []tempPoint + // Hysteresis tracking lastOverThreshold time.Time } @@ -45,13 +101,13 @@ type tempPoint struct { } type WorkloadHint struct { - Type string `json:"type"` - Action string `json:"action"` - Intensity string `json:"intensity"` - Source string `json:"source"` - MinFanSpeed int `json:"min_fan_speed"` - ExpiresAt time.Time `json:"expires_at"` - CreatedAt time.Time `json:"created_at"` + Type string `json:"type"` + Action string `json:"action"` + Intensity string `json:"intensity"` + Source string `json:"source"` + MinFanSpeed int `json:"min_fan_speed"` + ExpiresAt time.Time `json:"expires_at"` + CreatedAt time.Time `json:"created_at"` } type Override struct { @@ -62,29 +118,35 @@ type Override struct { } type Status struct { - Timestamp time.Time `json:"timestamp"` - CPU *monitor.CPUReading `json:"cpu"` - GPU *monitor.GPUReading `json:"gpu"` - CurrentSpeed int `json:"current_speed"` - TargetSpeed int `json:"target_speed"` - Zone string `json:"zone"` - Mode string `json:"mode"` - ActiveHints []*WorkloadHint `json:"active_hints"` - Override *Override `json:"override,omitempty"` - CPUTrend float64 `json:"cpu_trend"` - GPUTrend float64 `json:"gpu_trend"` - Zones []config.Zone `json:"zones"` - CPUThreshold int `json:"cpu_threshold"` - GPUThreshold int `json:"gpu_threshold"` - IdleSpeed int `json:"idle_speed"` -} - -func NewFanController(cfg *config.Config, cpuMon *monitor.CPUMonitor, gpuMon *monitor.GPUMonitor, store *storage.Store) *FanController { + Timestamp time.Time `json:"timestamp"` + CPU *monitor.CPUReading `json:"cpu"` + GPU *monitor.GPUReading `json:"gpu"` + CurrentSpeed int `json:"current_speed"` + TargetSpeed int `json:"target_speed"` + Zone string `json:"zone"` + Mode string `json:"mode"` + ActiveHints []*WorkloadHint `json:"active_hints"` + Override *Override `json:"override,omitempty"` + CPUTrend float64 `json:"cpu_trend"` + GPUTrend float64 `json:"gpu_trend"` + Zones []config.Zone `json:"zones"` + CPUThreshold int `json:"cpu_threshold"` + GPUThreshold int `json:"gpu_threshold"` + IdleSpeed int `json:"idle_speed"` + // Fail-safe visibility for operators / the dashboard. + FailsafeActive bool `json:"failsafe_active"` // true when cooling has been handed back to BMC auto mode + FailsafeReason string `json:"failsafe_reason"` // "none", "sensor-loss" or "write-failure" + RestorePending bool `json:"restore_pending"` // true when in fail-safe but RestoreAutoMode has not yet succeeded (BMC may still be in manual mode) + LastWriteFailed bool `json:"last_write_failed"` // true when the most recent fan-speed write failed +} + +func NewFanController(cfg *config.Config, cpuMon cpuReader, gpuMon gpuReader, store *storage.Store) *FanController { return &FanController{ cfg: cfg, cpuMon: cpuMon, gpuMon: gpuMon, store: store, + runCommand: realRunCommand, hints: make(map[string]*WorkloadHint), stopChan: make(chan struct{}), cpuHistory: make([]tempPoint, 0), @@ -92,6 +154,22 @@ func NewFanController(cfg *config.Config, cpuMon *monitor.CPUMonitor, gpuMon *mo } } +// realRunCommand runs an external command bounded by the context deadline. On a +// deadline it returns a descriptive error so the caller can distinguish a hung +// command (e.g. a stuck `ipmitool -I lanplus`) from a normal failure. +func realRunCommand(ctx context.Context, name string, args ...string) error { + cmd := exec.CommandContext(ctx, name, args...) + var stderr bytes.Buffer + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + if ctx.Err() == context.DeadlineExceeded { + return fmt.Errorf("%s timed out: %w", name, err) + } + return fmt.Errorf("%s error: %v, stderr: %s", name, err, stderr.String()) + } + return nil +} + func (fc *FanController) Run() { fc.mu.Lock() fc.running = true @@ -126,48 +204,272 @@ func (fc *FanController) Stop() { } func (fc *FanController) controlLoop() { - // Read temperatures - cpuReading, err := fc.cpuMon.Read() - if err != nil { - log.Printf("CPU read error: %v", err) - cpuReading = &monitor.CPUReading{Temps: []int{}, Max: 0} + // If we are in fail-safe but the hand-back to BMC auto has not been confirmed + // (a previous RestoreAutoMode failed — e.g. the BMC was unreachable, which is + // exactly the condition that trips fail-safe), keep retrying every tick until + // it succeeds. This does not reintroduce flapping: restore-to-auto is + // idempotent and nothing re-enables manual mode while a restore is unconfirmed. + fc.ensureAutoRestored() + + // A WRITE fail-safe is sticky: once the fan-write channel has proven + // unreliable we leave the BMC in charge until the process restarts. Probing + // it every tick (reclaim manual -> write fails -> restore auto) would flap the + // BMC forever, so we deliberately do not auto-recover. We still refresh the + // readings for /api/status visibility. + if fc.currentFailsafeCause() == failsafeWrite { + if cpuReading, gpuReading, ok := fc.readSensors(); ok { + fc.recordReadings(cpuReading, gpuReading) + } + return + } + + // Read temperatures. A read error must NEVER be treated as 0°C ("cold"), + // which would ramp the fans down and cook the machine. Instead we hold the + // current speed and, after repeated failures, hand cooling back to the BMC. + cpuReading, gpuReading, ok := fc.readSensors() + if !ok { + fc.handleSensorFailure() + return + } + + // Sensors are healthy again: clear the failure count. + if fc.sensorFailCount > 0 { + log.Printf("Sensors recovered after %d consecutive failure(s)", fc.sensorFailCount) + fc.sensorFailCount = 0 + } + + // If we were in a (recoverable) sensor fail-safe, reclaim manual control + // before acting on the reading — but only from a CONFIRMED-restored state, so + // the manual<->auto handoff stays coherent. If the hand-back to BMC auto is + // still unconfirmed, ensureAutoRestored (above) keeps retrying and we wait. + // If reclaiming the write channel fails we stay in fail-safe and retry next + // tick; a failed enableManualMode does not change the BMC state, so it cannot + // flap. + if fc.currentFailsafeCause() == failsafeSensor { + if !fc.isRestoreConfirmed() { + log.Printf("Sensors recovered but BMC auto hand-back not yet confirmed; deferring manual reclaim") + return + } + if err := fc.enableManualMode(); err != nil { + log.Printf("Sensors recovered but reclaiming manual mode failed; staying in BMC auto: %v", err) + return + } + log.Printf("FAILSAFE CLEARED: sensors recovered, manual fan control reclaimed") + fc.clearFailsafe() } - gpuReading, err := fc.gpuMon.Read() - if err != nil { - log.Printf("GPU read error: %v", err) - gpuReading = &monitor.GPUReading{Devices: []monitor.GPUDevice{}, Max: 0} + fc.recordReadings(cpuReading, gpuReading) + + // Calculate target fan speed + target := fc.calculateTarget(cpuReading, gpuReading) + + // Apply fan speed. A failed write is a watchdog concern: after repeated + // failures we restore BMC auto mode rather than leave the fans wherever + // they were last set. + if err := fc.setFanSpeed(target); err != nil { + fc.handleWriteFailure(err) + } else { + fc.writeFailCount = 0 } + // Store reading + fc.store.RecordReading(cpuReading.Max, gpuReading.Max, target) + + fc.mu.RLock() + zone := fc.currentZone + fc.mu.RUnlock() + log.Printf("CPU: %d°C | GPU: %d°C | Zone: %s | Fan: %d%%", + cpuReading.Max, gpuReading.Max, zone, target) +} + +// readSensors reads both temperature sources. It returns ok=false if either read +// failed, logging each failure. It never fabricates a 0°C reading on failure. +func (fc *FanController) readSensors() (*monitor.CPUReading, *monitor.GPUReading, bool) { + cpuReading, cpuErr := fc.cpuMon.Read() + gpuReading, gpuErr := fc.gpuMon.Read() + if cpuErr != nil { + log.Printf("CPU sensor read failed: %v", cpuErr) + } + if gpuErr != nil { + log.Printf("GPU sensor read failed: %v", gpuErr) + } + if cpuErr != nil || gpuErr != nil { + return nil, nil, false + } + return cpuReading, gpuReading, true +} + +// recordReadings updates the last-known readings and history under the lock. +func (fc *FanController) recordReadings(cpuReading *monitor.CPUReading, gpuReading *monitor.GPUReading) { fc.mu.Lock() + defer fc.mu.Unlock() fc.lastCPUReading = cpuReading fc.lastGPUReading = gpuReading - - // Update history now := time.Now() fc.cpuHistory = append(fc.cpuHistory, tempPoint{temp: cpuReading.Max, timestamp: now}) fc.gpuHistory = append(fc.gpuHistory, tempPoint{temp: gpuReading.Max, timestamp: now}) - - // Trim old history fc.trimHistory() - - // Clean expired hints and overrides fc.cleanExpired() +} + +// handleSensorFailure records a consecutive sensor read failure. It holds the +// current fan speed (does not touch the fans) and, once the configured limit is +// reached, hands cooling back to the BMC's automatic control. +func (fc *FanController) handleSensorFailure() { + fc.sensorFailCount++ + limit := fc.sensorFailureLimit() + log.Printf("SENSOR FAILURE %d/%d - holding fan speed (not treating missing data as 0°C)", + fc.sensorFailCount, limit) + if fc.sensorFailCount >= limit { + fc.enterFailsafe(failsafeSensor) + } +} + +// handleWriteFailure records a consecutive fan-write failure and restores auto +// mode once the configured limit is reached. +func (fc *FanController) handleWriteFailure(err error) { + fc.writeFailCount++ + limit := fc.writeFailureLimit() + log.Printf("Fan write failure %d/%d: %v", fc.writeFailCount, limit, err) + if fc.writeFailCount >= limit { + fc.enterFailsafe(failsafeWrite) + } +} + +func (fc *FanController) currentFailsafeCause() failsafeCause { + fc.mu.RLock() + defer fc.mu.RUnlock() + return fc.failsafeCause +} + +func (fc *FanController) isRestoreConfirmed() bool { + fc.mu.RLock() + defer fc.mu.RUnlock() + return fc.restoreConfirmed +} + +// enterFailsafe hands cooling back to the BMC's automatic fan control, recording +// WHY. It only attempts RestoreAutoMode on the transition from active control +// into fail-safe; a write fail-safe never downgrades to sensor, so once sticky +// it stays sticky. If the restore attempt fails, restoreConfirmed stays false +// and ensureAutoRestored retries on subsequent ticks. +func (fc *FanController) enterFailsafe(cause failsafeCause) { + fc.mu.Lock() + prev := fc.failsafeCause + // Write is the stickiest cause; never let a later sensor failure overwrite it. + if prev == failsafeWrite { + fc.mu.Unlock() + return + } + fc.failsafeCause = cause fc.mu.Unlock() - // Calculate target fan speed - target := fc.calculateTarget(cpuReading, gpuReading) + if prev != failsafeNone { + // Already handed to BMC auto for some reason; ensureAutoRestored owns the + // (possibly still-pending) restore retry. + return + } + log.Printf("FAILSAFE ACTIVATED (%s): restoring BMC automatic fan control", cause) + fc.attemptRestore() +} - // Apply fan speed - if err := fc.setFanSpeed(target); err != nil { - log.Printf("Failed to set fan speed: %v", err) +// ensureAutoRestored retries the hand-back to BMC auto mode while we are in +// fail-safe but the restore has not yet been confirmed. Restore-to-auto is +// idempotent and nothing re-enables manual mode while unconfirmed, so this +// cannot flap. +func (fc *FanController) ensureAutoRestored() { + fc.mu.RLock() + pending := fc.failsafeCause != failsafeNone && !fc.restoreConfirmed + fc.mu.RUnlock() + if !pending { + return } + log.Printf("Fail-safe: BMC auto hand-back not confirmed, retrying RestoreAutoMode") + fc.attemptRestore() +} - // Store reading - fc.store.RecordReading(cpuReading.Max, gpuReading.Max, target) +// attemptRestore calls RestoreAutoMode and records whether it succeeded. +func (fc *FanController) attemptRestore() { + if err := fc.RestoreAutoMode(); err != nil { + log.Printf("CRITICAL: RestoreAutoMode failed; BMC hand-back NOT confirmed, will retry: %v", err) + return + } + fc.mu.Lock() + fc.restoreConfirmed = true + fc.mu.Unlock() + log.Printf("Fail-safe: BMC automatic fan control confirmed restored") +} + +// clearFailsafe returns to normal control and resets the fail-safe state so the +// next failure starts fresh. +func (fc *FanController) clearFailsafe() { + fc.mu.Lock() + fc.failsafeCause = failsafeNone + fc.restoreConfirmed = false + // sensorFailCount / writeFailCount are only ever touched from the single + // control-loop goroutine (here and in handleSensor/WriteFailure), so they + // need no lock; grouped here for a coherent reset of all fail-safe state. + fc.sensorFailCount = 0 + fc.writeFailCount = 0 + fc.mu.Unlock() +} + +func (fc *FanController) sensorFailureLimit() int { + if fc.cfg.FanControl.SensorFailureLimit > 0 { + return fc.cfg.FanControl.SensorFailureLimit + } + return 3 +} - log.Printf("CPU: %d°C | GPU: %d°C | Zone: %s | Fan: %d%%", - cpuReading.Max, gpuReading.Max, fc.currentZone, target) +func (fc *FanController) writeFailureLimit() int { + if fc.cfg.FanControl.WriteFailureLimit > 0 { + return fc.cfg.FanControl.WriteFailureLimit + } + return 3 +} + +// commandTimeout bounds every ipmitool invocation so a hung BMC connection can +// never freeze the control loop (or shutdown), while keeping a floor so that a +// short control interval does not make slow-but-healthy lanplus calls read as +// timeouts (which would feed the fail-safe counters). +func (fc *FanController) commandTimeout() time.Duration { + return commandTimeout(fc.cfg.Monitoring.Interval) +} + +// commandTimeout computes the external-command deadline: max(3s, min(10s, +// interval)). The 3s floor protects slow-but-healthy remote BMC calls. +func commandTimeout(intervalSeconds int) time.Duration { + const ( + minTimeout = 3 * time.Second + maxTimeout = 10 * time.Second + ) + interval := time.Duration(intervalSeconds) * time.Second + if interval > maxTimeout { + return maxTimeout + } + if interval < minTimeout { + return minTimeout + } + return interval +} + +// ipmitool runs an ipmitool raw command against the configured BMC (local or +// remote) with a deadline. +func (fc *FanController) ipmitool(rawArgs ...string) error { + ctx, cancel := context.WithTimeout(context.Background(), fc.commandTimeout()) + defer cancel() + + var args []string + if fc.cfg.IDRAC.Host != "local" { + args = append(args, + "-I", "lanplus", + "-H", fc.cfg.IDRAC.Host, + "-U", fc.cfg.IDRAC.Username, + "-P", fc.cfg.IDRAC.Password, + ) + } + args = append(args, rawArgs...) + return fc.runCommand(ctx, "ipmitool", args...) } func (fc *FanController) calculateTarget(cpuReading *monitor.CPUReading, gpuReading *monitor.GPUReading) int { @@ -177,8 +479,25 @@ func (fc *FanController) calculateTarget(cpuReading *monitor.CPUReading, gpuRead cpuMax := cpuReading.Max gpuMax := gpuReading.Max + // Safety first: a critical temperature bypasses step ramping AND any manual + // override, driving fans straight to MaxSpeed (effectively 100%). The + // emergency speed is a FIXED CEILING (MaxSpeed), never operator-editable zone + // data — that way a bad zones list can never turn the "emergency" into a low + // speed. Leaving fans pinned low during a thermal emergency is exactly what + // this controller must never do, so critical cooling wins over everything. + if fc.cfg.IsCritical(cpuMax, gpuMax) { + speed := fc.cfg.FanControl.MaxSpeed + if speed <= 0 || speed > 100 { + speed = 100 + } + fc.currentZone = "critical" + fc.targetSpeed = speed + return speed + } + // Check for manual override if fc.override != nil { + fc.targetSpeed = fc.override.Speed return fc.override.Speed } @@ -201,17 +520,11 @@ func (fc *FanController) calculateTarget(cpuReading *monitor.CPUReading, gpuRead } // Check if we're over thresholds - cpuThreshold := fc.cfg.FanControl.CPUThreshold - gpuThreshold := fc.cfg.FanControl.GPUThreshold - if cpuThreshold == 0 { - cpuThreshold = 65 // Default CPU threshold - } - if gpuThreshold == 0 { - gpuThreshold = 60 // Default GPU threshold - } + cpuThreshold := fc.cfg.FanControl.EffectiveCPUThreshold() + gpuThreshold := fc.cfg.FanControl.EffectiveGPUThreshold() overThreshold := cpuMax > cpuThreshold || gpuMax > gpuThreshold - + // Determine zone name for display if cpuMax > 80 || gpuMax > 85 { fc.currentZone = "hot" @@ -234,16 +547,16 @@ func (fc *FanController) calculateTarget(cpuReading *monitor.CPUReading, gpuRead if overThreshold { // Over threshold - increase fan speed and reset cooldown timer fc.lastOverThreshold = now - + // Calculate how much over threshold we are cpuOver := max(0, cpuMax-cpuThreshold) gpuOver := max(0, gpuMax-gpuThreshold) maxOver := max(cpuOver, gpuOver) - + // Each 5°C over threshold = one step increase stepsNeeded := (maxOver / 5) + 1 neededSpeed := baseSpeed + (stepsNeeded * stepSize) - + // Ramp up immediately if needed if neededSpeed > target { target = min(target+stepSize, neededSpeed) @@ -254,7 +567,7 @@ func (fc *FanController) calculateTarget(cpuReading *monitor.CPUReading, gpuRead if cooldownDuration == 0 { cooldownDuration = 60 * time.Second } - + if fc.lastOverThreshold.IsZero() { // Never been over threshold - go directly to idle speed target = baseSpeed @@ -306,7 +619,7 @@ func (fc *FanController) calculateTrend(history []tempPoint) float64 { first := recent[0] last := recent[len(recent)-1] duration := last.timestamp.Sub(first.timestamp).Minutes() - + if duration < 0.1 { return 0 } @@ -316,7 +629,7 @@ func (fc *FanController) calculateTrend(history []tempPoint) float64 { func (fc *FanController) trimHistory() { cutoff := time.Now().Add(-time.Duration(fc.cfg.Monitoring.HistoryRetention) * time.Second) - + var newCPU []tempPoint for _, p := range fc.cpuHistory { if p.timestamp.After(cutoff) { @@ -336,79 +649,45 @@ func (fc *FanController) trimHistory() { func (fc *FanController) cleanExpired() { now := time.Now() - + for key, hint := range fc.hints { if !hint.ExpiresAt.IsZero() && hint.ExpiresAt.Before(now) { delete(fc.hints, key) } } - + if fc.override != nil && !fc.override.ExpiresAt.IsZero() && fc.override.ExpiresAt.Before(now) { fc.override = nil } } func (fc *FanController) setFanSpeed(speed int) error { - fc.mu.Lock() - fc.currentSpeed = speed - fc.mu.Unlock() - // Convert percentage to hex (0-100 -> 0x00-0x64) hexSpeed := fmt.Sprintf("0x%02x", speed) - var cmd *exec.Cmd - if fc.cfg.IDRAC.Host == "local" { - cmd = exec.Command("ipmitool", "raw", "0x30", "0x30", "0x02", "0xff", hexSpeed) - } else { - cmd = exec.Command("ipmitool", - "-I", "lanplus", - "-H", fc.cfg.IDRAC.Host, - "-U", fc.cfg.IDRAC.Username, - "-P", fc.cfg.IDRAC.Password, - "raw", "0x30", "0x30", "0x02", "0xff", hexSpeed, - ) - } - - var stderr bytes.Buffer - cmd.Stderr = &stderr - - if err := cmd.Run(); err != nil { - return fmt.Errorf("ipmitool error: %v, stderr: %s", err, stderr.String()) + // currentSpeed must reflect what the fans are ACTUALLY set to, so it is only + // updated after a confirmed successful write. A failed write is surfaced via + // lastWriteFailed in /api/status. + if err := fc.ipmitool("raw", "0x30", "0x30", "0x02", "0xff", hexSpeed); err != nil { + fc.mu.Lock() + fc.lastWriteFailed = true + fc.mu.Unlock() + return err } + fc.mu.Lock() + fc.currentSpeed = speed + fc.lastWriteFailed = false + fc.mu.Unlock() return nil } func (fc *FanController) enableManualMode() error { - var cmd *exec.Cmd - if fc.cfg.IDRAC.Host == "local" { - cmd = exec.Command("ipmitool", "raw", "0x30", "0x30", "0x01", "0x00") - } else { - cmd = exec.Command("ipmitool", - "-I", "lanplus", - "-H", fc.cfg.IDRAC.Host, - "-U", fc.cfg.IDRAC.Username, - "-P", fc.cfg.IDRAC.Password, - "raw", "0x30", "0x30", "0x01", "0x00", - ) - } - return cmd.Run() + return fc.ipmitool("raw", "0x30", "0x30", "0x01", "0x00") } func (fc *FanController) RestoreAutoMode() error { - var cmd *exec.Cmd - if fc.cfg.IDRAC.Host == "local" { - cmd = exec.Command("ipmitool", "raw", "0x30", "0x30", "0x01", "0x01") - } else { - cmd = exec.Command("ipmitool", - "-I", "lanplus", - "-H", fc.cfg.IDRAC.Host, - "-U", fc.cfg.IDRAC.Username, - "-P", fc.cfg.IDRAC.Password, - "raw", "0x30", "0x30", "0x01", "0x01", - ) - } - return cmd.Run() + return fc.ipmitool("raw", "0x30", "0x30", "0x01", "0x01") } // AddHint registers a workload hint @@ -430,7 +709,7 @@ func (fc *FanController) AddHint(hint *WorkloadHint) { hint.CreatedAt = time.Now() fc.hints[hint.Source] = hint - + log.Printf("Hint registered: %s from %s (min fan: %d%%)", hint.Action, hint.Source, hint.MinFanSpeed) } @@ -446,17 +725,17 @@ func (fc *FanController) RemoveHint(source string) { func (fc *FanController) SetOverride(speed int, duration time.Duration, reason string) { fc.mu.Lock() defer fc.mu.Unlock() - + fc.override = &Override{ Speed: speed, Reason: reason, CreatedAt: time.Now(), } - + if duration > 0 { fc.override.ExpiresAt = time.Now().Add(duration) } - + log.Printf("Override set: %d%% (%s)", speed, reason) } @@ -486,31 +765,29 @@ func (fc *FanController) GetStatus() *Status { } // Build threshold info for dashboard - cpuThreshold := fc.cfg.FanControl.CPUThreshold - gpuThreshold := fc.cfg.FanControl.GPUThreshold - if cpuThreshold == 0 { - cpuThreshold = 65 - } - if gpuThreshold == 0 { - gpuThreshold = 60 - } + cpuThreshold := fc.cfg.FanControl.EffectiveCPUThreshold() + gpuThreshold := fc.cfg.FanControl.EffectiveGPUThreshold() return &Status{ - Timestamp: time.Now(), - CPU: fc.lastCPUReading, - GPU: fc.lastGPUReading, - CurrentSpeed: fc.currentSpeed, - TargetSpeed: fc.targetSpeed, - Zone: fc.currentZone, - Mode: mode, - ActiveHints: hints, - Override: fc.override, - CPUTrend: fc.calculateTrend(fc.cpuHistory), - GPUTrend: fc.calculateTrend(fc.gpuHistory), - Zones: fc.cfg.Zones, - CPUThreshold: cpuThreshold, - GPUThreshold: gpuThreshold, - IdleSpeed: fc.cfg.FanControl.IdleSpeed, + Timestamp: time.Now(), + CPU: fc.lastCPUReading, + GPU: fc.lastGPUReading, + CurrentSpeed: fc.currentSpeed, + TargetSpeed: fc.targetSpeed, + Zone: fc.currentZone, + Mode: mode, + ActiveHints: hints, + Override: fc.override, + CPUTrend: fc.calculateTrend(fc.cpuHistory), + GPUTrend: fc.calculateTrend(fc.gpuHistory), + Zones: fc.cfg.Zones, + CPUThreshold: cpuThreshold, + GPUThreshold: gpuThreshold, + IdleSpeed: fc.cfg.FanControl.IdleSpeed, + FailsafeActive: fc.failsafeCause != failsafeNone, + FailsafeReason: fc.failsafeCause.String(), + RestorePending: fc.failsafeCause != failsafeNone && !fc.restoreConfirmed, + LastWriteFailed: fc.lastWriteFailed, } } diff --git a/internal/controller/fan_test.go b/internal/controller/fan_test.go new file mode 100644 index 0000000..0d47ae3 --- /dev/null +++ b/internal/controller/fan_test.go @@ -0,0 +1,493 @@ +package controller + +import ( + "context" + "errors" + "path/filepath" + "testing" + "time" + + "github.com/sethpjohnson/only-fan-controller/internal/config" + "github.com/sethpjohnson/only-fan-controller/internal/monitor" + "github.com/sethpjohnson/only-fan-controller/internal/storage" +) + +// testConfig returns a fresh default config for deterministic tests. +func testConfig() *config.Config { + return config.Default() +} + +func cpuGpu(cpuMax, gpuMax int) (*monitor.CPUReading, *monitor.GPUReading) { + return &monitor.CPUReading{Temps: []int{cpuMax}, Max: cpuMax}, + &monitor.GPUReading{Max: gpuMax} +} + +func TestCalculateTarget(t *testing.T) { + tests := []struct { + name string + // setup mutates the controller before the call. + setup func(fc *FanController) + cpu int + gpu int + want int + }{ + { + name: "idle below all thresholds goes to idle speed", + setup: func(fc *FanController) { fc.currentSpeed = 0 }, + cpu: 40, gpu: 35, + want: 20, // IdleSpeed + }, + { + name: "threshold crossing ramps up by one step", + setup: func(fc *FanController) { + fc.currentSpeed = 20 + }, + cpu: 75, gpu: 35, + want: 30, // 20 + one StepSize + }, + { + name: "hysteresis holds speed during cooldown", + setup: func(fc *FanController) { + fc.currentSpeed = 40 + fc.lastOverThreshold = time.Now() + }, + cpu: 50, gpu: 35, + want: 40, // unchanged during cooldown + }, + { + name: "ramps down one step after cooldown elapses", + setup: func(fc *FanController) { + fc.currentSpeed = 40 + fc.lastOverThreshold = time.Now().Add(-2 * time.Minute) + }, + cpu: 50, gpu: 35, + want: 30, // 40 - one StepSize + }, + { + name: "manual override takes precedence over normal logic", + setup: func(fc *FanController) { + fc.currentSpeed = 20 + fc.override = &Override{Speed: 55} + }, + cpu: 50, gpu: 35, + want: 55, + }, + { + name: "critical temp overrides even a low manual override", + setup: func(fc *FanController) { + fc.currentSpeed = 10 + fc.override = &Override{Speed: 20} + }, + cpu: 95, gpu: 35, + want: 100, // critical zone speed + }, + { + name: "critical temp fast-ramps straight to 100 from idle", + setup: func(fc *FanController) { + fc.currentSpeed = 10 + }, + cpu: 95, gpu: 35, + want: 100, + }, + { + name: "critical GPU temp also fast-ramps", + setup: func(fc *FanController) { + fc.currentSpeed = 10 + }, + cpu: 40, gpu: 95, + want: 100, + }, + { + name: "target clamped up to configured min speed", + setup: func(fc *FanController) { + fc.currentSpeed = 0 + fc.cfg.FanControl.MinSpeed = 30 + }, + cpu: 40, gpu: 35, + want: 30, // idle speed 20 clamped up to min 30 + }, + { + name: "target clamped down to configured max speed", + setup: func(fc *FanController) { + fc.currentSpeed = 0 + fc.cfg.FanControl.IdleSpeed = 90 + fc.cfg.FanControl.MaxSpeed = 80 + }, + cpu: 40, gpu: 35, + want: 80, // idle speed 90 clamped down to max 80 + }, + { + name: "workload hint sets a fan floor", + setup: func(fc *FanController) { + fc.currentSpeed = 0 + fc.hints["test"] = &WorkloadHint{Source: "test", MinFanSpeed: 45} + }, + cpu: 40, gpu: 35, + want: 45, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fc := NewFanController(testConfig(), nil, nil, nil) + if tt.setup != nil { + tt.setup(fc) + } + cpuR, gpuR := cpuGpu(tt.cpu, tt.gpu) + got := fc.calculateTarget(cpuR, gpuR) + if got != tt.want { + t.Fatalf("calculateTarget(cpu=%d, gpu=%d) = %d, want %d", tt.cpu, tt.gpu, got, tt.want) + } + }) + } +} + +// TestCriticalRampIgnoresOperatorZoneData proves the emergency ramp trusts the +// fixed MaxSpeed ceiling, never operator-editable zone fan speeds. Even a +// malicious/misconfigured Zones list whose top zone asks for a LOW speed must +// not weaken the emergency ramp, and critical still overrides a low manual +// override. +func TestCriticalRampIgnoresOperatorZoneData(t *testing.T) { + cfg := config.Default() + // Hostile zone list: "critical" zone requests a dangerously low 15%. + cfg.Zones = []config.Zone{ + {Name: "idle", CPUMax: 45, GPUMax: 40, FanSpeed: 10}, + {Name: "critical", CPUMax: 999, GPUMax: 999, FanSpeed: 15}, + } + cfg.FanControl.MaxSpeed = 100 + cfg.FanControl.CriticalCPUTemp = 85 + cfg.FanControl.CriticalGPUTemp = 90 + + fc := NewFanController(cfg, nil, nil, nil) + fc.override = &Override{Speed: 20} // low manual override + fc.currentSpeed = 10 + + cpuR, gpuR := cpuGpu(95, 35) // CPU critical + if got := fc.calculateTarget(cpuR, gpuR); got != 100 { + t.Fatalf("critical ramp used operator zone data / override instead of MaxSpeed: got %d, want 100", got) + } +} + +// --- fail-safe wiring tests --- + +// cmdRecorder captures the ipmitool invocations made through runCommand. +type cmdRecorder struct { + cmds [][]string + failOnFanSet bool + failRestore bool // simulate an unreachable BMC: RestoreAutoMode (0x01 0x01) fails +} + +func (r *cmdRecorder) run(_ context.Context, name string, args ...string) error { + call := append([]string{name}, args...) + r.cmds = append(r.cmds, call) + if r.failOnFanSet && containsArg(args, "0x02") { + return errors.New("simulated ipmitool fan-set failure") + } + if r.failRestore && endsWith(call, "0x01", "0x01") { + return errors.New("simulated unreachable BMC on RestoreAutoMode") + } + return nil +} + +func containsArg(args []string, want string) bool { + for _, a := range args { + if a == want { + return true + } + } + return false +} + +// restoreCount counts how many times BMC auto mode was restored (raw ... 0x01 0x01). +func (r *cmdRecorder) restoreCount() int { + n := 0 + for _, c := range r.cmds { + if endsWith(c, "0x01", "0x01") { + n++ + } + } + return n +} + +func (r *cmdRecorder) fanSetCount() int { + n := 0 + for _, c := range r.cmds { + if containsArg(c, "0x02") { + n++ + } + } + return n +} + +// manualModeCount counts how many times manual fan mode was (re-)enabled +// (raw ... 0x01 0x00). Together with restoreCount this exposes BMC flapping. +func (r *cmdRecorder) manualModeCount() int { + n := 0 + for _, c := range r.cmds { + if endsWith(c, "0x01", "0x00") { + n++ + } + } + return n +} + +func endsWith(s []string, a, b string) bool { + if len(s) < 2 { + return false + } + return s[len(s)-2] == a && s[len(s)-1] == b +} + +type failingCPU struct{} + +func (failingCPU) Read() (*monitor.CPUReading, error) { + return nil, errors.New("simulated CPU sensor loss") +} + +// flakyCPU fails its first failFor reads, then succeeds — used to test recovery. +type flakyCPU struct { + failFor int + calls int + max int +} + +func (f *flakyCPU) Read() (*monitor.CPUReading, error) { + f.calls++ + if f.calls <= f.failFor { + return nil, errors.New("simulated transient CPU sensor loss") + } + return &monitor.CPUReading{Temps: []int{f.max}, Max: f.max}, nil +} + +type staticCPU struct{ max int } + +func (s staticCPU) Read() (*monitor.CPUReading, error) { + return &monitor.CPUReading{Temps: []int{s.max}, Max: s.max}, nil +} + +type staticGPU struct{ max int } + +func (s staticGPU) Read() (*monitor.GPUReading, error) { + return &monitor.GPUReading{Max: s.max}, nil +} + +func newTestStore(t *testing.T) *storage.Store { + t.Helper() + store, err := storage.New(filepath.Join(t.TempDir(), "test.db")) + if err != nil { + t.Fatalf("failed to create test store: %v", err) + } + t.Cleanup(func() { store.Close() }) + return store +} + +func TestControlLoopHoldsSpeedOnSensorFailure(t *testing.T) { + rec := &cmdRecorder{} + fc := NewFanController(testConfig(), failingCPU{}, staticGPU{max: 40}, newTestStore(t)) + fc.runCommand = rec.run + fc.currentSpeed = 40 + + fc.controlLoop() + + if fc.currentSpeed != 40 { + t.Fatalf("currentSpeed changed on sensor failure: got %d, want 40", fc.currentSpeed) + } + if writes := rec.fanSetCount(); writes != 0 { + t.Fatalf("fan speed was written on sensor failure (%d writes); must hold instead", writes) + } +} + +func TestSensorFailureThresholdRestoresAutoMode(t *testing.T) { + rec := &cmdRecorder{} + cfg := testConfig() + cfg.FanControl.SensorFailureLimit = 3 + fc := NewFanController(cfg, failingCPU{}, staticGPU{max: 40}, newTestStore(t)) + fc.runCommand = rec.run + fc.currentSpeed = 40 + + fc.controlLoop() + fc.controlLoop() + if got := rec.restoreCount(); got != 0 { + t.Fatalf("restored auto mode too early after 2 failures: %d restores", got) + } + fc.controlLoop() // third failure crosses the threshold + if got := rec.restoreCount(); got != 1 { + t.Fatalf("expected exactly one auto-mode restore after %d failures, got %d", cfg.FanControl.SensorFailureLimit, got) + } + + // Keep failing well past the threshold: must NOT re-restore or flap. + for i := 0; i < 6; i++ { + fc.controlLoop() + } + if got := rec.restoreCount(); got != 1 { + t.Fatalf("sensor fail-safe re-restored auto mode (oscillation): %d restores, want 1", got) + } + if got := rec.manualModeCount(); got != 0 { + t.Fatalf("sensor fail-safe flapped back to manual while sensors still failing: %d manual toggles", got) + } +} + +// TestWriteFailureStickyNoOscillation reproduces the reviewer's scenario: +// healthy sensors + persistently failing fan writes. The fail-safe must engage +// exactly once and then stay in BMC auto — no manual/auto flapping. +func TestWriteFailureStickyNoOscillation(t *testing.T) { + rec := &cmdRecorder{failOnFanSet: true} + cfg := testConfig() + cfg.FanControl.WriteFailureLimit = 3 + fc := NewFanController(cfg, staticCPU{max: 50}, staticGPU{max: 40}, newTestStore(t)) + fc.runCommand = rec.run + + fc.controlLoop() + fc.controlLoop() + if got := rec.restoreCount(); got != 0 { + t.Fatalf("restored auto mode too early after 2 write failures: %d restores", got) + } + fc.controlLoop() // third write failure crosses the threshold + if got := rec.restoreCount(); got != 1 { + t.Fatalf("expected exactly one auto-mode restore after %d write failures, got %d", cfg.FanControl.WriteFailureLimit, got) + } + + fanSetsAtTrip := rec.fanSetCount() + + // Run many more ticks with writes still failing. The reviewer's bug flapped + // once per tick; the sticky write fail-safe must converge. + for i := 0; i < 10; i++ { + fc.controlLoop() + } + if got := rec.restoreCount(); got != 1 { + t.Fatalf("write fail-safe oscillated: %d auto-mode restores after threshold, want 1", got) + } + if got := rec.manualModeCount(); got != 0 { + t.Fatalf("write fail-safe flapped back to manual: %d manual toggles, want 0", got) + } + if got := rec.fanSetCount(); got != fanSetsAtTrip { + t.Fatalf("write fail-safe kept attempting fan writes after going sticky: %d attempts, want %d", got, fanSetsAtTrip) + } + if fc.currentFailsafeCause() != failsafeWrite { + t.Fatalf("expected sticky write fail-safe, got cause %v", fc.currentFailsafeCause()) + } + // Readings must still be refreshed for /api/status visibility while sticky. + if fc.lastCPUReading == nil || fc.lastCPUReading.Max != 50 { + t.Fatalf("readings not refreshed during write fail-safe: %+v", fc.lastCPUReading) + } +} + +// TestSensorFailsafeRecoversAndReclaimsManual proves the recoverable domain: +// once sensors read successfully again, manual control is reclaimed exactly once. +func TestSensorFailsafeRecoversAndReclaimsManual(t *testing.T) { + rec := &cmdRecorder{} + cfg := testConfig() + cfg.FanControl.SensorFailureLimit = 3 + cpu := &flakyCPU{failFor: 3, max: 50} + fc := NewFanController(cfg, cpu, staticGPU{max: 40}, newTestStore(t)) + fc.runCommand = rec.run + fc.currentSpeed = 30 + + // 3 failing reads -> enter sensor fail-safe. + fc.controlLoop() + fc.controlLoop() + fc.controlLoop() + if fc.currentFailsafeCause() != failsafeSensor { + t.Fatalf("expected sensor fail-safe after 3 failures, got %v", fc.currentFailsafeCause()) + } + if got := rec.restoreCount(); got != 1 { + t.Fatalf("expected one auto restore, got %d", got) + } + + // 4th read succeeds -> reclaim manual and resume control. + fc.controlLoop() + if fc.currentFailsafeCause() != failsafeNone { + t.Fatalf("fail-safe not cleared after sensor recovery: %v", fc.currentFailsafeCause()) + } + if got := rec.manualModeCount(); got != 1 { + t.Fatalf("expected exactly one manual-mode reclaim on recovery, got %d", got) + } + if got := rec.fanSetCount(); got != 1 { + t.Fatalf("expected one fan write after resuming control, got %d", got) + } + // Counters reset for a fresh future failure window. + if fc.sensorFailCount != 0 { + t.Fatalf("sensorFailCount not reset after recovery: %d", fc.sensorFailCount) + } +} + +// TestFailsafeRetriesRestoreUntilConfirmed reproduces an unreachable BMC: the +// very condition that trips fail-safe also fails RestoreAutoMode. The controller +// must keep retrying every tick (not give up after one attempt) and report the +// restore as pending; once the BMC returns, restore succeeds once and retries +// stop. +func TestFailsafeRetriesRestoreUntilConfirmed(t *testing.T) { + rec := &cmdRecorder{failRestore: true} + cfg := testConfig() + cfg.FanControl.SensorFailureLimit = 3 + fc := NewFanController(cfg, failingCPU{}, staticGPU{max: 40}, newTestStore(t)) + fc.runCommand = rec.run + fc.currentSpeed = 40 + + // Cross the sensor threshold -> first (failed) restore attempt. + fc.controlLoop() + fc.controlLoop() + fc.controlLoop() + if got := rec.restoreCount(); got != 1 { + t.Fatalf("expected 1 restore attempt at trip, got %d", got) + } + if fc.isRestoreConfirmed() { + t.Fatal("restore should not be confirmed while BMC is unreachable") + } + + // Keep failing for N more ticks: each tick must retry the restore. + const extra = 5 + for i := 0; i < extra; i++ { + fc.controlLoop() + } + if got := rec.restoreCount(); got != 1+extra { + t.Fatalf("expected %d restore attempts (retrying every tick), got %d", 1+extra, got) + } + if got := fc.GetStatus(); !got.RestorePending || !got.FailsafeActive { + t.Fatalf("status should show failsafe active + restore pending, got %+v", got) + } + + // BMC comes back: the next tick's retry succeeds, and retries then stop. + rec.failRestore = false + fc.controlLoop() + attemptsAfterRecovery := rec.restoreCount() + if !fc.isRestoreConfirmed() { + t.Fatal("restore should be confirmed once the BMC is reachable again") + } + fc.controlLoop() + fc.controlLoop() + if got := rec.restoreCount(); got != attemptsAfterRecovery { + t.Fatalf("restore retries did not stop after confirmation: %d -> %d", attemptsAfterRecovery, got) + } + if fc.GetStatus().RestorePending { + t.Fatal("restore_pending should be false after confirmation") + } +} + +func TestSetFanSpeedOnlyUpdatesCurrentSpeedOnSuccess(t *testing.T) { + rec := &cmdRecorder{failOnFanSet: true} + fc := NewFanController(testConfig(), nil, nil, nil) + fc.runCommand = rec.run + fc.currentSpeed = 25 + + if err := fc.setFanSpeed(80); err == nil { + t.Fatal("expected setFanSpeed to return the simulated failure") + } + if fc.currentSpeed != 25 { + t.Fatalf("currentSpeed updated despite failed write: got %d, want 25", fc.currentSpeed) + } + if !fc.lastWriteFailed { + t.Fatal("lastWriteFailed should be true after a failed write") + } + + rec.failOnFanSet = false + if err := fc.setFanSpeed(80); err != nil { + t.Fatalf("unexpected error on successful write: %v", err) + } + if fc.currentSpeed != 80 { + t.Fatalf("currentSpeed not updated after successful write: got %d, want 80", fc.currentSpeed) + } + if fc.lastWriteFailed { + t.Fatal("lastWriteFailed should be cleared after a successful write") + } +} diff --git a/internal/controller/mock.go b/internal/controller/mock.go index d349fc9..0327cc2 100644 --- a/internal/controller/mock.go +++ b/internal/controller/mock.go @@ -23,6 +23,7 @@ func NewMockFanController(cfg *config.Config, store *storage.Store) *MockFanCont cpuMon: nil, gpuMon: nil, store: store, + runCommand: realRunCommand, hints: make(map[string]*WorkloadHint), stopChan: make(chan struct{}), cpuHistory: make([]tempPoint, 0), @@ -84,13 +85,14 @@ func (mfc *MockFanController) mockControlLoop() { // Just update internal state (no actual IPMI commands) mfc.mu.Lock() mfc.currentSpeed = target + zone := mfc.currentZone mfc.mu.Unlock() // Store reading mfc.store.RecordReading(cpuReading.Max, gpuReading.Max, target) log.Printf("[MOCK] CPU: %d°C | GPU: %d°C | Zone: %s | Fan: %d%%", - cpuReading.Max, gpuReading.Max, mfc.currentZone, target) + cpuReading.Max, gpuReading.Max, zone, target) } func (mfc *MockFanController) generateMockCPU() *monitor.CPUReading { @@ -104,7 +106,7 @@ func (mfc *MockFanController) generateMockCPU() *monitor.CPUReading { } else { mockCPUBase -= 0.1 // Cooling } - + if mockCPUBase < 35 { mockCPUBase = 35 } diff --git a/internal/monitor/cpu.go b/internal/monitor/cpu.go index 1840f2b..cf57894 100644 --- a/internal/monitor/cpu.go +++ b/internal/monitor/cpu.go @@ -2,15 +2,37 @@ package monitor import ( "bytes" + "context" + "fmt" "log" "os/exec" "regexp" "strconv" "strings" + "time" "github.com/sethpjohnson/only-fan-controller/internal/config" ) +// commandTimeout bounds how long an external monitoring command may run. A hung +// command must never freeze the control loop, so it is capped at 10s; a 3s floor +// keeps a short monitoring interval from making slow-but-healthy remote calls +// read as timeouts. +func commandTimeout(cfg *config.Config) time.Duration { + const ( + minTimeout = 3 * time.Second + maxTimeout = 10 * time.Second + ) + interval := time.Duration(cfg.Monitoring.Interval) * time.Second + if interval > maxTimeout { + return maxTimeout + } + if interval < minTimeout { + return minTimeout + } + return interval +} + type CPUMonitor struct { cfg *config.Config } @@ -26,14 +48,17 @@ func NewCPUMonitor(cfg *config.Config) *CPUMonitor { // Read gets current CPU temperatures via IPMI func (m *CPUMonitor) Read() (*CPUReading, error) { + ctx, cancel := context.WithTimeout(context.Background(), commandTimeout(m.cfg)) + defer cancel() + var cmd *exec.Cmd if m.cfg.IDRAC.Host == "local" { // Local IPMI access - cmd = exec.Command("ipmitool", "sdr", "type", "temperature") + cmd = exec.CommandContext(ctx, "ipmitool", "sdr", "type", "temperature") } else { // Remote IPMI access - cmd = exec.Command("ipmitool", + cmd = exec.CommandContext(ctx, "ipmitool", "-I", "lanplus", "-H", m.cfg.IDRAC.Host, "-U", m.cfg.IDRAC.Username, @@ -47,12 +72,22 @@ func (m *CPUMonitor) Read() (*CPUReading, error) { cmd.Stderr = &stderr if err := cmd.Run(); err != nil { + if ctx.Err() == context.DeadlineExceeded { + log.Printf("ipmitool CPU read timed out after %s", commandTimeout(m.cfg)) + return nil, fmt.Errorf("ipmitool CPU read timed out: %w", err) + } log.Printf("ipmitool error: %v, stderr: %s", err, stderr.String()) return nil, err } - temps := parseCPUTemps(stdout.String()) - + // Empty parse (e.g. after an iDRAC firmware update changed the output + // format) is an error, never a silent 0°C reading. + temps, err := parseCPUTemps(stdout.String()) + if err != nil { + log.Printf("CPU temperature parse failed: %v; raw output: %q", err, stdout.String()) + return nil, err + } + reading := &CPUReading{ Temps: temps, Max: maxInt(temps), @@ -63,13 +98,14 @@ func (m *CPUMonitor) Read() (*CPUReading, error) { // parseCPUTemps extracts CPU temperatures from ipmitool output // Example output from R730: -// Inlet Temp | 04h | ok | 7.1 | 20 degrees C -// Exhaust Temp | 01h | ok | 7.1 | 28 degrees C -// Temp | 0Eh | ok | 3.1 | 33 degrees C <- CPU 1 -// Temp | 0Fh | ok | 3.2 | 35 degrees C <- CPU 2 -func parseCPUTemps(output string) []int { +// +// Inlet Temp | 04h | ok | 7.1 | 20 degrees C +// Exhaust Temp | 01h | ok | 7.1 | 28 degrees C +// Temp | 0Eh | ok | 3.1 | 33 degrees C <- CPU 1 +// Temp | 0Fh | ok | 3.2 | 35 degrees C <- CPU 2 +func parseCPUTemps(output string) ([]int, error) { var temps []int - + lines := strings.Split(output, "\n") for _, line := range lines { // Skip inlet/exhaust temps, focus on CPU temps @@ -77,13 +113,13 @@ func parseCPUTemps(output string) []int { if strings.Contains(lower, "inlet") || strings.Contains(lower, "exhaust") { continue } - + // Only process lines that start with "Temp" (CPU temps on Dell servers) trimmed := strings.TrimSpace(line) if !strings.HasPrefix(trimmed, "Temp") { continue } - + // Extract temperature value: look for "NN degrees" re := regexp.MustCompile(`(\d+)\s*degrees`) matches := re.FindStringSubmatch(line) @@ -98,6 +134,11 @@ func parseCPUTemps(output string) []int { if len(temps) == 0 { re2 := regexp.MustCompile(`(\d+)\s*degrees`) for _, line := range lines { + // Still skip chassis inlet/exhaust sensors in the fallback path. + lower := strings.ToLower(line) + if strings.Contains(lower, "inlet") || strings.Contains(lower, "exhaust") { + continue + } matches := re2.FindStringSubmatch(line) if len(matches) >= 2 { if temp, err := strconv.Atoi(matches[1]); err == nil && temp > 0 && temp < 120 { @@ -107,7 +148,11 @@ func parseCPUTemps(output string) []int { } } - return temps + if len(temps) == 0 { + return nil, fmt.Errorf("no valid CPU temperatures found in ipmitool output") + } + + return temps, nil } func maxInt(vals []int) int { diff --git a/internal/monitor/cpu_test.go b/internal/monitor/cpu_test.go new file mode 100644 index 0000000..36c15eb --- /dev/null +++ b/internal/monitor/cpu_test.go @@ -0,0 +1,82 @@ +package monitor + +import "testing" + +func TestParseCPUTemps(t *testing.T) { + tests := []struct { + name string + output string + want []int + wantErr bool + }{ + { + name: "valid R730 output", + output: `Inlet Temp | 04h | ok | 7.1 | 20 degrees C +Exhaust Temp | 01h | ok | 7.1 | 28 degrees C +Temp | 0Eh | ok | 3.1 | 33 degrees C +Temp | 0Fh | ok | 3.2 | 35 degrees C`, + want: []int{33, 35}, + wantErr: false, + }, + { + name: "empty output is an error, never 0C", + output: "", + want: nil, + wantErr: true, + }, + { + name: "garbage output is an error", + output: "this is not ipmitool output at all\nrandom text", + want: nil, + wantErr: true, + }, + { + name: "only inlet and exhaust temps yields no CPU temps", + output: `Inlet Temp | 04h | ok | 7.1 | 20 degrees C +Exhaust Temp | 01h | ok | 7.1 | 28 degrees C`, + want: nil, + wantErr: true, + }, + { + name: "firmware update changed labels but temps still present via fallback", + output: `CPU1 Temp | 0Eh | ok | 3.1 | 41 degrees C +CPU2 Temp | 0Fh | ok | 3.2 | 44 degrees C`, + want: []int{41, 44}, + wantErr: false, + }, + { + name: "out-of-range readings are ignored and remaining empty is an error", + output: `Temp | 0Eh | ok | 3.1 | 0 degrees C +Temp | 0Fh | ok | 3.2 | 999 degrees C`, + want: nil, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := parseCPUTemps(tt.output) + if tt.wantErr && err == nil { + t.Fatalf("expected error, got nil (temps=%v)", got) + } + if !tt.wantErr && err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !equalInts(got, tt.want) { + t.Fatalf("temps = %v, want %v", got, tt.want) + } + }) + } +} + +func equalInts(a, b []int) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} diff --git a/internal/monitor/gpu.go b/internal/monitor/gpu.go index 19f8f71..534dfb2 100644 --- a/internal/monitor/gpu.go +++ b/internal/monitor/gpu.go @@ -2,7 +2,9 @@ package monitor import ( "bytes" + "context" "encoding/csv" + "fmt" "log" "os/exec" "strconv" @@ -20,9 +22,9 @@ type GPUDevice struct { Name string `json:"name"` Temp int `json:"temp"` Utilization int `json:"utilization"` - MemoryUsed int `json:"memory_used"` // MB - MemoryTotal int `json:"memory_total"` // MB - PowerDraw int `json:"power_draw"` // Watts + MemoryUsed int `json:"memory_used"` // MB + MemoryTotal int `json:"memory_total"` // MB + PowerDraw int `json:"power_draw"` // Watts } type GPUReading struct { @@ -40,8 +42,11 @@ func (m *GPUMonitor) Read() (*GPUReading, error) { return &GPUReading{}, nil } + ctx, cancel := context.WithTimeout(context.Background(), commandTimeout(m.cfg)) + defer cancel() + // Query nvidia-smi for key metrics in CSV format - cmd := exec.Command(m.cfg.GPU.NvidiaSmiPath, + cmd := exec.CommandContext(ctx, m.cfg.GPU.NvidiaSmiPath, "--query-gpu=index,name,temperature.gpu,utilization.gpu,memory.used,memory.total,power.draw", "--format=csv,noheader,nounits", ) @@ -51,12 +56,24 @@ func (m *GPUMonitor) Read() (*GPUReading, error) { cmd.Stderr = &stderr if err := cmd.Run(); err != nil { + if ctx.Err() == context.DeadlineExceeded { + log.Printf("nvidia-smi timed out after %s", commandTimeout(m.cfg)) + return nil, fmt.Errorf("nvidia-smi timed out: %w", err) + } log.Printf("nvidia-smi error: %v, stderr: %s", err, stderr.String()) return nil, err } - devices := parseGPUOutput(stdout.String()) - + // Zero devices parsed while GPU monitoring is enabled is an error, not a + // silent 0°C reading — otherwise empty/garbage nvidia-smi output would look + // like a healthy "cold" GPU and the fail-safe would never engage. Mirrors the + // CPU treatment. + devices, err := parseGPUOutput(stdout.String()) + if err != nil { + log.Printf("GPU output parse failed: %v; raw output: %q", err, stdout.String()) + return nil, err + } + reading := &GPUReading{ Devices: devices, Max: maxGPUTemp(devices), @@ -65,15 +82,18 @@ func (m *GPUMonitor) Read() (*GPUReading, error) { return reading, nil } -// parseGPUOutput parses nvidia-smi CSV output -func parseGPUOutput(output string) []GPUDevice { +// parseGPUOutput parses nvidia-smi CSV output. It returns an error when no valid +// device rows are present. +func parseGPUOutput(output string) ([]GPUDevice, error) { var devices []GPUDevice reader := csv.NewReader(strings.NewReader(output)) + // Tolerate rows with unexpected field counts (skipped below) rather than + // failing the whole read; nvidia-smi occasionally emits ragged rows. + reader.FieldsPerRecord = -1 records, err := reader.ReadAll() if err != nil { - log.Printf("Failed to parse nvidia-smi output: %v", err) - return devices + return nil, fmt.Errorf("failed to parse nvidia-smi CSV output: %w", err) } for _, record := range records { @@ -99,7 +119,11 @@ func parseGPUOutput(output string) []GPUDevice { devices = append(devices, device) } - return devices + if len(devices) == 0 { + return nil, fmt.Errorf("no valid GPU device rows found in nvidia-smi output") + } + + return devices, nil } func parseInt(s string) int { @@ -108,12 +132,12 @@ func parseInt(s string) int { if s == "" || s == "[N/A]" || s == "N/A" { return 0 } - + // Remove any decimal part (e.g., "45.00" -> "45") if idx := strings.Index(s, "."); idx != -1 { s = s[:idx] } - + val, err := strconv.Atoi(s) if err != nil { return 0 diff --git a/internal/monitor/gpu_test.go b/internal/monitor/gpu_test.go new file mode 100644 index 0000000..0f38032 --- /dev/null +++ b/internal/monitor/gpu_test.go @@ -0,0 +1,69 @@ +package monitor + +import "testing" + +func TestParseGPUOutput(t *testing.T) { + tests := []struct { + name string + output string + wantTemps []int // temps of parsed devices, in order + wantErr bool + }{ + { + name: "valid two-GPU CSV", + output: "0, Tesla P40, 45, 80, 2048, 24576, 220\n1, Tesla P40, 47, 85, 4096, 24576, 230\n", + wantTemps: []int{45, 47}, + wantErr: false, + }, + { + name: "empty output is an error, never 0C", + output: "", + wantTemps: nil, + wantErr: true, + }, + { + name: "whitespace-only output is an error", + output: " \n\n", + wantTemps: nil, + wantErr: true, + }, + { + name: "garbage / non-CSV output is an error", + output: "nvidia-smi: command not found", + wantTemps: nil, + wantErr: true, + }, + { + name: "partial rows (too few fields) are skipped; all-skipped is an error", + output: "0, Tesla P40, 45\n1, Tesla P40\n", + wantTemps: nil, + wantErr: true, + }, + { + name: "mixed valid and partial rows keeps the valid one", + output: "0, Tesla P40, 45\n1, Tesla P40, 50, 80, 4096, 24576, 230\n", + wantTemps: []int{50}, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + devices, err := parseGPUOutput(tt.output) + if tt.wantErr && err == nil { + t.Fatalf("expected error, got nil (devices=%+v)", devices) + } + if !tt.wantErr && err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(devices) != len(tt.wantTemps) { + t.Fatalf("device count = %d, want %d", len(devices), len(tt.wantTemps)) + } + for i, want := range tt.wantTemps { + if devices[i].Temp != want { + t.Fatalf("device[%d].Temp = %d, want %d", i, devices[i].Temp, want) + } + } + }) + } +}